From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56581) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkqnF-0006CX-Oe for qemu-devel@nongnu.org; Wed, 22 Apr 2015 05:18:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YkqnC-0004lZ-Dl for qemu-devel@nongnu.org; Wed, 22 Apr 2015 05:18:05 -0400 Received: from mail-pa0-f44.google.com ([209.85.220.44]:35493) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkqnC-0004l9-1t for qemu-devel@nongnu.org; Wed, 22 Apr 2015 05:18:02 -0400 Received: by pabtp1 with SMTP id tp1so267665995pab.2 for ; Wed, 22 Apr 2015 02:18:01 -0700 (PDT) Message-ID: <55376742.3060703@ozlabs.ru> Date: Wed, 22 Apr 2015 19:17:54 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1428679484-15451-1-git-send-email-aik@ozlabs.ru> <1428679484-15451-12-git-send-email-aik@ozlabs.ru> <20150422063903.GP31815@voom.redhat.com> In-Reply-To: <20150422063903.GP31815@voom.redhat.com> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qemu v6 11/15] spapr_pci: Do complete reset of DMA config when resetting PHB List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: Alexander Graf , Michael Roth , qemu-devel@nongnu.org, Alex Williamson , qemu-ppc@nongnu.org, Gavin Shan On 04/22/2015 04:39 PM, David Gibson wrote: > On Sat, Apr 11, 2015 at 01:24:40AM +1000, Alexey Kardashevskiy wrote: >> On a system reset, DMA configuration has to reset too. At the moment >> it clears the table content. This is enough for the single table case >> but with DDW, we will also have to disable all DMA windows except >> the default one. Furthermore according to sPAPR, if the guest removed >> the default window and created a huge one at the same zero offset on >> a PCI bus, the reset handler has to recreate the default window with >> the default properties (2GB big, 4K pages). >> >> This reworks SPAPR PHB code to disable the existing DMA window on reset >> and then configure and enable the default window. >> Without DDW that means that the same window will be disabled and then >> enabled with no other change in behaviour. >> >> This changes the table creation to do it in one place in PHB (VFIO PHB >> just inherits the behaviour from PHB). The actual table allocation is >> done from the reset handler and this is where finish_realize() is called. > > Looks like your commit message needs an update - you already got rid > of finish_realize() at this point in the series. > >> This disables all DMA windows on a PHB reset. It does not make any >> difference now as there is just one DMA window but it will later with DDW >> patches. >> >> spapr_phb_dma_reset() and spapr_phb_dma_remove_window() will be used >> in following patch so make it public. >> >> Signed-off-by: Alexey Kardashevskiy >> --- >> hw/ppc/spapr_pci.c | 45 ++++++++++++++++++++++++++++++++++++--------- >> hw/ppc/spapr_pci_vfio.c | 6 ------ >> include/hw/pci-host/spapr.h | 3 +++ >> 3 files changed, 39 insertions(+), 15 deletions(-) >> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index 664687c..3d40f5b 100644 >> --- a/hw/ppc/spapr_pci.c >> +++ b/hw/ppc/spapr_pci.c >> @@ -736,7 +736,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> SysBusDevice *s = SYS_BUS_DEVICE(dev); >> sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s); >> PCIHostState *phb = PCI_HOST_BRIDGE(s); >> - sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(s); >> char *namebuf; >> int i; >> PCIBus *bus; >> @@ -887,14 +886,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> return; >> } >> >> - 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, "failed to create TCE table"); >> - return; >> - } >> memory_region_add_subregion(&sphb->iommu_root, 0, >> spapr_tce_get_iommu(tcet)); >> >> @@ -923,6 +914,40 @@ static int spapr_phb_dma_init_window(sPAPRPHBState *sphb, >> return 0; >> } >> >> +int spapr_phb_dma_remove_window(sPAPRPHBState *sphb, >> + sPAPRTCETable *tcet) > > The only caller of this is just below, why is it not static? rtas_ibm_remove_pe_dma_window() will call it in "[PATCH qemu v6 14/15] spapr_pci/spapr_pci_vfio: Support Dynamic DMA Windows (DDW)". I do not see much sense in making it static first and then making it public in later patch, should I fix this anyway (do not care that much) or updated commit log will be enough? > >> +{ >> + spapr_tce_table_disable(tcet); >> + >> + return 0; >> +} >> + >> +static int spapr_phb_disable_dma_windows(Object *child, void *opaque) >> +{ >> + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(opaque); >> + sPAPRTCETable *tcet = (sPAPRTCETable *) >> + object_dynamic_cast(child, TYPE_SPAPR_TCE_TABLE); >> + >> + if (tcet) { >> + spapr_phb_dma_remove_window(sphb, tcet); >> + } >> + >> + return 0; >> +} >> + >> +int spapr_phb_dma_reset(sPAPRPHBState *sphb) >> +{ >> + const uint32_t liobn = SPAPR_PCI_LIOBN(sphb->index, 0); >> + sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >> + >> + spc->dma_capabilities_update(sphb); /* Refresh @has_vfio status */ >> + object_child_foreach(OBJECT(sphb), spapr_phb_disable_dma_windows, sphb); >> + spc->dma_init_window(sphb, liobn, SPAPR_TCE_PAGE_SHIFT, >> + sphb->dma32_window_size); >> + >> + return 0; >> +} >> + >> static int spapr_phb_children_reset(Object *child, void *opaque) >> { >> DeviceState *dev = (DeviceState *) object_dynamic_cast(child, TYPE_DEVICE); >> @@ -936,6 +961,8 @@ static int spapr_phb_children_reset(Object *child, void *opaque) >> >> static void spapr_phb_reset(DeviceState *qdev) >> { >> + spapr_phb_dma_reset(SPAPR_PCI_HOST_BRIDGE(qdev)); >> + >> /* Reset the IOMMU state */ >> object_child_foreach(OBJECT(qdev), spapr_phb_children_reset, NULL); >> } >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c >> index a5b97d0..f89e053 100644 >> --- a/hw/ppc/spapr_pci_vfio.c >> +++ b/hw/ppc/spapr_pci_vfio.c >> @@ -58,11 +58,6 @@ static int spapr_phb_vfio_dma_init_window(sPAPRPHBState *sphb, >> return 0; >> } >> >> -static void spapr_phb_vfio_reset(DeviceState *qdev) >> -{ >> - /* Do nothing */ >> -} >> - >> static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb, >> unsigned int addr, int option) >> { >> @@ -176,7 +171,6 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data) >> sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_CLASS(klass); >> >> dc->props = spapr_phb_vfio_properties; >> - dc->reset = spapr_phb_vfio_reset; >> spc->dma_capabilities_update = spapr_phb_vfio_dma_capabilities_update; >> spc->dma_init_window = spapr_phb_vfio_dma_init_window; >> spc->eeh_set_option = spapr_phb_vfio_eeh_set_option; >> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h >> index 3074145..7fda78e 100644 >> --- a/include/hw/pci-host/spapr.h >> +++ b/include/hw/pci-host/spapr.h >> @@ -140,5 +140,8 @@ void spapr_pci_rtas_init(void); >> sPAPRPHBState *spapr_pci_find_phb(sPAPREnvironment *spapr, uint64_t buid); >> PCIDevice *spapr_pci_find_dev(sPAPREnvironment *spapr, uint64_t buid, >> uint32_t config_addr); >> +int spapr_phb_dma_remove_window(sPAPRPHBState *sphb, >> + sPAPRTCETable *tcet); >> +int spapr_phb_dma_reset(sPAPRPHBState *sphb); >> >> #endif /* __HW_SPAPR_PCI_H__ */ > -- Alexey