On Thu, Oct 24, 2019 at 08:34:25AM -0400, Liu Yi L wrote: > From: Peter Xu <[email protected]> > > This patch adds IOMMUContext as an abstract layer of IOMMU related > operations. The current usage of this abstract layer is setup dual- > stage IOMMU translation (vSVA) for vIOMMU. > > To setup dual-stage IOMMU translation, vIOMMU needs to propagate > guest changes to host via passthru channels (e.g. VFIO). To have > a better abstraction, it is better to avoid direct calling between > vIOMMU and VFIO. So we have this new structure to act as abstract > layer between VFIO and vIOMMU. So far, it is proposed to provide a > notifier mechanism, which registered by VFIO and fired by vIOMMU. > > For more background, may refer to the discussion below: > > https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg05022.html > > Cc: Kevin Tian <[email protected]> > Cc: Jacob Pan <[email protected]> > Cc: Peter Xu <[email protected]> > Cc: Eric Auger <[email protected]> > Cc: Yi Sun <[email protected]> > Cc: David Gibson <[email protected]> > Signed-off-by: Peter Xu <[email protected]> > Signed-off-by: Liu Yi L <[email protected]> > --- > hw/Makefile.objs | 1 + > hw/iommu/Makefile.objs | 1 + > hw/iommu/iommu.c | 66 ++++++++++++++++++++++++++++++++++++++++ > include/hw/iommu/iommu.h | 79 > ++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 147 insertions(+) > create mode 100644 hw/iommu/Makefile.objs > create mode 100644 hw/iommu/iommu.c > create mode 100644 include/hw/iommu/iommu.h > > diff --git a/hw/Makefile.objs b/hw/Makefile.objs > index ece6cc3..ac19f9c 100644 > --- a/hw/Makefile.objs > +++ b/hw/Makefile.objs > @@ -39,6 +39,7 @@ devices-dirs-y += xen/ > devices-dirs-$(CONFIG_MEM_DEVICE) += mem/ > devices-dirs-y += semihosting/ > devices-dirs-y += smbios/ > +devices-dirs-y += iommu/ > endif > > common-obj-y += $(devices-dirs-y) > diff --git a/hw/iommu/Makefile.objs b/hw/iommu/Makefile.objs > new file mode 100644 > index 0000000..0484b79 > --- /dev/null > +++ b/hw/iommu/Makefile.objs > @@ -0,0 +1 @@ > +obj-y += iommu.o > diff --git a/hw/iommu/iommu.c b/hw/iommu/iommu.c > new file mode 100644 > index 0000000..2391b0d > --- /dev/null > +++ b/hw/iommu/iommu.c > @@ -0,0 +1,66 @@ > +/* > + * QEMU abstract of IOMMU context > + * > + * Copyright (C) 2019 Red Hat Inc. > + * > + * Authors: Peter Xu <[email protected]>, > + * Liu Yi L <[email protected]> > + * > + * 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/iommu/iommu.h" > + > +void iommu_ctx_notifier_register(IOMMUContext *iommu_ctx, > + IOMMUCTXNotifier *n, > + IOMMUCTXNotifyFn fn, > + IOMMUCTXEvent event) > +{ > + n->event = event; > + n->iommu_ctx_event_notify = fn; > + QLIST_INSERT_HEAD(&iommu_ctx->iommu_ctx_notifiers, n, node);
Having this both modify the IOMMUCTXNotifier structure and insert it
in the list seems confusing to me - and gratuitously different from
the interface for both IOMMUNotifier and Notifier.
Separating out a iommu_ctx_notifier_init() as a helper and having
register take a fully initialized structure seems better to me.
> + return;
Using an explicit return at the end of a function returning void is an
odd style.
> +}
> +
> +void iommu_ctx_notifier_unregister(IOMMUContext *iommu_ctx,
> + IOMMUCTXNotifier *notifier)
> +{
> + IOMMUCTXNotifier *cur, *next;
> +
> + QLIST_FOREACH_SAFE(cur, &iommu_ctx->iommu_ctx_notifiers, node, next) {
> + if (cur == notifier) {
> + QLIST_REMOVE(cur, node);
> + break;
> + }
> + }
> +}
> +
> +void iommu_ctx_event_notify(IOMMUContext *iommu_ctx,
> + IOMMUCTXEventData *event_data)
> +{
> + IOMMUCTXNotifier *cur;
> +
> + QLIST_FOREACH(cur, &iommu_ctx->iommu_ctx_notifiers, node) {
> + if ((cur->event == event_data->event) &&
> + cur->iommu_ctx_event_notify) {
Do you actually need the test on iommu_ctx_event_notify? I can't see
any reason to register a notifier with a NULL function pointer.
> + cur->iommu_ctx_event_notify(cur, event_data);
> + }
> + }
> +}
> +
> +void iommu_context_init(IOMMUContext *iommu_ctx)
> +{
> + QLIST_INIT(&iommu_ctx->iommu_ctx_notifiers);
> +}
> diff --git a/include/hw/iommu/iommu.h b/include/hw/iommu/iommu.h
> new file mode 100644
> index 0000000..c22c442
> --- /dev/null
> +++ b/include/hw/iommu/iommu.h
> @@ -0,0 +1,79 @@
> +/*
> + * QEMU abstraction of IOMMU Context
> + *
> + * Copyright (C) 2019 Red Hat Inc.
> + *
> + * Authors: Peter Xu <[email protected]>,
> + * Liu, Yi L <[email protected]>
> + *
> + * 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_PCI_PASID_H
> +#define HW_PCI_PASID_H
These guards need to be updated for the new header name.
> +
> +#include "qemu/queue.h"
> +#ifndef CONFIG_USER_ONLY
> +#include "exec/hwaddr.h"
> +#endif
> +
> +typedef struct IOMMUContext IOMMUContext;
> +
> +enum IOMMUCTXEvent {
> + IOMMU_CTX_EVENT_NUM,
> +};
> +typedef enum IOMMUCTXEvent IOMMUCTXEvent;
> +
> +struct IOMMUCTXEventData {
> + IOMMUCTXEvent event;
> + uint64_t length;
> + void *data;
> +};
> +typedef struct IOMMUCTXEventData IOMMUCTXEventData;
> +
> +typedef struct IOMMUCTXNotifier IOMMUCTXNotifier;
> +
> +typedef void (*IOMMUCTXNotifyFn)(IOMMUCTXNotifier *notifier,
> + IOMMUCTXEventData *event_data);
> +
> +struct IOMMUCTXNotifier {
> + IOMMUCTXNotifyFn iommu_ctx_event_notify;
> + /*
> + * What events we are listening to. Let's allow multiple event
> + * registrations from beginning.
> + */
> + IOMMUCTXEvent event;
> + QLIST_ENTRY(IOMMUCTXNotifier) node;
> +};
> +
> +/*
> + * This is an abstraction of IOMMU context.
> + */
> +struct IOMMUContext {
> + uint32_t pasid;
This confuses me a bit. I thought the idea was that IOMMUContext with
SVM would represent all the PASIDs in use, but here we have a specific
pasid stored in the structure.
> + QLIST_HEAD(, IOMMUCTXNotifier) iommu_ctx_notifiers;
> +};
> +
> +void iommu_ctx_notifier_register(IOMMUContext *iommu_ctx,
> + IOMMUCTXNotifier *n,
> + IOMMUCTXNotifyFn fn,
> + IOMMUCTXEvent event);
> +void iommu_ctx_notifier_unregister(IOMMUContext *iommu_ctx,
> + IOMMUCTXNotifier *notifier);
> +void iommu_ctx_event_notify(IOMMUContext *iommu_ctx,
> + IOMMUCTXEventData *event_data);
> +
> +void iommu_context_init(IOMMUContext *iommu_ctx);
> +
> +#endif
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
