Re: [PATCH v10 6/9] s390x/cpu topology: add topology-disable machine property

[Pierre Morel]

On 10/18/22 19:34, Cédric Le Goater wrote:
> On 10/12/22 18:21, Pierre Morel wrote:
> > S390 CPU topology is only allowed for s390-virtio-ccw-7.3 and
> > newer S390 machines.
> > We keep the possibility to disable the topology on these newer
> > machines with the property topology-disable.
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   include/hw/boards.h                |  3 ++
> >   include/hw/s390x/cpu-topology.h    | 18 +++++++++-
> >   include/hw/s390x/s390-virtio-ccw.h |  2 ++
> >   hw/core/machine.c                  |  5 +++
> >   hw/s390x/s390-virtio-ccw.c         | 53 +++++++++++++++++++++++++++++-
> >   util/qemu-config.c                 |  4 +++
> >   qemu-options.hx                    |  6 +++-
> >   7 files changed, 88 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 311ed17e18..67147c47bf 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -379,6 +379,9 @@ struct MachineState {
> >       } \
> >       type_init(machine_initfn##_register_types)
> > +extern GlobalProperty hw_compat_7_2[];
> > +extern const size_t hw_compat_7_2_len;
>
> QEMU 7.2 is not out yet.

OK, thanks, I will use 7.2 instead of 7.3 as the new machine.
We see later if it goes to 8.0

> > +
> >   extern GlobalProperty hw_compat_7_1[];
> >   extern const size_t hw_compat_7_1_len;
> > diff --git a/include/hw/s390x/cpu-topology.h 
> > b/include/hw/s390x/cpu-topology.h
> > index 35a8a981ec..747c9ab4c6 100644
> > --- a/include/hw/s390x/cpu-topology.h
> > +++ b/include/hw/s390x/cpu-topology.h
> > @@ -12,6 +12,8 @@
> >   #include "hw/qdev-core.h"
> >   #include "qom/object.h"
> > +#include "cpu.h"
> > +#include "hw/s390x/s390-virtio-ccw.h"
> >   #define S390_TOPOLOGY_POLARITY_H  0x00
> > @@ -43,7 +45,21 @@ void s390_topology_new_cpu(int core_id);
> >   static inline bool s390_has_topology(void)
> >   {
> > -    return false;
> > +    static S390CcwMachineState *ccw;
>
> hmm, s390_has_topology is a static inline. It would be preferable to
> change its definition to extern.

OK

> > +    Object *obj;
> > +
> > +    if (ccw) {
> > +        return !ccw->topology_disable;
> > +    }
> > +
> > +    /* we have to bail out for the "none" machine */
> > +    obj = object_dynamic_cast(qdev_get_machine(),
> > +                              TYPE_S390_CCW_MACHINE);
> > +    if (!obj) {
> > +        return false;
> > +    }
> > +    ccw = S390_CCW_MACHINE(obj);
> > +    return !ccw->topology_disable;
> >   }
> >   #endif
> > diff --git a/include/hw/s390x/s390-virtio-ccw.h 
> > b/include/hw/s390x/s390-virtio-ccw.h
> > index 9e7a0d75bc..6c4b4645fc 100644
> > --- a/include/hw/s390x/s390-virtio-ccw.h
> > +++ b/include/hw/s390x/s390-virtio-ccw.h
> > @@ -28,6 +28,7 @@ struct S390CcwMachineState {
> >       bool dea_key_wrap;
> >       bool pv;
> >       bool zpcii_disable;
> > +    bool topology_disable;
> >       uint8_t loadparm[8];
> >   };
> > @@ -46,6 +47,7 @@ struct S390CcwMachineClass {
> >       bool cpu_model_allowed;
> >       bool css_migration_enabled;
> >       bool hpage_1m_allowed;
> > +    bool topology_allowed;
>
> 'topology_disable' in the state and 'topology_allowed' in the class.
> This is confusing :/
>
> you should add 'topology_allowed' in its own patch and maybe call
> it 'topology_capable' ? it is a QEMU capability AIUI

yes, OK, I separate the two.

. topology_capable to know if the QEMU version is capable of handling 
topology

- topology_disable for migration if KVM is not able to handle the 
topology on one of the machines involved.

> >   };
> >   /* runtime-instrumentation allowed by the machine */
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index aa520e74a8..93c497655e 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -40,6 +40,11 @@
> >   #include "hw/virtio/virtio-pci.h"
> >   #include "qom/object_interfaces.h"
> > +GlobalProperty hw_compat_7_2[] = {
> > +    { "s390-topology", "topology-disable", "true" },
>
> May be use TYPE_S390_CPU_TOPOLOGY instead.
>
> But again, this should only apply to 7.1 machines and below. 7.2 is
> not out yet.

OK

> > +};
> > +const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
> > +
> >   GlobalProperty hw_compat_7_1[] = {};
> >   const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index 362378454a..3a13fad4df 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -616,6 +616,7 @@ static void ccw_machine_class_init(ObjectClass 
> > *oc, void *data)
> >       s390mc->cpu_model_allowed = true;
> >       s390mc->css_migration_enabled = true;
> >       s390mc->hpage_1m_allowed = true;
> > +    s390mc->topology_allowed = true;
> >       mc->init = ccw_init;
> >       mc->reset = s390_machine_reset;
> >       mc->block_default_type = IF_VIRTIO;
> > @@ -726,6 +727,27 @@ bool hpage_1m_allowed(void)
> >       return get_machine_class()->hpage_1m_allowed;
> >   }
> > +static inline bool machine_get_topology_disable(Object *obj, Error 
> > **errp)
> > +{
> > +    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> > +
> > +    return ms->topology_disable;
> > +}
> > +
> > +static inline void machine_set_topology_disable(Object *obj, bool value,
> > +                                                Error **errp)
> > +{
> > +    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> > +
> > +    if (!get_machine_class()->topology_allowed) {
> > +        error_setg(errp, "Property topology-disable not available on 
> > machine %s",
> > +                   get_machine_class()->parent_class.name);
>
> OK. I get it now. May be we should consider adding the capability concept
> David introduced in the pseries machine. Please take a look. That's not
> for this patchset though. It would be too much work.

Yes, interesting, would be something to do in the near future.

> > +        return;
> > +    }
> > +
> > +    ms->topology_disable = value;
> > +}
> > +
> >   static char *machine_get_loadparm(Object *obj, Error **errp)
> >   {
> >       S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> > @@ -784,6 +806,13 @@ static inline void s390_machine_initfn(Object *obj)
> >       object_property_set_description(obj, "zpcii-disable",
> >               "disable zPCI interpretation facilties");
> >       object_property_set_bool(obj, "zpcii-disable", false, NULL);
> > +
> > +    object_property_add_bool(obj, "topology-disable",
> > +                             machine_get_topology_disable,
> > +                             machine_set_topology_disable);
> > +    object_property_set_description(obj, "topology-disable",
> > +            "disable CPU topology");
> > +    object_property_set_bool(obj, "topology-disable", false, NULL);
>
> All the properties should be added in the machine class_init routine.
> There is a preliminary cleanup patch required to move them all :/

OK, I will move it to the class_init


> >   }
> >   static const TypeInfo ccw_machine_info = {
> > @@ -836,14 +865,36 @@ bool css_migration_enabled(void)
> >       
> > }                                                                         \
> >       type_init(ccw_machine_register_##suffix)
> > +static void ccw_machine_7_3_instance_options(MachineState *machine)
> > +{
> > +}
> > +
> > +static void ccw_machine_7_3_class_options(MachineClass *mc)
> > +{
> > +}
> > +DEFINE_CCW_MACHINE(7_3, "7.3", true);
>
> That's too early.
>
> > +
> >   static void ccw_machine_7_2_instance_options(MachineState *machine)
> >   {
> > +    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
> > +
> > +    ccw_machine_7_3_instance_options(machine);
> > +    ms->topology_disable = true;
> >   }
> >   static void ccw_machine_7_2_class_options(MachineClass *mc)
> >   {
> > +    S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
> > +    static GlobalProperty compat[] = {
> > +        { TYPE_S390_CPU_TOPOLOGY, "topology-allowed", "off", },
>
> hmm, "topology-allowed" is not a TYPE_S390_CPU_TOPOLOGY property.

I do not understand what I did there and why.
I guess I better do nothing there.

Thanks for the comments,

Regards,
Pierre

--
Pierre Morel
IBM Lab Boeblingen