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

[Laszlo Ersek]

On 06/11/15 18:48, 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

Not the root bus numbers, as far as I understand.

(Please see the rest of my reply in the other sub-thread.)

Thanks
Laszlo

> 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.
> 
>> 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
> 
> _______________________________________________
> SeaBIOS mailing list
> address@hidden
> http://www.seabios.org/mailman/listinfo/seabios
>