Re: [Qemu-devel] [RFC PATCH v2 4/4] pci: enable RedHat PCI bridges to reserve additional buses on PCI init

["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.

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