Re: [Qemu-devel] [PATCH V2] pci: fixes to allow booting from extra root pci buses.

[Michael S. Tsirkin]

On Sun, Jun 14, 2015 at 10:50:22AM -0400, Kevin O'Connor wrote:
> On Sun, Jun 14, 2015 at 02:05:52PM +0200, Michael S. Tsirkin wrote:
> > On Fri, Jun 12, 2015 at 02:40:10PM -0400, Kevin O'Connor wrote:
> > > > (2) The QEMU command line and the effects the command line has on the
> > > > virtual hardware should not change. However, all of the following have
> > > > to be updated:
> > > > 
> > > > - the "explicit_ofw_unit_address" property assignments in
> > > >   pxb_dev_initfn() have to be done separately, after all PXBs have been
> > > >   seen, and sorted. This probably requires another "machine init done"
> > > >   notifier.
> > > 
> > > I admit the sorting of pxb objects just to reverse engineer what
> > > SeaBIOS expects would not be fun.  Doesn't QEMU have to sort the buses
> > > anyway to know which secondary bus ranges are associated with each
> > > root bus?
> > 
> > I guess it can be done - though this means it will break if
> > we ever support hot-plugging of these roots.
> 
> As I understand it, the use case for multiple PCI roots is large
> servers that process a lot of IO.

For better or worse, guest OS-es assume that numa locality
can only be specified for PCI roots.

So the use case is to specify numa locality for virtual
devices.


> For example, if you have a server
> with two 8-core cpu chips and two 40 gig ethernet cards, it might be a
> significant performance boost if the ethernet traffic from one card is
> on a different bus from the other card.  These systems (at least in
> the AMD case) have multiple southbridge chips on the motherboard that
> each implement a PCI bus and each southbridge chip is associated with
> a particular CPU.  This allows (in theory) a particular CPU to process
> the IO from it's associated PCI bus without IO contention.  I'm not an
> expert on this, but that's my general understanding.
> 
> I'm not aware of real world hardware with hot-plugable root buses.
> Should it come about then some kind of OS visible spec would be needed
> for the OS to identify and enumerate newly added buses, and I suppose
> we could figure out how to handle it once that type of thing happens.
> 
> > But more importantly, if the sort is by the bus number,
> > then how is it better than just using the bus number directly?
> 
> Coreboot supports real machines with multiple southbridge chips.  The
> number of southbridge chips on the motherboard is static, but the bus
> id associated with them is not.  So, SeaBIOS attempts to give a unique
> id to the southbridge chip that doesn't rely on the bus id (it's the
> N'th chip instead of the chip's current bus id).
> 
> I asked the coreboot developers about this again and they reiterated
> that bus id is dynamic and it could change between boots.
> 
> The SeaBIOS scheme is not perfect of course - for example nothing
> technically stops coreboot from assigning the bus ids in a different
> order from one boot to the next and if a bus doesn't show any devices
> on it at all then it would skew the ordering.  Neither happens in
> practice today.
> 
> All of the above aside, I'm confused why one would want to add the
> logical bus-id to the open firmware style physical topology.  We know
> the bus-id is a logical value in the general case - I find it
> confusing to use it in a description of physical topology.  To wit,
> what happens if OSes learn how to alter the bus id of extra root buses
> (eg, to support hot plugging them) - then we'd have built an interface
> that's not stable even within a single boot.
> 
> -Kevin

To summarise, you feel that modifying bus id without reordering
bus ids between roots is likely, modifications that would
cause reordering are unlikely, thus counting bus ids
in order gives a stable index. Is that right?

To be on the safe side, it would be nice to have bios skip some
fields/properties when parsing paths, so that if we want to use another
id in the future, we can supply both id types.  I haven't looked at the
parsing code - maybe it does this already?

-- 
MST