On 05/09/16 19:33, Jordan Justen wrote: > On 2016-05-09 08:31:38, Laszlo Ersek wrote: >> On 05/08/16 13:19, Chang, Abner (HPS SW/FW Technologist) wrote: >> >>>>>> What would prevent this from living under OvmfPkg? Say >>>>>> OvmfPkg/OvmfPkgRiscV64.dsc? >> >>> I think it should be no problem to use OvmfPkg as common >>> processor/platform emulator. Just you still can see few Intel things. >>> ARM QEMU implementation must be considered as well if we move RISC-V >>> to OvmfPkg, thus we can have the consistent implementation. RISC-V >>> QEMU implementation just follows ARM QEMU implement, that is to have >>> a separate package. But I agree with you that all >>> processors/platforms emulator should be under OvmfPkg. >> >> I strongly disagree with this. Although OvmfPkg currently contains both >> platform-specific modules (for Ia32 and X64), and a number of reusable, >> platform-independent modules, it should not be considered a grab-bag of >> guest platform code for any and all hypervisors (and their various targets). >> >> ArmVirtPkg has always been separate, despite being "just another" >> upstream QEMU target. It has worked splendidly. >> >> Xen support lives right under OvmfPkg, and it has worked terribly. >> > > My opinion on this is reversed.
I know :) > I wish more effort had been made to > have arm-virt in OVMF, The reuse is very reasonable. Please browse the DSC files under ArmVirtPkg: OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf OvmfPkg/Library/QemuNewBootOrderLib/QemuBootOrderLib.inf OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf OvmfPkg/Library/VirtioLib/VirtioLib.inf OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf OvmfPkg/Library/XenIoMmioLib/XenIoMmioLib.inf OvmfPkg/PlatformDxe/Platform.inf OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf OvmfPkg/Virtio10Dxe/Virtio10.inf OvmfPkg/VirtioBlkDxe/VirtioBlk.inf OvmfPkg/VirtioNetDxe/VirtioNet.inf OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf OvmfPkg/VirtioRngDxe/VirtioRng.inf OvmfPkg/VirtioScsiDxe/VirtioScsi.inf OvmfPkg/XenBusDxe/XenBusDxe.inf OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf The drivers and library instances that are under ArmVirtPkg/ and ArmVirtPkg/Library are strongly specific to ARM. I don't know why you imply that the effort we made for OvmfPkg reuse wasn't strong enough. > and I can't see how splitting Xen out to > separate packages would have helped. Splitting / duplicating Xen code out from OvmfPkg would have prevented the recent PciHostBridgeDxe regression. I cannot test on Xen, but whenever I touch code in OvmfPkg, it has a large chance of modifying behavior on Xen. I try to be careful, but without dedicated regression testing, that's not always enough. Duplication / splitting would trivially prevent me from messing with (most of) the Xen code, which I cannot test. Of course, there is another way. You may have noticed that I tend to test patch series from Intel developers that they post for core modules; in particular patches that touch SMM or S3 code. Those developers have learned by now to CC me on such changed directly -- this helps us all: I catch OVMF regressions early, and sometimes they get early feedback about bugs that could lie dormant for long on physical platforms. In general I try to skim all the subjects, and if something looks suspicious, I test the work just to be sure. That's how I caught this today, *before* the fact: http://thread.gmane.org/gmane.comp.bios.edk2.devel/11834/focus=11873 So, that "other way" is: Xen should delegate someone who puts in the same effort to prevent regressions, puts in the same effort to review patches with an eye toward Xen. Maintains OVMF for Xen specifically. > >> It all comes down to maintenance. Maintainers in edk2 are assigned to >> top-level Pkg directories. If you introduce Risc-V virt in a separate >> top level package, and become its maintainer, I welcome that with open >> arms. If you try to push Risc-V stuff under OvmfPkg, which will make me >> responsible for reviewing patches and fighting regressions for the sake >> of a platform I have no interest in, I will definitely block that. >> >> ISA- and platform-specific modules live in separate directories in all >> of the projects I can think of as examples. QEMU? Check. The kernel? >> Check. Inter-mixing within a module is done only if the customization is >> minimal. (There are a few examples in OvmfPkg, and I dislike them.) >> >> If edk2 elects to introduce a finer (= subdirectory- and file-level) >> granularity to Maintainers.txt, *and* we can carve out a well separated >> directory structure for RISC-V under OvmfPkg, then maybe I could be >> convinced. >> > > I think we should alter Maintainers.txt to work best for the tree > rather than having the tree bend to Maintainers.txt. I agree. Well, we need both; they need to meet in the middle. I don't insist on a new top level Pkg; subdirectories in existent packages might work fine. Even separate files would work fine. For example, in this review today: http://thread.gmane.org/gmane.comp.bios.edk2.devel/11845/focus=11882 I asked Ray to split out the Xen-specific stuff to a separate file. If we had a fine-grained Maintainers.txt today, I would also ask Ray to add the new file to the Xen maintainer's turf. We don't have that Maintainers.txt today, so I'll just make a mental note that the new file is Xen-related. > My main concern about Maintainers.txt is adding too many committers. > Maybe we could add a reviewer level for people that can review for a > package, but not commit. That's a great idea; QEMU does it too. (Or at least something very similar.) > They can review and prep branches/patches for > the maintainers to fetch and then push. In this case, maybe a RISC-V > reviewer would work well for OVMF... Why not. I'd like to see the Maintainers.txt changes first (the general ones), so that I can require a patch series that adds a new architecture or platform to OvmfPkg to modify Maintainers.txt at once. For example, if Abner resurrected the antique PciHostBridgeDxe driver in OvmfPkg, and then there was a problem with it (or there were patches for it), I wouldn't even have to look at those, because that driver clearly belonged to Abner. Jordan -- my uptime today, at this moment, is 14 hours 33 minutes. (Local time: 23:44, and I'm not done.) From that time, I spent maybe one hour away from the computer. I started the morning with the thought to write and post a fix for RHBZ 1333238. Instead of that, I spent more than a normal workday on supporting OVMF users, reviewing patches, testing patches (even outside of OvmfPkg -- see above), discussing and filing github issue #87, and discussing this question. (I also tested and commented on a qemu-devel series that is directly related to OVMF use.) I got around starting my *actual* work at about half past 7PM. That's the time when people normally sit down to have dinner with the family. I will *vehemently* resist attempts that widen my responsibilities in OvmfPkg, including patches that permanently open up new avenues for regressions. If new co-maintainers step up (willing to put in the same effort as I have been), plus we can also mark the responsibilities in Maintainers.txt -- sure, I'll let them go wild. Thanks Laszlo > I also note that ShellBinPkg seems to have some arch specific > maintainers. > > -Jordan > >> For example, this *too* has worked perfectly well under ArmVirtPkg. In >> ArmVirtPkg the separation between Xen and QEMU is utterly clear (thanks >> again Ard!), and I immediately know if a quick skim and an Acked-by are >> enough from me, or I need to dive in and spend a week or more on >> reviewing patches. >> >> Regressions are terrible if they affect the code that I run, while they >> are "meh" *for me* if they affect Xen. (This is not to say that Xen >> should be ignored. Instead, it means that Xen people should delegate >> their own full-time edk2 maintainers, and we should assign them with the >> right granularity in Maintainers.txt. This happens to work very well in >> ArmVirtPkg, where we informally follow such a subdivision with Ard.) >> >> Thanks >> Laszlo >> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
