| Message ID | cover.1739997129.git.ashish.kalra@amd.com |
|---|---|
| Headers | show |
| Series | Move initializing SEV/SNP functionality to KVM | expand |
On Wed, Feb 19, 2025 at 12:53 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote: > > From: Ashish Kalra <ashish.kalra@amd.com> > > When SEV-SNP is enabled the TMR needs to be 2MB aligned and 2MB sized, > ensure that TMR size is reset back to default when SNP is shutdown as > SNP initialization and shutdown as part of some SNP ioctls may leave > TMR size modified and cause subsequent SEV only initialization to fail. > > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> Acked-by: Dionna Glaze <dionnaglaze@google.com> > --- > drivers/crypto/ccp/sev-dev.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index b06f43eb18f7..be8a84ce24c7 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -1751,6 +1751,9 @@ static int __sev_snp_shutdown_locked(int *error, bool panic) > sev->snp_initialized = false; > dev_dbg(sev->dev, "SEV-SNP firmware shutdown\n"); > > + /* Reset TMR size back to default */ > + sev_es_tmr_size = SEV_TMR_SIZE; > + > return ret; > } > > -- > 2.34.1 >
On 2/19/25 14:52, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > > Move dev_info and dev_err messages related to SEV/SNP initialization > and shutdown into __sev_platform_init_locked(), __sev_snp_init_locked() > and __sev_platform_shutdown_locked(), __sev_snp_shutdown_locked() so > that they don't need to be issued from callers. > > This allows both _sev_platform_init_locked() and various SEV/SNP ioctls > to call __sev_platform_init_locked(), __sev_snp_init_locked() and > __sev_platform_shutdown_locked(), __sev_snp_shutdown_locked() for > implicit SEV/SNP initialization and shutdown without additionally > printing any errors/success messages. > > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > --- > drivers/crypto/ccp/sev-dev.c | 39 +++++++++++++++++++++++++++--------- > 1 file changed, 30 insertions(+), 9 deletions(-) > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index 2e87ca0e292a..8f5c474b9d1c 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -1176,21 +1176,30 @@ static int __sev_snp_init_locked(int *error) > wbinvd_on_all_cpus(); > > rc = __sev_do_cmd_locked(cmd, arg, error); > - if (rc) > + if (rc) { > + dev_err(sev->dev, "SEV-SNP: failed to INIT rc %d, error %#x\n", > + rc, *error); How about doing: dev_err(sev->dev, "SEV-SNP: %s failed rc %d, error %#x\n", cmd == SEV_CMD_SNP_INIT_EX ? "SNP_INIT_EX" : "SNP_INIT", rc, *error); > return rc; > + } > > /* Prepare for first SNP guest launch after INIT. */ > wbinvd_on_all_cpus(); > rc = __sev_do_cmd_locked(SEV_CMD_SNP_DF_FLUSH, NULL, error); > - if (rc) > + if (rc) { > + dev_err(sev->dev, "SEV-SNP: SNP_DF_FLUSH failed rc %d, error %#x\n", > + rc, *error); > return rc; > + } > > sev->snp_initialized = true; > dev_dbg(sev->dev, "SEV-SNP firmware initialized\n"); > > + dev_info(sev->dev, "SEV-SNP API:%d.%d build:%d\n", sev->api_major, > + sev->api_minor, sev->build); > + > sev_es_tmr_size = SNP_TMR_SIZE; > > - return rc; > + return 0; > } > > static void __sev_platform_init_handle_tmr(struct sev_device *sev) > @@ -1267,8 +1276,10 @@ static int __sev_platform_init_locked(int *error) > __sev_platform_init_handle_tmr(sev); > > rc = __sev_platform_init_handle_init_ex_path(sev); > - if (rc) > + if (rc) { > + dev_err(sev->dev, "SEV: handle_init_ex_path failed, rc %d\n", rc); > return rc; > + } Messages should be issued in __sev_platform_init_handle_init_ex_path(). The only non-zero rc value that doesn't cause a message would come from sev_read_init_ex_file() when sev_init_ex_buffer is NULL, but sev_read_init_ex_file() isn't called if the allocation for that buffer fails. So I don't think this message is necessary. But double-check me on that. > > rc = __sev_do_init_locked(&psp_ret); > if (rc && psp_ret == SEV_RET_SECURE_DATA_INVALID) { > @@ -1287,16 +1298,22 @@ static int __sev_platform_init_locked(int *error) > if (error) > *error = psp_ret; > > - if (rc) > + if (rc) { > + dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n", > + psp_ret, rc); Similar to the SNP INIT comment above, how about: dev_err(sev->dev, "SEV: %s failed %#x, rc %d\n", sev_init_ex_buffer ? "INIT_EX" : "INIT", psp_ret, rc); > return rc; > + } > > sev->state = SEV_STATE_INIT; > > /* Prepare for first SEV guest launch after INIT */ > wbinvd_on_all_cpus(); > rc = __sev_do_cmd_locked(SEV_CMD_DF_FLUSH, NULL, error); > - if (rc) > + if (rc) { > + dev_err(sev->dev, "SEV: DF_FLUSH failed %#x, rc %d\n", > + *error, rc); > return rc; > + } > > dev_dbg(sev->dev, "SEV firmware initialized\n"); > > @@ -1367,8 +1384,11 @@ static int __sev_platform_shutdown_locked(int *error) > return 0; > > ret = __sev_do_cmd_locked(SEV_CMD_SHUTDOWN, NULL, error); > - if (ret) > + if (ret) { > + dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n", > + *error, ret); > return ret; > + } > > sev->state = SEV_STATE_UNINIT; > dev_dbg(sev->dev, "SEV firmware shutdown\n"); > @@ -1684,7 +1704,7 @@ static int __sev_snp_shutdown_locked(int *error, bool panic) > if (*error == SEV_RET_DFFLUSH_REQUIRED) { > ret = __sev_do_cmd_locked(SEV_CMD_SNP_DF_FLUSH, NULL, NULL); > if (ret) { > - dev_err(sev->dev, "SEV-SNP DF_FLUSH failed\n"); > + dev_err(sev->dev, "SEV-SNP DF_FLUSH failed, ret = %d\n", ret); Should provide as much info as possible, so create a local int variable that you can pass into __sev_do_cmd_locked() and output that in the failure message. (I should go through this file later and make all the message formats consistent.) Thanks, Tom > return ret; > } > /* reissue the shutdown command */ > @@ -1692,7 +1712,8 @@ static int __sev_snp_shutdown_locked(int *error, bool panic) > error); > } > if (ret) { > - dev_err(sev->dev, "SEV-SNP firmware shutdown failed\n"); > + dev_err(sev->dev, "SEV-SNP firmware shutdown failed, rc %d, error %#x\n", > + ret, *error); > return ret; > } >
On 2/19/25 14:54, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > > Add new API interface to do SEV/SNP platform shutdown when KVM module > is unloaded. Just a nit below if you have to respin. Otherwise: Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> > > Reviewed-by: Dionna Glaze <dionnaglaze@google.com> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > --- > drivers/crypto/ccp/sev-dev.c | 13 +++++++++++++ > include/linux/psp-sev.h | 3 +++ > 2 files changed, 16 insertions(+) > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index 582304638319..f0f3e6d29200 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -2445,6 +2445,19 @@ static void sev_firmware_shutdown(struct sev_device *sev) > mutex_unlock(&sev_cmd_mutex); > } > > +void sev_platform_shutdown(void) > +{ > + struct sev_device *sev; > + > + if (!psp_master || !psp_master->sev_data) > + return; > + > + sev = psp_master->sev_data; > + > + sev_firmware_shutdown(sev); sev_firmware_shutdown(psp->master->sev_data); and then you can get rid of the sev variable. Thanks, Tom > +} > +EXPORT_SYMBOL_GPL(sev_platform_shutdown); > + > void sev_dev_destroy(struct psp_device *psp) > { > struct sev_device *sev = psp->sev_data; > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h > index f3cad182d4ef..0b3a36bdaa90 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); > +void sev_platform_shutdown(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 void sev_platform_shutdown(void) { } > + > #endif /* CONFIG_CRYPTO_DEV_SP_PSP */ > > #endif /* __PSP_SEV_H__ */
On 2/19/25 14:55, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > > SNP initialization is forced during PSP driver probe purely because SNP > can't be initialized if VMs are running. But the only in-tree user of > SEV/SNP functionality is KVM, and KVM depends on PSP driver for the same. > Forcing SEV/SNP initialization because a hypervisor could be running > legacy non-confidential VMs make no sense. > > This patch removes SEV/SNP initialization from the PSP driver probe > time and moves the requirement to initialize SEV/SNP functionality > to KVM if it wants to use SEV/SNP. > > Suggested-by: Sean Christopherson <seanjc@google.com> > Reviewed-by: Alexey Kardashevskiy <aik@amd.com> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > --- > drivers/crypto/ccp/sev-dev.c | 25 +------------------------ > 1 file changed, 1 insertion(+), 24 deletions(-) > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index f0f3e6d29200..99a663dbc2b6 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -1346,18 +1346,13 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args) > if (sev->state == SEV_STATE_INIT) > return 0; > > - /* > - * Legacy guests cannot be running while SNP_INIT(_EX) is executing, > - * so perform SEV-SNP initialization at probe time. > - */ > rc = __sev_snp_init_locked(&args->error); > if (rc && rc != -ENODEV) { > /* > * Don't abort the probe if SNP INIT failed, > * continue to initialize the legacy SEV firmware. > */ > - dev_err(sev->dev, "SEV-SNP: failed to INIT rc %d, error %#x\n", > - rc, args->error); > + dev_err(sev->dev, "SEV-SNP: failed to INIT, continue SEV INIT\n"); Please don't remove the error information. > } > > /* Defer legacy SEV/SEV-ES support if allowed by caller/module. */ > @@ -2505,9 +2500,7 @@ EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user); > void sev_pci_init(void) > { > struct sev_device *sev = psp_master->sev_data; > - struct sev_platform_init_args args = {0}; > u8 api_major, api_minor, build; > - int rc; > > if (!sev) > return; > @@ -2530,16 +2523,6 @@ void sev_pci_init(void) > api_major, api_minor, build, > sev->api_major, sev->api_minor, sev->build); > > - /* Initialize the platform */ > - args.probe = true; > - rc = sev_platform_init(&args); > - if (rc) > - dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n", > - args.error, rc); > - > - dev_info(sev->dev, "SEV%s API:%d.%d build:%d\n", sev->snp_initialized ? > - "-SNP" : "", sev->api_major, sev->api_minor, sev->build); > - > return; > > err: > @@ -2550,10 +2533,4 @@ void sev_pci_init(void) > > void sev_pci_exit(void) > { > - struct sev_device *sev = psp_master->sev_data; > - > - if (!sev) > - return; > - > - sev_firmware_shutdown(sev); Should this remain? If there's a bug in KVM that somehow skips the shutdown call, then SEV will remain initialized. I think the path is safe to call a second time. Thanks, Tom > }
Hello Tom, On 2/20/2025 2:03 PM, Tom Lendacky wrote: > On 2/19/25 14:55, Ashish Kalra wrote: >> From: Ashish Kalra <ashish.kalra@amd.com> >> >> SNP initialization is forced during PSP driver probe purely because SNP >> can't be initialized if VMs are running. But the only in-tree user of >> SEV/SNP functionality is KVM, and KVM depends on PSP driver for the same. >> Forcing SEV/SNP initialization because a hypervisor could be running >> legacy non-confidential VMs make no sense. >> >> This patch removes SEV/SNP initialization from the PSP driver probe >> time and moves the requirement to initialize SEV/SNP functionality >> to KVM if it wants to use SEV/SNP. >> >> Suggested-by: Sean Christopherson <seanjc@google.com> >> Reviewed-by: Alexey Kardashevskiy <aik@amd.com> >> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> >> --- >> drivers/crypto/ccp/sev-dev.c | 25 +------------------------ >> 1 file changed, 1 insertion(+), 24 deletions(-) >> >> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c >> index f0f3e6d29200..99a663dbc2b6 100644 >> --- a/drivers/crypto/ccp/sev-dev.c >> +++ b/drivers/crypto/ccp/sev-dev.c >> @@ -1346,18 +1346,13 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args) >> if (sev->state == SEV_STATE_INIT) >> return 0; >> >> - /* >> - * Legacy guests cannot be running while SNP_INIT(_EX) is executing, >> - * so perform SEV-SNP initialization at probe time. >> - */ >> rc = __sev_snp_init_locked(&args->error); >> if (rc && rc != -ENODEV) { >> /* >> * Don't abort the probe if SNP INIT failed, >> * continue to initialize the legacy SEV firmware. >> */ >> - dev_err(sev->dev, "SEV-SNP: failed to INIT rc %d, error %#x\n", >> - rc, args->error); >> + dev_err(sev->dev, "SEV-SNP: failed to INIT, continue SEV INIT\n"); > > Please don't remove the error information. > The error(s) are already being printed in __sev_snp_init_locked() otherwise the same error will be printed twice, hence removing it here. >> } >> >> /* Defer legacy SEV/SEV-ES support if allowed by caller/module. */ >> @@ -2505,9 +2500,7 @@ EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user); >> void sev_pci_init(void) >> { >> struct sev_device *sev = psp_master->sev_data; >> - struct sev_platform_init_args args = {0}; >> u8 api_major, api_minor, build; >> - int rc; >> >> if (!sev) >> return; >> @@ -2530,16 +2523,6 @@ void sev_pci_init(void) >> api_major, api_minor, build, >> sev->api_major, sev->api_minor, sev->build); >> >> - /* Initialize the platform */ >> - args.probe = true; >> - rc = sev_platform_init(&args); >> - if (rc) >> - dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n", >> - args.error, rc); >> - >> - dev_info(sev->dev, "SEV%s API:%d.%d build:%d\n", sev->snp_initialized ? >> - "-SNP" : "", sev->api_major, sev->api_minor, sev->build); >> - >> return; >> >> err: >> @@ -2550,10 +2533,4 @@ void sev_pci_init(void) >> >> void sev_pci_exit(void) >> { >> - struct sev_device *sev = psp_master->sev_data; >> - >> - if (!sev) >> - return; >> - >> - sev_firmware_shutdown(sev); > > Should this remain? If there's a bug in KVM that somehow skips the > shutdown call, then SEV will remain initialized. I think the path is > safe to call a second time. Ok. Thanks, Ashish
From: Ashish Kalra <ashish.kalra@amd.com> Remove initializing SEV/SNP functionality from PSP driver and instead add support to KVM to explicitly initialize the PSP if KVM wants to use SEV/SNP functionality. This removes SEV/SNP initialization at PSP module probe time and does on-demand SEV/SNP initialization when KVM really wants to use SEV/SNP functionality. This will allow running legacy non-confidential VMs without initializating SEV functionality. This will assist in adding SNP CipherTextHiding support and SEV firmware hotloading support in KVM without sharing SEV ASID management and SNP guest context support between PSP driver and KVM and keeping all that support only in KVM. To support SEV firmware hotloading, SEV Shutdown will be done explicitly prior to DOWNLOAD_FIRMWARE_EX and SEV INIT post it to work with the requirement of SEV to be in UNINIT state for DOWNLOAD_FIRMWARE_EX. NOTE: SEV firmware hotloading will only be supported if there are no active SEV/SEV-ES guests. v4: - Rebase on linux-next which has the fix for SNP broken with kvm_amd module built-in. - Fix commit logs. - Add explicit SEV/SNP initialization and shutdown error logs instead of using a common exit point. - Move SEV/SNP shutdown error logs from callers into __sev_platform_shutdown_locked() and __sev_snp_shutdown_locked(). - Make sure that we continue to support both the probe field and psp_init_on_probe module parameter for PSP module to support SEV INIT_EX. - Add reviewed-by's. v3: - Move back to do both SNP and SEV platform initialization at KVM module load time instead of SEV initialization on demand at SEV/SEV-ES VM launch to prevent breaking QEMU which has a check for SEV to be initialized prior to launching SEV/SEV-ES VMs. - As both SNP and SEV platform initialization and shutdown is now done at KVM module load and unload time remove patches for separate SEV and SNP platform initialization and shutdown. v2: - Added support for separate SEV and SNP platform initalization, while SNP platform initialization is done at KVM module load time, SEV platform initialization is done on demand at SEV/SEV-ES VM launch. - Added support for separate SEV and SNP platform shutdown, both SEV and SNP shutdown done at KVM module unload time, only SEV shutdown down when all SEV/SEV-ES VMs have been destroyed, this allows SEV firmware hotloading support anytime during system lifetime. - Updated commit messages for couple of patches in the series with reference to the feedback received on v1 patches. Ashish Kalra (7): crypto: ccp: Move dev_info/err messages for SEV/SNP init and shutdown crypto: ccp: Ensure implicit SEV/SNP init and shutdown in ioctls crypto: ccp: Reset TMR size at SNP Shutdown crypto: ccp: Register SNP panic notifier only if SNP is enabled crypto: ccp: Add new SEV/SNP platform shutdown API KVM: SVM: Add support to initialize SEV/SNP functionality in KVM crypto: ccp: Move SEV/SNP Platform initialization to KVM arch/x86/kvm/svm/sev.c | 15 +++ drivers/crypto/ccp/sev-dev.c | 219 ++++++++++++++++++++++++----------- include/linux/psp-sev.h | 3 + 3 files changed, 171 insertions(+), 66 deletions(-)