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

[Jason Wang]

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

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.

Thanks

>
> --
> Peter Xu
>