On 03/22/19 02:33, Wu, Hao A wrote: >> -----Original Message----- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Friday, March 22, 2019 3:04 AM >> To: Ard Biesheuvel; Wu, Hao A >> Cc: edk2-devel@lists.01.org; Justen, Jordan L; Ni, Ray >> Subject: Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within >> IntelFrameworkModulePkg >> >> On 03/21/19 11:08, Ard Biesheuvel wrote: >>> On Thu, 21 Mar 2019 at 07:44, Wu, Hao A <hao.a...@intel.com> wrote: >>>> >>>>>>> >>>>>>> Just a couple of notes from my side - I'm sure Laszlo will have a much >>>>>>> longer list :-) >>>>>>> >>>>>>> - Dropping the floppy driver is fine with me. >>>>>>> - What is OVMF specific about this driver? Is it only the hardcoded >>>>>>> list of COM1/COM2/PS2 keyboard? If so, should we split this into a >>>>>>> driver and a library class, where the driver lives in MdeModulePkg, >>>>>>> and the library is implemented in the context of OVMF? >>>>>> >>>>>> Hello Ard, >>>>>> >>>>>> I think the special thing for this one is that: >>>>>> For QEMU, it does not have a Super I/O (SIO) chip. While, as far as I >>>>>> know, the SIO chip exists on other platforms. The driver proposed here >>>>>> simulates the behavior of an SIO chip. IMO, if we find more platforms >> that >>>>>> do not have a SIO chip, we can convert the driver into a general one. >>>>>> >>>>>> Also, for the implementation of the services in the Super I/O protocol, >>>>>> the proposed driver just does the minimal effort in order to support the >>>>>> serial/PS2 keyboard. >>>>> >>>>> Here's why I'd like the majority of this driver to live under >>>>> MdeModulePkg (for example through a lib class separation like Ard >> suggests): >>>>> >>>>> Because then its maintenance would not be the responsibility of OvmfPkg >>>>> maintainers. >>>>> >>>>> Consider, this driver is absolutely huge (1.5-2 kLOC), for doing "the >>>>> minimal effort in order to support the serial/PS2 keyboard". >>>>> >>>>> The risk of regressions is extreme (the PS/2 keyboard is the default >>>>> one, and if it breaks *subtly*, almost all users will be inconvenienced, >>>>> but not necessarily soon enough for us to get reports about it *early* >>>>> in the current development cycle). >>>>> >>>>> I realize that IntelFrameworkModulePkg/Bus/Isa/* drivers are frowned >>>>> upon nowadays, they may be ugly / platform specific / etc etc etc, but >>>>> they have also proved themselves to *work*, and (as far as I remember) >>>>> they have required practically zero fixes in order to function well on >>>>> QEMU. >>>>> >>>>> It is very unwelcome by me to take on the maintenance burden for a >>>>> driver that is all of: >>>>> - not widely tested, >>>>> - replacing a proven set of drivers that is critical to users, >>>>> - large. >>>>> >>>>> I understand that Intel wants to stop maintaining >>>>> IntelFrameworkModulePkg/Bus/Isa/*, but the above price is too high for >> me. >>>>> >>>>> Compare the case if we simply moved the >>>>> IntelFrameworkModulePkg/Bus/Isa/* drivers under OvmfPkg: >>>>> - still large, >>>>> - but widely tested (with minimal churn in the past), >>>>> - and no risk of regressions. >>>>> >>>>> So in this form, I'm generally opposed to the switch. The two sets of >>>>> drivers need to coexist for a while, and we must expose the new drivers >>>>> to users while providing them with some sort of easy fallback. (I'd >>>>> prefer that fallback to be dynamically configurable, but, again, if your >>>>> keyboard breaks, how do you interact with e.g. the UEFI shell? So I >>>>> guess a static build flag would do as well.) I think the old drivers >>>> >>>> Hello Laszlo, >>>> >>>> I agree with your point. So your suggestion is to: >>>> >>>> 1. Duplicate the below drivers into OvmfPkg: >>>> PcAtChipsetPkg/IsaAcpiDxe/IsaAcpi.inf >>>> IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBusDxe.inf >>>> IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf >>>> IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf >>>> >>>> 2. Meanwhile, add the proposed SioBusDxe driver in the OvmfPkg as well >>>> >>>> 3. Add a static build flag within OvmfPkg to let users choose between: >>>> a) New OVMF SioBusDxe driver + ISA device drivers under >>>> MdeModulePkg/Bus/Isa; >>>> b) Legacy ISA stack copied from PcAtChipsetPkg & >> IntelFrameworkModulePkg >>>> >>>> Is my understanding correct? >> >> Yes (but see below, at the end). >> >>>>> should be removed only in the edk2 stable tag that comes *after* the >>>>> next one, once we've given the drivers enough time to "prove themselves". >>>> >>>> Do you mean we should keep the copy of the legacy ISA stack from >>>> PcAtChipsetPkg & IntelFrameworkModulePkg until the announcement of >>>> edk2-stable201905 tag? >> >> Yes, exactly. People that adopt "edk2-stable201905" should be able to >> switch back to the old driver stack. >> >> NB: I certainly agree that the new code should be made the *default*. >> >>>> >>> >>> I think we should just keep the IntelFrameworkModulePkg components in >>> place until we are ready to stop using them in OVMF. Cloning them into >>> OvmfPkg now just so we can remove IntelFrameworkModulePkg in its >>> entirety has little added value IMO. >> >> I fully agree with this modification (it minimizes the churn), but I'm >> unsure how quickly Intel would like to rid themselves of >> IntelFrameworkModulePkg. If their deadline is edk2-stable201905, then >> that conflicts with my request above, and we might have no choice in >> moving the code to OvmfPkg, for the sake of one more stable tag. > > Hello Laszlo and Ard, > > How about the below approach: > > 1. Keep the current ISA stack in Ovmf: > PcAtChipsetPkg/IsaAcpiDxe/IsaAcpi.inf > IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBusDxe.inf > IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf > IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf > > 2. Add the proposed SioBusDxe driver in the OvmfPkg. > > 3. Add a static build flag within OvmfPkg to let users choose between: > a) New OVMF SioBusDxe driver + ISA device drivers under > MdeModulePkg/Bus/Isa; > b) Origin ISA stack (the PcAtChipsetPkg & IntelFrameworkModulePkg one); > c) Default behavior will be using the stack mentioned in a). > > 4a. If the removal of PcAtChipsetPkg/IsaAcpiDxe & IntelFrameworkModulePkg > comes before the edk2-stable201905 tag, copy the drivers in 1. into > OvmfPkg. > > 4b. If the removal of PcAtChipsetPkg/IsaAcpiDxe & IntelFrameworkModulePkg > comes after the edk2-stable201905 tag, drop the > PcAtChipsetPkg/IntelFrameworkModulePkg ISA stack in OVMF together with > the removal of PcAtChipsetPkg/IsaAcpiDxe & IntelFrameworkModulePkg. > > What do you think of the above strategy? Thanks.
It sounds exactly right to me. The important thing is that "edk2-stable201905" be released with the new stuff (SioBusDxe + MdeModulePkg/Bus/Isa) as the default driver stack in OVMF, but with an easy build option for users to revert to the old stack. (And if they feel forced to exercise that option, they should report it to us, so we can make an informed decision about dropping the old stack for good in the stable release that follows "edk2-stable201905".) Both (4a) and (4b) ensure this, so the plan looks good to me. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
