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

[Jag Raman]

> 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.

Thank you!
--
Jag

> 
> Stefan