Re: [Qemu-devel] [PATCH v2 1/2] intel_iommu: move ce fetching out when sync shadow

[Auger Eric]

Hi Peter,

On 10/8/18 8:47 AM, Peter Xu wrote:
> There are two callers for vtd_sync_shadow_page_table_range(): one
> provided a valid context entry and one not.  Move that fetching
> operation into the caller vtd_sync_shadow_page_table() where we need to
> fetch the context entry.
> 
> Meanwhile, remove the error_report_once() directly since we're already
> tracing all the error cases in the previous call.  Instead, return error
> number back to caller.  This will not change anything functional since
> callers are dropping it after all.
> 
> We do this move majorly because we want to do something more later in
> vtd_sync_shadow_page_table().
> 
> Signed-off-by: Peter Xu <address@hidden>
Reviewed-by: Eric Auger <address@hidden>

Thanks

Eric

> ---
>  dtc                   |  2 +-
>  hw/i386/intel_iommu.c | 41 +++++++++++++----------------------------
>  2 files changed, 14 insertions(+), 29 deletions(-)
> 
> diff --git a/dtc b/dtc
> index 88f18909db..e54388015a 160000
> --- a/dtc
> +++ b/dtc
> @@ -1 +1 @@
> -Subproject commit 88f18909db731a627456f26d779445f84e449536
> +Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 3dfada19a6..d6b4f8705d 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1035,7 +1035,6 @@ static int vtd_sync_shadow_page_hook(IOMMUTLBEntry 
> *entry,
>      return 0;
>  }
>  
> -/* If context entry is NULL, we'll try to fetch it on our own. */
>  static int vtd_sync_shadow_page_table_range(VTDAddressSpace *vtd_as,
>                                              VTDContextEntry *ce,
>                                              hwaddr addr, hwaddr size)
> @@ -1047,39 +1046,25 @@ static int 
> vtd_sync_shadow_page_table_range(VTDAddressSpace *vtd_as,
>          .notify_unmap = true,
>          .aw = s->aw_bits,
>          .as = vtd_as,
> +        .domain_id = VTD_CONTEXT_ENTRY_DID(ce->hi),
>      };
> -    VTDContextEntry ce_cache;
> -    int ret;
> -
> -    if (ce) {
> -        /* If the caller provided context entry, use it */
> -        ce_cache = *ce;
> -    } else {
> -        /* If the caller didn't provide ce, try to fetch */
> -        ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),
> -                                       vtd_as->devfn, &ce_cache);
> -        if (ret) {
> -            /*
> -             * This should not really happen, but in case it happens,
> -             * we just skip the sync for this time.  After all we even
> -             * don't have the root table pointer!
> -             */
> -            error_report_once("%s: invalid context entry for bus 0x%x"
> -                              " devfn 0x%x",
> -                              __func__, pci_bus_num(vtd_as->bus),
> -                              vtd_as->devfn);
> -            return 0;
> -        }
> -    }
>  
> -    info.domain_id = VTD_CONTEXT_ENTRY_DID(ce_cache.hi);
> -
> -    return vtd_page_walk(&ce_cache, addr, addr + size, &info);
> +    return vtd_page_walk(ce, addr, addr + size, &info);
>  }
>  
>  static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as)
>  {
> -    return vtd_sync_shadow_page_table_range(vtd_as, NULL, 0, UINT64_MAX);
> +    int ret;
> +    VTDContextEntry ce;
> +
> +    ret = vtd_dev_to_context_entry(vtd_as->iommu_state,
> +                                   pci_bus_num(vtd_as->bus),
> +                                   vtd_as->devfn, &ce);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    return vtd_sync_shadow_page_table_range(vtd_as, &ce, 0, UINT64_MAX);
>  }
>  
>  /*
>