Message-ID: <1e43dade-3fa7-4668-8fd8-01875ef91c2b@amd.com>
Date: Tue, 19 Nov 2024 21:14:24 -0600
From: "Kalra, Ashish" <ashish.kalra@....com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Peter Gonda <pgonda@...gle.com>, pbonzini@...hat.com, tglx@...utronix.de,
 mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com, hpa@...or.com,
 herbert@...dor.apana.org.au, x86@...nel.org, john.allen@....com,
 davem@...emloft.net, thomas.lendacky@....com, michael.roth@....com,
 kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-crypto@...r.kernel.org
Subject: Re: [PATCH v2 3/3] x86/sev: Add SEV-SNP CipherTextHiding support

Hello Sean,

On 10/11/2024 11:04 AM, Sean Christopherson wrote:
> On Wed, Oct 02, 2024, Ashish Kalra wrote:
>> Hello Peter,
>>
>> On 10/2/2024 9:58 AM, Peter Gonda wrote:
>>> On Tue, Sep 17, 2024 at 2:17 PM Ashish Kalra <Ashish.Kalra@....com> wrote:
>>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>>>> index 564daf748293..77900abb1b46 100644
>>>> --- a/drivers/crypto/ccp/sev-dev.c
>>>> +++ b/drivers/crypto/ccp/sev-dev.c
>>>> @@ -73,11 +73,27 @@ static bool psp_init_on_probe = true;
>>>>  module_param(psp_init_on_probe, bool, 0444);
>>>>  MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
>>>>
>>>> +static bool cipher_text_hiding = true;
>>>> +module_param(cipher_text_hiding, bool, 0444);
>>>> +MODULE_PARM_DESC(cipher_text_hiding, "  if true, the PSP will enable Cipher Text Hiding");
>>>> +
>>>> +static int max_snp_asid;
>>>> +module_param(max_snp_asid, int, 0444);
>>>> +MODULE_PARM_DESC(max_snp_asid, "  override MAX_SNP_ASID for Cipher Text Hiding");
>>> My read of the spec is if Ciphertext hiding is not enabled there is no
>>> additional split in the ASID space. Am I understanding that correctly?
>> Yes that is correct.
>>> If so, I don't think we want to enable ciphertext hiding by default
>>> because it might break whatever management of ASIDs systems already
>>> have. For instance right now we have to split SEV-ES and SEV ASIDS,
>>> and SNP guests need SEV-ES ASIDS. This change would half the # of SNP
>>> enable ASIDs on a system.
>>
>> My thought here is that we probably want to enable Ciphertext hiding by
>> default as that should fix any security issues and concerns around SNP
>> encryption as .Ciphertext hiding prevents host accesses from reading the
>> ciphertext of SNP guest private memory.
>>
>> This patch does add a new CCP module parameter, max_snp_asid, which can be
>> used to dedicate all SEV-ES ASIDs to SNP guests.
>>
>>>
>>> Also should we move the ASID splitting code to be all in one place?
>>> Right now KVM handles it in sev_hardware_setup().
>>
>> Yes, but there is going to be a separate set of patches to move all ASID
>> handling code to CCP module.
>>
>> This refactoring won't be part of the SNP ciphertext hiding support patches.
> 
> It should, because that's not a "refactoring", that's a change of roles and
> responsibilities.  And this series does the same; even worse, this series leaves
> things in a half-baked state, where the CCP and KVM have a weird shared ownership
> of ASID management.
> 

Sorry for the delayed reply to your response, the SNP DOWNLOAD_FIRMWARE_EX patches got posted
in the meanwhile and that had additional considerations of moving SNP GCTX pages stuff
into the PSP driver from KVM and that again got into this discussion about splitting ASID 
management across KVM and PSP driver and as you pointed out on those patches that there is
zero reason that the PSP driver needs to care about ASIDs. 

Well, CipherText Hiding (CTH) support is one reason where the PSP driver gets involved with ASIDs
as CTH feature has to be enabled as part of SNP_INIT_EX and once CTH feature is enabled, the 
SEV-ES ASID space is split across SEV-SNP and SEV-ES VMs. 

With reference to SNP GCTX pages, we are looking at some possibilities to push the requirement
to update SNP GCTX pages to SNP firmware and remove that requirement from the kernel/KVM side.

Considering that, I will still like to keep ASID management in KVM, there are issues with locking, for example,
sev_deactivate_lock is used to protect SNP ASID allocations (or actually for protecting ASID 
reuse/lazy-allocation requiring WBINVD/DF_FLUSH) and guarding this DF_FLUSH from VM destruction
(DEACTIVATE). Moving ASID management stuff into PSP driver will then add complexity of adding
this synchronization between different kernel modules or handling locking in two different kernel
modules, to guard ASID allocation in PSP driver with VM destruction in KVM module.

There is also this sev_vmcbs[] array indexed by ASID (part of svm_cpu_data) which gets referenced
during the ASID free code path in KVM. It just makes it simpler to keep ASID management stuff
in KVM. 

So probably we can add an API interface exported by the PSP driver something like
is_sev_ciphertext_hiding_enabled() or sev_override_max_snp_asid() instead of using
external variables in PSP driver, which KVM can call in sev_hardware_setup() to
retrieve MAX_SNP_ASID and also overriding max_asid (when CTH feature is enabled) in sev_asid_new(). 

Thanks,
Ashish

> I'm ok with essentially treating CipherText Hiding enablement as an extension of
> firmware, e.g. it's better than having to go into UEFI settings to toggle the
> feature on/off.  But we need to have a clear, well-defined vision for how we want
> this all to look in the end. 

Powered by blists - more mailing lists