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
