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

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

[Eric Auger]

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;
 }
-- 
2.20.1

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

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

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

[Peter Xu]

On Wed, Feb 26, 2020 at 06:26:28PM +0100, 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>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

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

[Peter Maydell]

On Wed, 26 Feb 2020 at 17:37, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi Eric,

> Patch is easy to review but code not. By inverting the if() statement I
> find the code easier to review.



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

I think I agree with Philippe here -- the current code is already
a bit confusing in that it looks at first glance as if the "find
the bus in the hash table" code works by falling through into
the "we already had the cached value" code on success, but in
fact the success case is dealt with by the return in the middle
of the while loop and it's only the not-found case that falls
through. Adding this patch on top fixes the bug but leaves in
place the odd code structure that caused it. Rearranging it as
Philippe does above makes it much clearer what's happening,
I think.

Would one of you like to submit a patch that does it that way
round, please?

thanks
-- PMM

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

[Peter Maydell]

On Mon, 2 Mar 2020 at 11:02, Peter Maydell <peter.maydell@linaro.org> wrote:
> Would one of you like to submit a patch that does it that way
> round, please?

Aha, I see you already did:
https://patchew.org/QEMU/20200227164728.11635-1-philmd@redhat.com/
(I process my to-review queue mostly oldest-first).

thanks
-- PMM