Date   

[PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count

Krzysztof Koch
 

1. Check if the 'Number of System Localities' provided can be
represented in the SLIT table. The table 'Length' field is a 32-bit
value while the 'Number of System Localities' field is 64-bit long.

2. Check if the SLIT matrix fits in the table buffer. If N is the SLIT
locality count, then the matrix used to represent the localities is
N*N bytes long. The ACPI table length must be big enough to fit the
matrix.

3. Remove (now) redundant 64x64 bit multiplication.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
v1:
- Validate the 'Number of System Localities' Field [Krzysztof]
- Remove redundant 64x64 bit multiplication [Krzysztof]

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c | 47 +++++++++++++++++---
1 file changed, 42 insertions(+), 5 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
index 17e2166a09d8615b714e0c51d4d93d293fcdf601..e4625ee8b13907893a9b6990ecb956baf91cc3b9 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
@@ -30,7 +30,7 @@ STATIC CONST ACPI_PARSER SlitParser[] = {
/**
Macro to get the value of a System Locality
**/
-#define SLIT_ELEMENT(Ptr, i, j) *(Ptr + (MultU64x64 (i, LocalityCount)) + j)
+#define SLIT_ELEMENT(Ptr, i, j) *(Ptr + (i * LocalityCount) + j)

/**
This function parses the ACPI SLIT table.
@@ -57,9 +57,9 @@ ParseAcpiSlit (
)
{
UINT32 Offset;
- UINT64 Count;
- UINT64 Index;
- UINT64 LocalityCount;
+ UINT32 Count;
+ UINT32 Index;
+ UINT32 LocalityCount;
UINT8* LocalityPtr;
CHAR16 Buffer[80]; // Used for AsciiName param of ParseAcpi

@@ -87,8 +87,45 @@ ParseAcpiSlit (
return;
}

+ /*
+ Despite the 'Number of System Localities' being a 64-bit field in SLIT,
+ the maximum number of localities that can be represented in SLIT is limited
+ by the 'Length' field of the ACPI table.
+
+ Since the ACPI table length field is 32-bit wide. The maximum number of
+ localities that can be represented in SLIT can be calculated as:
+
+ MaxLocality = sqrt (MAX_UINT32 - sizeof (EFI_ACPI_6_3_SYSTEM_LOCALITY_DISTANCE_INFORMATION_TABLE_HEADER))
+ = 65535
+ = MAX_UINT16
+ */
+ if (*SlitSystemLocalityCount > MAX_UINT16) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: The Number of System Localities provided can't be represented " \
+ L"in the SLIT table. SlitSystemLocalityCount = %ld. " \
+ L"MaxLocalityCountAllowed = %d.\n",
+ *SlitSystemLocalityCount,
+ MAX_UINT16
+ );
+ return;
+ }
+
+ LocalityCount = (UINT32)*SlitSystemLocalityCount;
+
+ // Make sure system localities fit in the table buffer provided
+ if (Offset + (LocalityCount * LocalityCount) > AcpiTableLength) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Invalid Number of System Localities. " \
+ L"SlitSystemLocalityCount = %ld. AcpiTableLength = %d.\n",
+ *SlitSystemLocalityCount,
+ AcpiTableLength
+ );
+ return;
+ }
+
LocalityPtr = Ptr + Offset;
- LocalityCount = *SlitSystemLocalityCount;

// We only print the Localities if the count is less than 16
// If the locality count is more than 16 then refer to the
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


Re: [edk2-platforms: PATCH 1/1] Platforms/RPi3: Add multiple embedded Device Tree selection

Pete Batard
 

Hi Leif,

On 2019.08.15 13:13, Leif Lindholm wrote:
On Mon, Aug 12, 2019 at 02:06:34PM +0100, Pete Batard wrote:
The Raspberry Pi 3 platform currently has 2 different models, each with a
different Device Tree. Rather than embedding a single one, and requiring
users to manually provide the other, this patch ensures that we now embed
both and and serve the relevant one at runtime.

Signed-off-by: Pete Batard <pete@akeo.ie>
Changeset looks very useful. Some style issues. And one question.
First the question: is there no practical size restriction on the
firmware image? This is a 24k increase in image size.
I thought about that too, but we still have plenty of unused payload to go by, which is why I've been adding stuff like the EBC driver, and now this, without worrying too much about the extra space taken.

For one, in the current 2 MB firmware image, we're still barely scratching 1.2 MB of effective data (for RELEASE. For DEBUG that's ~1.5 MB), so we have plenty of space to add additional Device Trees, logos, and similar non critical data. As a matter of fact, I was almost tempted to have this patchset also include the Raspberry Pi 4 Device Tree, so that we would just have all of the DT's we might ever be interested in available at runtime, regardless of whether they are applicable or not (as this would simplify FdtDxe reuse a well GUID handling).

Also, I would expect the build process to complain loudly if we go over capacity.

Finally, I'm pretty sure we could relatively easily switch to a 4 MB or 8 MB firmware image, if we start to run out of space with the current 2 MB, since, from what I am aware of, the SoC is designed to be able to boot large Linux kernels directly and doesn't enforce a hardcoded limit on our FV size.

In other words, even if we start to regularly add a tens of KB here and there, it's going to take some time before we have to start to worry about going over capacity...

---
Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 57 ++++++++++++++++----
Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf | 3 +-
Platform/RaspberryPi/RPi3/RPi3.dec | 3 +-
Platform/RaspberryPi/RPi3/RPi3.fdf | 6 ++-
4 files changed, 55 insertions(+), 14 deletions(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
index c84e5b2767e2..7c0ab75e7cb1 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
@@ -20,6 +20,11 @@
#include <Guid/Fdt.h>
+CONST EFI_GUID *mFdtGuid[] = {
+ &gRaspberryPiFdtGuid1,
+ &gRaspberryPiFdtGuid2,
+ };
+
STATIC VOID *mFdtImage;
STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;
@@ -368,7 +373,9 @@ FdtDxeInitialize (
EFI_STATUS Status;
VOID *FdtImage;
UINTN FdtSize;
+ UINTN FdtIndex;
INT32 Retval;
+ UINT32 BoardRevision;
BOOLEAN Internal;
Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, NULL,
@@ -383,13 +390,41 @@ FdtDxeInitialize (
* Have FDT passed via config.txt.
*/
FdtSize = fdt_totalsize (FdtImage);
This
- DEBUG ((DEBUG_INFO, "DTB passed via config.txt of 0x%lx bytes\n", FdtSize));
+ DEBUG ((DEBUG_INFO, "Device Tree passed via config.txt (0x%lx bytes)\n", FdtSize));
looks unrelated to the changeset.

Status = EFI_SUCCESS;
} else {
+ /*
+ * Use one of the embedded FDT's.
+ */
Internal = TRUE;
This DEBUG changes bit
- DEBUG ((DEBUG_INFO, "No/bad FDT at %p (%a), trying internal DTB...\n",
- FdtImage, fdt_strerror (Retval)));
- Status = GetSectionFromAnyFv (&gRaspberryPiFdtFileGuid, EFI_SECTION_RAW, 0,
+ DEBUG ((DEBUG_INFO, "No/Bad Device Tree found at address 0x%p (%a), "
+ "looking up internal one...\n", FdtImage, fdt_strerror (Retval)));
to here looks completely unrelated to the changeset.
Which garbles the diff somewhat with regards to the functional line in
the middle..
I mentioned in the cover letter that changes were also added to harmonize the debug output to avoid acronyms and provide human readable error codes. But you're right, at the very least, these changes should have been mentioned in the commit message for actual patch itself, and are probably better off on their own.

I'll break them out as a separate patch as requested.

Regards,

/Pete


+ /*
+ * Query the board revision to differentiate between models.
+ */
+ Status = mFwProtocol->GetModelRevision (&BoardRevision);
+ if (EFI_ERROR (Status)) {
+ FdtIndex = 0;
+ DEBUG ((DEBUG_ERROR, "Failed to get board type: %r\n", Status));
+ } else {
+ // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
+ switch ((BoardRevision >> 4) & 0xFF) {
+ case 0x08: // Raspberry 3 Model B
+ DEBUG ((DEBUG_INFO, "Using RPi3 Model B internal Device Tree\n"));
+ FdtIndex = 0;
+ break;
+ case 0x0D: // Raspberry 3 Model B+
+ DEBUG ((DEBUG_INFO, "Using RPi3 Model B+ internal Device Tree\n"));
+ FdtIndex = 1;
+ break;
+ default:
+ DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n"));
+ FdtIndex = 0;
+ break;
+ }
+ }
+ ASSERT (FdtIndex < ARRAY_SIZE (mFdtGuid));
+ Status = GetSectionFromAnyFv (mFdtGuid[FdtIndex], EFI_SECTION_RAW, 0,
&FdtImage, &FdtSize);
if (Status == EFI_SUCCESS) {
if (fdt_check_header (FdtImage) != 0) {
Everything from here...

@@ -419,27 +454,27 @@ FdtDxeInitialize (
Status = SanitizePSCI ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to sanitize PSCI (error %d)\n", Status);
+ Print (L"Failed to sanitize PSCI: %r\n", Status);
}
Status = CleanMemoryNodes ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to clean memory nodes (error %d)\n", Status);
+ Print (L"Failed to clean memory nodes: %r\n", Status);
}
Status = CleanSimpleFramebuffer ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to clean frame buffer (error %d)\n", Status);
+ Print (L"Failed to clean frame buffer: %r\n", Status);
}
Status = FixEthernetAliases ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to fix ethernet aliases (error %d)\n", Status);
+ Print (L"Failed to fix ethernet aliases: %r\n", Status);
}
Status = UpdateMacAddress ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to update MAC address (error %d)\n", Status);
+ Print (L"Failed to update MAC address: %r\n", Status);
}
if (Internal) {
@@ -448,11 +483,11 @@ FdtDxeInitialize (
*/
Status = UpdateBootArgs ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to update boot arguments (error %d)\n", Status);
+ Print (L"Failed to update boot arguments: %r\n", Status);
}
}
- DEBUG ((DEBUG_INFO, "Installed FDT is at %p\n", mFdtImage));
+ DEBUG ((DEBUG_INFO, "Installed Device Tree at address 0x%p\n", mFdtImage));
Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage);
ASSERT_EFI_ERROR (Status);
...to here is unrelated printout message changes.
So, I don't object to the debug printout changes as such, but they
obscure the functional changeset. Can you please break them out as a
separate patch?
Best Regards,
Leif

diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
index 5b0b1a09f374..c994a2b69d78 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
+++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
@@ -35,7 +35,8 @@ [LibraryClasses]
[Guids]
gFdtTableGuid
- gRaspberryPiFdtFileGuid
+ gRaspberryPiFdtGuid1
+ gRaspberryPiFdtGuid2
[Protocols]
gRaspberryPiFirmwareProtocolGuid ## CONSUMES
diff --git a/Platform/RaspberryPi/RPi3/RPi3.dec b/Platform/RaspberryPi/RPi3/RPi3.dec
index 22de439fde8f..d90ea8329818 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dec
+++ b/Platform/RaspberryPi/RPi3/RPi3.dec
@@ -24,7 +24,8 @@ [Protocols]
[Guids]
gRaspberryPiTokenSpaceGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
- gRaspberryPiFdtFileGuid = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}}
+ gRaspberryPiFdtGuid1 = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}}
+ gRaspberryPiFdtGuid2 = {0x3D523012, 0x73FE, 0x40E5, { 0x89, 0x2E, 0x1A, 0x4D, 0xF6, 0x0F, 0x3C, 0x0C}}
gRaspberryPiEventResetGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB4, 0xB4, 0xE4, 0xD4, 0xB4}}
gConfigDxeFormSetGuid = {0xCD7CC258, 0x31DB, 0x22E6, {0x9F, 0x22, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
diff --git a/Platform/RaspberryPi/RPi3/RPi3.fdf b/Platform/RaspberryPi/RPi3/RPi3.fdf
index c62d649834c7..83753d84badc 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.fdf
+++ b/Platform/RaspberryPi/RPi3/RPi3.fdf
@@ -300,11 +300,15 @@ [FV.FvMain]
INF Platform/RaspberryPi/$(PLATFORM_NAME)/Drivers/LogoDxe/LogoDxe.inf
#
- # FDT (GUID matches gRaspberryPiFdtFileGuid in FdtDxe)
+ # Device Tree support
+ # GUIDs match gRaspberryPiFdtGuid# from FdtDxe.
#
FILE FREEFORM = DF5DA223-1D27-47C3-8D1B-9A41B55A18BC {
SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b.dtb
}
+ FILE FREEFORM = 3D523012-73FE-40E5-892E-1A4DF60F3C0C {
+ SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b-plus.dtb
+ }
[FV.FVMAIN_COMPACT]
FvAlignment = 16
--
2.21.0.windows.1


[PATCH v1 02/11] ShellPkg: acpiview: RSDP: Validate global pointer before use

Krzysztof Koch
 

Check if XsdtAddress pointer has been successfully updated before it
is used for further table parsing.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
v1:
- Test against NULL pointers [Krzysztof]

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
index 5a5c4b50c12e6eb0aa0efb1765df7e123f614da3..f4a8732a7db7c437031f2a3d2f266b80eff17b4b 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
@@ -138,6 +138,18 @@ ParseAcpiRsdp (
PARSER_PARAMS (RsdpParser)
);

+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if (XsdtAddress == NULL) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient table length. AcpiTableLength = %d." \
+ L"RSDP parsing aborted.\n",
+ AcpiTableLength
+ );
+ return;
+ }
+
// This code currently supports parsing of XSDT table only
// and does not parse the RSDT table. Platforms provide the
// RSDT to enable compatibility with ACPI 1.0 operating systems.
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


[PATCH v1 06/11] ShellPkg: acpiview: SRAT: Validate global pointers before use

Krzysztof Koch
 

Check if SratRAType and SratRALength pointers have been successfully
updated before they are used for further table parsing.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
v1:
- Test against NULL pointers [Krzysztof]

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
index a8aa420487bb6bf29fc38221d0b221573c64b8b3..e09a7db8f5c92b44c96b6c37a44a39693352b442 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
@@ -219,6 +219,19 @@ ParseAcpiSrat (
PARSER_PARAMS (SratResourceAllocationParser)
);

+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((SratRAType == NULL) ||
+ (SratRALength == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient remaining table buffer length to read the " \
+ L"Static Resource Allocation structure header. Length = %d.\n",
+ AcpiTableLength - Offset
+ );
+ return;
+ }
+
// Make sure the SRAT structure lies inside the table
if ((Offset + *SratRALength) > AcpiTableLength) {
IncrementErrorCount ();
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


[PATCH v1 03/11] ShellPkg: acpiview: FADT: Validate global pointer before use

Krzysztof Koch
 

Check if global pointers have been successfully updated before they
are used for further table parsing.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
v1:
- Test against NULL pointers [Krzysztof]

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
index e40c9ef8ee4b3285faf8c6edf3cb6236ee367397..e218e45926abced1096e75441e22108db7a3a811 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
@@ -203,6 +203,20 @@ ParseAcpiFadt (
PARSER_PARAMS (FadtParser)
);

+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((DsdtAddress == NULL) ||
+ (FadtMinorRevision == NULL) ||
+ (X_DsdtAddress == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient table length. AcpiTableLength = %d. " \
+ L"FADT parsing aborted.\n",
+ AcpiTableLength
+ );
+ return;
+ }
+
if (Trace) {
Print (L"\nSummary:\n");
PrintFieldName (2, L"FADT Version");
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


[PATCH v1 00/11] Test against invalid pointers in acpiview

Krzysztof Koch
 

Prevent the use of invalid pointers when parsing ACPI tables in the UEFI
shell acpiview tool.

The parsing of ACPI tables is often controlled with the values read
earlier from the same table. For example, the 'Offset' or 'Count' fields
found in a structure are later used to parse the substructures. If such
fields lie outside the structure's buffer length provided, then there
is a possibility for a wild or dangling pointer.

Currently, if the ParseAcpi() function terminates early because the end
of the input table data buffer has been reached, then the pointers
which were supposed to be updated by this function are left untouched.
This is a security issue as the values pointed to by these pointers are
later used for flow control.

This patch series aims to solve this security issue by explicitly
initializing any pointers lying outside the input ACPI data buffer to
NULL and testing for NULL whenever these pointers are dereferenced.

Changes can be seet at: https://github.com/KrzysztofKoch1/edk2/tree/612_add_pointer_validation_v1

Krzysztof Koch (11):
ShellPkg: acpiview: Set ItemPtr to NULL for unprocessed table fields
ShellPkg: acpiview: RSDP: Validate global pointer before use
ShellPkg: acpiview: FADT: Validate global pointer before use
ShellPkg: acpiview: SLIT: Validate global pointer before use
ShellPkg: acpiview: SLIT: Validate System Locality count
ShellPkg: acpiview: SRAT: Validate global pointers before use
ShellPkg: acpiview: MADT: Validate global pointers before use
ShellPkg: acpiview: PPTT: Validate global pointers before use
ShellPkg: acpiview: IORT: Validate global pointers before use
ShellPkg: acpiview: GTDT: Validate global pointers before use
ShellPkg: acpiview: DBG2: Validate global pointers before use

ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 9 ++-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c | 43 ++++++++++++++
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 14 +++++
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 37 ++++++++++++
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 52 +++++++++++++++++
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 13 +++++
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 25 ++++++++
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c | 12 ++++
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c | 61 ++++++++++++++++++--
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 13 +++++
10 files changed, 272 insertions(+), 7 deletions(-)

--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


[PATCH v1 01/11] ShellPkg: acpiview: Set ItemPtr to NULL for unprocessed table fields

Krzysztof Koch
 

For fields outside the buffer length provided, reset any pointers,
which were supposed to be updated by a ParseAcpi() function call to
NULL. This way one can easily validate if a pointer was successfully
updated.

The ParseAcpi() function parses the given ACPI table buffer by a
number of bytes which is a minimum of the buffer length and the length
described by ACPI_PARSER array. If the buffer length is shorter than
the array describing how to process the ACPI structure, then it is
possible that the ItemPtr inside ACPI_PARSER may not get updated or
initialized. This can lead to an error if the value pointed to by
ItemPtr is later used to control the parsing logic.

A typical example would be a 'number of elements' field in an ACPI
structure header which defines how many substructures of a given type
are present in the structure body. If the 'number of elements' field
is not parsed, this could result in a dangling pointer which could
cause a problem later.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
v1:
- Set ItemPtr to NULL for unprocessed table fields [Krzysztof]

ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
index 2d6ff80e299eebe7853061d3db89332197c0dc0e..1ede12859721db75d17fd0bfc14dc9e9c0d573aa 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
@@ -502,8 +502,15 @@ ParseAcpi (

for (Index = 0; Index < ParserItems; Index++) {
if ((Offset + Parser[Index].Length) > Length) {
+
+ // For fields outside the buffer length provided, reset any pointers
+ // which were supposed to be updated by this function call
+ if (Parser[Index].ItemPtr != NULL) {
+ *Parser[Index].ItemPtr = NULL;
+ }
+
// We don't parse past the end of the max length specified
- break;
+ continue;
}

if (GetConsistencyChecking () &&
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


[PATCH v1 04/11] ShellPkg: acpiview: SLIT: Validate global pointer before use

Krzysztof Koch
 

Check if SlitSystemLocalityCount pointer has been successfully updated
before it is used for further table parsing.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
v1:
- Test against NULL pointers [Krzysztof]

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
index ca2808db526b1bbb79aeb21ccfc0ae6c79b2dfd8..17e2166a09d8615b714e0c51d4d93d293fcdf601 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
@@ -1,7 +1,7 @@
/** @file
SLIT table parser

- Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -75,9 +75,21 @@ ParseAcpiSlit (
AcpiTableLength,
PARSER_PARAMS (SlitParser)
);
+
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if (SlitSystemLocalityCount == NULL) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient table length. AcpiTableLength = %d.\n",
+ AcpiTableLength
+ );
+ return;
+ }
+
LocalityPtr = Ptr + Offset;
-
LocalityCount = *SlitSystemLocalityCount;
+
// We only print the Localities if the count is less than 16
// If the locality count is more than 16 then refer to the
// raw data dump.
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


Re: [PATCH v1 0/1] Added GOP driver for DisplayLink-based Universal USB Docks to edk2-platforms

Andy Hayes
 

On Thu, Aug 15, 2019 at 10:15:48AM +0000, Andy Hayes wrote:
> Thanks very much for the very quick feedback from both of you - I
> really appreciate it. I'll fix the issues that you pointed out and
> re-submit the patch.

Thanks!

> I've been testing the build with VisualStudio 2015 and GCC 7.3. (VS
> for X64, GCC5 for IA32/X64/ARM/AARCH64) I had a bit of trouble
> getting the clang build to work but I'll try it again to make sure
> that I don't introduce any other errors.

Yeah, the compilers (especially GCC/clang) are getting pickier
My baseline today is Clang 7 and GCC 8.3. Testing with Clang 8 and
GCC9 is definitely becoming relevant, but could in most cases replace
testing with GCC 8.

> The way that the C structures are initialized - what other compilers
> need to be tested to make sure there is sufficient support for this
> syntax? (I believe the syntax is a C99 thing).

Well, in this case, the "problem" is that the compiler generates calls
to "known to be there" libc helper functions. Which aren't for us.
On the IA32/X64 side, these rules have been put in place.
On the ARM/AARCH64 side, we have ArmPkg/Library/CompilerIntrinsicsLib,
which on (32-bit) ARM was a fundamental necessity for other reasons.

So testing with a particular compiler version may or may not trigger
the issue, and seemingly unrelated future code changes may nudge the
compiler into doing something else instead. So it's not exactly
foolproof.

> I know that there is a big list in tools.def, but realistically
> which ones do you have to be sure work correctly, to cover the
> majority of users? It's easy enough to cover all of the versions of
> GCC and CLANG for, say, X86/X64/ARM/AARCH64, but how far back with
> Visual Studio versions do I need to go?

I don't expect everyone to test everything - only to test more than
one option and fix things as reported. Unless it's core code.

It *would* be perfetcly valid (though less than ideal) to have
external device drivers not supported by the full set of toolchains.
Of course, making your code build with multiple toolchains frequently
flush out latent bugs, so...
In this case, the changes were all pretty trivial, so I would prefer
to see them implemented.

We have some cleanup work to do on the cross-compilation aspect in
general for !VS. While it can be nudged into working, it works
differently on ARM and X64.

Personally, I worry mostly about compatibility with the toolchain
versions included in the latest stable version of Debian (since that's
the distribution people tend to complain about lagging behind :). From
Debian Buster, we finally have cross-compilation symmetry
AARCH64<->X64.

For something like UDK, we *should* probably worry about all versions
included in all LTS Linux distro releases, but...

> > I am curious how this driver interacts with the ConSplitter when
> > displays are hot added and hot removed. I see a reference to a
> > feature that appears to sync the GOP content between frame buffers
> > when a display is hot added. I would be better if the common code in
> > the ConSplitter handled these types of operations instead of putting
> > that code in individual GOP drivers. I have added Ray to this thread
> > who may have ideas on how to handle these hot add events.
>
> I think the code that you are looking at that syncs GOP content
> might be a debug feature, rather than a hotplug handler. We found
> that some platforms only render to the first GOP device that they
> find, so I added the ability to build a version of the driver (using

> the build flag COPY_PIXELS_FROM_PRIMARY_GOP_DEVICE) that
> "steals" the pixels from another GOP framebuffer and displays them
> on our output, to check that our driver fundamentally works on that
> platform. This code is excluded in a normal build. Most platforms
> are well-behaved and render to all GOP devices correctly.

Best Regards,

Leif

> In general we've found that hotplug of our device works fine,
> without us having to do anything special in the driver. Note that
> the system will initially see this as a USB hotplug rather than a
> display hotplug. For various reasons we decided not to support
> display hotplug (i.e. plugging and unplugging displays into out
> device once our device is plugged in to USB and initialized) in the
> driver, mainly to keep things simple.
>
> Thanks again for your help,
>
> Andy Hayes
>
> From: Kinney, Michael D <michael.d.kinney@...>
> Sent: 14 August 2019 16:23
> To: Andy Hayes <andy.hayes@...>; devel@edk2.groups.io; Ni, Ray <ray.ni@...>; Kinney, Michael D <michael.d.kinney@...>
> Cc: Leif Lindholm <leif.lindholm@...>
> Subject: [External] RE: [PATCH v1 0/1] Added GOP driver for DisplayLink-based Universal USB Docks to edk2-platforms
>
> Hi Andy,
>
> Thanks for the contribution. Is this patch for the edk2-platform repo? The email subject can help clarify the intended repo.
>
> Also, we prefer the patches to be provided inline instead of an attachment. You can use the
> 'git send-email' feature to do this. Here are a couple links to useful wikis.
>
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process>
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers<https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers>
>
> I also noticed looking at the patch file that there are some inconsistencies with the
> EDK II C Coding Standards. Most are around function headers and indents. Here is a link
> to the latest version:
>
> https://www.gitbook.com/download/pdf/book/edk2-docs/edk-ii-c-coding-standards-specification<https://www.gitbook.com/download/pdf/book/edk2-docs/edk-ii-c-coding-standards-specification>
>
> I see a few #if 0 and commented out lines of code and TODO comments. Can you please clean these up
> and if needed add Tianocore Bugzillas for future enhancements/work items for this driver.
>
> https://bugzilla.tianocore.org/<https://bugzilla.tianocore.org/>
>
> I also noticed that you are using named fields to initialize C structures:
>
> STATIC CONST struct VideoMode ModeData[] = {
> + {
> + // 640 x 480 @ 60Hz
> + .Reserved2 = 2,
> + .PixelClock = 2517,
> + .HActive = 640,
> + .HBlanking = 160,
> + .HSyncOffset = 16,
> + .HSyncWidth = 96,
>
> I do like this style because it has better self documentation of the field being
> initialized to a value. However, I do not see this style in the rest of the EDK II
> sources, so I am concerned that we may have required compilers for the EDK II
> that may not support this syntax.
>
> If we find all the supported compilers do support this syntax, this would be
> a great addition to the EDK II C Coding Standards.
>
> I am curious how this driver interacts with the ConSplitter when displays
> are hot added and hot removed. I see a reference to a feature that appears
> to sync the GOP content between frame buffers when a display is hot added.
> I would be better if the common code in the ConSplitter handled these types
> of operations instead of putting that code in individual GOP drivers. I have
> added Ray to this thread who may have ideas on how to handle these hot
> add events.
>
> Thanks,
>
> Mike
>
> From: Andy Hayes [mailto:andy.hayes@...]
> Sent: Wednesday, August 14, 2019 7:44 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Leif Lindholm <leif.lindholm@...<mailto:leif.lindholm@...>>; Kinney, Michael D <michael.d.kinney@...<mailto:michael.d.kinney@...>>
> Subject: [PATCH v1 0/1] Added GOP driver for DisplayLink-based Universal USB Docks to edk2-platforms
>
> From 4a42eb997aeb3699217b40bf3bc47dec56458847 Mon Sep 17 00:00:00 2001
> From: "Andy Hayes" < andy.hayes@...<mailto:andy.hayes@...> >
> Date: Wed, 14 Aug 2019 15:30:20 +0100
> Subject: [PATCH v1 0/1] Added GOP graphics driver for DisplayLink-based Universal USB Docks to edk2-platforms
>
> andy.hayes@...<mailto:andy.hayes@...> (1):
> Added GOP driver for USB Docks which use DisplayLink chips.
>
> .../DisplayLinkPkg/DisplayLinkPkg.dsc | 61 +
> .../DisplayLinkGop/DisplayLinkGopDxe.inf | 63 +
> .../DisplayLinkPkg/DisplayLinkGop/Edid.h | 129 ++
> .../DisplayLinkGop/UsbDescriptors.h | 109 ++
> .../DisplayLinkGop/UsbDisplayLink.h | 284 +++++
> .../DisplayLinkGop/CapabilityDescriptor.c | 137 ++
> .../DisplayLinkGop/ComponentName.c | 235 ++++
> .../DisplayLinkPkg/DisplayLinkGop/Edid.c | 598 +++++++++
> .../DisplayLinkPkg/DisplayLinkGop/Gop.c | 587 +++++++++
> .../DisplayLinkGop/UsbDescriptors.c | 144 +++
> .../DisplayLinkGop/UsbDisplayLink.c | 1109 +++++++++++++++++
> .../DisplayLinkGop/UsbTransfer.c | 180 +++
> .../DisplayLinkGop/VideoModes.c | 254 ++++
> Drivers/DisplayLink/DisplayLinkPkg/ReadMe.md | 77 ++
> Maintainers.txt | 5 +
> 15 files changed, 3972 insertions(+)
> create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkPkg.dsc
> create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/DisplayLinkGopDxe.inf
> create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Edid.h
> create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDescriptors.h
> create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDisplayLink.h
> create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/CapabilityDescriptor.c
> create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/ComponentName.c
> create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Edid.c
> create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Gop.c
> create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDescriptors.c
> create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDisplayLink.c
> create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbTransfer.c
> create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/VideoModes.c
> create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/ReadMe.md
>
> --
> 2.17.1


Re: [edk2-platforms: PATCH 1/1] Platforms/RPi3: Add multiple embedded Device Tree selection

Leif Lindholm
 

On Mon, Aug 12, 2019 at 02:06:34PM +0100, Pete Batard wrote:
The Raspberry Pi 3 platform currently has 2 different models, each with a
different Device Tree. Rather than embedding a single one, and requiring
users to manually provide the other, this patch ensures that we now embed
both and and serve the relevant one at runtime.

Signed-off-by: Pete Batard <pete@akeo.ie>
Changeset looks very useful. Some style issues. And one question.

First the question: is there no practical size restriction on the
firmware image? This is a 24k increase in image size.

---
Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 57 ++++++++++++++++----
Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf | 3 +-
Platform/RaspberryPi/RPi3/RPi3.dec | 3 +-
Platform/RaspberryPi/RPi3/RPi3.fdf | 6 ++-
4 files changed, 55 insertions(+), 14 deletions(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
index c84e5b2767e2..7c0ab75e7cb1 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
@@ -20,6 +20,11 @@

#include <Guid/Fdt.h>

+CONST EFI_GUID *mFdtGuid[] = {
+ &gRaspberryPiFdtGuid1,
+ &gRaspberryPiFdtGuid2,
+ };
+
STATIC VOID *mFdtImage;

STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;
@@ -368,7 +373,9 @@ FdtDxeInitialize (
EFI_STATUS Status;
VOID *FdtImage;
UINTN FdtSize;
+ UINTN FdtIndex;
INT32 Retval;
+ UINT32 BoardRevision;
BOOLEAN Internal;

Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, NULL,
@@ -383,13 +390,41 @@ FdtDxeInitialize (
* Have FDT passed via config.txt.
*/
FdtSize = fdt_totalsize (FdtImage);
This
- DEBUG ((DEBUG_INFO, "DTB passed via config.txt of 0x%lx bytes\n", FdtSize));
+ DEBUG ((DEBUG_INFO, "Device Tree passed via config.txt (0x%lx bytes)\n", FdtSize));
looks unrelated to the changeset.

Status = EFI_SUCCESS;
} else {
+ /*
+ * Use one of the embedded FDT's.
+ */
Internal = TRUE;
This DEBUG changes bit
- DEBUG ((DEBUG_INFO, "No/bad FDT at %p (%a), trying internal DTB...\n",
- FdtImage, fdt_strerror (Retval)));
- Status = GetSectionFromAnyFv (&gRaspberryPiFdtFileGuid, EFI_SECTION_RAW, 0,
+ DEBUG ((DEBUG_INFO, "No/Bad Device Tree found at address 0x%p (%a), "
+ "looking up internal one...\n", FdtImage, fdt_strerror (Retval)));
to here looks completely unrelated to the changeset.
Which garbles the diff somewhat with regards to the functional line in
the middle..

+ /*
+ * Query the board revision to differentiate between models.
+ */
+ Status = mFwProtocol->GetModelRevision (&BoardRevision);
+ if (EFI_ERROR (Status)) {
+ FdtIndex = 0;
+ DEBUG ((DEBUG_ERROR, "Failed to get board type: %r\n", Status));
+ } else {
+ // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
+ switch ((BoardRevision >> 4) & 0xFF) {
+ case 0x08: // Raspberry 3 Model B
+ DEBUG ((DEBUG_INFO, "Using RPi3 Model B internal Device Tree\n"));
+ FdtIndex = 0;
+ break;
+ case 0x0D: // Raspberry 3 Model B+
+ DEBUG ((DEBUG_INFO, "Using RPi3 Model B+ internal Device Tree\n"));
+ FdtIndex = 1;
+ break;
+ default:
+ DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n"));
+ FdtIndex = 0;
+ break;
+ }
+ }
+ ASSERT (FdtIndex < ARRAY_SIZE (mFdtGuid));
+ Status = GetSectionFromAnyFv (mFdtGuid[FdtIndex], EFI_SECTION_RAW, 0,
&FdtImage, &FdtSize);
if (Status == EFI_SUCCESS) {
if (fdt_check_header (FdtImage) != 0) {
Everything from here...

@@ -419,27 +454,27 @@ FdtDxeInitialize (

Status = SanitizePSCI ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to sanitize PSCI (error %d)\n", Status);
+ Print (L"Failed to sanitize PSCI: %r\n", Status);
}

Status = CleanMemoryNodes ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to clean memory nodes (error %d)\n", Status);
+ Print (L"Failed to clean memory nodes: %r\n", Status);
}

Status = CleanSimpleFramebuffer ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to clean frame buffer (error %d)\n", Status);
+ Print (L"Failed to clean frame buffer: %r\n", Status);
}

Status = FixEthernetAliases ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to fix ethernet aliases (error %d)\n", Status);
+ Print (L"Failed to fix ethernet aliases: %r\n", Status);
}

Status = UpdateMacAddress ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to update MAC address (error %d)\n", Status);
+ Print (L"Failed to update MAC address: %r\n", Status);
}

if (Internal) {
@@ -448,11 +483,11 @@ FdtDxeInitialize (
*/
Status = UpdateBootArgs ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to update boot arguments (error %d)\n", Status);
+ Print (L"Failed to update boot arguments: %r\n", Status);
}
}

- DEBUG ((DEBUG_INFO, "Installed FDT is at %p\n", mFdtImage));
+ DEBUG ((DEBUG_INFO, "Installed Device Tree at address 0x%p\n", mFdtImage));
Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage);
ASSERT_EFI_ERROR (Status);
...to here is unrelated printout message changes.

So, I don't object to the debug printout changes as such, but they
obscure the functional changeset. Can you please break them out as a
separate patch?

Best Regards,

Leif

diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
index 5b0b1a09f374..c994a2b69d78 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
+++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
@@ -35,7 +35,8 @@ [LibraryClasses]

[Guids]
gFdtTableGuid
- gRaspberryPiFdtFileGuid
+ gRaspberryPiFdtGuid1
+ gRaspberryPiFdtGuid2

[Protocols]
gRaspberryPiFirmwareProtocolGuid ## CONSUMES
diff --git a/Platform/RaspberryPi/RPi3/RPi3.dec b/Platform/RaspberryPi/RPi3/RPi3.dec
index 22de439fde8f..d90ea8329818 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dec
+++ b/Platform/RaspberryPi/RPi3/RPi3.dec
@@ -24,7 +24,8 @@ [Protocols]

[Guids]
gRaspberryPiTokenSpaceGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
- gRaspberryPiFdtFileGuid = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}}
+ gRaspberryPiFdtGuid1 = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}}
+ gRaspberryPiFdtGuid2 = {0x3D523012, 0x73FE, 0x40E5, { 0x89, 0x2E, 0x1A, 0x4D, 0xF6, 0x0F, 0x3C, 0x0C}}
gRaspberryPiEventResetGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB4, 0xB4, 0xE4, 0xD4, 0xB4}}
gConfigDxeFormSetGuid = {0xCD7CC258, 0x31DB, 0x22E6, {0x9F, 0x22, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}

diff --git a/Platform/RaspberryPi/RPi3/RPi3.fdf b/Platform/RaspberryPi/RPi3/RPi3.fdf
index c62d649834c7..83753d84badc 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.fdf
+++ b/Platform/RaspberryPi/RPi3/RPi3.fdf
@@ -300,11 +300,15 @@ [FV.FvMain]
INF Platform/RaspberryPi/$(PLATFORM_NAME)/Drivers/LogoDxe/LogoDxe.inf

#
- # FDT (GUID matches gRaspberryPiFdtFileGuid in FdtDxe)
+ # Device Tree support
+ # GUIDs match gRaspberryPiFdtGuid# from FdtDxe.
#
FILE FREEFORM = DF5DA223-1D27-47C3-8D1B-9A41B55A18BC {
SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b.dtb
}
+ FILE FREEFORM = 3D523012-73FE-40E5-892E-1A4DF60F3C0C {
+ SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b-plus.dtb
+ }

[FV.FVMAIN_COMPACT]
FvAlignment = 16
--
2.21.0.windows.1


Re: [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf

Vitaly Cheptsov
 

Hi Donald,

Glad to hear it helped a little, and sorry for some outdated quotes.

Your clarification regarding model range is very helpful. Xeon W being client and thus having client clock makes sense, though I must say it was quite not obvious. I was referring to the same SDM table, and it would have been great to have vertical range binding instead of the misleading CPUID.

With that, however, I still do not see a way to dynamically determine the difference between Xeon client and server.

For us it is needed as we use TimerLib differently. For you TimerLib is a part of BSP, so you face no issue statically setting the clock frequency as a PCD. For us TimerLib is used as a part of an UEFI application compatible with different hardware, and in fact not just Intel. We have such "generic TimerLib" library, and to determine CPU frequency, as a fallback to CPUID 15H, it relies on ACPI PM timer. This is not great for two reasons:
- we have to maintain it ourselves, while we would have liked that be part of EDK II with different vendors contributing to the project.
- we still cannot find an obvious way to determine crystal clock when it is not reported, in particular for Xeon W and Xeon Scalable case.

I guess this is now out of the scope of this patch and perhaps even this exact library, but it will be helpful to hear relevant technical details on the issue and opinions on such "generic TimerLib" library to later appear in EDK II.

Best regards,
Vitaly

15 авг. 2019 г., в 11:40, Kuo, Donald <donald.kuo@...> написал(а):

Hi Vitaly,
 
Appreciated for reviewing very detail. And looks your captured some piece of code is older version. And should be ok, I do got your point.
 
Item #1
-          I do missed RegEax error handling in case for CpuidCoreClockCalculateTscFrequency () should return 0 and EFI_UNSUPPORTED. AR: Will update it.
 
Item #2:
-          Actually the information is from SDM Table 18-85
o   As we know CPUID signature could be hard to identify processor XTAL frequency is? So we only check CPUID Leaf 0x15
o   And the PCD will be defined depends on platform specific and during project early development. Based on SDM, Intel processor for CPUID.15h EAX and EBX is enumerated, but ECX could be possible not enumerated. So that is why we based on  SDM Table 18-85 to create PCD as below:
§  Intel Xeon Server family: 25MHz
§  Intel Core and Intel Xeon W family: 24MHz
§  Intel Atom : 19.2MHz
§  So regards the “06_55h” might cause the confused, we could review it whether remove it??
Item #3:
-          Again, the XTAL frequency is optional and should be define in early phase of new project. Default should still use AcpiTimer.
o    Platform / developer owner should well know the CPU generation XTAL frequency is? Server / Client / Atom ?
o   For those CPU not supported should still use AcpiTimerLib (default)
 
Thanks,
Donald
 
From: vit9696 via [] [mailto:vit9696=protonmail.com@[]] 
Sent: Thursday, August 15, 2019 3:20 PM
To: Kuo, Donald <donald.kuo@...>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf
 
Hello,

Thank you for the patch! I reviewed the code and noticed select issues explained below.

1. The following construction:

if (RegEbx == 0) {
DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Frequency !!\n"));
ASSERT (RegEbx != 0);
}

Does not look good to me, and in my opinion, should be written differently:

if (RegEbx == 0 || RegEax == 0) {
DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Frequency !!\n"));
ASSERT (RegEbx != 0);
  ASSERT (RegEax != 0);
  return 0;
}

The reason for the above code being wrong is potential division by zero below, which behaviour is undefined (and in fact unknown due to unspecified interrupt table state). Even though the intent is to not permit the use of this library on an unsupported platform, runtime checks and assertions are essentially NO-OPs, should be separate and not confused. For this to work properly the call to CpuidCoreClockCalculateTscFrequency should happen in library constructor with EFI_UNSUPPORTED return on CpuidCoreClockCalculateTscFrequency returning 0.

2. The notes about crystal clock frequency for 06_55H CPU signature:
"25000000 - Intel Xeon Processor Scalable Family with CPUID signature 06_55H(25MHz).<BR>\n"
# Intel Xeon Processor Scalable Family with CPUID signature 06_55H = 25000000 (25MHz)

are misleading in both this library and Intel SDM. Intel Xeon W processors have the same CPUID signature (06_55H), they report 0 crystal clock frequency, and their crystal clock frequency is 24 MHz. This should at least be mentioned in the lower part mentioning Intel Xeon W Processor Family(24MHz).

Actually, given that many Intel guys are here, I wonder whether anybody knows if there is any better approach to distinguish Xeon Scalable CPUs and Xeon W CPUs besides using brand string or using marketing frequency from CPUID 16H to determine crystal clock frequency (this is what Linux does at the moment)?

3. Intel Atom Denverton with CPUID signature (06_5FH), also known as Goldmont X, reports 0 crystal clock frequency as well, and has 25 MHz frequency. This is missing from SDM, but once again I believe it should be included in the two places from the above to avoid confusion.

Besides these 3 points, honestly, the library itself appears to be very limited for anything but embedding it into the firmware with known hardware, which already works just fine by defining the PCD. Missing 0 crystal clock frequency handling in runtime with hardcoding the PCD value looks very bad, because the number of modern Intel CPU models reporting 0 crystal clock frequency and having non 24 MHz frequency is quite far from 0. This makes the library essentially impossible to use in any UEFI application or third-party product even when targeting Skylake+ generation. To resolute this I vote for additional detection functionality to be added to the library to obtain crystal clock frequency when the CPUID reports 0. The PCD could be the last resort when no other method works. My opinion is that this should be resolved prior to merging the patch.

Best regards,
Vitaly 


Re: [PATCH 1/1] CryptoPkg/BaseCryptLib: Update pHkdfCtx to PHkdfCtx

Zhang, Shenglei
 

Jian,

Yes, you are right.
According to CCS_2_1_Draft, 4.3.3.3, Pointer variables may optionally be prefixed with a 'p'.
So please skip this change.

Thanks,
Shenglei

-----Original Message-----
From: Wang, Jian J
Sent: Thursday, August 15, 2019 5:01 PM
To: Zhang, Shenglei <shenglei.zhang@intel.com>; devel@edk2.groups.io
Cc: Ye, Ting <ting.ye@intel.com>
Subject: RE: [PATCH 1/1] CryptoPkg/BaseCryptLib: Update pHkdfCtx to
PHkdfCtx

Shenglei,

I remember the edk2 coding style allows prefix 'p' (optional) to represent a
pointer variable.

Regards,
Jian


-----Original Message-----
From: Zhang, Shenglei
Sent: Thursday, August 15, 2019 4:49 PM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
Subject: [PATCH 1/1] CryptoPkg/BaseCryptLib: Update pHkdfCtx to
PHkdfCtx

Update pHkdfCtx to PHkdfCtx, to follow the variable naming
rule.

Cc: Jian Wang <jian.j.wang@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
.../Library/BaseCryptLib/Kdf/CryptHkdf.c | 22 +++++++++----------
1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdf.c
b/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdf.c
index f0fcef211d3f..488346a38b8c 100644
--- a/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdf.c
+++ b/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdf.c
@@ -39,7 +39,7 @@ HkdfSha256ExtractAndExpand (
IN UINTN OutSize
)
{
- EVP_PKEY_CTX *pHkdfCtx;
+ EVP_PKEY_CTX *PHkdfCtx;
BOOLEAN Result;

if (Key == NULL || Salt == NULL || Info == NULL || Out == NULL ||
@@ -47,29 +47,29 @@ HkdfSha256ExtractAndExpand (
return FALSE;
}

- pHkdfCtx = EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, NULL);
- if (pHkdfCtx == NULL) {
+ PHkdfCtx = EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, NULL);
+ if (PHkdfCtx == NULL) {
return FALSE;
}

- Result = EVP_PKEY_derive_init(pHkdfCtx) > 0;
+ Result = EVP_PKEY_derive_init(PHkdfCtx) > 0;
if (Result) {
- Result = EVP_PKEY_CTX_set_hkdf_md(pHkdfCtx, EVP_sha256()) > 0;
+ Result = EVP_PKEY_CTX_set_hkdf_md(PHkdfCtx, EVP_sha256()) > 0;
}
if (Result) {
- Result = EVP_PKEY_CTX_set1_hkdf_salt(pHkdfCtx, Salt,
(UINT32)SaltSize) > 0;
+ Result = EVP_PKEY_CTX_set1_hkdf_salt(PHkdfCtx, Salt,
(UINT32)SaltSize) > 0;
}
if (Result) {
- Result = EVP_PKEY_CTX_set1_hkdf_key(pHkdfCtx, Key,
(UINT32)KeySize) > 0;
+ Result = EVP_PKEY_CTX_set1_hkdf_key(PHkdfCtx, Key,
(UINT32)KeySize) > 0;
}
if (Result) {
- Result = EVP_PKEY_CTX_add1_hkdf_info(pHkdfCtx, Info,
(UINT32)InfoSize) > 0;
+ Result = EVP_PKEY_CTX_add1_hkdf_info(PHkdfCtx, Info,
(UINT32)InfoSize) > 0;
}
if (Result) {
- Result = EVP_PKEY_derive(pHkdfCtx, Out, &OutSize) > 0;
+ Result = EVP_PKEY_derive(PHkdfCtx, Out, &OutSize) > 0;
}

- EVP_PKEY_CTX_free(pHkdfCtx);
- pHkdfCtx = NULL;
+ EVP_PKEY_CTX_free(PHkdfCtx);
+ PHkdfCtx = NULL;
return Result;
}
--
2.18.0.windows.1


Re: [PATCH v5 00/35] Specific platform to run OVMF in Xen PVH and HVM guests

Laszlo Ersek
 

On 08/13/19 13:30, Anthony PERARD wrote:
Patch series available in this git branch:
https://xenbits.xen.org/git-http/people/aperard/ovmf.git br.platform-xen-pvh-v5

Changes in v5:
- patch 23 got a rework of the lapic range skipping
- small fixups in patch 20, 22, 23, 31, 32, 33.
see notes for detail.
This series is now fully covered with maintainer R-b's and A-b's.

I've also done some regression-tests, after applying the set in a topic
branch on top of commit caa7d3a896f6 ("BaseTools/Scripts: Add
GetUtcDateTime script.", 2019-08-15). Including build tests and my usual
boot & S3 tests.

Building for DEBUG (with GCC48) requires the independent fix

[edk2-devel] [PATCH 1/1]
MdeModulePkg/DxeIplPeim: Initialize pointer PageMapLevel5Entry

which was posted at

http://mid.mail-archive.com/20190814073741.16080-1-shenglei.zhang@intel.com
https://edk2.groups.io/g/devel/message/45591

(again, that issue is independent of this series). With that independent
fix, RELEASE builds fine too.

Given that this v5 feature series has now been fully reviewed before
entering the Soft Feature Freeze for edk2-stable201908 -- which will
commence on 2019-08-16 at 00:00:00 UTC-8) --, the set is eligible for
pushing during the Soft Feature Freeze:

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
https://github.com/tianocore/tianocore.github.io/wiki/SoftFeatureFreeze

Therefore I'll push v5 no later than 2019-Aug-21, unless a NACK arrives
before that date, from xen-devel or elsewhere.

Thanks
Laszlo

Hi,

I've started to create a Xen specific platform, in OvmfPkg/XenOvmf.dsc
with the goal to make it work on both Xen HVM and Xen PVH.

The first few patches only create the platform and duplicate some code from
OvmfPkg and the later patches makes OVMF boot in a Xen PVH guest and can boot
a Linux guest.

After this patch series, I'd like to wait a bit before removing Xen support
from the OvmfPkg*.dsc, to allow time to switch to the new Xen only platform,
maybe 1 year.

To build and boot:

To build, simply run OvmfPkg/build.sh -p OvmfPkg/OvmfXen.dsc
Then use OVMF.fd as a kernel of a pvh guest config file (with xl/libxl).

Patch series available in this git branch:
https://xenbits.xen.org/git-http/people/aperard/ovmf.git br.platform-xen-pvh-v5

Anthony PERARD (35):
OvmfPkg/ResetSystemLib: Add missing dependency on PciLib
OvmfPkg: Create platform OvmfXen
OvmfPkg: Introduce XenResetVector
OvmfPkg: Introduce XenPlatformPei
OvmfPkg/OvmfXen: Creating an ELF header
OvmfPkg/XenResetVector: Add new entry point for Xen PVH
OvmfPkg/XenResetVector: Saving start of day pointer for PVH guests
OvmfPkg/XenResetVector: Allow jumpstart from either hvmloader or PVH
OvmfPkg/OvmfXen: use a TimerLib instance that depends only on the CPU
OvmfPkg/XenPlatformPei: Detect OVMF_INFO from hvmloader
OvmfPkg/XenPlatformPei: Use mXenHvmloaderInfo to get E820
OvmfPkg/XenPlatformPei: Grab RSDP from PVH guest start of day struct
OvmfPkg/Library/XenPlatformLib: New library
OvmfPkg/AcpiPlatformDxe: Use XenPlatformLib
OvmfPkg/AcpiPlatformDxe: Use Xen PVH RSDP if it exist
OvmfPkg/XenHypercallLib: Enable it in PEIM
OvmfPkg/XenPlatformPei: Reinit XenHypercallLib
OvmfPkg/XenPlatformPei: Introduce XenHvmloaderDetected
OvmfPkg/XenPlatformPei: Setup HyperPages earlier
OvmfPkg/XenPlatformPei: Introduce XenPvhDetected
OvmfPkg: Import XENMEM_memory_map hypercall to Xen/memory.h
OvmfPkg/XenPlatformPei: no hvmloader: get the E820 table via hypercall
OvmfPkg/XenPlatformPei: Rework memory detection
OvmfPkg/XenPlatformPei: Reserve VGA memory region, to boot Linux
OvmfPkg/XenPlatformPei: Ignore missing PCI Host Bridge on Xen PVH
OvmfPkg/XenPlatformLib: Cache result for XenDetected
OvmfPkg/PlatformBootManagerLib: Use XenDetected from XenPlatformLib
OvmfPkg/PlatformBootManagerLib: Handle the absence of PCI bus on Xen
PVH
OvmfPkg/OvmfXen: Override PcdFSBClock to Xen vLAPIC timer frequency
OvmfPkg/OvmfXen: Introduce XenTimerDxe
OvmfPkg/PlatformBootManagerLib: Use a Xen console for ConOut/ConIn
OvmfPkg: Introduce PcdXenGrantFrames
OvmfPkg: Introduce XenIoPvhDxe to initialize Grant Tables
OvmfPkg: Move XenRealTimeClockLib from ArmVirtPkg
OvmfPkg/OvmfXen: use RealTimeClockRuntimeDxe from EmbeddedPkg

OvmfPkg/OvmfPkg.dec | 10 +
ArmVirtPkg/ArmVirtXen.dsc | 2 +-
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
OvmfPkg/{OvmfPkgX64.dsc => OvmfXen.dsc} | 238 +-------
OvmfPkg/OvmfXen.fdf | 539 ++++++++++++++++++
OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 3 +-
.../PlatformBootManagerLib.inf | 6 +-
.../Library/ResetSystemLib/ResetSystemLib.inf | 1 +
.../XenHypercallLib/XenHypercallLib.inf | 4 +-
.../Library/XenPlatformLib/XenPlatformLib.inf | 33 ++
.../XenRealTimeClockLib.inf | 0
OvmfPkg/XenBusDxe/XenBusDxe.inf | 3 +
OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf | 36 ++
OvmfPkg/XenPlatformPei/XenPlatformPei.inf | 100 ++++
OvmfPkg/XenResetVector/XenResetVector.inf | 41 ++
OvmfPkg/XenTimerDxe/XenTimerDxe.inf | 42 ++
OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h | 6 +-
OvmfPkg/Include/Guid/XenInfo.h | 8 +-
.../Xen/arch-x86/hvm/start_info.h | 143 +++++
OvmfPkg/Include/IndustryStandard/Xen/memory.h | 23 +
OvmfPkg/Include/Library/XenHypercallLib.h | 12 +
OvmfPkg/Include/Library/XenPlatformLib.h | 53 ++
.../PlatformBootManagerLib/BdsPlatform.h | 1 +
OvmfPkg/XenBusDxe/XenBusDxe.h | 1 +
OvmfPkg/XenPlatformPei/Cmos.h | 52 ++
OvmfPkg/XenPlatformPei/Platform.h | 136 +++++
OvmfPkg/XenPlatformPei/Xen.h | 39 ++
OvmfPkg/XenTimerDxe/XenTimerDxe.h | 177 ++++++
OvmfPkg/AcpiPlatformDxe/Xen.c | 41 +-
.../PlatformBootManagerLib/BdsPlatform.c | 43 +-
.../PlatformBootManagerLib/PlatformData.c | 49 +-
.../Library/ResetSystemLib/ResetSystemLib.c | 3 +-
.../Library/XenHypercallLib/X86XenHypercall.c | 8 +-
.../Library/XenHypercallLib/XenHypercall.c | 16 +
.../Library/XenPlatformLib/XenPlatformLib.c | 81 +++
.../XenRealTimeClockLib/XenRealTimeClockLib.c | 0
OvmfPkg/OvmfXenElfHeaderGenerator.c | 140 +++++
OvmfPkg/PlatformPei/Xen.c | 3 -
OvmfPkg/XenBusDxe/GrantTable.c | 3 +-
OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c | 54 ++
OvmfPkg/XenPlatformPei/AmdSev.c | 64 +++
OvmfPkg/XenPlatformPei/ClearCache.c | 112 ++++
OvmfPkg/XenPlatformPei/Cmos.c | 60 ++
OvmfPkg/XenPlatformPei/Fv.c | 76 +++
OvmfPkg/XenPlatformPei/MemDetect.c | 490 ++++++++++++++++
OvmfPkg/XenPlatformPei/Platform.c | 463 +++++++++++++++
OvmfPkg/XenPlatformPei/Xen.c | 388 +++++++++++++
OvmfPkg/XenTimerDxe/XenTimerDxe.c | 355 ++++++++++++
Maintainers.txt | 10 +-
.../XenResetVector/Ia16/Real16ToFlat32.asm | 137 +++++
.../XenResetVector/Ia16/ResetVectorVtf0.asm | 79 +++
.../XenResetVector/Ia32/Flat32ToFlat64.asm | 68 +++
OvmfPkg/XenResetVector/Ia32/PageTables64.asm | 149 +++++
.../XenResetVector/Ia32/SearchForBfvBase.asm | 87 +++
OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm | 84 +++
OvmfPkg/XenResetVector/XenResetVector.nasmb | 71 +++
58 files changed, 4541 insertions(+), 305 deletions(-)
copy OvmfPkg/{OvmfPkgX64.dsc => OvmfXen.dsc} (76%)
create mode 100644 OvmfPkg/OvmfXen.fdf
create mode 100644 OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
rename {ArmVirtPkg => OvmfPkg}/Library/XenRealTimeClockLib/XenRealTimeClockLib.inf (100%)
create mode 100644 OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
create mode 100644 OvmfPkg/XenPlatformPei/XenPlatformPei.inf
create mode 100644 OvmfPkg/XenResetVector/XenResetVector.inf
create mode 100644 OvmfPkg/XenTimerDxe/XenTimerDxe.inf
create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/arch-x86/hvm/start_info.h
create mode 100644 OvmfPkg/Include/Library/XenPlatformLib.h
create mode 100644 OvmfPkg/XenPlatformPei/Cmos.h
create mode 100644 OvmfPkg/XenPlatformPei/Platform.h
create mode 100644 OvmfPkg/XenPlatformPei/Xen.h
create mode 100644 OvmfPkg/XenTimerDxe/XenTimerDxe.h
create mode 100644 OvmfPkg/Library/XenPlatformLib/XenPlatformLib.c
rename {ArmVirtPkg => OvmfPkg}/Library/XenRealTimeClockLib/XenRealTimeClockLib.c (100%)
create mode 100644 OvmfPkg/OvmfXenElfHeaderGenerator.c
create mode 100644 OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c
create mode 100644 OvmfPkg/XenPlatformPei/AmdSev.c
create mode 100644 OvmfPkg/XenPlatformPei/ClearCache.c
create mode 100644 OvmfPkg/XenPlatformPei/Cmos.c
create mode 100644 OvmfPkg/XenPlatformPei/Fv.c
create mode 100644 OvmfPkg/XenPlatformPei/MemDetect.c
create mode 100644 OvmfPkg/XenPlatformPei/Platform.c
create mode 100644 OvmfPkg/XenPlatformPei/Xen.c
create mode 100644 OvmfPkg/XenTimerDxe/XenTimerDxe.c
create mode 100644 OvmfPkg/XenResetVector/Ia16/Real16ToFlat32.asm
create mode 100644 OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
create mode 100644 OvmfPkg/XenResetVector/Ia32/Flat32ToFlat64.asm
create mode 100644 OvmfPkg/XenResetVector/Ia32/PageTables64.asm
create mode 100644 OvmfPkg/XenResetVector/Ia32/SearchForBfvBase.asm
create mode 100644 OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
create mode 100644 OvmfPkg/XenResetVector/XenResetVector.nasmb


Re: CPU hotplug using SMM with QEMU+OVMF

Yao, Jiewen
 

Hi Paolo
I am not sure what do you mean - "You do not need a reset vector ...".
If so, where is the first instruction of the new CPU in the virtualization environment?
Please help me understand that at first. Then we can continue the discussion.

Thank you
Yao Jiewen

-----Original Message-----
From: Paolo Bonzini [mailto:pbonzini@redhat.com]
Sent: Wednesday, August 14, 2019 10:05 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; Laszlo Ersek
<lersek@redhat.com>; edk2-devel-groups-io <devel@edk2.groups.io>
Cc: edk2-rfc-groups-io <rfc@edk2.groups.io>; qemu devel list
<qemu-devel@nongnu.org>; Igor Mammedov <imammedo@redhat.com>;
Chen, Yingwen <yingwen.chen@intel.com>; Nakajima, Jun
<jun.nakajima@intel.com>; Boris Ostrovsky <boris.ostrovsky@oracle.com>;
Joao Marcal Lemos Martins <joao.m.martins@oracle.com>; Phillip Goerl
<phillip.goerl@oracle.com>
Subject: Re: CPU hotplug using SMM with QEMU+OVMF

On 14/08/19 15:20, Yao, Jiewen wrote:
- Does this part require a new branch somewhere in the OVMF SEC code?
How do we determine whether the CPU executing SEC is BSP or
hot-plugged AP?
[Jiewen] I think this is blocked from hardware perspective, since the first
instruction.
There are some hardware specific registers can be used to determine if the
CPU is new added.
I don’t think this must be same as the real hardware.
You are free to invent some registers in device model to be used in OVMF
hot plug driver.

Yes, this would be a new operation mode for QEMU, that only applies to
hot-plugged CPUs. In this mode the AP doesn't reply to INIT or SMI, in
fact it doesn't reply to anything at all.

- How do we tell the hot-plugged AP where to start execution? (I.e. that
it should execute code at a particular pflash location.)
[Jiewen] Same real mode reset vector at FFFF:FFF0.
You do not need a reset vector or INIT/SIPI/SIPI sequence at all in
QEMU. The AP does not start execution at all when it is unplugged, so
no cache-as-RAM etc.
We only need to modify QEMU so that hot-plugged APIs do not reply to
INIT/SIPI/SMI.

I don’t think there is problem for real hardware, who always has CAR.
Can QEMU provide some CPU specific space, such as MMIO region?
Why is a CPU-specific region needed if every other processor is in SMM
and thus trusted.
Does CPU hotplug apply only at the socket level? If the CPU is
multi-core, what is responsible for hot-plugging all cores present in
the socket?
I can answer this: the SMM handler would interact with the hotplug
controller in the same way that ACPI DSDT does normally. This supports
multiple hotplugs already.

Writes to the hotplug controller from outside SMM would be ignored.

(03) New CPU: (Flash) send board message to tell host CPU (GPIO->SCI)
-- I am waiting for hot-add message.
Maybe we can simplify this in QEMU by broadcasting an SMI to existent
processors immediately upon plugging the new CPU.
The QEMU DSDT could be modified (when secure boot is in effect) to OUT
to 0xB2 when hotplug happens. It could write a well-known value to
0xB2, to be read by an SMI handler in edk2.



(NOTE: Host CPU can
only
send
instruction in SMM mode. -- The register is SMM only)
Sorry, I don't follow -- what register are we talking about here, and
why is the BSP needed to send anything at all? What "instruction" do you
have in mind?
[Jiewen] The new CPU does not enable SMI at reset.
At some point of time later, the CPU need enable SMI, right?
The "instruction" here means, the host CPUs need tell to CPU to enable
SMI.

Right, this would be a write to the CPU hotplug controller

(04) Host CPU: (OS) get message from board that a new CPU is added.
(GPIO -> SCI)

(05) Host CPU: (OS) All CPUs enter SMM (SCI->SWSMI) (NOTE: New CPU
will not enter CPU because SMI is disabled)
I don't understand the OS involvement here. But, again, perhaps QEMU
can
force all existent CPUs into SMM immediately upon adding the new CPU.
[Jiewen] OS here means the Host CPU running code in OS environment, not
in SMM environment.

See above.

(06) Host CPU: (SMM) Save 38000, Update 38000 -- fill simple SMM
rebase code.

(07) Host CPU: (SMM) Send message to New CPU to Enable SMI.
Aha, so this is the SMM-only register you mention in step (03). Is the
register specified in the Intel SDM?
[Jiewen] Right. That is the register to let host CPU tell new CPU to enable
SMI.
It is platform specific register. Not defined in SDM.
You may invent one in device model.
See above.

(10) New CPU: (SMM) Response first SMI at 38000, and rebase SMBASE
to
TSEG.
What code does the new CPU execute after it completes step (10)? Does
it
halt?
[Jiewen] The new CPU exits SMM and return to original place - where it is
interrupted to enter SMM - running code on the flash.
So in our case we'd need an INIT/SIPI/SIPI sequence between (06) and (07).

(11) Host CPU: (SMM) Restore 38000.
These steps (i.e., (06) through (11)) don't appear RAS-specific. The
only platform-specific feature seems to be SMI masking register, which
could be extracted into a new SmmCpuFeaturesLib API.

Thus, would you please consider open sourcing firmware code for steps
(06) through (11)?

Alternatively -- and in particular because the stack for step (01)
concerns me --, we could approach this from a high-level, functional
perspective. The states that really matter are the relocated SMBASE for
the new CPU, and the state of the full system, right at the end of step
(11).

When the SMM setup quiesces during normal firmware boot, OVMF could
use
existent (finalized) SMBASE infomation to *pre-program* some virtual
QEMU hardware, with such state that would be expected, as "final" state,
of any new hotplugged CPU. Afterwards, if / when the hotplug actually
happens, QEMU could blanket-apply this state to the new CPU, and
broadcast a hardware SMI to all CPUs except the new one.
I'd rather avoid this and stay as close as possible to real hardware.

Paolo


Re: [PATCH v5 32/35] OvmfPkg: Introduce PcdXenGrantFrames

Laszlo Ersek
 

On 08/15/19 11:40, Laszlo Ersek wrote:
On 08/13/19 13:31, Anthony PERARD wrote:
Introduce PcdXenGrantFrames to replace a define in XenBusDxe and allow
the same value to be used in a different module.

The reason for the number of page to be 4 doesn't exist anymore, so
simply remove the comment.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
v5:
- add missing PcdLib to [LibraryClasses]
Yes, that's for

http://mid.mail-archive.com/365f2b95-b6c9-03cf-5346-5e1192bfa437@redhat.com
https://edk2.groups.io/g/devel/message/44621

Thanks for it,
Laszlo

v4:
- new patch
Also thanks for adding the v4 note retro-actively.

Thanks
Laszlo

OvmfPkg/OvmfPkg.dec | 3 +++
OvmfPkg/XenBusDxe/XenBusDxe.inf | 3 +++
OvmfPkg/XenBusDxe/XenBusDxe.h | 1 +
OvmfPkg/XenBusDxe/GrantTable.c | 3 +--
4 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 04d5e29272..d5fee805ef 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -225,6 +225,9 @@ [PcdsFixedAtBuild]
gUefiOvmfPkgTokenSpaceGuid.PcdXenPvhStartOfDayStructPtr|0x0|UINT32|0x17
gUefiOvmfPkgTokenSpaceGuid.PcdXenPvhStartOfDayStructPtrSize|0x0|UINT32|0x32

+ ## Number of page frames to use for storing grant table entries.
+ gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames|4|UINT32|0x33
+
[PcdsDynamic, PcdsDynamicEx]
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.inf b/OvmfPkg/XenBusDxe/XenBusDxe.inf
index 86e0fb8224..536b49fa8c 100644
--- a/OvmfPkg/XenBusDxe/XenBusDxe.inf
+++ b/OvmfPkg/XenBusDxe/XenBusDxe.inf
@@ -51,6 +51,7 @@ [LibraryClasses]
XenHypercallLib
SynchronizationLib
PrintLib
+ PcdLib

[Protocols]
gEfiDriverBindingProtocolGuid
@@ -59,3 +60,5 @@ [Protocols]
gXenBusProtocolGuid
gXenIoProtocolGuid

+[FixedPcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames
diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.h b/OvmfPkg/XenBusDxe/XenBusDxe.h
index 8510361bca..b1dcc3549c 100644
--- a/OvmfPkg/XenBusDxe/XenBusDxe.h
+++ b/OvmfPkg/XenBusDxe/XenBusDxe.h
@@ -22,6 +22,7 @@
#include <Library/UefiLib.h>
#include <Library/DevicePathLib.h>
#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>


//
diff --git a/OvmfPkg/XenBusDxe/GrantTable.c b/OvmfPkg/XenBusDxe/GrantTable.c
index 6575e9b88c..1130404cd1 100644
--- a/OvmfPkg/XenBusDxe/GrantTable.c
+++ b/OvmfPkg/XenBusDxe/GrantTable.c
@@ -22,8 +22,7 @@

#define NR_RESERVED_ENTRIES 8

-/* NR_GRANT_FRAMES must be less than or equal to that configured in Xen */
-#define NR_GRANT_FRAMES 4
+#define NR_GRANT_FRAMES (FixedPcdGet32 (PcdXenGrantFrames))
#define NR_GRANT_ENTRIES (NR_GRANT_FRAMES * EFI_PAGE_SIZE / sizeof(grant_entry_v1_t))

STATIC grant_entry_v1_t *GrantTable = NULL;


Re: [PATCH v5 33/35] OvmfPkg: Introduce XenIoPvhDxe to initialize Grant Tables

Laszlo Ersek
 

On 08/13/19 13:31, Anthony PERARD wrote:
XenIoPvhDxe use XenIoMmioLib to reserve some space to be use by the
Grant Tables.

The call is only done if it is necessary, we simply detect if the
guest is PVH, as in this case there is currently no PCI bus, and no
PCI Xen platform device which would start the XenIoPciDxe and allocate
the space for the Grant Tables.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
v5:
- add missing PcdLib as #include and in [LibraryClasses]
http://mid.mail-archive.com/8a5d9e63-12f1-2ff8-7c40-773d15baf333@redhat.com
https://edk2.groups.io/g/devel/message/44622

The update looks good, thanks!
Laszlo

v4:
- Removed XenIoPvhDxeNotifyExitBoot() which was doing action that should
not be done in an ExitBootServices notification.
(InitializeXenIoPvhDxe() has been cleaned up following this.)
- Use new PcdXenGrantFrames.
- Some coding style fix
- Update Maintainers.txt

v3:
- downgrade type to DXE_DRIVER
- use SPDX
- rework InitializeXenIoPvhDxe, and handle errors properly.
- Free the reserved allocation in ExitBootServices even if the XenIo
protocol could successfully been uninstalled.

v2:
- do allocation in EntryPoint like the other user of XenIoMmioLib.
- allocate memory instead of hardcoded addr.
- cleanup, add copyright
- detect if we are running in PVH mode

OvmfPkg/OvmfXen.dsc | 2 ++
OvmfPkg/OvmfXen.fdf | 1 +
OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf | 36 +++++++++++++++++++
OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c | 54 +++++++++++++++++++++++++++++
Maintainers.txt | 1 +
5 files changed, 94 insertions(+)
create mode 100644 OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
create mode 100644 OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c

diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index e719a168f8..5e07b37279 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -195,6 +195,7 @@ [LibraryClasses]
OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
+ XenIoMmioLib|OvmfPkg/Library/XenIoMmioLib/XenIoMmioLib.inf

Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
@@ -583,6 +584,7 @@ [Components]
NULL|OvmfPkg/Csm/LegacyBootMaintUiLib/LegacyBootMaintUiLib.inf
!endif
}
+ OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
OvmfPkg/XenBusDxe/XenBusDxe.inf
OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf
index 5c1a925d6a..517a492f14 100644
--- a/OvmfPkg/OvmfXen.fdf
+++ b/OvmfPkg/OvmfXen.fdf
@@ -309,6 +309,7 @@ [FV.DXEFV]
INF MdeModulePkg/Universal/Metronome/Metronome.inf
INF PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf

+INF OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
INF OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
INF OvmfPkg/XenBusDxe/XenBusDxe.inf
INF OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
diff --git a/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
new file mode 100644
index 0000000000..1c27f8aae0
--- /dev/null
+++ b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
@@ -0,0 +1,36 @@
+## @file
+# Driver for the XenIo protocol
+#
+# Copyright (c) 2019, Citrix Systems, Inc.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = XenIoPvhDxe
+ FILE_GUID = 7a567cc4-0e75-4d7a-a305-c3db109b53ad
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = InitializeXenIoPvhDxe
+
+[Packages]
+ MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[Sources]
+ XenIoPvhDxe.c
+
+[LibraryClasses]
+ MemoryAllocationLib
+ PcdLib
+ UefiDriverEntryPoint
+ XenIoMmioLib
+ XenPlatformLib
+
+[FixedPcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames
+
+[Depex]
+ TRUE
diff --git a/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c
new file mode 100644
index 0000000000..9264a85df1
--- /dev/null
+++ b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c
@@ -0,0 +1,54 @@
+/** @file
+
+ Driver for the XenIo protocol
+
+ This driver simply allocate space for the grant tables.
+
+ Copyright (c) 2019, Citrix Systems, Inc.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Library/MemoryAllocationLib.h>
+#include <Library/PcdLib.h>
+#include <Library/XenIoMmioLib.h>
+#include <Library/XenPlatformLib.h>
+
+EFI_STATUS
+EFIAPI
+InitializeXenIoPvhDxe (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ VOID *Allocation;
+ EFI_STATUS Status;
+ EFI_HANDLE XenIoHandle;
+
+ Allocation = NULL;
+ XenIoHandle = NULL;
+
+ if (!XenPvhDetected ()) {
+ return EFI_UNSUPPORTED;
+ }
+
+ Allocation = AllocateReservedPages (FixedPcdGet32 (PcdXenGrantFrames));
+ if (Allocation == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Error;
+ }
+
+ Status = XenIoMmioInstall (&XenIoHandle, (UINTN) Allocation);
+ if (EFI_ERROR (Status)) {
+ goto Error;
+ }
+
+ return EFI_SUCCESS;
+
+Error:
+ if (Allocation != NULL) {
+ FreePages (Allocation, FixedPcdGet32 (PcdXenGrantFrames));
+ }
+ return Status;
+}
diff --git a/Maintainers.txt b/Maintainers.txt
index 78e9f889ab..79defd13bf 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -382,6 +382,7 @@ F: OvmfPkg/PlatformPei/Xen.*
F: OvmfPkg/SmbiosPlatformDxe/*Xen.c
F: OvmfPkg/XenBusDxe/
F: OvmfPkg/XenIoPciDxe/
+F: OvmfPkg/XenIoPvhDxe/
F: OvmfPkg/XenPlatformPei/
F: OvmfPkg/XenPvBlkDxe/
F: OvmfPkg/XenResetVector/


Re: [PATCH v5 32/35] OvmfPkg: Introduce PcdXenGrantFrames

Laszlo Ersek
 

On 08/13/19 13:31, Anthony PERARD wrote:
Introduce PcdXenGrantFrames to replace a define in XenBusDxe and allow
the same value to be used in a different module.

The reason for the number of page to be 4 doesn't exist anymore, so
simply remove the comment.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
v5:
- add missing PcdLib to [LibraryClasses]
Yes, that's for

http://mid.mail-archive.com/365f2b95-b6c9-03cf-5346-5e1192bfa437@redhat.com
https://edk2.groups.io/g/devel/message/44621

Thanks for it,
Laszlo

v4:
- new patch

OvmfPkg/OvmfPkg.dec | 3 +++
OvmfPkg/XenBusDxe/XenBusDxe.inf | 3 +++
OvmfPkg/XenBusDxe/XenBusDxe.h | 1 +
OvmfPkg/XenBusDxe/GrantTable.c | 3 +--
4 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 04d5e29272..d5fee805ef 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -225,6 +225,9 @@ [PcdsFixedAtBuild]
gUefiOvmfPkgTokenSpaceGuid.PcdXenPvhStartOfDayStructPtr|0x0|UINT32|0x17
gUefiOvmfPkgTokenSpaceGuid.PcdXenPvhStartOfDayStructPtrSize|0x0|UINT32|0x32

+ ## Number of page frames to use for storing grant table entries.
+ gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames|4|UINT32|0x33
+
[PcdsDynamic, PcdsDynamicEx]
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.inf b/OvmfPkg/XenBusDxe/XenBusDxe.inf
index 86e0fb8224..536b49fa8c 100644
--- a/OvmfPkg/XenBusDxe/XenBusDxe.inf
+++ b/OvmfPkg/XenBusDxe/XenBusDxe.inf
@@ -51,6 +51,7 @@ [LibraryClasses]
XenHypercallLib
SynchronizationLib
PrintLib
+ PcdLib

[Protocols]
gEfiDriverBindingProtocolGuid
@@ -59,3 +60,5 @@ [Protocols]
gXenBusProtocolGuid
gXenIoProtocolGuid

+[FixedPcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames
diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.h b/OvmfPkg/XenBusDxe/XenBusDxe.h
index 8510361bca..b1dcc3549c 100644
--- a/OvmfPkg/XenBusDxe/XenBusDxe.h
+++ b/OvmfPkg/XenBusDxe/XenBusDxe.h
@@ -22,6 +22,7 @@
#include <Library/UefiLib.h>
#include <Library/DevicePathLib.h>
#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>


//
diff --git a/OvmfPkg/XenBusDxe/GrantTable.c b/OvmfPkg/XenBusDxe/GrantTable.c
index 6575e9b88c..1130404cd1 100644
--- a/OvmfPkg/XenBusDxe/GrantTable.c
+++ b/OvmfPkg/XenBusDxe/GrantTable.c
@@ -22,8 +22,7 @@

#define NR_RESERVED_ENTRIES 8

-/* NR_GRANT_FRAMES must be less than or equal to that configured in Xen */
-#define NR_GRANT_FRAMES 4
+#define NR_GRANT_FRAMES (FixedPcdGet32 (PcdXenGrantFrames))
#define NR_GRANT_ENTRIES (NR_GRANT_FRAMES * EFI_PAGE_SIZE / sizeof(grant_entry_v1_t))

STATIC grant_entry_v1_t *GrantTable = NULL;


Re: [PATCH v5 31/35] OvmfPkg/PlatformBootManagerLib: Use a Xen console for ConOut/ConIn

Laszlo Ersek
 

On 08/13/19 13:31, Anthony PERARD wrote:
On a Xen PVH guest, none of the existing serial or console interface
works, so we add a new one, based on XenConsoleSerialPortLib, and
implemented via SerialDxe.

That is a simple console implementation that can work on both PVH
guest and HVM guests, even if it is rarely going to be used on HVM.

Have PlatformBootManagerLib look for the new console, when running as a
Xen guest.

Since we use VENDOR_UART_DEVICE_PATH, fix its description and coding
style.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
v5:
- fix typos in commit message.
Thanks for those fixes, my R-b stands.

Laszlo

v4:
- instead of creating a new XEN_CONSOLE_DEVICE_PATH, use the existing
VENDOR_UART_DEVICE_PATH. And explain why VENDOR_UART_DEVICE_PATH
changed in the commit message.

v3:
- removed PciSioSerialDxe and IsaSerialDxe from OvmfXen, since they
would not be used, maybe, to check.
- some coding style fix

- not changed: PciSioSerialDxe: even if we add SerialDxe, we still needs
PciSioSerialDxe to have OVMF use the emulated serial port on HVM.

v2:
- Use MdeModulePkg/Universal/SerialDxe instead of something new.
- Have PlatformInitializeConsole() look for it by using the
known-in-advance device path for the xen console in the
PLATFORM_CONSOLE_CONNECT_ENTRY.

OvmfPkg/OvmfXen.dsc | 4 ++
OvmfPkg/OvmfXen.fdf | 1 +
.../PlatformBootManagerLib.inf | 4 ++
.../PlatformBootManagerLib/BdsPlatform.h | 1 +
.../PlatformBootManagerLib/BdsPlatform.c | 3 +-
.../PlatformBootManagerLib/PlatformData.c | 49 +++++++++++++++++--
6 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 54ac910d8e..e719a168f8 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -586,6 +586,10 @@ [Components]
OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
OvmfPkg/XenBusDxe/XenBusDxe.inf
OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+ MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {
+ <LibraryClasses>
+ SerialPortLib|OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf
+ }
MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf
index fa0830a324..5c1a925d6a 100644
--- a/OvmfPkg/OvmfXen.fdf
+++ b/OvmfPkg/OvmfXen.fdf
@@ -312,6 +312,7 @@ [FV.DXEFV]
INF OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
INF OvmfPkg/XenBusDxe/XenBusDxe.inf
INF OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+INF MdeModulePkg/Universal/SerialDxe/SerialDxe.inf

INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
INF MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index 04d614cd49..f89cce1879 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -61,6 +61,10 @@ [Pcd]
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate ## CONSUMES
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits ## CONSUMES
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity ## CONSUMES
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits ## CONSUMES

[Pcd.IA32, Pcd.X64]
gEfiMdePkgTokenSpaceGuid.PcdFSBClock
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
index 49a072b400..153e215101 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
@@ -165,6 +165,7 @@ typedef struct {
#define CONSOLE_IN BIT1
#define STD_ERROR BIT2
extern PLATFORM_CONSOLE_CONNECT_ENTRY gPlatformConsole[];
+extern PLATFORM_CONSOLE_CONNECT_ENTRY gXenPlatformConsole[];

//
// Platform BDS Functions
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 1eba304f09..70df6b841a 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -398,7 +398,8 @@ PlatformBootManagerBeforeConsole (
//
EfiBootManagerDispatchDeferredImages ();

- PlatformInitializeConsole (gPlatformConsole);
+ PlatformInitializeConsole (
+ XenDetected() ? gXenPlatformConsole : gPlatformConsole);
PcdStatus = PcdSet16S (PcdPlatformBootTimeOut,
GetFrontPageTimeoutFromQemu ());
ASSERT_RETURN_ERROR (PcdStatus);
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
index 36aab784d7..2858c3dfd5 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
@@ -9,18 +9,19 @@

#include "BdsPlatform.h"
#include <Guid/QemuRamfb.h>
+#include <Guid/SerialPortLibVendor.h>

//
-// Debug Agent UART Device Path structure
+// Vendor UART Device Path structure
//
-#pragma pack(1)
+#pragma pack (1)
typedef struct {
VENDOR_DEVICE_PATH VendorHardware;
UART_DEVICE_PATH Uart;
VENDOR_DEVICE_PATH TerminalType;
EFI_DEVICE_PATH_PROTOCOL End;
} VENDOR_UART_DEVICE_PATH;
-#pragma pack()
+#pragma pack ()

//
// USB Keyboard Device Path structure
@@ -141,6 +142,37 @@ STATIC VENDOR_RAMFB_DEVICE_PATH gQemuRamfbDevicePath = {
gEndEntire
};

+STATIC VENDOR_UART_DEVICE_PATH gXenConsoleDevicePath = {
+ {
+ {
+ HARDWARE_DEVICE_PATH,
+ HW_VENDOR_DP,
+ {
+ (UINT8) (sizeof (VENDOR_DEVICE_PATH)),
+ (UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8)
+ }
+ },
+ EDKII_SERIAL_PORT_LIB_VENDOR_GUID
+ },
+ {
+ {
+ MESSAGING_DEVICE_PATH,
+ MSG_UART_DP,
+ {
+ (UINT8) (sizeof (UART_DEVICE_PATH)),
+ (UINT8) ((sizeof (UART_DEVICE_PATH)) >> 8)
+ }
+ },
+ 0,
+ FixedPcdGet64 (PcdUartDefaultBaudRate),
+ FixedPcdGet8 (PcdUartDefaultDataBits),
+ FixedPcdGet8 (PcdUartDefaultParity),
+ FixedPcdGet8 (PcdUartDefaultStopBits),
+ },
+ gPcAnsiTerminal,
+ gEndEntire
+};
+
//
// Predefined platform default console device path
//
@@ -163,6 +195,17 @@ PLATFORM_CONSOLE_CONNECT_ENTRY gPlatformConsole[] = {
}
};

+PLATFORM_CONSOLE_CONNECT_ENTRY gXenPlatformConsole[] = {
+ {
+ (EFI_DEVICE_PATH_PROTOCOL *)&gXenConsoleDevicePath,
+ (CONSOLE_OUT | CONSOLE_IN | STD_ERROR)
+ },
+ {
+ NULL,
+ 0
+ }
+};
+
//
// Predefined platform connect sequence
//


Re: [PATCH v5 23/35] OvmfPkg/XenPlatformPei: Rework memory detection

Laszlo Ersek
 

On 08/13/19 13:31, Anthony PERARD wrote:
When running as a Xen PVH guest, there is no CMOS to read the memory
size from. Rework GetSystemMemorySize(Below|Above)4gb() so they can
work without CMOS by reading the e820 table.

Rework XenPublishRamRegions to also care for the reserved and ACPI
entry in the e820 table. The region that was added by InitializeXen()
isn't needed as that same entry is in the e820 table provided by
hvmloader.

MTRR settings aren't modified anymore, on HVM it's already done by
hvmloader, on PVH it is supposed to have sane default. MTRR will need
to be done properly but keeping what's already been done by programs
that have run before OVMF will do for now.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
v5:
- fix coding style
- fix typo in commit message
- Handle all possible cases of a E820 reserved range overlapping with the
LAPIC range.

v4:
- some coding style
- Added AddReservedMemoryRangeHob, and using it.
- this patch now replace "OvmfPkg/XenPlatformPei: Reserve hvmloader's memory only when it has run"
from v3. hvmloader have added an entry in the e820 table, there is no
need for a special case.
- now, everything that is in the e820 table is added to OVMF's memory
map, no more skipping ACPI entries or hvmloader's reserved entries.
Instead, we look for the local APIC region and avoid it if it is
present in the e820.
- rework commit message

OvmfPkg/XenPlatformPei/Platform.h | 13 ++++++
OvmfPkg/XenPlatformPei/MemDetect.c | 69 +++++++++++++++++++++++++++
OvmfPkg/XenPlatformPei/Platform.c | 11 +++++
OvmfPkg/XenPlatformPei/Xen.c | 75 +++++++++++++++++++++---------
4 files changed, 147 insertions(+), 21 deletions(-)

diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
index db9a62572f..7661f4a8de 100644
--- a/OvmfPkg/XenPlatformPei/Platform.h
+++ b/OvmfPkg/XenPlatformPei/Platform.h
@@ -44,6 +44,13 @@ AddReservedMemoryBaseSizeHob (
BOOLEAN Cacheable
);

+VOID
+AddReservedMemoryRangeHob (
+ EFI_PHYSICAL_ADDRESS MemoryBase,
+ EFI_PHYSICAL_ADDRESS MemoryLimit,
+ BOOLEAN Cacheable
+ );
+
VOID
AddressWidthInitialization (
VOID
@@ -114,6 +121,12 @@ XenPublishRamRegions (
VOID
);

+EFI_STATUS
+XenGetE820Map (
+ EFI_E820_ENTRY64 **Entries,
+ UINT32 *Count
+ );
+
extern EFI_BOOT_MODE mBootMode;

extern UINT8 mPhysMemAddressWidth;
diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c b/OvmfPkg/XenPlatformPei/MemDetect.c
index cf95f9c474..1f81eee407 100644
--- a/OvmfPkg/XenPlatformPei/MemDetect.c
+++ b/OvmfPkg/XenPlatformPei/MemDetect.c
@@ -96,6 +96,45 @@ Q35TsegMbytesInitialization (
mQ35TsegMbytes = ExtendedTsegMbytes;
}

+STATIC
+UINT64
+GetHighestSystemMemoryAddress (
+ BOOLEAN Below4gb
+ )
+{
+ EFI_E820_ENTRY64 *E820Map;
+ UINT32 E820EntriesCount;
+ EFI_E820_ENTRY64 *Entry;
+ EFI_STATUS Status;
+ UINT32 Loop;
+ UINT64 HighestAddress;
+ UINT64 EntryEnd;
+
+ HighestAddress = 0;
+
+ Status = XenGetE820Map (&E820Map, &E820EntriesCount);
+ ASSERT_EFI_ERROR (Status);
+
+ for (Loop = 0; Loop < E820EntriesCount; Loop++) {
+ Entry = E820Map + Loop;
+ EntryEnd = Entry->BaseAddr + Entry->Length;
+
+ if (Entry->Type == EfiAcpiAddressRangeMemory &&
+ EntryEnd > HighestAddress) {
+
+ if (Below4gb && (EntryEnd <= BASE_4GB)) {
+ HighestAddress = EntryEnd;
+ } else if (!Below4gb && (EntryEnd >= BASE_4GB)) {
+ HighestAddress = EntryEnd;
+ }
+ }
+ }
+
+ //
+ // Round down the end address.
+ //
+ return HighestAddress & ~(UINT64)EFI_PAGE_MASK;
+}

UINT32
GetSystemMemorySizeBelow4gb (
@@ -105,6 +144,19 @@ GetSystemMemorySizeBelow4gb (
UINT8 Cmos0x34;
UINT8 Cmos0x35;

+ //
+ // In PVH case, there is no CMOS, we have to calculate the memory size
+ // from parsing the E820
+ //
+ if (XenPvhDetected ()) {
+ UINT64 HighestAddress;
+
+ HighestAddress = GetHighestSystemMemoryAddress (TRUE);
+ ASSERT (HighestAddress > 0 && HighestAddress <= BASE_4GB);
+
+ return HighestAddress;
+ }
+
//
// CMOS 0x34/0x35 specifies the system memory above 16 MB.
// * CMOS(0x35) is the high byte
@@ -129,6 +181,23 @@ GetSystemMemorySizeAbove4gb (
UINT32 Size;
UINTN CmosIndex;

+ //
+ // In PVH case, there is no CMOS, we have to calculate the memory size
+ // from parsing the E820
+ //
+ if (XenPvhDetected ()) {
+ UINT64 HighestAddress;
+
+ HighestAddress = GetHighestSystemMemoryAddress (FALSE);
+ ASSERT (HighestAddress == 0 || HighestAddress >= BASE_4GB);
+
+ if (HighestAddress >= BASE_4GB) {
+ HighestAddress -= BASE_4GB;
+ }
+
+ return HighestAddress;
+ }
+
//
// CMOS 0x5b-0x5d specifies the system memory above 4GB MB.
// * CMOS(0x5d) is the most significant size byte
diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c
index 6aaafc3ee9..2f42ca6ccd 100644
--- a/OvmfPkg/XenPlatformPei/Platform.c
+++ b/OvmfPkg/XenPlatformPei/Platform.c
@@ -102,6 +102,17 @@ AddReservedMemoryBaseSizeHob (
);
}

+VOID
+AddReservedMemoryRangeHob (
+ EFI_PHYSICAL_ADDRESS MemoryBase,
+ EFI_PHYSICAL_ADDRESS MemoryLimit,
+ BOOLEAN Cacheable
+ )
+{
+ AddReservedMemoryBaseSizeHob (MemoryBase,
+ (UINT64)(MemoryLimit - MemoryBase), Cacheable);
+}
+
VOID
AddIoMemoryRangeHob (
EFI_PHYSICAL_ADDRESS MemoryBase,
diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
index 72f6f37b46..c4506def9a 100644
--- a/OvmfPkg/XenPlatformPei/Xen.c
+++ b/OvmfPkg/XenPlatformPei/Xen.c
@@ -276,9 +276,14 @@ XenPublishRamRegions (
VOID
)
{
- EFI_E820_ENTRY64 *E820Map;
- UINT32 E820EntriesCount;
- EFI_STATUS Status;
+ EFI_E820_ENTRY64 *E820Map;
+ UINT32 E820EntriesCount;
+ EFI_STATUS Status;
+ EFI_E820_ENTRY64 *Entry;
+ UINTN Index;
+ UINT64 LapicBase;
+ UINT64 LapicEnd;
+
The extra newline is not really justified, but it's also harmless and not too annoying.

Otherwise, the update looks good to me, addressing:

* http://mid.mail-archive.com/7b8cb5ef-2163-9e22-350f-877be6951b34@redhat.com
https://edk2.groups.io/g/devel/message/44615

* http://mid.mail-archive.com/20190807152852.e47kogsxubbjkue5@Air-de-Roger
https://edk2.groups.io/g/devel/message/45012

So my ACK stands.

Thanks
Laszlo

DEBUG ((DEBUG_INFO, "Using memory map provided by Xen\n"));

@@ -287,26 +292,60 @@ XenPublishRamRegions (
//
E820EntriesCount = 0;
Status = XenGetE820Map (&E820Map, &E820EntriesCount);
-
ASSERT_EFI_ERROR (Status);

- if (E820EntriesCount > 0) {
- EFI_E820_ENTRY64 *Entry;
- UINT32 Loop;
+ LapicBase = PcdGet32 (PcdCpuLocalApicBaseAddress);
+ LapicEnd = LapicBase + SIZE_1MB;
+ AddIoMemoryRangeHob (LapicBase, LapicEnd);

- for (Loop = 0; Loop < E820EntriesCount; Loop++) {
- Entry = E820Map + Loop;
+ for (Index = 0; Index < E820EntriesCount; Index++) {
+ UINT64 Base;
+ UINT64 End;
+ UINT64 ReservedBase;
+ UINT64 ReservedEnd;

+ Entry = &E820Map[Index];
+
+ //
+ // Round up the start address, and round down the end address.
+ //
+ Base = ALIGN_VALUE (Entry->BaseAddr, (UINT64)EFI_PAGE_SIZE);
+ End = (Entry->BaseAddr + Entry->Length) & ~(UINT64)EFI_PAGE_MASK;
+
+ switch (Entry->Type) {
+ case EfiAcpiAddressRangeMemory:
+ AddMemoryRangeHob (Base, End);
+ break;
+ case EfiAcpiAddressRangeACPI:
+ AddReservedMemoryRangeHob (Base, End, FALSE);
+ break;
+ case EfiAcpiAddressRangeReserved:
//
- // Only care about RAM
+ // hvmloader marks a range that overlaps with the local APIC memory
+ // mapped region as reserved, but CpuDxe wants it as mapped IO. We
+ // have already added it as mapped IO, so skip it here.
//
- if (Entry->Type != EfiAcpiAddressRangeMemory) {
- continue;
- }

- AddMemoryBaseSizeHob (Entry->BaseAddr, Entry->Length);
+ //
+ // add LAPIC predecessor range, if any
+ //
+ ReservedBase = Base;
+ ReservedEnd = MIN (End, LapicBase);
+ if (ReservedBase < ReservedEnd) {
+ AddReservedMemoryRangeHob (ReservedBase, ReservedEnd, FALSE);
+ }

- MtrrSetMemoryAttribute (Entry->BaseAddr, Entry->Length, CacheWriteBack);
+ //
+ // add LAPIC successor range, if any
+ //
+ ReservedBase = MAX (Base, LapicEnd);
+ ReservedEnd = End;
+ if (ReservedBase < ReservedEnd) {
+ AddReservedMemoryRangeHob (ReservedBase, ReservedEnd, FALSE);
+ }
+ break;
+ default:
+ break;
}
}
}
@@ -326,12 +365,6 @@ InitializeXen (
{
RETURN_STATUS PcdStatus;

- //
- // Reserve away HVMLOADER reserved memory [0xFC000000,0xFD000000).
- // This needs to match HVMLOADER RESERVED_MEMBASE/RESERVED_MEMSIZE.
- //
- AddReservedMemoryBaseSizeHob (0xFC000000, 0x1000000, FALSE);
-
PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, TRUE);
ASSERT_RETURN_ERROR (PcdStatus);


Re: [PATCH V2 00/10] Multiple Controllers Support solution

Liming Gao
 

Eric:
I push the change for edk2platform.
And, I push the remaining patches a6ee24fbddcd901347014e045611eb6108f4f741..a5944b6a13e227da23daa0ab59b5d6f4b06bb49b

Thanks
Liming

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Liming Gao
Sent: Wednesday, August 14, 2019 12:38 PM
To: Jin, Eric <eric.jin@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH V2 00/10] Multiple Controllers Support
solution

Eric:
I push the first 3 patches
82407bd129dca8ec6d96e85f541b0974c8d7e087..1f06aa24c29405f271f514f01c3
96c2ba19c1370.
Then, the changes in edk2 platform can be submitted. After edk2platform
change is ready, I will push the remaining patch set.

Thanks
Liming
-----Original Message-----
From: Jin, Eric
Sent: Monday, August 12, 2019 11:17 PM
To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io
Cc: Jin, Eric <eric.jin@intel.com>
Subject: RE: [edk2-devel] [PATCH V2 00/10] Multiple Controllers Support
solution

Liming,

Thank you for comment.
We will update GetLowestSupportedVersion() API function header to
include
Private param when we push the code to repo. Thanks.

Best Regards
Eric

-----Original Message-----
From: Gao, Liming
Sent: Monday, August 12, 2019 5:24 PM
To: Jin, Eric <eric.jin@intel.com>; devel@edk2.groups.io
Subject: RE: [edk2-devel] [PATCH V2 00/10] Multiple Controllers Support
solution

That's good information.

In patch 5, GetLowestSupportedVersion() API function header should be
updated to include Private param. I have no other comments.

With this change, Reviewed-by: Liming Gao <liming.gao@intel.com>

///
@@ -193,7 +200,7 @@ GetImageTypeNameString ( **/
UINT32
GetLowestSupportedVersion (
- VOID
+ FIRMWARE_MANAGEMENT_PRIVATE_DATA *Private
)

Thanks
Liming
-----Original Message-----
From: Jin, Eric
Sent: Monday, August 12, 2019 3:14 PM
To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io
Subject: RE: [edk2-devel] [PATCH V2 00/10] Multiple Controllers Support
solution

Liming,

The differences between V2 and V1 are listed below.

1) The series is composed of 10 patches in V2 (14 in V1). patch 14 is
merged into patch 4, and patch 11/12/13 are merged into patch 5.
2) Try to fix the issue exposed by ECC.

Btw, the patch of edk2-platform
Platform\Intel\Vlv2TbltDevicePkg\Feature\Capsule\Library\FmpDeviceLib
to support new APIs is provided in separated patch
https://edk2.groups.io/g/devel/message/45328

Best Regards
Eric

-----Original Message-----
From: Gao, Liming
Sent: Monday, August 12, 2019 1:19 PM
To: devel@edk2.groups.io; Jin, Eric <eric.jin@intel.com>
Subject: RE: [edk2-devel] [PATCH V2 00/10] Multiple Controllers Support
solution

Eric:
Can you list the difference compared to version 1?

Thanks
Liming
-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Eric Jin
Sent: Monday, August 12, 2019 9:46 AM
To: devel@edk2.groups.io
Subject: [edk2-devel] [PATCH V2 00/10] Multiple Controllers Support
solution

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1525

The patch set is to support drivers that manage multiple controllers
and also provide a firmware update capability to each managed controller.

The following modules are related to Multiple Controllers Support
solution

FmpDevicePkg\FmpDxe\FmpDxe.inf - Driver to manage multiple
controllers
and provide the firmware update capability to each managed controller.
FmpDevicePkg\CapsuleUpdatePolicyDxe\CapsuleUpdatePolicyDxe.inf -
Driver
to produce the Capsule Update Policy Protocol using the services of
the CapsuleUpdatePolicyLib class. The protocol is a private interface
to the FmpDevicePkg
FmpDevicePkg\Library\CapsuleUpdatePolicyLibOnProtocol\CapsuleUpdat
e
P
o
licyLibOnProtocol.inf -
CapsuleUpdatePolicyLib instance that uses the services of the Capsule
Update Policy Protocol produced by CapsuleUpdatePolicyDxe
FmpDevicePkg\Library\CapsuleUpdatePolicyLibNull\CapsuleUpdatePolicy
Li
b
N
ull.inf -
Null CapsuleUpdatePolicyLib instance and the template for platform
specific implementation
FmpDevicePkg\Library\FmpDeviceLibNull\FmpDeviceLibNull.inf - Null
FmpDeviceLib instance and the template for platform specific
implementation

Eric Jin (10):
FmpDevicePkg: Add UEFI_DRIVER support
FmpDevicePkg: Add APIs to FmpDeviceLib
FmpDEvicePkg/FmpDeviceLibNull: Implement new APIs
FmpDevicePkg/FmpDxe: Use new FmpDeviceLib APIs
FmpDevicePkg/FmpDxe: Different variable for each FMP Descriptor
FmpDevicePkg: Add Capsule Update Policy Protocol
FmpDevicePkg/FmpDxe: Improve all DEBUG() messages
FmpDevicePkg/FmpDxe: Add PcdFmpDeviceImageTypeIdGuid
FmpDevicePkg/FmpDxe: Add PcdFmpDeviceStorageAccessEnable
FmpDevicePkg/FmpDxe: Remove use of CatSprint()

.../CapsuleUpdatePolicyDxe.c | 173 ++++
.../CapsuleUpdatePolicyDxe.h | 140 +++
.../CapsuleUpdatePolicyDxe.inf | 48 +
.../CapsuleUpdatePolicyDxe.uni | 14 +
.../CapsuleUpdatePolicyDxeExtra.uni | 14 +
FmpDevicePkg/FmpDevicePkg.dec | 43 +-
FmpDevicePkg/FmpDevicePkg.dsc | 64 +-
FmpDevicePkg/FmpDevicePkg.uni | 16 +-
FmpDevicePkg/FmpDxe/DetectTestKey.c | 16 +-
FmpDevicePkg/FmpDxe/FmpDxe.c | 792 ++++++++++------
FmpDevicePkg/FmpDxe/FmpDxe.h | 355 ++++++++
FmpDevicePkg/FmpDxe/FmpDxe.inf | 7 +-
FmpDevicePkg/FmpDxe/FmpDxeLib.inf | 7 +-
FmpDevicePkg/FmpDxe/VariableSupport.c | 844 +++++++++++++----
-
FmpDevicePkg/FmpDxe/VariableSupport.h | 135 ++-
FmpDevicePkg/Include/Library/FmpDeviceLib.h | 104 ++-
.../CapsuleUpdatePolicyLibOnProtocol.c | 171 ++++
.../CapsuleUpdatePolicyLibOnProtocol.inf | 40 +
.../CapsuleUpdatePolicyLibOnProtocol.uni | 15 +
.../Library/FmpDeviceLibNull/FmpDeviceLib.c | 93 +-
.../FmpDeviceLibNull/FmpDeviceLibNull.inf | 4 +-
.../FmpPayloadHeaderLibV1.inf | 4 +-
.../Library/FmpPayloadHeaderLib.h | 0
.../Protocol/CapsuleUpdatePolicy.h | 132 +++
24 files changed, 2644 insertions(+), 587 deletions(-) create mode
100644
FmpDevicePkg/CapsuleUpdatePolicyDxe/CapsuleUpdatePolicyDxe.c
create mode 100644
FmpDevicePkg/CapsuleUpdatePolicyDxe/CapsuleUpdatePolicyDxe.h
create mode 100644
FmpDevicePkg/CapsuleUpdatePolicyDxe/CapsuleUpdatePolicyDxe.inf
create mode 100644
FmpDevicePkg/CapsuleUpdatePolicyDxe/CapsuleUpdatePolicyDxe.uni
create mode 100644
FmpDevicePkg/CapsuleUpdatePolicyDxe/CapsuleUpdatePolicyDxeExtra.u
n
i
create mode 100644 FmpDevicePkg/FmpDxe/FmpDxe.h create mode
100644
FmpDevicePkg/Library/CapsuleUpdatePolicyLibOnProtocol/CapsuleUpdat
e
P
o
licyLibOnProtocol.c
create mode 100644
FmpDevicePkg/Library/CapsuleUpdatePolicyLibOnProtocol/CapsuleUpdat
e
P
o
licyLibOnProtocol.inf
create mode 100644
FmpDevicePkg/Library/CapsuleUpdatePolicyLibOnProtocol/CapsuleUpdat
e
P
o
licyLibOnProtocol.uni
rename FmpDevicePkg/{Include =>
PrivateInclude}/Library/FmpPayloadHeaderLib.h (100%) create mode
100644 FmpDevicePkg/PrivateInclude/Protocol/CapsuleUpdatePolicy.h

--
2.20.1.windows.1