Re: [PATCH v19 02/21] s390x/cpu topology: add topology entries on CPU hotplug

[Pierre Morel]

On 4/20/23 10:59, Nina Schoetterl-Glausch wrote:
> On Mon, 2023-04-03 at 18:28 +0200, Pierre Morel wrote:
> > The topology information are attributes of the CPU and are
> > specified during the CPU device creation.
> >
> > On hot plug we:
> > - calculate the default values for the topology for drawers,
> >    books and sockets in the case they are not specified.
> > - verify the CPU attributes
> > - check that we have still room on the desired socket
> >
> > The possibility to insert a CPU in a mask is dependent on the
> > number of cores allowed in a socket, a book or a drawer, the
> > checking is done during the hot plug of the CPU to have an
> > immediate answer.
> >
> > If the complete topology is not specified, the core is added
> > in the physical topology based on its core ID and it gets
> > defaults values for the modifier attributes.
> >
> > This way, starting QEMU without specifying the topology can
> > still get some advantage of the CPU topology.
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   MAINTAINERS                        |   1 +
> >   include/hw/s390x/cpu-topology.h    |  44 +++++
> >   include/hw/s390x/s390-virtio-ccw.h |   1 +
> >   hw/s390x/cpu-topology.c            | 282 +++++++++++++++++++++++++++++
> >   hw/s390x/s390-virtio-ccw.c         |  22 ++-
> >   hw/s390x/meson.build               |   1 +
> >   6 files changed, 349 insertions(+), 2 deletions(-)
> >   create mode 100644 hw/s390x/cpu-topology.c
> [...]
>
> >   #endif
> > diff --git a/include/hw/s390x/s390-virtio-ccw.h 
> > b/include/hw/s390x/s390-virtio-ccw.h
> > index 9bba21a916..ea10a6c6e1 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;
> >       uint8_t loadparm[8];
> > +    bool vertical_polarization;
> Why is this here and not in s390_topology?
> This splits up the topology state somewhat.
> I don't quite recall, did you use to have s390_topology in S390CcwMachineState 
> at some point?
> I think putting everything in S390CcwMachineState might make sense too.

Hum.

This is a left over from an abandoned try.

I put it back where it was, in s390_topology.


> >   };
> >   
> >   struct S390CcwMachineClass {
> > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> > new file mode 100644
> > index 0000000000..96da67bd7e
> > --- /dev/null
> > +++ b/hw/s390x/cpu-topology.c
> [...]
>
> > +/**
> > + * 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 and dedication have defaults values set in the
> > + * s390x_cpu_properties, entitlement must be adjust depending on the
> > + * dedication.
> > + */
> > +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);
> > +    }
> > +
> > +    /*
> > +     * Even the user can specify the entitlement as horizontal on the
> > +     * command line, qemu will only use env->entitlement during vertical
> > +     * polarization.
> > +     * Medium entitlement is chosen as the default entitlement when the CPU
> > +     * is not dedicated.
> > +     * A dedicated CPU always receives a high entitlement.
> > +     */
> > +    if (env->entitlement >= S390_CPU_ENTITLEMENT__MAX ||
> > +        env->entitlement == S390_CPU_ENTITLEMENT_HORIZONTAL) {
> > +        if (env->dedicated) {
> > +            env->entitlement = S390_CPU_ENTITLEMENT_HIGH;
> > +        } else {
> > +            env->entitlement = S390_CPU_ENTITLEMENT_MEDIUM;
> > +        }
> > +    }
> As you know, in my opinion there should be not possibility for the user
> to set the entitlement to horizontal and dedicated && != HIGH should be
> rejected as an error.
> If you do this, you can delete this.

In the next version with entitlement being an enum it is right.

However, deleting this means that the default value for entitlement 
depends on dedication.

If we have only low, medium, high and default for entitlement is medium.

If the user specifies the dedication true without specifying entitlement 
we could force entitlement to high.

But we can not distinguish this from the user specifying dedication true 
with a medium entitlement, which is wrong.

So three solution:

1) We ignore what the user say if dedication is specified as true

2) We specify that both dedication and entitlement must be specified if 
dedication is true

3) We set an impossible default to distinguish default from medium 
entitlement


For me the solution 3 is the best one, it is more flexible for the user.

Solution 1 is obviously bad.

Solution 2 forces the user to specify entitlement high and only high if 
it specifies dedication true.

AFAIU, you prefer the solution 2, forcing user to specify both 
dedication and entitlement to suppress a default value in the enum.
Why is it bad to have a default value in the enum that we do not use to 
specify that the value must be calculated?


> > +}
> > +
> > +/**
> > + * s390_topology_check:
> > + * @socket_id: socket to check
> > + * @book_id: book to check
> > + * @drawer_id: drawer to check
> > + * @entitlement: entitlement to check
> > + * @dedicated: dedication to check
> > + * @errp: Error pointer
> > + *
> > + * The function checks if the topology
> > + * attributes fits inside the system topology.
> fitS
>
> The function checks the validity of the provided topology arguments,
> namely that they're in bounds and non contradictory.


OK, thanks.


> > + */
> > +static void s390_topology_check(uint16_t socket_id, uint16_t book_id,
> I'd prefer if you stick to one id type. There defined as int32_t in env,
> here you use uint16_t and below int.
>
> In env, you want a signed type with sufficient range, int16_t should suffice 
> there,
> but bigger is also fine.
> You don't want the user to pass a negative id, so by using an unsigned type you
> can avoid this without extra code.
> But IMO there should be one point where a type conversion occurs.

OK


> > +                                uint16_t drawer_id, uint16_t entitlement,
> > +                                bool dedicated, Error **errp)
> > +{
> > +    CpuTopology *smp = s390_topology.smp;
> > +    ERRP_GUARD();
> > +
> > +    if (socket_id >= smp->sockets) {
> > +        error_setg(errp, "Unavailable socket: %d", socket_id);
> > +        return;
> > +    }
> > +    if (book_id >= smp->books) {
> > +        error_setg(errp, "Unavailable book: %d", book_id);
> > +        return;
> > +    }
> > +    if (drawer_id >= smp->drawers) {
> > +        error_setg(errp, "Unavailable drawer: %d", drawer_id);
> > +        return;
> > +    }
> > +    if (entitlement >= S390_CPU_ENTITLEMENT__MAX) {
> > +        error_setg(errp, "Unknown entitlement: %d", entitlement);
> > +        return;
> > +    }
> I think you can delete this, too, there is no way that entitlement is > MAX.

With entitlement being an enum in CPU properties yes.


> > +    if (dedicated && (entitlement == S390_CPU_ENTITLEMENT_LOW ||
> > +                      entitlement == S390_CPU_ENTITLEMENT_MEDIUM)) {
> Without HORIZONTAL you can do != HIGH and save one line, but that is
> cosmetic only.

Right, HORIZONTAL is eliminated during s390_topology_cpu_default()


Regards,

Pierre