From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60897) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5HtE-0007DE-32 for qemu-devel@nongnu.org; Wed, 17 Jun 2015 14:16:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5Ht8-0001Qh-1Y for qemu-devel@nongnu.org; Wed, 17 Jun 2015 14:16:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49135) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5Ht7-0001QU-Pa for qemu-devel@nongnu.org; Wed, 17 Jun 2015 14:16:37 -0400 Message-ID: <5581B97E.8020707@redhat.com> Date: Wed, 17 Jun 2015 20:16:30 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1434545105-5811-1-git-send-email-lersek@redhat.com> <1434545105-5811-8-git-send-email-lersek@redhat.com> <20150617155237-mutt-send-email-mst@redhat.com> <20150617141820.GA11337@morn.localdomain> <55818819.3010107@redhat.com> <20150617150544.GA26500@redhat.com> In-Reply-To: <20150617150544.GA26500@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Marcel Apfelbaum , Kevin O'Connor , qemu-devel@nongnu.org, Markus Armbruster 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 >>>>> >>>>> /pci-root@N/pci@i0cf8/... >>>> >>>> It's kind of crazy, isn't it? >>>> /pci@i0cf8/pci-root@N 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 "/pci@i0cf8/pci-root@%x/" as well as >>> "/pci@i0cf8,%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 /pci-root@N/pci@i0cf8/... 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 /pci-root@N/... ie. dropping the second node ("pci@i0cf8"). - 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