| Message ID | 20230104115111.3240594-3-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/pv: Improve protected VM support | expand |
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. But with "-cpu els=off", it sounds like we could fit all 248 also with protected VMs? Could you please give it a try? 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); Thomas
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. > > But with "-cpu els=off", it sounds like we could fit all 248 also with > protected VMs? Could you please give it a try? > > 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 > > Thomas > >
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. >> 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. Let's simplify and use : int offset_cpu = offsetof(ReadInfo, entries); pv_max_cpus = (TARGET_PAGE_SIZE - offset_cpu) /sizeof(CPUEntry); ? Thanks, C.
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
[ .. ] >> 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? s390_pv_init() could be the place. It looks like a good option since the CPUs have been initialized. To be explored. Thanks, C. >> 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 >
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; + + 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(const Object *obj, Error **errp) +{ + return s390_pv_check_cpus(errp); +} + OBJECT_DEFINE_TYPE_WITH_INTERFACES(S390PVGuest, s390_pv_guest, S390_PV_GUEST, @@ -275,6 +295,9 @@ OBJECT_DEFINE_TYPE_WITH_INTERFACES(S390PVGuest, static void s390_pv_guest_class_init(ObjectClass *oc, void *data) { + ConfidentialGuestSupportClass *cgsc = CONFIDENTIAL_GUEST_SUPPORT_CLASS(oc); + + cgsc->check = s390_pv_guest_check; } static void s390_pv_guest_init(Object *obj)