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

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

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


Best Regards,
Hao Wu

> 
> (We did something similar when we switched to the central PCI root
> bridge driver in OVMF as well -- an OVMF lib instance for it was
> implemented, so the maintenance burden wasn't fully under OvmfPkg to
> begin with, plus we kept the old driver around with a build flag for a
> while. IIRC.)
> 
> Obviously I might feel safer if I could do a thorough review of the
> driver, but I *absolutely* don't have time for it now. I'm sorry about that.
> 
> Thanks,
> Laszlo
>