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

["Kalra, Ashish" <ashish.kalra@amd.com>]

On 11/11/2024 3:35 PM, Dionna Amalie Glaze wrote:
> 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.

I am currently working on this refactoring work.

> If it's
> currently underway, do you see a rebase on this patch set as
> particularly challenging?

No i don't think it will be difficult to rebase the refactoring work 
with respect to this patchset.

Thanks,
Ashish

> 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);
> 
> 
>