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@xxxxxxxxxxxxx>
> ---
> 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.
> };
>
> 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.
> +}
> +
> +/**
> + * 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.
> + */
> +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.
> + 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.
> + 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.
> + error_setg(errp, "A dedicated cpu implies high entitlement");
> + return;
> + }
> +}
[...]
