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

[David Gibson]

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

signature.asc
Description: PGP signature