Date   

[PATCH 1/1] OvmfPkg: Bhyve: Delete unused AcpiTables/Ssdt.asl file

Rebecca Cran
 

The Ssdt.asl file isn't used, so delete it.

Signed-off-by: Rebecca Cran <rebecca@...>
---
OvmfPkg/Bhyve/AcpiTables/Ssdt.asl | 15 ---------------
1 file changed, 15 deletions(-)

diff --git a/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl b/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl
deleted file mode 100644
index 175ab3b7e66d..000000000000
--- a/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl
+++ /dev/null
@@ -1,15 +0,0 @@
-/** @file
- Placeholder for runtime-generated objects.
-
- This empty table provides only a header for dynamic copying and extension,
- and a trigger for QemuInstallAcpiSsdtTable().
-
- Copyright (C) 2020, Rebecca Cran <rebecca@...>
- Copyright (C) 2012 Red Hat, Inc.
-
- SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-DefinitionBlock ("Ssdt.aml", "SSDT", 1, "REDHAT", "OVMF ", 1) {
-}
--
2.34.1


Re: [PATCH 1/1] OvmfPkg: Bhyve: Correct the SSDT Revision field

Rebecca Cran
 

On 12/21/21 09:31, Ard Biesheuvel wrote:
On Tue, 21 Dec 2021 at 16:06, Rebecca Cran <rebecca@...> wrote:
According to the ACPI Specification version 6.0 and newer, the SSDT
Revision field should be 2. Fix AcpiTables/Ssdt.aml.

Signed-off-by: Rebecca Cran <rebecca@...>
---
OvmfPkg/Bhyve/AcpiTables/Ssdt.asl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl b/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl
index 175ab3b7e66d..761b8267b9a5 100644
--- a/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl
+++ b/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl
@@ -11,5 +11,5 @@

**/

-DefinitionBlock ("Ssdt.aml", "SSDT", 1, "REDHAT", "OVMF ", 1) {
+DefinitionBlock ("Ssdt.aml", "SSDT", 2, "REDHAT", "OVMF ", 1) {
}
Wouldn't it be better to simply remove this file?
It's no longer used, so yes I'll remove it.


--

Rebecca Cran


Re: [edk2-platforms: PATCH] System will occur a CPU exception error when sorting CPU APIC map, because of a pointer point to an wrong space.

Kumar, Chandana C
 

Reviewed-by: Chandana C Kumar <chandana.c.kumar@...>

-----Original Message-----
From: Lin, JackX <jackx.lin@...>
Sent: Tuesday, December 21, 2021 12:18 PM
To: devel@edk2.groups.io
Cc: Lin, JackX <jackx.lin@...>; Lin, JackX <jackx.lin@...>; Chiu,
Chasel <chasel.chiu@...>; Dong, Eric <eric.dong@...>; Yao, Jiewen
<jiewen.yao@...>; Ni, Ray <ray.ni@...>; Chaganty, Rangasai V
<rangasai.v.chaganty@...>; Kuo, Donald <donald.kuo@...>; Kumar,
Chandana C <chandana.c.kumar@...>
Subject: [edk2-platforms: PATCH] System will occur a CPU exception error when
sorting CPU APIC map, because of a pointer point to an wrong space.

Signed-off-by: JackX Lin <JackX.Lin@...>
Cc: Chasel Chiu <chasel.chiu@...>
Cc: Dong Eric <eric.dong@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Ray Ni <ray.ni@...>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@...>
Cc: Donald Kuo <Donald.Kuo@...>
Cc: Chandana C Kumar <chandana.c.kumar@...>
Cc: JackX Lin <JackX.Lin@...>
---
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index 785cf4c2f9..05fc7799fb 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -169,7 +169,7 @@ SortCpuLocalApicInTable (
UINT32 Index;
UINT32 CurrProcessor;
UINT32 BspApicId;
- EFI_CPU_ID_ORDER_MAP *TempVal;
+ EFI_CPU_ID_ORDER_MAP TempVal;
EFI_CPU_ID_ORDER_MAP *CpuIdMapPtr;
UINT32 CoreThreadMask;
EFI_CPU_ID_ORDER_MAP *TempCpuApicIdOrderTable;
@@ -183,7 +183,6 @@ SortCpuLocalApicInTable (
}

TempCpuApicIdOrderTable = AllocateZeroPool (mNumberOfCpus * sizeof
(EFI_CPU_ID_ORDER_MAP));
- TempVal = AllocateZeroPool (sizeof (EFI_CPU_ID_ORDER_MAP));
CoreThreadMask = (UINT32) ((1 << mNumOfBitShift) - 1);

for (CurrProcessor = 0, Index = 0; CurrProcessor < mNumberOfCpus;
CurrProcessor++, Index++) {
--
2.32.0.windows.2


Re: [PATCH 1/1] OvmfPkg: Bhyve: Correct the SSDT Revision field

Ard Biesheuvel
 

On Tue, 21 Dec 2021 at 16:06, Rebecca Cran <rebecca@...> wrote:

According to the ACPI Specification version 6.0 and newer, the SSDT
Revision field should be 2. Fix AcpiTables/Ssdt.aml.

Signed-off-by: Rebecca Cran <rebecca@...>
---
OvmfPkg/Bhyve/AcpiTables/Ssdt.asl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl b/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl
index 175ab3b7e66d..761b8267b9a5 100644
--- a/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl
+++ b/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl
@@ -11,5 +11,5 @@

**/

-DefinitionBlock ("Ssdt.aml", "SSDT", 1, "REDHAT", "OVMF ", 1) {
+DefinitionBlock ("Ssdt.aml", "SSDT", 2, "REDHAT", "OVMF ", 1) {
}
Wouldn't it be better to simply remove this file?


ShellPkg: acpiview errors on Processor Local APIC struct in MADT

Rebecca Cran
 

acpiview is generating errors for the MADT table when it contains a Processor Local APIC entry:


ERROR: Unknown Interrupt Controller Structure, Type = 0, Length = 8


Looking at ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c:475 I see there's no case statement for EFI_ACPI_6_3_PROCESSOR_LOCAL_APIC.


--
Rebecca Cran


[PATCH edk2 v2 1/3] StandaloneMmPkg: Fix issue about SpPcpuSharedBufSize field

Ming Huang
 

TF-A: TrustedFirmware-A
SPM: Secure Partition Manager(MM)

In TF-A, the name of this field is sp_shared_buf_size. This field is
the size of range for transmit data from TF-A to standaloneMM when
SPM enable.

SpPcpuSharedBufSize is pass from TF-A while StandaloneMM initialize.
So, SpPcpuSharedBufSize should be rename to SpSharedBufSize and this field
should no multiply by PayloadBootInfo->NumCpus;

Signed-off-by: Ming Huang <huangming@...>
---
StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h | 2 +-
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c | 2 +-
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
index c44f7066c6..f1683ecb61 100644
--- a/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
+++ b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
@@ -41,7 +41,7 @@ typedef struct {
UINT64 SpPcpuStackSize;
UINT64 SpHeapSize;
UINT64 SpNsCommBufSize;
- UINT64 SpPcpuSharedBufSize;
+ UINT64 SpSharedBufSize;
UINT32 NumSpMemRegions;
UINT32 NumCpus;
EFI_SECURE_PARTITION_CPU_INFO *CpuInfo;
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
index 85f8194687..93773c9fe8 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
@@ -173,7 +173,7 @@ CreateHobListFromBootInfo (
// Base and size of buffer shared with privileged Secure world software
MmramRanges[1].PhysicalStart = PayloadBootInfo->SpSharedBufBase;
MmramRanges[1].CpuStart = PayloadBootInfo->SpSharedBufBase;
- MmramRanges[1].PhysicalSize = PayloadBootInfo->SpPcpuSharedBufSize * PayloadBootInfo->NumCpus;
+ MmramRanges[1].PhysicalSize = PayloadBootInfo->SpSharedBufSize;
MmramRanges[1].RegionState = EFI_CACHEABLE | EFI_ALLOCATED;

// Base and size of buffer used for synchronous communication with Normal
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
index 49cf51a789..5db7019dda 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
@@ -87,7 +87,7 @@ GetAndPrintBootinformation (
DEBUG ((DEBUG_INFO, "SpPcpuStackSize - 0x%x\n", PayloadBootInfo->SpPcpuStackSize));
DEBUG ((DEBUG_INFO, "SpHeapSize - 0x%x\n", PayloadBootInfo->SpHeapSize));
DEBUG ((DEBUG_INFO, "SpNsCommBufSize - 0x%x\n", PayloadBootInfo->SpNsCommBufSize));
- DEBUG ((DEBUG_INFO, "SpPcpuSharedBufSize - 0x%x\n", PayloadBootInfo->SpPcpuSharedBufSize));
+ DEBUG ((DEBUG_INFO, "SpSharedBufSize - 0x%x\n", PayloadBootInfo->SpSharedBufSize));

DEBUG ((DEBUG_INFO, "NumCpus - 0x%x\n", PayloadBootInfo->NumCpus));
DEBUG ((DEBUG_INFO, "CpuInfo - 0x%p\n", PayloadBootInfo->CpuInfo));
--
2.17.1


[PATCH edk2 v2 2/3] StandaloneMmPkg: Replace DEBUG_INFO with DEBUG_ERROR

Ming Huang
 

DEBUG_ERROR should be used in error branch.

Signed-off-by: Ming Huang <huangming@...>
---
StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c | 6 +++---
StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 12 ++++++------
2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
index 165d696f99..5dfaf9d751 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
@@ -95,7 +95,7 @@ PiMmStandaloneArmTfCpuDriverEntry (
//
if ((ARM_SMC_ID_MM_COMMUNICATE != EventId) &&
(ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ != EventId)) {
- DEBUG ((DEBUG_INFO, "UnRecognized Event - 0x%x\n", EventId));
+ DEBUG ((DEBUG_ERROR, "UnRecognized Event - 0x%x\n", EventId));
return EFI_INVALID_PARAMETER;
}

@@ -133,7 +133,7 @@ PiMmStandaloneArmTfCpuDriverEntry (
);

if (Status != EFI_SUCCESS) {
- DEBUG ((DEBUG_INFO, "Mem alloc failed - 0x%x\n", EventId));
+ DEBUG ((DEBUG_ERROR, "Mem alloc failed - 0x%x\n", EventId));
return EFI_OUT_OF_RESOURCES;
}

@@ -156,7 +156,7 @@ PiMmStandaloneArmTfCpuDriverEntry (
mMmst->CpuSaveState = NULL;

if (mMmEntryPoint == NULL) {
- DEBUG ((DEBUG_INFO, "Mm Entry point Not Found\n"));
+ DEBUG ((DEBUG_ERROR, "Mm Entry point Not Found\n"));
return EFI_UNSUPPORTED;
}

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
index 10097f792f..fd9c59b4da 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
@@ -143,7 +143,7 @@ StandaloneMmCpuInitialize (

// Bail out if the Hoblist could not be found
if (Index >= mMmst->NumberOfTableEntries) {
- DEBUG ((DEBUG_INFO, "Hoblist not found - 0x%x\n", Index));
+ DEBUG ((DEBUG_ERROR, "Hoblist not found - 0x%x\n", Index));
return EFI_OUT_OF_RESOURCES;
}

@@ -158,7 +158,7 @@ StandaloneMmCpuInitialize (
(VOID **) &CpuDriverEntryPointDesc
);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_INFO, "ArmTfCpuDriverEpDesc HOB data extraction failed - 0x%x\n", Status));
+ DEBUG ((DEBUG_ERROR, "ArmTfCpuDriverEpDesc HOB data extraction failed - 0x%x\n", Status));
return Status;
}

@@ -176,7 +176,7 @@ StandaloneMmCpuInitialize (
(VOID **) &NsCommBufMmramRange
);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_INFO, "NsCommBufMmramRange HOB data extraction failed - 0x%x\n", Status));
+ DEBUG ((DEBUG_ERROR, "NsCommBufMmramRange HOB data extraction failed - 0x%x\n", Status));
return Status;
}

@@ -195,7 +195,7 @@ StandaloneMmCpuInitialize (
(VOID **) &MpInformationHobData
);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_INFO, "MpInformationHob extraction failed - 0x%x\n", Status));
+ DEBUG ((DEBUG_ERROR, "MpInformationHob extraction failed - 0x%x\n", Status));
return Status;
}

@@ -213,7 +213,7 @@ StandaloneMmCpuInitialize (
(VOID **) &mMpInformationHobData
);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_INFO, "mMpInformationHobData mem alloc failed - 0x%x\n", Status));
+ DEBUG ((DEBUG_ERROR, "mMpInformationHobData mem alloc failed - 0x%x\n", Status));
return Status;
}

@@ -243,7 +243,7 @@ StandaloneMmCpuInitialize (
(VOID **) &PerCpuGuidedEventContext
);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_INFO, "PerCpuGuidedEventContext mem alloc failed - 0x%x\n", Status));
+ DEBUG ((DEBUG_ERROR, "PerCpuGuidedEventContext mem alloc failed - 0x%x\n", Status));
return Status;
}
return Status;
--
2.17.1


[PATCH edk2 v2 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A

Ming Huang
 

There are two scene communicate with StandaloneMm(MM):
1 edk2 -> TF-A -> MM, communicate MM use non-secure buffer which
specify by EFI_SECURE_PARTITION_BOOT_INFO.SpNsCommBufBase;
2 RAS scene: fiq -> TF-A -> MM, use secure buffer which
specify by EFI_SECURE_PARTITION_BOOT_INFO.SpShareBufBase;

For now, the second scene will failed because check buffer address.
This patch add CheckBufferAddr() to support check address for secure
buffer.

Signed-off-by: Ming Huang <huangming@...>
---
StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c | 59 +++++++++++++++-----
StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 +++++++
StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h | 1 +
3 files changed, 68 insertions(+), 13 deletions(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
index 5dfaf9d751..ba8639a26a 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
@@ -50,6 +50,7 @@ EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext = NULL;

// Descriptor with whereabouts of memory used for communication with the normal world
EFI_MMRAM_DESCRIPTOR mNsCommBuffer;
+EFI_MMRAM_DESCRIPTOR mSCommBuffer;

MP_INFORMATION_HOB_DATA *mMpInformationHobData;

@@ -60,6 +61,47 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = {

STATIC EFI_MM_ENTRY_POINT mMmEntryPoint = NULL;

+STATIC
+EFI_STATUS
+CheckBufferAddr (
+ IN UINTN CommBufferAddr
+ )
+{
+ UINTN CommBufferSize;
+ EFI_STATUS Status;
+ UINT64 NsCommBufferEnd;
+ UINT64 SCommBufferEnd;
+ UINT64 CommBufferEnd;
+
+ Status = EFI_SUCCESS;
+ NsCommBufferEnd = mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize;
+ SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize;
+
+ if ((CommBufferAddr >= mNsCommBuffer.PhysicalStart) &&
+ (CommBufferAddr < NsCommBufferEnd)) {
+ CommBufferEnd = NsCommBufferEnd;
+ } else if ((CommBufferAddr >= mSCommBuffer.PhysicalStart) &&
+ (CommBufferAddr <= SCommBufferEnd)) {
+ CommBufferEnd = SCommBufferEnd;
+ } else {
+ return EFI_ACCESS_DENIED;
+ }
+
+ if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {
+ Status = EFI_INVALID_PARAMETER;
+ }
+
+ // Find out the size of the buffer passed
+ CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
+ sizeof (EFI_MM_COMMUNICATE_HEADER);
+ // perform bounds check.
+ if (CommBufferAddr + CommBufferSize >= CommBufferEnd) {
+ Status = EFI_ACCESS_DENIED;
+ }
+
+ return Status;
+}
+
/**
The PI Standalone MM entry point for the TF-A CPU driver.

@@ -104,25 +146,16 @@ PiMmStandaloneArmTfCpuDriverEntry (
return EFI_INVALID_PARAMETER;
}

- if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
- return EFI_ACCESS_DENIED;
- }
-
- if ((NsCommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
- (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
- return EFI_INVALID_PARAMETER;
+ Status = CheckBufferAddr (NsCommBufferAddr);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Check Buffer failed: %r\n", Status));
+ return Status;
}

// Find out the size of the buffer passed
NsCommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) NsCommBufferAddr)->MessageLength +
sizeof (EFI_MM_COMMUNICATE_HEADER);

- // perform bounds check.
- if (NsCommBufferAddr + NsCommBufferSize >=
- mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
- return EFI_ACCESS_DENIED;
- }
-
GuidedEventContext = NULL;
// Now that the secure world can see the normal world buffer, allocate
// memory to copy the communication buffer to the secure world.
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
index fd9c59b4da..96dad20dd1 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
@@ -107,6 +107,7 @@ StandaloneMmCpuInitialize (
UINTN Index;
UINTN ArraySize;
VOID *HobStart;
+ EFI_MMRAM_HOB_DESCRIPTOR_BLOCK *MmramRangesHob;

ASSERT (SystemTable != NULL);
mMmst = SystemTable;
@@ -186,6 +187,26 @@ StandaloneMmCpuInitialize (
CopyMem (&mNsCommBuffer, NsCommBufMmramRange, sizeof(EFI_MMRAM_DESCRIPTOR));
DEBUG ((DEBUG_INFO, "mNsCommBuffer: 0x%016lx - 0x%lx\n", mNsCommBuffer.CpuStart, mNsCommBuffer.PhysicalSize));

+ Status = GetGuidedHobData (
+ HobStart,
+ &gEfiMmPeiMmramMemoryReserveGuid,
+ (VOID **) &MmramRangesHob
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "MmramRangesHob data extraction failed - 0x%x\n", Status));
+ return Status;
+ }
+
+ //
+ // As CreateHobListFromBootInfo(), the base and size of buffer shared with
+ // privileged Secure world software is in second one.
+ //
+ CopyMem (
+ &mSCommBuffer,
+ &MmramRangesHob->Descriptor[0] + 1,
+ sizeof(EFI_MMRAM_DESCRIPTOR)
+ );
+
//
// Extract the MP information from the Hoblist
//
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
index 2c96439c15..2e03b20d85 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
@@ -30,6 +30,7 @@ extern EFI_MM_CPU_PROTOCOL mMmCpuState;
//
extern EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext;
extern EFI_MMRAM_DESCRIPTOR mNsCommBuffer;
+extern EFI_MMRAM_DESCRIPTOR mSCommBuffer;
extern MP_INFORMATION_HOB_DATA *mMpInformationHobData;
extern EFI_MM_CONFIGURATION_PROTOCOL mMmConfig;

--
2.17.1


[PATCH edk2 v2 0/3] Fix several issues in StanaloneMmPkg

Ming Huang
 

Changes since v1:
Modify CheckBufferAddr() function.

Ming Huang (3):
StandaloneMmPkg: Fix issue about SpPcpuSharedBufSize field
StandaloneMmPkg: Replace DEBUG_INFO with DEBUG_ERROR
StandaloneMmPkg: Fix check buffer address failed issue from TF-A

.../Drivers/StandaloneMmCpu/EventHandle.c | 65 ++++++++++++++-----
.../Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 33 ++++++++--
.../Drivers/StandaloneMmCpu/StandaloneMmCpu.h | 1 +
.../Library/Arm/StandaloneMmCoreEntryPoint.h | 2 +-
.../Arm/CreateHobList.c | 2 +-
.../Arm/StandaloneMmCoreEntryPoint.c | 2 +-
6 files changed, 80 insertions(+), 25 deletions(-)

--
2.17.1


[PATCH 1/1] OvmfPkg: Bhyve: Correct the SSDT Revision field

Rebecca Cran
 

According to the ACPI Specification version 6.0 and newer, the SSDT
Revision field should be 2. Fix AcpiTables/Ssdt.aml.

Signed-off-by: Rebecca Cran <rebecca@...>
---
OvmfPkg/Bhyve/AcpiTables/Ssdt.asl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl b/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl
index 175ab3b7e66d..761b8267b9a5 100644
--- a/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl
+++ b/OvmfPkg/Bhyve/AcpiTables/Ssdt.asl
@@ -11,5 +11,5 @@

**/

-DefinitionBlock ("Ssdt.aml", "SSDT", 1, "REDHAT", "OVMF ", 1) {
+DefinitionBlock ("Ssdt.aml", "SSDT", 2, "REDHAT", "OVMF ", 1) {
}
--
2.34.1


Re: [PATCH edk2 v1 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A

Ming Huang
 

在 12/9/21 1:46 AM, Omkar Anand Kulkarni 写道:
Hi Ming,

Thanks for this patch. This patch helps to resolve Standalone MM issue while exercising RAS use case.
Few comments mentioned inline.

- Omkar


On 10/15/21 2:39 PM, Ming Huang via groups.io wrote:
There are two scene communicate with StandaloneMm(MM):
1 edk2 -> TF-A -> MM, communicate MM use non-secure buffer which
specify by EFI_SECURE_PARTITION_BOOT_INFO.SpNsCommBufBase;
2 RAS scene: fiq -> TF-A -> MM, use secure buffer which
specify by EFI_SECURE_PARTITION_BOOT_INFO.SpShareBufBase;

For now, the second scene will failed because check buffer address.
This patch add CheckBufferAddr() to support check address for secure buffer.

Signed-off-by: Ming Huang <huangming@...>
---
StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c | 70
++++++++++++++++----
StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21
++++++
StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h | 1 +
3 files changed, 79 insertions(+), 13 deletions(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
index 5dfaf9d751..63fab1bd78 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
@@ -50,6 +50,7 @@ EFI_MM_COMMUNICATE_HEADER
**PerCpuGuidedEventContext = NULL;

// Descriptor with whereabouts of memory used for communication with
the normal world EFI_MMRAM_DESCRIPTOR mNsCommBuffer;
+EFI_MMRAM_DESCRIPTOR mSCommBuffer;

MP_INFORMATION_HOB_DATA *mMpInformationHobData;

@@ -60,6 +61,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig =
{

STATIC EFI_MM_ENTRY_POINT mMmEntryPoint = NULL;

+STATIC
+EFI_STATUS
+CheckBufferAddr (
+ IN UINTN CommBufferAddr
+ )
+{
+ UINTN CommBufferSize;
+ EFI_STATUS Status;
+
+ Status = EFI_SUCCESS;
+ if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
+ Status = EFI_ACCESS_DENIED;
+ }
+
+ if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
+ (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
+ Status = EFI_INVALID_PARAMETER;
Single space after "Status = "

- Omkar


+ }
+
+ // Find out the size of the buffer passed CommBufferSize =
+ ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength
+
+ sizeof (EFI_MM_COMMUNICATE_HEADER);
+
+ // perform bounds check.
+ if (CommBufferAddr + CommBufferSize >=
+ mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
+ Status = EFI_ACCESS_DENIED;
Single space after "Status = "

- Omkar

+ }
+
+ if (!EFI_ERROR (Status)) {

In case of error this function call will not return from here. It will execute the code below comparing the MM Communicate buffer address with the Secure buffer address, which may cause wrong return type being returned. Can you check this, please?

- Omkar


+ return EFI_SUCCESS;
+ }
+
+ Status = EFI_SUCCESS;
+ if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
+ Status = EFI_ACCESS_DENIED;
+ }
+
+ if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
+ (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
+ Status = EFI_INVALID_PARAMETER;
+ }
+
+ // perform bounds check.
+ if (CommBufferAddr + CommBufferSize >=
+ mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
+ Status = EFI_ACCESS_DENIED;
+ }
+
+ return Status;
+}
+

CheckBufferAddr() function performs validity and overflow checks on the Communication buffers. These checks are same for both the non-secure
MM communicate buffer and secure buffer shared between EL3 and S-EL0. Can this code be combined ( example below)? This will help mitigate the above mentioned return type issue as well.

STATIC
EFI_STATUS
CheckBufferAddr (
IN UINTN CommBufferAddr
)
{
UINTN CommBufferSize;
EFI_STATUS Status;
EFI_MMRAM_DESCRIPTOR CommBuffer;

if (CommBufferAddr < mNsCommBuffer.PhysicalStart ||
CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
CommBuffer = mSCommBuffer;
} else {
CommBuffer = mNsCommBuffer;
}

if (CommBufferAddr < CommBuffer.PhysicalStart) {
Status = EFI_ACCESS_DENIED;
}

if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
(CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) {
Status = EFI_INVALID_PARAMETER;
}

// Find out the size of the buffer passed
CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
sizeof (EFI_MM_COMMUNICATE_HEADER);

// perform bounds check.
if (CommBufferAddr + CommBufferSize >=
CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) {
Status = EFI_ACCESS_DENIED;
}

return Status;
}

- Omkar


/**
The PI Standalone MM entry point for the TF-A CPU driver.

@@ -104,25 +157,16 @@ PiMmStandaloneArmTfCpuDriverEntry (
return EFI_INVALID_PARAMETER;
}

- if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
- return EFI_ACCESS_DENIED;
- }
-
- if ((NsCommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
- (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
- return EFI_INVALID_PARAMETER;
+ Status = CheckBufferAddr (NsCommBufferAddr); if (EFI_ERROR (Status))
+ {
+ DEBUG ((DEBUG_ERROR, "Check Buffer failed: %r\n", Status));
+ return Status;
}

// Find out the size of the buffer passed
NsCommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *)
NsCommBufferAddr)->MessageLength +
sizeof (EFI_MM_COMMUNICATE_HEADER);

- // perform bounds check.
- if (NsCommBufferAddr + NsCommBufferSize >=
- mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
- return EFI_ACCESS_DENIED;
- }
-
GuidedEventContext = NULL;
// Now that the secure world can see the normal world buffer, allocate
// memory to copy the communication buffer to the secure world.
diff --git
a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
index fd9c59b4da..96dad20dd1 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
@@ -107,6 +107,7 @@ StandaloneMmCpuInitialize (
UINTN Index;
UINTN ArraySize;
VOID *HobStart;
+ EFI_MMRAM_HOB_DESCRIPTOR_BLOCK *MmramRangesHob;

ASSERT (SystemTable != NULL);
mMmst = SystemTable;
@@ -186,6 +187,26 @@ StandaloneMmCpuInitialize (
CopyMem (&mNsCommBuffer, NsCommBufMmramRange,
sizeof(EFI_MMRAM_DESCRIPTOR));
DEBUG ((DEBUG_INFO, "mNsCommBuffer: 0x%016lx - 0x%lx\n",
mNsCommBuffer.CpuStart, mNsCommBuffer.PhysicalSize));

+ Status = GetGuidedHobData (
+ HobStart,
+ &gEfiMmPeiMmramMemoryReserveGuid,
+ (VOID **) &MmramRangesHob
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "MmramRangesHob data extraction failed -
0x%x\n", Status));
+ return Status;
+ }
+
+ //
+ // As CreateHobListFromBootInfo(), the base and size of buffer shared
+ with // privileged Secure world software is in second one.
+ //
+ CopyMem (
+ &mSCommBuffer,
+ &MmramRangesHob->Descriptor[0] + 1,
Can this be changed to
&MmramRangesHob->Descriptor[1],
I missed this comment at last email.
The struct define:
EFI_MMRAM_DESCRIPTOR Descriptor[1];

I guess some static code analysis tool may report out of array range, if modify to
&MmramRangesHob->Descriptor[1].

- Ming


- Omkar

+ sizeof(EFI_MMRAM_DESCRIPTOR)
+ );
+
//
// Extract the MP information from the Hoblist
//
diff --git
a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
index 2c96439c15..2e03b20d85 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
@@ -30,6 +30,7 @@ extern EFI_MM_CPU_PROTOCOL mMmCpuState; //
extern EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext;
extern EFI_MMRAM_DESCRIPTOR mNsCommBuffer;
+extern EFI_MMRAM_DESCRIPTOR mSCommBuffer;
extern MP_INFORMATION_HOB_DATA *mMpInformationHobData;
extern EFI_MM_CONFIGURATION_PROTOCOL mMmConfig;

--
2.17.1




IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.




Re: [PATCH 1/1] UsbBusDxe: fix NOOPT build error

Wu, Hao A
 

-----Original Message-----
From: Gerd Hoffmann <kraxel@...>
Sent: Monday, December 20, 2021 10:33 PM
To: devel@edk2.groups.io
Cc: Wu, Hao A <hao.a.wu@...>; Philippe Mathieu-Daudé
<philmd@...>; Wang, Jian J <jian.j.wang@...>; Pawel
Polawski <ppolawsk@...>; Ni, Ray <ray.ni@...>; Gao, Liming
<gaoliming@...>; Gerd Hoffmann <kraxel@...>
Subject: [PATCH 1/1] UsbBusDxe: fix NOOPT build error

Reviewed-by: Hao A Wu <hao.a.wu@...>
Will tweak the subject to "MdeModulePkg/UsbBusDxe: fix NOOPT build error" before merging. If concerns, please help to raise.

Best Regards,
Hao Wu



gcc-11 (fedora 35):

/home/kraxel/projects/edk2/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus
.c: In function ‘UsbIoBulkTransfer’:
/home/kraxel/projects/edk2/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus
.c:277:12: error: ‘UsbHcBulkTransfer’ accessing 80 bytes in a region of size 8 [-
Werror=stringop-overflow=]

Signed-off-by: Gerd Hoffmann <kraxel@...>
---
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.h | 2 +-
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.h
b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.h
index 04cf36d3c860..d93370a6c21e 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.h
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.h
@@ -148,7 +148,7 @@ UsbHcBulkTransfer (
IN UINT8 DevSpeed,
IN UINTN MaxPacket,
IN UINT8 BufferNum,
- IN OUT VOID *Data[EFI_USB_MAX_BULK_BUFFER_NUM],
+ IN OUT VOID *Data[],
IN OUT UINTN *DataLength,
IN OUT UINT8 *DataToggle,
IN UINTN TimeOut,
diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c
b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c
index 12d08c0b740f..740e7babb0ca 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c
@@ -267,7 +267,7 @@ UsbHcBulkTransfer (
IN UINT8 DevSpeed,
IN UINTN MaxPacket,
IN UINT8 BufferNum,
- IN OUT VOID *Data[EFI_USB_MAX_BULK_BUFFER_NUM],
+ IN OUT VOID *Data[],
IN OUT UINTN *DataLength,
IN OUT UINT8 *DataToggle,
IN UINTN TimeOut,
--
2.33.1


Re: [edk2-platforms: PATCH] System will occur a CPU exception error when sorting CPU APIC map, because of a pointer point to an wrong space.

Ni, Ray
 

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

-----Original Message-----
From: Lin, JackX <jackx.lin@...>
Sent: Tuesday, December 21, 2021 2:48 PM
To: devel@edk2.groups.io
Cc: Lin, JackX <jackx.lin@...>; Lin, JackX <jackx.lin@...>; Chiu, Chasel <chasel.chiu@...>; Dong, Eric
<eric.dong@...>; Yao, Jiewen <jiewen.yao@...>; Ni, Ray <ray.ni@...>; Chaganty, Rangasai V
<rangasai.v.chaganty@...>; Kuo, Donald <donald.kuo@...>; Kumar, Chandana C <chandana.c.kumar@...>
Subject: [edk2-platforms: PATCH] System will occur a CPU exception error when sorting CPU APIC map, because of a pointer
point to an wrong space.

Signed-off-by: JackX Lin <JackX.Lin@...>
Cc: Chasel Chiu <chasel.chiu@...>
Cc: Dong Eric <eric.dong@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Ray Ni <ray.ni@...>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@...>
Cc: Donald Kuo <Donald.Kuo@...>
Cc: Chandana C Kumar <chandana.c.kumar@...>
Cc: JackX Lin <JackX.Lin@...>
---
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index 785cf4c2f9..05fc7799fb 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -169,7 +169,7 @@ SortCpuLocalApicInTable (
UINT32 Index;
UINT32 CurrProcessor;
UINT32 BspApicId;
- EFI_CPU_ID_ORDER_MAP *TempVal;
+ EFI_CPU_ID_ORDER_MAP TempVal;
EFI_CPU_ID_ORDER_MAP *CpuIdMapPtr;
UINT32 CoreThreadMask;
EFI_CPU_ID_ORDER_MAP *TempCpuApicIdOrderTable;
@@ -183,7 +183,6 @@ SortCpuLocalApicInTable (
}

TempCpuApicIdOrderTable = AllocateZeroPool (mNumberOfCpus * sizeof (EFI_CPU_ID_ORDER_MAP));
- TempVal = AllocateZeroPool (sizeof (EFI_CPU_ID_ORDER_MAP));
CoreThreadMask = (UINT32) ((1 << mNumOfBitShift) - 1);

for (CurrProcessor = 0, Index = 0; CurrProcessor < mNumberOfCpus; CurrProcessor++, Index++) {
--
2.32.0.windows.2


[edk2-platforms: PATCH] System will occur a CPU exception error when sorting CPU APIC map, because of a pointer point to an wrong space.

JackX Lin
 

Signed-off-by: JackX Lin <JackX.Lin@...>
Cc: Chasel Chiu <chasel.chiu@...>
Cc: Dong Eric <eric.dong@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Ray Ni <ray.ni@...>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@...>
Cc: Donald Kuo <Donald.Kuo@...>
Cc: Chandana C Kumar <chandana.c.kumar@...>
Cc: JackX Lin <JackX.Lin@...>
---
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index 785cf4c2f9..05fc7799fb 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -169,7 +169,7 @@ SortCpuLocalApicInTable (
UINT32 Index;
UINT32 CurrProcessor;
UINT32 BspApicId;
- EFI_CPU_ID_ORDER_MAP *TempVal;
+ EFI_CPU_ID_ORDER_MAP TempVal;
EFI_CPU_ID_ORDER_MAP *CpuIdMapPtr;
UINT32 CoreThreadMask;
EFI_CPU_ID_ORDER_MAP *TempCpuApicIdOrderTable;
@@ -183,7 +183,6 @@ SortCpuLocalApicInTable (
}

TempCpuApicIdOrderTable = AllocateZeroPool (mNumberOfCpus * sizeof (EFI_CPU_ID_ORDER_MAP));
- TempVal = AllocateZeroPool (sizeof (EFI_CPU_ID_ORDER_MAP));
CoreThreadMask = (UINT32) ((1 << mNumOfBitShift) - 1);

for (CurrProcessor = 0, Index = 0; CurrProcessor < mNumberOfCpus; CurrProcessor++, Index++) {
--
2.32.0.windows.2


Re: [PATCH v4] MdeModulePkg: Replace with UFS_UNIT_DESC to fix timeout problem

Wu, Hao A
 

Reviewed-by: Hao A Wu <hao.a.wu@...>
Will merge in a couple of days.

Best Regards,
Hao Wu

-----Original Message-----
From: Ke, VincentX <vincentx.ke@...>
Sent: Friday, December 17, 2021 4:10 PM
To: devel@edk2.groups.io
Cc: Ke, VincentX <vincentx.ke@...>; Wu, Hao A <hao.a.wu@...>;
Ni, Ray <ray.ni@...>; Chiu, Ian <Ian.chiu@...>; Chu, Maggie
<maggie.chu@...>
Subject: [PATCH v4] MdeModulePkg: Replace with UFS_UNIT_DESC to fix
timeout problem

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3714

Replace with UFS_UNIT_DESC to fix response timeout problem.

Cc: Hao A Wu <hao.a.wu@...>
Cc: Ray Ni <ray.ni@...>
Cc: Ian Chiu <Ian.chiu@...>
Cc: Maggie Chu <maggie.chu@...>
Signed-off-by: VincentX Ke <vincentx.ke@...>
---
.../Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c | 20 +++++++++----------
1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c
b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c
index b331c0f3e3..9ad1e19fe0 100644
--- a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c
+++ b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c
@@ -1037,9 +1037,9 @@ InitializeUfsBlockIoPeim (
UFS_PEIM_HC_PRIVATE_DATA *Private;
EDKII_UFS_HOST_CONTROLLER_PPI *UfsHcPpi;
UINT32 Index;
- UFS_CONFIG_DESC Config;
UINTN MmioBase;
UINT8 Controller;
+ UFS_UNIT_DESC UnitDescriptor;

//
// Shadow this PEIM to run from memory @@ -1126,19 +1126,17 @@
InitializeUfsBlockIoPeim (
}

//
- // Get Ufs Device's Lun Info by reading Configuration Descriptor.
+ // Check if 8 common luns are active and set corresponding bit mask.
//
- Status = UfsRwDeviceDesc (Private, TRUE, UfsConfigDesc, 0, 0, &Config,
sizeof (UFS_CONFIG_DESC));
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "Ufs Get Configuration Descriptor Error, Status
= %r\n", Status));
- Controller++;
- continue;
- }
-
for (Index = 0; Index < UFS_PEIM_MAX_LUNS; Index++) {
- if (Config.UnitDescConfParams[Index].LunEn != 0) {
- Private->Luns.BitMask |= (BIT0 << Index);
+ Status = UfsRwDeviceDesc (Private, TRUE, UfsUnitDesc, (UINT8) Index, 0,
&UnitDescriptor, sizeof (UFS_UNIT_DESC));
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Fail to read UFS Unit Descriptor, Index = %X,
Status = %r\n", Index, Status));
+ continue;
+ }
+ if (UnitDescriptor.LunEn == 0x1) {
DEBUG ((DEBUG_INFO, "Ufs %d Lun %d is enabled\n", Controller, Index));
+ Private->Luns.BitMask |= (BIT0 << Index);
}
}

--
2.18.0.windows.1


Event: TianoCore Bug Triage - APAC / NAMO - 12/21/2021 #cal-reminder

devel@edk2.groups.io Calendar <noreply@...>
 

Reminder: TianoCore Bug Triage - APAC / NAMO

When:
12/21/2021
6:30pm to 7:30pm
(UTC-08:00) America/Los Angeles

Where:
https://teams.microsoft.com/l/meetup-join/19%3ameeting_OTUyZTg2NjgtNDhlNS00ODVlLTllYTUtYzg1OTNjNjdiZjFh%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%22b286b53a-1218-4db3-bfc9-3d4c5aa7669e%22%7d

Organizer: Liming Gao gaoliming@...

View Event

Description:

TianoCore Bug Triage - APAC / NAMO

Hosted by Liming Gao

 

________________________________________________________________________________

Microsoft Teams meeting

Join on your computer or mobile app

Click here to join the meeting

Join with a video conferencing device

teams@...

Video Conference ID: 116 062 094 0

Alternate VTC dialing instructions

Or call in (audio only)

+1 916-245-6934,,77463821#   United States, Sacramento

Phone Conference ID: 774 638 21#

Find a local number | Reset PIN

Learn More | Meeting options


Re: [PATCH v1 2/2] ArmPkg: MmCommunicationDxe: Update MM communicate input arguments checks

Kun Qin
 

Hi Sami,

Thanks for your review. But this v1 patch was splitted into multiple patches as in https://edk2.groups.io/g/devel/message/85116. Please feel free leave feedback on the new series.

Regards,
Kun

On 12/13/2021 13:03, Sami Mujawar wrote:
Hi Kun,
Thank you for this patch.
These changes look good to me.
Reviewed-by: Sami Mujawar <sami.mujawar@...>
Regards,
Sami Mujawar
On 30/11/2021 12:39 AM, Kun Qin via groups.io wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3751

Current MM communicate routine from ArmPkg would conduct few steps before
proceeding with SMC calls. However, some inspection steps are different
from PI specification.

This patch updated MM communicate input argument inspection routine to
match the following PI descriptions:
1. Return code `EFI_INVALID_PARAMETER` represents "the `CommBuffer**`
parameters do not refer to the same location in memory".
2. `CommSize` represents "the size of the data buffer being passed in"
instead of "the size of the data being used from data buffer".
3. Regarding `MessageLength`, "if the `MessageLength` is zero, or too
large for the MM implementation to manage, the MM implementation must
update the `MessageLength` to reflect the size of the `Data` buffer that
it can tolerate".

Cc: Leif Lindholm <leif@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Bret Barkelew <Bret.Barkelew@...>
Cc: Michael Kubacki <michael.kubacki@...>

Signed-off-by: Kun Qin <kuqin12@...>
---
  ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 44 ++++++++++++--------
  1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index b1e309580988..8a2bd222957f 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -41,15 +41,19 @@ STATIC EFI_HANDLE  mMmCommunicateHandle;
    This function provides a service to send and receive messages from a registered UEFI service.
-  @param[in] This                The EFI_MM_COMMUNICATION_PROTOCOL instance.
-  @param[in] CommBufferPhysical  Physical address of the MM communication buffer
-  @param[in] CommBufferVirtual   Virtual address of the MM communication buffer
-  @param[in] CommSize            The size of the data buffer being passed in. On exit, the size of data
-                                 being returned. Zero if the handler does not wish to reply with any data.
-                                 This parameter is optional and may be NULL.
+  @param[in] This                     The EFI_MM_COMMUNICATION_PROTOCOL instance.
+  @param[in, out] CommBufferPhysical  Physical address of the MM communication buffer
+  @param[in, out] CommBufferVirtual   Virtual address of the MM communication buffer
+  @param[in, out] CommSize            The size of the data buffer being passed in. On input, when not
+                                      omitted, the buffer should cover EFI_MM_COMMUNICATE_HEADER and the
+                                      value of MessageLength field. On exit, the size of data being
+                                      returned. Zero if the handler does not wish to reply with any data.
+                                      This parameter is optional and may be NULL.
    @retval EFI_SUCCESS            The message was successfully posted.
-  @retval EFI_INVALID_PARAMETER  CommBufferPhysical was NULL or CommBufferVirtual was NULL.
+  @retval EFI_INVALID_PARAMETER  CommBufferPhysical or CommBufferVirtual was NULL, or integer value
+                                 pointed by CommSize does not cover EFI_MM_COMMUNICATE_HEADER and the
+                                 value of MessageLength field.
    @retval EFI_BAD_BUFFER_SIZE    The buffer is too large for the MM implementation.
                                   If this error is returned, the MessageLength field
                                   in the CommBuffer header or the integer pointed by
@@ -82,10 +86,11 @@ MmCommunication2Communicate (
    //
    // Check parameters
    //
-  if (CommBufferVirtual == NULL) {
+  if (CommBufferVirtual == NULL || CommBufferPhysical == NULL) {
      return EFI_INVALID_PARAMETER;
    }
+  Status = EFI_SUCCESS;
    CommunicateHeader = CommBufferVirtual;
    // CommBuffer is a mandatory parameter. Hence, Rely on
    // MessageLength + Header to ascertain the
@@ -95,33 +100,38 @@ MmCommunication2Communicate (
                 sizeof (CommunicateHeader->HeaderGuid) +
                 sizeof (CommunicateHeader->MessageLength);
-  // If the length of the CommBuffer is 0 then return the expected length.
-  if (CommSize != 0) {
+  // If CommSize is not omitted, perform size inspection before proceeding.
+  if (CommSize != NULL) {
      // This case can be used by the consumer of this driver to find out the
      // max size that can be used for allocating CommBuffer.
      if ((*CommSize == 0) ||
          (*CommSize > mNsCommBuffMemRegion.Length)) {
        *CommSize = mNsCommBuffMemRegion.Length;
-      return EFI_BAD_BUFFER_SIZE;
+      Status = EFI_BAD_BUFFER_SIZE;
      }
      //
-    // CommSize must match MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
+    // CommSize should cover at least MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
      //
-    if (*CommSize != BufferSize) {
-        return EFI_INVALID_PARAMETER;
+    if (*CommSize < BufferSize) {
+      Status = EFI_INVALID_PARAMETER;
      }
    }
    //
-  // If the buffer size is 0 or greater than what can be tolerated by the MM
+  // If the message length is 0 or greater than what can be tolerated by the MM
    // environment then return the expected size.
    //
-  if ((BufferSize == 0) ||
+  if ((CommunicateHeader->MessageLength == 0) ||
        (BufferSize > mNsCommBuffMemRegion.Length)) {
      CommunicateHeader->MessageLength = mNsCommBuffMemRegion.Length -
                                         sizeof (CommunicateHeader->HeaderGuid) -
                                         sizeof (CommunicateHeader->MessageLength);
-    return EFI_BAD_BUFFER_SIZE;
+    Status = EFI_BAD_BUFFER_SIZE;
+  }
+
+  // MessageLength or CommSize check has failed, return here.
+  if (EFI_ERROR (Status)) {
+    return Status;
    }
    // SMC Function ID


Re: [PATCH v1 2/2] ArmPkg: MmCommunicationDxe: Update MM communicate input arguments checks

Kun Qin
 

Sure, the updated patches are sent here: https://edk2.groups.io/g/devel/message/85116. Please provide any further feedback. Any input is appreciated.

Regards,
Kun

On 12/15/2021 00:52, Ard Biesheuvel wrote:
On Tue, 30 Nov 2021 at 01:39, Kun Qin <kuqin12@...> wrote:

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

Current MM communicate routine from ArmPkg would conduct few steps before
proceeding with SMC calls. However, some inspection steps are different
from PI specification.

This patch updated MM communicate input argument inspection routine to
match the following PI descriptions:
1. Return code `EFI_INVALID_PARAMETER` represents "the `CommBuffer**`
parameters do not refer to the same location in memory".
2. `CommSize` represents "the size of the data buffer being passed in"
instead of "the size of the data being used from data buffer".
3. Regarding `MessageLength`, "if the `MessageLength` is zero, or too
large for the MM implementation to manage, the MM implementation must
update the `MessageLength` to reflect the size of the `Data` buffer that
it can tolerate".
A bullet list like this is usually a strong hint that the patch should
be split up, and this case is no different. Please split this up into
3 separate patches so that us poor reviewers have an easier job.

Cc: Leif Lindholm <leif@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Bret Barkelew <Bret.Barkelew@...>
Cc: Michael Kubacki <michael.kubacki@...>

Signed-off-by: Kun Qin <kuqin12@...>
---
ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 44 ++++++++++++--------
1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index b1e309580988..8a2bd222957f 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -41,15 +41,19 @@ STATIC EFI_HANDLE mMmCommunicateHandle;

This function provides a service to send and receive messages from a registered UEFI service.

- @param[in] This The EFI_MM_COMMUNICATION_PROTOCOL instance.
- @param[in] CommBufferPhysical Physical address of the MM communication buffer
- @param[in] CommBufferVirtual Virtual address of the MM communication buffer
- @param[in] CommSize The size of the data buffer being passed in. On exit, the size of data
- being returned. Zero if the handler does not wish to reply with any data.
- This parameter is optional and may be NULL.
+ @param[in] This The EFI_MM_COMMUNICATION_PROTOCOL instance.
+ @param[in, out] CommBufferPhysical Physical address of the MM communication buffer
+ @param[in, out] CommBufferVirtual Virtual address of the MM communication buffer
+ @param[in, out] CommSize The size of the data buffer being passed in. On input, when not
+ omitted, the buffer should cover EFI_MM_COMMUNICATE_HEADER and the
+ value of MessageLength field. On exit, the size of data being
+ returned. Zero if the handler does not wish to reply with any data.
+ This parameter is optional and may be NULL.

@retval EFI_SUCCESS The message was successfully posted.
- @retval EFI_INVALID_PARAMETER CommBufferPhysical was NULL or CommBufferVirtual was NULL.
+ @retval EFI_INVALID_PARAMETER CommBufferPhysical or CommBufferVirtual was NULL, or integer value
+ pointed by CommSize does not cover EFI_MM_COMMUNICATE_HEADER and the
+ value of MessageLength field.
@retval EFI_BAD_BUFFER_SIZE The buffer is too large for the MM implementation.
If this error is returned, the MessageLength field
in the CommBuffer header or the integer pointed by
@@ -82,10 +86,11 @@ MmCommunication2Communicate (
//
// Check parameters
//
- if (CommBufferVirtual == NULL) {
+ if (CommBufferVirtual == NULL || CommBufferPhysical == NULL) {
return EFI_INVALID_PARAMETER;
}

+ Status = EFI_SUCCESS;
CommunicateHeader = CommBufferVirtual;
// CommBuffer is a mandatory parameter. Hence, Rely on
// MessageLength + Header to ascertain the
@@ -95,33 +100,38 @@ MmCommunication2Communicate (
sizeof (CommunicateHeader->HeaderGuid) +
sizeof (CommunicateHeader->MessageLength);

- // If the length of the CommBuffer is 0 then return the expected length.
- if (CommSize != 0) {
+ // If CommSize is not omitted, perform size inspection before proceeding.
+ if (CommSize != NULL) {
// This case can be used by the consumer of this driver to find out the
// max size that can be used for allocating CommBuffer.
if ((*CommSize == 0) ||
(*CommSize > mNsCommBuffMemRegion.Length)) {
*CommSize = mNsCommBuffMemRegion.Length;
- return EFI_BAD_BUFFER_SIZE;
+ Status = EFI_BAD_BUFFER_SIZE;
}
//
- // CommSize must match MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
+ // CommSize should cover at least MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
//
- if (*CommSize != BufferSize) {
- return EFI_INVALID_PARAMETER;
+ if (*CommSize < BufferSize) {
+ Status = EFI_INVALID_PARAMETER;
}
}

//
- // If the buffer size is 0 or greater than what can be tolerated by the MM
+ // If the message length is 0 or greater than what can be tolerated by the MM
// environment then return the expected size.
//
- if ((BufferSize == 0) ||
+ if ((CommunicateHeader->MessageLength == 0) ||
(BufferSize > mNsCommBuffMemRegion.Length)) {
CommunicateHeader->MessageLength = mNsCommBuffMemRegion.Length -
sizeof (CommunicateHeader->HeaderGuid) -
sizeof (CommunicateHeader->MessageLength);
- return EFI_BAD_BUFFER_SIZE;
+ Status = EFI_BAD_BUFFER_SIZE;
+ }
+
+ // MessageLength or CommSize check has failed, return here.
+ if (EFI_ERROR (Status)) {
+ return Status;
}

// SMC Function ID
--
2.32.0.windows.1


[PATCH v2 6/6] ArmPkg: MmCommunicationDxe: Update MM communicate `MessageLength` check

Kun Qin
 

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

Current MM communicate routine from ArmPkg would conduct few checks prior
to proceeding with SMC calls. However, the inspection step is different
from PI specification.

This patch updated MM communicate input argument inspection routine to
assure that "if the `MessageLength` is zero, or too large for the MM
implementation to manage, the MM implementation must update the
`MessageLength` to reflect the size of the `Data` buffer that it can
tolerate", as described by `EFI_MM_COMMUNICATION_PROTOCOL.Communicate()`
section in PI specification.

Cc: Leif Lindholm <leif@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Bret Barkelew <Bret.Barkelew@...>
Cc: Michael Kubacki <michael.kubacki@...>

Signed-off-by: Kun Qin <kuqin12@...>
---

Notes:
v2:
- Splitting patch into 4 of 4 [Ard]
- Uncrustify style update

ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index 2f89b7c5b6c4..85d9034555f0 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -92,6 +92,7 @@ MmCommunication2Communicate (
return EFI_INVALID_PARAMETER;
}

+ Status = EFI_SUCCESS;
CommunicateHeader = CommBufferVirtual;
// CommBuffer is a mandatory parameter. Hence, Rely on
// MessageLength + Header to ascertain the
@@ -109,28 +110,33 @@ MmCommunication2Communicate (
(*CommSize > mNsCommBuffMemRegion.Length))
{
*CommSize = mNsCommBuffMemRegion.Length;
- return EFI_BAD_BUFFER_SIZE;
+ Status = EFI_BAD_BUFFER_SIZE;
}

//
// CommSize should cover at least MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
//
if (*CommSize < BufferSize) {
- return EFI_INVALID_PARAMETER;
+ Status = EFI_INVALID_PARAMETER;
}
}

//
- // If the buffer size is 0 or greater than what can be tolerated by the MM
+ // If the message length is 0 or greater than what can be tolerated by the MM
// environment then return the expected size.
//
- if ((BufferSize == 0) ||
+ if ((CommunicateHeader->MessageLength == 0) ||
(BufferSize > mNsCommBuffMemRegion.Length))
{
CommunicateHeader->MessageLength = mNsCommBuffMemRegion.Length -
sizeof (CommunicateHeader->HeaderGuid) -
sizeof (CommunicateHeader->MessageLength);
- return EFI_BAD_BUFFER_SIZE;
+ Status = EFI_BAD_BUFFER_SIZE;
+ }
+
+ // MessageLength or CommSize check has failed, return here.
+ if (EFI_ERROR (Status)) {
+ return Status;
}

// SMC Function ID
--
2.32.0.windows.1


[PATCH v2 5/6] ArmPkg: MmCommunicationDxe: Update MM communicate `CommSize` check

Kun Qin
 

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

Current MM communicate routine from ArmPkg would conduct few checks prior
to proceeding with SMC calls. However, the inspection step is different
from PI specification.

This patch updated MM communicate input argument inspection routine to
assure `CommSize` represents "the size of the data buffer being passed
in" instead of the size of the data being used from data buffer, as
described by section `EFI_MM_COMMUNICATION2_PROTOCOL.Communicate()` in PI
specification.

Cc: Leif Lindholm <leif@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Bret Barkelew <Bret.Barkelew@...>
Cc: Michael Kubacki <michael.kubacki@...>

Signed-off-by: Kun Qin <kuqin12@...>
---

Notes:
v2:
- Splitting patch into 3 of 4 [Ard]

ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index 0283be430dff..2f89b7c5b6c4 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -44,13 +44,18 @@ STATIC EFI_HANDLE mMmCommunicateHandle;
@param[in] This The EFI_MM_COMMUNICATION_PROTOCOL instance.
@param[in, out] CommBufferPhysical Physical address of the MM communication buffer
@param[in, out] CommBufferVirtual Virtual address of the MM communication buffer
- @param[in, out] CommSize The size of the data buffer being passed in. On exit, the
- size of data being returned. Zero if the handler does not
+ @param[in, out] CommSize The size of the data buffer being passed in. On input,
+ when not omitted, the buffer should cover EFI_MM_COMMUNICATE_HEADER
+ and the value of MessageLength field. On exit, the size
+ of data being returned. Zero if the handler does not
wish to reply with any data. This parameter is optional
and may be NULL.

@retval EFI_SUCCESS The message was successfully posted.
- @retval EFI_INVALID_PARAMETER CommBufferPhysical was NULL or CommBufferVirtual was NULL.
+ @retval EFI_INVALID_PARAMETER CommBufferPhysical or CommBufferVirtual was NULL, or
+ integer value pointed by CommSize does not cover
+ EFI_MM_COMMUNICATE_HEADER and the value of MessageLength
+ field.
@retval EFI_BAD_BUFFER_SIZE The buffer is too large for the MM implementation.
If this error is returned, the MessageLength field
in the CommBuffer header or the integer pointed by
@@ -96,8 +101,8 @@ MmCommunication2Communicate (
sizeof (CommunicateHeader->HeaderGuid) +
sizeof (CommunicateHeader->MessageLength);

- // If the length of the CommBuffer is 0 then return the expected length.
- if (CommSize != 0) {
+ // If CommSize is not omitted, perform size inspection before proceeding.
+ if (CommSize != NULL) {
// This case can be used by the consumer of this driver to find out the
// max size that can be used for allocating CommBuffer.
if ((*CommSize == 0) ||
@@ -108,9 +113,9 @@ MmCommunication2Communicate (
}

//
- // CommSize must match MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
+ // CommSize should cover at least MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
//
- if (*CommSize != BufferSize) {
+ if (*CommSize < BufferSize) {
return EFI_INVALID_PARAMETER;
}
}
--
2.32.0.windows.1

9521 - 9540 of 94575