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

[Aleksandr Bezzubikov]

In case of Red Hat PCI bridges reserve additional buses, which number is 
provided
in a vendor-specific capability. 

Signed-off-by: Aleksandr Bezzubikov 
---
 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) {
+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);
 }
-- 
2.7.4

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

[Michael S. Tsirkin]

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

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

[Alexander Bezzubikov]

2017-07-23 5:49 GMT+03:00 Michael S. Tsirkin :

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

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

[Alexander Bezzubikov]

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 :

> 2017-07-23 5:49 GMT+03:00 Michael S. Tsirkin :
>
>> 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 
>> > ---
>> >  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
>



-- 
Alexander Bezzubikov

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

[Gerd Hoffmann]

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

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

[Alexander Bezzubikov]

2017-07-24 12:42 GMT+03:00 Gerd Hoffmann :

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


-- 
Alexander Bezzubikov

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

[Marcel Apfelbaum]

On 24/07/2017 17:39, Alexander Bezzubikov wrote:
2017-07-24 12:42 GMT+03:00 Gerd Hoffmann >:


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