Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, March 22, 2019 6:56 PM
> To: Ard Biesheuvel
> Cc: Wu, Hao A; 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/22/19 10:41, Ard Biesheuvel wrote:
> > On Fri, 22 Mar 2019 at 10:25, Laszlo Ersek <lersek@redhat.com> wrote:
> >>
> >> On 03/22/19 02:33, Wu, Hao A wrote:
> >>>> -----Original Message-----
> >>>> From: Laszlo Ersek [mailto:lersek@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.wu@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.
> >>
> >
> > I think 4a is pointless: it permits IntelFrameworkModulePkg/ to be
> > removed, but the exact same code lives on under OvmfPkg/. So if the
> > intent is to remove IntelFrameworkModulePkg entirely, this only
> > achieves it in a very bureaucratic sense, with extra churn in the
> > OvmfPkg for no other reason than to make the dates on your bugzilla
> > entries look good.
> 
> I think that's an accurate characterization :)
> 
> > So I strongly favour keeping IntelFrameworkModulePkg around (or at
> > least the pieces OVMF depends on) until we are ready to stop using it
> > altogether. Anything beyond that only satisfies artificial process
> > needs rather than technical needs.
> 
> Agreed on both counts; (4b) is much better.

Agree.
I will post a V2 series following the direction described by:
1, 2, 3, 4a.

Best Regards,
Hao Wu

> 
> Thanks,
> Laszlo