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);

> 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);
> 

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