Re: [PATCH] intel-iommu: ignore SNP bit in scalable mode

[Peter Xu]

On Wed, Nov 24, 2021 at 05:35:18PM +0800, Jason Wang wrote:
> On Wed, Nov 24, 2021 at 5:23 PM Peter Xu <peterx@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