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

[Pierre Morel <pmorel@linux.ibm.com>]

On 4/27/23 19:01, Thomas Huth wrote:
> 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@linux.ibm.com>
>> ---
>>   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@linux.ibm.com>
>>   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?


If I keep the two files (see answer to your comments on patch 2) then 
yes I can change it to stsi-topology.c,


>
>> 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?)?


Yes, in fact Magnitude bytes.


>> +#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@linux.ibm.com>
>> + *
>> + */
>> +#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/


yes, thanks,

regards,

Pierre


>
>  Thomas
>