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?
