[PATCH v4 0/6] Support SEV firmware hotloading

[PATCH v4 0/6] Support SEV firmware hotloading

[Dionna Glaze]

The SEV-SNP API specifies a command for hotloading the SEV firmware.
when no SEV or SEV-ES guests are running. The firmware hotloading
support is dependent on the firmware_upload API for better ease-of-use,
and to not necessarily require SEV firmware hotloading support when
building the ccp driver.

For safety, there are steps the kernel should take before allowing a
firmware to be committed:

1. Writeback invalidate all.
2. Data fabric flush.
3. All GCTX pages must be updated successfully with SNP_GUEST_STATUS

The snp_context_create function had the possibility to leak GCTX pages,
so the first patch fixes that bug in KVM.

The ccp driver must continue to be unloadable, so the second patch in
this series fixes a cyclic refcount bug in firmware_loader.

The third patch tracks GCTX pages created and bound to ASIDs at the end
of __sev_do_cmd_locked. These pages are necessary to know at firmware
update time since any running VM must have their context pages updated
before the firmware can be committed.

The fourth patch adds SEV_CMD_DOWNLOAD_FIRMWARE_EX support with its
required cache invalidation steps, but without a means of calling it
from user space.

The fifth patch uses the added command from an instantiation of the
firmware_upload API. When the firmware successfully updates, the GCTX
pages are all refreshed. If any single page's update fails, the driver
treats itself as if the firmware were in a bad state and needs an
immediate restore. All commands that are not DOWNLOAD_FIRMWARE_EX will
fail with RESTORE_REQUIRED, similar to SEV FW on older PSP bootloaders.

The sixth patch avoids platform initialization for KVM VM guests when
vm_type is not legacy SEV/SEV-ES.

The KVM_EXIT for requesting certificates on extended guest request is
not part of this patch series. Any such support must be designed with
races between SNP_COMMIT and servicing extended guest requests such that
the REPORTED_TCB in an attestation_report always correctly corresponds
to the certificates returned by the extended guest request handler.

Changes from v3:
  - Removed added init_args field since it was duplicative of probe.
  - Split ccp change into three changes.
Changes from v2:
  - Fix download_firmware_ex struct definition to be the proper size,
    and clear to 0 before using. Thanks to Alexey Kardashevskiy.
Changes from v1:
  - Fix double-free with incorrect goto label on error.
  - checkpatch cleanup.
  - firmware_loader comment cleanup and one-use local variable inlining.

CC: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Dave Hansen <dave.hansen@linux.intel.com>
CC: Ashish Kalra <ashish.kalra@amd.com>
CC: Tom Lendacky <thomas.lendacky@amd.com>
CC: John Allen <john.allen@amd.com>
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: "David S. Miller" <davem@davemloft.net>
CC: Michael Roth <michael.roth@amd.com>
CC: Luis Chamberlain <mcgrof@kernel.org>
CC: Russ Weight <russ.weight@linux.dev>
CC: Danilo Krummrich <dakr@redhat.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>
CC: Tianfei zhang <tianfei.zhang@intel.com>
CC: Alexey Kardashevskiy <aik@amd.com>

Dionna Glaze (6):
  kvm: svm: Fix gctx page leak on invalid inputs
  firmware_loader: Move module refcounts to allow unloading
  crypto: ccp: Track GCTX through sev commands
  crypto: ccp: Add DOWNLOAD_FIRMWARE_EX support
  crypto: ccp: Use firmware_upload API for SNP firmware
  KVM: SVM: Delay legacy platform initialization on SNP

 arch/x86/kvm/svm/sev.c                      |  10 +-
 drivers/base/firmware_loader/sysfs_upload.c |  16 +-
 drivers/crypto/ccp/Kconfig                  |   2 +
 drivers/crypto/ccp/Makefile                 |   1 +
 drivers/crypto/ccp/sev-dev.c                |  83 ++++-
 drivers/crypto/ccp/sev-dev.h                |  31 ++
 drivers/crypto/ccp/sev-fw.c                 | 330 ++++++++++++++++++++
 include/linux/psp-sev.h                     |  26 ++
 include/uapi/linux/psp-sev.h                |   5 +
 9 files changed, 480 insertions(+), 24 deletions(-)
 create mode 100644 drivers/crypto/ccp/sev-fw.c

-- 
2.47.0.199.ga7371fff76-goog

[PATCH v4 1/6] kvm: svm: Fix gctx page leak on invalid inputs

[Dionna Glaze]

Ensure that snp gctx page allocation is adequately deallocated on
failure during snp_launch_start.

Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command")

CC: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Dave Hansen <dave.hansen@linux.intel.com>
CC: Ashish Kalra <ashish.kalra@amd.com>
CC: Tom Lendacky <thomas.lendacky@amd.com>
CC: John Allen <john.allen@amd.com>
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: "David S. Miller" <davem@davemloft.net>
CC: Michael Roth <michael.roth@amd.com>
CC: Luis Chamberlain <mcgrof@kernel.org>
CC: Russ Weight <russ.weight@linux.dev>
CC: Danilo Krummrich <dakr@redhat.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>
CC: Tianfei zhang <tianfei.zhang@intel.com>
CC: Alexey Kardashevskiy <aik@amd.com>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 arch/x86/kvm/svm/sev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 714c517dd4b72..f6e96ec0a5caa 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (sev->snp_context)
 		return -EINVAL;
 
-	sev->snp_context = snp_context_create(kvm, argp);
-	if (!sev->snp_context)
-		return -ENOTTY;
-
 	if (params.flags)
 		return -EINVAL;
 
@@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
 		return -EINVAL;
 
+	sev->snp_context = snp_context_create(kvm, argp);
+	if (!sev->snp_context)
+		return -ENOTTY;
+
 	start.gctx_paddr = __psp_pa(sev->snp_context);
 	start.policy = params.policy;
 	memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));
-- 
2.47.0.199.ga7371fff76-goog

Re: [PATCH v4 1/6] kvm: svm: Fix gctx page leak on invalid inputs

[Tom Lendacky]

On 11/4/24 19:05, Dionna Glaze wrote:
> Ensure that snp gctx page allocation is adequately deallocated on
> failure during snp_launch_start.
> 
> Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command")
> 
> CC: Sean Christopherson <seanjc@google.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: Dave Hansen <dave.hansen@linux.intel.com>
> CC: Ashish Kalra <ashish.kalra@amd.com>
> CC: Tom Lendacky <thomas.lendacky@amd.com>
> CC: John Allen <john.allen@amd.com>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Michael Roth <michael.roth@amd.com>
> CC: Luis Chamberlain <mcgrof@kernel.org>
> CC: Russ Weight <russ.weight@linux.dev>
> CC: Danilo Krummrich <dakr@redhat.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: "Rafael J. Wysocki" <rafael@kernel.org>
> CC: Tianfei zhang <tianfei.zhang@intel.com>
> CC: Alexey Kardashevskiy <aik@amd.com>
> 
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  arch/x86/kvm/svm/sev.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 714c517dd4b72..f6e96ec0a5caa 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	if (sev->snp_context)
>  		return -EINVAL;
>  
> -	sev->snp_context = snp_context_create(kvm, argp);
> -	if (!sev->snp_context)
> -		return -ENOTTY;
> -
>  	if (params.flags)
>  		return -EINVAL;
>  
> @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
>  		return -EINVAL;
>  
> +	sev->snp_context = snp_context_create(kvm, argp);
> +	if (!sev->snp_context)
> +		return -ENOTTY;
> +
>  	start.gctx_paddr = __psp_pa(sev->snp_context);
>  	start.policy = params.policy;
>  	memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));

Re: [PATCH v4 1/6] kvm: svm: Fix gctx page leak on invalid inputs

[Paolo Bonzini]

On 11/6/24 15:29, Tom Lendacky wrote:
> On 11/4/24 19:05, Dionna Glaze wrote:
>> Ensure that snp gctx page allocation is adequately deallocated on
>> failure during snp_launch_start.
>>
>> Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command")
>>
>> CC: Sean Christopherson <seanjc@google.com>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Thomas Gleixner <tglx@linutronix.de>
>> CC: Ingo Molnar <mingo@redhat.com>
>> CC: Borislav Petkov <bp@alien8.de>
>> CC: Dave Hansen <dave.hansen@linux.intel.com>
>> CC: Ashish Kalra <ashish.kalra@amd.com>
>> CC: Tom Lendacky <thomas.lendacky@amd.com>
>> CC: John Allen <john.allen@amd.com>
>> CC: Herbert Xu <herbert@gondor.apana.org.au>
>> CC: "David S. Miller" <davem@davemloft.net>
>> CC: Michael Roth <michael.roth@amd.com>
>> CC: Luis Chamberlain <mcgrof@kernel.org>
>> CC: Russ Weight <russ.weight@linux.dev>
>> CC: Danilo Krummrich <dakr@redhat.com>
>> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> CC: "Rafael J. Wysocki" <rafael@kernel.org>
>> CC: Tianfei zhang <tianfei.zhang@intel.com>
>> CC: Alexey Kardashevskiy <aik@amd.com>
>>
>> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> 
>> ---
>>   arch/x86/kvm/svm/sev.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 714c517dd4b72..f6e96ec0a5caa 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   	if (sev->snp_context)
>>   		return -EINVAL;
>>   
>> -	sev->snp_context = snp_context_create(kvm, argp);
>> -	if (!sev->snp_context)
>> -		return -ENOTTY;
>> -
>>   	if (params.flags)
>>   		return -EINVAL;
>>   
>> @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   	if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
>>   		return -EINVAL;
>>   
>> +	sev->snp_context = snp_context_create(kvm, argp);
>> +	if (!sev->snp_context)
>> +		return -ENOTTY;
>> +
>>   	start.gctx_paddr = __psp_pa(sev->snp_context);
>>   	start.policy = params.policy;
>>   	memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));
> 

Applied, thanks.

Paolo

Re: [PATCH v4 1/6] kvm: svm: Fix gctx page leak on invalid inputs

[Sean Christopherson]

KVM: SVM:

In the future, please post bug fixes separately from new features series, especially
when the fix has very little to do with the rest of the series (AFAICT, this has
no relation whatsoever beyond SNP).

On Tue, Nov 05, 2024, Dionna Glaze wrote:
> Ensure that snp gctx page allocation is adequately deallocated on
> failure during snp_launch_start.
> 
> Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command")

This needs

Cc: stable@vger.kernel.org

especially if it doesn't get into 6.12.

> CC: Sean Christopherson <seanjc@google.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: Dave Hansen <dave.hansen@linux.intel.com>
> CC: Ashish Kalra <ashish.kalra@amd.com>
> CC: Tom Lendacky <thomas.lendacky@amd.com>
> CC: John Allen <john.allen@amd.com>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Michael Roth <michael.roth@amd.com>
> CC: Luis Chamberlain <mcgrof@kernel.org>
> CC: Russ Weight <russ.weight@linux.dev>
> CC: Danilo Krummrich <dakr@redhat.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: "Rafael J. Wysocki" <rafael@kernel.org>
> CC: Tianfei zhang <tianfei.zhang@intel.com>
> CC: Alexey Kardashevskiy <aik@amd.com>
> 
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>

Acked-by: Sean Christopherson <seanjc@google.com>

Paolo, do you want to grab this one for 6.12 too?

> ---
>  arch/x86/kvm/svm/sev.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 714c517dd4b72..f6e96ec0a5caa 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	if (sev->snp_context)
>  		return -EINVAL;
>  
> -	sev->snp_context = snp_context_create(kvm, argp);
> -	if (!sev->snp_context)
> -		return -ENOTTY;
> -
>  	if (params.flags)
>  		return -EINVAL;
>  
> @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
>  		return -EINVAL;
>  
> +	sev->snp_context = snp_context_create(kvm, argp);
> +	if (!sev->snp_context)
> +		return -ENOTTY;

Related to this fix, the return values from snp_context_create() are garbage.  It
should return ERR_PTR(), not NULL.  -ENOTTY on an OOM scenatio is blatantly wrong,
as -ENOTTY on any SEV_CMD_SNP_GCTX_CREATE failure is too.

> +
>  	start.gctx_paddr = __psp_pa(sev->snp_context);
>  	start.policy = params.policy;
>  	memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));
> -- 
> 2.47.0.199.ga7371fff76-goog
> 

Re: [PATCH v4 1/6] kvm: svm: Fix gctx page leak on invalid inputs

[Dionna Amalie Glaze]

On Wed, Nov 6, 2024 at 6:34 AM Sean Christopherson <seanjc@google.com> wrote:
>
> KVM: SVM:
>
> In the future, please post bug fixes separately from new features series, especially
> when the fix has very little to do with the rest of the series (AFAICT, this has
> no relation whatsoever beyond SNP).
>

Understood. Are dependent series best shared through links to a dev
branch containing all patches?

> On Tue, Nov 05, 2024, Dionna Glaze wrote:
> > Ensure that snp gctx page allocation is adequately deallocated on
> > failure during snp_launch_start.
> >
> > Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command")
>
> This needs
>
> Cc: stable@vger.kernel.org
>
> especially if it doesn't get into 6.12.
>
> > CC: Sean Christopherson <seanjc@google.com>
> > CC: Paolo Bonzini <pbonzini@redhat.com>
> > CC: Thomas Gleixner <tglx@linutronix.de>
> > CC: Ingo Molnar <mingo@redhat.com>
> > CC: Borislav Petkov <bp@alien8.de>
> > CC: Dave Hansen <dave.hansen@linux.intel.com>
> > CC: Ashish Kalra <ashish.kalra@amd.com>
> > CC: Tom Lendacky <thomas.lendacky@amd.com>
> > CC: John Allen <john.allen@amd.com>
> > CC: Herbert Xu <herbert@gondor.apana.org.au>
> > CC: "David S. Miller" <davem@davemloft.net>
> > CC: Michael Roth <michael.roth@amd.com>
> > CC: Luis Chamberlain <mcgrof@kernel.org>
> > CC: Russ Weight <russ.weight@linux.dev>
> > CC: Danilo Krummrich <dakr@redhat.com>
> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > CC: "Rafael J. Wysocki" <rafael@kernel.org>
> > CC: Tianfei zhang <tianfei.zhang@intel.com>
> > CC: Alexey Kardashevskiy <aik@amd.com>
> >
> > Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
>
> Acked-by: Sean Christopherson <seanjc@google.com>
>
> Paolo, do you want to grab this one for 6.12 too?
>
> > ---
> >  arch/x86/kvm/svm/sev.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index 714c517dd4b72..f6e96ec0a5caa 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> >       if (sev->snp_context)
> >               return -EINVAL;
> >
> > -     sev->snp_context = snp_context_create(kvm, argp);
> > -     if (!sev->snp_context)
> > -             return -ENOTTY;
> > -
> >       if (params.flags)
> >               return -EINVAL;
> >
> > @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> >       if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
> >               return -EINVAL;
> >
> > +     sev->snp_context = snp_context_create(kvm, argp);
> > +     if (!sev->snp_context)
> > +             return -ENOTTY;
>
> Related to this fix, the return values from snp_context_create() are garbage.  It
> should return ERR_PTR(), not NULL.  -ENOTTY on an OOM scenatio is blatantly wrong,
> as -ENOTTY on any SEV_CMD_SNP_GCTX_CREATE failure is too.
>

I caught this too. I'll be changing that behavior with the new gctx
management API from ccp in v5, i.e.,

/**
 * sev_snp_create_context - allocates an SNP context firmware page
 *
 * Associates the created context with the ASID that an activation
 * call after SNP_LAUNCH_START will commit. The association is needed
 * to track active GCTX pages to refresh during firmware hotload.
 *
 * @asid:    The ASID allocated to the caller that will be used in a
subsequent SNP_ACTIVATE.
 * @psp_ret: sev command return code.
 *
 * Returns:
 * A pointer to the SNP context page, or an ERR_PTR of
 * -%ENODEV    if the PSP device is not available
 * -%ENOTSUPP  if PSP device does not support SEV
 * -%ETIMEDOUT if the SEV command timed out
 * -%EIO       if PSP device returned a non-zero return code
 */
void *sev_snp_create_context(int asid, int *psp_ret);

/**
 * sev_snp_activate_asid - issues SNP_ACTIVATE for the asid and
associated GCTX page.
 *
 * @asid:    The ASID to activate.
 * @psp_ret: sev command return code.
 *
 * Returns:
 * 0 if the SEV device successfully processed the command
 * -%ENODEV    if the PSP device is not available
 * -%ENOTSUPP  if PSP device does not support SEV
 * -%ETIMEDOUT if the SEV command timed out
 * -%EIO       if PSP device returned a non-zero return code
 */
int sev_snp_activate_asid(int asid, int *psp_ret);

/**
 * sev_snp_guest_decommission - issues SNP_DECOMMISSION for an asid's
GCTX page and frees it.
 *
 * @asid:    The ASID to activate.
 * @psp_ret: sev command return code.
 *
 * Returns:
 * 0 if the SEV device successfully processed the command
 * -%ENODEV    if the PSP device is not available
 * -%ENOTSUPP  if PSP device does not support SEV
 * -%ETIMEDOUT if the SEV command timed out
 * -%EIO       if PSP device returned a non-zero return code
 */
int sev_snp_guest_decommission(int asid, int *psp_ret);

> > +
> >       start.gctx_paddr = __psp_pa(sev->snp_context);
> >       start.policy = params.policy;
> >       memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));
> > --
> > 2.47.0.199.ga7371fff76-goog
> >



-- 
-Dionna Glaze, PhD, CISSP, CCSP (she/her)

Re: [PATCH v4 1/6] kvm: svm: Fix gctx page leak on invalid inputs

[Sean Christopherson]

On Wed, Nov 06, 2024, Dionna Amalie Glaze wrote:
> On Wed, Nov 6, 2024 at 6:34 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > KVM: SVM:
> >
> > In the future, please post bug fixes separately from new features series, especially
> > when the fix has very little to do with the rest of the series (AFAICT, this has
> > no relation whatsoever beyond SNP).
> >
> 
> Understood. Are dependent series best shared through links to a dev
> branch containing all patches?

I don't follow.  There is no dependency here.  If this series were moving
snp_context_create() out of KVM, then that would be a different story, i.e. then
it _would_ be appropriate to include the fix at the front of the series.

If you end up a situation where a dependency is created after the initial posting,
e.g. you post this fix, then later decide to move snp_context_create() out of KVM,
then simply call that out in the cover letter and provide a lore.kernel.org link.

For large scale dependencies, e.g. multi-patch series that build on other multi-patch
series, then providing a link to a git branch is helpful.  But for something this
trivial, it's overkill.

> > > ---
> > >  arch/x86/kvm/svm/sev.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > index 714c517dd4b72..f6e96ec0a5caa 100644
> > > --- a/arch/x86/kvm/svm/sev.c
> > > +++ b/arch/x86/kvm/svm/sev.c
> > > @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > >       if (sev->snp_context)
> > >               return -EINVAL;
> > >
> > > -     sev->snp_context = snp_context_create(kvm, argp);
> > > -     if (!sev->snp_context)
> > > -             return -ENOTTY;
> > > -
> > >       if (params.flags)
> > >               return -EINVAL;
> > >
> > > @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > >       if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET)
> > >               return -EINVAL;
> > >
> > > +     sev->snp_context = snp_context_create(kvm, argp);
> > > +     if (!sev->snp_context)
> > > +             return -ENOTTY;
> >
> > Related to this fix, the return values from snp_context_create() are garbage.  It
> > should return ERR_PTR(), not NULL.  -ENOTTY on an OOM scenatio is blatantly wrong,
> > as -ENOTTY on any SEV_CMD_SNP_GCTX_CREATE failure is too.
> 
> I caught this too. I'll be changing that behavior with the new gctx
> management API from ccp in v5, i.e.,

Please fix the KVM flaws before moving code out of KVM, i.e. ensure the flaws are
cleaned up even if we opt not to go the route of moving the code out of KVM (which
I assume is what you plan to do with sev_snp_create_context()).

[PATCH v4 2/6] firmware_loader: Move module refcounts to allow unloading

[Dionna Glaze]

If a kernel module registers a firmware upload API ops set, then it's
unable to be moved due to effectively a cyclic reference that the module
depends on the upload which depends on the module.

Instead, only require the try_module_get when an upload is requested to
disallow unloading a module only while the upload is in progress.

Fixes: 97730bbb242c ("firmware_loader: Add firmware-upload support")

CC: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Dave Hansen <dave.hansen@linux.intel.com>
CC: Ashish Kalra <ashish.kalra@amd.com>
CC: Tom Lendacky <thomas.lendacky@amd.com>
CC: John Allen <john.allen@amd.com>
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: "David S. Miller" <davem@davemloft.net>
CC: Michael Roth <michael.roth@amd.com>
CC: Luis Chamberlain <mcgrof@kernel.org>
CC: Russ Weight <russ.weight@linux.dev>
CC: Danilo Krummrich <dakr@redhat.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>
CC: Tianfei zhang <tianfei.zhang@intel.com>
CC: Alexey Kardashevskiy <aik@amd.com>

Tested-by: Ashish Kalra <ashish.kalra@amd.com>
Reviewed-by: Russ Weight <russ.weight@linux.dev>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 drivers/base/firmware_loader/sysfs_upload.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
index 829270067d163..7d9c6aef7720a 100644
--- a/drivers/base/firmware_loader/sysfs_upload.c
+++ b/drivers/base/firmware_loader/sysfs_upload.c
@@ -204,6 +204,7 @@ static void fw_upload_main(struct work_struct *work)
 		fwlp->ops->cleanup(fwl);
 
 putdev_exit:
+	module_put(fwlp->module);
 	put_device(fw_dev->parent);
 
 	/*
@@ -239,6 +240,9 @@ int fw_upload_start(struct fw_sysfs *fw_sysfs)
 	}
 
 	fwlp = fw_sysfs->fw_upload_priv;
+	if (!try_module_get(fwlp->module)) /* released in fw_upload_main */
+		return -EFAULT;
+
 	mutex_lock(&fwlp->lock);
 
 	/* Do not interfere with an on-going fw_upload */
@@ -310,13 +314,10 @@ firmware_upload_register(struct module *module, struct device *parent,
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (!try_module_get(module))
-		return ERR_PTR(-EFAULT);
-
 	fw_upload = kzalloc(sizeof(*fw_upload), GFP_KERNEL);
 	if (!fw_upload) {
 		ret = -ENOMEM;
-		goto exit_module_put;
+		goto exit_err;
 	}
 
 	fw_upload_priv = kzalloc(sizeof(*fw_upload_priv), GFP_KERNEL);
@@ -358,7 +359,7 @@ firmware_upload_register(struct module *module, struct device *parent,
 	if (ret) {
 		dev_err(fw_dev, "%s: device_register failed\n", __func__);
 		put_device(fw_dev);
-		goto exit_module_put;
+		goto exit_err;
 	}
 
 	return fw_upload;
@@ -372,8 +373,7 @@ firmware_upload_register(struct module *module, struct device *parent,
 free_fw_upload:
 	kfree(fw_upload);
 
-exit_module_put:
-	module_put(module);
+exit_err:
 
 	return ERR_PTR(ret);
 }
@@ -387,7 +387,6 @@ void firmware_upload_unregister(struct fw_upload *fw_upload)
 {
 	struct fw_sysfs *fw_sysfs = fw_upload->priv;
 	struct fw_upload_priv *fw_upload_priv = fw_sysfs->fw_upload_priv;
-	struct module *module = fw_upload_priv->module;
 
 	mutex_lock(&fw_upload_priv->lock);
 	if (fw_upload_priv->progress == FW_UPLOAD_PROG_IDLE) {
@@ -403,6 +402,5 @@ void firmware_upload_unregister(struct fw_upload *fw_upload)
 
 unregister:
 	device_unregister(&fw_sysfs->dev);
-	module_put(module);
 }
 EXPORT_SYMBOL_GPL(firmware_upload_unregister);
-- 
2.47.0.199.ga7371fff76-goog

[PATCH v4 3/6] crypto: ccp: Track GCTX through sev commands

[Dionna Glaze]

In preparation for SEV firmware hotloading support, add bookkeeping
structures for GCTX pages that are in use.

Compliance with SEV-SNP API section 3.3 Firmware Updates and 4.1.1
Live Update: before a firmware is committed, all active GCTX pages
should be updated with SNP_GUEST_STATUS to ensure their data structure
remains consistent for the new firmware version.
There can only be cpuid_edx(0x8000001f)-1 many SEV-SNP asids in use at
one time, so this map associates asid to gctx in order to track which
addresses are active gctx pages that need updating. When an asid and
gctx page are decommissioned, the page is removed from tracking for
update-purposes.
Given that GCTX page creation and binding through the SNP_ACTIVATE
command are separate, the creation operation also tracks pages that are
yet to be bound to an asid.

The hotloading support depends on FW_UPLOAD, so the new functions are
added in a new file whose object file is conditionally included in the
module build.

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 drivers/crypto/ccp/Makefile  |   1 +
 drivers/crypto/ccp/sev-dev.c |   5 ++
 drivers/crypto/ccp/sev-dev.h |  15 +++++
 drivers/crypto/ccp/sev-fw.c  | 117 +++++++++++++++++++++++++++++++++++
 4 files changed, 138 insertions(+)
 create mode 100644 drivers/crypto/ccp/sev-fw.c

diff --git a/drivers/crypto/ccp/Makefile b/drivers/crypto/ccp/Makefile
index 394484929dae3..b8b5102cc7973 100644
--- a/drivers/crypto/ccp/Makefile
+++ b/drivers/crypto/ccp/Makefile
@@ -14,6 +14,7 @@ ccp-$(CONFIG_CRYPTO_DEV_SP_PSP) += psp-dev.o \
                                    platform-access.o \
                                    dbc.o \
                                    hsti.o
+ccp-$(CONFIG_FW_UPLOAD) += sev-fw.o
 
 obj-$(CONFIG_CRYPTO_DEV_CCP_CRYPTO) += ccp-crypto.o
 ccp-crypto-objs := ccp-crypto-main.o \
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 9810edbb272d2..9265b6d534bbe 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -982,6 +982,9 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 	print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
 			     buf_len, false);
 
+	if (!ret)
+		snp_cmd_bookkeeping_locked(cmd, sev, data);
+
 	return ret;
 }
 
@@ -1179,6 +1182,8 @@ static int __sev_snp_init_locked(int *error)
 
 	sev_es_tmr_size = SNP_TMR_SIZE;
 
+	rc = sev_snp_platform_init_firmware_upload(sev);
+
 	return rc;
 }
 
diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
index 3e4e5574e88a3..28add34484ed1 100644
--- a/drivers/crypto/ccp/sev-dev.h
+++ b/drivers/crypto/ccp/sev-dev.h
@@ -57,6 +57,13 @@ struct sev_device {
 	bool cmd_buf_backup_active;
 
 	bool snp_initialized;
+
+#ifdef CONFIG_FW_UPLOAD
+	u32 last_snp_asid;
+	u64 *snp_asid_to_gctx_pages_map;
+	u64 *snp_unbound_gctx_pages;
+	u32 snp_unbound_gctx_end;
+#endif /* CONFIG_FW_UPLOAD */
 };
 
 int sev_dev_init(struct psp_device *psp);
@@ -65,4 +72,12 @@ void sev_dev_destroy(struct psp_device *psp);
 void sev_pci_init(void);
 void sev_pci_exit(void);
 
+#ifdef CONFIG_FW_UPLOAD
+void snp_cmd_bookkeeping_locked(int cmd, struct sev_device *sev, void *data);
+int sev_snp_platform_init_firmware_upload(struct sev_device *sev);
+#else
+static inline void snp_cmd_bookkeeping_locked(int cmd, struct sev_device *sev, void *data) { }
+static inline int sev_snp_platform_init_firmware_upload(struct sev_device *sev) { return 0; }
+#endif /* CONFIG_FW_UPLOAD */
+
 #endif /* __SEV_DEV_H */
diff --git a/drivers/crypto/ccp/sev-fw.c b/drivers/crypto/ccp/sev-fw.c
new file mode 100644
index 0000000000000..55a5a572da8f1
--- /dev/null
+++ b/drivers/crypto/ccp/sev-fw.c
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AMD Secure Encrypted Virtualization (SEV) firmware upload API
+ */
+
+#include <linux/firmware.h>
+#include <linux/psp-sev.h>
+
+#include <asm/sev.h>
+
+#include "sev-dev.h"
+
+/*
+ * After a gctx is created, it is used by snp_launch_start before getting
+ * bound to an asid. The launch protocol allocates an asid before creating a
+ * matching gctx page, so there should never be more unbound gctx pages than
+ * there are possible SNP asids.
+ *
+ * The unbound gctx pages must be updated after executing DOWNLOAD_FIRMWARE_EX
+ * and before committing the firmware.
+ */
+static void snp_gctx_create_track_locked(struct sev_device *sev, void *data)
+{
+	struct sev_data_snp_addr *gctx_create = data;
+
+	/* This condition should never happen, but is needed for memory safety. */
+	if (sev->snp_unbound_gctx_end >= sev->last_snp_asid) {
+		dev_warn(sev->dev, "Too many unbound SNP GCTX pages to track\n");
+		return;
+	}
+
+	sev->snp_unbound_gctx_pages[sev->snp_unbound_gctx_end] = gctx_create->address;
+	sev->snp_unbound_gctx_end++;
+}
+
+/*
+ * PREREQUISITE: The snp_activate command was successful, meaning the asid
+ * is in the acceptable range 1..sev->last_snp_asid.
+ *
+ * The gctx_paddr must be in the unbound gctx buffer.
+ */
+static void snp_activate_track_locked(struct sev_device *sev, void *data)
+{
+	struct sev_data_snp_activate *data_activate = data;
+
+	sev->snp_asid_to_gctx_pages_map[data_activate->asid] = data_activate->gctx_paddr;
+
+	for (int i = 0; i < sev->snp_unbound_gctx_end; i++) {
+		if (sev->snp_unbound_gctx_pages[i] == data_activate->gctx_paddr) {
+			/*
+			 * Swap the last unbound gctx page with the now-bound
+			 * gctx page to shrink the buffer.
+			 */
+			sev->snp_unbound_gctx_end--;
+			sev->snp_unbound_gctx_pages[i] =
+				sev->snp_unbound_gctx_pages[sev->snp_unbound_gctx_end];
+			sev->snp_unbound_gctx_pages[sev->snp_unbound_gctx_end] = 0;
+			break;
+		}
+	}
+}
+
+static void snp_decommission_track_locked(struct sev_device *sev, void *data)
+{
+	struct sev_data_snp_addr *data_decommission = data;
+
+	for (int i = 1; i <= sev->last_snp_asid; i++) {
+		if (sev->snp_asid_to_gctx_pages_map[i] == data_decommission->address) {
+			sev->snp_asid_to_gctx_pages_map[i] = 0;
+			break;
+		}
+	}
+}
+
+void snp_cmd_bookkeeping_locked(int cmd, struct sev_device *sev, void *data)
+{
+	if (!sev->snp_asid_to_gctx_pages_map)
+		return;
+
+	switch (cmd) {
+	case SEV_CMD_SNP_GCTX_CREATE:
+		snp_gctx_create_track_locked(sev, data);
+		break;
+	case SEV_CMD_SNP_ACTIVATE:
+		snp_activate_track_locked(sev, data);
+		break;
+	case SEV_CMD_SNP_DECOMMISSION:
+		snp_decommission_track_locked(sev, data);
+		break;
+	default:
+		break;
+	}
+}
+
+int sev_snp_platform_init_firmware_upload(struct sev_device *sev)
+{
+	u32 max_snp_asid;
+
+	/*
+	 * cpuid_edx(0x8000001f) is the minimum SEV ASID, hence the exclusive
+	 * maximum SEV-SNP ASID. Save the inclusive maximum to avoid confusing
+	 * logic elsewhere.
+	 */
+	max_snp_asid = cpuid_edx(0x8000001f);
+	sev->last_snp_asid = max_snp_asid - 1;
+	if (sev->last_snp_asid) {
+		sev->snp_asid_to_gctx_pages_map = devm_kmalloc_array(
+			sev->dev, max_snp_asid * 2, sizeof(u64), GFP_KERNEL | __GFP_ZERO);
+		sev->snp_unbound_gctx_pages = &sev->snp_asid_to_gctx_pages_map[max_snp_asid];
+		if (!sev->snp_asid_to_gctx_pages_map) {
+			dev_err(sev->dev,
+				"SEV-SNP: snp_asid_to_gctx_pages_map memory allocation failed\n");
+			return -ENOMEM;
+		}
+	}
+	return 0;
+}
-- 
2.47.0.199.ga7371fff76-goog

Re: [PATCH v4 3/6] crypto: ccp: Track GCTX through sev commands

[kernel test robot]

Hi Dionna,

kernel test robot noticed the following build errors:

[auto build test ERROR on herbert-cryptodev-2.6/master]
[also build test ERROR on herbert-crypto-2.6/master kvm/queue linus/master v6.12-rc6 next-20241105]
[cannot apply to kvm/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dionna-Glaze/kvm-svm-Fix-gctx-page-leak-on-invalid-inputs/20241105-090822
base:   https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
patch link:    https://lore.kernel.org/r/20241105010558.1266699-4-dionnaglaze%40google.com
patch subject: [PATCH v4 3/6] crypto: ccp: Track GCTX through sev commands
config: arm64-allmodconfig (https://download.01.org/0day-ci/archive/20241105/202411051934.6vECpMIv-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 639a7ac648f1e50ccd2556e17d401c04f9cce625)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241105/202411051934.6vECpMIv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411051934.6vECpMIv-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/crypto/ccp/sev-fw.c:9:10: fatal error: 'asm/sev.h' file not found
       9 | #include <asm/sev.h>
         |          ^~~~~~~~~~~
   1 error generated.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for MODVERSIONS
   Depends on [n]: MODULES [=y] && !COMPILE_TEST [=y]
   Selected by [y]:
   - RANDSTRUCT_FULL [=y] && (CC_HAS_RANDSTRUCT [=y] || GCC_PLUGINS [=n]) && MODULES [=y]


vim +9 drivers/crypto/ccp/sev-fw.c

     8	
   > 9	#include <asm/sev.h>
    10	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 3/6] crypto: ccp: Track GCTX through sev commands

[Tom Lendacky]

On 11/4/24 19:05, Dionna Glaze wrote:
> In preparation for SEV firmware hotloading support, add bookkeeping
> structures for GCTX pages that are in use.
> 
> Compliance with SEV-SNP API section 3.3 Firmware Updates and 4.1.1
> Live Update: before a firmware is committed, all active GCTX pages
> should be updated with SNP_GUEST_STATUS to ensure their data structure
> remains consistent for the new firmware version.
> There can only be cpuid_edx(0x8000001f)-1 many SEV-SNP asids in use at
> one time, so this map associates asid to gctx in order to track which
> addresses are active gctx pages that need updating. When an asid and
> gctx page are decommissioned, the page is removed from tracking for
> update-purposes.
> Given that GCTX page creation and binding through the SNP_ACTIVATE
> command are separate, the creation operation also tracks pages that are
> yet to be bound to an asid.

I believe we have an ASID "allocated" by the time we call
snp_context_create(). And snp_decommission_context() is always called to
remove the context. Maybe a ccp driver API to create and destroy a
context would be good here. The ASID would be an additional parameter
and allow for associating the guest context page to the ASID right from
the start.

This way you don't need an snp_unbound_gctx_pages array.

I think it will make the code a lot simpler, but it does add an API
dependency between the CCP and KVM that needs to be worked out between
the maintainers.

> 
> The hotloading support depends on FW_UPLOAD, so the new functions are
> added in a new file whose object file is conditionally included in the
> module build.
> 
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  drivers/crypto/ccp/Makefile  |   1 +
>  drivers/crypto/ccp/sev-dev.c |   5 ++
>  drivers/crypto/ccp/sev-dev.h |  15 +++++
>  drivers/crypto/ccp/sev-fw.c  | 117 +++++++++++++++++++++++++++++++++++
>  4 files changed, 138 insertions(+)
>  create mode 100644 drivers/crypto/ccp/sev-fw.c
> 
> diff --git a/drivers/crypto/ccp/Makefile b/drivers/crypto/ccp/Makefile
> index 394484929dae3..b8b5102cc7973 100644
> --- a/drivers/crypto/ccp/Makefile
> +++ b/drivers/crypto/ccp/Makefile
> @@ -14,6 +14,7 @@ ccp-$(CONFIG_CRYPTO_DEV_SP_PSP) += psp-dev.o \
>                                     platform-access.o \
>                                     dbc.o \
>                                     hsti.o
> +ccp-$(CONFIG_FW_UPLOAD) += sev-fw.o

As you saw from the krobot mail, you will probably need to create
something like CRYPTO_DEV_SP_PSP_FW_UPLOAD, which depends on
CRYPTO_DEV_SP_PSP and FW_UPLOAD.

>  
>  obj-$(CONFIG_CRYPTO_DEV_CCP_CRYPTO) += ccp-crypto.o
>  ccp-crypto-objs := ccp-crypto-main.o \
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 9810edbb272d2..9265b6d534bbe 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -982,6 +982,9 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>  	print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
>  			     buf_len, false);
>  
> +	if (!ret)
> +		snp_cmd_bookkeeping_locked(cmd, sev, data);

I prefer to see this flow as:

	if (ret)
		return ret;

	snp_cmd_bookkeeping_locked(cmd, sev, data);

	return 0;

And if you end up creating APIs to create and destroy context pages,
then this call can be removed and each API call directly into the
appropriate tracking function.

> +
>  	return ret;
>  }
>  
> @@ -1179,6 +1182,8 @@ static int __sev_snp_init_locked(int *error)
>  
>  	sev_es_tmr_size = SNP_TMR_SIZE;
>  
> +	rc = sev_snp_platform_init_firmware_upload(sev);

Since this is mainly doing memory allocation, this should be moved to
just after the minimum firmware version check so that a failure would be
before SNP_INIT.

> +
>  	return rc;
>  }
>  
> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> index 3e4e5574e88a3..28add34484ed1 100644
> --- a/drivers/crypto/ccp/sev-dev.h
> +++ b/drivers/crypto/ccp/sev-dev.h
> @@ -57,6 +57,13 @@ struct sev_device {
>  	bool cmd_buf_backup_active;
>  
>  	bool snp_initialized;
> +
> +#ifdef CONFIG_FW_UPLOAD

This would be changed to whatever your new CONFIG option is.

> +	u32 last_snp_asid;

unsigned int max_snp_asid;

> +	u64 *snp_asid_to_gctx_pages_map;

s/_pages//

> +	u64 *snp_unbound_gctx_pages;

s/_pages//

> +	u32 snp_unbound_gctx_end;

unsigned int snp_unbound_gctx_end;

> +#endif /* CONFIG_FW_UPLOAD */
>  };
>  
>  int sev_dev_init(struct psp_device *psp);
> @@ -65,4 +72,12 @@ void sev_dev_destroy(struct psp_device *psp);
>  void sev_pci_init(void);
>  void sev_pci_exit(void);
>  
> +#ifdef CONFIG_FW_UPLOAD
> +void snp_cmd_bookkeeping_locked(int cmd, struct sev_device *sev, void *data);
> +int sev_snp_platform_init_firmware_upload(struct sev_device *sev);
> +#else
> +static inline void snp_cmd_bookkeeping_locked(int cmd, struct sev_device *sev, void *data) { }
> +static inline int sev_snp_platform_init_firmware_upload(struct sev_device *sev) { return 0; }
> +#endif /* CONFIG_FW_UPLOAD */
> +
>  #endif /* __SEV_DEV_H */
> diff --git a/drivers/crypto/ccp/sev-fw.c b/drivers/crypto/ccp/sev-fw.c
> new file mode 100644
> index 0000000000000..55a5a572da8f1
> --- /dev/null
> +++ b/drivers/crypto/ccp/sev-fw.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * AMD Secure Encrypted Virtualization (SEV) firmware upload API
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/psp-sev.h>
> +
> +#include <asm/sev.h>
> +
> +#include "sev-dev.h"
> +
> +/*
> + * After a gctx is created, it is used by snp_launch_start before getting

s/gctx/guest context page/

> + * bound to an asid. The launch protocol allocates an asid before creating a

s/asid/ASID/

> + * matching gctx page, so there should never be more unbound gctx pages than
> + * there are possible SNP asids.
> + *
> + * The unbound gctx pages must be updated after executing DOWNLOAD_FIRMWARE_EX
> + * and before committing the firmware.

Not needed here.

> + */
> +static void snp_gctx_create_track_locked(struct sev_device *sev, void *data)
> +{
> +	struct sev_data_snp_addr *gctx_create = data;
> +
> +	/* This condition should never happen, but is needed for memory safety. */
> +	if (sev->snp_unbound_gctx_end >= sev->last_snp_asid) {
> +		dev_warn(sev->dev, "Too many unbound SNP GCTX pages to track\n");
> +		return;

Should this return an error and fail the command?

> +	}
> +
> +	sev->snp_unbound_gctx_pages[sev->snp_unbound_gctx_end] = gctx_create->address;
> +	sev->snp_unbound_gctx_end++;
> +}
> +
> +/*
> + * PREREQUISITE: The snp_activate command was successful, meaning the asid

s/snp_activate/SNP_ACTIVATE/
s/asid/ASID/

> + * is in the acceptable range 1..sev->last_snp_asid.
> + *
> + * The gctx_paddr must be in the unbound gctx buffer.

Do you mean unbound gctx array?

> + */
> +static void snp_activate_track_locked(struct sev_device *sev, void *data)
> +{
> +	struct sev_data_snp_activate *data_activate = data;
> +
> +	sev->snp_asid_to_gctx_pages_map[data_activate->asid] = data_activate->gctx_paddr;
> +
> +	for (int i = 0; i < sev->snp_unbound_gctx_end; i++) {
> +		if (sev->snp_unbound_gctx_pages[i] == data_activate->gctx_paddr) {
> +			/*
> +			 * Swap the last unbound gctx page with the now-bound
> +			 * gctx page to shrink the buffer.
> +			 */
> +			sev->snp_unbound_gctx_end--;
> +			sev->snp_unbound_gctx_pages[i] =
> +				sev->snp_unbound_gctx_pages[sev->snp_unbound_gctx_end];
> +			sev->snp_unbound_gctx_pages[sev->snp_unbound_gctx_end] = 0;
> +			break;
> +		}
> +	}

What if it isn't found for some reason, should an error be returned and
fail the SNP_ACTIVATE command?

> +}
> +
> +static void snp_decommission_track_locked(struct sev_device *sev, void *data)
> +{
> +	struct sev_data_snp_addr *data_decommission = data;
> +
> +	for (int i = 1; i <= sev->last_snp_asid; i++) {
> +		if (sev->snp_asid_to_gctx_pages_map[i] == data_decommission->address) {
> +			sev->snp_asid_to_gctx_pages_map[i] = 0;
> +			break;
> +		}
> +	}
> +}
> +
> +void snp_cmd_bookkeeping_locked(int cmd, struct sev_device *sev, void *data)
> +{
> +	if (!sev->snp_asid_to_gctx_pages_map)
> +		return;
> +
> +	switch (cmd) {
> +	case SEV_CMD_SNP_GCTX_CREATE:
> +		snp_gctx_create_track_locked(sev, data);
> +		break;
> +	case SEV_CMD_SNP_ACTIVATE:
> +		snp_activate_track_locked(sev, data);
> +		break;
> +	case SEV_CMD_SNP_DECOMMISSION:
> +		snp_decommission_track_locked(sev, data);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +int sev_snp_platform_init_firmware_upload(struct sev_device *sev)

snp_init_guest_context_tracking

> +{
> +	u32 max_snp_asid;

s/max_snp_asid/min_sev_asid/

> +
> +	/*
> +	 * cpuid_edx(0x8000001f) is the minimum SEV ASID, hence the exclusive

CPUID 0x8000001f_EDX contains the ...

> +	 * maximum SEV-SNP ASID. Save the inclusive maximum to avoid confusing
> +	 * logic elsewhere.
> +	 */
> +	max_snp_asid = cpuid_edx(0x8000001f);

if (max_snp_asid <= 1)
	return 0;

Then the indentation below can be removed and the alignment fixed.

> +	sev->last_snp_asid = max_snp_asid - 1;
> +	if (sev->last_snp_asid) {
> +		sev->snp_asid_to_gctx_pages_map = devm_kmalloc_array(
> +			sev->dev, max_snp_asid * 2, sizeof(u64), GFP_KERNEL | __GFP_ZERO);
> +		sev->snp_unbound_gctx_pages = &sev->snp_asid_to_gctx_pages_map[max_snp_asid];
> +		if (!sev->snp_asid_to_gctx_pages_map) {

I'd prefer to see the success check immediately after the allocation call.

Thanks,
Tom


> +			dev_err(sev->dev,
> +				"SEV-SNP: snp_asid_to_gctx_pages_map memory allocation failed\n");
> +			return -ENOMEM;
> +		}
> +	}
> +	return 0;
> +}

[PATCH v4 4/6] crypto: ccp: Add DOWNLOAD_FIRMWARE_EX support

[Dionna Glaze]

The DOWNLOAD_FIRMWARE_EX command requires cache flushing and introduces
new error codes that could be returned to user space.

The function is in sev-dev.c rather than sev-fw.c to avoid exposing the
__sev_do_cmd_locked function in the sev-dev.h header.

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 drivers/crypto/ccp/sev-dev.c | 66 +++++++++++++++++++++++++++++++-----
 include/linux/psp-sev.h      | 26 ++++++++++++++
 include/uapi/linux/psp-sev.h |  5 +++
 3 files changed, 89 insertions(+), 8 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 9265b6d534bbe..32f7b6147905e 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd)
 	case SEV_CMD_SNP_GUEST_REQUEST:		return sizeof(struct sev_data_snp_guest_request);
 	case SEV_CMD_SNP_CONFIG:		return sizeof(struct sev_user_data_snp_config);
 	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
+	case SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX:	return sizeof(struct sev_data_download_firmware_ex);
 	default:				return 0;
 	}
 
@@ -1597,14 +1598,7 @@ static int sev_update_firmware(struct device *dev)
 		return -1;
 	}
 
-	/*
-	 * SEV FW expects the physical address given to it to be 32
-	 * byte aligned. Memory allocated has structure placed at the
-	 * beginning followed by the firmware being passed to the SEV
-	 * FW. Allocate enough memory for data structure + alignment
-	 * padding + SEV FW.
-	 */
-	data_size = ALIGN(sizeof(struct sev_data_download_firmware), 32);
+	data_size = ALIGN(sizeof(struct sev_data_download_firmware), SEV_FW_ALIGNMENT);
 
 	order = get_order(firmware->size + data_size);
 	p = alloc_pages(GFP_KERNEL, order);
@@ -1645,6 +1639,62 @@ static int sev_update_firmware(struct device *dev)
 	return ret;
 }
 
+int sev_snp_download_firmware_ex(struct sev_device *sev, const u8 *data, u32 size, int *error)
+{
+	struct sev_data_download_firmware_ex *data_ex;
+	int ret, order;
+	struct page *p;
+	u64 data_size;
+	void *fw_dest;
+
+	data_size = ALIGN(sizeof(struct sev_data_download_firmware_ex),
+			  SEV_FW_ALIGNMENT);
+
+	order = get_order(size + data_size);
+	p = alloc_pages(GFP_KERNEL, order);
+	if (!p)
+		return -ENOMEM;
+
+	/*
+	 * Copy firmware data to a kernel allocated contiguous
+	 * memory region.
+	 */
+	data_ex = page_address(p);
+	fw_dest = page_address(p) + data_size;
+	memset(data_ex, 0, data_size);
+	memcpy(fw_dest, data, size);
+
+	data_ex->address = __psp_pa(fw_dest);
+	data_ex->len = size;
+	data_ex->cmdlen = sizeof(struct sev_data_download_firmware_ex);
+
+	/*
+	 * SNP_COMMIT should be issued explicitly to commit the updated
+	 * firmware after guest context pages have been updated.
+	 */
+	ret = sev_do_cmd(SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX, data_ex, error);
+
+	if (ret)
+		goto free_err;
+
+	__free_pages(p, order);
+
+	/* Need to do a DF_FLUSH after live firmware update */
+	wbinvd_on_all_cpus();
+	ret = __sev_do_cmd_locked(SEV_CMD_SNP_DF_FLUSH, NULL, error);
+	if (ret) {
+		dev_dbg(sev->dev, "DF_FLUSH error %d\n", *error);
+		goto fw_err;
+	}
+
+	return 0;
+
+free_err:
+	__free_pages(p, order);
+fw_err:
+	return ret;
+}
+
 static int __sev_snp_shutdown_locked(int *error, bool panic)
 {
 	struct psp_device *psp = psp_master;
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 903ddfea85850..6a08c56cd9771 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -16,6 +16,15 @@
 
 #define SEV_FW_BLOB_MAX_SIZE	0x4000	/* 16KB */
 
+/*
+ * SEV FW expects the physical address given to it to be 32
+ * byte aligned. Memory allocated has structure placed at the
+ * beginning followed by the firmware being passed to the SEV
+ * FW. Allocate enough memory for data structure + alignment
+ * padding + SEV FW.
+ */
+#define SEV_FW_ALIGNMENT       32
+
 /**
  * SEV platform state
  */
@@ -185,6 +194,23 @@ struct sev_data_download_firmware {
 	u32 len;				/* In */
 } __packed;
 
+/**
+ * struct sev_data_download_firmware_ex - DOWNLOAD_FIRMWARE_EX command parameters
+ *
+ * @length: length of this command buffer
+ * @address: physical address of firmware image
+ * @len: len of the firmware image
+ * @commit: automatically commit the newly installed image
+ */
+struct sev_data_download_firmware_ex {
+	u32 cmdlen;				/* In */
+	u32 reserved;				/* In */
+	u64 address;				/* In */
+	u32 len;				/* In */
+	u32 commit:1;				/* In */
+	u32 reserved2:31;			/* In */
+} __packed;
+
 /**
  * struct sev_data_get_id - GET_ID command parameters
  *
diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
index 832c15d9155bd..936464d4f282a 100644
--- a/include/uapi/linux/psp-sev.h
+++ b/include/uapi/linux/psp-sev.h
@@ -80,6 +80,11 @@ typedef enum {
 	SEV_RET_INVALID_PAGE_OWNER,
 	SEV_RET_INVALID_PAGE_AEAD_OFLOW,
 	SEV_RET_RMP_INIT_REQUIRED,
+	SEV_RET_BAD_SVN,
+	SEV_RET_BAD_VERSION,
+	SEV_RET_SHUTDOWN_REQUIRED,
+	SEV_RET_UPDATE_FAILED,
+	SEV_RET_RESTORE_REQUIRED,
 	SEV_RET_MAX,
 } sev_ret_code;
 
-- 
2.47.0.199.ga7371fff76-goog

Re: [PATCH v4 4/6] crypto: ccp: Add DOWNLOAD_FIRMWARE_EX support

[Tom Lendacky]

On 11/4/24 19:05, Dionna Glaze wrote:
> The DOWNLOAD_FIRMWARE_EX command requires cache flushing and introduces
> new error codes that could be returned to user space.
> 
> The function is in sev-dev.c rather than sev-fw.c to avoid exposing the
> __sev_do_cmd_locked function in the sev-dev.h header.

Please say "why" this is being added.

> 
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  drivers/crypto/ccp/sev-dev.c | 66 +++++++++++++++++++++++++++++++-----
>  include/linux/psp-sev.h      | 26 ++++++++++++++
>  include/uapi/linux/psp-sev.h |  5 +++
>  3 files changed, 89 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 9265b6d534bbe..32f7b6147905e 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd)
>  	case SEV_CMD_SNP_GUEST_REQUEST:		return sizeof(struct sev_data_snp_guest_request);
>  	case SEV_CMD_SNP_CONFIG:		return sizeof(struct sev_user_data_snp_config);
>  	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
> +	case SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX:	return sizeof(struct sev_data_download_firmware_ex);
>  	default:				return 0;
>  	}
>  
> @@ -1597,14 +1598,7 @@ static int sev_update_firmware(struct device *dev)
>  		return -1;
>  	}
>  
> -	/*
> -	 * SEV FW expects the physical address given to it to be 32
> -	 * byte aligned. Memory allocated has structure placed at the
> -	 * beginning followed by the firmware being passed to the SEV
> -	 * FW. Allocate enough memory for data structure + alignment
> -	 * padding + SEV FW.
> -	 */
> -	data_size = ALIGN(sizeof(struct sev_data_download_firmware), 32);
> +	data_size = ALIGN(sizeof(struct sev_data_download_firmware), SEV_FW_ALIGNMENT);
>  
>  	order = get_order(firmware->size + data_size);
>  	p = alloc_pages(GFP_KERNEL, order);
> @@ -1645,6 +1639,62 @@ static int sev_update_firmware(struct device *dev)
>  	return ret;
>  }
>  
> +int sev_snp_download_firmware_ex(struct sev_device *sev, const u8 *data, u32 size, int *error)
> +{
> +	struct sev_data_download_firmware_ex *data_ex;
> +	int ret, order;
> +	struct page *p;
> +	u64 data_size;
> +	void *fw_dest;
> +
> +	data_size = ALIGN(sizeof(struct sev_data_download_firmware_ex),
> +			  SEV_FW_ALIGNMENT);

This can be a single line.

> +
> +	order = get_order(size + data_size);
> +	p = alloc_pages(GFP_KERNEL, order);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Copy firmware data to a kernel allocated contiguous
> +	 * memory region.
> +	 */
> +	data_ex = page_address(p);
> +	fw_dest = page_address(p) + data_size;
> +	memset(data_ex, 0, data_size);
> +	memcpy(fw_dest, data, size);
> +
> +	data_ex->address = __psp_pa(fw_dest);
> +	data_ex->len = size;
> +	data_ex->cmdlen = sizeof(struct sev_data_download_firmware_ex);
> +
> +	/*
> +	 * SNP_COMMIT should be issued explicitly to commit the updated
> +	 * firmware after guest context pages have been updated.
> +	 */
> +	ret = sev_do_cmd(SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX, data_ex, error);
> +

Remove blank line.

> +	if (ret)
> +		goto free_err;
> +
> +	__free_pages(p, order);

If you remove this...

> +
> +	/* Need to do a DF_FLUSH after live firmware update */
> +	wbinvd_on_all_cpus();
> +	ret = __sev_do_cmd_locked(SEV_CMD_SNP_DF_FLUSH, NULL, error);
> +	if (ret) {
> +		dev_dbg(sev->dev, "DF_FLUSH error %d\n", *error);
> +		goto fw_err;

and remove this...

> +	}
> +
> +	return 0;

... you can remove this return 0 statement and the fw_err label.

> +
> +free_err:
> +	__free_pages(p, order);
> +fw_err:
> +	return ret;
> +}
> +
>  static int __sev_snp_shutdown_locked(int *error, bool panic)
>  {
>  	struct psp_device *psp = psp_master;
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 903ddfea85850..6a08c56cd9771 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -16,6 +16,15 @@
>  
>  #define SEV_FW_BLOB_MAX_SIZE	0x4000	/* 16KB */
>  
> +/*
> + * SEV FW expects the physical address given to it to be 32
> + * byte aligned. Memory allocated has structure placed at the
> + * beginning followed by the firmware being passed to the SEV
> + * FW. Allocate enough memory for data structure + alignment
> + * padding + SEV FW.
> + */
> +#define SEV_FW_ALIGNMENT       32

Does this need to be in include/linux/psp-sev.h or can it go in
drivers/crypto/ccp/sev-dev.h ?

> +
>  /**
>   * SEV platform state
>   */
> @@ -185,6 +194,23 @@ struct sev_data_download_firmware {
>  	u32 len;				/* In */
>  } __packed;
>  
> +/**
> + * struct sev_data_download_firmware_ex - DOWNLOAD_FIRMWARE_EX command parameters
> + *
> + * @length: length of this command buffer
> + * @address: physical address of firmware image
> + * @len: len of the firmware image
> + * @commit: automatically commit the newly installed image
> + */
> +struct sev_data_download_firmware_ex {
> +	u32 cmdlen;				/* In */

This doesn't match the name above in the comment.

> +	u32 reserved;				/* In */
> +	u64 address;				/* In */
> +	u32 len;				/* In */
> +	u32 commit:1;				/* In */
> +	u32 reserved2:31;			/* In */

These names typically match what is in the spec, so I would expect to
see length, fw_paddr (or fw_address), and fw_len.

> +} __packed;
> +
>  /**
>   * struct sev_data_get_id - GET_ID command parameters
>   *
> diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
> index 832c15d9155bd..936464d4f282a 100644
> --- a/include/uapi/linux/psp-sev.h
> +++ b/include/uapi/linux/psp-sev.h
> @@ -80,6 +80,11 @@ typedef enum {
>  	SEV_RET_INVALID_PAGE_OWNER,
>  	SEV_RET_INVALID_PAGE_AEAD_OFLOW,
>  	SEV_RET_RMP_INIT_REQUIRED,
> +	SEV_RET_BAD_SVN,
> +	SEV_RET_BAD_VERSION,
> +	SEV_RET_SHUTDOWN_REQUIRED,
> +	SEV_RET_UPDATE_FAILED,
> +	SEV_RET_RESTORE_REQUIRED,

Hmmm... yeah, as Alexey mentioned, these return values are not correct.

SEV_RET_INVALID_PAGE_SIZE should follow SEV_RET_SECURE_DATA_INVALID

so "SEV_RET_INVALID_KEY = 0x27" should be moved after
SEV_RET_RMP_INIT_REQUIRED

and SEV_RET_RMP_INIT_REQUIRED should be = 0x20, since RB_MODE_EXITED is
0x1f.

And then you might as well include RMP_INIT_FAILED in the list.

The list should look like:

	...
	SEV_RET_SECURE_DATA_INVALID,
	SEV_RET_INVALID_PAGE_SIZE,
	SEV_RET_INVALID_PAGE_STATE,
	SEV_RET_INVALID_MDATA_ENTRY,
	SEV_RET_INVALID_PAGE_OWNER,
	SEV_RET_AEAD_OFLOW,
	SEV_RET_RB_MODE_EXITED,
	SEV_RET_RMP_INIT_REQUIRED,
	SEV_RET_BAD_SVN,
	SEV_RET_BAD_VERSION,
	SEV_RET_SHUTDOWN_REQUIRED,
	SEV_RET_UPDATE_FAILED,
	SEV_RET_RESTORE_REQUIRED,
	SEV_RET_RMP_INIT_FAILED,
	SEV_RET_INVALID_KEY,
	SEV_RET_MAX,

Of course this is already upstream in uapi, so I don't know how it is
supposed to be handled.

Thanks,
Tom

>  	SEV_RET_MAX,
>  } sev_ret_code;
>  

[PATCH v4 5/6] crypto: ccp: Use firmware_upload API for SNP firmware

[Dionna Glaze]

On init, the ccp device will make /sys/class/firmware/amd/loading etc
firmware upload API attributes available to late-load a SEV-SNP firmware
binary.

The firmware upload api errors reported are actionable in the following
ways:
* FW_UPLOAD_ERR_HW_ERROR: the machine is in an unstable state and must
  be reset.
* FW_UPLOAD_ERR_RW_ERROR: the firmware update went bad but can be
  recovered by hotloading the previous firmware version.
  Also used in the case that the kernel used the API wrong (bug).
* FW_UPLOAD_ERR_FW_INVALID: user error with the data provided, but no
  instability is expected and no recovery actions are needed.
* FW_UPLOAD_ERR_BUSY: upload attempted at a bad time either due to
  overload or the machine is in the wrong platform state.

synthetic_restore_required:
Instead of tracking the status of whether an individual GCTX is safe for
use in a firmware command, force all following commands to fail with an
error that is indicative of needing a firmware rollback.

To test:
1. Build the kernel enabling SEV-SNP as normal and add CONFIG_FW_UPLOAD=y.
2. Add the following to your kernel_cmdline: ccp.psp_init_on_probe=0.
3.Get an AMD SEV-SNP firmware sbin appropriate to your Epyc chip model at
https://www.amd.com/en/developer/sev.html and extract to get a .sbin
file.
4. Run the following with your sbinfile in FW:

echo 1 > /sys/class/firmware/snp_dlfw_ex/loading
cat "${FW?}" > /sys/class/firmware/snp_dlfw_ex/data
echo 0 > /sys/class/firmware/snp_dlfw_ex/loading

5. Verify the firmware update message in dmesg.

Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
Tested-by: Ashish Kalra <ashish.kalra@amd.com>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 drivers/crypto/ccp/Kconfig   |   2 +
 drivers/crypto/ccp/sev-dev.c |  12 +-
 drivers/crypto/ccp/sev-dev.h |  16 +++
 drivers/crypto/ccp/sev-fw.c  | 213 +++++++++++++++++++++++++++++++++++
 4 files changed, 241 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
index f394e45e11ab4..520b1c84d11f4 100644
--- a/drivers/crypto/ccp/Kconfig
+++ b/drivers/crypto/ccp/Kconfig
@@ -39,6 +39,8 @@ config CRYPTO_DEV_SP_PSP
 	bool "Platform Security Processor (PSP) device"
 	default y
 	depends on CRYPTO_DEV_CCP_DD && X86_64 && AMD_IOMMU
+	select FW_LOADER
+	select FW_UPLOAD
 	help
 	 Provide support for the AMD Platform Security Processor (PSP).
 	 The PSP is a dedicated processor that provides support for key
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 32f7b6147905e..8c73f023b6420 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -485,7 +485,7 @@ void snp_free_firmware_page(void *addr)
 }
 EXPORT_SYMBOL_GPL(snp_free_firmware_page);
 
-static void *sev_fw_alloc(unsigned long len)
+void *sev_fw_alloc(unsigned long len)
 {
 	struct page *page;
 
@@ -853,6 +853,10 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 	if (WARN_ON_ONCE(!data != !buf_len))
 		return -EINVAL;
 
+	ret = sev_snp_synthetic_error(sev, psp_ret);
+	if (ret)
+		return ret;
+
 	/*
 	 * Copy the incoming data to driver's scratch buffer as __pa() will not
 	 * work for some memory, e.g. vmalloc'd addresses, and @data may not be
@@ -1523,7 +1527,7 @@ void *psp_copy_user_blob(u64 uaddr, u32 len)
 }
 EXPORT_SYMBOL_GPL(psp_copy_user_blob);
 
-static int sev_get_api_version(void)
+int sev_get_api_version(void)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_user_data_status status;
@@ -2320,6 +2324,8 @@ int sev_dev_init(struct psp_device *psp)
 	if (ret)
 		goto e_irq;
 
+	sev_snp_dev_init_firmware_upload(sev);
+
 	dev_notice(dev, "sev enabled\n");
 
 	return 0;
@@ -2398,6 +2404,8 @@ void sev_dev_destroy(struct psp_device *psp)
 		kref_put(&misc_dev->refcount, sev_exit);
 
 	psp_clear_sev_irq_handler(psp);
+
+	sev_snp_dev_init_firmware_upload(sev);
 }
 
 static int snp_shutdown_on_panic(struct notifier_block *nb,
diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
index 28add34484ed1..52a423e7df84f 100644
--- a/drivers/crypto/ccp/sev-dev.h
+++ b/drivers/crypto/ccp/sev-dev.h
@@ -59,7 +59,13 @@ struct sev_device {
 	bool snp_initialized;
 
 #ifdef CONFIG_FW_UPLOAD
+	/* Lock to protect fw_cancel */
+	struct mutex fw_lock;
+	struct fw_upload *fwl;
+	bool fw_cancel;
+
 	u32 last_snp_asid;
+	bool synthetic_restore_required;
 	u64 *snp_asid_to_gctx_pages_map;
 	u64 *snp_unbound_gctx_pages;
 	u32 snp_unbound_gctx_end;
@@ -72,12 +78,22 @@ void sev_dev_destroy(struct psp_device *psp);
 void sev_pci_init(void);
 void sev_pci_exit(void);
 
+void *sev_fw_alloc(unsigned long len);
+int sev_get_api_version(void);
+int sev_snp_download_firmware_ex(struct sev_device *sev, const u8 *data, u32 size, int *error);
+
 #ifdef CONFIG_FW_UPLOAD
 void snp_cmd_bookkeeping_locked(int cmd, struct sev_device *sev, void *data);
 int sev_snp_platform_init_firmware_upload(struct sev_device *sev);
+void sev_snp_dev_init_firmware_upload(struct sev_device *sev);
+void sev_snp_destroy_firmware_upload(struct sev_device *sev);
+int sev_snp_synthetic_error(struct sev_device *sev, int *psp_ret);
 #else
 static inline void snp_cmd_bookkeeping_locked(int cmd, struct sev_device *sev, void *data) { }
 static inline int sev_snp_platform_init_firmware_upload(struct sev_device *sev) { return 0; }
+static inline void sev_snp_dev_init_firmware_upload(struct sev_device *sev) { }
+static inline void sev_snp_destroy_firmware_upload(struct sev_device *sev) { }
+static inline int sev_snp_synthetic_error(struct sev_device *sev, int *psp_ret) { return 0; }
 #endif /* CONFIG_FW_UPLOAD */
 
 #endif /* __SEV_DEV_H */
diff --git a/drivers/crypto/ccp/sev-fw.c b/drivers/crypto/ccp/sev-fw.c
index 55a5a572da8f1..97ce90f2af29a 100644
--- a/drivers/crypto/ccp/sev-fw.c
+++ b/drivers/crypto/ccp/sev-fw.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/firmware.h>
+#include <linux/psp.h>
 #include <linux/psp-sev.h>
 
 #include <asm/sev.h>
@@ -115,3 +116,215 @@ int sev_snp_platform_init_firmware_upload(struct sev_device *sev)
 	}
 	return 0;
 }
+
+static enum fw_upload_err snp_dlfw_ex_prepare(struct fw_upload *fw_upload,
+					      const u8 *data, u32 size)
+{
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static enum fw_upload_err snp_dlfw_ex_poll_complete(struct fw_upload *fw_upload)
+{
+	return FW_UPLOAD_ERR_NONE;
+}
+
+/*
+ * This may be called asynchronously with an on-going update.  All other
+ * functions are called sequentially in a single thread. To avoid contention on
+ * register accesses, only update the cancel_request flag. Other functions will
+ * check this flag and handle the cancel request synchronously.
+ */
+static void snp_dlfw_ex_cancel(struct fw_upload *fw_upload)
+{
+	struct sev_device *sev = fw_upload->dd_handle;
+
+	mutex_lock(&sev->fw_lock);
+	sev->fw_cancel = true;
+	mutex_unlock(&sev->fw_lock);
+}
+
+static enum fw_upload_err snp_dlfw_ex_err_translate(struct sev_device *sev, int psp_ret)
+{
+	dev_dbg(sev->dev, "Failed to update SEV firmware: %#x\n", psp_ret);
+	/*
+	 * Operation error:
+	 *   HW_ERROR: Critical error. Machine needs repairs now.
+	 *   RW_ERROR: Severe error. Roll back to the prior version to recover.
+	 * User error:
+	 *   FW_INVALID: Bad input for this interface.
+	 *   BUSY: Wrong machine state to run download_firmware_ex.
+	 */
+	switch (psp_ret) {
+	case SEV_RET_RESTORE_REQUIRED:
+		dev_warn(sev->dev, "Firmware updated but unusable\n");
+		dev_warn(sev->dev, "Need to do manual firmware rollback!!!\n");
+		return FW_UPLOAD_ERR_RW_ERROR;
+	case SEV_RET_SHUTDOWN_REQUIRED:
+		/* No state changes made. Not a hardware error. */
+		dev_warn(sev->dev, "Firmware image cannot be live updated\n");
+		return FW_UPLOAD_ERR_FW_INVALID;
+	case SEV_RET_BAD_VERSION:
+		/* No state changes made. Not a hardware error. */
+		dev_warn(sev->dev, "Firmware image is not well formed\n");
+		return FW_UPLOAD_ERR_FW_INVALID;
+		/* SEV-specific errors that can still happen. */
+	case SEV_RET_BAD_SIGNATURE:
+		/* No state changes made. Not a hardware error. */
+		dev_warn(sev->dev, "Firmware image signature is bad\n");
+		return FW_UPLOAD_ERR_FW_INVALID;
+	case SEV_RET_INVALID_PLATFORM_STATE:
+		/* Calling at the wrong time. Not a hardware error. */
+		dev_warn(sev->dev, "Firmware not updated as SEV in INIT state\n");
+		return FW_UPLOAD_ERR_BUSY;
+	case SEV_RET_HWSEV_RET_UNSAFE:
+		dev_err(sev->dev, "Firmware is unstable. Reset your machine!!!\n");
+		return FW_UPLOAD_ERR_HW_ERROR;
+		/* Kernel bug cases. */
+	case SEV_RET_INVALID_PARAM:
+		dev_err(sev->dev, "Download-firmware-EX invalid parameter\n");
+		return FW_UPLOAD_ERR_RW_ERROR;
+	case SEV_RET_INVALID_ADDRESS:
+		dev_err(sev->dev, "Download-firmware-EX invalid address\n");
+		return FW_UPLOAD_ERR_RW_ERROR;
+	default:
+		dev_err(sev->dev, "Unhandled download_firmware_ex err %d\n", psp_ret);
+		return FW_UPLOAD_ERR_HW_ERROR;
+	}
+}
+
+static enum fw_upload_err snp_update_guest_statuses(struct sev_device *sev)
+{
+	struct sev_data_snp_guest_status status_data;
+	void *snp_guest_status;
+	enum fw_upload_err ret;
+	int error;
+
+	/*
+	 * Force an update of guest context pages after SEV firmware
+	 * live update by issuing SNP_GUEST_STATUS on all guest
+	 * context pages.
+	 */
+	snp_guest_status = sev_fw_alloc(PAGE_SIZE);
+	if (!snp_guest_status)
+		return FW_UPLOAD_ERR_INVALID_SIZE;
+
+	/*
+	 * After the last bound asid-to-gctx page is snp_unbound_gctx_end-many
+	 * unbound gctx pages that also need updating.
+	 */
+	for (int i = 1; i <= sev->last_snp_asid + sev->snp_unbound_gctx_end; i++) {
+		if (sev->snp_asid_to_gctx_pages_map[i]) {
+			status_data.gctx_paddr = sev->snp_asid_to_gctx_pages_map[i];
+			status_data.address = __psp_pa(snp_guest_status);
+			ret = sev_do_cmd(SEV_CMD_SNP_GUEST_STATUS, &status_data, &error);
+			if (ret) {
+				/*
+				 * Handle race with SNP VM being destroyed/decommissoned,
+				 * if guest context page invalid error is returned,
+				 * assume guest has been destroyed.
+				 */
+				if (error == SEV_RET_INVALID_GUEST)
+					continue;
+				sev->synthetic_restore_required = true;
+				dev_err(sev->dev, "SNP GCTX update error: %#x\n", error);
+				dev_err(sev->dev, "Roll back SNP firmware!\n");
+				snp_free_firmware_page(snp_guest_status);
+				ret = FW_UPLOAD_ERR_RW_ERROR;
+				goto fw_err;
+			}
+		}
+	}
+fw_err:
+	snp_free_firmware_page(snp_guest_status);
+	return ret;
+}
+
+static enum fw_upload_err snp_dlfw_ex_write(struct fw_upload *fwl, const u8 *data,
+					    u32 offset, u32 size, u32 *written)
+{
+	struct sev_device *sev = fwl->dd_handle;
+	u8 api_major, api_minor, build;
+	int ret, error;
+	bool cancel;
+
+	if (!sev)
+		return FW_UPLOAD_ERR_HW_ERROR;
+
+	mutex_lock(&sev->fw_lock);
+	cancel = sev->fw_cancel;
+	mutex_unlock(&sev->fw_lock);
+
+	if (cancel)
+		return FW_UPLOAD_ERR_CANCELED;
+
+	/*
+	 * SEV firmware update is a one-shot update operation, the write()
+	 * callback to be invoked multiple times for the same update is
+	 * unexpected.
+	 */
+	if (offset)
+		return FW_UPLOAD_ERR_INVALID_SIZE;
+
+	if (sev_get_api_version())
+		return FW_UPLOAD_ERR_HW_ERROR;
+
+	api_major = sev->api_major;
+	api_minor = sev->api_minor;
+	build     = sev->build;
+
+	ret = sev_snp_download_firmware_ex(sev, data, size, &error);
+	if (ret)
+		return snp_dlfw_ex_err_translate(sev, error);
+
+	ret = snp_update_guest_statuses(sev);
+	if (ret)
+		return ret;
+
+	sev_get_api_version();
+	if (api_major != sev->api_major || api_minor != sev->api_minor ||
+	    build != sev->build) {
+		dev_info(sev->dev, "SEV firmware updated from %d.%d.%d to %d.%d.%d\n",
+			 api_major, api_minor, build,
+			 sev->api_major, sev->api_minor, sev->build);
+	} else {
+		dev_info(sev->dev, "SEV firmware same as old %d.%d.%d\n",
+			 api_major, api_minor, build);
+	}
+
+	*written = size;
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static const struct fw_upload_ops snp_dlfw_ex_ops = {
+	.prepare = snp_dlfw_ex_prepare,
+	.write = snp_dlfw_ex_write,
+	.poll_complete = snp_dlfw_ex_poll_complete,
+	.cancel = snp_dlfw_ex_cancel,
+};
+
+void sev_snp_dev_init_firmware_upload(struct sev_device *sev)
+{
+	struct fw_upload *fwl;
+
+	fwl = firmware_upload_register(THIS_MODULE, sev->dev, "snp_dlfw_ex", &snp_dlfw_ex_ops, sev);
+
+	if (IS_ERR(fwl))
+		dev_err(sev->dev, "SEV firmware upload initialization error %ld\n", PTR_ERR(fwl));
+
+	sev->fwl = fwl;
+	mutex_init(&sev->fw_lock);
+}
+
+void sev_snp_destroy_firmware_upload(struct sev_device *sev)
+{
+	firmware_upload_unregister(sev->fwl);
+}
+
+int sev_snp_synthetic_error(struct sev_device *sev, int *psp_ret)
+{
+	if (sev->synthetic_restore_required) {
+		*psp_ret = SEV_RET_RESTORE_REQUIRED;
+		return -EIO;
+	}
+	return 0;
+}
-- 
2.47.0.199.ga7371fff76-goog

Re: [PATCH v4 5/6] crypto: ccp: Use firmware_upload API for SNP firmware

[Tom Lendacky]

On 11/4/24 19:05, Dionna Glaze wrote:
> On init, the ccp device will make /sys/class/firmware/amd/loading etc
> firmware upload API attributes available to late-load a SEV-SNP firmware
> binary.
> 
> The firmware upload api errors reported are actionable in the following
> ways:
> * FW_UPLOAD_ERR_HW_ERROR: the machine is in an unstable state and must
>   be reset.
> * FW_UPLOAD_ERR_RW_ERROR: the firmware update went bad but can be
>   recovered by hotloading the previous firmware version.
>   Also used in the case that the kernel used the API wrong (bug).
> * FW_UPLOAD_ERR_FW_INVALID: user error with the data provided, but no
>   instability is expected and no recovery actions are needed.
> * FW_UPLOAD_ERR_BUSY: upload attempted at a bad time either due to
>   overload or the machine is in the wrong platform state.
> 
> synthetic_restore_required:
> Instead of tracking the status of whether an individual GCTX is safe for
> use in a firmware command, force all following commands to fail with an
> error that is indicative of needing a firmware rollback.
> 
> To test:
> 1. Build the kernel enabling SEV-SNP as normal and add CONFIG_FW_UPLOAD=y.
> 2. Add the following to your kernel_cmdline: ccp.psp_init_on_probe=0.
> 3.Get an AMD SEV-SNP firmware sbin appropriate to your Epyc chip model at
> https://www.amd.com/en/developer/sev.html and extract to get a .sbin
> file.
> 4. Run the following with your sbinfile in FW:
> 
> echo 1 > /sys/class/firmware/snp_dlfw_ex/loading
> cat "${FW?}" > /sys/class/firmware/snp_dlfw_ex/data
> echo 0 > /sys/class/firmware/snp_dlfw_ex/loading
> 
> 5. Verify the firmware update message in dmesg.
> 
> Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> Tested-by: Ashish Kalra <ashish.kalra@amd.com>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  drivers/crypto/ccp/Kconfig   |   2 +
>  drivers/crypto/ccp/sev-dev.c |  12 +-
>  drivers/crypto/ccp/sev-dev.h |  16 +++
>  drivers/crypto/ccp/sev-fw.c  | 213 +++++++++++++++++++++++++++++++++++
>  4 files changed, 241 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
> index f394e45e11ab4..520b1c84d11f4 100644
> --- a/drivers/crypto/ccp/Kconfig
> +++ b/drivers/crypto/ccp/Kconfig
> @@ -39,6 +39,8 @@ config CRYPTO_DEV_SP_PSP
>  	bool "Platform Security Processor (PSP) device"
>  	default y
>  	depends on CRYPTO_DEV_CCP_DD && X86_64 && AMD_IOMMU
> +	select FW_LOADER
> +	select FW_UPLOAD
>  	help
>  	 Provide support for the AMD Platform Security Processor (PSP).
>  	 The PSP is a dedicated processor that provides support for key
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 32f7b6147905e..8c73f023b6420 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -485,7 +485,7 @@ void snp_free_firmware_page(void *addr)
>  }
>  EXPORT_SYMBOL_GPL(snp_free_firmware_page);
>  
> -static void *sev_fw_alloc(unsigned long len)
> +void *sev_fw_alloc(unsigned long len)
>  {
>  	struct page *page;
>  
> @@ -853,6 +853,10 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>  	if (WARN_ON_ONCE(!data != !buf_len))
>  		return -EINVAL;
>  
> +	ret = sev_snp_synthetic_error(sev, psp_ret);

A comment similar to what you have in the commit log would be nice above
this call.

> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * Copy the incoming data to driver's scratch buffer as __pa() will not
>  	 * work for some memory, e.g. vmalloc'd addresses, and @data may not be
> @@ -1523,7 +1527,7 @@ void *psp_copy_user_blob(u64 uaddr, u32 len)
>  }
>  EXPORT_SYMBOL_GPL(psp_copy_user_blob);
>  
> -static int sev_get_api_version(void)
> +int sev_get_api_version(void)
>  {
>  	struct sev_device *sev = psp_master->sev_data;
>  	struct sev_user_data_status status;
> @@ -2320,6 +2324,8 @@ int sev_dev_init(struct psp_device *psp)
>  	if (ret)
>  		goto e_irq;
>  
> +	sev_snp_dev_init_firmware_upload(sev);
> +
>  	dev_notice(dev, "sev enabled\n");
>  
>  	return 0;
> @@ -2398,6 +2404,8 @@ void sev_dev_destroy(struct psp_device *psp)
>  		kref_put(&misc_dev->refcount, sev_exit);
>  
>  	psp_clear_sev_irq_handler(psp);
> +
> +	sev_snp_dev_init_firmware_upload(sev);

Shouldn't this be the destroy call?

>  }
>  
>  static int snp_shutdown_on_panic(struct notifier_block *nb,
> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> index 28add34484ed1..52a423e7df84f 100644
> --- a/drivers/crypto/ccp/sev-dev.h
> +++ b/drivers/crypto/ccp/sev-dev.h
> @@ -59,7 +59,13 @@ struct sev_device {
>  	bool snp_initialized;
>  
>  #ifdef CONFIG_FW_UPLOAD
> +	/* Lock to protect fw_cancel */
> +	struct mutex fw_lock;
> +	struct fw_upload *fwl;
> +	bool fw_cancel;
> +
>  	u32 last_snp_asid;
> +	bool synthetic_restore_required;
>  	u64 *snp_asid_to_gctx_pages_map;
>  	u64 *snp_unbound_gctx_pages;
>  	u32 snp_unbound_gctx_end;
> @@ -72,12 +78,22 @@ void sev_dev_destroy(struct psp_device *psp);
>  void sev_pci_init(void);
>  void sev_pci_exit(void);
>  
> +void *sev_fw_alloc(unsigned long len);
> +int sev_get_api_version(void);
> +int sev_snp_download_firmware_ex(struct sev_device *sev, const u8 *data, u32 size, int *error);
> +
>  #ifdef CONFIG_FW_UPLOAD
>  void snp_cmd_bookkeeping_locked(int cmd, struct sev_device *sev, void *data);
>  int sev_snp_platform_init_firmware_upload(struct sev_device *sev);
> +void sev_snp_dev_init_firmware_upload(struct sev_device *sev);
> +void sev_snp_destroy_firmware_upload(struct sev_device *sev);
> +int sev_snp_synthetic_error(struct sev_device *sev, int *psp_ret);
>  #else
>  static inline void snp_cmd_bookkeeping_locked(int cmd, struct sev_device *sev, void *data) { }
>  static inline int sev_snp_platform_init_firmware_upload(struct sev_device *sev) { return 0; }
> +static inline void sev_snp_dev_init_firmware_upload(struct sev_device *sev) { }
> +static inline void sev_snp_destroy_firmware_upload(struct sev_device *sev) { }
> +static inline int sev_snp_synthetic_error(struct sev_device *sev, int *psp_ret) { return 0; }
>  #endif /* CONFIG_FW_UPLOAD */
>  
>  #endif /* __SEV_DEV_H */
> diff --git a/drivers/crypto/ccp/sev-fw.c b/drivers/crypto/ccp/sev-fw.c
> index 55a5a572da8f1..97ce90f2af29a 100644
> --- a/drivers/crypto/ccp/sev-fw.c
> +++ b/drivers/crypto/ccp/sev-fw.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <linux/firmware.h>
> +#include <linux/psp.h>
>  #include <linux/psp-sev.h>
>  
>  #include <asm/sev.h>
> @@ -115,3 +116,215 @@ int sev_snp_platform_init_firmware_upload(struct sev_device *sev)
>  	}
>  	return 0;
>  }
> +
> +static enum fw_upload_err snp_dlfw_ex_prepare(struct fw_upload *fw_upload,
> +					      const u8 *data, u32 size)
> +{
> +	return FW_UPLOAD_ERR_NONE;
> +}
> +
> +static enum fw_upload_err snp_dlfw_ex_poll_complete(struct fw_upload *fw_upload)
> +{
> +	return FW_UPLOAD_ERR_NONE;
> +}
> +
> +/*
> + * This may be called asynchronously with an on-going update.  All other
> + * functions are called sequentially in a single thread. To avoid contention on
> + * register accesses, only update the cancel_request flag. Other functions will
> + * check this flag and handle the cancel request synchronously.
> + */
> +static void snp_dlfw_ex_cancel(struct fw_upload *fw_upload)
> +{
> +	struct sev_device *sev = fw_upload->dd_handle;
> +
> +	mutex_lock(&sev->fw_lock);
> +	sev->fw_cancel = true;
> +	mutex_unlock(&sev->fw_lock);

Does this really need a lock? Also, I don't see it being set to false
ever, so the first time an update is canceled it can never be updated again.

> +}
> +
> +static enum fw_upload_err snp_dlfw_ex_err_translate(struct sev_device *sev, int psp_ret)
> +{
> +	dev_dbg(sev->dev, "Failed to update SEV firmware: %#x\n", psp_ret);

Blank line.

> +	/*
> +	 * Operation error:
> +	 *   HW_ERROR: Critical error. Machine needs repairs now.
> +	 *   RW_ERROR: Severe error. Roll back to the prior version to recover.
> +	 * User error:
> +	 *   FW_INVALID: Bad input for this interface.
> +	 *   BUSY: Wrong machine state to run download_firmware_ex.
> +	 */
> +	switch (psp_ret) {
> +	case SEV_RET_RESTORE_REQUIRED:
> +		dev_warn(sev->dev, "Firmware updated but unusable\n");
> +		dev_warn(sev->dev, "Need to do manual firmware rollback!!!\n");

Please keep to a single message so that other messages don't get in
between when outputted.

> +		return FW_UPLOAD_ERR_RW_ERROR;
> +	case SEV_RET_SHUTDOWN_REQUIRED:
> +		/* No state changes made. Not a hardware error. */
> +		dev_warn(sev->dev, "Firmware image cannot be live updated\n");
> +		return FW_UPLOAD_ERR_FW_INVALID;
> +	case SEV_RET_BAD_VERSION:
> +		/* No state changes made. Not a hardware error. */
> +		dev_warn(sev->dev, "Firmware image is not well formed\n");
> +		return FW_UPLOAD_ERR_FW_INVALID;
> +		/* SEV-specific errors that can still happen. */
> +	case SEV_RET_BAD_SIGNATURE:
> +		/* No state changes made. Not a hardware error. */
> +		dev_warn(sev->dev, "Firmware image signature is bad\n");
> +		return FW_UPLOAD_ERR_FW_INVALID;
> +	case SEV_RET_INVALID_PLATFORM_STATE:
> +		/* Calling at the wrong time. Not a hardware error. */
> +		dev_warn(sev->dev, "Firmware not updated as SEV in INIT state\n");
> +		return FW_UPLOAD_ERR_BUSY;
> +	case SEV_RET_HWSEV_RET_UNSAFE:
> +		dev_err(sev->dev, "Firmware is unstable. Reset your machine!!!\n");
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +		/* Kernel bug cases. */
> +	case SEV_RET_INVALID_PARAM:
> +		dev_err(sev->dev, "Download-firmware-EX invalid parameter\n");
> +		return FW_UPLOAD_ERR_RW_ERROR;
> +	case SEV_RET_INVALID_ADDRESS:
> +		dev_err(sev->dev, "Download-firmware-EX invalid address\n");
> +		return FW_UPLOAD_ERR_RW_ERROR;
> +	default:
> +		dev_err(sev->dev, "Unhandled download_firmware_ex err %d\n", psp_ret);
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +	}
> +}
> +
> +static enum fw_upload_err snp_update_guest_statuses(struct sev_device *sev)

snp_update_guest_contexts

> +{
> +	struct sev_data_snp_guest_status status_data;
> +	void *snp_guest_status;
> +	enum fw_upload_err ret;
> +	int error;
> +
> +	/*
> +	 * Force an update of guest context pages after SEV firmware
> +	 * live update by issuing SNP_GUEST_STATUS on all guest
> +	 * context pages.
> +	 */
> +	snp_guest_status = sev_fw_alloc(PAGE_SIZE);
> +	if (!snp_guest_status)
> +		return FW_UPLOAD_ERR_INVALID_SIZE;
> +
> +	/*
> +	 * After the last bound asid-to-gctx page is snp_unbound_gctx_end-many
> +	 * unbound gctx pages that also need updating.
> +	 */
> +	for (int i = 1; i <= sev->last_snp_asid + sev->snp_unbound_gctx_end; i++) {
> +		if (sev->snp_asid_to_gctx_pages_map[i]) {

Let reduce some indentation and do

		if (!sev->snp_asid_to_gctx_pages_map[i])
			continue;

> +			status_data.gctx_paddr = sev->snp_asid_to_gctx_pages_map[i];
> +			status_data.address = __psp_pa(snp_guest_status);
> +			ret = sev_do_cmd(SEV_CMD_SNP_GUEST_STATUS, &status_data, &error);

... and
		if (!ret)
			continue;

> +			if (ret) {
> +				/*
> +				 * Handle race with SNP VM being destroyed/decommissoned,
> +				 * if guest context page invalid error is returned,
> +				 * assume guest has been destroyed.
> +				 */
> +				if (error == SEV_RET_INVALID_GUEST)
> +					continue;

Add a blank line here.

And add a comment here about synthetic_restore_required being set.

> +				sev->synthetic_restore_required = true;
> +				dev_err(sev->dev, "SNP GCTX update error: %#x\n", error);
> +				dev_err(sev->dev, "Roll back SNP firmware!\n");

Single message, something like:

"SNP guest context update error %#x, recommend SNP firmware roll back."

> +				snp_free_firmware_page(snp_guest_status);

This is done in the goto below, so must be removed.

> +				ret = FW_UPLOAD_ERR_RW_ERROR;
> +				goto fw_err;
> +			}
> +		}
> +	}
> +fw_err:
> +	snp_free_firmware_page(snp_guest_status);
> +	return ret;
> +}
> +
> +static enum fw_upload_err snp_dlfw_ex_write(struct fw_upload *fwl, const u8 *data,
> +					    u32 offset, u32 size, u32 *written)
> +{
> +	struct sev_device *sev = fwl->dd_handle;
> +	u8 api_major, api_minor, build;
> +	int ret, error;
> +	bool cancel;
> +
> +	if (!sev)
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +
> +	mutex_lock(&sev->fw_lock);
> +	cancel = sev->fw_cancel;
> +	mutex_unlock(&sev->fw_lock);

Same question on the mutex, I don't see how this helps with any timing.

> +
> +	if (cancel)
> +		return FW_UPLOAD_ERR_CANCELED;
> +
> +	/*
> +	 * SEV firmware update is a one-shot update operation, the write()
> +	 * callback to be invoked multiple times for the same update is
> +	 * unexpected.
> +	 */
> +	if (offset)
> +		return FW_UPLOAD_ERR_INVALID_SIZE;
> +
> +	if (sev_get_api_version())
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +
> +	api_major = sev->api_major;
> +	api_minor = sev->api_minor;
> +	build     = sev->build;
> +
> +	ret = sev_snp_download_firmware_ex(sev, data, size, &error);
> +	if (ret)
> +		return snp_dlfw_ex_err_translate(sev, error);
> +
> +	ret = snp_update_guest_statuses(sev);
> +	if (ret)
> +		return ret;
> +
> +	sev_get_api_version();
> +	if (api_major != sev->api_major || api_minor != sev->api_minor ||
> +	    build != sev->build) {
> +		dev_info(sev->dev, "SEV firmware updated from %d.%d.%d to %d.%d.%d\n",
> +			 api_major, api_minor, build,
> +			 sev->api_major, sev->api_minor, sev->build);
> +	} else {
> +		dev_info(sev->dev, "SEV firmware same as old %d.%d.%d\n",
> +			 api_major, api_minor, build);

Maybe something like:
"SEV firmware not updated, same as current version %d.%d.%d"

> +	}
> +
> +	*written = size;

Blank line.

> +	return FW_UPLOAD_ERR_NONE;
> +}
> +
> +static const struct fw_upload_ops snp_dlfw_ex_ops = {
> +	.prepare = snp_dlfw_ex_prepare,
> +	.write = snp_dlfw_ex_write,
> +	.poll_complete = snp_dlfw_ex_poll_complete,
> +	.cancel = snp_dlfw_ex_cancel,
> +};
> +
> +void sev_snp_dev_init_firmware_upload(struct sev_device *sev)

snp_init_firmware_upload

> +{
> +	struct fw_upload *fwl;
> +
> +	fwl = firmware_upload_register(THIS_MODULE, sev->dev, "snp_dlfw_ex", &snp_dlfw_ex_ops, sev);
> +

Remove blank line.

> +	if (IS_ERR(fwl))
> +		dev_err(sev->dev, "SEV firmware upload initialization error %ld\n", PTR_ERR(fwl));
> +
> +	sev->fwl = fwl;
> +	mutex_init(&sev->fw_lock);

Probably can't happen, but shouldn't the mutex be initialized before
calling firmware_upload_register()?

> +}
> +
> +void sev_snp_destroy_firmware_upload(struct sev_device *sev)

snp_destroy_firmware_upload

> +{
> +	firmware_upload_unregister(sev->fwl);

Should this check that the sev->fwl is valid before trying to call
firmware_upload_unregister()?

> +}
> +
> +int sev_snp_synthetic_error(struct sev_device *sev, int *psp_ret)
> +{
> +	if (sev->synthetic_restore_required) {
> +		*psp_ret = SEV_RET_RESTORE_REQUIRED;
> +		return -EIO;
> +	}

Blank line.

Thanks,
Tom

> +	return 0;
> +}

[PATCH v4 6/6] KVM: SVM: Delay legacy platform initialization on SNP

[Dionna Glaze]

When no SEV or SEV-ES guests are active, then the firmware can be
updated while (SEV-SNP) VM guests are active.

CC: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Dave Hansen <dave.hansen@linux.intel.com>
CC: Ashish Kalra <ashish.kalra@amd.com>
CC: Tom Lendacky <thomas.lendacky@amd.com>
CC: John Allen <john.allen@amd.com>
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: "David S. Miller" <davem@davemloft.net>
CC: Michael Roth <michael.roth@amd.com>
CC: Luis Chamberlain <mcgrof@kernel.org>
CC: Russ Weight <russ.weight@linux.dev>
CC: Danilo Krummrich <dakr@redhat.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>
CC: Tianfei zhang <tianfei.zhang@intel.com>
CC: Alexey Kardashevskiy <aik@amd.com>

Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
Reviewed-by: Ashish Kalra <ashish.kalra@amd.com>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 arch/x86/kvm/svm/sev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index f6e96ec0a5caa..3c2079ba7c76f 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -444,7 +444,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (ret)
 		goto e_no_asid;
 
-	init_args.probe = false;
+	init_args.probe = vm_type != KVM_X86_SEV_VM && vm_type != KVM_X86_SEV_ES_VM;
 	ret = sev_platform_init(&init_args);
 	if (ret)
 		goto e_free;
-- 
2.47.0.199.ga7371fff76-goog

Re: [PATCH v4 6/6] KVM: SVM: Delay legacy platform initialization on SNP

[Tom Lendacky]

On 11/4/24 19:05, Dionna Glaze wrote:
> When no SEV or SEV-ES guests are active, then the firmware can be
> updated while (SEV-SNP) VM guests are active.
> 
> CC: Sean Christopherson <seanjc@google.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: Dave Hansen <dave.hansen@linux.intel.com>
> CC: Ashish Kalra <ashish.kalra@amd.com>
> CC: Tom Lendacky <thomas.lendacky@amd.com>
> CC: John Allen <john.allen@amd.com>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Michael Roth <michael.roth@amd.com>
> CC: Luis Chamberlain <mcgrof@kernel.org>
> CC: Russ Weight <russ.weight@linux.dev>
> CC: Danilo Krummrich <dakr@redhat.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: "Rafael J. Wysocki" <rafael@kernel.org>
> CC: Tianfei zhang <tianfei.zhang@intel.com>
> CC: Alexey Kardashevskiy <aik@amd.com>
> 
> Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> Reviewed-by: Ashish Kalra <ashish.kalra@amd.com>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  arch/x86/kvm/svm/sev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index f6e96ec0a5caa..3c2079ba7c76f 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -444,7 +444,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>  	if (ret)
>  		goto e_no_asid;
>  
> -	init_args.probe = false;
> +	init_args.probe = vm_type != KVM_X86_SEV_VM && vm_type != KVM_X86_SEV_ES_VM;

Add a comment above the setting of probe to indicate the intent of doing
this. Otherwise you have to go to the ccp code to understand what is
happening.

Thanks,
Tom

>  	ret = sev_platform_init(&init_args);
>  	if (ret)
>  		goto e_free;