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

[Laszlo Ersek <lersek@redhat.com>]

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.

Thanks
Laszlo