Re: [Qemu-devel] [RESEND PATCH 2/6] memory: introduce AddressSpaceOps and IOMMUObject

[Auger Eric <eric.auger@redhat.com>]

Hi Yi L,

On 03/11/2017 13:01, Liu, Yi L wrote:
> From: Peter Xu <peterx@redhat.com>
> 
> AddressSpaceOps is similar to MemoryRegionOps, it's just for address
> spaces to store arch-specific hooks.
> 
> The first hook I would like to introduce is iommu_get(). Return an
> IOMMUObject behind the AddressSpace.

David had an objection in the past about this method, saying that
several IOMMUs could translate a single AS?

https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01610.html

On ARM I think it works in general:
In
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/pci/pci-iommu.txt,
it is said
"a given PCI device can only master through one IOMMU"
> 
> For systems that have IOMMUs, we will create a special address
> space per device which is different from system default address
> space for it (please refer to pci_device_iommu_address_space()).
> Normally when that happens, there will be one specific IOMMU (or
> say, translation unit) stands right behind that new address space.
standing
> 
> This iommu_get() fetches that guy behind the address space. Here,
> the guy is defined as IOMMUObject, which includes a notifier_list
> so far, may extend in future. Along with IOMMUObject, a new iommu
> notifier mechanism is introduced. It would be used for virt-svm.
> Also IOMMUObject can further have a IOMMUObjectOps which is similar
> to MemoryRegionOps. The difference is IOMMUObjectOps is not relied
relying
> on MemoryRegion.
> 
I think I would split this patch into a 1 first patch introducing the
iommu object and a second adding the AS ops and address_space_iommu_get()
> Signed-off-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Liu, Yi L <yi.l.liu@linux.intel.com>
> ---
>  hw/core/Makefile.objs   |  1 +
>  hw/core/iommu.c         | 58 +++++++++++++++++++++++++++++++++++++++
>  include/exec/memory.h   | 22 +++++++++++++++
>  include/hw/core/iommu.h | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
>  memory.c                | 10 +++++--
>  5 files changed, 162 insertions(+), 2 deletions(-)
>  create mode 100644 hw/core/iommu.c
>  create mode 100644 include/hw/core/iommu.h
> 
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index f8d7a4a..d688412 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -5,6 +5,7 @@ common-obj-y += fw-path-provider.o
>  # irq.o needed for qdev GPIO handling:
>  common-obj-y += irq.o
>  common-obj-y += hotplug.o
> +common-obj-y += iommu.o
>  common-obj-y += nmi.o
>  
>  common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
> diff --git a/hw/core/iommu.c b/hw/core/iommu.c
> new file mode 100644
> index 0000000..7c4fcfe
> --- /dev/null
> +++ b/hw/core/iommu.c
> @@ -0,0 +1,58 @@
> +/*
> + * QEMU emulation of IOMMU logic
May be rephrased as it does not really explain what the iommu object
exposes as an API
> + *
> + * Copyright (C) 2017 Red Hat Inc.
> + *
> + * Authors: Peter Xu <peterx@redhat.com>,
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/core/iommu.h"

> +                             IOMMUNotifier *n)
> +
> +void iommu_notifier_register(IOMMUObject *iommu,
> +                             IOMMUNotifier *n,
> +                             IOMMUNotifyFn fn,
> +                             IOMMUEvent event)
> +{
> +    n->event = event;
> +    n->iommu_notify = fn;
> +    QLIST_INSERT_HEAD(&iommu->iommu_notifiers, n, node);
> +    return;
> +}
> +
> +void iommu_notifier_unregister(IOMMUObject *iommu,
> +                               IOMMUNotifier *notifier)
> +{
> +    IOMMUNotifier *cur, *next;
> +
> +    QLIST_FOREACH_SAFE(cur, &iommu->iommu_notifiers, node, next) {
> +        if (cur == notifier) {
> +            QLIST_REMOVE(cur, node);
> +            break;
> +        }
> +    }
> +}
> +
> +void iommu_notify(IOMMUObject *iommu, IOMMUEventData *event_data)
> +{
> +    IOMMUNotifier *cur;
> +
> +    QLIST_FOREACH(cur, &iommu->iommu_notifiers, node) {
> +        if ((cur->event == event_data->event) && cur->iommu_notify) {
can cur->iommu_notify be NULL if registered as above?
> +            cur->iommu_notify(cur, event_data);
> +        }
> +    }
> +}
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 03595e3..8350973 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -26,6 +26,7 @@
>  #include "qom/object.h"
>  #include "qemu/rcu.h"
>  #include "hw/qdev-core.h"
> +#include "hw/core/iommu.h"
>  
>  #define RAM_ADDR_INVALID (~(ram_addr_t)0)
>  
> @@ -301,6 +302,19 @@ struct MemoryListener {
>  };
>  
>  /**
> + * AddressSpaceOps: callbacks structure for address space specific operations
> + *
> + * @iommu_get: returns an IOMMU object that backs the address space.
> + *             Normally this should be NULL for generic address
> + *             spaces, and it's only used when there is one
> + *             translation unit behind this address space.
> + */
> +struct AddressSpaceOps {
> +    IOMMUObject *(*iommu_get)(AddressSpace *as);
> +};
> +typedef struct AddressSpaceOps AddressSpaceOps;
> +
> +/**
>   * AddressSpace: describes a mapping of addresses to #MemoryRegion objects
>   */
>  struct AddressSpace {
> @@ -316,6 +330,7 @@ struct AddressSpace {
>      struct MemoryRegionIoeventfd *ioeventfds;
>      QTAILQ_HEAD(memory_listeners_as, MemoryListener) listeners;
>      QTAILQ_ENTRY(AddressSpace) address_spaces_link;
> +    AddressSpaceOps as_ops;
>  };
>  
>  FlatView *address_space_to_flatview(AddressSpace *as);
> @@ -1988,6 +2003,13 @@ address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
>      address_space_write(cache->as, cache->xlat + addr, MEMTXATTRS_UNSPECIFIED, buf, len);
>  }
>  
> +/**
> + * address_space_iommu_get: Get the backend IOMMU for the address space
> + *
> + * @as: the address space to fetch IOMMU from
> + */
> +IOMMUObject *address_space_iommu_get(AddressSpace *as);
> +
>  #endif
>  
>  #endif
> diff --git a/include/hw/core/iommu.h b/include/hw/core/iommu.h
> new file mode 100644
> index 0000000..34387c0
> --- /dev/null
> +++ b/include/hw/core/iommu.h
> @@ -0,0 +1,73 @@
> +/*
> + * QEMU emulation of IOMMU logic
> + *
> + * Copyright (C) 2017 Red Hat Inc.
> + *
> + * Authors: Peter Xu <peterx@redhat.com>,
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef HW_CORE_IOMMU_H
> +#define HW_CORE_IOMMU_H
> +
> +#include "qemu/queue.h"
> +
> +enum IOMMUEvent {
> +    IOMMU_EVENT_BIND_PASIDT,
> +};
> +typedef enum IOMMUEvent IOMMUEvent;
> +
> +struct IOMMUEventData {
> +    IOMMUEvent event;
/* length and opaque data passed to notifiers */ ?
> +    uint64_t length;
> +    void *data;
> +};
> +typedef struct IOMMUEventData IOMMUEventData;
> +
> +typedef struct IOMMUNotifier IOMMUNotifier;
> +
> +typedef void (*IOMMUNotifyFn)(IOMMUNotifier *notifier,
> +                              IOMMUEventData *event_data);
> +
> +struct IOMMUNotifier {
> +    IOMMUNotifyFn iommu_notify;
> +    /*
> +     * What events we are listening to. Let's allow multiple event
> +     * registrations from beginning.
> +     */
> +    IOMMUEvent event; 
/* the event the notifier is sensitive to ? */
> +    QLIST_ENTRY(IOMMUNotifier) node;
> +};
> +
> +typedef struct IOMMUObject IOMMUObject;
> +
> +/*
> + * This stands for an IOMMU unit. Any translation device should have
> + * this struct inside its own structure to make sure it can leverage
> + * common IOMMU functionalities.
> + */
> +struct IOMMUObject {
> +    QLIST_HEAD(, IOMMUNotifier) iommu_notifiers;
where is the QLIST_INIT supposed to be done?

Thanks

Eric
> +};
> +
> +void iommu_notifier_register(IOMMUObject *iommu,
> +                             IOMMUNotifier *n,
> +                             IOMMUNotifyFn fn,
> +                             IOMMUEvent event);
> +void iommu_notifier_unregister(IOMMUObject *iommu,
> +                               IOMMUNotifier *notifier);
> +void iommu_notify(IOMMUObject *iommu, IOMMUEventData *event_data);
> +
> +#endif
> diff --git a/memory.c b/memory.c
> index 77fb3ef..307f665 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -235,8 +235,6 @@ struct FlatView {
>      MemoryRegion *root;
>  };
>  
> -typedef struct AddressSpaceOps AddressSpaceOps;
> -
>  #define FOR_EACH_FLAT_RANGE(var, view)          \
>      for (var = (view)->ranges; var < (view)->ranges + (view)->nr; ++var)
>  
> @@ -2793,6 +2791,14 @@ static void do_address_space_destroy(AddressSpace *as)
>      memory_region_unref(as->root);
>  }
>  
> +IOMMUObject *address_space_iommu_get(AddressSpace *as)
> +{
> +    if (!as->as_ops.iommu_get) {
> +        return NULL;
> +    }
> +    return as->as_ops.iommu_get(as);
> +}
> +
>  void address_space_destroy(AddressSpace *as)
>  {
>      MemoryRegion *root = as->root;
>