| Message ID | 9d8cae623934489b46dc5abdf65a3034800351d9.1741300901.git.ashish.kalra@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Move initializing SEV/SNP functionality to KVM | expand |
On 3/6/25 17:09, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > > If SNP host support (SYSCFG.SNPEn) is set, then RMP table must be s/RMP/the RMP/ > initialized up before calling SEV INIT. s/up// > > In other words, if SNP_INIT(_EX) is not issued or fails then > SEV INIT will fail once SNP host support (SYSCFG.SNPEn) is enabled. s/once/if/ > > Fixes: 1ca5614b84eed ("crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP") > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > --- > drivers/crypto/ccp/sev-dev.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index 2e87ca0e292a..a0e3de94704e 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -1112,7 +1112,7 @@ static int __sev_snp_init_locked(int *error) > 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 0; > + return -EOPNOTSUPP; > } > > /* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */ > @@ -1325,12 +1325,9 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args) > */ > rc = __sev_snp_init_locked(&args->error); > if (rc && rc != -ENODEV) { Can we get ride of this extra -ENODEV check? It can only be returned because of the same check that is made earlier in this function so it doesn't really serve any purpose from what I can tell. Just make this "if (rc) {" Thanks, Tom > - /* > - * 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); > + return rc; > } > > /* Defer legacy SEV/SEV-ES support if allowed by caller/module. */
On 3/7/25 14:54, Tom Lendacky wrote: > On 3/6/25 17:09, Ashish Kalra wrote: >> From: Ashish Kalra <ashish.kalra@amd.com> >> >> If SNP host support (SYSCFG.SNPEn) is set, then RMP table must be > > s/RMP/the RMP/ > >> initialized up before calling SEV INIT. > > s/up// > >> >> In other words, if SNP_INIT(_EX) is not issued or fails then >> SEV INIT will fail once SNP host support (SYSCFG.SNPEn) is enabled. > > s/once/if/ > >> >> Fixes: 1ca5614b84eed ("crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP") >> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> >> --- >> drivers/crypto/ccp/sev-dev.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c >> index 2e87ca0e292a..a0e3de94704e 100644 >> --- a/drivers/crypto/ccp/sev-dev.c >> +++ b/drivers/crypto/ccp/sev-dev.c >> @@ -1112,7 +1112,7 @@ static int __sev_snp_init_locked(int *error) >> 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 0; >> + return -EOPNOTSUPP; >> } >> >> /* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */ >> @@ -1325,12 +1325,9 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args) >> */ >> rc = __sev_snp_init_locked(&args->error); >> if (rc && rc != -ENODEV) { > > Can we get ride of this extra -ENODEV check? It can only be returned > because of the same check that is made earlier in this function so it > doesn't really serve any purpose from what I can tell. > > Just make this "if (rc) {" My bad... -ENODEV is returned if cc_platform_has(CC_ATTR_HOST_SEV_SNP) is false, never mind. Thanks, Tom > > Thanks, > Tom > >> - /* >> - * 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); >> + return rc; >> } >> >> /* Defer legacy SEV/SEV-ES support if allowed by caller/module. */
On 3/7/2025 2:57 PM, Tom Lendacky wrote: > On 3/7/25 14:54, Tom Lendacky wrote: >> On 3/6/25 17:09, Ashish Kalra wrote: >>> From: Ashish Kalra <ashish.kalra@amd.com> >>> >>> If SNP host support (SYSCFG.SNPEn) is set, then RMP table must be >> >> s/RMP/the RMP/ >> >>> initialized up before calling SEV INIT. >> >> s/up// >> >>> >>> In other words, if SNP_INIT(_EX) is not issued or fails then >>> SEV INIT will fail once SNP host support (SYSCFG.SNPEn) is enabled. >> >> s/once/if/ >> >>> >>> Fixes: 1ca5614b84eed ("crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP") >>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> >>> --- >>> drivers/crypto/ccp/sev-dev.c | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c >>> index 2e87ca0e292a..a0e3de94704e 100644 >>> --- a/drivers/crypto/ccp/sev-dev.c >>> +++ b/drivers/crypto/ccp/sev-dev.c >>> @@ -1112,7 +1112,7 @@ static int __sev_snp_init_locked(int *error) >>> 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 0; >>> + return -EOPNOTSUPP; >>> } >>> >>> /* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */ >>> @@ -1325,12 +1325,9 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args) >>> */ >>> rc = __sev_snp_init_locked(&args->error); >>> if (rc && rc != -ENODEV) { >> >> Can we get ride of this extra -ENODEV check? It can only be returned >> because of the same check that is made earlier in this function so it >> doesn't really serve any purpose from what I can tell. >> >> Just make this "if (rc) {" > > My bad... -ENODEV is returned if cc_platform_has(CC_ATTR_HOST_SEV_SNP) is > false, never mind. Yes, that's what i was going to reply with ... we want to continue with SEV INIT if SNP host support is not enabled. Thanks, Ashish > > Thanks, > Tom > >> >> Thanks, >> Tom >> >>> - /* >>> - * 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); >>> + return rc; >>> } >>> >>> /* Defer legacy SEV/SEV-ES support if allowed by caller/module. */
On 3/7/25 15:06, Kalra, Ashish wrote: > On 3/7/2025 2:57 PM, Tom Lendacky wrote: >> On 3/7/25 14:54, Tom Lendacky wrote: >>> On 3/6/25 17:09, Ashish Kalra wrote: >>>> From: Ashish Kalra <ashish.kalra@amd.com> >>>> >>>> If SNP host support (SYSCFG.SNPEn) is set, then RMP table must be >>> >>> s/RMP/the RMP/ >>> >>>> initialized up before calling SEV INIT. >>> >>> s/up// >>> >>>> >>>> In other words, if SNP_INIT(_EX) is not issued or fails then >>>> SEV INIT will fail once SNP host support (SYSCFG.SNPEn) is enabled. >>> >>> s/once/if/ >>> >>>> >>>> Fixes: 1ca5614b84eed ("crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP") >>>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> >>>> --- >>>> drivers/crypto/ccp/sev-dev.c | 7 ++----- >>>> 1 file changed, 2 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c >>>> index 2e87ca0e292a..a0e3de94704e 100644 >>>> --- a/drivers/crypto/ccp/sev-dev.c >>>> +++ b/drivers/crypto/ccp/sev-dev.c >>>> @@ -1112,7 +1112,7 @@ static int __sev_snp_init_locked(int *error) >>>> 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 0; >>>> + return -EOPNOTSUPP; >>>> } >>>> >>>> /* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */ >>>> @@ -1325,12 +1325,9 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args) >>>> */ >>>> rc = __sev_snp_init_locked(&args->error); >>>> if (rc && rc != -ENODEV) { >>> >>> Can we get ride of this extra -ENODEV check? It can only be returned >>> because of the same check that is made earlier in this function so it >>> doesn't really serve any purpose from what I can tell. >>> >>> Just make this "if (rc) {" >> >> My bad... -ENODEV is returned if cc_platform_has(CC_ATTR_HOST_SEV_SNP) is >> false, never mind. > > Yes, that's what i was going to reply with ... we want to continue with > SEV INIT if SNP host support is not enabled. Although we could get rid of that awkward if statement by doing... if (cc_platform_has(CC_ATTR_HOST_SEV_SNP)) { rc = __sev_snp_init_locked(&args->error); if (rc) { dev_err(sev->dev, ... return rc; } } And deleting the cc_platform_has() check from __sev_snp_init_locked(). Thanks, Tom > > Thanks, > Ashish > >> >> Thanks, >> Tom >> >>> >>> Thanks, >>> Tom >>> >>>> - /* >>>> - * 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); >>>> + return rc; >>>> } >>>> >>>> /* Defer legacy SEV/SEV-ES support if allowed by caller/module. */ >
On 3/7/2025 3:29 PM, Tom Lendacky wrote: > On 3/7/25 15:06, Kalra, Ashish wrote: >> On 3/7/2025 2:57 PM, Tom Lendacky wrote: >>> On 3/7/25 14:54, Tom Lendacky wrote: >>>> On 3/6/25 17:09, Ashish Kalra wrote: >>>>> From: Ashish Kalra <ashish.kalra@amd.com> >>>>> >>>>> If SNP host support (SYSCFG.SNPEn) is set, then RMP table must be >>>> >>>> s/RMP/the RMP/ >>>> >>>>> initialized up before calling SEV INIT. >>>> >>>> s/up// >>>> >>>>> >>>>> In other words, if SNP_INIT(_EX) is not issued or fails then >>>>> SEV INIT will fail once SNP host support (SYSCFG.SNPEn) is enabled. >>>> >>>> s/once/if/ >>>> >>>>> >>>>> Fixes: 1ca5614b84eed ("crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP") >>>>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> >>>>> --- >>>>> drivers/crypto/ccp/sev-dev.c | 7 ++----- >>>>> 1 file changed, 2 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c >>>>> index 2e87ca0e292a..a0e3de94704e 100644 >>>>> --- a/drivers/crypto/ccp/sev-dev.c >>>>> +++ b/drivers/crypto/ccp/sev-dev.c >>>>> @@ -1112,7 +1112,7 @@ static int __sev_snp_init_locked(int *error) >>>>> 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 0; >>>>> + return -EOPNOTSUPP; >>>>> } >>>>> >>>>> /* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */ >>>>> @@ -1325,12 +1325,9 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args) >>>>> */ >>>>> rc = __sev_snp_init_locked(&args->error); >>>>> if (rc && rc != -ENODEV) { >>>> >>>> Can we get ride of this extra -ENODEV check? It can only be returned >>>> because of the same check that is made earlier in this function so it >>>> doesn't really serve any purpose from what I can tell. >>>> >>>> Just make this "if (rc) {" >>> >>> My bad... -ENODEV is returned if cc_platform_has(CC_ATTR_HOST_SEV_SNP) is >>> false, never mind. >> >> Yes, that's what i was going to reply with ... we want to continue with >> SEV INIT if SNP host support is not enabled. > > Although we could get rid of that awkward if statement by doing... > > if (cc_platform_has(CC_ATTR_HOST_SEV_SNP)) { > rc = __sev_snp_init_locked(&args->error); > if (rc) { > dev_err(sev->dev, ... > return rc; > } > } > > And deleting the cc_platform_has() check from __sev_snp_init_locked(). We probably have to keep the cc_platform_has() check in __sev_snp_init_locked() for the implicit SNP INIT being issued for various SNP specific ioctl's (via snp_move_to_init_state()). Thanks, Ashish >>>> >>>>> - /* >>>>> - * 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); >>>>> + return rc; >>>>> } >>>>> >>>>> /* Defer legacy SEV/SEV-ES support if allowed by caller/module. */ >>
On 3/7/25 15:39, Kalra, Ashish wrote: > On 3/7/2025 3:29 PM, Tom Lendacky wrote: >> On 3/7/25 15:06, Kalra, Ashish wrote: >>> On 3/7/2025 2:57 PM, Tom Lendacky wrote: >>>> On 3/7/25 14:54, Tom Lendacky wrote: >>>>> On 3/6/25 17:09, Ashish Kalra wrote: >>>>>> From: Ashish Kalra <ashish.kalra@amd.com> >>>>>> >>>>>> If SNP host support (SYSCFG.SNPEn) is set, then RMP table must be >>>>> >>>>> s/RMP/the RMP/ >>>>> >>>>>> initialized up before calling SEV INIT. >>>>> >>>>> s/up// >>>>> >>>>>> >>>>>> In other words, if SNP_INIT(_EX) is not issued or fails then >>>>>> SEV INIT will fail once SNP host support (SYSCFG.SNPEn) is enabled. >>>>> >>>>> s/once/if/ >>>>> >>>>>> >>>>>> Fixes: 1ca5614b84eed ("crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP") >>>>>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> >>>>>> --- >>>>>> drivers/crypto/ccp/sev-dev.c | 7 ++----- >>>>>> 1 file changed, 2 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c >>>>>> index 2e87ca0e292a..a0e3de94704e 100644 >>>>>> --- a/drivers/crypto/ccp/sev-dev.c >>>>>> +++ b/drivers/crypto/ccp/sev-dev.c >>>>>> @@ -1112,7 +1112,7 @@ static int __sev_snp_init_locked(int *error) >>>>>> 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 0; >>>>>> + return -EOPNOTSUPP; >>>>>> } >>>>>> >>>>>> /* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */ >>>>>> @@ -1325,12 +1325,9 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args) >>>>>> */ >>>>>> rc = __sev_snp_init_locked(&args->error); >>>>>> if (rc && rc != -ENODEV) { >>>>> >>>>> Can we get ride of this extra -ENODEV check? It can only be returned >>>>> because of the same check that is made earlier in this function so it >>>>> doesn't really serve any purpose from what I can tell. >>>>> >>>>> Just make this "if (rc) {" >>>> >>>> My bad... -ENODEV is returned if cc_platform_has(CC_ATTR_HOST_SEV_SNP) is >>>> false, never mind. >>> >>> Yes, that's what i was going to reply with ... we want to continue with >>> SEV INIT if SNP host support is not enabled. >> >> Although we could get rid of that awkward if statement by doing... >> >> if (cc_platform_has(CC_ATTR_HOST_SEV_SNP)) { >> rc = __sev_snp_init_locked(&args->error); >> if (rc) { >> dev_err(sev->dev, ... >> return rc; >> } >> } >> >> And deleting the cc_platform_has() check from __sev_snp_init_locked(). > > We probably have to keep the cc_platform_has() check in > __sev_snp_init_locked() for the implicit SNP INIT being issued > for various SNP specific ioctl's (via snp_move_to_init_state()). Oh, yeah, the very next patch calls into __sev_snp_init_locked() now. Thanks, Tom > > Thanks, > Ashish > >>>>> >>>>>> - /* >>>>>> - * 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); >>>>>> + return rc; >>>>>> } >>>>>> >>>>>> /* Defer legacy SEV/SEV-ES support if allowed by caller/module. */ >>> >
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index 2e87ca0e292a..a0e3de94704e 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -1112,7 +1112,7 @@ static int __sev_snp_init_locked(int *error) 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 0; + return -EOPNOTSUPP; } /* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */ @@ -1325,12 +1325,9 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args) */ 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); + return rc; } /* Defer legacy SEV/SEV-ES support if allowed by caller/module. */