On Fri, Aug 05, 2022, David Matlack wrote: > On Thu, Aug 4, 2022 at 5:04 PM Huang, Kai <kai.huang@xxxxxxxxx> wrote: > > > > > > In addition to the suggestions above, I'd suggest breaking this patch > > > > up, since it is doing multiple things: > > > > > > > > 1. Patch initialize shadow page tables to EMPTY_SPTE (0) and > > > > replace TDP MMU hard-coded 0 with EMPTY_SPTE. > > > > 2. Patch to change FNAME(sync_page) to not assume EMPTY_SPTE is 0. > > > > 3. Patch to set bit 63 in EMPTY_SPTE. > > > > 4. Patch to set bit 63 in REMOVED_SPTE. > > > > I think 1/2 can be separate patches, but 3/4 should be done together. > > > > Patch 3 alone is broken as when TDP MMU zaps SPTE and replaces it with > > REMOVED_SPTE, it loses bit 63. This is not what we want. We always want > > bit 63 set if it is supposed to be set to a non-present SPTE. > > How is patch 3 alone be broken? The TDX support that depends on bit 63 > does not exist at this point in the series, i.e. setting bit 63 is > entirely optional and only done in preparation for future patches. Hmm, I agree with Kai on this specific point. Will it cause functional problems? No. Is KVM technically broken? IMO, yes, because the intent of the code is to ensure bit 63 is set for all SPTEs that are not-present (and not misconfigured) in hardware. I 100% agree on patches doing too much, but in this particular case it's easy to capture the above semantics in a shortlog: KVM: x86/mmu: Set bit 63 (EPT's SUPPRESS_VE) in all not-present SPTEs
