Re: [PATCH v9 09/10] s390x/cpu_topology: activating CPU topology

[Cédric Le Goater <clg@xxxxxxxx>]

On 9/2/22 09:55, Pierre Morel wrote:
> Starting with a new machine, s390-virtio-ccw-7.2, the machine
> property topology-disable is set to false while it is kept to
> true for older machine.

We probably need a machine class option also because we don't want
this to be possible :

   -M s390-ccw-virtio-7.1,topology-disable=false


> This allows migrating older machine without disabling the ctop
> CPU feature for older machine, thus keeping existing start scripts.
>
> The KVM capability, KVM_CAP_S390_CPU_TOPOLOGY is used to
> activate the S390_FEAT_CONFIGURATION_TOPOLOGY feature and
> the topology facility for the guest in the case the topology
> is not disabled.
>
> Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> ---
>   hw/core/machine.c                  |  5 +++
>   hw/s390x/s390-virtio-ccw.c         | 55 ++++++++++++++++++++++++++----
>   include/hw/boards.h                |  3 ++
>   include/hw/s390x/s390-virtio-ccw.h |  1 +
>   target/s390x/kvm/kvm.c             | 14 ++++++++
>   5 files changed, 72 insertions(+), 6 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 4c5c8d1655..cbcdd40763 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_1[] = {
> +    { "s390x-cpu", "ctop", "off"},
> +};
> +const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
> +
>   GlobalProperty hw_compat_7_0[] = {
>       { "arm-gicv3-common", "force-8-bit-prio", "on" },
>       { "nvme-ns", "eui64-default", "on"},
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 1fa98740de..3078e68df7 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -249,11 +249,16 @@ static void ccw_init(MachineState *machine)
>       /* init memory + setup max page size. Required for the CPU model */
>       s390_memory_init(machine->ram);
>   
> -    /* Adding the topology must be done before CPU intialization*/
> -    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
> -    object_property_add_child(qdev_get_machine(), TYPE_S390_CPU_TOPOLOGY,
> -                              OBJECT(dev));
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    /*
> +     * Adding the topology must be done before CPU intialization but
> +     * only in the case it is not disabled for migration purpose.
> +     */
> +    if (!S390_CCW_MACHINE(machine)->topology_disable) {
> +        dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
> +        object_property_add_child(qdev_get_machine(), TYPE_S390_CPU_TOPOLOGY,
> +                                  OBJECT(dev));
> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    }
>   
>       /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>       s390_init_cpus(machine);
> @@ -676,6 +681,21 @@ static inline void machine_set_zpcii_disable(Object *obj, bool value,
>       ms->zpcii_disable = value;
>   }
>   
> +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);
> +
> +    ms->topology_disable = value;
> +}
> +
>   static S390CcwMachineClass *current_mc;
>   
>   /*
> @@ -778,6 +798,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 zPCI interpretation facilties");
> +    object_property_set_bool(obj, "topology-disable", false, NULL);
>   }
>   
>   static const TypeInfo ccw_machine_info = {
> @@ -830,14 +857,29 @@ bool css_migration_enabled(void)
>       }                                                                         \
>       type_init(ccw_machine_register_##suffix)
>   
> +static void ccw_machine_7_2_instance_options(MachineState *machine)
> +{
> +}
> +
> +static void ccw_machine_7_2_class_options(MachineClass *mc)
> +{
> +}
> +DEFINE_CCW_MACHINE(7_2, "7.2", true);
> +
>   static void ccw_machine_7_1_instance_options(MachineState *machine)
>   {
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
> +
> +    ccw_machine_7_2_instance_options(machine);
> +    ms->topology_disable = true;
>   }
>   
>   static void ccw_machine_7_1_class_options(MachineClass *mc)
>   {
> +    ccw_machine_7_2_class_options(mc);
> +    compat_props_add(mc->compat_props, hw_compat_7_1, hw_compat_7_1_len);
>   }
> -DEFINE_CCW_MACHINE(7_1, "7.1", true);
> +DEFINE_CCW_MACHINE(7_1, "7.1", false);
>   
>   static void ccw_machine_7_0_instance_options(MachineState *machine)
>   {
> @@ -847,6 +889,7 @@ static void ccw_machine_7_0_instance_options(MachineState *machine)
>       ccw_machine_7_1_instance_options(machine);
>       s390_set_qemu_cpu_model(0x8561, 15, 1, qemu_cpu_feat);
>       ms->zpcii_disable = true;
> +
>   }
>   
>   static void ccw_machine_7_0_class_options(MachineClass *mc)
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 69e20c1252..6e9803aa2d 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -387,6 +387,9 @@ struct MachineState {
>       } \
>       type_init(machine_initfn##_register_types)
>   
> +extern GlobalProperty hw_compat_7_1[];
> +extern const size_t hw_compat_7_1_len;
> +
>   extern GlobalProperty hw_compat_7_0[];
>   extern const size_t hw_compat_7_0_len;
>   
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 9e7a0d75bc..b14660eecb 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];
>   };
>   
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index cb14bcc012..6b7efee511 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -2385,6 +2385,7 @@ bool kvm_s390_cpu_models_supported(void)
>   
>   void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>   {
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
>       struct kvm_s390_vm_cpu_machine prop = {};
>       struct kvm_device_attr attr = {
>           .group = KVM_S390_VM_CPU_MODEL,
> @@ -2466,6 +2467,19 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>           set_bit(S390_FEAT_UNPACK, model->features);
>       }
>   
> +    /*
> +     * If we have the CPU Topology implemented in KVM activate
> +     * the CPU TOPOLOGY feature.
> +     */
> +    if ((!ms->topology_disable) &&

'topology_disable' is a platform level configuration. May be instead,
the feature could be cleared at the machine level ?

Thanks,

C.

> +        kvm_check_extension(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY)) {
> +        if (kvm_vm_enable_cap(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY, 0) < 0) {
> +            error_setg(errp, "KVM: Error enabling KVM_CAP_S390_CPU_TOPOLOGY");
> +            return;
> +        }
> +        set_bit(S390_FEAT_CONFIGURATION_TOPOLOGY, model->features);
> +    }
> +
>       /* We emulate a zPCI bus and AEN, therefore we don't need HW support */
>       set_bit(S390_FEAT_ZPCI, model->features);
>       set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);