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

["Cédric Le Goater" <clg@kaod.org>]

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.

> +
>   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.

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

>   };
>   
>   /* 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.


> +};
> +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.

> +        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 :/
   
>   }
>   
>   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.


> +    };
> +
> +    ccw_machine_7_3_class_options(mc);
> +    compat_props_add(mc->compat_props, hw_compat_7_2, hw_compat_7_2_len);
> +    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
> +    s390mc->topology_allowed = false;
>   }
> -DEFINE_CCW_MACHINE(7_2, "7.2", true);
> +DEFINE_CCW_MACHINE(7_2, "7.2", false);
>   
>   static void ccw_machine_7_1_instance_options(MachineState *machine)
>   {
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 5325f6bf80..c19e8bc8f3 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -240,6 +240,10 @@ static QemuOptsList machine_opts = {
>               .name = "zpcii-disable",
>               .type = QEMU_OPT_BOOL,
>               .help = "disable zPCI interpretation facilities",
> +        },{
> +            .name = "topology-disable",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "disable CPU topology",
>           },
>           { /* End of list */ }
>       }
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 95b998a13b..c804b0f899 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -38,7 +38,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>       "                hmat=on|off controls ACPI HMAT support (default=off)\n"
>       "                memory-backend='backend-id' specifies explicitly provided backend for main RAM (default=none)\n"
>       "                cxl-fmw.0.targets.0=firsttarget,cxl-fmw.0.targets.1=secondtarget,cxl-fmw.0.size=size[,cxl-fmw.0.interleave-granularity=granularity]\n"
> -    "                zpcii-disable=on|off disables zPCI interpretation facilities (default=off)\n",
> +    "                zpcii-disable=on|off disables zPCI interpretation facilities (default=off)\n"
> +    "                topology-disable=on|off disables CPU topology (default=off)\n",
>       QEMU_ARCH_ALL)
>   SRST
>   ``-machine [type=]name[,prop=value[,...]]``
> @@ -163,6 +164,9 @@ SRST
>           Disables zPCI interpretation facilties on s390-ccw hosts.
>           This feature can be used to disable hardware virtual assists
>           related to zPCI devices. The default is off.
> +
> +    ``topology-disable=on|off``
> +        Disables CPU topology on for S390 machines starting with s390-ccw-virtio-7.3.
>   ERST
>   
>   DEF("M", HAS_ARG, QEMU_OPTION_M,