On 05/28/15 05:30, Michael S. Tsirkin wrote:
> On Thu, May 28, 2015 at 04:46:37AM -0400, Don Slutz wrote:
>> The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 assumed that a
>> PCI device has a static address. This is not true for PCI devices
>> that are on the secondary bus of a PCI to PCI bridge.
>>
>> BIOS and/or guest OS can change the secondary bus number to a new
>> value at any time.
>>
>> When a PCI to PCI bridge bridge is reset, the secondary bus number
>> is set to zero. Normally the BIOS will set it to 255 during PCI bus
>> scanning so that only the PCI devices on the root can be accessed
>> via bus 0. Later it is set to a number between 1 and 254.
>>
>> Adjust xen_map_pcidev() to not register with Xen for secondary bus
>> numbers 0 and 255.
>>
>> Extend the device listener interface to be called when ever the
>> secondary bus number is set to a usable value. This includes
>> a call on unrealize if the secondary bus number was valid.
>>
>> Signed-off-by: Don Slutz <dsl...@verizon.com>
>> ---
>>
>> Note: Right now hvmloader in Xen does not handle PCI to PCI bridges
>> and so SeaBIOS does not have access to PCI device(s) on secondary
>> buses. How ever domU can setup the secondary bus(es) and this patch
>> will restore access to these PCI devices.
>>
>> hw/core/qdev.c | 10 ++++++++++
>> hw/pci/pci_bridge.c | 30 ++++++++++++++++++++++++++++++
>> include/hw/qdev-core.h | 2 ++
>> include/hw/xen/xen_common.h | 31 +++++++++++++++++++++++++------
>> trace-events | 1 +
>> 5 files changed, 68 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index b0f0f84..6a514ee 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -239,6 +239,16 @@ void device_listener_unregister(DeviceListener
>> *listener)
>> QTAILQ_REMOVE(&device_listeners, listener, link);
>> }
>>
>> +void device_listener_realize(DeviceState *dev)
>> +{
>> + DEVICE_LISTENER_CALL(realize, Forward, dev);
>> +}
>> +
>> +void device_listener_unrealize(DeviceState *dev)
>> +{
>> + DEVICE_LISTENER_CALL(unrealize, Forward, dev);
>> +}
>> +
>> static void device_realize(DeviceState *dev, Error **errp)
>> {
>> DeviceClass *dc = DEVICE_GET_CLASS(dev);
>
>
> This looks wrong. Devices not accessible to config cycles are still
> accessible e.g. to memory or IO. It's not the same as unrealize.
>
> You need some other API that makes sense,
> probably pci specific.
>
If I am understanding you correctly, you would like:
void device_listener_change_pci_bus_num(PCIDevice *d, uint8_t oldbus)
{
DEVICE_LISTENER_CALL(change_pci_bus_num, Forward, d, oldbus);
}
>
>
>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>> index 40c97b1..042680d 100644
>> --- a/hw/pci/pci_bridge.c
>> +++ b/hw/pci/pci_bridge.c
>> @@ -241,6 +241,18 @@ void pci_bridge_update_mappings(PCIBridge *br)
>> pci_bridge_region_cleanup(br, w);
>> }
>>
>> +static void pci_bridge_realize_sub(PCIBus *b, PCIDevice *d,
>> + void *opaque)
>> +{
>> + device_listener_realize(DEVICE(d));
>> +}
>> +
>> +static void pci_bridge_unrealize_sub(PCIBus *b, PCIDevice *d,
>> + void *opaque)
>> +{
>> + device_listener_unrealize(DEVICE(d));
>> +}
>> +
>> /* default write_config function for PCI-to-PCI bridge */
>> void pci_bridge_write_config(PCIDevice *d,
>> uint32_t address, uint32_t val, int len)
>> @@ -248,6 +260,8 @@ void pci_bridge_write_config(PCIDevice *d,
>> PCIBridge *s = PCI_BRIDGE(d);
>> uint16_t oldctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
>> uint16_t newctl;
>> + uint8_t oldbus = pci_get_byte(d->config + PCI_SECONDARY_BUS);
>> + uint8_t newbus;
>>
>> pci_default_write_config(d, address, val, len);
>>
>> @@ -265,6 +279,22 @@ void pci_bridge_write_config(PCIDevice *d,
>> pci_bridge_update_mappings(s);
>> }
>>
>> + newbus = pci_get_byte(d->config + PCI_SECONDARY_BUS);
>> + if (newbus != oldbus) {
>> + PCIBus *sec_bus = pci_bridge_get_sec_bus(s);
>> +
>> + if (oldbus && oldbus != 255) {
>> + pci_set_byte(d->config + PCI_SECONDARY_BUS, oldbus);
>> + pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
>> + pci_bridge_unrealize_sub, NULL);
>> + pci_set_byte(d->config + PCI_SECONDARY_BUS, newbus);
>> + }
>> + if (newbus && newbus != 255) {
>> + pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
>> + pci_bridge_realize_sub, NULL);
>> + }
>> + }
>> +
>
>
> This is relying on undocumented assumptions and how specific firmware
> works. There's nothing special about bus number 255, and 0 is not very
> special either (except it happens to be the reset value).
>
Ok, using the above it would change to (almost):
if (newbus != oldbus) {
pci_for_each_device(pci_bridge_get_sec_bus(s),
pci_bus_num(sec_bus),
device_listener_change_pci_bus_num,
oldbus);
}
Would it be better to have:
void device_listener_change_pci_bus_num(PCIBus *b, PCIDevice *d, void
*opaque)
{
uint8_t oldbus = (uint8_t)opaque;
DEVICE_LISTENER_CALL(change_pci_bus_num, Forward, d, oldbus);
}
So that the above works, or to add a function to convert args?
> To know whether device is accessible to config cycles, you
> really need to scan the hierarchy from root downwards.
>
Yes, that is correct. However what I am doing here is not
changing how QEMU checks if the device is accessible, but
changing what pci config Xen sends to QEMU. If the change
to PCI_SECONDARY_BUS hides this PCI to PCI bridge, that is
not an issue.
-Don Slutz
signature.asc
Description: OpenPGP digital signature
