1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH v3] spapr_pci: make index property mandatory

Greg Kurz posted 1 patch 6 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/150591878000.25597.7317602171707035692.stgit@bahia
Test checkpatch passed
Test docker passed
Test s390x passed
hw/ppc/spapr_pci.c |   62 +++++++---------------------------------------------
1 file changed, 8 insertions(+), 54 deletions(-)
[Qemu-devel] [PATCH v3] spapr_pci: make index property mandatory
Posted by Greg Kurz 6 years, 7 months ago 
PHBs can be created with an index property, in which case the machine
code automatically sets all the MMIO windows at addresses derived from
the index. Alternatively, they can be manually created without index,
but the user has to provide addresses for all MMIO windows.

The non-index way happens to be more trouble than it's worth: it's
difficult to use, keeps requiring (potentially incompatible) changes
when some new parameter needs adding, and is awkward to check for
collisions. It currently even has a bug that prevents to use two
non-index PHBs because their child DRCs are all derived from the
same index == -1 value, and, thus, collide.

This patch hence makes the index property mandatory. As a consequence,
the PHB's memory regions and BUID are now always configured according
to the index, and it is no longer possible to set them from the command
line.

This DOES BREAK backwards compat, but we don't think the non-index
PHB feature was used in practice (at least libvirt doesn't) and the
simplification is worth it.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
v2->v3: - re-write commit message
        - mem64_win_pciaddr no longer configurable
        - simplified check to map 64-bit window

v1->v2: - error out if mem64_win_pciaddr is set but mem64_win_size
          isn't
        - set mem64_win_addr to -1 for old configuration with 32-bit
          window below 2G in spapr_phb_realize()
        - drop instance init function

RFC->v1: - as suggested dy David, updated the changelog to explicitely
           mention that we intentionally break backwards compat.
---
---
 hw/ppc/spapr_pci.c |   62 +++++++---------------------------------------------
 1 file changed, 8 insertions(+), 54 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index cf54160526fa..6126c800440f 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1523,16 +1523,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
         sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
         Error *local_err = NULL;
 
-        if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn[0] != (uint32_t)-1)
-            || (sphb->dma_liobn[1] != (uint32_t)-1 && windows_supported == 2)
-            || (sphb->mem_win_addr != (hwaddr)-1)
-            || (sphb->mem64_win_addr != (hwaddr)-1)
-            || (sphb->io_win_addr != (hwaddr)-1)) {
-            error_setg(errp, "Either \"index\" or other parameters must"
-                       " be specified for PAPR PHB, not both");
-            return;
-        }
-
         smc->phb_placement(spapr, sphb->index,
                            &sphb->buid, &sphb->io_win_addr,
                            &sphb->mem_win_addr, &sphb->mem64_win_addr,
@@ -1541,46 +1531,20 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
             error_propagate(errp, local_err);
             return;
         }
-    }
-
-    if (sphb->buid == (uint64_t)-1) {
-        error_setg(errp, "BUID not specified for PHB");
-        return;
-    }
-
-    if ((sphb->dma_liobn[0] == (uint32_t)-1) ||
-        ((sphb->dma_liobn[1] == (uint32_t)-1) && (windows_supported > 1))) {
-        error_setg(errp, "LIOBN(s) not specified for PHB");
-        return;
-    }
-
-    if (sphb->mem_win_addr == (hwaddr)-1) {
-        error_setg(errp, "Memory window address not specified for PHB");
-        return;
-    }
-
-    if (sphb->io_win_addr == (hwaddr)-1) {
-        error_setg(errp, "IO window address not specified for PHB");
+    } else {
+        error_setg(errp, "\"index\" for PAPR PHB is mandatory");
         return;
     }
 
     if (sphb->mem64_win_size != 0) {
-        if (sphb->mem64_win_addr == (hwaddr)-1) {
-            error_setg(errp,
-                       "64-bit memory window address not specified for PHB");
-            return;
-        }
-
         if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {
             error_setg(errp, "32-bit memory window of size 0x%"HWADDR_PRIx
                        " (max 2 GiB)", sphb->mem_win_size);
             return;
         }
 
-        if (sphb->mem64_win_pciaddr == (hwaddr)-1) {
-            /* 64-bit window defaults to identity mapping */
-            sphb->mem64_win_pciaddr = sphb->mem64_win_addr;
-        }
+        /* 64-bit window defaults to identity mapping */
+        sphb->mem64_win_pciaddr = sphb->mem64_win_addr;
     } else if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {
         /*
          * For compatibility with old configuration, if no 64-bit MMIO
@@ -1622,18 +1586,16 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr,
                                 &sphb->mem32window);
 
-    if (sphb->mem64_win_pciaddr != (hwaddr)-1) {
+    if (sphb->mem64_win_size != 0) {
         namebuf = g_strdup_printf("%s.mmio64-alias", sphb->dtbusname);
         memory_region_init_alias(&sphb->mem64window, OBJECT(sphb),
                                  namebuf, &sphb->memspace,
                                  sphb->mem64_win_pciaddr, sphb->mem64_win_size);
         g_free(namebuf);
 
-        if (sphb->mem64_win_addr != (hwaddr)-1) {
-            memory_region_add_subregion(get_system_memory(),
-                                        sphb->mem64_win_addr,
-                                        &sphb->mem64window);
-        }
+        memory_region_add_subregion(get_system_memory(),
+                                    sphb->mem64_win_addr,
+                                    &sphb->mem64window);
     }
 
     /* Initialize IO regions */
@@ -1789,18 +1751,10 @@ 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[0], -1),
-    DEFINE_PROP_UINT32("liobn64", sPAPRPHBState, dma_liobn[1], -1),
-    DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
     DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size,
                        SPAPR_PCI_MEM32_WIN_SIZE),
-    DEFINE_PROP_UINT64("mem64_win_addr", sPAPRPHBState, mem64_win_addr, -1),
     DEFINE_PROP_UINT64("mem64_win_size", sPAPRPHBState, mem64_win_size,
                        SPAPR_PCI_MEM64_WIN_SIZE),
-    DEFINE_PROP_UINT64("mem64_win_pciaddr", sPAPRPHBState, mem64_win_pciaddr,
-                       -1),
-    DEFINE_PROP_UINT64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
     DEFINE_PROP_UINT64("io_win_size", sPAPRPHBState, io_win_size,
                        SPAPR_PCI_IO_WIN_SIZE),
     DEFINE_PROP_BOOL("dynamic-reconfiguration", sPAPRPHBState, dr_enabled,
Re: [Qemu-devel] [PATCH v3] spapr_pci: make index property mandatory
Posted by David Gibson 6 years, 7 months ago 
On Wed, Sep 20, 2017 at 04:46:20PM +0200, Greg Kurz wrote:
> PHBs can be created with an index property, in which case the machine
> code automatically sets all the MMIO windows at addresses derived from
> the index. Alternatively, they can be manually created without index,
> but the user has to provide addresses for all MMIO windows.
> 
> The non-index way happens to be more trouble than it's worth: it's
> difficult to use, keeps requiring (potentially incompatible) changes
> when some new parameter needs adding, and is awkward to check for
> collisions. It currently even has a bug that prevents to use two
> non-index PHBs because their child DRCs are all derived from the
> same index == -1 value, and, thus, collide.
> 
> This patch hence makes the index property mandatory. As a consequence,
> the PHB's memory regions and BUID are now always configured according
> to the index, and it is no longer possible to set them from the command
> line.
> 
> This DOES BREAK backwards compat, but we don't think the non-index
> PHB feature was used in practice (at least libvirt doesn't) and the
> simplification is worth it.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v2->v3: - re-write commit message
>         - mem64_win_pciaddr no longer configurable
>         - simplified check to map 64-bit window
> 
> v1->v2: - error out if mem64_win_pciaddr is set but mem64_win_size
>           isn't
>         - set mem64_win_addr to -1 for old configuration with 32-bit
>           window below 2G in spapr_phb_realize()
>         - drop instance init function
> 
> RFC->v1: - as suggested dy David, updated the changelog to explicitely
>            mention that we intentionally break backwards compat.

Applied to ppc-for-2.11, thanks.

> ---

> ---
>  hw/ppc/spapr_pci.c |   62 +++++++---------------------------------------------
>  1 file changed, 8 insertions(+), 54 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index cf54160526fa..6126c800440f 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1523,16 +1523,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>          sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>          Error *local_err = NULL;
>  
> -        if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn[0] != (uint32_t)-1)
> -            || (sphb->dma_liobn[1] != (uint32_t)-1 && windows_supported == 2)
> -            || (sphb->mem_win_addr != (hwaddr)-1)
> -            || (sphb->mem64_win_addr != (hwaddr)-1)
> -            || (sphb->io_win_addr != (hwaddr)-1)) {
> -            error_setg(errp, "Either \"index\" or other parameters must"
> -                       " be specified for PAPR PHB, not both");
> -            return;
> -        }
> -
>          smc->phb_placement(spapr, sphb->index,
>                             &sphb->buid, &sphb->io_win_addr,
>                             &sphb->mem_win_addr, &sphb->mem64_win_addr,
> @@ -1541,46 +1531,20 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>              error_propagate(errp, local_err);
>              return;
>          }
> -    }
> -
> -    if (sphb->buid == (uint64_t)-1) {
> -        error_setg(errp, "BUID not specified for PHB");
> -        return;
> -    }
> -
> -    if ((sphb->dma_liobn[0] == (uint32_t)-1) ||
> -        ((sphb->dma_liobn[1] == (uint32_t)-1) && (windows_supported > 1))) {
> -        error_setg(errp, "LIOBN(s) not specified for PHB");
> -        return;
> -    }
> -
> -    if (sphb->mem_win_addr == (hwaddr)-1) {
> -        error_setg(errp, "Memory window address not specified for PHB");
> -        return;
> -    }
> -
> -    if (sphb->io_win_addr == (hwaddr)-1) {
> -        error_setg(errp, "IO window address not specified for PHB");
> +    } else {
> +        error_setg(errp, "\"index\" for PAPR PHB is mandatory");
>          return;
>      }
>  
>      if (sphb->mem64_win_size != 0) {
> -        if (sphb->mem64_win_addr == (hwaddr)-1) {
> -            error_setg(errp,
> -                       "64-bit memory window address not specified for PHB");
> -            return;
> -        }
> -
>          if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {
>              error_setg(errp, "32-bit memory window of size 0x%"HWADDR_PRIx
>                         " (max 2 GiB)", sphb->mem_win_size);
>              return;
>          }
>  
> -        if (sphb->mem64_win_pciaddr == (hwaddr)-1) {
> -            /* 64-bit window defaults to identity mapping */
> -            sphb->mem64_win_pciaddr = sphb->mem64_win_addr;
> -        }
> +        /* 64-bit window defaults to identity mapping */
> +        sphb->mem64_win_pciaddr = sphb->mem64_win_addr;
>      } else if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {
>          /*
>           * For compatibility with old configuration, if no 64-bit MMIO
> @@ -1622,18 +1586,16 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr,
>                                  &sphb->mem32window);
>  
> -    if (sphb->mem64_win_pciaddr != (hwaddr)-1) {
> +    if (sphb->mem64_win_size != 0) {
>          namebuf = g_strdup_printf("%s.mmio64-alias", sphb->dtbusname);
>          memory_region_init_alias(&sphb->mem64window, OBJECT(sphb),
>                                   namebuf, &sphb->memspace,
>                                   sphb->mem64_win_pciaddr, sphb->mem64_win_size);
>          g_free(namebuf);
>  
> -        if (sphb->mem64_win_addr != (hwaddr)-1) {
> -            memory_region_add_subregion(get_system_memory(),
> -                                        sphb->mem64_win_addr,
> -                                        &sphb->mem64window);
> -        }
> +        memory_region_add_subregion(get_system_memory(),
> +                                    sphb->mem64_win_addr,
> +                                    &sphb->mem64window);
>      }
>  
>      /* Initialize IO regions */
> @@ -1789,18 +1751,10 @@ 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[0], -1),
> -    DEFINE_PROP_UINT32("liobn64", sPAPRPHBState, dma_liobn[1], -1),
> -    DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
>      DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size,
>                         SPAPR_PCI_MEM32_WIN_SIZE),
> -    DEFINE_PROP_UINT64("mem64_win_addr", sPAPRPHBState, mem64_win_addr, -1),
>      DEFINE_PROP_UINT64("mem64_win_size", sPAPRPHBState, mem64_win_size,
>                         SPAPR_PCI_MEM64_WIN_SIZE),
> -    DEFINE_PROP_UINT64("mem64_win_pciaddr", sPAPRPHBState, mem64_win_pciaddr,
> -                       -1),
> -    DEFINE_PROP_UINT64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
>      DEFINE_PROP_UINT64("io_win_size", sPAPRPHBState, io_win_size,
>                         SPAPR_PCI_IO_WIN_SIZE),
>      DEFINE_PROP_BOOL("dynamic-reconfiguration", sPAPRPHBState, dr_enabled,
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.