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

[Liu, Yi L]

> From: Peter Xu [mailto:address@hidden]
> Sent: Friday, November 1, 2019 10:55 PM
> To: Liu, Yi L <address@hidden>
> Subject: Re: [RFC v2 08/22] intel_iommu: provide get_iommu_context() callback
> 
> 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()?

Just wanted to make the protected snippet smaller. But I'm fine to move it
to vtd_find_add_as() if there is no much value for putting it here.

> 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.

Got it. It functions if you missed to put a mirrored unlock after a lock. (joke)

> 
> > +    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]?

yes, it's old writing. Will modify it.

> 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]

exactly.

> 
> >  };
> >
> >  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
> >

Thanks,
Yi Liu