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

Pierre Morel
2022-11-07T13:58:12Z

On 11/7/22 14:20, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-10-28 at 12:00 +0200, Pierre Morel wrote:
>>
>> 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);
>>
> I don't think you need the QEMU_ALIGNED since SysIB_151x already has it. Not that it hurts to be
> explicit. If you declared the tle member as uint64_t[], you should get the correct alignment
> automatically and can then drop the explicit one.

I find the explicit statement better. Why make it non explicit?

> Btw, [] seems to be preferred over [0], at least there is a commit doing a conversion:
> f7795e4096 ("misc: Replace zero-length arrays with flexible array member (automatic)")

OK

>>
>>>
>>>> + 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?
>
> Says so at the top of the description of STSI:
>
> The SYSIB is 4K bytes and must begin at a 4 K-byte
> boundary; otherwise, a specification exception may
> be recognized.

Right, I guess I can not read.

So I will return CC=3 in case the length is greater than 4K

thanks,
Regards,

Pierre

--
Pierre Morel
IBM Lab Boeblingen