| Message ID | 5fe0faa1070d5225c19e3df207825d0e337ee3b9.1739997129.git.ashish.kalra@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Move initializing SEV/SNP functionality to KVM | expand |
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; > } >
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); 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; + } 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); 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); 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; }