Message-ID: <1e077351-6fc4-4106-b4fe-a36b8be75233@redhat.com>
Date: Mon, 3 Mar 2025 11:25:08 +0100
From: Paolo Bonzini <pbonzini@...hat.com>
To: Yan Zhao <yan.y.zhao@...el.com>
Cc: seanjc@...gle.com, rick.p.edgecombe@...el.com, kevin.tian@...el.com,
 linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH 0/3] KVM: x86: Introduce quirk
 KVM_X86_QUIRK_EPT_IGNORE_GUEST_PAT

On 3/3/25 02:11, Yan Zhao wrote:
>> the main issue with this series is that the quirk is not disabled only for
>> TDX VMs, but for *all* VMs if TDX is available.
> Yes, once TDX is enabled, the quirk is disabled for all VMs.
> My thought is that on TDX as a new platform, users have the option to update
> guest software to address bugs caused by incorrect guest PAT settings.
> 
> If you think it's a must to support old unmodifiable non-TDX VMs on TDX
> platforms, then it's indeed an issue of this series.

Yeah, unfortunately I think we need to keep the quirk for old VMs.  But 
I think the code changes needed to do so are small and good to have anyway.

>> There are two concepts here:
>>
>> - which quirks can be disabled
>>
>> - which quirks are active
>>
>> I agree with making the first vendor-dependent, but for a different reason:
>> the new KVM_X86_QUIRK_EPT_IGNORE_GUEST_PAT must be hidden if self-snoop is
>> not present.
>
> I think it's a good idea to make KVM_X86_QUIRK_EPT_IGNORE_GUEST_PAT out of
> KVM_CAP_DISABLE_QUIRKS2, so that the quirk is always enabled when self-snoop is
> not present as userspace has no way to disable this quirk.
> 
> However, this seems to contradict your point below, especially since it is even
> present on AMD platforms.
> 
> "we need to expose the quirk anyway in KVM_CAP_DISABLE_QUIRKS2, so that
> userspace knows that KVM is *aware* of a particular issue",  "even if disabling
> it has no effect, userspace may want to know that it can rely on the problematic
> behavior not being present".

There are four cases:

* quirk cannot be disabled: example, "ignore guest PAT" on 
non-self-snoop machines: the quirk must not be in KVM_CAP_DISABLE_QUIRKS2

* quirk can be disabled: the quirk must be in KVM_CAP_DISABLE_QUIRKS2

* quirk is always disabled: right now we're always exposing those in 
KVM_CAP_DISABLE_QUIRKS2, so we should keep that behavior.  If desired we 
could add a capability like KVM_CAP_DISABLED_QUIRKS

* for some VMs, quirk is always disabled: this is the case also for the 
zap_all quirk that you have previously introduced.  Right now there's no 
way to query it, but KVM_CAP_DISABLED_QUIRKS would also cover this.  If 
KVM_CAP_DISABLED_QUIRKS was introduced, zap_all could be added too.

> So, could we also expose KVM_X86_QUIRK_EPT_IGNORE_GUEST_PAT in
> KVM_CAP_DISABLE_QUIRKS2 on Intel platforms without self-snoop, but ensure that
> disabling the quirk has no effect?

To keep the API clear, disabling the quirk should *always* have the 
effect of going to the non-quirky behavior.  Which may be no effect at 
all if the non-quirky behavior is the only one---but the important thing 
is that you don't want the quirky/buggy/non-architectural behavior after 
a successful KVM_ENABLE_CAP(KVM_CAP_DISABLE_QUIRKS2).

There is a pre-existing bug in that I think 
KVM_ENABLE_CAP(KVM_CAP_DISABLE_QUIRKS2) should be cumulative, i.e. 
should not allow re-enabling a previously-disabled quirk.  I think we 
can change that without worrying about breaking userspace there, as the 
current behavior is the most surprising.

>> As to the second, we already have an example of a quirk that is also active,
>> though we don't represent that in kvm->arch.disabled_quirks: that's
>> KVM_X86_QUIRK_CD_NW_CLEARED which is for AMD only and is effectively always
>> disabled on Intel platforms.  For those cases, we need to expose the quirk
> I also have a concern about this one. Please find my comments in v2.

Ok, I'll reply there too.

>> anyway in KVM_CAP_DISABLE_QUIRKS2, so that userspace knows that KVM is
>> *aware* of a particular issue.  In other words, even if disabling it has no
>> effect, userspace may want to know that it can rely on the problematic
>> behavior not being present.
>>
>> I'm testing an alternative series and will post it shortly.
>   
> Thanks a lot for helping with refining the patches!

Thanks to you and sorry that the patches weren't of the best quality - I 
mostly wanted to start the discussion on the userspace API side before 
the beginning of the week in your time zone.

Paolo

Powered by blists - more mailing lists