* [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg
@ 2019-03-15 7:16 Hao Wu
2019-03-15 7:16 ` [PATCH v1 1/2] OvmfPkg: Drop the ISA Floppy device support Hao Wu
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Hao Wu @ 2019-03-15 7:16 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Ray Ni
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.
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v1 1/2] OvmfPkg: Drop the ISA Floppy device support
2019-03-15 7:16 [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg Hao Wu
@ 2019-03-15 7:16 ` Hao Wu
2019-03-15 7:16 ` [PATCH v1 2/2] OvmfPkg: Add an Super IO bus driver to replace the current ISA support Hao Wu
2019-03-15 11:09 ` [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg Ard Biesheuvel
2 siblings, 0 replies; 16+ messages in thread
From: Hao Wu @ 2019-03-15 7:16 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Ray Ni
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1495
There is a plan to remove the IntelFrameworkModulePkg:
https://bugzilla.tianocore.org/show_bug.cgi?id=1605
And for driver:
IntelFrameworkModulePkg/Bus/Isa/IsaFloppyDxe
This patch proposes to drop the ISA Floppy device support in OVMF.
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>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
OvmfPkg/OvmfPkgIa32.dsc | 3 +--
OvmfPkg/OvmfPkgIa32X64.dsc | 3 +--
OvmfPkg/OvmfPkgX64.dsc | 3 +--
OvmfPkg/OvmfPkgIa32.fdf | 3 +--
OvmfPkg/OvmfPkgIa32X64.fdf | 3 +--
OvmfPkg/OvmfPkgX64.fdf | 3 +--
6 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 5b885590b2..1710ab5a88 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -1,7 +1,7 @@
## @file
# EFI/Framework Open Virtual Machine Firmware (OVMF) platform
#
-# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
# (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
#
# This program and the accompanying materials
@@ -756,7 +756,6 @@
IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBusDxe.inf
IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf
IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf
- IntelFrameworkModulePkg/Bus/Isa/IsaFloppyDxe/IsaFloppyDxe.inf
#
# SMBIOS Support
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index bbf0853ee6..5bceef3116 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -1,7 +1,7 @@
## @file
# EFI/Framework Open Virtual Machine Firmware (OVMF) platform
#
-# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
# (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
#
# This program and the accompanying materials
@@ -765,7 +765,6 @@
IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBusDxe.inf
IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf
IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf
- IntelFrameworkModulePkg/Bus/Isa/IsaFloppyDxe/IsaFloppyDxe.inf
#
# SMBIOS Support
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index d81460f520..3f5d948dbb 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -1,7 +1,7 @@
## @file
# EFI/Framework Open Virtual Machine Firmware (OVMF) platform
#
-# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
# (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
#
# This program and the accompanying materials
@@ -763,7 +763,6 @@
IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBusDxe.inf
IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf
IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf
- IntelFrameworkModulePkg/Bus/Isa/IsaFloppyDxe/IsaFloppyDxe.inf
#
# SMBIOS Support
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 4999403ad7..54d7f06a70 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -1,7 +1,7 @@
## @file
# Open Virtual Machine Firmware: FDF
#
-# Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
# (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
#
# This program and the accompanying materials
@@ -273,7 +273,6 @@ INF IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf
!endif
INF IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf
-INF IntelFrameworkModulePkg/Bus/Isa/IsaFloppyDxe/IsaFloppyDxe.inf
INF MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
INF OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index d0cc107928..7519b53a9b 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -1,7 +1,7 @@
## @file
# Open Virtual Machine Firmware: FDF
#
-# Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
# (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
#
# This program and the accompanying materials
@@ -274,7 +274,6 @@ INF IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf
!endif
INF IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf
-INF IntelFrameworkModulePkg/Bus/Isa/IsaFloppyDxe/IsaFloppyDxe.inf
INF MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
INF OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index d0cc107928..7519b53a9b 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -1,7 +1,7 @@
## @file
# Open Virtual Machine Firmware: FDF
#
-# Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
# (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
#
# This program and the accompanying materials
@@ -274,7 +274,6 @@ INF IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf
!endif
INF IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf
-INF IntelFrameworkModulePkg/Bus/Isa/IsaFloppyDxe/IsaFloppyDxe.inf
INF MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
INF OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v1 2/2] OvmfPkg: Add an Super IO bus driver to replace the current ISA support
2019-03-15 7:16 [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg Hao Wu
2019-03-15 7:16 ` [PATCH v1 1/2] OvmfPkg: Drop the ISA Floppy device support Hao Wu
@ 2019-03-15 7:16 ` Hao Wu
2019-03-15 11:09 ` [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg Ard Biesheuvel
2 siblings, 0 replies; 16+ messages in thread
From: Hao Wu @ 2019-03-15 7:16 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Ray Ni
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1495
There is a plan to remove the IntelFrameworkModulePkg:
https://bugzilla.tianocore.org/show_bug.cgi?id=1605
This patch will replace the current ISA support:
PcAtChipsetPkg/IsaAcpiDxe/IsaAcpi.inf
IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBusDxe.inf
IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf
IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf
with a new OVMF Super I/O bus driver and the serial & PS2 keyboard
drviers:
MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf
MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KeyboardDxe.inf
The new OVMF Super IO bus driver will create the below child devices:
* COM 1 UART
* COM 2 UART
* PS/2 Keyboard
and installs the Super I/O Protocol on them.
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>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
OvmfPkg/OvmfPkgIa32.dsc | 7 +-
OvmfPkg/OvmfPkgIa32X64.dsc | 7 +-
OvmfPkg/OvmfPkgX64.dsc | 7 +-
OvmfPkg/OvmfPkgIa32.fdf | 7 +-
OvmfPkg/OvmfPkgIa32X64.fdf | 7 +-
OvmfPkg/OvmfPkgX64.fdf | 7 +-
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, 1840 insertions(+), 24 deletions(-)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 1710ab5a88..3caaeab2f3 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -752,10 +752,9 @@
#
# ISA Support
#
- PcAtChipsetPkg/IsaAcpiDxe/IsaAcpi.inf
- IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBusDxe.inf
- IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf
- IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf
+ OvmfPkg/SioBusDxe/SioBusDxe.inf
+ MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf
+ MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KeyboardDxe.inf
#
# SMBIOS Support
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 5bceef3116..21bf4eeb5a 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -761,10 +761,9 @@
#
# ISA Support
#
- PcAtChipsetPkg/IsaAcpiDxe/IsaAcpi.inf
- IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBusDxe.inf
- IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf
- IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf
+ OvmfPkg/SioBusDxe/SioBusDxe.inf
+ MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf
+ MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KeyboardDxe.inf
#
# SMBIOS Support
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 3f5d948dbb..6102e8370d 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -759,10 +759,9 @@
#
# ISA Support
#
- PcAtChipsetPkg/IsaAcpiDxe/IsaAcpi.inf
- IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBusDxe.inf
- IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf
- IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf
+ OvmfPkg/SioBusDxe/SioBusDxe.inf
+ MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf
+ MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KeyboardDxe.inf
#
# SMBIOS Support
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 54d7f06a70..05d9477fc8 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -265,14 +265,13 @@ INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
INF MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
-INF PcAtChipsetPkg/IsaAcpiDxe/IsaAcpi.inf
-INF IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBusDxe.inf
+INF OvmfPkg/SioBusDxe/SioBusDxe.inf
!ifndef $(SOURCE_DEBUG_ENABLE)
-INF IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf
+INF MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf
!endif
-INF IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf
+INF MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KeyboardDxe.inf
INF MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
INF OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index 7519b53a9b..53b29ddd64 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -266,14 +266,13 @@ INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
INF MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
-INF PcAtChipsetPkg/IsaAcpiDxe/IsaAcpi.inf
-INF IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBusDxe.inf
+INF OvmfPkg/SioBusDxe/SioBusDxe.inf
!ifndef $(SOURCE_DEBUG_ENABLE)
-INF IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf
+INF MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf
!endif
-INF IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf
+INF MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KeyboardDxe.inf
INF MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
INF OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 7519b53a9b..53b29ddd64 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -266,14 +266,13 @@ INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
INF MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
-INF PcAtChipsetPkg/IsaAcpiDxe/IsaAcpi.inf
-INF IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBusDxe.inf
+INF OvmfPkg/SioBusDxe/SioBusDxe.inf
!ifndef $(SOURCE_DEBUG_ENABLE)
-INF IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf
+INF MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf
!endif
-INF IntelFrameworkModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2keyboardDxe.inf
+INF MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KeyboardDxe.inf
INF MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
INF OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
diff --git a/OvmfPkg/SioBusDxe/SioBusDxe.inf b/OvmfPkg/SioBusDxe/SioBusDxe.inf
new file mode 100644
index 0000000000..5c462f1a8c
--- /dev/null
+++ b/OvmfPkg/SioBusDxe/SioBusDxe.inf
@@ -0,0 +1,54 @@
+## @file
+# The SioBusDxe driver is used to create child devices on the ISA bus and
+# installs the Super I/O protocols on them.
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+#
+# This program and the accompanying materials
+# are licensed and made available under the terms and conditions of the BSD License
+# which accompanies this distribution. The full text of the license may be found at
+# http://opensource.org/licenses/bsd-license.php
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = SioBusDxe
+ MODULE_UNI_FILE = SioBusDxe.uni
+ FILE_GUID = 864E1CA8-85EB-4D63-9DCC-6E0FC90FFD55
+ MODULE_TYPE = UEFI_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = SioBusDxeDriverEntryPoint
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 EBC
+#
+
+[Sources]
+ ComponentName.c
+ SioService.c
+ SioService.h
+ SioBusDxe.c
+ SioBusDxe.h
+
+[Packages]
+ MdePkg/MdePkg.dec
+
+[LibraryClasses]
+ UefiDriverEntryPoint
+ UefiLib
+ UefiBootServicesTableLib
+ DebugLib
+ ReportStatusCodeLib
+ MemoryAllocationLib
+ BaseMemoryLib
+ DevicePathLib
+
+[Protocols]
+ gEfiPciIoProtocolGuid ## TO_START
+ gEfiSioProtocolGuid ## BY_START
diff --git a/OvmfPkg/SioBusDxe/SioBusDxe.h b/OvmfPkg/SioBusDxe/SioBusDxe.h
new file mode 100644
index 0000000000..1455c48f63
--- /dev/null
+++ b/OvmfPkg/SioBusDxe/SioBusDxe.h
@@ -0,0 +1,332 @@
+/** @file
+ The SioBusDxe driver is used to create child devices on the ISA bus and
+ installs the Super I/O protocols on them.
+
+ Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions
+ of the BSD License which accompanies this distribution. The
+ full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef __SIO_BUS_DXE_H__
+#define __SIO_BUS_DXE_H__
+
+#include <Uefi.h>
+
+#include <IndustryStandard/Pci.h>
+
+#include <Protocol/PciIo.h>
+#include <Protocol/SuperIo.h>
+
+#include <Library/UefiDriverEntryPoint.h>
+#include <Library/UefiLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/DebugLib.h>
+#include <Library/ReportStatusCodeLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DevicePathLib.h>
+
+#include "SioService.h"
+
+//
+// SIO Bus driver private data structure
+//
+typedef struct {
+ EFI_PCI_IO_PROTOCOL *PciIo;
+ UINT64 OriginalAttributes;
+} SIO_BUS_DRIVER_PRIVATE_DATA;
+
+
+//
+// Global Variables
+//
+extern EFI_COMPONENT_NAME_PROTOCOL gSioBusComponentName;
+extern EFI_COMPONENT_NAME2_PROTOCOL gSioBusComponentName2;
+
+
+//
+// EFI Component Name Functions
+//
+
+/**
+ Retrieves a Unicode string that is the user readable name of the driver.
+
+ This function retrieves the user readable name of a driver in the form of a
+ Unicode string. If the driver specified by This has a user readable name in
+ the language specified by Language, then a pointer to the driver name is
+ returned in DriverName, and EFI_SUCCESS is returned. If the driver specified
+ by This does not support the language specified by Language, then
+ EFI_UNSUPPORTED is returned.
+
+ @param[in] This A pointer to the EFI_COMPONENT_NAME2_PROTOCOL
+ or EFI_COMPONENT_NAME_PROTOCOL instance.
+ @param[in] Language A pointer to a Null-terminated ASCII string
+ array indicating the language. This is the
+ language of the driver name that the caller is
+ requesting, and it must match one of the
+ languages specified in SupportedLanguages. The
+ number of languages supported by a driver is up
+ to the driver writer. Language is specified
+ in RFC 4646 or ISO 639-2 language code format.
+ @param[out] DriverName A pointer to the Unicode string to return. This
+ Unicode string is the name of the driver
+ specified by This in the language specified by
+ Language.
+
+ @retval EFI_SUCCESS The Unicode string for the Driver specified by
+ This and the language specified by Language was
+ returned in DriverName.
+ @retval EFI_INVALID_PARAMETER Language is NULL.
+ @retval EFI_INVALID_PARAMETER DriverName is NULL.
+ @retval EFI_UNSUPPORTED The driver specified by This does not support
+ the language specified by Language.
+
+**/
+EFI_STATUS
+EFIAPI
+SioBusComponentNameGetDriverName (
+ IN EFI_COMPONENT_NAME_PROTOCOL *This,
+ IN CHAR8 *Language,
+ OUT CHAR16 **DriverName
+ );
+
+/**
+ Retrieves a Unicode string that is the user readable name of the controller
+ that is being managed by a driver.
+
+ This function retrieves the user readable name of the controller specified by
+ ControllerHandle and ChildHandle in the form of a Unicode string. If the
+ driver specified by This has a user readable name in the language specified
+ by Language, then a pointer to the controller name is returned in
+ ControllerName, and EFI_SUCCESS is returned. If the driver specified by This
+ is not currently managing the controller specified by ControllerHandle and
+ ChildHandle, then EFI_UNSUPPORTED is returned. If the driver specified by
+ This does not support the language specified by Language, then
+ EFI_UNSUPPORTED is returned.
+
+ @param[in] This A pointer to the EFI_COMPONENT_NAME2_PROTOCOL
+ or EFI_COMPONENT_NAME_PROTOCOL instance.
+ @param[in] ControllerHandle The handle of a controller that the driver
+ specified by This is managing. This handle
+ specifies the controller whose name is to be
+ returned.
+ @param[in] ChildHandle The handle of the child controller to retrieve
+ the name of. This is an optional parameter
+ that may be NULL. It will be NULL for device
+ drivers. It will also be NULL for a bus
+ drivers that wish to retrieve the name of the
+ bus controller. It will not be NULL for a bus
+ driver that wishes to retrieve the name of a
+ child controller.
+ @param[in] Language A pointer to a Null-terminated ASCII string
+ array indicating the language. This is the
+ language of the driver name that the caller is
+ requesting, and it must match one of the
+ languages specified in SupportedLanguages. The
+ number of languages supported by a driver is up
+ to the driver writer. Language is specified in
+ RFC 4646 or ISO 639-2 language code format.
+ @param[out] ControllerName A pointer to the Unicode string to return.
+ This Unicode string is the name of the
+ controller specified by ControllerHandle and
+ ChildHandle in the language specified by
+ Language from the point of view of the driver
+ specified by This.
+
+ @retval EFI_SUCCESS The Unicode string for the user readable name
+ in the language specified by Language for the
+ driver specified by This was returned in
+ DriverName.
+ @retval EFI_INVALID_PARAMETER ControllerHandle is NULL.
+ @retval EFI_INVALID_PARAMETER ChildHandle is not NULL and it is not a valid
+ EFI_HANDLE.
+ @retval EFI_INVALID_PARAMETER Language is NULL.
+ @retval EFI_INVALID_PARAMETER ControllerName is NULL.
+ @retval EFI_UNSUPPORTED The driver specified by This is not currently
+ managing the controller specified by
+ ControllerHandle and ChildHandle.
+ @retval EFI_UNSUPPORTED The driver specified by This does not support
+ the language specified by Language.
+
+**/
+EFI_STATUS
+EFIAPI
+SioBusComponentNameGetControllerName (
+ IN EFI_COMPONENT_NAME_PROTOCOL *This,
+ IN EFI_HANDLE ControllerHandle,
+ IN EFI_HANDLE ChildHandle OPTIONAL,
+ IN CHAR8 *Language,
+ OUT CHAR16 **ControllerName
+ );
+
+
+//
+// Driver Binding Protocol interfaces
+//
+
+/**
+ Tests to see if this driver supports a given controller. If a child device is
+ provided, it further tests to see if this driver supports creating a handle
+ for the specified child device.
+
+ This function checks to see if the driver specified by This supports the
+ device specified by ControllerHandle. Drivers will typically use the device
+ path attached to ControllerHandle and/or the services from the bus I/O
+ abstraction attached to ControllerHandle to determine if the driver supports
+ ControllerHandle. This function may be called many times during platform
+ initialization. In order to reduce boot times, the tests performed by this
+ function must be very small, and take as little time as possible to execute.
+ This function must not change the state of any hardware devices, and this
+ function must be aware that the device specified by ControllerHandle may
+ already be managed by the same driver or a different driver. This function
+ must match its calls to AllocatePages() with FreePages(), AllocatePool() with
+ FreePool(), and OpenProtocol() with CloseProtocol(). Since ControllerHandle
+ may have been previously started by the same driver, if a protocol is already
+ in the opened state, then it must not be closed with CloseProtocol(). This is
+ required to guarantee the state of ControllerHandle is not modified by this
+ function.
+
+ @param[in] This A pointer to the EFI_DRIVER_BINDING_PROTOCOL
+ instance.
+ @param[in] ControllerHandle The handle of the controller to test. This
+ handle must support a protocol interface
+ that supplies an I/O abstraction to the
+ driver.
+ @param[in] RemainingDevicePath A pointer to the remaining portion of a
+ device path. This parameter is ignored by
+ device drivers, and is optional for bus
+ drivers. For bus drivers, if this parameter
+ is not NULL, then the bus driver must
+ determine if the bus controller specified by
+ ControllerHandle and the child controller
+ specified by RemainingDevicePath are both
+ supported by this bus driver.
+
+ @retval EFI_SUCCESS The device specified by ControllerHandle and
+ RemainingDevicePath is supported by the
+ driver specified by This.
+ @retval EFI_ALREADY_STARTED The device specified by ControllerHandle and
+ RemainingDevicePath is already being managed
+ by the driver specified by This.
+ @retval EFI_ACCESS_DENIED The device specified by ControllerHandle and
+ RemainingDevicePath is already being managed
+ by a different driver or an application that
+ requires exclusive access.
+ @retval EFI_UNSUPPORTED The device specified by ControllerHandle and
+ RemainingDevicePath is not supported by the
+ driver specified by This.
+
+**/
+EFI_STATUS
+EFIAPI
+SioBusDriverBindingSupported (
+ IN EFI_DRIVER_BINDING_PROTOCOL *This,
+ IN EFI_HANDLE Controller,
+ IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath
+ );
+
+/**
+ Starts a device controller or a bus controller.
+
+ The Start() function is designed to be invoked from the EFI boot service
+ ConnectController(). As a result, much of the error checking on the
+ parameters to Start() has been moved into this common boot service. It is
+ legal to call Start() from other locations, but the following calling
+ restrictions must be followed or the system behavior will not be
+ deterministic.
+ 1. ControllerHandle must be a valid EFI_HANDLE.
+ 2. If RemainingDevicePath is not NULL, then it must be a pointer to a
+ naturally aligned EFI_DEVICE_PATH_PROTOCOL.
+ 3. Prior to calling Start(), the Supported() function for the driver
+ specified by This must have been called with the same calling parameters,
+ and Supported() must have returned EFI_SUCCESS.
+
+ @param[in] This A pointer to the EFI_DRIVER_BINDING_PROTOCOL
+ instance.
+ @param[in] ControllerHandle The handle of the controller to start. This
+ handle must support a protocol interface
+ that supplies an I/O abstraction to the
+ driver.
+ @param[in] RemainingDevicePath A pointer to the remaining portion of a
+ device path. This parameter is ignored by
+ device drivers, and is optional for bus
+ drivers. For a bus driver, if this parameter
+ is NULL, then handles for all the children
+ of Controller are created by this driver. If
+ this parameter is not NULL and the first
+ Device Path Node is not the End of Device
+ Path Node, then only the handle for the
+ child device specified by the first Device
+ Path Node of RemainingDevicePath is created
+ by this driver. If the first Device Path
+ Node of RemainingDevicePath is the End of
+ Device Path Node, no child handle is created
+ by this driver.
+
+ @retval EFI_SUCCESS The device was started.
+ @retval EFI_DEVICE_ERROR The device could not be started due to a
+ device error.
+ @retval EFI_OUT_OF_RESOURCES The request could not be completed due to a
+ lack of resources.
+ @retval Others The driver failded to start the device.
+
+**/
+EFI_STATUS
+EFIAPI
+SioBusDriverBindingStart (
+ IN EFI_DRIVER_BINDING_PROTOCOL *This,
+ IN EFI_HANDLE Controller,
+ IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath
+ );
+
+/**
+ Stops a device controller or a bus controller.
+
+ The Stop() function is designed to be invoked from the EFI boot service
+ DisconnectController(). As a result, much of the error checking on the
+ parameters to Stop() has been moved into this common boot service. It is
+ legal to call Stop() from other locations, but the following calling
+ restrictions must be followed or the system behavior will not be
+ deterministic.
+ 1. ControllerHandle must be a valid EFI_HANDLE that was used on a previous
+ call to this same driver's Start() function.
+ 2. The first NumberOfChildren handles of ChildHandleBuffer must all be a
+ valid EFI_HANDLE. In addition, all of these handles must have been created
+ in this driver's Start() function, and the Start() function must have
+ called OpenProtocol() on ControllerHandle with an Attribute of
+ EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER.
+
+ @param[in] This A pointer to the EFI_DRIVER_BINDING_PROTOCOL
+ instance.
+ @param[in] ControllerHandle A handle to the device being stopped. The
+ handle must support a bus specific I/O
+ protocol for the driver to use to stop the
+ device.
+ @param[in] NumberOfChildren The number of child device handles in
+ ChildHandleBuffer.
+ @param[in] ChildHandleBuffer An array of child handles to be freed. May be
+ NULL if NumberOfChildren is 0.
+
+ @retval EFI_SUCCESS The device was stopped.
+ @retval EFI_DEVICE_ERROR The device could not be stopped due to a
+ device error.
+
+**/
+EFI_STATUS
+EFIAPI
+SioBusDriverBindingStop (
+ IN EFI_DRIVER_BINDING_PROTOCOL *This,
+ IN EFI_HANDLE Controller,
+ IN UINTN NumberOfChildren,
+ IN EFI_HANDLE *ChildHandleBuffer
+ );
+
+#endif // __SIO_BUS_DXE_H__
diff --git a/OvmfPkg/SioBusDxe/SioService.h b/OvmfPkg/SioBusDxe/SioService.h
new file mode 100644
index 0000000000..758703dd89
--- /dev/null
+++ b/OvmfPkg/SioBusDxe/SioService.h
@@ -0,0 +1,221 @@
+/** @file
+ The SioBusDxe driver is used to create child devices on the ISA bus and
+ installs the Super I/O protocols on them.
+
+ Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions
+ of the BSD License which accompanies this distribution. The
+ full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef __SIO_SERVICE_H__
+#define __SIO_SERVICE_H__
+
+#pragma pack(1)
+
+typedef struct {
+ EFI_ACPI_FIXED_LOCATION_IO_PORT_DESCRIPTOR Io;
+ EFI_ACPI_END_TAG_DESCRIPTOR End;
+} SIO_RESOURCES_IO;
+
+#pragma pack()
+
+typedef struct {
+ UINT32 Hid;
+ UINT32 Uid;
+ ACPI_RESOURCE_HEADER_PTR Resources;
+} SIO_DEVICE_INFO;
+
+//
+// SIO device private data structure
+//
+typedef struct {
+ UINT32 Signature;
+ EFI_HANDLE Handle;
+ EFI_PCI_IO_PROTOCOL *PciIo;
+ EFI_DEVICE_PATH_PROTOCOL *DevicePath;
+
+ EFI_SIO_PROTOCOL Sio;
+ UINT32 DeviceIndex;
+} SIO_DEV;
+#define SIO_DEV_SIGNATURE SIGNATURE_32 ('S', 'I', 'O', 'D')
+#define SIO_DEV_FROM_SIO(a) CR (a, SIO_DEV, Sio, SIO_DEV_SIGNATURE)
+
+
+//
+// Super I/O Protocol interfaces
+//
+
+/**
+ Provides a low level access to the registers for the Super I/O.
+
+ @param[in] This Indicates a pointer to the calling context.
+ @param[in] Write Specifies the type of the register operation.
+ If this parameter is TRUE, Value is interpreted
+ as an input parameter and the operation is a
+ register write. If this parameter is FALSE,
+ Value is interpreted as an output parameter and
+ the operation is a register read.
+ @param[in] ExitCfgMode Exit Configuration Mode Indicator. If this
+ parameter is set to TRUE, the Super I/O driver
+ will turn off configuration mode of the Super
+ I/O prior to returning from this function. If
+ this parameter is set to FALSE, the Super I/O
+ driver will leave Super I/O in the
+ configuration mode. The Super I/O driver must
+ track the current state of the Super I/O and
+ enable the configuration mode of Super I/O if
+ necessary prior to register access.
+ @param[in] Register Register number.
+ @param[in,out] Value If Write is TRUE, Value is a pointer to the
+ buffer containing the byte of data to be
+ written to the Super I/O register. If Write is
+ FALSE, Value is a pointer to the destination
+ buffer for the byte of data to be read from the
+ Super I/O register.
+
+ @retval EFI_SUCCESS The operation completed successfully.
+ @retval EFI_INVALID_PARAMETER The Value is NULL.
+ @retval EFI_INVALID_PARAMETER Invalid Register number.
+
+**/
+EFI_STATUS
+EFIAPI
+SioRegisterAccess (
+ IN CONST EFI_SIO_PROTOCOL *This,
+ IN BOOLEAN Write,
+ IN BOOLEAN ExitCfgMode,
+ IN UINT8 Register,
+ IN OUT UINT8 *Value
+ );
+
+/**
+ Provides an interface to get a list of the current resources consumed by the
+ device in the ACPI Resource Descriptor format.
+
+ GetResources() returns a list of resources currently consumed by the device.
+ The ResourceList is a pointer to the buffer containing resource descriptors
+ for the device. The descriptors are in the format of Small or Large ACPI
+ resource descriptor as defined by ACPI specification (2.0 & 3.0). The buffer
+ of resource descriptors is terminated with the 'End tag' resource descriptor.
+
+ @param[in] This Indicates a pointer to the calling context.
+ @param[out] ResourceList A pointer to an ACPI resource descriptor list
+ that defines the current resources used by the
+ device.
+
+ @retval EFI_SUCCESS The operation completed successfully.
+ @retval EFI_INVALID_PARAMETER ResourceList is NULL.
+
+**/
+EFI_STATUS
+EFIAPI
+SioGetResources (
+ IN CONST EFI_SIO_PROTOCOL *This,
+ OUT ACPI_RESOURCE_HEADER_PTR *ResourceList
+ );
+
+/**
+ Sets the resources for the device.
+
+ @param[in] This Indicates a pointer to the calling context.
+ @param[in] ResourceList Pointer to the ACPI resource descriptor list.
+
+ @retval EFI_SUCCESS The operation completed successfully.
+ @retval EFI_INVALID_PARAMETER ResourceList is invalid.
+ @retval EFI_ACCESS_DENIED Some of the resources in ResourceList are in
+ use.
+
+**/
+EFI_STATUS
+EFIAPI
+SioSetResources (
+ IN CONST EFI_SIO_PROTOCOL *This,
+ IN ACPI_RESOURCE_HEADER_PTR ResourceList
+ );
+
+/**
+ Provides a collection of resource descriptor lists. Each resource descriptor
+ list in the collection defines a combination of resources that can
+ potentially be used by the device.
+
+ @param[in] This Indicates a pointer to the calling context.
+ @param[out] ResourceCollection Collection of the resource descriptor
+ lists.
+
+ @retval EFI_SUCCESS The operation completed successfully.
+ @retval EFI_INVALID_PARAMETER ResourceCollection is NULL.
+
+**/
+EFI_STATUS
+EFIAPI
+SioPossibleResources (
+ IN CONST EFI_SIO_PROTOCOL *This,
+ OUT ACPI_RESOURCE_HEADER_PTR *ResourceCollection
+ );
+
+/**
+ Provides an interface for a table based programming of the Super I/O
+ registers.
+
+ The Modify() function provides an interface for table based programming of
+ the Super I/O registers. This function can be used to perform programming of
+ multiple Super I/O registers with a single function call. For each table
+ entry, the Register is read, its content is bitwise ANDed with AndMask, and
+ then ORed with OrMask before being written back to the Register. The Super
+ I/O driver must track the current state of the Super I/O and enable the
+ configuration mode of Super I/O if necessary prior to table processing. Once
+ the table is processed, the Super I/O device has to be returned to the
+ original state.
+
+ @param[in] This Indicates a pointer to the calling context.
+ @param[in] Command A pointer to an array of NumberOfCommands
+ EFI_SIO_REGISTER_MODIFY structures. Each
+ structure specifies a single Super I/O register
+ modify operation.
+ @param[in] NumberOfCommands Number of elements in the Command array.
+
+ @retval EFI_SUCCESS The operation completed successfully.
+ @retval EFI_INVALID_PARAMETER Command is NULL.
+
+**/
+EFI_STATUS
+EFIAPI
+SioModify (
+ IN CONST EFI_SIO_PROTOCOL *This,
+ IN CONST EFI_SIO_REGISTER_MODIFY *Command,
+ IN UINTN NumberOfCommands
+ );
+
+//
+// Internal functions
+//
+
+/**
+ Create all the ISA child devices on the ISA bus controller (PCI to ISA
+ bridge).
+
+ @param[in] This The EFI_DRIVER_BINDING_PROTOCOL instance.
+ @param[in] Controller The handle of ISA bus controller.
+ @param[in] PciIo The pointer to the PCI protocol.
+ @param[in] ParentDevicePath Device path of the ISA bus controller.
+
+ @retval The number of child device that is successfully created.
+
+**/
+UINT32
+SioCreateAllChildDevices (
+ IN EFI_DRIVER_BINDING_PROTOCOL *This,
+ IN EFI_HANDLE Controller,
+ IN EFI_PCI_IO_PROTOCOL *PciIo,
+ IN EFI_DEVICE_PATH_PROTOCOL *ParentDevicePath
+ );
+
+#endif // __SIO_SERVICE_H__
diff --git a/OvmfPkg/SioBusDxe/ComponentName.c b/OvmfPkg/SioBusDxe/ComponentName.c
new file mode 100644
index 0000000000..668a2c6a7a
--- /dev/null
+++ b/OvmfPkg/SioBusDxe/ComponentName.c
@@ -0,0 +1,167 @@
+/** @file
+ UEFI Component Name(2) protocol implementation for SioBusDxe driver.
+
+ Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions
+ of the BSD License which accompanies this distribution. The
+ full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include "SioBusDxe.h"
+
+//
+// Driver name table
+//
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_UNICODE_STRING_TABLE mSioBusDriverNameTable[] = {
+ { "eng;en", L"OVMF Sio Bus Driver" },
+ { NULL , NULL }
+};
+
+//
+// EFI Component Name Protocol
+//
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_COMPONENT_NAME_PROTOCOL gSioBusComponentName = {
+ SioBusComponentNameGetDriverName,
+ SioBusComponentNameGetControllerName,
+ "eng"
+};
+
+//
+// EFI Component Name 2 Protocol
+//
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_COMPONENT_NAME2_PROTOCOL gSioBusComponentName2 = {
+ (EFI_COMPONENT_NAME2_GET_DRIVER_NAME) SioBusComponentNameGetDriverName,
+ (EFI_COMPONENT_NAME2_GET_CONTROLLER_NAME) SioBusComponentNameGetControllerName,
+ "en"
+};
+
+
+/**
+ Retrieves a Unicode string that is the user readable name of the driver.
+
+ This function retrieves the user readable name of a driver in the form of a
+ Unicode string. If the driver specified by This has a user readable name in
+ the language specified by Language, then a pointer to the driver name is
+ returned in DriverName, and EFI_SUCCESS is returned. If the driver specified
+ by This does not support the language specified by Language, then
+ EFI_UNSUPPORTED is returned.
+
+ @param[in] This A pointer to the EFI_COMPONENT_NAME2_PROTOCOL
+ or EFI_COMPONENT_NAME_PROTOCOL instance.
+ @param[in] Language A pointer to a Null-terminated ASCII string
+ array indicating the language. This is the
+ language of the driver name that the caller is
+ requesting, and it must match one of the
+ languages specified in SupportedLanguages. The
+ number of languages supported by a driver is up
+ to the driver writer. Language is specified
+ in RFC 4646 or ISO 639-2 language code format.
+ @param[out] DriverName A pointer to the Unicode string to return. This
+ Unicode string is the name of the driver
+ specified by This in the language specified by
+ Language.
+
+ @retval EFI_SUCCESS The Unicode string for the Driver specified by
+ This and the language specified by Language was
+ returned in DriverName.
+ @retval EFI_INVALID_PARAMETER Language is NULL.
+ @retval EFI_INVALID_PARAMETER DriverName is NULL.
+ @retval EFI_UNSUPPORTED The driver specified by This does not support
+ the language specified by Language.
+
+**/
+EFI_STATUS
+EFIAPI
+SioBusComponentNameGetDriverName (
+ IN EFI_COMPONENT_NAME_PROTOCOL *This,
+ IN CHAR8 *Language,
+ OUT CHAR16 **DriverName
+ )
+{
+ return LookupUnicodeString2 (
+ Language,
+ This->SupportedLanguages,
+ mSioBusDriverNameTable,
+ DriverName,
+ (BOOLEAN)(This == &gSioBusComponentName)
+ );
+}
+
+/**
+ Retrieves a Unicode string that is the user readable name of the controller
+ that is being managed by a driver.
+
+ This function retrieves the user readable name of the controller specified by
+ ControllerHandle and ChildHandle in the form of a Unicode string. If the
+ driver specified by This has a user readable name in the language specified
+ by Language, then a pointer to the controller name is returned in
+ ControllerName, and EFI_SUCCESS is returned. If the driver specified by This
+ is not currently managing the controller specified by ControllerHandle and
+ ChildHandle, then EFI_UNSUPPORTED is returned. If the driver specified by
+ This does not support the language specified by Language, then
+ EFI_UNSUPPORTED is returned.
+
+ @param[in] This A pointer to the EFI_COMPONENT_NAME2_PROTOCOL
+ or EFI_COMPONENT_NAME_PROTOCOL instance.
+ @param[in] ControllerHandle The handle of a controller that the driver
+ specified by This is managing. This handle
+ specifies the controller whose name is to be
+ returned.
+ @param[in] ChildHandle The handle of the child controller to retrieve
+ the name of. This is an optional parameter
+ that may be NULL. It will be NULL for device
+ drivers. It will also be NULL for a bus
+ drivers that wish to retrieve the name of the
+ bus controller. It will not be NULL for a bus
+ driver that wishes to retrieve the name of a
+ child controller.
+ @param[in] Language A pointer to a Null-terminated ASCII string
+ array indicating the language. This is the
+ language of the driver name that the caller is
+ requesting, and it must match one of the
+ languages specified in SupportedLanguages. The
+ number of languages supported by a driver is up
+ to the driver writer. Language is specified in
+ RFC 4646 or ISO 639-2 language code format.
+ @param[out] ControllerName A pointer to the Unicode string to return.
+ This Unicode string is the name of the
+ controller specified by ControllerHandle and
+ ChildHandle in the language specified by
+ Language from the point of view of the driver
+ specified by This.
+
+ @retval EFI_SUCCESS The Unicode string for the user readable name
+ in the language specified by Language for the
+ driver specified by This was returned in
+ DriverName.
+ @retval EFI_INVALID_PARAMETER ControllerHandle is NULL.
+ @retval EFI_INVALID_PARAMETER ChildHandle is not NULL and it is not a valid
+ EFI_HANDLE.
+ @retval EFI_INVALID_PARAMETER Language is NULL.
+ @retval EFI_INVALID_PARAMETER ControllerName is NULL.
+ @retval EFI_UNSUPPORTED The driver specified by This is not currently
+ managing the controller specified by
+ ControllerHandle and ChildHandle.
+ @retval EFI_UNSUPPORTED The driver specified by This does not support
+ the language specified by Language.
+
+**/
+EFI_STATUS
+EFIAPI
+SioBusComponentNameGetControllerName (
+ IN EFI_COMPONENT_NAME_PROTOCOL *This,
+ IN EFI_HANDLE ControllerHandle,
+ IN EFI_HANDLE ChildHandle OPTIONAL,
+ IN CHAR8 *Language,
+ OUT CHAR16 **ControllerName
+ )
+{
+ return EFI_UNSUPPORTED;
+}
diff --git a/OvmfPkg/SioBusDxe/SioBusDxe.c b/OvmfPkg/SioBusDxe/SioBusDxe.c
new file mode 100644
index 0000000000..0cf9a2b524
--- /dev/null
+++ b/OvmfPkg/SioBusDxe/SioBusDxe.c
@@ -0,0 +1,622 @@
+/** @file
+ The SioBusDxe driver is used to create child devices on the ISA bus and
+ installs the Super I/O protocols on them.
+
+ Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions
+ of the BSD License which accompanies this distribution. The
+ full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include "SioBusDxe.h"
+
+//
+// SioBus Driver Binding Protocol
+//
+EFI_DRIVER_BINDING_PROTOCOL gSioBusDriverBinding = {
+ SioBusDriverBindingSupported,
+ SioBusDriverBindingStart,
+ SioBusDriverBindingStop,
+ 0x10,
+ NULL,
+ NULL
+};
+
+
+/**
+ Tests to see if this driver supports a given controller. If a child device is
+ provided, it further tests to see if this driver supports creating a handle
+ for the specified child device.
+
+ This function checks to see if the driver specified by This supports the
+ device specified by ControllerHandle. Drivers will typically use the device
+ path attached to ControllerHandle and/or the services from the bus I/O
+ abstraction attached to ControllerHandle to determine if the driver supports
+ ControllerHandle. This function may be called many times during platform
+ initialization. In order to reduce boot times, the tests performed by this
+ function must be very small, and take as little time as possible to execute.
+ This function must not change the state of any hardware devices, and this
+ function must be aware that the device specified by ControllerHandle may
+ already be managed by the same driver or a different driver. This function
+ must match its calls to AllocatePages() with FreePages(), AllocatePool() with
+ FreePool(), and OpenProtocol() with CloseProtocol(). Since ControllerHandle
+ may have been previously started by the same driver, if a protocol is already
+ in the opened state, then it must not be closed with CloseProtocol(). This is
+ required to guarantee the state of ControllerHandle is not modified by this
+ function.
+
+ @param[in] This A pointer to the EFI_DRIVER_BINDING_PROTOCOL
+ instance.
+ @param[in] ControllerHandle The handle of the controller to test. This
+ handle must support a protocol interface
+ that supplies an I/O abstraction to the
+ driver.
+ @param[in] RemainingDevicePath A pointer to the remaining portion of a
+ device path. This parameter is ignored by
+ device drivers, and is optional for bus
+ drivers. For bus drivers, if this parameter
+ is not NULL, then the bus driver must
+ determine if the bus controller specified by
+ ControllerHandle and the child controller
+ specified by RemainingDevicePath are both
+ supported by this bus driver.
+
+ @retval EFI_SUCCESS The device specified by ControllerHandle and
+ RemainingDevicePath is supported by the
+ driver specified by This.
+ @retval EFI_ALREADY_STARTED The device specified by ControllerHandle and
+ RemainingDevicePath is already being managed
+ by the driver specified by This.
+ @retval EFI_ACCESS_DENIED The device specified by ControllerHandle and
+ RemainingDevicePath is already being managed
+ by a different driver or an application that
+ requires exclusive access.
+ @retval EFI_UNSUPPORTED The device specified by ControllerHandle and
+ RemainingDevicePath is not supported by the
+ driver specified by This.
+
+**/
+EFI_STATUS
+EFIAPI
+SioBusDriverBindingSupported (
+ IN EFI_DRIVER_BINDING_PROTOCOL *This,
+ IN EFI_HANDLE Controller,
+ IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath
+ )
+{
+ EFI_STATUS Status;
+ EFI_PCI_IO_PROTOCOL *PciIo;
+ PCI_TYPE00 Pci;
+ UINTN SegmentNumber;
+ UINTN BusNumber;
+ UINTN DeviceNumber;
+ UINTN FunctionNumber;
+
+ //
+ // Get PciIo protocol instance
+ //
+ Status = gBS->OpenProtocol (
+ Controller,
+ &gEfiPciIoProtocolGuid,
+ (VOID**)&PciIo,
+ This->DriverBindingHandle,
+ Controller,
+ EFI_OPEN_PROTOCOL_BY_DRIVER
+ );
+ if (EFI_ERROR(Status)) {
+ return Status;
+ }
+
+ Status = PciIo->Pci.Read (
+ PciIo,
+ EfiPciIoWidthUint32,
+ 0,
+ sizeof(Pci) / sizeof(UINT32),
+ &Pci);
+
+ if (!EFI_ERROR (Status)) {
+ Status = EFI_UNSUPPORTED;
+ if ((Pci.Hdr.Command & 0x03) == 0x03) {
+ if (Pci.Hdr.ClassCode[2] == PCI_CLASS_BRIDGE) {
+ //
+ // See if this is a standard PCI to ISA Bridge from the Base Code and
+ // Class Code
+ //
+ if (Pci.Hdr.ClassCode[1] == PCI_CLASS_BRIDGE_ISA) {
+ Status = EFI_SUCCESS;
+ }
+
+ //
+ // See if this is an Intel PCI to ISA bridge in Positive Decode Mode
+ //
+ if (Pci.Hdr.ClassCode[1] == PCI_CLASS_BRIDGE_ISA_PDECODE &&
+ Pci.Hdr.VendorId == 0x8086) {
+ //
+ // See if this is on Function #0 to avoid false positives on
+ // PCI_CLASS_BRIDGE_OTHER that has the same value as
+ // PCI_CLASS_BRIDGE_ISA_PDECODE
+ //
+ Status = PciIo->GetLocation (
+ PciIo,
+ &SegmentNumber,
+ &BusNumber,
+ &DeviceNumber,
+ &FunctionNumber
+ );
+ if (!EFI_ERROR (Status) && FunctionNumber == 0) {
+ Status = EFI_SUCCESS;
+ } else {
+ Status = EFI_UNSUPPORTED;
+ }
+ }
+ }
+ }
+ }
+
+ gBS->CloseProtocol (
+ Controller,
+ &gEfiPciIoProtocolGuid,
+ This->DriverBindingHandle,
+ Controller
+ );
+
+ return Status;
+}
+
+/**
+ Starts a device controller or a bus controller.
+
+ The Start() function is designed to be invoked from the EFI boot service
+ ConnectController(). As a result, much of the error checking on the
+ parameters to Start() has been moved into this common boot service. It is
+ legal to call Start() from other locations, but the following calling
+ restrictions must be followed or the system behavior will not be
+ deterministic.
+ 1. ControllerHandle must be a valid EFI_HANDLE.
+ 2. If RemainingDevicePath is not NULL, then it must be a pointer to a
+ naturally aligned EFI_DEVICE_PATH_PROTOCOL.
+ 3. Prior to calling Start(), the Supported() function for the driver
+ specified by This must have been called with the same calling parameters,
+ and Supported() must have returned EFI_SUCCESS.
+
+ @param[in] This A pointer to the EFI_DRIVER_BINDING_PROTOCOL
+ instance.
+ @param[in] ControllerHandle The handle of the controller to start. This
+ handle must support a protocol interface
+ that supplies an I/O abstraction to the
+ driver.
+ @param[in] RemainingDevicePath A pointer to the remaining portion of a
+ device path. This parameter is ignored by
+ device drivers, and is optional for bus
+ drivers. For a bus driver, if this parameter
+ is NULL, then handles for all the children
+ of Controller are created by this driver. If
+ this parameter is not NULL and the first
+ Device Path Node is not the End of Device
+ Path Node, then only the handle for the
+ child device specified by the first Device
+ Path Node of RemainingDevicePath is created
+ by this driver. If the first Device Path
+ Node of RemainingDevicePath is the End of
+ Device Path Node, no child handle is created
+ by this driver.
+
+ @retval EFI_SUCCESS The device was started.
+ @retval EFI_DEVICE_ERROR The device could not be started due to a
+ device error.
+ @retval EFI_OUT_OF_RESOURCES The request could not be completed due to a
+ lack of resources.
+ @retval Others The driver failded to start the device.
+
+**/
+EFI_STATUS
+EFIAPI
+SioBusDriverBindingStart (
+ IN EFI_DRIVER_BINDING_PROTOCOL *This,
+ IN EFI_HANDLE Controller,
+ IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath
+ )
+{
+ EFI_STATUS Status;
+ EFI_PCI_IO_PROTOCOL *PciIo;
+ EFI_DEVICE_PATH_PROTOCOL *ParentDevicePath;
+ UINT64 Supports;
+ UINT64 OriginalAttributes;
+ UINT64 Attributes;
+ BOOLEAN Enabled;
+ SIO_BUS_DRIVER_PRIVATE_DATA *Private;
+ UINT32 ChildDeviceNumber;
+
+ Enabled = FALSE;
+ Supports = 0;
+ OriginalAttributes = 0;
+ Private = NULL;
+
+ //
+ // Open the PCI I/O Protocol Interface
+ //
+ PciIo = NULL;
+ Status = gBS->OpenProtocol (
+ Controller,
+ &gEfiPciIoProtocolGuid,
+ (VOID**) &PciIo,
+ This->DriverBindingHandle,
+ Controller,
+ EFI_OPEN_PROTOCOL_BY_DRIVER
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // Open Device Path Protocol
+ //
+ Status = gBS->OpenProtocol (
+ Controller,
+ &gEfiDevicePathProtocolGuid,
+ (VOID **) &ParentDevicePath,
+ This->DriverBindingHandle,
+ Controller,
+ EFI_OPEN_PROTOCOL_BY_DRIVER
+ );
+ if (EFI_ERROR (Status)) {
+ gBS->CloseProtocol (
+ Controller,
+ &gEfiPciIoProtocolGuid,
+ This->DriverBindingHandle,
+ Controller
+ );
+ return Status;
+ }
+
+ //
+ // Get supported PCI attributes
+ //
+ Status = PciIo->Attributes (
+ PciIo,
+ EfiPciIoAttributeOperationSupported,
+ 0,
+ &Supports
+ );
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ Supports &= (UINT64) (EFI_PCI_IO_ATTRIBUTE_ISA_IO |
+ EFI_PCI_IO_ATTRIBUTE_ISA_IO_16);
+ if (Supports == 0 ||
+ Supports == (EFI_PCI_IO_ATTRIBUTE_ISA_IO |
+ EFI_PCI_IO_ATTRIBUTE_ISA_IO_16)) {
+ Status = EFI_UNSUPPORTED;
+ goto Done;
+ }
+
+ Status = PciIo->Attributes (
+ PciIo,
+ EfiPciIoAttributeOperationGet,
+ 0,
+ &OriginalAttributes
+ );
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ Attributes = EFI_PCI_DEVICE_ENABLE |
+ Supports |
+ EFI_PCI_IO_ATTRIBUTE_ISA_MOTHERBOARD_IO;
+ Status = PciIo->Attributes (
+ PciIo,
+ EfiPciIoAttributeOperationEnable,
+ Attributes,
+ NULL
+ );
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ Enabled = TRUE;
+
+ //
+ // Store the OriginalAttributes for the restore in BindingStop()
+ //
+ Private = AllocateZeroPool (sizeof (SIO_BUS_DRIVER_PRIVATE_DATA));
+ if (Private == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Done;
+ }
+ Private->PciIo = PciIo;
+ Private->OriginalAttributes = OriginalAttributes;
+
+ Status = gBS->InstallProtocolInterface (
+ &Controller,
+ &gEfiCallerIdGuid,
+ EFI_NATIVE_INTERFACE,
+ Private
+ );
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ //
+ // Report status code for the start of general controller initialization
+ //
+ REPORT_STATUS_CODE_WITH_DEVICE_PATH (
+ EFI_PROGRESS_CODE,
+ (EFI_IO_BUS_LPC | EFI_IOB_PC_INIT),
+ ParentDevicePath
+ );
+
+ //
+ // Report status code for the start of enabling devices on the bus
+ //
+ REPORT_STATUS_CODE_WITH_DEVICE_PATH (
+ EFI_PROGRESS_CODE,
+ (EFI_IO_BUS_LPC | EFI_IOB_PC_ENABLE),
+ ParentDevicePath
+ );
+
+ //
+ // Create all the children upon the first entrance
+ //
+ ChildDeviceNumber = SioCreateAllChildDevices (
+ This,
+ Controller,
+ PciIo,
+ ParentDevicePath
+ );
+ if (ChildDeviceNumber == 0) {
+ Status = EFI_DEVICE_ERROR;
+ }
+
+Done:
+ if (EFI_ERROR (Status)) {
+ if (PciIo != NULL && Enabled) {
+ PciIo->Attributes (
+ PciIo,
+ EfiPciIoAttributeOperationSet,
+ OriginalAttributes,
+ NULL
+ );
+ }
+
+ gBS->CloseProtocol (
+ Controller,
+ &gEfiDevicePathProtocolGuid,
+ This->DriverBindingHandle,
+ Controller
+ );
+
+ gBS->CloseProtocol (
+ Controller,
+ &gEfiPciIoProtocolGuid,
+ This->DriverBindingHandle,
+ Controller
+ );
+
+ if (Private != NULL) {
+ gBS->UninstallMultipleProtocolInterfaces (
+ Controller,
+ &gEfiCallerIdGuid,
+ Private,
+ NULL
+ );
+ FreePool (Private);
+ }
+
+ return Status;
+ }
+
+ return EFI_SUCCESS;
+}
+
+/**
+ Stops a device controller or a bus controller.
+
+ The Stop() function is designed to be invoked from the EFI boot service
+ DisconnectController(). As a result, much of the error checking on the
+ parameters to Stop() has been moved into this common boot service. It is
+ legal to call Stop() from other locations, but the following calling
+ restrictions must be followed or the system behavior will not be
+ deterministic.
+ 1. ControllerHandle must be a valid EFI_HANDLE that was used on a previous
+ call to this same driver's Start() function.
+ 2. The first NumberOfChildren handles of ChildHandleBuffer must all be a
+ valid EFI_HANDLE. In addition, all of these handles must have been created
+ in this driver's Start() function, and the Start() function must have
+ called OpenProtocol() on ControllerHandle with an Attribute of
+ EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER.
+
+ @param[in] This A pointer to the EFI_DRIVER_BINDING_PROTOCOL
+ instance.
+ @param[in] ControllerHandle A handle to the device being stopped. The
+ handle must support a bus specific I/O
+ protocol for the driver to use to stop the
+ device.
+ @param[in] NumberOfChildren The number of child device handles in
+ ChildHandleBuffer.
+ @param[in] ChildHandleBuffer An array of child handles to be freed. May be
+ NULL if NumberOfChildren is 0.
+
+ @retval EFI_SUCCESS The device was stopped.
+ @retval EFI_DEVICE_ERROR The device could not be stopped due to a
+ device error.
+
+**/
+EFI_STATUS
+EFIAPI
+SioBusDriverBindingStop (
+ IN EFI_DRIVER_BINDING_PROTOCOL *This,
+ IN EFI_HANDLE Controller,
+ IN UINTN NumberOfChildren,
+ IN EFI_HANDLE *ChildHandleBuffer
+ )
+{
+ EFI_STATUS Status;
+ SIO_BUS_DRIVER_PRIVATE_DATA *Private;
+ UINTN Index;
+ BOOLEAN AllChildrenStopped;
+ EFI_SIO_PROTOCOL *Sio;
+ SIO_DEV *SioDevice;
+ EFI_PCI_IO_PROTOCOL *PciIo;
+
+ if (NumberOfChildren == 0) {
+ //
+ // Restore PCI attributes
+ //
+ Status = gBS->OpenProtocol (
+ Controller,
+ &gEfiCallerIdGuid,
+ (VOID **) &Private,
+ This->DriverBindingHandle,
+ Controller,
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = Private->PciIo->Attributes (
+ Private->PciIo,
+ EfiPciIoAttributeOperationSet,
+ Private->OriginalAttributes,
+ NULL
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ gBS->UninstallProtocolInterface (
+ Controller,
+ &gEfiCallerIdGuid,
+ Private
+ );
+ FreePool (Private);
+
+ //
+ // Close the bus driver
+ //
+ Status = gBS->CloseProtocol (
+ Controller,
+ &gEfiDevicePathProtocolGuid,
+ This->DriverBindingHandle,
+ Controller
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = gBS->CloseProtocol (
+ Controller,
+ &gEfiPciIoProtocolGuid,
+ This->DriverBindingHandle,
+ Controller
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ return EFI_SUCCESS;
+ }
+
+ //
+ // Stop all the children
+ //
+ AllChildrenStopped = TRUE;
+
+ for (Index = 0; Index < NumberOfChildren; Index++) {
+ Status = gBS->OpenProtocol (
+ ChildHandleBuffer[Index],
+ &gEfiSioProtocolGuid,
+ (VOID **) &Sio,
+ This->DriverBindingHandle,
+ Controller,
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL
+ );
+ if (!EFI_ERROR (Status)) {
+ SioDevice = SIO_DEV_FROM_SIO (Sio);
+
+ //
+ // Close the child handle
+ //
+ Status = gBS->CloseProtocol (
+ Controller,
+ &gEfiPciIoProtocolGuid,
+ This->DriverBindingHandle,
+ ChildHandleBuffer[Index]
+ );
+ Status = gBS->UninstallMultipleProtocolInterfaces (
+ ChildHandleBuffer[Index],
+ &gEfiDevicePathProtocolGuid,
+ SioDevice->DevicePath,
+ &gEfiSioProtocolGuid,
+ &SioDevice->Sio,
+ NULL
+ );
+
+ if (!EFI_ERROR (Status)) {
+ FreePool (SioDevice->DevicePath);
+ FreePool (SioDevice);
+ } else {
+ //
+ // Re-open PCI IO Protocol on behalf of the child device
+ // because of failure of destroying the child device handle
+ //
+ gBS->OpenProtocol (
+ Controller,
+ &gEfiPciIoProtocolGuid,
+ (VOID **) &PciIo,
+ This->DriverBindingHandle,
+ ChildHandleBuffer[Index],
+ EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
+ );
+ }
+ }
+
+ if (EFI_ERROR (Status)) {
+ AllChildrenStopped = FALSE;
+ }
+ }
+
+ if (!AllChildrenStopped) {
+ return EFI_DEVICE_ERROR;
+ }
+
+ return EFI_SUCCESS;
+}
+
+/**
+ The entry point for the SioBusDxe driver.
+
+ @param[in] ImageHandle The firmware allocated handle for the EFI image.
+ @param[in] SystemTable A pointer to the EFI System Table.
+
+ @retval EFI_SUCCESS The entry point is executed successfully.
+ @retval other Some error occurs when executing this entry point.
+
+**/
+EFI_STATUS
+EFIAPI
+SioBusDxeDriverEntryPoint (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ //
+ // Install driver model protocol(s).
+ //
+ return EfiLibInstallDriverBindingComponentName2 (
+ ImageHandle,
+ SystemTable,
+ &gSioBusDriverBinding,
+ ImageHandle,
+ &gSioBusComponentName,
+ &gSioBusComponentName2
+ );
+}
diff --git a/OvmfPkg/SioBusDxe/SioService.c b/OvmfPkg/SioBusDxe/SioService.c
new file mode 100644
index 0000000000..f160406edf
--- /dev/null
+++ b/OvmfPkg/SioBusDxe/SioService.c
@@ -0,0 +1,405 @@
+/** @file
+ The SioBusDxe driver is used to create child devices on the ISA bus and
+ installs the Super I/O protocols on them.
+
+ Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions
+ of the BSD License which accompanies this distribution. The
+ full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include "SioBusDxe.h"
+
+//
+// Super I/O Protocol interfaces
+//
+EFI_SIO_PROTOCOL mSioInterface = {
+ SioRegisterAccess,
+ SioGetResources,
+ SioSetResources,
+ SioPossibleResources,
+ SioModify
+};
+
+//
+// COM 1 UART Controller
+//
+GLOBAL_REMOVE_IF_UNREFERENCED
+SIO_RESOURCES_IO mCom1Resources = {
+ { { ACPI_FIXED_LOCATION_IO_PORT_DESCRIPTOR }, 0x3F8, 8 },
+ { ACPI_END_TAG_DESCRIPTOR, 0 }
+};
+
+//
+// COM 2 UART Controller
+//
+GLOBAL_REMOVE_IF_UNREFERENCED
+SIO_RESOURCES_IO mCom2Resources = {
+ { { ACPI_FIXED_LOCATION_IO_PORT_DESCRIPTOR }, 0x2F8, 8 },
+ { ACPI_END_TAG_DESCRIPTOR, 0 }
+};
+
+//
+// PS/2 Keyboard Controller
+//
+GLOBAL_REMOVE_IF_UNREFERENCED
+SIO_RESOURCES_IO mPs2KeyboardDeviceResources = {
+ { { ACPI_FIXED_LOCATION_IO_PORT_DESCRIPTOR }, 0x60, 5 },
+ { ACPI_END_TAG_DESCRIPTOR, 0 }
+};
+
+//
+// Table of SIO Controllers
+//
+GLOBAL_REMOVE_IF_UNREFERENCED
+SIO_DEVICE_INFO mDevicesInfo[] = {
+ {
+ EISA_PNP_ID (0x501),
+ 0,
+ { (ACPI_SMALL_RESOURCE_HEADER *) &mCom1Resources }
+ }, // COM 1 UART Controller
+ {
+ EISA_PNP_ID (0x501),
+ 1,
+ { (ACPI_SMALL_RESOURCE_HEADER *) &mCom2Resources }
+ }, // COM 2 UART Controller
+ {
+ EISA_PNP_ID(0x303),
+ 0,
+ { (ACPI_SMALL_RESOURCE_HEADER *) &mPs2KeyboardDeviceResources }
+ } // PS/2 Keyboard Controller
+};
+
+//
+// ACPI Device Path Node template
+//
+GLOBAL_REMOVE_IF_UNREFERENCED
+ACPI_HID_DEVICE_PATH mAcpiDeviceNodeTemplate = {
+ { // Header
+ ACPI_DEVICE_PATH,
+ ACPI_DP,
+ {
+ (UINT8) (sizeof (ACPI_HID_DEVICE_PATH)),
+ (UINT8) ((sizeof (ACPI_HID_DEVICE_PATH)) >> 8)
+ }
+ },
+ 0x0, // HID
+ 0x0 // UID
+};
+
+
+/**
+ Provides a low level access to the registers for the Super I/O.
+
+ @param[in] This Indicates a pointer to the calling context.
+ @param[in] Write Specifies the type of the register operation.
+ If this parameter is TRUE, Value is interpreted
+ as an input parameter and the operation is a
+ register write. If this parameter is FALSE,
+ Value is interpreted as an output parameter and
+ the operation is a register read.
+ @param[in] ExitCfgMode Exit Configuration Mode Indicator. If this
+ parameter is set to TRUE, the Super I/O driver
+ will turn off configuration mode of the Super
+ I/O prior to returning from this function. If
+ this parameter is set to FALSE, the Super I/O
+ driver will leave Super I/O in the
+ configuration mode. The Super I/O driver must
+ track the current state of the Super I/O and
+ enable the configuration mode of Super I/O if
+ necessary prior to register access.
+ @param[in] Register Register number.
+ @param[in,out] Value If Write is TRUE, Value is a pointer to the
+ buffer containing the byte of data to be
+ written to the Super I/O register. If Write is
+ FALSE, Value is a pointer to the destination
+ buffer for the byte of data to be read from the
+ Super I/O register.
+
+ @retval EFI_SUCCESS The operation completed successfully.
+ @retval EFI_INVALID_PARAMETER The Value is NULL.
+ @retval EFI_INVALID_PARAMETER Invalid Register number.
+
+**/
+EFI_STATUS
+EFIAPI
+SioRegisterAccess (
+ IN CONST EFI_SIO_PROTOCOL *This,
+ IN BOOLEAN Write,
+ IN BOOLEAN ExitCfgMode,
+ IN UINT8 Register,
+ IN OUT UINT8 *Value
+ )
+{
+ return EFI_SUCCESS;
+}
+
+/**
+ Provides an interface to get a list of the current resources consumed by the
+ device in the ACPI Resource Descriptor format.
+
+ GetResources() returns a list of resources currently consumed by the device.
+ The ResourceList is a pointer to the buffer containing resource descriptors
+ for the device. The descriptors are in the format of Small or Large ACPI
+ resource descriptor as defined by ACPI specification (2.0 & 3.0). The buffer
+ of resource descriptors is terminated with the 'End tag' resource descriptor.
+
+ @param[in] This Indicates a pointer to the calling context.
+ @param[out] ResourceList A pointer to an ACPI resource descriptor list
+ that defines the current resources used by the
+ device.
+
+ @retval EFI_SUCCESS The operation completed successfully.
+ @retval EFI_INVALID_PARAMETER ResourceList is NULL.
+
+**/
+EFI_STATUS
+EFIAPI
+SioGetResources (
+ IN CONST EFI_SIO_PROTOCOL *This,
+ OUT ACPI_RESOURCE_HEADER_PTR *ResourceList
+ )
+{
+ SIO_DEV *SioDevice;
+
+ if (ResourceList == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ SioDevice = SIO_DEV_FROM_SIO (This);
+ if (SioDevice->DeviceIndex < ARRAY_SIZE (mDevicesInfo)) {
+ *ResourceList = mDevicesInfo[SioDevice->DeviceIndex].Resources;
+ }
+
+ return EFI_SUCCESS;
+}
+
+/**
+ Sets the resources for the device.
+
+ @param[in] This Indicates a pointer to the calling context.
+ @param[in] ResourceList Pointer to the ACPI resource descriptor list.
+
+ @retval EFI_SUCCESS The operation completed successfully.
+ @retval EFI_INVALID_PARAMETER ResourceList is invalid.
+ @retval EFI_ACCESS_DENIED Some of the resources in ResourceList are in
+ use.
+
+**/
+EFI_STATUS
+EFIAPI
+SioSetResources (
+ IN CONST EFI_SIO_PROTOCOL *This,
+ IN ACPI_RESOURCE_HEADER_PTR ResourceList
+ )
+{
+ return EFI_SUCCESS;
+}
+
+/**
+ Provides a collection of resource descriptor lists. Each resource descriptor
+ list in the collection defines a combination of resources that can
+ potentially be used by the device.
+
+ @param[in] This Indicates a pointer to the calling context.
+ @param[out] ResourceCollection Collection of the resource descriptor
+ lists.
+
+ @retval EFI_SUCCESS The operation completed successfully.
+ @retval EFI_INVALID_PARAMETER ResourceCollection is NULL.
+
+**/
+EFI_STATUS
+EFIAPI
+SioPossibleResources (
+ IN CONST EFI_SIO_PROTOCOL *This,
+ OUT ACPI_RESOURCE_HEADER_PTR *ResourceCollection
+ )
+{
+ return EFI_SUCCESS;
+}
+
+/**
+ Provides an interface for a table based programming of the Super I/O
+ registers.
+
+ The Modify() function provides an interface for table based programming of
+ the Super I/O registers. This function can be used to perform programming of
+ multiple Super I/O registers with a single function call. For each table
+ entry, the Register is read, its content is bitwise ANDed with AndMask, and
+ then ORed with OrMask before being written back to the Register. The Super
+ I/O driver must track the current state of the Super I/O and enable the
+ configuration mode of Super I/O if necessary prior to table processing. Once
+ the table is processed, the Super I/O device has to be returned to the
+ original state.
+
+ @param[in] This Indicates a pointer to the calling context.
+ @param[in] Command A pointer to an array of NumberOfCommands
+ EFI_SIO_REGISTER_MODIFY structures. Each
+ structure specifies a single Super I/O register
+ modify operation.
+ @param[in] NumberOfCommands Number of elements in the Command array.
+
+ @retval EFI_SUCCESS The operation completed successfully.
+ @retval EFI_INVALID_PARAMETER Command is NULL.
+
+**/
+EFI_STATUS
+EFIAPI
+SioModify (
+ IN CONST EFI_SIO_PROTOCOL *This,
+ IN CONST EFI_SIO_REGISTER_MODIFY *Command,
+ IN UINTN NumberOfCommands
+ )
+{
+ return EFI_SUCCESS;
+}
+
+/**
+ Create the child device with a given device index.
+
+ @param[in] This The EFI_DRIVER_BINDING_PROTOCOL instance.
+ @param[in] Controller The handle of ISA bus controller.
+ @param[in] PciIo The pointer to the PCI protocol.
+ @param[in] ParentDevicePath Device path of the ISA bus controller.
+ @param[in] DeviceIndex Index of the device supported by this driver.
+
+ @retval EFI_SUCCESS The child device has been created successfully.
+ @retval Others Error occured during the child device creation.
+
+**/
+EFI_STATUS
+SioCreateChildDevice (
+ IN EFI_DRIVER_BINDING_PROTOCOL *This,
+ IN EFI_HANDLE Controller,
+ IN EFI_PCI_IO_PROTOCOL *PciIo,
+ IN EFI_DEVICE_PATH_PROTOCOL *ParentDevicePath,
+ IN UINT32 DeviceIndex
+ )
+{
+ EFI_STATUS Status;
+ SIO_DEV *SioDevice;
+
+ //
+ // Initialize the PCI_IO_DEVICE structure
+ //
+ SioDevice = AllocateZeroPool (sizeof (SIO_DEV));
+ if (SioDevice == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ SioDevice->Signature = SIO_DEV_SIGNATURE;
+ SioDevice->Handle = NULL;
+ SioDevice->PciIo = PciIo;
+
+ //
+ // Construct the child device path
+ //
+ mAcpiDeviceNodeTemplate.HID = mDevicesInfo[DeviceIndex].Hid;
+ mAcpiDeviceNodeTemplate.UID = mDevicesInfo[DeviceIndex].Uid;
+ SioDevice->DevicePath = AppendDevicePathNode (
+ ParentDevicePath,
+ (EFI_DEVICE_PATH_PROTOCOL *) &mAcpiDeviceNodeTemplate
+ );
+ if (SioDevice->DevicePath == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Done;
+ }
+
+ CopyMem (&SioDevice->Sio, &mSioInterface, sizeof (EFI_SIO_PROTOCOL));
+ SioDevice->DeviceIndex = DeviceIndex;
+
+ //
+ // Create a child handle and install Device Path and Super I/O protocols
+ //
+ Status = gBS->InstallMultipleProtocolInterfaces (
+ &SioDevice->Handle,
+ &gEfiDevicePathProtocolGuid,
+ SioDevice->DevicePath,
+ &gEfiSioProtocolGuid,
+ &SioDevice->Sio,
+ NULL
+ );
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ Status = gBS->OpenProtocol (
+ Controller,
+ &gEfiPciIoProtocolGuid,
+ (VOID **) &PciIo,
+ This->DriverBindingHandle,
+ SioDevice->Handle,
+ EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
+ );
+ if (EFI_ERROR (Status)) {
+ gBS->UninstallMultipleProtocolInterfaces (
+ SioDevice->Handle,
+ &gEfiDevicePathProtocolGuid,
+ SioDevice->DevicePath,
+ &gEfiSioProtocolGuid,
+ &SioDevice->Sio,
+ NULL
+ );
+ }
+
+Done:
+ if (EFI_ERROR (Status)) {
+ if (SioDevice->DevicePath != NULL) {
+ FreePool (SioDevice->DevicePath);
+ }
+
+ FreePool (SioDevice);
+ }
+
+ return Status;
+}
+
+/**
+ Create all the ISA child devices on the ISA bus controller (PCI to ISA
+ bridge).
+
+ @param[in] This The EFI_DRIVER_BINDING_PROTOCOL instance.
+ @param[in] Controller The handle of ISA bus controller.
+ @param[in] PciIo The pointer to the PCI protocol.
+ @param[in] ParentDevicePath Device path of the ISA bus controller.
+
+ @retval The number of child device that is successfully created.
+
+**/
+UINT32
+SioCreateAllChildDevices (
+ IN EFI_DRIVER_BINDING_PROTOCOL *This,
+ IN EFI_HANDLE Controller,
+ IN EFI_PCI_IO_PROTOCOL *PciIo,
+ IN EFI_DEVICE_PATH_PROTOCOL *ParentDevicePath
+ )
+{
+ UINT32 Index;
+ UINT32 ChildDeviceNumber;
+ EFI_STATUS Status;
+
+ ChildDeviceNumber = 0;
+
+ for (Index = 0; Index < ARRAY_SIZE (mDevicesInfo); Index++) {
+ Status = SioCreateChildDevice (
+ This,
+ Controller,
+ PciIo,
+ ParentDevicePath,
+ Index
+ );
+ if (!EFI_ERROR (Status)) {
+ ChildDeviceNumber++;
+ }
+ }
+
+ return ChildDeviceNumber;
+}
diff --git a/OvmfPkg/SioBusDxe/SioBusDxe.uni b/OvmfPkg/SioBusDxe/SioBusDxe.uni
new file mode 100644
index 0000000000..4793180557
--- /dev/null
+++ b/OvmfPkg/SioBusDxe/SioBusDxe.uni
@@ -0,0 +1,21 @@
+// /** @file
+// The SioBusDxe driver is used to create child devices on the ISA bus and
+// installs the Super I/O protocols on them.
+//
+// Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+//
+// This program and the accompanying materials
+// are licensed and made available under the terms and conditions
+// of the BSD License which accompanies this distribution. The
+// full text of the license may be found at
+// http://opensource.org/licenses/bsd-license.php
+//
+// THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+// WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+//
+// **/
+
+
+#string STR_MODULE_ABSTRACT #language en-US "Manage ISA devices on the ISA bus"
+
+#string STR_MODULE_DESCRIPTION #language en-US "The SioBusDxe driver is used to create child devices on the ISA bus and installs the Super I/O protocols on them."
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg
2019-03-15 7:16 [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg Hao Wu
2019-03-15 7:16 ` [PATCH v1 1/2] OvmfPkg: Drop the ISA Floppy device support Hao Wu
2019-03-15 7:16 ` [PATCH v1 2/2] OvmfPkg: Add an Super IO bus driver to replace the current ISA support Hao Wu
@ 2019-03-15 11:09 ` Ard Biesheuvel
2019-03-15 18:16 ` Jordan Justen
2019-03-18 3:45 ` Wu, Hao A
2 siblings, 2 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2019-03-15 11:09 UTC (permalink / raw)
To: Hao Wu; +Cc: edk2-devel@lists.01.org, Jordan Justen, Laszlo Ersek, Ray Ni
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?
- Regardless of the previous, adding the new driver should be a patch
of its own.
- I spotted an incorrect reference to PCI_IO_DEV in a comment (patch #2)
>
> 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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg
2019-03-15 11:09 ` [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg Ard Biesheuvel
@ 2019-03-15 18:16 ` Jordan Justen
2019-03-18 3:47 ` Wu, Hao A
2019-03-18 3:45 ` Wu, Hao A
1 sibling, 1 reply; 16+ messages in thread
From: Jordan Justen @ 2019-03-15 18:16 UTC (permalink / raw)
To: Ard Biesheuvel, Hao Wu; +Cc: edk2-devel@lists.01.org, Laszlo Ersek
On 2019-03-15 04:09:56, Ard Biesheuvel wrote:
> 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?
> - Regardless of the previous, adding the new driver should be a patch
> of its own.
Yeah. I think the first patch should add the new driver, and update
the .dsc files to build the new driver. But, don't update the .fdf, so
there shouldn't be a functional change to the OVMF output after the
first patch.
Then the second patch can modify the .dsc and .fdf files to swap out
the drivers.
-Jordan
> - I spotted an incorrect reference to PCI_IO_DEV in a comment (patch #2)
>
>
>
> >
> > 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
> >
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg
2019-03-15 11:09 ` [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg Ard Biesheuvel
2019-03-15 18:16 ` Jordan Justen
@ 2019-03-18 3:45 ` Wu, Hao A
2019-03-20 12:34 ` Laszlo Ersek
1 sibling, 1 reply; 16+ messages in thread
From: Wu, Hao A @ 2019-03-18 3:45 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Justen, Jordan L, Laszlo Ersek, Ni, Ray
> -----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.
> - 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
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg
2019-03-15 18:16 ` Jordan Justen
@ 2019-03-18 3:47 ` Wu, Hao A
0 siblings, 0 replies; 16+ messages in thread
From: Wu, Hao A @ 2019-03-18 3:47 UTC (permalink / raw)
To: Justen, Jordan L, Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Laszlo Ersek
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Jordan Justen
> Sent: Saturday, March 16, 2019 2:17 AM
> To: Ard Biesheuvel; Wu, Hao A
> Cc: edk2-devel@lists.01.org; Laszlo Ersek
> Subject: Re: [edk2] [PATCH v1 0/2] Ovmf: Stop using ISA drivers within
> IntelFrameworkModulePkg
>
> On 2019-03-15 04:09:56, Ard Biesheuvel wrote:
> > 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?
> > - Regardless of the previous, adding the new driver should be a patch
> > of its own.
>
> Yeah. I think the first patch should add the new driver, and update
> the .dsc files to build the new driver. But, don't update the .fdf, so
> there shouldn't be a functional change to the OVMF output after the
> first patch.
>
> Then the second patch can modify the .dsc and .fdf files to swap out
> the drivers.
Hello Jordan,
Agree. I will follow your suggestion in the next version of the patch.
Thanks for the feedback.
Best Regards,
Hao Wu
>
> -Jordan
>
> > - I spotted an incorrect reference to PCI_IO_DEV in a comment (patch #2)
> >
> >
> >
> > >
> > > 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
> > >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg
2019-03-18 3:45 ` Wu, Hao A
@ 2019-03-20 12:34 ` Laszlo Ersek
2019-03-21 6:44 ` Wu, Hao A
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2019-03-20 12:34 UTC (permalink / raw)
To: Wu, Hao A, Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Justen, Jordan L, Ni, Ray
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
>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg
2019-03-20 12:34 ` Laszlo Ersek
@ 2019-03-21 6:44 ` Wu, Hao A
2019-03-21 10:08 ` Ard Biesheuvel
0 siblings, 1 reply; 16+ messages in thread
From: Wu, Hao A @ 2019-03-21 6:44 UTC (permalink / raw)
To: Laszlo Ersek, Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Justen, Jordan L, Ni, Ray
> >>
> >> 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?
> 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?
Best Regards,
Hao Wu
>
> (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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg
2019-03-21 6:44 ` Wu, Hao A
@ 2019-03-21 10:08 ` Ard Biesheuvel
2019-03-21 19:03 ` Laszlo Ersek
0 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2019-03-21 10:08 UTC (permalink / raw)
To: Wu, Hao A
Cc: Laszlo Ersek, edk2-devel@lists.01.org, Justen, Jordan L, Ni, Ray
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?
>
> > 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?
>
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg
2019-03-21 10:08 ` Ard Biesheuvel
@ 2019-03-21 19:03 ` Laszlo Ersek
2019-03-22 1:33 ` Wu, Hao A
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2019-03-21 19:03 UTC (permalink / raw)
To: Ard Biesheuvel, Wu, Hao A
Cc: edk2-devel@lists.01.org, Justen, Jordan L, Ni, Ray
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg
2019-03-21 19:03 ` Laszlo Ersek
@ 2019-03-22 1:33 ` Wu, Hao A
2019-03-22 9:25 ` Laszlo Ersek
0 siblings, 1 reply; 16+ messages in thread
From: Wu, Hao A @ 2019-03-22 1:33 UTC (permalink / raw)
To: Laszlo Ersek, Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Justen, Jordan L, Ni, Ray
> -----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.
Best Regards,
Hao Wu
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg
2019-03-22 1:33 ` Wu, Hao A
@ 2019-03-22 9:25 ` Laszlo Ersek
2019-03-22 9:41 ` Ard Biesheuvel
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2019-03-22 9:25 UTC (permalink / raw)
To: Wu, Hao A, Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Justen, Jordan L, Ni, Ray
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.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg
2019-03-22 9:25 ` Laszlo Ersek
@ 2019-03-22 9:41 ` Ard Biesheuvel
2019-03-22 10:55 ` Laszlo Ersek
0 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2019-03-22 9:41 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Wu, Hao A, edk2-devel@lists.01.org, Justen, Jordan L, Ni, Ray
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.
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg
2019-03-22 9:41 ` Ard Biesheuvel
@ 2019-03-22 10:55 ` Laszlo Ersek
2019-03-25 2:19 ` Wu, Hao A
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2019-03-22 10:55 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Wu, Hao A, edk2-devel@lists.01.org, Justen, Jordan L, Ni, Ray
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg
2019-03-22 10:55 ` Laszlo Ersek
@ 2019-03-25 2:19 ` Wu, Hao A
0 siblings, 0 replies; 16+ messages in thread
From: Wu, Hao A @ 2019-03-25 2:19 UTC (permalink / raw)
To: Laszlo Ersek, Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Justen, Jordan L, Ni, Ray
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, March 22, 2019 6:56 PM
> To: Ard Biesheuvel
> Cc: Wu, Hao A; 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/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.
Agree.
I will post a V2 series following the direction described by:
1, 2, 3, 4a.
Best Regards,
Hao Wu
>
> Thanks,
> Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-03-25 2:19 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-15 7:16 [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg Hao Wu
2019-03-15 7:16 ` [PATCH v1 1/2] OvmfPkg: Drop the ISA Floppy device support Hao Wu
2019-03-15 7:16 ` [PATCH v1 2/2] OvmfPkg: Add an Super IO bus driver to replace the current ISA support Hao Wu
2019-03-15 11:09 ` [PATCH v1 0/2] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg Ard Biesheuvel
2019-03-15 18:16 ` Jordan Justen
2019-03-18 3:47 ` Wu, Hao A
2019-03-18 3:45 ` Wu, Hao A
2019-03-20 12:34 ` Laszlo Ersek
2019-03-21 6:44 ` Wu, Hao A
2019-03-21 10:08 ` Ard Biesheuvel
2019-03-21 19:03 ` Laszlo Ersek
2019-03-22 1:33 ` Wu, Hao A
2019-03-22 9:25 ` Laszlo Ersek
2019-03-22 9:41 ` Ard Biesheuvel
2019-03-22 10:55 ` Laszlo Ersek
2019-03-25 2:19 ` Wu, Hao A
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox