Re: [PATCH v1] s390x/kvm: Set default cpu model for all machine classes

[Thomas Huth]

On 21/10/2019 11.54, David Hildenbrand wrote:
> On 21.10.19 11:52, Thomas Huth wrote:
>> On 21/10/2019 11.34, David Hildenbrand wrote:
>>> We have to set the default model of all machine classes, not just for
>>> the active one. Otherwise, "query-machines" will indicate the wrong
>>> CPU model ("qemu-s390x-cpu" instead of "host-s390x-cpu") as
>>> "default-cpu-type".
>>>
>>> Doing a
>>>      {"execute":"query-machines"}
>>> under KVM now results in
>>>      {"return": [
>>>          {
>>>              "hotpluggable-cpus": true,
>>>              "name": "s390-ccw-virtio-4.0",
>>>              "numa-mem-supported": false,
>>>              "default-cpu-type": "host-s390x-cpu",
>>>              "cpu-max": 248,
>>>              "deprecated": false},
>>>          {
>>>              "hotpluggable-cpus": true,
>>>              "name": "s390-ccw-virtio-2.7",
>>>              "numa-mem-supported": false,
>>>              "default-cpu-type": "host-s390x-cpu",
>>>              "cpu-max": 248,
>>>              "deprecated": false
>>>          } ...
>>>
>>> Reported-by: Jiri Denemark <address@hidden>
>>> Fixes: b6805e127c6b ("s390x: use generic cpu_model parsing")
>>> Cc: Igor Mammedov <address@hidden>
>>> Signed-off-by: David Hildenbrand <address@hidden>
>>> ---
>>>   target/s390x/kvm.c | 10 ++++++++--
>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index c24c869e77..5966ab0d37 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -320,11 +320,17 @@ void kvm_s390_set_max_pagesize(uint64_t
>>> pagesize, Error **errp)
>>>       cap_hpage_1m = 1;
>>>   }
>>>   -int kvm_arch_init(MachineState *ms, KVMState *s)
>>> +static void ccw_machine_class_foreach(ObjectClass *klass, void *opaque)
>>>   {
>>> -    MachineClass *mc = MACHINE_GET_CLASS(ms);
>>> +    MachineClass *mc = MACHINE_CLASS(klass);
>>
>> I think we rather wanted to avoid using "klass" in new code... maybe use
>> "oc" instead of "klass" ?
> 
> Can do, this was a copy and paste :)
> 
>>
>>>       mc->default_cpu_type = S390_CPU_TYPE_NAME("host");
>>> +}
>>> +
>>> +int kvm_arch_init(MachineState *ms, KVMState *s)
>>> +{
>>> +    object_class_foreach(ccw_machine_class_foreach,
>>> TYPE_S390_CCW_MACHINE,
>>> +                         false, NULL);
>>>         if (!kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) {
>>>           error_report("KVM is missing capability KVM_CAP_DEVICE_CTRL
>>> - "
>>>
>>
>> Weird, if you start an older machine, you still get the "host" CPU
>> without your patch, too:
>>
>> echo -e "info qom-tree \n quit" | \
>>   qemu-system-s390x -display none -monitor stdio -no-shutdown \
>>     -accel kvm -M s390-ccw-virtio-3.0 | grep s390x-cpu
>>
>> Results in:
>>
>>      /device[0] (host-s390x-cpu)
>>
>> ... so I wonder why that differs from the "query-machines" command?
> 
> query-machines probes with the "none" machine all other machines.
> Current code only fixes up the active machine.
> 
> (that's why you won't notice when starting a machine - you will always
> get "host" for the active one)

Ah, right, thanks for the explanation. I somehow read the patch
description that it is only fixed for the latest one (I should really
read more closely) ... but now it makes perfect sense.

 Thomas