diff mbox series

[v2,07/22] intel_iommu: add set/unset_iommu_context callback

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

Commit Message

Liu, Yi L March 30, 2020, 4:24 a.m. UTC
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(-)

Comments

Auger Eric March 30, 2020, 8:23 p.m. UTC | #1
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;
>  };
>
Liu, Yi L March 31, 2020, 12:25 p.m. UTC | #2
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
Auger Eric March 31, 2020, 12:57 p.m. UTC | #3
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 mbox series

Patch

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;
 };