Re: [PATCH v10 2/9] s390x/cpu topology: reporting the CPU topology to the guest

Pierre Morel
2022-10-28T10:05:28Z

On 10/27/22 22:42, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-10-12 at 18:21 +0200, Pierre Morel wrote:
>> The guest can use the STSI instruction to get a buffer filled
>> with the CPU topology description.
>>
>> Let us implement the STSI instruction for the basis CPU topology
>> level, level 2.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>> include/hw/s390x/cpu-topology.h | 3 +
>> target/s390x/cpu.h | 48 ++++++++++++++
>> hw/s390x/cpu-topology.c | 8 ++-
>> target/s390x/cpu_topology.c | 109 ++++++++++++++++++++++++++++++++
>> target/s390x/kvm/kvm.c | 6 +-
>> target/s390x/meson.build | 1 +
>> 6 files changed, 172 insertions(+), 3 deletions(-)
>> create mode 100644 target/s390x/cpu_topology.c
>>
>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
>> index 66c171d0bc..61c11db017 100644
>> --- a/include/hw/s390x/cpu-topology.h
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -13,6 +13,8 @@
>> #include "hw/qdev-core.h"
>> #include "qom/object.h"
>>
>> +#define S390_TOPOLOGY_POLARITY_H 0x00
>> +
>> typedef struct S390TopoContainer {
>> int active_count;
>> } S390TopoContainer;
>> @@ -29,6 +31,7 @@ struct S390Topology {
>> S390TopoContainer *socket;
>> S390TopoTLE *tle;
>> MachineState *ms;
>> + QemuMutex topo_mutex;
>> };
>>
>> #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 7d6d01325b..d604aa9c78 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>>
> [...]
>> +
>> +/* Maxi size of a SYSIB structure is when all CPU are alone in a container */
>
> Max or Maximum.
>
>> +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) + \
>> + S390_MAX_CPUS * (sizeof(SysIBTl_container) + \
>> + sizeof(SysIBTl_cpu)))
>
> Currently this is 16+248*3*8 == 5968 and will grow with books, drawer support to
> 16+248*5*8 == 9936 ...
>
> [...]
>>
>> +
>> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
>> +{
>> + uint64_t page[S390_TOPOLOGY_SYSIB_SIZE / sizeof(uint64_t)] = {};
>
> ... so calling this page is a bit misleading. Also why not make it a char[]?
> And maybe use a union for type punning.

OK, what about:

union {
char place_holder[S390_TOPOLOGY_SYSIB_SIZE];
SysIB_151x sysib;
} buffer QEMU_ALIGNED(8);

>
>> + SysIB_151x *sysib = (SysIB_151x *) page;
>> + int len;
>> +
>> + if (s390_is_pv() || !s390_has_topology() ||
>> + sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
>> + setcc(cpu, 3);
>> + return;
>> + }
>> +
>> + len = setup_stsi(sysib, sel2);
>
> This should now be memory safe, but might be larger than 4k,
> the maximum size of the SYSIB. I guess you want to set cc code 3
> in this case and return.

I do not find why the SYSIB can not be larger than 4k.
Can you point me to this restriction?

Regards,
Pierre

--
Pierre Morel
IBM Lab Boeblingen