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

[Laszlo Ersek <lersek@redhat.com>]

On 03/22/19 10:41, Ard Biesheuvel wrote:
> On Fri, 22 Mar 2019 at 10:25, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 03/22/19 02:33, Wu, Hao A wrote:
>>>> -----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.
>>
>> It sounds exactly right to me.
>>
>> The important thing is that "edk2-stable201905" be released with the new
>> stuff (SioBusDxe + MdeModulePkg/Bus/Isa) as the default driver stack in
>> OVMF, but with an easy build option for users to revert to the old stack.
>>
>> (And if they feel forced to exercise that option, they should report it
>> to us, so we can make an informed decision about dropping the old stack
>> for good in the stable release that follows "edk2-stable201905".)
>>
>> Both (4a) and (4b) ensure this, so the plan looks good to me.
>>
> 
> I think 4a is pointless: it permits IntelFrameworkModulePkg/ to be
> removed, but the exact same code lives on under OvmfPkg/. So if the
> intent is to remove IntelFrameworkModulePkg entirely, this only
> achieves it in a very bureaucratic sense, with extra churn in the
> OvmfPkg for no other reason than to make the dates on your bugzilla
> entries look good.

I think that's an accurate characterization :)

> So I strongly favour keeping IntelFrameworkModulePkg around (or at
> least the pieces OVMF depends on) until we are ready to stop using it
> altogether. Anything beyond that only satisfies artificial process
> needs rather than technical needs.

Agreed on both counts; (4b) is much better.

Thanks,
Laszlo