Re: [PATCH for-4.18] iommu/vt-d: use max supported AGAW

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.10.2023 15:09, Roger Pau Monne wrote:
> SAGAW is a bitmap field, with bits 1 and 2 signaling support for AGAW 1 and
> AGAW 2 respectively.  According to the Intel VT-d specification, an IOMMU 
> might
> support multiple AGAW values.
> 
> The AGAW value for each device is set in the device context entry, however
> there's a caveat related to the value the field supports depending on the
> translation type:
> 
> "When the Translation-type (T) field indicates pass-through (010b) or
> guest-mode (100b or 101b), this field must be programmed to indicate the
> largest AGAW value supported by hardware."

Considering SAGAW=3 was reserved in earlier versions, and considering SAGAW=4
and higher continue to be reserved, how is one to write forward-compatible
code? (In retrospect I think this is what mislead me to wrongly use
find_first_set_bit().)

Furthermore, which version of the spec are you looking at? The newest public
one I found is 4.1 (-016), which only mentions pass-through, and only as a
2-bit quantity. (Doesn't matter much for the purposes of the actual code
change, but still.)

> Of the translation types listed above Xen only uses pass-through (010b), and
> hence we need to make sure the context entry AGAW field is set appropriately,
> or else the IOMMU will report invalid context entry errors.
> 
> To do so calculate the IOMMU supported page table levels based on the last bit
> set in the SAGAW field, instead of the first one.  This also allows making use
> of the widest address width supported by the IOMMU, in case multiple AGAWs are
> supported.

To truly achieve that (with the 5-level spec), ...

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1328,7 +1328,7 @@ 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);
> +        agaw = fls(sagaw & 6) - 1;

... the mask here needs widening to 0xe. But see my forward-compatibility
remark above: It may need widening even further. Yet I'm not sure our code
is uniformly ready to handle levels > 4. As a result I think we need to
further alter the use of context_set_address_width(): We don't necessarily
want to use the maximum value with CONTEXT_TT_{DEV_IOTLB,MULTI_LEVEL}.
Specifically I don't think we want to use levels=5 (aw=3) there, until
such time that we support 5-level page tables (which as it looks right now
may well end up being never).

Furthermore just out of context we have

    iommu->nr_pt_levels = agaw_to_level(agaw);
    if ( min_pt_levels > iommu->nr_pt_levels )
        min_pt_levels = iommu->nr_pt_levels;

With fls() instead of find_first_set_bit() this won't be correct anymore.
Yet looking at the sole use (and depending on the resolution of the other
issue) it may be a mere matter of renaming the variable to properly
reflect its purpose.

Taking together perhaps:
- nr_pt_levels needs setting to the larger of 3 and 4, depending on what
  hardware supports, for use in non-pass-through entries,
- a new max_pt_levels field needs setting to what would result from your
  code change above, for use in pass-through entries,
- min_pt_levels could then remain as is,
- for the moment we ignore the forward-compatibility aspect, until the
  underlying principle has been clarified by Intel.

A possible further complication then is that we will end up switching
context entries between different AW values. That's not an issue when
we use CMPXCHG16B or transiently clear the present bit, but our best
effort fallback would likely be of security concern then.

Jan