Re: [PATCH for-4.18 v2] iommu/vt-d: fix SAGAW capability parsing

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Oct 19, 2023 at 09:41:41AM +0200, Jan Beulich wrote:
> On 18.10.2023 18:07, Roger Pau Monne wrote:
> > SAGAW is a bitmap field, with bits 1, 2 and 3 signaling support for 3, 4 
> > and 5
> > level page tables respectively.  According to the Intel VT-d specification, 
> > an
> > IOMMU can report multiple SAGAW bits being set.
> > 
> > Commit 859d11b27912 claims to replace the open-coded find_first_set_bit(), 
> > but
> > it's actually replacing an open coded implementation to find the last set 
> > bit.
> > The change forces the used AGAW to the lowest supported by the IOMMU 
> > instead of
> > the highest one between 1 and 2.
> > 
> > Restore the previous SAGAW parsing by using fls() instead of
> > find_first_set_bit(), in order to get the highest (supported) AGAW to be 
> > used.
> > 
> > However there's a caveat related to the value the AW context entry field 
> > must
> > be set to when using passthrough mode:
> > 
> > "When the Translation-type (TT) field indicates pass-through processing 
> > (10b),
> > this field must be programmed to indicate the largest AGAW value supported 
> > by
> > hardware." [0]
> > 
> > Newer Intel IOMMU implementations support 5 level page tables for the IOMMU,
> > and signal such support in SAGAW bit 3.
> > 
> > Enabling 5 level paging support (AGAW 3) at this point in the release is too
> > risky, so instead put a bodge to unconditionally disable passthough mode if
> > SAGAW has any bits greater than 2 set.  Ignore bit 0, it's reserved in the
> > specification but unlikely to have any meaning in the future.
> 
> May be worth mentioning that in earlier versions this indicated 2-level
> paging support.

Oh, that's not even present in my copy of the spec from 2016.  I guess
it was removed very, very long time ago?

> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -1327,15 +1327,24 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
> >  
> >      /* Calculate number of pagetable levels: 3 or 4. */
> >      sagaw = cap_sagaw(iommu->cap);
> > -    if ( sagaw & 6 )
> > -        agaw = find_first_set_bit(sagaw & 6);
> > -    if ( !agaw )
> > +    agaw = fls(sagaw & 6) - 1;
> > +    if ( agaw == -1 )
> 
> Would you mind making this "< 0" or even "<= 0"? The latter in particular
> would already cover the likely future change of dropping the masking by 6.

My plan wasn't to drop the masking, but use 0xe if we support AGAW 3.
I'm fine with using < or <= if you think it's more robust.

> >      {
> >          printk(XENLOG_ERR VTDPREFIX "IOMMU: unsupported sagaw %x\n", 
> > sagaw);
> >          print_iommu_regs(drhd);
> >          rc = -ENODEV;
> >          goto free;
> >      }
> > +    if ( sagaw >> 3 )
> > +    {
> > +        printk_once(XENLOG_WARNING VTDPREFIX
> > +                    "IOMMU: unhandled bits set in sagaw (%#x)%s\n",
> 
> I think IOMMU: is redundant with VTDPREFIX (or alternatively iommu->index
> would also want logging). Also note that VTDPREFIX (bogusly) has no
> trailing space. (I realize both apply to the other log message in context
> as well, but still. I'd be inclined to adjust that at the same time,
> including switching to %#x as you have it in the new log message.)

Oh, I didn't realize VTDPREFIX had no trailing space.

Since it's a printk_once(), not sure iommu->index is really useful
here, as we would report just one IOMMU has having an unhandled SAGAW.
IMO if we switch to printing iommu->index we must also use a plain
printk.  But I don't see a lot of benefit in printing this for likely
each IOMMU on the system, and hence I would rather use printk_once()
and not print the index.

Feel free to drop the IOMMU prefix, but I'm not sure what to do with
VTDPREFIX and the missing trialing space, as some users of VTDPREFIX
already account for such missing space.

> > +                    sagaw,
> > +                    iommu_hwdom_passthrough ? " disabling passthrough" : 
> > "" );
> 
> May want a leading comma (or some other separator) in the string.
> 
> > +        if ( iommu_hwdom_passthrough )
> > +            iommu_hwdom_passthrough = false;
> 
> No real need for if() here.

Not really, but also no need for a write to iommu_hwdom_passthrough
every time an IOMMU is initialized if the condition is removed.

> I'd be happy to adjust any of the mentioned items while committing, but
> of course I would first need to know which ones you agree with. Since all
> of them are cosmetic, either way
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Let me know what you think re the log message.

Thanks, Roger.