Message ID | 20200226174956.17018-1-philmd@redhat.com |
---|---|
State | New |
Headers | show |
Series | hw/i386/intel_iommu: Simplify vtd_find_as_from_bus_num() logic | expand |
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 >
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 >> >
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
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(-)