[edk2] [PATCH 07/11] MdeModulePkg/UsbKb: Don't access key codes when length is wrong
Per USB HID spec, the buffer holding key codes should be 8-byte
long.
Today's code assumes that the key codes buffer length is 8-byte
long and unconditionally accesses the key codes buffer.
It's incorrect.
The patch fixes the issue by returning Device Error when the
length is less than 8-byte.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni
Cc: Star Zeng
Cc: Jiewen Yao
Cc: Steven Shi
---
MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c | 4
1 file changed, 4 insertions(+)
diff --git a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
index 9cb4b5db6b..384d7e2f07 100644
--- a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
+++ b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
@@ -1059,6 +1059,10 @@ KeyboardHandler (
// Byte 1 is reserved.
// Bytes 2 to 7 are keycodes.
//
+ if ((Data != NULL) && (DataLength < 8)) {
+return EFI_DEVICE_ERROR;
+ }
+
CurKeyCodeBuffer = (UINT8 *) Data;
OldKeyCodeBuffer = UsbKeyboardDevice->LastKeyCodeArray;
--
2.16.1.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 09/11] MdeModulePkg/UsbMouse: Don't access key codes when length is wrong
Per USB HID spec, the buffer holding key codes should at least 3-byte
long.
Today's code assumes that the key codes buffer length is longer than
3-byte and unconditionally accesses the key codes buffer.
It's incorrect.
The patch fixes the issue by returning Device Error when the
length is less than 3-byte.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni
Cc: Star Zeng
Cc: Jiewen Yao
Cc: Steven Shi
---
MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
index 9324994975..d4bcf15929 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
@@ -811,8 +811,6 @@ OnMouseInterruptComplete (
return EFI_SUCCESS;
}
- UsbMouseDevice->StateChanged = TRUE;
-
//
// Check mouse Data
// USB HID Specification specifies following data format:
@@ -825,6 +823,12 @@ OnMouseInterruptComplete (
// 2 0 to 7 Y displacement
// 3 to n 0 to 7 Device specific (optional)
//
+ if ((Data != NULL) && (DataLength < 3)) {
+return EFI_DEVICE_ERROR;
+ }
+
+ UsbMouseDevice->StateChanged = TRUE;
+
UsbMouseDevice->State.LeftButton = (BOOLEAN) ((*(UINT8 *) Data & BIT0) !=
0);
UsbMouseDevice->State.RightButton = (BOOLEAN) ((*(UINT8 *) Data & BIT1) !=
0);
UsbMouseDevice->State.RelativeMovementX += *((INT8 *) Data + 1);
--
2.16.1.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 11/11] MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected
From: Hao Wu
This commit adds checks to make sure the UFS devices do not return more
data than the driver expected.
Cc: Ruiyu Ni
Cc: Jiewen Yao
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c| 19 --
.../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c| 30 +++---
2 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
index 936f25da5e..9b87835ed8 100644
--- a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
+++ b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
@@ -857,6 +857,14 @@ UfsRwDeviceDesc (
SwapLittleEndianToBigEndian ((UINT8*)&ReturnDataSize, sizeof (UINT16));
if (Read) {
+ //
+ // Make sure the hardware device does not return more data than expected.
+ //
+ if (ReturnDataSize > Packet.InTransferLength) {
+Status = EFI_DEVICE_ERROR;
+goto Exit;
+ }
+
CopyMem (Packet.InDataBuffer, (QueryResp + 1), ReturnDataSize);
Packet.InTransferLength = ReturnDataSize;
} else {
@@ -1170,8 +1178,15 @@ UfsExecScsiCmds (
SwapLittleEndianToBigEndian ((UINT8*)&SenseDataLen, sizeof (UINT16));
if ((Packet->SenseDataLength != 0) && (Packet->SenseData != NULL)) {
-CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
-Packet->SenseDataLength = (UINT8)SenseDataLen;
+//
+// Make sure the hardware device does not return more data than expected.
+//
+if (SenseDataLen <= Packet->SenseDataLength) {
+ CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
+ Packet->SenseDataLength = (UINT8)SenseDataLen;
+} else {
+ Packet->SenseDataLength = 0;
+}
}
//
diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
index 5756f637fd..0238264878 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
@@ -833,6 +833,7 @@ UfsStopExecCmd (
@param[in] QueryResp Pointer to the query response.
@retval EFI_INVALID_PARAMETER Packet or QueryResp are empty or opcode is
invalid.
+ @retval EFI_DEVICE_ERROR Data returned from device is invalid.
@retval EFI_SUCCESS Data extracted.
**/
@@ -853,6 +854,13 @@ UfsGetReturnDataFromQueryResponse (
case UtpQueryFuncOpcodeRdDesc:
ReturnDataSize = QueryResp->Tsf.Length;
SwapLittleEndianToBigEndian ((UINT8*)&ReturnDataSize, sizeof (UINT16));
+ //
+ // Make sure the hardware device does not return more data than expected.
+ //
+ if (ReturnDataSize > Packet->TransferLength) {
+return EFI_DEVICE_ERROR;
+ }
+
CopyMem (Packet->DataBuffer, (QueryResp + 1), ReturnDataSize);
Packet->TransferLength = ReturnDataSize;
break;
@@ -1469,8 +1477,15 @@ UfsExecScsiCmds (
SwapLittleEndianToBigEndian ((UINT8*)&SenseDataLen, sizeof (UINT16));
if ((Packet->SenseDataLength != 0) && (Packet->SenseData != NULL)) {
-CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
-Packet->SenseDataLength = (UINT8)SenseDataLen;
+//
+// Make sure the hardware device does not return more data than expected.
+//
+if (SenseDataLen <= Packet->SenseDataLength) {
+ CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
+ Packet->SenseDataLength = (UINT8)SenseDataLen;
+} else {
+ Packet->SenseDataLength = 0;
+}
}
//
@@ -2226,8 +2241,15 @@ ProcessAsyncTaskList (
SwapLittleEndianToBigEndian ((UINT8*)&SenseDataLen, sizeof (UINT16));
if ((Packet->SenseDataLength != 0) && (Packet->SenseData != NULL)) {
- CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
- Packet->SenseDataLength = (UINT8)SenseDataLen;
+ //
+ // Make sure the hardware device does not return more data than
expected.
+ //
+ if (SenseDataLen <= Packet->SenseDataLength) {
+CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
+Packet->SenseDataLength = (UINT8)SenseDataLen;
+ } else {
+Packet->SenseDataLength = 0;
+ }
}
//
--
2.16.1.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 10/11] MdeModulePkg/UsbBus: Deny when the string descriptor length is odd
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni
Cc: Jiewen Yao
Cc: Star Zeng
---
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
index d9bc1f9e28..182a3f97c9 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
@@ -650,7 +650,7 @@ UsbGetOneString (
//
Status = UsbCtrlGetDesc (UsbDev, USB_DESC_TYPE_STRING, Index, LangId, &Desc,
2);
- if (EFI_ERROR (Status)) {
+ if (EFI_ERROR (Status) || (Desc.Length % 2 != 0)) {
return NULL;
}
--
2.16.1.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 08/11] MdeModulePkg/AbsPointer: Don't access key codes when length is wrong
Per USB HID spec, the buffer holding key codes should at least 3-byte
long.
Today's code assumes that the key codes buffer length is longer than
3-byte and unconditionally accesses the key codes buffer.
It's incorrect.
The patch fixes the issue by returning Device Error when the
length is less than 3-byte.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni
Cc: Star Zeng
Cc: Jiewen Yao
Cc: Steven Shi
---
.../Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git
a/MdeModulePkg/Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c
b/MdeModulePkg/Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c
index 965195ca34..b4638961d9 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c
@@ -813,8 +813,6 @@ OnMouseInterruptComplete (
return EFI_SUCCESS;
}
- UsbMouseAbsolutePointerDevice->StateChanged = TRUE;
-
//
// Check mouse Data
// USB HID Specification specifies following data format:
@@ -827,6 +825,12 @@ OnMouseInterruptComplete (
// 2 0 to 7 Y displacement
// 3 to n 0 to 7 Device specific (optional)
//
+ if ((Data != NULL) && (DataLength < 3)) {
+return EFI_DEVICE_ERROR;
+ }
+
+ UsbMouseAbsolutePointerDevice->StateChanged = TRUE;
+
UsbMouseAbsolutePointerDevice->State.ActiveButtons = *(UINT8 *) Data & (BIT0
| BIT1 | BIT2);
UsbMouseAbsolutePointerDevice->State.CurrentX =
--
2.16.1.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 04/11] MdeModulePkg/UsbBus: Reject descriptor whose length is bad
Today's implementation doesn't check whether the length of
descriptor is valid before using it.
The patch fixes this issue.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni
Cc: Star Zeng
Cc: Jiewen Yao
---
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
index a93060deea..d9bc1f9e28 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
@@ -767,6 +767,13 @@ UsbGetOneConfig (
DEBUG (( EFI_D_INFO, "UsbGetOneConfig: total length is %d\n",
Desc.TotalLength));
+ //
+ // Reject if TotalLength even cannot cover itself.
+ //
+ if (Desc.TotalLength < OFFSET_OF (EFI_USB_CONFIG_DESCRIPTOR, TotalLength) +
sizeof (Desc.TotalLength)) {
+return NULL;
+ }
+
Buf = AllocateZeroPool (Desc.TotalLength);
if (Buf == NULL) {
--
2.16.1.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 06/11] SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni
Cc: Hao A Wu
---
.../DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c | 7 +++
1 file changed, 7 insertions(+)
diff --git
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
index fb48010a9a..fda43279a3 100644
---
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
+++
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
@@ -556,6 +556,13 @@ XhcDataTransfer (
XhcExecTransfer (Handle, Urb, Timeout);
+ //
+ // Make sure the data received from HW can fit in the received buffer.
+ //
+ if (Urb->Completed > *DataLength) {
+return EFI_DEVICE_ERROR;
+ }
+
*DataLength = Urb->Completed;
Status = EFI_TIMEOUT;
--
2.16.1.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 05/11] MdeModulePkg/Usb: Make sure data from HW is no more than expected
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni
Cc: Jiewen Yao
Cc: Star Zeng
Cc: Hao A Wu
---
MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 9 ++---
MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 7 ---
MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 9 ++---
3 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
index fea6f47f4c..168280be81 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
@@ -1009,9 +1009,12 @@ EhcMonitorAsyncRequests (
ProcBuf = NULL;
if (Urb->Result == EFI_USB_NOERROR) {
- ASSERT (Urb->Completed <= Urb->DataLen);
-
- ProcBuf = AllocatePool (Urb->Completed);
+ //
+ // Make sure the data received from HW is no more than expected.
+ //
+ if (Urb->Completed <= Urb->DataLen) {
+ProcBuf = AllocatePool (Urb->Completed);
+ }
if (ProcBuf == NULL) {
EhcUpdateAsyncRequest (Ehc, Urb);
diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
index 90f010c998..f7510f3ec0 100644
--- a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
+++ b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
@@ -2,7 +2,7 @@
The EHCI register operation routines.
-Copyright (c) 2007 - 2013, Intel Corporation. All rights reserved.
+Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD
License
which accompanies this distribution. The full text of the license may be
found at
@@ -1001,11 +1001,12 @@ UhciMonitorAsyncReqList (
//
// Copy the data to temporary buffer if there are some
-// data transferred. We may have zero-length packet
+// data transferred. We may have zero-length packet.
+// Make sure the data received from HW is no more than expected.
//
Data = NULL;
-if (QhResult.Complete != 0) {
+if ((QhResult.Complete != 0) && (QhResult.Complete <= AsyncReq->DataLen)) {
Data = AllocatePool (QhResult.Complete);
if (Data == NULL) {
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 6a2ef4cd5d..166c44bf5e 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -1556,9 +1556,12 @@ XhcMonitorAsyncRequests (
//
ProcBuf = NULL;
if (Urb->Result == EFI_USB_NOERROR) {
- ASSERT (Urb->Completed <= Urb->DataLen);
-
- ProcBuf = AllocateZeroPool (Urb->Completed);
+ //
+ // Make sure the data received from HW is no more than expected.
+ //
+ if (Urb->Completed <= Urb->DataLen) {
+ProcBuf = AllocateZeroPool (Urb->Completed);
+ }
if (ProcBuf == NULL) {
XhcUpdateAsyncRequest (Xhc, Urb);
--
2.16.1.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 03/11] MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors
Today's implementation reads the Type/Length field in the USB
descriptors data without checking whether the offset to read is
beyond the data boundary.
The patch fixes this issue.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni
Cc: Star Zeng
Cc: Jiewen Yao
---
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c | 50
1 file changed, 38 insertions(+), 12 deletions(-)
diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
index 061e4622e8..a93060deea 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
@@ -177,6 +177,17 @@ UsbCreateDesc (
DescLen = sizeof (EFI_USB_ENDPOINT_DESCRIPTOR);
CtrlLen = sizeof (USB_ENDPOINT_DESC);
break;
+
+ default:
+ASSERT (FALSE);
+return NULL;
+ }
+
+ //
+ // Total length is too small that cannot hold the single descriptor header
plus data.
+ //
+ if (Len <= sizeof (USB_DESC_HEAD)) {
+return NULL;
}
//
@@ -184,24 +195,39 @@ UsbCreateDesc (
// format. Skip the descriptor that isn't of this Type
//
Offset = 0;
- Head = (USB_DESC_HEAD*)DescBuf;
+ Head = (USB_DESC_HEAD *)DescBuf;
+ while (Offset < Len - sizeof (USB_DESC_HEAD)) {
+//
+// Above condition make sure Head->Len and Head->Type are safe to access
+//
+Head = (USB_DESC_HEAD *)&DescBuf[Offset];
- while ((Offset < Len) && (Head->Type != Type)) {
-Offset += Head->Len;
-if (Len <= Offset) {
- DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor, Beyond
boundary!\n"));
+if (Head->Len == 0) {
+ DEBUG ((DEBUG_ERROR, "UsbCreateDesc: met mal-format descriptor,
Head->Len = 0!\n"));
return NULL;
}
-Head= (USB_DESC_HEAD*)(DescBuf + Offset);
-if (Head->Len == 0) {
- DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor,
Head->Len = 0!\n"));
+
+//
+// Make sure no overflow when adding Head->Len to Offset.
+//
+if (Head->Len > MAX_UINTN - Offset) {
+ DEBUG ((DEBUG_ERROR, "UsbCreateDesc: met mal-format descriptor,
Head->Len = %d!\n", Head->Len));
return NULL;
}
+
+Offset += Head->Len;
+
+if (Head->Type == Type) {
+ break;
+}
}
- if ((Len <= Offset) || (Len < Offset + Head->Len) ||
- (Head->Type != Type) || (Head->Len < DescLen)) {
-DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor\n"));
+ //
+ // Head->Len is invalid resulting data beyond boundary, or
+ // Descriptor cannot be found: No such type.
+ //
+ if ((Len < Offset) || (Head->Type != Type) || (Head->Len < DescLen)) {
+DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor.
Offset/Len = %d/%d, Header(T/L) = %d/%d\n", Offset, Len, Head->Type,
Head->Len));
return NULL;
}
@@ -212,7 +238,7 @@ UsbCreateDesc (
CopyMem (Desc, Head, (UINTN) DescLen);
- *Consumed = Offset + Head->Len;
+ *Consumed = Offset;
return Desc;
}
--
2.16.1.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 00/11] Need to validate data from HW
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1250 The patches contain logic to check the data from HW before using. It can avoid corrupted data from HW causes software behave abnormally. Hao Wu (1): MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected Ruiyu Ni (10): MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16) MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1 MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors MdeModulePkg/UsbBus: Reject descriptor whose length is bad MdeModulePkg/Usb: Make sure data from HW is no more than expected SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer MdeModulePkg/UsbKb: Don't access key codes when length is wrong MdeModulePkg/AbsPointer: Don't access key codes when length is wrong MdeModulePkg/UsbMouse: Don't access key codes when length is wrong MdeModulePkg/UsbBus: Deny when the string descriptor length is odd MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 9 +- MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 7 +- MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 9 +- MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c| 19 +- .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c| 30 ++- MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c | 59 - MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c | 4 + .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c| 269 + .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h| 76 ++ .../Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c| 8 +- .../UsbMouseAbsolutePointer.c | 8 +- MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c| 8 +- .../DebugCommunicationLibUsb3Transfer.c| 7 + 13 files changed, 219 insertions(+), 294 deletions(-) -- 2.16.1.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 01/11] MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16)
The change doesn't have functionality impact.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni
Cc: Star Zeng
---
.../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c| 248 +
.../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h| 76 ++-
.../Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c| 8 +-
3 files changed, 80 insertions(+), 252 deletions(-)
diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
index dd5950c54c..581571ab45 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
@@ -792,32 +792,34 @@ UsbBootDetectMedia (
/**
- Read some blocks from the device.
+ Read or write some blocks from the device.
- @param UsbMassThe USB mass storage device to read from
+ @param UsbMassThe USB mass storage device to access
+ @param Write TRUE for write operation.
@param LbaThe start block number
- @param TotalBlock Total block number to read
- @param Buffer The buffer to read to
+ @param TotalBlock Total block number to read or write
+ @param Buffer The buffer to read to or write from
- @retval EFI_SUCCESSData are read into the buffer
- @retval Others Failed to read all the data
+ @retval EFI_SUCCESSData are read into the buffer or writen into
the device.
+ @retval Others Failed to read or write all the data
**/
EFI_STATUS
-UsbBootReadBlocks (
+UsbBootReadWriteBlocks (
IN USB_MASS_DEVICE *UsbMass,
+ IN BOOLEAN Write,
IN UINT32Lba,
IN UINTN TotalBlock,
- OUT UINT8 *Buffer
+ IN OUT UINT8 *Buffer
)
{
- USB_BOOT_READ10_CMD ReadCmd;
- EFI_STATUSStatus;
- UINT16Count;
- UINT16CountMax;
- UINT32BlockSize;
- UINT32ByteSize;
- UINT32Timeout;
+ USB_BOOT_READ_WRITE_10_CMD Cmd;
+ EFI_STATUS Status;
+ UINT16 Count;
+ UINT16 CountMax;
+ UINT32 BlockSize;
+ UINT32 ByteSize;
+ UINT32 Timeout;
BlockSize = UsbMass->BlockIoMedia.BlockSize;
CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize);
@@ -840,17 +842,17 @@ UsbBootReadBlocks (
//
// Fill in the command then execute
//
-ZeroMem (&ReadCmd, sizeof (USB_BOOT_READ10_CMD));
+ZeroMem (&Cmd, sizeof (USB_BOOT_READ_WRITE_10_CMD));
-ReadCmd.OpCode = USB_BOOT_READ10_OPCODE;
-ReadCmd.Lun = (UINT8) (USB_BOOT_LUN (UsbMass->Lun));
-WriteUnaligned32 ((UINT32 *) ReadCmd.Lba, SwapBytes32 (Lba));
-WriteUnaligned16 ((UINT16 *) ReadCmd.TransferLen, SwapBytes16 (Count));
+Cmd.OpCode = Write ? USB_BOOT_WRITE10_OPCODE : USB_BOOT_READ10_OPCODE;
+Cmd.Lun = (UINT8) (USB_BOOT_LUN (UsbMass->Lun));
+WriteUnaligned32 ((UINT32 *) Cmd.Lba, SwapBytes32 (Lba));
+WriteUnaligned16 ((UINT16 *) Cmd.TransferLen, SwapBytes16 (Count));
Status = UsbBootExecCmdWithRetry (
UsbMass,
- &ReadCmd,
- (UINT8) sizeof (USB_BOOT_READ10_CMD),
+ &Cmd,
+ (UINT8) sizeof (USB_BOOT_READ_WRITE_10_CMD),
EfiUsbDataIn,
Buffer,
ByteSize,
@@ -859,86 +861,11 @@ UsbBootReadBlocks (
if (EFI_ERROR (Status)) {
return Status;
}
-DEBUG ((EFI_D_BLKIO, "UsbBootReadBlocks: LBA (0x%x), Blk (0x%x)\n", Lba,
Count));
-Lba+= Count;
-Buffer += Count * BlockSize;
-TotalBlock -= Count;
- }
-
- return Status;
-}
-
-
-/**
- Write some blocks to the device.
-
- @param UsbMassThe USB mass storage device to write to
- @param LbaThe start block number
- @param TotalBlock Total block number to write
- @param Buffer Pointer to the source buffer for the data.
-
- @retval EFI_SUCCESSData are written into the buffer
- @retval Others Failed to write all the data
-
-**/
-EFI_STATUS
-UsbBootWriteBlocks (
- IN USB_MASS_DEVICE *UsbMass,
- IN UINT32 Lba,
- IN UINTN TotalBlock,
- IN UINT8 *Buffer
- )
-{
- USB_BOOT_WRITE10_CMD WriteCmd;
- EFI_STATUSStatus;
- UINT16Count;
- UINT16CountMax;
- UINT32BlockSize;
- UINT32ByteSize;
- UINT32Timeout;
-
- BlockSize = UsbMass->BlockIoMedia.BlockSize;
- CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize);
- Status= EFI_SUCCESS;
-
- while (TotalBlock > 0)
[edk2] [PATCH 02/11] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1
UsbBootReadWriteBlocks() and UsbBootReadWriteBlocks16() use a UINT16
local variable to hold the value of
USB_BOOT_MAX_CARRY_SIZE (=0x1) / BlockSize.
When BlockSize is 1, the UINT16 local variable is set to 0x1
but the high-16 bits are truncated resulting the final value be 0.
It causes the while-loop in the two functions accesses 0 block in
each loop, resulting the loop never ends.
The patch fixes the two functions to make sure no integer overflow
happens.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni
Cc: Star Zeng
Cc: Steven Shi
---
.../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c| 27 +++---
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
index 581571ab45..37fbeedbeb 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
@@ -815,14 +815,14 @@ UsbBootReadWriteBlocks (
{
USB_BOOT_READ_WRITE_10_CMD Cmd;
EFI_STATUS Status;
- UINT16 Count;
- UINT16 CountMax;
+ UINT32 Count;
+ UINT32 CountMax;
UINT32 BlockSize;
UINT32 ByteSize;
UINT32 Timeout;
BlockSize = UsbMass->BlockIoMedia.BlockSize;
- CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize);
+ CountMax = USB_BOOT_MAX_CARRY_SIZE / BlockSize;
Status= EFI_SUCCESS;
while (TotalBlock > 0) {
@@ -831,8 +831,9 @@ UsbBootReadWriteBlocks (
// on the device. We must split the total block because the READ10
// command only has 16 bit transfer length (in the unit of block).
//
-Count = (UINT16)((TotalBlock < CountMax) ? TotalBlock : CountMax);
-ByteSize = (UINT32)Count * BlockSize;
+Count= (UINT32)MIN (TotalBlock, CountMax);
+Count= MIN (MAX_UINT16, Count);
+ByteSize = Count * BlockSize;
//
// USB command's upper limit timeout is 5s. [USB2.0-9.2.6.1]
@@ -847,7 +848,7 @@ UsbBootReadWriteBlocks (
Cmd.OpCode = Write ? USB_BOOT_WRITE10_OPCODE : USB_BOOT_READ10_OPCODE;
Cmd.Lun = (UINT8) (USB_BOOT_LUN (UsbMass->Lun));
WriteUnaligned32 ((UINT32 *) Cmd.Lba, SwapBytes32 (Lba));
-WriteUnaligned16 ((UINT16 *) Cmd.TransferLen, SwapBytes16 (Count));
+WriteUnaligned16 ((UINT16 *) Cmd.TransferLen, SwapBytes16 ((UINT16)Count));
Status = UsbBootExecCmdWithRetry (
UsbMass,
@@ -867,7 +868,7 @@ UsbBootReadWriteBlocks (
Lba, Count
));
Lba+= Count;
-Buffer += Count * BlockSize;
+Buffer += ByteSize;
TotalBlock -= Count;
}
@@ -897,22 +898,22 @@ UsbBootReadWriteBlocks16 (
{
UINT8 Cmd[16];
EFI_STATUSStatus;
- UINT16Count;
- UINT16CountMax;
+ UINT32Count;
+ UINT32CountMax;
UINT32BlockSize;
UINT32ByteSize;
UINT32Timeout;
BlockSize = UsbMass->BlockIoMedia.BlockSize;
- CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize);
+ CountMax = USB_BOOT_MAX_CARRY_SIZE / BlockSize;
Status= EFI_SUCCESS;
while (TotalBlock > 0) {
//
// Split the total blocks into smaller pieces.
//
-Count = (UINT16)((TotalBlock < CountMax) ? TotalBlock : CountMax);
-ByteSize = (UINT32)Count * BlockSize;
+Count= (UINT32)MIN (TotalBlock, CountMax);
+ByteSize = Count * BlockSize;
//
// USB command's upper limit timeout is 5s. [USB2.0-9.2.6.1]
@@ -947,7 +948,7 @@ UsbBootReadWriteBlocks16 (
Lba, Count
));
Lba+= Count;
-Buffer += Count * BlockSize;
+Buffer += ByteSize;
TotalBlock -= Count;
}
--
2.16.1.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v1 2/7] MdeModulePkg/UdfDxe: ASSERT for false positives of NULL ptr deref
This commit adds ASSERTs to address false positive reports of NULL
pointer dereference issues raised from static analysis with regard to
function ReadDirectoryEntry().
Cc: Paulo Alcantara
Cc: Ruiyu Ni
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdeModulePkg/Universal/Disk/UdfDxe/File.c | 9 +
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 9 +
2 files changed, 18 insertions(+)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 6f07bf2066..2249f4ea0e 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -408,6 +408,15 @@ UdfRead (
goto Done;
}
+ //
+ // After calling function ReadDirectoryEntry(), if
'NewFileIdentifierDesc'
+ // is NULL, then the 'Status' must be EFI_OUT_OF_RESOURCES. Hence, if the
+ // code reaches here, 'NewFileIdentifierDesc' must be not NULL.
+ //
+ // The ASSERT here is for addressing a false positive NULL pointer
+ // dereference issue raised from static analysis.
+ //
+ ASSERT (NewFileIdentifierDesc != NULL);
if (!IS_FID_PARENT_FILE (NewFileIdentifierDesc)) {
break;
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 638f31bd82..8b58cc9eb1 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -1404,6 +1404,15 @@ InternalFindFile (
break;
}
+//
+// After calling function ReadDirectoryEntry(), if 'FileIdentifierDesc' is
+// NULL, then the 'Status' must be EFI_OUT_OF_RESOURCES. Hence, if the code
+// reaches here, 'FileIdentifierDesc' must be not NULL.
+//
+// The ASSERT here is for addressing a false positive NULL pointer
+// dereference issue raised from static analysis.
+//
+ASSERT (FileIdentifierDesc != NULL);
if (FileIdentifierDesc->FileCharacteristics & PARENT_FILE) {
//
--
2.12.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v1 7/7] MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1249
We found potential dead codes within File.c during the code coverage test.
After manual review, we think the below ones are positive reports:
A. For function GetAllocationDescriptor():
Due to the all the calling places for this function, the input parameter
'RecordingFlags' can only with value 'LongAdsSequence' or
'ShortAdsSequence'.
So the below code will never be reached:
return EFI_DEVICE_ERROR;
This commit will add "ASSERT (FALSE);" before the above line to indicate
this.
B. For function GetAllocationDescriptorLsn():
Due to the all the calling places for this function, the input parameter
'RecordingFlags' can only with value 'LongAdsSequence' or
'ShortAdsSequence'.
So the below code will never be reached:
return EFI_UNSUPPORTED;
This commit will add "ASSERT (FALSE);" before the above line to indicate
this.
C. For function SetFileInfo():
Due to the all the calling places for this function, the input parameter
'FileName' will never be a NULL pointer.
So the below codes will never be reached:
} else {
FileInfo->FileName[0] = '\0';
}
This commit will add "ASSERT (FALSE);" before the above lines to indicate
this.
Cc: Paulo Alcantara
Cc: Ruiyu Ni
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 12
1 file changed, 12 insertions(+)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 1400846bf1..19acd0554c 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -738,6 +738,10 @@ GetAllocationDescriptor (
);
}
+ //
+ // Code should never reach here.
+ //
+ ASSERT (FALSE);
return EFI_DEVICE_ERROR;
}
@@ -784,6 +788,10 @@ GetAllocationDescriptorLsn (
return EFI_SUCCESS;
}
+ //
+ // Code should never reach here.
+ //
+ ASSERT (FALSE);
return EFI_UNSUPPORTED;
}
@@ -2413,6 +2421,10 @@ SetFileInfo (
if (FileName != NULL) {
StrCpyS (FileInfo->FileName, StrLen (FileName) + 1, FileName);
} else {
+//
+// Code should never reach here.
+//
+ASSERT (FALSE);
FileInfo->FileName[0] = '\0';
}
--
2.12.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v1 4/7] MdeModulePkg/UdfDxe: Use debug msg instead of ASSERT in UdfOpen()
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1248
Within function UdfOpen():
This commit will use debug messages instead of using ASSERT when an error
occurs after calling GetFileSize().
Cc: Paulo Alcantara
Cc: Ruiyu Ni
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 2249f4ea0e..0730e6a3fa 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -257,8 +257,12 @@ UdfOpen (
&NewPrivFileData->File,
&NewPrivFileData->FileSize
);
- ASSERT_EFI_ERROR (Status);
if (EFI_ERROR (Status)) {
+DEBUG ((
+ DEBUG_ERROR,
+ "%a: GetFileSize() fails with status - %r.\n",
+ __FUNCTION__, Status
+ ));
goto Error_Get_File_Size;
}
--
2.12.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v1 1/7] MdeModulePkg/UdfDxe: Use error handling for memory allocation failure
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1247
For functions DuplicateFid() and DuplicateFe(), this commit will use error
handling logic instead of ASSERTs for memory allocation failure.
Cc: Paulo Alcantara
Cc: Ruiyu Ni
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 40
+---
1 file changed, 34 insertions(+), 6 deletions(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index ecc172303e..638f31bd82 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -468,8 +468,6 @@ DuplicateFid (
*NewFileIdentifierDesc =
(UDF_FILE_IDENTIFIER_DESCRIPTOR *)AllocateCopyPool (
(UINTN) GetFidDescriptorLength (FileIdentifierDesc), FileIdentifierDesc);
-
- ASSERT (*NewFileIdentifierDesc != NULL);
}
/**
@@ -490,8 +488,6 @@ DuplicateFe (
)
{
*NewFileEntry = AllocateCopyPool (Volume->FileEntrySize, FileEntry);
-
- ASSERT (*NewFileEntry != NULL);
}
/**
@@ -1370,7 +1366,15 @@ InternalFindFile (
}
DuplicateFe (BlockIo, Volume, Parent->FileEntry, &File->FileEntry);
+if (File->FileEntry == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+}
+
DuplicateFid (Parent->FileIdentifierDesc, &File->FileIdentifierDesc);
+if (File->FileIdentifierDesc == NULL) {
+ FreePool (File->FileEntry);
+ return EFI_OUT_OF_RESOURCES;
+}
return EFI_SUCCESS;
}
@@ -1732,9 +1736,20 @@ FindFile (
// We've already a file pointer (Root) for the root directory.
Duplicate
// its FE/EFE and FID descriptors.
//
-DuplicateFe (BlockIo, Volume, Root->FileEntry, &File->FileEntry);
-DuplicateFid (Root->FileIdentifierDesc, &File->FileIdentifierDesc);
Status = EFI_SUCCESS;
+DuplicateFe (BlockIo, Volume, Root->FileEntry, &File->FileEntry);
+if (File->FileEntry == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+} else {
+ //
+ // File->FileEntry is not NULL.
+ //
+ DuplicateFid (Root->FileIdentifierDesc, &File->FileIdentifierDesc);
+ if (File->FileIdentifierDesc == NULL) {
+FreePool (File->FileEntry);
+Status = EFI_OUT_OF_RESOURCES;
+ }
+}
}
} else {
//
@@ -1874,6 +1889,9 @@ ReadDirectoryEntry (
} while (FileIdentifierDesc->FileCharacteristics & DELETED_FILE);
DuplicateFid (FileIdentifierDesc, FoundFid);
+ if (*FoundFid == NULL) {
+return EFI_OUT_OF_RESOURCES;
+ }
return EFI_SUCCESS;
}
@@ -2031,8 +2049,18 @@ ResolveSymlink (
// "." (current file). Duplicate both FE/EFE and FID of this file.
//
DuplicateFe (BlockIo, Volume, PreviousFile.FileEntry, &File->FileEntry);
+ if (File->FileEntry == NULL) {
+Status = EFI_OUT_OF_RESOURCES;
+goto Error_Find_File;
+ }
+
DuplicateFid (PreviousFile.FileIdentifierDesc,
&File->FileIdentifierDesc);
+ if (File->FileIdentifierDesc == NULL) {
+FreePool (File->FileEntry);
+Status = EFI_OUT_OF_RESOURCES;
+goto Error_Find_File;
+ }
goto Next_Path_Component;
case 5:
//
--
2.12.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v1 6/7] MdeModulePkg/UdfDxe: Remove dead codes in FileName.c
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1249
We found potential dead codes within File.c during the code coverage test.
After manual review, we think the below ones are positive reports:
A. In function MangleFileName():
FileName = TrimString (FileName);
// Begin of dead codes
if (*FileName == L'\0') {
goto Exit;
}
// End of dead codes
When the code reaches the TrimString() call, the string in 'FileName' is
guaranteed to have a '\' character due to the call patterns of
MangleFileName(). So after trimming the lead-off/tailing white spaces,
string in 'FileName' will not be an empty string.
B. In function MangleFileName():
if (FileName[0] == L'.') {
if (FileName[1] == L'.') {
if (FileName[2] == L'\0') {
goto Exit;
} else {
FileName += 2;
}
} else if (FileName[1] == L'\0') {
goto Exit;
}
}
When the code hits the above checks, string in 'FileName' will always have
a leading '\' character (denoting an absolute path) due to the call
patterns of MangleFileName(). So no leading '.' can be there in string
'FileName'.
This commit will remove those dead codes.
Cc: Paulo Alcantara
Cc: Ruiyu Ni
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdeModulePkg/Universal/Disk/UdfDxe/FileName.c | 15 ---
1 file changed, 15 deletions(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileName.c
b/MdeModulePkg/Universal/Disk/UdfDxe/FileName.c
index 36551a4dba..18549e4e45 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileName.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileName.c
@@ -128,9 +128,6 @@ MangleFileName (
}
FileName = TrimString (FileName);
- if (*FileName == L'\0') {
-goto Exit;
- }
if ((StrLen (FileName) > 1) && (FileName[StrLen (FileName) - 1] == L'\\')) {
FileName[StrLen (FileName) - 1] = L'\0';
@@ -138,18 +135,6 @@ MangleFileName (
FileNameSavedPointer = FileName;
- if (FileName[0] == L'.') {
-if (FileName[1] == L'.') {
- if (FileName[2] == L'\0') {
-goto Exit;
- } else {
-FileName += 2;
- }
-} else if (FileName[1] == L'\0') {
- goto Exit;
-}
- }
-
while (*FileName != L'\0') {
if (*FileName == L'\\') {
FileName = ExcludeTrailingBackslashes (FileName);
--
2.12.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v1 3/7] MdeModulePkg/UdfDxe: Use error handling when fail to return LSN
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1248
This commit will refine the ASSERTs in function GetLongAdLsn() and
GetAllocationDescriptorLsn() when the logical sector number cannot be
returned due to unrecognized media format.
Error handling logic will be used for those ASSERTs.
Cc: Paulo Alcantara
Cc: Ruiyu Ni
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 101
+---
1 file changed, 69 insertions(+), 32 deletions(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 8b58cc9eb1..1400846bf1 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -271,26 +271,39 @@ GetPdFromLongAd (
/**
Return logical sector number of a given Long Allocation Descriptor.
- @param[in] Volume Volume information pointer.
- @param[in] LongAd Long Allocation Descriptor pointer.
+ @param[in] Volume Volume information pointer.
+ @param[in] LongAd Long Allocation Descriptor pointer.
+ @param[out] LsnLogical sector number pointer.
- @return The logical sector number of a given Long Allocation Descriptor.
+ @retval EFI_SUCCESS Logical sector number successfully returned.
+ @retval EFI_UNSUPPORTED Logical sector number is not returned due to
+ unrecognized format.
**/
-UINT64
+EFI_STATUS
GetLongAdLsn (
- IN UDF_VOLUME_INFO *Volume,
- IN UDF_LONG_ALLOCATION_DESCRIPTOR *LongAd
+ IN UDF_VOLUME_INFO *Volume,
+ IN UDF_LONG_ALLOCATION_DESCRIPTOR *LongAd,
+ OUT UINT64 *Lsn
)
{
UDF_PARTITION_DESCRIPTOR *PartitionDesc;
PartitionDesc = GetPdFromLongAd (Volume, LongAd);
- ASSERT (PartitionDesc != NULL);
+ if (PartitionDesc == NULL) {
+DEBUG ((
+ DEBUG_ERROR,
+ "%a: Fail to get the Partition Descriptor from the given Long Allocation
Descriptor.\n",
+ __FUNCTION__
+ ));
+return EFI_UNSUPPORTED;
+ }
- return (UINT64)PartitionDesc->PartitionStartingLocation -
-Volume->MainVdsStartLocation +
-LongAd->ExtentLocation.LogicalBlockNumber;
+ *Lsn = (UINT64)PartitionDesc->PartitionStartingLocation -
+ Volume->MainVdsStartLocation +
+ LongAd->ExtentLocation.LogicalBlockNumber;
+
+ return EFI_SUCCESS;
}
/**
@@ -342,7 +355,10 @@ FindFileSetDescriptor (
UDF_DESCRIPTOR_TAG *DescriptorTag;
LogicalVolDesc = &Volume->LogicalVolDesc;
- Lsn = GetLongAdLsn (Volume, &LogicalVolDesc->LogicalVolumeContentsUse);
+ Status = GetLongAdLsn (Volume, &LogicalVolDesc->LogicalVolumeContentsUse,
&Lsn);
+ if (EFI_ERROR (Status)) {
+return Status;
+ }
//
// As per UDF 2.60 specification:
@@ -732,34 +748,43 @@ GetAllocationDescriptor (
@param[in] Volume Volume information pointer.
@param[in] ParentIcb Long Allocation Descriptor pointer.
@param[in] Ad Allocation Descriptor pointer.
+ @param[out] Lsn Logical sector number pointer.
- @return The logical sector number of the given Allocation Descriptor.
+ @retval EFI_SUCCESS Logical sector number of the given Allocation
+ Descriptor successfully returned.
+ @retval EFI_UNSUPPORTED Logical sector number of the given Allocation
+ Descriptor is not returned due to
unrecognized
+ format.
**/
-UINT64
+EFI_STATUS
GetAllocationDescriptorLsn (
- IN UDF_FE_RECORDING_FLAGS RecordingFlags,
- IN UDF_VOLUME_INFO *Volume,
- IN UDF_LONG_ALLOCATION_DESCRIPTOR *ParentIcb,
- IN VOID*Ad
+ IN UDF_FE_RECORDING_FLAGS RecordingFlags,
+ IN UDF_VOLUME_INFO *Volume,
+ IN UDF_LONG_ALLOCATION_DESCRIPTOR *ParentIcb,
+ IN VOID*Ad,
+ OUT UINT64 *Lsn
)
{
UDF_PARTITION_DESCRIPTOR *PartitionDesc;
if (RecordingFlags == LongAdsSequence) {
-return GetLongAdLsn (Volume, (UDF_LONG_ALLOCATION_DESCRIPTOR *)Ad);
+return GetLongAdLsn (Volume, (UDF_LONG_ALLOCATION_DESCRIPTOR *)Ad, Lsn);
} else if (RecordingFlags == ShortAdsSequence) {
PartitionDesc = GetPdFromLongAd (Volume, ParentIcb);
-ASSERT (PartitionDesc != NULL);
+if (PartitionDesc == NULL) {
+ return EFI_UNSUPPORTED;
+}
-return GetShortAdLsn (
- Volume,
- PartitionDesc,
- (UDF_SHORT_ALLOCATION_DESCRIPTOR *)Ad
- );
+*Lsn = GetShortAdLsn (
+ Volume,
+ PartitionDesc,
+ (UDF_SHORT_ALLOCATION_DESCRIPTOR *)Ad
+ );
+return EFI_SUCCESS;
}
-
[edk2] [PATCH v1 5/7] MdeModulePkg/UdfDxe: Handle dead codes in File.c
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1249
We found potential dead codes within File.c during the code coverage test.
After manual review, we think the below ones are positive reports:
A. In function UdfRead():
else if (IS_FID_DELETED_FILE (Parent->FileIdentifierDesc)) {
Status = EFI_DEVICE_ERROR;
}
A File Identifier Descriptor will be get from the UDF media only by
function ReadDirectoryEntry(). And within this function, all the File
Identifier Descriptor with 'DELETED_FILE' characteristics will be skipped
and will not be returned. Hence, the above codes in function UdfRead()
will never be hit.
This commit will add "ASSERT (FALSE);" before the above line to indicate
this.
Cc: Paulo Alcantara
Cc: Ruiyu Ni
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdeModulePkg/Universal/Disk/UdfDxe/File.c | 4
1 file changed, 4 insertions(+)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 0730e6a3fa..eb7d79692b 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -500,6 +500,10 @@ UdfRead (
PrivFileData->FilePosition++;
Status = EFI_SUCCESS;
} else if (IS_FID_DELETED_FILE (Parent->FileIdentifierDesc)) {
+//
+// Code should never reach here.
+//
+ASSERT (FALSE);
Status = EFI_DEVICE_ERROR;
}
--
2.12.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v1 0/7] Code refinements in UdfDxe
This series will refine the codes in MdeModulePkg/Universal/Disk/UdfDxe for: A. Refine asserts used for memory allocation failure and error cases that are possible to happen. Will use error handling logic for them; B. Address some dead codes within this module. Cc: Paulo Alcantara Cc: Ruiyu Ni Cc: Star Zeng Hao Wu (7): MdeModulePkg/UdfDxe: Use error handling for memory allocation failure MdeModulePkg/UdfDxe: ASSERT for false positives of NULL ptr deref MdeModulePkg/UdfDxe: Use error handling when fail to return LSN MdeModulePkg/UdfDxe: Use debug msg instead of ASSERT in UdfOpen() MdeModulePkg/UdfDxe: Handle dead codes in File.c MdeModulePkg/UdfDxe: Remove dead codes in FileName.c MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c MdeModulePkg/Universal/Disk/UdfDxe/File.c | 19 ++- MdeModulePkg/Universal/Disk/UdfDxe/FileName.c | 15 -- MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 162 +++- 3 files changed, 142 insertions(+), 54 deletions(-) -- 2.12.0.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 1/1] BaseTools: Update Makefile to exclude deleted file.
GenFds/Attributes.py has been deleted in 1ad635b, but was not removed from the Python Makefile. Remove it for successful compilation on Windows. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Marvin Haeuser --- BaseTools/Source/Python/Makefile | 1 - 1 file changed, 1 deletion(-) diff --git a/BaseTools/Source/Python/Makefile b/BaseTools/Source/Python/Makefile index ac99259eb18a..2a80f1cf7fe9 100644 --- a/BaseTools/Source/Python/Makefile +++ b/BaseTools/Source/Python/Makefile @@ -114,7 +114,6 @@ CMD_BUILD=$(BASE_TOOLS_PATH)\Source\Python\build\BuildReport.py \ $(BASE_TOOLS_PATH)\Source\Python\Eot\Report.py CMD_GENFDS=$(BASE_TOOLS_PATH)\Source\Python\GenFds\AprioriSection.py \ -$(BASE_TOOLS_PATH)\Source\Python\GenFds\Attribute.py \ $(BASE_TOOLS_PATH)\Source\Python\GenFds\Capsule.py \ $(BASE_TOOLS_PATH)\Source\Python\GenFds\CapsuleData.py \ $(BASE_TOOLS_PATH)\Source\Python\GenFds\ComponentStatement.py \ -- 2.19.1.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [Patch 2/4] UefiCpuPkg/RegisterCpuFeaturesLib.h: Add new dependence types.
Add new core/package dependence types which consumed by different MSRs.
Cc: Ruiyu Ni
Cc: Laszlo Ersek
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong
---
.../Include/Library/RegisterCpuFeaturesLib.h | 25 ++
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
index 9331e49d13..e6f0ebe4bc 100644
--- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
+++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
@@ -73,10 +73,17 @@
#define CPU_FEATURE_PPIN(32+11)
#define CPU_FEATURE_PROC_TRACE (32+12)
-#define CPU_FEATURE_BEFORE_ALL BIT27
-#define CPU_FEATURE_AFTER_ALL BIT28
-#define CPU_FEATURE_BEFORE BIT29
-#define CPU_FEATURE_AFTER BIT30
+#define CPU_FEATURE_BEFORE_ALL BIT23
+#define CPU_FEATURE_AFTER_ALL BIT24
+#define CPU_FEATURE_BEFORE BIT25
+#define CPU_FEATURE_AFTER BIT26
+
+#define CPU_FEATURE_THREAD_BEFORE CPU_FEATURE_BEFORE
+#define CPU_FEATURE_THREAD_AFTERCPU_FEATURE_AFTER
+#define CPU_FEATURE_CORE_BEFORE BIT27
+#define CPU_FEATURE_CORE_AFTER BIT28
+#define CPU_FEATURE_PACKAGE_BEFORE BIT29
+#define CPU_FEATURE_PACKAGE_AFTER BIT30
#define CPU_FEATURE_END MAX_UINT32
/// @}
@@ -116,6 +123,16 @@ typedef struct {
CPUID_VERSION_INFO_EDX CpuIdVersionInfoEdx;
} REGISTER_CPU_FEATURE_INFORMATION;
+//
+// Describe the dependency type for different features.
+//
+typedef enum {
+ NoneDepType,
+ ThreadDepType,
+ CoreDepType,
+ PackageDepType
+} CPU_FEATURE_DEPENDENCE_TYPE;
+
/**
Determines if a CPU feature is enabled in PcdCpuFeaturesSupport bit mask.
If a CPU feature is disabled in PcdCpuFeaturesSupport then all the code/data
--
2.15.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [Patch 0/4] Fix performance issue caused by Set MSR task.
In a system which has multiple cores, current set register value task costs
huge times.
After investigation, current set MSR task costs most of the times. Current
logic uses
SpinLock to let set MSR task as an single thread task for all cores. Because
MSR has
scope attribute which may cause GP fault if multiple APs set MSR at the same
time,
current logic use an easiest solution (use SpinLock) to avoid this issue, but
it will
cost huge times.
In order to fix this performance issue, new solution will set MSRs base on
their scope
attribute. After this, the SpinLock will not needed. Without SpinLock, new
issue raised
which is caused by MSR dependence. For example, MSR A depends on MSR B which
means MSR A
must been set after MSR B has been set. Also MSR B is package scope level and
MSR A is
thread scope level. If system has multiple threads, Thread 1 needs to set the
thread level
MSRs and thread 2 needs to set thread and package level MSRs. Set MSRs task for
thread 1
and thread 2 like below:
Thread 1 Thread 2
MSR B NY
MSR A YY
If driver don't control execute MSR order, for thread 1, it will execute MSR A
first, but
at this time, MSR B not been executed yet by thread 2. system may trig
exception at this
time.
In order to fix the above issue, driver introduces semaphore logic to control
the MSR
execute sequence. For the above case, a semaphore will be add between MSR A and
B for
all threads. Semaphore has scope info for it. The possible scope value is core
or package.
For each thread, when it meets a semaphore during it set registers, it will 1)
release
semaphore (+1) for each threads in this core or package(based on the scope info
for this
semaphore) 2) acquire semaphore (-1) for all the threads in this core or
package(based
on the scope info for this semaphore). With these two steps, driver can control
MSR
sequence. Sample code logic like below:
//
// First increase semaphore count by 1 for processors in this package.
//
for (ProcessorIndex = 0; ProcessorIndex < PackageThreadsCount ;
ProcessorIndex ++) {
LibReleaseSemaphore ((UINT32 *) &SemaphorePtr[PackageOffset +
ProcessorIndex]);
}
//
// Second, check whether the count has reach the check number.
//
for (ProcessorIndex = 0; ProcessorIndex < ValidApCount; ProcessorIndex ++) {
LibWaitForSemaphore (&SemaphorePtr[ApOffset]);
}
Platform Requirement:
1. This change requires register MSR setting base on MSR scope info. If still
register MSR
for all threads, exception may raised.
Known limitation:
1. Current CpuFeatures driver supports DXE instance and PEI instance. But
semaphore logic
requires Aps execute in async mode which is not supported by PEI driver. So
CpuFeature
PEI instance not works after this change. We plan to support async mode for
PEI in phase
2 for this task.
2. Current execute MSR task code in duplicated in PiSmmCpuDxeSmm driver and
RegisterCpuFeaturesLib library because the schedule limitation. Will merge
the code to
RegisterCpuFeaturesLib and export as an API in phase 2 for this task.
Extra Notes:
I will send the other patch to set MSR base on scope info and check in it
before check in
this serial.
Cc: Ruiyu Ni
Cc: Laszlo Ersek
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong
Eric Dong (4):
UefiCpuPkg/Include/AcpiCpuData.h: Add Semaphore related Information.
UefiCpuPkg/RegisterCpuFeaturesLib.h: Add new dependence types.
UefiCpuPkg/RegisterCpuFeaturesLib: Add logic to support semaphore
type.
UefiCpuPkg/PiSmmCpuDxeSmm: Add logic to support semaphore type.
UefiCpuPkg/Include/AcpiCpuData.h | 23 +-
.../Include/Library/RegisterCpuFeaturesLib.h | 25 +-
.../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 324 ---
.../DxeRegisterCpuFeaturesLib.c| 71 +++-
.../DxeRegisterCpuFeaturesLib.inf | 3 +
.../PeiRegisterCpuFeaturesLib.c| 55 ++-
.../PeiRegisterCpuFeaturesLib.inf | 1 +
.../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 51 ++-
.../RegisterCpuFeaturesLib.c | 452 ++---
UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 316 +++---
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 3 -
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 3 +-
12 files changed, 1063 insertions(+), 264 deletions(-)
--
2.15.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [Patch 3/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add logic to support semaphore type.
In a system which has multiple cores, current set register value task costs
huge times.
After investigation, current set MSR task costs most of the times. Current
logic uses
SpinLock to let set MSR task as an single thread task for all cores. Because
MSR has
scope attribute which may cause GP fault if multiple APs set MSR at the same
time,
current logic use an easiest solution (use SpinLock) to avoid this issue, but
it will
cost huge times.
In order to fix this performance issue, new solution will set MSRs base on
their scope
attribute. After this, the SpinLock will not needed. Without SpinLock, new
issue raised
which is caused by MSR dependence. For example, MSR A depends on MSR B which
means MSR A
must been set after MSR B has been set. Also MSR B is package scope level and
MSR A is
thread scope level. If system has multiple threads, Thread 1 needs to set the
thread level
MSRs and thread 2 needs to set thread and package level MSRs. Set MSRs task for
thread 1
and thread 2 like below:
Thread 1 Thread 2
MSR B NY
MSR A YY
If driver don't control execute MSR order, for thread 1, it will execute MSR A
first, but
at this time, MSR B not been executed yet by thread 2. system may trig
exception at this
time.
In order to fix the above issue, driver introduces semaphore logic to control
the MSR
execute sequence. For the above case, a semaphore will be add between MSR A and
B for
all threads. Semaphore has scope info for it. The possible scope value is core
or package.
For each thread, when it meets a semaphore during it set registers, it will 1)
release
semaphore (+1) for each threads in this core or package(based on the scope info
for this
semaphore) 2) acquire semaphore (-1) for all the threads in this core or
package(based
on the scope info for this semaphore). With these two steps, driver can control
MSR
sequence. Sample code logic like below:
//
// First increase semaphore count by 1 for processors in this package.
//
for (ProcessorIndex = 0; ProcessorIndex < PackageThreadsCount ;
ProcessorIndex ++) {
LibReleaseSemaphore ((UINT32 *) &SemaphorePtr[PackageOffset +
ProcessorIndex]);
}
//
// Second, check whether the count has reach the check number.
//
for (ProcessorIndex = 0; ProcessorIndex < ValidApCount; ProcessorIndex ++) {
LibWaitForSemaphore (&SemaphorePtr[ApOffset]);
}
Platform Requirement:
1. This change requires register MSR setting base on MSR scope info. If still
register MSR
for all threads, exception may raised.
Known limitation:
1. Current CpuFeatures driver supports DXE instance and PEI instance. But
semaphore logic
requires Aps execute in async mode which is not supported by PEI driver. So
CpuFeature
PEI instance not works after this change. We plan to support async mode for
PEI in phase
2 for this task.
Cc: Ruiyu Ni
Cc: Laszlo Ersek
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong
---
.../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 324 ---
.../DxeRegisterCpuFeaturesLib.c| 71 +++-
.../DxeRegisterCpuFeaturesLib.inf | 3 +
.../PeiRegisterCpuFeaturesLib.c| 55 ++-
.../PeiRegisterCpuFeaturesLib.inf | 1 +
.../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 51 ++-
.../RegisterCpuFeaturesLib.c | 452 ++---
7 files changed, 840 insertions(+), 117 deletions(-)
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
index ba3fb3250f..f820b4fed7 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
@@ -145,6 +145,20 @@ CpuInitDataInitialize (
CPU_FEATURES_INIT_ORDER *InitOrder;
CPU_FEATURES_DATA*CpuFeaturesData;
LIST_ENTRY *Entry;
+ UINT32 Core;
+ UINT32 Package;
+ UINT32 Thread;
+ EFI_CPU_PHYSICAL_LOCATION*Location;
+ UINT32 *CoreArray;
+ UINTNIndex;
+ UINT32 ValidCount;
+ UINTNCoreIndex;
+ ACPI_CPU_DATA*AcpiCpuData;
+ CPU_STATUS_INFORMATION *CpuStatus;
+
+ Core= 0;
+ Package = 0;
+ Thread = 0;
CpuFeaturesData = GetCpuFeaturesData ();
CpuFeaturesData->InitOrder = AllocateZeroPool (sizeof
(CPU_FEATURES_INIT_ORDER) * NumberOfCpus);
@@ -163,6 +177,16 @@ CpuInitDataInitialize (
Entry = Entry->ForwardLink;
}
+ CpuFeaturesData->NumberOfCpus = (UINT32) NumberOfCpus;
+
+ AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64 (PcdCp
[edk2] [Patch 1/4] UefiCpuPkg/Include/AcpiCpuData.h: Add Semaphore related Information.
In order to support semaphore related logic, add new definition for it.
Cc: Ruiyu Ni
Cc: Laszlo Ersek
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong
---
UefiCpuPkg/Include/AcpiCpuData.h | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/UefiCpuPkg/Include/AcpiCpuData.h b/UefiCpuPkg/Include/AcpiCpuData.h
index 9e51145c08..b3cf2f664a 100644
--- a/UefiCpuPkg/Include/AcpiCpuData.h
+++ b/UefiCpuPkg/Include/AcpiCpuData.h
@@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
EXPRESS OR IMPLIED.
#ifndef _ACPI_CPU_DATA_H_
#define _ACPI_CPU_DATA_H_
+#include
+
//
// Register types in register table
//
@@ -22,9 +24,20 @@ typedef enum {
Msr,
ControlRegister,
MemoryMapped,
- CacheControl
+ CacheControl,
+ Semaphore
} REGISTER_TYPE;
+//
+// CPU information.
+//
+typedef struct {
+ UINT32PackageCount; // Packages in this CPU.
+ UINT32CoreCount;// Max Core count in the packages.
+ UINT32ThreadCount; // MAx thread count in the cores.
+ UINT32*ValidCoresInPackages;// Valid cores in each package.
+} CPU_STATUS_INFORMATION;
+
//
// Element of register table entry
//
@@ -147,6 +160,14 @@ typedef struct {
// provided.
//
UINT32ApMachineCheckHandlerSize;
+ //
+ // CPU information which is required when set the register table.
+ //
+ CPU_STATUS_INFORMATION CpuStatus;
+ //
+ // Location info for each ap.
+ //
+ EFI_CPU_PHYSICAL_LOCATION *ApLocation;
} ACPI_CPU_DATA;
#endif
--
2.15.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [Patch 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: Add logic to support semaphore type.
Because this driver needs to set MSRs saved in normal boot phase, sync semaphore
logic from RegisterCpuFeaturesLib code which used for normal boot phase.
Detail see change SHA-1: dcdf1774212d87e2d7feb36286a408ea7475fd7b for
RegisterCpuFeaturesLib.
Cc: Ruiyu Ni
Cc: Laszlo Ersek
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong
---
UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 316 -
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 3 -
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 3 +-
3 files changed, 180 insertions(+), 142 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 52ff9679d5..5a35f7a634 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -38,9 +38,12 @@ typedef struct {
} MP_ASSEMBLY_ADDRESS_MAP;
//
-// Spin lock used to serialize MemoryMapped operation
+// Flags used when program the register.
//
-SPIN_LOCK*mMemoryMappedLock = NULL;
+typedef struct {
+ volatile UINTN MemoryMappedLock; // Spinlock used to program
mmio
+ volatile UINT32 *SemaphoreCount; // Semaphore used to program
semaphore.
+} PROGRAM_CPU_REGISTER_FLAGS;
//
// Signal that SMM BASE relocation is complete.
@@ -62,13 +65,11 @@ AsmGetAddressMap (
#define LEGACY_REGION_SIZE(2 * 0x1000)
#define LEGACY_REGION_BASE(0xA - LEGACY_REGION_SIZE)
+PROGRAM_CPU_REGISTER_FLAGS mCpuFlags;
ACPI_CPU_DATAmAcpiCpuData;
volatile UINT32 mNumberToFinish;
MP_CPU_EXCHANGE_INFO *mExchangeInfo;
BOOLEAN mRestoreSmmConfigurationInS3 = FALSE;
-MP_MSR_LOCK *mMsrSpinLocks = NULL;
-UINTNmMsrSpinLockCount;
-UINTNmMsrCount = 0;
//
// S3 boot flag
@@ -91,89 +92,6 @@ UINT8mApHltLoopCodeTemplate[] = {
0xEB, 0xFC // jmp $-2
};
-/**
- Get MSR spin lock by MSR index.
-
- @param MsrIndex MSR index value.
-
- @return Pointer to MSR spin lock.
-
-**/
-SPIN_LOCK *
-GetMsrSpinLockByIndex (
- IN UINT32 MsrIndex
- )
-{
- UINTN Index;
- for (Index = 0; Index < mMsrCount; Index++) {
-if (MsrIndex == mMsrSpinLocks[Index].MsrIndex) {
- return mMsrSpinLocks[Index].SpinLock;
-}
- }
- return NULL;
-}
-
-/**
- Initialize MSR spin lock by MSR index.
-
- @param MsrIndex MSR index value.
-
-**/
-VOID
-InitMsrSpinLockByIndex (
- IN UINT32 MsrIndex
- )
-{
- UINTNMsrSpinLockCount;
- UINTNNewMsrSpinLockCount;
- UINTNIndex;
- UINTNAddedSize;
-
- if (mMsrSpinLocks == NULL) {
-MsrSpinLockCount = mSmmCpuSemaphores.SemaphoreMsr.AvailableCounter;
-mMsrSpinLocks = (MP_MSR_LOCK *) AllocatePool (sizeof (MP_MSR_LOCK) *
MsrSpinLockCount);
-ASSERT (mMsrSpinLocks != NULL);
-for (Index = 0; Index < MsrSpinLockCount; Index++) {
- mMsrSpinLocks[Index].SpinLock =
- (SPIN_LOCK *)((UINTN)mSmmCpuSemaphores.SemaphoreMsr.Msr + Index *
mSemaphoreSize);
- mMsrSpinLocks[Index].MsrIndex = (UINT32)-1;
-}
-mMsrSpinLockCount = MsrSpinLockCount;
-mSmmCpuSemaphores.SemaphoreMsr.AvailableCounter = 0;
- }
- if (GetMsrSpinLockByIndex (MsrIndex) == NULL) {
-//
-// Initialize spin lock for MSR programming
-//
-mMsrSpinLocks[mMsrCount].MsrIndex = MsrIndex;
-InitializeSpinLock (mMsrSpinLocks[mMsrCount].SpinLock);
-mMsrCount ++;
-if (mMsrCount == mMsrSpinLockCount) {
- //
- // If MSR spin lock buffer is full, enlarge it
- //
- AddedSize = SIZE_4KB;
- mSmmCpuSemaphores.SemaphoreMsr.Msr =
-AllocatePages (EFI_SIZE_TO_PAGES(AddedSize));
- ASSERT (mSmmCpuSemaphores.SemaphoreMsr.Msr != NULL);
- NewMsrSpinLockCount = mMsrSpinLockCount + AddedSize / mSemaphoreSize;
- mMsrSpinLocks = ReallocatePool (
-sizeof (MP_MSR_LOCK) * mMsrSpinLockCount,
-sizeof (MP_MSR_LOCK) * NewMsrSpinLockCount,
-mMsrSpinLocks
-);
- ASSERT (mMsrSpinLocks != NULL);
- mMsrSpinLockCount = NewMsrSpinLockCount;
- for (Index = mMsrCount; Index < mMsrSpinLockCount; Index++) {
-mMsrSpinLocks[Index].SpinLock =
- (SPIN_LOCK *)((UINTN)mSmmCpuSemaphores.SemaphoreMsr.Msr +
- (Index - mMsrCount) * mSemaphoreSize);
-mMsrSpinLocks[Index].MsrIndex = (UINT32)-1;
- }
-}
- }
-}
-
/**
Sync up the MTRR values for all processors.
@@ -204,42 +122,89 @@ Returns:
}
/**
- Programs registers for the calling processor.
+ Increment semaphore by 1.
- This function programs registers for the calling processor.
+ @param SemIN: 32-bit unsigned integer
- @param RegisterTablesPointer to register table of the running
proc
Re: [edk2] [PATCH] MdeModulePkg PcdDxe: ASSERT PcdSetNvStoreDefaultId set
Reviewed-by: Jian J Wang
> -Original Message-
> From: Zeng, Star
> Sent: Friday, October 12, 2018 6:23 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Gao, Liming ;
> Yao, Jiewen ; Wang, Jian J
> Subject: [PATCH] MdeModulePkg PcdDxe: ASSERT PcdSetNvStoreDefaultId set
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1245
>
> In current implementation and according to the description in
> MdeModulePkg.dec, PcdSetNvStoreDefaultId should be set in PEI
> phase to take effect.
>
> This patch ASSERTs PcdSetNvStoreDefaultId set in PcdDxe to alert
> the invalid operation.
>
> Cc: Liming Gao
> Cc: Jiewen Yao
> Cc: Jian J Wang
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng
> ---
> MdeModulePkg/Universal/PCD/Dxe/Pcd.c | 5 +
> MdeModulePkg/Universal/PCD/Dxe/Pcd.inf | 4 +++-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Universal/PCD/Dxe/Pcd.c
> b/MdeModulePkg/Universal/PCD/Dxe/Pcd.c
> index bc308af1c5d8..f977c7f18e19 100644
> --- a/MdeModulePkg/Universal/PCD/Dxe/Pcd.c
> +++ b/MdeModulePkg/Universal/PCD/Dxe/Pcd.c
> @@ -890,6 +890,11 @@ DxePcdSet16Ex (
>IN UINT16Value
>)
> {
> + //
> + // PcdSetNvStoreDefaultId should be set in PEI phase to take effect.
> + //
> + ASSERT (!(CompareGuid (Guid, &gEfiMdeModulePkgTokenSpaceGuid) &&
> +(ExTokenNumber == PcdToken(PcdSetNvStoreDefaultId;
>return ExSetValueWorker (ExTokenNumber, Guid, &Value, sizeof (Value));
> }
>
> diff --git a/MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
> b/MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
> index 066b86aaa5f3..1f41a316bd89 100644
> --- a/MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
> +++ b/MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
> @@ -331,6 +331,7 @@ [LibraryClasses]
> [Guids]
>gPcdDataBaseHobGuid ## SOMETIMES_CONSUMES ## HOB
>gPcdDataBaseSignatureGuid ## CONSUMES ## GUID # PCD
> database signature GUID.
> + gEfiMdeModulePkgTokenSpaceGuid## SOMETIMES_CONSUMES ##
> GUID
>
> [Protocols]
>gPcdProtocolGuid ## PRODUCES
> @@ -342,7 +343,8 @@ [Protocols]
>gEdkiiVariableLockProtocolGuid
>
> [Pcd]
> - gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress ##
> SOMETIMES_CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress ##
> SOMETIMES_CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSetNvStoreDefaultId ##
> SOMETIMES_CONSUMES
>
> [Depex]
>TRUE
> --
> 2.7.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch] MdeModulePkg BrotliCustomDecompressLib: Don't build it for EBC arch
Reviewed-by: Star Zeng
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Liming
Gao
Sent: Thursday, October 11, 2018 1:53 PM
To: edk2-devel@lists.01.org
Cc: Bi, Dandan ; Zeng, Star
Subject: [edk2] [Patch] MdeModulePkg BrotliCustomDecompressLib: Don't build it
for EBC arch
BrotliCustomDecompressLib has the definition with float type. But,
Floating-point types are not supported by EBC compiler.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao
Cc: Star Zeng
Cc: Bi Dandan
---
MdeModulePkg/MdeModulePkg.dsc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
index 3ff3b12..2465d39 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -286,7 +286,6 @@
MdeModulePkg/Library/CpuExceptionHandlerLibNull/CpuExceptionHandlerLibNull.inf
MdeModulePkg/Library/PlatformHookLibSerialPortPpi/PlatformHookLibSerialPortPpi.inf
MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
- MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf
MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
MdeModulePkg/Library/PlatformBootManagerLibNull/PlatformBootManagerLibNull.inf
@@ -430,6 +429,7 @@
MdeModulePkg/Universal/EbcDxe/EbcDebuggerConfig.inf
[Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
+
+ MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressL
+ ib.inf
MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
MdeModulePkg/Core/Dxe/DxeMain.inf {
--
2.10.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1 1/6] PcAtChipsetPkg: Add MMIO Support to SerialIo Lib
On 10/12/2018 11:33 PM, Ard Biesheuvel wrote: On 12 October 2018 at 17:06, Sami Mujawar wrote: Hi Ard, I believe you are referring to MdeModulePkg\Library\BaseSerialPortLib16550\BaseSerialPortLib16550.inf I have tried using this Serial Port Library. However, this has a dependency on PciLib which is difficult to resolve as we use the Serial port early in the boot. Please do let me know if you know a workaround to solve this issue. Yes. Just keep PcdSerialPciDeviceInfo at its default of 0xff, and the PCI dependencies in that driver are essentially circumvented, so you can use any PciLib implementation as a dummy (or create a new one that ASSERT()s in all functions if you prefer) Yes I was just about to say so. Actually I plan to retire/remove the SerialIoLib in PcAtChipsetPkg. -- Thanks, Ray ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg PcdDxe: ASSERT PcdSetNvStoreDefaultId set
Reviewed-by: Liming Gao
>-Original Message-
>From: Zeng, Star
>Sent: Friday, October 12, 2018 6:23 PM
>To: edk2-devel@lists.01.org
>Cc: Zeng, Star ; Gao, Liming ;
>Yao, Jiewen ; Wang, Jian J
>Subject: [PATCH] MdeModulePkg PcdDxe: ASSERT PcdSetNvStoreDefaultId
>set
>
>REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1245
>
>In current implementation and according to the description in
>MdeModulePkg.dec, PcdSetNvStoreDefaultId should be set in PEI
>phase to take effect.
>
>This patch ASSERTs PcdSetNvStoreDefaultId set in PcdDxe to alert
>the invalid operation.
>
>Cc: Liming Gao
>Cc: Jiewen Yao
>Cc: Jian J Wang
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Star Zeng
>---
> MdeModulePkg/Universal/PCD/Dxe/Pcd.c | 5 +
> MdeModulePkg/Universal/PCD/Dxe/Pcd.inf | 4 +++-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
>diff --git a/MdeModulePkg/Universal/PCD/Dxe/Pcd.c
>b/MdeModulePkg/Universal/PCD/Dxe/Pcd.c
>index bc308af1c5d8..f977c7f18e19 100644
>--- a/MdeModulePkg/Universal/PCD/Dxe/Pcd.c
>+++ b/MdeModulePkg/Universal/PCD/Dxe/Pcd.c
>@@ -890,6 +890,11 @@ DxePcdSet16Ex (
> IN UINT16Value
> )
> {
>+ //
>+ // PcdSetNvStoreDefaultId should be set in PEI phase to take effect.
>+ //
>+ ASSERT (!(CompareGuid (Guid, &gEfiMdeModulePkgTokenSpaceGuid) &&
>+(ExTokenNumber == PcdToken(PcdSetNvStoreDefaultId;
> return ExSetValueWorker (ExTokenNumber, Guid, &Value, sizeof (Value));
> }
>
>diff --git a/MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
>b/MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
>index 066b86aaa5f3..1f41a316bd89 100644
>--- a/MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
>+++ b/MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
>@@ -331,6 +331,7 @@ [LibraryClasses]
> [Guids]
> gPcdDataBaseHobGuid ## SOMETIMES_CONSUMES ## HOB
> gPcdDataBaseSignatureGuid ## CONSUMES ## GUID # PCD
>database signature GUID.
>+ gEfiMdeModulePkgTokenSpaceGuid## SOMETIMES_CONSUMES
>## GUID
>
> [Protocols]
> gPcdProtocolGuid ## PRODUCES
>@@ -342,7 +343,8 @@ [Protocols]
> gEdkiiVariableLockProtocolGuid
>
> [Pcd]
>- gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress ##
>SOMETIMES_CONSUMES
>+ gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress ##
>SOMETIMES_CONSUMES
>+ gEfiMdeModulePkgTokenSpaceGuid.PcdSetNvStoreDefaultId ##
>SOMETIMES_CONSUMES
>
> [Depex]
> TRUE
>--
>2.7.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] Revert BaseTools Python3 Migration has been done at 1ccc4d895dd8d659d016efcd6ef8a48749aba1d0
Hi, all With the community discussion, Python3 migration is the fundamental change. It requires every developer to install Python3. Before this migration, the well communication and wide verification must be done. But now, most people is not aware of this change, and not try it. So, Python3 migration is reverted and be moved to edk2-staging Python3 branch (https://github.com/tianocore/edk2-staging/tree/Python3). BaseTools has been reverted at commit 1ccc4d895dd8d659d016efcd6ef8a48749aba1d0. So, please update edk2 to the latest trunk, and continue to your work. There is no change now. Python3 migration will not in the near edk2-stable201811 release. I will add it for next edk2 stable release plan. If you have any comments or confuse, please let me know. Thanks Liming ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] TianoCore Community Meeting Minutes
On 10/12/2018 6:27 AM, Leif Lindholm wrote:> I know at least one organisation that would be prevented from using it until they had explicit approval (which could take a while), and it puts me in the position of having to keep a spare x86 machine on my desktop in order to participate. Solutions I know would work in this scenario (i.e. work without plugins or downloads in any modern browser) are: - google hangouts (requires accounts, not ideal for China) - Skype (not SFB though, only plain old Skype) - Bluejeans (requires organiser account) We needed an organizer account for Zoom since our meeting was 1 hour long. So switching to something paid is not an issue. I've used Bluejeans with success in the past. I'll look into switching to this platform if no one else has any objections. I'd like to stay away from my other preferred google hangouts because so many of our members are in PRC. The was a concern raised over potential lock-in to Github's, specifically in regards to history retention. Several Github users brought up that this shouldn't be an issue. Hopefully they said more than that. What does "shouldn't be an issue" mean. Were these users from multiple organisations? I will ask for more details in the next meeting, but the comments were that the API is quite robust and lends itself to readily accessible data. Shawn mentioned some benefits to stock Github such as it is always up to date, it includes APIs to extract data, pull requests Since we are discussing multiple different development systems, can we try to be a bit more explicit? This is referring to github's web-based branch-based ticketing system, yes? Yes that's correct. I know language drift and all, but https://www.kernel.org/doc/html/latest/maintainer/pull-requests.html is what pull request means to at least 3 participants in this conversation. I think this is an important distinction to make and I'm sure it will come up in future discussions. I'll be sure to bring this up. lend themselves to simple(r) automation, and it has a much more modern web UI than Gerrit. It seems somewhat less than ideal to me that all of the github proponents were on the opposite call to me and Laszlo (and Ard). Were any of them Asia-based or could we try to get them on the call with Europe next time? I'm sure me and Laszlo could be somewhat more accommodating than your 7AM, but we're not going to stay up for a 3/4AM meeting about source control. That's understandable. I'm not sure where Shawn and Nate are located, but I believe it is in the US. We could certainly move the AM call a bit later if it makes it easier to get more folks from the US on that call. -How can we better track the code quality of BaseTools and the current build system on the whole. Should we add a "requires documentation change" flag to BZ so that it will be easier to compile a list of required doc changes. Could we also have a thread on someting that you sort of mention in the text, but not the bullet points?: - Suggestions for improvements to BaseTools. Of course. I'll add this to the list of "community conversations" that I will start up on the list this week. Cheers, Stephano ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH] uefi-sct/SctPkg:Enhance the EraseBlock Test
The EraseSize in the EraseBlocks conf test should be bytes.
Cover the case that the size of the data to erase is a
multiple of the 'EraseLengthGranularity' value of an Erase Block
Protocol instance. And check whether the data on adjacent blocks
are mistakenly erased.
Cc: Supreeth Venkatesh
Cc: Jiaxin Wu
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Jin
---
.../EraseBlockBBTestConformance.c | 25 +-
.../BlackBoxTest/EraseBlockBBTestFunction.c | 600 --
.../BlackBoxTest/EraseBlockBBTestMain.h | 16 +-
.../Protocol/EraseBlock/BlackBoxTest/Guid.c | 4 +-
.../Protocol/EraseBlock/BlackBoxTest/Guid.h | 7 +
5 files changed, 589 insertions(+), 63 deletions(-)
diff --git
a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/EraseBlock/BlackBoxTest/EraseBlockBBTestConformance.c
b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/EraseBlock/BlackBoxTest/EraseBlockBBTestConformance.c
index df057b26..7e848239 100644
---
a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/EraseBlock/BlackBoxTest/EraseBlockBBTestConformance.c
+++
b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/EraseBlock/BlackBoxTest/EraseBlockBBTestConformance.c
@@ -1,7 +1,7 @@
/** @file
Copyright 2017 Unified EFI, Inc.
- Copyright (c) 2017, Intel Corporation. All rights reserved.
+ Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD
License
@@ -51,8 +51,8 @@ BBTestEraseBlocksConformanceTest (
UINT32BlockSize;
UINT32IoAlign;
EFI_LBA LastBlock;
-
- UINT32BlockNumber;
+ UINT32EraseLengthGranularity;
+ UINTN EraseSize;
EFI_ERASE_BLOCK_TOKEN Token;
@@ -121,10 +121,11 @@ BBTestEraseBlocksConformanceTest (
IoAlign = Media->IoAlign;
LastBlock = Media->LastBlock;
- BlockNumber = (UINT32) MINIMUM(LastBlock,
MAX_NUMBER_OF_READ_BLOCK_BUFFER);
+ EraseLengthGranularity = EraseBlock->EraseLengthGranularity;
+ EraseSize = (UINTN)SctMultU64x32 (EraseLengthGranularity,
BlockSize);
if (MediaPresent == FALSE) {
-Status = EraseBlock->EraseBlocks(EraseBlock, MediaId, 0, &Token,
BlockNumber);
+Status = EraseBlock->EraseBlocks(EraseBlock, MediaId, 0, &Token,
EraseSize);
if (Status == EFI_NO_MEDIA)
AssertionType = EFI_TEST_ASSERTION_PASSED;
else
@@ -141,7 +142,7 @@ BBTestEraseBlocksConformanceTest (
Status
);
-Status = EraseBlock->EraseBlocks(EraseBlock, MediaId, LastBlock + 1,
&Token, BlockNumber);
+Status = EraseBlock->EraseBlocks(EraseBlock, MediaId, LastBlock + 1,
&Token, EraseSize);
if (Status == EFI_NO_MEDIA)
AssertionType = EFI_TEST_ASSERTION_PASSED;
else
@@ -158,7 +159,7 @@ BBTestEraseBlocksConformanceTest (
Status
);
-Status = EraseBlock->EraseBlocks(EraseBlock, MediaId, LastBlock - 10,
&Token, BlockNumber + 1);
+Status = EraseBlock->EraseBlocks(EraseBlock, MediaId, LastBlock - 10,
&Token, EraseSize + 1);
if (Status == EFI_NO_MEDIA)
AssertionType = EFI_TEST_ASSERTION_PASSED;
else
@@ -177,7 +178,7 @@ BBTestEraseBlocksConformanceTest (
} else {
if (ReadOnly == TRUE) {
- Status = EraseBlock->EraseBlocks(EraseBlock, MediaId, 0, &Token,
BlockNumber);
+ Status = EraseBlock->EraseBlocks(EraseBlock, MediaId, 0, &Token,
EraseSize);
if (Status == EFI_WRITE_PROTECTED)
AssertionType = EFI_TEST_ASSERTION_PASSED;
else
@@ -195,7 +196,7 @@ BBTestEraseBlocksConformanceTest (
);
} else {
- Status = EraseBlock->EraseBlocks(EraseBlock, MediaId + 1, 0, &Token,
BlockNumber);
+ Status = EraseBlock->EraseBlocks(EraseBlock, MediaId + 1, 0, &Token,
EraseSize);
if (Status == EFI_MEDIA_CHANGED)
AssertionType = EFI_TEST_ASSERTION_PASSED;
else
@@ -212,7 +213,7 @@ BBTestEraseBlocksConformanceTest (
Status
);
- Status = EraseBlock->EraseBlocks(EraseBlock, MediaId + 1, LastBlock + 1,
&Token, BlockNumber);
+ Status = EraseBlock->EraseBlocks(EraseBlock, MediaId + 1, LastBlock + 1,
&Token, EraseSize);
if (Status == EFI_MEDIA_CHANGED)
AssertionType = EFI_TEST_ASSERTION_PASSED;
else
@@ -229,7 +230,7 @@ BBTestEraseBlocksConformanceTest (
Status
);
- Status = EraseBlock->EraseBlocks(EraseBlock, MediaId + 1, LastBlock -
10, &Token, BlockNumber + 1);
+ Status = EraseBlock->EraseBlocks(EraseBlock, MediaId + 1, LastBlock -
10, &Token, EraseSize + 1);
if (Status == EFI_MEDIA_CHANGED)
Asserti
Re: [edk2] [Patch] BaseTools/Tests: Update GNUmakefile to use python3 variable
On Sun, Oct 14, 2018 at 02:51:06PM +, Gao, Liming wrote:
> Leif:
> OK. I will move this patch set to edk2 staging repo for people
> verification.
Thats sounds like a good idea.
> I will use git revert command to revert total 27 patches one by
> one. Then, there will be 27 commit for the revert patches. Or, one
> commit to combine all revert patches. One commit is enough. Do you
> agree it?
On commit is enough.
> Below is the commit message for the revert patch. Could you help
> review it?
> Python3 migration is the fundamental change. It requires every
> developer to install Python3. Before this migration, the well
> communication and wide verification must be done. But now, most
> people is not aware of this change, and not try it. So, Python3
> migration is reverted and be moved to edk2-staging Python3 branch
> for the edk2 user evaluation.
This looks good to me, thanks!
Regards,
Leif
> Thanks
> Liming
> > -Original Message-
> > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > Sent: Sunday, October 14, 2018 4:40 AM
> > To: Zhu, Yonghong
> > Cc: Ard Biesheuvel ; edk2-devel@lists.01.org;
> > Gao, Liming
> > Subject: Re: [Patch] BaseTools/Tests: Update GNUmakefile to use python3
> > variable
> >
> > Hi Yonghong,
> >
> > Please revert this patch. The broken state of the master branch must
> > be undone, not hacked on until individual bits seem to work.
> >
> > Plese do no further work on the master branch until the contents of
> > the tree is identical to what it was at
> > 301402fa4797ac3a141e575329ca2ea91756414c. (I am not talking about a
> > git push -f, I am talking about a git revert.)
> >
> > Best Regards,
> >
> > Leif
> >
> > On Sat, Oct 13, 2018 at 04:53:21PM +, Zhu, Yonghong wrote:
> > > Hi Ard,
> > >
> > > Thanks. I pushed this patch since it is critical block issue.
> > > Version SHA-1: 678f85131238622e576705117e299d81cff755c9
> > >
> > > Best Regards,
> > > Zhu Yonghong
> > >
> > > -Original Message-
> > > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> > > Sent: Sunday, October 14, 2018 12:44 AM
> > > To: Zhu, Yonghong
> > > Cc: edk2-devel@lists.01.org; Leif Lindholm
> > > Subject: Re: [Patch] BaseTools/Tests: Update GNUmakefile to use python3
> > > variable
> > >
> > > On 13 October 2018 at 18:40, Yonghong Zhu wrote:
> > > > Cover the case use do make -C BaseTools before run the .edksetup.sh
> > > > file.
> > > >
> > > > Cc: Ard Biesheuvel
> > > > Cc: Leif Lindholm
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Yonghong Zhu
> > >
> > > Reviewed-by: Ard Biesheuvel
> > > Tested-by: Ard Biesheuvel
> > >
> > > > ---
> > > > BaseTools/Tests/GNUmakefile | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/BaseTools/Tests/GNUmakefile b/BaseTools/Tests/GNUmakefile
> > > > index af334a8a..536f0b7 100644
> > > > --- a/BaseTools/Tests/GNUmakefile
> > > > +++ b/BaseTools/Tests/GNUmakefile
> > > > @@ -12,10 +12,10 @@
> > > > #
> > > >
> > > > all: test
> > > >
> > > > test:
> > > > - @if command -v $(PYTHON3) >/dev/null 2>&1; then $(PYTHON3)
> > > > RunTests.py; else python RunTests.py; fi
> > > > + @if command -v python3 >/dev/null 2>&1; then python3
> > > > + RunTests.py; else echo "Error: Please install a python 3 tool!"; fi
> > > >
> > > > clean:
> > > > find . -name '*.pyc' -exec rm '{}' ';'
> > > >
> > > > --
> > > > 2.6.1.windows.1
> > > >
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch] BaseTools/Tests: Update GNUmakefile to use python3 variable
Leif:
OK. I will move this patch set to edk2 staging repo for people verification.
I will use git revert command to revert total 27 patches one by one. Then,
there will be 27 commit for the revert patches. Or, one commit to combine all
revert patches. One commit is enough. Do you agree it?
Below is the commit message for the revert patch. Could you help review it?
Python3 migration is the fundamental change. It requires every developer to
install Python3. Before this migration, the well communication and wide
verification must be done. But now, most people is not aware of this change,
and not try it. So, Python3 migration is reverted and be moved to edk2-staging
Python3 branch for the edk2 user evaluation.
Thanks
Liming
> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Sunday, October 14, 2018 4:40 AM
> To: Zhu, Yonghong
> Cc: Ard Biesheuvel ; edk2-devel@lists.01.org; Gao,
> Liming
> Subject: Re: [Patch] BaseTools/Tests: Update GNUmakefile to use python3
> variable
>
> Hi Yonghong,
>
> Please revert this patch. The broken state of the master branch must
> be undone, not hacked on until individual bits seem to work.
>
> Plese do no further work on the master branch until the contents of
> the tree is identical to what it was at
> 301402fa4797ac3a141e575329ca2ea91756414c. (I am not talking about a
> git push -f, I am talking about a git revert.)
>
> Best Regards,
>
> Leif
>
> On Sat, Oct 13, 2018 at 04:53:21PM +, Zhu, Yonghong wrote:
> > Hi Ard,
> >
> > Thanks. I pushed this patch since it is critical block issue.
> > Version SHA-1: 678f85131238622e576705117e299d81cff755c9
> >
> > Best Regards,
> > Zhu Yonghong
> >
> > -Original Message-
> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> > Sent: Sunday, October 14, 2018 12:44 AM
> > To: Zhu, Yonghong
> > Cc: edk2-devel@lists.01.org; Leif Lindholm
> > Subject: Re: [Patch] BaseTools/Tests: Update GNUmakefile to use python3
> > variable
> >
> > On 13 October 2018 at 18:40, Yonghong Zhu wrote:
> > > Cover the case use do make -C BaseTools before run the .edksetup.sh
> > > file.
> > >
> > > Cc: Ard Biesheuvel
> > > Cc: Leif Lindholm
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Yonghong Zhu
> >
> > Reviewed-by: Ard Biesheuvel
> > Tested-by: Ard Biesheuvel
> >
> > > ---
> > > BaseTools/Tests/GNUmakefile | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/BaseTools/Tests/GNUmakefile b/BaseTools/Tests/GNUmakefile
> > > index af334a8a..536f0b7 100644
> > > --- a/BaseTools/Tests/GNUmakefile
> > > +++ b/BaseTools/Tests/GNUmakefile
> > > @@ -12,10 +12,10 @@
> > > #
> > >
> > > all: test
> > >
> > > test:
> > > - @if command -v $(PYTHON3) >/dev/null 2>&1; then $(PYTHON3)
> > > RunTests.py; else python RunTests.py; fi
> > > + @if command -v python3 >/dev/null 2>&1; then python3
> > > + RunTests.py; else echo "Error: Please install a python 3 tool!"; fi
> > >
> > > clean:
> > > find . -name '*.pyc' -exec rm '{}' ';'
> > >
> > > --
> > > 2.6.1.windows.1
> > >
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch] BaseTools/Tests: Update GNUmakefile to use python3 variable
On 10/13/18 22:40, Leif Lindholm wrote: > Hi Yonghong, > > Please revert this patch. The broken state of the master branch must > be undone, not hacked on until individual bits seem to work. > > Plese do no further work on the master branch until the contents of > the tree is identical to what it was at > 301402fa4797ac3a141e575329ca2ea91756414c. (I am not talking about a > git push -f, I am talking about a git revert.) +1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
