Re: [PATCH v2 1/4] s390x/pv: Implement a CGS check helper

["Cédric Le Goater" <clg@redhat.com>]

On 1/9/23 14:34, Thomas Huth wrote:
> On 06/01/2023 08.53, 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
>>
>> When protected virtualization is initialized, compute the maximum
>> number of vCPUs supported by the machine and return useful information
>> to the user before the machine starts in case of error.
>>
>> Suggested-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   hw/s390x/pv.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>
>> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
>> index 8dfe92d8df..8a1c71436b 100644
>> --- a/hw/s390x/pv.c
>> +++ b/hw/s390x/pv.c
>> @@ -20,6 +20,7 @@
>>   #include "exec/confidential-guest-support.h"
>>   #include "hw/s390x/ipl.h"
>>   #include "hw/s390x/pv.h"
>> +#include "hw/s390x/sclp.h"
>>   #include "target/s390x/kvm/kvm_s390x.h"
>>   static bool info_valid;
>> @@ -249,6 +250,41 @@ struct S390PVGuestClass {
>>       ConfidentialGuestSupportClass parent_class;
>>   };
>> +/*
>> + * If protected virtualization is enabled, the amount of data that the
>> + * Read SCP Info Service Call can use is limited to one page. The
>> + * available space also depends on the Extended-Length SCCB (ELS)
>> + * feature which can take more buffer space to store feature
>> + * information. This impacts the maximum number of CPUs supported in
>> + * the machine.
>> + */
>> +static uint32_t s390_pv_get_max_cpus(void)
>> +{
>> +    int offset_cpu = s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
>> +        offsetof(ReadInfo, entries) : SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
>> +
>> +    return (TARGET_PAGE_SIZE - offset_cpu) / sizeof(CPUEntry);
>> +}
>> +
>> +static bool s390_pv_check_cpus(Error **errp)
>> +{
>> +    MachineState *ms = MACHINE(qdev_get_machine());
>> +    uint32_t pv_max_cpus = s390_pv_get_max_cpus();
>> +
>> +    if (ms->smp.max_cpus > pv_max_cpus) {
>> +        error_setg(errp, "Protected VMs support a maximum of %d CPUs",
>> +                   pv_max_cpus);
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +static bool s390_pv_guest_check(ConfidentialGuestSupport *cgs, Error **errp)
>> +{
>> +    return s390_pv_check_cpus(errp);
>> +}
>> +
>>   int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>>   {
>>       if (!object_dynamic_cast(OBJECT(cgs), TYPE_S390_PV_GUEST)) {
>> @@ -261,6 +297,10 @@ int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>>           return -1;
>>       }
>> +    if (!s390_pv_guest_check(cgs, errp)) {
>> +        return -1;
>> +    }
>> +
>>       cgs->ready = true;
>>       return 0;
> 
> Looks good to me now.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

I think we could move the huge page test in s390_pv_guest_check() also.
We are finishing a discussion with Janosch on the runtime test and I will
send a v3.

Thanks,

C.