[PATCH 0/4] IOMMU/PCI: respect device specifics

[PATCH 0/4] IOMMU/PCI: respect device specifics

[Jan Beulich]

While making the first change here it occurred to me that the recent
vPCI-related discussion about hidden devices has some relevance also
elsewhere. In the course I then came to also notice a phantom device
related quirk.

1: VT-d: defer "no DRHD" check when (un)mapping devices
2: VT-d: consider hidden devices when unmapping
3: VT-d: skip IOMMU bitmap cleanup for phantom devices
4: AMD/IOMMU: consider hidden devices when flushing device I/O TLBs

Jan

(I'm sorry for the resend, but I thought I would better add the
previously missing tag.)

[PATCH 1/4] VT-d: defer "no DRHD" check when (un)mapping devices

[Jan Beulich]

If devices are to be skipped anyway (which is the case in particular for
host bridges), there's no point complaining about a missing DRHD (and
hence a missing association with an IOMMU).

While there convert assignments to initializers and constify "drhd"
local variables.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1460,14 +1460,10 @@ static int domain_context_unmap(struct d
 static int domain_context_mapping(struct domain *domain, u8 devfn,
                                   struct pci_dev *pdev)
 {
-    struct acpi_drhd_unit *drhd;
+    const struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev);
     int ret = 0;
     u8 seg = pdev->seg, bus = pdev->bus, secbus;
 
-    drhd = acpi_find_matched_drhd_unit(pdev);
-    if ( !drhd )
-        return -ENODEV;
-
     /*
      * Generally we assume only devices from one node to get assigned to a
      * given guest.  But even if not, by replacing the prior value here we
@@ -1476,7 +1472,7 @@ static int domain_context_mapping(struct
      * this or other devices may be penalized then, but some would also be
      * if we left other than NUMA_NO_NODE untouched here.
      */
-    if ( drhd->iommu->node != NUMA_NO_NODE )
+    if ( drhd && drhd->iommu->node != NUMA_NO_NODE )
         dom_iommu(domain)->node = drhd->iommu->node;
 
     ASSERT(pcidevs_locked());
@@ -1497,6 +1493,9 @@ static int domain_context_mapping(struct
         break;
 
     case DEV_TYPE_PCIe_ENDPOINT:
+        if ( !drhd )
+            return -ENODEV;
+
         if ( iommu_debug )
             printk(VTDPREFIX "%pd:PCIe: map %pp\n",
                    domain, &PCI_SBDF3(seg, bus, devfn));
@@ -1508,6 +1507,9 @@ static int domain_context_mapping(struct
         break;
 
     case DEV_TYPE_PCI:
+        if ( !drhd )
+            return -ENODEV;
+
         if ( iommu_debug )
             printk(VTDPREFIX "%pd:PCI: map %pp\n",
                    domain, &PCI_SBDF3(seg, bus, devfn));
@@ -1651,17 +1653,12 @@ int domain_context_unmap_one(
 static int domain_context_unmap(struct domain *domain, u8 devfn,
                                 struct pci_dev *pdev)
 {
-    struct acpi_drhd_unit *drhd;
-    struct vtd_iommu *iommu;
+    const struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev);
+    struct vtd_iommu *iommu = drhd ? drhd->iommu : NULL;
     int ret;
     u8 seg = pdev->seg, bus = pdev->bus, tmp_bus, tmp_devfn, secbus;
     int found = 0;
 
-    drhd = acpi_find_matched_drhd_unit(pdev);
-    if ( !drhd )
-        return -ENODEV;
-    iommu = drhd->iommu;
-
     switch ( pdev->type )
     {
     case DEV_TYPE_PCI_HOST_BRIDGE:
@@ -1676,6 +1673,9 @@ static int domain_context_unmap(struct d
         return 0;
 
     case DEV_TYPE_PCIe_ENDPOINT:
+        if ( !iommu )
+            return -ENODEV;
+
         if ( iommu_debug )
             printk(VTDPREFIX "%pd:PCIe: unmap %pp\n",
                    domain, &PCI_SBDF3(seg, bus, devfn));
@@ -1686,6 +1686,9 @@ static int domain_context_unmap(struct d
         break;
 
     case DEV_TYPE_PCI:
+        if ( !iommu )
+            return -ENODEV;
+
         if ( iommu_debug )
             printk(VTDPREFIX "%pd:PCI: unmap %pp\n",
                    domain, &PCI_SBDF3(seg, bus, devfn));

[PATCH 2/4] VT-d: consider hidden devices when unmapping

[Jan Beulich]

Whether to clear an IOMMU's bit in the domain's bitmap should depend on
all devices the domain can control. For the hardware domain this
includes hidden devices, which are associated with DomXEN.

While touching related logic, convert "found" to "bool".

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1657,7 +1657,7 @@ static int domain_context_unmap(struct d
     struct vtd_iommu *iommu = drhd ? drhd->iommu : NULL;
     int ret;
     u8 seg = pdev->seg, bus = pdev->bus, tmp_bus, tmp_devfn, secbus;
-    int found = 0;
+    bool found;
 
     switch ( pdev->type )
     {
@@ -1737,23 +1737,33 @@ static int domain_context_unmap(struct d
         return ret;
 
     /*
-     * if no other devices under the same iommu owned by this domain,
-     * clear iommu in iommu_bitmap and clear domain_id in domid_bitmp
+     * If no other devices under the same iommu owned by this domain,
+     * clear iommu in iommu_bitmap and clear domain_id in domid_bitmap.
      */
-    for_each_pdev ( domain, pdev )
+    for ( found = false; ; domain = dom_xen )
     {
-        if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
-            continue;
-
-        drhd = acpi_find_matched_drhd_unit(pdev);
-        if ( drhd && drhd->iommu == iommu )
+        for_each_pdev ( domain, pdev )
         {
-            found = 1;
-            break;
+            if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
+                continue;
+
+            drhd = acpi_find_matched_drhd_unit(pdev);
+            if ( drhd && drhd->iommu == iommu )
+            {
+                found = true;
+                break;
+            }
         }
+
+        /*
+         * Hidden devices are associated with DomXEN but usable by the
+         * hardware domain. Hence they need considering here as well.
+         */
+        if ( found || !is_hardware_domain(domain) )
+            break;
     }
 
-    if ( found == 0 )
+    if ( !found )
     {
         clear_bit(iommu->index, &dom_iommu(domain)->arch.vtd.iommu_bitmap);
         cleanup_domid_map(domain, iommu);

[PATCH 3/4] VT-d: skip IOMMU bitmap cleanup for phantom devices

[Jan Beulich]

Doing the cleanup also for phantom devices is at best redundant with
doing it for the corresponding real device. I couldn't force myself into
checking all the code paths whether it really is: It seems better to
explicitly skip this step in such cases.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1733,7 +1733,7 @@ static int domain_context_unmap(struct d
         return -EINVAL;
     }
 
-    if ( ret || QUARANTINE_SKIP(domain) )
+    if ( ret || QUARANTINE_SKIP(domain) || pdev->devfn != devfn )
         return ret;
 
     /*

[PATCH 4/4] AMD/IOMMU: consider hidden devices when flushing device I/O TLBs

[Jan Beulich]

Hidden devices are associated with DomXEN but usable by the
hardware domain. Hence they need flushing as well when all devices are
to have flushes invoked.

While there drop a redundant ATS-enabled check and constify the first
parameter of the involved function.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -308,14 +308,11 @@ void amd_iommu_flush_iotlb(u8 devfn, con
     flush_command_buffer(iommu, iommu_dev_iotlb_timeout);
 }
 
-static void amd_iommu_flush_all_iotlbs(struct domain *d, daddr_t daddr,
+static void amd_iommu_flush_all_iotlbs(const struct domain *d, daddr_t daddr,
                                        unsigned int order)
 {
     struct pci_dev *pdev;
 
-    if ( !ats_enabled )
-        return;
-
     for_each_pdev( d, pdev )
     {
         u8 devfn = pdev->devfn;
@@ -343,7 +340,16 @@ static void _amd_iommu_flush_pages(struc
     }
 
     if ( ats_enabled )
+    {
         amd_iommu_flush_all_iotlbs(d, daddr, order);
+
+        /*
+         * Hidden devices are associated with DomXEN but usable by the
+         * hardware domain. Hence they need dealing with here as well.
+         */
+        if ( is_hardware_domain(d) )
+            amd_iommu_flush_all_iotlbs(dom_xen, daddr, order);
+    }
 }
 
 void amd_iommu_flush_all_pages(struct domain *d)

RE: [PATCH 1/4] VT-d: defer "no DRHD" check when (un)mapping devices

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, September 15, 2021 5:12 PM
> 
> If devices are to be skipped anyway (which is the case in particular for
> host bridges), there's no point complaining about a missing DRHD (and
> hence a missing association with an IOMMU).
> 
> While there convert assignments to initializers and constify "drhd"
> local variables.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1460,14 +1460,10 @@ static int domain_context_unmap(struct d
>  static int domain_context_mapping(struct domain *domain, u8 devfn,
>                                    struct pci_dev *pdev)
>  {
> -    struct acpi_drhd_unit *drhd;
> +    const struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev);
>      int ret = 0;
>      u8 seg = pdev->seg, bus = pdev->bus, secbus;
> 
> -    drhd = acpi_find_matched_drhd_unit(pdev);
> -    if ( !drhd )
> -        return -ENODEV;
> -
>      /*
>       * Generally we assume only devices from one node to get assigned to a
>       * given guest.  But even if not, by replacing the prior value here we
> @@ -1476,7 +1472,7 @@ static int domain_context_mapping(struct
>       * this or other devices may be penalized then, but some would also be
>       * if we left other than NUMA_NO_NODE untouched here.
>       */
> -    if ( drhd->iommu->node != NUMA_NO_NODE )
> +    if ( drhd && drhd->iommu->node != NUMA_NO_NODE )
>          dom_iommu(domain)->node = drhd->iommu->node;
> 
>      ASSERT(pcidevs_locked());
> @@ -1497,6 +1493,9 @@ static int domain_context_mapping(struct
>          break;
> 
>      case DEV_TYPE_PCIe_ENDPOINT:
> +        if ( !drhd )
> +            return -ENODEV;
> +
>          if ( iommu_debug )
>              printk(VTDPREFIX "%pd:PCIe: map %pp\n",
>                     domain, &PCI_SBDF3(seg, bus, devfn));
> @@ -1508,6 +1507,9 @@ static int domain_context_mapping(struct
>          break;
> 
>      case DEV_TYPE_PCI:
> +        if ( !drhd )
> +            return -ENODEV;
> +
>          if ( iommu_debug )
>              printk(VTDPREFIX "%pd:PCI: map %pp\n",
>                     domain, &PCI_SBDF3(seg, bus, devfn));
> @@ -1651,17 +1653,12 @@ int domain_context_unmap_one(
>  static int domain_context_unmap(struct domain *domain, u8 devfn,
>                                  struct pci_dev *pdev)
>  {
> -    struct acpi_drhd_unit *drhd;
> -    struct vtd_iommu *iommu;
> +    const struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev);
> +    struct vtd_iommu *iommu = drhd ? drhd->iommu : NULL;
>      int ret;
>      u8 seg = pdev->seg, bus = pdev->bus, tmp_bus, tmp_devfn, secbus;
>      int found = 0;
> 
> -    drhd = acpi_find_matched_drhd_unit(pdev);
> -    if ( !drhd )
> -        return -ENODEV;
> -    iommu = drhd->iommu;
> -
>      switch ( pdev->type )
>      {
>      case DEV_TYPE_PCI_HOST_BRIDGE:
> @@ -1676,6 +1673,9 @@ static int domain_context_unmap(struct d
>          return 0;
> 
>      case DEV_TYPE_PCIe_ENDPOINT:
> +        if ( !iommu )
> +            return -ENODEV;
> +
>          if ( iommu_debug )
>              printk(VTDPREFIX "%pd:PCIe: unmap %pp\n",
>                     domain, &PCI_SBDF3(seg, bus, devfn));
> @@ -1686,6 +1686,9 @@ static int domain_context_unmap(struct d
>          break;
> 
>      case DEV_TYPE_PCI:
> +        if ( !iommu )
> +            return -ENODEV;
> +
>          if ( iommu_debug )
>              printk(VTDPREFIX "%pd:PCI: unmap %pp\n",
>                     domain, &PCI_SBDF3(seg, bus, devfn));

RE: [PATCH 2/4] VT-d: consider hidden devices when unmapping

[Tian, Kevin]

> From: Jan Beulich
> Sent: Wednesday, September 15, 2021 5:13 PM
> 
> Whether to clear an IOMMU's bit in the domain's bitmap should depend on
> all devices the domain can control. For the hardware domain this
> includes hidden devices, which are associated with DomXEN.
> 
> While touching related logic, convert "found" to "bool".
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1657,7 +1657,7 @@ static int domain_context_unmap(struct d
>      struct vtd_iommu *iommu = drhd ? drhd->iommu : NULL;
>      int ret;
>      u8 seg = pdev->seg, bus = pdev->bus, tmp_bus, tmp_devfn, secbus;
> -    int found = 0;
> +    bool found;
> 
>      switch ( pdev->type )
>      {
> @@ -1737,23 +1737,33 @@ static int domain_context_unmap(struct d
>          return ret;
> 
>      /*
> -     * if no other devices under the same iommu owned by this domain,
> -     * clear iommu in iommu_bitmap and clear domain_id in domid_bitmp
> +     * If no other devices under the same iommu owned by this domain,
> +     * clear iommu in iommu_bitmap and clear domain_id in domid_bitmap.

what is changed above?

>       */
> -    for_each_pdev ( domain, pdev )
> +    for ( found = false; ; domain = dom_xen )

honesty speaking I had to read several times to understand the break
condition of this loop. It'd be easier to understand if putting for_each_
pdev(){} into a function and then:

	found = func(domain);
	if (!found && is_hardware_domain(domain))
		found = func(dom_xen);

>      {
> -        if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
> -            continue;
> -
> -        drhd = acpi_find_matched_drhd_unit(pdev);
> -        if ( drhd && drhd->iommu == iommu )
> +        for_each_pdev ( domain, pdev )
>          {
> -            found = 1;
> -            break;
> +            if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
> +                continue;
> +
> +            drhd = acpi_find_matched_drhd_unit(pdev);
> +            if ( drhd && drhd->iommu == iommu )
> +            {
> +                found = true;
> +                break;
> +            }
>          }
> +
> +        /*
> +         * Hidden devices are associated with DomXEN but usable by the
> +         * hardware domain. Hence they need considering here as well.
> +         */
> +        if ( found || !is_hardware_domain(domain) )
> +            break;
>      }
> 
> -    if ( found == 0 )
> +    if ( !found )
>      {
>          clear_bit(iommu->index, &dom_iommu(domain)-
> >arch.vtd.iommu_bitmap);
>          cleanup_domid_map(domain, iommu);
> 

RE: [PATCH 3/4] VT-d: skip IOMMU bitmap cleanup for phantom devices

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, September 15, 2021 5:13 PM
> 
> Doing the cleanup also for phantom devices is at best redundant with
> doing it for the corresponding real device. I couldn't force myself into
> checking all the code paths whether it really is: It seems better to
> explicitly skip this step in such cases.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1733,7 +1733,7 @@ static int domain_context_unmap(struct d
>          return -EINVAL;
>      }
> 
> -    if ( ret || QUARANTINE_SKIP(domain) )
> +    if ( ret || QUARANTINE_SKIP(domain) || pdev->devfn != devfn )
>          return ret;
> 
>      /*

Re: [PATCH 2/4] VT-d: consider hidden devices when unmapping

[Jan Beulich]

On 16.09.2021 10:18, Tian, Kevin wrote:
>> @@ -1737,23 +1737,33 @@ static int domain_context_unmap(struct d
>>          return ret;
>>
>>      /*
>> -     * if no other devices under the same iommu owned by this domain,
>> -     * clear iommu in iommu_bitmap and clear domain_id in domid_bitmp
>> +     * If no other devices under the same iommu owned by this domain,
>> +     * clear iommu in iommu_bitmap and clear domain_id in domid_bitmap.
> 
> what is changed above?

A style and a spelling correction: Capital letter at the start and
a missing 'a' in domid_bitmap.

>>       */
>> -    for_each_pdev ( domain, pdev )
>> +    for ( found = false; ; domain = dom_xen )
> 
> honesty speaking I had to read several times to understand the break
> condition of this loop. It'd be easier to understand if putting for_each_
> pdev(){} into a function and then:
> 
> 	found = func(domain);
> 	if (!found && is_hardware_domain(domain))
> 		found = func(dom_xen);

Can do so, sure.

Jan