Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Michael S. Tsirkin]

On Tue, Jul 25, 2017 at 12:41:12AM +0300, Alexander Bezzubikov wrote:
> 2017-07-24 23:46 GMT+03:00 Michael S. Tsirkin <address@hidden>:
> > On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote:
> >> On 23/07/2017 15:22, Michael S. Tsirkin wrote:
> >> > On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote:
> >> > > To enable hotplugging of a newly created pcie-pci-bridge,
> >> > > we need to tell firmware (SeaBIOS in this case)
> >> >
> >>
> >> Hi Michael,
> >>
> >> > Presumably, EFI would need to support this too?
> >> >
> >>
> >> Sure, Eduardo added to CC, but he is in PTO now.
> >>
> >> > > to reserve
> >> > > additional buses for pcie-root-port, that allows us to
> >> > > hotplug pcie-pci-bridge into this root port.
> >> > > The number of buses to reserve is provided to the device via a 
> >> > > corresponding
> >> > > property, and to the firmware via new PCI capability (next patch).
> >> > > The property's default value is 1 as we want to hotplug at least 1 
> >> > > bridge.
> >> >
> >> > If so you should just teach firmware to allocate one bus #
> >> > unconditionally.
> >> >
> >>
> >> That would be a problem for the PCIe machines, since each PCIe
> >> devices is plugged in a different bus and we are already
> >> limited to 256 PCIe devices. Allocating an extra-bus always
> >> would really limit the PCIe devices we can use.
> >
> > But this is exactly what this patch does as the property is added to all
> > buses and default to 1 (1 extra bus).
> >
> >> > But why would that be so? What's wrong with a device
> >> > directly in the root port?
> >> >
> >>
> >> First, plugging a legacy PCI device into a PCIe Root Port
> >> looks strange at least, and it can;t be done on real HW anyway.
> >> (incompatible slots)
> >
> > You can still plug in PCIe devices there.
> >
> >> Second (and more important), if we want 2 or more PCI
> >> devices we would loose both IO ports space and bus numbers.
> >
> > What I am saying is maybe default should not be 1.
> 
> The only sensible variant left is 0.
> But as we want pcie-pci-bridge to be used for every legacy PCI device
> on q35 machine, every time one hotplugs the bridge into the root port,
> he must be sure rp's prop value >0 (for Linux). I'm not sure
> that it is a very convenient way to utilize the bridge - always remember
> to set property.

That's what I'm saying then - if in your opinion default is >0 anyway,
tweak firmware to do it by default.

> Another way - we can set this to 0 by default, and to 1 for pcie-root-port,
> and recommend to use it for hotplugging of the pcie-pci-bridge itself.

I wonder about something: imagine hotplugging a hierarchy of bridges
below a root port. It seems that nothing prevents guest from finding a
free range of buses to cover this hierarchy and setting that as
secondary/subordinate bus for this bridge.

This does need support on QEMU side to hotplug a hierarchy at once,
and might need some fixes in Linux, on the plus side you can defer
management decision on how many are needed until you are actually
adding something, and you don't need vendor specific patches.


> >
> >> >
> >> > >
> >> > > Signed-off-by: Aleksandr Bezzubikov <address@hidden>
> >> > > ---
> >> > >   hw/pci-bridge/pcie_root_port.c | 1 +
> >> > >   include/hw/pci/pcie_port.h     | 3 +++
> >> > >   2 files changed, 4 insertions(+)
> >> > >
> >> > > diff --git a/hw/pci-bridge/pcie_root_port.c 
> >> > > b/hw/pci-bridge/pcie_root_port.c
> >> > > index 4d588cb..b0e49e1 100644
> >> > > --- a/hw/pci-bridge/pcie_root_port.c
> >> > > +++ b/hw/pci-bridge/pcie_root_port.c
> >> > > @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
> >> > >   static Property rp_props[] = {
> >> > >       DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
> >> > >                       QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> >> > > +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
> >> > >       DEFINE_PROP_END_OF_LIST()
> >> > >   };
> >> > > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> >> > > index 1333266..1b2dd1f 100644
> >> > > --- a/include/hw/pci/pcie_port.h
> >> > > +++ b/include/hw/pci/pcie_port.h
> >> > > @@ -34,6 +34,9 @@ struct PCIEPort {
> >> > >       /* pci express switch port */
> >> > >       uint8_t     port;
> >> > > +
> >> > > +    /* additional buses to reserve on firmware init */
> >> > > +    uint8_t     bus_reserve;
> >> > >   };
> >> > >   void pcie_port_init_reg(PCIDevice *d);
> >> >
> >> > So here is a property and it does not do anything.
> >> > It makes it easier to work on series maybe, but review
> >> > is harder since we do not see what it does at all.
> >> > Please do not split up patches like this - you can maintain
> >> > it split up in your branch if you like and merge before sending.
> >> >
> >>
> >> Agreed, Alexandr please merge patches 4-5-6 for your next submission.
> >>
> >> Thanks,
> >> Marcel
> >>
> >>
> >> > > --
> >> > > 2.7.4
> 
> 
> 
> -- 
> Alexander Bezzubikov