[Qemu-devel] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Qemu-devel] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Laurent Vivier]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

Some kernels program a 0 address for io regions. PCI 3.0 spec
section 6.2.5.1 doesn't seem to disallow this.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
[lvivier: add pci_allow_0_addr in MachineClass to conditionally
allow addr 0 for pseries, as this can break other architectures]
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
v2: move flag from PCIBus to MachineClass, rename it pci_allow_0_address
 hw/pci/pci.c        | 12 +++++++++---
 hw/ppc/spapr.c      |  1 +
 include/hw/boards.h |  3 ++-
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a017614..9f57aea 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -38,6 +38,7 @@
 #include "hw/pci/msix.h"
 #include "exec/address-spaces.h"
 #include "hw/hotplug.h"
+#include "hw/boards.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI
@@ -1065,6 +1066,10 @@ static pcibus_t pci_bar_address(PCIDevice *d,
     pcibus_t new_addr, last_addr;
     int bar = pci_bar(d, reg);
     uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
+    Object *machine = qdev_get_machine();
+    ObjectClass *oc = object_get_class(machine);
+    MachineClass *mc = MACHINE_CLASS(oc);
+    bool allow_0_address = mc->pci_allow_0_address;
 
     if (type & PCI_BASE_ADDRESS_SPACE_IO) {
         if (!(cmd & PCI_COMMAND_IO)) {
@@ -1075,7 +1080,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
         /* Check if 32 bit BAR wraps around explicitly.
          * TODO: make priorities correct and remove this work around.
          */
-        if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) {
+        if (last_addr <= new_addr || last_addr >= UINT32_MAX ||
+            (!allow_0_address && new_addr == 0)) {
             return PCI_BAR_UNMAPPED;
         }
         return new_addr;
@@ -1099,8 +1105,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
     /* XXX: as we cannot support really dynamic
        mappings, we handle specific values as invalid
        mappings. */
-    if (last_addr <= new_addr || new_addr == 0 ||
-        last_addr == PCI_BAR_UNMAPPED) {
+    if (last_addr <= new_addr || last_addr == PCI_BAR_UNMAPPED ||
+        (!allow_0_address && new_addr == 0)) {
         return PCI_BAR_UNMAPPED;
     }
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a6f1947..bf0c64f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1835,6 +1835,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->default_ram_size = 512 * M_BYTE;
     mc->kvm_type = spapr_kvm_type;
     mc->has_dynamic_sysbus = true;
+    mc->pci_allow_0_address = true;
 
     fwc->get_dev_path = spapr_get_fw_dev_path;
     nc->nmi_monitor_handler = spapr_nmi;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 2aec9cb..3f84afd 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -100,7 +100,8 @@ struct MachineClass {
         no_cdrom:1,
         no_sdcard:1,
         has_dynamic_sysbus:1,
-        no_tco:1;
+        no_tco:1,
+        pci_allow_0_address:1;
     int is_default;
     const char *default_machine_opts;
     const char *default_boot_order;
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Michael S. Tsirkin]

On Thu, Jul 23, 2015 at 08:24:18PM +0200, Laurent Vivier wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>

I think at this point it's your patches, I'd write
"based on patch by Michael Roth" instead.

> Some kernels program a 0 address for io regions. PCI 3.0 spec
> section 6.2.5.1 doesn't seem to disallow this.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> [lvivier: add pci_allow_0_addr in MachineClass to conditionally
> allow addr 0 for pseries, as this can break other architectures]

Please spell the detailed explanation out in the commit log, not here.

> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

I don't know whether this is the right thing to do for spapr,
for pci.c I'm fine with this as a temporary hack:

Acked-by: Michael S. Tsirkin <mst@redhat.com>

but please look (in 2.5 timeframe) at fixing priorities for more machine
types, esp the PC.

> ---
> v2: move flag from PCIBus to MachineClass, rename it pci_allow_0_address
>  hw/pci/pci.c        | 12 +++++++++---
>  hw/ppc/spapr.c      |  1 +
>  include/hw/boards.h |  3 ++-
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index a017614..9f57aea 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -38,6 +38,7 @@
>  #include "hw/pci/msix.h"
>  #include "exec/address-spaces.h"
>  #include "hw/hotplug.h"
> +#include "hw/boards.h"
>  
>  //#define DEBUG_PCI
>  #ifdef DEBUG_PCI
> @@ -1065,6 +1066,10 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>      pcibus_t new_addr, last_addr;
>      int bar = pci_bar(d, reg);
>      uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
> +    Object *machine = qdev_get_machine();
> +    ObjectClass *oc = object_get_class(machine);
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    bool allow_0_address = mc->pci_allow_0_address;
>  
>      if (type & PCI_BASE_ADDRESS_SPACE_IO) {
>          if (!(cmd & PCI_COMMAND_IO)) {
> @@ -1075,7 +1080,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>          /* Check if 32 bit BAR wraps around explicitly.
>           * TODO: make priorities correct and remove this work around.
>           */
> -        if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) {
> +        if (last_addr <= new_addr || last_addr >= UINT32_MAX ||
> +            (!allow_0_address && new_addr == 0)) {
>              return PCI_BAR_UNMAPPED;
>          }
>          return new_addr;
> @@ -1099,8 +1105,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>      /* XXX: as we cannot support really dynamic
>         mappings, we handle specific values as invalid
>         mappings. */
> -    if (last_addr <= new_addr || new_addr == 0 ||
> -        last_addr == PCI_BAR_UNMAPPED) {
> +    if (last_addr <= new_addr || last_addr == PCI_BAR_UNMAPPED ||
> +        (!allow_0_address && new_addr == 0)) {
>          return PCI_BAR_UNMAPPED;
>      }
>  
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a6f1947..bf0c64f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1835,6 +1835,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->default_ram_size = 512 * M_BYTE;
>      mc->kvm_type = spapr_kvm_type;
>      mc->has_dynamic_sysbus = true;
> +    mc->pci_allow_0_address = true;
>  
>      fwc->get_dev_path = spapr_get_fw_dev_path;
>      nc->nmi_monitor_handler = spapr_nmi;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 2aec9cb..3f84afd 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -100,7 +100,8 @@ struct MachineClass {
>          no_cdrom:1,
>          no_sdcard:1,
>          has_dynamic_sysbus:1,
> -        no_tco:1;
> +        no_tco:1,
> +        pci_allow_0_address:1;
>      int is_default;
>      const char *default_machine_opts;
>      const char *default_boot_order;
> -- 
> 2.1.0

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Benjamin Herrenschmidt]

On Thu, 2015-07-23 at 20:24 +0200, Laurent Vivier wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Some kernels program a 0 address for io regions. PCI 3.0 spec
> section 6.2.5.1 doesn't seem to disallow this.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> [lvivier: add pci_allow_0_addr in MachineClass to conditionally
> allow addr 0 for pseries, as this can break other architectures]
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---

Why would it break other architectures ? The PCI bus will forward
address 0 just fine and some devices will decode it just fine too,
regardless of the architecture they are put on. I don't see why
having BARs capable of decoding it would break anything...

> v2: move flag from PCIBus to MachineClass, rename it pci_allow_0_address
>  hw/pci/pci.c        | 12 +++++++++---
>  hw/ppc/spapr.c      |  1 +
>  include/hw/boards.h |  3 ++-
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index a017614..9f57aea 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -38,6 +38,7 @@
>  #include "hw/pci/msix.h"
>  #include "exec/address-spaces.h"
>  #include "hw/hotplug.h"
> +#include "hw/boards.h"
>  
>  //#define DEBUG_PCI
>  #ifdef DEBUG_PCI
> @@ -1065,6 +1066,10 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>      pcibus_t new_addr, last_addr;
>      int bar = pci_bar(d, reg);
>      uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
> +    Object *machine = qdev_get_machine();
> +    ObjectClass *oc = object_get_class(machine);
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    bool allow_0_address = mc->pci_allow_0_address;
>  
>      if (type & PCI_BASE_ADDRESS_SPACE_IO) {
>          if (!(cmd & PCI_COMMAND_IO)) {
> @@ -1075,7 +1080,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>          /* Check if 32 bit BAR wraps around explicitly.
>           * TODO: make priorities correct and remove this work around.
>           */
> -        if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) {
> +        if (last_addr <= new_addr || last_addr >= UINT32_MAX ||
> +            (!allow_0_address && new_addr == 0)) {
>              return PCI_BAR_UNMAPPED;
>          }
>          return new_addr;
> @@ -1099,8 +1105,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>      /* XXX: as we cannot support really dynamic
>         mappings, we handle specific values as invalid
>         mappings. */
> -    if (last_addr <= new_addr || new_addr == 0 ||
> -        last_addr == PCI_BAR_UNMAPPED) {
> +    if (last_addr <= new_addr || last_addr == PCI_BAR_UNMAPPED ||
> +        (!allow_0_address && new_addr == 0)) {
>          return PCI_BAR_UNMAPPED;
>      }
>  
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a6f1947..bf0c64f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1835,6 +1835,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->default_ram_size = 512 * M_BYTE;
>      mc->kvm_type = spapr_kvm_type;
>      mc->has_dynamic_sysbus = true;
> +    mc->pci_allow_0_address = true;
>  
>      fwc->get_dev_path = spapr_get_fw_dev_path;
>      nc->nmi_monitor_handler = spapr_nmi;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 2aec9cb..3f84afd 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -100,7 +100,8 @@ struct MachineClass {
>          no_cdrom:1,
>          no_sdcard:1,
>          has_dynamic_sysbus:1,
> -        no_tco:1;
> +        no_tco:1,
> +        pci_allow_0_address:1;
>      int is_default;
>      const char *default_machine_opts;
>      const char *default_boot_order;

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Benjamin Herrenschmidt]

On Thu, 2015-07-23 at 21:30 +0300, Michael S. Tsirkin wrote:
> > @@ -1075,7 +1080,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
> >          /* Check if 32 bit BAR wraps around explicitly.
> >           * TODO: make priorities correct and remove this work
> around.
> >           */
> > -        if (last_addr <= new_addr || new_addr == 0 || last_addr >=
> UINT32_MAX) {
> > +        if (last_addr <= new_addr || last_addr >= UINT32_MAX ||
> > +            (!allow_0_address && new_addr == 0)) {
> >              return PCI_BAR_UNMAPPED;
> >          }

Talking of which, how can a BAR wrap around ? BARs are always power-of
two aligned and naturally aligned (to their own size), they can't
wrap...

Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Peter Maydell]

On 23 July 2015 at 21:46, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Thu, 2015-07-23 at 20:24 +0200, Laurent Vivier wrote:
>> From: Michael Roth <mdroth@linux.vnet.ibm.com>
>>
>> Some kernels program a 0 address for io regions. PCI 3.0 spec
>> section 6.2.5.1 doesn't seem to disallow this.
>>
>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>> [lvivier: add pci_allow_0_addr in MachineClass to conditionally
>> allow addr 0 for pseries, as this can break other architectures]
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>
> Why would it break other architectures ? The PCI bus will forward
> address 0 just fine and some devices will decode it just fine too,
> regardless of the architecture they are put on. I don't see why
> having BARs capable of decoding it would break anything...

Discussion from last time around:
http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg01358.html

suggests that it's a workaround for our PC model being buggy
and putting 0-address BARs over the top of some other system
device rather than underneath them...

(Also, none of our PCI device models actually try to do
the "BAR at zero means I won't respond" behaviour, which
presumably they might do in real life.)

-- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Michael S. Tsirkin]

On Thu, Jul 23, 2015 at 10:00:30PM +0100, Peter Maydell wrote:
> On 23 July 2015 at 21:46, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Thu, 2015-07-23 at 20:24 +0200, Laurent Vivier wrote:
> >> From: Michael Roth <mdroth@linux.vnet.ibm.com>
> >>
> >> Some kernels program a 0 address for io regions. PCI 3.0 spec
> >> section 6.2.5.1 doesn't seem to disallow this.
> >>
> >> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >> [lvivier: add pci_allow_0_addr in MachineClass to conditionally
> >> allow addr 0 for pseries, as this can break other architectures]
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> ---
> >
> > Why would it break other architectures ? The PCI bus will forward
> > address 0 just fine and some devices will decode it just fine too,
> > regardless of the architecture they are put on. I don't see why
> > having BARs capable of decoding it would break anything...
> 
> Discussion from last time around:
> http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg01358.html
> 
> suggests that it's a workaround for our PC model being buggy
> and putting 0-address BARs over the top of some other system
> device rather than underneath them...
> 
> (Also, none of our PCI device models actually try to do
> the "BAR at zero means I won't respond" behaviour, which
> presumably they might do in real life.)
> 
> -- PMM

Maybe some devices do this, but I'm guessing not all of them,
since there's no hint in the pci spec that they should.

-- 
MST

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Michael S. Tsirkin]

On Fri, Jul 24, 2015 at 06:48:57AM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2015-07-23 at 21:30 +0300, Michael S. Tsirkin wrote:
> > > @@ -1075,7 +1080,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
> > >          /* Check if 32 bit BAR wraps around explicitly.
> > >           * TODO: make priorities correct and remove this work
> > around.
> > >           */
> > > -        if (last_addr <= new_addr || new_addr == 0 || last_addr >=
> > UINT32_MAX) {
> > > +        if (last_addr <= new_addr || last_addr >= UINT32_MAX ||
> > > +            (!allow_0_address && new_addr == 0)) {
> > >              return PCI_BAR_UNMAPPED;
> > >          }
> 
> Talking of which, how can a BAR wrap around ? BARs are always power-of
> two aligned and naturally aligned (to their own size), they can't
> wrap...
> 
> Ben.
> 

True.

I seem to have thought it important:
https://lists.nongnu.org/archive/html/qemu-devel/2013-09/msg02594.html
but it makes no sense to me now.


-- 
MST

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Peter Maydell]

On 23 July 2015 at 22:10, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Jul 23, 2015 at 10:00:30PM +0100, Peter Maydell wrote:
>> (Also, none of our PCI device models actually try to do
>> the "BAR at zero means I won't respond" behaviour, which
>> presumably they might do in real life.)

> Maybe some devices do this, but I'm guessing not all of them,
> since there's no hint in the pci spec that they should.

I think this depends on which version of the spec you
read. The PCI 2.1 spec has an implementation note that
says:

# Note: A Base Address register does not contain a valid
# address when it is equal to "0".

which could be taken to mean "0 is invalid". A 'note'
isn't part of the formal spec, of course (and the text
got deleted from later spec revisions), but that
doesn't mean implementers necessarily ignored it.

There's an ancient Torvalds rant where he claims
in passing that "a lot" of devices do this:
http://yarchive.net/comp/linux/zero.html

-- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Peter Maydell]

On 23 July 2015 at 22:19, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 23 July 2015 at 22:10, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Thu, Jul 23, 2015 at 10:00:30PM +0100, Peter Maydell wrote:
>>> (Also, none of our PCI device models actually try to do
>>> the "BAR at zero means I won't respond" behaviour, which
>>> presumably they might do in real life.)
>
>> Maybe some devices do this, but I'm guessing not all of them,
>> since there's no hint in the pci spec that they should.
>
> I think this depends on which version of the spec you
> read.

Bikeshedding about ancient specs aside, I think it's the
bugs in the PC model's memory region priorities that
are the real reason the special case of zero is sticking
around. If we fixed those we should be able to drop it.

-- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Michael Roth]

Quoting Peter Maydell (2015-07-23 16:24:20)
> On 23 July 2015 at 22:19, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 23 July 2015 at 22:10, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> On Thu, Jul 23, 2015 at 10:00:30PM +0100, Peter Maydell wrote:
> >>> (Also, none of our PCI device models actually try to do
> >>> the "BAR at zero means I won't respond" behaviour, which
> >>> presumably they might do in real life.)
> >
> >> Maybe some devices do this, but I'm guessing not all of them,
> >> since there's no hint in the pci spec that they should.
> >
> > I think this depends on which version of the spec you
> > read.
> 
> Bikeshedding about ancient specs aside, I think it's the
> bugs in the PC model's memory region priorities that
> are the real reason the special case of zero is sticking
> around. If we fixed those we should be able to drop it.

What's the intended fix? That legacy/platform regions
should hide any regions a guest attempts to map over it?

> 
> -- PMM
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Benjamin Herrenschmidt]

On Thu, 2015-07-23 at 22:00 +0100, Peter Maydell wrote:
> (Also, none of our PCI device models actually try to do
> the "BAR at zero means I won't respond" behaviour, which
> presumably they might do in real life.)

Devices aren't supposed to do that ... In fact, I don't see HW designers
adding a comparator if they don't have to :-)

Cheers,
Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Michael Roth]

Quoting Michael Roth (2015-07-23 16:38:19)
> Quoting Peter Maydell (2015-07-23 16:24:20)
> > On 23 July 2015 at 22:19, Peter Maydell <peter.maydell@linaro.org> wrote:
> > > On 23 July 2015 at 22:10, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >> On Thu, Jul 23, 2015 at 10:00:30PM +0100, Peter Maydell wrote:
> > >>> (Also, none of our PCI device models actually try to do
> > >>> the "BAR at zero means I won't respond" behaviour, which
> > >>> presumably they might do in real life.)
> > >
> > >> Maybe some devices do this, but I'm guessing not all of them,
> > >> since there's no hint in the pci spec that they should.
> > >
> > > I think this depends on which version of the spec you
> > > read.
> > 
> > Bikeshedding about ancient specs aside, I think it's the
> > bugs in the PC model's memory region priorities that
> > are the real reason the special case of zero is sticking
> > around. If we fixed those we should be able to drop it.
> 
> What's the intended fix? That legacy/platform regions
> should hide any regions a guest attempts to map over it?

nm, i see this was already covered :)

I seem to recall Michael suggesting it may have already been
fixed on x86. I think we had a TODO to figure out all
the architectures that don't use IO windows and figure out
if they need a fix as well.

> 
> > 
> > -- PMM
> > 
> 
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Michael S. Tsirkin]

On Fri, Jul 24, 2015 at 07:46:55AM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2015-07-23 at 22:00 +0100, Peter Maydell wrote:
> > (Also, none of our PCI device models actually try to do
> > the "BAR at zero means I won't respond" behaviour, which
> > presumably they might do in real life.)
> 
> Devices aren't supposed to do that ... In fact, I don't see HW designers
> adding a comparator if they don't have to :-)
> 
> Cheers,
> Ben.
> 

You never can tell with HW designers. For example, maybe they
used this as a trick to avoid implementing memory/io enable
properly.

-- 
MST

[Qemu-devel] [PATCH v3] pci: allow 0 address for PCI IO/MEM regions

[Laurent Vivier]

Some kernels program a 0 address for io regions. PCI 3.0 spec
section 6.2.5.1 doesn't seem to disallow this.

based on patch by Michael Roth <mdroth@linux.vnet.ibm.com>

Add pci_allow_0_addr in MachineClass to conditionally
allow addr 0 for pseries, as this can break other architectures.

This patch allows to hotplug PCI card in pseries machine, as the first
added card BAR0 is always set to 0 address.

This as a temporary hack, waiting to fix PCI memory priorities for more
machine types...

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
v2: move flag from PCIBus to MachineClass, rename it pci_allow_0_address
v3: change author, update commit message.
 hw/pci/pci.c        | 12 +++++++++---
 hw/ppc/spapr.c      |  1 +
 include/hw/boards.h |  3 ++-
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a017614..9f57aea 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -38,6 +38,7 @@
 #include "hw/pci/msix.h"
 #include "exec/address-spaces.h"
 #include "hw/hotplug.h"
+#include "hw/boards.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI
@@ -1065,6 +1066,10 @@ static pcibus_t pci_bar_address(PCIDevice *d,
     pcibus_t new_addr, last_addr;
     int bar = pci_bar(d, reg);
     uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
+    Object *machine = qdev_get_machine();
+    ObjectClass *oc = object_get_class(machine);
+    MachineClass *mc = MACHINE_CLASS(oc);
+    bool allow_0_address = mc->pci_allow_0_address;
 
     if (type & PCI_BASE_ADDRESS_SPACE_IO) {
         if (!(cmd & PCI_COMMAND_IO)) {
@@ -1075,7 +1080,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
         /* Check if 32 bit BAR wraps around explicitly.
          * TODO: make priorities correct and remove this work around.
          */
-        if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) {
+        if (last_addr <= new_addr || last_addr >= UINT32_MAX ||
+            (!allow_0_address && new_addr == 0)) {
             return PCI_BAR_UNMAPPED;
         }
         return new_addr;
@@ -1099,8 +1105,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
     /* XXX: as we cannot support really dynamic
        mappings, we handle specific values as invalid
        mappings. */
-    if (last_addr <= new_addr || new_addr == 0 ||
-        last_addr == PCI_BAR_UNMAPPED) {
+    if (last_addr <= new_addr || last_addr == PCI_BAR_UNMAPPED ||
+        (!allow_0_address && new_addr == 0)) {
         return PCI_BAR_UNMAPPED;
     }
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a6f1947..bf0c64f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1835,6 +1835,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->default_ram_size = 512 * M_BYTE;
     mc->kvm_type = spapr_kvm_type;
     mc->has_dynamic_sysbus = true;
+    mc->pci_allow_0_address = true;
 
     fwc->get_dev_path = spapr_get_fw_dev_path;
     nc->nmi_monitor_handler = spapr_nmi;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 2aec9cb..3f84afd 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -100,7 +100,8 @@ struct MachineClass {
         no_cdrom:1,
         no_sdcard:1,
         has_dynamic_sysbus:1,
-        no_tco:1;
+        no_tco:1,
+        pci_allow_0_address:1;
     int is_default;
     const char *default_machine_opts;
     const char *default_boot_order;
-- 
2.1.0

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Peter Maydell]

On 23 July 2015 at 22:49, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> I seem to recall Michael suggesting it may have already been
> fixed on x86. I think we had a TODO to figure out all
> the architectures that don't use IO windows and figure out
> if they need a fix as well.

If we think we've fixed x86, then I think for 2.5 I would
be happy with "allow 0 addresses globally and see if anything
breaks". I'm pretty confident none of the ARM platforms will
have issues -- they don't do any kind of overlapping of
non-PCI memory regions with the MMIO window(s).

-- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

[Laurent Vivier]

On 24/07/2015 10:46, Peter Maydell wrote:
> On 23 July 2015 at 22:49, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>> I seem to recall Michael suggesting it may have already been
>> fixed on x86. I think we had a TODO to figure out all
>> the architectures that don't use IO windows and figure out
>> if they need a fix as well.
> 
> If we think we've fixed x86, then I think for 2.5 I would
> be happy with "allow 0 addresses globally and see if anything
> breaks". I'm pretty confident none of the ARM platforms will

I agree. It was exactly what I was thinking...

> have issues -- they don't do any kind of overlapping of
> non-PCI memory regions with the MMIO window(s).

So, after a "grep PCI" in defconfigs, we can have issue with powerpc,
s390x and sparc64 (I guess it is already fixed for x86_64 and i386 ?).

Laurent

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3] pci: allow 0 address for PCI IO/MEM regions

[Laurent Vivier]

Alex,

could you ACK this patch ?

It's not perfect and it will be removed later, but for the moment it
allows to hotplug PCI card in pseries.

Laurent

On 24/07/2015 10:35, Laurent Vivier wrote:
> Some kernels program a 0 address for io regions. PCI 3.0 spec
> section 6.2.5.1 doesn't seem to disallow this.
> 
> based on patch by Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Add pci_allow_0_addr in MachineClass to conditionally
> allow addr 0 for pseries, as this can break other architectures.
> 
> This patch allows to hotplug PCI card in pseries machine, as the first
> added card BAR0 is always set to 0 address.
> 
> This as a temporary hack, waiting to fix PCI memory priorities for more
> machine types...
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> v2: move flag from PCIBus to MachineClass, rename it pci_allow_0_address
> v3: change author, update commit message.
>  hw/pci/pci.c        | 12 +++++++++---
>  hw/ppc/spapr.c      |  1 +
>  include/hw/boards.h |  3 ++-
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index a017614..9f57aea 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -38,6 +38,7 @@
>  #include "hw/pci/msix.h"
>  #include "exec/address-spaces.h"
>  #include "hw/hotplug.h"
> +#include "hw/boards.h"
>  
>  //#define DEBUG_PCI
>  #ifdef DEBUG_PCI
> @@ -1065,6 +1066,10 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>      pcibus_t new_addr, last_addr;
>      int bar = pci_bar(d, reg);
>      uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
> +    Object *machine = qdev_get_machine();
> +    ObjectClass *oc = object_get_class(machine);
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    bool allow_0_address = mc->pci_allow_0_address;
>  
>      if (type & PCI_BASE_ADDRESS_SPACE_IO) {
>          if (!(cmd & PCI_COMMAND_IO)) {
> @@ -1075,7 +1080,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>          /* Check if 32 bit BAR wraps around explicitly.
>           * TODO: make priorities correct and remove this work around.
>           */
> -        if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) {
> +        if (last_addr <= new_addr || last_addr >= UINT32_MAX ||
> +            (!allow_0_address && new_addr == 0)) {
>              return PCI_BAR_UNMAPPED;
>          }
>          return new_addr;
> @@ -1099,8 +1105,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>      /* XXX: as we cannot support really dynamic
>         mappings, we handle specific values as invalid
>         mappings. */
> -    if (last_addr <= new_addr || new_addr == 0 ||
> -        last_addr == PCI_BAR_UNMAPPED) {
> +    if (last_addr <= new_addr || last_addr == PCI_BAR_UNMAPPED ||
> +        (!allow_0_address && new_addr == 0)) {
>          return PCI_BAR_UNMAPPED;
>      }
>  
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a6f1947..bf0c64f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1835,6 +1835,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->default_ram_size = 512 * M_BYTE;
>      mc->kvm_type = spapr_kvm_type;
>      mc->has_dynamic_sysbus = true;
> +    mc->pci_allow_0_address = true;
>  
>      fwc->get_dev_path = spapr_get_fw_dev_path;
>      nc->nmi_monitor_handler = spapr_nmi;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 2aec9cb..3f84afd 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -100,7 +100,8 @@ struct MachineClass {
>          no_cdrom:1,
>          no_sdcard:1,
>          has_dynamic_sysbus:1,
> -        no_tco:1;
> +        no_tco:1,
> +        pci_allow_0_address:1;
>      int is_default;
>      const char *default_machine_opts;
>      const char *default_boot_order;
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3] pci: allow 0 address for PCI IO/MEM regions

[Laurent Vivier]

Ping ?

On 27/07/2015 10:19, Laurent Vivier wrote:
> Alex,
> 
> could you ACK this patch ?
> 
> It's not perfect and it will be removed later, but for the moment it
> allows to hotplug PCI card in pseries.
> 
> Laurent
> 
> On 24/07/2015 10:35, Laurent Vivier wrote:
>> Some kernels program a 0 address for io regions. PCI 3.0 spec
>> section 6.2.5.1 doesn't seem to disallow this.
>>
>> based on patch by Michael Roth <mdroth@linux.vnet.ibm.com>
>>
>> Add pci_allow_0_addr in MachineClass to conditionally
>> allow addr 0 for pseries, as this can break other architectures.
>>
>> This patch allows to hotplug PCI card in pseries machine, as the first
>> added card BAR0 is always set to 0 address.
>>
>> This as a temporary hack, waiting to fix PCI memory priorities for more
>> machine types...
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>> v2: move flag from PCIBus to MachineClass, rename it pci_allow_0_address
>> v3: change author, update commit message.
>>  hw/pci/pci.c        | 12 +++++++++---
>>  hw/ppc/spapr.c      |  1 +
>>  include/hw/boards.h |  3 ++-
>>  3 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index a017614..9f57aea 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -38,6 +38,7 @@
>>  #include "hw/pci/msix.h"
>>  #include "exec/address-spaces.h"
>>  #include "hw/hotplug.h"
>> +#include "hw/boards.h"
>>  
>>  //#define DEBUG_PCI
>>  #ifdef DEBUG_PCI
>> @@ -1065,6 +1066,10 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>>      pcibus_t new_addr, last_addr;
>>      int bar = pci_bar(d, reg);
>>      uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
>> +    Object *machine = qdev_get_machine();
>> +    ObjectClass *oc = object_get_class(machine);
>> +    MachineClass *mc = MACHINE_CLASS(oc);
>> +    bool allow_0_address = mc->pci_allow_0_address;
>>  
>>      if (type & PCI_BASE_ADDRESS_SPACE_IO) {
>>          if (!(cmd & PCI_COMMAND_IO)) {
>> @@ -1075,7 +1080,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>>          /* Check if 32 bit BAR wraps around explicitly.
>>           * TODO: make priorities correct and remove this work around.
>>           */
>> -        if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) {
>> +        if (last_addr <= new_addr || last_addr >= UINT32_MAX ||
>> +            (!allow_0_address && new_addr == 0)) {
>>              return PCI_BAR_UNMAPPED;
>>          }
>>          return new_addr;
>> @@ -1099,8 +1105,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>>      /* XXX: as we cannot support really dynamic
>>         mappings, we handle specific values as invalid
>>         mappings. */
>> -    if (last_addr <= new_addr || new_addr == 0 ||
>> -        last_addr == PCI_BAR_UNMAPPED) {
>> +    if (last_addr <= new_addr || last_addr == PCI_BAR_UNMAPPED ||
>> +        (!allow_0_address && new_addr == 0)) {
>>          return PCI_BAR_UNMAPPED;
>>      }
>>  
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index a6f1947..bf0c64f 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1835,6 +1835,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>>      mc->default_ram_size = 512 * M_BYTE;
>>      mc->kvm_type = spapr_kvm_type;
>>      mc->has_dynamic_sysbus = true;
>> +    mc->pci_allow_0_address = true;
>>  
>>      fwc->get_dev_path = spapr_get_fw_dev_path;
>>      nc->nmi_monitor_handler = spapr_nmi;
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 2aec9cb..3f84afd 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -100,7 +100,8 @@ struct MachineClass {
>>          no_cdrom:1,
>>          no_sdcard:1,
>>          has_dynamic_sysbus:1,
>> -        no_tco:1;
>> +        no_tco:1,
>> +        pci_allow_0_address:1;
>>      int is_default;
>>      const char *default_machine_opts;
>>      const char *default_boot_order;
>>
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3] pci: allow 0 address for PCI IO/MEM regions

[Alexander Graf]

On 27.07.15 10:19, Laurent Vivier wrote:
> Alex,
> 
> could you ACK this patch ?
> 
> It's not perfect and it will be removed later, but for the moment it
> allows to hotplug PCI card in pseries.

PCIe definitely allows for BARs to be allocated to 0, so this is more or
less "the right thing". I really just wanted to make everyone aware that
I've run into issues with BARs actually located at address 0 in generic
PCI code in Linux ;).

So I don't think you really need my ack, as you definitely don't have a
nack from my side.


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3] pci: allow 0 address for PCI IO/MEM regions

[Laurent Vivier]

On 11/08/2015 10:50, Alexander Graf wrote:
> 
> 
> On 27.07.15 10:19, Laurent Vivier wrote:
>> Alex,
>>
>> could you ACK this patch ?
>>
>> It's not perfect and it will be removed later, but for the moment it
>> allows to hotplug PCI card in pseries.
> 
> PCIe definitely allows for BARs to be allocated to 0, so this is more or
> less "the right thing". I really just wanted to make everyone aware that
> I've run into issues with BARs actually located at address 0 in generic
> PCI code in Linux ;).
> 
> So I don't think you really need my ack, as you definitely don't have a
> nack from my side.

Thank you Alex !

Michael S. T, could you take the patch ?

Thanks,
Laurent