From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55715) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1advCI-0004tN-3W for qemu-devel@nongnu.org; Thu, 10 Mar 2016 02:39:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1advCE-0003lG-R8 for qemu-devel@nongnu.org; Thu, 10 Mar 2016 02:39:50 -0500 Received: from mail-pa0-x242.google.com ([2607:f8b0:400e:c03::242]:34667) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1advCE-0003lB-D7 for qemu-devel@nongnu.org; Thu, 10 Mar 2016 02:39:46 -0500 Received: by mail-pa0-x242.google.com with SMTP id hj7so4978635pac.1 for ; Wed, 09 Mar 2016 23:39:46 -0800 (PST) References: <1456823441-46757-1-git-send-email-aik@ozlabs.ru> <1456823441-46757-5-git-send-email-aik@ozlabs.ru> <20160303030002.GE1620@voom.redhat.com> From: Alexey Kardashevskiy Message-ID: <56E124BB.4010207@ozlabs.ru> Date: Thu, 10 Mar 2016 18:39:39 +1100 MIME-Version: 1.0 In-Reply-To: <20160303030002.GE1620@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 04/16] spapr_iommu: Introduce "enabled" state for TCE table 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 02:00 PM, David Gibson wrote: > On Tue, Mar 01, 2016 at 08:10:29PM +1100, Alexey Kardashevskiy wrote: >> Currently TCE tables are created once at start and their sizes never >> change. We are going to change that by introducing a Dynamic DMA windows >> support where DMA configuration may change during the guest execution. >> >> This changes spapr_tce_new_table() to create an empty zero-size IOMMU >> memory region (IOMMU MR). Only LIOBN is assigned by the time of creation. >> It still will be called once at the owner object (VIO or PHB) creation. >> >> This introduces an "enabled" state for TCE table objects with two >> helper functions - spapr_tce_table_enable()/spapr_tce_table_disable(). >> - spapr_tce_table_enable() receives TCE table parameters, allocates >> a guest view of the TCE table (in the user space or KVM) and >> sets the correct size on the IOMMU MR. >> - spapr_tce_table_disable() disposes the table and resets the IOMMU MR >> size. >> >> This changes the PHB reset handler to do the default DMA initialization >> instead of spapr_phb_realize(). This does not make differenct now but >> later with more than just one DMA window, we will have to remove them all >> and create the default one on a system reset. >> >> No visible change in behaviour is expected except the actual table >> will be reallocated every reset. We might optimize this later. >> >> The other way to implement this would be dynamically create/remove >> the TCE table QOM objects but this would make migration impossible >> as the migration code expects all QOM objects to exist at the receiver >> so we have to have TCE table objects created when migration begins. >> >> spapr_tce_table_do_enable() is separated from from spapr_tce_table_enable() >> as later it will be called at the sPAPRTCETable post-migration stage when >> it already has all the properties set after the migration. >> >> Signed-off-by: Alexey Kardashevskiy > > Reviewed-by: David Gibson > > Although there's one nit that could be improved: > > >> --- >> hw/ppc/spapr_iommu.c | 80 +++++++++++++++++++++++++++++++++++--------------- >> hw/ppc/spapr_pci.c | 21 +++++++++---- >> hw/ppc/spapr_vio.c | 9 +++--- >> include/hw/ppc/spapr.h | 10 +++---- >> 4 files changed, 80 insertions(+), 40 deletions(-) >> >> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c >> index 8132f64..e66e128 100644 >> --- a/hw/ppc/spapr_iommu.c >> +++ b/hw/ppc/spapr_iommu.c >> @@ -174,15 +174,8 @@ static int spapr_tce_table_realize(DeviceState *dev) >> sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev); >> >> tcet->fd = -1; >> - tcet->table = spapr_tce_alloc_table(tcet->liobn, >> - tcet->page_shift, >> - tcet->nb_table, >> - &tcet->fd, >> - tcet->need_vfio); >> - >> memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_ops, >> - "iommu-spapr", >> - (uint64_t)tcet->nb_table << tcet->page_shift); >> + "iommu-spapr", 0); >> >> QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list); >> >> @@ -224,14 +217,10 @@ void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, bool need_vfio) >> tcet->table = newtable; >> } >> >> -sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, >> - uint64_t bus_offset, >> - uint32_t page_shift, >> - uint32_t nb_table, >> - bool need_vfio) >> +sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn) >> { >> sPAPRTCETable *tcet; >> - char tmp[64]; >> + char tmp[32]; >> >> if (spapr_tce_find_by_liobn(liobn)) { >> fprintf(stderr, "Attempted to create TCE table with duplicate" >> @@ -239,16 +228,8 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, >> return NULL; >> } >> >> - if (!nb_table) { >> - return NULL; >> - } >> - >> tcet = SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE)); >> tcet->liobn = liobn; >> - tcet->bus_offset = bus_offset; >> - tcet->page_shift = page_shift; >> - tcet->nb_table = nb_table; >> - tcet->need_vfio = need_vfio; >> >> snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn); >> object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL); >> @@ -258,14 +239,65 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, >> return tcet; >> } >> >> +static void spapr_tce_table_do_enable(sPAPRTCETable *tcet) >> +{ >> + if (!tcet->nb_table) { >> + return; >> + } >> + >> + tcet->table = spapr_tce_alloc_table(tcet->liobn, >> + tcet->page_shift, >> + tcet->nb_table, >> + &tcet->fd, >> + tcet->need_vfio); >> + >> + memory_region_set_size(&tcet->iommu, >> + (uint64_t)tcet->nb_table << tcet->page_shift); >> + >> + tcet->enabled = true; >> +} >> + >> +void spapr_tce_table_enable(sPAPRTCETable *tcet, >> + uint32_t page_shift, uint64_t bus_offset, >> + uint32_t nb_table, bool need_vfio) >> +{ >> + if (tcet->enabled) { >> + return; > > If the given parameters are different from the current ones, treating > this as a no-op is rather misleading. I gather that to resize the > window you're expected to disable, then re-enable. In which case I > think it would be safer to actually throw some kind of error on a > double enable. I'll add here error_report("Warning: trying to enable already enabled TCE table"); ... > > >> + } >> + >> + tcet->bus_offset = bus_offset; >> + tcet->page_shift = page_shift; >> + tcet->nb_table = nb_table; >> + tcet->need_vfio = need_vfio; >> + >> + spapr_tce_table_do_enable(tcet); >> +} >> + >> +static void spapr_tce_table_disable(sPAPRTCETable *tcet) >> +{ >> + if (!tcet->enabled) { ...and error_report("Warning: trying to disable already disabled TCE table"); or g_assert()? >> + return; >> + } >> + >> + memory_region_set_size(&tcet->iommu, 0); >> + >> + spapr_tce_free_table(tcet->table, tcet->fd, tcet->nb_table); >> + tcet->fd = -1; >> + tcet->table = NULL; >> + tcet->enabled = false; >> + tcet->bus_offset = 0; >> + tcet->page_shift = 0; >> + tcet->nb_table = 0; >> + tcet->need_vfio = false; >> +} >> + >> static void spapr_tce_table_unrealize(DeviceState *dev, Error **errp) >> { >> sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev); >> >> QLIST_REMOVE(tcet, list); >> >> - spapr_tce_free_table(tcet->table, tcet->fd, tcet->nb_table); >> - tcet->fd = -1; >> + spapr_tce_table_disable(tcet); >> } >> >> MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet) >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index 248f20a..c34a906 100644 >> --- a/hw/ppc/spapr_pci.c >> +++ b/hw/ppc/spapr_pci.c >> @@ -815,12 +815,13 @@ static int spapr_phb_dma_window_enable(sPAPRPHBState *sphb, >> return -1; >> } >> >> - tcet = spapr_tce_new_table(DEVICE(sphb), liobn, window_addr, >> - page_shift, nb_table, false); >> + tcet = spapr_tce_find_by_liobn(liobn); >> if (!tcet) { >> return -1; >> } >> >> + spapr_tce_table_enable(tcet, page_shift, window_addr, nb_table, false); >> + >> memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset, >> spapr_tce_get_iommu(tcet)); >> return 0; >> @@ -1251,6 +1252,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; >> @@ -1402,11 +1404,18 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> } >> } >> >> + /* DMA setup */ >> + tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn); >> + if (!tcet) { >> + error_report("No default TCE table for %s", sphb->dtbusname); >> + return; >> + } >> + >> /* Register default 32bit DMA window */ >> - if (spapr_phb_dma_window_enable(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_SHIFT, >> - sphb->dma_win_addr, sphb->dma_win_size)) { >> - error_setg(errp, "Unable to create TCE table for %s", sphb->dtbusname); >> - } >> + spapr_phb_dma_window_enable(sphb, sphb->dma_liobn, >> + SPAPR_TCE_PAGE_SHIFT, >> + sphb->dma_win_addr, >> + sphb->dma_win_size); >> >> sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free); >> } >> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c >> index 0f61a55..a745884 100644 >> --- a/hw/ppc/spapr_vio.c >> +++ b/hw/ppc/spapr_vio.c >> @@ -481,11 +481,10 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp) >> memory_region_add_subregion_overlap(&dev->mrroot, 0, &dev->mrbypass, 1); >> address_space_init(&dev->as, &dev->mrroot, qdev->id); >> >> - dev->tcet = spapr_tce_new_table(qdev, liobn, >> - 0, >> - SPAPR_TCE_PAGE_SHIFT, >> - pc->rtce_window_size >> >> - SPAPR_TCE_PAGE_SHIFT, false); >> + dev->tcet = spapr_tce_new_table(qdev, liobn); >> + spapr_tce_table_enable(dev->tcet, SPAPR_TCE_PAGE_SHIFT, 0, >> + pc->rtce_window_size >> SPAPR_TCE_PAGE_SHIFT, >> + false); >> dev->tcet->vdev = dev; >> memory_region_add_subregion_overlap(&dev->mrroot, 0, >> spapr_tce_get_iommu(dev->tcet), 2); >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >> index 098d85d..3e6bb84 100644 >> --- a/include/hw/ppc/spapr.h >> +++ b/include/hw/ppc/spapr.h >> @@ -539,6 +539,7 @@ typedef struct sPAPRTCETable sPAPRTCETable; >> >> struct sPAPRTCETable { >> DeviceState parent; >> + bool enabled; >> uint32_t liobn; >> uint32_t nb_table; >> uint64_t bus_offset; >> @@ -566,11 +567,10 @@ void spapr_events_fdt_skel(void *fdt, uint32_t epow_irq); >> int spapr_h_cas_compose_response(sPAPRMachineState *sm, >> target_ulong addr, target_ulong size, >> bool cpu_update, bool memory_update); >> -sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, >> - uint64_t bus_offset, >> - uint32_t page_shift, >> - uint32_t nb_table, >> - bool need_vfio); >> +sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn); >> +void spapr_tce_table_enable(sPAPRTCETable *tcet, >> + uint32_t page_shift, uint64_t bus_offset, >> + uint32_t nb_table, bool vfio_accel); >> void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, bool need_vfio); >> >> MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet); > -- Alexey