RE: [PATCH 1/2] VT-d: generalize and correct "iommu=no-igfx" handling

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, October 11, 2021 4:49 PM
> 
> Linux'es supposedly equivalent "intel_iommu=igfx_off" deals with any
> graphics devices (not just Intel ones) while at the same time limiting
> the effect to IOMMUs covering only graphics devices. Keying the decision
> to leave translation disabled for an IOMMU to merely a magic SBDF tuple
> was wrong in the first place - systems may very well have non-graphics
> devices at 0000:00:02.0 (ordinary root ports commonly live there, for
> example). Any use of igd_drhd_address (and hence is_igd_drhd()) needs
> further qualification.
> 
> Introduce a new "graphics only" field in struct acpi_drhd_unit and set
> it according to device scope parsing outcome. Replace the bad use of
> is_igd_drhd() in iommu_enable_translation() by use of this new field.
> 
> While adding the new field also convert the adjacent include_all one to
> "bool".
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

> ---
> I assume an implication is that these devices then may not be passed
> through to guests, yet I don't see us enforcing this anywhere.
> 
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1494,8 +1494,8 @@ The following options are specific to In
>      version 6 and greater as Registered-Based Invalidation isn't supported
>      by them.
> 
> -*   The `igfx` boolean is active by default, and controls whether the IOMMU
> in
> -    front of an Intel Graphics Device is enabled or not.
> +*   The `igfx` boolean is active by default, and controls whether IOMMUs in
> +    front of solely graphics devices get enabled or not.
> 
>      It is intended as a debugging mechanism for graphics issues, and to be
>      similar to Linux's `intel_iommu=igfx_off` option.  If specifying 
> `no-igfx`
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -315,6 +315,7 @@ static int __init acpi_parse_dev_scope(
>      struct acpi_drhd_unit *drhd = type == DMAR_TYPE ?
>          container_of(scope, struct acpi_drhd_unit, scope) : NULL;
>      int depth, cnt, didx = 0, ret;
> +    bool gfx_only = false;
> 
>      if ( (cnt = scope_device_count(start, end)) < 0 )
>          return cnt;
> @@ -324,6 +325,8 @@ static int __init acpi_parse_dev_scope(
>          scope->devices = xzalloc_array(u16, cnt);
>          if ( !scope->devices )
>              return -ENOMEM;
> +
> +        gfx_only = drhd && !drhd->include_all;
>      }
>      scope->devices_cnt = cnt;
> 
> @@ -354,6 +357,7 @@ static int __init acpi_parse_dev_scope(
>                         acpi_scope->bus, sec_bus, sub_bus);
> 
>              dmar_scope_add_buses(scope, sec_bus, sub_bus);
> +            gfx_only = false;
>              break;
> 
>          case ACPI_DMAR_SCOPE_TYPE_HPET:
> @@ -374,6 +378,8 @@ static int __init acpi_parse_dev_scope(
>                  acpi_hpet_unit->dev = path->dev;
>                  acpi_hpet_unit->func = path->fn;
>                  list_add(&acpi_hpet_unit->list, &drhd->hpet_list);
> +
> +                gfx_only = false;
>              }
> 
>              break;
> @@ -388,6 +394,12 @@ static int __init acpi_parse_dev_scope(
>                  if ( (seg == 0) && (bus == 0) && (path->dev == 2) &&
>                       (path->fn == 0) )
>                      igd_drhd_address = drhd->address;
> +
> +                if ( gfx_only &&
> +                     pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
> +                                    PCI_CLASS_DEVICE + 1) != 0x03
> +                                    /* PCI_BASE_CLASS_DISPLAY */ )
> +                    gfx_only = false;
>              }
> 
>              break;
> @@ -408,6 +420,8 @@ static int __init acpi_parse_dev_scope(
>                  acpi_ioapic_unit->ioapic.bdf.dev = path->dev;
>                  acpi_ioapic_unit->ioapic.bdf.func = path->fn;
>                  list_add(&acpi_ioapic_unit->list, &drhd->ioapic_list);
> +
> +                gfx_only = false;
>              }
> 
>              break;
> @@ -417,11 +431,15 @@ static int __init acpi_parse_dev_scope(
>                  printk(XENLOG_WARNING VTDPREFIX "Unknown scope type %#x\n",
>                         acpi_scope->entry_type);
>              start += acpi_scope->length;
> +            gfx_only = false;
>              continue;
>          }
>          scope->devices[didx++] = PCI_BDF(bus, path->dev, path->fn);
>          start += acpi_scope->length;
> -   }
> +    }
> +
> +    if ( drhd && gfx_only )
> +        drhd->gfx_only = true;
> 
>      ret = 0;
> 
> --- a/xen/drivers/passthrough/vtd/dmar.h
> +++ b/xen/drivers/passthrough/vtd/dmar.h
> @@ -62,7 +62,8 @@ struct acpi_drhd_unit {
>      struct list_head list;
>      u64    address;                     /* register base address of the unit 
> */
>      u16    segment;
> -    u8     include_all:1;
> +    bool   include_all:1;
> +    bool   gfx_only:1;
>      struct vtd_iommu *iommu;
>      struct list_head ioapic_list;
>      struct list_head hpet_list;
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -751,7 +751,7 @@ static void iommu_enable_translation(str
>      unsigned long flags;
>      struct vtd_iommu *iommu = drhd->iommu;
> 
> -    if ( is_igd_drhd(drhd) )
> +    if ( drhd->gfx_only )
>      {
>          if ( !iommu_igfx )
>          {