RE: [PATCH v1 07/22] intel_iommu: add set/unset_iommu_context callback
> From: Peter Xu
> Sent: Tuesday, March 24, 2020 11:24 PM
> To: Liu, Yi L
> Subject: Re: [PATCH v1 07/22] intel_iommu: add set/unset_iommu_context
> callback
>
> On Tue, Mar 24, 2020 at 11:15:24AM +, Liu, Yi L wrote:
>
> [...]
>
> > > > struct VTDIOTLBEntry {
> > > > @@ -271,6 +282,8 @@ struct IntelIOMMUState {
> > > > /*
> > > > * Protects IOMMU states in general. Currently it protects the
> > > > * per-IOMMU IOTLB cache, and context entry cache in
> > > > VTDAddressSpace.
> > > > + * Protect the update/usage of HostIOMMUContext pointer cached in
> > > > + * VTDBus->dev_icx array as array elements may be updated by
> > > > + hotplug
> > >
> > > I think the context update does not need to be updated, because they
> > > should always be with the BQL, right?
> >
> > H, maybe I used bad description. My purpose is to protect the
> > stored HostIOMMUContext pointer in vIOMMU. With
> > pci_device_set/unset_iommu_context,
> > vIOMMU have a copy of HostIOMMUContext. If VFIO container is released
> > (e.g. hotpulg out device), HostIOMMUContext will alos be released.
> > This will trigger the pci_device_unset_iommu_context() to clean the
> > copy. To avoid using a staled HostIOMMUContext in vIOMMU, vIOMMU
> > should have a lock to block the pci_device_unset_iommu_context()
> > calling until other threads finished their HostIOMMUContext usage. Do
> > you want a description update here or other preference?
>
> Yeah, but hot plug/unplug will still take the BQL?
>
> Ah btw I think it's also OK to take the lock if you want or not sure about
> whether
> we'll always take the BQL in these paths.
I guess better to have an internal sync to avoid reference stales
HostIOMMUContext. :-)
> But if so, instead of adding another
> "Protect the ..." sentence to the comment, would you mind list out what the
> lock is
> protecting?
>
> /*
>* iommu_lock protects:
>* - per-IOMMU IOTLB caches
>* - context entry caches
>* - ...
>*/
>
> Or anything better than that. Thanks,
It looks good to me. Let me update it in next version.
Regards,
Yi Liu
Re: [PATCH v1 07/22] intel_iommu: add set/unset_iommu_context callback
On Tue, Mar 24, 2020 at 11:15:24AM +, Liu, Yi L wrote:
[...]
> > > struct VTDIOTLBEntry {
> > > @@ -271,6 +282,8 @@ struct IntelIOMMUState {
> > > /*
> > > * Protects IOMMU states in general. Currently it protects the
> > > * per-IOMMU IOTLB cache, and context entry cache in VTDAddressSpace.
> > > + * Protect the update/usage of HostIOMMUContext pointer cached in
> > > + * VTDBus->dev_icx array as array elements may be updated by hotplug
> >
> > I think the context update does not need to be updated, because they
> > should always be with the BQL, right?
>
> H, maybe I used bad description. My purpose is to protect the stored
> HostIOMMUContext pointer in vIOMMU. With pci_device_set/unset_iommu_context,
> vIOMMU have a copy of HostIOMMUContext. If VFIO container is released
> (e.g. hotpulg out device), HostIOMMUContext will alos be released. This
> will trigger the pci_device_unset_iommu_context() to clean the copy. To
> avoid using a staled HostIOMMUContext in vIOMMU, vIOMMU should have a
> lock to block the pci_device_unset_iommu_context() calling until other
> threads finished their HostIOMMUContext usage. Do you want a description
> update here or other preference?
Yeah, but hot plug/unplug will still take the BQL?
Ah btw I think it's also OK to take the lock if you want or not sure
about whether we'll always take the BQL in these paths. But if so,
instead of adding another "Protect the ..." sentence to the comment,
would you mind list out what the lock is protecting?
/*
* iommu_lock protects:
* - per-IOMMU IOTLB caches
* - context entry caches
* - ...
*/
Or anything better than that. Thanks,
--
Peter Xu
RE: [PATCH v1 07/22] intel_iommu: add set/unset_iommu_context callback
> From: Peter Xu
> Sent: Tuesday, March 24, 2020 5:29 AM
> To: Liu, Yi L
> Subject: Re: [PATCH v1 07/22] intel_iommu: add set/unset_iommu_context
> callback
>
> On Sun, Mar 22, 2020 at 05:36:04AM -0700, Liu Yi L wrote:
> > 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
> > Cc: Jacob Pan
> > Cc: Peter Xu
> > Cc: Yi Sun
> > Cc: Paolo Bonzini
> > Cc: Richard Henderson
> > Cc: Eduardo Habkost
> > Signed-off-by: Liu Yi L
> > ---
> > hw/i386/intel_iommu.c | 70
> +++
> > include/hw/i386/intel_iommu.h | 17 +--
> > 2 files changed, 80 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 4b22910..8d9204f 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -3354,23 +3354,35 @@ 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) + PCI_DEVFN_MAX * \
> > +(sizeof(VTDAddressSpace *) + \
> > + sizeof(VTDHostIOMMUContext *)));
>
> IIRC I commented on this before... Shouldn't sizeof(VTDBus) be
> enough?
Right. My bad. Will do it in next version.
> > 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 +3448,52 @@ 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 *host_icx)
> > +{
> > +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];
> > +
> > +if (!vtd_dev_icx) {
>
> We can assert this directly I think, in case we accidentally set the
> context twice without notice.
good idea. will add it.
>
> > +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->host_icx = host_icx;
> > +}
> > +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];
> >
Re: [PATCH v1 07/22] intel_iommu: add set/unset_iommu_context callback
On Sun, Mar 22, 2020 at 05:36:04AM -0700, Liu Yi L wrote:
> 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
> Cc: Jacob Pan
> Cc: Peter Xu
> Cc: Yi Sun
> Cc: Paolo Bonzini
> Cc: Richard Henderson
> Cc: Eduardo Habkost
> Signed-off-by: Liu Yi L
> ---
> hw/i386/intel_iommu.c | 70
> +++
> include/hw/i386/intel_iommu.h | 17 +--
> 2 files changed, 80 insertions(+), 7 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 4b22910..8d9204f 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3354,23 +3354,35 @@ 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) + PCI_DEVFN_MAX * \
> +(sizeof(VTDAddressSpace *) + \
> + sizeof(VTDHostIOMMUContext *)));
IIRC I commented on this before... Shouldn't sizeof(VTDBus) be
enough?
> 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 +3448,52 @@ 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 *host_icx)
> +{
> +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];
> +
> +if (!vtd_dev_icx) {
We can assert this directly I think, in case we accidentally set the
context twice without notice.
> +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->host_icx = host_icx;
> +}
> +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);
Better set it as NULL, and can also drop vtd_dev_icx which seems
meaningless..
g_free(vtd_bus->dev_icx[devfn]);
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 +3789,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..9b4fc0a 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 */
>
