From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46330) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1auskP-0004jH-Jv for qemu-devel@nongnu.org; Mon, 25 Apr 2016 22:29:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1auskM-00088x-1I for qemu-devel@nongnu.org; Mon, 25 Apr 2016 22:29:09 -0400 Received: from mail-pf0-x22e.google.com ([2607:f8b0:400e:c00::22e]:35819) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1auskL-00087g-OQ for qemu-devel@nongnu.org; Mon, 25 Apr 2016 22:29:05 -0400 Received: by mail-pf0-x22e.google.com with SMTP id n1so785534pfn.2 for ; Mon, 25 Apr 2016 19:29:04 -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> <57185569.3070405@ozlabs.ru> From: Alexey Kardashevskiy Message-ID: <571ED265.7070301@ozlabs.ru> Date: Tue, 26 Apr 2016 12:28:53 +1000 MIME-Version: 1.0 In-Reply-To: <57185569.3070405@ozlabs.ru> 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 02:22 PM, Alexey Kardashevskiy wrote: > 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? Ping? > > >> 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