From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BF0DA211E7452 for ; Fri, 22 Mar 2019 03:56:03 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3CCC33082E69; Fri, 22 Mar 2019 10:56:02 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-95.rdu2.redhat.com [10.10.120.95]) by smtp.corp.redhat.com (Postfix) with ESMTP id A7D5F5C6A5; Fri, 22 Mar 2019 10:56:00 +0000 (UTC) To: Ard Biesheuvel Cc: "Wu, Hao A" , "edk2-devel@lists.01.org" , "Justen, Jordan L" , "Ni, Ray" References: <20190315071603.16936-1-hao.a.wu@intel.com> <2cef7754-a5ab-274e-44ab-14ba092f7d40@redhat.com> <8aecf13a-97d9-357d-b343-094f95f36f86@redhat.com> <746e3c63-6db6-ac1b-7b46-32e1aba99768@redhat.com> From: Laszlo Ersek Message-ID: <5484c84a-cbaf-918d-d9fa-4888d85e6855@redhat.com> Date: Fri, 22 Mar 2019 11:55:59 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.46]); Fri, 22 Mar 2019 10:56:02 +0000 (UTC) Subject: Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Mar 2019 10:56:04 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 03/22/19 10:41, Ard Biesheuvel wrote: > On Fri, 22 Mar 2019 at 10:25, Laszlo Ersek 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 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