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

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

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 <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:

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?