Date   
Re: question about qemu+kvm+ovmf+winxp

Junhao Gao
 

Hi David

I have found this compiled OVMF-with-csm.fd can support winxp booting up.
OVMF-with-csm.fd path: https://www.kraxel.org/repos/jenkins/seabios/seabios.git-csm-1.12.0-33.63.g43f5df7.x86_64.rpm
Then could you help to provide me the compile method and base code to reproduce this OVMF-with-csm.fd?

Thanks,
Junhao

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Junhao Gao
Sent: Friday, October 11, 2019 9:33 AM
To: devel@edk2.groups.io; lersek@...
Cc: David Woodhouse <dwmw2@...>
Subject: Re: [edk2-devel] question about qemu+kvm+ovmf+winxp

Hi Laszlo
Thank you very much for your great support.

Hi David
Refer to http://www.linux-kvm.org/downloads/lersek/ovmf-whitepaper-c770f8c.txt.

Interested users and developers should look for OVMF's "-D CSM_ENABLE"
build-time option, and check out the <https://www.kraxel.org/repos/> continuous
integration repository, which provides CSM-enabled OVMF builds.

Could you help me to choose which branch supporting CSM, and more details, thanks very much.

Thanks,
Junhao

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Friday, October 11, 2019 2:11 AM
To: Gao, Junhao <junhao.gao@...>
Cc: devel@edk2.groups.io; David Woodhouse <dwmw2@...>
Subject: Re: [edk2-devel] question about qemu+kvm+ovmf+winxp

On 10/10/19 16:37, Junhao Gao wrote:
Hi edk2 members

I have a question for your help.
I want to enable qemu+kvm+ovmf to boot up windows xp,
then ovmf support winxp starting-up?
To my understanding, the first Windows "family" with any kind of UEFI support is Windows 7.

OvmfPkg/README has some comments on Windows support:

* UEFI Windows 8 boots
* UEFI Windows 7 & Windows 2008 Server boot (see important notes
below!)
and

=== UEFI Windows 7 & Windows 2008 Server ===

* One of the '-vga std' and '-vga qxl' QEMU options should be used.
* Only one video mode, 1024x768x32, is supported at OS runtime.
* The '-vga qxl' QEMU option is recommended. After booting the installed
guest OS, select the video card in Device Manager, and upgrade its driver
to the QXL XDDM one. Download location:
<http://www.spice-space.org/download.html>, Guest | Windows binaries.
This enables further resolutions at OS runtime, and provides S3
(suspend/resume) capability.
If you'd like to virtualize Windows XP on QEMU/KVM, please use SeaBIOS for guest firmware.

You can also try to build OVMF with -D CSM_ENABLE, but for that, you'll have to build SeaBIOS in CSM mode first, and embed that binary into OVMF at build time. Please ask David for details (CC'd).

Thanks
Laszlo

qemu command:
qemu-system-x86_64 -hda winxp.img -boot c -enable-kvm -cpu host -bios ./OVMF.fd -m 512 -vga cirrus -net nic,model=rtl8139 -net user -usbdevice tablet -localtime
Then if ovmf support, could you provide me the way to compile the OVMF.fd?

Thanks,
Junhao




Re: [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Wu, Jiaxin
 

I'm surprising my detailed and patient explanation become a poor excuses! If you think there is anything wrong with my explanation, please correct me instead of blaming directly.

I think I have *repeated* several times that we are targeting to fix
the HostName validation issue, not the IP or email address. *But*
even so, the series patches for UEFI TLS is also allowable to
specify IP as host name for CN or dNSName of SAN in the certificate.
That's why I said "if the CN or SAN in the certificate are set
correctly, it should be OK to pass the verification". The failure you
mentioned here is to set the IP in iPAddress of SAN, I agree it's the
routine and suggested setting, *but* obviously, it's not the target
we are supported according the implementation/description of
TlsSetVerifyHost. We are targeting to the hostname verification, and
meanwhile compatible with the IP in the URI (But need the *correct*
certificate setting).

IP addresses stored in the DNS names and CN are of cause ignored by
X509_check_ip & X509_check_ip_asc().
I cannot coherently express how disappointed I am by this response.

The current state is that EDK2 doesn't check the subject of the
certificate at all.
Highlight again: we do check the certificate peername in SAN & Subject CommonName (CN) instead of nothing.


We're trying to fix that, and you have expended more effort typing in
poor excuses for doing an incomplete job, than the typing it would have
taken just to get it right in the first place.
My typing is only poor excuses? I'm trying my best to explain the patch intention. I said in the previous email, "We are targeting to the hostname verification, and meanwhile compatible with the IP in the URI". I also agree your suggestion & requires is reasonable & meaning to support the IP check in the certificate. So, my friendly advice is to separate the issues you raised instead of mixing them up.


Thanks,
Jiaxin

Re: [edk2-platforms: PATCH v3 8/9] Marvell/Drivers: SmbiosPlatformDxe: Load SMBIOS strings from PCD

Marcin Wojtas
 

Leif,

pt., 11 paź 2019 o 09:30 Marcin Wojtas <mw@...> napisał(a):

Hi Leif,

pt., 11 paź 2019 o 01:51 Leif Lindholm <leif.lindholm@...> napisał(a):

On Fri, Oct 11, 2019 at 01:33:49AM +0200, Marcin Wojtas wrote:
Hi Leif,

pt., 11 paź 2019 o 01:04 Leif Lindholm <leif.lindholm@...> napisał(a):

On Thu, Oct 10, 2019 at 07:42:18AM +0200, Marcin Wojtas wrote:
From: Patryk Duda <pdk@...>

This patch implements convenient way of changing strings included
in SMBIOS Table1, Table2, Table3.

Strings can be altered by defining following PCDs:
gMarvellTokenSpaceGuid.PcdProductManufacturer
gMarvellTokenSpaceGuid.PcdProductPlatformName
gMarvellTokenSpaceGuid.PcdProductVersion
gMarvellTokenSpaceGuid.PcdProductSerial

This patch adds also limit for length of string which can be increased
if necessary in future.

Signed-off-by: Patryk Duda <pdk@...>
---
Silicon/Marvell/Marvell.dec | 6 ++
Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf | 4 +
Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c | 79 +++++++++++++++++---
3 files changed, 78 insertions(+), 11 deletions(-)

diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec
index d337d3e..a84b056 100644
--- a/Silicon/Marvell/Marvell.dec
+++ b/Silicon/Marvell/Marvell.dec
@@ -169,6 +169,12 @@
gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x0 }|VOID*|0x3000034
gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x0 }|VOID*|0x3000035

+#Platform description
+ gMarvellTokenSpaceGuid.PcdProductManufacturer|"Marvell \0"|VOID*|0x50000100
+ gMarvellTokenSpaceGuid.PcdProductPlatformName|"Marvell Development Board \0"|VOID*|0x50000101
+ gMarvellTokenSpaceGuid.PcdProductSerial|"Serial Not Set \0"|VOID*|0x50000103
+ gMarvellTokenSpaceGuid.PcdProductVersion|"Revision unknown \0"|VOID*|0x50000102
+
I'm not too pleased about this random number of spaces. I'm cool with
the strings, but they should be treated as such, not magical data
structures.
In v4 the trailing spaces will be removed from the defaults (as well as "\0").

+STATIC
+VOID
+MvSmbiosCopyStrings (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+
+ ASSERT (AsciiStrnLenS ((CHAR8 *)PcdGetPtr (PcdProductManufacturer),
+ MV_SMBIOS_STRING_MAX_SIZE) < MV_SMBIOS_STRING_MAX_SIZE);
+ ASSERT (AsciiStrnLenS ((CHAR8 *)PcdGetPtr (PcdProductPlatformName),
+ MV_SMBIOS_STRING_MAX_SIZE) < MV_SMBIOS_STRING_MAX_SIZE);
+ ASSERT (AsciiStrnLenS ((CHAR8 *)PcdGetPtr (PcdProductVersion),
+ MV_SMBIOS_STRING_MAX_SIZE) < MV_SMBIOS_STRING_MAX_SIZE);
+ ASSERT (AsciiStrnLenS ((CHAR8 *)PcdGetPtr (PcdProductSerial),
+ MV_SMBIOS_STRING_MAX_SIZE) < MV_SMBIOS_STRING_MAX_SIZE);
Especially given the current design, these ASSERTs seem a bit
... unhelpful. In fact, this whole MAX_SIZE thing seems ... restricted
by the implementation, not by external constraints. What is the
benefit? Not having to do a bunch of pointer conversions at
SetVirtualAddressMap?
The default static tables require constant initializers and the
placeholders should have some predefined size in current approach. So
max of 32 characters was picked and with the asserts, ensuring the
user will not exceed it. Do you think they should be removed?
Oh, you're saying this is basically "TO BE FILLED BY OEM"?
*yurgh*.

I still say it's horrible, but not more horrible than most existing
platforms. Nevertheless, the arbitrary size should be something
defined by the code generating the tables - it shouldn't depend on
what's in the .dec (or more usefully, the .dsc).

I also feel that if it is effectively "TO BE FILLED BY OEM", it would
be better if it said that than something that looks like it may be
proper names.

I would also prefer a compilation failure over an ASSERT if the string
ended up not fitting.
I think I found a solution to remove any fixed size constraint and asserts:
STATIC CHAR8 mSysInfoManufacturer[sizeof ((CHAR8 *)PcdGetPtr
(PcdProductManufacturer))];
STATIC CHAR8 mSysInfoProductName[sizeof ((CHAR8 *)PcdGetPtr
(PcdProductPlatformName))];
STATIC CHAR8 mSysInfoVersion[sizeof ((CHAR8 *)PcdGetPtr (PcdProductVersion))];
STATIC CHAR8 mSysInfoSerial[sizeof ((CHAR8 *)PcdGetPtr (PcdProductSerial))];
Compilation test was not enough, please ignore my previous email - I
have to find out something better :)

Best regards,
Marcin




+
+ Status = AsciiStrCpyS (mSysInfoManufacturer,
+ MV_SMBIOS_STRING_MAX_SIZE,
+ (CHAR8 *)PcdGetPtr (PcdProductManufacturer));
+ ASSERT_EFI_ERROR (Status);
+ Status = AsciiStrCpyS (mSysInfoProductName,
+ MV_SMBIOS_STRING_MAX_SIZE,
+ (CHAR8 *)PcdGetPtr (PcdProductPlatformName));
+ ASSERT_EFI_ERROR (Status);
+ Status = AsciiStrCpyS (mSysInfoVersion,
+ MV_SMBIOS_STRING_MAX_SIZE,
+ (CHAR8 *)PcdGetPtr (PcdProductVersion));
+ ASSERT_EFI_ERROR (Status);
+ Status = AsciiStrCpyS (mSysInfoSerial,
+ MV_SMBIOS_STRING_MAX_SIZE,
+ (CHAR8 *)PcdGetPtr (PcdProductSerial));
+ ASSERT_EFI_ERROR (Status);
+}
+
+/**
Install all structures from the DefaultTables structure

@param Smbios SMBIOS protocol
@@ -760,6 +815,8 @@ SmbiosInstallAllStructures (
FirmwareMajorRevisionNumber = (PcdGet32 (PcdFirmwareRevision) >> 16) & 0xFF;
FirmwareMinorRevisionNumber = PcdGet32 (PcdFirmwareRevision) & 0xFF;

+ MvSmbiosCopyStrings();
+
//
// Update Firmware Revision, CPU and DRAM frequencies.
//
--
2.7.4

Re: [PATCH v2 1/2] UefiCpuPkg/MpInitLib: expand comment on initial AP enumeration

Ni, Ray
 

Reviewed-by: Ray Ni <ray.ni@...>

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Thursday, October 10, 2019 7:30 PM
To: edk2-devel-groups-io <devel@edk2.groups.io>
Cc: Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>
Subject: [PATCH v2 1/2] UefiCpuPkg/MpInitLib: expand comment on initial
AP enumeration

Before adding another AP enumeration mode, clarify the documentation on
the current logic. No functional changes.

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
Signed-off-by: Laszlo Ersek <lersek@...>
---

Notes:
v2:
- new patch

UefiCpuPkg/Library/MpInitLib/MpLib.c | 38 +++++++++++++++-----
1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index d6f84c6f45c0..594a035d8b92 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1045,14 +1045,36 @@ WakeUpAP (
}
if (CpuMpData->InitFlag == ApInitConfig) {
//
- // Here support two methods to collect AP count through adjust
- // PcdCpuApInitTimeOutInMicroSeconds values.
- //
- // one way is set a value to just let the first AP to start the
- // initialization, then through the later while loop to wait all Aps
- // finsh the initialization.
- // The other way is set a value to let all APs finished the initialzation.
- // In this case, the later while loop is useless.
+ // The AP enumeration algorithm below is suitable for two use cases.
+ //
+ // (1) The check-in time for an individual AP is bounded, and APs run
+ // through their initialization routines strongly concurrently. In
+ // particular, the number of concurrently running APs
+ // ("NumApsExecuting") is never expected to fall to zero
+ // *temporarily* -- it is expected to fall to zero only when all
+ // APs have checked-in.
+ //
+ // In this case, the platform is supposed to set
+ // PcdCpuApInitTimeOutInMicroSeconds to a low-ish value (just long
+ // enough for one AP to start initialization). The timeout will be
+ // reached soon, and remaining APs are collected by watching
+ // NumApsExecuting fall to zero. If NumApsExecuting falls to zero
+ // mid-process, while some APs have not completed initialization,
+ // the behavior is undefined.
+ //
+ // (2) The check-in time for an individual AP is unbounded, and/or APs
+ // may complete their initializations widely spread out. In
+ // particular, some APs may finish initialization before some APs
+ // even start.
+ //
+ // In this case, the platform is supposed to set
+ // PcdCpuApInitTimeOutInMicroSeconds to a high-ish value. The AP
+ // enumeration will always take that long (except when the boot CPU
+ // count happens to be maximal, that is,
+ // PcdCpuMaxLogicalProcessorNumber). All APs are expected to
+ // check-in before the timeout, and NumApsExecuting is assumed zero
+ // at timeout. APs that miss the time-out may cause undefined
+ // behavior.
//
TimedWaitForApFinish (
CpuMpData,
--
2.19.1.3.g30247aa5d201

Re: [PATCH v2 2/2] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection

Ni, Ray
 

Laszlo, the comments couldn't be better! Thanks!!

Reviewed-by: Ray Ni <ray.ni@...>

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Thursday, October 10, 2019 7:30 PM
To: edk2-devel-groups-io <devel@edk2.groups.io>
Cc: Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>
Subject: [PATCH v2 2/2] UefiCpuPkg/MpInitLib: honor the platform's boot
CPU count in AP detection

- If a platform boots such that the boot CPU count is smaller than
PcdCpuMaxLogicalProcessorNumber, then the platform cannot use the
"fast
AP detection" logic added in commit 6e1987f19af7. (Which has been
documented as a subset of use case (2) in the previous patch.)

Said logic depends on the boot CPU count being equal to
PcdCpuMaxLogicalProcessorNumber. If the equality does not hold, the
platform either has to wait too long, or risk missing APs due to an
early timeout.

- The platform may not be able to use the variant added in commit
0594ec417c89 either. (Which has been documented as use case (1) in the
previous patch.)

See commit 861218740d6d. When OVMF runs on QEMU/KVM, APs may
check in
with the BSP in arbitrary order, plus the individual AP may take
arbitrarily long to check-in. If "NumApsExecuting" falls to zero
mid-enumeration, APs will be missed.

Allow platforms to specify the exact boot CPU count, independently of
PcdCpuMaxLogicalProcessorNumber. In this mode, the BSP waits for all APs
to check-in regardless of timeout. If at least one AP fails to check-in, then the
AP enumeration hangs forever. That is the desired behavior when the exact
boot CPU count is known in advance. (A hung boot is better than an AP
checking-in after timeout, and executing code from released
storage.)

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
Signed-off-by: Laszlo Ersek <lersek@...>
---

Notes:
v2:
- update commit message
- update docs in DEC file
- add code comments
- no functional changes

UefiCpuPkg/UefiCpuPkg.dec | 13 +++
UefiCpuPkg/UefiCpuPkg.uni | 4 +
UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 +
UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 3 +-
UefiCpuPkg/Library/MpInitLib/MpLib.c | 99 ++++++++++++--------
5 files changed, 80 insertions(+), 40 deletions(-)

diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 031a2ccd680a..12f4413ea5b0 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -227,6 +227,19 @@ [PcdsFixedAtBuild, PcdsPatchableInModule,
PcdsDynamic, PcdsDynamicEx]
## Specifies timeout value in microseconds for the BSP to detect all APs for
the first time.
# @Prompt Timeout for the BSP to detect all APs for the first time.

gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000|
UINT32|0x00000004
+ ## Specifies the number of Logical Processors that are available in
+ the # preboot environment after platform reset, including BSP and
+ APs. Possible # values:<BR><BR> # zero (default) -
+ PcdCpuBootLogicalProcessorNumber is ignored, and
+ # PcdCpuApInitTimeOutInMicroSeconds limits the initial AP
+ # detection by the BSP.<BR>
+ # nonzero - PcdCpuApInitTimeOutInMicroSeconds is ignored. The
initial
+ # AP detection finishes only when the detected CPU count
+ # (BSP plus APs) reaches the value of
+ # PcdCpuBootLogicalProcessorNumber, regardless of how long
+ # that takes.<BR>
+ # @Prompt Number of Logical Processors available after platform reset.
+
+
gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0|UINT
32|0x
+ 00000008
## Specifies the base address of the first microcode Patch in the microcode
Region.
# @Prompt Microcode Region base address.

gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress|0x0|UINT64|
0x00000005
diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni index
fbf768072668..a7e279c5cb14 100644
--- a/UefiCpuPkg/UefiCpuPkg.uni
+++ b/UefiCpuPkg/UefiCpuPkg.uni
@@ -37,6 +37,10 @@

#string
STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApInitTimeOutInMicroSeconds_
HELP #language en-US "Specifies timeout value in microseconds for the BSP
to detect all APs for the first time."

+#string
STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_PR
OMPT #language en-US "Number of Logical Processors available after
platform reset."
+
+#string
STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_HE
LP #language en-US "Specifies the number of Logical Processors that are
available in the preboot environment after platform reset, including BSP and
APs."
+
#string
STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuMicrocodePatchAddress_PROMP
T #language en-US "Microcode Region base address."

#string
STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuMicrocodePatchAddress_HELP
#language en-US "Specifies the base address of the first microcode Patch in
the microcode Region."
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
index 37b3f64e578a..cd912ab0c5ee 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -61,6 +61,7 @@ [Guids]

[Pcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ##
CONSUMES
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber ##
CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ##
SOMETIMES_CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ##
CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ##
CONSUMES
diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
index 82b77b63ea87..1538185ef99a 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
@@ -53,7 +53,8 @@ [LibraryClasses]

[Pcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ##
CONSUMES
- gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ##
CONSUMES
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber ##
CONSUMES
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ##
SOMETIMES_CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ##
CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ##
CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize ##
CONSUMES
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 594a035d8b92..622b70ca3c4e 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1044,46 +1044,67 @@ WakeUpAP (
SendInitSipiSipiAllExcludingSelf ((UINT32) ExchangeInfo->BufferStart);
}
if (CpuMpData->InitFlag == ApInitConfig) {
- //
- // The AP enumeration algorithm below is suitable for two use cases.
- //
- // (1) The check-in time for an individual AP is bounded, and APs run
- // through their initialization routines strongly concurrently. In
- // particular, the number of concurrently running APs
- // ("NumApsExecuting") is never expected to fall to zero
- // *temporarily* -- it is expected to fall to zero only when all
- // APs have checked-in.
- //
- // In this case, the platform is supposed to set
- // PcdCpuApInitTimeOutInMicroSeconds to a low-ish value (just long
- // enough for one AP to start initialization). The timeout will be
- // reached soon, and remaining APs are collected by watching
- // NumApsExecuting fall to zero. If NumApsExecuting falls to zero
- // mid-process, while some APs have not completed initialization,
- // the behavior is undefined.
- //
- // (2) The check-in time for an individual AP is unbounded, and/or APs
- // may complete their initializations widely spread out. In
- // particular, some APs may finish initialization before some APs
- // even start.
- //
- // In this case, the platform is supposed to set
- // PcdCpuApInitTimeOutInMicroSeconds to a high-ish value. The AP
- // enumeration will always take that long (except when the boot CPU
- // count happens to be maximal, that is,
- // PcdCpuMaxLogicalProcessorNumber). All APs are expected to
- // check-in before the timeout, and NumApsExecuting is assumed zero
- // at timeout. APs that miss the time-out may cause undefined
- // behavior.
- //
- TimedWaitForApFinish (
- CpuMpData,
- PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
- PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
- );
+ if (PcdGet32 (PcdCpuBootLogicalProcessorNumber) > 0) {
+ //
+ // The AP enumeration algorithm below is suitable only when the
+ // platform can tell us the *exact* boot CPU count in advance.
+ //
+ // The wait below finishes only when the detected AP count reaches
+ // (PcdCpuBootLogicalProcessorNumber - 1), regardless of how long
that
+ // takes. If at least one AP fails to check in (meaning a platform
+ // hardware bug), the detection hangs forever, by design. If the actual
+ // boot CPU count in the system is higher than
+ // PcdCpuBootLogicalProcessorNumber (meaning a platform
+ // misconfiguration), then some APs may complete initialization after
+ // the wait finishes, and cause undefined behavior.
+ //
+ TimedWaitForApFinish (
+ CpuMpData,
+ PcdGet32 (PcdCpuBootLogicalProcessorNumber) - 1,
+ MAX_UINT32 // approx. 71 minutes
+ );
+ } else {
+ //
+ // The AP enumeration algorithm below is suitable for two use cases.
+ //
+ // (1) The check-in time for an individual AP is bounded, and APs run
+ // through their initialization routines strongly concurrently. In
+ // particular, the number of concurrently running APs
+ // ("NumApsExecuting") is never expected to fall to zero
+ // *temporarily* -- it is expected to fall to zero only when all
+ // APs have checked-in.
+ //
+ // In this case, the platform is supposed to set
+ // PcdCpuApInitTimeOutInMicroSeconds to a low-ish value (just long
+ // enough for one AP to start initialization). The timeout will be
+ // reached soon, and remaining APs are collected by watching
+ // NumApsExecuting fall to zero. If NumApsExecuting falls to zero
+ // mid-process, while some APs have not completed initialization,
+ // the behavior is undefined.
+ //
+ // (2) The check-in time for an individual AP is unbounded, and/or APs
+ // may complete their initializations widely spread out. In
+ // particular, some APs may finish initialization before some APs
+ // even start.
+ //
+ // In this case, the platform is supposed to set
+ // PcdCpuApInitTimeOutInMicroSeconds to a high-ish value. The AP
+ // enumeration will always take that long (except when the boot CPU
+ // count happens to be maximal, that is,
+ // PcdCpuMaxLogicalProcessorNumber). All APs are expected to
+ // check-in before the timeout, and NumApsExecuting is assumed
zero
+ // at timeout. APs that miss the time-out may cause undefined
+ // behavior.
+ //
+ TimedWaitForApFinish (
+ CpuMpData,
+ PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
+ PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
+ );

- while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) {
- CpuPause();
+ while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) {
+ CpuPause();
+ }
}
} else {
//
--
2.19.1.3.g30247aa5d201

Re: [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES

Laszlo Ersek
 

On 10/11/19 01:17, Lendacky, Thomas wrote:
On 10/3/19 10:12 AM, Tom Lendacky wrote:


On 10/3/19 5:32 AM, Laszlo Ersek wrote:
On 10/03/19 12:12, Laszlo Ersek wrote:

UINT32 ApEntryPoint;
EFI_GUID SevEsFooterGuid;
UINT16 Size;
It's probably better to reverse the order of "Size" and
"SevEsFooterGuid", like this:

UINT32 ApEntryPoint;
UINT16 Size;
EFI_GUID SevEsFooterGuid;

because then even the "Size" field can be changed (or resized), as a
function of the footer GUID.
Cool, I'll look into doing this and see how it works out.
Just an update on this idea. This has worked out well, but has a couple of
caveats. Removing the Qemu change to make the flash mapped read-only in
the nested page tables, caused the following:

1. QemuFlashDetected() will attempt to detect how the flash memory device
behaves. Because it is marked as read-only by the hypervisor, writing
to the area results in a #NPF for the write-fault. With SEV-ES,
emulation of the instruction can't be performed (can't read guest
memory and not provided the faulting instruction bytes), so the vCPU is
just restarted. This results in an infinite #NPF occurring.

The solution here was to check for SEV-ES being enabled and just return
false from QemuFlashDetected(). Any downfalls to doing that?
Short-circuiting QemuFlashDetected() on SEV-ES seems appropriate.

However, I don't understand why you return FALSE in that case. You
should return TRUE. If QemuFlashDetected() returns FALSE, then the UEFI
variable store will not be backed by the real pflash chip, it will be
emulated with an \NvVars file on the EFI system partition. That
emulation should really not be used nowadays.

So IMO the right approach here is:
- declare that SEV-ES only targets the "two pflash chips" setup
- return TRUE from QemuFlashDetected() when SEV-ES is on.


2. Commit 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass
XCODE5 tool chain") causes a similar situation to #1. It attempts to do
some address fixups and write to the flash device.
That's... stunning.

Commit 2db0ccc2d7fe changes the file

UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm

such that it does in-place binary patching.

This source file is referenced from:

UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf

as well. Note "SecPei".

That makes the commit buggy, to my eyes, regardless of SEV-ES. Because:

The binary patching appears to occur in the SEC phase as well, i.e. at a
time when the exception handler is located in flash. That's incorrect on
physical hardware too.

Upon re-reading <https://bugzilla.tianocore.org/show_bug.cgi?id=849>,
this commit worked around an XCODE toolchain bug.

Unfortunately, the workaround is not suitable for the SEC phase. (Also
not suitable for the PEI phase, for such PEIMs that still execute from
flash.)

Please open a new bug for UefiCpuPkg in the TianoCore Bugzilla,
reference BZ#849 in the See Also field, and please also make the new bug
block BZ#2198.

(I'll comment on this issue in a different thread too; I'll CC you on it.)

Reverting that commit fixes the issue. I don't think that will be an
acceptable solution, though, so need to think about what to do here.

After those two changes, the above method works well.
I'm happy to hear!

Thanks,
Laszlo

Re: [Patch 11/12] OvmfPkg: Enable CLANG9 tool chain

Laszlo Ersek
 

Hi Liming,

On 10/09/19 16:44, Gao, Liming wrote:

The difference between XCODE/CLANG and GCCXX is the linker. Current
patches are introduced for the different linker. Clang supports most
usage of GCC compiler. So, CLANG and XCODE uses GCC family. When I
enable XCODE or CLANG tool chain in the platform, I don't find other
incompatible behavior with GCC compiler. So, I think it is safe to let
CLANG inherit GCC option except for these two cases. When you make new
changes, and verify them with GCC compiler, that's enough. You don't
need to specially verify them with XCODE or CLANG. In most case, GCC
compiler pass, XCODE or CLANG can also pass.
I'd like to provide a counter-example for this.

Consider the issue that Tom reported, at

http://mid.mail-archive.com/8eb55d97-0ba3-c217-a160-c24730b9f036@...
https://edk2.groups.io/g/devel/message/48762

in point (2).

(Both links point to the same message, just in different archives.)

Let me summarize that problem.

- In BZ#849, an XCODE toolchain bug was reported, and a workaround was
requested.

- The workaround was commit 2db0ccc2d7fe ("UefiCpuPkg: Update
CpuExceptionHandlerLib pass XCODE5 tool chain", 2018-01-16).

- The workaround is binary patching (self-modification) in the exception
handler's assembly code.

- Unfortunately, this code is also linked into SEC modules, which run
from flash. Therefore, self-modification is not permitted, and the
workaround is not good enough for SEC. (Nor for PEI modules that run
from flash.)

Now, let's consider how we could mitigate this issue for *temporarily*,
for all toolchains *except* XCODE5, until the issue is fixed. Clearly,
toolchains other than XCODE5 have no problems with the original assembly
code (i.e., with the 64-bit absolute address relocations). Therefore, we
could consider reverting the commit, and capturing the results of the
revert in a *separate* NASM source file. This source file (that is, the
original, pre-2db0ccc2d7fe assembly code) would apply to all toolchain
families, except the XCODE family.

Let's say the new NASM source files would be called
- X64/ExceptionHandlerAsmGeneric.nasm -- all except XCODE,
- X64/ExceptionHandlerAsmXcode.nasm -- XCODE,

replacing the current file

- X64/ExceptionHandlerAsm.nasm

So, how can we use the new files, in the INF file

UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf

?

We could attempt,

[Sources.X64]
X64/ExceptionHandlerAsmGeneric.nasm | GCC
X64/ExceptionHandlerAsmXcode.nasm | XCODE
X64/ExceptionHandlerAsmGeneric.nasm | INTEL
X64/ExceptionHandlerAsmGeneric.nasm | MSFT
Unfortunately, this does not work.

While it solves the issue on GCC, INTEL and MSFT, it breaks on XCODE:
XCODE picks up *both* the GCC line and the XCODE line. And that's
because XCODE inherits GCC, and there is presently no way to express
"real GCC only".

But, at least, this suggests a solution too. In
"BaseTools/Conf/tools_def.template", for *every* toolchain that
specifies FAMILY=GCC, we should spell out an explicit BUILDRULEFAMILY.
Like this:

@@ -1995,6 +1995,7 @@
#
####################################################################################
*_GCC48_*_*_FAMILY = GCC
+*_GCC48_*_*_BUILDRULEFAMILY = GENUINE_GCC

*_GCC48_*_MAKE_PATH = DEF(GCC_HOST_PREFIX)make
*_GCC48_*_*_DLL = ENV(GCC48_DLL)
@@ -2134,6 +2135,7 @@
#
####################################################################################
*_GCC49_*_*_FAMILY = GCC
+*_GCC49_*_*_BUILDRULEFAMILY = GENUINE_GCC

*_GCC49_*_MAKE_PATH = DEF(GCC_HOST_PREFIX)make
*_GCC49_*_*_DLL = ENV(GCC49_DLL)
@@ -2280,6 +2282,7 @@
#
####################################################################################
*_GCC5_*_*_FAMILY = GCC
+*_GCC5_*_*_BUILDRULEFAMILY = GENUINE_GCC

*_GCC5_*_MAKE_PATH = DEF(GCC_HOST_PREFIX)make
*_GCC5_*_*_DLL = ENV(GCC5_DLL)
@@ -2437,6 +2440,7 @@
#
####################################################################################
*_CLANG35_*_*_FAMILY = GCC
+*_CLANG35_*_*_BUILDRULEFAMILY = CLANG

*_CLANG35_*_MAKE_PATH = make
*_CLANG35_*_*_DLL = ENV(CLANG35_DLL)
@@ -2517,6 +2521,8 @@
#
####################################################################################
*_CLANG38_*_*_FAMILY = GCC
+*_CLANG38_*_*_BUILDRULEFAMILY = CLANG
+
*_CLANG38_*_MAKE_PATH = make
*_CLANG38_*_*_DLL = ENV(CLANG38_DLL)
*_CLANG38_*_ASL_PATH = DEF(UNIX_IASL_BIN)
And then we could write:

[Sources.X64]
X64/ExceptionHandlerAsmGeneric.nasm | GENUINE_GCC
X64/ExceptionHandlerAsmGeneric.nasm | CLANG
X64/ExceptionHandlerAsmXcode.nasm | XCODE
X64/ExceptionHandlerAsmGeneric.nasm | INTEL
X64/ExceptionHandlerAsmGeneric.nasm | MSFT
replacing plain "GCC", which XCODE inherits, with "GENUINE_GCC" and
"CLANG", neither of which XCODE inherits.

Would you accept the above patch, for
"BaseTools/Conf/tools_def.template"?

Thanks,
Laszlo

Re: [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event

Laszlo Ersek
 

On 10/11/19 02:14, Fu, Siyuan wrote:
-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: 2019年10月11日 0:06
To: Fu, Siyuan <@sfu5>; devel@edk2.groups.io; Rabeda,
Maciej <maciej.rabeda@...>
Cc: Wu, Jiaxin <jiaxin.wu@...>
Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
ExitBootServices event

On 10/10/19 11:29, Fu, Siyuan wrote:
-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: 2019年10月10日 16:06
To: Fu, Siyuan <@sfu5>; devel@edk2.groups.io; Rabeda,
Maciej <maciej.rabeda@...>
Cc: Wu, Jiaxin <jiaxin.wu@...>
Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
ExitBootServices event

On 10/10/19 05:32, Fu, Siyuan wrote:
Hi, Maciej

Considering that this patch has to co-work with corresponding UNDI
device
driver
bug fix, in order to avoid potential compatibility problem, please add a
PCD
to
NetworkPkg for this fix, and set the default value to disable state (no
behavior
change). The platform which need this fix could set the PCD to enable in
its
platform DSC.
Should the new PCD go into "NetworkPkg/NetworkPcds.dsc.inc", and be
gated with a new build flag defined in
"NetworkPkg/NetworkDefines.dsc.inc"?

Currently not all the network package PCDs are listed in the
NetworkPcds.dsc.inc,
But I do not oppose it if you think this PCD should be controlled by a build
flag.

I mainly asked from a consistency point of view.

The inclusion of SnpDxe is already controlled by NETWORK_SNP_ENABLE, at
least in platforms that utilize the NetworkPkg *.inc files. And the PCD
in question would control the behavior of SnpDxe at ExitBootServices(),
if I understand correctly.

This is similar to NETWORK_HTTP_BOOT_ENABLE and
NETWORK_ALLOW_HTTP_CONNECTIONS:
- The former includes HttpDxe and HttpBootDxe (and some other drivers).
- The latter controls PcdAllowHttpConnections, which is consumed by
HttpDxe and HttpBootDxe.

I don't feel too strongly about it, I just thought it worth raising.
It's not an existing "consistency" in current network package. The current macros
are mostly used for control more high level feature enable/disable, not such kind
of bug fix,
For example, NETWORK_ISCSI_ENABLE includes IScsiDxe driver, while
PcdIScsiAIPNetworkBootPolicy is consumed by IScsiDriver, but not controlled by
a macro. And also other PXE, DHCP and PXE related PCDs.
Good point!

So, I'm fine if the new feature PCD is not exposed with a build flag.

Thanks
Laszlo

Re: [Patch 12/12] OvmfPkg SecMain: Add build option "-fno-omit-frame-pointer" for CLANG9 X64

Laszlo Ersek
 

On 10/11/19 03:30, Liming Gao wrote:
Laszlo:

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Friday, October 11, 2019 1:35 AM
To: devel@edk2.groups.io; Gao, Liming <liming.gao@...>; Justen,
Jordan L <jordan.l.justen@...>
Subject: Re: [edk2-devel] [Patch 12/12] OvmfPkg SecMain: Add build option "-
fno-omit-frame-pointer" for CLANG9 X64

Hi Liming,

On 10/10/19 16:08, Liming Gao wrote:
Laszlo:
Option (a) works. Jordan patch can fix this issue.
Option (b) doesn't work. Even if disable optimization, CLANG doesn't
generate the code with push rbp & pop rbp.

So, Jordan patch becomes only option. We can discuss this topic again. But,
I don't think this is the block issue
to add new CLANG9 tool chain. I will try to add CLANG9 tool chain patch
without this change, and use BZ 2024 to track this issue.

not sure I understand you correctly.

- If you intend to drop this patch from the series, and enable CLANG9
for edk2 without enabling it for OvmfPkg specifically (for the time
being), that's fine with me.
I mean to drop this patch from this patch set. I will use BZ 2024 to track this issue.
Without this fix, CLANG9 OVMF X64 boot will hang. CLANG9 OVMF Ia32 and Ia32X64 boots fine.
OK, thank you.
Laszlo

- If you'd like to push this patch first, and work on the general PEI
Core issue second, then I defer to Jordan for reviewing this patch. I'd
like to neither approve nor reject this patch myself, in that case.

Thanks
Laszlo

Re: [Patch 11/12] OvmfPkg: Enable CLANG9 tool chain -

Laszlo Ersek
 

On 10/11/19 03:47, Liming Gao wrote:
Laszlo:

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Friday, October 11, 2019 12:43 AM
To: devel@edk2.groups.io; Gao, Liming <liming.gao@...>; Andrew Fish
<afish@...>
Cc: Justen, Jordan L <jordan.l.justen@...>
Subject: Re: [edk2-devel] [Patch 11/12] OvmfPkg: Enable CLANG9 tool chain -

Hi Liming,

On 10/10/19 14:18, Liming Gao wrote:
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Thursday, October 10, 2019 3:35 PM
To: Andrew Fish <afish@...>; Gao, Liming <liming.gao@...>
Cc: devel@edk2.groups.io
Subject: Re: [edk2-devel] [Patch 11/12] OvmfPkg: Enable CLANG9 tool
chain -

Hi Andrew,

On 10/09/19 18:22, Andrew Fish wrote:

I thought the thing we were discussing was compiler flags.
Specifically -mno-mmx -mno-sse. It seems to me if OVMF requires
-mno-mmx -mno-sse then it is a bug in the tools_def.txt definition
for those compilers? As far as I can tell -mno-implicit-float should
prevent the optimizer from using floating point. The -mno-mmx
-mno-sse flags most change how floating point code gets compiled [1].
it looks like -mno-mmx -mno-sse just down grade floating point
instructions that get used. Thus it seems like either we have some
code doing float and that code should set -mno-mmx -mno-sse, or the
-mno-mmx -mno-sse should be set generically.
The origin of those build flags in OVMF is commit 4a8266f570ef
("OvmfPkg: Work around issue seen with kvm + grub2 (efi)", 2010-12-31):

OvmfPkg: Work around issue seen with kvm + grub2 (efi)

When OVMF is run with kvm and grub2 (efi), an exception
occurs when mmx/sse registers are accessed.

As a work around, this change eliminates firmware usage
of these register types.

First, only the BaseMemoryLib implementation
MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
is used.

Second, the GCC compiler is passes the additional
'-mno-mmx -mno-sse' parameters.

The eternal problem with this kind of workaround is that we never know
when it becomes safe to remove.

It would be nice to understand the exact details around the problem.
Given that the commit is from 2010, I assume the issue is difficult to
reproduce with recent components (KVM, grub2). On the other hand, if
we
just remove the flags (which we should, in a perfect world), someone
could report a bug in three months: "my host distro upgraded the OVMF
package to the next edk2-stableYYYYMM tag, and now my virtual machine,
which runs a distro from 2009, no longer boots". (We've seen similar
reports before.)
Yes. I agree it is hard to decide to remove this option, because we don't
know its impact.
With the option -mno-mmx -mno-sse, it will cause CLANG9 linker failure like
below. So, I say
this patch is to support the different linker.

0. Program arguments: C:\Program Files\LLVM\bin\lld-link.EXE
/OUT:d:\allpkg\edk2\Build\Ovmf3264\DEBUG_CLANG9\X64\Se
curityPkg\VariableAuthenticated\SecureBootConfigDxe\SecureBootConfigDx
e\DEBUG\SecureBootConfigDxe.dll /NOLOGO /NODEFAULT
LIB /IGNORE:4001 /OPT:REF /OPT:ICF=10 /ALIGN:32 /FILEALIGN:32
/SECTION:.xdata,D /SECTION:.pdata,D /Machine:X64 /DLL /ENT
RY:_ModuleEntryPoint /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER
/SAFESEH:NO /BASE:0 /DEBUG:GHASH /lldmap @d:\allpkg\edk2\Build\O
vmf3264\DEBUG_CLANG9\X64\SecurityPkg\VariableAuthenticated\SecureBo
otConfigDxe\SecureBootConfigDxe\OUTPUT\static_library
_files.lst
1. Running pass 'Function Pass Manager' on module 'ld-temp.o'.
2. Running pass 'X86 FP Stackifier' on function '@drbg_add'
#0 0x00007ff696bad7f8 C:\Program Files\LLVM\bin\lld-link.EXE 0x148d7f8
C:\Program Files\LLVM\bin\lld-link.EXE 0x142ba7f

can you please try the following:

(a) replace this patch with two patches, as follows

(b) in the first patch, only rework

!if $(TOOL_CHAIN_TAG) != "XCODE5"
GCC:*_*_*_CC_FLAGS = -mno-mmx -mno-sse
!endif

into:

GCC:*_*_*_CC_FLAGS = -mno-mmx -mno-sse
XCODE:*_*_*_CC_FLAGS =

(c) in the second patch, append:

CLANGPE:*_*_*_CC_FLAGS =
This way clear CLANG9 CC flags. It forbids CLANG9 to inherit other GCC flags.
Below flags are not applied into CLANG9. The purpose is not to include
specific -mno-mmx -mno-sse option, and still inherit other GCC flags.
So, I have to use $(TOOL_CHAIN_TAG) check.

MSFT:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
INTEL:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
GCC:*_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES

and also introduce

CLANGPE: *_*_*_DLINK_FLAGS = /ALIGN:4096


If (b) preserves the intended effect for the XCODE5 toolchain, and (c)
does the right thing for the CLANG9 toolchain as well, then that
approach would be preferable to the currently proposed patch#11.

If the above does *not* work, then I'm fine with the currently proposed
patch, but then please update the commit message; it should say that
"-mno-mmx -mno-sse" is *excluded* for CLANG9.
Yes. I will update the commit message.
OK. With the commit message updated, for this patch:

Reviewed-by: Laszlo Ersek <lersek@...>

For later, please consider the GENUINE_GCC suggestion (for
BUILDRULEFAMILY) in my other message in the present thread:

http://mid.mail-archive.com/43b303eb-fd74-7480-aa6d-6907300af3e0@...
https://edk2.groups.io/g/devel/message/48808

If you think that the "BaseTools/Conf/tools_def.template" patch that I
suggested there is acceptable, then I would like to update the OVMF DSC
files (later) as follows:

GENUINE_GCC:*_*_*_CC_FLAGS = -mno-mmx -mno-sse
CLANG:*_*_*_CC_FLAGS = -mno-mmx -mno-sse
/* say nothing about XCODE */
/* say nothing CLANGPE */

Thanks
Laszlo

Re: [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event

Rabeda, Maciej
 

Laszlo, Siyuan,

Very good input, thanks.
I'll introduce the V2 patch with that fix + control with fixed PCD - it works for me :)

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Friday, October 11, 2019 11:42
To: Fu, Siyuan <@sfu5>; devel@edk2.groups.io; Rabeda, Maciej <maciej.rabeda@...>
Cc: Wu, Jiaxin <jiaxin.wu@...>
Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event

On 10/11/19 02:14, Fu, Siyuan wrote:
-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: 2019年10月11日 0:06
To: Fu, Siyuan <@sfu5>; devel@edk2.groups.io; Rabeda,
Maciej <maciej.rabeda@...>
Cc: Wu, Jiaxin <jiaxin.wu@...>
Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
ExitBootServices event

On 10/10/19 11:29, Fu, Siyuan wrote:
-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: 2019年10月10日 16:06
To: Fu, Siyuan <@sfu5>; devel@edk2.groups.io; Rabeda,
Maciej <maciej.rabeda@...>
Cc: Wu, Jiaxin <jiaxin.wu@...>
Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
ExitBootServices event

On 10/10/19 05:32, Fu, Siyuan wrote:
Hi, Maciej

Considering that this patch has to co-work with corresponding UNDI
device
driver
bug fix, in order to avoid potential compatibility problem, please
add a
PCD
to
NetworkPkg for this fix, and set the default value to disable
state (no
behavior
change). The platform which need this fix could set the PCD to
enable in
its
platform DSC.
Should the new PCD go into "NetworkPkg/NetworkPcds.dsc.inc", and be
gated with a new build flag defined in
"NetworkPkg/NetworkDefines.dsc.inc"?

Currently not all the network package PCDs are listed in the
NetworkPcds.dsc.inc,
But I do not oppose it if you think this PCD should be controlled by
a build
flag.

I mainly asked from a consistency point of view.

The inclusion of SnpDxe is already controlled by NETWORK_SNP_ENABLE,
at least in platforms that utilize the NetworkPkg *.inc files. And
the PCD in question would control the behavior of SnpDxe at
ExitBootServices(), if I understand correctly.

This is similar to NETWORK_HTTP_BOOT_ENABLE and
NETWORK_ALLOW_HTTP_CONNECTIONS:
- The former includes HttpDxe and HttpBootDxe (and some other drivers).
- The latter controls PcdAllowHttpConnections, which is consumed by
HttpDxe and HttpBootDxe.

I don't feel too strongly about it, I just thought it worth raising.
It's not an existing "consistency" in current network package. The
current macros are mostly used for control more high level feature
enable/disable, not such kind of bug fix, For example,
NETWORK_ISCSI_ENABLE includes IScsiDxe driver, while
PcdIScsiAIPNetworkBootPolicy is consumed by IScsiDriver, but not
controlled by a macro. And also other PXE, DHCP and PXE related PCDs.
Good point!

So, I'm fine if the new feature PCD is not exposed with a build flag.

Thanks
Laszlo
--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

Re: [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Laszlo Ersek
 

On 10/11/19 04:24, Wu, Jiaxin wrote:
Hi Laszlo & David,

I think I have *repeated* several times that we are targeting to fix the HostName validation issue, not the IP or email address. *But* even so, the series patches for UEFI TLS is also allowable to specify IP as host name for CN or dNSName of SAN in the certificate. That's why I said "if the CN or SAN in the certificate are set correctly, it should be OK to pass the verification". The failure you mentioned here is to set the IP in iPAddress of SAN, I agree it's the routine and suggested setting, *but* obviously, it's not the target we are supported according the implementation/description of TlsSetVerifyHost. We are targeting to the hostname verification, and meanwhile compatible with the IP in the URI (But need the *correct* certificate setting).

IP addresses stored in the DNS names and CN are of cause ignored by X509_check_ip & X509_check_ip_asc().

Post my explain again:
UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe),
not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2:
"...TLS session hostname for validation which is used to verify whether the name
within the peer certificate matches a given host name..."
In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real
FQDN (www.xxx.com) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])),
and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST.
That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature
by following the standard TLS HostName verification capability.
Now, let's talking the iPAddress setting in SAN (same as email address), if you do need such feature that verify the IP in X509_check_ip & X509_check_ip_asc , please report new Bugzilla (need TLS Spec update the expose the setting interface), don't mix up the HTTPS hostname verification here.
(To clarify my stance.)

Given the above statement of scope, and given the test env details that
I included in my previous message:

series
Tested-by: Laszlo Ersek <lersek@...>

I understand that my testing does not cover the use case described by David.

So my Tested-by is certainly *not* intended as the "last word" in this
thread, or anything like that. I'm only saying this patch set is good
enough for me, not that everyone should find it good enough for them.

Thanks
Laszlo


-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Friday, October 11, 2019 2:04 AM
To: David Woodhouse <dwmw2@...>; Wu, Jiaxin
<jiaxin.wu@...>; devel@edk2.groups.io; Wang, Jian J
<jian.j.wang@...>; Bret Barkelew <Bret.Barkelew@...>
Cc: Richard Levitte <levitte@...>
Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName
validation feature(CVE-2019-14553)

On 10/10/19 17:45, David Woodhouse wrote:
On Thu, 2019-10-10 at 10:00 +0200, Laszlo Ersek wrote:
Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office,
OU=IPv6 cert, CN=fd33:eb1b:9b36::2

Yeah, you're not actually testing the case I'm talking about. You want
a GEN_IP in the x509v3 Subject Alternative Name.

Compare with...

$ openssl s_client -connect vpn-i-ha01.intel.com:443 2>/dev/null | openssl
x509 -noout -text | grep -A1 Alternative
X509v3 Subject Alternative Name:
DNS:vpn-int.intel.com, DNS:scsidcint01-a.intel.com, IP
Address:134.191.232.101

$ curl https://134.191.232.101/
OK, thank you.

I can imagine two failure modes, with the patches applied.

(1) Edk2 ignores the GEN_IP in the SAN, and rejects a matching server
certificate.

(2) Edk2 is confused by the GEN_IP in the SAN, and accepts an invalid
(mismatched) server certificate.

Can we tell which failure mode applies?

(I can't test it easily myself, as I don't even know how to create a
server certificate with a SAN -- any kind of SAN, let alone GEN_IP.)

Case (2) is clearly bad, and it would be a sign that the patch series
does not fully fix the issue.

Case (1) would be tolerable, in my opinion. I assume a GEN_IP SAN is
pretty rare in practice. Thus regressing it (perhaps temporarily) should
be an acceptable trade-off for fixing the current gaping hole (= subject
name not checked at all).

Thanks
Laszlo

[edk2-platforms][PATCH v3 0/5] Various SMBIOS improvements

Pete Batard
 

Changes from v2:
- Consistent use of curly brackets for if statements
- Remove unused variables that may produce an issue with git bisect
- Use gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor to populate vendor

Note that 5/5, which has been R-b, is unchanged.

Pete Batard (5):
Platform/RPi3/RpiFirmwareDxe: Add more query functions
Platform/RPi3/RpiFirmwareDxe: Improve serial number population
Platform/RPi3/PlatformSmbiosDxe: Improve population of SMBIOS entries
Platform/RPi3/PlatformSmbiosDxe: Populate BIOS major/minor from PCD
Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision

Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c | 198 ++++++++++++--------
Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf | 2 +
Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 167 ++++++++++++++++-
Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h | 58 ++++--
Platform/RaspberryPi/RPi3/RPi3.dsc | 4 +-
5 files changed, 333 insertions(+), 96 deletions(-)

--
2.21.0.windows.1

[edk2-platforms][PATCH v3 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions

Pete Batard
 

This patch introduces the capability to also query the Model Name/
Manufacturer Name/CPU Name/Firmware Revision using the RpiFirmware
protocol. This is aims at making the driver more suitable to cater
for platforms other than the Raspberry Pi 3 as well as simplifying
the population of entries in PlatformSmbiosDxe.

Signed-off-by: Pete Batard <@pbatard>
---
Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 158 +++++++++++++++++++-
Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h | 58 +++++--
2 files changed, 199 insertions(+), 17 deletions(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
index 9b5ee1946279..25d7fa3974c0 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
@@ -461,7 +461,7 @@ typedef struct {
RPI_FW_TAG_HEAD TagHead;
RPI_FW_MODEL_REVISION_TAG TagBody;
UINT32 EndTag;
-} RPI_FW_GET_MODEL_REVISION_CMD;
+} RPI_FW_GET_REVISION_CMD;
#pragma pack()

STATIC
@@ -471,7 +471,7 @@ RpiFirmwareGetModelRevision (
OUT UINT32 *Revision
)
{
- RPI_FW_GET_MODEL_REVISION_CMD *Cmd;
+ RPI_FW_GET_REVISION_CMD *Cmd;
EFI_STATUS Status;
UINT32 Result;

@@ -506,6 +506,156 @@ RpiFirmwareGetModelRevision (
return EFI_SUCCESS;
}

+STATIC
+EFI_STATUS
+EFIAPI
+RpiFirmwareGetFirmwareRevision (
+ OUT UINT32 *Revision
+ )
+{
+ RPI_FW_GET_REVISION_CMD *Cmd;
+ EFI_STATUS Status;
+ UINT32 Result;
+
+ if (!AcquireSpinLockOrFail (&mMailboxLock)) {
+ DEBUG ((DEBUG_ERROR, "%a: failed to acquire spinlock\n", __FUNCTION__));
+ return EFI_DEVICE_ERROR;
+ }
+
+ Cmd = mDmaBuffer;
+ ZeroMem (Cmd, sizeof (*Cmd));
+
+ Cmd->BufferHead.BufferSize = sizeof (*Cmd);
+ Cmd->BufferHead.Response = 0;
+ Cmd->TagHead.TagId = RPI_MBOX_GET_REVISION;
+ Cmd->TagHead.TagSize = sizeof (Cmd->TagBody);
+ Cmd->TagHead.TagValueSize = 0;
+ Cmd->EndTag = 0;
+
+ Status = MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_CHANNEL, &Result);
+
+ ReleaseSpinLock (&mMailboxLock);
+
+ if (EFI_ERROR (Status) ||
+ Cmd->BufferHead.Response != RPI_MBOX_RESP_SUCCESS) {
+ DEBUG ((DEBUG_ERROR,
+ "%a: mailbox transaction error: Status == %r, Response == 0x%x\n",
+ __FUNCTION__, Status, Cmd->BufferHead.Response));
+ return EFI_DEVICE_ERROR;
+ }
+
+ *Revision = Cmd->TagBody.Revision;
+ return EFI_SUCCESS;
+}
+
+STATIC
+CHAR8*
+EFIAPI
+RpiFirmwareGetModelName (
+ IN INTN ModelId
+ )
+{
+ UINT32 Revision;
+
+ // If a negative ModelId is passed, detect it.
+ if ((ModelId < 0) && (RpiFirmwareGetModelRevision (&Revision) == EFI_SUCCESS)) {
+ ModelId = (Revision >> 4) & 0xFF;
+ }
+
+ switch (ModelId) {
+ // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
+ case 0x00:
+ return "Raspberry Pi Model A";
+ case 0x01:
+ return "Raspberry Pi Model B";
+ case 0x02:
+ return "Raspberry Pi Model A+";
+ case 0x03:
+ return "Raspberry Pi Model B+";
+ case 0x04:
+ return "Raspberry Pi 2 Model B";
+ case 0x06:
+ return "Raspberry Pi Compute Module 1";
+ case 0x08:
+ return "Raspberry Pi 3 Model B";
+ case 0x09:
+ return "Raspberry Pi Zero";
+ case 0x0A:
+ return "Raspberry Pi Compute Module 3";
+ case 0x0C:
+ return "Raspberry Pi Zero W";
+ case 0x0D:
+ return "Raspberry Pi 3 Model B+";
+ case 0x0E:
+ return "Raspberry Pi 3 Model A+";
+ case 0x11:
+ return "Raspberry Pi 4 Model B";
+ default:
+ return "Unknown Raspberry Pi Model";
+ }
+}
+
+STATIC
+CHAR8*
+EFIAPI
+RpiFirmwareGetManufacturerName (
+ IN INTN ManufacturerId
+ )
+{
+ UINT32 Revision;
+
+ // If a negative ModelId is passed, detect it.
+ if ((ManufacturerId < 0) && (RpiFirmwareGetModelRevision (&Revision) == EFI_SUCCESS)) {
+ ManufacturerId = (Revision >> 16) & 0x0F;
+ }
+
+ switch (ManufacturerId) {
+ // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
+ case 0x00:
+ return "Sony UK";
+ case 0x01:
+ return "Egoman";
+ case 0x02:
+ case 0x04:
+ return "Embest";
+ case 0x03:
+ return "Sony Japan";
+ case 0x05:
+ return "Stadium";
+ default:
+ return "Unknown Manufacturer";
+ }
+}
+
+STATIC
+CHAR8*
+EFIAPI
+RpiFirmwareGetCpuName (
+ IN INTN CpuId
+ )
+{
+ UINT32 Revision;
+
+ // If a negative CpuId is passed, detect it.
+ if ((CpuId < 0) && (RpiFirmwareGetModelRevision (&Revision) == EFI_SUCCESS)) {
+ CpuId = (Revision >> 12) & 0x0F;
+ }
+
+ switch (CpuId) {
+ // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
+ case 0x00:
+ return "BCM2835 (ARM11)";
+ case 0x01:
+ return "BCM2836 (ARM Cortex-A7)";
+ case 0x02:
+ return "BCM2837 (ARM Cortex-A53)";
+ case 0x03:
+ return "BCM2711 (ARM Cortex-A72)";
+ default:
+ return "Unknown CPU Model";
+ }
+}
+
#pragma pack()
typedef struct {
UINT32 Width;
@@ -1009,6 +1159,10 @@ STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL mRpiFirmwareProtocol = {
RpiFirmwareGetSerial,
RpiFirmwareGetModel,
RpiFirmwareGetModelRevision,
+ RpiFirmwareGetModelName,
+ RpiFirmwareGetFirmwareRevision,
+ RpiFirmwareGetManufacturerName,
+ RpiFirmwareGetCpuName,
RpiFirmwareGetArmMemory
};

diff --git a/Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h b/Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h
index ec70f28efe1a..e49d6e6132d9 100644
--- a/Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h
+++ b/Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h
@@ -96,6 +96,30 @@ EFI_STATUS
UINT32 *Revision
);

+typedef
+CHAR8*
+(EFIAPI *GET_MODEL_NAME) (
+ INTN ModelId
+ );
+
+typedef
+EFI_STATUS
+(EFIAPI *GET_FIRMWARE_REVISION) (
+ UINT32 *Revision
+ );
+
+typedef
+CHAR8*
+(EFIAPI *GET_MANUFACTURER_NAME) (
+ INTN ManufacturerId
+ );
+
+typedef
+CHAR8*
+(EFIAPI *GET_CPU_NAME) (
+ INTN CpuId
+ );
+
typedef
EFI_STATUS
(EFIAPI *GET_ARM_MEM) (
@@ -104,21 +128,25 @@ EFI_STATUS
);

typedef struct {
- SET_POWER_STATE SetPowerState;
- GET_MAC_ADDRESS GetMacAddress;
- GET_COMMAND_LINE GetCommandLine;
- GET_CLOCK_RATE GetClockRate;
- GET_CLOCK_RATE GetMaxClockRate;
- GET_CLOCK_RATE GetMinClockRate;
- SET_CLOCK_RATE SetClockRate;
- GET_FB GetFB;
- FREE_FB FreeFB;
- GET_FB_SIZE GetFBSize;
- SET_LED SetLed;
- GET_SERIAL GetSerial;
- GET_MODEL GetModel;
- GET_MODEL_REVISION GetModelRevision;
- GET_ARM_MEM GetArmMem;
+ SET_POWER_STATE SetPowerState;
+ GET_MAC_ADDRESS GetMacAddress;
+ GET_COMMAND_LINE GetCommandLine;
+ GET_CLOCK_RATE GetClockRate;
+ GET_CLOCK_RATE GetMaxClockRate;
+ GET_CLOCK_RATE GetMinClockRate;
+ SET_CLOCK_RATE SetClockRate;
+ GET_FB GetFB;
+ FREE_FB FreeFB;
+ GET_FB_SIZE GetFBSize;
+ SET_LED SetLed;
+ GET_SERIAL GetSerial;
+ GET_MODEL GetModel;
+ GET_MODEL_REVISION GetModelRevision;
+ GET_MODEL_NAME GetModelName;
+ GET_FIRMWARE_REVISION GetFirmwareRevision;
+ GET_MANUFACTURER_NAME GetManufacturerName;
+ GET_CPU_NAME GetCpuName;
+ GET_ARM_MEM GetArmMem;
} RASPBERRY_PI_FIRMWARE_PROTOCOL;

extern EFI_GUID gRaspberryPiFirmwareProtocolGuid;
--
2.21.0.windows.1

[edk2-platforms][PATCH v3 2/5] Platform/RPi3/RpiFirmwareDxe: Improve serial number population

Pete Batard
 

Improve RpiFirmwareGetSerial() to derive a serial number from the
MAC address, in case the platform returns 0 for the serial number.

Signed-off-by: Pete Batard <@pbatard>
---
Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
index 25d7fa3974c0..5a9d4c3f1787 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
@@ -393,7 +393,14 @@ RpiFirmwareGetSerial (
}

*Serial = Cmd->TagBody.Serial;
- return EFI_SUCCESS;
+ // Some platforms return 0 for serial. For those, try to use the MAC address.
+ if (*Serial == 0) {
+ Status = RpiFirmwareGetMacAddress ((UINT8*) Serial);
+ // Convert to a more user-friendly value
+ *Serial = SwapBytes64 (*Serial << 16);
+ }
+
+ return Status;
}

#pragma pack()
--
2.21.0.windows.1

[edk2-platforms][PATCH v3 3/5] Platform/RPi3/PlatformSmbiosDxe: Improve population of SMBIOS entries

Pete Batard
 

This patch cleans up the population SMBIOS entries by removing elements
that we don't have data for, as well as properly filling the ones for
which we do, through the newly added queries from RpiFirmwareDxe.

Signed-off-by: Pete Batard <@pbatard>
---
Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c | 135 ++++++++++----------
Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf | 2 +
Platform/RaspberryPi/RPi3/RPi3.dsc | 4 +-
3 files changed, 69 insertions(+), 72 deletions(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
index bc35175279f2..9150dcfd8c5b 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
@@ -35,6 +35,7 @@
#include <Library/UefiDriverEntryPoint.h>
#include <Library/UefiLib.h>
#include <Library/BaseLib.h>
+#include <Library/PcdLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/UefiBootServicesTableLib.h>
@@ -104,16 +105,19 @@ SMBIOS_TABLE_TYPE0 mBIOSInfoType0 = {
// VirtualMachineSupported :1;
// ExtensionByte2Reserved :3;
},
- 0xFF, // SystemBiosMajorRelease
- 0xFF, // SystemBiosMinorRelease
- 0xFF, // EmbeddedControllerFirmwareMajorRelease
- 0xFF, // EmbeddedControllerFirmwareMinorRelease
+ 0, // SystemBiosMajorRelease
+ 0, // SystemBiosMinorRelease
+ 0, // EmbeddedControllerFirmwareMajorRelease
+ 0, // EmbeddedControllerFirmwareMinorRelease
};

+CHAR8 mBiosVendor[128] = "EDK2";
+CHAR8 mBiosVersion[128] = "EDK2-DEV";
+
CHAR8 *mBIOSInfoType0Strings[] = {
- "https://github.com/andreiw/RaspberryPiPkg", // Vendor String
- "Raspberry Pi 64-bit UEFI (" __DATE__ " " __TIME__ ")", // BiosVersion String
- __DATE__,
+ mBiosVendor, // Vendor
+ mBiosVersion, // Version
+ __DATE__ " " __TIME__, // Release Date
NULL
};

@@ -132,42 +136,19 @@ SMBIOS_TABLE_TYPE1 mSysInfoType1 = {
6, // Family String
};

-#define PROD_BASE 8
-#define PROD_KNOWN 13
-#define PROD_UNKNOWN 11
-CHAR8 *ProductNames[] = {
- /* 8 */ "3",
- /* 9 */ "Zero",
- /* 10 */ "CM3",
- /* 11 */ "???",
- /* 12 */ "Zero W",
- /* 13 */ "3B+"
-};
-
-#define MANU_UNKNOWN 0
-#define MANU_KNOWN 4
-#define MANU_BASE 1
-CHAR8 *ManufNames[] = {
- "???",
- /* 0 */ "Sony",
- /* 1 */ "Egoman",
- /* 2 */ "Embest",
- /* 3 */ "Sony Japan",
- /* 4 */ "Embest"
-};
-
-CHAR8 mSysInfoManufName[sizeof ("Sony Japan")];
-CHAR8 mSysInfoProductName[sizeof ("64-bit Raspberry Pi XXXXXX (rev. xxxxxxxx)")];
+CHAR8 mSysInfoManufName[128];
+CHAR8 mSysInfoProductName[128];
+CHAR8 mSysInfoVersionName[128];
CHAR8 mSysInfoSerial[sizeof (UINT64) * 2 + 1];
CHAR8 mSysInfoSKU[sizeof (UINT64) * 2 + 1];

CHAR8 *mSysInfoType1Strings[] = {
mSysInfoManufName,
mSysInfoProductName,
- mSysInfoProductName,
+ mSysInfoVersionName,
mSysInfoSerial,
mSysInfoSKU,
- "edk2",
+ "Raspberry Pi",
NULL
};

@@ -180,7 +161,7 @@ SMBIOS_TABLE_TYPE2 mBoardInfoType2 = {
2, // ProductName String
3, // Version String
4, // SerialNumber String
- 5, // AssetTag String
+ 0, // AssetTag String
{ // FeatureFlag
1, // Motherboard :1;
0, // RequiresDaughterCard :1;
@@ -189,7 +170,7 @@ SMBIOS_TABLE_TYPE2 mBoardInfoType2 = {
0, // HotSwappable :1;
0, // Reserved :3;
},
- 6, // LocationInChassis String
+ 0, // LocationInChassis String
0, // ChassisHandle;
BaseBoardTypeMotherBoard, // BoardType;
0, // NumberOfContainedObjectHandles;
@@ -198,10 +179,8 @@ SMBIOS_TABLE_TYPE2 mBoardInfoType2 = {
CHAR8 *mBoardInfoType2Strings[] = {
mSysInfoManufName,
mSysInfoProductName,
- mSysInfoProductName,
+ mSysInfoVersionName,
mSysInfoSerial,
- "None",
- mSysInfoSKU,
NULL
};

@@ -214,7 +193,7 @@ SMBIOS_TABLE_TYPE3 mEnclosureInfoType3 = {
MiscChassisEmbeddedPc, // Type;
2, // Version String
3, // SerialNumber String
- 4, // AssetTag String
+ 0, // AssetTag String
ChassisStateSafe, // BootupState;
ChassisStateSafe, // PowerSupplyState;
ChassisStateSafe, // ThermalState;
@@ -230,7 +209,6 @@ CHAR8 *mEnclosureInfoType3Strings[] = {
mSysInfoManufName,
mSysInfoProductName,
mSysInfoSerial,
- "None",
NULL
};

@@ -306,9 +284,9 @@ SMBIOS_TABLE_TYPE4 mProcessorInfoType4 = {
0, // L1CacheHandle;
0, // L2CacheHandle;
0, // L3CacheHandle;
- 4, // SerialNumber;
- 5, // AssetTag;
- 6, // PartNumber;
+ 0, // SerialNumber;
+ 0, // AssetTag;
+ 0, // PartNumber;
4, // CoreCount;
4, // EnabledCoreCount;
4, // ThreadCount;
@@ -316,13 +294,12 @@ SMBIOS_TABLE_TYPE4 mProcessorInfoType4 = {
ProcessorFamilyARM, // ARM Processor Family;
};

+CHAR8 mCpuName[128] = "Unknown ARM CPU";
+
CHAR8 *mProcessorInfoType4Strings[] = {
"Socket",
- "ARM",
- "BCM2837 ARMv8",
- "1.0",
- "1.0",
- "1.0",
+ "Broadcom",
+ mCpuName,
NULL
};

@@ -618,6 +595,29 @@ BIOSInfoUpdateSmbiosType0 (
VOID
)
{
+ UINT32 FirmwareRevision = 0;
+ EFI_STATUS Status = EFI_SUCCESS;
+
+ // Populate the Firmware major and minor.
+ Status = mFwProtocol->GetFirmwareRevision (&FirmwareRevision);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed to get firmware revision: %r\n", Status));
+ } else {
+ // This expects Broadcom / The Raspberry Pi Foundation to switch to
+ // less nonsensical VideoCore firmware revisions in the future...
+ mBIOSInfoType0.EmbeddedControllerFirmwareMajorRelease =
+ (UINT8)((FirmwareRevision >> 16) & 0xFF);
+ mBIOSInfoType0.EmbeddedControllerFirmwareMinorRelease =
+ (UINT8)(FirmwareRevision & 0xFF);
+ }
+
+ // mBiosVendor and mBiosVersion, which are referenced in mBIOSInfoType0Strings,
+ // are left unchanged if the following calls fail.
+ UnicodeStrToAsciiStrS ((CHAR16*)PcdGetPtr (PcdFirmwareVendor),
+ mBiosVendor, sizeof (mBiosVendor));
+ UnicodeStrToAsciiStrS ((CHAR16*)PcdGetPtr (PcdFirmwareVersionString),
+ mBiosVersion, sizeof (mBiosVersion));
+
LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mBIOSInfoType0, mBIOSInfoType0Strings, NULL);
}

@@ -664,34 +664,25 @@ SysInfoUpdateSmbiosType1 (
UINT32 BoardRevision = 0;
EFI_STATUS Status = EFI_SUCCESS;
UINT64 BoardSerial = 0;
- UINTN Prod = PROD_UNKNOWN;
- UINTN Manu = MANU_UNKNOWN;
+ INTN Prod = -1;
+ INTN Manu = -1;

Status = mFwProtocol->GetModelRevision (&BoardRevision);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "Failed to get board model: %r\n", Status));
} else {
Prod = (BoardRevision >> 4) & 0xFF;
+ Manu = (BoardRevision >> 16) & 0x0F;
}

- if (Prod > PROD_KNOWN) {
- Prod = PROD_UNKNOWN;
- }
- Prod -= PROD_BASE;
- AsciiSPrint (mSysInfoProductName, sizeof (mSysInfoProductName),
- "64-bit Raspberry Pi %a (rev. %x)", ProductNames[Prod], BoardRevision);
-
- Manu = (BoardRevision >> 16) & 0xF;
- if (Manu > MANU_KNOWN) {
- Manu = MANU_UNKNOWN;
- } else {
- Manu += MANU_BASE;
- }
- AsciiSPrint (mSysInfoManufName, sizeof (mSysInfoManufName), "%a", ManufNames[Manu]);
+ AsciiStrCpyS (mSysInfoProductName, sizeof (mSysInfoProductName),
+ mFwProtocol->GetModelName (Prod));
+ AsciiStrCpyS (mSysInfoManufName, sizeof (mSysInfoManufName),
+ mFwProtocol->GetManufacturerName (Manu));
+ AsciiSPrint (mSysInfoVersionName, sizeof (mSysInfoVersionName),
+ "%X", BoardRevision);

- I64ToHexString (mSysInfoSKU,
- sizeof (mSysInfoSKU),
- BoardRevision);
+ I64ToHexString (mSysInfoSKU, sizeof (mSysInfoSKU), BoardRevision);

Status = mFwProtocol->GetSerial (&BoardSerial);
if (EFI_ERROR (Status)) {
@@ -702,9 +693,11 @@ SysInfoUpdateSmbiosType1 (

DEBUG ((DEBUG_ERROR, "Board Serial Number: %a\n", mSysInfoSerial));

- mSysInfoType1.Uuid.Data1 = *(UINT32*)"RPi3";
+ mSysInfoType1.Uuid.Data1 = BoardRevision;
mSysInfoType1.Uuid.Data2 = 0x0;
mSysInfoType1.Uuid.Data3 = 0x0;
+ // Swap endianness, so that the serial is more user-friendly as a UUID
+ BoardSerial = SwapBytes64 (BoardSerial);
CopyMem (mSysInfoType1.Uuid.Data4, &BoardSerial, sizeof (BoardSerial));

LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mSysInfoType1, mSysInfoType1Strings, NULL);
@@ -763,6 +756,8 @@ ProcessorInfoUpdateSmbiosType4 (
DEBUG ((DEBUG_INFO, "Current CPU speed: %uHz\n", Rate));
}

+ AsciiStrCpyS (mCpuName, sizeof (mCpuName), mFwProtocol->GetCpuName (-1));
+
LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mProcessorInfoType4, mProcessorInfoType4Strings, NULL);
}

diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
index f7c74f7f5456..485450625b54 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
+++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
@@ -48,3 +48,5 @@ [Depex]

[Pcd]
gArmTokenSpaceGuid.PcdSystemMemorySize
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
index b37a02e97da7..bc424a06bb45 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dsc
+++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
@@ -309,7 +309,7 @@ [PcdsFixedAtBuild.common]

gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0xc0000000

- gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"edk2-1.0"
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"EDK2-DEV"

!if $(SECURE_BOOT_ENABLE) == TRUE
# override the default values from SecurityPkg to ensure images from all sources are verified in secure boot
@@ -383,7 +383,7 @@ [PcdsFixedAtBuild.common]
gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa, 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23, 0x31 }

- gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"Raspberry Pi 3 64-bit UEFI"
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"
gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE

[PcdsDynamicHii.common.DEFAULT]
--
2.21.0.windows.1

[edk2-platforms][PATCH v3 4/5] Platform/RPi3/PlatformSmbiosDxe: Populate BIOS major/minor from PCD

Pete Batard
 

String parsing code is added to BIOSInfoUpdateSmbiosType0() so that
any numeric "x.y" value being passed in PcdFirmwareVersionString is
now used to populate the BIOS major and minor.

Signed-off-by: Pete Batard <@pbatard>
---
Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c | 45 ++++++++++++++++++++
1 file changed, 45 insertions(+)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
index 9150dcfd8c5b..b5dcff897a59 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
@@ -41,6 +41,8 @@
#include <Library/UefiBootServicesTableLib.h>
#include <Library/PrintLib.h>

+#define SMB_IS_DIGIT(c) (((c) >= '0') && ((c) <= '9'))
+
STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;

/***********************************************************************
@@ -597,6 +599,9 @@ BIOSInfoUpdateSmbiosType0 (
{
UINT32 FirmwareRevision = 0;
EFI_STATUS Status = EFI_SUCCESS;
+ INTN i;
+ INTN State = 0;
+ INTN Value[2];

// Populate the Firmware major and minor.
Status = mFwProtocol->GetFirmwareRevision (&FirmwareRevision);
@@ -618,6 +623,46 @@ BIOSInfoUpdateSmbiosType0 (
UnicodeStrToAsciiStrS ((CHAR16*)PcdGetPtr (PcdFirmwareVersionString),
mBiosVersion, sizeof (mBiosVersion));

+ // Look for a "x.y" numeric string anywhere in mBiosVersion and
+ // try to parse it to populate the BIOS major and minor.
+ for (i = 0; (i < AsciiStrLen (mBiosVersion)) && (State < 4); i++) {
+ switch (State) {
+ case 0:
+ if (!SMB_IS_DIGIT (mBiosVersion[i]))
+ break;
+ Value[0] = Value[1] = 0;
+ State++;
+ // Fall through
+ case 1:
+ case 3:
+ if (SMB_IS_DIGIT (mBiosVersion[i])) {
+ Value[State / 2] = (Value[State / 2] * 10) + (mBiosVersion[i] - '0');
+ if (Value[State / 2] > 255) {
+ while (SMB_IS_DIGIT (mBiosVersion[i + 1]))
+ i++;
+ // Reset our state (we may have something like "Firmware X83737.1 v1.23")
+ State = 0;
+ }
+ } else {
+ State++;
+ }
+ if (State != 2)
+ break;
+ // Fall through
+ case 2:
+ if ((mBiosVersion[i] == '.') && (SMB_IS_DIGIT (mBiosVersion[i + 1]))) {
+ State++;
+ } else {
+ State = 0;
+ }
+ break;
+ }
+ }
+ if ((State == 3) || (State == 4)) {
+ mBIOSInfoType0.SystemBiosMajorRelease = (UINT8)Value[0];
+ mBIOSInfoType0.SystemBiosMinorRelease = (UINT8)Value[1];
+ }
+
LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mBIOSInfoType0, mBIOSInfoType0Strings, NULL);
}

--
2.21.0.windows.1

[edk2-platforms][PATCH v3 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision

Pete Batard
 

The board revision is the proper channel to use to detect the amount of
RAM available as bits [20-22] report the effective RAM size for the board
starting with 256 MB (000b) and doubling in size for each value.

Signed-off-by: Pete Batard <@pbatard>
Reviewed-by: Leif Lindholm <leif.lindholm@...>
---
Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
index b5dcff897a59..5abc82b8d363 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
@@ -866,16 +866,22 @@ MemArrMapInfoUpdateSmbiosType19 (
)
{
EFI_STATUS Status;
- UINT32 Base;
- UINT32 Size;
+ UINT32 BoardRevision = 0;

- Status = mFwProtocol->GetArmMem (&Base, &Size);
+ // Note: Type 19 addresses are expressed in KB, not bytes
+ mMemArrMapInfoType19.StartingAddress = 0;
+ // The minimum RAM size used on any Raspberry Pi model is 256 MB
+ mMemArrMapInfoType19.EndingAddress = 256 * 1024;
+ Status = mFwProtocol->GetModelRevision (&BoardRevision);
if (Status != EFI_SUCCESS) {
- DEBUG ((DEBUG_ERROR, "Couldn't get the ARM memory size: %r\n", Status));
+ DEBUG ((DEBUG_WARNING, "Couldn't get the board memory size - defaulting to 256 MB: %r\n", Status));
} else {
- mMemArrMapInfoType19.StartingAddress = Base / 1024;
- mMemArrMapInfoType19.EndingAddress = (Base + Size - 1) / 1024;
+ // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
+ // Bits [20-22] indicate the amount of memory starting with 256MB (000b)
+ // and doubling in size for each value (001b = 512 MB, 010b = 1GB, etc.)
+ mMemArrMapInfoType19.EndingAddress <<= (BoardRevision >> 20) & 0x07;
}
+ mMemArrMapInfoType19.EndingAddress -= 1;

LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mMemArrMapInfoType19, mMemArrMapInfoType19Strings, NULL);
}
--
2.21.0.windows.1

Re: [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

David Woodhouse
 

On Fri, 2019-10-11 at 12:55 +0200, Laszlo Ersek wrote:
On 10/11/19 04:24, Wu, Jiaxin wrote:
Hi Laszlo & David,

I think I have *repeated* several times that we are targeting to fix the HostName validation issue, not the IP or email address. *But* even so, the series patches for UEFI TLS is also allowable to specify IP as host name for CN or dNSName of SAN in the certificate. That's why I said "if the CN or SAN in the certificate are set correctly, it should be OK to pass the verification". The failure you mentioned here is to set the IP in iPAddress of SAN, I agree it's the routine and suggested setting, *but* obviously, it's not the target we are supported according the implementation/description of TlsSetVerifyHost. We are targeting to the hostname verification, and meanwhile compatible with the IP in the URI (But need the *correct* certificate setting).

IP addresses stored in the DNS names and CN are of cause ignored by X509_check_ip & X509_check_ip_asc().

Post my explain again:
UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe),
not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2:
"...TLS session hostname for validation which is used to verify whether the name
within the peer certificate matches a given host name..."
In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real
FQDN (www.xxx.com) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])),
and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST.
That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature
by following the standard TLS HostName verification capability.
Now, let's talking the iPAddress setting in SAN (same as email address), if you do need such feature that verify the IP in X509_check_ip & X509_check_ip_asc , please report new Bugzilla (need TLS Spec update the expose the setting interface), don't mix up the HTTPS hostname verification here.
(To clarify my stance.)

Given the above statement of scope, and given the test env details that
I included in my previous message:

series
Tested-by: Laszlo Ersek <lersek@...>

I understand that my testing does not cover the use case described by David.

So my Tested-by is certainly *not* intended as the "last word" in this
thread, or anything like that. I'm only saying this patch set is good
enough for me, not that everyone should find it good enough for them.

As you wish.

Note for the record that that this is a functional regression. We go
from accepting all certs regardless of the target/subject, to rejecting
some valid certificates. I first started looking at this when it was
reported as such, on the list.

Given the length of time it's taken to fix this CVE already, I
understand that Laszlo is keen to get *something* committed, at last.

But I stand by the assertion that this could be done properly, without
the regression, with much less additional typing even than we've
already done in this thread the last couple of days. It just isn't that
hard.

Ultimately, though, do whatever you like. I am not the final arbiter of
engineering sanity and common sense for the EDK2 project.

If I were, the world would be a very different place.

Re: OVMF is crashing for me in master

Laszlo Ersek
 

Hi,

On 10/11/19 03:19, Liming Gao wrote:
Andrew:
I verify the change (2de1f611be06ded3a59726a4052a9039be7d459b
MdeModulePkg/BdsDxe: Also call PlatformBootManagerWaitCallback on 0)
in Emulator.
It works, because PCD value is set to 10 in Emulator.

Before this change, if TimeOut PCD is zero, BdsEntry doesn't call
PlatformBootManagerWaitCallback().
After this change, if TimeOut PCD is zero, BdsEntry still call
PlatformBootManagerWaitCallback(). So, it trigs this issue. I agree
your fix.

Pete:
Will you contribute the patch to fix this hang issue in OVMF?
So, when Pete submitted the BDS patch, I didn't test it, I only looked
at the patch email. See my feedback here:

http://mid.mail-archive.com/8bacbb53-98df-724e-b704-b5059d11e378@...
https://edk2.groups.io/g/devel/message/48133

It seemed OK to me. In particular, I reasoned about (TimeoutRemain==0),
and the patch seemed to do the right thing for that.

Unfortunately, what I missed was the following case:

- if PcdPlatformBootTimeOut is zero, then the patch by Pete does not
simply make one more call to PlatformBootManagerWaitCallback(), with
argument zero, but it makes the *ONLY* call to
PlatformBootManagerWaitCallback().

I missed this because (like normal) the patch was sent with a 3-line
context, and I didn't think it necessary to review the full function
context.

Here is the commit, with full function context:

diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index f3d5e5ac0615..7968a58f3454 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -310,47 +310,48 @@ VOID
BdsWait (
IN EFI_EVENT HotkeyTriggered
)
{
EFI_STATUS Status;
UINT16 TimeoutRemain;

DEBUG ((EFI_D_INFO, "[Bds]BdsWait ...Zzzzzzzzzzzz...\n"));

TimeoutRemain = PcdGet16 (PcdPlatformBootTimeOut);
while (TimeoutRemain != 0) {
DEBUG ((EFI_D_INFO, "[Bds]BdsWait(%d)..Zzzz...\n", (UINTN) TimeoutRemain));
PlatformBootManagerWaitCallback (TimeoutRemain);

BdsReadKeys (); // BUGBUG: Only reading can signal HotkeyTriggered
// Can be removed after all keyboard drivers invoke callback in timer callback.

if (HotkeyTriggered != NULL) {
Status = BdsWaitForSingleEvent (HotkeyTriggered, EFI_TIMER_PERIOD_SECONDS (1));
if (!EFI_ERROR (Status)) {
break;
}
} else {
gBS->Stall (1000000);
}

//
// 0xffff means waiting forever
// BDS with no hotkey provided and 0xffff as timeout will "hang" in the loop
//
if (TimeoutRemain != 0xffff) {
TimeoutRemain--;
}
}
+ PlatformBootManagerWaitCallback (0);
DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
}
We can see that the function used to handle the
PcdPlatformBootTimeOut==0 situation well, and make *no* call to
PlatformBootManagerWaitCallback(). The patch broke that behavior.

Therefore, I disagree with the idea to fix this in OvmfPkg. Any number
of platforms may exist that rely on PlatformBootManagerWaitCallback()
not being called when PcdPlatformBootTimeOut is zero.

(To name just one other example, ArmVirtPkg too is affected.)

And for OVMF and ArmVirtPkg, it is definitely correct to *not* display
any progress bar if PcdPlatformBootTimeOut is 0.

Please consult the definition of the PCD in "MdePkg/MdePkg.dec":

## The number of seconds that the firmware will wait before initiating the original default boot selection.
# A value of 0 indicates that the default boot selection is to be initiated immediately on boot.
# The value of 0xFFFF then firmware will wait for user input before booting.
# @Prompt Boot Timeout (s)
gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0xffff|UINT16|0x0000002c
Also see the specification of PlatformBootManagerWaitCallback(), in
"MdeModulePkg/Include/Library/PlatformBootManagerLib.h":

/**
This function is called each second during the boot manager waits the timeout.

@param TimeoutRemain The remaining timeout.
**/
VOID
EFIAPI
PlatformBootManagerWaitCallback (
UINT16 TimeoutRemain
);
According to the DEC file, there is no waiting if the PCD is zero.
Therefore, if the PCD is zero, the function should not be called at all.


There is another complication, namely, if the PCD is 0xFFFF. In this
case, the loop goes on until a hotkey is pressed. In every iteration
(that is, every second), PlatformBootManagerWaitCallback() is called
with argument 0xFFFF, invariably. Clearly, this *cannot* cause a
platform to display any progress *changes*. In the most common callback
implementation, namely in the formula

(Timeout - TimeoutRemain) * 100 / Timeout,

it will mean

(0xFFFF - 0xFFFF) * 100 / 0xFFFF,

that is, "zero percent", in every iteration.

Based on that, it looks wrong to suddenly call
PlatformBootManagerWaitCallback() with a zero argument ("hundred percent
completion"), after it's been called, let's say for a minute or more,
with a constant 0xFFFF argument ("zero percent completion"), until the
user finally presses a hotkey.

Jumping from zero to 100% is discontiguous and doesn't appear helpful to
me.


Further yet, what if there is an actual progress bar and timeout (like
10 seconds), and the user presses a hotkey after 5 seconds? Should the
progress bar jump to 100% at once? I wouldn't think so.


Therefore, I claim that the right fix is:

diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index 7968a58f3454..d12829afeb8b 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -341,7 +341,16 @@ BdsWait (
TimeoutRemain--;
}
}
- PlatformBootManagerWaitCallback (0);
+ //
+ // If the platform configured a nonzero and finite time-out, and we have
+ // actually reached that, report 100% completion to the platform.
+ //
+ // Note that the (TimeoutRemain == 0) condition excludes
+ // PcdPlatformBootTimeOut=0xFFFF, and that's deliberate.
+ //
+ if (PcdGet16 (PcdPlatformBootTimeOut) != 0 && TimeoutRemain == 0) {
+ PlatformBootManagerWaitCallback (0);
+ }
DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
}
Thanks!
Laszlo