| Message ID | 1585542301-84087-8-git-send-email-yi.l.liu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | intel_iommu: expose Shared Virtual Addressing to VMs | expand |
Yi, On 3/30/20 6:24 AM, Liu Yi L wrote: > This patch adds set/unset_iommu_context() impelementation in Intel This patch implements the set/unset_iommu_context() ops for Intel vIOMMU. > vIOMMU. For Intel platform, pass-through modules (e.g. VFIO) could > set HostIOMMUContext to Intel vIOMMU emulator. > > Cc: Kevin Tian <kevin.tian@intel.com> > Cc: Jacob Pan <jacob.jun.pan@linux.intel.com> > Cc: Peter Xu <peterx@redhat.com> > Cc: Yi Sun <yi.y.sun@linux.intel.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Richard Henderson <rth@twiddle.net> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Liu Yi L <yi.l.liu@intel.com> > --- > hw/i386/intel_iommu.c | 71 ++++++++++++++++++++++++++++++++++++++++--- > include/hw/i386/intel_iommu.h | 21 ++++++++++--- > 2 files changed, 83 insertions(+), 9 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 4b22910..fd349c6 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -3354,23 +3354,33 @@ static const MemoryRegionOps vtd_mem_ir_ops = { > }, > }; > > -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) > +/** > + * Fetch a VTDBus instance for given PCIBus. If no existing instance, > + * allocate one. > + */ > +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]; > > 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)); > vtd_bus->bus = bus; > g_hash_table_insert(s->vtd_as_by_busptr, new_key, vtd_bus); > } > + return vtd_bus; > +} > > +VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) > +{ > + VTDBus *vtd_bus; > + VTDAddressSpace *vtd_dev_as; > + char name[128]; > + > + vtd_bus = vtd_find_add_bus(s, bus); > vtd_dev_as = vtd_bus->dev_as[devfn]; > > if (!vtd_dev_as) { > @@ -3436,6 +3446,55 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) > return vtd_dev_as; > } > > +static int vtd_dev_set_iommu_context(PCIBus *bus, void *opaque, > + int devfn, > + HostIOMMUContext *iommu_ctx) > +{ > + IntelIOMMUState *s = opaque; > + VTDBus *vtd_bus; > + VTDHostIOMMUContext *vtd_dev_icx; > + > + assert(0 <= devfn && devfn < PCI_DEVFN_MAX); > + > + vtd_bus = vtd_find_add_bus(s, bus); > + > + vtd_iommu_lock(s); > + > + vtd_dev_icx = vtd_bus->dev_icx[devfn]; > + > + assert(!vtd_dev_icx); > + > + vtd_bus->dev_icx[devfn] = vtd_dev_icx = > + g_malloc0(sizeof(VTDHostIOMMUContext)); > + vtd_dev_icx->vtd_bus = vtd_bus; > + vtd_dev_icx->devfn = (uint8_t)devfn; > + vtd_dev_icx->iommu_state = s; > + vtd_dev_icx->iommu_ctx = iommu_ctx; > + > + vtd_iommu_unlock(s); > + > + return 0; > +} > + > +static void vtd_dev_unset_iommu_context(PCIBus *bus, void *opaque, int devfn) > +{ > + IntelIOMMUState *s = opaque; > + VTDBus *vtd_bus; > + VTDHostIOMMUContext *vtd_dev_icx; > + > + assert(0 <= devfn && devfn < PCI_DEVFN_MAX); > + > + vtd_bus = vtd_find_add_bus(s, bus); > + > + vtd_iommu_lock(s); > + > + vtd_dev_icx = vtd_bus->dev_icx[devfn]; > + g_free(vtd_dev_icx); > + vtd_bus->dev_icx[devfn] = NULL; > + > + vtd_iommu_unlock(s); > +} > + > static uint64_t get_naturally_aligned_size(uint64_t start, > uint64_t size, int gaw) > { > @@ -3731,6 +3790,8 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) > > static PCIIOMMUOps vtd_iommu_ops = { > .get_address_space = vtd_host_dma_iommu, > + .set_iommu_context = vtd_dev_set_iommu_context, > + .unset_iommu_context = vtd_dev_unset_iommu_context, > }; > > static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index 3870052..b5fefb9 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -64,6 +64,7 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry; > typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress; > typedef struct VTDPASIDDirEntry VTDPASIDDirEntry; > typedef struct VTDPASIDEntry VTDPASIDEntry; > +typedef struct VTDHostIOMMUContext VTDHostIOMMUContext; > > /* Context-Entry */ > struct VTDContextEntry { > @@ -112,10 +113,20 @@ struct VTDAddressSpace { > IOVATree *iova_tree; /* Traces mapped IOVA ranges */ > }; > > +struct VTDHostIOMMUContext { > + VTDBus *vtd_bus; > + uint8_t devfn; > + HostIOMMUContext *iommu_ctx; I don't get why we don't have standard QOM inheritance instead of this handle? VTDHostContext parent_obj; like IOMMUMemoryRegion <- MemoryRegion <- Object > + IntelIOMMUState *iommu_state; > +}; > + > struct VTDBus { > - PCIBus* bus; /* A reference to the bus to provide translation for */ > + /* A reference to the bus to provide translation for */ > + PCIBus *bus; > /* A table of VTDAddressSpace objects indexed by devfn */ > - VTDAddressSpace *dev_as[]; > + VTDAddressSpace *dev_as[PCI_DEVFN_MAX]; > + /* A table of VTDHostIOMMUContext objects indexed by devfn */ > + VTDHostIOMMUContext *dev_icx[PCI_DEVFN_MAX]; At this point of the review, it is unclear to me why the context is associated to a device. Up to now you have not explained it should. If so why isn't it part of VTDAddressSpace? Thanks Eric > }; > > struct VTDIOTLBEntry { > @@ -269,8 +280,10 @@ struct IntelIOMMUState { > bool dma_drain; /* Whether DMA r/w draining enabled */ > > /* > - * Protects IOMMU states in general. Currently it protects the > - * per-IOMMU IOTLB cache, and context entry cache in VTDAddressSpace. > + * iommu_lock protects below: > + * - per-IOMMU IOTLB caches > + * - context entry cache in VTDAddressSpace > + * - HostIOMMUContext pointer cached in vIOMMU > */ > QemuMutex iommu_lock; > }; >
Hi Eric, > From: Auger Eric < eric.auger@redhat.com> > Sent: Tuesday, March 31, 2020 4:24 AM > To: Liu, Yi L <yi.l.liu@intel.com>; qemu-devel@nongnu.org; > Subject: Re: [PATCH v2 07/22] intel_iommu: add set/unset_iommu_context callback > > Yi, > > On 3/30/20 6:24 AM, Liu Yi L wrote: > > This patch adds set/unset_iommu_context() impelementation in Intel > This patch implements the set/unset_iommu_context() ops for Intel vIOMMU. > > vIOMMU. For Intel platform, pass-through modules (e.g. VFIO) could > > set HostIOMMUContext to Intel vIOMMU emulator. > > > > Cc: Kevin Tian <kevin.tian@intel.com> > > Cc: Jacob Pan <jacob.jun.pan@linux.intel.com> > > Cc: Peter Xu <peterx@redhat.com> > > Cc: Yi Sun <yi.y.sun@linux.intel.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Richard Henderson <rth@twiddle.net> > > Cc: Eduardo Habkost <ehabkost@redhat.com> > > Signed-off-by: Liu Yi L <yi.l.liu@intel.com> > > --- > > hw/i386/intel_iommu.c | 71 > ++++++++++++++++++++++++++++++++++++++++--- > > include/hw/i386/intel_iommu.h | 21 ++++++++++--- > > 2 files changed, 83 insertions(+), 9 deletions(-) > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index 4b22910..fd349c6 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -3354,23 +3354,33 @@ static const MemoryRegionOps vtd_mem_ir_ops = { > > }, > > }; > > > > -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) > > +/** > > + * Fetch a VTDBus instance for given PCIBus. If no existing instance, > > + * allocate one. > > + */ > > +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]; > > > > 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)); > > vtd_bus->bus = bus; > > g_hash_table_insert(s->vtd_as_by_busptr, new_key, vtd_bus); > > } > > + return vtd_bus; > > +} > > > > +VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) > > +{ > > + VTDBus *vtd_bus; > > + VTDAddressSpace *vtd_dev_as; > > + char name[128]; > > + > > + vtd_bus = vtd_find_add_bus(s, bus); > > vtd_dev_as = vtd_bus->dev_as[devfn]; > > > > if (!vtd_dev_as) { > > @@ -3436,6 +3446,55 @@ VTDAddressSpace > *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) > > return vtd_dev_as; > > } > > > > +static int vtd_dev_set_iommu_context(PCIBus *bus, void *opaque, > > + int devfn, > > + HostIOMMUContext *iommu_ctx) > > +{ > > + IntelIOMMUState *s = opaque; > > + VTDBus *vtd_bus; > > + VTDHostIOMMUContext *vtd_dev_icx; > > + > > + assert(0 <= devfn && devfn < PCI_DEVFN_MAX); > > + > > + vtd_bus = vtd_find_add_bus(s, bus); > > + > > + vtd_iommu_lock(s); > > + > > + vtd_dev_icx = vtd_bus->dev_icx[devfn]; > > + > > + assert(!vtd_dev_icx); > > + > > + vtd_bus->dev_icx[devfn] = vtd_dev_icx = > > + g_malloc0(sizeof(VTDHostIOMMUContext)); > > + vtd_dev_icx->vtd_bus = vtd_bus; > > + vtd_dev_icx->devfn = (uint8_t)devfn; > > + vtd_dev_icx->iommu_state = s; > > + vtd_dev_icx->iommu_ctx = iommu_ctx; > > + > > + vtd_iommu_unlock(s); > > + > > + return 0; > > +} > > + > > +static void vtd_dev_unset_iommu_context(PCIBus *bus, void *opaque, int devfn) > > +{ > > + IntelIOMMUState *s = opaque; > > + VTDBus *vtd_bus; > > + VTDHostIOMMUContext *vtd_dev_icx; > > + > > + assert(0 <= devfn && devfn < PCI_DEVFN_MAX); > > + > > + vtd_bus = vtd_find_add_bus(s, bus); > > + > > + vtd_iommu_lock(s); > > + > > + vtd_dev_icx = vtd_bus->dev_icx[devfn]; > > + g_free(vtd_dev_icx); > > + vtd_bus->dev_icx[devfn] = NULL; > > + > > + vtd_iommu_unlock(s); > > +} > > + > > static uint64_t get_naturally_aligned_size(uint64_t start, > > uint64_t size, int gaw) > > { > > @@ -3731,6 +3790,8 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus > *bus, void *opaque, int devfn) > > > > static PCIIOMMUOps vtd_iommu_ops = { > > .get_address_space = vtd_host_dma_iommu, > > + .set_iommu_context = vtd_dev_set_iommu_context, > > + .unset_iommu_context = vtd_dev_unset_iommu_context, > > }; > > > > static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > > index 3870052..b5fefb9 100644 > > --- a/include/hw/i386/intel_iommu.h > > +++ b/include/hw/i386/intel_iommu.h > > @@ -64,6 +64,7 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry; > > typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress; > > typedef struct VTDPASIDDirEntry VTDPASIDDirEntry; > > typedef struct VTDPASIDEntry VTDPASIDEntry; > > +typedef struct VTDHostIOMMUContext VTDHostIOMMUContext; > > > > /* Context-Entry */ > > struct VTDContextEntry { > > @@ -112,10 +113,20 @@ struct VTDAddressSpace { > > IOVATree *iova_tree; /* Traces mapped IOVA ranges */ > > }; > > > > +struct VTDHostIOMMUContext { > > > > + VTDBus *vtd_bus; > > + uint8_t devfn; > > + HostIOMMUContext *iommu_ctx; > I don't get why we don't have standard QOM inheritance instead of this > handle? > VTDHostContext parent_obj; > > like IOMMUMemoryRegion <- MemoryRegion <- Object Here it is not inherit the object. It's just cache the HostIOMMUContext pointer in vIOMMU. Just like AddressSpace, it has a MemoryRegion pointer. Here is the same, VTDHostIOMMUContext is just a wrapper to better manage it in vVT-d. It's not inheriting. > > + IntelIOMMUState *iommu_state; > > +}; > > + > > struct VTDBus { > > - PCIBus* bus; /* A reference to the bus to provide translation for > */ > > + /* A reference to the bus to provide translation for */ > > + PCIBus *bus; > > /* A table of VTDAddressSpace objects indexed by devfn */ > > - VTDAddressSpace *dev_as[]; > > + VTDAddressSpace *dev_as[PCI_DEVFN_MAX]; > > + /* A table of VTDHostIOMMUContext objects indexed by devfn */ > > + VTDHostIOMMUContext *dev_icx[PCI_DEVFN_MAX]; > At this point of the review, it is unclear to me why the context is > associated to a device. HostIOMMUContext can be per-device or not. It depends on how vIOMMU manage it. For vVT-d, it's per device as the container is per-device. > Up to now you have not explained it should. If > so why isn't it part of VTDAddressSpace? Ah, I did have considered it. But I chose to use a separate one as context is not really tied with an addresspace. It's better to mange it with a separate structure. Regards, Yi Liu
Hi Yi, On 3/31/20 2:25 PM, Liu, Yi L wrote: > Hi Eric, > >> From: Auger Eric < eric.auger@redhat.com> >> Sent: Tuesday, March 31, 2020 4:24 AM >> To: Liu, Yi L <yi.l.liu@intel.com>; qemu-devel@nongnu.org; >> Subject: Re: [PATCH v2 07/22] intel_iommu: add set/unset_iommu_context callback >> >> Yi, >> >> On 3/30/20 6:24 AM, Liu Yi L wrote: >>> This patch adds set/unset_iommu_context() impelementation in Intel >> This patch implements the set/unset_iommu_context() ops for Intel vIOMMU. >>> vIOMMU. For Intel platform, pass-through modules (e.g. VFIO) could >>> set HostIOMMUContext to Intel vIOMMU emulator. >>> >>> Cc: Kevin Tian <kevin.tian@intel.com> >>> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com> >>> Cc: Peter Xu <peterx@redhat.com> >>> Cc: Yi Sun <yi.y.sun@linux.intel.com> >>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>> Cc: Richard Henderson <rth@twiddle.net> >>> Cc: Eduardo Habkost <ehabkost@redhat.com> >>> Signed-off-by: Liu Yi L <yi.l.liu@intel.com> >>> --- >>> hw/i386/intel_iommu.c | 71 >> ++++++++++++++++++++++++++++++++++++++++--- >>> include/hw/i386/intel_iommu.h | 21 ++++++++++--- >>> 2 files changed, 83 insertions(+), 9 deletions(-) >>> >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>> index 4b22910..fd349c6 100644 >>> --- a/hw/i386/intel_iommu.c >>> +++ b/hw/i386/intel_iommu.c >>> @@ -3354,23 +3354,33 @@ static const MemoryRegionOps vtd_mem_ir_ops = { >>> }, >>> }; >>> >>> -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) >>> +/** >>> + * Fetch a VTDBus instance for given PCIBus. If no existing instance, >>> + * allocate one. >>> + */ >>> +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]; >>> >>> 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)); >>> vtd_bus->bus = bus; >>> g_hash_table_insert(s->vtd_as_by_busptr, new_key, vtd_bus); >>> } >>> + return vtd_bus; >>> +} >>> >>> +VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) >>> +{ >>> + VTDBus *vtd_bus; >>> + VTDAddressSpace *vtd_dev_as; >>> + char name[128]; >>> + >>> + vtd_bus = vtd_find_add_bus(s, bus); >>> vtd_dev_as = vtd_bus->dev_as[devfn]; >>> >>> if (!vtd_dev_as) { >>> @@ -3436,6 +3446,55 @@ VTDAddressSpace >> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) >>> return vtd_dev_as; >>> } >>> >>> +static int vtd_dev_set_iommu_context(PCIBus *bus, void *opaque, >>> + int devfn, >>> + HostIOMMUContext *iommu_ctx) >>> +{ >>> + IntelIOMMUState *s = opaque; >>> + VTDBus *vtd_bus; >>> + VTDHostIOMMUContext *vtd_dev_icx; >>> + >>> + assert(0 <= devfn && devfn < PCI_DEVFN_MAX); >>> + >>> + vtd_bus = vtd_find_add_bus(s, bus); >>> + >>> + vtd_iommu_lock(s); >>> + >>> + vtd_dev_icx = vtd_bus->dev_icx[devfn]; >>> + >>> + assert(!vtd_dev_icx); >>> + >>> + vtd_bus->dev_icx[devfn] = vtd_dev_icx = >>> + g_malloc0(sizeof(VTDHostIOMMUContext)); >>> + vtd_dev_icx->vtd_bus = vtd_bus; >>> + vtd_dev_icx->devfn = (uint8_t)devfn; >>> + vtd_dev_icx->iommu_state = s; >>> + vtd_dev_icx->iommu_ctx = iommu_ctx; >>> + >>> + vtd_iommu_unlock(s); >>> + >>> + return 0; >>> +} >>> + >>> +static void vtd_dev_unset_iommu_context(PCIBus *bus, void *opaque, int devfn) >>> +{ >>> + IntelIOMMUState *s = opaque; >>> + VTDBus *vtd_bus; >>> + VTDHostIOMMUContext *vtd_dev_icx; >>> + >>> + assert(0 <= devfn && devfn < PCI_DEVFN_MAX); >>> + >>> + vtd_bus = vtd_find_add_bus(s, bus); >>> + >>> + vtd_iommu_lock(s); >>> + >>> + vtd_dev_icx = vtd_bus->dev_icx[devfn]; >>> + g_free(vtd_dev_icx); >>> + vtd_bus->dev_icx[devfn] = NULL; >>> + >>> + vtd_iommu_unlock(s); >>> +} >>> + >>> static uint64_t get_naturally_aligned_size(uint64_t start, >>> uint64_t size, int gaw) >>> { >>> @@ -3731,6 +3790,8 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus >> *bus, void *opaque, int devfn) >>> >>> static PCIIOMMUOps vtd_iommu_ops = { >>> .get_address_space = vtd_host_dma_iommu, >>> + .set_iommu_context = vtd_dev_set_iommu_context, >>> + .unset_iommu_context = vtd_dev_unset_iommu_context, >>> }; >>> >>> static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) >>> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h >>> index 3870052..b5fefb9 100644 >>> --- a/include/hw/i386/intel_iommu.h >>> +++ b/include/hw/i386/intel_iommu.h >>> @@ -64,6 +64,7 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry; >>> typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress; >>> typedef struct VTDPASIDDirEntry VTDPASIDDirEntry; >>> typedef struct VTDPASIDEntry VTDPASIDEntry; >>> +typedef struct VTDHostIOMMUContext VTDHostIOMMUContext; >>> >>> /* Context-Entry */ >>> struct VTDContextEntry { >>> @@ -112,10 +113,20 @@ struct VTDAddressSpace { >>> IOVATree *iova_tree; /* Traces mapped IOVA ranges */ >>> }; >>> >>> +struct VTDHostIOMMUContext { >> >> >>> + VTDBus *vtd_bus; >>> + uint8_t devfn; >>> + HostIOMMUContext *iommu_ctx; >> I don't get why we don't have standard QOM inheritance instead of this >> handle? >> VTDHostContext parent_obj; >> >> like IOMMUMemoryRegion <- MemoryRegion <- Object > > Here it is not inherit the object. It's just cache the HostIOMMUContext > pointer in vIOMMU. Just like AddressSpace, it has a MemoryRegion pointer. > Here is the same, VTDHostIOMMUContext is just a wrapper to better manage > it in vVT-d. It's not inheriting. Yep I've got it now ;-) > >>> + IntelIOMMUState *iommu_state; >>> +}; >>> + >>> struct VTDBus { >>> - PCIBus* bus; /* A reference to the bus to provide translation for >> */ >>> + /* A reference to the bus to provide translation for */ >>> + PCIBus *bus; >>> /* A table of VTDAddressSpace objects indexed by devfn */ >>> - VTDAddressSpace *dev_as[]; >>> + VTDAddressSpace *dev_as[PCI_DEVFN_MAX]; >>> + /* A table of VTDHostIOMMUContext objects indexed by devfn */ >>> + VTDHostIOMMUContext *dev_icx[PCI_DEVFN_MAX]; >> At this point of the review, it is unclear to me why the context is >> associated to a device. > > HostIOMMUContext can be per-device or not. It depends on how vIOMMU > manage it. For vVT-d, it's per device as the container is per-device. > >> Up to now you have not explained it should. If >> so why isn't it part of VTDAddressSpace? > > Ah, I did have considered it. But I chose to use a separate one as > context is not really tied with an addresspace. It's better to mange > it with a separate structure. OK Thanks Eric > > Regards, > Yi Liu >
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 4b22910..fd349c6 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -3354,23 +3354,33 @@ static const MemoryRegionOps vtd_mem_ir_ops = { }, }; -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) +/** + * Fetch a VTDBus instance for given PCIBus. If no existing instance, + * allocate one. + */ +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]; 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)); vtd_bus->bus = bus; g_hash_table_insert(s->vtd_as_by_busptr, new_key, vtd_bus); } + return vtd_bus; +} +VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) +{ + VTDBus *vtd_bus; + VTDAddressSpace *vtd_dev_as; + char name[128]; + + vtd_bus = vtd_find_add_bus(s, bus); vtd_dev_as = vtd_bus->dev_as[devfn]; if (!vtd_dev_as) { @@ -3436,6 +3446,55 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) return vtd_dev_as; } +static int vtd_dev_set_iommu_context(PCIBus *bus, void *opaque, + int devfn, + HostIOMMUContext *iommu_ctx) +{ + IntelIOMMUState *s = opaque; + VTDBus *vtd_bus; + VTDHostIOMMUContext *vtd_dev_icx; + + assert(0 <= devfn && devfn < PCI_DEVFN_MAX); + + vtd_bus = vtd_find_add_bus(s, bus); + + vtd_iommu_lock(s); + + vtd_dev_icx = vtd_bus->dev_icx[devfn]; + + assert(!vtd_dev_icx); + + vtd_bus->dev_icx[devfn] = vtd_dev_icx = + g_malloc0(sizeof(VTDHostIOMMUContext)); + vtd_dev_icx->vtd_bus = vtd_bus; + vtd_dev_icx->devfn = (uint8_t)devfn; + vtd_dev_icx->iommu_state = s; + vtd_dev_icx->iommu_ctx = iommu_ctx; + + vtd_iommu_unlock(s); + + return 0; +} + +static void vtd_dev_unset_iommu_context(PCIBus *bus, void *opaque, int devfn) +{ + IntelIOMMUState *s = opaque; + VTDBus *vtd_bus; + VTDHostIOMMUContext *vtd_dev_icx; + + assert(0 <= devfn && devfn < PCI_DEVFN_MAX); + + vtd_bus = vtd_find_add_bus(s, bus); + + vtd_iommu_lock(s); + + vtd_dev_icx = vtd_bus->dev_icx[devfn]; + g_free(vtd_dev_icx); + vtd_bus->dev_icx[devfn] = NULL; + + vtd_iommu_unlock(s); +} + static uint64_t get_naturally_aligned_size(uint64_t start, uint64_t size, int gaw) { @@ -3731,6 +3790,8 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) static PCIIOMMUOps vtd_iommu_ops = { .get_address_space = vtd_host_dma_iommu, + .set_iommu_context = vtd_dev_set_iommu_context, + .unset_iommu_context = vtd_dev_unset_iommu_context, }; static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index 3870052..b5fefb9 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -64,6 +64,7 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry; typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress; typedef struct VTDPASIDDirEntry VTDPASIDDirEntry; typedef struct VTDPASIDEntry VTDPASIDEntry; +typedef struct VTDHostIOMMUContext VTDHostIOMMUContext; /* Context-Entry */ struct VTDContextEntry { @@ -112,10 +113,20 @@ struct VTDAddressSpace { IOVATree *iova_tree; /* Traces mapped IOVA ranges */ }; +struct VTDHostIOMMUContext { + VTDBus *vtd_bus; + uint8_t devfn; + HostIOMMUContext *iommu_ctx; + IntelIOMMUState *iommu_state; +}; + struct VTDBus { - PCIBus* bus; /* A reference to the bus to provide translation for */ + /* A reference to the bus to provide translation for */ + PCIBus *bus; /* A table of VTDAddressSpace objects indexed by devfn */ - VTDAddressSpace *dev_as[]; + VTDAddressSpace *dev_as[PCI_DEVFN_MAX]; + /* A table of VTDHostIOMMUContext objects indexed by devfn */ + VTDHostIOMMUContext *dev_icx[PCI_DEVFN_MAX]; }; struct VTDIOTLBEntry { @@ -269,8 +280,10 @@ struct IntelIOMMUState { bool dma_drain; /* Whether DMA r/w draining enabled */ /* - * Protects IOMMU states in general. Currently it protects the - * per-IOMMU IOTLB cache, and context entry cache in VTDAddressSpace. + * iommu_lock protects below: + * - per-IOMMU IOTLB caches + * - context entry cache in VTDAddressSpace + * - HostIOMMUContext pointer cached in vIOMMU */ QemuMutex iommu_lock; };
This patch adds set/unset_iommu_context() impelementation in Intel vIOMMU. For Intel platform, pass-through modules (e.g. VFIO) could set HostIOMMUContext to Intel vIOMMU emulator. Cc: Kevin Tian <kevin.tian@intel.com> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com> Cc: Peter Xu <peterx@redhat.com> Cc: Yi Sun <yi.y.sun@linux.intel.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Richard Henderson <rth@twiddle.net> Cc: Eduardo Habkost <ehabkost@redhat.com> Signed-off-by: Liu Yi L <yi.l.liu@intel.com> --- hw/i386/intel_iommu.c | 71 ++++++++++++++++++++++++++++++++++++++++--- include/hw/i386/intel_iommu.h | 21 ++++++++++--- 2 files changed, 83 insertions(+), 9 deletions(-)