On Mon, 2014-04-07 at 16:34 +0300, Michael S. Tsirkin wrote:
On Mon, Apr 07, 2014 at 02:44:06PM +0200, Gerd Hoffmann wrote:
Hi,
u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC);One thing I'd do is maybe check that the relevant memory type is enabled in the bridge (probably just by writing fff to base and reading it back).
This will give hypervisors an option to avoid wasting resources: e.g. it's uncommon for express devices to claim IO.
I don't think we'll need that for the SHPC bridge.
Why not?
Because "has shpc" => not an PCIe port. (as far as I know) Anyway, why have shpc capability but no I/O or mem to support it?
Thanks, Marcel
I'm referring to this text in the bridge specification:
The I/O Base and I/O Limit registers are optional and define an address range that is used by the bridge to determine when to forward I/O transactions from one interface to the other. If a bridge does not implement an I/O address range, then both the I/O Base and I/O Limit registers must be implemented as read-only registers that return zero when read. If a bridge supports an I/O address range, then these registers must be initialized by configuration software so default states are not specified.
So we should probe bridge for I/O support before wasting I/O resources on it. The spec does not provide a way to detect this, but we can do it like this:
- write value ffffffff to I/O base register
Why write? A simple read would be enough. It will never be 0(if I/O or mem is required) because of the "Base Address" part of the register which represents the address range, right?
Thanks, Marcel
- read back value
value 0 means bridge does not support I/O.
A similar trick should work for other optional resources.
For express it indeed makes sense to avoid claiming IO address space. I'd try to find something more automatic though, where you don't need some kind of "disable io for this express port" config option.
Won't same trick as above work?
For express ports which can only have a single device underneath we can check whenever we have a device and if one is present already don't bother claiming extra resources for hotplug.
- for (cap = pci_config_readb(pci->bdf, PCI_CAPABILITY_LIST); cap;
cap = pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_NEXT))if (pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_ID) == cap_id)return cap;I would also limit this to 256 iterations, to make sure we dont' get into an infinite loop with a broken device.
Good point.
cheers, Gerd