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

[Jason Wang]

On Wed, Nov 24, 2021 at 6:10 PM Peter Xu <peterx@redhat.com> wrote:
>
> 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.

A side-effect is to bring troubles for the migration compatibility :(

>
> 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.

Adding BaoLu.

As discussed, according to my test with vIOMMU, there are side effect
of that commit which uses SNP even for the second level page table for
domains that is not IOMMU_DOMAIN_UNMANAGED. If I was wrong, we can
refer to that in the code.

Thanks

>
> --
> Peter Xu
>