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 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.

Best Regards,
Hao Wu

> 
> Thanks
> Laszlo