Re: [RFC v2 08/22] intel_iommu: provide get_iommu_context() callback

[Peter Xu]

On Thu, Oct 24, 2019 at 08:34:29AM -0400, Liu Yi L wrote:
> This patch adds get_iommu_context() callback to return an iommu_context
> Intel VT-d platform.
> 
> Cc: Kevin Tian <address@hidden>
> Cc: Jacob Pan <address@hidden>
> Cc: Peter Xu <address@hidden>
> Cc: Yi Sun <address@hidden>
> Signed-off-by: Liu Yi L <address@hidden>
> ---
>  hw/i386/intel_iommu.c         | 57 
> ++++++++++++++++++++++++++++++++++++++-----
>  include/hw/i386/intel_iommu.h | 14 ++++++++++-
>  2 files changed, 64 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 67a7836..e9f8692 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3288,22 +3288,33 @@ static const MemoryRegionOps vtd_mem_ir_ops = {
>      },
>  };
>  
> -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
> +static VTDBus *vtd_find_add_bus(IntelIOMMUState *s, PCIBus *bus)
>  {
>      uintptr_t key = (uintptr_t)bus;
> -    VTDBus *vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr, &key);
> -    VTDAddressSpace *vtd_dev_as;
> -    char name[128];
> +    VTDBus *vtd_bus;
>  
> +    vtd_iommu_lock(s);

Why explicitly take the IOMMU lock here?  I mean, it's fine to take
it, but if so why not take it to cover the whole vtd_find_add_as()?

For now it'll be fine in either way because I believe iommu_lock is
not really functioning when we're still with BQL here, however if you
add that explicitly then I don't see why it's not covering that.

> +    vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr, &key);
>      if (!vtd_bus) {
>          uintptr_t *new_key = g_malloc(sizeof(*new_key));
>          *new_key = (uintptr_t)bus;
>          /* No corresponding free() */
> -        vtd_bus = g_malloc0(sizeof(VTDBus) + sizeof(VTDAddressSpace *) * \
> -                            PCI_DEVFN_MAX);
> +        vtd_bus = g_malloc0(sizeof(VTDBus) + PCI_DEVFN_MAX * \
> +                    (sizeof(VTDAddressSpace *) + sizeof(VTDIOMMUContext *)));

Should this be as simple as g_malloc0(sizeof(VTDBus) since [1]?

Otherwise the patch looks sane to me.

>          vtd_bus->bus = bus;
>          g_hash_table_insert(s->vtd_as_by_busptr, new_key, vtd_bus);
>      }
> +    vtd_iommu_unlock(s);
> +    return vtd_bus;
> +}

[...]

>  struct VTDBus {
>      PCIBus* bus;             /* A reference to the bus to provide 
> translation for */
> -    VTDAddressSpace *dev_as[0];      /* A table of VTDAddressSpace objects 
> indexed by devfn */
> +    /* A table of VTDAddressSpace objects indexed by devfn */
> +    VTDAddressSpace *dev_as[PCI_DEVFN_MAX];
> +    /* A table of VTDIOMMUContext objects indexed by devfn */
> +    VTDIOMMUContext *dev_ic[PCI_DEVFN_MAX];

[1]

>  };
>  
>  struct VTDIOTLBEntry {
> @@ -282,5 +293,6 @@ struct IntelIOMMUState {
>   * create a new one if none exists
>   */
>  VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn);
> +VTDIOMMUContext *vtd_find_add_ic(IntelIOMMUState *s, PCIBus *bus, int devfn);
>  
>  #endif
> -- 
> 2.7.4
> 

Regards,

-- 
Peter Xu