Message-ID: <d3e78d92-29f0-4f56-a1fe-f8131cbc2555@amd.com>
Date: Wed, 20 Nov 2024 17:43:38 -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


On 11/20/2024 3:53 PM, Sean Christopherson wrote:
> On Tue, Nov 19, 2024, Ashish Kalra wrote:
>> On 10/11/2024 11:04 AM, Sean Christopherson wrote:
>>> On Wed, Oct 02, 2024, Ashish Kalra wrote:
>>>> 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. 
> 
> Right, but that's just a case where KVM needs to react to the setup done by the
> PSP, correct?  E.g. it's similar to SEV-ES being enabled/disabled in firmware,
> only that "firmware" happens to be a kernel driver.

Yes that is true.

> 
>> 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.
> 
> Heh, that'd work too.
> 
>> 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()
> 
> What about adding a cc_attr_flags entry?

Yes, that is a possibility i will look into. 

But, along with an additional cc_attr_flags entry, max_snp_asid (which is a PSP driver module parameter) also needs to be propagated to KVM, 
that's what i was considering passing as parameter to the above API interface.

Thanks,
Ashish

Powered by blists - more mailing lists