From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51209) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1at683-0002Zb-TG for qemu-devel@nongnu.org; Thu, 21 Apr 2016 00:22:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1at680-00014q-Js for qemu-devel@nongnu.org; Thu, 21 Apr 2016 00:22:11 -0400 Received: from mail-pa0-x22c.google.com ([2607:f8b0:400e:c03::22c]:35927) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1at680-00014k-8N for qemu-devel@nongnu.org; Thu, 21 Apr 2016 00:22:08 -0400 Received: by mail-pa0-x22c.google.com with SMTP id er2so24687707pad.3 for ; Wed, 20 Apr 2016 21:22:08 -0700 (PDT) References: <1459762426-18440-1-git-send-email-aik@ozlabs.ru> <1459762426-18440-15-git-send-email-aik@ozlabs.ru> <20160407004056.GE16485@voom.fritz.box> <571748A3.4070105@ozlabs.ru> <20160421035954.GH1133@voom> From: Alexey Kardashevskiy Message-ID: <57185569.3070405@ozlabs.ru> Date: Thu, 21 Apr 2016 14:22:01 +1000 MIME-Version: 1.0 In-Reply-To: <20160421035954.GH1133@voom> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qemu v15 14/17] spapr_iommu, vfio, memory: Notify IOMMU about starting/stopping being used by VFIO List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Alex Williamson , Alexander Graf 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 >>> >>> This looks correct, but there's one remaining ugly. >>> >>>> --- >>>> Changes: >>>> v15: >>>> * 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 *iommu) >>>> 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 >> refcounting? > > 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 > implementation. I agree, I am just asking where exactly to put this counter. -- Alexey