| Message ID | 20230116174607.2459498-3-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/pv: Improve error reporting of protected VMs | expand |
On 16/01/2023 18.46, Cédric Le Goater wrote: > From: Cédric Le Goater <clg@redhat.com> > > If a secure kernel is started in a non-protected VM, the OS will hang > during boot without giving a proper error message to the user. > > Perform the checks on Confidential Guest support at runtime with an > helper called from the service call switching the guest to protected > mode. > > Signed-off-by: Cédric Le Goater <clg@redhat.com> > --- > > In s390_pv_check(), drop the call to s390_pv_guest_check() since > init time has already checked the same conditions. However, to > report an error when the machine is not protected and the kernel > secure, we still need s390_pv_check(). Basically Ack for this patch ... I'm just wondering whether we should maybe use a different name for the function. We now have s390_pv_guest_check() and 390_pv_check() ... hard to distinguish. Maybe we should call them s390_pv_initial_check() and s390_pv_runtime_check() (or s390_pv_diag308_check()) or something similar instead? Thomas
On 1/16/23 18:46, Cédric Le Goater wrote: > From: Cédric Le Goater <clg@redhat.com> > > If a secure kernel is started in a non-protected VM, the OS will hang > during boot without giving a proper error message to the user. Didn't we establish that you were missing the IOMMU flag so this statement isn't correct anymore? I haven't yet fully ingested my coffee, but from what I understand you would block a switch into PV mode if cgs is not set. Which would mean that PV KVM unit tests wouldn't start anymore as well as any VMs that have the unpack feature but not cgs. And that's not something that we want. You can start a PV VM without cgs if unpack is in the CPU model. The ONLY requirement that we should fail on is unpack. Have a look at what David Gibson put in the commit message when he introduced that in 651615d9: """ To integrate this with the option used by other platforms, we implement the following compromise: - When the confidential-guest-support option is set, s390 will recognize it, verify that the CPU can support PV (failing if not) and set virtio default options necessary for encrypted or protected guests, as on other platforms. i.e. if confidential-guest-support is set, we will either create a guest capable of entering PV mode, or fail outright. - If confidential-guest-support is not set, guests might still be able to enter PV mode, if the CPU has the right model. This may be a little surprising, but shouldn't actually be harmful. """
On 1/17/23 09:40, Janosch Frank wrote: > On 1/16/23 18:46, Cédric Le Goater wrote: >> From: Cédric Le Goater <clg@redhat.com> >> >> If a secure kernel is started in a non-protected VM, the OS will hang >> during boot without giving a proper error message to the user. > > Didn't we establish that you were missing the IOMMU flag so this statement isn't correct anymore? yes. Which means it is pointless to run the machine because it will fail to boot with no means to understand why. > I haven't yet fully ingested my coffee, but from what I understand you would block a switch into PV mode if cgs is not set. Which would mean that PV KVM unit tests wouldn't start anymore as well as any VMs that have the unpack feature but not cgs. > > > And that's not something that we want. > > You can start a PV VM without cgs if unpack is in the CPU model. The ONLY requirement that we should fail on is unpack. ok. > Have a look at what David Gibson put in the commit message when he introduced that in 651615d9: > > """ > To integrate this with the option used by other platforms, we > implement the following compromise: > > - When the confidential-guest-support option is set, s390 will > recognize it, verify that the CPU can support PV (failing if not) > and set virtio default options necessary for encrypted or protected > guests, as on other platforms. i.e. if confidential-guest-support > is set, we will either create a guest capable of entering PV mode, > or fail outright. > > - If confidential-guest-support is not set, guests might still be > able to enter PV mode, if the CPU has the right model. This may be > a little surprising, but shouldn't actually be harmful. > """ yes and it is not that clear how a s390 PV machine should be started, even for a developer. Thanks for looking, C.
On 17/01/2023 09.40, Janosch Frank wrote: > On 1/16/23 18:46, Cédric Le Goater wrote: >> From: Cédric Le Goater <clg@redhat.com> >> >> If a secure kernel is started in a non-protected VM, the OS will hang >> during boot without giving a proper error message to the user. > > Didn't we establish that you were missing the IOMMU flag so this statement > isn't correct anymore? > > > I haven't yet fully ingested my coffee, but from what I understand you would > block a switch into PV mode if cgs is not set. Which would mean that PV KVM > unit tests wouldn't start anymore as well as any VMs that have the unpack > feature but not cgs. > > And that's not something that we want. > > You can start a PV VM without cgs if unpack is in the CPU model. The ONLY > requirement that we should fail on is unpack. So would it make sense to check for S390_FEAT_UNPACK (or something else?) here, or should the patch completely be dropped instead? Thomas
On 1/17/23 10:09, Thomas Huth wrote: > On 17/01/2023 09.40, Janosch Frank wrote: >> On 1/16/23 18:46, Cédric Le Goater wrote: >>> From: Cédric Le Goater <clg@redhat.com> >>> >>> If a secure kernel is started in a non-protected VM, the OS will hang >>> during boot without giving a proper error message to the user. >> >> Didn't we establish that you were missing the IOMMU flag so this statement >> isn't correct anymore? >> >> >> I haven't yet fully ingested my coffee, but from what I understand you would >> block a switch into PV mode if cgs is not set. Which would mean that PV KVM >> unit tests wouldn't start anymore as well as any VMs that have the unpack >> feature but not cgs. >> >> And that's not something that we want. >> >> You can start a PV VM without cgs if unpack is in the CPU model. The ONLY >> requirement that we should fail on is unpack. > > So would it make sense to check for S390_FEAT_UNPACK (or something else?) > here, or should the patch completely be dropped instead? Subcodes 8 - 10 already result in a spec PGM for !unpack and if memory serves me right then Marc will catch that and print a warning in the genprotimg kernel: if (subcode >= DIAG308_PV_SET && !s390_has_feat(S390_FEAT_UNPACK)) { s390_program_interrupt(env, PGM_SPECIFICATION, ra); return; } So other than shuffling code around I see no benefit in this patch.
diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h index 9360aa1091..ca7dac2e20 100644 --- a/include/hw/s390x/pv.h +++ b/include/hw/s390x/pv.h @@ -55,6 +55,7 @@ int kvm_s390_dump_init(void); int kvm_s390_dump_cpu(S390CPU *cpu, void *buff); int kvm_s390_dump_mem_state(uint64_t addr, size_t len, void *dest); int kvm_s390_dump_completion_data(void *buff); +bool s390_pv_check(Error **errp); #else /* CONFIG_KVM */ static inline bool s390_is_pv(void) { return false; } static inline int s390_pv_query_info(void) { return 0; } @@ -75,6 +76,7 @@ static inline int kvm_s390_dump_cpu(S390CPU *cpu, void *buff) { return 0; } static inline int kvm_s390_dump_mem_state(uint64_t addr, size_t len, void *dest) { return 0; } static inline int kvm_s390_dump_completion_data(void *buff) { return 0; } +static inline bool s390_pv_check(Error **errp) { return false; } #endif /* CONFIG_KVM */ int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp); diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c index 8a1c71436b..8405e73a1b 100644 --- a/hw/s390x/pv.c +++ b/hw/s390x/pv.c @@ -306,6 +306,19 @@ int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) return 0; } +bool s390_pv_check(Error **errp) +{ + MachineState *ms = MACHINE(qdev_get_machine()); + + if (!ms->cgs) { + error_setg(errp, "Protected VM is started without Confidential" + " Guest support"); + return false; + } + + return true; +} + OBJECT_DEFINE_TYPE_WITH_INTERFACES(S390PVGuest, s390_pv_guest, S390_PV_GUEST, diff --git a/target/s390x/diag.c b/target/s390x/diag.c index 76b01dcd68..9b16e25930 100644 --- a/target/s390x/diag.c +++ b/target/s390x/diag.c @@ -79,6 +79,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra) uint64_t addr = env->regs[r1]; uint64_t subcode = env->regs[r3]; IplParameterBlock *iplb; + Error *local_err = NULL; if (env->psw.mask & PSW_MASK_PSTATE) { s390_program_interrupt(env, PGM_PRIVILEGED, ra); @@ -176,6 +177,12 @@ out: return; } + if (!s390_pv_check(&local_err)) { + error_report_err(local_err); + env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV; + return; + } + s390_ipl_reset_request(cs, S390_RESET_PV); break; default: