From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46694) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCGko-0005HD-Ra for qemu-devel@nongnu.org; Mon, 06 Jul 2015 20:28:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCGkl-0008At-JG for qemu-devel@nongnu.org; Mon, 06 Jul 2015 20:28:54 -0400 Received: from mail-pd0-f170.google.com ([209.85.192.170]:36775) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCGkl-0008Am-Al for qemu-devel@nongnu.org; Mon, 06 Jul 2015 20:28:51 -0400 Received: by pddu5 with SMTP id u5so27068460pdd.3 for ; Mon, 06 Jul 2015 17:28:50 -0700 (PDT) References: <1436148670-6592-1-git-send-email-aik@ozlabs.ru> <1436148670-6592-4-git-send-email-aik@ozlabs.ru> <559AAFD5.7000004@redhat.com> From: Alexey Kardashevskiy Message-ID: <559B1D3A.3060004@ozlabs.ru> Date: Tue, 7 Jul 2015 10:28:42 +1000 MIME-Version: 1.0 In-Reply-To: <559AAFD5.7000004@redhat.com> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qemu v10 03/14] spapr_pci: Convert finish_realize() to dma_capabilities_update()+dma_init_window() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier , qemu-devel@nongnu.org Cc: Alex Williamson , David Gibson , qemu-ppc@nongnu.org, Michael Roth , Gavin Shan On 07/07/2015 02:41 AM, Laurent Vivier wrote: > > > On 06/07/2015 04:10, Alexey Kardashevskiy wrote: >> This reworks finish_realize() which used to finalize DMA setup with >> an assumption that it will not change later. >> >> New callbacks supports various window parameters such as page and >> windows sizes. The new callback return error code rather than Error**. >> >> This is a mechanical change so no change in behaviour is expected. >> This is a part of getting rid of spapr-pci-vfio-host-bridge type. >> >> Signed-off-by: Alexey Kardashevskiy >> Reviewed-by: David Gibson >> --- >> Changes: >> v8: >> * moved spapr_phb_dma_capabilities_update() higher to avoid forward >> declaration in following patches and keep DMA code together (i.e. next >> to spapr_pci_dma_iommu()) >> --- >> hw/ppc/spapr_pci.c | 59 ++++++++++++++++++++++++++------------------- >> hw/ppc/spapr_pci_vfio.c | 53 ++++++++++++++++------------------------ >> include/hw/pci-host/spapr.h | 8 +++++- >> 3 files changed, 62 insertions(+), 58 deletions(-) >> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index a8f79d8..c1ca13d 100644 >> --- a/hw/ppc/spapr_pci.c >> +++ b/hw/ppc/spapr_pci.c >> @@ -808,6 +808,28 @@ static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >> return buf; >> } >> >> +static int spapr_phb_dma_capabilities_update(sPAPRPHBState *sphb) >> +{ >> + sphb->dma32_window_start = 0; >> + sphb->dma32_window_size = SPAPR_PCI_DMA32_SIZE; >> + >> + return 0; >> +} >> + >> +static int spapr_phb_dma_init_window(sPAPRPHBState *sphb, >> + uint32_t liobn, uint32_t page_shift, >> + uint64_t window_size) >> +{ >> + uint64_t bus_offset = sphb->dma32_window_start; >> + sPAPRTCETable *tcet; >> + >> + tcet = spapr_tce_new_table(DEVICE(sphb), liobn, bus_offset, page_shift, >> + window_size >> page_shift, >> + false); >> + >> + return tcet ? 0 : -1; >> +} >> + >> /* Macros to operate with address in OF binding to PCI */ >> #define b_x(x, p, l) (((x) & ((1<<(l))-1)) << (p)) >> #define b_n(x) b_x((x), 31, 1) /* 0 if relocatable */ >> @@ -1220,6 +1242,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> int i; >> PCIBus *bus; >> uint64_t msi_window_size = 4096; >> + sPAPRTCETable *tcet; >> >> if (sphb->index != (uint32_t)-1) { >> hwaddr windows_base; >> @@ -1369,33 +1392,18 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> } >> } >> >> - if (!info->finish_realize) { >> - error_setg(errp, "finish_realize not defined"); >> - return; >> - } >> - >> - info->finish_realize(sphb, errp); >> - >> - sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free); >> -} >> - >> -static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp) >> -{ >> - sPAPRTCETable *tcet; >> - uint32_t nb_table; >> - >> - nb_table = SPAPR_PCI_DMA32_SIZE >> SPAPR_TCE_PAGE_SHIFT; >> - tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn, >> - 0, SPAPR_TCE_PAGE_SHIFT, nb_table, false); >> + info->dma_capabilities_update(sphb); >> + info->dma_init_window(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_SHIFT, >> + sphb->dma32_window_size); >> + tcet = spapr_tce_find_by_liobn(sphb->dma_liobn); >> if (!tcet) { >> - error_setg(errp, "Unable to create TCE table for %s", >> - sphb->dtbusname); >> - return ; >> + error_setg(errp, "failed to create TCE table"); >> + return; >> } >> - >> - /* Register default 32bit DMA window */ >> - memory_region_add_subregion(&sphb->iommu_root, 0, >> + memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset, >> spapr_tce_get_iommu(tcet)); >> + >> + sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free); >> } >> >> static int spapr_phb_children_reset(Object *child, void *opaque) >> @@ -1543,9 +1551,10 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data) >> dc->vmsd = &vmstate_spapr_pci; >> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); >> dc->cannot_instantiate_with_device_add_yet = false; >> - spc->finish_realize = spapr_phb_finish_realize; >> hp->plug = spapr_phb_hot_plug_child; >> hp->unplug = spapr_phb_hot_unplug_child; >> + spc->dma_capabilities_update = spapr_phb_dma_capabilities_update; >> + spc->dma_init_window = spapr_phb_dma_init_window; >> } >> >> static const TypeInfo spapr_phb_info = { >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c >> index cca45ed..6e3e17b 100644 >> --- a/hw/ppc/spapr_pci_vfio.c >> +++ b/hw/ppc/spapr_pci_vfio.c >> @@ -28,48 +28,36 @@ static Property spapr_phb_vfio_properties[] = { >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> -static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp) >> +static int spapr_phb_vfio_dma_capabilities_update(sPAPRPHBState *sphb) >> { >> sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); >> struct vfio_iommu_spapr_tce_info info = { .argsz = sizeof(info) }; >> int ret; >> - sPAPRTCETable *tcet; >> - uint32_t liobn = svphb->phb.dma_liobn; >> >> - if (svphb->iommugroupid == -1) { >> - error_setg(errp, "Wrong IOMMU group ID %d", svphb->iommugroupid); >> - return; >> - } >> - >> - ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid, >> - VFIO_CHECK_EXTENSION, >> - (void *) VFIO_SPAPR_TCE_IOMMU); > > > This ioctl() disappears completely, was it useless ? It is called in hw/vfio/common.c and if it fails there, we never get here so there is no point in calling it here too. -- Alexey