From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57246) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abMKU-00058h-Kc for qemu-devel@nongnu.org; Thu, 03 Mar 2016 01:01:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abMKS-0000z3-8G for qemu-devel@nongnu.org; Thu, 03 Mar 2016 01:01:42 -0500 Received: from mail-pf0-x244.google.com ([2607:f8b0:400e:c00::244]:33787) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abMKR-0000xh-UE for qemu-devel@nongnu.org; Thu, 03 Mar 2016 01:01:40 -0500 Received: by mail-pf0-x244.google.com with SMTP id 63so763956pfe.0 for ; Wed, 02 Mar 2016 22:01:38 -0800 (PST) References: <1456823441-46757-1-git-send-email-aik@ozlabs.ru> <1456823441-46757-8-git-send-email-aik@ozlabs.ru> <20160303052820.GH1620@voom.redhat.com> From: Alexey Kardashevskiy Message-ID: <56D7D33B.8000904@ozlabs.ru> Date: Thu, 3 Mar 2016 17:01:31 +1100 MIME-Version: 1.0 In-Reply-To: <20160303052820.GH1620@voom.redhat.com> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 07/16] 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: Alex Williamson , qemu-ppc@nongnu.org, qemu-devel@nongnu.org On 03/03/2016 04:28 PM, David Gibson wrote: > On Tue, Mar 01, 2016 at 08:10:32PM +1100, Alexey Kardashevskiy wrote: >> This adds a vfio_votify() callback to inform an IOMMU (and then its owner) >> that VFIO started using the IOMMU. This is used by the pseries machine to >> enable/disable in-kernel acceleration of TCE hypercalls. >> >> Signed-off-by: Alexey Kardashevskiy > > Hmm.. the current approach of having a hook when vfio-pci devices are > attached is pretty ugly, but what exactly the case that it doesn't > handle and this approach does? Sorry, I am not following you here. What hook do you mean here? My hook fixes the case when I want to enable/disable KVM acceleration, without these patches, I need to re-count how many vfio-pci devices are there and it is more ugly with PCI hotplug/unplug... > This two tiered notify system for a single bit is also kinda ugly. > >> --- >> hw/ppc/spapr_iommu.c | 9 +++++++++ >> hw/ppc/spapr_pci.c | 14 ++++++++------ >> hw/vfio/common.c | 7 +++++++ >> include/exec/memory.h | 2 ++ >> include/hw/ppc/spapr.h | 4 ++++ >> 5 files changed, 30 insertions(+), 6 deletions(-) >> >> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c >> index 8a88a74..67a8356 100644 >> --- a/hw/ppc/spapr_iommu.c >> +++ b/hw/ppc/spapr_iommu.c >> @@ -136,6 +136,13 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr, >> return ret; >> } >> >> +static int spapr_tce_vfio_notify(MemoryRegion *iommu, bool attached) >> +{ >> + sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu); >> + >> + return spapr_tce_vfio_notify_owner(tcet->owner, tcet, attached); > > I'm guessing the "owner" is the PHB, but I'm not entirely clear. > > Could you use the QOM parent to get the the PHB instead of storing it > explicitly? I am pretty sure I am not allowed to use the QOM parent, this is why there is no object_get_parent() helper. > >> +} >> + >> static int spapr_tce_table_post_load(void *opaque, int version_id) >> { >> sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque); >> @@ -167,6 +174,7 @@ static const VMStateDescription vmstate_spapr_tce_table = { >> >> static MemoryRegionIOMMUOps spapr_iommu_ops = { >> .translate = spapr_tce_translate_iommu, >> + .vfio_notify = spapr_tce_vfio_notify, >> }; >> >> static int spapr_tce_table_realize(DeviceState *dev) >> @@ -235,6 +243,7 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn) >> >> tcet = SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE)); >> tcet->liobn = liobn; >> + tcet->owner = owner; >> >> snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn); >> object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL); >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index ee0fecf..b0cd148 100644 >> --- a/hw/ppc/spapr_pci.c >> +++ b/hw/ppc/spapr_pci.c >> @@ -1084,6 +1084,14 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset, >> return 0; >> } >> >> +int spapr_tce_vfio_notify_owner(DeviceState *dev, sPAPRTCETable *tcet, >> + bool attached) >> +{ >> + spapr_tce_set_need_vfio(tcet, attached); > > Hmm.. you go to the trouble of storing owner in dev, then don't > actually use it. Yeah, I need to clean this, I removed spapr_tce_vfio_notify_owner() from my working branch already and call spapr_tce_set_need_vfio() directly from spapr_tce_vfio_notify(). > >> + return 0; >> +} >> + >> /* create OF node for pci device and required OF DT properties */ >> static int spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev, >> void *fdt, int node_offset) >> @@ -1118,12 +1126,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc, >> void *fdt = NULL; >> int fdt_start_offset = 0, fdt_size; >> >> - if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { >> - sPAPRTCETable *tcet = spapr_tce_find_by_liobn(phb->dma_liobn); >> - >> - spapr_tce_set_need_vfio(tcet, true); >> - } >> - >> if (dev->hotplugged) { >> fdt = create_device_tree(&fdt_size); >> fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0); >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 9bf4c3b..ca3fd47 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -384,6 +384,7 @@ static void vfio_listener_region_add(MemoryListener *listener, >> QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next); >> >> memory_region_register_iommu_notifier(giommu->iommu, &giommu->n); >> + giommu->iommu->iommu_ops->vfio_notify(section->mr, true); >> memory_region_iommu_replay(giommu->iommu, &giommu->n, >> vfio_container_granularity(container), >> false); >> @@ -430,6 +431,7 @@ static void vfio_listener_region_del(MemoryListener *listener, >> VFIOContainer *container = container_of(listener, VFIOContainer, listener); >> hwaddr iova, end; >> int ret; >> + MemoryRegion *iommu = NULL; >> >> if (vfio_listener_skipped_section(section)) { >> trace_vfio_listener_region_del_skip( >> @@ -451,6 +453,7 @@ static void vfio_listener_region_del(MemoryListener *listener, >> QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) { >> if (giommu->iommu == section->mr) { >> memory_region_unregister_iommu_notifier(&giommu->n); >> + iommu = giommu->iommu; >> QLIST_REMOVE(giommu, giommu_next); >> g_free(giommu); >> break; >> @@ -483,6 +486,10 @@ static void vfio_listener_region_del(MemoryListener *listener, >> "0x%"HWADDR_PRIx") = %d (%m)", >> container, iova, end - iova, ret); >> } >> + >> + if (iommu && iommu->iommu_ops && iommu->iommu_ops->vfio_notify) { >> + iommu->iommu_ops->vfio_notify(section->mr, false); >> + } > > So, if an IOMMU is removed from the guest, this will turn off VFIO > enablement. However, IIUC this won't get caled in the more likely > case that the address space stays the same, but the VFIO device is > removed. When VFIO device is removed, its listener gets removed and this is supposed to end up calling vfio_listener_region_del(). > >> } >> >> static const MemoryListener vfio_memory_listener = { >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index d5284c2..9f82629 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -150,6 +150,8 @@ typedef struct MemoryRegionIOMMUOps MemoryRegionIOMMUOps; >> struct MemoryRegionIOMMUOps { >> /* Return a TLB entry that contains a given address. */ >> IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool is_write); >> + /* Called when VFIO starts/stops using this */ >> + int (*vfio_notify)(MemoryRegion *iommu, bool attached); >> }; >> >> typedef struct CoalescedMemoryRange CoalescedMemoryRange; >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >> index 8aa0c45..5d2f8f4 100644 >> --- a/include/hw/ppc/spapr.h >> +++ b/include/hw/ppc/spapr.h >> @@ -550,6 +550,7 @@ struct sPAPRTCETable { >> int fd; >> MemoryRegion root, iommu; >> struct VIOsPAPRDevice *vdev; /* for @bypass migration compatibility only */ >> + DeviceState *owner; >> QLIST_ENTRY(sPAPRTCETable) list; >> }; >> >> @@ -629,4 +630,7 @@ int spapr_rng_populate_dt(void *fdt); >> */ >> #define SPAPR_LMB_FLAGS_ASSIGNED 0x00000008 >> >> +int spapr_tce_vfio_notify_owner(DeviceState *dev, sPAPRTCETable *tcet, >> + bool attached); >> + >> #endif /* !defined (__HW_SPAPR_H__) */ > -- Alexey