Re: [PATCH v8 12/17] vfio-user: IOMMU support for remote device

[Stefan Hajnoczi]

On Wed, Apr 20, 2022 at 11:15:16AM +0000, Jag Raman wrote:
> 
> 
> > On Apr 19, 2022, at 4:45 PM, Jag Raman <jag.raman@oracle.com> wrote:
> > 
> > Assign separate address space for each device in the remote processes.
> > 
> > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> > ---
> > include/hw/remote/iommu.h |  40 +++++++++++++
> > hw/remote/iommu.c         | 114 ++++++++++++++++++++++++++++++++++++++
> > hw/remote/machine.c       |  13 ++++-
> > MAINTAINERS               |   2 +
> > hw/remote/meson.build     |   1 +
> > 5 files changed, 169 insertions(+), 1 deletion(-)
> > create mode 100644 include/hw/remote/iommu.h
> > create mode 100644 hw/remote/iommu.c
> > 
> > diff --git a/include/hw/remote/iommu.h b/include/hw/remote/iommu.h
> > new file mode 100644
> > index 0000000000..33b68a8f4b
> > --- /dev/null
> > +++ b/include/hw/remote/iommu.h
> > @@ -0,0 +1,40 @@
> > +/**
> > + * Copyright © 2022 Oracle and/or its affiliates.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#ifndef REMOTE_IOMMU_H
> > +#define REMOTE_IOMMU_H
> > +
> > +#include "hw/pci/pci_bus.h"
> > +#include "hw/pci/pci.h"
> > +
> > +#ifndef INT2VOIDP
> > +#define INT2VOIDP(i) (void *)(uintptr_t)(i)
> > +#endif
> > +
> > +typedef struct RemoteIommuElem {
> > +    MemoryRegion *mr;
> > +
> > +    AddressSpace as;
> > +} RemoteIommuElem;
> > +
> > +#define TYPE_REMOTE_IOMMU "x-remote-iommu"
> > +OBJECT_DECLARE_SIMPLE_TYPE(RemoteIommu, REMOTE_IOMMU)
> > +
> > +struct RemoteIommu {
> > +    Object parent;
> > +
> > +    GHashTable *elem_by_devfn;
> > +
> > +    QemuMutex lock;
> > +};
> > +
> > +void remote_iommu_setup(PCIBus *pci_bus);
> > +
> > +void remote_iommu_unplug_dev(PCIDevice *pci_dev);
> > +
> > +#endif
> > diff --git a/hw/remote/iommu.c b/hw/remote/iommu.c
> > new file mode 100644
> > index 0000000000..16c6b0834e
> > --- /dev/null
> > +++ b/hw/remote/iommu.c
> > @@ -0,0 +1,114 @@
> > +/**
> > + * IOMMU for remote device
> > + *
> > + * Copyright © 2022 Oracle and/or its affiliates.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu-common.h"
> > +
> > +#include "hw/remote/iommu.h"
> > +#include "hw/pci/pci_bus.h"
> > +#include "hw/pci/pci.h"
> > +#include "exec/memory.h"
> > +#include "exec/address-spaces.h"
> > +#include "trace.h"
> > +
> > +static AddressSpace *remote_iommu_find_add_as(PCIBus *pci_bus,
> > +                                              void *opaque, int devfn)
> > +{
> > +    RemoteIommu *iommu = opaque;
> > +    RemoteIommuElem *elem = NULL;
> > +
> > +    qemu_mutex_lock(&iommu->lock);
> > +
> > +    elem = g_hash_table_lookup(iommu->elem_by_devfn, INT2VOIDP(devfn));
> > +
> > +    if (!elem) {
> > +        elem = g_malloc0(sizeof(RemoteIommuElem));
> > +        g_hash_table_insert(iommu->elem_by_devfn, INT2VOIDP(devfn), elem);
> > +    }
> > +
> > +    if (!elem->mr) {
> > +        elem->mr = MEMORY_REGION(object_new(TYPE_MEMORY_REGION));
> > +        memory_region_set_size(elem->mr, UINT64_MAX);
> > +        address_space_init(&elem->as, elem->mr, NULL);
> 
> Hi,
> 
> I’d like to add a note here.
> 
> We tried to add "elem->mr” as a child of PCIDevice. That way, when PCIDevice 
> is
> unplugged, the child is also finalized.

Do you mean via a memory_region_init()-family function where a parent
object is given? Or do you mean by adding a QOM child property?

> However, there was some issue with hotplug. During the hotplug, there’s a 
> window
> during initialization where we couldn’t lookup the PCIDevice by “devfn”.
> 
> do_pci_register_device() -> pci_init_bus_master() -> 
> pci_device_iommu_address_space()
> happens before do_pci_register_device() -> “bus->devices[devfn] = pci_dev”. 
> As such,
> pci_find_device() doesn’t work at this time.

I don't follow. What calls pci_find_device()?

Stefan

signature.asc
Description: PGP signature