[v1] s390x/kvm: Set default cpu model for all machine classes
diff mbox series

Message ID 20191021093456.6168-1-david@redhat.com
State New
Headers show
Series
  • [v1] s390x/kvm: Set default cpu model for all machine classes
Related show

Commit Message

David Hildenbrand Oct. 21, 2019, 9:34 a.m. UTC
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(-)

Comments

Thomas Huth Oct. 21, 2019, 9:52 a.m. UTC | #1
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" ?

>      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?

Anyway, your patch sounds fine, so (with "klass" replaced by "oc"):

Reviewed-by: Thomas Huth <thuth@redhat.com>
David Hildenbrand Oct. 21, 2019, 9:54 a.m. UTC | #2
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>
>
Thomas Huth Oct. 21, 2019, 9:58 a.m. UTC | #3
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 <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)

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
David Hildenbrand Oct. 21, 2019, 9:58 a.m. UTC | #4
> query-machines probes with the "none" machine all other machines.
> Current code only fixes up the active machine.
> 

To be more precise, libvirt probes with "-machine none,accel=kvm:tcg" 
all other machines.

> (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>
>>
> 
>
Cornelia Huck Oct. 21, 2019, 10:01 a.m. UTC | #5
On Mon, 21 Oct 2019 11:58:35 +0200
David Hildenbrand <david@redhat.com> wrote:

> > query-machines probes with the "none" machine all other machines.
> > Current code only fixes up the active machine.
> >   
> 
> To be more precise, libvirt probes with "-machine none,accel=kvm:tcg" 
> all other machines.

Add that to the patch description?

> 
> > (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"):

Respin with that? Makes it easier to pick up :)

> >>
> >> Reviewed-by: Thomas Huth <thuth@redhat.com>
> >>  
> > 
> >   
> 
>

Patch
diff mbox series

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);
 
     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 - "