Re: [PATCH v16 02/11] s390x/cpu topology: add topology entries on CPU hotplug

[pierre <pierre@imap.linux.ibm.com>]

On 2023-02-23 13:53, Thomas Huth wrote:
> On 22/02/2023 15.20, Pierre Morel wrote:
>> The topology information are attributes of the CPU and are
>> specified during the CPU device creation.
> ...
>> diff --git a/include/hw/s390x/cpu-topology.h 
>> b/include/hw/s390x/cpu-topology.h
>> index 83f31604cc..fa7f885a9f 100644
>> --- a/include/hw/s390x/cpu-topology.h
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -10,6 +10,47 @@
>>   #ifndef HW_S390X_CPU_TOPOLOGY_H
>>   #define HW_S390X_CPU_TOPOLOGY_H
>>   +#include "qemu/queue.h"
>> +#include "hw/boards.h"
>> +#include "qapi/qapi-types-machine-target.h"
>> +
>>   #define S390_TOPOLOGY_CPU_IFL   0x03
>>   +typedef struct S390Topology {
>> +    uint8_t *cores_per_socket;
>> +    CpuTopology *smp;
>> +    CpuS390Polarization polarization;
>> +} S390Topology;
>> +
>> +#ifdef CONFIG_KVM
>> +bool s390_has_topology(void);
>> +void s390_topology_setup_cpu(MachineState *ms, S390CPU *cpu, Error 
>> **errp);
>> +#else
>> +static inline bool s390_has_topology(void)
>> +{
>> +       return false;
>> +}
>> +static inline void s390_topology_setup_cpu(MachineState *ms,
>> +                                           S390CPU *cpu,
>> +                                           Error **errp) {}
>> +#endif
>> +
>> +extern S390Topology s390_topology;
>> +int s390_socket_nb(S390CPU *cpu);
>> +
>> +static inline int s390_std_socket(int n, CpuTopology *smp)
>> +{
>> +    return (n / smp->cores) % smp->sockets;
>> +}
>> +
>> +static inline int s390_std_book(int n, CpuTopology *smp)
>> +{
>> +    return (n / (smp->cores * smp->sockets)) % smp->books;
>> +}
>> +
>> +static inline int s390_std_drawer(int n, CpuTopology *smp)
>> +{
>> +    return (n / (smp->cores * smp->sockets * smp->books)) % 
>> smp->books;
> 
> Shouldn't that be " % smp->drawers" instead?

/o\  Yes it is of course.
thanks.

> 
>> +}
>> +
>>   #endif
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> new file mode 100644
>> index 0000000000..59f2cc15c7
>> --- /dev/null
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -0,0 +1,270 @@
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright IBM Corp. 2022
>> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
>> +
>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>> (at
>> + * your option) any later version. See the COPYING file in the 
>> top-level
>> + * directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu/error-report.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/boards.h"
>> +#include "qemu/typedefs.h"
>> +#include "target/s390x/cpu.h"
>> +#include "hw/s390x/s390-virtio-ccw.h"
>> +#include "hw/s390x/cpu-topology.h"
>> +
>> +/*
>> + * s390_topology is used to keep the topology information.
>> + * .cores_per_socket: tracks information on the count of cores
>> + *                    per socket.
>> + * .smp: keeps track of the machine topology.
>> + *
>> + */
>> +S390Topology s390_topology = {
>> +    /* will be initialized after the cpu model is realized */
>> +    .cores_per_socket = NULL,
>> +    .smp = NULL,
>> +    .polarization = S390_CPU_POLARIZATION_HORIZONTAL,
>> +};
>> +
>> +/**
>> + * s390_socket_nb:
>> + * @cpu: s390x CPU
>> + *
>> + * Returns the socket number used inside the cores_per_socket array
>> + * for a cpu.
>> + */
>> +int s390_socket_nb(S390CPU *cpu)
>> +{
>> +    return (cpu->env.drawer_id * s390_topology.smp->books + 
>> cpu->env.book_id) *
>> +           s390_topology.smp->sockets + cpu->env.socket_id;
>> +}
>> +
>> +/**
>> + * s390_has_topology:
>> + *
>> + * Return value: if the topology is supported by the machine.
>> + */
>> +bool s390_has_topology(void)
>> +{
>> +    return false;
>> +}
>> +
>> +/**
>> + * s390_topology_init:
>> + * @ms: the machine state where the machine topology is defined
>> + *
>> + * Keep track of the machine topology.
>> + *
>> + * Allocate an array to keep the count of cores per socket.
>> + * The index of the array starts at socket 0 from book 0 and
>> + * drawer 0 up to the maximum allowed by the machine topology.
>> + */
>> +static void s390_topology_init(MachineState *ms)
>> +{
>> +    CpuTopology *smp = &ms->smp;
>> +
>> +    s390_topology.smp = smp;
>> +    s390_topology.cores_per_socket = g_new0(uint8_t, smp->sockets *
>> +                                            smp->books * 
>> smp->drawers);
>> +}
>> +
>> +/**
>> + * s390_topology_cpu_default:
>> + * @cpu: pointer to a S390CPU
>> + * @errp: Error pointer
>> + *
>> + * Setup the default topology if no attributes are already set.
>> + * Passing a CPU with some, but not all, attributes set is considered
>> + * an error.
>> + *
>> + * The function calculates the (drawer_id, book_id, socket_id)
>> + * topology by filling the cores starting from the first socket
>> + * (0, 0, 0) up to the last (smp->drawers, smp->books, smp->sockets).
>> + *
>> + * CPU type, entitlement and dedication have defaults values set in 
>> the
>> + * s390x_cpu_properties, however entitlement is forced to 0 'none' 
>> when
>> + * the polarization is horizontale.
>> + */
>> +static void s390_topology_cpu_default(S390CPU *cpu, Error **errp)
>> +{
>> +    CpuTopology *smp = s390_topology.smp;
>> +    CPUS390XState *env = &cpu->env;
>> +
>> +    /* All geometry topology attributes must be set or all unset */
>> +    if ((env->socket_id < 0 || env->book_id < 0 || env->drawer_id < 
>> 0) &&
>> +        (env->socket_id >= 0 || env->book_id >= 0 || env->drawer_id 
>> >= 0)) {
>> +        error_setg(errp,
>> +                   "Please define all or none of the topology 
>> geometry attributes");
>> +        return;
>> +    }
>> +
>> +    /* Check if one of the geometry topology is unset */
>> +    if (env->socket_id < 0) {
>> +        /* Calculate default geometry topology attributes */
>> +        env->socket_id = s390_std_socket(env->core_id, smp);
>> +        env->book_id = s390_std_book(env->core_id, smp);
>> +        env->drawer_id = s390_std_drawer(env->core_id, smp);
>> +    }
>> +
>> +    if (s390_topology.polarization == 
>> S390_CPU_POLARIZATION_HORIZONTAL) {
>> +        env->entitlement = 0;
> 
> Should this be S390_CPU_ENTITLEMENT_HORIZONTAL instead of 0 ?

Yes it is better thanks.

Regards,
Pierre