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

From: Jan Beulich <jbeulich@suse.com>
To: Roger Pau Monne <roger.pau@citrix.com>
Cc: Henry Wang <Henry.Wang@arm.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Kevin Tian <kevin.tian@intel.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH for-4.18 v2] iommu/vt-d: fix SAGAW capability parsing
Date: Thu, 19 Oct 2023 09:41:41 +0200	[thread overview]
Message-ID: <709ade00-9112-2866-4d2b-3bb718b13b46@suse.com> (raw)
In-Reply-To: <20231018160733.24655-1-roger.pau@citrix.com>

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.

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

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

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

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@suse.com>

Jan