| Message ID | 20250221210200.244405-2-prsampat@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v7,01/10] KVM: SEV: Disable SEV-SNP support on initialization failure | expand |
On 21/02/2025 21:01, Pratik R. Sampat wrote: > During platform init, SNP initialization may fail for several reasons, > such as firmware command failures and incompatible versions. However, > the KVM capability may continue to advertise support for it. Export this > information to KVM and withdraw SEV-SNP support if has not been > successfully initialized. > > Fixes: 1dfe571c12cf ("KVM: SEV: Add initial SEV-SNP support") > Suggested-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Pratik R. Sampat <prsampat@amd.com> > --- > v6..v7: > > * Replace FW version check with sev->snp_initialized (Sean) > --- > arch/x86/kvm/svm/sev.c | 4 +++- > drivers/crypto/ccp/sev-dev.c | 8 ++++++++ > include/linux/psp-sev.h | 3 +++ > 3 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 0dbb25442ec1..87b5d63a5817 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -3050,7 +3050,9 @@ void __init sev_hardware_setup(void) > sev_es_asid_count = min_sev_asid - 1; > WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count)); > sev_es_supported = true; > - sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP); > + sev_snp_supported = (sev_snp_enabled && > + cc_platform_has(CC_ATTR_HOST_SEV_SNP) && > + snp_initialized()); > > out: > if (boot_cpu_has(X86_FEATURE_SEV)) > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index 2e87ca0e292a..8d2cf8552bc2 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -1352,6 +1352,14 @@ int sev_platform_init(struct sev_platform_init_args *args) > } > EXPORT_SYMBOL_GPL(sev_platform_init); > > +bool snp_initialized(void) > +{ > + struct sev_device *sev = psp_master->sev_data; Should check psp_master isn't NULL before accessing just in case (particularly for future potential callers). (e.g. see ccb88e9549e7 ("crypto: ccp - Fix null pointer dereference in __sev_platform_shutdown_locked") > + > + return sev->snp_initialized; > +} > +EXPORT_SYMBOL_GPL(snp_initialized); > + > static int __sev_platform_shutdown_locked(int *error) > { > struct psp_device *psp = psp_master; > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h > index f3cad182d4ef..d34068c87a28 100644 > --- a/include/linux/psp-sev.h > +++ b/include/linux/psp-sev.h > @@ -954,6 +954,7 @@ int sev_do_cmd(int cmd, void *data, int *psp_ret); > void *psp_copy_user_blob(u64 uaddr, u32 len); > void *snp_alloc_firmware_page(gfp_t mask); > void snp_free_firmware_page(void *addr); > +bool snp_initialized(void); > > #else /* !CONFIG_CRYPTO_DEV_SP_PSP */ > > @@ -988,6 +989,8 @@ static inline void *snp_alloc_firmware_page(gfp_t mask) > > static inline void snp_free_firmware_page(void *addr) { } > > +static inline bool snp_initialized(void) { return false; } > + > #endif /* CONFIG_CRYPTO_DEV_SP_PSP */ > > #endif /* __PSP_SEV_H__ */
On 2/21/25 15:01, Pratik R. Sampat wrote: > During platform init, SNP initialization may fail for several reasons, > such as firmware command failures and incompatible versions. However, > the KVM capability may continue to advertise support for it. Export this > information to KVM and withdraw SEV-SNP support if has not been > successfully initialized. Hmmm... rather than creating a new API, can you just issue an SNP_PLATFORM_STATUS command and see if the SNP is not in the UNINIT state? Thanks, Tom > > Fixes: 1dfe571c12cf ("KVM: SEV: Add initial SEV-SNP support") > Suggested-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Pratik R. Sampat <prsampat@amd.com> > --- > v6..v7: > > * Replace FW version check with sev->snp_initialized (Sean) > --- > arch/x86/kvm/svm/sev.c | 4 +++- > drivers/crypto/ccp/sev-dev.c | 8 ++++++++ > include/linux/psp-sev.h | 3 +++ > 3 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 0dbb25442ec1..87b5d63a5817 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -3050,7 +3050,9 @@ void __init sev_hardware_setup(void) > sev_es_asid_count = min_sev_asid - 1; > WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count)); > sev_es_supported = true; > - sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP); > + sev_snp_supported = (sev_snp_enabled && > + cc_platform_has(CC_ATTR_HOST_SEV_SNP) && > + snp_initialized()); > > out: > if (boot_cpu_has(X86_FEATURE_SEV)) > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index 2e87ca0e292a..8d2cf8552bc2 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -1352,6 +1352,14 @@ int sev_platform_init(struct sev_platform_init_args *args) > } > EXPORT_SYMBOL_GPL(sev_platform_init); > > +bool snp_initialized(void) > +{ > + struct sev_device *sev = psp_master->sev_data; > + > + return sev->snp_initialized; > +} > +EXPORT_SYMBOL_GPL(snp_initialized); > + > static int __sev_platform_shutdown_locked(int *error) > { > struct psp_device *psp = psp_master; > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h > index f3cad182d4ef..d34068c87a28 100644 > --- a/include/linux/psp-sev.h > +++ b/include/linux/psp-sev.h > @@ -954,6 +954,7 @@ int sev_do_cmd(int cmd, void *data, int *psp_ret); > void *psp_copy_user_blob(u64 uaddr, u32 len); > void *snp_alloc_firmware_page(gfp_t mask); > void snp_free_firmware_page(void *addr); > +bool snp_initialized(void); > > #else /* !CONFIG_CRYPTO_DEV_SP_PSP */ > > @@ -988,6 +989,8 @@ static inline void *snp_alloc_firmware_page(gfp_t mask) > > static inline void snp_free_firmware_page(void *addr) { } > > +static inline bool snp_initialized(void) { return false; } > + > #endif /* CONFIG_CRYPTO_DEV_SP_PSP */ > > #endif /* __PSP_SEV_H__ */
Hi Tom, On 2/24/2025 3:28 PM, Tom Lendacky wrote: > On 2/21/25 15:01, Pratik R. Sampat wrote: >> During platform init, SNP initialization may fail for several reasons, >> such as firmware command failures and incompatible versions. However, >> the KVM capability may continue to advertise support for it. Export this >> information to KVM and withdraw SEV-SNP support if has not been >> successfully initialized. > > Hmmm... rather than creating a new API, can you just issue an > SNP_PLATFORM_STATUS command and see if the SNP is not in the UNINIT state? > Although reading sev->snp_initialized is probably cheaper to do, it is cleaner to query the platform status. Querying SNP_PLATFORM_STATUS requires the pages to transition to firmware-owned and back, and the helpers for it are implemented within sev-dev.c. So, similar to sev_platform_status(), I'm thinking it is probably better to create the snp_platform_status() API as well and use that within KVM to check the state. Thanks! Pratik
Hi Liam, Thanks for review! On 2/24/2025 1:01 PM, Liam Merwick wrote: > > > On 21/02/2025 21:01, Pratik R. Sampat wrote: >> During platform init, SNP initialization may fail for several reasons, >> such as firmware command failures and incompatible versions. However, >> the KVM capability may continue to advertise support for it. Export this >> information to KVM and withdraw SEV-SNP support if has not been >> successfully initialized. >> >> Fixes: 1dfe571c12cf ("KVM: SEV: Add initial SEV-SNP support") >> Suggested-by: Sean Christopherson <seanjc@google.com> >> Signed-off-by: Pratik R. Sampat <prsampat@amd.com> >> --- >> v6..v7: >> >> * Replace FW version check with sev->snp_initialized (Sean) >> --- >> arch/x86/kvm/svm/sev.c | 4 +++- >> drivers/crypto/ccp/sev-dev.c | 8 ++++++++ >> include/linux/psp-sev.h | 3 +++ >> 3 files changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >> index 0dbb25442ec1..87b5d63a5817 100644 >> --- a/arch/x86/kvm/svm/sev.c >> +++ b/arch/x86/kvm/svm/sev.c >> @@ -3050,7 +3050,9 @@ void __init sev_hardware_setup(void) >> sev_es_asid_count = min_sev_asid - 1; >> WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, >> sev_es_asid_count)); >> sev_es_supported = true; >> - sev_snp_supported = sev_snp_enabled && >> cc_platform_has(CC_ATTR_HOST_SEV_SNP); >> + sev_snp_supported = (sev_snp_enabled && >> + cc_platform_has(CC_ATTR_HOST_SEV_SNP) && >> + snp_initialized()); >> out: >> if (boot_cpu_has(X86_FEATURE_SEV)) >> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c >> index 2e87ca0e292a..8d2cf8552bc2 100644 >> --- a/drivers/crypto/ccp/sev-dev.c >> +++ b/drivers/crypto/ccp/sev-dev.c >> @@ -1352,6 +1352,14 @@ int sev_platform_init(struct >> sev_platform_init_args *args) >> } >> EXPORT_SYMBOL_GPL(sev_platform_init); >> +bool snp_initialized(void) >> +{ >> + struct sev_device *sev = psp_master->sev_data; > > > Should check psp_master isn't NULL before accessing just in case > (particularly for future potential callers). > > (e.g. see ccb88e9549e7 ("crypto: ccp - Fix null pointer dereference in > __sev_platform_shutdown_locked") > Thanks for pointing this out, if I end up using this interface, I'll put the NULL check in. Thanks! Pratik > > > >> + >> + return sev->snp_initialized; >> +} >> +EXPORT_SYMBOL_GPL(snp_initialized); >> + >> static int __sev_platform_shutdown_locked(int *error) >> { >> struct psp_device *psp = psp_master; >> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h >> index f3cad182d4ef..d34068c87a28 100644 >> --- a/include/linux/psp-sev.h >> +++ b/include/linux/psp-sev.h >> @@ -954,6 +954,7 @@ int sev_do_cmd(int cmd, void *data, int *psp_ret); >> void *psp_copy_user_blob(u64 uaddr, u32 len); >> void *snp_alloc_firmware_page(gfp_t mask); >> void snp_free_firmware_page(void *addr); >> +bool snp_initialized(void); >> #else /* !CONFIG_CRYPTO_DEV_SP_PSP */ >> @@ -988,6 +989,8 @@ static inline void >> *snp_alloc_firmware_page(gfp_t mask) >> static inline void snp_free_firmware_page(void *addr) { } >> +static inline bool snp_initialized(void) { return false; } >> + >> #endif /* CONFIG_CRYPTO_DEV_SP_PSP */ >> #endif /* __PSP_SEV_H__ */ >
On 2/25/2025 10:41 AM, Pratik R. Sampat wrote: > Hi Tom, > > On 2/24/2025 3:28 PM, Tom Lendacky wrote: >> On 2/21/25 15:01, Pratik R. Sampat wrote: >>> During platform init, SNP initialization may fail for several reasons, >>> such as firmware command failures and incompatible versions. However, >>> the KVM capability may continue to advertise support for it. Export this >>> information to KVM and withdraw SEV-SNP support if has not been >>> successfully initialized. >> >> Hmmm... rather than creating a new API, can you just issue an >> SNP_PLATFORM_STATUS command and see if the SNP is not in the UNINIT state? >> > > Although reading sev->snp_initialized is probably cheaper to do, it is > cleaner to query the platform status. > > Querying SNP_PLATFORM_STATUS requires the pages to transition to > firmware-owned and back, and the helpers for it are implemented within > sev-dev.c. So, similar to sev_platform_status(), I'm thinking it is > probably better to create the snp_platform_status() API as well and use > that within KVM to check the state. > Although I am guessing the initial intent was to not have an API exposed at all from CCP and only make the SNP_PLATFORM_STATUS call instead? Since that may not be cleanly possible (we have helpers for page state conversions such as rmp_mark_pages_firmware() in ccp) without duplicating functionality in KVM as well, I guess the question really boils down to whether we export the cheaper snp_initialized() or the snp_platform_status() API instead? Thanks again! Pratik
On 2/25/25 11:45, Pratik R. Sampat wrote: > On 2/25/2025 10:41 AM, Pratik R. Sampat wrote: >> Hi Tom, >> >> On 2/24/2025 3:28 PM, Tom Lendacky wrote: >>> On 2/21/25 15:01, Pratik R. Sampat wrote: >>>> During platform init, SNP initialization may fail for several reasons, >>>> such as firmware command failures and incompatible versions. However, >>>> the KVM capability may continue to advertise support for it. Export this >>>> information to KVM and withdraw SEV-SNP support if has not been >>>> successfully initialized. >>> >>> Hmmm... rather than creating a new API, can you just issue an >>> SNP_PLATFORM_STATUS command and see if the SNP is not in the UNINIT state? >>> >> >> Although reading sev->snp_initialized is probably cheaper to do, it is >> cleaner to query the platform status. >> >> Querying SNP_PLATFORM_STATUS requires the pages to transition to >> firmware-owned and back, and the helpers for it are implemented within >> sev-dev.c. So, similar to sev_platform_status(), I'm thinking it is >> probably better to create the snp_platform_status() API as well and use >> that within KVM to check the state. >> > > Although I am guessing the initial intent was to not have an API exposed > at all from CCP and only make the SNP_PLATFORM_STATUS call instead? > > Since that may not be cleanly possible (we have helpers for page state > conversions such as rmp_mark_pages_firmware() in ccp) without > duplicating functionality in KVM as well, I guess the question really > boils down to whether we export the cheaper snp_initialized() or the > snp_platform_status() API instead? Taking a closer look, we do already have APIs that KVM uses to allocate firmware pages (output pages for SNP APIs) that can be used: snp_alloc_firmware_page() and snp_free_firmware_page(). I think that should be enough to use sev_do_cmd() to perform the SEV_CMD_SNP_PLATFORM_STATUS command without exposing a new API. Thanks, Tom > > Thanks again! > Pratik
On 2/25/2025 1:09 PM, Tom Lendacky wrote: > On 2/25/25 11:45, Pratik R. Sampat wrote: >> On 2/25/2025 10:41 AM, Pratik R. Sampat wrote: >>> Hi Tom, >>> >>> On 2/24/2025 3:28 PM, Tom Lendacky wrote: >>>> On 2/21/25 15:01, Pratik R. Sampat wrote: >>>>> During platform init, SNP initialization may fail for several reasons, >>>>> such as firmware command failures and incompatible versions. However, >>>>> the KVM capability may continue to advertise support for it. Export this >>>>> information to KVM and withdraw SEV-SNP support if has not been >>>>> successfully initialized. >>>> >>>> Hmmm... rather than creating a new API, can you just issue an >>>> SNP_PLATFORM_STATUS command and see if the SNP is not in the UNINIT state? >>>> >>> >>> Although reading sev->snp_initialized is probably cheaper to do, it is >>> cleaner to query the platform status. >>> >>> Querying SNP_PLATFORM_STATUS requires the pages to transition to >>> firmware-owned and back, and the helpers for it are implemented within >>> sev-dev.c. So, similar to sev_platform_status(), I'm thinking it is >>> probably better to create the snp_platform_status() API as well and use >>> that within KVM to check the state. >>> >> >> Although I am guessing the initial intent was to not have an API exposed >> at all from CCP and only make the SNP_PLATFORM_STATUS call instead? >> >> Since that may not be cleanly possible (we have helpers for page state >> conversions such as rmp_mark_pages_firmware() in ccp) without >> duplicating functionality in KVM as well, I guess the question really >> boils down to whether we export the cheaper snp_initialized() or the >> snp_platform_status() API instead? > > Taking a closer look, we do already have APIs that KVM uses to allocate > firmware pages (output pages for SNP APIs) that can be used: > snp_alloc_firmware_page() and snp_free_firmware_page(). > > I think that should be enough to use sev_do_cmd() to perform the > SEV_CMD_SNP_PLATFORM_STATUS command without exposing a new API. > Ah, I had missed that! Calling the SNP_PLATFORM_STATUS this way works. Pratik > Thanks, > Tom > >> >> Thanks again! >> Pratik
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 0dbb25442ec1..87b5d63a5817 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -3050,7 +3050,9 @@ void __init sev_hardware_setup(void) sev_es_asid_count = min_sev_asid - 1; WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count)); sev_es_supported = true; - sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP); + sev_snp_supported = (sev_snp_enabled && + cc_platform_has(CC_ATTR_HOST_SEV_SNP) && + snp_initialized()); out: if (boot_cpu_has(X86_FEATURE_SEV)) diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index 2e87ca0e292a..8d2cf8552bc2 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -1352,6 +1352,14 @@ int sev_platform_init(struct sev_platform_init_args *args) } EXPORT_SYMBOL_GPL(sev_platform_init); +bool snp_initialized(void) +{ + struct sev_device *sev = psp_master->sev_data; + + return sev->snp_initialized; +} +EXPORT_SYMBOL_GPL(snp_initialized); + static int __sev_platform_shutdown_locked(int *error) { struct psp_device *psp = psp_master; diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h index f3cad182d4ef..d34068c87a28 100644 --- a/include/linux/psp-sev.h +++ b/include/linux/psp-sev.h @@ -954,6 +954,7 @@ int sev_do_cmd(int cmd, void *data, int *psp_ret); void *psp_copy_user_blob(u64 uaddr, u32 len); void *snp_alloc_firmware_page(gfp_t mask); void snp_free_firmware_page(void *addr); +bool snp_initialized(void); #else /* !CONFIG_CRYPTO_DEV_SP_PSP */ @@ -988,6 +989,8 @@ static inline void *snp_alloc_firmware_page(gfp_t mask) static inline void snp_free_firmware_page(void *addr) { } +static inline bool snp_initialized(void) { return false; } + #endif /* CONFIG_CRYPTO_DEV_SP_PSP */ #endif /* __PSP_SEV_H__ */
During platform init, SNP initialization may fail for several reasons, such as firmware command failures and incompatible versions. However, the KVM capability may continue to advertise support for it. Export this information to KVM and withdraw SEV-SNP support if has not been successfully initialized. Fixes: 1dfe571c12cf ("KVM: SEV: Add initial SEV-SNP support") Suggested-by: Sean Christopherson <seanjc@google.com> Signed-off-by: Pratik R. Sampat <prsampat@amd.com> --- v6..v7: * Replace FW version check with sev->snp_initialized (Sean) --- arch/x86/kvm/svm/sev.c | 4 +++- drivers/crypto/ccp/sev-dev.c | 8 ++++++++ include/linux/psp-sev.h | 3 +++ 3 files changed, 14 insertions(+), 1 deletion(-)