| Message ID | 20221214194056.161492-38-michael.roth@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support | expand |
On Wed, Dec 14, 2022 at 01:40:29PM -0600, Michael Roth wrote: > static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > { > struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > @@ -260,13 +279,23 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > return ret; > > sev->active = true; > - sev->es_active = argp->id == KVM_SEV_ES_INIT; > + sev->es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT); > + sev->snp_active = argp->id == KVM_SEV_SNP_INIT; > asid = sev_asid_new(sev); > if (asid < 0) > goto e_no_asid; > sev->asid = asid; > > - ret = sev_platform_init(&argp->error); > + if (sev->snp_active) { > + ret = verify_snp_init_flags(kvm, argp); > + if (ret) > + goto e_free; > + > + ret = sev_snp_init(&argp->error, false); > + } else { > + ret = sev_platform_init(&argp->error); > + } Couldn't sev_snp_init() and sev_platform_init() be called unconditionally in order? Since there is a hardware constraint that SNP init needs to always happen before platform init, shouldn't SNP init happen as part of __sev_platform_init_locked() instead? I found these call sites for __sev_platform_init_locked(), none of which follow the correct call order: * sev_guest_init() * sev_ioctl_do_pek_csr * sev_ioctl_do_pdh_export() * sev_ioctl_do_pek_import() * sev_ioctl_do_pek_pdh_gen() * sev_pci_init() For me it looks like a bit flakky API use to have sev_snp_init() as an API call. I would suggest to make SNP init internal to the ccp driver and take care of the correct orchestration over there. Also, how it currently works in this patch set, if the firmware did not load correctly, SNP init halts the whole system. The version check needs to be in all call paths. BR, Jarkko
A couple of fixups. On Sat, Dec 31, 2022 at 02:27:57PM +0000, Jarkko Sakkinen wrote: > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index 6c4fdcaed72b..462c9aaa2e7e 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -1381,6 +1381,12 @@ static int __sev_snp_init_locked(int *error) > if (sev->snp_initialized) > return 0; > > + if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) { > + dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n", > + SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR); > + return -ENODEV; return 0; It is not a failure case anyway. > + } > + > /* > * The SNP_INIT requires the MSR_VM_HSAVE_PA must be set to 0h > * across all cores. > @@ -2313,25 +2319,19 @@ void sev_pci_init(void) > } > } > > + rc = sev_snp_init(&error, true); > + if (rc != -ENODEV) if (rc) Because other wise there would need to be nasty "if (rc && rc != ENODEV)" so that this does not happen: [ 9.321588] ccp 0000:49:00.1: SEV-SNP: failed to INIT error 0x0 BR, Jarkko
On Sat, Dec 31, 2022 at 02:47:29PM +0000, Jarkko Sakkinen wrote: > A couple of fixups. > > On Sat, Dec 31, 2022 at 02:27:57PM +0000, Jarkko Sakkinen wrote: > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > > index 6c4fdcaed72b..462c9aaa2e7e 100644 > > --- a/drivers/crypto/ccp/sev-dev.c > > +++ b/drivers/crypto/ccp/sev-dev.c > > @@ -1381,6 +1381,12 @@ static int __sev_snp_init_locked(int *error) > > if (sev->snp_initialized) > > return 0; > > > > + if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) { > > + dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n", > > + SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR); > > + return -ENODEV; > > return 0; > > It is not a failure case anyway. > > > + } > > + > > /* > > * The SNP_INIT requires the MSR_VM_HSAVE_PA must be set to 0h > > * across all cores. > > @@ -2313,25 +2319,19 @@ void sev_pci_init(void) > > } > > } > > > > + rc = sev_snp_init(&error, true); > > + if (rc != -ENODEV) > > > if (rc) > > Because other wise there would need to be nasty "if (rc && rc != ENODEV)" > so that this does not happen: > > [ 9.321588] ccp 0000:49:00.1: SEV-SNP: failed to INIT error 0x0 > > BR, Jarkko This patch (not dependent on the series) is kind of related to my feedback. Since platform init can span from quite many locations it would be useful to get errors reported from all locations: https://www.lkml.org/lkml/2022/12/31/175 Would be IMHO good to have this in the baseline when testing SNP init functionality. BR, Jarkko
Hello Jarkko, On 12/31/2022 8:27 AM, Jarkko Sakkinen wrote: > On Wed, Dec 14, 2022 at 01:40:29PM -0600, Michael Roth wrote: >> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) >> { >> struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; >> @@ -260,13 +279,23 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) >> return ret; >> >> sev->active = true; >> - sev->es_active = argp->id == KVM_SEV_ES_INIT; >> + sev->es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT); >> + sev->snp_active = argp->id == KVM_SEV_SNP_INIT; >> asid = sev_asid_new(sev); >> if (asid < 0) >> goto e_no_asid; >> sev->asid = asid; >> >> - ret = sev_platform_init(&argp->error); >> + if (sev->snp_active) { >> + ret = verify_snp_init_flags(kvm, argp); >> + if (ret) >> + goto e_free; >> + >> + ret = sev_snp_init(&argp->error, false); >> + } else { >> + ret = sev_platform_init(&argp->error); >> + } > > Couldn't sev_snp_init() and sev_platform_init() be called unconditionally > in order? > > Since there is a hardware constraint that SNP init needs to always happen > before platform init, shouldn't SNP init happen as part of > __sev_platform_init_locked() instead? > On Genoa there is currently an issue that if we do an SNP_INIT before an SEV_INIT and then attempt to launch a SEV guest that may fail, so we need to keep SNP INIT and SEV INIT separate. We need to provide a way to run (existing) SEV guests on a system that supports SNP without doing an SNP_INIT at all. This is done using psp_init_on_probe parameter of the CCP module to avoid doing either SNP/SEV firmware initialization during module load and then defer the firmware initialization till someone launches a guest of one flavor or the other. And then sev_guest_init() does either SNP or SEV firmware init depending on the type of the guest being launched. > I found these call sites for __sev_platform_init_locked(), none of which > follow the correct call order: > > * sev_guest_init() As explained above, this call site is important for deferring the firmware initialization to an actual guest launch. > * sev_ioctl_do_pek_csr > * sev_ioctl_do_pdh_export() > * sev_ioctl_do_pek_import() > * sev_ioctl_do_pek_pdh_gen() > * sev_pci_init() > > For me it looks like a bit flakky API use to have sev_snp_init() as an API > call. > > I would suggest to make SNP init internal to the ccp driver and take care > of the correct orchestration over there. > Due to Genoa issue, we may still need SNP init and SEV init to be invoked separately outside the CCP driver. > Also, how it currently works in this patch set, if the firmware did not > load correctly, SNP init halts the whole system. The version check needs > to be in all call paths. > Yes, i agree with that. Thanks, Ashish
On Thu, Jan 05, 2023 at 05:37:20PM -0600, Kalra, Ashish wrote: > Hello Jarkko, > > On 12/31/2022 8:27 AM, Jarkko Sakkinen wrote: > > On Wed, Dec 14, 2022 at 01:40:29PM -0600, Michael Roth wrote: > > > static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > > > { > > > struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > > > @@ -260,13 +279,23 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > > > return ret; > > > sev->active = true; > > > - sev->es_active = argp->id == KVM_SEV_ES_INIT; > > > + sev->es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT); > > > + sev->snp_active = argp->id == KVM_SEV_SNP_INIT; > > > asid = sev_asid_new(sev); > > > if (asid < 0) > > > goto e_no_asid; > > > sev->asid = asid; > > > - ret = sev_platform_init(&argp->error); > > > + if (sev->snp_active) { > > > + ret = verify_snp_init_flags(kvm, argp); > > > + if (ret) > > > + goto e_free; > > > + > > > + ret = sev_snp_init(&argp->error, false); > > > + } else { > > > + ret = sev_platform_init(&argp->error); > > > + } > > > > Couldn't sev_snp_init() and sev_platform_init() be called unconditionally > > in order? > > > > Since there is a hardware constraint that SNP init needs to always happen > > before platform init, shouldn't SNP init happen as part of > > __sev_platform_init_locked() instead? > > > > On Genoa there is currently an issue that if we do an SNP_INIT before an > SEV_INIT and then attempt to launch a SEV guest that may fail, so we need to > keep SNP INIT and SEV INIT separate. > > We need to provide a way to run (existing) SEV guests on a system that > supports SNP without doing an SNP_INIT at all. > > This is done using psp_init_on_probe parameter of the CCP module to avoid > doing either SNP/SEV firmware initialization during module load and then > defer the firmware initialization till someone launches a guest of one > flavor or the other. > > And then sev_guest_init() does either SNP or SEV firmware init depending on > the type of the guest being launched. OK, got it, thank you. I have not noticed the init_on_probe for sev_snp_init() before. Was it in earlier patch set version? The benefit of having everything in __sev_platform_init_lock() would be first less risk of shooting yourself into foot, and also no need to pass init_on_probe to sev_snp_init() as it would be internal to sev-dev.c, and no need for special cases for callers. It is in my opinion internals of the SEV driver to guarantee the order. E.g. changes to svm/sev.c would be then quite trivial. > > I found these call sites for __sev_platform_init_locked(), none of which > > follow the correct call order: > > > > * sev_guest_init() > > As explained above, this call site is important for deferring the firmware > initialization to an actual guest launch. > > > * sev_ioctl_do_pek_csr > > * sev_ioctl_do_pdh_export() > > * sev_ioctl_do_pek_import() > > * sev_ioctl_do_pek_pdh_gen() What happens if any of these are called before sev_guest_init()? They only call __sev_platform_init_locked(). > > * sev_pci_init() > > > > For me it looks like a bit flakky API use to have sev_snp_init() as an API > > call. > > > > I would suggest to make SNP init internal to the ccp driver and take care > > of the correct orchestration over there. > > > > Due to Genoa issue, we may still need SNP init and SEV init to be invoked > separately outside the CCP driver. > > > Also, how it currently works in this patch set, if the firmware did not > > load correctly, SNP init halts the whole system. The version check needs > > to be in all call paths. > > > > Yes, i agree with that. Attached the fix I sent in private earlier. > Thanks, > Ashish BR, Jarkko
There was an early firmware issue on Genoa which supported only SNP_INIT or SEV_INIT, but this issue is resolved now. Now, the main constraints are that SNP_INIT is always required before SEV_INIT in case we want to launch SNP guests. In other words, if only SEV_INIT is done on a platform which supports SNP we won't be able to launch SNP guests after that. So once we have RMP table setup (in BIOS) we will always do an SNP_INIT and SEV_INIT will be ideally done only (on demand) when an SEV guest is launched. Thanks, Ashish On 1/5/2023 5:37 PM, Kalra, Ashish wrote: > Hello Jarkko, > > On 12/31/2022 8:27 AM, Jarkko Sakkinen wrote: >> On Wed, Dec 14, 2022 at 01:40:29PM -0600, Michael Roth wrote: >>> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) >>> { >>> struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; >>> @@ -260,13 +279,23 @@ static int sev_guest_init(struct kvm *kvm, >>> struct kvm_sev_cmd *argp) >>> return ret; >>> sev->active = true; >>> - sev->es_active = argp->id == KVM_SEV_ES_INIT; >>> + sev->es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == >>> KVM_SEV_SNP_INIT); >>> + sev->snp_active = argp->id == KVM_SEV_SNP_INIT; >>> asid = sev_asid_new(sev); >>> if (asid < 0) >>> goto e_no_asid; >>> sev->asid = asid; >>> - ret = sev_platform_init(&argp->error); >>> + if (sev->snp_active) { >>> + ret = verify_snp_init_flags(kvm, argp); >>> + if (ret) >>> + goto e_free; >>> + >>> + ret = sev_snp_init(&argp->error, false); >>> + } else { >>> + ret = sev_platform_init(&argp->error); >>> + } >> >> Couldn't sev_snp_init() and sev_platform_init() be called unconditionally >> in order? >> >> Since there is a hardware constraint that SNP init needs to always happen >> before platform init, shouldn't SNP init happen as part of >> __sev_platform_init_locked() instead? >> > > On Genoa there is currently an issue that if we do an SNP_INIT before an > SEV_INIT and then attempt to launch a SEV guest that may fail, so we > need to keep SNP INIT and SEV INIT separate. > > We need to provide a way to run (existing) SEV guests on a system that > supports SNP without doing an SNP_INIT at all. > > This is done using psp_init_on_probe parameter of the CCP module to > avoid doing either SNP/SEV firmware initialization during module load > and then defer the firmware initialization till someone launches a guest > of one flavor or the other. > > And then sev_guest_init() does either SNP or SEV firmware init depending > on the type of the guest being launched. > >> I found these call sites for __sev_platform_init_locked(), none of which >> follow the correct call order: >> >> * sev_guest_init() > > As explained above, this call site is important for deferring the > firmware initialization to an actual guest launch. > >> * sev_ioctl_do_pek_csr >> * sev_ioctl_do_pdh_export() >> * sev_ioctl_do_pek_import() >> * sev_ioctl_do_pek_pdh_gen() >> * sev_pci_init() >> >> For me it looks like a bit flakky API use to have sev_snp_init() as an >> API >> call. >> >> I would suggest to make SNP init internal to the ccp driver and take care >> of the correct orchestration over there. >> > > Due to Genoa issue, we may still need SNP init and SEV init to be > invoked separately outside the CCP driver. > >> Also, how it currently works in this patch set, if the firmware did not >> load correctly, SNP init halts the whole system. The version check needs >> to be in all call paths. >> > > Yes, i agree with that. > > Thanks, > Ashish
On Mon, Jan 23, 2023 at 04:49:14PM -0600, Kalra, Ashish wrote: > There was an early firmware issue on Genoa which supported only SNP_INIT or > SEV_INIT, but this issue is resolved now. > > Now, the main constraints are that SNP_INIT is always required before > SEV_INIT in case we want to launch SNP guests. In other words, if only > SEV_INIT is done on a platform which supports SNP we won't be able to launch > SNP guests after that. > > So once we have RMP table setup (in BIOS) we will always do an SNP_INIT and > SEV_INIT will be ideally done only (on demand) when an SEV guest is > launched. OK, thanks for the clarification! BR, Jarkko
diff --git a/Documentation/virt/kvm/x86/amd-memory-encryption.rst b/Documentation/virt/kvm/x86/amd-memory-encryption.rst index 935aaeb97fe6..2432213bd0ea 100644 --- a/Documentation/virt/kvm/x86/amd-memory-encryption.rst +++ b/Documentation/virt/kvm/x86/amd-memory-encryption.rst @@ -434,6 +434,33 @@ issued by the hypervisor to make the guest ready for execution. Returns: 0 on success, -negative on error +18. KVM_SNP_INIT +---------------- + +The KVM_SNP_INIT command can be used by the hypervisor to initialize SEV-SNP +context. In a typical workflow, this command should be the first command issued. + +Parameters (in/out): struct kvm_snp_init + +Returns: 0 on success, -negative on error + +:: + + struct kvm_snp_init { + __u64 flags; + }; + +The flags bitmap is defined as:: + + /* enable the restricted injection */ + #define KVM_SEV_SNP_RESTRICTED_INJET (1<<0) + + /* enable the restricted injection timer */ + #define KVM_SEV_SNP_RESTRICTED_TIMER_INJET (1<<1) + +If the specified flags is not supported then return -EOPNOTSUPP, and the supported +flags are returned. + References ========== diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h index cb1ee53ad3b1..c18d78d5e505 100644 --- a/arch/x86/include/asm/svm.h +++ b/arch/x86/include/asm/svm.h @@ -278,6 +278,7 @@ enum avic_ipi_failure_cause { #define AVIC_HPA_MASK ~((0xFFFULL << 52) | 0xFFF) #define VMCB_AVIC_APIC_BAR_MASK 0xFFFFFFFFFF000ULL +#define SVM_SEV_FEAT_SNP_ACTIVE BIT(0) struct vmcb_seg { u16 selector; diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index f34da1203e09..e3f857cde8c0 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -247,6 +247,25 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle) sev_decommission(handle); } +static int verify_snp_init_flags(struct kvm *kvm, struct kvm_sev_cmd *argp) +{ + struct kvm_snp_init params; + int ret = 0; + + if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data, sizeof(params))) + return -EFAULT; + + if (params.flags & ~SEV_SNP_SUPPORTED_FLAGS) + ret = -EOPNOTSUPP; + + params.flags = SEV_SNP_SUPPORTED_FLAGS; + + if (copy_to_user((void __user *)(uintptr_t)argp->data, ¶ms, sizeof(params))) + ret = -EFAULT; + + return ret; +} + static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) { struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; @@ -260,13 +279,23 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) return ret; sev->active = true; - sev->es_active = argp->id == KVM_SEV_ES_INIT; + sev->es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT); + sev->snp_active = argp->id == KVM_SEV_SNP_INIT; asid = sev_asid_new(sev); if (asid < 0) goto e_no_asid; sev->asid = asid; - ret = sev_platform_init(&argp->error); + if (sev->snp_active) { + ret = verify_snp_init_flags(kvm, argp); + if (ret) + goto e_free; + + ret = sev_snp_init(&argp->error, false); + } else { + ret = sev_platform_init(&argp->error); + } + if (ret) goto e_free; @@ -281,6 +310,7 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) sev_asid_free(sev); sev->asid = 0; e_no_asid: + sev->snp_active = false; sev->es_active = false; sev->active = false; return ret; @@ -741,6 +771,10 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm) save->xss = svm->vcpu.arch.ia32_xss; save->dr6 = svm->vcpu.arch.dr6; + /* Enable the SEV-SNP feature */ + if (sev_snp_guest(svm->vcpu.kvm)) + save->sev_features |= SVM_SEV_FEAT_SNP_ACTIVE; + pr_debug("Virtual Machine Save Area (VMSA):\n"); print_hex_dump_debug("", DUMP_PREFIX_NONE, 16, 1, save, sizeof(*save), false); @@ -1993,6 +2027,12 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp) } switch (sev_cmd.id) { + case KVM_SEV_SNP_INIT: + if (!sev_snp_enabled) { + r = -ENOTTY; + goto out; + } + fallthrough; case KVM_SEV_ES_INIT: if (!sev_es_enabled) { r = -ENOTTY; diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index a48fe5d2bea5..379b253d2464 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -80,6 +80,9 @@ enum { /* TPR and CR2 are always written before VMRUN */ #define VMCB_ALWAYS_DIRTY_MASK ((1U << VMCB_INTR) | (1U << VMCB_CR2)) +/* Supported init feature flags */ +#define SEV_SNP_SUPPORTED_FLAGS 0x0 + struct kvm_sev_info { bool active; /* SEV enabled guest */ bool es_active; /* SEV-ES enabled guest */ @@ -95,6 +98,7 @@ struct kvm_sev_info { struct list_head mirror_entry; /* Use as a list entry of mirrors */ struct misc_cg *misc_cg; /* For misc cgroup accounting */ atomic_t migration_in_progress; + u64 snp_init_flags; }; struct kvm_svm { diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index cc9424ccf9b2..a6c73297a62d 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -1938,6 +1938,9 @@ enum sev_cmd_id { /* Guest Migration Extension */ KVM_SEV_SEND_CANCEL, + /* SNP specific commands */ + KVM_SEV_SNP_INIT, + KVM_SEV_NR_MAX, }; @@ -2034,6 +2037,16 @@ struct kvm_sev_receive_update_data { __u32 trans_len; }; +/* enable the restricted injection */ +#define KVM_SEV_SNP_RESTRICTED_INJET (1 << 0) + +/* enable the restricted injection timer */ +#define KVM_SEV_SNP_RESTRICTED_TIMER_INJET (1 << 1) + +struct kvm_snp_init { + __u64 flags; +}; + #define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0) #define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1) #define KVM_DEV_ASSIGN_MASK_INTX (1 << 2)