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

[Kai Huang <kai.huang@intel.com>]

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