On Tue, Jun 18, 2024 at 07:34:15AM -0700, Sean Christopherson wrote: > On Mon, Jun 17, 2024, Yan Zhao wrote: > > On Fri, Jun 14, 2024 at 04:01:07AM +0800, Edgecombe, Rick P wrote: > > > On Thu, 2024-06-13 at 14:06 +0800, Yan Zhao wrote: > > > > a) Add a condition for TDX VM type in kvm_arch_flush_shadow_memslot() > > > > besides the testing of kvm_check_has_quirk(). It is similar to > > > > "all new VM types have the quirk disabled". e.g. > > > > > > > > static inline bool kvm_memslot_flush_zap_all(struct kvm > > > > *kvm) > > > > > > > > { > > > > > > > > return kvm->arch.vm_type != KVM_X86_TDX_VM > > > > && > > > > kvm_check_has_quirk(kvm, > > > > KVM_X86_QUIRK_SLOT_ZAP_ALL); > > > > } > > > > > > > > b) Init the disabled_quirks based on VM type in kernel, extend > > > > disabled_quirk querying/setting interface to enforce the quirk to > > > > be disabled for TDX. > > There's also option: > > c) Init disabled_quirks based on VM type. > > I.e. let userspace enable the quirk. If the VMM wants to shoot its TDX VM guests, > then so be it. That said, I don't like this option because it would create a very > bizarre ABI. > > > > > > > I'd prefer to go with option (a) here. Because we don't have any behavior > > > defined yet for KVM_X86_TDX_VM, we don't really need to "disable a quirk" of it. > > I vote for (a) as well. > > > > Instead we could just define KVM_X86_QUIRK_SLOT_ZAP_ALL to be about the behavior > > > of the existing vm_types. It would be a few lines of documentation to save > > > implementing and maintaining a whole interface with special logic for TDX. So to > > > me it doesn't seem worth it, unless there is some other user for a new more > > > complex quirk interface. > > What about introducing a forced disabled_quirk field? > > Nah, it'd require manual opt-in for every VM type for almost no benefit. In fact, > IMO the code itself would be a net negative versus: > > return kvm->arch.vm_type == KVM_X86_DEFAULT_VM && > kvm_check_has_quirk(kvm, KVM_X86_QUIRK_SLOT_ZAP_ALL); > > because explicitly checking for KVM_X86_DEFAULT_VM would directly match the > documentation (which would state that the quirk only applies to DEFAULT_VM). Makes sense. Then, (a) looks good to me :) Do you prefer to include the document update (i.e. the quirk only applies to DEFAULT_VM) in this series or in TDX MMU series? And it means the quirk will not be enabled for all other VM types, e.g. KVM_X86_SW_PROTECTED_VM, and SEV, SNP..., right?