[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] hw/i386/intel_iommu: Simplify vtd_find_as_from_bus_num()
From: |
Paolo Bonzini |
Subject: |
Re: [PATCH v2] hw/i386/intel_iommu: Simplify vtd_find_as_from_bus_num() logic |
Date: |
Fri, 28 Feb 2020 11:40:32 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 |
On 26/02/20 21:10, Philippe Mathieu-Daudé wrote:
> The vtd_find_as_from_bus_num() function was introduced (in commit
> dbaabb25f) in a code format that could return an incorrect pointer,
> which was later fixed by commit a2e1cd41ccf.
> We could have avoid this by writing the if() statement differently.
> Do it now, in case this function is re-used. The code is easier to
> review (harder to miss bugs).
>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
> Since v2: Re-worded commit description, patch sent without -w.
>
> This patch is easier to review with 'git-diff -w' (--ignore-all-space):
>
> @@ -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;
> }
> ---
> hw/i386/intel_iommu.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 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,24 +987,26 @@ 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) {
> - /*
> - * Iterate over the registered buses to find the one which
> - * currently hold this bus number, and update the bus_num
> - * lookup table:
> - */
> - GHashTableIter iter;
> + 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) {
> - s->vtd_as_by_bus_num[bus_num] = vtd_bus;
> - return vtd_bus;
> - }
> - }
> - vtd_bus = NULL;
> + if (vtd_bus) {
> + return 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:
> + */
> + 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) {
> + s->vtd_as_by_bus_num[bus_num] = vtd_bus;
> + return vtd_bus;
> + }
> + }
> +
> + return NULL;
> }
>
> /* Given the @iova, get relevant @slptep. @slpte_level will be the last level
>
Queued, thanks.
Paolo