Re: [Qemu-devel] [PATCH 1/1] Fix device listener interface for PCI to PCI bridges

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, May 28, 2015 at 07:25:50AM -0400, Don Slutz wrote:
> 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 <dslutz@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);
> }
> 

I'm not sure what oldbus is but basically ok.
And it must be invoked whenever bus visibility changes,
not just its number.

> > 
> > 
> >> 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);
> }

Not really because it's not just secondary bus number.
Changing subordinate bus numbers can hide/unhide whole buses.



> 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
> 
> 

Imagine a bridge with secondary bus number 5
behind another one with subordinate numbers 1-3.
You should not send conf cycles for bus number 5 to qemu.

-- 
MST