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

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

[Michael Roth]

This patch enables the programming of address 0 for IO/MMIO BARs for
PCI devices.

It was originally included as part of a series implementing PCI
hotplug for pseries guests, where it is needed due to the fact
that pseries guests access IO space via MMIO, and that IO
space is dedicated to PCI devices, with RTAS calls being used in
place of common/legacy IO ports such as config-data/config-address.

Thus, the entire range is unhindered by legacy IO ports, and
pseries guest kernels may attempt to program an IO BAR to address 0
as a result.

This has led to a conflict with the existing PCI config space
emulation code, where it has been assumed that 0 address are always
invalid.

Some background from discussions can be viewed here:

  https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg03063.html

The general summary from that discussion seems to be that 0-addresses are
not (at least, are no longer) prohibited by current versions of the PCI
spec, and that the same should apply for MMIO addresses (where allowing
0-addresses are also needed for some ARM-based PCI controllers).

This patch includes support for 0-address MMIO BARs based on that
discussion.

One still-lingering concern is whether this change will impact
compatibility with guests where 0-addresses are invalid. There was
some discussion on whether this issue could be addressed using
memory region priorities, but I think that's still an open question
that we can hopefully address here.

[Qemu-devel] [PATCH] pci: allow 0 address for PCI IO/MEM 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 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 371699c..8dda27d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1034,7 +1034,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;
@@ -1058,8 +1058,7 @@ 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) {
         return PCI_BAR_UNMAPPED;
     }
 
-- 
1.9.1

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

[Michael Roth]

Quoting Michael Roth (2014-12-23 13:33:35)
> This patch enables the programming of address 0 for IO/MMIO BARs for
> PCI devices.
> 
> It was originally included as part of a series implementing PCI
> hotplug for pseries guests, where it is needed due to the fact
> that pseries guests access IO space via MMIO, and that IO
> space is dedicated to PCI devices, with RTAS calls being used in
> place of common/legacy IO ports such as config-data/config-address.
> 
> Thus, the entire range is unhindered by legacy IO ports, and
> pseries guest kernels may attempt to program an IO BAR to address 0
> as a result.
> 
> This has led to a conflict with the existing PCI config space
> emulation code, where it has been assumed that 0 address are always
> invalid.
> 
> Some background from discussions can be viewed here:
> 
>   https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg03063.html
> 
> The general summary from that discussion seems to be that 0-addresses are
> not (at least, are no longer) prohibited by current versions of the PCI
> spec, and that the same should apply for MMIO addresses (where allowing
> 0-addresses are also needed for some ARM-based PCI controllers).
> 
> This patch includes support for 0-address MMIO BARs based on that
> discussion.
> 
> One still-lingering concern is whether this change will impact
> compatibility with guests where 0-addresses are invalid. There was
> some discussion on whether this issue could be addressed using
> memory region priorities, but I think that's still an open question
> that we can hopefully address here.

Ping

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

[David Gibson]

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

On Mon, Jan 12, 2015 at 07:24:06AM -0600, Michael Roth wrote:
> Quoting Michael Roth (2014-12-23 13:33:35)
> > This patch enables the programming of address 0 for IO/MMIO BARs for
> > PCI devices.
> > 
> > It was originally included as part of a series implementing PCI
> > hotplug for pseries guests, where it is needed due to the fact
> > that pseries guests access IO space via MMIO, and that IO
> > space is dedicated to PCI devices, with RTAS calls being used in
> > place of common/legacy IO ports such as config-data/config-address.
> > 
> > Thus, the entire range is unhindered by legacy IO ports, and
> > pseries guest kernels may attempt to program an IO BAR to address 0
> > as a result.
> > 
> > This has led to a conflict with the existing PCI config space
> > emulation code, where it has been assumed that 0 address are always
> > invalid.
> > 
> > Some background from discussions can be viewed here:
> > 
> >   https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg03063.html
> > 
> > The general summary from that discussion seems to be that 0-addresses are
> > not (at least, are no longer) prohibited by current versions of the PCI
> > spec, and that the same should apply for MMIO addresses (where allowing
> > 0-addresses are also needed for some ARM-based PCI controllers).
> > 
> > This patch includes support for 0-address MMIO BARs based on that
> > discussion.
> > 
> > One still-lingering concern is whether this change will impact
> > compatibility with guests where 0-addresses are invalid. There was
> > some discussion on whether this issue could be addressed using
> > memory region priorities, but I think that's still an open question
> > that we can hopefully address here.
> 
> Ping

Sorry, I just got back from vacation.

It looks same to me.

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

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

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

[Claudio Fontana]

On 13.01.2015 06:46, David Gibson wrote:
> On Mon, Jan 12, 2015 at 07:24:06AM -0600, Michael Roth wrote:
>> Quoting Michael Roth (2014-12-23 13:33:35)
>>> This patch enables the programming of address 0 for IO/MMIO BARs for
>>> PCI devices.
>>>
>>> It was originally included as part of a series implementing PCI
>>> hotplug for pseries guests, where it is needed due to the fact
>>> that pseries guests access IO space via MMIO, and that IO
>>> space is dedicated to PCI devices, with RTAS calls being used in
>>> place of common/legacy IO ports such as config-data/config-address.
>>>
>>> Thus, the entire range is unhindered by legacy IO ports, and
>>> pseries guest kernels may attempt to program an IO BAR to address 0
>>> as a result.
>>>
>>> This has led to a conflict with the existing PCI config space
>>> emulation code, where it has been assumed that 0 address are always
>>> invalid.
>>>
>>> Some background from discussions can be viewed here:
>>>
>>>   https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg03063.html
>>>
>>> The general summary from that discussion seems to be that 0-addresses are
>>> not (at least, are no longer) prohibited by current versions of the PCI
>>> spec, and that the same should apply for MMIO addresses (where allowing
>>> 0-addresses are also needed for some ARM-based PCI controllers).
>>>
>>> This patch includes support for 0-address MMIO BARs based on that
>>> discussion.
>>>
>>> One still-lingering concern is whether this change will impact
>>> compatibility with guests where 0-addresses are invalid. There was
>>> some discussion on whether this issue could be addressed using
>>> memory region priorities, but I think that's still an open question
>>> that we can hopefully address here.
>>
>> Ping
> 
> Sorry, I just got back from vacation.
> 
> It looks same to me.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 

Can you guys take a look also at this:

http://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01048.html

I really think we should give diagnostics under PCI_DEBUG when trying to set new BAR addresses fails for any reason.

Ciao,

Claudio

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

[Michael S. Tsirkin]

On Mon, Jan 12, 2015 at 07:24:06AM -0600, Michael Roth wrote:
> Quoting Michael Roth (2014-12-23 13:33:35)
> > This patch enables the programming of address 0 for IO/MMIO BARs for
> > PCI devices.
> > 
> > It was originally included as part of a series implementing PCI
> > hotplug for pseries guests, where it is needed due to the fact
> > that pseries guests access IO space via MMIO, and that IO
> > space is dedicated to PCI devices, with RTAS calls being used in
> > place of common/legacy IO ports such as config-data/config-address.
> > 
> > Thus, the entire range is unhindered by legacy IO ports, and
> > pseries guest kernels may attempt to program an IO BAR to address 0
> > as a result.
> > 
> > This has led to a conflict with the existing PCI config space
> > emulation code, where it has been assumed that 0 address are always
> > invalid.
> > 
> > Some background from discussions can be viewed here:
> > 
> >   https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg03063.html
> > 
> > The general summary from that discussion seems to be that 0-addresses are
> > not (at least, are no longer) prohibited by current versions of the PCI
> > spec, and that the same should apply for MMIO addresses (where allowing
> > 0-addresses are also needed for some ARM-based PCI controllers).
> > 
> > This patch includes support for 0-address MMIO BARs based on that
> > discussion.
> > 
> > One still-lingering concern is whether this change will impact
> > compatibility with guests where 0-addresses are invalid. There was
> > some discussion on whether this issue could be addressed using
> > memory region priorities, but I think that's still an open question
> > that we can hopefully address here.
> 
> Ping

Michael, I can't apply this patch to all platforms: guests program 0 address
and expect that to not override all system devices.

If you want a quick hack, you need to find a way to make this apply
only to pseries.

One quick work-around for pseries is to limit your patch to devices
behind a pci to pci bridge: I think pseries places most devices behind
such bridges, am I right?

The right solution is to locate, for each target, all system devices
that overlap with pci space, and figure out what the correct priorities
are. It's far from trivial, which is likely why no one did this yet.

Another issue is that - assuming what we are targeting is purely
theorectical PCI spec compliance - the patch does not go far enough.
We also should drop the check for the all-ones pattern in the
same function, that, too, should be a platform thing.

In particular, things break badly if guests size BARs e.g. using e.g.
8 single-byte accesses, each one being a write of 0xff followed by read and
write of 0x00, as opposed to two 32 bit writes of 0xffffffff followed
by reads and then writes of 0x0000000 - which no one seems to go
but is definitely legal.

-- 
MST

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

[Michael Roth]

Quoting Michael S. Tsirkin (2015-01-13 04:12:19)
> On Mon, Jan 12, 2015 at 07:24:06AM -0600, Michael Roth wrote:
> > Quoting Michael Roth (2014-12-23 13:33:35)
> > > This patch enables the programming of address 0 for IO/MMIO BARs for
> > > PCI devices.
> > > 
> > > It was originally included as part of a series implementing PCI
> > > hotplug for pseries guests, where it is needed due to the fact
> > > that pseries guests access IO space via MMIO, and that IO
> > > space is dedicated to PCI devices, with RTAS calls being used in
> > > place of common/legacy IO ports such as config-data/config-address.
> > > 
> > > Thus, the entire range is unhindered by legacy IO ports, and
> > > pseries guest kernels may attempt to program an IO BAR to address 0
> > > as a result.
> > > 
> > > This has led to a conflict with the existing PCI config space
> > > emulation code, where it has been assumed that 0 address are always
> > > invalid.
> > > 
> > > Some background from discussions can be viewed here:
> > > 
> > >   https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg03063.html
> > > 
> > > The general summary from that discussion seems to be that 0-addresses are
> > > not (at least, are no longer) prohibited by current versions of the PCI
> > > spec, and that the same should apply for MMIO addresses (where allowing
> > > 0-addresses are also needed for some ARM-based PCI controllers).
> > > 
> > > This patch includes support for 0-address MMIO BARs based on that
> > > discussion.
> > > 
> > > One still-lingering concern is whether this change will impact
> > > compatibility with guests where 0-addresses are invalid. There was
> > > some discussion on whether this issue could be addressed using
> > > memory region priorities, but I think that's still an open question
> > > that we can hopefully address here.
> > 
> > Ping
> 
> Michael, I can't apply this patch to all platforms: guests program 0 address
> and expect that to not override all system devices.
> 
> If you want a quick hack, you need to find a way to make this apply
> only to pseries.
> 
> One quick work-around for pseries is to limit your patch to devices
> behind a pci to pci bridge: I think pseries places most devices behind
> such bridges, am I right?

Not to my knowledge. It's true that pseries guests on PowerVM generally
(but not always) get a dedicated host bridge for hotplugged
devices/expansion slots. Maybe that's what you're referring to?

> 
> The right solution is to locate, for each target, all system devices
> that overlap with pci space, and figure out what the correct priorities
> are. It's far from trivial, which is likely why no one did this yet.

So there are cases where system devices should have higher priorities,
and others where PCI devices can re-use the addresses?

Would it be possible to have a check to see if a BAR overlaps a region,
and then examine the mr->owner of the region to determine if it's a PCI
device? If it's not, then assume it's a system device and avoid mapping
the BAR as we do now? This would maintain the current behavior while
allowing devices to use address 0 when it hasn't been claimed by a
system device.

I'm sure it's not that easy, but if it a catch-all solution of this
sort it acceptable/work-able I can look into it.

Otherwise, as far as quick fixes, I was thinking of adding a
allow_zero_addr flag to TYPE_PCI_HOST_BRIDGE that can be set by
instance_init for the various implementations of it. That would allow us
to enable it for pseries, as well as (I think) some of the ARM use-cases
Peter mentioned in the older thread.

> 
> Another issue is that - assuming what we are targeting is purely
> theorectical PCI spec compliance - the patch does not go far enough.
> We also should drop the check for the all-ones pattern in the
> same function, that, too, should be a platform thing.
> 
> In particular, things break badly if guests size BARs e.g. using e.g.
> 8 single-byte accesses, each one being a write of 0xff followed by read and
> write of 0x00, as opposed to two 32 bit writes of 0xffffffff followed
> by reads and then writes of 0x0000000 - which no one seems to go
> but is definitely legal.

Yikes. Personally I'm more focused on a specific bug, but perhaps
controlling this via a host-bridge flag also makes sense?

> 
> -- 
> MST

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

[Michael S. Tsirkin]

On Tue, Jan 13, 2015 at 09:34:42AM -0600, Michael Roth wrote:
> Quoting Michael S. Tsirkin (2015-01-13 04:12:19)
> > On Mon, Jan 12, 2015 at 07:24:06AM -0600, Michael Roth wrote:
> > > Quoting Michael Roth (2014-12-23 13:33:35)
> > > > This patch enables the programming of address 0 for IO/MMIO BARs for
> > > > PCI devices.
> > > > 
> > > > It was originally included as part of a series implementing PCI
> > > > hotplug for pseries guests, where it is needed due to the fact
> > > > that pseries guests access IO space via MMIO, and that IO
> > > > space is dedicated to PCI devices, with RTAS calls being used in
> > > > place of common/legacy IO ports such as config-data/config-address.
> > > > 
> > > > Thus, the entire range is unhindered by legacy IO ports, and
> > > > pseries guest kernels may attempt to program an IO BAR to address 0
> > > > as a result.
> > > > 
> > > > This has led to a conflict with the existing PCI config space
> > > > emulation code, where it has been assumed that 0 address are always
> > > > invalid.
> > > > 
> > > > Some background from discussions can be viewed here:
> > > > 
> > > >   https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg03063.html
> > > > 
> > > > The general summary from that discussion seems to be that 0-addresses are
> > > > not (at least, are no longer) prohibited by current versions of the PCI
> > > > spec, and that the same should apply for MMIO addresses (where allowing
> > > > 0-addresses are also needed for some ARM-based PCI controllers).
> > > > 
> > > > This patch includes support for 0-address MMIO BARs based on that
> > > > discussion.
> > > > 
> > > > One still-lingering concern is whether this change will impact
> > > > compatibility with guests where 0-addresses are invalid. There was
> > > > some discussion on whether this issue could be addressed using
> > > > memory region priorities, but I think that's still an open question
> > > > that we can hopefully address here.
> > > 
> > > Ping
> > 
> > Michael, I can't apply this patch to all platforms: guests program 0 address
> > and expect that to not override all system devices.
> > 
> > If you want a quick hack, you need to find a way to make this apply
> > only to pseries.
> > 
> > One quick work-around for pseries is to limit your patch to devices
> > behind a pci to pci bridge: I think pseries places most devices behind
> > such bridges, am I right?
> 
> Not to my knowledge. It's true that pseries guests on PowerVM generally
> (but not always) get a dedicated host bridge for hotplugged
> devices/expansion slots. Maybe that's what you're referring to?
> 
> > 
> > The right solution is to locate, for each target, all system devices
> > that overlap with pci space, and figure out what the correct priorities
> > are. It's far from trivial, which is likely why no one did this yet.
> 
> So there are cases where system devices should have higher priorities,
> and others where PCI devices can re-use the addresses?

I'm not sure. On PC APIC should have higher priority but doesn't.
We'll have to examine each system on case by case basis.

> Would it be possible to have a check to see if a BAR overlaps a region,
> and then examine the mr->owner of the region to determine if it's a PCI
> device? If it's not, then assume it's a system device and avoid mapping
> the BAR as we do now? This would maintain the current behavior while
> allowing devices to use address 0 when it hasn't been claimed by a
> system device.
> 
> I'm sure it's not that easy, but if it a catch-all solution of this
> sort it acceptable/work-able I can look into it.

Sounds like just giving system devices higher priority than
system devices.

I think we already do this for PC:
	commit 83d08f2673504a299194dcac1657a13754b5932a
	Author: Michael S. Tsirkin <mst@redhat.com>
	Date:   Tue Oct 29 13:57:34 2013 +0100

	    pc: map PCI address space as catchall region for not mapped addresses

but we need to find and fix all other targets.



> Otherwise, as far as quick fixes, I was thinking of adding a
> allow_zero_addr flag to TYPE_PCI_HOST_BRIDGE that can be set by
> instance_init for the various implementations of it. That would allow us
> to enable it for pseries, as well as (I think) some of the ARM use-cases
> Peter mentioned in the older thread.

It's ugly, but possible.


> > 
> > Another issue is that - assuming what we are targeting is purely
> > theorectical PCI spec compliance - the patch does not go far enough.
> > We also should drop the check for the all-ones pattern in the
> > same function, that, too, should be a platform thing.
> > 
> > In particular, things break badly if guests size BARs e.g. using e.g.
> > 8 single-byte accesses, each one being a write of 0xff followed by read and
> > write of 0x00, as opposed to two 32 bit writes of 0xffffffff followed
> > by reads and then writes of 0x0000000 - which no one seems to go
> > but is definitely legal.
> 
> Yikes. Personally I'm more focused on a specific bug, but perhaps
> controlling this via a host-bridge flag also makes sense?

Since it's a theoretical issue (no known guest needs the fix)
I'm not inclined to add more hacks.
The right fix is probably to teach e.g. kvm to find out legal
range of physical addresses from CPU flags, and
skip ones that are out of range when passing them to kernel.


> > 
> > -- 
> > MST

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

[Michael S. Tsirkin]

On Tue, Jan 13, 2015 at 05:54:46PM +0200, Michael S. Tsirkin wrote:
> I think we already do this for PC:
> 	commit 83d08f2673504a299194dcac1657a13754b5932a
> 	Author: Michael S. Tsirkin <mst@redhat.com>
> 	Date:   Tue Oct 29 13:57:34 2013 +0100
> 
> 	    pc: map PCI address space as catchall region for not mapped addresses
> 
> but we need to find and fix all other targets.

BTW this is very easy to test.
Add an unused device (like ivshmem) enable BAR, and set it to 0.
System should survive, as opposed to hanging.

But the big question is whether this is the right
thing to do for each platform.
For PIIX whatever is not system memory, is PCI.
But other boxes might have a different view of the matter.



-- 
MST

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

[Alexander Graf]

On 01/13/15 17:17, Michael S. Tsirkin wrote:
> On Tue, Jan 13, 2015 at 05:54:46PM +0200, Michael S. Tsirkin wrote:
>> I think we already do this for PC:
>> 	commit 83d08f2673504a299194dcac1657a13754b5932a
>> 	Author: Michael S. Tsirkin <mst@redhat.com>
>> 	Date:   Tue Oct 29 13:57:34 2013 +0100
>>
>> 	    pc: map PCI address space as catchall region for not mapped addresses
>>
>> but we need to find and fix all other targets.
> BTW this is very easy to test.
> Add an unused device (like ivshmem) enable BAR, and set it to 0.
> System should survive, as opposed to hanging.
>
> But the big question is whether this is the right
> thing to do for each platform.
> For PIIX whatever is not system memory, is PCI.
> But other boxes might have a different view of the matter.

Only few platforms have PCI mapped at 0, no? So in most cases you get 
windows that simply don't have overlapping at all.


Alex

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

[Michael S. Tsirkin]

On Tue, Jan 13, 2015 at 06:01:53PM +0100, Alexander Graf wrote:
> On 01/13/15 17:17, Michael S. Tsirkin wrote:
> >On Tue, Jan 13, 2015 at 05:54:46PM +0200, Michael S. Tsirkin wrote:
> >>I think we already do this for PC:
> >>	commit 83d08f2673504a299194dcac1657a13754b5932a
> >>	Author: Michael S. Tsirkin <mst@redhat.com>
> >>	Date:   Tue Oct 29 13:57:34 2013 +0100
> >>
> >>	    pc: map PCI address space as catchall region for not mapped addresses
> >>
> >>but we need to find and fix all other targets.
> >BTW this is very easy to test.
> >Add an unused device (like ivshmem) enable BAR, and set it to 0.
> >System should survive, as opposed to hanging.
> >
> >But the big question is whether this is the right
> >thing to do for each platform.
> >For PIIX whatever is not system memory, is PCI.
> >But other boxes might have a different view of the matter.
> 
> Only few platforms have PCI mapped at 0, no? So in most cases you get
> windows that simply don't have overlapping at all.
> 
> 
> Alex

What's missing is complete analysis of the problem.
Ideally testing on several targets.

-- 
MST