Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 09:35:20PM +0200, Laszlo Ersek wrote:
> On 06/17/15 21:21, Michael S. Tsirkin wrote:
> > On Wed, Jun 17, 2015 at 02:45:05PM +0200, Laszlo Ersek wrote:
> >> SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
> >> to follow the pattern
> >>
> >>   /address@hidden/address@hidden/...
> >>
> >> for devices that live behind an extra root bus. The extra root bus in
> >> question is the N'th among the extra root bridges. (In other words, N
> >> gives the position of the affected extra root bus relative to the other
> >> extra root buses, in bus_nr order.) N starts at 1, and is formatted in
> >> hex.
> >>
> >> The "address@hidden" node text is hardcoded in SeaBIOS (see the macro
> >> FW_PCI_DOMAIN).
> >>
> >> Cc: Kevin O'Connor <address@hidden>
> >> Cc: Marcel Apfelbaum <address@hidden>
> >> Cc: Michael S. Tsirkin <address@hidden>
> >> Signed-off-by: Laszlo Ersek <address@hidden>
> >> Tested-by: Marcel Apfelbaum <address@hidden>
> >> Reviewed-by: Marcel Apfelbaum <address@hidden>
> >> ---
> >>
> >> Notes:
> >>     v6:
> >>     - no changes
> >>     
> >>     v5:
> >>     - constify parameter and local variables of 
> >> pxb_host_ofw_unit_address(),
> >>       in accord with the previous patch
> >>     
> >>     v4:
> >>     - new in v4
> >>
> >>  hw/pci-bridge/pci_expander_bridge.c | 39 
> >> ++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 38 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/pci-bridge/pci_expander_bridge.c 
> >> b/hw/pci-bridge/pci_expander_bridge.c
> >> index 4398d98..37574ec 100644
> >> --- a/hw/pci-bridge/pci_expander_bridge.c
> >> +++ b/hw/pci-bridge/pci_expander_bridge.c
> >> @@ -42,6 +42,8 @@ typedef struct PXBDev {
> >>      uint16_t numa_node;
> >>  } PXBDev;
> >>  
> >> +static GList *pxb_dev_list;
> >> +
> >>  #define TYPE_PXB_HOST "pxb-host"
> >>  
> >>  static int pxb_bus_num(PCIBus *bus)
> > 
> > 
> > So this trick breaks if we ever do have multiple roots, with expanders
> > under each.
> 
> Assuming I understand what you mean by "multiple roots", I guess the
> trick could be extended to maintain a separate list of expanders for
> each root.

You don't need an extra list: expander are already linked
under root.

> > I guess you could go to parent bus, scan child devices of parent and
> > count TYPE_PXB_DEVICE devices there.
> > 
> > 
> > BTW does this work if we have pci bridges under the default root in
> > addition to PXBs?
> 
> I have no clue how to configure that on the QEMU command line, sorry.

Simple -device pvi-bridge-device,chassis_nr=XXX,id=foo then attach
devices there.

> > Does seabios know to skip regular bridges when it's
> > counting roots?
> 
> Something for Kevin to answer :)
> 
> The initial scan in OVMF doesn't look for regular bridges specifically.
> When a device in 0x00..0x1f, function 0, responds from a bus in
> 0x01..0xff, that bus is considered a live extra root bus.

Do you do it before or after configuring bridges under root bus 0?
If after, then it's a bug - you will discover regular bridges this way.


> The bridges
> that hang off the expanders come to life much later, when the generic
> edk2 PCI bus driver enumerates devices (on the root buses that the OVMF
> PCI host bridge / root bridge driver produced). No clue about regular
> bridges.
> 
> Thanks
> Laszlo
> 
> >> @@ -88,12 +90,29 @@ static const char *pxb_host_root_bus_path(PCIHostState 
> >> *host_bridge,
> >>      return bus->bus_path;
> >>  }
> >>  
> >> +static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
> >> +{
> >> +    const PCIHostState *host = PCI_HOST_BRIDGE(dev);
> >> +    const PCIBus *bus;
> >> +    const PXBDev *pxb;
> >> +    int position;
> >> +
> >> +    bus = host->bus;
> >> +    pxb = PXB_DEV(bus->parent_dev);
> >> +    position = g_list_index(pxb_dev_list, pxb);
> >> +    assert(position >= 0);
> >> +
> >> +    return g_strdup_printf("%x/address@hidden", position + 1);
> >> +}
> >> +
> >>  static void pxb_host_class_init(ObjectClass *class, void *data)
> >>  {
> >>      DeviceClass *dc = DEVICE_CLASS(class);
> >> +    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class);
> >>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
> >>  
> >> -    dc->fw_name = "pci";
> >> +    dc->fw_name = "pci-root";
> >> +    sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
> >>      hc->root_bus_path = pxb_host_root_bus_path;
> >>  }
> >>  
> >> @@ -148,6 +167,15 @@ static int pxb_map_irq_fn(PCIDevice *pci_dev, int pin)
> >>      return pin - PCI_SLOT(pxb->devfn);
> >>  }
> >>  
> >> +static gint pxb_compare(gconstpointer a, gconstpointer b)
> >> +{
> >> +    const PXBDev *pxb_a = a, *pxb_b = b;
> >> +
> >> +    return pxb_a->bus_nr < pxb_b->bus_nr ? -1 :
> >> +           pxb_a->bus_nr > pxb_b->bus_nr ?  1 :
> >> +           0;
> >> +}
> >> +
> >>  static int pxb_dev_initfn(PCIDevice *dev)
> >>  {
> >>      PXBDev *pxb = PXB_DEV(dev);
> >> @@ -191,9 +219,17 @@ static int pxb_dev_initfn(PCIDevice *dev)
> >>                                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
> >>      pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
> >>  
> >> +    pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb, pxb_compare);
> >>      return 0;
> >>  }
> >>  
> >> +static void pxb_dev_exitfn(PCIDevice *pci_dev)
> >> +{
> >> +    PXBDev *pxb = PXB_DEV(pci_dev);
> >> +
> >> +    pxb_dev_list = g_list_remove(pxb_dev_list, pxb);
> >> +}
> >> +
> >>  static Property pxb_dev_properties[] = {
> >>      /* Note: 0 is not a legal a PXB bus number. */
> >>      DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
> >> @@ -207,6 +243,7 @@ static void pxb_dev_class_init(ObjectClass *klass, 
> >> void *data)
> >>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >>  
> >>      k->init = pxb_dev_initfn;
> >> +    k->exit = pxb_dev_exitfn;
> >>      k->vendor_id = PCI_VENDOR_ID_REDHAT;
> >>      k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
> >>      k->class_id = PCI_CLASS_BRIDGE_HOST;
> >> -- 
> >> 1.8.3.1