Re: [PATCH 2/4] KVM: x86/mmu: Fully re-evaluate MMIO caching when SPTE masks change

Kai Huang
2022-08-02T22:19:44Z

On Tue, 2022-08-02 at 21:15 +0000, Sean Christopherson wrote:
> On Tue, Aug 02, 2022, Kai Huang wrote:
> > On Mon, 2022-08-01 at 23:20 +0000, Sean Christopherson wrote:
> > > On Tue, Aug 02, 2022, Kai Huang wrote:
> > > > On Mon, 2022-08-01 at 14:15 +0000, Sean Christopherson wrote:
> > > > > Another thing to note is that only the value needs to be per-VM, the mask can be
> > > > > KVM-wide, i.e. "mask = SUPPRESS_VE | RWX" will work for TDX and non-TDX VMs when
> > > > > EPT is enabled.
> > > >
> > > > Yeah, but is more like VMX and TDX both *happen* to have the same mask?
> > > > Theoretically, VMX only need RWX to trigger EPT misconfiguration but doesn't
> > > > need SUPPRESS_VE.
> > >
> > > Right, SUPPRESS_VE isn't strictly necessary, but KVM already deliberately avoids
> > > bit 63 because it has meaning, e.g. SUPPRESS_VE for EPT and NX for PAE and 64-bit
> > > paging.
> > >
> > > > I don't see making mask/value both per-vm is a big issue?
> > >
> > > Yes and no.
> > >
> > > No, in the sense that it's not a big issue in terms of code.
> > >
> > > Yes, because of the connotations of having a per-VM mask. While having SUPPRESS_VE
> > > in the mask for non-TDX EPT isn't strictly necessary, it's also not strictly necessary
> > > to _not_ have it in the mask.
> > >
> >
> > I think the 'mask' itself is ambiguous, i.e. it doesn't say in what circumstance
> > we should include one bit to the mask. My understanding is any bit in the
> > 'mask' should at least be related to the 'value' that can enable MMIO caching.
>
> The purpose of the mask isn't ambiguous, though it's definitely not well documented.
> The mask defines what bits should be included in the check to identify an MMIO SPTE.

Yes this is true.

>
> > So if SUPPRESS_VE bit is not related to non-TDX EPT (as we want EPT
> > misconfiguration, but not EPT violation), I don't see why we need to include it
> > to the 'mask'.
>
> Again, it's not strictly necessary, but by doing so we don't need a per-VM mask.
> And KVM should also never set SUPPRESS_VE for MMIO SPTEs, i.e. checking that bit
> by including it in the mask adds some sanitcy check (albeit a miniscule amount).

OK.

>
> > > In other words, having a per-VM mask incorrectly implies that TDX _must_
> > > have a different mask.
> >
> > I interpret as TDX _can_, but not _must_.
>
> Right, but if we write the KVM code such that it doesn't have a different mask,
> then even that "can" is wrong/misleading.
>
> > > It's also one more piece of information that developers have to track down and
> > > account for, i.e. one more thing we can screw up.
> > >
> > > The other aspect of MMIO SPTEs are that the mask bits must not overlap the generation
> > > bits or shadow-present bit, and changing any of those bits requires careful
> > > consideration, i.e. defining the set of _allowed_ mask bits on a per-VM basis would
> > > incur significant complexity without providing meaningful benefit.
> > >
> >
> > Agreed on this.
> >
> > But we are not checking any of those in kvm_mmu_set_mmio_spte_mask(), right? :)
>
> No, but we really should.

I can come up a patch if you are not planning to do so?

>
> > Also Isaku's patch extends kvm_mmu_set_mmio_spte_mask() to take 'kvm' or 'vcpu'
> > as parameter so it's easy to check there -- not 100% sure about other places,
> > though.
> >
> > > As a result,
> > > it's highly unlikely that we'll ever want to opportunsitically "reclaim" bit 63
> > > for MMIO SPTEs, so there's practically zero cost if it's included in the mask for
> > > non-TDX EPT.
> >
> > Sorry I don't understand this. If we will never "reclaim" bit 63 for MMIO SPTEs
> > (for non-TDX EPT), then why bother including it to the mask?
>
> Because then we don't need to track a per-VM mask.

OK.

--
Thanks,
-Kai