| Message ID | 1428679484-15451-12-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
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 <aik@ozlabs.ru> > --- > 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? > +{ > + 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__ */
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 <aik@ozlabs.ru> >> --- >> 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__ */ >
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) +{ + 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__ */
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. 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 <aik@ozlabs.ru> --- 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(-)