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é <address@hidden>
> > ---
> >   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