[PATCH] pci: panic when out of bus numbers

[Marcel Apfelbaum]

Currently the bios goes into an endless loop if more than
255 PCI buses are requested.

Change that to panic which is preferred in this case.

Signed-off-by: Marcel Apfelbaum 
---

Hi,

I am well aware that this is not a common scenario, but the bug was found
and maybe it deserves a little attention.

I opted for using panic because I saw it used for other scenarios
where the bios is out of resources.

Another way to look at this would be to *stop* registering buses
over 255 and loose some of the devices, but bring the system up.

Thanks,
Marcel

src/fw/pciinit.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index c31c2fa..5da6cf6 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -469,6 +469,9 @@ pci_bios_init_bus_rec(int bus, u8 *pci_bus)

u8 secbus = pci_config_readb(bdf, PCI_SECONDARY_BUS);
         (*pci_bus)++;
+        if (!(*pci_bus)) {
+            panic("PCI: out of bus numbers!\n");
+        }
         if (*pci_bus != secbus) {
             dprintf(1, "PCI: secondary bus = 0x%x -> 0x%x\n",
                     secbus, *pci_bus);

[Marcel Apfelbaum]

On 01/04/2016 01:43 PM, Marcel Apfelbaum wrote:
> Currently the bios goes into an endless loop if more than
> 255 PCI buses are requested.

Hi,

This is the reproducing script:

cli="<qemu-bin> -M q35 <img>"

while [ ${i:=0} -lt ${1:-0} ]
do
     dstreamId=$((i))
     ustreamId=$((dstreamId/32))
     chassisId=$((dstreamId+1))
     blkDiskId=$((dstreamId))

if [ $((dstreamId%32)) -eq 0 ]
     then
         cli="$cli -device ioh3420,bus=pcie.0,id=root.$ustreamId,slot=$ustreamId"
         cli="$cli -device x3130-upstream,bus=root.$ustreamId,id=upstream$ustreamId"
     fi

cli="$cli -device xio3130-downstream,bus=upstream$ustreamId,id=downstream$dstreamId,chassis=$chassisId"
     i=$((i+1))
done

$cli

Run it with a numeric parameter > 240.

Thanks,
Marcel
> Change that to panic which is preferred in this case.
> Signed-off-by: Marcel Apfelbaum
>
> Hi,
>
> I am well aware that this is not a common scenario, but the bug was found
> and maybe it deserves a little attention.
>
> I opted for using panic because I saw it used for other scenarios
> where the bios is out of resources.
>
> Another way to look at this would be to *stop* registering buses
> over 255 and loose some of the devices, but bring the system up.
>
> Thanks,
> Marcel
>
> src/fw/pciinit.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index c31c2fa..5da6cf6 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -469,6 +469,9 @@ pci_bios_init_bus_rec(int bus, u8 *pci_bus)
>       u8 secbus = pci_config_readb(bdf, PCI_SECONDARY_BUS);
>       (*pci_bus)++;
>    if (!(*pci_bus)) {
>        panic("PCI: out of bus numbers!\n");
>    }
>     if (*pci_bus != secbus) {
>         dprintf(1, "PCI: secondary bus = 0x%x -> 0x%x\n",
>                 secbus, *pci_bus);

[Gerd Hoffmann]

On Mo, 2016-01-04 at 13:43 +0200, Marcel Apfelbaum wrote:
> Currently the bios goes into an endless loop if more than
> 255 PCI buses are requested.

Given the bus number register is 8bit I'm wondering whenever this is a
valid hardware configuration in the first place?

In case it isn't I think qemu should throw an error instead if you try
to create a vm with more than 255 pci busses.

cheers,
  Gerd

[Marcel Apfelbaum]

On 01/04/2016 02:19 PM, Gerd Hoffmann wrote:
> On Mo, 2016-01-04 at 13:43 +0200, Marcel Apfelbaum wrote:
> > Currently the bios goes into an endless loop if more than
> > 255 PCI buses are requested.

Hi Gerd,
Thanks for looking into this.
> Given the bus number register is 8bit I'm wondering whenever this is a
> valid hardware configuration in the first place?

For sure no :), however we do have a possible endless loop
and maybe is cleaner to panic. (no matter who is "responsible")
> In case it isn't I think qemu should throw an error instead if you try
> to create a vm with more than 255 pci busses.

I suppose it could go into QEMU too, I'll give it a try.
Thanks,
Marcel
> cheers,
>    Gerd

[Peter Stuge]

Marcel Apfelbaum wrote:
> > Given the bus number register is 8bit I'm wondering whenever this is a
> > valid hardware configuration in the first place?
>
> For sure no :), however we do have a possible endless loop
> and maybe is cleaner to panic. (no matter who is "responsible")

I would prefer SeaBIOS to discard invalid requests and boot anyway.
> > In case it isn't I think qemu should throw an error instead if you try
> > to create a vm with more than 255 pci busses.
>
> I suppose it could go into QEMU too, I'll give it a try.

Please do, I think the check should be in both places. I think QEMU
should throw an error if the user requests an invalid hardware
configuration.

//Peter