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

[Laszlo Ersek]

On 06/17/15 17:05, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 04:45:45PM +0200, Laszlo Ersek wrote:
>> On 06/17/15 16:18, Kevin O'Connor wrote:
>>> On Wed, Jun 17, 2015 at 03:57:36PM +0200, Michael S. Tsirkin wrote:
>>>> On Wed, Jun 17, 2015 at 02:45:05PM +0200, Laszlo Ersek wrote:
>>>>> SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
>>>>> to follow the pattern
>>>>>
>>>>>   /address@hidden/address@hidden/...
>>>>
>>>> It's kind of crazy, isn't it?
>>>> /address@hidden/address@hidden would make some sense: access rootN through 
>>>> cf8.
>>>>
>>>> But if bios needs to keep this for compatibility, maybe
>>>> we have too, to. Kevin?
>>>
>>> I have no issue with changing the string in SeaBIOS.  In a previous
>>> email we discussed "/address@hidden/address@hidden/" as well as
>>> "/address@hidden,%x/", but anything that makes sense is fine with me.
>>
>> It is not fine with me.
>>
>> Every time there is another idea about this format, I get to update and
>> repost the OVMF series (consisting of 24 patches), which of course
>> nobody on qemu-devel@ and seabios@ cares about, while it is actually the
>> *only* thing that matters to me. Plus, this patch appeared in v4 and has
>> been reposted without changes twice.
>>
>> Honestly, the format looks outright retarded to me, but I didn't
>> complain, because adopting it (and not patching SeaBIOS at all) was the
>> most direct way forward. (Most direct in the sense that we're now at
>> v6.) I will *not* repeat the entire discussion about the format, and I
>> won't revisit that outcome. I have spent several nights and weekend days
>> on implementing SeaBIOS-compatible code in qemu and OVMF, and I won't go
>> back on that work.
>>
>> Similarly, the patch "hw/pci-bridge: create interrupt-less, hotplug-less
>> bridge for PXB" has been present in the QEMU series without functional
>> changes since v2. I've been aware that it doesn't meet Michael's taste
>> (that fact was documented in v2), but I'm appalled that it has taken 4
>> reposts (v3 to v6) to arrive at specifics. Not only did that cause me to
>> miss 4 opportunities to post an ultimately acceptable patch, it also
>> wasted the reviews of Marcel and Markus, plus my work to address
>> Markus's review.
>>
>> I've been going out of my way to be cooperative, responsive, and just do
>> whatever I've been told, minimize the impact, etc. As I said, I'm
>> willing to post a v7 for the SHPC-less pci-bridge device model, but no
>> more versions, and no other changes.
>>
>> Thanks
>> Laszlo
> 
> Sorry you feel bad.  Looks like the patches are pretty close to being
> ready.

I apologize about losing my temper. I had made a resolution to be as
patient as I can (or, preferably, more patient than I can be :)) on
qemu-devel. Looks like I lasted until v6. Sorry about not lasting
longer; it's been a very taxing development for me.

> If you just address the comments about the bridge then I can merge
> patches 1-5 directly.

Thanks. I'll do my best to address your bridge-related comments in v7.

> We do need to agree about the correct paths however, this is host/guest
> interface which we have to maintain forever, and it's important to get
> it right. I kept hoping we can come up with something saner than
> the sequence # but oh well. Do you disagree with the statement
> that seabios path is currently incorrect? Kevin seems to agree.

As discussed earlier, there are two questions to consider about the OFW
devpath pattern

  /address@hidden/address@hidden/...

that SeaBIOS currently recognizes for devices that reside behind extra
PCI root buses.

Q1: everything in that pattern that is not "N"
Q2: what goes into N

These are independent questions.

Marcel's patch for SeaBIOS intended to change SeaBIOS's behavior for
both Q1 and Q2:

- For Q1, the proposed OFW devpath fragment was

  /address@hidden/...

  ie. dropping the second node ("address@hidden").

- For Q2, the proposed change was: instead of making N a *serial number*
(where N stood for the N'th extra root bus discovered by SeaBIOS), make
N an actual bus number.

However, applying these changes unconditionally would have broken the
interface between Coreboot and SeaBIOS, in physical hardware
environments (because Coreboot agrees with SeaBIOS on the current syntax
(Q1) and interpretation (Q2) of the devpath pattern). Therefore there
was an idea to make both SeaBIOS changes conditional on runningOnQemu().

As far as I remember, Kevin was more or less okay with that, but (again,
as far as I remember) he did express a mild dislike for such tweaks.

Reluctantly, I agreed, for two reasons: first, it's never a good long
term investment to get a maintainer to commit something he dislikes;
second, such a tweak would have introduced yet another version
dependency between QEMU and SeaBIOS. So I decided to try to write code
for QEMU that is compatible with SeaBIOS out of the box.

Patches #6 and #7 in this v6 series are that code.

To answer your question whether I think the "seabios path is currently
incorrect": that was my initial impression, yes. My opinion is now
different. I think:

- Re Q1: it's just static syntax. No need to bother. Just keep SeaBIOS
  happy if we can. (And yes, we can.)

- Re Q2: using N in the sense of "N'th extra root bus" rather than
  "extra root bus with bus number N" is peculiar and a little bit
  illogical to me. However:

  - It is an established interface between Coreboot and SeaBIOS that
    works in practice, on physical hardware.

  - As long as the enumeration algorithm stays the same in SeaBIOS (ie.
    probe extra root buses in increasing order), the "N'th extra root
    bus" scheme has identical expressive power, and a bijective
    mapping, to the "extra root bus with bus number N".

  - Given that the code that transforms between said mappings is ready,
    in both QEMU and OVMF, my personal verdict is that the SeaBIOS path
    is somewhat unwieldy, and has had me jump through some hoops, but
    ultimately it is correct.

It is of course possible to rewind the last seven or so days, return to
Marcel's SeaBIOS patch (and make it depend on runningOnQemu()), but that
will have the following consequences:
- Kevin could be mildly displeased by the SeaBIOS tweaks.
- A new version dependency between SeaBIOS and QEMU will be brought
  about.
- I'll have to rework and repost earlier patch set versions for both
  QEMU and OVMF, which sort of qualifies as "making a fool out of
  Laszlo".

So, please don't. Let's stick with this.

Thanks
Laszlo