On Wed, Nov 24, 2021 at 05:35:18PM +0800, Jason Wang wrote: > On Wed, Nov 24, 2021 at 5:23 PM Peter Xu <pet...@redhat.com> wrote: > > > > On Wed, Nov 24, 2021 at 05:01:42PM +0800, Jason Wang wrote: > > > > > > > -static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t > > > > > > > level) > > > > > > > +static bool vtd_slpte_nonzero_rsvd(IntelIOMMUState *s, > > > > > > > + uint64_t slpte, uint32_t > > > > > > > level) > > > > > > > { > > > > > > > uint64_t rsvd_mask = vtd_spte_rsvd[level]; > > > > > > > > > > > > > > @@ -979,6 +980,10 @@ static bool vtd_slpte_nonzero_rsvd(uint64_t > > > > > > > slpte, uint32_t level) > > > > > > > rsvd_mask = vtd_spte_rsvd_large[level]; > > > > > > > } > > > > > > > > > > > > > > + if (s->scalable_mode) { > > > > > > > + rsvd_mask &= ~VTD_SPTE_SNP; > > > > > > > + } > > > > > > > > > > > > IMHO what we want to do is only to skip the leaves of pgtables on > > > > > > SNP, so maybe > > > > > > we still want to keep checking the bit 11 reserved for e.g. common > > > > > > pgtable dir > > > > > > entries? > > > > > > Maybe, but it's probably a question that can only be answered by > > > Intel. I can change it for the next version if you stick. > > > > I'm reading vtd spec v3.1 (June 2019) here, and chap 9.8 told me they're > > reserved bits for pgdir entries, as no SNP bit defined on pgdir entries. > > Yes, you're right. > > > > > > > > > > > > > > > > > > To do so, how about directly modifying the vtd_spte_rsvd* fields in > > > > > > vtd_init()? > > > > > > I think we only need to modify 4k/2m/1g entries to mask bit 11, > > > > > > they're: > > > > > > > > > > > > - vtd_spte_rsvd[1] (4K) > > > > > > - vtd_spte_rsvd_large[2] (2M) > > > > > > - vtd_spte_rsvd_large[3] (1G) > > > > > > > > > > > > What do you think? Then we avoid passing IntelIOMMUState* all over > > > > > > too. > > > > > > I started a version like that:), it should work, I will change that if > > > it was agreed by everyone. > > > > > > The reason that I changed to pass IntelIOMMUState is that it results > > > in a smaller changeset. The reason is that I tend to introduce new > > > rsvd bits for SM mode since after checking vtd 3.3 it looks have > > > different reserved bit requirement than before (at least 1.2) > > > > Oh I thought changing vtd_spte_rsvd* should have smaller changeset instead, > > hmm? :) > > > > IMHO it'll be: > > > > if (s->scalable_mode) { > > vtd_spte_rsvd[1] &= ~BIT(11); > > vtd_spte_rsvd_large[2] &= ~BIT(11); > > vtd_spte_rsvd_large[3] &= ~BIT(11); > > } > > > > Would that work? Thanks, > > Works for sure, if we just want to fix the SNP bit. > > (I actually have a version like this as well). I can go this way
Sounds good at least to me. Thanks! > > The reason why I had another big changset is to align the reserved > bits to vtd 3.3. E.g it equires e.g bit 62 to be reserved 63 to be > ignored which seems not as strict as VTD_SL_IGN_COM. But it can be > addressed in the future. Ah I see. We can do that later, or if the patch is ready anway IMHO we can have them fixed altogether too. It's weird that VT-d spec seems to be very prone to changes.. that's rare as a spec, and it even happened multiple times. Another trivial thing: for that SNP bit code change, shall we also reference the Linux commit 6c00612d0cba ("iommu/vt-d: Report right snoop capability when using FL for IOVA", 2021-04-07) directly in the code as comment? Just want to make sure we'll never forget why we added it as no one will be able to find a clue in the spec, meanwhile that explicit hint let us remember when we added SC support we can drop it. -- Peter Xu