> From: Peter Xu [mailto:peterx@xxxxxxxxxx] > Sent: Friday, November 1, 2019 10:55 PM > To: Liu, Yi L <yi.l.liu@xxxxxxxxx> > 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 <kevin.tian@xxxxxxxxx> > > Cc: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx> > > Cc: Peter Xu <peterx@xxxxxxxxxx> > > Cc: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> > > Signed-off-by: Liu Yi L <yi.l.liu@xxxxxxxxx> > > --- > > 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
