[PATCH] hw/i386/intel_iommu: Simplify vtd_find_as_from_bus_num() logic

[PATCH] hw/i386/intel_iommu: Simplify vtd_find_as_from_bus_num() logic

[Philippe Mathieu-Daudé]

Reorder the if() statement to simplify the function.
This avoid bugs like the one fixed by commit a2e1cd41ccf.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/intel_iommu.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 6258c58ac9..e720a8939c 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -987,14 +987,17 @@ static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t level)
 static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
 {
     VTDBus *vtd_bus = s->vtd_as_by_bus_num[bus_num];
-    if (!vtd_bus) {
+    GHashTableIter iter;
+
+    if (vtd_bus) {
+        return vtd_bus;
+    }
+
     /*
      * Iterate over the registered buses to find the one which
      * currently hold this bus number, and update the bus_num
      * lookup table:
      */
-        GHashTableIter iter;
-
     g_hash_table_iter_init(&iter, s->vtd_as_by_busptr);
     while (g_hash_table_iter_next(&iter, NULL, (void **)&vtd_bus)) {
         if (pci_bus_num(vtd_bus->bus) == bus_num) {
@@ -1002,9 +1005,8 @@ static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
             return vtd_bus;
         }
     }
-        vtd_bus = NULL;
-    }
-    return vtd_bus;
+
+    return NULL;
 }
 
 /* Given the @iova, get relevant @slptep. @slpte_level will be the last level
-- 
2.21.1

Re: [PATCH] hw/i386/intel_iommu: Simplify vtd_find_as_from_bus_num() logic

[Peter Xu]

On Wed, Feb 26, 2020 at 06:49:56PM +0100, Philippe Mathieu-Daudé wrote:
> Reorder the if() statement to simplify the function.
> This avoid bugs like the one fixed by commit a2e1cd41ccf.

This seems to hint that this patch fixes a problem, but it's pure
refactoring, right?

And I'm not sure about git-diff -w or so, but... I can't apply such a
patch cleanly using "git am".  Are you sure a patch like this would
work (without space change information)?

Thanks,

> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 6258c58ac9..e720a8939c 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -987,14 +987,17 @@ static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t level)
>  static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
>  {
>      VTDBus *vtd_bus = s->vtd_as_by_bus_num[bus_num];
> -    if (!vtd_bus) {
> +    GHashTableIter iter;
> +
> +    if (vtd_bus) {
> +        return vtd_bus;
> +    }
> +
>      /*
>       * Iterate over the registered buses to find the one which
>       * currently hold this bus number, and update the bus_num
>       * lookup table:
>       */
> -        GHashTableIter iter;
> -
>      g_hash_table_iter_init(&iter, s->vtd_as_by_busptr);
>      while (g_hash_table_iter_next(&iter, NULL, (void **)&vtd_bus)) {
>          if (pci_bus_num(vtd_bus->bus) == bus_num) {
> @@ -1002,9 +1005,8 @@ static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
>              return vtd_bus;
>          }
>      }
> -        vtd_bus = NULL;
> -    }
> -    return vtd_bus;
> +
> +    return NULL;
>  }
>  
>  /* Given the @iova, get relevant @slptep. @slpte_level will be the last level
> -- 
> 2.21.1
> 

-- 
Peter Xu

Re: [PATCH] hw/i386/intel_iommu: Simplify vtd_find_as_from_bus_num() logic

[Philippe Mathieu-Daudé]

On 2/26/20 7:01 PM, Peter Xu wrote:
> On Wed, Feb 26, 2020 at 06:49:56PM +0100, Philippe Mathieu-Daudé wrote:
>> Reorder the if() statement to simplify the function.
>> This avoid bugs like the one fixed by commit a2e1cd41ccf.
> 
> This seems to hint that this patch fixes a problem, but it's pure
> refactoring, right?

Pure refactoring yes, I meant: If the code was written this way, we 
wouldn't had the bug fixed by a2e1cd41ccf.

> 
> And I'm not sure about git-diff -w or so, but... I can't apply such a
> patch cleanly using "git am".  Are you sure a patch like this would
> work (without space change information)?

Ah sorry I didn't think of that (I really thought git-am would apply it).

> 
> Thanks,
> 
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   hw/i386/intel_iommu.c | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 6258c58ac9..e720a8939c 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -987,14 +987,17 @@ static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t level)
>>   static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
>>   {
>>       VTDBus *vtd_bus = s->vtd_as_by_bus_num[bus_num];
>> -    if (!vtd_bus) {
>> +    GHashTableIter iter;
>> +
>> +    if (vtd_bus) {
>> +        return vtd_bus;
>> +    }
>> +
>>       /*
>>        * Iterate over the registered buses to find the one which
>>        * currently hold this bus number, and update the bus_num
>>        * lookup table:
>>        */
>> -        GHashTableIter iter;
>> -
>>       g_hash_table_iter_init(&iter, s->vtd_as_by_busptr);
>>       while (g_hash_table_iter_next(&iter, NULL, (void **)&vtd_bus)) {
>>           if (pci_bus_num(vtd_bus->bus) == bus_num) {
>> @@ -1002,9 +1005,8 @@ static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
>>               return vtd_bus;
>>           }
>>       }
>> -        vtd_bus = NULL;
>> -    }
>> -    return vtd_bus;
>> +
>> +    return NULL;
>>   }
>>   
>>   /* Given the @iova, get relevant @slptep. @slpte_level will be the last level
>> -- 
>> 2.21.1
>>
>