Re: [PATCH] hw/arm/smmu-common: a fix to smmu_find_smmu_pcibus

[Philippe Mathieu-Daudé]

Hi Eric,

On 2/26/20 6:26 PM, Eric Auger wrote:
> Make sure a null SMMUPciBus is returned in case we were
> not able to identify a pci bus matching the @bus_num.
>
> This matches the fix done on intel iommu in commit:
> a2e1cd41ccfe796529abfd1b6aeb1dd4393762a2
>
> Signed-off-by: Eric Auger <address@hidden>
> ---
>   hw/arm/smmu-common.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 0f2573f004..67d7b2d0fd 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -301,6 +301,7 @@ SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, uint8_t 
> bus_num)
>                   return smmu_pci_bus;
>               }
>           }
> +        smmu_pci_bus = NULL;
>       }
>       return smmu_pci_bus;
>   }

Patch is easy to review but code not. By inverting the if() statement I 
find the code easier to review. The patch isn't however:

-- >8 --
@@ -284,25 +284,27 @@ inline int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t 
iova, IOMMUAccessFlags perm,
 /**
  * The bus number is used for lookup when SID based invalidation occurs.
  * In that case we lazily populate the SMMUPciBus array from the bus hash
  * table. At the time the SMMUPciBus is created (smmu_find_add_as), 
the bus
  * numbers may not be always initialized yet.
  */
 SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, uint8_t bus_num)
 {
     SMMUPciBus *smmu_pci_bus = s->smmu_pcibus_by_bus_num[bus_num];
+    GHashTableIter iter;

-    if (!smmu_pci_bus) {
-        GHashTableIter iter;
+    if (smmu_pci_bus) {
+        return smmu_pci_bus;
+    }

-        g_hash_table_iter_init(&iter, s->smmu_pcibus_by_busptr);
-        while (g_hash_table_iter_next(&iter, NULL, (void 
**)&smmu_pci_bus)) {
-            if (pci_bus_num(smmu_pci_bus->bus) == bus_num) {
-                s->smmu_pcibus_by_bus_num[bus_num] = smmu_pci_bus;
-                return smmu_pci_bus;
-            }
+    g_hash_table_iter_init(&iter, s->smmu_pcibus_by_busptr);
+    while (g_hash_table_iter_next(&iter, NULL, (void **)&smmu_pci_bus)) {
+        if (pci_bus_num(smmu_pci_bus->bus) == bus_num) {
+            s->smmu_pcibus_by_bus_num[bus_num] = smmu_pci_bus;
+            return smmu_pci_bus;
         }
     }
-    return smmu_pci_bus;
+
+    return NULL;
 }
---

The code is easier although:

SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, uint8_t bus_num)
{
    SMMUPciBus *smmu_pci_bus = s->smmu_pcibus_by_bus_num[bus_num];
    GHashTableIter iter;

    if (smmu_pci_bus) {
        return smmu_pci_bus;
    }

    g_hash_table_iter_init(&iter, s->smmu_pcibus_by_busptr);
    while (g_hash_table_iter_next(&iter, NULL, (void **)&smmu_pci_bus)) {
        if (pci_bus_num(smmu_pci_bus->bus) == bus_num) {
            s->smmu_pcibus_by_bus_num[bus_num] = smmu_pci_bus;
            return smmu_pci_bus;
        }
    }

    return NULL;
}

What do you think?