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
