Re: [PATCH v5 05/10] crypto: ccp: Add GCTX API to track ASID assignment

Dionna Amalie Glaze
2024-11-11T21:35:29Z

On Mon, Nov 11, 2024 at 1:16 PM Kalra, Ashish <ashish.kalra@amd.com> wrote:
>
>
>
> On 11/7/2024 5:24 PM, Dionna Glaze wrote:
> > In preparation for SEV firmware hotloading support, introduce a new way
> > to create, activate, and decommission GCTX pages such that ccp is has
> > all GCTX pages available to update as needed.
> >
> > 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 0x8000001f_EDX-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.
> >
> > 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>
> > ---
> > drivers/crypto/ccp/sev-dev.c | 107 +++++++++++++++++++++++++++++++++++
> > drivers/crypto/ccp/sev-dev.h | 8 +++
> > include/linux/psp-sev.h | 52 +++++++++++++++++
> > 3 files changed, 167 insertions(+)
> >
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index af018afd9cd7f..036e8d5054fcc 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -109,6 +109,10 @@ static void *sev_init_ex_buffer;
> > */
> > static struct sev_data_range_list *snp_range_list;
> >
> > +/* SEV ASID data tracks resources associated with an ASID to safely manage operations. */
> > +struct sev_asid_data *sev_asid_data;
> > +u32 nr_asids, sev_min_asid, sev_max_asid, sev_es_max_asid;
>
> This looks to be duplication of ASID management variables and support in KVM.
>

Agreed, though there will be duplication until all of the replacement
is ready and KVM can swap over.

> Probably this stuff needs to be merged with the ASID refactoring work being done to
> move all SEV/SNP ASID allocation/management stuff to CCP from KVM.
>

Who's doing that work? I'm not clear on timelines either. If it's
currently underway, do you see a rebase on this patch set as
particularly challenging?
I wouldn't want to block hotloading support until it's all ready.

> > +
> > static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
> > {
> > struct sev_device *sev = psp_master->sev_data;
> > @@ -1093,6 +1097,81 @@ static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
> > return 0;
> > }
> >
> > +void *sev_snp_create_context(int asid, int *psp_ret)
> > +{
> > + struct sev_data_snp_addr data = {};
> > + void *context;
> > + int rc;
> > +
> > + if (!sev_asid_data)
> > + return ERR_PTR(-ENODEV);
> > +
> > + /* Can't create a context for a used ASID. */
> > + if (sev_asid_data[asid].snp_context)
> > + return ERR_PTR(-EBUSY);
> > +
> > + /* Allocate memory for context page */
> > + context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
> > + if (!context)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + data.address = __psp_pa(context);
> > + rc = sev_do_cmd(SEV_CMD_SNP_GCTX_CREATE, &data, psp_ret);
> > + if (rc) {
> > + pr_warn("Failed to create SEV-SNP context, rc %d fw_error %d",
> > + rc, *psp_ret);
> > + snp_free_firmware_page(context);
> > + return ERR_PTR(-EIO);
> > + }
> > +
> > + sev_asid_data[asid].snp_context = context;
> > +
> > + return context;
> > +}
> > +
> > +int sev_snp_activate_asid(int asid, int *psp_ret)
> > +{
> > + struct sev_data_snp_activate data = {0};
> > + void *context;
> > +
> > + if (!sev_asid_data)
> > + return -ENODEV;
> > +
> > + context = sev_asid_data[asid].snp_context;
> > + if (!context)
> > + return -EINVAL;
> > +
> > + data.gctx_paddr = __psp_pa(context);
> > + data.asid = asid;
> > + return sev_do_cmd(SEV_CMD_SNP_ACTIVATE, &data, psp_ret);
> > +}
> > +
> > +int sev_snp_guest_decommission(int asid, int *psp_ret)
> > +{
> > + struct sev_data_snp_addr addr = {};
> > + struct sev_asid_data *data = &sev_asid_data[asid];
> > + int ret;
> > +
> > + if (!sev_asid_data)
> > + return -ENODEV;
> > +
> > + /* If context is not created then do nothing */
> > + if (!data->snp_context)
> > + return 0;
> > +
> > + /* Do the decommision, which will unbind the ASID from the SNP context */
> > + addr.address = __sme_pa(data->snp_context);
> > + ret = sev_do_cmd(SEV_CMD_SNP_DECOMMISSION, &addr, NULL);
> > +
> > + if (WARN_ONCE(ret, "Failed to release guest context, ret %d", ret))
> > + return ret;
> > +
> > + snp_free_firmware_page(data->snp_context);
> > + data->snp_context = NULL;
> > +
> > + return 0;
> > +}
> > +
> > static int __sev_snp_init_locked(int *error)
> > {
> > struct psp_device *psp = psp_master;
> > @@ -1306,6 +1385,27 @@ static int __sev_platform_init_locked(int *error)
> > return 0;
> > }
> >
> > +static int __sev_asid_data_init(void)
> > +{
> > + u32 eax, ebx;
> > +
> > + if (sev_asid_data)
> > + return 0;
> > +
> > + cpuid(0x8000001f, &eax, &ebx, &sev_max_asid, &sev_min_asid);
> > + if (!sev_max_asid)
> > + return -ENODEV;
> > +
> > + nr_asids = sev_max_asid + 1;
> > + sev_es_max_asid = sev_min_asid - 1;
> > +
> > + sev_asid_data = kcalloc(nr_asids, sizeof(*sev_asid_data), GFP_KERNEL);
> > + if (!sev_asid_data)
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
>
> Again, looks to be duplicating ASID setup code in sev_hardware_setup() (in KVM),
> maybe all this should be part of the ASID refactoring work to move all SEV/SNP
> ASID code to CCP from KVM module, that should then really streamline all ASID/GCTX
> tracking.
>
> Thanks,
> Ashish
>
> > +
> > static int _sev_platform_init_locked(struct sev_platform_init_args *args)
> > {
> > struct sev_device *sev;
> > @@ -1319,6 +1419,10 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args)
> > if (sev->state == SEV_STATE_INIT)
> > return 0;
> >
> > + rc = __sev_asid_data_init();
> > + if (rc)
> > + return rc;
> > +
> > /*
> > * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
> > * so perform SEV-SNP initialization at probe time.
> > @@ -2329,6 +2433,9 @@ static void __sev_firmware_shutdown(struct sev_device *sev, bool panic)
> > snp_range_list = NULL;
> > }
> >
> > + kfree(sev_asid_data);
> > + sev_asid_data = NULL;
> > +
> > __sev_snp_shutdown_locked(&error, panic);
> > }
> >
> > diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> > index 3e4e5574e88a3..7d0fdfdda30b6 100644
> > --- a/drivers/crypto/ccp/sev-dev.h
> > +++ b/drivers/crypto/ccp/sev-dev.h
> > @@ -65,4 +65,12 @@ void sev_dev_destroy(struct psp_device *psp);
> > void sev_pci_init(void);
> > void sev_pci_exit(void);
> >
> > +struct sev_asid_data {
> > + void *snp_context;
> > +};
> > +
> > +/* Extern to be shared with firmware_upload API implementation if configured. */
> > +extern struct sev_asid_data *sev_asid_data;
> > +extern u32 nr_asids, sev_min_asid, sev_max_asid, sev_es_max_asid;
> > +
> > #endif /* __SEV_DEV_H */
> > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> > index 903ddfea85850..ac36b5ddf717d 100644
> > --- a/include/linux/psp-sev.h
> > +++ b/include/linux/psp-sev.h
> > @@ -942,6 +942,58 @@ int sev_guest_decommission(struct sev_data_decommission *data, int *error);
> > */
> > int sev_do_cmd(int cmd, void *data, int *psp_ret);
> >
> > +/**
> > + * 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 guest context 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 guest context 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 guest context page, and frees
> > + * it.
> > + *
> > + * The caller must ensure mutual exclusion with any process that may deactivate ASIDs.
> > + *
> > + * @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);
> > +
> > void *psp_copy_user_blob(u64 uaddr, u32 len);
> > void *snp_alloc_firmware_page(gfp_t mask);
> > void snp_free_firmware_page(void *addr);

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