Re: [PATCH v20 03/21] target/s390x/cpu topology: handle STSI(15) and build the SYSIB

[Thomas Huth <thuth@xxxxxxxxxx>]

On 25/04/2023 18.14, Pierre Morel wrote:
> On interception of STSI(15.1.x) the System Information Block
> (SYSIB) is built from the list of pre-ordered topology entries.
>
> Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> ---
>   MAINTAINERS                     |   1 +
>   include/hw/s390x/cpu-topology.h |  24 +++
>   include/hw/s390x/sclp.h         |   1 +
>   target/s390x/cpu.h              |  72 ++++++++
>   hw/s390x/cpu-topology.c         |  13 +-
>   target/s390x/kvm/cpu_topology.c | 308 ++++++++++++++++++++++++++++++++
>   target/s390x/kvm/kvm.c          |   5 +-
>   target/s390x/kvm/meson.build    |   3 +-
>   8 files changed, 424 insertions(+), 3 deletions(-)
>   create mode 100644 target/s390x/kvm/cpu_topology.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bb7b34d0d8..de9052f753 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1659,6 +1659,7 @@ M: Pierre Morel <pmorel@xxxxxxxxxxxxx>
>   S: Supported
>   F: include/hw/s390x/cpu-topology.h
>   F: hw/s390x/cpu-topology.c
> +F: target/s390x/kvm/cpu_topology.c

It's somewhat weird to have one file "cpu-topology.c" (in hw/s390x, with a 
dash), and one file cpu_topology.c (in target/s390x, with an underscore) ... 
could you come up with a better naming? Maybe call the new file 
stsi-topology.c or so?

> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index bb7cfb0cab..9f97989bd7 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -561,6 +561,25 @@ typedef struct SysIB_322 {
>   } SysIB_322;
>   QEMU_BUILD_BUG_ON(sizeof(SysIB_322) != 4096);


Maybe add a short comment here what MAG stands for (magnitude fields?)?
> +#define S390_TOPOLOGY_MAG  6
> +#define S390_TOPOLOGY_MAG6 0
> +#define S390_TOPOLOGY_MAG5 1
> +#define S390_TOPOLOGY_MAG4 2
> +#define S390_TOPOLOGY_MAG3 3
> +#define S390_TOPOLOGY_MAG2 4
> +#define S390_TOPOLOGY_MAG1 5
> +/* Configuration topology */
> +typedef struct SysIB_151x {
> +    uint8_t  reserved0[2];
> +    uint16_t length;
> +    uint8_t  mag[S390_TOPOLOGY_MAG];
> +    uint8_t  reserved1;
> +    uint8_t  mnest;
> +    uint32_t reserved2;
> +    char tle[];
> +} SysIB_151x;
> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
...

> diff --git a/target/s390x/kvm/cpu_topology.c b/target/s390x/kvm/cpu_topology.c
> new file mode 100644
> index 0000000000..86a286afe2
> --- /dev/null
> +++ b/target/s390x/kvm/cpu_topology.c
> @@ -0,0 +1,308 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * QEMU S390x CPU Topology
> + *
> + * Copyright IBM Corp. 2022,2023
> + * Author(s): Pierre Morel <pmorel@xxxxxxxxxxxxx>
> + *
> + */
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "hw/s390x/pv.h"
> +#include "hw/sysbus.h"
> +#include "hw/s390x/sclp.h"
> +#include "hw/s390x/cpu-topology.h"
> +
> +/**
> + * fill_container:
> + * @p: The address of the container TLE to fill
> + * @level: The level of nesting for this container
> + * @id: The container receives a uniq ID inside its own container

s/uniq/unique/

 Thomas