Re: [RFC v2 04/22] hw/iommu: introduce IOMMUContext

[David Gibson]

On Thu, Oct 24, 2019 at 08:34:25AM -0400, Liu Yi L wrote:
> From: Peter Xu <address@hidden>
> 
> 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 <address@hidden>
> Cc: Jacob Pan <address@hidden>
> Cc: Peter Xu <address@hidden>
> Cc: Eric Auger <address@hidden>
> Cc: Yi Sun <address@hidden>
> Cc: David Gibson <address@hidden>
> Signed-off-by: Peter Xu <address@hidden>
> Signed-off-by: Liu Yi L <address@hidden>
> ---
>  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 <address@hidden>,
> + *          Liu Yi L <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/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 <address@hidden>,
> + *          Liu, Yi L <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_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