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

[David Hildenbrand <david@redhat.com>]

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 <jdenemar@redhat.com>
>> Fixes: b6805e127c6b ("s390x: use generic cpu_model parsing")
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   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)

> 
> Anyway, your patch sounds fine, so (with "klass" replaced by "oc"):
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 


-- 

Thanks,

David / dhildenb