| Message ID | 20230106075330.3662549-2-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/pv: Improve protected VM support | expand |
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>
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.
On 09/01/2023 14.57, Cédric Le Goater wrote: > 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. Core question is likely: What if the hypervisor admin does not know whether the guest will run in protected mode or not, and thus always wants to enable the feature (so that the owner of the guest can decide)? So we cannot know right from the start whether we have a confidential guest or not? ... should we then really check the condition at the beginning, or is it better to check when the guest tries to switch to protected mode? Thomas
On 1/9/23 15:12, Thomas Huth wrote: > On 09/01/2023 14.57, Cédric Le Goater wrote: >> 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. > > Core question is likely: What if the hypervisor admin does not know whether the guest will run in protected mode or not, and thus always wants to enable the feature (so that the owner of the guest can decide)? If we take this direction, then, on a protected host, libvirt (or QEMU ?) would need to set the VM as protected always : machine option and CGS object passed on the command line. > So we cannot know right from the start whether we have a confidential > guest or not? ... AFAIUI, it only depends on the kernel of the guest OS. A non secure kernel runs fine in a protected VM. The PV switch is not performed in diag308. C. > should we then really check the condition at the beginning, > or is it better to check when the guest tries to switch to protected mode?
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;