[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH qemu v15 15/17] spapr_pci: Get rid of dma_loibn

From: Alexey Kardashevskiy
Subject: Re: [Qemu-devel] [PATCH qemu v15 15/17] spapr_pci: Get rid of dma_loibn
Date: Thu, 7 Apr 2016 17:10:53 +1000
User-agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.1

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 <address@hidden>

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.

* 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 
      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 

          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 

-    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");
@@ -1453,7 +1447,7 @@ static void spapr_phb_realize(DeviceState *dev, Error 

-    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",
@@ -1479,7 +1473,8 @@ static int spapr_phb_children_reset(Object *child, void 

  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) {
@@ -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,
@@ -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;


reply via email to

[Prev in Thread] Current Thread [Next in Thread]