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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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 <eric.auger@redhat.com>
> ---
>   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?