Re: [PATCH v2 3/3] x86/sev: Add SEV-SNP CipherTextHiding support

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

On 11/20/2024 5:43 PM, Kalra, Ashish wrote:
> 
> 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.
> 

Adding a new cc_attr_flags entry indicating CTH support is enabled.

And as discussed with Boris, using the cc_platform_set() to add a new attr like max_asid and adding a getter interface on top to return the
max_snp_asid.

Thanks,
Ashish