Re: [Qemu-devel] [PATCH v10 03/26] x86-iommu: q35: generalize find_add_as()

[Michael S. Tsirkin]

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 <address@hidden>
> > 
> > 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
> > 
> >