| Message ID | 1500761510-1556-5-git-send-email-zuban32s@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Jul 23, 2017 at 01:11:50AM +0300, Aleksandr Bezzubikov wrote: > In case of Red Hat PCI bridges reserve additional buses, which number is provided > in a vendor-specific capability. > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> > --- > src/fw/pciinit.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c > index 864954f..f05a8b9 100644 > --- a/src/fw/pciinit.c > +++ b/src/fw/pciinit.c > @@ -15,6 +15,7 @@ > #include "hw/pcidevice.h" // pci_probe_devices > #include "hw/pci_ids.h" // PCI_VENDOR_ID_INTEL > #include "hw/pci_regs.h" // PCI_COMMAND > +#include "hw/pci_cap.h" // qemu_pci_cap > #include "list.h" // struct hlist_node > #include "malloc.h" // free > #include "output.h" // dprintf > @@ -578,9 +579,18 @@ pci_bios_init_bus_rec(int bus, u8 *pci_bus) > pci_bios_init_bus_rec(secbus, pci_bus); > > if (subbus != *pci_bus) { > + u8 res_bus = 0; > + if (pci_config_readw(bdf, PCI_VENDOR_ID) == PCI_VENDOR_ID_REDHAT) { Check device ID as well. > + u8 cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, 0); There could be multiple vendor capabilities. You want to scan them all. > + if (cap) { > + res_bus = pci_config_readb(bdf, > + cap + offsetof(struct redhat_pci_bridge_cap, > + bus_res)); You might want to add sanity checks e.g. overflow, and capability length. Also, if all you use is offsetof, don't bother with a struct, just add some defines. > + } > + } > dprintf(1, "PCI: subordinate bus = 0x%x -> 0x%x\n", > - subbus, *pci_bus); > - subbus = *pci_bus; > + subbus, *pci_bus + res_bus); > + subbus = *pci_bus + res_bus; So you take all present devices and add reserved ones - is that it? If so it looks like this will steal extra buses each time you add a child bus and reboot. > } else { > dprintf(1, "PCI: subordinate bus = 0x%x\n", subbus); > } > -- > 2.7.4
2017-07-23 5:49 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>: > On Sun, Jul 23, 2017 at 01:11:50AM +0300, Aleksandr Bezzubikov wrote: > > In case of Red Hat PCI bridges reserve additional buses, which number is > provided > > in a vendor-specific capability. > > > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> > > --- > > src/fw/pciinit.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c > > index 864954f..f05a8b9 100644 > > --- a/src/fw/pciinit.c > > +++ b/src/fw/pciinit.c > > @@ -15,6 +15,7 @@ > > #include "hw/pcidevice.h" // pci_probe_devices > > #include "hw/pci_ids.h" // PCI_VENDOR_ID_INTEL > > #include "hw/pci_regs.h" // PCI_COMMAND > > +#include "hw/pci_cap.h" // qemu_pci_cap > > #include "list.h" // struct hlist_node > > #include "malloc.h" // free > > #include "output.h" // dprintf > > @@ -578,9 +579,18 @@ pci_bios_init_bus_rec(int bus, u8 *pci_bus) > > pci_bios_init_bus_rec(secbus, pci_bus); > > > > if (subbus != *pci_bus) { > > + u8 res_bus = 0; > > + if (pci_config_readw(bdf, PCI_VENDOR_ID) == > PCI_VENDOR_ID_REDHAT) { > > Check device ID as well. Not sure what ID/IDs we want to see here exactly. Now it can be only pcie-root-port (0xC then), but > > > + u8 cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, 0); > > There could be multiple vendor capabilities. You want to scan them all. > > > > + if (cap) { > > + res_bus = pci_config_readb(bdf, > > + cap + offsetof(struct redhat_pci_bridge_cap, > > + bus_res)); > > > You might want to add sanity checks e.g. overflow, and capability > length. > > Also, if all you use is offsetof, don't bother with a struct, just > add some defines. > OK, will move this to defines. > > > + } > > + } > > dprintf(1, "PCI: subordinate bus = 0x%x -> 0x%x\n", > > - subbus, *pci_bus); > > - subbus = *pci_bus; > > + subbus, *pci_bus + res_bus); > > + subbus = *pci_bus + res_bus; > > So you take all present devices and add reserved ones - is that it? > If so it looks like this will steal extra buses each time you > add a child bus and reboot. > You're right, such problem persists. Will think on what can be done here. > > > > } else { > > dprintf(1, "PCI: subordinate bus = 0x%x\n", subbus); > > } > > -- > > 2.7.4 >
By the way, any ideas on how to avoid 'bus overstealing' would be greatly appreciated. Static BIOS variable isn't applicable since its value isn't saved across reboots. 2017-07-23 19:43 GMT+03:00 Alexander Bezzubikov <zuban32s@gmail.com>: > 2017-07-23 5:49 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>: > >> On Sun, Jul 23, 2017 at 01:11:50AM +0300, Aleksandr Bezzubikov wrote: >> > In case of Red Hat PCI bridges reserve additional buses, which number >> is provided >> > in a vendor-specific capability. >> > >> > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> >> > --- >> > src/fw/pciinit.c | 14 ++++++++++++-- >> > 1 file changed, 12 insertions(+), 2 deletions(-) >> > >> > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c >> > index 864954f..f05a8b9 100644 >> > --- a/src/fw/pciinit.c >> > +++ b/src/fw/pciinit.c >> > @@ -15,6 +15,7 @@ >> > #include "hw/pcidevice.h" // pci_probe_devices >> > #include "hw/pci_ids.h" // PCI_VENDOR_ID_INTEL >> > #include "hw/pci_regs.h" // PCI_COMMAND >> > +#include "hw/pci_cap.h" // qemu_pci_cap >> > #include "list.h" // struct hlist_node >> > #include "malloc.h" // free >> > #include "output.h" // dprintf >> > @@ -578,9 +579,18 @@ pci_bios_init_bus_rec(int bus, u8 *pci_bus) >> > pci_bios_init_bus_rec(secbus, pci_bus); >> > >> > if (subbus != *pci_bus) { >> > + u8 res_bus = 0; >> > + if (pci_config_readw(bdf, PCI_VENDOR_ID) == >> PCI_VENDOR_ID_REDHAT) { >> >> Check device ID as well. > > Not sure what ID/IDs we want to see here exactly. Now it can be > only pcie-root-port (0xC then), but > > >> >> > + u8 cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, 0); >> >> There could be multiple vendor capabilities. You want to scan them all. >> >> >> > + if (cap) { >> > + res_bus = pci_config_readb(bdf, >> > + cap + offsetof(struct >> redhat_pci_bridge_cap, >> > + bus_res)); >> >> >> You might want to add sanity checks e.g. overflow, and capability >> length. >> >> Also, if all you use is offsetof, don't bother with a struct, just >> add some defines. >> > OK, will move this to defines. > > >> >> > + } >> > + } >> > dprintf(1, "PCI: subordinate bus = 0x%x -> 0x%x\n", >> > - subbus, *pci_bus); >> > - subbus = *pci_bus; >> > + subbus, *pci_bus + res_bus); >> > + subbus = *pci_bus + res_bus; >> >> So you take all present devices and add reserved ones - is that it? >> If so it looks like this will steal extra buses each time you >> add a child bus and reboot. >> > > You're right, such problem persists. Will think on what can be done here. > > >> >> >> > } else { >> > dprintf(1, "PCI: subordinate bus = 0x%x\n", subbus); >> > } >> > -- >> > 2.7.4 >> > > > > -- > Alexander Bezzubikov >
On Sun, 2017-07-23 at 22:44 +0300, Alexander Bezzubikov wrote: > By the way, any ideas on how to avoid 'bus overstealing' would > be greatly appreciated. > Static BIOS variable isn't applicable since its value isn't saved > across reboots. I think the reservation hints should be a absolute number, not a increment. i.e. if qemu suggests to reserve three extra bus numbers seabios should reserve three, no matter whenever there are zero, one, two or three child busses present. And I guess seabios should interpret that as minimum, so in case it finds five child busses it will allocate five bus numbers of course ... Same with the other limit hints. If the hint says to allocate 16M, and existing device bars sum up to 4M, allocate 16M (and therefore leave 12M address space for hotplug). If the device bars sum up to 32M, allocate that. While being at it: I have my doubts the capability struct layout (which mimics register layout) buys us that much, seabios wouldn't blindly copy over the values anyway. Having regular u32 fields looks more useful to me. cheers, Gerd
2017-07-24 12:42 GMT+03:00 Gerd Hoffmann <kraxel@redhat.com>: > On Sun, 2017-07-23 at 22:44 +0300, Alexander Bezzubikov wrote: > > By the way, any ideas on how to avoid 'bus overstealing' would > > be greatly appreciated. > > Static BIOS variable isn't applicable since its value isn't saved > > across reboots. > > I think the reservation hints should be a absolute number, not a > increment. i.e. if qemu suggests to reserve three extra bus numbers > seabios should reserve three, no matter whenever there are zero, one, > two or three child busses present. And I guess seabios should > interpret that as minimum, so in case it finds five child busses it > will allocate five bus numbers of course ... > Personally I have nothing against it. Marcel, Michael, what do you think? > > Same with the other limit hints. If the hint says to allocate 16M, and > existing device bars sum up to 4M, allocate 16M (and therefore leave > 12M address space for hotplug). If the device bars sum up to 32M, > allocate that. > > While being at it: I have my doubts the capability struct layout > (which mimics register layout) buys us that much, seabios wouldn't > blindly copy over the values anyway. Having regular u32 fields looks > more useful to me. > > Again, if nobody has any objections, I can change it in v3. > cheers, > Gerd > >
On 24/07/2017 17:39, Alexander Bezzubikov wrote: > 2017-07-24 12:42 GMT+03:00 Gerd Hoffmann <kraxel@redhat.com > <mailto:kraxel@redhat.com>>: > > On Sun, 2017-07-23 at 22:44 +0300, Alexander Bezzubikov wrote: > > By the way, any ideas on how to avoid 'bus overstealing' would > > be greatly appreciated. > > Static BIOS variable isn't applicable since its value isn't saved > > across reboots. > > I think the reservation hints should be a absolute number, not a > increment. i.e. if qemu suggests to reserve three extra bus numbers > seabios should reserve three, no matter whenever there are zero, one, > two or three child busses present. And I guess seabios should > interpret that as minimum, so in case it finds five child busses it > will allocate five bus numbers of course ... > > > Personally I have nothing against it. Marcel, Michael, what do you think? Sounds good to me. > > > Same with the other limit hints. If the hint says to allocate 16M, and > existing device bars sum up to 4M, allocate 16M (and therefore leave > 12M address space for hotplug). If the device bars sum up to 32M, > allocate that. > > While being at it: I have my doubts the capability struct layout > (which mimics register layout) buys us that much, seabios wouldn't > blindly copy over the values anyway. Having regular u32 fields looks > more useful to me. > > > Again, if nobody has any objections, I can change it in v3. > I also had my reservations about prev layout, let's see if it will look cleaner. Thanks, Marcel > cheers, > Gerd > > > > > -- > Alexander Bezzubikov
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index 864954f..f05a8b9 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -15,6 +15,7 @@ #include "hw/pcidevice.h" // pci_probe_devices #include "hw/pci_ids.h" // PCI_VENDOR_ID_INTEL #include "hw/pci_regs.h" // PCI_COMMAND +#include "hw/pci_cap.h" // qemu_pci_cap #include "list.h" // struct hlist_node #include "malloc.h" // free #include "output.h" // dprintf @@ -578,9 +579,18 @@ pci_bios_init_bus_rec(int bus, u8 *pci_bus) pci_bios_init_bus_rec(secbus, pci_bus); if (subbus != *pci_bus) { + u8 res_bus = 0; + if (pci_config_readw(bdf, PCI_VENDOR_ID) == PCI_VENDOR_ID_REDHAT) { + u8 cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, 0); + if (cap) { + res_bus = pci_config_readb(bdf, + cap + offsetof(struct redhat_pci_bridge_cap, + bus_res)); + } + } dprintf(1, "PCI: subordinate bus = 0x%x -> 0x%x\n", - subbus, *pci_bus); - subbus = *pci_bus; + subbus, *pci_bus + res_bus); + subbus = *pci_bus + res_bus; } else { dprintf(1, "PCI: subordinate bus = 0x%x\n", subbus); }
In case of Red Hat PCI bridges reserve additional buses, which number is provided in a vendor-specific capability. Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> --- src/fw/pciinit.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)