Re: [PATCH 2/5] s390x/pv: Implement CGS check handler

[Thomas Huth <thuth@redhat.com>]

On 05/01/2023 15.47, Cédric Le Goater wrote:
> On 1/5/23 14:58, Claudio Imbrenda wrote:
>> On Thu, 5 Jan 2023 12:42:54 +0100
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> On 04/01/2023 12.51, Cédric Le Goater wrote:
>>>> From: Cédric Le Goater <clg@redhat.com>
>>>>
>>>> When a protected VM is started with the maximum number of CPUs (248),
>>>> the service call providing information on the CPUs requires more
>>>> buffer space than allocated and QEMU disgracefully aborts :
>>>>
>>>>       LOADPARM=[........]
>>>>       Using virtio-blk.
>>>>       Using SCSI scheme.
>>>>       
>>>> ................................................................................... 
>>>>
>>>>       qemu-system-s390x: KVM_S390_MEM_OP failed: Argument list too long
>>>>
>>>> Implement a test for this limitation in the ConfidentialGuestSupportClass
>>>> check handler and provide some valid information to the user before the
>>>> machine starts.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>>> ---
>>>>    hw/s390x/pv.c | 23 +++++++++++++++++++++++
>>>>    1 file changed, 23 insertions(+)
>>>>
>>>> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
>>>> index 8dfe92d8df..3a7ec70634 100644
>>>> --- a/hw/s390x/pv.c
>>>> +++ b/hw/s390x/pv.c
>>>> @@ -266,6 +266,26 @@ int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, 
>>>> Error **errp)
>>>>        return 0;
>>>>    }
>>>> +static bool s390_pv_check_cpus(Error **errp)
>>>> +{
>>>> +    MachineState *ms = MACHINE(qdev_get_machine());
>>>> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
>>>> +    uint32_t pv_max_cpus = mc->max_cpus - 1;
>>>
>>> Not sure whether "mc->max_cpus - 1" is the right approach here. I think it
>>> would be better to calculate the amount of CPUs that we can support.
>>>
>>> So AFAIK the problem is that SCLP information that is gathered during
>>> read_SCP_info() in hw/s390x/sclp.c. If protected virtualization is enabled,
>>> everything has to fit in one page (i.e. 4096 bytes) there.
>>>
>>> So we have space for
>>>
>>>    (TARGET_PAGE_SIZE - offset_cpu) / sizeof(CPUEntry)
>>>
>>> CPUs.
>>>
>>> With S390_FEAT_EXTENDED_LENGTH_SCCB enabled, offset_cpu is 144 (see struct
>>> ReadInfo in sclp.h), otherwise it is 128.
>>>
>>> That means, with S390_FEAT_EXTENDED_LENGTH_SCCB we can have a maximum of:
>>>
>>>    (4096 - 144) / 16 = 247 CPUs
>>>
>>> which is what you were trying to check with the mc->max_cpus - 1 here.
> 
> yes. That's much better.
> 
>>> But with "-cpu els=off", it sounds like we could fit all 248 also with
>>> protected VMs? Could you please give it a try?
> 
> It runs. Unfortunately, QEMU also complains with :
> 
>    qemu-system-s390x: warning: 'diag318' requires 'els'.
>    qemu-system-s390x: warning: 'diag318' requires 'els'.
>    qemu-system-s390x: warning: 'diag318' requires 'els'.
> 
> when els is off.

There is also a switch for that: -cpu els=off,diag318=off

>>> Anyway, instead of using "pv_max_cpus = mc->max_cpus - 1" I'd suggest to use
>>> something like this instead:
>>>
>>>    int offset_cpu = s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
>>>                        offsetof(ReadInfo, entries) :
>>>                        SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
>>>    pv_max_cpus = (TARGET_PAGE_SIZE - offset_cpu) /sizeof(CPUEntry);
>>
>> I agree with Thomas here
> 
> 
> The problem is that QEMU doesn't know about the S390_FEAT_EXTENDED_LENGTH_SCCB
> feature when the PV object link is first checked. So #248 CPUs is considered
> valid, but when DIAG308_PV_START is called, it fails.

Drat. Is there any chance that the check could be done somewhere later?

> Let's simplify and use :
> 
>      int offset_cpu = offsetof(ReadInfo, entries);
>      pv_max_cpus = (TARGET_PAGE_SIZE - offset_cpu) /sizeof(CPUEntry);
> 
> ?

Depends ... if it is possible to use 248 CPUs with -cpu els=off,diag318=off 
then it would be nicer to allow that, too?

  Thomas