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
