* [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
@ 2018-02-24 14:25 Ard Biesheuvel
2018-02-24 14:25 ` [PATCH edk2-platforms 2/2] Silicon/SynQuacerI2cDxe: remove special runtime treatment of DEBUG()s Ard Biesheuvel
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2018-02-24 14:25 UTC (permalink / raw)
To: edk2-devel; +Cc: leif.lindholm, lersek, Ard Biesheuvel
Commit 4bf95a9f361e ("MdeModulePkg/ResetSystemRuntimeDxe: Add more
debug message") broke the DEBUG build for all platforms that rely on
MMIO mapped UART devices, since it introduces a DEBUG() print that
may trigger at runtime, at which such UART devices are usually not
mapped, resulting in an OS crash.
Given that this mostly only affects ARM and AARCH64, it is not unlikely
that similar inadvertent breakage will occur again in the future, so
let's fix this once and for all by switching affected platforms to the
new DxeRuntimeDebugLibSerialPort DebugLib implementation that takes care
not to touch the UART hardware after ExitBootServices().
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
Platform/AMD/OverdriveBoard/OverdriveBoard.dsc | 3 +++
Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc | 3 +++
Platform/LeMaker/CelloBoard/CelloBoard.dsc | 3 +++
Platform/Socionext/DeveloperBox/DeveloperBox.dsc | 3 +++
Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc | 3 +++
Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc | 3 +++
Silicon/Hisilicon/Hisilicon.dsc.inc | 3 +++
Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 3 +++
8 files changed, 24 insertions(+)
diff --git a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
index 7d85b78642da..348828e18d44 100644
--- a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
+++ b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
@@ -227,6 +227,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
+!if $(TARGET) != RELEASE
+ DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
+!endif
[LibraryClasses.common.UEFI_APPLICATION]
UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
diff --git a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
index b026ce3a420a..7cb47937329e 100644
--- a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
+++ b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
@@ -227,6 +227,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
!if $(SECURE_BOOT_ENABLE) == TRUE
BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
!endif
+!if $(TARGET) != RELEASE
+ DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
+!endif
[LibraryClasses.ARM, LibraryClasses.AARCH64]
#
diff --git a/Platform/LeMaker/CelloBoard/CelloBoard.dsc b/Platform/LeMaker/CelloBoard/CelloBoard.dsc
index 2468583c0daa..569e444f6b26 100644
--- a/Platform/LeMaker/CelloBoard/CelloBoard.dsc
+++ b/Platform/LeMaker/CelloBoard/CelloBoard.dsc
@@ -211,6 +211,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
+!if $(TARGET) != RELEASE
+ DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
+!endif
[LibraryClasses.common.UEFI_APPLICATION]
UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
index 8450d7800e43..925ce36d278b 100644
--- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
+++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
@@ -208,6 +208,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
+!if $(TARGET) != RELEASE
+ DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
+!endif
################################################################################
#
diff --git a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
index 45ab2afc4069..2d5a94ed1dab 100644
--- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
+++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
@@ -203,6 +203,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
+!if $(TARGET) != RELEASE
+ DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
+!endif
################################################################################
#
diff --git a/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc b/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
index f0a7e9794153..3f4c7c8a3eef 100644
--- a/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
+++ b/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
@@ -217,6 +217,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
+!if $(TARGET) != RELEASE
+ DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
+!endif
[LibraryClasses.common.UEFI_APPLICATION]
UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
diff --git a/Silicon/Hisilicon/Hisilicon.dsc.inc b/Silicon/Hisilicon/Hisilicon.dsc.inc
index 9050f29be381..b196322a20e2 100644
--- a/Silicon/Hisilicon/Hisilicon.dsc.inc
+++ b/Silicon/Hisilicon/Hisilicon.dsc.inc
@@ -213,6 +213,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
!ifndef CONFIG_NO_DEBUGLIB
DebugLib|IntelFrameworkModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
!endif
+!if $(TARGET) != RELEASE
+ DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
+!endif
[LibraryClasses.AARCH64]
ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf
diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
index ef70b5216727..535cc3ff4c19 100644
--- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
+++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
@@ -184,6 +184,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
+!if $(TARGET) != RELEASE
+ DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
+!endif
[LibraryClasses.ARM, LibraryClasses.AARCH64]
#
--
2.11.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH edk2-platforms 2/2] Silicon/SynQuacerI2cDxe: remove special runtime treatment of DEBUG()s
2018-02-24 14:25 [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers Ard Biesheuvel
@ 2018-02-24 14:25 ` Ard Biesheuvel
2018-02-26 14:50 ` [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers Laszlo Ersek
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2018-02-24 14:25 UTC (permalink / raw)
To: edk2-devel; +Cc: leif.lindholm, lersek, Ard Biesheuvel
It is no longer necessary to inhibit DEBUG() calls at runtime, due to
the fact that we switched to a DebugLib implementation that takes care
of this. Since a platform could theoretically wire up DXE_RUNTIME_DRIVER
modules to a DebugLib/SerialPortLib implementation combo that does remap
the UART MMIO region for runtime and produces UEFI debug output on a UART
that is not owned by the OS, it is better not to handle this in the
driver directly.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 28 +++++++++-----------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
index 5e70f9d921c3..415e3f0804ed 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
+++ b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
@@ -13,8 +13,6 @@
#include "SynQuacerI2cDxe.h"
-#define BOOTTIME_DEBUG(x) do { if (!EfiAtRuntime()) DEBUG (x); } while (0)
-
//
// We cannot use Stall () or timer events at runtime, so we need to busy-wait
// for the controller to signal the completion interrupts. This value was
@@ -168,27 +166,27 @@ SynQuacerI2cMasterStart (
MmioWrite8 (I2c->MmioBase + F_I2C_REG_DAR, SlaveAddress << 1);
}
- BOOTTIME_DEBUG ((DEBUG_INFO, "%a: slave:0x%02x\n", __FUNCTION__,
+ DEBUG ((DEBUG_INFO, "%a: slave:0x%02x\n", __FUNCTION__,
SlaveAddress));
Bsr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BSR);
Bcr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BCR);
if ((Bsr & F_I2C_BSR_BB) && !(Bcr & F_I2C_BCR_MSS)) {
- BOOTTIME_DEBUG ((DEBUG_INFO, "%a: bus is busy\n", __FUNCTION__));
+ DEBUG ((DEBUG_INFO, "%a: bus is busy\n", __FUNCTION__));
return EFI_ALREADY_STARTED;
}
if (Bsr & F_I2C_BSR_BB) { // Bus is busy
- BOOTTIME_DEBUG ((DEBUG_INFO, "%a: Continuous Start\n", __FUNCTION__));
+ DEBUG ((DEBUG_INFO, "%a: Continuous Start\n", __FUNCTION__));
MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, Bcr | F_I2C_BCR_SCC);
} else {
if (Bcr & F_I2C_BCR_MSS) {
- BOOTTIME_DEBUG ((DEBUG_WARN,
+ DEBUG ((DEBUG_WARN,
"%a: is not in master mode\n", __FUNCTION__));
return EFI_DEVICE_ERROR;
}
- BOOTTIME_DEBUG ((DEBUG_INFO, "%a: Start Condition\n", __FUNCTION__));
+ DEBUG ((DEBUG_INFO, "%a: Start Condition\n", __FUNCTION__));
MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR,
Bcr | F_I2C_BCR_MSS | F_I2C_BCR_INTE | F_I2C_BCR_BEIE);
}
@@ -329,13 +327,13 @@ SynQuacerI2cStartRequest (
Status = WaitForInterrupt (I2c);
if (EFI_ERROR (Status)) {
- BOOTTIME_DEBUG ((DEBUG_WARN, "%a: Timeout waiting for interrupt - %r\n",
+ DEBUG ((DEBUG_WARN, "%a: Timeout waiting for interrupt - %r\n",
__FUNCTION__, Status));
break;
}
if (MmioRead8 (I2c->MmioBase + F_I2C_REG_BSR) & F_I2C_BSR_LRB) {
- BOOTTIME_DEBUG ((DEBUG_WARN, "%a: No ack received\n", __FUNCTION__));
+ DEBUG ((DEBUG_WARN, "%a: No ack received\n", __FUNCTION__));
Status = EFI_DEVICE_ERROR;
break;
}
@@ -346,13 +344,13 @@ SynQuacerI2cStartRequest (
Bcr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BCR);
if (Bcr & F_I2C_BCR_BER) {
- BOOTTIME_DEBUG ((DEBUG_WARN, "%a: Bus error detected\n", __FUNCTION__));
+ DEBUG ((DEBUG_WARN, "%a: Bus error detected\n", __FUNCTION__));
Status = EFI_DEVICE_ERROR;
break;
}
if ((Bsr & F_I2C_BSR_AL) || !(Bcr & F_I2C_BCR_MSS)) {
- BOOTTIME_DEBUG ((DEBUG_WARN, "%a: Arbitration lost\n", __FUNCTION__));
+ DEBUG ((DEBUG_WARN, "%a: Arbitration lost\n", __FUNCTION__));
Status = EFI_DEVICE_ERROR;
break;
}
@@ -368,7 +366,7 @@ SynQuacerI2cStartRequest (
Status = WaitForInterrupt (I2c);
if (EFI_ERROR (Status)) {
- BOOTTIME_DEBUG ((DEBUG_WARN,
+ DEBUG ((DEBUG_WARN,
"%a: Timeout waiting for interrupt - %r\n", __FUNCTION__, Status));
break;
}
@@ -383,13 +381,13 @@ SynQuacerI2cStartRequest (
Status = WaitForInterrupt (I2c);
if (EFI_ERROR (Status)) {
- BOOTTIME_DEBUG ((DEBUG_WARN,
+ DEBUG ((DEBUG_WARN,
"%a: Timeout waiting for interrupt - %r\n", __FUNCTION__, Status));
break;
}
if (MmioRead8 (I2c->MmioBase + F_I2C_REG_BSR) & F_I2C_BSR_LRB) {
- BOOTTIME_DEBUG ((DEBUG_WARN, "%a: No ack received\n", __FUNCTION__));
+ DEBUG ((DEBUG_WARN, "%a: No ack received\n", __FUNCTION__));
Status = EFI_DEVICE_ERROR;
break;
}
@@ -486,7 +484,7 @@ SynQuacerI2cInit (
Dev->Resources[0].AddrLen,
EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
if (EFI_ERROR (Status)) {
- BOOTTIME_DEBUG ((DEBUG_WARN, "%a: failed to add memory space - %r\n",
+ DEBUG ((DEBUG_WARN, "%a: failed to add memory space - %r\n",
__FUNCTION__, Status));
}
--
2.11.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
2018-02-24 14:25 [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers Ard Biesheuvel
2018-02-24 14:25 ` [PATCH edk2-platforms 2/2] Silicon/SynQuacerI2cDxe: remove special runtime treatment of DEBUG()s Ard Biesheuvel
@ 2018-02-26 14:50 ` Laszlo Ersek
2018-02-27 1:50 ` Guo Heyi
2018-02-27 10:39 ` Leif Lindholm
3 siblings, 0 replies; 16+ messages in thread
From: Laszlo Ersek @ 2018-02-26 14:50 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel; +Cc: leif.lindholm
On 02/24/18 15:25, Ard Biesheuvel wrote:
> Commit 4bf95a9f361e ("MdeModulePkg/ResetSystemRuntimeDxe: Add more
> debug message") broke the DEBUG build for all platforms that rely on
> MMIO mapped UART devices, since it introduces a DEBUG() print that
> may trigger at runtime, at which such UART devices are usually not
> mapped, resulting in an OS crash.
>
> Given that this mostly only affects ARM and AARCH64, it is not unlikely
> that similar inadvertent breakage will occur again in the future, so
> let's fix this once and for all by switching affected platforms to the
> new DxeRuntimeDebugLibSerialPort DebugLib implementation that takes care
> not to touch the UART hardware after ExitBootServices().
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Platform/AMD/OverdriveBoard/OverdriveBoard.dsc | 3 +++
> Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc | 3 +++
> Platform/LeMaker/CelloBoard/CelloBoard.dsc | 3 +++
> Platform/Socionext/DeveloperBox/DeveloperBox.dsc | 3 +++
> Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc | 3 +++
> Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc | 3 +++
> Silicon/Hisilicon/Hisilicon.dsc.inc | 3 +++
> Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 3 +++
> 8 files changed, 24 insertions(+)
>
> diff --git a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> index 7d85b78642da..348828e18d44 100644
> --- a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> +++ b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> @@ -227,6 +227,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.common.UEFI_APPLICATION]
> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
> diff --git a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
> index b026ce3a420a..7cb47937329e 100644
> --- a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
> +++ b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
> @@ -227,6 +227,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> !if $(SECURE_BOOT_ENABLE) == TRUE
> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> !endif
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.ARM, LibraryClasses.AARCH64]
> #
> diff --git a/Platform/LeMaker/CelloBoard/CelloBoard.dsc b/Platform/LeMaker/CelloBoard/CelloBoard.dsc
> index 2468583c0daa..569e444f6b26 100644
> --- a/Platform/LeMaker/CelloBoard/CelloBoard.dsc
> +++ b/Platform/LeMaker/CelloBoard/CelloBoard.dsc
> @@ -211,6 +211,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.common.UEFI_APPLICATION]
> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> index 8450d7800e43..925ce36d278b 100644
> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> @@ -208,6 +208,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
> ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
> BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> ################################################################################
> #
> diff --git a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> index 45ab2afc4069..2d5a94ed1dab 100644
> --- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> +++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> @@ -203,6 +203,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
> ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> ################################################################################
> #
> diff --git a/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc b/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
> index f0a7e9794153..3f4c7c8a3eef 100644
> --- a/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
> +++ b/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
> @@ -217,6 +217,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.common.UEFI_APPLICATION]
> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
> diff --git a/Silicon/Hisilicon/Hisilicon.dsc.inc b/Silicon/Hisilicon/Hisilicon.dsc.inc
> index 9050f29be381..b196322a20e2 100644
> --- a/Silicon/Hisilicon/Hisilicon.dsc.inc
> +++ b/Silicon/Hisilicon/Hisilicon.dsc.inc
> @@ -213,6 +213,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> !ifndef CONFIG_NO_DEBUGLIB
> DebugLib|IntelFrameworkModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
> !endif
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.AARCH64]
> ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf
> diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> index ef70b5216727..535cc3ff4c19 100644
> --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> @@ -184,6 +184,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.ARM, LibraryClasses.AARCH64]
> #
>
I'm not overly familiar with edk2-platforms, but the patches look sane.
Series
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
2018-02-24 14:25 [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers Ard Biesheuvel
2018-02-24 14:25 ` [PATCH edk2-platforms 2/2] Silicon/SynQuacerI2cDxe: remove special runtime treatment of DEBUG()s Ard Biesheuvel
2018-02-26 14:50 ` [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers Laszlo Ersek
@ 2018-02-27 1:50 ` Guo Heyi
2018-02-27 9:23 ` Ard Biesheuvel
2018-02-27 10:39 ` Leif Lindholm
3 siblings, 1 reply; 16+ messages in thread
From: Guo Heyi @ 2018-02-27 1:50 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, lersek, leif.lindholm
Hi Ard,
Sorry for the late of seeing this patch. I have one question: why don't we
implement a runtime serial port lib, which will switch UART base address in
virtual address map change? I think this will be useful when we want to debug
runtime driver in OS stage. And if we have a runtime version of SerialPortLib,
then we don't need a runtime version of DebugLib which just disable touching
serial port.
Thanks,
Heyi
On Sat, Feb 24, 2018 at 02:25:14PM +0000, Ard Biesheuvel wrote:
> Commit 4bf95a9f361e ("MdeModulePkg/ResetSystemRuntimeDxe: Add more
> debug message") broke the DEBUG build for all platforms that rely on
> MMIO mapped UART devices, since it introduces a DEBUG() print that
> may trigger at runtime, at which such UART devices are usually not
> mapped, resulting in an OS crash.
>
> Given that this mostly only affects ARM and AARCH64, it is not unlikely
> that similar inadvertent breakage will occur again in the future, so
> let's fix this once and for all by switching affected platforms to the
> new DxeRuntimeDebugLibSerialPort DebugLib implementation that takes care
> not to touch the UART hardware after ExitBootServices().
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Platform/AMD/OverdriveBoard/OverdriveBoard.dsc | 3 +++
> Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc | 3 +++
> Platform/LeMaker/CelloBoard/CelloBoard.dsc | 3 +++
> Platform/Socionext/DeveloperBox/DeveloperBox.dsc | 3 +++
> Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc | 3 +++
> Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc | 3 +++
> Silicon/Hisilicon/Hisilicon.dsc.inc | 3 +++
> Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 3 +++
> 8 files changed, 24 insertions(+)
>
> diff --git a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> index 7d85b78642da..348828e18d44 100644
> --- a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> +++ b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> @@ -227,6 +227,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.common.UEFI_APPLICATION]
> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
> diff --git a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
> index b026ce3a420a..7cb47937329e 100644
> --- a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
> +++ b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
> @@ -227,6 +227,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> !if $(SECURE_BOOT_ENABLE) == TRUE
> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> !endif
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.ARM, LibraryClasses.AARCH64]
> #
> diff --git a/Platform/LeMaker/CelloBoard/CelloBoard.dsc b/Platform/LeMaker/CelloBoard/CelloBoard.dsc
> index 2468583c0daa..569e444f6b26 100644
> --- a/Platform/LeMaker/CelloBoard/CelloBoard.dsc
> +++ b/Platform/LeMaker/CelloBoard/CelloBoard.dsc
> @@ -211,6 +211,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.common.UEFI_APPLICATION]
> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> index 8450d7800e43..925ce36d278b 100644
> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> @@ -208,6 +208,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
> ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
> BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> ################################################################################
> #
> diff --git a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> index 45ab2afc4069..2d5a94ed1dab 100644
> --- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> +++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> @@ -203,6 +203,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
> ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> ################################################################################
> #
> diff --git a/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc b/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
> index f0a7e9794153..3f4c7c8a3eef 100644
> --- a/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
> +++ b/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
> @@ -217,6 +217,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.common.UEFI_APPLICATION]
> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
> diff --git a/Silicon/Hisilicon/Hisilicon.dsc.inc b/Silicon/Hisilicon/Hisilicon.dsc.inc
> index 9050f29be381..b196322a20e2 100644
> --- a/Silicon/Hisilicon/Hisilicon.dsc.inc
> +++ b/Silicon/Hisilicon/Hisilicon.dsc.inc
> @@ -213,6 +213,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> !ifndef CONFIG_NO_DEBUGLIB
> DebugLib|IntelFrameworkModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
> !endif
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.AARCH64]
> ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf
> diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> index ef70b5216727..535cc3ff4c19 100644
> --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> @@ -184,6 +184,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.ARM, LibraryClasses.AARCH64]
> #
> --
> 2.11.0
>
> _______________________________________________
> 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 edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
2018-02-27 1:50 ` Guo Heyi
@ 2018-02-27 9:23 ` Ard Biesheuvel
2018-02-27 10:43 ` Laszlo Ersek
0 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2018-02-27 9:23 UTC (permalink / raw)
To: Guo Heyi; +Cc: edk2-devel@lists.01.org, Laszlo Ersek, Leif Lindholm
On 27 February 2018 at 01:50, Guo Heyi <heyi.guo@linaro.org> wrote:
> Hi Ard,
>
> Sorry for the late of seeing this patch. I have one question: why don't we
> implement a runtime serial port lib, which will switch UART base address in
> virtual address map change? I think this will be useful when we want to debug
> runtime driver in OS stage. And if we have a runtime version of SerialPortLib,
> then we don't need a runtime version of DebugLib which just disable touching
> serial port.
>
Well, only if the serial port is not exposed to the OS as well. The
Linux PL011 driver is especially easy to confuse, and having both the
firmware and the OS control it at the same time is likely to cause
problems.
However, I do agree that having the ability to assign a UART to DEBUG
at runtime is useful, and so I do intend to create a runtime version
of the PL011 library, in which case DxeRuntimeDebugLibSerialPort can
be replaced with BaseDebugLibSerialPort for DXE_RUNTIME_DRIVER
modules.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
2018-02-24 14:25 [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers Ard Biesheuvel
` (2 preceding siblings ...)
2018-02-27 1:50 ` Guo Heyi
@ 2018-02-27 10:39 ` Leif Lindholm
2018-02-27 10:45 ` Ard Biesheuvel
3 siblings, 1 reply; 16+ messages in thread
From: Leif Lindholm @ 2018-02-27 10:39 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, lersek
On Sat, Feb 24, 2018 at 02:25:14PM +0000, Ard Biesheuvel wrote:
> Commit 4bf95a9f361e ("MdeModulePkg/ResetSystemRuntimeDxe: Add more
> debug message") broke the DEBUG build for all platforms that rely on
> MMIO mapped UART devices, since it introduces a DEBUG() print that
> may trigger at runtime, at which such UART devices are usually not
> mapped, resulting in an OS crash.
>
> Given that this mostly only affects ARM and AARCH64, it is not unlikely
> that similar inadvertent breakage will occur again in the future, so
> let's fix this once and for all by switching affected platforms to the
> new DxeRuntimeDebugLibSerialPort DebugLib implementation that takes care
> not to touch the UART hardware after ExitBootServices().
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
For the series (pending DxeRuntimeDebugLibSerialPort hitting edk2):
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> Platform/AMD/OverdriveBoard/OverdriveBoard.dsc | 3 +++
> Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc | 3 +++
> Platform/LeMaker/CelloBoard/CelloBoard.dsc | 3 +++
> Platform/Socionext/DeveloperBox/DeveloperBox.dsc | 3 +++
> Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc | 3 +++
> Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc | 3 +++
> Silicon/Hisilicon/Hisilicon.dsc.inc | 3 +++
> Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 3 +++
> 8 files changed, 24 insertions(+)
>
> diff --git a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> index 7d85b78642da..348828e18d44 100644
> --- a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> +++ b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> @@ -227,6 +227,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.common.UEFI_APPLICATION]
> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
> diff --git a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
> index b026ce3a420a..7cb47937329e 100644
> --- a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
> +++ b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
> @@ -227,6 +227,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> !if $(SECURE_BOOT_ENABLE) == TRUE
> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> !endif
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.ARM, LibraryClasses.AARCH64]
> #
> diff --git a/Platform/LeMaker/CelloBoard/CelloBoard.dsc b/Platform/LeMaker/CelloBoard/CelloBoard.dsc
> index 2468583c0daa..569e444f6b26 100644
> --- a/Platform/LeMaker/CelloBoard/CelloBoard.dsc
> +++ b/Platform/LeMaker/CelloBoard/CelloBoard.dsc
> @@ -211,6 +211,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.common.UEFI_APPLICATION]
> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> index 8450d7800e43..925ce36d278b 100644
> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> @@ -208,6 +208,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
> ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
> BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> ################################################################################
> #
> diff --git a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> index 45ab2afc4069..2d5a94ed1dab 100644
> --- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> +++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> @@ -203,6 +203,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
> ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> ################################################################################
> #
> diff --git a/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc b/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
> index f0a7e9794153..3f4c7c8a3eef 100644
> --- a/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
> +++ b/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
> @@ -217,6 +217,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.common.UEFI_APPLICATION]
> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
> diff --git a/Silicon/Hisilicon/Hisilicon.dsc.inc b/Silicon/Hisilicon/Hisilicon.dsc.inc
> index 9050f29be381..b196322a20e2 100644
> --- a/Silicon/Hisilicon/Hisilicon.dsc.inc
> +++ b/Silicon/Hisilicon/Hisilicon.dsc.inc
> @@ -213,6 +213,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> !ifndef CONFIG_NO_DEBUGLIB
> DebugLib|IntelFrameworkModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
> !endif
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.AARCH64]
> ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf
> diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> index ef70b5216727..535cc3ff4c19 100644
> --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> @@ -184,6 +184,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
> +!if $(TARGET) != RELEASE
> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>
> [LibraryClasses.ARM, LibraryClasses.AARCH64]
> #
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
2018-02-27 9:23 ` Ard Biesheuvel
@ 2018-02-27 10:43 ` Laszlo Ersek
2018-02-27 11:30 ` Guo Heyi
2018-02-27 14:21 ` Ard Biesheuvel
0 siblings, 2 replies; 16+ messages in thread
From: Laszlo Ersek @ 2018-02-27 10:43 UTC (permalink / raw)
To: Ard Biesheuvel, Guo Heyi; +Cc: edk2-devel@lists.01.org, Leif Lindholm
On 02/27/18 10:23, Ard Biesheuvel wrote:
> On 27 February 2018 at 01:50, Guo Heyi <heyi.guo@linaro.org> wrote:
>> Hi Ard,
>>
>> Sorry for the late of seeing this patch. I have one question: why don't we
>> implement a runtime serial port lib, which will switch UART base address in
>> virtual address map change? I think this will be useful when we want to debug
>> runtime driver in OS stage. And if we have a runtime version of SerialPortLib,
>> then we don't need a runtime version of DebugLib which just disable touching
>> serial port.
>>
>
> Well, only if the serial port is not exposed to the OS as well. The
> Linux PL011 driver is especially easy to confuse, and having both the
> firmware and the OS control it at the same time is likely to cause
> problems.
>
> However, I do agree that having the ability to assign a UART to DEBUG
> at runtime is useful, and so I do intend to create a runtime version
> of the PL011 library, in which case DxeRuntimeDebugLibSerialPort can
> be replaced with BaseDebugLibSerialPort for DXE_RUNTIME_DRIVER
> modules.
>
Converting the PL011 base address from phys to virt can be done in the
library instance, yes (and then every runtime driver module linked
against this library instance will individually convert the address for
its own use). The messier aspect is getting the PL011 base address into
the UEFI memmap, marked as MMIO / RUNTIME, so that the OS assign a
virtual mapping to it in the first place.
The flash drivers generally do this with AddMemorySpace /
SetMemorySpaceAttributes.
(Side point: while I agree that those are good APIs to invoke, I think
they should also call AllocateMemorySpace right after; otherwise a
"stray" AllocateMemorySpace elsewhere could allocate a chunk out of the
middle of what the flash driver *thinks* it owns.)
The question is where this pair (or triplet) of GCD APIs should be called:
- In a platform DXE driver? Perhaps.
- In the DebugLib instance constructor / destructor? That could result
in some ugly reference counting -- you might want to keep the PL011 area
registered in GCD as long as *at least one* such runtime driver is loaded.
This is different from the flash driver because the flash driver is the
sole runtime DXE (or SMM) driver that accesses & owns the flash MMIO
range. With the PL011 register block, that's not the case; all runtime
drivers that produce debug messages own it co-operatively.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
2018-02-27 10:39 ` Leif Lindholm
@ 2018-02-27 10:45 ` Ard Biesheuvel
0 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2018-02-27 10:45 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Laszlo Ersek
On 27 February 2018 at 10:39, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Sat, Feb 24, 2018 at 02:25:14PM +0000, Ard Biesheuvel wrote:
>> Commit 4bf95a9f361e ("MdeModulePkg/ResetSystemRuntimeDxe: Add more
>> debug message") broke the DEBUG build for all platforms that rely on
>> MMIO mapped UART devices, since it introduces a DEBUG() print that
>> may trigger at runtime, at which such UART devices are usually not
>> mapped, resulting in an OS crash.
>>
>> Given that this mostly only affects ARM and AARCH64, it is not unlikely
>> that similar inadvertent breakage will occur again in the future, so
>> let's fix this once and for all by switching affected platforms to the
>> new DxeRuntimeDebugLibSerialPort DebugLib implementation that takes care
>> not to touch the UART hardware after ExitBootServices().
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> For the series (pending DxeRuntimeDebugLibSerialPort hitting edk2):
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
That's already in so I pushed this as d8e4d4565841..dbf93a0a853a
Thanks
>> ---
>> Platform/AMD/OverdriveBoard/OverdriveBoard.dsc | 3 +++
>> Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc | 3 +++
>> Platform/LeMaker/CelloBoard/CelloBoard.dsc | 3 +++
>> Platform/Socionext/DeveloperBox/DeveloperBox.dsc | 3 +++
>> Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc | 3 +++
>> Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc | 3 +++
>> Silicon/Hisilicon/Hisilicon.dsc.inc | 3 +++
>> Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 3 +++
>> 8 files changed, 24 insertions(+)
>>
>> diff --git a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
>> index 7d85b78642da..348828e18d44 100644
>> --- a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
>> +++ b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
>> @@ -227,6 +227,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
>> +!if $(TARGET) != RELEASE
>> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>> +!endif
>>
>> [LibraryClasses.common.UEFI_APPLICATION]
>> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
>> diff --git a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
>> index b026ce3a420a..7cb47937329e 100644
>> --- a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
>> +++ b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
>> @@ -227,6 +227,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>> !if $(SECURE_BOOT_ENABLE) == TRUE
>> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>> !endif
>> +!if $(TARGET) != RELEASE
>> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>> +!endif
>>
>> [LibraryClasses.ARM, LibraryClasses.AARCH64]
>> #
>> diff --git a/Platform/LeMaker/CelloBoard/CelloBoard.dsc b/Platform/LeMaker/CelloBoard/CelloBoard.dsc
>> index 2468583c0daa..569e444f6b26 100644
>> --- a/Platform/LeMaker/CelloBoard/CelloBoard.dsc
>> +++ b/Platform/LeMaker/CelloBoard/CelloBoard.dsc
>> @@ -211,6 +211,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>> ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
>> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
>> +!if $(TARGET) != RELEASE
>> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>> +!endif
>>
>> [LibraryClasses.common.UEFI_APPLICATION]
>> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
>> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
>> index 8450d7800e43..925ce36d278b 100644
>> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
>> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
>> @@ -208,6 +208,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
>> ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
>> BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>> +!if $(TARGET) != RELEASE
>> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>> +!endif
>>
>> ################################################################################
>> #
>> diff --git a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
>> index 45ab2afc4069..2d5a94ed1dab 100644
>> --- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
>> +++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
>> @@ -203,6 +203,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
>> ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
>> +!if $(TARGET) != RELEASE
>> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>> +!endif
>>
>> ################################################################################
>> #
>> diff --git a/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc b/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
>> index f0a7e9794153..3f4c7c8a3eef 100644
>> --- a/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
>> +++ b/Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
>> @@ -217,6 +217,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>> ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
>> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
>> +!if $(TARGET) != RELEASE
>> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>> +!endif
>>
>> [LibraryClasses.common.UEFI_APPLICATION]
>> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
>> diff --git a/Silicon/Hisilicon/Hisilicon.dsc.inc b/Silicon/Hisilicon/Hisilicon.dsc.inc
>> index 9050f29be381..b196322a20e2 100644
>> --- a/Silicon/Hisilicon/Hisilicon.dsc.inc
>> +++ b/Silicon/Hisilicon/Hisilicon.dsc.inc
>> @@ -213,6 +213,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>> !ifndef CONFIG_NO_DEBUGLIB
>> DebugLib|IntelFrameworkModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>> !endif
>> +!if $(TARGET) != RELEASE
>> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>> +!endif
>>
>> [LibraryClasses.AARCH64]
>> ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf
>> diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
>> index ef70b5216727..535cc3ff4c19 100644
>> --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
>> +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
>> @@ -184,6 +184,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
>> +!if $(TARGET) != RELEASE
>> + DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>> +!endif
>>
>> [LibraryClasses.ARM, LibraryClasses.AARCH64]
>> #
>> --
>> 2.11.0
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
2018-02-27 10:43 ` Laszlo Ersek
@ 2018-02-27 11:30 ` Guo Heyi
2018-02-27 15:59 ` Laszlo Ersek
2018-02-27 14:21 ` Ard Biesheuvel
1 sibling, 1 reply; 16+ messages in thread
From: Guo Heyi @ 2018-02-27 11:30 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Ard Biesheuvel, Guo Heyi, edk2-devel@lists.01.org, Leif Lindholm
On Tue, Feb 27, 2018 at 11:43:35AM +0100, Laszlo Ersek wrote:
> On 02/27/18 10:23, Ard Biesheuvel wrote:
> > On 27 February 2018 at 01:50, Guo Heyi <heyi.guo@linaro.org> wrote:
> >> Hi Ard,
> >>
> >> Sorry for the late of seeing this patch. I have one question: why don't we
> >> implement a runtime serial port lib, which will switch UART base address in
> >> virtual address map change? I think this will be useful when we want to debug
> >> runtime driver in OS stage. And if we have a runtime version of SerialPortLib,
> >> then we don't need a runtime version of DebugLib which just disable touching
> >> serial port.
> >>
> >
> > Well, only if the serial port is not exposed to the OS as well. The
> > Linux PL011 driver is especially easy to confuse, and having both the
> > firmware and the OS control it at the same time is likely to cause
> > problems.
> >
> > However, I do agree that having the ability to assign a UART to DEBUG
> > at runtime is useful, and so I do intend to create a runtime version
> > of the PL011 library, in which case DxeRuntimeDebugLibSerialPort can
> > be replaced with BaseDebugLibSerialPort for DXE_RUNTIME_DRIVER
> > modules.
> >
>
> Converting the PL011 base address from phys to virt can be done in the
> library instance, yes (and then every runtime driver module linked
> against this library instance will individually convert the address for
> its own use). The messier aspect is getting the PL011 base address into
> the UEFI memmap, marked as MMIO / RUNTIME, so that the OS assign a
> virtual mapping to it in the first place.
>
> The flash drivers generally do this with AddMemorySpace /
> SetMemorySpaceAttributes.
>
> (Side point: while I agree that those are good APIs to invoke, I think
> they should also call AllocateMemorySpace right after; otherwise a
> "stray" AllocateMemorySpace elsewhere could allocate a chunk out of the
> middle of what the flash driver *thinks* it owns.)
>
> The question is where this pair (or triplet) of GCD APIs should be called:
>
> - In a platform DXE driver? Perhaps.
>
> - In the DebugLib instance constructor / destructor? That could result
> in some ugly reference counting -- you might want to keep the PL011 area
> registered in GCD as long as *at least one* such runtime driver is loaded.
>
> This is different from the flash driver because the flash driver is the
> sole runtime DXE (or SMM) driver that accesses & owns the flash MMIO
> range. With the PL011 register block, that's not the case; all runtime
> drivers that produce debug messages own it co-operatively.
On our platforms, we actually use
IntelFrameworkModulePkg/PeiDxeDebugLibReportStatusCode,
MdeModulePkg/RuntimeDxeReportStatusCodeLib and
IntelFrameworkModulePkg/StatusCodeRuntimeDxe as the whole DebugLib call chain
for runtime drivers. Is StatusCodeRuntimeDxe a good place for GCD manipulation?
But I've no idea on how to distinguish the code between ARM and X86, for X86
uses port IO for serial port access.
Regards,
Heyi
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
2018-02-27 10:43 ` Laszlo Ersek
2018-02-27 11:30 ` Guo Heyi
@ 2018-02-27 14:21 ` Ard Biesheuvel
2018-02-27 16:09 ` Laszlo Ersek
1 sibling, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2018-02-27 14:21 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Guo Heyi, edk2-devel@lists.01.org, Leif Lindholm
On 27 February 2018 at 10:43, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/27/18 10:23, Ard Biesheuvel wrote:
>> On 27 February 2018 at 01:50, Guo Heyi <heyi.guo@linaro.org> wrote:
>>> Hi Ard,
>>>
>>> Sorry for the late of seeing this patch. I have one question: why don't we
>>> implement a runtime serial port lib, which will switch UART base address in
>>> virtual address map change? I think this will be useful when we want to debug
>>> runtime driver in OS stage. And if we have a runtime version of SerialPortLib,
>>> then we don't need a runtime version of DebugLib which just disable touching
>>> serial port.
>>>
>>
>> Well, only if the serial port is not exposed to the OS as well. The
>> Linux PL011 driver is especially easy to confuse, and having both the
>> firmware and the OS control it at the same time is likely to cause
>> problems.
>>
>> However, I do agree that having the ability to assign a UART to DEBUG
>> at runtime is useful, and so I do intend to create a runtime version
>> of the PL011 library, in which case DxeRuntimeDebugLibSerialPort can
>> be replaced with BaseDebugLibSerialPort for DXE_RUNTIME_DRIVER
>> modules.
>>
>
> Converting the PL011 base address from phys to virt can be done in the
> library instance, yes (and then every runtime driver module linked
> against this library instance will individually convert the address for
> its own use). The messier aspect is getting the PL011 base address into
> the UEFI memmap, marked as MMIO / RUNTIME, so that the OS assign a
> virtual mapping to it in the first place.
>
> The flash drivers generally do this with AddMemorySpace /
> SetMemorySpaceAttributes.
>
> (Side point: while I agree that those are good APIs to invoke, I think
> they should also call AllocateMemorySpace right after; otherwise a
> "stray" AllocateMemorySpace elsewhere could allocate a chunk out of the
> middle of what the flash driver *thinks* it owns.)
>
> The question is where this pair (or triplet) of GCD APIs should be called:
>
> - In a platform DXE driver? Perhaps.
>
> - In the DebugLib instance constructor / destructor? That could result
> in some ugly reference counting -- you might want to keep the PL011 area
> registered in GCD as long as *at least one* such runtime driver is loaded.
>
> This is different from the flash driver because the flash driver is the
> sole runtime DXE (or SMM) driver that accesses & owns the flash MMIO
> range. With the PL011 register block, that's not the case; all runtime
> drivers that produce debug messages own it co-operatively.
>
Given that this SerialPortLib will be of the DXE_RUNTIME_DRIVER type
anyway, couldn't we base it on a generic RuntimeUart protocol that we
can depex on in the library, and produce in a separate [singleton]
runtime DXE driver, which takes care of the UART initialization as
well as the GCD memory space handling?
In fact, we could modify DxeRuntimeDebugLibSerialPort to attach to
this protocol if it exists at EndOfDxe, and either do nothing at
runtime (as it does currently) or produce UART output via the protocol
if it exists.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
2018-02-27 11:30 ` Guo Heyi
@ 2018-02-27 15:59 ` Laszlo Ersek
0 siblings, 0 replies; 16+ messages in thread
From: Laszlo Ersek @ 2018-02-27 15:59 UTC (permalink / raw)
To: Guo Heyi; +Cc: Ard Biesheuvel, edk2-devel@lists.01.org, Leif Lindholm
On 02/27/18 12:30, Guo Heyi wrote:
> On Tue, Feb 27, 2018 at 11:43:35AM +0100, Laszlo Ersek wrote:
>> On 02/27/18 10:23, Ard Biesheuvel wrote:
>>> On 27 February 2018 at 01:50, Guo Heyi <heyi.guo@linaro.org> wrote:
>>>> Hi Ard,
>>>>
>>>> Sorry for the late of seeing this patch. I have one question: why don't we
>>>> implement a runtime serial port lib, which will switch UART base address in
>>>> virtual address map change? I think this will be useful when we want to debug
>>>> runtime driver in OS stage. And if we have a runtime version of SerialPortLib,
>>>> then we don't need a runtime version of DebugLib which just disable touching
>>>> serial port.
>>>>
>>>
>>> Well, only if the serial port is not exposed to the OS as well. The
>>> Linux PL011 driver is especially easy to confuse, and having both the
>>> firmware and the OS control it at the same time is likely to cause
>>> problems.
>>>
>>> However, I do agree that having the ability to assign a UART to DEBUG
>>> at runtime is useful, and so I do intend to create a runtime version
>>> of the PL011 library, in which case DxeRuntimeDebugLibSerialPort can
>>> be replaced with BaseDebugLibSerialPort for DXE_RUNTIME_DRIVER
>>> modules.
>>>
>>
>> Converting the PL011 base address from phys to virt can be done in the
>> library instance, yes (and then every runtime driver module linked
>> against this library instance will individually convert the address for
>> its own use). The messier aspect is getting the PL011 base address into
>> the UEFI memmap, marked as MMIO / RUNTIME, so that the OS assign a
>> virtual mapping to it in the first place.
>>
>> The flash drivers generally do this with AddMemorySpace /
>> SetMemorySpaceAttributes.
>>
>> (Side point: while I agree that those are good APIs to invoke, I think
>> they should also call AllocateMemorySpace right after; otherwise a
>> "stray" AllocateMemorySpace elsewhere could allocate a chunk out of the
>> middle of what the flash driver *thinks* it owns.)
>>
>> The question is where this pair (or triplet) of GCD APIs should be called:
>>
>> - In a platform DXE driver? Perhaps.
>>
>> - In the DebugLib instance constructor / destructor? That could result
>> in some ugly reference counting -- you might want to keep the PL011 area
>> registered in GCD as long as *at least one* such runtime driver is loaded.
>>
>> This is different from the flash driver because the flash driver is the
>> sole runtime DXE (or SMM) driver that accesses & owns the flash MMIO
>> range. With the PL011 register block, that's not the case; all runtime
>> drivers that produce debug messages own it co-operatively.
>
> On our platforms, we actually use
> IntelFrameworkModulePkg/PeiDxeDebugLibReportStatusCode,
> MdeModulePkg/RuntimeDxeReportStatusCodeLib and
> IntelFrameworkModulePkg/StatusCodeRuntimeDxe as the whole DebugLib call chain
> for runtime drivers. Is StatusCodeRuntimeDxe a good place for GCD manipulation?
Sure; if DebugLib (and the library instances below it) don't directly
access hardware, but make calls to some shared code -- i.e. into another
runtime DXE driver via a protocol interface originally looked up /
stashed at boot time, and then converted to a virtual address --, then
only that runtime DXE driver needs to manage the GCD map.
> But I've no idea on how to distinguish the code between ARM and X86, for X86
> uses port IO for serial port access.
If you can ascertain that the lib instance linked into
StatusCodeRuntimeDxe (a) is not used by any other module, and (b)
performs the hardware access, then you could push the GCD management
into that lib instance. Or else, you could write a plug-in lib instance
and hook that with NULL resolution into the driver.
But, I guess, at this point it's simplest to modify PlatformPei or
PlatformDxe to add that entry to GCD in advance, regardless of the
actual number of runtime DXE drivers that will rely on the mapping.
Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
2018-02-27 14:21 ` Ard Biesheuvel
@ 2018-02-27 16:09 ` Laszlo Ersek
2018-02-27 16:10 ` Ard Biesheuvel
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2018-02-27 16:09 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Guo Heyi, edk2-devel@lists.01.org, Leif Lindholm
On 02/27/18 15:21, Ard Biesheuvel wrote:
> On 27 February 2018 at 10:43, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 02/27/18 10:23, Ard Biesheuvel wrote:
>>> On 27 February 2018 at 01:50, Guo Heyi <heyi.guo@linaro.org> wrote:
>>>> Hi Ard,
>>>>
>>>> Sorry for the late of seeing this patch. I have one question: why don't we
>>>> implement a runtime serial port lib, which will switch UART base address in
>>>> virtual address map change? I think this will be useful when we want to debug
>>>> runtime driver in OS stage. And if we have a runtime version of SerialPortLib,
>>>> then we don't need a runtime version of DebugLib which just disable touching
>>>> serial port.
>>>>
>>>
>>> Well, only if the serial port is not exposed to the OS as well. The
>>> Linux PL011 driver is especially easy to confuse, and having both the
>>> firmware and the OS control it at the same time is likely to cause
>>> problems.
>>>
>>> However, I do agree that having the ability to assign a UART to DEBUG
>>> at runtime is useful, and so I do intend to create a runtime version
>>> of the PL011 library, in which case DxeRuntimeDebugLibSerialPort can
>>> be replaced with BaseDebugLibSerialPort for DXE_RUNTIME_DRIVER
>>> modules.
>>>
>>
>> Converting the PL011 base address from phys to virt can be done in the
>> library instance, yes (and then every runtime driver module linked
>> against this library instance will individually convert the address for
>> its own use). The messier aspect is getting the PL011 base address into
>> the UEFI memmap, marked as MMIO / RUNTIME, so that the OS assign a
>> virtual mapping to it in the first place.
>>
>> The flash drivers generally do this with AddMemorySpace /
>> SetMemorySpaceAttributes.
>>
>> (Side point: while I agree that those are good APIs to invoke, I think
>> they should also call AllocateMemorySpace right after; otherwise a
>> "stray" AllocateMemorySpace elsewhere could allocate a chunk out of the
>> middle of what the flash driver *thinks* it owns.)
>>
>> The question is where this pair (or triplet) of GCD APIs should be called:
>>
>> - In a platform DXE driver? Perhaps.
>>
>> - In the DebugLib instance constructor / destructor? That could result
>> in some ugly reference counting -- you might want to keep the PL011 area
>> registered in GCD as long as *at least one* such runtime driver is loaded.
>>
>> This is different from the flash driver because the flash driver is the
>> sole runtime DXE (or SMM) driver that accesses & owns the flash MMIO
>> range. With the PL011 register block, that's not the case; all runtime
>> drivers that produce debug messages own it co-operatively.
>>
>
> Given that this SerialPortLib will be of the DXE_RUNTIME_DRIVER type
> anyway, couldn't we base it on a generic RuntimeUart protocol that we
> can depex on in the library, and produce in a separate [singleton]
> runtime DXE driver, which takes care of the UART initialization as
> well as the GCD memory space handling?
Yes, that should totally work.
> In fact, we could modify DxeRuntimeDebugLibSerialPort to attach to
> this protocol if it exists at EndOfDxe, and either do nothing at
> runtime (as it does currently) or produce UART output via the protocol
> if it exists.
This is a good idea IMO. What about the following: since you'd have to
register an event notification function for EndOfDxe, why not register a
protocol notify directly for the runtime UART protocol?
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
2018-02-27 16:09 ` Laszlo Ersek
@ 2018-02-27 16:10 ` Ard Biesheuvel
2018-02-27 17:33 ` Ard Biesheuvel
0 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2018-02-27 16:10 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Guo Heyi, edk2-devel@lists.01.org, Leif Lindholm
On 27 February 2018 at 16:09, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/27/18 15:21, Ard Biesheuvel wrote:
>> On 27 February 2018 at 10:43, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 02/27/18 10:23, Ard Biesheuvel wrote:
>>>> On 27 February 2018 at 01:50, Guo Heyi <heyi.guo@linaro.org> wrote:
>>>>> Hi Ard,
>>>>>
>>>>> Sorry for the late of seeing this patch. I have one question: why don't we
>>>>> implement a runtime serial port lib, which will switch UART base address in
>>>>> virtual address map change? I think this will be useful when we want to debug
>>>>> runtime driver in OS stage. And if we have a runtime version of SerialPortLib,
>>>>> then we don't need a runtime version of DebugLib which just disable touching
>>>>> serial port.
>>>>>
>>>>
>>>> Well, only if the serial port is not exposed to the OS as well. The
>>>> Linux PL011 driver is especially easy to confuse, and having both the
>>>> firmware and the OS control it at the same time is likely to cause
>>>> problems.
>>>>
>>>> However, I do agree that having the ability to assign a UART to DEBUG
>>>> at runtime is useful, and so I do intend to create a runtime version
>>>> of the PL011 library, in which case DxeRuntimeDebugLibSerialPort can
>>>> be replaced with BaseDebugLibSerialPort for DXE_RUNTIME_DRIVER
>>>> modules.
>>>>
>>>
>>> Converting the PL011 base address from phys to virt can be done in the
>>> library instance, yes (and then every runtime driver module linked
>>> against this library instance will individually convert the address for
>>> its own use). The messier aspect is getting the PL011 base address into
>>> the UEFI memmap, marked as MMIO / RUNTIME, so that the OS assign a
>>> virtual mapping to it in the first place.
>>>
>>> The flash drivers generally do this with AddMemorySpace /
>>> SetMemorySpaceAttributes.
>>>
>>> (Side point: while I agree that those are good APIs to invoke, I think
>>> they should also call AllocateMemorySpace right after; otherwise a
>>> "stray" AllocateMemorySpace elsewhere could allocate a chunk out of the
>>> middle of what the flash driver *thinks* it owns.)
>>>
>>> The question is where this pair (or triplet) of GCD APIs should be called:
>>>
>>> - In a platform DXE driver? Perhaps.
>>>
>>> - In the DebugLib instance constructor / destructor? That could result
>>> in some ugly reference counting -- you might want to keep the PL011 area
>>> registered in GCD as long as *at least one* such runtime driver is loaded.
>>>
>>> This is different from the flash driver because the flash driver is the
>>> sole runtime DXE (or SMM) driver that accesses & owns the flash MMIO
>>> range. With the PL011 register block, that's not the case; all runtime
>>> drivers that produce debug messages own it co-operatively.
>>>
>>
>> Given that this SerialPortLib will be of the DXE_RUNTIME_DRIVER type
>> anyway, couldn't we base it on a generic RuntimeUart protocol that we
>> can depex on in the library, and produce in a separate [singleton]
>> runtime DXE driver, which takes care of the UART initialization as
>> well as the GCD memory space handling?
>
> Yes, that should totally work.
>
>> In fact, we could modify DxeRuntimeDebugLibSerialPort to attach to
>> this protocol if it exists at EndOfDxe, and either do nothing at
>> runtime (as it does currently) or produce UART output via the protocol
>> if it exists.
>
> This is a good idea IMO. What about the following: since you'd have to
> register an event notification function for EndOfDxe, why not register a
> protocol notify directly for the runtime UART protocol?
>
Even better.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
2018-02-27 16:10 ` Ard Biesheuvel
@ 2018-02-27 17:33 ` Ard Biesheuvel
2018-02-27 17:47 ` Laszlo Ersek
0 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2018-02-27 17:33 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Guo Heyi, edk2-devel@lists.01.org, Leif Lindholm
On 27 February 2018 at 16:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 27 February 2018 at 16:09, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 02/27/18 15:21, Ard Biesheuvel wrote:
>>> On 27 February 2018 at 10:43, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> On 02/27/18 10:23, Ard Biesheuvel wrote:
>>>>> On 27 February 2018 at 01:50, Guo Heyi <heyi.guo@linaro.org> wrote:
>>>>>> Hi Ard,
>>>>>>
>>>>>> Sorry for the late of seeing this patch. I have one question: why don't we
>>>>>> implement a runtime serial port lib, which will switch UART base address in
>>>>>> virtual address map change? I think this will be useful when we want to debug
>>>>>> runtime driver in OS stage. And if we have a runtime version of SerialPortLib,
>>>>>> then we don't need a runtime version of DebugLib which just disable touching
>>>>>> serial port.
>>>>>>
>>>>>
>>>>> Well, only if the serial port is not exposed to the OS as well. The
>>>>> Linux PL011 driver is especially easy to confuse, and having both the
>>>>> firmware and the OS control it at the same time is likely to cause
>>>>> problems.
>>>>>
>>>>> However, I do agree that having the ability to assign a UART to DEBUG
>>>>> at runtime is useful, and so I do intend to create a runtime version
>>>>> of the PL011 library, in which case DxeRuntimeDebugLibSerialPort can
>>>>> be replaced with BaseDebugLibSerialPort for DXE_RUNTIME_DRIVER
>>>>> modules.
>>>>>
>>>>
>>>> Converting the PL011 base address from phys to virt can be done in the
>>>> library instance, yes (and then every runtime driver module linked
>>>> against this library instance will individually convert the address for
>>>> its own use). The messier aspect is getting the PL011 base address into
>>>> the UEFI memmap, marked as MMIO / RUNTIME, so that the OS assign a
>>>> virtual mapping to it in the first place.
>>>>
>>>> The flash drivers generally do this with AddMemorySpace /
>>>> SetMemorySpaceAttributes.
>>>>
>>>> (Side point: while I agree that those are good APIs to invoke, I think
>>>> they should also call AllocateMemorySpace right after; otherwise a
>>>> "stray" AllocateMemorySpace elsewhere could allocate a chunk out of the
>>>> middle of what the flash driver *thinks* it owns.)
>>>>
>>>> The question is where this pair (or triplet) of GCD APIs should be called:
>>>>
>>>> - In a platform DXE driver? Perhaps.
>>>>
>>>> - In the DebugLib instance constructor / destructor? That could result
>>>> in some ugly reference counting -- you might want to keep the PL011 area
>>>> registered in GCD as long as *at least one* such runtime driver is loaded.
>>>>
>>>> This is different from the flash driver because the flash driver is the
>>>> sole runtime DXE (or SMM) driver that accesses & owns the flash MMIO
>>>> range. With the PL011 register block, that's not the case; all runtime
>>>> drivers that produce debug messages own it co-operatively.
>>>>
>>>
>>> Given that this SerialPortLib will be of the DXE_RUNTIME_DRIVER type
>>> anyway, couldn't we base it on a generic RuntimeUart protocol that we
>>> can depex on in the library, and produce in a separate [singleton]
>>> runtime DXE driver, which takes care of the UART initialization as
>>> well as the GCD memory space handling?
>>
>> Yes, that should totally work.
>>
>>> In fact, we could modify DxeRuntimeDebugLibSerialPort to attach to
>>> this protocol if it exists at EndOfDxe, and either do nothing at
>>> runtime (as it does currently) or produce UART output via the protocol
>>> if it exists.
>>
>> This is a good idea IMO. What about the following: since you'd have to
>> register an event notification function for EndOfDxe, why not register a
>> protocol notify directly for the runtime UART protocol?
>>
What I also like about this approach is that it defers the decision
whether to enable the DEBUG UART at OS runtime from compile time to
runtime [where 'runtime' is a tad overloaded here]
IOW, you can use a variable, PCD or simply do 'load
fs0:\UartRuntimeDxe.efi' from the Shell to promote a 'runtime silent'
DEBUG build to one that produces DEBUG output while running under the
OS.
I guess the only problem is that DxeRuntimeDebugLibSerialPort lives in
MdePkg, and so any extensions to it involving protocols require an ECR
against the PI spec first.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
2018-02-27 17:33 ` Ard Biesheuvel
@ 2018-02-27 17:47 ` Laszlo Ersek
2018-02-27 17:55 ` Ard Biesheuvel
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2018-02-27 17:47 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Guo Heyi, edk2-devel@lists.01.org, Leif Lindholm
On 02/27/18 18:33, Ard Biesheuvel wrote:
> On 27 February 2018 at 16:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 27 February 2018 at 16:09, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 02/27/18 15:21, Ard Biesheuvel wrote:
>>>> On 27 February 2018 at 10:43, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>> On 02/27/18 10:23, Ard Biesheuvel wrote:
>>>>>> On 27 February 2018 at 01:50, Guo Heyi <heyi.guo@linaro.org> wrote:
>>>>>>> Hi Ard,
>>>>>>>
>>>>>>> Sorry for the late of seeing this patch. I have one question: why don't we
>>>>>>> implement a runtime serial port lib, which will switch UART base address in
>>>>>>> virtual address map change? I think this will be useful when we want to debug
>>>>>>> runtime driver in OS stage. And if we have a runtime version of SerialPortLib,
>>>>>>> then we don't need a runtime version of DebugLib which just disable touching
>>>>>>> serial port.
>>>>>>>
>>>>>>
>>>>>> Well, only if the serial port is not exposed to the OS as well. The
>>>>>> Linux PL011 driver is especially easy to confuse, and having both the
>>>>>> firmware and the OS control it at the same time is likely to cause
>>>>>> problems.
>>>>>>
>>>>>> However, I do agree that having the ability to assign a UART to DEBUG
>>>>>> at runtime is useful, and so I do intend to create a runtime version
>>>>>> of the PL011 library, in which case DxeRuntimeDebugLibSerialPort can
>>>>>> be replaced with BaseDebugLibSerialPort for DXE_RUNTIME_DRIVER
>>>>>> modules.
>>>>>>
>>>>>
>>>>> Converting the PL011 base address from phys to virt can be done in the
>>>>> library instance, yes (and then every runtime driver module linked
>>>>> against this library instance will individually convert the address for
>>>>> its own use). The messier aspect is getting the PL011 base address into
>>>>> the UEFI memmap, marked as MMIO / RUNTIME, so that the OS assign a
>>>>> virtual mapping to it in the first place.
>>>>>
>>>>> The flash drivers generally do this with AddMemorySpace /
>>>>> SetMemorySpaceAttributes.
>>>>>
>>>>> (Side point: while I agree that those are good APIs to invoke, I think
>>>>> they should also call AllocateMemorySpace right after; otherwise a
>>>>> "stray" AllocateMemorySpace elsewhere could allocate a chunk out of the
>>>>> middle of what the flash driver *thinks* it owns.)
>>>>>
>>>>> The question is where this pair (or triplet) of GCD APIs should be called:
>>>>>
>>>>> - In a platform DXE driver? Perhaps.
>>>>>
>>>>> - In the DebugLib instance constructor / destructor? That could result
>>>>> in some ugly reference counting -- you might want to keep the PL011 area
>>>>> registered in GCD as long as *at least one* such runtime driver is loaded.
>>>>>
>>>>> This is different from the flash driver because the flash driver is the
>>>>> sole runtime DXE (or SMM) driver that accesses & owns the flash MMIO
>>>>> range. With the PL011 register block, that's not the case; all runtime
>>>>> drivers that produce debug messages own it co-operatively.
>>>>>
>>>>
>>>> Given that this SerialPortLib will be of the DXE_RUNTIME_DRIVER type
>>>> anyway, couldn't we base it on a generic RuntimeUart protocol that we
>>>> can depex on in the library, and produce in a separate [singleton]
>>>> runtime DXE driver, which takes care of the UART initialization as
>>>> well as the GCD memory space handling?
>>>
>>> Yes, that should totally work.
>>>
>>>> In fact, we could modify DxeRuntimeDebugLibSerialPort to attach to
>>>> this protocol if it exists at EndOfDxe, and either do nothing at
>>>> runtime (as it does currently) or produce UART output via the protocol
>>>> if it exists.
>>>
>>> This is a good idea IMO. What about the following: since you'd have to
>>> register an event notification function for EndOfDxe, why not register a
>>> protocol notify directly for the runtime UART protocol?
>>>
>
> What I also like about this approach is that it defers the decision
> whether to enable the DEBUG UART at OS runtime from compile time to
> runtime [where 'runtime' is a tad overloaded here]
>
> IOW, you can use a variable, PCD or simply do 'load
> fs0:\UartRuntimeDxe.efi' from the Shell to promote a 'runtime silent'
> DEBUG build to one that produces DEBUG output while running under the
> OS.
>
> I guess the only problem is that DxeRuntimeDebugLibSerialPort lives in
> MdePkg, and so any extensions to it involving protocols require an ECR
> against the PI spec first.
>
This is a brand new library instance, not many platform DSCs use it just
yet. Can we simply move it to MdeModulePkg? Do we see any downsides to
not offering the lib instance in MdePkg?
Thanks
Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers
2018-02-27 17:47 ` Laszlo Ersek
@ 2018-02-27 17:55 ` Ard Biesheuvel
0 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2018-02-27 17:55 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Guo Heyi, edk2-devel@lists.01.org, Leif Lindholm
On 27 February 2018 at 17:47, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/27/18 18:33, Ard Biesheuvel wrote:
>> On 27 February 2018 at 16:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> On 27 February 2018 at 16:09, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> On 02/27/18 15:21, Ard Biesheuvel wrote:
>>>>> On 27 February 2018 at 10:43, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>>> On 02/27/18 10:23, Ard Biesheuvel wrote:
>>>>>>> On 27 February 2018 at 01:50, Guo Heyi <heyi.guo@linaro.org> wrote:
>>>>>>>> Hi Ard,
>>>>>>>>
>>>>>>>> Sorry for the late of seeing this patch. I have one question: why don't we
>>>>>>>> implement a runtime serial port lib, which will switch UART base address in
>>>>>>>> virtual address map change? I think this will be useful when we want to debug
>>>>>>>> runtime driver in OS stage. And if we have a runtime version of SerialPortLib,
>>>>>>>> then we don't need a runtime version of DebugLib which just disable touching
>>>>>>>> serial port.
>>>>>>>>
>>>>>>>
>>>>>>> Well, only if the serial port is not exposed to the OS as well. The
>>>>>>> Linux PL011 driver is especially easy to confuse, and having both the
>>>>>>> firmware and the OS control it at the same time is likely to cause
>>>>>>> problems.
>>>>>>>
>>>>>>> However, I do agree that having the ability to assign a UART to DEBUG
>>>>>>> at runtime is useful, and so I do intend to create a runtime version
>>>>>>> of the PL011 library, in which case DxeRuntimeDebugLibSerialPort can
>>>>>>> be replaced with BaseDebugLibSerialPort for DXE_RUNTIME_DRIVER
>>>>>>> modules.
>>>>>>>
>>>>>>
>>>>>> Converting the PL011 base address from phys to virt can be done in the
>>>>>> library instance, yes (and then every runtime driver module linked
>>>>>> against this library instance will individually convert the address for
>>>>>> its own use). The messier aspect is getting the PL011 base address into
>>>>>> the UEFI memmap, marked as MMIO / RUNTIME, so that the OS assign a
>>>>>> virtual mapping to it in the first place.
>>>>>>
>>>>>> The flash drivers generally do this with AddMemorySpace /
>>>>>> SetMemorySpaceAttributes.
>>>>>>
>>>>>> (Side point: while I agree that those are good APIs to invoke, I think
>>>>>> they should also call AllocateMemorySpace right after; otherwise a
>>>>>> "stray" AllocateMemorySpace elsewhere could allocate a chunk out of the
>>>>>> middle of what the flash driver *thinks* it owns.)
>>>>>>
>>>>>> The question is where this pair (or triplet) of GCD APIs should be called:
>>>>>>
>>>>>> - In a platform DXE driver? Perhaps.
>>>>>>
>>>>>> - In the DebugLib instance constructor / destructor? That could result
>>>>>> in some ugly reference counting -- you might want to keep the PL011 area
>>>>>> registered in GCD as long as *at least one* such runtime driver is loaded.
>>>>>>
>>>>>> This is different from the flash driver because the flash driver is the
>>>>>> sole runtime DXE (or SMM) driver that accesses & owns the flash MMIO
>>>>>> range. With the PL011 register block, that's not the case; all runtime
>>>>>> drivers that produce debug messages own it co-operatively.
>>>>>>
>>>>>
>>>>> Given that this SerialPortLib will be of the DXE_RUNTIME_DRIVER type
>>>>> anyway, couldn't we base it on a generic RuntimeUart protocol that we
>>>>> can depex on in the library, and produce in a separate [singleton]
>>>>> runtime DXE driver, which takes care of the UART initialization as
>>>>> well as the GCD memory space handling?
>>>>
>>>> Yes, that should totally work.
>>>>
>>>>> In fact, we could modify DxeRuntimeDebugLibSerialPort to attach to
>>>>> this protocol if it exists at EndOfDxe, and either do nothing at
>>>>> runtime (as it does currently) or produce UART output via the protocol
>>>>> if it exists.
>>>>
>>>> This is a good idea IMO. What about the following: since you'd have to
>>>> register an event notification function for EndOfDxe, why not register a
>>>> protocol notify directly for the runtime UART protocol?
>>>>
>>
>> What I also like about this approach is that it defers the decision
>> whether to enable the DEBUG UART at OS runtime from compile time to
>> runtime [where 'runtime' is a tad overloaded here]
>>
>> IOW, you can use a variable, PCD or simply do 'load
>> fs0:\UartRuntimeDxe.efi' from the Shell to promote a 'runtime silent'
>> DEBUG build to one that produces DEBUG output while running under the
>> OS.
>>
>> I guess the only problem is that DxeRuntimeDebugLibSerialPort lives in
>> MdePkg, and so any extensions to it involving protocols require an ECR
>> against the PI spec first.
>>
>
> This is a brand new library instance, not many platform DSCs use it just
> yet. Can we simply move it to MdeModulePkg? Do we see any downsides to
> not offering the lib instance in MdePkg?
>
Good point. I think that should be fine
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-02-27 17:49 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-24 14:25 [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers Ard Biesheuvel
2018-02-24 14:25 ` [PATCH edk2-platforms 2/2] Silicon/SynQuacerI2cDxe: remove special runtime treatment of DEBUG()s Ard Biesheuvel
2018-02-26 14:50 ` [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers Laszlo Ersek
2018-02-27 1:50 ` Guo Heyi
2018-02-27 9:23 ` Ard Biesheuvel
2018-02-27 10:43 ` Laszlo Ersek
2018-02-27 11:30 ` Guo Heyi
2018-02-27 15:59 ` Laszlo Ersek
2018-02-27 14:21 ` Ard Biesheuvel
2018-02-27 16:09 ` Laszlo Ersek
2018-02-27 16:10 ` Ard Biesheuvel
2018-02-27 17:33 ` Ard Biesheuvel
2018-02-27 17:47 ` Laszlo Ersek
2018-02-27 17:55 ` Ard Biesheuvel
2018-02-27 10:39 ` Leif Lindholm
2018-02-27 10:45 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox