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

[Laszlo Ersek <lersek@redhat.com>]

On 03/18/19 04:45, Wu, Hao A wrote:
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Friday, March 15, 2019 7:10 PM
>> To: Wu, Hao A
>> Cc: edk2-devel@lists.01.org; Justen, Jordan L; Laszlo Ersek; Ni, Ray
>> Subject: Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within
>> IntelFrameworkModulePkg
>>
>> On Fri, 15 Mar 2019 at 08:16, Hao Wu <hao.a.wu@intel.com> wrote:
>>>
>>> This series will update the OVMF to stop using the ISA drivers within
>>> IntelFrameworkModulePkg.
>>>
>>> As the replacement, a new OVMF Super I/O bus driver has been add which
>>> will install the Super I/O protocol for ISA serial and PS2 keyboard
>>> devices. By doing so, these devices can be managed by:
>>>
>>>   MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf
>>>   MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KeyboardDxe.inf
>>>
>>> respectively.
>>>
>>
>> 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
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".

(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

>> - Regardless of the previous, adding the new driver should be a patch
>> of its own.
> 
> Agree. I will adopt the detailed approach suggested by Jordan for this one.
> Thanks for the feedback.
> 
>> - I spotted an incorrect reference to PCI_IO_DEV in a comment (patch #2)
> 
> Thanks for the catch. I will address this comment typo in the next version of
> the series.
> 
> 
> I will wait a little longer to see if Laszlo has additional
> comments/feedbacks on this.
> 
> Best Regards,
> Hao Wu
> 
>>
>>
>>
>>>
>>> Tests done:
>>> A. GCC5 & VS2015x86 tool chains build pass
>>> B. Launch QEMU (2.4.50, Windows) with command:
>>>    > qemu-system-x86_64.exe -pflash <SOME_PATH>\OVMF.fd -serial
>> file:1.txt -serial file:2.txt
>>>
>>>    Able to see the ISA COM1/COM2 UART and PS2Keyboard devices under
>> Shell
>>>    using command 'devtree';
>>>
>>>    Both the serials and PS2 keyboard are working fine;
>>>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>>
>>> Hao Wu (2):
>>>   OvmfPkg: Drop the ISA Floppy device support
>>>   OvmfPkg: Add an Super IO bus driver to replace the current ISA support
>>>
>>>  OvmfPkg/OvmfPkgIa32.dsc           |  10 +-
>>>  OvmfPkg/OvmfPkgIa32X64.dsc        |  10 +-
>>>  OvmfPkg/OvmfPkgX64.dsc            |  10 +-
>>>  OvmfPkg/OvmfPkgIa32.fdf           |  10 +-
>>>  OvmfPkg/OvmfPkgIa32X64.fdf        |  10 +-
>>>  OvmfPkg/OvmfPkgX64.fdf            |  10 +-
>>>  OvmfPkg/SioBusDxe/SioBusDxe.inf   |  54 ++
>>>  OvmfPkg/SioBusDxe/SioBusDxe.h     | 332 +++++++++++
>>>  OvmfPkg/SioBusDxe/SioService.h    | 221 +++++++
>>>  OvmfPkg/SioBusDxe/ComponentName.c | 167 ++++++
>>>  OvmfPkg/SioBusDxe/SioBusDxe.c     | 622 ++++++++++++++++++++
>>>  OvmfPkg/SioBusDxe/SioService.c    | 405 +++++++++++++
>>>  OvmfPkg/SioBusDxe/SioBusDxe.uni   |  21 +
>>>  13 files changed, 1846 insertions(+), 36 deletions(-)
>>>  create mode 100644 OvmfPkg/SioBusDxe/SioBusDxe.inf
>>>  create mode 100644 OvmfPkg/SioBusDxe/SioBusDxe.h
>>>  create mode 100644 OvmfPkg/SioBusDxe/SioService.h
>>>  create mode 100644 OvmfPkg/SioBusDxe/ComponentName.c
>>>  create mode 100644 OvmfPkg/SioBusDxe/SioBusDxe.c
>>>  create mode 100644 OvmfPkg/SioBusDxe/SioService.c
>>>  create mode 100644 OvmfPkg/SioBusDxe/SioBusDxe.uni
>>>
>>> --
>>> 2.12.0.windows.1
>>>