| Message ID | 20211102142331.3753798-2-pgonda@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Add SEV_INIT_EX support | expand |
On Tue, Nov 02, 2021, Peter Gonda wrote: > Currently only the firmware error code is printed. This is incomplete > and also incorrect as error cases exists where the firmware is never > called and therefore does not set an error code. This change zeros the > firmware error code in case the call does not get that far and prints > the return code for non firmware errors. > > Signed-off-by: Peter Gonda <pgonda@google.com> > Reviewed-by: Marc Orr <marcorr@google.com> > Acked-by: David Rientjes <rientjes@google.com> > Acked-by: Tom Lendacky <thomas.lendacky@amd.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Cc: Brijesh Singh <brijesh.singh@amd.com> > Cc: Marc Orr <marcorr@google.com> > Cc: Joerg Roedel <jroedel@suse.de> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: David Rientjes <rientjes@google.com> > Cc: John Allen <john.allen@amd.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: linux-crypto@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/crypto/ccp/sev-dev.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index 2ecb0e1f65d8..ec89a82ba267 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -1065,7 +1065,7 @@ void sev_pci_init(void) > { > struct sev_device *sev = psp_master->sev_data; > struct page *tmr_page; > - int error, rc; > + int error = 0, rc; Wouldn't it be more appropriate to use something the PSP can't return, e.g. -1? '0' is SEV_RET_SUCCESS, which is quite misleading, e.g. the below error message will print SEV: failed to INIT error 0, rc -16 which a bit head scratching without looking at the code. AFAICT, the PSP return codes aren't intrinsically hex, so printing error as a signed demical and thus SEV: failed to INIT error -1, rc -16 would be less confusing. And IMO requiring the caller to initialize error is will be neverending game of whack-a-mole. E.g. sev_ioctl() fails to set "error" in the userspace structure, and literally every function exposed via include/linux/psp-sev.h has this same issue. Case in point, the retry path fails to re-initialize "error" and will display stale information if the second sev_platform_init() fails without reaching the PSP. rc = sev_platform_init(&error); if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) { /* * INIT command returned an integrity check failure * status code, meaning that firmware load and * validation of SEV related persistent data has * failed and persistent state has been erased. * Retrying INIT command here should succeed. */ dev_dbg(sev->dev, "SEV: retrying INIT command"); rc = sev_platform_init(&error); <------ error may or may not be set } Ideally, error wouldn't be an output param and instead would be squished into the true return value, but that'd required quite the refactoring, and might yield ugly code generation on non-64-bit architectures (does this code support those?). As a minimal step toward sanity, sev_ioctl(), __sev_platform_init_locked(), and __sev_do_cmd_locked() should initialize the incoming error. Long term, sev-dev really needs to either have well-defined API for when "error" is meaningful, or ensure the pointer is initialized in all paths. E.g. diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index ec89a82ba267..549686a1e812 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -149,6 +149,9 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) unsigned int reg, ret = 0; int buf_len; + if (psp_ret) + *psp_ret = -1; + if (!psp || !psp->sev_data) return -ENODEV; @@ -192,9 +195,6 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) /* wait for command completion */ ret = sev_wait_cmd_ioc(sev, ®, psp_timeout); if (ret) { - if (psp_ret) - *psp_ret = 0; - dev_err(sev->dev, "sev command %#x timed out, disabling PSP\n", cmd); psp_dead = true; @@ -243,6 +243,9 @@ static int __sev_platform_init_locked(int *error) struct sev_device *sev; int rc = 0; + if (error) + *error = -1; + if (!psp || !psp->sev_data) return -ENODEV; @@ -838,6 +841,8 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) if (input.cmd > SEV_MAX) return -EINVAL; + input.error = -1; + mutex_lock(&sev_cmd_mutex); switch (input.cmd) { > if (!sev) > return; > @@ -1104,7 +1104,8 @@ void sev_pci_init(void) > } > > if (rc) { > - dev_err(sev->dev, "SEV: failed to INIT error %#x\n", error); > + dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n", > + error, rc); > return; > } > > -- > 2.33.1.1089.g2158813163f-goog >
On Tue, Nov 9, 2021 at 9:27 AM Sean Christopherson <seanjc@google.com> wrote: > > On Tue, Nov 02, 2021, Peter Gonda wrote: > > Currently only the firmware error code is printed. This is incomplete > > and also incorrect as error cases exists where the firmware is never > > called and therefore does not set an error code. This change zeros the > > firmware error code in case the call does not get that far and prints > > the return code for non firmware errors. > > > > Signed-off-by: Peter Gonda <pgonda@google.com> > > Reviewed-by: Marc Orr <marcorr@google.com> > > Acked-by: David Rientjes <rientjes@google.com> > > Acked-by: Tom Lendacky <thomas.lendacky@amd.com> > > Cc: Tom Lendacky <thomas.lendacky@amd.com> > > Cc: Brijesh Singh <brijesh.singh@amd.com> > > Cc: Marc Orr <marcorr@google.com> > > Cc: Joerg Roedel <jroedel@suse.de> > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > Cc: David Rientjes <rientjes@google.com> > > Cc: John Allen <john.allen@amd.com> > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: linux-crypto@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > --- > > drivers/crypto/ccp/sev-dev.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > > index 2ecb0e1f65d8..ec89a82ba267 100644 > > --- a/drivers/crypto/ccp/sev-dev.c > > +++ b/drivers/crypto/ccp/sev-dev.c > > @@ -1065,7 +1065,7 @@ void sev_pci_init(void) > > { > > struct sev_device *sev = psp_master->sev_data; > > struct page *tmr_page; > > - int error, rc; > > + int error = 0, rc; > > Wouldn't it be more appropriate to use something the PSP can't return, e.g. -1? > '0' is SEV_RET_SUCCESS, which is quite misleading, e.g. the below error message > will print > > SEV: failed to INIT error 0, rc -16 > > which a bit head scratching without looking at the code. AFAICT, the PSP return > codes aren't intrinsically hex, so printing error as a signed demical and thus > > SEV: failed to INIT error -1, rc -16 > > would be less confusing. > > And IMO requiring the caller to initialize error is will be neverending game of > whack-a-mole. E.g. sev_ioctl() fails to set "error" in the userspace structure, > and literally every function exposed via include/linux/psp-sev.h has this same > issue. Case in point, the retry path fails to re-initialize "error" and will > display stale information if the second sev_platform_init() fails without reaching > the PSP. OK I can update __sev_platform_init_locked() to set error to -1. That seems pretty reasonable. Tom, is that OK with you? > > rc = sev_platform_init(&error); > if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) { > /* > * INIT command returned an integrity check failure > * status code, meaning that firmware load and > * validation of SEV related persistent data has > * failed and persistent state has been erased. > * Retrying INIT command here should succeed. > */ > dev_dbg(sev->dev, "SEV: retrying INIT command"); > rc = sev_platform_init(&error); <------ error may or may not be set > } > > Ideally, error wouldn't be an output param and instead would be squished into the > true return value, but that'd required quite the refactoring, and might yield ugly > code generation on non-64-bit architectures (does this code support those?). > > As a minimal step toward sanity, sev_ioctl(), __sev_platform_init_locked(), and > __sev_do_cmd_locked() should initialize the incoming error. Long term, sev-dev > really needs to either have well-defined API for when "error" is meaningful, or > ensure the pointer is initialized in all paths. These comments seem fine to me. But I'll refrain from updating anything here since this seems out-of-scope of this series. Happy to discuss further and work on that if Tom is interested in those refactors too. > > E.g. > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index ec89a82ba267..549686a1e812 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -149,6 +149,9 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) > unsigned int reg, ret = 0; > int buf_len; > > + if (psp_ret) > + *psp_ret = -1; > + > if (!psp || !psp->sev_data) > return -ENODEV; > > @@ -192,9 +195,6 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) > /* wait for command completion */ > ret = sev_wait_cmd_ioc(sev, ®, psp_timeout); > if (ret) { > - if (psp_ret) > - *psp_ret = 0; > - > dev_err(sev->dev, "sev command %#x timed out, disabling PSP\n", cmd); > psp_dead = true; > > @@ -243,6 +243,9 @@ static int __sev_platform_init_locked(int *error) > struct sev_device *sev; > int rc = 0; > > + if (error) > + *error = -1; > + > if (!psp || !psp->sev_data) > return -ENODEV; > > @@ -838,6 +841,8 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) > if (input.cmd > SEV_MAX) > return -EINVAL; > > + input.error = -1; > + > mutex_lock(&sev_cmd_mutex); > > switch (input.cmd) { > > > if (!sev) > > return; > > @@ -1104,7 +1104,8 @@ void sev_pci_init(void) > > } > > > > if (rc) { > > - dev_err(sev->dev, "SEV: failed to INIT error %#x\n", error); > > + dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n", > > + error, rc); > > return; > > } > > > > -- > > 2.33.1.1089.g2158813163f-goog > >
On 11/9/21 10:46 AM, Peter Gonda wrote: > On Tue, Nov 9, 2021 at 9:27 AM Sean Christopherson <seanjc@google.com> wrote: >> On Tue, Nov 02, 2021, Peter Gonda wrote: ... >> >> SEV: failed to INIT error 0, rc -16 >> >> which a bit head scratching without looking at the code. AFAICT, the PSP return >> codes aren't intrinsically hex, so printing error as a signed demical and thus >> >> SEV: failed to INIT error -1, rc -16 >> >> would be less confusing. >> >> And IMO requiring the caller to initialize error is will be neverending game of >> whack-a-mole. E.g. sev_ioctl() fails to set "error" in the userspace structure, >> and literally every function exposed via include/linux/psp-sev.h has this same >> issue. Case in point, the retry path fails to re-initialize "error" and will >> display stale information if the second sev_platform_init() fails without reaching >> the PSP. > > OK I can update __sev_platform_init_locked() to set error to -1. That > seems pretty reasonable. Tom, is that OK with you? Yup, I'm ok with using -1. > ... > > These comments seem fine to me. But I'll refrain from updating > anything here since this seems out-of-scope of this series. Happy to > discuss further and work on that if Tom is interested in those > refactors too. That's one of those things we've wanted to get around to improving but just haven't had the time. So, yes, if you wish to refactor the 'error' related area, that would be great. Thanks, Tom >
On Tue, Nov 9, 2021 at 12:25 PM Tom Lendacky <thomas.lendacky@amd.com> wrote: > > On 11/9/21 10:46 AM, Peter Gonda wrote: > > On Tue, Nov 9, 2021 at 9:27 AM Sean Christopherson <seanjc@google.com> wrote: > >> On Tue, Nov 02, 2021, Peter Gonda wrote: > > ... > > >> > >> SEV: failed to INIT error 0, rc -16 > >> > >> which a bit head scratching without looking at the code. AFAICT, the PSP return > >> codes aren't intrinsically hex, so printing error as a signed demical and thus > >> > >> SEV: failed to INIT error -1, rc -16 > >> > >> would be less confusing. > >> > >> And IMO requiring the caller to initialize error is will be neverending game of > >> whack-a-mole. E.g. sev_ioctl() fails to set "error" in the userspace structure, > >> and literally every function exposed via include/linux/psp-sev.h has this same > >> issue. Case in point, the retry path fails to re-initialize "error" and will > >> display stale information if the second sev_platform_init() fails without reaching > >> the PSP. > > > > OK I can update __sev_platform_init_locked() to set error to -1. That > > seems pretty reasonable. Tom, is that OK with you? > > Yup, I'm ok with using -1. > > > > > ... > > > > > These comments seem fine to me. But I'll refrain from updating > > anything here since this seems out-of-scope of this series. Happy to > > discuss further and work on that if Tom is interested in those > > refactors too. > > That's one of those things we've wanted to get around to improving but > just haven't had the time. So, yes, if you wish to refactor the 'error' > related area, that would be great. OK so when I actually sat down to work on this. I realized this is bigger than I thought. If we want to have error == -1 for all return from psp-sev.h functions where the PSP isn't called yet there are a lot of changes. For example if CONFIG_CRYPTO_DEV_SP_PSP is not defined all these stubs need to be to set `*errror == -`, basically all these functions need to be updated. So to keep this series more targeted. I think I'll drop the error here. And just have this patch print the rc value. If what I said above seems reasonable I'll do those error refactors. Are people envisioning something else for the error fixups? > > Thanks, > Tom > > >
On 11/10/21 11:29 AM, Peter Gonda wrote: > On Tue, Nov 9, 2021 at 12:25 PM Tom Lendacky <thomas.lendacky@amd.com> wrote: >> On 11/9/21 10:46 AM, Peter Gonda wrote: >>> On Tue, Nov 9, 2021 at 9:27 AM Sean Christopherson <seanjc@google.com> wrote: >>>> On Tue, Nov 02, 2021, Peter Gonda wrote: >> ... >> >> That's one of those things we've wanted to get around to improving but >> just haven't had the time. So, yes, if you wish to refactor the 'error' >> related area, that would be great. > > OK so when I actually sat down to work on this. I realized this is > bigger than I thought. If we want to have error == -1 for all return > from psp-sev.h functions where the PSP isn't called yet there are a > lot of changes. For example if CONFIG_CRYPTO_DEV_SP_PSP is not defined > all these stubs need to be to set `*errror == -`, basically all these > functions need to be updated. > > So to keep this series more targeted. I think I'll drop the error > here. And just have this patch print the rc value. If what I said In that case, I think you should keep the error value and initialize it to 0. That is consistent with the other paths. Then, if you take on the fixups, it can be changed then. > above seems reasonable I'll do those error refactors. Are people > envisioning something else for the error fixups? The main refactoring I wanted was to make sure the caller didn't have to initialize the error variable. Whether to initialize it to 0 or -1 wasn't part of my original thoughts. But I do like the -1 value because, theoretically, we shouldn't get such a value back from the PSP. So if the value printed is not -1, that is an indication that the PSP API was called no matter the value of rc. Thanks, Tom > >> >> Thanks, >> Tom >> >>>
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index 2ecb0e1f65d8..ec89a82ba267 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -1065,7 +1065,7 @@ void sev_pci_init(void) { struct sev_device *sev = psp_master->sev_data; struct page *tmr_page; - int error, rc; + int error = 0, rc; if (!sev) return; @@ -1104,7 +1104,8 @@ void sev_pci_init(void) } if (rc) { - dev_err(sev->dev, "SEV: failed to INIT error %#x\n", error); + dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n", + error, rc); return; }