| Message ID | 1466495274-5011-4-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jun 21, 2016 at 03:47:31PM +0800, Peter Xu wrote: > Remove VT-d calls in common q35 codes. Instead, we provide a general > find_add_as() for x86-iommu type. > > Signed-off-by: Peter Xu <peterx@redhat.com> I think it would be cleaner in the device, not in the type. In theory you could then mix multiple different iommu types in the same machine. > --- > hw/i386/intel_iommu.c | 17 +++++++++-------- > include/hw/i386/intel_iommu.h | 5 ----- > include/hw/i386/x86-iommu.h | 3 +++ > 3 files changed, 12 insertions(+), 13 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 1936c41..b487224 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -1900,8 +1900,10 @@ static Property vtd_properties[] = { > }; > > > -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) > +static AddressSpace *vtd_find_add_as(X86IOMMUState *x86_iommu, PCIBus *bus, > + int devfn) > { > + IntelIOMMUState *s = (IntelIOMMUState *)x86_iommu; > uintptr_t key = (uintptr_t)bus; > VTDBus *vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr, &key); > VTDAddressSpace *vtd_dev_as; > @@ -1929,7 +1931,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) > address_space_init(&vtd_dev_as->as, > &vtd_dev_as->iommu, "intel_iommu"); > } > - return vtd_dev_as; > + return &vtd_dev_as->as; > } > > /* Do the initialization. It will also be called when reset, so pay > @@ -2021,13 +2023,11 @@ static void vtd_reset(DeviceState *dev) > > static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) > { > - IntelIOMMUState *s = opaque; > - VTDAddressSpace *vtd_as; > - > - assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); > + X86IOMMUState *x86_iommu = opaque; > + X86IOMMUClass *x86_class = X86_IOMMU_GET_CLASS(x86_iommu); > > - vtd_as = vtd_find_add_as(s, bus, devfn); > - return &vtd_as->as; > + assert(0 <= devfn && devfn <= X86_IOMMU_PCI_DEVFN_MAX); > + return x86_class->find_add_as(x86_iommu, bus, devfn); > } > > static void vtd_realize(DeviceState *dev, Error **errp) > @@ -2060,6 +2060,7 @@ static void vtd_class_init(ObjectClass *klass, void *data) > dc->vmsd = &vtd_vmstate; > dc->props = vtd_properties; > x86_class->realize = vtd_realize; > + x86_class->find_add_as = vtd_find_add_as; > } > > static const TypeInfo vtd_info = { > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index 0794309..e36b896 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -125,9 +125,4 @@ struct IntelIOMMUState { > VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by bus number */ > }; > > -/* Find the VTD Address space associated with the given bus pointer, > - * create a new one if none exists > - */ > -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn); > - > #endif > diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h > index d6991cb..2070cd1 100644 > --- a/include/hw/i386/x86-iommu.h > +++ b/include/hw/i386/x86-iommu.h > @@ -21,6 +21,7 @@ > #define IOMMU_COMMON_H > > #include "hw/sysbus.h" > +#include "exec/memory.h" > > #define TYPE_X86_IOMMU_DEVICE ("x86-iommu") > #define X86_IOMMU_DEVICE(obj) \ > @@ -40,6 +41,8 @@ struct X86IOMMUClass { > SysBusDeviceClass parent; > /* Intel/AMD specific realize() hook */ > DeviceRealize realize; > + /* Find/Add IOMMU address space for specific PCI device */ > + AddressSpace *(*find_add_as)(X86IOMMUState *s, PCIBus *bus, int devfn); > }; > > struct X86IOMMUState { > -- > 2.4.11
On 04/07/2016 17:16, Michael S. Tsirkin wrote: > On Tue, Jun 21, 2016 at 03:47:31PM +0800, Peter Xu wrote: >> Remove VT-d calls in common q35 codes. Instead, we provide a general >> find_add_as() for x86-iommu type. >> >> Signed-off-by: Peter Xu <peterx@redhat.com> > > I think it would be cleaner in the device, > not in the type. In theory you could then mix > multiple different iommu types in the same machine. Not sure what you mean. Each IOMMU subclass can define its own find_add_as implementation, what's wrong with that? Paolo > >> --- >> hw/i386/intel_iommu.c | 17 +++++++++-------- >> include/hw/i386/intel_iommu.h | 5 ----- >> include/hw/i386/x86-iommu.h | 3 +++ >> 3 files changed, 12 insertions(+), 13 deletions(-) >> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> index 1936c41..b487224 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -1900,8 +1900,10 @@ static Property vtd_properties[] = { >> }; >> >> >> -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) >> +static AddressSpace *vtd_find_add_as(X86IOMMUState *x86_iommu, PCIBus *bus, >> + int devfn) >> { >> + IntelIOMMUState *s = (IntelIOMMUState *)x86_iommu; >> uintptr_t key = (uintptr_t)bus; >> VTDBus *vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr, &key); >> VTDAddressSpace *vtd_dev_as; >> @@ -1929,7 +1931,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) >> address_space_init(&vtd_dev_as->as, >> &vtd_dev_as->iommu, "intel_iommu"); >> } >> - return vtd_dev_as; >> + return &vtd_dev_as->as; >> } >> >> /* Do the initialization. It will also be called when reset, so pay >> @@ -2021,13 +2023,11 @@ static void vtd_reset(DeviceState *dev) >> >> static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) >> { >> - IntelIOMMUState *s = opaque; >> - VTDAddressSpace *vtd_as; >> - >> - assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); >> + X86IOMMUState *x86_iommu = opaque; >> + X86IOMMUClass *x86_class = X86_IOMMU_GET_CLASS(x86_iommu); >> >> - vtd_as = vtd_find_add_as(s, bus, devfn); >> - return &vtd_as->as; >> + assert(0 <= devfn && devfn <= X86_IOMMU_PCI_DEVFN_MAX); >> + return x86_class->find_add_as(x86_iommu, bus, devfn); >> } >> >> static void vtd_realize(DeviceState *dev, Error **errp) >> @@ -2060,6 +2060,7 @@ static void vtd_class_init(ObjectClass *klass, void *data) >> dc->vmsd = &vtd_vmstate; >> dc->props = vtd_properties; >> x86_class->realize = vtd_realize; >> + x86_class->find_add_as = vtd_find_add_as; >> } >> >> static const TypeInfo vtd_info = { >> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h >> index 0794309..e36b896 100644 >> --- a/include/hw/i386/intel_iommu.h >> +++ b/include/hw/i386/intel_iommu.h >> @@ -125,9 +125,4 @@ struct IntelIOMMUState { >> VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by bus number */ >> }; >> >> -/* Find the VTD Address space associated with the given bus pointer, >> - * create a new one if none exists >> - */ >> -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn); >> - >> #endif >> diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h >> index d6991cb..2070cd1 100644 >> --- a/include/hw/i386/x86-iommu.h >> +++ b/include/hw/i386/x86-iommu.h >> @@ -21,6 +21,7 @@ >> #define IOMMU_COMMON_H >> >> #include "hw/sysbus.h" >> +#include "exec/memory.h" >> >> #define TYPE_X86_IOMMU_DEVICE ("x86-iommu") >> #define X86_IOMMU_DEVICE(obj) \ >> @@ -40,6 +41,8 @@ struct X86IOMMUClass { >> SysBusDeviceClass parent; >> /* Intel/AMD specific realize() hook */ >> DeviceRealize realize; >> + /* Find/Add IOMMU address space for specific PCI device */ >> + AddressSpace *(*find_add_as)(X86IOMMUState *s, PCIBus *bus, int devfn); >> }; >> >> struct X86IOMMUState { >> -- >> 2.4.11 > >
On Mon, Jul 04, 2016 at 06:08:28PM +0200, Paolo Bonzini wrote: > > > On 04/07/2016 17:16, Michael S. Tsirkin wrote: > > On Tue, Jun 21, 2016 at 03:47:31PM +0800, Peter Xu wrote: > >> Remove VT-d calls in common q35 codes. Instead, we provide a general > >> find_add_as() for x86-iommu type. > >> > >> Signed-off-by: Peter Xu <peterx@redhat.com> > > > > I think it would be cleaner in the device, > > not in the type. In theory you could then mix > > multiple different iommu types in the same machine. > > Not sure what you mean. Each IOMMU subclass can define its own > find_add_as implementation, what's wrong with that? > > Paolo this: @@ -2060,6 +2060,7 @@ static void vtd_class_init(ObjectClass *klass, void *data) dc->vmsd = &vtd_vmstate; dc->props = vtd_properties; x86_class->realize = vtd_realize; + x86_class->find_add_as = vtd_find_add_as; } I think this means that if there are two classes inheriting x86_class, they will conflict over-writing vtd_find_add_as in the parent. What did I miss? > > > >> --- > >> hw/i386/intel_iommu.c | 17 +++++++++-------- > >> include/hw/i386/intel_iommu.h | 5 ----- > >> include/hw/i386/x86-iommu.h | 3 +++ > >> 3 files changed, 12 insertions(+), 13 deletions(-) > >> > >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > >> index 1936c41..b487224 100644 > >> --- a/hw/i386/intel_iommu.c > >> +++ b/hw/i386/intel_iommu.c > >> @@ -1900,8 +1900,10 @@ static Property vtd_properties[] = { > >> }; > >> > >> > >> -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) > >> +static AddressSpace *vtd_find_add_as(X86IOMMUState *x86_iommu, PCIBus *bus, > >> + int devfn) > >> { > >> + IntelIOMMUState *s = (IntelIOMMUState *)x86_iommu; > >> uintptr_t key = (uintptr_t)bus; > >> VTDBus *vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr, &key); > >> VTDAddressSpace *vtd_dev_as; > >> @@ -1929,7 +1931,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) > >> address_space_init(&vtd_dev_as->as, > >> &vtd_dev_as->iommu, "intel_iommu"); > >> } > >> - return vtd_dev_as; > >> + return &vtd_dev_as->as; > >> } > >> > >> /* Do the initialization. It will also be called when reset, so pay > >> @@ -2021,13 +2023,11 @@ static void vtd_reset(DeviceState *dev) > >> > >> static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) > >> { > >> - IntelIOMMUState *s = opaque; > >> - VTDAddressSpace *vtd_as; > >> - > >> - assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); > >> + X86IOMMUState *x86_iommu = opaque; > >> + X86IOMMUClass *x86_class = X86_IOMMU_GET_CLASS(x86_iommu); > >> > >> - vtd_as = vtd_find_add_as(s, bus, devfn); > >> - return &vtd_as->as; > >> + assert(0 <= devfn && devfn <= X86_IOMMU_PCI_DEVFN_MAX); > >> + return x86_class->find_add_as(x86_iommu, bus, devfn); > >> } > >> > >> static void vtd_realize(DeviceState *dev, Error **errp) > >> @@ -2060,6 +2060,7 @@ static void vtd_class_init(ObjectClass *klass, void *data) > >> dc->vmsd = &vtd_vmstate; > >> dc->props = vtd_properties; > >> x86_class->realize = vtd_realize; > >> + x86_class->find_add_as = vtd_find_add_as; > >> } > >> > >> static const TypeInfo vtd_info = { > >> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > >> index 0794309..e36b896 100644 > >> --- a/include/hw/i386/intel_iommu.h > >> +++ b/include/hw/i386/intel_iommu.h > >> @@ -125,9 +125,4 @@ struct IntelIOMMUState { > >> VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by bus number */ > >> }; > >> > >> -/* Find the VTD Address space associated with the given bus pointer, > >> - * create a new one if none exists > >> - */ > >> -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn); > >> - > >> #endif > >> diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h > >> index d6991cb..2070cd1 100644 > >> --- a/include/hw/i386/x86-iommu.h > >> +++ b/include/hw/i386/x86-iommu.h > >> @@ -21,6 +21,7 @@ > >> #define IOMMU_COMMON_H > >> > >> #include "hw/sysbus.h" > >> +#include "exec/memory.h" > >> > >> #define TYPE_X86_IOMMU_DEVICE ("x86-iommu") > >> #define X86_IOMMU_DEVICE(obj) \ > >> @@ -40,6 +41,8 @@ struct X86IOMMUClass { > >> SysBusDeviceClass parent; > >> /* Intel/AMD specific realize() hook */ > >> DeviceRealize realize; > >> + /* Find/Add IOMMU address space for specific PCI device */ > >> + AddressSpace *(*find_add_as)(X86IOMMUState *s, PCIBus *bus, int devfn); > >> }; > >> > >> struct X86IOMMUState { > >> -- > >> 2.4.11 > > > >
On 04/07/2016 18:35, Michael S. Tsirkin wrote: >> > >> > Not sure what you mean. Each IOMMU subclass can define its own >> > find_add_as implementation, what's wrong with that? >> > >> > Paolo > > this: > > @@ -2060,6 +2060,7 @@ static void vtd_class_init(ObjectClass *klass, void *data) > dc->vmsd = &vtd_vmstate; > dc->props = vtd_properties; > x86_class->realize = vtd_realize; > + x86_class->find_add_as = vtd_find_add_as; > } > > I think this > means that if there are two classes inheriting x86_class, > they will conflict over-writing vtd_find_add_as in the > parent. No, x86_class is really just (X86IOMMUClass *)klass, and klass is local. Paolo
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 1936c41..b487224 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -1900,8 +1900,10 @@ static Property vtd_properties[] = { }; -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) +static AddressSpace *vtd_find_add_as(X86IOMMUState *x86_iommu, PCIBus *bus, + int devfn) { + IntelIOMMUState *s = (IntelIOMMUState *)x86_iommu; uintptr_t key = (uintptr_t)bus; VTDBus *vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr, &key); VTDAddressSpace *vtd_dev_as; @@ -1929,7 +1931,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) address_space_init(&vtd_dev_as->as, &vtd_dev_as->iommu, "intel_iommu"); } - return vtd_dev_as; + return &vtd_dev_as->as; } /* Do the initialization. It will also be called when reset, so pay @@ -2021,13 +2023,11 @@ static void vtd_reset(DeviceState *dev) static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) { - IntelIOMMUState *s = opaque; - VTDAddressSpace *vtd_as; - - assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); + X86IOMMUState *x86_iommu = opaque; + X86IOMMUClass *x86_class = X86_IOMMU_GET_CLASS(x86_iommu); - vtd_as = vtd_find_add_as(s, bus, devfn); - return &vtd_as->as; + assert(0 <= devfn && devfn <= X86_IOMMU_PCI_DEVFN_MAX); + return x86_class->find_add_as(x86_iommu, bus, devfn); } static void vtd_realize(DeviceState *dev, Error **errp) @@ -2060,6 +2060,7 @@ static void vtd_class_init(ObjectClass *klass, void *data) dc->vmsd = &vtd_vmstate; dc->props = vtd_properties; x86_class->realize = vtd_realize; + x86_class->find_add_as = vtd_find_add_as; } static const TypeInfo vtd_info = { diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index 0794309..e36b896 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -125,9 +125,4 @@ struct IntelIOMMUState { VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by bus number */ }; -/* Find the VTD Address space associated with the given bus pointer, - * create a new one if none exists - */ -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn); - #endif diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h index d6991cb..2070cd1 100644 --- a/include/hw/i386/x86-iommu.h +++ b/include/hw/i386/x86-iommu.h @@ -21,6 +21,7 @@ #define IOMMU_COMMON_H #include "hw/sysbus.h" +#include "exec/memory.h" #define TYPE_X86_IOMMU_DEVICE ("x86-iommu") #define X86_IOMMU_DEVICE(obj) \ @@ -40,6 +41,8 @@ struct X86IOMMUClass { SysBusDeviceClass parent; /* Intel/AMD specific realize() hook */ DeviceRealize realize; + /* Find/Add IOMMU address space for specific PCI device */ + AddressSpace *(*find_add_as)(X86IOMMUState *s, PCIBus *bus, int devfn); }; struct X86IOMMUState {
Remove VT-d calls in common q35 codes. Instead, we provide a general find_add_as() for x86-iommu type. Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/i386/intel_iommu.c | 17 +++++++++-------- include/hw/i386/intel_iommu.h | 5 ----- include/hw/i386/x86-iommu.h | 3 +++ 3 files changed, 12 insertions(+), 13 deletions(-)