| Message ID | 20191021093456.6168-1-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] s390x/kvm: Set default cpu model for all machine classes | expand |
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>
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> >
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
> 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> >> > >
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> > >> > > > > > >
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 - "
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(-)