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

[Michael S. Tsirkin]

On Thu, Jun 11, 2015 at 12:48:22PM -0400, Kevin O'Connor wrote:
> On Thu, Jun 11, 2015 at 04:35:33PM +0200, Laszlo Ersek wrote:
> > On 06/11/15 15:58, Kevin O'Connor wrote:
> > > On Thu, Jun 11, 2015 at 04:37:08PM +0300, Marcel Apfelbaum wrote:
> > >> The fixes solves the following issue:
> > >> The PXB device exposes a new  pci root bridge with the
> > >> fw path:  /address@hidden/..., in which 4 is the root bus number.
> > >> Before this patch the fw path was wrongly computed:
> > >>     /address@hidden/address@hidden/...
> > >> Fix the above issues: Correct the bus number and remove the
> > >> extra host bridge description.
> > > 
> > > Why is that wrong?  The previous path looks correct to me.
> > > 
> > >> The IEEE Std 1275-1994:
> > >>
> > >>   IEEE Standard for Boot (Initialization Configuration)
> > >>     Firmware: Core Requirements and Practices
> > >>       3.2.1.1 Node names
> > >>           Each node in the device tree is identified by a node name
> > >>           using the following notation:
> > >>               address@hidden:device-arguments
> > >>
> > >>           The driver name field is a sequence of between one and 31
> > >>           letters [...]. By convention, this name includes the name of
> > >>           the device’s manufacturer and the device’s model name 
> > >> separated by
> > >>           a “,”.
> > >>
> > >>           The unit address field is the text representation of the
> > >>           physical address of the device within the address space
> > >>           defined by its parent node. The form of the text
> > >>           representation is bus-dependent.
> > > 
> > > Note the "physical address" part in the above.  Your patch changes the
> > > "pci-root@" syntax to use a logical address instead of a physical
> > > address.  That is, unless I've missed something, SeaBIOS today uses a
> > > physical address (the n'th root bus) and the patch would change it to
> > > use a logical address.
> > > 
> > > One of the goals of using an "openfirmware" like address was so that
> > > they would be stable across boots (the same mechanism is also used
> > > with coreboot).  Using a physical address is key for this, because
> > > simply adding or removing a PCI device could cause the logical PCI
> > > bridge enumeration to change - and that would mess up the bootorder
> > > list if it was based on logical addresses.
> > 
> > There are two questions here. The first is the inclusion of the
> > "address@hidden" node even if a "address@hidden" node is present in front 
> > of it.
> > The hunk that changes that is not your main concern, right? (And Marcel
> > just described that hunk in more detail.)
> > 
> > The other question is how "x" is selected in "address@hidden".
> > 
> > On the QEMU side, and in OVMF, "x" is keyed off of the bus_nr property.
> > If you change that property from (say) 3 to 4, then the device paths
> > exported by QEMU will change. However, the location (in the PCI
> > hierarchy) of all the affected devices will *also* change at once, and
> > their auto-enumerated, firmware-side device paths will reflect that.
> > Therefore the new "bootorder" fw_cfg entries will match the freshly
> > generated firmware-side device paths.
> > 
> > So why is this not stable? If you change the hardware without
> > automatically updating any stashed firmware-side device paths, then
> > things will fall apart without "bootorder" entries in the picture anyway.
> > 
> > Also, assuming you key off "x" of the running counter that counts root
> > buses as they are found during enumeration, that's a possibility too,
> > but I don't see how it gives more stability. If you insert a new root
> > bus (with a device on it) between to preexistent ones, that will offset
> > all the "x" values for the root buses that come after it by one.
> 
> The SeaBIOS code is used on both virtual machines and real machines.
> The bus number is something that is generated by software and it is
> not assured to be stable between boots.  (For example, if someone adds
> a PCI device to their machine between boots then every bus number in
> the system might be different on the next boot.)  The open firmware
> paths go to great length to avoid arbitrary bus numbers today - for
> example:
> 
> /address@hidden/address@hidden/address@hidden,2/address@hidden/address@hidden/address@hidden/address@hidden,0
> 
> Given the complexity to avoid arbitrary bus numbers I'm confused why
> one would want to add them.

Could you give an example real-hardware path when there are multiple
roots though?
I'd like to make sure what qemu generates matches that.


> > In UEFI at least (I'm not speaking about OVMF in particular, but the
> > UEFI spec), there is a "short-form device path" concept for hard drive
> > and USB boot options. For hard disks, it is practically a relative
> > device path that lacks the path fragment from the root node until just
> > before the GPT partition identifier. The idea being, if you plug your
> > SCSI controller in another PCI slot, the change in the full device path
> > will be local to the path fragment that is not captured in the
> > (persistent) boot option. The GPT GUID can identify the partition
> > uniquely in the system wherever it exists, so it can be booted even
> > without fully enumerating all devices and reproducing all the default
> > boot options.
> > 
> > Short of such a "uniquely identifying relative devpath" trick, I don't
> > think stability in firmware-stashed (ie. not regenerated) device paths
> > exists in general, if the underlying hardware configuration is changed.
> 
> I'm not sure why you say that - it works just fine.  The open firmware
> device paths relate a physical path to the given hardware and as long
> as one doesn't alter that physical path it will be the same path on
> every boot.  (Specifically, one can add or remove unrelated PCI
> devices, USB devices, etc. without impacting the open firmware paths
> to devices not modified.)
> 
> > In summary: I think we could modify both QEMU and OVMF to use the
> > "serial numbers" of the extra PCI root buses, in increasing bus number
> > order, instead of their actual bus numbers, for identifying them. That's
> > just a convention. Then the second hunk of this patch would not be
> > necessary for SeaBIOS. But I think this convention would be only less
> > logical, and not more stable.
> > 
> > Can you please elaborate? I'm confused.
> 
> -Kevin