| Message ID | 20221103170150.20789-2-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: CPU Topology | expand |
On 03/11/2022 18.01, Pierre Morel wrote: > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 127 +++++++++++++++++++++---------------- > 1 file changed, 72 insertions(+), 55 deletions(-) -EMISSINGPATCHDESCRIPTION ... please add some words *why* this is a good idea / necessary. Thanks, Thomas
On 11/4/22 07:32, Thomas Huth wrote: > On 03/11/2022 18.01, Pierre Morel wrote: >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 127 +++++++++++++++++++++---------------- >> 1 file changed, 72 insertions(+), 55 deletions(-) > > -EMISSINGPATCHDESCRIPTION > > ... please add some words *why* this is a good idea / necessary. I saw that the i386 patch had no description for the same patch so... To be honest I do not know why it is necessary. The only reason I see is to be in sync with the PC implementation. So what about: " Register TYPE_S390_CCW_MACHINE properties as class properties to be conform with the X architectures " ? @Cédric , any official recommendation for doing that? > > Thanks, > Thomas > >
On 11/4/22 11:16, Pierre Morel wrote: > > > On 11/4/22 07:32, Thomas Huth wrote: >> On 03/11/2022 18.01, Pierre Morel wrote: >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>> --- >>> hw/s390x/s390-virtio-ccw.c | 127 +++++++++++++++++++++---------------- >>> 1 file changed, 72 insertions(+), 55 deletions(-) >> >> -EMISSINGPATCHDESCRIPTION >> >> ... please add some words *why* this is a good idea / necessary. > > I saw that the i386 patch had no description for the same patch so... > > To be honest I do not know why it is necessary. > The only reason I see is to be in sync with the PC implementation. > > So what about: > " > Register TYPE_S390_CCW_MACHINE properties as class properties > to be conform with the X architectures > " > ? > > @Cédric , any official recommendation for doing that? There was a bunch of commits related to QOM in this series : 91def7b83 arm/virt: Register most properties as class properties f5730c69f0 i386: Register feature bit properties as class properties which moved property definitions at the class level. Then, commit d8fb7d0969 ("vl: switch -M parsing to keyval") changed machine_help_func() to use a machine class and not machine instance anymore. I would use the same kind of commit log and add a Fixes tag to get it merged in 7.2 With that, Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C.
On 11/4/22 11:53, Cédric Le Goater wrote: > On 11/4/22 11:16, Pierre Morel wrote: >> >> >> On 11/4/22 07:32, Thomas Huth wrote: >>> On 03/11/2022 18.01, Pierre Morel wrote: >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>> --- >>>> hw/s390x/s390-virtio-ccw.c | 127 >>>> +++++++++++++++++++++---------------- >>>> 1 file changed, 72 insertions(+), 55 deletions(-) >>> >>> -EMISSINGPATCHDESCRIPTION >>> >>> ... please add some words *why* this is a good idea / necessary. >> >> I saw that the i386 patch had no description for the same patch so... >> >> To be honest I do not know why it is necessary. >> The only reason I see is to be in sync with the PC implementation. >> >> So what about: >> " >> Register TYPE_S390_CCW_MACHINE properties as class properties >> to be conform with the X architectures >> " >> ? >> >> @Cédric , any official recommendation for doing that? > > There was a bunch of commits related to QOM in this series : > > 91def7b83 arm/virt: Register most properties as class properties > f5730c69f0 i386: Register feature bit properties as class properties > > which moved property definitions at the class level. > > Then, > > commit d8fb7d0969 ("vl: switch -M parsing to keyval") > > changed machine_help_func() to use a machine class and not machine > instance anymore. > > I would use the same kind of commit log and add a Fixes tag to get it > merged in 7.2 > > With that, > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > Thanks, > > C. > > OK, Thanks, Pierre
On 04/11/2022 11.53, Cédric Le Goater wrote: > On 11/4/22 11:16, Pierre Morel wrote: >> >> >> On 11/4/22 07:32, Thomas Huth wrote: >>> On 03/11/2022 18.01, Pierre Morel wrote: >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>> --- >>>> hw/s390x/s390-virtio-ccw.c | 127 +++++++++++++++++++++---------------- >>>> 1 file changed, 72 insertions(+), 55 deletions(-) >>> >>> -EMISSINGPATCHDESCRIPTION >>> >>> ... please add some words *why* this is a good idea / necessary. >> >> I saw that the i386 patch had no description for the same patch so... >> >> To be honest I do not know why it is necessary. >> The only reason I see is to be in sync with the PC implementation. >> >> So what about: >> " >> Register TYPE_S390_CCW_MACHINE properties as class properties >> to be conform with the X architectures >> " >> ? >> >> @Cédric , any official recommendation for doing that? > > There was a bunch of commits related to QOM in this series : > > 91def7b83 arm/virt: Register most properties as class properties > f5730c69f0 i386: Register feature bit properties as class properties > > which moved property definitions at the class level. > > Then, > > commit d8fb7d0969 ("vl: switch -M parsing to keyval") > > changed machine_help_func() to use a machine class and not machine > instance anymore. > > I would use the same kind of commit log and add a Fixes tag to get it > merged in 7.2 Ah, so this fixes the problem that running QEMU with " -M s390-ccw-virtio,help" does not show the s390x-specific properties anymore? ... that's certainly somethings that should be mentioned in the commit message! What about something like this: "Currently, when running 'qemu-system-s390x -M -M s390-ccw-virtio,help' the s390x-specific properties are not listed anymore. This happens because since commit d8fb7d0969 ("vl: switch -M parsing to keyval") the properties have to be defined at the class level and not at the instance level anymore. Fix it on s390x now, too, by moving the registration of the properties to the class level" Fixes: d8fb7d0969 ("vl: switch -M parsing to keyval") ? Thomas
On 11/4/22 15:29, Thomas Huth wrote: > On 04/11/2022 11.53, Cédric Le Goater wrote: >> On 11/4/22 11:16, Pierre Morel wrote: >>> >>> >>> On 11/4/22 07:32, Thomas Huth wrote: >>>> On 03/11/2022 18.01, Pierre Morel wrote: >>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>>> --- >>>>> hw/s390x/s390-virtio-ccw.c | 127 >>>>> +++++++++++++++++++++---------------- >>>>> 1 file changed, 72 insertions(+), 55 deletions(-) >>>> >>>> -EMISSINGPATCHDESCRIPTION >>>> >>>> ... please add some words *why* this is a good idea / necessary. >>> >>> I saw that the i386 patch had no description for the same patch so... >>> >>> To be honest I do not know why it is necessary. >>> The only reason I see is to be in sync with the PC implementation. >>> >>> So what about: >>> " >>> Register TYPE_S390_CCW_MACHINE properties as class properties >>> to be conform with the X architectures >>> " >>> ? >>> >>> @Cédric , any official recommendation for doing that? >> >> There was a bunch of commits related to QOM in this series : >> >> 91def7b83 arm/virt: Register most properties as class properties >> f5730c69f0 i386: Register feature bit properties as class properties >> >> which moved property definitions at the class level. >> >> Then, >> >> commit d8fb7d0969 ("vl: switch -M parsing to keyval") >> >> changed machine_help_func() to use a machine class and not machine >> instance anymore. >> >> I would use the same kind of commit log and add a Fixes tag to get it >> merged in 7.2 > > Ah, so this fixes the problem that running QEMU with " -M > s390-ccw-virtio,help" does not show the s390x-specific properties > anymore? ... that's certainly somethings that should be mentioned in the > commit message! What about something like this: > > "Currently, when running 'qemu-system-s390x -M -M s390-ccw-virtio,help' > the s390x-specific properties are not listed anymore. This happens > because since commit d8fb7d0969 ("vl: switch -M parsing to keyval") the > properties have to be defined at the class level and not at the instance > level anymore. Fix it on s390x now, too, by moving the registration of > the properties to the class level" > > Fixes: d8fb7d0969 ("vl: switch -M parsing to keyval") > > ? > > Thomas > That seems really good :) Thank you Thomas! Regards, Pierre
On 04/11/2022 15.57, Pierre Morel wrote: > > > On 11/4/22 15:29, Thomas Huth wrote: >> On 04/11/2022 11.53, Cédric Le Goater wrote: >>> On 11/4/22 11:16, Pierre Morel wrote: >>>> >>>> >>>> On 11/4/22 07:32, Thomas Huth wrote: >>>>> On 03/11/2022 18.01, Pierre Morel wrote: >>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>>>> --- >>>>>> hw/s390x/s390-virtio-ccw.c | 127 +++++++++++++++++++++---------------- >>>>>> 1 file changed, 72 insertions(+), 55 deletions(-) >>>>> >>>>> -EMISSINGPATCHDESCRIPTION >>>>> >>>>> ... please add some words *why* this is a good idea / necessary. >>>> >>>> I saw that the i386 patch had no description for the same patch so... >>>> >>>> To be honest I do not know why it is necessary. >>>> The only reason I see is to be in sync with the PC implementation. >>>> >>>> So what about: >>>> " >>>> Register TYPE_S390_CCW_MACHINE properties as class properties >>>> to be conform with the X architectures >>>> " >>>> ? >>>> >>>> @Cédric , any official recommendation for doing that? >>> >>> There was a bunch of commits related to QOM in this series : >>> >>> 91def7b83 arm/virt: Register most properties as class properties >>> f5730c69f0 i386: Register feature bit properties as class properties >>> >>> which moved property definitions at the class level. >>> >>> Then, >>> >>> commit d8fb7d0969 ("vl: switch -M parsing to keyval") >>> >>> changed machine_help_func() to use a machine class and not machine >>> instance anymore. >>> >>> I would use the same kind of commit log and add a Fixes tag to get it >>> merged in 7.2 >> >> Ah, so this fixes the problem that running QEMU with " -M >> s390-ccw-virtio,help" does not show the s390x-specific properties anymore? >> ... that's certainly somethings that should be mentioned in the commit >> message! What about something like this: >> >> "Currently, when running 'qemu-system-s390x -M -M s390-ccw-virtio,help' >> the s390x-specific properties are not listed anymore. This happens because >> since commit d8fb7d0969 ("vl: switch -M parsing to keyval") the properties >> have to be defined at the class level and not at the instance level >> anymore. Fix it on s390x now, too, by moving the registration of the >> properties to the class level" >> >> Fixes: d8fb7d0969 ("vl: switch -M parsing to keyval") >> >> ? >> >> Thomas >> > > That seems really good :) All right, I've queued this patch (with the updated commit description) and the next one on my s390x-branch for QEMU 7.2: https://gitlab.com/thuth/qemu/-/commits/s390x-next/ Thomas
On 11/6/22 12:37, Thomas Huth wrote: > On 04/11/2022 15.57, Pierre Morel wrote: >> >> >> On 11/4/22 15:29, Thomas Huth wrote: >>> On 04/11/2022 11.53, Cédric Le Goater wrote: >>>> On 11/4/22 11:16, Pierre Morel wrote: >>>>> >>>>> >>>>> On 11/4/22 07:32, Thomas Huth wrote: >>>>>> On 03/11/2022 18.01, Pierre Morel wrote: >>>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>>>>> --- >>>>>>> hw/s390x/s390-virtio-ccw.c | 127 >>>>>>> +++++++++++++++++++++---------------- >>>>>>> 1 file changed, 72 insertions(+), 55 deletions(-) >>>>>> >>>>>> -EMISSINGPATCHDESCRIPTION >>>>>> >>>>>> ... please add some words *why* this is a good idea / necessary. >>>>> >>>>> I saw that the i386 patch had no description for the same patch so... >>>>> >>>>> To be honest I do not know why it is necessary. >>>>> The only reason I see is to be in sync with the PC implementation. >>>>> >>>>> So what about: >>>>> " >>>>> Register TYPE_S390_CCW_MACHINE properties as class properties >>>>> to be conform with the X architectures >>>>> " >>>>> ? >>>>> >>>>> @Cédric , any official recommendation for doing that? >>>> >>>> There was a bunch of commits related to QOM in this series : >>>> >>>> 91def7b83 arm/virt: Register most properties as class properties >>>> f5730c69f0 i386: Register feature bit properties as class properties >>>> >>>> which moved property definitions at the class level. >>>> >>>> Then, >>>> >>>> commit d8fb7d0969 ("vl: switch -M parsing to keyval") >>>> >>>> changed machine_help_func() to use a machine class and not machine >>>> instance anymore. >>>> >>>> I would use the same kind of commit log and add a Fixes tag to get it >>>> merged in 7.2 >>> >>> Ah, so this fixes the problem that running QEMU with " -M >>> s390-ccw-virtio,help" does not show the s390x-specific properties >>> anymore? ... that's certainly somethings that should be mentioned in >>> the commit message! What about something like this: >>> >>> "Currently, when running 'qemu-system-s390x -M -M >>> s390-ccw-virtio,help' the s390x-specific properties are not listed >>> anymore. This happens because since commit d8fb7d0969 ("vl: switch -M >>> parsing to keyval") the properties have to be defined at the class >>> level and not at the instance level anymore. Fix it on s390x now, >>> too, by moving the registration of the properties to the class level" >>> >>> Fixes: d8fb7d0969 ("vl: switch -M parsing to keyval") >>> >>> ? >>> >>> Thomas >>> >> >> That seems really good :) > > All right, I've queued this patch (with the updated commit description) > and the next one on my s390x-branch for QEMU 7.2: > > https://gitlab.com/thuth/qemu/-/commits/s390x-next/ > > Thomas > > Thank you! Regards, Pierre
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 1cc20d8717..567498e780 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -43,6 +43,7 @@ #include "sysemu/sysemu.h" #include "hw/s390x/pv.h" #include "migration/blocker.h" +#include "qapi/visitor.h" static Error *pv_mig_blocker; @@ -589,38 +590,6 @@ static ram_addr_t s390_fixup_ram_size(ram_addr_t sz) return newsz; } -static void ccw_machine_class_init(ObjectClass *oc, void *data) -{ - MachineClass *mc = MACHINE_CLASS(oc); - NMIClass *nc = NMI_CLASS(oc); - HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc); - S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc); - - s390mc->ri_allowed = true; - s390mc->cpu_model_allowed = true; - s390mc->css_migration_enabled = true; - s390mc->hpage_1m_allowed = true; - mc->init = ccw_init; - mc->reset = s390_machine_reset; - mc->block_default_type = IF_VIRTIO; - mc->no_cdrom = 1; - mc->no_floppy = 1; - mc->no_parallel = 1; - mc->no_sdcard = 1; - mc->max_cpus = S390_MAX_CPUS; - mc->has_hotpluggable_cpus = true; - assert(!mc->get_hotplug_handler); - mc->get_hotplug_handler = s390_get_hotplug_handler; - mc->cpu_index_to_instance_props = s390_cpu_index_to_props; - mc->possible_cpu_arch_ids = s390_possible_cpu_arch_ids; - /* it is overridden with 'host' cpu *in kvm_arch_init* */ - mc->default_cpu_type = S390_CPU_TYPE_NAME("qemu"); - hc->plug = s390_machine_device_plug; - hc->unplug_request = s390_machine_device_unplug_request; - nc->nmi_monitor_handler = s390_nmi; - mc->default_ram_id = "s390.ram"; -} - static inline bool machine_get_aes_key_wrap(Object *obj, Error **errp) { S390CcwMachineState *ms = S390_CCW_MACHINE(obj); @@ -710,19 +679,29 @@ bool hpage_1m_allowed(void) return get_machine_class()->hpage_1m_allowed; } -static char *machine_get_loadparm(Object *obj, Error **errp) +static void machine_get_loadparm(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) { S390CcwMachineState *ms = S390_CCW_MACHINE(obj); + char *str = g_strndup((char *) ms->loadparm, sizeof(ms->loadparm)); - /* make a NUL-terminated string */ - return g_strndup((char *) ms->loadparm, sizeof(ms->loadparm)); + visit_type_str(v, name, &str, errp); + g_free(str); } -static void machine_set_loadparm(Object *obj, const char *val, Error **errp) +static void machine_set_loadparm(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) { S390CcwMachineState *ms = S390_CCW_MACHINE(obj); + char *val; int i; + if (!visit_type_str(v, name, &val, errp)) { + return; + } + for (i = 0; i < sizeof(ms->loadparm) && val[i]; i++) { uint8_t c = qemu_toupper(val[i]); /* mimic HMC */ @@ -740,34 +719,72 @@ static void machine_set_loadparm(Object *obj, const char *val, Error **errp) ms->loadparm[i] = ' '; /* pad right with spaces */ } } -static inline void s390_machine_initfn(Object *obj) + +static void ccw_machine_class_init(ObjectClass *oc, void *data) { - object_property_add_bool(obj, "aes-key-wrap", - machine_get_aes_key_wrap, - machine_set_aes_key_wrap); - object_property_set_description(obj, "aes-key-wrap", + MachineClass *mc = MACHINE_CLASS(oc); + NMIClass *nc = NMI_CLASS(oc); + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc); + S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc); + + s390mc->ri_allowed = true; + s390mc->cpu_model_allowed = true; + s390mc->css_migration_enabled = true; + s390mc->hpage_1m_allowed = true; + mc->init = ccw_init; + mc->reset = s390_machine_reset; + mc->block_default_type = IF_VIRTIO; + mc->no_cdrom = 1; + mc->no_floppy = 1; + mc->no_parallel = 1; + mc->no_sdcard = 1; + mc->max_cpus = S390_MAX_CPUS; + mc->has_hotpluggable_cpus = true; + assert(!mc->get_hotplug_handler); + mc->get_hotplug_handler = s390_get_hotplug_handler; + mc->cpu_index_to_instance_props = s390_cpu_index_to_props; + mc->possible_cpu_arch_ids = s390_possible_cpu_arch_ids; + /* it is overridden with 'host' cpu *in kvm_arch_init* */ + mc->default_cpu_type = S390_CPU_TYPE_NAME("qemu"); + hc->plug = s390_machine_device_plug; + hc->unplug_request = s390_machine_device_unplug_request; + nc->nmi_monitor_handler = s390_nmi; + mc->default_ram_id = "s390.ram"; + + object_class_property_add_bool(oc, "aes-key-wrap", + machine_get_aes_key_wrap, + machine_set_aes_key_wrap); + object_class_property_set_description(oc, "aes-key-wrap", "enable/disable AES key wrapping using the CPACF wrapping key"); - object_property_set_bool(obj, "aes-key-wrap", true, NULL); - object_property_add_bool(obj, "dea-key-wrap", - machine_get_dea_key_wrap, - machine_set_dea_key_wrap); - object_property_set_description(obj, "dea-key-wrap", + object_class_property_add_bool(oc, "dea-key-wrap", + machine_get_dea_key_wrap, + machine_set_dea_key_wrap); + object_class_property_set_description(oc, "dea-key-wrap", "enable/disable DEA key wrapping using the CPACF wrapping key"); - object_property_set_bool(obj, "dea-key-wrap", true, NULL); - object_property_add_str(obj, "loadparm", - machine_get_loadparm, machine_set_loadparm); - object_property_set_description(obj, "loadparm", + + object_class_property_add(oc, "loadparm", "loadparm", + machine_get_loadparm, machine_set_loadparm, + NULL, NULL); + object_class_property_set_description(oc, "loadparm", "Up to 8 chars in set of [A-Za-z0-9. ] (lower case chars converted" " to upper case) to pass to machine loader, boot manager," " and guest kernel"); - object_property_add_bool(obj, "zpcii-disable", - machine_get_zpcii_disable, - machine_set_zpcii_disable); - object_property_set_description(obj, "zpcii-disable", + object_class_property_add_bool(oc, "zpcii-disable", + machine_get_zpcii_disable, + machine_set_zpcii_disable); + object_class_property_set_description(oc, "zpcii-disable", "disable zPCI interpretation facilties"); - object_property_set_bool(obj, "zpcii-disable", false, NULL); +} + +static inline void s390_machine_initfn(Object *obj) +{ + S390CcwMachineState *ms = S390_CCW_MACHINE(obj); + + ms->aes_key_wrap = true; + ms->dea_key_wrap = true; + ms->zpcii_disable = false; } static const TypeInfo ccw_machine_info = {
Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- hw/s390x/s390-virtio-ccw.c | 127 +++++++++++++++++++++---------------- 1 file changed, 72 insertions(+), 55 deletions(-)