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

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

[Michael Roth]

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>
---
 hw/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 351d320..9578749 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1035,7 +1035,7 @@ 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) {
             return PCI_BAR_UNMAPPED;
         }
         return new_addr;
-- 
1.9.1

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

[Alexey Kardashevskiy]

On 08/19/2014 10:21 AM, Michael Roth 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.


I remember there was discussion about it but I forgot :) Why does it have
to be a part of this patchset? Worth mentioning in the commit log I believe.


> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  hw/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 351d320..9578749 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1035,7 +1035,7 @@ 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) {
>              return PCI_BAR_UNMAPPED;
>          }
>          return new_addr;
> 


-- 
Alexey

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

[Alexander Graf]

On 19.08.14 02:21, Michael Roth 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.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

This patch does not need to be inside of this patch set. It also should
go via Michael's tree.


Alex

> ---
>  hw/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 351d320..9578749 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1035,7 +1035,7 @@ 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) {
>              return PCI_BAR_UNMAPPED;
>          }
>          return new_addr;
> 

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

[Peter Maydell]

On 26 August 2014 10:14, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> On 08/19/2014 10:21 AM, Michael Roth 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.
>
>
> I remember there was discussion about it but I forgot :)

I think the conclusion we came to was that there may have been
a note in the PCI 2.1 spec that implied that 0 addresses meant
"disabled" but this seems to have gone from later versions,
suggesting it was erroneous.

Personally I'm happy for us to remove the "0 means disabled"
check, but I'd prefer it if we do it consistently for both IO and
MMIO regions -- this patch only changes the IO BAR code.

thanks
-- PMM

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

[Michael Roth]

Quoting Alexey Kardashevskiy (2014-08-26 04:14:27)
> On 08/19/2014 10:21 AM, Michael Roth 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.
> 
> 
> I remember there was discussion about it but I forgot :) Why does it have
> to be a part of this patchset? Worth mentioning in the commit log I believe.

Unfortunately with ppc guests the first bar allocation tends to be the
0-address case, so to me it seemed necessary for a testable series. Can
simply document this in the series and re-send separately though.

> 
> 
> > 
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> >  hw/pci/pci.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 351d320..9578749 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1035,7 +1035,7 @@ 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) {
> >              return PCI_BAR_UNMAPPED;
> >          }
> >          return new_addr;
> > 
> 
> 
> -- 
> Alexey

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

[Michael S. Tsirkin]

On Mon, Aug 18, 2014 at 07:21:54PM -0500, Michael Roth 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.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Yes the PCI spec does not care.

But unfortunately as documented in the comment, at
least for PC (maybe others) priorities aren't
currently setup correctly, so programming PCI BAR at
address zero (during sizing) conflicts with
whatever else is there.

To make address 0 work, you'll have to fix up the prioriorities for a
bunch of machine types :(

> ---
>  hw/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 351d320..9578749 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1035,7 +1035,7 @@ 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) {
>              return PCI_BAR_UNMAPPED;
>          }
>          return new_addr;
> -- 
> 1.9.1
> 

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

[Michael Roth]

Quoting Michael S. Tsirkin (2014-08-27 08:47:51)
> On Mon, Aug 18, 2014 at 07:21:54PM -0500, Michael Roth 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.
> > 
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Yes the PCI spec does not care.
> 
> But unfortunately as documented in the comment, at
> least for PC (maybe others) priorities aren't
> currently setup correctly, so programming PCI BAR at
> address zero (during sizing) conflicts with
> whatever else is there.

I'm not sure I understand: that note was included as part of the following
fixup to 9f1a029abf15751e32a4b1df99ed2b8315f9072c:

-        if (last_addr <= new_addr || new_addr == 0) {
+        /* Check if BAR is being sized explicitly.
+         * TODO: make priorities correct and remove this work around.
+         */
+        if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX)


which forces the BAR to PCI_BAR_UNMAPPED and unmaps the io region if the
address range extends beyond UINT32_MAX (which would happen during sizing
when guest writes -1...and I guess maybe last_addr <= new_addr covered the
same case back when we used uint32_t for pcibus_t?) ...

But the (new_addr == 0) seems to be something unrelated..., it means the
guest actually attempted to program a 0 address, or...

since pci_update_mappings unconditionally updates all IO regions for a
device whenever a particular BAR is written to, it would prevent us from
temporarily mapping all the IO regions to 0 (until guest re-assigns them)
...

You mentioned in the past this could lead to dispatch tables getting
permanantly corrupted, so maybe that's what the check was for?

But I guess there's still a separate issue, where there's a high liklihood that
a 0 address would conflict with some hard-wired IO address? Wouldn't this be a
guest bug though? Well, I guess it would be a QEMU bug if the above scenario
is a real one...but if we fix or verify that's not the case, would this be
an acceptable change?

> 
> To make address 0 work, you'll have to fix up the prioriorities for a
> bunch of machine types :(
> 
> > ---
> >  hw/pci/pci.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 351d320..9578749 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1035,7 +1035,7 @@ 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) {
> >              return PCI_BAR_UNMAPPED;
> >          }
> >          return new_addr;
> > -- 
> > 1.9.1
> >

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

[Peter Maydell]

On 28 August 2014 22:21, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> But I guess there's still a separate issue, where there's a high liklihood that
> a 0 address would conflict with some hard-wired IO address? Wouldn't this be a
> guest bug though?

Even if it's a guest bug, we should act like the hardware does
if the guest does this. If that differs between PCI controllers
then we need a flag so the host controller model can select
the required behaviour. (The versatile PB PCI controller we
model does have "address 0 is valid", and we'd need to have
this working if we implemented DMA accesses properly.)

-- PMM

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

[Michael S. Tsirkin]

On Thu, Aug 28, 2014 at 10:33:02PM +0100, Peter Maydell wrote:
> On 28 August 2014 22:21, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > But I guess there's still a separate issue, where there's a high liklihood that
> > a 0 address would conflict with some hard-wired IO address? Wouldn't this be a
> > guest bug though?

Real hardware behaves in a specific way.
the problem is we don't always emulate it properly, and PCI
has some work arounds for that.

> Even if it's a guest bug, we should act like the hardware does
> if the guest does this. If that differs between PCI controllers
> then we need a flag so the host controller model can select
> the required behaviour. (The versatile PB PCI controller we
> model does have "address 0 is valid", and we'd need to have
> this working if we implemented DMA accesses properly.)
> 
> -- PMM

Exactly.
Actually, I forgot that I might have already fixed it for PC:
83d08f2673504a299194dcac1657a13754b5932a
    pc: map PCI address space as catchall region for not mapped addresses

But need to go back and re-check other systems.


-- 
MST

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

[Claudio Fontana]

Hello,

resurrecting an old thread.. I incurred in the same issue being
discussed before,
where QEMU silently ignores PCI BAR address programming attempts where
the I/O space offset is 0 (zero).

I think that from a QEMU "user" standpoint, beside this particular issue,
which can be easily worked around just using a minimum offset,
it would be good if QEMU would be a bit verbose in producing a warning
about this.

I think that at least it would be worth a message if DEBUG_PCI is set.

This silent discarding of BAR programming attempts has been painful
while doing early enablement
even for other cases (like the requirement to set I/O space bit before
hand etc), which are legitimate,
but are still worthy of a diagnostic I think, at the very least if
doing pci enablement (which to me translates in having DEBUG_PCI set).

What do you guys think, would a patch be welcome trying to address that?
Would you make the diagnostic dependent on DEBUG_PCI?

Thanks,

Claudio

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

[Michael Roth]

Quoting Claudio Fontana (2015-01-09 08:57:39)
> Hello,
> 
> resurrecting an old thread.. I incurred in the same issue being
> discussed before,
> where QEMU silently ignores PCI BAR address programming attempts where
> the I/O space offset is 0 (zero).
> 
> I think that from a QEMU "user" standpoint, beside this particular issue,
> which can be easily worked around just using a minimum offset,
> it would be good if QEMU would be a bit verbose in producing a warning
> about this.
> 
> I think that at least it would be worth a message if DEBUG_PCI is set.
> 
> This silent discarding of BAR programming attempts has been painful
> while doing early enablement
> even for other cases (like the requirement to set I/O space bit before
> hand etc), which are legitimate,
> but are still worthy of a diagnostic I think, at the very least if
> doing pci enablement (which to me translates in having DEBUG_PCI set).
> 
> What do you guys think, would a patch be welcome trying to address that?
> Would you make the diagnostic dependent on DEBUG_PCI?

I've sent an updated patch (which allows 0-address MEM regions as well)
as a separate patchset. My hope is that we can simply allow the
programming of 0-address mem/io bars, but there are some concerns I
still don't quite understand but have attempted to summarize to
continue the discussion:

http://lists.gnu.org/archive/html/qemu-devel/2014-12/msg03453.html

Debugging would be useful, but there are undoubtedly cases where the
current behavior prevents proper initialization of guest devices on
existing guests which expect 0 to be valid, so at the very least I
think we should allow the behavior to be enabled on a
machine/host-bridge level of granularity, if not for all
machines/host-bridges as the patch above does.

> 
> Thanks,
> 
> Claudio