[Qemu-devel] [PATCH v2 0/2] spapr_pci: make index property mandatory

[Qemu-devel] [PATCH v2 0/2] spapr_pci: make index property mandatory

[Greg Kurz]

Patch 1 is a proposal to silence patchew when it parses patch 2 :)

--
Greg

---

Greg Kurz (2):
      checkpatch: add hwaddr to @typeList
      spapr_pci: make index property mandatory


 hw/ppc/spapr_pci.c    |   53 ++++++++++---------------------------------------
 scripts/checkpatch.pl |    1 +
 2 files changed, 12 insertions(+), 42 deletions(-)

[Qemu-devel] [PATCH v2 1/2] checkpatch: add hwaddr to @typeList

[Greg Kurz]

The script doesn't know about all possible types and learn them as
it parses the code. If it reaches a line with a type cast but the
type isn't known yet, it is misinterpreted as an identifier.

For example the following line:

    foo = (hwaddr) -1;

results in the following false-positive to be reported:

ERROR: spaces required around that '-' (ctx:VxV)

Let's add this standard QEMU type to the list of pre-known types.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 scripts/checkpatch.pl |    1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fa478074b88d..def5bc1cc0e1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -213,6 +213,7 @@ our @typeList = (
 	qr{${Ident}_handler},
 	qr{${Ident}_handler_fn},
 	qr{target_(?:u)?long},
+	qr{hwaddr},
 );
 
 # This can be modified by sub possible.  Since it can be empty, be careful

[Qemu-devel] [PATCH v2 2/2] spapr_pci: make index property mandatory

[Greg Kurz]

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(-)

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);
@@ -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;
     } 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.
+         */
+        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,

Re: [Qemu-devel] [PATCH v2 1/2] checkpatch: add hwaddr to @typeList

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

On Thu, Sep 14, 2017 at 04:14:33PM +0200, Greg Kurz wrote:
> The script doesn't know about all possible types and learn them as
> it parses the code. If it reaches a line with a type cast but the
> type isn't known yet, it is misinterpreted as an identifier.
> 
> For example the following line:
> 
>     foo = (hwaddr) -1;
> 
> results in the following false-positive to be reported:
> 
> ERROR: spaces required around that '-' (ctx:VxV)
> 
> Let's add this standard QEMU type to the list of pre-known types.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Not sure who should queue this, though.

> ---
>  scripts/checkpatch.pl |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index fa478074b88d..def5bc1cc0e1 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -213,6 +213,7 @@ our @typeList = (
>  	qr{${Ident}_handler},
>  	qr{${Ident}_handler_fn},
>  	qr{target_(?:u)?long},
> +	qr{hwaddr},
>  );
>  
>  # This can be modified by sub possible.  Since it can be empty, be careful
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: make index property mandatory

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7905 bytes --]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: make index property mandatory

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 8773 bytes --]

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



-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/LTC                      http://www.ibm.com
Tel 33-5-6218-1607

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: make index property mandatory

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 9256 bytes --]

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



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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]