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

[Philippe Mathieu-Daudé]

On 2/26/20 6:37 PM, Philippe Mathieu-Daudé wrote:
> 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:

I used 'git-diff -W' instead of 'git-diff -w'. -w works better:

-- >8 --
@@ -290,10 +290,12 @@ inline int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t 
iova, IOMMUAccessFlags perm,
 SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, uint8_t bus_num)
 {
     SMMUPciBus *smmu_pci_bus = s->smmu_pcibus_by_bus_num[bus_num];
-
-    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) {
@@ -301,8 +303,8 @@ SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, 
uint8_t bus_num)
             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?