[v2,2/2] spapr_pci: make index property mandatory

Creating several PHBs without index property confuses the DRC code
and causes issues:
- only the first index-less PHB is functional, the other ones will
  silently ignore hotplugging of PCI devices
- QEMU will even terminate if these PHBs have cold-plugged devices

qemu-system-ppc64: -device virtio-net,bus=pci2.0: an attached device
 is still awaiting release

This happens because DR connectors for child PCI devices are created
with a DRC index that is derived from the PHB's index property. If the
PHBs are created without index, then the same value of -1 is used to
compute the DRC indexes for both PHBs, hence causing the collision.

Also, the index property is used to compute the placement of the PHB's
memory regions. It is limited to 31 or 255, depending on the machine
type version. This fits well with the requirements of DRC indexes, which
need the PHB index to be a 16-bit value.

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. We have to introduce a PHB instance init function to initialize
the 64-bit window address to -1 because pseries-2.7 and older machines
don't set it.

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>
---
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 |   53 +++++++++++-----------------------------------------
 1 file changed, 11 insertions(+), 42 deletions(-)

On Thu, Sep 14, 2017 at 04:14:44PM +0200, Greg Kurz wrote:
> Creating several PHBs without index property confuses the DRC code
> and causes issues:
> - only the first index-less PHB is functional, the other ones will
>   silently ignore hotplugging of PCI devices
> - QEMU will even terminate if these PHBs have cold-plugged devices

So, this is true, but it's kind of missing the point; we could fix
those things if we wanted.  The real point here is that the non-index
option is 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.

> qemu-system-ppc64: -device virtio-net,bus=pci2.0: an attached device
>  is still awaiting release
> 
> This happens because DR connectors for child PCI devices are created
> with a DRC index that is derived from the PHB's index property. If the
> PHBs are created without index, then the same value of -1 is used to
> compute the DRC indexes for both PHBs, hence causing the collision.
> 
> Also, the index property is used to compute the placement of the PHB's
> memory regions. It is limited to 31 or 255, depending on the machine
> type version. This fits well with the requirements of DRC indexes, which
> need the PHB index to be a 16-bit value.
> 
> 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. We have to introduce a PHB instance init function to initialize
> the 64-bit window address to -1 because pseries-2.7 and older machines
> don't set it.
> 
> 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>
> ---
> 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 |   53 +++++++++++-----------------------------------------
>  1 file changed, 11 insertions(+), 42 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index cf54160526fa..024638e18b53 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,36 +1531,12 @@ 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);

Some of these checks could probably just become assert()s now, where
they're things determined by the placement code and no longer have
parameters, a problem indicates a bug in the machine rather than a
user error.

> @@ -1581,6 +1547,9 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>              /* 64-bit window defaults to identity mapping */
>              sphb->mem64_win_pciaddr = sphb->mem64_win_addr;
>          }
> +    } else if (sphb->mem64_win_pciaddr != (hwaddr) -1) {
> +        error_setg(errp, "64-bit memory window requires \"mem64_win_size\"");
> +        return;

Likewise this new one.

>      } else if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {
>          /*
>           * For compatibility with old configuration, if no 64-bit MMIO
> @@ -1594,6 +1563,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>          sphb->mem64_win_pciaddr =
>              SPAPR_PCI_MEM_WIN_BUS_OFFSET + SPAPR_PCI_MEM32_WIN_SIZE;
>          sphb->mem_win_size = SPAPR_PCI_MEM32_WIN_SIZE;
> +    } else {
> +        /* Old configuration with the 32-bit MMIO window <= 2GiB don't need a
> +         * 64-bit MMIO window.
> +         */

Wouldn't it be easier to just change the test below to check for size
!= 0 instead of checking for addr != -1 as it does now.

> +        sphb->mem64_win_addr = (hwaddr) -1;
> +        sphb->mem64_win_pciaddr = (hwaddr) -1;
>      }
>  
>      if (spapr_pci_find_phb(spapr, sphb->buid)) {
> @@ -1789,18 +1764,12 @@ 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,
>

On Tue, 19 Sep 2017 09:03:58 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Sep 14, 2017 at 04:14:44PM +0200, Greg Kurz wrote:
> > Creating several PHBs without index property confuses the DRC code
> > and causes issues:
> > - only the first index-less PHB is functional, the other ones will
> >   silently ignore hotplugging of PCI devices
> > - QEMU will even terminate if these PHBs have cold-plugged devices  
> 
> So, this is true, but it's kind of missing the point; we could fix
> those things if we wanted.  The real point here is that the non-index
> option is 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.
> 

Ok, I'll rewrite the changelog accordingly in the next spin.

> > qemu-system-ppc64: -device virtio-net,bus=pci2.0: an attached device
> >  is still awaiting release
> > 
> > This happens because DR connectors for child PCI devices are created
> > with a DRC index that is derived from the PHB's index property. If the
> > PHBs are created without index, then the same value of -1 is used to
> > compute the DRC indexes for both PHBs, hence causing the collision.
> > 
> > Also, the index property is used to compute the placement of the PHB's
> > memory regions. It is limited to 31 or 255, depending on the machine
> > type version. This fits well with the requirements of DRC indexes, which
> > need the PHB index to be a 16-bit value.
> > 
> > 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. We have to introduce a PHB instance init function to initialize
> > the 64-bit window address to -1 because pseries-2.7 and older machines
> > don't set it.
> > 
> > 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>
> > ---
> > 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 |   53 +++++++++++-----------------------------------------
> >  1 file changed, 11 insertions(+), 42 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index cf54160526fa..024638e18b53 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,36 +1531,12 @@ 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);  
> 
> Some of these checks could probably just become assert()s now, where
> they're things determined by the placement code and no longer have
> parameters, a problem indicates a bug in the machine rather than a
> user error.
> 

All the *_size properties are still user settable, as before... should we
make them internal and have the placement hook initialize them instead ?

> > @@ -1581,6 +1547,9 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >              /* 64-bit window defaults to identity mapping */
> >              sphb->mem64_win_pciaddr = sphb->mem64_win_addr;
> >          }
> > +    } else if (sphb->mem64_win_pciaddr != (hwaddr) -1) {
> > +        error_setg(errp, "64-bit memory window requires \"mem64_win_size\"");
> > +        return;  
> 
> Likewise this new one.
> 

mem64_win_pciaddr is still user settable as well...

> >      } else if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {
> >          /*
> >           * For compatibility with old configuration, if no 64-bit MMIO
> > @@ -1594,6 +1563,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >          sphb->mem64_win_pciaddr =
> >              SPAPR_PCI_MEM_WIN_BUS_OFFSET + SPAPR_PCI_MEM32_WIN_SIZE;
> >          sphb->mem_win_size = SPAPR_PCI_MEM32_WIN_SIZE;
> > +    } else {
> > +        /* Old configuration with the 32-bit MMIO window <= 2GiB don't need a
> > +         * 64-bit MMIO window.
> > +         */  
> 
> Wouldn't it be easier to just change the test below to check for size
> != 0 instead of checking for addr != -1 as it does now.
> 

Yeah indeed.

> > +        sphb->mem64_win_addr = (hwaddr) -1;
> > +        sphb->mem64_win_pciaddr = (hwaddr) -1;
> >      }
> >  
> >      if (spapr_pci_find_phb(spapr, sphb->buid)) {
> > @@ -1789,18 +1764,12 @@ 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,
> >   
>

On Tue, Sep 19, 2017 at 04:03:57PM +0200, Greg Kurz wrote:
> On Tue, 19 Sep 2017 09:03:58 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Sep 14, 2017 at 04:14:44PM +0200, Greg Kurz wrote:
> > > Creating several PHBs without index property confuses the DRC code
> > > and causes issues:
> > > - only the first index-less PHB is functional, the other ones will
> > >   silently ignore hotplugging of PCI devices
> > > - QEMU will even terminate if these PHBs have cold-plugged devices  
> > 
> > So, this is true, but it's kind of missing the point; we could fix
> > those things if we wanted.  The real point here is that the non-index
> > option is 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.
> > 
> 
> Ok, I'll rewrite the changelog accordingly in the next spin.
> 
> > > qemu-system-ppc64: -device virtio-net,bus=pci2.0: an attached device
> > >  is still awaiting release
> > > 
> > > This happens because DR connectors for child PCI devices are created
> > > with a DRC index that is derived from the PHB's index property. If the
> > > PHBs are created without index, then the same value of -1 is used to
> > > compute the DRC indexes for both PHBs, hence causing the collision.
> > > 
> > > Also, the index property is used to compute the placement of the PHB's
> > > memory regions. It is limited to 31 or 255, depending on the machine
> > > type version. This fits well with the requirements of DRC indexes, which
> > > need the PHB index to be a 16-bit value.
> > > 
> > > 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. We have to introduce a PHB instance init function to initialize
> > > the 64-bit window address to -1 because pseries-2.7 and older machines
> > > don't set it.
> > > 
> > > 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>
> > > ---
> > > 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 |   53 +++++++++++-----------------------------------------
> > >  1 file changed, 11 insertions(+), 42 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > > index cf54160526fa..024638e18b53 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,36 +1531,12 @@ 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);  
> > 
> > Some of these checks could probably just become assert()s now, where
> > they're things determined by the placement code and no longer have
> > parameters, a problem indicates a bug in the machine rather than a
> > user error.
> > 
> 
> All the *_size properties are still user settable, as before... should we
> make them internal and have the placement hook initialize them instead ?

Ah, good point.  I guess that still makes sense.

> > > @@ -1581,6 +1547,9 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> > >              /* 64-bit window defaults to identity mapping */
> > >              sphb->mem64_win_pciaddr = sphb->mem64_win_addr;
> > >          }
> > > +    } else if (sphb->mem64_win_pciaddr != (hwaddr) -1) {
> > > +        error_setg(errp, "64-bit memory window requires \"mem64_win_size\"");
> > > +        return;  
> > 
> > Likewise this new one.
> > 
> 
> mem64_win_pciaddr is still user settable as well...

That doesn't sound like a good idea.  If we're making index mandatory,
we might has well have it set *all* the windows.

> 
> > >      } else if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {
> > >          /*
> > >           * For compatibility with old configuration, if no 64-bit MMIO
> > > @@ -1594,6 +1563,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> > >          sphb->mem64_win_pciaddr =
> > >              SPAPR_PCI_MEM_WIN_BUS_OFFSET + SPAPR_PCI_MEM32_WIN_SIZE;
> > >          sphb->mem_win_size = SPAPR_PCI_MEM32_WIN_SIZE;
> > > +    } else {
> > > +        /* Old configuration with the 32-bit MMIO window <= 2GiB don't need a
> > > +         * 64-bit MMIO window.
> > > +         */  
> > 
> > Wouldn't it be easier to just change the test below to check for size
> > != 0 instead of checking for addr != -1 as it does now.
> > 
> 
> Yeah indeed.
> 
> > > +        sphb->mem64_win_addr = (hwaddr) -1;
> > > +        sphb->mem64_win_pciaddr = (hwaddr) -1;
> > >      }
> > >  
> > >      if (spapr_pci_find_phb(spapr, sphb->buid)) {
> > > @@ -1789,18 +1764,12 @@ 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,
> > >   
> > 
> 
> 
>