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 E63082194EB70 for ; Wed, 20 Mar 2019 05:34:07 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 97BE73082231; Wed, 20 Mar 2019 12:34:06 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-91.rdu2.redhat.com [10.10.120.91]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4664B6B49D; Wed, 20 Mar 2019 12:34:04 +0000 (UTC) To: "Wu, Hao A" , Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , "Justen, Jordan L" , "Ni, Ray" References: <20190315071603.16936-1-hao.a.wu@intel.com> From: Laszlo Ersek Message-ID: <2cef7754-a5ab-274e-44ab-14ba092f7d40@redhat.com> Date: Wed, 20 Mar 2019 13:34:03 +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.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.47]); Wed, 20 Mar 2019 12:34:06 +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: Wed, 20 Mar 2019 12:34:08 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 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 \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 >>> Cc: Laszlo Ersek >>> Cc: Ard Biesheuvel >>> Cc: Ray Ni >>> >>> 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 >>>