[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH qemu v15 14/17] spapr_iommu, vfio, memory: Notif

From: Alexey Kardashevskiy
Subject: Re: [Qemu-devel] [PATCH qemu v15 14/17] spapr_iommu, vfio, memory: Notify IOMMU about starting/stopping being used by VFIO
Date: Thu, 21 Apr 2016 14:22:01 +1000
User-agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2

On 04/21/2016 01:59 PM, David Gibson wrote:
On Wed, Apr 20, 2016 at 07:15:15PM +1000, Alexey Kardashevskiy wrote:
On 04/07/2016 10:40 AM, David Gibson wrote:
On Mon, Apr 04, 2016 at 07:33:43PM +1000, Alexey Kardashevskiy wrote:
The sPAPR TCE tables manage 2 copies when VFIO is using an IOMMU -
a guest view of the table and a hardware TCE table. If there is no VFIO
presense in the address space, then just the guest view is used, if
this is the case, it is allocated in the KVM. However since there is no
support yet for VFIO in KVM TCE hypercalls, when we start using VFIO,
we need to move the guest view from KVM to the userspace; and we need
to do this for every IOMMU on a bus with VFIO devices.

This adds vfio_start/vfio_stop callbacks in MemoryRegionIOMMUOps to
notifiy IOMMU about changing environment so it can reallocate the table
to/from KVM or (when available) hook the IOMMU groups with the logical
bus (LIOBN) in the KVM.

This removes explicit spapr_tce_set_need_vfio() call from PCI hotplug
path as the new callbacks do this better - they notify IOMMU at
the exact moment when the configuration is changed, and this also
includes the case of PCI hot unplug.

As there can be multiple containers attached to the same PHB/LIOBN,
this replaces the @need_vfio flag in sPAPRTCETable with the counter
of VFIO users.

Signed-off-by: Alexey Kardashevskiy <address@hidden>

This looks correct, but there's one remaining ugly.

* s/need_vfio/vfio-Users/g
  hw/ppc/spapr_iommu.c   | 30 ++++++++++++++++++++----------
  hw/ppc/spapr_pci.c     |  6 ------
  hw/vfio/common.c       |  9 +++++++++
  include/exec/memory.h  |  4 ++++
  include/hw/ppc/spapr.h |  2 +-
  5 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index c945dba..ea09414 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -155,6 +155,16 @@ static uint64_t spapr_tce_get_page_sizes(MemoryRegion 
      return 1ULL << tcet->page_shift;

+static void spapr_tce_vfio_start(MemoryRegion *iommu)
+    spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), true);
+static void spapr_tce_vfio_stop(MemoryRegion *iommu)
+    spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), false);
  static void spapr_tce_table_do_enable(sPAPRTCETable *tcet);
  static void spapr_tce_table_do_disable(sPAPRTCETable *tcet);

@@ -239,6 +249,8 @@ static const VMStateDescription vmstate_spapr_tce_table = {
  static MemoryRegionIOMMUOps spapr_iommu_ops = {
      .translate = spapr_tce_translate_iommu,
      .get_page_sizes = spapr_tce_get_page_sizes,
+    .vfio_start = spapr_tce_vfio_start,
+    .vfio_stop = spapr_tce_vfio_stop,

Ok, so AFAICT these callbacks are called whenever a VFIO context is
added / removed from the gIOMMU's address space, and it's up to the
gIOMMU code to ref count that to see if there are any current vfio
users.  That makes "vfio_start" and "vfio_stop" not great names.

But.. better than changing the names would be to move the refcounting
to the generic code if you can manage it, so the individual gIOMMU
backends don't need to - they just told when they need to start / stop
providing VFIO support.

Everything is manageable...

This referencing is needed for the case of >=2 containers so
2xvfio_listener_region_add will create 2xVFIOGuestIOMMU as they are per
VFIOContainer so VFIOGuestIOMMU is not the right place for the reference
counting, VFIOAddressSpace seems to be that place (=> add list of IOMMU MRs
with refcounter). Or even IOMMU MR. Or move VFIOGuestIOMMU list from
VFIOContainer to VFIOAddressSpace and then gIOMMU can handle

I'm having a lot of trouble parsing that.  I think the ref parsing has
to be per-giommu (because individual giommus could, in theory, be
mapped or unmapped from an address space).

Example 1.
POWER8, no DDW, one QEMU PHB, 2 IOMMU groups, table sharing so just 1 container, one TCE table (aka gIOMMU), one TCE table in KVM, no reference counting needed at all, simple.

Example 2.
POWER7, no DDW, one QEMU PHB, 2 IOMMU groups, no table sharing so there are 2 containers but still one IOMMU MR which is added to each container so there are 2 gIOMMU objects. And there is still one TCE table in KVM (which is a guest view). Where do I put the reference counter which will count that there are 2 gIOMMUs per KVM TCE table in this example?

But I think that should be
in the vfio core, rather than being necessary in every giommu

I agree, I am just asking where exactly to put this counter.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]