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

[Stefan Hajnoczi]

On Mon, Apr 25, 2022 at 05:30:19PM +0000, Jag Raman wrote:
> 
> 
> > On Apr 25, 2022, at 5:38 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > 
> > 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?
> 
> I mean by adding “elem->mr” as a QOM child property of PCIDevice.
> 
> > 
> >> 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()?
> 
> To add the MemoryRegion as a child of PCIDevice, remote_iommu_find_add_as()
> would need to lookup the PCIDevice using devfn. The function that looks up
> PCIDevice by devfn is pci_find_device().
> 
> The above note explains why we didn’t lookup the PCIDevice using 
> pci_find_device()
> and then adding the MemoryRegion as its child.

If I understand correctly you're saying it's not possible to use
memory_region_init(&elem->mr, OBJECT(pci_dev), ...) inside
remote_iommu_find_add_as() because there is no way to find the
PCIDevice?

It would be nice to automatically clean up the memory region but the
AddressSpace needs to be destroyed too and there isn't an automatic way
of doing that, so the approach in this patch is fine.

Stefan

signature.asc
Description: PGP signature