| Message ID | 1459762426-18440-16-git-send-email-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
s/dma_loibn/dma_liobn/ in subject line. On Mon, Apr 04, 2016 at 07:33:44PM +1000, Alexey Kardashevskiy wrote: > We are going to have 2 DMA windows which LIOBNs are calculated from > the PHB index and the window number using the SPAPR_PCI_LIOBN macro > so there is no actual use for dma_liobn. > > This replaces dma_liobn with SPAPR_PCI_LIOBN. This marks it as unused > in the migration stream. This renames dma_liobn to _dma_liobn as we have > to keep the property for the CLI compatibility and we need a storage > for it, although it has never really been used. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> This doesn't quite make sense. We can't really do that without entirely removing support for PHBs without an 'index' value. Basically the idea of the PHB config parameters what that you either specified just "index" or you specified *all* the relevant addresses. Removing option 2 might be a reasonable idea, but it shouldn't just be done as a side effect of this other change. With this patch the "specify everything" approach still has code, but can't work, because such a device will never get a reasonable liobn (or worse, it might get a duplicate liobn, because the index isn't verified in this mode). Then again.. the "index" approach has also bitten us with the problem of the not-quite-big-enough MMIO space per-PHB, so I'm not entirely sure that making it the only choice is the right way to go either. The short term approach to handle DDW might be to instead add a dma64_liobn property. > --- > Changes: > v15: > * new to the series > --- > hw/ppc/spapr_pci.c | 17 ++++++----------- > include/hw/pci-host/spapr.h | 2 +- > 2 files changed, 7 insertions(+), 12 deletions(-) > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index f864fde..d4bdb27 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1306,7 +1306,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > if (sphb->index != (uint32_t)-1) { > hwaddr windows_base; > > - if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn != (uint32_t)-1) > + if ((sphb->buid != (uint64_t)-1) > || (sphb->mem_win_addr != (hwaddr)-1) > || (sphb->io_win_addr != (hwaddr)-1)) { > error_setg(errp, "Either \"index\" or other parameters must" > @@ -1321,7 +1321,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > } > > sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index; > - sphb->dma_liobn = SPAPR_PCI_LIOBN(sphb->index, 0); > > windows_base = SPAPR_PCI_WINDOW_BASE > + sphb->index * SPAPR_PCI_WINDOW_SPACING; > @@ -1334,11 +1333,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > return; > } > > - if (sphb->dma_liobn == (uint32_t)-1) { > - error_setg(errp, "LIOBN not specified for PHB"); > - return; > - } > - > if (sphb->mem_win_addr == (hwaddr)-1) { > error_setg(errp, "Memory window address not specified for PHB"); > return; > @@ -1453,7 +1447,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > } > } > > - tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn); > + tcet = spapr_tce_new_table(DEVICE(sphb), SPAPR_PCI_LIOBN(sphb->index, 0)); > if (!tcet) { > error_setg(errp, "Unable to create TCE table for %s", > sphb->dtbusname); > @@ -1479,7 +1473,8 @@ static int spapr_phb_children_reset(Object *child, void *opaque) > > void spapr_phb_dma_reset(sPAPRPHBState *sphb) > { > - sPAPRTCETable *tcet = spapr_tce_find_by_liobn(sphb->dma_liobn); > + uint32_t liobn = SPAPR_PCI_LIOBN(sphb->index, 0); > + sPAPRTCETable *tcet = spapr_tce_find_by_liobn(liobn); > > if (tcet && tcet->enabled) { > spapr_tce_table_disable(tcet); > @@ -1507,7 +1502,7 @@ static void spapr_phb_reset(DeviceState *qdev) > static Property spapr_phb_properties[] = { > DEFINE_PROP_UINT32("index", sPAPRPHBState, index, -1), > DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1), > - DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn, -1), > + DEFINE_PROP_UINT32("liobn", sPAPRPHBState, _dma_liobn, -1), > DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1), > DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size, > SPAPR_PCI_MMIO_WIN_SIZE), > @@ -1595,7 +1590,7 @@ static const VMStateDescription vmstate_spapr_pci = { > .post_load = spapr_pci_post_load, > .fields = (VMStateField[]) { > VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState), > - VMSTATE_UINT32_EQUAL(dma_liobn, sPAPRPHBState), > + VMSTATE_UNUSED(4), /* former dma_liobn */ > VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState), > VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState), > VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState), > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h > index 7848366..3fca1c3 100644 > --- a/include/hw/pci-host/spapr.h > +++ b/include/hw/pci-host/spapr.h > @@ -56,7 +56,7 @@ struct sPAPRPHBState { > hwaddr mem_win_addr, mem_win_size, io_win_addr, io_win_size; > MemoryRegion memwindow, iowindow, msiwindow; > > - uint32_t dma_liobn; > + uint32_t _dma_liobn; > hwaddr dma_win_addr, dma_win_size; > AddressSpace iommu_as; > MemoryRegion iommu_root;
On 04/07/2016 10:50 AM, David Gibson wrote: > s/dma_loibn/dma_liobn/ in subject line. > > On Mon, Apr 04, 2016 at 07:33:44PM +1000, Alexey Kardashevskiy wrote: >> We are going to have 2 DMA windows which LIOBNs are calculated from >> the PHB index and the window number using the SPAPR_PCI_LIOBN macro >> so there is no actual use for dma_liobn. >> >> This replaces dma_liobn with SPAPR_PCI_LIOBN. This marks it as unused >> in the migration stream. This renames dma_liobn to _dma_liobn as we have >> to keep the property for the CLI compatibility and we need a storage >> for it, although it has never really been used. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > This doesn't quite make sense. We can't really do that without > entirely removing support for PHBs without an 'index' value. > Basically the idea of the PHB config parameters what that you either > specified just "index" or you specified *all* the relevant addresses. > Removing option 2 might be a reasonable idea, but it shouldn't just be > done as a side effect of this other change. With this patch the > "specify everything" approach still has code, but can't work, because > such a device will never get a reasonable liobn (or worse, it might > get a duplicate liobn, because the index isn't verified in this mode). > > Then again.. the "index" approach has also bitten us with the problem > of the not-quite-big-enough MMIO space per-PHB, Any new ideas on that front? I also keep in mind that we rather want to assign interrupt numbers pool to a PHB based on its index rather than allocate them from the global machine interrupt number space so this is one more vote for keeping the index. > so I'm not entirely > sure that making it the only choice is the right way to go either. I can imagine the user wanting to change MMIO addresses (so they should remain properties) but changing LIOBN from the command line does not seem useful at all. > The short term approach to handle DDW might be to instead add a > dma64_liobn property. So everywhere where I want to have a loop through all TCE tables and use SPAPR_PCI_LIOBN(index, i), I cannot really do that if I have 2 separate properties. Not extremely convenient. > >> --- >> Changes: >> v15: >> * new to the series >> --- >> hw/ppc/spapr_pci.c | 17 ++++++----------- >> include/hw/pci-host/spapr.h | 2 +- >> 2 files changed, 7 insertions(+), 12 deletions(-) >> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index f864fde..d4bdb27 100644 >> --- a/hw/ppc/spapr_pci.c >> +++ b/hw/ppc/spapr_pci.c >> @@ -1306,7 +1306,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> if (sphb->index != (uint32_t)-1) { >> hwaddr windows_base; >> >> - if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn != (uint32_t)-1) >> + if ((sphb->buid != (uint64_t)-1) >> || (sphb->mem_win_addr != (hwaddr)-1) >> || (sphb->io_win_addr != (hwaddr)-1)) { >> error_setg(errp, "Either \"index\" or other parameters must" >> @@ -1321,7 +1321,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> } >> >> sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index; >> - sphb->dma_liobn = SPAPR_PCI_LIOBN(sphb->index, 0); >> >> windows_base = SPAPR_PCI_WINDOW_BASE >> + sphb->index * SPAPR_PCI_WINDOW_SPACING; >> @@ -1334,11 +1333,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> return; >> } >> >> - if (sphb->dma_liobn == (uint32_t)-1) { >> - error_setg(errp, "LIOBN not specified for PHB"); >> - return; >> - } >> - >> if (sphb->mem_win_addr == (hwaddr)-1) { >> error_setg(errp, "Memory window address not specified for PHB"); >> return; >> @@ -1453,7 +1447,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> } >> } >> >> - tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn); >> + tcet = spapr_tce_new_table(DEVICE(sphb), SPAPR_PCI_LIOBN(sphb->index, 0)); >> if (!tcet) { >> error_setg(errp, "Unable to create TCE table for %s", >> sphb->dtbusname); >> @@ -1479,7 +1473,8 @@ static int spapr_phb_children_reset(Object *child, void *opaque) >> >> void spapr_phb_dma_reset(sPAPRPHBState *sphb) >> { >> - sPAPRTCETable *tcet = spapr_tce_find_by_liobn(sphb->dma_liobn); >> + uint32_t liobn = SPAPR_PCI_LIOBN(sphb->index, 0); >> + sPAPRTCETable *tcet = spapr_tce_find_by_liobn(liobn); >> >> if (tcet && tcet->enabled) { >> spapr_tce_table_disable(tcet); >> @@ -1507,7 +1502,7 @@ static void spapr_phb_reset(DeviceState *qdev) >> static Property spapr_phb_properties[] = { >> DEFINE_PROP_UINT32("index", sPAPRPHBState, index, -1), >> DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1), >> - DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn, -1), >> + DEFINE_PROP_UINT32("liobn", sPAPRPHBState, _dma_liobn, -1), >> DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1), >> DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size, >> SPAPR_PCI_MMIO_WIN_SIZE), >> @@ -1595,7 +1590,7 @@ static const VMStateDescription vmstate_spapr_pci = { >> .post_load = spapr_pci_post_load, >> .fields = (VMStateField[]) { >> VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState), >> - VMSTATE_UINT32_EQUAL(dma_liobn, sPAPRPHBState), >> + VMSTATE_UNUSED(4), /* former dma_liobn */ >> VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState), >> VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState), >> VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState), >> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h >> index 7848366..3fca1c3 100644 >> --- a/include/hw/pci-host/spapr.h >> +++ b/include/hw/pci-host/spapr.h >> @@ -56,7 +56,7 @@ struct sPAPRPHBState { >> hwaddr mem_win_addr, mem_win_size, io_win_addr, io_win_size; >> MemoryRegion memwindow, iowindow, msiwindow; >> >> - uint32_t dma_liobn; >> + uint32_t _dma_liobn; >> hwaddr dma_win_addr, dma_win_size; >> AddressSpace iommu_as; >> MemoryRegion iommu_root; >
On Thu, Apr 07, 2016 at 05:10:53PM +1000, Alexey Kardashevskiy wrote: > On 04/07/2016 10:50 AM, David Gibson wrote: > >s/dma_loibn/dma_liobn/ in subject line. > > > >On Mon, Apr 04, 2016 at 07:33:44PM +1000, Alexey Kardashevskiy wrote: > >>We are going to have 2 DMA windows which LIOBNs are calculated from > >>the PHB index and the window number using the SPAPR_PCI_LIOBN macro > >>so there is no actual use for dma_liobn. > >> > >>This replaces dma_liobn with SPAPR_PCI_LIOBN. This marks it as unused > >>in the migration stream. This renames dma_liobn to _dma_liobn as we have > >>to keep the property for the CLI compatibility and we need a storage > >>for it, although it has never really been used. > >> > >>Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > > >This doesn't quite make sense. We can't really do that without > >entirely removing support for PHBs without an 'index' value. > >Basically the idea of the PHB config parameters what that you either > >specified just "index" or you specified *all* the relevant addresses. > >Removing option 2 might be a reasonable idea, but it shouldn't just be > >done as a side effect of this other change. With this patch the > >"specify everything" approach still has code, but can't work, because > >such a device will never get a reasonable liobn (or worse, it might > >get a duplicate liobn, because the index isn't verified in this mode). > > > >Then again.. the "index" approach has also bitten us with the problem > >of the not-quite-big-enough MMIO space per-PHB, > > Any new ideas on that front? No, not really :(. > I also keep in mind that we rather want to > assign interrupt numbers pool to a PHB based on its index rather than > allocate them from the global machine interrupt number space so this is one > more vote for keeping the index. True, if we removed the index option we'd have to have the irqs set from a property as well, I think, which would just increase the pain. > >so I'm not entirely > >sure that making it the only choice is the right way to go either. > > I can imagine the user wanting to change MMIO addresses (so they should > remain properties) but changing LIOBN from the command line does not seem > useful at all. Sure, but that doesn't alter the fact that we shouldn't break the no-index option without completely removing the no-index option. > >The short term approach to handle DDW might be to instead add a > >dma64_liobn property. > > So everywhere where I want to have a loop through all TCE tables and use > SPAPR_PCI_LIOBN(index, i), I cannot really do that if I have 2 separate > properties. Not extremely convenient. Hmm.. you could work around that. Inside the structure you could have a liobns[2] array which you step through, then the properties just set the individual elements of the array.
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index f864fde..d4bdb27 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1306,7 +1306,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) if (sphb->index != (uint32_t)-1) { hwaddr windows_base; - if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn != (uint32_t)-1) + if ((sphb->buid != (uint64_t)-1) || (sphb->mem_win_addr != (hwaddr)-1) || (sphb->io_win_addr != (hwaddr)-1)) { error_setg(errp, "Either \"index\" or other parameters must" @@ -1321,7 +1321,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) } sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index; - sphb->dma_liobn = SPAPR_PCI_LIOBN(sphb->index, 0); windows_base = SPAPR_PCI_WINDOW_BASE + sphb->index * SPAPR_PCI_WINDOW_SPACING; @@ -1334,11 +1333,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) return; } - if (sphb->dma_liobn == (uint32_t)-1) { - error_setg(errp, "LIOBN not specified for PHB"); - return; - } - if (sphb->mem_win_addr == (hwaddr)-1) { error_setg(errp, "Memory window address not specified for PHB"); return; @@ -1453,7 +1447,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) } } - tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn); + tcet = spapr_tce_new_table(DEVICE(sphb), SPAPR_PCI_LIOBN(sphb->index, 0)); if (!tcet) { error_setg(errp, "Unable to create TCE table for %s", sphb->dtbusname); @@ -1479,7 +1473,8 @@ static int spapr_phb_children_reset(Object *child, void *opaque) void spapr_phb_dma_reset(sPAPRPHBState *sphb) { - sPAPRTCETable *tcet = spapr_tce_find_by_liobn(sphb->dma_liobn); + uint32_t liobn = SPAPR_PCI_LIOBN(sphb->index, 0); + sPAPRTCETable *tcet = spapr_tce_find_by_liobn(liobn); if (tcet && tcet->enabled) { spapr_tce_table_disable(tcet); @@ -1507,7 +1502,7 @@ static void spapr_phb_reset(DeviceState *qdev) static Property spapr_phb_properties[] = { DEFINE_PROP_UINT32("index", sPAPRPHBState, index, -1), DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1), - DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn, -1), + DEFINE_PROP_UINT32("liobn", sPAPRPHBState, _dma_liobn, -1), DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1), DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size, SPAPR_PCI_MMIO_WIN_SIZE), @@ -1595,7 +1590,7 @@ static const VMStateDescription vmstate_spapr_pci = { .post_load = spapr_pci_post_load, .fields = (VMStateField[]) { VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState), - VMSTATE_UINT32_EQUAL(dma_liobn, sPAPRPHBState), + VMSTATE_UNUSED(4), /* former dma_liobn */ VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState), VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState), VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState), diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h index 7848366..3fca1c3 100644 --- a/include/hw/pci-host/spapr.h +++ b/include/hw/pci-host/spapr.h @@ -56,7 +56,7 @@ struct sPAPRPHBState { hwaddr mem_win_addr, mem_win_size, io_win_addr, io_win_size; MemoryRegion memwindow, iowindow, msiwindow; - uint32_t dma_liobn; + uint32_t _dma_liobn; hwaddr dma_win_addr, dma_win_size; AddressSpace iommu_as; MemoryRegion iommu_root;
We are going to have 2 DMA windows which LIOBNs are calculated from the PHB index and the window number using the SPAPR_PCI_LIOBN macro so there is no actual use for dma_liobn. This replaces dma_liobn with SPAPR_PCI_LIOBN. This marks it as unused in the migration stream. This renames dma_liobn to _dma_liobn as we have to keep the property for the CLI compatibility and we need a storage for it, although it has never really been used. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- Changes: v15: * new to the series --- hw/ppc/spapr_pci.c | 17 ++++++----------- include/hw/pci-host/spapr.h | 2 +- 2 files changed, 7 insertions(+), 12 deletions(-)