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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Oct 19, 2023 at 10:49:29AM +0200, Jan Beulich wrote:
> On 19.10.2023 10:15, Roger Pau Monné wrote:
> > 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?
> 
> Indeed, as mentioned in the commit you're fixing. Version 1.3 of the
> spec still has it. Judging by the document numbers 2.2 may have been
> its direct successor (i.e. no further 1.x and no 2.0 or 2.1).
> 
> >>> --- 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.
> 
> But we will also need to deal with bit 4 (at which point applying a mask
> is going to be useless code). We can either guess that it's going to mean
> 6-level paging, or we need to treat it as having unknown meaning when set
> (implying that we'd then still need to either fail initialization or
> disable pass-through mode).

I wouldn't enable support for AGAW 4 unless we have a way to test it.
It's safer to just disable passthrough mode if SAGAW bit 4 is set.

> > I'm fine with using < or <= if you think it's more robust.
> 
> Good, will do so then.
> 
> >>>      {
> >>>          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.
> 
> Well, logging the index in printk_once() has the benefit of identifying
> the first IOMMU with the issue, which may help further analysis if not
> all of them have bits beyond 2 set. But I'm not going to insist on this
> aspect.
> 
> > 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.
> 
> I'd simply insert a leading space in the string literal.

Ack.

> >>> +                    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.
> 
> This is init-time code, and hence the excess writes aren't going to be
> noticable.

I don't have a strong opinion, TBH I used to have it without the
conditional and added it later.  If you prefer to drop the conditional
I won't oppose.

Thanks, Roger.