| Message ID | 20200226172628.17449-1-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/smmu-common: a fix to smmu_find_smmu_pcibus | expand |
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?
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?
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>
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
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
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; }
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(+)