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
