| Message ID | 1519900415-30314-12-git-send-email-yi.l.liu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce new iommu notifier framework for virt-SVA | expand |
On 01/03/2018 11:33, Liu, Yi L wrote: > This patch introduces a framework to manage PASID tagged AddressSpace > in Intel vIOMMU emulator. PASID tagged AddressSpace is an address sapce > which is an abstract of guest process address space in Qemu. The > management framework is as below: > > s->pasid_as_list > /|\ \ > / | \ \ > pasid_as_node ... > /|\ \ > / | \ \ > device ... > > There is a list to store all the PASID tagged AddressSpace, and each > PASID tagged AddressSpace has a device list behind it. This is due to > the fact that a PASID tagged AddressSpace can have multiple devices > binded. > > Signed-off-by: Liu, Yi L <yi.l.liu@linux.intel.com> > --- > hw/i386/intel_iommu.c | 1 + > include/hw/i386/intel_iommu.h | 24 ++++++++++++++++++++++++ > 2 files changed, 25 insertions(+) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index d92a66d..b8e8dbb 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -3174,6 +3174,7 @@ static void vtd_realize(DeviceState *dev, Error **errp) > } > > QLIST_INIT(&s->notifiers_list); > + QLIST_INIT(&s->pasid_as_list); > memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num)); > memory_region_init_io(&s->csrmem, OBJECT(s), &vtd_mem_ops, s, > "intel_iommu", DMAR_REG_SIZE); > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index 0b6dc32..c45dbfe 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -61,6 +61,7 @@ typedef struct VTDContextEntry VTDContextEntry; > typedef struct VTDContextCacheEntry VTDContextCacheEntry; > typedef struct IntelIOMMUState IntelIOMMUState; > typedef struct VTDAddressSpace VTDAddressSpace; > +typedef struct VTDPASIDAddressSpace VTDPASIDAddressSpace; > typedef struct VTDIOTLBEntry VTDIOTLBEntry; > typedef struct VTDBus VTDBus; > typedef union VTD_IR_TableEntry VTD_IR_TableEntry; > @@ -69,6 +70,8 @@ typedef struct VTDIrq VTDIrq; > typedef struct VTD_MSIMessage VTD_MSIMessage; > typedef struct IntelIOMMUMRNotifierNode IntelIOMMUMRNotifierNode; > typedef struct IntelIOMMUAssignedDeviceNode IntelIOMMUAssignedDeviceNode; > +typedef struct IntelPASIDNode IntelPASIDNode; > +typedef struct VTDDeviceNode VTDDeviceNode; > > /* Context-Entry */ > struct VTDContextEntry { > @@ -84,6 +87,20 @@ struct VTDContextCacheEntry { > struct VTDContextEntry context_entry; > }; > > +struct VTDDeviceNode { > + PCIBus *bus; > + uint8_t devfn; > + QLIST_ENTRY(VTDDeviceNode) next; > +}; > + > +struct VTDPASIDAddressSpace { > + AddressSpace as; > + IOMMUSVAContext sva_ctx; > + IntelIOMMUState *iommu_state; > + /* list of devices binded to a pasid tagged address space */ > + QLIST_HEAD(, VTDDeviceNode) device_list; > +}; > + > struct VTDAddressSpace { > PCIBus *bus; > uint8_t devfn; > @@ -264,6 +281,11 @@ struct IntelIOMMUAssignedDeviceNode { > QLIST_ENTRY(IntelIOMMUAssignedDeviceNode) next; > }; > > +struct IntelPASIDNode { > + VTDPASIDAddressSpace *pasid_as; > + QLIST_ENTRY(IntelPASIDNode) next; > +}; > + > /* The iommu (DMAR) device state struct */ > struct IntelIOMMUState { > X86IOMMUState x86_iommu; > @@ -304,6 +326,8 @@ struct IntelIOMMUState { > QLIST_HEAD(, IntelIOMMUMRNotifierNode) notifiers_list; > /* list of assigned devices */ > QLIST_HEAD(, IntelIOMMUAssignedDeviceNode) assigned_device_list; > + /* list of pasid tagged address space */ > + QLIST_HEAD(, IntelPASIDNode) pasid_as_list; > > /* interrupt remapping */ > bool intr_enabled; /* Whether guest enabled IR */ > Please merge this patch with the next one, since they are basically the .h and .c sides of the same thing. Paolo
On 01/03/2018 11:33, Liu, Yi L wrote: > +struct VTDDeviceNode { > + PCIBus *bus; > + uint8_t devfn; > + QLIST_ENTRY(VTDDeviceNode) next; > +}; Do you really need VTDDeviceNode? I think can you simply put the QLIST_ENTRY in VTDAddressSpace (named e.g. next_by_pasid), since VTDAddressSpace already includes a (bus, devfn). Thanks, Paolo > +struct VTDPASIDAddressSpace { > + AddressSpace as; > + IOMMUSVAContext sva_ctx; > + IntelIOMMUState *iommu_state; > + /* list of devices binded to a pasid tagged address space */ > + QLIST_HEAD(, VTDDeviceNode) device_list; > +}; > +
On Fri, Mar 02, 2018 at 04:00:23PM +0100, Paolo Bonzini wrote: > On 01/03/2018 11:33, Liu, Yi L wrote: > > +struct VTDDeviceNode { > > + PCIBus *bus; > > + uint8_t devfn; > > + QLIST_ENTRY(VTDDeviceNode) next; > > +}; > > Do you really need VTDDeviceNode? I think can you simply put the > QLIST_ENTRY in VTDAddressSpace (named e.g. next_by_pasid), since > VTDAddressSpace already includes a (bus, devfn). Existing VTDAddressSpace is actaully per-device. While for PASID tagged address space, it is possible to have multiple devices tied to a single PASID tagged address space. Reuse VTDAddressSpace could be a choice since it is a per-device structure, but it may be missleading since there is other fileds in VTDAddressSpace. This is why I proposed to have VTDDeviceNode. But consolidation is possible here. Thanks, Yi Liu > > +struct VTDPASIDAddressSpace { > > + AddressSpace as; > > + IOMMUSVAContext sva_ctx; > > + IntelIOMMUState *iommu_state; > > + /* list of devices binded to a pasid tagged address space */ > > + QLIST_HEAD(, VTDDeviceNode) device_list; > > +}; > > + > >
On Fri, Mar 02, 2018 at 03:52:44PM +0100, Paolo Bonzini wrote: > On 01/03/2018 11:33, Liu, Yi L wrote: > > Please merge this patch with the next one, since they are basically the > .h and .c sides of the same thing. yes, would do it in next version. Thanks, Yi Liu
On 05/03/2018 10:11, Liu, Yi L wrote: >> Do you really need VTDDeviceNode? I think can you simply put the >> QLIST_ENTRY in VTDAddressSpace (named e.g. next_by_pasid), since >> VTDAddressSpace already includes a (bus, devfn). > Existing VTDAddressSpace is actaully per-device. While for PASID tagged > address space, it is possible to have multiple devices tied to a single > PASID tagged address space. Yes, that's the purpose of VTDPASIDAddressSpace. > Reuse VTDAddressSpace could be a choice since > it is a per-device structure, but it may be missleading since there is > other fileds in VTDAddressSpace. This is why I proposed to have VTDDeviceNode. I think it's okay to put all per-device setup in VTDAddressSpace. Later if it makes sense VTDAddressSpace could become a union, according to whether the IOMMU is configured for PASID or requester ID operation, and could be renamed to VTDDeviceInfo. But for now it's not needed. Paolo > But consolidation is possible here.
> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Sent: Tuesday, March 6, 2018 6:27 PM > Subject: Re: [Qemu-devel] [PATCH v3 11/12] intel_iommu: add framework for PASID > AddressSpace management > > On 05/03/2018 10:11, Liu, Yi L wrote: > >> Do you really need VTDDeviceNode? I think can you simply put the > >> QLIST_ENTRY in VTDAddressSpace (named e.g. next_by_pasid), since > >> VTDAddressSpace already includes a (bus, devfn). > > Existing VTDAddressSpace is actaully per-device. While for PASID > > tagged address space, it is possible to have multiple devices tied to > > a single PASID tagged address space. > > Yes, that's the purpose of VTDPASIDAddressSpace. > > > Reuse VTDAddressSpace could be a choice since it is a per-device > > structure, but it may be missleading since there is other fileds in > > VTDAddressSpace. This is why I proposed to have VTDDeviceNode. > > I think it's okay to put all per-device setup in VTDAddressSpace. Later if it makes > sense VTDAddressSpace could become a union, according to whether the IOMMU is > configured for PASID or requester ID operation, and could be renamed to > VTDDeviceInfo. But for now it's not needed. Agreed. Let me apply the idea in next version. Thanks, Yi Liu
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index d92a66d..b8e8dbb 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -3174,6 +3174,7 @@ static void vtd_realize(DeviceState *dev, Error **errp) } QLIST_INIT(&s->notifiers_list); + QLIST_INIT(&s->pasid_as_list); memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num)); memory_region_init_io(&s->csrmem, OBJECT(s), &vtd_mem_ops, s, "intel_iommu", DMAR_REG_SIZE); diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index 0b6dc32..c45dbfe 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -61,6 +61,7 @@ typedef struct VTDContextEntry VTDContextEntry; typedef struct VTDContextCacheEntry VTDContextCacheEntry; typedef struct IntelIOMMUState IntelIOMMUState; typedef struct VTDAddressSpace VTDAddressSpace; +typedef struct VTDPASIDAddressSpace VTDPASIDAddressSpace; typedef struct VTDIOTLBEntry VTDIOTLBEntry; typedef struct VTDBus VTDBus; typedef union VTD_IR_TableEntry VTD_IR_TableEntry; @@ -69,6 +70,8 @@ typedef struct VTDIrq VTDIrq; typedef struct VTD_MSIMessage VTD_MSIMessage; typedef struct IntelIOMMUMRNotifierNode IntelIOMMUMRNotifierNode; typedef struct IntelIOMMUAssignedDeviceNode IntelIOMMUAssignedDeviceNode; +typedef struct IntelPASIDNode IntelPASIDNode; +typedef struct VTDDeviceNode VTDDeviceNode; /* Context-Entry */ struct VTDContextEntry { @@ -84,6 +87,20 @@ struct VTDContextCacheEntry { struct VTDContextEntry context_entry; }; +struct VTDDeviceNode { + PCIBus *bus; + uint8_t devfn; + QLIST_ENTRY(VTDDeviceNode) next; +}; + +struct VTDPASIDAddressSpace { + AddressSpace as; + IOMMUSVAContext sva_ctx; + IntelIOMMUState *iommu_state; + /* list of devices binded to a pasid tagged address space */ + QLIST_HEAD(, VTDDeviceNode) device_list; +}; + struct VTDAddressSpace { PCIBus *bus; uint8_t devfn; @@ -264,6 +281,11 @@ struct IntelIOMMUAssignedDeviceNode { QLIST_ENTRY(IntelIOMMUAssignedDeviceNode) next; }; +struct IntelPASIDNode { + VTDPASIDAddressSpace *pasid_as; + QLIST_ENTRY(IntelPASIDNode) next; +}; + /* The iommu (DMAR) device state struct */ struct IntelIOMMUState { X86IOMMUState x86_iommu; @@ -304,6 +326,8 @@ struct IntelIOMMUState { QLIST_HEAD(, IntelIOMMUMRNotifierNode) notifiers_list; /* list of assigned devices */ QLIST_HEAD(, IntelIOMMUAssignedDeviceNode) assigned_device_list; + /* list of pasid tagged address space */ + QLIST_HEAD(, IntelPASIDNode) pasid_as_list; /* interrupt remapping */ bool intr_enabled; /* Whether guest enabled IR */
This patch introduces a framework to manage PASID tagged AddressSpace in Intel vIOMMU emulator. PASID tagged AddressSpace is an address sapce which is an abstract of guest process address space in Qemu. The management framework is as below: s->pasid_as_list /|\ \ / | \ \ pasid_as_node ... /|\ \ / | \ \ device ... There is a list to store all the PASID tagged AddressSpace, and each PASID tagged AddressSpace has a device list behind it. This is due to the fact that a PASID tagged AddressSpace can have multiple devices binded. Signed-off-by: Liu, Yi L <yi.l.liu@linux.intel.com> --- hw/i386/intel_iommu.c | 1 + include/hw/i386/intel_iommu.h | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+)