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

[Tom Lendacky <thomas.lendacky@amd.com>]

On 11/8/24 16:13, Dionna Amalie Glaze wrote:
> On Fri, Nov 8, 2024 at 9:24 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 11/7/24 17:24, 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
>>
>> s/is has/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.
>>
>> You should be consistent with capitalization of gctx and also capitalize ASID.
>>
>>>
>>> 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

>>
>> But, I don't think that SEV_CMD_SNP_ACTIVATE needs to be here since it
>> doesn't change anything related to the sev_asid_data struct. KVM has the
>> guest context and can issue the commands similar to the other commands KVM
>> issues that use the guest context. So this function can be removed and
>> still performed in KVM.
> 
> My intention for adding it was for safety, not raw capability.
> Is it not safer to ensure that the GCTX used for activation is the one
> that is tracked?
> 

I'm not sure... all the code is really doing at this moment is tracking
guest context pages so that you can update them on firmware changes. Any
misuse of the context page and ASIDs can happen today in KVM so I'm not
sure it matters. And any duplicate ASID usage is recognized when
creating the guest context page.

I guess we can keep it here, though.

>>> +     cpuid(0x8000001f, &eax, &ebx, &sev_max_asid, &sev_min_asid);
>>> +     if (!sev_max_asid)
>>> +             return -ENODEV;
>>> +
>>> +     nr_asids = sev_max_asid + 1;
>>
>> Can we get rid of sev_max_asid and then just use nr_asids or sev_asids in
>> the cpuid() call and adjust by 1 after the above check.
>>
> I'm not sure I know what you mean.

You only need one of either nr_asids or sev_max_asid. So you could do:

	cpuid(0x8000001f, &eax, &ebx, &sev_max_asid, &sev_min_asid);
	if (!sev_max_asid)
		return -ENODEV;

	/* Bump SEV ASIDs count to allow for simple array checking */
	sev_max_asid++;

Then you can get rid of nr_asids and just use sev_max_asid in the
appropriate places and manner.

Thanks,
Tom

>>> +     sev_es_max_asid = sev_min_asid - 1;
>>> +
>>> +     sev_asid_data = kcalloc(nr_asids, sizeof(*sev_asid_data), GFP_KERNEL);
>>