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

Cédric Le Goater
2022-10-18T18:51:22Z

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,