[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RESEND PATCH 2/6] memory: introduce AddressSpaceOps an
From: |
Liu, Yi L |
Subject: |
Re: [Qemu-devel] [RESEND PATCH 2/6] memory: introduce AddressSpaceOps and IOMMUObject |
Date: |
Tue, 14 Nov 2017 22:20:26 +0800 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Hi Eric,
On Tue, Nov 14, 2017 at 11:21:59AM +0100, Auger Eric wrote:
> Hi Yi L,
>
> On 03/11/2017 13:01, Liu, Yi L wrote:
> > From: Peter Xu <address@hidden>
> >
> > 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()
Good point.
> > Signed-off-by: Peter Xu <address@hidden>
> > Signed-off-by: Liu, Yi L <address@hidden>
> > ---
> > 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
yes, may need to rephrase to address it clear.
> > + *
> > + * Copyright (C) 2017 Red Hat Inc.
> > + *
> > + * Authors: Peter Xu <address@hidden>,
> > + *
> > + * 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?
the cur->event is actually initialized with a event and also a iommu_notify.
So if the cur->event meets the requested event type, then it should be
non-NULL.
> > + 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 <address@hidden>,
> > + *
> > + * 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 */ ?
yes, it is. But the "void *data" would be replaced with well defined structure.
No plan to do it as opaque. Once this patchset is accepted, later patchset
would define it as;
struct IOMMUEventData {
IOMMUEvent event;
uint64_t length;
union {
struct pasid_table_config pasidt_info;
struct tlb_invalidate_info tlb_info;
};
};
typedef struct IOMMUEventData IOMMUEventData;
This is in my further patchset. Currently, we want to show the idea of
introducing this new notifier framework.
> > + 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 ? */
events(aka. operations) like bind pasid table/flush 1st level tlb. etc.
> > + 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?
yeah, need to add it accordingly.
Thanks,
Yi L
> 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;
> >
>
- [Qemu-devel] [RESEND PATCH 0/6] Introduce new iommu notifier framework, Liu, Yi L, 2017/11/03
- [Qemu-devel] [RESEND PATCH 2/6] memory: introduce AddressSpaceOps and IOMMUObject, Liu, Yi L, 2017/11/03
- Re: [Qemu-devel] [RESEND PATCH 2/6] memory: introduce AddressSpaceOps and IOMMUObject, Liu, Yi L, 2017/11/13
- Re: [Qemu-devel] [RESEND PATCH 2/6] memory: introduce AddressSpaceOps and IOMMUObject, Auger Eric, 2017/11/14
- Re: [Qemu-devel] [RESEND PATCH 2/6] memory: introduce AddressSpaceOps and IOMMUObject, Liu, Yi L, 2017/11/14
Re: [Qemu-devel] [RESEND PATCH 2/6] memory: introduce AddressSpaceOps and IOMMUObject, Auger Eric, 2017/11/14
- Re: [Qemu-devel] [RESEND PATCH 2/6] memory: introduce AddressSpaceOps and IOMMUObject,
Liu, Yi L <=
[Qemu-devel] [RESEND PATCH 3/6] intel_iommu: provide AddressSpaceOps.iommu_get instance, Liu, Yi L, 2017/11/03
[Qemu-devel] [RESEND PATCH 1/6] memory: rename existing iommu notifier to be iommu mr notifier, Liu, Yi L, 2017/11/03
[Qemu-devel] [RESEND PATCH 4/6] vfio: rename GuestIOMMU to be GuestIOMMUMR, Liu, Yi L, 2017/11/03
[Qemu-devel] [RESEND PATCH 6/6] vfio/pci: register vfio_iommu_bind_pasidtbl_notify notifier, Liu, Yi L, 2017/11/03
[Qemu-devel] [RESEND PATCH 5/6] vfio/pci: add notify framework based on IOMMUObject, Liu, Yi L, 2017/11/03