| Message ID | 1519900415-30314-4-git-send-email-yi.l.liu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/03/2018 11:33, Liu, Yi L wrote: > +void iommu_sva_notifier_unregister(IOMMUSVAContext *sva_ctx, > + IOMMUSVANotifier *notifier) > +{ > + IOMMUSVANotifier *cur, *next; > + > + QLIST_FOREACH_SAFE(cur, &sva_ctx->sva_notifiers, node, next) { > + if (cur == notifier) { > + QLIST_REMOVE(cur, node); > + break; > + } > + } > +} It's enough to just do QLIST_REMOVE(notifier, node) here. Paolo
On Fri, Mar 02, 2018 at 04:13:17PM +0100, Paolo Bonzini wrote: > On 01/03/2018 11:33, Liu, Yi L wrote: > > +void iommu_sva_notifier_unregister(IOMMUSVAContext *sva_ctx, > > + IOMMUSVANotifier *notifier) > > +{ > > + IOMMUSVANotifier *cur, *next; > > + > > + QLIST_FOREACH_SAFE(cur, &sva_ctx->sva_notifiers, node, next) { > > + if (cur == notifier) { > > + QLIST_REMOVE(cur, node); > > + break; > > + } > > + } > > +} > > It's enough to just do QLIST_REMOVE(notifier, node) here. Thanks, will apply in next version. Regards, Yi Liu
On Mon, Mar 05, 2018 at 02:25:09PM +1100, David Gibson wrote: > On Thu, Mar 01, 2018 at 06:31:53PM +0800, Liu, Yi L wrote: > > From: Peter Xu <peterx@redhat.com> > > > > This patch adds IOMMUSVAContext as an abstract for virt-SVA in > > Qemu. > > > > IOMMUSVAContext is per-PASID(Process Address Space Identity). > > A PASID Tagged AddressSpace should have an IOMMUSVAContext > > created for it. virt-SVA emulation for emulated SVA capable > > devices would use IOMMUSVAContext. And for assigned devices, > > Qemu also needs to propagate guest tlb flush to host through > > the sva_notifer based on IOMMUSVAContext. > > > > This patch proposes to include a sva_notifier list and > > an IOMMUSVAContextOps in IOMMUSVAContext. > > > > * The sva_notifier list would include tlb invalidate nofitifer > > to propagate guest's iotlb flush to host. > > * The first callback in IOMMUSVAContextOps would be an address > > translation callback. For the SVA aware DMAs issued by emulated > > SVA capable devices, it requires Qemu to emulate data read/write > > to guest process address space. Qemu needs to do address translation > > with guest process page table. So the IOMMUSVAContextOps.translate() > > callback would be helpful for emulating SVA capable devices. > > > > Note: to fulfill the IOMMUSVAContext based address translation > > framework, may duplicate quite a few existing MemoryRegion based > > translation code in Qemu. As this patchset is mainly to support > > assigned SVA capable devices. So this patchset hasn't done the > > duplication. In future, if any requirement for emulating SVA > > capable device, it would require a separate patchset to fulfill > > the translation framework. > > > > 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/pasid.c | 64 ++++++++++++++++++++++++++++ > > include/hw/core/pasid.h | 110 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 175 insertions(+) > > create mode 100644 hw/core/pasid.c > > create mode 100644 include/hw/core/pasid.h > > [snip] > > + > > +#ifndef HW_PCI_PASID_H > > +#define HW_PCI_PASID_H > > + > > +#include "qemu/queue.h" > > +#ifndef CONFIG_USER_ONLY > > +#include "exec/hwaddr.h" > > +#endif > > + > > +typedef struct IOMMUSVAContext IOMMUSVAContext; > > + > > +enum IOMMUSVAEvent { > > + IOMMU_SVA_EVENT_TLB_INV, > > +}; > > +typedef enum IOMMUSVAEvent IOMMUSVAEvent; > > + > > +struct IOMMUSVAEventData { > > + IOMMUSVAEvent event; > > + uint64_t length; > > + void *data; > > +}; > > +typedef struct IOMMUSVAEventData IOMMUSVAEventData; > > + > > +typedef struct IOMMUSVANotifier IOMMUSVANotifier; > > + > > +typedef void (*IOMMUSVANotifyFn)(IOMMUSVANotifier *notifier, > > + IOMMUSVAEventData *event_data); > > + > > +typedef struct IOMMUSVATLBEntry IOMMUSVATLBEntry; > > + > > +/* See address_space_translate: bit 0 is read, bit 1 is write. */ > > +typedef enum { > > + IOMMU_SVA_NONE = 0, > > + IOMMU_SVA_RO = 1, > > + IOMMU_SVA_WO = 2, > > + IOMMU_SVA_RW = 3, > > +} IOMMUSVAAccessFlags; > > + > > +#define IOMMU_SVA_ACCESS_FLAG(r, w) (((r) ? IOMMU_SVA_RO : 0) | \ > > + ((w) ? IOMMU_SVA_WO : 0)) > > + > > +struct IOMMUSVATLBEntry { > > + AddressSpace *target_as; > > + hwaddr va; > > + hwaddr translated_addr; > > + hwaddr addr_mask; /* 0xfff = 4k translation */ > > + IOMMUSVAAccessFlags perm; > > +}; > > + > > +typedef struct IOMMUSVAContextOps IOMMUSVAContextOps; > > +struct IOMMUSVAContextOps { > > + /* Return a TLB entry that contains a given address. */ > > + IOMMUSVATLBEntry (*translate)(IOMMUSVAContext *sva_ctx, > > + hwaddr addr, bool is_write); > > +}; > > A lot of the above seems to just duplicate stuff from IOMMU MRs and > it's not clear why we need both. yes, this is for the potential SVA aware DMA emulation. And this is similar to IOMMU MRs. Only difference is the translation for PASID tagged address space is based on IOMMUSVAContext. As why we need both, it is due to not proper to mix SVA notifier with MAP/UNMAP notifier in a chain. > > +struct IOMMUSVANotifier { > > + IOMMUSVANotifyFn sva_notify; > > + /* > > + * What events we are listening to. Let's allow multiple event > > + * registrations from beginning. > > + */ > > + IOMMUSVAEvent event; > > + QLIST_ENTRY(IOMMUSVANotifier) node; > > +}; > > + > > +/* > > + * 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 IOMMUSVAContext { > > + uint32_t pasid; > > + QLIST_HEAD(, IOMMUSVANotifier) sva_notifiers; > > + const IOMMUSVAContextOps *sva_ctx_ops; > > +}; > > I think the problem is here. The SVAContext represents a *single* > PASID, and once you have a single PASID the resulting object *is* > functionally equivalent to an AddressSpace (though effectively > required to have nothing but a single IOMMUMR within it). I also evaluated reusing IOMMU MR. If reuse IOMMU MR, then the SVA notifiers would be in the same list which MAP/UNMAP notifier locates. This may break some existing logic. e.g. each time the registration of MR notifier would result in flag changed, and some vIOMMU emulator logic relies on it. Also, the replay logic in intel_iommu emulator also relies on the MAP/UNMAP notifier, if new notifier added in the list, it may be a confusion. So I didn't go with reusing IOMMU MR. But any better idea would be welcomed. > It also seems to me unlikely that different PASIDs for the same device > / IOMMU domain will have truly different sva_ctx_ops. yes, sva_ctx_ops should be the same for different PASIDs. So far, translate callback is the only candidate. > It really seems to me the object you actually want is a level up from > that, representing the whole cluster of address spaces indexed by > PASID. They would have the same operations for all PASIDs in the > cluster, but those would take the pasid number. yes, that's also my thought. Here IOMMUSVAContext is supposed to be per-PASID. But the sva_ctx_ops pointer is actually shared by all IOMMUSVAContext instances. For the sva_notifiers list, I think it should be per-PASID since some address spaces indexed by PASID doesn't require SVA notifier. e.g. the one binded to an emulated SVA capable device. But the notifier functions are also shared by all IOMMUSVAContext instances. In this series, IOMMUSVAContext co-exists with an AddressSpace within a super structure(VTDPASIDAddressSpace in patch 11 of this series). Thanks, Yi Liu
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs index 1240728..01989d2 100644 --- a/hw/core/Makefile.objs +++ b/hw/core/Makefile.objs @@ -6,6 +6,7 @@ common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o # irq.o needed for qdev GPIO handling: common-obj-y += irq.o common-obj-y += hotplug.o +common-obj-y += pasid.o common-obj-$(CONFIG_SOFTMMU) += nmi.o common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o diff --git a/hw/core/pasid.c b/hw/core/pasid.c new file mode 100644 index 0000000..c4b0c5d --- /dev/null +++ b/hw/core/pasid.c @@ -0,0 +1,64 @@ +/* + * QEMU abstract of Shared Virtual Memory logic + * + * Copyright (C) 2018 Red Hat Inc. + * + * Authors: Peter Xu <peterx@redhat.com>, + * Liu, Yi L <yi.l.liu@intel.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/pasid.h" + +void iommu_sva_notifier_register(IOMMUSVAContext *sva_ctx, + IOMMUSVANotifier *n, + IOMMUSVANotifyFn fn, + IOMMUSVAEvent event) +{ + n->event = event; + n->sva_notify = fn; + QLIST_INSERT_HEAD(&sva_ctx->sva_notifiers, n, node); + return; +} + +void iommu_sva_notifier_unregister(IOMMUSVAContext *sva_ctx, + IOMMUSVANotifier *notifier) +{ + IOMMUSVANotifier *cur, *next; + + QLIST_FOREACH_SAFE(cur, &sva_ctx->sva_notifiers, node, next) { + if (cur == notifier) { + QLIST_REMOVE(cur, node); + break; + } + } +} + +void iommu_sva_notify(IOMMUSVAContext *sva_ctx, IOMMUSVAEventData *event_data) +{ + IOMMUSVANotifier *cur; + + QLIST_FOREACH(cur, &sva_ctx->sva_notifiers, node) { + if ((cur->event == event_data->event) && cur->sva_notify) { + cur->sva_notify(cur, event_data); + } + } +} + +void iommu_sva_ctx_init(IOMMUSVAContext *sva_ctx) +{ + QLIST_INIT(&sva_ctx->sva_notifiers); +} diff --git a/include/hw/core/pasid.h b/include/hw/core/pasid.h new file mode 100644 index 0000000..4c7dccb --- /dev/null +++ b/include/hw/core/pasid.h @@ -0,0 +1,110 @@ +/* + * QEMU abstraction of Shared Virtual Memory + * + * Copyright (C) 2018 Red Hat Inc. + * + * Authors: Peter Xu <peterx@redhat.com>, + * Liu, Yi L <yi.l.liu@intel.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_PCI_PASID_H +#define HW_PCI_PASID_H + +#include "qemu/queue.h" +#ifndef CONFIG_USER_ONLY +#include "exec/hwaddr.h" +#endif + +typedef struct IOMMUSVAContext IOMMUSVAContext; + +enum IOMMUSVAEvent { + IOMMU_SVA_EVENT_TLB_INV, +}; +typedef enum IOMMUSVAEvent IOMMUSVAEvent; + +struct IOMMUSVAEventData { + IOMMUSVAEvent event; + uint64_t length; + void *data; +}; +typedef struct IOMMUSVAEventData IOMMUSVAEventData; + +typedef struct IOMMUSVANotifier IOMMUSVANotifier; + +typedef void (*IOMMUSVANotifyFn)(IOMMUSVANotifier *notifier, + IOMMUSVAEventData *event_data); + +typedef struct IOMMUSVATLBEntry IOMMUSVATLBEntry; + +/* See address_space_translate: bit 0 is read, bit 1 is write. */ +typedef enum { + IOMMU_SVA_NONE = 0, + IOMMU_SVA_RO = 1, + IOMMU_SVA_WO = 2, + IOMMU_SVA_RW = 3, +} IOMMUSVAAccessFlags; + +#define IOMMU_SVA_ACCESS_FLAG(r, w) (((r) ? IOMMU_SVA_RO : 0) | \ + ((w) ? IOMMU_SVA_WO : 0)) + +struct IOMMUSVATLBEntry { + AddressSpace *target_as; + hwaddr va; + hwaddr translated_addr; + hwaddr addr_mask; /* 0xfff = 4k translation */ + IOMMUSVAAccessFlags perm; +}; + +typedef struct IOMMUSVAContextOps IOMMUSVAContextOps; +struct IOMMUSVAContextOps { + /* Return a TLB entry that contains a given address. */ + IOMMUSVATLBEntry (*translate)(IOMMUSVAContext *sva_ctx, + hwaddr addr, bool is_write); +}; + +struct IOMMUSVANotifier { + IOMMUSVANotifyFn sva_notify; + /* + * What events we are listening to. Let's allow multiple event + * registrations from beginning. + */ + IOMMUSVAEvent event; + QLIST_ENTRY(IOMMUSVANotifier) node; +}; + +/* + * 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 IOMMUSVAContext { + uint32_t pasid; + QLIST_HEAD(, IOMMUSVANotifier) sva_notifiers; + const IOMMUSVAContextOps *sva_ctx_ops; +}; + +void iommu_sva_notifier_register(IOMMUSVAContext *sva_ctx, + IOMMUSVANotifier *n, + IOMMUSVANotifyFn fn, + IOMMUSVAEvent event); +void iommu_sva_notifier_unregister(IOMMUSVAContext *sva_ctx, + IOMMUSVANotifier *notifier); +void iommu_sva_notify(IOMMUSVAContext *sva_ctx, + IOMMUSVAEventData *event_data); + +void iommu_sva_ctx_init(IOMMUSVAContext *sva_ctx); + +#endif