Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB

[Laszlo Ersek]

On 06/18/15 15:40, Michael S. Tsirkin wrote:
> On Thu, Jun 18, 2015 at 03:22:59PM +0200, Laszlo Ersek wrote:
>> On 06/17/15 23:50, Michael S. Tsirkin wrote:
>>> On Wed, Jun 17, 2015 at 09:44:07PM +0200, Laszlo Ersek wrote:
>>>> On 06/17/15 21:32, Michael S. Tsirkin wrote:
>>>>> On Wed, Jun 17, 2015 at 03:28:44PM -0400, Kevin O'Connor wrote:
>>>>>> On Wed, Jun 17, 2015 at 09:15:24PM +0200, Laszlo Ersek wrote:
>>>>>>> On 06/17/15 20:54, Michael S. Tsirkin wrote:
>>>>>>>> Right. But what I was discussing is a different issue.  The point is
>>>>>>>> that it does not make sense to have /address@hidden under two 
>>>>>>>> hierarchies:
>>>>>>>> it's the same register.  What happens is that you access 
>>>>>>>> /address@hidden and
>>>>>>>> then *through that* you access another pci root.  Not the other way
>>>>>>>> around.  The proposal thus is to switch to 
>>>>>>>> /address@hidden/address@hidden in
>>>>>>>> seabios,
>>>>>>>
>>>>>>> For me this is still Question 1 -- 'everything in that pattern that is
>>>>>>> not "N"'.
>>>>>>>
>>>>>>> You seem to care about the *semantics* of that OFW device path fragment.
>>>>>>> I don't. First, the relevant IEEE spec is prohibitively hard for me to
>>>>>>> interpret semantically. Second, there is no known firmware that actually
>>>>>>> looks at the "i0cf8" unit-address term and decides *based on that term*
>>>>>>> that it has to talk PCI via 0xCF8 and 0xCFC. In other words, the current
>>>>>>> second node is entirely opaque in my interpretation.
>>>>>>>
>>>>>>>> unconditionally - not if (QEMU).
>>>>>>>
>>>>>>> This might qualify as some kind of semantic cleanup, but it will
>>>>>>> nonetheless break the SeaBIOS boot options expressed in OFW notation
>>>>>>> that are already persistently stored in cbfs, on physical machines. (As
>>>>>>> far as I understood.) It might not break the Coreboot-SeaBIOS interface,
>>>>>>> but it might invalidate preexistent entries that exist in the prior form
>>>>>>> (wherever they exist on physical hardware).
>>>>>>>
>>>>>>>> And I thought Kevin agreed
>>>>>>>> it's a good idea.
>>>>>>>>
>>>>>>>> Kevin - is this a good summary of your opinion?
>>>>>>>
>>>>>>> Kevin, please do answer.
>>>>>>
>>>>>> It is true that it would "invalidate preexistent entries" for
>>>>>> coreboot/seabios users that upgrade, but I think that is manageable.
>>>>>> So I defer the syntax discussion and decisions to the QEMU developers
>>>>>> that are doing the bulk of the work.
>>>>>>
>>>>>> -Kevin
>>>>>
>>>>> I'm fine with either /address@hidden,%x or 
>>>>> /address@hidden/address@hidden, with a
>>>>> slight preference to the later - in particular it's easier
>>>>> to implement in QEMU.
>>>>>
>>>>> It means old bios won't boot from a pxb, but I think that's
>>>>> manageable - it works otherwise.
>>>>
>>>> I don't understand -- the second option you named
>>>> ("/address@hidden/address@hidden") is what this patch implements, and 
>>>> "old" (ie.
>>>> current) SeaBIOS does boot from it.
>>>>
>>>> Laszlo
>>>
>>> Ouch. I meant /address@hidden//address@hidden
>>> As you see, it's confusing.
>>
>> If you insist on /address@hidden/address@hidden, then all of SeaBIOS, QEMU, 
>> and
>> OVMF must be (further) modified. Please confirm if this is what you'd like.
>>
>> (As I said, IMO this change is not warranted for; it just replaces one
>> opaque string with another opaque string, without semantic effects, but
>> it causes extra churn and even requires a patch for SeaBIOS.)
>>
>> Laszlo
> 
> I think I prefer the string to match the actual hierarchy (using any
> syntax), yes. Current guests don't seem to care but this needs to
> be maintained forever, worth being as correct as we can be.

Alright. When I find the drive in myself to do so, I'll post a v7 with
patches v6 #1 through #4 included, addressing your pci-bridge comments
on top. (If Marcel would prefer to take over those patches immediately,
I'm game.)

Patch #5 you have already included in a pull request, Cc'ing stable;
thanks for that.

Patches #6 and #7 I am hereby dropping (the boot order sub-feature). I
might revoke the related OVMF-side patches from my latest (v2) OVMF
series, or just let them go in in their current form. Once this
sub-feature is sorted out between QEMU and SeaBIOS, I might revisit the
related OVMF patches. Since we discussed this topic several times over,
I trust whatever we'll find in QEMU at that point shall be possible to
support in OVMF.

I don't necessarily want to sneak patches #6 and #7 onto Marcel's plate
-- because they are a feature not intimately related to the expander
bridge's core functionality --, so I guess #6 and #7 are free for the
taking, for anyone who cares enough (including you).

Thanks
Laszlo