Message ID | 1434203538-8075-5-git-send-email-lersek@redhat.com |
---|---|
State | New |
Headers | show |
On 06/13/2015 04:52 PM, Laszlo Ersek wrote: > SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file > to follow the pattern > > /pci-root@N/pci@i0cf8/... > > 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 "pci@i0cf8" node text is hardcoded in SeaBIOS (see the macro > FW_PCI_DOMAIN). > > Cc: Kevin O'Connor <kevin@koconnor.net> > Cc: Marcel Apfelbaum <marcel@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > 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 c7a085d..bed8ec9 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) > @@ -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(SysBusDevice *dev) > +{ > + PCIHostState *host = PCI_HOST_BRIDGE(dev); > + PCIBus *bus; > + 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/pci@i0cf8", 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; > +} return pxb_a->bus_nr - pxb_b->bus_nr ? :) This will not hold the series... I can send a patch on top. Thanks, Marcel > + > static int pxb_dev_initfn(PCIDevice *dev) > { > PXBDev *pxb = PXB_DEV(dev); > @@ -190,9 +218,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), > @@ -206,6 +242,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; >
On 06/14/15 12:08, Marcel Apfelbaum wrote: > On 06/13/2015 04:52 PM, Laszlo Ersek wrote: >> SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file >> to follow the pattern >> >> /pci-root@N/pci@i0cf8/... >> >> 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 "pci@i0cf8" node text is hardcoded in SeaBIOS (see the macro >> FW_PCI_DOMAIN). >> >> Cc: Kevin O'Connor <kevin@koconnor.net> >> Cc: Marcel Apfelbaum <marcel@redhat.com> >> Cc: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> >> Notes: >> 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 c7a085d..bed8ec9 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) >> @@ -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(SysBusDevice *dev) >> +{ >> + PCIHostState *host = PCI_HOST_BRIDGE(dev); >> + PCIBus *bus; >> + 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/pci@i0cf8", 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; >> +} > return pxb_a->bus_nr - pxb_b->bus_nr ? :) In this specific case, it would work, yes. The reason is that PXBDev.bus_nr has type "uint8_t" (== unsigned char), which is promoted (by the /integer promotions/) to "int", since on all the platforms that we care about, "an int can represent all values of the original type" (ie. those of unsigned char). The upshot is that you'd have val1_int - val2_int /* performed in "int" */ retval_int := difference where val1_int is in the range [0, 255] and val2_int is in the range [0, 255], hence their difference is in the range [-255, 255], which can be represented by an int on all platforms that we care about. So, yes, *in this specific case* what you propose would be safe. In general, however, I always use this pattern in comparator functions, because it is safe - against undefined behavior in integer expressions (example: (INT_MAX - (-1)) is undefined behavior), - and against implementation defined behavior in integer assignments (example: return (int)(1u - 2u) is implementation defined) > This will not hold the series... I can send a patch on top. I'd prefer if you didn't :) I didn't write the expression this way because I like the ternary operator that much (aside: I do like it very much, but that's not the cause here); it was a calculated decision instead. See above. ... When you proposed the subtraction, did you first consider the /integer promotions/, the /usual arithmetic conversions/, and the range of the difference (ie. [-255, 255])? :) Thanks Laszlo > Thanks, > Marcel > >> + >> static int pxb_dev_initfn(PCIDevice *dev) >> { >> PXBDev *pxb = PXB_DEV(dev); >> @@ -190,9 +218,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), >> @@ -206,6 +242,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; >> >
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c index c7a085d..bed8ec9 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) @@ -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(SysBusDevice *dev) +{ + PCIHostState *host = PCI_HOST_BRIDGE(dev); + PCIBus *bus; + 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/pci@i0cf8", 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); @@ -190,9 +218,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), @@ -206,6 +242,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;
SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file to follow the pattern /pci-root@N/pci@i0cf8/... 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 "pci@i0cf8" node text is hardcoded in SeaBIOS (see the macro FW_PCI_DOMAIN). Cc: Kevin O'Connor <kevin@koconnor.net> Cc: Marcel Apfelbaum <marcel@redhat.com> Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v4: - new in v4 hw/pci-bridge/pci_expander_bridge.c | 39 ++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)