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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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

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.