* [PATCH] hw/arm/smmu-common: a fix to smmu_find_smmu_pcibus @ 2020-02-26 17:26 Eric Auger 2020-02-26 17:37 ` Philippe Mathieu-Daudé 2020-02-26 17:57 ` Peter Xu 0 siblings, 2 replies; 6+ messages in thread From: Eric Auger @ 2020-02-26 17:26 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell, peterx 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/arm/smmu-common: a fix to smmu_find_smmu_pcibus 2020-02-26 17:26 [PATCH] hw/arm/smmu-common: a fix to smmu_find_smmu_pcibus Eric Auger @ 2020-02-26 17:37 ` Philippe Mathieu-Daudé 2020-02-26 17:42 ` Philippe Mathieu-Daudé 2020-03-02 11:02 ` Peter Maydell 2020-02-26 17:57 ` Peter Xu 1 sibling, 2 replies; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2020-02-26 17:37 UTC (permalink / raw) To: Eric Auger, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell, peterx 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/arm/smmu-common: a fix to smmu_find_smmu_pcibus 2020-02-26 17:37 ` Philippe Mathieu-Daudé @ 2020-02-26 17:42 ` Philippe Mathieu-Daudé 2020-03-02 11:02 ` Peter Maydell 1 sibling, 0 replies; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2020-02-26 17:42 UTC (permalink / raw) To: Eric Auger, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell, peterx 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/arm/smmu-common: a fix to smmu_find_smmu_pcibus 2020-02-26 17:37 ` Philippe Mathieu-Daudé 2020-02-26 17:42 ` Philippe Mathieu-Daudé @ 2020-03-02 11:02 ` Peter Maydell 2020-03-02 11:34 ` Peter Maydell 1 sibling, 1 reply; 6+ messages in thread From: Peter Maydell @ 2020-03-02 11:02 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Eric Auger, qemu-arm, QEMU Developers, Peter Xu, Eric Auger 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/arm/smmu-common: a fix to smmu_find_smmu_pcibus 2020-03-02 11:02 ` Peter Maydell @ 2020-03-02 11:34 ` Peter Maydell 0 siblings, 0 replies; 6+ messages in thread From: Peter Maydell @ 2020-03-02 11:34 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Eric Auger, qemu-arm, QEMU Developers, Peter Xu, Eric Auger 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/arm/smmu-common: a fix to smmu_find_smmu_pcibus 2020-02-26 17:26 [PATCH] hw/arm/smmu-common: a fix to smmu_find_smmu_pcibus Eric Auger 2020-02-26 17:37 ` Philippe Mathieu-Daudé @ 2020-02-26 17:57 ` Peter Xu 1 sibling, 0 replies; 6+ messages in thread From: Peter Xu @ 2020-02-26 17:57 UTC (permalink / raw) To: Eric Auger; +Cc: peter.maydell, qemu-arm, qemu-devel, eric.auger.pro 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-02 11:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-26 17:26 [PATCH] hw/arm/smmu-common: a fix to smmu_find_smmu_pcibus Eric Auger 2020-02-26 17:37 ` Philippe Mathieu-Daudé 2020-02-26 17:42 ` Philippe Mathieu-Daudé 2020-03-02 11:02 ` Peter Maydell 2020-03-02 11:34 ` Peter Maydell 2020-02-26 17:57 ` Peter Xu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.