Date   

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

Sami Mujawar
 

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

-----Original Message-----
From: Krzysztof Koch <krzysztof.koch@arm.com>
Sent: 15 August 2019 02:11 PM
To: devel@edk2.groups.io
Cc: jaben.carsey@intel.com; ray.ni@intel.com; zhichao.gao@intel.com; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 00/11] Test against invalid pointers in acpiview

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

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

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

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

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

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

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

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


[edk2-platforms: PATCH v2] Python run fail if env variable PYTHON_HOME is not set

Cheng, Ching JenX
 

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

[PATCH v2] Update related files

In Platform\Intel\MinPlatformPkg\Tools\Fsp\RebaseFspBinBaseAddress.py
It will run another python code.
But if the environment variable "PYTHON_HOME" is not exist
and we didn't add any python's path to "PATH".
It will cause error because python command not found.

the error message as below:
'python' is not recognized as an internal or external command,
operable program or batch file.

So we set the python's path from which execute the python code
if PYTHON_HOME was not exist.

Cc: Amy Chan <amy.chan@intel.com>
Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Ching JenX Cheng <ching.jenx.cheng@intel.com>
---
Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseFspBinBaseAddress.py | 2 ++
1 file changed, 2 insertions(+)

diff --git a/Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseFspBinBaseAddress.py b/Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseFspBinBaseAddress.py
index a8165b08e6..fb4cf4f9b7 100644
--- a/Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseFspBinBaseAddress.py
+++ b/Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseFspBinBaseAddress.py
@@ -68,6 +68,8 @@ file.close()
pythontool = 'python'
if 'PYTHON_HOME' in os.environ:
pythontool = os.environ['PYTHON_HOME'] + os.sep + 'python'
+else:
+ pythontool = sys.executable
Process = subprocess.Popen([pythontool, splitFspBinPath, "info","-f",fspBinFilePath], stdout=subprocess.PIPE)
Output = Process.communicate()[0]
FsptInfo = Output.rsplit(b"FSP_M", 1);
--
2.21.0.windows.1


Re: [PATCH] Python run fail if env variable PYTHON_HOME is not set

Chiu, Chasel
 

Please correct subject as you are modifying edk2-platform files: --subject-prefix="edk2-platforms: PATCH"
Also the RebaseAndPatchFspBinBaseAddress.py should be obsolete soon, so please focus on RebaseFspBinBaseAddress.py.

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Cheng, Ching JenX
Sent: Monday, August 19, 2019 3:21 PM
To: devel@edk2.groups.io
Cc: Chan, Amy <amy.chan@intel.com>; Kubacki, Michael A
<michael.a.kubacki@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>;
Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming
<liming.gao@intel.com>
Subject: [edk2-devel] [PATCH] Python run fail if env variable PYTHON_HOME is
not set

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

In Platform\Intel\MinPlatformPkg\Tools\Fsp\RebaseFspBinBaseAddress.py
It will run another python code.
But if the environment variable "PYTHON_HOME" is not exist and we didn't
add any python's path to "PATH".
It will cause error because python command not found.

the error message as below:
'python' is not recognized as an internal or external command, operable
program or batch file.

So we set the python's path from which execute the python code if
PYTHON_HOME was not exist.

Cc: Amy Chan <amy.chan@intel.com>
Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Ching JenX Cheng <ching.jenx.cheng@intel.com>
---

Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseAndPatchFspBinBaseAddre
ss.py | 2 ++
1 file changed, 2 insertions(+)

diff --git
a/Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseAndPatchFspBinBaseAdd
ress.py
b/Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseAndPatchFspBinBaseAd
dress.py
index 406e5ec130..1d72b4112f 100644
---
a/Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseAndPatchFspBinBaseAdd
ress.py
+++
b/Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseAndPatchFspBinBaseAd
+++ dress.py
@@ -76,6 +76,8 @@ file.close()
pythontool = 'python'
if 'PYTHON_HOME' in os.environ:
pythontool = os.environ['PYTHON_HOME'] + os.sep + 'python'
+else:
+ pythontool = sys.executable
Process = subprocess.Popen([pythontool, splitFspBinPath,
"info","-f",fspBinFilePath], stdout=subprocess.PIPE) Output =
Process.communicate()[0] FsptInfo = Output.rsplit(b"FSP_M", 1);
--
2.21.0.windows.1



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

Alexei Fedorov
 

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@...>


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

Krzysztof Koch
 

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

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

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/tree/612_add_pointer_validation_v2

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

v2:
- Do not require FadtMinorRevision and X_DsdtAddress pointers to be
valid in order to process the remaining ACPI tables [Zhichao]

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
index e40c9ef8ee4b3285faf8c6edf3cb6236ee367397..6859c4824c2866fd3eb9a789a8dfc950724b27ca 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
@@ -204,9 +204,11 @@ ParseAcpiFadt (
);

if (Trace) {
- Print (L"\nSummary:\n");
- PrintFieldName (2, L"FADT Version");
- Print (L"%d.%d\n", *AcpiHdrInfo.Revision, *FadtMinorRevision);
+ if (FadtMinorRevision != NULL) {
+ Print (L"\nSummary:\n");
+ PrintFieldName (2, L"FADT Version");
+ Print (L"%d.%d\n", *AcpiHdrInfo.Revision, *FadtMinorRevision);
+ }

if (*GetAcpiXsdtHeaderInfo ()->OemTableId != *AcpiHdrInfo.OemTableId) {
IncrementErrorCount ();
@@ -214,21 +216,20 @@ ParseAcpiFadt (
}
}

- // If X_DSDT is not zero then use X_DSDT and ignore DSDT,
- // else use DSDT.
- if (*X_DsdtAddress != 0) {
+ // If X_DSDT is valid then use X_DSDT and ignore DSDT, else use DSDT.
+ if ((X_DsdtAddress != NULL) && (*X_DsdtAddress != 0)) {
DsdtPtr = (UINT8*)(UINTN)(*X_DsdtAddress);
- } else if (*DsdtAddress != 0) {
+ } else if ((DsdtAddress != NULL) && (*DsdtAddress != 0)) {
DsdtPtr = (UINT8*)(UINTN)(*DsdtAddress);
} else {
- // Both DSDT and X_DSDT cannot be zero.
+ // Both DSDT and X_DSDT cannot be invalid.
#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
if (Trace) {
// The DSDT Table is mandatory for ARM systems
// as the CPU information MUST be presented in
// the DSDT.
IncrementErrorCount ();
- Print (L"ERROR: Both X_DSDT and DSDT are NULL.\n");
+ Print (L"ERROR: Both X_DSDT and DSDT are invalid.\n");
}
#endif
return;
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


Re: [PATCH v4 2/2] MdeModulePkg/ScsiDiskDxe: Support Storage Security Command Protocol

Wu, Hao A
 

-----Original Message-----
From: Zurcher, Christopher J
Sent: Saturday, August 17, 2019 4:22 AM
To: Wu, Hao A; devel@edk2.groups.io
Cc: Kinney, Michael D; Yao, Jiewen; Wang, Jian J; Gao, Liming
Subject: RE: [edk2-devel] [PATCH v4 2/2] MdeModulePkg/ScsiDiskDxe:
Support Storage Security Command Protocol

Hao,
Why will this cause CDROM devices to fail to be recognized? If they require a

Hello,

My observation is that after the patch, the below assignment:
"ScsiDiskDevice->BlkIo.Media->MediaPresent = TRUE;"
is moved from GetMediaInfo() to ScsiDiskTestUnitReady().

The flow for CD-ROM devices is that:
ScsiDiskDriverBindingStart()
|
└-> ScsiDiskDetectMedia()
|
└-> ScsiDiskTestUnitReady()
|
└-> DetectMediaParsingSenseKeys() - Outputs 'Action'
|
└-> If Action is READ_CAPACITY, ScsiDiskReadCapacity()

Now, 'MediaPresent' is set to TRUE in ScsiDiskTestUnitReady(), which leads to
DetectMediaParsingSenseKeys() returning NO_ACTION:

*Action = ACTION_NO_ACTION;

...

if (!ScsiDiskHaveSenseKey (SenseData, NumberOfSenseKeys)) {
//
// No Sense Key returned from last submitted command
//
if (ScsiDiskDevice->BlkIo.Media->MediaPresent == TRUE) {
}
return EFI_SUCCESS;
}


capacity read, I can change the MustReadCapacity flag to TRUE. On
subsequent media change, the sense key parsing will set Action to
ACTION_READ_CAPACITY so that case is covered as well. Is there a reason
the MustReadCapacity flag was originally set to FALSE for CDROM devices?

I think setting 'MustReadCapacity' to FALSE for CD-ROM devices is for the
performance consideration.

Before the patch, logic in DetectMediaParsingSenseKeys():

if (ScsiDiskIsNoMedia (SenseData, NumberOfSenseKeys)) {
ScsiDiskDevice->BlkIo.Media->MediaPresent = FALSE;
ScsiDiskDevice->BlkIo.Media->LastBlock = 0;
*Action = ACTION_NO_ACTION;
DEBUG ((EFI_D_VERBOSE, "ScsiDisk: ScsiDiskIsNoMedia\n"));
return EFI_SUCCESS;
}

will decide whether there is media within the CD-ROM devices after parsing the
sense data returned from the device. If the media does not exist, the
Read Capacity command will not be sent.

Setting 'MustReadCapacity' to TRUE will then always require a Read Capacity
command be sent.

Best Regards,
Hao Wu


With the default action being to read capacity anyway, the existing
implementation seems to make the flag mostly useless.

I have implemented the rest of the suggestions.

Thanks,
Christopher Zurcher

-----Original Message-----
From: Wu, Hao A
Sent: Monday, June 17, 2019 19:12
To: devel@edk2.groups.io; Zurcher, Christopher J
<christopher.j.zurcher@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen
<jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming
<liming.gao@intel.com>
Subject: RE: [edk2-devel] [PATCH v4 2/2] MdeModulePkg/ScsiDiskDxe:
Support Storage Security Command Protocol

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Zurcher, Christopher J
Sent: Thursday, June 13, 2019 10:05 AM
To: devel@edk2.groups.io
Cc: Kinney, Michael D; Yao, Jiewen; Wang, Jian J; Gao, Liming
Subject: [edk2-devel] [PATCH v4 2/2] MdeModulePkg/ScsiDiskDxe:
Support
Storage Security Command Protocol

This patch implements the
EFI_STORAGE_SECURITY_COMMAND_PROTOCOL
in the
ScsiDiskDxe driver.

Support is currently limited to the RPMB Well-known LUN for UFS devices.

Hello,

Some general level comments:
1. CDROM device issue
This patch will bring an issue to the CDROM devices that CD/DVD will not
be recognized properly.

I think the cause of the issue is the relocation of the below logic:
ScsiDiskDevice->BlkIo.Media->MediaPresent = TRUE;
from GetMediaInfo() to ScsiDiskTestUnitReady(). It will lead to the read
capacity command being skipped for CDROM devices, which results into the
LastBlock for the device equals to 0.

We may need to find out a better approach to handle the case for UFS RPMB.

2. Split this patch into three commits
Besides adding the Storage Security Command Protocol support in ScsiDisk
driver, the patch also:
* Updates the ScsiBus driver to recognize the Well known logical unit
* Updates the UfsPassThruDxe driver to expose the RPMB WLUN

Please help to split the patch into 3 separate commits.

3. Updates for the BlockIO(2) services
For functions ScsiDiskReadBlocks(Ex) & ScsiDiskWriteBlocks(Ex), below codes
are added for the dummy BlockIO(2) protocol instance on the UFS RPMB Lun:

if (BlockSize == 0) {
Status = EFI_UNSUPPORTED;
goto Done;
}

I suggest to use status 'EFI_DEVICE_ERROR' for such case, since
'EFI_UNSUPPORTED' is not listed as an expected return status in the UEFI
spec.

4. Reinstallation of the EFI_STORAGE_SECURITY_COMMAND_PROTOCOL
For the reinstallation of the SSC protocol to handle media change, I saw
this is only handled within ScsiDiskReceiveData() & ScsiDiskSendData().
The below functions should be considered as well:

ScsiDiskReadBlocks(Ex)
ScsiDiskWriteBlocks(Ex)
ScsiDiskFlushBlocksEx
ScsiDiskEraseBlocks


Some inline comments below:



Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
---
MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf | 3 +-
MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.h | 171 ++++++-
MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c | 5 +-
MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c | 522
+++++++++++++++++++-
MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c | 19 +-
5 files changed, 698 insertions(+), 22 deletions(-)

diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
index 5500d828e9..40818e669b 100644
--- a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
+++ b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
@@ -3,7 +3,7 @@
# It detects the SCSI disk media and installs Block I/O and Block I/O2
Protocol
on
# the device handle.
#
-# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -52,6 +52,7 @@
gEfiBlockIoProtocolGuid ## BY_START
gEfiBlockIo2ProtocolGuid ## BY_START
gEfiEraseBlockProtocolGuid ## BY_START
+ gEfiStorageSecurityCommandProtocolGuid ## BY_START
gEfiScsiIoProtocolGuid ## TO_START
gEfiScsiPassThruProtocolGuid ## TO_START
gEfiExtScsiPassThruProtocolGuid ## TO_START
diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.h
b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.h
index 42c0aaaa95..2d8679ec6f 100644
--- a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.h
+++ b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.h
@@ -1,7 +1,7 @@
/** @file
Header file for SCSI Disk Driver.

-Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2004 - 2019, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -22,6 +22,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Protocol/ScsiPassThruExt.h>
#include <Protocol/ScsiPassThru.h>
#include <Protocol/DiskInfo.h>
+#include <Protocol/StorageSecurityCommand.h>


#include <Library/DebugLib.h>
@@ -38,6 +39,10 @@ SPDX-License-Identifier: BSD-2-Clause-Patent

#define IS_DEVICE_FIXED(a) (a)->FixedDevice ? 1 : 0

+#define IS_ALIGNED(addr, size) (((UINTN) (addr) & (size - 1)) == 0)
+
+#define UFS_WLUN_RPMB 0xC4
+
typedef struct {
UINT32 MaxLbaCnt;
UINT32 MaxBlkDespCnt;
@@ -51,6 +56,8 @@ typedef struct {

EFI_HANDLE Handle;

+ EFI_STORAGE_SECURITY_COMMAND_PROTOCOL StorageSecurity;
+
EFI_BLOCK_IO_PROTOCOL BlkIo;
EFI_BLOCK_IO2_PROTOCOL BlkIo2;
EFI_BLOCK_IO_MEDIA BlkIoMedia;
@@ -95,6 +102,7 @@ typedef struct {
#define SCSI_DISK_DEV_FROM_BLKIO(a) CR (a, SCSI_DISK_DEV, BlkIo,
SCSI_DISK_DEV_SIGNATURE)
#define SCSI_DISK_DEV_FROM_BLKIO2(a) CR (a, SCSI_DISK_DEV, BlkIo2,
SCSI_DISK_DEV_SIGNATURE)
#define SCSI_DISK_DEV_FROM_ERASEBLK(a) CR (a, SCSI_DISK_DEV,
EraseBlock, SCSI_DISK_DEV_SIGNATURE)
+#define SCSI_DISK_DEV_FROM_STORSEC(a) CR (a, SCSI_DISK_DEV,
StorageSecurity, SCSI_DISK_DEV_SIGNATURE)

#define SCSI_DISK_DEV_FROM_DISKINFO(a) CR (a, SCSI_DISK_DEV,
DiskInfo, SCSI_DISK_DEV_SIGNATURE)

@@ -638,6 +646,151 @@ ScsiDiskEraseBlocks (
);


+/**
+ Send a security protocol command to a device that receives data and/or
the result
+ of one or more commands sent by SendData.
+
+ The ReceiveData function sends a security protocol command to the
given
MediaId.
+ The security protocol command sent is defined by SecurityProtocolId and
contains
+ the security protocol specific data SecurityProtocolSpecificData. The
function
+ returns the data from the security protocol command in PayloadBuffer.
+
+ For devices supporting the SCSI command set, the security protocol
command is sent
+ using the SECURITY PROTOCOL IN command defined in SPC-4.
+
+ If PayloadBufferSize is too small to store the available data from the
security
+ protocol command, the function shall copy PayloadBufferSize bytes into
the
+ PayloadBuffer and return EFI_WARN_BUFFER_TOO_SMALL.
+
+ If PayloadBuffer or PayloadTransferSize is NULL and PayloadBufferSize is
non-zero,
+ the function shall return EFI_INVALID_PARAMETER.
+
+ If the given MediaId does not support security protocol commands, the
function shall
+ return EFI_UNSUPPORTED. If there is no media in the device, the
function
returns
+ EFI_NO_MEDIA. If the MediaId is not the ID for the current media in the
device,
+ the function returns EFI_MEDIA_CHANGED.
+
+ If the security protocol fails to complete within the Timeout period, the
function
+ shall return EFI_TIMEOUT.
+
+ If the security protocol command completes without an error, the
function
shall
+ return EFI_SUCCESS. If the security protocol command completes with
an
error, the
+ function shall return EFI_DEVICE_ERROR.
+
+ @param This Indicates a pointer to the calling context.
+ @param MediaId ID of the medium to receive data from.
+ @param Timeout The timeout, in 100ns units, to use for the
execution
+ of the security protocol command. A Timeout value of
0
+ means that this function will wait indefinitely for the
+ security protocol command to execute. If Timeout is
greater
+ than zero, then this function will return EFI_TIMEOUT
if
the
+ time required to execute the receive data command is
greater than Timeout.
+ @param SecurityProtocolId The value of the "Security Protocol"
parameter of
+ the security protocol command to be sent.
+ @param SecurityProtocolSpecificData The value of the "Security
Protocol
Specific" parameter
+ of the security protocol command to be sent.
+ @param PayloadBufferSize Size in bytes of the payload data buffer.
+ @param PayloadBuffer A pointer to a destination buffer to store
the security
+ protocol command specific payload data for the
security
+ protocol command. The caller is responsible for having
+ either implicit or explicit ownership of the buffer.
+ @param PayloadTransferSize A pointer to a buffer to store the size
in
bytes of the
+ data written to the payload data buffer.
+
+ @retval EFI_SUCCESS The security protocol command
completed
successfully.
+ @retval EFI_WARN_BUFFER_TOO_SMALL The PayloadBufferSize was
too
small to store the available
+ data from the device. The PayloadBuffer contains the
truncated data.
+ @retval EFI_UNSUPPORTED The given MediaId does not support
security protocol commands.
+ @retval EFI_DEVICE_ERROR The security protocol command
completed with an error.
+ @retval EFI_NO_MEDIA There is no media in the device.
+ @retval EFI_MEDIA_CHANGED The MediaId is not for the current
media.
+ @retval EFI_INVALID_PARAMETER The PayloadBuffer or
PayloadTransferSize is NULL and
+ PayloadBufferSize is non-zero.
+ @retval EFI_TIMEOUT A timeout occurred while waiting for the
security
+ protocol command to execute.
+
+**/
+EFI_STATUS
+EFIAPI
+ScsiDiskReceiveData (
+ IN EFI_STORAGE_SECURITY_COMMAND_PROTOCOL *This,
+ IN UINT32 MediaId OPTIONAL,
+ IN UINT64 Timeout,
+ IN UINT8 SecurityProtocolId,
+ IN UINT16 SecurityProtocolSpecificData,
+ IN UINTN PayloadBufferSize,
+ OUT VOID *PayloadBuffer,
+ OUT UINTN *PayloadTransferSize
+ );
+
+/**
+ Send a security protocol command to a device.
+
+ The SendData function sends a security protocol command containing
the
payload
+ PayloadBuffer to the given MediaId. The security protocol command
sent
is
+ defined by SecurityProtocolId and contains the security protocol specific
data
+ SecurityProtocolSpecificData. If the underlying protocol command
requires
a
+ specific padding for the command payload, the SendData function shall
add padding
+ bytes to the command payload to satisfy the padding requirements.
+
+ For devices supporting the SCSI command set, the security protocol
command is sent
+ using the SECURITY PROTOCOL OUT command defined in SPC-4.
+
+ If PayloadBuffer is NULL and PayloadBufferSize is non-zero, the function
shall
+ return EFI_INVALID_PARAMETER.
+
+ If the given MediaId does not support security protocol commands, the
function
+ shall return EFI_UNSUPPORTED. If there is no media in the device, the
function
+ returns EFI_NO_MEDIA. If the MediaId is not the ID for the current
media
in the
+ device, the function returns EFI_MEDIA_CHANGED.
+
+ If the security protocol fails to complete within the Timeout period, the
function
+ shall return EFI_TIMEOUT.
+
+ If the security protocol command completes without an error, the
function
shall return
+ EFI_SUCCESS. If the security protocol command completes with an error,
the function
+ shall return EFI_DEVICE_ERROR.
+
+ @param This Indicates a pointer to the calling context.
+ @param MediaId ID of the medium to receive data from.
+ @param Timeout The timeout, in 100ns units, to use for the
execution
+ of the security protocol command. A Timeout value of
0
+ means that this function will wait indefinitely for the
+ security protocol command to execute. If Timeout is
greater
+ than zero, then this function will return EFI_TIMEOUT
if
the
+ time required to execute the receive data command is
greater than Timeout.
+ @param SecurityProtocolId The value of the "Security Protocol"
parameter of
+ the security protocol command to be sent.
+ @param SecurityProtocolSpecificData The value of the "Security
Protocol
Specific" parameter
+ of the security protocol command to be sent.
+ @param PayloadBufferSize Size in bytes of the payload data buffer.
+ @param PayloadBuffer A pointer to a destination buffer to store
the security
+ protocol command specific payload data for the
security
+ protocol command.
+
+ @retval EFI_SUCCESS The security protocol command
completed
successfully.
+ @retval EFI_UNSUPPORTED The given MediaId does not support
security protocol commands.
+ @retval EFI_DEVICE_ERROR The security protocol command
completed with an error.
+ @retval EFI_NO_MEDIA There is no media in the device.
+ @retval EFI_MEDIA_CHANGED The MediaId is not for the current
media.
+ @retval EFI_INVALID_PARAMETER The PayloadBuffer is NULL and
PayloadBufferSize is non-zero.
+ @retval EFI_TIMEOUT A timeout occurred while waiting for the
security
+ protocol command to execute.
+
+**/
+EFI_STATUS
+EFIAPI
+ScsiDiskSendData (
+ IN EFI_STORAGE_SECURITY_COMMAND_PROTOCOL *This,
+ IN UINT32 MediaId OPTIONAL,
+ IN UINT64 Timeout,
+ IN UINT8 SecurityProtocolId,
+ IN UINT16 SecurityProtocolSpecificData,
+ IN UINTN PayloadBufferSize,
+ OUT VOID *PayloadBuffer
+ );
+
+
/**
Provides inquiry information for the controller type.

@@ -1428,4 +1581,20 @@ DetermineInstallEraseBlock (
IN EFI_HANDLE ChildHandle
);

+/**
+ Determine if EFI Storage Security Command Protocol should be produced.
+
+ @param ScsiDiskDevice The pointer of SCSI_DISK_DEV.
+ @param ChildHandle Handle of device.
+
+ @retval TRUE Should produce EFI Storage Security Command Protocol.
+ @retval FALSE Should not produce EFI Storage Security Command
Protocol.
+
+**/
+BOOLEAN
+DetermineInstallStorageSecurity (
+ IN SCSI_DISK_DEV *ScsiDiskDevice,
+ IN EFI_HANDLE ChildHandle
+ );
+
#endif
diff --git a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
index c4069aec0f..1caffd38cd 100644
--- a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
+++ b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
@@ -2,7 +2,7 @@
SCSI Bus driver that layers on every SCSI Pass Thru and
Extended SCSI Pass Thru protocol in the system.

-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -1368,7 +1368,8 @@ DiscoverScsiDevice (
goto Done;
}

- if (0x1e >= InquiryData->Peripheral_Type && InquiryData-
Peripheral_Type >= 0xa) {
+ if ((InquiryData->Peripheral_Type >= EFI_SCSI_TYPE_RESERVED_LOW)
&&
+ (InquiryData->Peripheral_Type <= EFI_SCSI_TYPE_RESERVED_HIGH)) {
ScsiDeviceFound = FALSE;
goto Done;
}
diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
index fbdf927a11..01d5ad4969 100644
--- a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
+++ b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
@@ -1,7 +1,7 @@
/** @file
SCSI disk driver that layers on every SCSI IO protocol in the system.

-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -151,7 +151,9 @@ ScsiDiskDriverBindingSupported (

Status = ScsiIo->GetDeviceType (ScsiIo, &DeviceType);
if (!EFI_ERROR (Status)) {
- if ((DeviceType == EFI_SCSI_TYPE_DISK) || (DeviceType ==
EFI_SCSI_TYPE_CDROM)) {
+ if ((DeviceType == EFI_SCSI_TYPE_DISK) ||
+ (DeviceType == EFI_SCSI_TYPE_CDROM) ||
+ (DeviceType == EFI_SCSI_TYPE_WLUN)) {
Status = EFI_SUCCESS;
} else {
Status = EFI_UNSUPPORTED;
@@ -238,6 +240,8 @@ ScsiDiskDriverBindingStart (
ScsiDiskDevice->BlkIo2.ReadBlocksEx = ScsiDiskReadBlocksEx;
ScsiDiskDevice->BlkIo2.WriteBlocksEx = ScsiDiskWriteBlocksEx;
ScsiDiskDevice->BlkIo2.FlushBlocksEx = ScsiDiskFlushBlocksEx;
+ ScsiDiskDevice->StorageSecurity.ReceiveData = ScsiDiskReceiveData;
+ ScsiDiskDevice->StorageSecurity.SendData = ScsiDiskSendData;
ScsiDiskDevice->EraseBlock.Revision =
EFI_ERASE_BLOCK_PROTOCOL_REVISION;
ScsiDiskDevice->EraseBlock.EraseLengthGranularity = 1;
ScsiDiskDevice->EraseBlock.EraseBlocks = ScsiDiskEraseBlocks;
@@ -258,6 +262,10 @@ ScsiDiskDriverBindingStart (
ScsiDiskDevice->BlkIo.Media->ReadOnly = TRUE;
MustReadCapacity = FALSE;
break;
+
+ case EFI_SCSI_TYPE_WLUN:
+ MustReadCapacity = FALSE;
+ break;
}
//
// The Sense Data Array's initial size is 6
@@ -309,8 +317,8 @@ ScsiDiskDriverBindingStart (
// Determine if Block IO & Block IO2 should be produced on this
controller
// handle
//
- if (DetermineInstallBlockIo(Controller)) {
- InitializeInstallDiskInfo(ScsiDiskDevice, Controller);
+ if (DetermineInstallBlockIo (Controller)) {
+ InitializeInstallDiskInfo (ScsiDiskDevice, Controller);
Status = gBS->InstallMultipleProtocolInterfaces (
&Controller,
&gEfiBlockIoProtocolGuid,
@@ -321,16 +329,27 @@ ScsiDiskDriverBindingStart (
&ScsiDiskDevice->DiskInfo,
NULL
);
- if (!EFI_ERROR(Status)) {
- if (DetermineInstallEraseBlock(ScsiDiskDevice, Controller)) {
+ if (!EFI_ERROR (Status)) {
+ if (DetermineInstallEraseBlock (ScsiDiskDevice, Controller)) {
Status = gBS->InstallProtocolInterface (
&Controller,
&gEfiEraseBlockProtocolGuid,
EFI_NATIVE_INTERFACE,
&ScsiDiskDevice->EraseBlock
);
- if (EFI_ERROR(Status)) {
- DEBUG ((EFI_D_ERROR, "ScsiDisk: Failed to install the Erase Block
Protocol! Status = %r\n", Status));
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "ScsiDisk: Failed to install the Erase Block
Protocol! Status = %r\n", Status));
+ }
+ }
+ if (DetermineInstallStorageSecurity (ScsiDiskDevice, Controller)) {
+ Status = gBS->InstallProtocolInterface (
+ &Controller,
+ &gEfiStorageSecurityCommandProtocolGuid,
+ EFI_NATIVE_INTERFACE,
+ &ScsiDiskDevice->StorageSecurity
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "ScsiDisk: Failed to install the Storage
Security Command Protocol! Status = %r\n", Status));
}
}
ScsiDiskDevice->ControllerNameTable = NULL;
@@ -606,6 +625,11 @@ ScsiDiskReadBlocks (
//
BlockSize = Media->BlockSize;

+ if (BlockSize == 0) {
+ Status = EFI_UNSUPPORTED;
+ goto Done;
+ }
+
NumberOfBlocks = BufferSize / BlockSize;

if (!(Media->MediaPresent)) {
@@ -742,6 +766,11 @@ ScsiDiskWriteBlocks (
//
BlockSize = Media->BlockSize;

+ if (BlockSize == 0) {
+ Status = EFI_UNSUPPORTED;
+ goto Done;
+ }
+
NumberOfBlocks = BufferSize / BlockSize;

if (!(Media->MediaPresent)) {
@@ -968,6 +997,11 @@ ScsiDiskReadBlocksEx (
//
BlockSize = Media->BlockSize;

+ if (BlockSize == 0) {
+ Status = EFI_UNSUPPORTED;
+ goto Done;
+ }
+
NumberOfBlocks = BufferSize / BlockSize;

if (!(Media->MediaPresent)) {
@@ -1131,6 +1165,11 @@ ScsiDiskWriteBlocksEx (
//
BlockSize = Media->BlockSize;

+ if (BlockSize == 0) {
+ Status = EFI_UNSUPPORTED;
+ goto Done;
+ }
+
NumberOfBlocks = BufferSize / BlockSize;

if (!(Media->MediaPresent)) {
@@ -1708,6 +1747,393 @@ Done:
return Status;
}

+/**
+ Send a security protocol command to a device that receives data and/or
the result
+ of one or more commands sent by SendData.
+
+ The ReceiveData function sends a security protocol command to the
given
MediaId.
+ The security protocol command sent is defined by SecurityProtocolId and
contains
+ the security protocol specific data SecurityProtocolSpecificData. The
function
+ returns the data from the security protocol command in PayloadBuffer.
+
+ For devices supporting the SCSI command set, the security protocol
command is sent
+ using the SECURITY PROTOCOL IN command defined in SPC-4.
+
+ If PayloadBufferSize is too small to store the available data from the
security
+ protocol command, the function shall copy PayloadBufferSize bytes into
the
+ PayloadBuffer and return EFI_WARN_BUFFER_TOO_SMALL.
+
+ If PayloadBuffer or PayloadTransferSize is NULL and PayloadBufferSize is
non-zero,
+ the function shall return EFI_INVALID_PARAMETER.
+
+ If the given MediaId does not support security protocol commands, the
function shall
+ return EFI_UNSUPPORTED. If there is no media in the device, the
function
returns
+ EFI_NO_MEDIA. If the MediaId is not the ID for the current media in the
device,
+ the function returns EFI_MEDIA_CHANGED.
+
+ If the security protocol fails to complete within the Timeout period, the
function
+ shall return EFI_TIMEOUT.
+
+ If the security protocol command completes without an error, the
function
shall
+ return EFI_SUCCESS. If the security protocol command completes with
an
error, the
+ function shall return EFI_DEVICE_ERROR.
+
+ @param This Indicates a pointer to the calling context.
+ @param MediaId ID of the medium to receive data from.
+ @param Timeout The timeout, in 100ns units, to use for the
execution
+ of the security protocol command. A Timeout value of
0
+ means that this function will wait indefinitely for the
+ security protocol command to execute. If Timeout is
greater
+ than zero, then this function will return EFI_TIMEOUT
if
the
+ time required to execute the receive data command is
greater than Timeout.
+ @param SecurityProtocolId The value of the "Security Protocol"
parameter of
+ the security protocol command to be sent.
+ @param SecurityProtocolSpecificData The value of the "Security
Protocol
Specific" parameter
+ of the security protocol command to be sent.
+ @param PayloadBufferSize Size in bytes of the payload data buffer.
+ @param PayloadBuffer A pointer to a destination buffer to store
the security
+ protocol command specific payload data for the
security
+ protocol command. The caller is responsible for having
+ either implicit or explicit ownership of the buffer.
+ @param PayloadTransferSize A pointer to a buffer to store the size
in
bytes of the
+ data written to the payload data buffer.
+
+ @retval EFI_SUCCESS The security protocol command
completed
successfully.
+ @retval EFI_WARN_BUFFER_TOO_SMALL The PayloadBufferSize was
too
small to store the available
+ data from the device. The PayloadBuffer contains the
truncated data.
+ @retval EFI_UNSUPPORTED The given MediaId does not support
security protocol commands.
+ @retval EFI_DEVICE_ERROR The security protocol command
completed with an error.
+ @retval EFI_NO_MEDIA There is no media in the device.
+ @retval EFI_MEDIA_CHANGED The MediaId is not for the current
media.
+ @retval EFI_INVALID_PARAMETER The PayloadBuffer or
PayloadTransferSize is NULL and
+ PayloadBufferSize is non-zero.
+ @retval EFI_TIMEOUT A timeout occurred while waiting for the
security
+ protocol command to execute.
+
+**/
+EFI_STATUS
+EFIAPI
+ScsiDiskReceiveData (
+ IN EFI_STORAGE_SECURITY_COMMAND_PROTOCOL *This,
+ IN UINT32 MediaId OPTIONAL,
+ IN UINT64 Timeout,
+ IN UINT8 SecurityProtocolId,
+ IN UINT16 SecurityProtocolSpecificData,
+ IN UINTN PayloadBufferSize,
+ OUT VOID *PayloadBuffer,
+ OUT UINTN *PayloadTransferSize
+ )
+{
+ SCSI_DISK_DEV *ScsiDiskDevice;
+ EFI_BLOCK_IO_MEDIA *Media;
+ EFI_STATUS Status;
+ BOOLEAN MediaChange;
+ EFI_TPL OldTpl;
+ UINT8 SenseDataLength;
+ UINT8 HostAdapterStatus;
+ UINT8 TargetStatus;
+ VOID *AlignedBuffer;
+ BOOLEAN AlignedBufferAllocated;
+
+ AlignedBuffer = NULL;
+ MediaChange = FALSE;
+ AlignedBufferAllocated = FALSE;
+ OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
+ ScsiDiskDevice = SCSI_DISK_DEV_FROM_STORSEC (This);
+ Media = ScsiDiskDevice->BlkIo.Media;
+
+ SenseDataLength = (UINT8) (ScsiDiskDevice->SenseDataNumber * sizeof
(EFI_SCSI_SENSE_DATA));
+
+ if (!DetermineInstallStorageSecurity (ScsiDiskDevice, ScsiDiskDevice-
Handle)) {
+ Status = EFI_UNSUPPORTED;
+ goto Done;
+ }

I think the above 'if' statement is not needed, and should be placed
within the below 'if' statement when handling media change:

if (MediaChange) {...}

(Similar case to ScsiDiskSendData().)

+
+ if (!IS_DEVICE_FIXED (ScsiDiskDevice)) {
+ Status = ScsiDiskDetectMedia (ScsiDiskDevice, FALSE, &MediaChange);
+ if (EFI_ERROR (Status)) {
+ Status = EFI_DEVICE_ERROR;
+ goto Done;
+ }
+
+ if (MediaChange) {
+ gBS->ReinstallProtocolInterface (
+ ScsiDiskDevice->Handle,
+ &gEfiStorageSecurityCommandProtocolGuid,
+ &ScsiDiskDevice->StorageSecurity,
+ &ScsiDiskDevice->StorageSecurity
+ );

The BlockIO(2) & EraseBlocks (if supported) should be reinstalled as well.

(Similar case to ScsiDiskSendData().)


+ if (Media->MediaPresent) {
+ Status = EFI_MEDIA_CHANGED;
+ } else {
+ Status = EFI_NO_MEDIA;
+ }
+ goto Done;
+ }
+ }
+
+ //
+ // Validate Media
+ //
+ if (!(Media->MediaPresent)) {
+ Status = EFI_NO_MEDIA;
+ goto Done;
+ }
+
+ if ((MediaId != 0) && (MediaId != Media->MediaId)) {
+ Status = EFI_MEDIA_CHANGED;
+ goto Done;
+ }
+
+ if (PayloadBufferSize != 0) {
+ if ((PayloadBuffer == NULL) || (PayloadTransferSize == NULL)) {
+ Status = EFI_INVALID_PARAMETER;
+ goto Done;
+ }
+
+ if ((ScsiDiskDevice->ScsiIo->IoAlign > 1) && !IS_ALIGNED (PayloadBuffer,
ScsiDiskDevice->ScsiIo->IoAlign)) {
+ AlignedBuffer = AllocateAlignedBuffer (ScsiDiskDevice,
PayloadBufferSize);
+ if (AlignedBuffer == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Done;
+ }
+ ZeroMem (AlignedBuffer, PayloadBufferSize);
+ AlignedBufferAllocated = TRUE;
+ } else {
+ AlignedBuffer = PayloadBuffer;
+ }
+ }
+
+ Status = ScsiSecurityProtocolInCommand (
+ ScsiDiskDevice->ScsiIo,
+ Timeout,
+ ScsiDiskDevice->SenseData,
+ &SenseDataLength,
+ &HostAdapterStatus,
+ &TargetStatus,
+ SecurityProtocolId,
+ SecurityProtocolSpecificData,
+ (UINT32) PayloadBufferSize,
+ AlignedBuffer,
+ (UINT32 *) PayloadTransferSize
+ );
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ if (AlignedBufferAllocated) {
+ CopyMem (PayloadBuffer, AlignedBuffer, PayloadBufferSize);
+ }
+
+ if (PayloadBufferSize < *PayloadTransferSize) {
+ Status = EFI_WARN_BUFFER_TOO_SMALL;
+ goto Done;
+ }
+
+ Status = CheckHostAdapterStatus (HostAdapterStatus);
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ Status = CheckTargetStatus (TargetStatus);
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+Done:
+ if (AlignedBufferAllocated) {

IMO, the content in 'AlignedBuffer' should be wiped out, since it might
contain sensitive information. SSC protocol user can control the data in
'PayloadBuffer' but not in 'AlignedBuffer'.


+ FreeAlignedBuffer (AlignedBuffer, PayloadBufferSize);
+ }
+ gBS->RestoreTPL (OldTpl);
+ return Status;
+}
+
+/**
+ Send a security protocol command to a device.
+
+ The SendData function sends a security protocol command containing
the
payload
+ PayloadBuffer to the given MediaId. The security protocol command
sent
is
+ defined by SecurityProtocolId and contains the security protocol specific
data
+ SecurityProtocolSpecificData. If the underlying protocol command
requires
a
+ specific padding for the command payload, the SendData function shall
add padding
+ bytes to the command payload to satisfy the padding requirements.
+
+ For devices supporting the SCSI command set, the security protocol
command is sent
+ using the SECURITY PROTOCOL OUT command defined in SPC-4.
+
+ If PayloadBuffer is NULL and PayloadBufferSize is non-zero, the function
shall
+ return EFI_INVALID_PARAMETER.
+
+ If the given MediaId does not support security protocol commands, the
function
+ shall return EFI_UNSUPPORTED. If there is no media in the device, the
function
+ returns EFI_NO_MEDIA. If the MediaId is not the ID for the current
media
in the
+ device, the function returns EFI_MEDIA_CHANGED.
+
+ If the security protocol fails to complete within the Timeout period, the
function
+ shall return EFI_TIMEOUT.
+
+ If the security protocol command completes without an error, the
function
shall return
+ EFI_SUCCESS. If the security protocol command completes with an error,
the function
+ shall return EFI_DEVICE_ERROR.
+
+ @param This Indicates a pointer to the calling context.
+ @param MediaId ID of the medium to receive data from.
+ @param Timeout The timeout, in 100ns units, to use for the
execution
+ of the security protocol command. A Timeout value of
0
+ means that this function will wait indefinitely for the
+ security protocol command to execute. If Timeout is
greater
+ than zero, then this function will return EFI_TIMEOUT
if
the
+ time required to execute the receive data command is
greater than Timeout.
+ @param SecurityProtocolId The value of the "Security Protocol"
parameter of
+ the security protocol command to be sent.
+ @param SecurityProtocolSpecificData The value of the "Security
Protocol
Specific" parameter
+ of the security protocol command to be sent.
+ @param PayloadBufferSize Size in bytes of the payload data buffer.
+ @param PayloadBuffer A pointer to a destination buffer to store
the security
+ protocol command specific payload data for the
security
+ protocol command.
+
+ @retval EFI_SUCCESS The security protocol command
completed
successfully.
+ @retval EFI_UNSUPPORTED The given MediaId does not support
security protocol commands.
+ @retval EFI_DEVICE_ERROR The security protocol command
completed with an error.
+ @retval EFI_NO_MEDIA There is no media in the device.
+ @retval EFI_MEDIA_CHANGED The MediaId is not for the current
media.
+ @retval EFI_INVALID_PARAMETER The PayloadBuffer is NULL and
PayloadBufferSize is non-zero.
+ @retval EFI_TIMEOUT A timeout occurred while waiting for the
security
+ protocol command to execute.
+
+**/
+EFI_STATUS
+EFIAPI
+ScsiDiskSendData (
+ IN EFI_STORAGE_SECURITY_COMMAND_PROTOCOL *This,
+ IN UINT32 MediaId OPTIONAL,
+ IN UINT64 Timeout,
+ IN UINT8 SecurityProtocolId,
+ IN UINT16 SecurityProtocolSpecificData,
+ IN UINTN PayloadBufferSize,
+ OUT VOID *PayloadBuffer
+ )
+{
+ SCSI_DISK_DEV *ScsiDiskDevice;
+ EFI_BLOCK_IO_MEDIA *Media;
+ EFI_STATUS Status;
+ BOOLEAN MediaChange;
+ EFI_TPL OldTpl;
+ UINT8 SenseDataLength;
+ UINT8 HostAdapterStatus;
+ UINT8 TargetStatus;
+ VOID *AlignedBuffer;
+ BOOLEAN AlignedBufferAllocated;
+
+ AlignedBuffer = NULL;
+ MediaChange = FALSE;
+ AlignedBufferAllocated = FALSE;
+ OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
+ ScsiDiskDevice = SCSI_DISK_DEV_FROM_STORSEC (This);
+ Media = ScsiDiskDevice->BlkIo.Media;
+
+ SenseDataLength = (UINT8) (ScsiDiskDevice->SenseDataNumber * sizeof
(EFI_SCSI_SENSE_DATA));
+
+ if (!DetermineInstallStorageSecurity (ScsiDiskDevice, ScsiDiskDevice-
Handle)) {
+ Status = EFI_UNSUPPORTED;
+ goto Done;
+ }
+
+ if (!IS_DEVICE_FIXED (ScsiDiskDevice)) {
+ Status = ScsiDiskDetectMedia (ScsiDiskDevice, FALSE, &MediaChange);
+ if (EFI_ERROR (Status)) {
+ Status = EFI_DEVICE_ERROR;
+ goto Done;
+ }
+
+ if (MediaChange) {
+ gBS->ReinstallProtocolInterface (
+ ScsiDiskDevice->Handle,
+ &gEfiStorageSecurityCommandProtocolGuid,
+ &ScsiDiskDevice->StorageSecurity,
+ &ScsiDiskDevice->StorageSecurity
+ );
+ if (Media->MediaPresent) {
+ Status = EFI_MEDIA_CHANGED;
+ } else {
+ Status = EFI_NO_MEDIA;
+ }
+ goto Done;
+ }
+ }
+
+ //
+ // Validate Media
+ //
+ if (!(Media->MediaPresent)) {
+ Status = EFI_NO_MEDIA;
+ goto Done;
+ }
+
+ if ((MediaId != 0) && (MediaId != Media->MediaId)) {
+ Status = EFI_MEDIA_CHANGED;
+ goto Done;
+ }
+
+ if (Media->ReadOnly) {
+ Status = EFI_WRITE_PROTECTED;
+ goto Done;
+ }
+
+ if (PayloadBufferSize != 0) {
+ if (PayloadBuffer == NULL) {
+ Status = EFI_INVALID_PARAMETER;
+ goto Done;
+ }
+
+ if ((ScsiDiskDevice->ScsiIo->IoAlign > 1) && !IS_ALIGNED (PayloadBuffer,
ScsiDiskDevice->ScsiIo->IoAlign)) {
+ AlignedBuffer = AllocateAlignedBuffer (ScsiDiskDevice,
PayloadBufferSize);
+ if (AlignedBuffer == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Done;
+ }
+ CopyMem (AlignedBuffer, PayloadBuffer, PayloadBufferSize);
+ AlignedBufferAllocated = TRUE;
+ } else {
+ AlignedBuffer = PayloadBuffer;
+ }
+ }
+
+ Status = ScsiSecurityProtocolOutCommand (
+ ScsiDiskDevice->ScsiIo,
+ Timeout,
+ ScsiDiskDevice->SenseData,
+ &SenseDataLength,
+ &HostAdapterStatus,
+ &TargetStatus,
+ SecurityProtocolId,
+ SecurityProtocolSpecificData,
+ (UINT32) PayloadBufferSize,
+ AlignedBuffer
+ );
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ Status = CheckHostAdapterStatus (HostAdapterStatus);
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ Status = CheckTargetStatus (TargetStatus);
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+Done:
+ if (AlignedBufferAllocated) {

IMO, the content in 'AlignedBuffer' should be wiped out, since it might
contain sensitive information. SSC protocol user can control the data in
'PayloadBuffer' but not in 'AlignedBuffer'.


+ FreeAlignedBuffer (AlignedBuffer, PayloadBufferSize);
+ }
+ gBS->RestoreTPL (OldTpl);
+ return Status;
+}
+

/**
Detect Device and read out capacity ,if error occurs, parse the sense key.
@@ -2261,6 +2687,8 @@ ScsiDiskTestUnitReady (
return EFI_DEVICE_ERROR;
}

+ ScsiDiskDevice->BlkIo.Media->MediaPresent = TRUE;
+
if (SenseDataLength != 0) {
*NumberOfSenseKeys = SenseDataLength / sizeof
(EFI_SCSI_SENSE_DATA);
*SenseDataArray = ScsiDiskDevice->SenseData;
@@ -2315,13 +2743,12 @@ DetectMediaParsingSenseKeys (
BOOLEAN RetryLater;

//
- // Default is to read capacity, unless..
+ // Default is no action
//
- *Action = ACTION_READ_CAPACITY;
+ *Action = ACTION_NO_ACTION;

if (NumberOfSenseKeys == 0) {
if (ScsiDiskDevice->BlkIo.Media->MediaPresent == TRUE) {
- *Action = ACTION_NO_ACTION;
}

I do not think the behavior is the same after the patch.
Can it be:

if (NumberOfSenseKeys == 0) {
if (!ScsiDiskDevice->BlkIo.Media->MediaPresent) {
*Action = ACTION_READ_CAPACITY;
}
return EFI_SUCCESS;
}


return EFI_SUCCESS;
}
@@ -2331,7 +2758,6 @@ DetectMediaParsingSenseKeys (
// No Sense Key returned from last submitted command
//
if (ScsiDiskDevice->BlkIo.Media->MediaPresent == TRUE) {
- *Action = ACTION_NO_ACTION;
}

I do not think the behavior is the same after the patch.
Can it be:

if (!ScsiDiskHaveSenseKey (SenseData, NumberOfSenseKeys)) {
//
// No Sense Key returned from last submitted command
//
if (!ScsiDiskDevice->BlkIo.Media->MediaPresent) {
*Action = ACTION_READ_CAPACITY;
}
return EFI_SUCCESS;
}


Best Regards,
Hao Wu


return EFI_SUCCESS;
}
@@ -2339,13 +2765,13 @@ DetectMediaParsingSenseKeys (
if (ScsiDiskIsNoMedia (SenseData, NumberOfSenseKeys)) {
ScsiDiskDevice->BlkIo.Media->MediaPresent = FALSE;
ScsiDiskDevice->BlkIo.Media->LastBlock = 0;
- *Action = ACTION_NO_ACTION;
DEBUG ((EFI_D_VERBOSE, "ScsiDisk: ScsiDiskIsNoMedia\n"));
return EFI_SUCCESS;
}

if (ScsiDiskIsMediaChange (SenseData, NumberOfSenseKeys)) {
ScsiDiskDevice->BlkIo.Media->MediaId++;
+ *Action = ACTION_READ_CAPACITY;
DEBUG ((EFI_D_VERBOSE, "ScsiDisk: ScsiDiskIsMediaChange!\n"));
return EFI_SUCCESS;
}
@@ -2374,7 +2800,6 @@ DetectMediaParsingSenseKeys (
DEBUG ((EFI_D_VERBOSE, "ScsiDisk: ScsiDiskDriveNotReady!\n"));
return EFI_SUCCESS;
}
- *Action = ACTION_NO_ACTION;
return EFI_DEVICE_ERROR;
}

@@ -2808,8 +3233,6 @@ GetMediaInfo (
}
}
}
-
- ScsiDiskDevice->BlkIo.Media->MediaPresent = TRUE;
}

/**
@@ -5358,6 +5781,14 @@ DetermineInstallEraseBlock (
RetVal = TRUE;
CapacityData16 = NULL;

+ //
+ // UNMAP command is not supported by any of the UFS WLUNs.
+ //
+ if (ScsiDiskDevice->DeviceType == EFI_SCSI_TYPE_WLUN) {
+ RetVal = FALSE;
+ goto Done;
+ }
+
Status = gBS->HandleProtocol (
ChildHandle,
&gEfiDevicePathProtocolGuid,
@@ -5460,6 +5891,65 @@ Done:
return RetVal;
}

+/**
+ Determine if EFI Storage Security Command Protocol should be produced.
+
+ @param ScsiDiskDevice The pointer of SCSI_DISK_DEV.
+ @param ChildHandle Handle of device.
+
+ @retval TRUE Should produce EFI Storage Security Command Protocol.
+ @retval FALSE Should not produce EFI Storage Security Command
Protocol.
+
+**/
+BOOLEAN
+DetermineInstallStorageSecurity (
+ IN SCSI_DISK_DEV *ScsiDiskDevice,
+ IN EFI_HANDLE ChildHandle
+ )
+{
+ EFI_STATUS Status;
+ UFS_DEVICE_PATH *UfsDevice;
+ BOOLEAN RetVal;
+ EFI_DEVICE_PATH_PROTOCOL *DevicePathNode;
+
+ UfsDevice = NULL;
+ RetVal = TRUE;
+
+ Status = gBS->HandleProtocol (
+ ChildHandle,
+ &gEfiDevicePathProtocolGuid,
+ (VOID **) &DevicePathNode
+ );
+ //
+ // Device Path protocol must be installed on the device handle.
+ //
+ ASSERT_EFI_ERROR (Status);
+
+ while (!IsDevicePathEndType (DevicePathNode)) {
+ //
+ // For now, only support Storage Security Command Protocol on UFS
devices.
+ //
+ if ((DevicePathNode->Type == MESSAGING_DEVICE_PATH) &&
+ (DevicePathNode->SubType == MSG_UFS_DP)) {
+ UfsDevice = (UFS_DEVICE_PATH *) DevicePathNode;
+ break;
+ }
+
+ DevicePathNode = NextDevicePathNode (DevicePathNode);
+ }
+ if (UfsDevice == NULL) {
+ RetVal = FALSE;
+ goto Done;
+ }
+
+ if (UfsDevice->Lun != UFS_WLUN_RPMB) {
+ RetVal = FALSE;
+ }
+
+Done:
+ return RetVal;
+}
+
/**
Provides inquiry information for the controller type.

diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
index 1518b251d8..0bb67f2ddc 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
@@ -1,6 +1,6 @@
/** @file

- Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2014 - 2019, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -819,7 +819,9 @@ UfsPassThruDriverBindingStart (
UINTN UfsHcBase;
UINT32 Index;
UFS_UNIT_DESC UnitDescriptor;
+ UFS_DEV_DESC DeviceDescriptor;
UINT32 UnitDescriptorSize;
+ UINT32 DeviceDescriptorSize;

Status = EFI_SUCCESS;
UfsHc = NULL;
@@ -894,7 +896,6 @@ UfsPassThruDriverBindingStart (

//
// Check if 8 common luns are active and set corresponding bit mask.
- // TODO: Parse device descriptor to decide if exposing RPMB LUN to
upper
layer for authentication access.
//
UnitDescriptorSize = sizeof (UFS_UNIT_DESC);
for (Index = 0; Index < 8; Index++) {
@@ -909,6 +910,20 @@ UfsPassThruDriverBindingStart (
}
}

+ //
+ // Check if RPMB WLUN is supported and set corresponding bit mask.
+ //
+ DeviceDescriptorSize = sizeof (UFS_DEV_DESC);
+ Status = UfsRwDeviceDesc (Private, TRUE, UfsDeviceDesc, 0, 0,
&DeviceDescriptor, &DeviceDescriptorSize);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed to read device descriptor, status
= %r\n",
Status));
+ } else {
+ if (DeviceDescriptor.SecurityLun == 0x1) {
+ DEBUG ((DEBUG_INFO, "UFS WLUN RPMB is supported\n"));
+ Private->Luns.BitMask |= BIT11;
+ }
+ }
+
//
// Start the asynchronous interrupt monitor
//
--
2.16.2.windows.1



[PATCH] Python run fail if env variable PYTHON_HOME is not set

Cheng, Ching JenX
 

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

In Platform\Intel\MinPlatformPkg\Tools\Fsp\RebaseFspBinBaseAddress.py
It will run another python code.
But if the environment variable "PYTHON_HOME" is not exist
and we didn't add any python's path to "PATH".
It will cause error because python command not found.

the error message as below:
'python' is not recognized as an internal or external command,
operable program or batch file.

So we set the python's path from which execute the python code
if PYTHON_HOME was not exist.

Cc: Amy Chan <amy.chan@intel.com>
Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Ching JenX Cheng <ching.jenx.cheng@intel.com>
---
Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseAndPatchFspBinBaseAddress.py | 2 ++
1 file changed, 2 insertions(+)

diff --git a/Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseAndPatchFspBinBaseAddress.py b/Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseAndPatchFspBinBaseAddress.py
index 406e5ec130..1d72b4112f 100644
--- a/Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseAndPatchFspBinBaseAddress.py
+++ b/Platform/Intel/MinPlatformPkg/Tools/Fsp/RebaseAndPatchFspBinBaseAddress.py
@@ -76,6 +76,8 @@ file.close()
pythontool = 'python'
if 'PYTHON_HOME' in os.environ:
pythontool = os.environ['PYTHON_HOME'] + os.sep + 'python'
+else:
+ pythontool = sys.executable
Process = subprocess.Popen([pythontool, splitFspBinPath, "info","-f",fspBinFilePath], stdout=subprocess.PIPE)
Output = Process.communicate()[0]
FsptInfo = Output.rsplit(b"FSP_M", 1);
--
2.21.0.windows.1


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

Gao, Zhichao
 

Sorry for missing consider of the commit message #1 and #2.
I got your point. SlitSystemLocalityCount's high 32 bit (actually high 48 bit) data is useless.
On my opinion, this field should be 2 bytes length and the spec should be updated. Then our code can be updated to match the spec.
For now, I think the checking (*SlitSystemLocalityCount > MAX_UINT16) before it is enough.

Thanks,
Zhichao

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Krzysztof Koch
Sent: Monday, August 19, 2019 2:28 PM
To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini
<Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate
System Locality count


Hi Zhichao,

Please find my comments inline marked as [Krzysztof].

Kind regards,

Krzysztof


-----Original Message-----
From: Gao, Zhichao <zhichao.gao@intel.com>
Sent: Monday, August 19, 2019 2:19
To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini
<Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: RE: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate
System Locality count



-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Krzysztof Koch
Sent: Thursday, August 15, 2019 9:11 PM
To: devel@edk2.groups.io
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray
<ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com
Subject: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT:
Validate System Locality count

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

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

3. Remove (now) redundant 64x64 bit multiplication.
Why removing? This change is added to fixed the issue build error with IA32
multiplication of two 64 bits data.
The change of #3 should be removed from the patch.
Keeping the variable size as UINT64 wouldn't affect the result.

Thanks,
Zhichao

[Krzysztof] If you look closely, in this patch I have removed the need to 64x64
bit multiplication. As I explain in the commit message, the specification of the
SLIT table has an error. The System Locality Count is a 64-bit value while the
ACPI table length field is 32-bit long.

Consequently, after the right checks are implemented (in this patch), it is
possible to operate on 32-bit values only. I believe that now MultU64x64() is
no longer needed so I reverted back to the '*' multiplication operator.

Please let me know what you think.


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

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


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

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

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

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

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

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





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

Krzysztof Koch
 

Hi Zhichao,

Please find my comments inline marked as [Krzysztof].

Kind regards,

Krzysztof

-----Original Message-----
From: Gao, Zhichao <zhichao.gao@intel.com>
Sent: Monday, August 19, 2019 2:19
To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: RE: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count



-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Krzysztof Koch
Sent: Thursday, August 15, 2019 9:11 PM
To: devel@edk2.groups.io
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray
<ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com
Subject: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT:
Validate System Locality count

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

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

3. Remove (now) redundant 64x64 bit multiplication.
Why removing? This change is added to fixed the issue build error with IA32 multiplication of two 64 bits data.
The change of #3 should be removed from the patch.
Keeping the variable size as UINT64 wouldn't affect the result.

Thanks,
Zhichao

[Krzysztof] If you look closely, in this patch I have removed the need to 64x64 bit multiplication. As I explain in the commit message, the specification of the SLIT table has an error. The System Locality Count is a 64-bit value while the ACPI table length field is 32-bit long.

Consequently, after the right checks are implemented (in this patch), it is possible to operate on 32-bit values only. I believe that now MultU64x64() is no longer needed so I reverted back to the '*' multiplication operator.

Please let me know what you think.


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

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


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

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

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

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

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

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




Re: [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for missing Memory Attributes Table (MAT) update

Wu, Hao A
 

-----Original Message-----
From: Gao, Liming
Sent: Friday, August 16, 2019 11:40 PM
To: devel@edk2.groups.io
Cc: Mike Turner; Wang, Jian J; Wu, Hao A; Bi, Dandan; Laszlo Ersek
Subject: [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for
missing Memory Attributes Table (MAT) update

From: Mike Turner <miketur@microsoft.com>

The Fpdt driver (FirmwarePerformanceDxe) saves a memory address across
reboots, and then does an AllocatePage for that memory address.
If, on this boot, that memory comes from a Runtime memory bucket,
the MAT table is not updated. This causes Windows to boot into Recovery.

This patch blocks the memory manager from changing the page
from a special bucket to a different memory type. Once the buckets are
allocated, we freeze the memory ranges for the OS, and fragmenting
the special buckets will cause errors resuming from hibernate (S4).

The references to S4 here are the use case that fails. This
failure is root caused to an inconsistent behavior of the
core memory services themselves when type AllocateAddress is used.

The main issue is apparently with the UEFI memory map -- the UEFI memory
map reflects the pre-allocated bins, but the actual allocations at fixed
addresses may go out of sync with that. Everything else, such as:
- EFI_MEMORY_ATTRIBUTES_TABLE (page protections) being out of sync,
- S4 failing
are just symptoms / consequences.

This patch is cherry pick from Project Mu:
https://github.com/microsoft/mu_basecore/commit/a9be767d9be96af9401
6ebd391ea6f340920735a
With the minor change,
1. Update commit message format to keep the message in 80 characters one
line.
2. Remove // MU_CHANGE comments in source code.
3. Update comments style to follow edk2 style.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
In v2, add more description for this issue.

MdeModulePkg/Core/Dxe/Mem/Page.c | 41
++++++++++++++++++++++++++++++++++------
1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
b/MdeModulePkg/Core/Dxe/Mem/Page.c
index bd9e116aa5..1f0e3d94b9 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1265,12 +1265,13 @@ CoreInternalAllocatePages (
IN BOOLEAN NeedGuard
)
{
- EFI_STATUS Status;
- UINT64 Start;
- UINT64 NumberOfBytes;
- UINT64 End;
- UINT64 MaxAddress;
- UINTN Alignment;
+ EFI_STATUS Status;
+ UINT64 Start;
+ UINT64 NumberOfBytes;
+ UINT64 End;
+ UINT64 MaxAddress;
+ UINTN Alignment;
+ EFI_MEMORY_TYPE CheckType;

if ((UINT32)Type >= MaxAllocateType) {
return EFI_INVALID_PARAMETER;
@@ -1321,6 +1322,7 @@ CoreInternalAllocatePages (
// if (Start + NumberOfBytes) rolls over 0 or
// if Start is above MAX_ALLOC_ADDRESS or
// if End is above MAX_ALLOC_ADDRESS,
+ // if Start..End overlaps any tracked MemoryTypeStatistics range
// return EFI_NOT_FOUND.
//
if (Type == AllocateAddress) {
@@ -1336,6 +1338,33 @@ CoreInternalAllocatePages (
(End > MaxAddress)) {
return EFI_NOT_FOUND;
}
+
+ //
+ // A driver is allowed to call AllocatePages using an AllocateAddress type.
This type of
+ // AllocatePage request the exact physical address if it is not used. The
existing code
+ // will allow this request even in 'special' pages. The problem with this is
that the
+ // reason to have 'special' pages for OS hibernate/resume is defeated as
memory is
+ // fragmented.
+ //
+
+ for (CheckType = (EFI_MEMORY_TYPE) 0; CheckType <
EfiMaxMemoryType; CheckType++) {
+ if (MemoryType != CheckType &&
+ mMemoryTypeStatistics[CheckType].Special &&
+ mMemoryTypeStatistics[CheckType].NumberOfPages > 0) {
+ if (Start >= mMemoryTypeStatistics[CheckType].BaseAddress &&
+ Start <= mMemoryTypeStatistics[CheckType].MaximumAddress) {
+ return EFI_NOT_FOUND;
+ }
+ if (End >= mMemoryTypeStatistics[CheckType].BaseAddress &&
+ End <= mMemoryTypeStatistics[CheckType].MaximumAddress) {
+ return EFI_NOT_FOUND;
+ }
+ if (Start < mMemoryTypeStatistics[CheckType].BaseAddress &&
+ End > mMemoryTypeStatistics[CheckType].MaximumAddress) {
+ return EFI_NOT_FOUND;
+ }
+ }
+ }

Acked-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


}

if (Type == AllocateMaxAddress) {
--
2.13.0.windows.1


Re: [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for missing Memory Attributes Table (MAT) update

Dandan Bi
 

Jian and Hao, could you also help take a look at this patch?

This patch is ok to me.
Reviewed-by: Dandan Bi <dandan.bi@intel.com>


Thanks,
Dandan

-----Original Message-----
From: Gao, Liming
Sent: Friday, August 16, 2019 11:40 PM
To: devel@edk2.groups.io
Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [patch v2][edk2-stable201908] MdeModulePkg DxeCore: Fix for
missing Memory Attributes Table (MAT) update

From: Mike Turner <miketur@microsoft.com>

The Fpdt driver (FirmwarePerformanceDxe) saves a memory address across
reboots, and then does an AllocatePage for that memory address.
If, on this boot, that memory comes from a Runtime memory bucket, the
MAT table is not updated. This causes Windows to boot into Recovery.

This patch blocks the memory manager from changing the page from a
special bucket to a different memory type. Once the buckets are allocated,
we freeze the memory ranges for the OS, and fragmenting the special
buckets will cause errors resuming from hibernate (S4).

The references to S4 here are the use case that fails. This failure is root
caused to an inconsistent behavior of the core memory services themselves
when type AllocateAddress is used.

The main issue is apparently with the UEFI memory map -- the UEFI memory
map reflects the pre-allocated bins, but the actual allocations at fixed
addresses may go out of sync with that. Everything else, such as:
- EFI_MEMORY_ATTRIBUTES_TABLE (page protections) being out of sync,
- S4 failing
are just symptoms / consequences.

This patch is cherry pick from Project Mu:
https://github.com/microsoft/mu_basecore/commit/a9be767d9be96af9401
6ebd391ea6f340920735a
With the minor change,
1. Update commit message format to keep the message in 80 characters one
line.
2. Remove // MU_CHANGE comments in source code.
3. Update comments style to follow edk2 style.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
In v2, add more description for this issue.

MdeModulePkg/Core/Dxe/Mem/Page.c | 41
++++++++++++++++++++++++++++++++++------
1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
b/MdeModulePkg/Core/Dxe/Mem/Page.c
index bd9e116aa5..1f0e3d94b9 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1265,12 +1265,13 @@ CoreInternalAllocatePages (
IN BOOLEAN NeedGuard
)
{
- EFI_STATUS Status;
- UINT64 Start;
- UINT64 NumberOfBytes;
- UINT64 End;
- UINT64 MaxAddress;
- UINTN Alignment;
+ EFI_STATUS Status;
+ UINT64 Start;
+ UINT64 NumberOfBytes;
+ UINT64 End;
+ UINT64 MaxAddress;
+ UINTN Alignment;
+ EFI_MEMORY_TYPE CheckType;

if ((UINT32)Type >= MaxAllocateType) {
return EFI_INVALID_PARAMETER;
@@ -1321,6 +1322,7 @@ CoreInternalAllocatePages (
// if (Start + NumberOfBytes) rolls over 0 or
// if Start is above MAX_ALLOC_ADDRESS or
// if End is above MAX_ALLOC_ADDRESS,
+ // if Start..End overlaps any tracked MemoryTypeStatistics range
// return EFI_NOT_FOUND.
//
if (Type == AllocateAddress) {
@@ -1336,6 +1338,33 @@ CoreInternalAllocatePages (
(End > MaxAddress)) {
return EFI_NOT_FOUND;
}
+
+ //
+ // A driver is allowed to call AllocatePages using an AllocateAddress type.
This type of
+ // AllocatePage request the exact physical address if it is not used. The
existing code
+ // will allow this request even in 'special' pages. The problem with this is
that the
+ // reason to have 'special' pages for OS hibernate/resume is defeated as
memory is
+ // fragmented.
+ //
+
+ for (CheckType = (EFI_MEMORY_TYPE) 0; CheckType <
EfiMaxMemoryType; CheckType++) {
+ if (MemoryType != CheckType &&
+ mMemoryTypeStatistics[CheckType].Special &&
+ mMemoryTypeStatistics[CheckType].NumberOfPages > 0) {
+ if (Start >= mMemoryTypeStatistics[CheckType].BaseAddress &&
+ Start <= mMemoryTypeStatistics[CheckType].MaximumAddress) {
+ return EFI_NOT_FOUND;
+ }
+ if (End >= mMemoryTypeStatistics[CheckType].BaseAddress &&
+ End <= mMemoryTypeStatistics[CheckType].MaximumAddress) {
+ return EFI_NOT_FOUND;
+ }
+ if (Start < mMemoryTypeStatistics[CheckType].BaseAddress &&
+ End > mMemoryTypeStatistics[CheckType].MaximumAddress) {
+ return EFI_NOT_FOUND;
+ }
+ }
+ }
}

if (Type == AllocateMaxAddress) {
--
2.13.0.windows.1


Re: [Patch V5 00/11] EmulatorPkg: Fix VS20xx IA32 boot and simplify build config

Jordan Justen
 

On 2019-08-16 17:57:04, Michael D Kinney wrote:

Andrew Fish (7):
EmulatorPkg/Unix/Host: Disable inline/optimizations
Maybe add XCODE5 in commit message subject?

Acked-by: Jordan Justen <jordan.l.justen@intel.com>

EmulatorPkg: Fix XCODE5 lldb issues
There was another long line in the EmulatorPkg/Unix/lldbefi.py change.

Acked-by: Jordan Justen <jordan.l.justen@intel.com>

EmulatorPkg/Unix/Host: Initialize field in BerkeleyPacketFilter.c
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

EmulatorPkg/Unix/Host: Remove debug code from BerkeleyPacketFilter.c
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

EmulatorPkg: Disable TftpDynamicCommand and LogoDxe for XCODE5
Acked-by: Jordan Justen <jordan.l.justen@intel.com>

EmulatorPkg/Sec: Change scope of PpiArray[10]
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

BaseTools/tools_def.template: Add -gdwarf to XCODE5 X64
Acked-by: Jordan Justen <jordan.l.justen@intel.com>


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

Gao, Zhichao
 

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Krzysztof Koch
Sent: Thursday, August 15, 2019 9:11 PM
To: devel@edk2.groups.io
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
Gao, Zhichao <zhichao.gao@intel.com>; Sami.Mujawar@arm.com;
Matteo.Carlini@arm.com; nd@arm.com
Subject: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate
System Locality count

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

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

3. Remove (now) redundant 64x64 bit multiplication.
Why removing? This change is added to fixed the issue build error with IA32 multiplication of two 64 bits data.
The change of #3 should be removed from the patch.
Keeping the variable size as UINT64 wouldn't affect the result.

Thanks,
Zhichao


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

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

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

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

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

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

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

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




Re: [PATCH v2 1/1] ShellPkg/UefiShellAcpiViewCommandLib: Replace shift logical left

Liming Gao
 

Push @5726bdd9a2dfd188a96129e8c00721f34cf3906e

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Zhang, Shenglei
Sent: Friday, August 16, 2019 3:51 PM
To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 1/1]
ShellPkg/UefiShellAcpiViewCommandLib: Replace shift logical left

Hi All,

This is patch to fix build failure. I'd like the patch to be checked in before this
stable tag.

Thanks,
Shenglei

-----Original Message-----
From: Gao, Zhichao
Sent: Friday, August 16, 2019 2:19 PM
To: Zhang, Shenglei <shenglei.zhang@intel.com>; devel@edk2.groups.io
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: RE: [PATCH v2 1/1] ShellPkg/UefiShellAcpiViewCommandLib:
Replace shift logical left

Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

Thanks,
Zhichao

-----Original Message-----
From: Zhang, Shenglei
Sent: Friday, August 16, 2019 2:11 PM
To: devel@edk2.groups.io
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
Gao, Zhichao <zhichao.gao@intel.com>
Subject: [PATCH v2 1/1] ShellPkg/UefiShellAcpiViewCommandLib: Replace
shift logical left

Replace the operation to shift logical left with the function LShiftU64,
which
has the same functionality.
The original code causes ShellPkg build failure with build target"-b NOOPT".

Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
v2: There is no return target from LShiftU64 in v1 patch.
So update code to "Val = LShiftU64(Val,32)".

ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
index 94bafa22ef4c..a569c3c55406 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
@@ -294,7 +294,7 @@ DumpUint64 (

Val = *(UINT32*)(Ptr + sizeof (UINT32));

- Val <<= 32;
+ Val = LShiftU64(Val,32);
Val |= (UINT64)*(UINT32*)Ptr;

Print (Format, Val);
--
2.18.0.windows.1


Re: [PATCH 1/1] CryptoPkg: Fix coding style

Liming Gao
 

push@ 944bd5cf1d69741f3705983f650f6adad696a7d1

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Zhang, Shenglei
Sent: Friday, August 16, 2019 3:56 PM
To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
Cc: Ye, Ting <ting.ye@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/1] CryptoPkg: Fix coding style

Hi All,

This patch has little impact. So I think it's ok to be checked in before code
freeze.

Thanks,
Shenglei

-----Original Message-----
From: Wang, Jian J
Sent: Friday, August 16, 2019 3:18 PM
To: Zhang, Shenglei <shenglei.zhang@intel.com>; devel@edk2.groups.io
Cc: Ye, Ting <ting.ye@intel.com>
Subject: RE: [PATCH 1/1] CryptoPkg: Fix coding style


Reviewed-by: Jian J Wang <jian.j.wang@intel.com>


-----Original Message-----
From: Zhang, Shenglei
Sent: Friday, August 16, 2019 2:54 PM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
Subject: [PATCH 1/1] CryptoPkg: Fix coding style

Update attribute "Out" to "out".
The original "Out" can not pass ECC check.

Cc: Jian Wang <jian.j.wang@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdf.c | 2 +-
CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdfNull.c | 2 +-
CryptoPkg/Include/Library/BaseCryptLib.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdf.c
b/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdf.c
index f0fcef211d3f..3a827dadfcbd 100644
--- a/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdf.c
+++ b/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdf.c
@@ -19,7 +19,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
@param[in] SaltSize Salt size in bytes.
@param[in] Info Pointer to the application specific info.
@param[in] InfoSize Info size in bytes.
- @param[Out] Out Pointer to buffer to receive hkdf value.
+ @param[out] Out Pointer to buffer to receive hkdf value.
@param[in] OutSize Size of hkdf bytes to generate.

@retval TRUE Hkdf generated successfully.
diff --git a/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdfNull.c
b/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdfNull.c
index 73deb5bc3614..19d795a4cc16 100644
--- a/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdfNull.c
+++ b/CryptoPkg/Library/BaseCryptLib/Kdf/CryptHkdfNull.c
@@ -18,7 +18,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
@param[in] SaltSize Salt size in bytes.
@param[in] Info Pointer to the application specific info.
@param[in] InfoSize Info size in bytes.
- @param[Out] Out Pointer to buffer to receive hkdf value.
+ @param[out] Out Pointer to buffer to receive hkdf value.
@param[in] OutSize Size of hkdf bytes to generate.

@retval TRUE Hkdf generated successfully.
diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
b/CryptoPkg/Include/Library/BaseCryptLib.h
index da32bb2444fd..8fe303a0b390 100644
--- a/CryptoPkg/Include/Library/BaseCryptLib.h
+++ b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -3135,7 +3135,7 @@ RandomBytes (
@param[in] SaltSize Salt size in bytes.
@param[in] Info Pointer to the application specific info.
@param[in] InfoSize Info size in bytes.
- @param[Out] Out Pointer to buffer to receive hkdf value.
+ @param[out] Out Pointer to buffer to receive hkdf value.
@param[in] OutSize Size of hkdf bytes to generate.

@retval TRUE Hkdf generated successfully.
--
2.18.0.windows.1


Re: [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

Liming Gao
 

Laszlo:

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Saturday, August 17, 2019 2:54 AM
To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney,
Michael D <michael.d.kinney@intel.com>
Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>
Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing
MAT update

On 08/16/19 17:24, Gao, Liming wrote:
Laszlo:

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Laszlo Ersek
Sent: Friday, August 16, 2019 11:18 PM
To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney,
Michael D <michael.d.kinney@intel.com>
Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>
Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for
missing MAT update

On 08/14/19 17:55, Gao, Liming wrote:

If Platform PEIM doesn't build HOB, DxeIpl will not build HOB,
My reading of the code is the opposite. If the platform PEIM does not
build the HOB, then the DXE IPL PEIM will attempt to build the HOB,
from the UEFI variable.

At commit caa7d3a896f6, in file
"MdeModulePkg/Core/DxeIplPeim/DxeLoad.c", function DxeLoadCore(),
we
have:

363 if (GetFirstGuidHob ((CONST EFI_GUID
*)&gEfiMemoryTypeInformationGuid) == NULL) {
364 //
365 // Don't build GuidHob if GuidHob has been installed.
366 //
367 Status = PeiServicesLocatePpi (
368 &gEfiPeiReadOnlyVariable2PpiGuid,
369 0,
370 NULL,
371 (VOID **)&Variable
372 );
373 if (!EFI_ERROR (Status)) {
374 DataSize = sizeof (MemoryData);
375 Status = Variable->GetVariable (
376 Variable,
377 EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
378 &gEfiMemoryTypeInformationGuid,
379 NULL,
380 &DataSize,
381 &MemoryData
382 );
383 if (!EFI_ERROR (Status) &&
ValidateMemoryTypeInfoVariable(MemoryData, DataSize)) {

Only when this variable exists, Hob will be built. But, if no PEIM
builds Hob, BDS will not set the variable.
So, there is still no HOB.
So how is a platform supposed to enable this feature?

If PlatformPei never builds the HOB, the variable will never be created,
so the DXE IPL PEIM will also not build the HOB, ever.

If PlatformPei builds the HOB with static data, then BDS will set
(update) the variable, yes, but the DXE IPL PEIM will ignore the
variable, because PlatformPei already built the HOB.

So... Is PlatformPei supposed to use the Variable PPI, check if the
variable exists, and create the static HOB only if the variable is
absent?

... Ugh, wait. I've actually implemented this for OVMF almost 2 years
ago! And the answer to my question is "yes":

https://bugzilla.tianocore.org/show_bug.cgi?id=386
https://lists.01.org/pipermail/edk2-devel/2017-November/018312.html

See the OnReadOnlyVariable2Available() function:

+ //
+ // Check if the "MemoryTypeInformation" variable exists, in the
+ // gEfiMemoryTypeInformationGuid namespace.
+ //
+ ReadOnlyVariable2 = Ppi;
+ DataSize = 0;
+ Status = ReadOnlyVariable2->GetVariable (
+ ReadOnlyVariable2,
+ EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
+ &gEfiMemoryTypeInformationGuid,
+ NULL,
+ &DataSize,
+ NULL
+ );
+ if (Status == EFI_BUFFER_TOO_SMALL) {
+ //
+ // The variable exists; the DXE IPL PEIM will build the HOB from it.
+ //
+ return EFI_SUCCESS;
+ }
+ //
+ // Install the default memory type information HOB.
+ //
+ BuildMemTypeInfoHob ();

Apologies for forgetting about this; you are right.

Too bad my work for TianoCore#386 was rejected. :(
So, this is one value to add PEI variable support in OVMF.
Do you consider to approve this feature request?

Thanks
Liming

Thanks
Laszlo


Re: CPU hotplug using SMM with QEMU+OVMF

Yao, Jiewen
 

in real world, we deprecate AB-seg usage because they are vulnerable to smm cache poison attack.
I assume cache poison is out of scope in the virtual world, or there is a way to prevent ABseg cache poison.

thank you!
Yao, Jiewen

在 2019年8月19日,上午3:50,Paolo Bonzini <pbonzini@redhat.com> 写道:

On 17/08/19 02:20, Yao, Jiewen wrote:
[Jiewen] That is OK. Then we MUST add the third adversary.
-- Adversary: Simple hardware attacker, who can use device to perform DMA attack in the virtual world.
NOTE: The DMA attack in the real world is out of scope. That is be handled by IOMMU in the real world, such as VTd. -- Please do clarify if this is TRUE.

In the real world:
#1: the SMM MUST be non-DMA capable region.
#2: the MMIO MUST be non-DMA capable region.
#3: the stolen memory MIGHT be DMA capable region or non-DMA capable
region. It depends upon the silicon design.
#4: the normal OS accessible memory - including ACPI reclaim, ACPI
NVS, and reserved memory not included by #3 - MUST be DMA capable region.
As such, IOMMU protection is NOT required for #1 and #2. IOMMU
protection MIGHT be required for #3 and MUST be required for #4.
I assume the virtual environment is designed in the same way. Please
correct me if I am wrong.
Correct. The 0x30000...0x3ffff area is the only problematic one;
Igor's idea (or a variant, for example optionally remapping
0xa0000..0xaffff SMRAM to 0x30000) is becoming more and more attractive.

Paolo


Re: [PATCH] MdePkg: Add MmAccess and MmControl definition.

Ni, Ray
 

Marc,
Thanks for your contribution! It's a big step already to have the header files in MdePkg.

Thanks,
Ray

-----Original Message-----
From: Ni, Ray
Sent: Sunday, August 18, 2019 1:20 PM
To: Chen, Marc W <marc.w.chen@intel.com>; Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io;
lersek@redhat.com
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and MmControl definition.

I don't have strong preference on removing SmmAccess.h from MdeModulePkg.
If people except me all agreed to keep that header file for compatibility, I am ok.

-----Original Message-----
From: Chen, Marc W
Sent: Wednesday, August 14, 2019 5:54 PM
To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; lersek@redhat.com; Ni, Ray <ray.ni@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and MmControl definition.

Thanks Liming for pushing the code.

Can we have a conclusion for the SmmAccess.h removal from MdeModulePkg? Who can make the decision? Or do we need
any
further discussion?

Thanks,
Marc

-----Original Message-----
From: Gao, Liming
Sent: Tuesday, August 13, 2019 5:20 PM
To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Chen, Marc
W <marc.w.chen@intel.com>; lersek@redhat.com; Ni, Ray
<ray.ni@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and MmControl
definition.

Push @6f33f7a262314af35e2b99c849e08928ea49aa55

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Liming Gao
Sent: Saturday, August 10, 2019 11:34 AM
To: Chen, Marc W <marc.w.chen@intel.com>; devel@edk2.groups.io;
lersek@redhat.com; Ni, Ray <ray.ni@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH] MdePkg: Add MmAccess and MmControl
definition.

I agree. The patch to add them in MdePkg is clear. I will push it next Monday.

Thanks
Liming
-----Original Message-----
From: Chen, Marc W
Sent: Saturday, August 10, 2019 11:33 AM
To: devel@edk2.groups.io; lersek@redhat.com; Ni, Ray
<ray.ni@intel.com>;
Gao, Liming <liming.gao@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

Hi Liming

Since the SmmAccess.h of MdeModulePkg removal is still under discussion,
can we push the MmAccess.h and MmControl.h to MdePkg
first? I have other tasks pending by this.
We can keep discussing the SmmAccess after push the code.

Thanks,
Marc

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Laszlo
Ersek
Sent: Thursday, August 8, 2019 12:11 AM
To: Ni, Ray <ray.ni@intel.com>; Chen, Marc W
<marc.w.chen@intel.com>;
devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

On 08/06/19 12:08, Ni, Ray wrote:
Laszlo,
Do you want to avoid touching the OvmfPkg due the file removal in
MdeModulePkg?

My main problem with this *specific* update proposal is that the
identifiers (type names and such) that would be subject to removal were
publicly specified in earlier PI spec releases.

I don't suggest that we maintain two parallel sets of typedefs and such
in edk2. It's fine to keep the new "MM" nomenclature the main one. But
we should keep the "SMM" set of terms too, as a thin wrapper, if that's
technically feasible.

If we remove "SMM", that will not only affect OvmfPkg unpleasantly, but
a bunch of other, out-of-tree code. And the reason I suddenly care
about
out-of-tree code is that such code was written against a public industry
spec (= earlier versions of the PI spec).

I don't think that the simplicity that would come from removing "SMM"
altogether, is well-balanced against the pain that it would cause for
platforms.

Earlier this was solved in the following commits:
- 07c6a47e70ba ("MdePkg: Add new definitions for Management
Mode.",
2017-08-29)
- 2f208e59e4b9 ("MdePkg: Reference new definitions for Management
Mode.", 2017-08-29)

Thanks
Laszlo

I prefer to remove the files in MdeModulePkg to avoid future
confusion.

Thanks,
Ray

-----Original Message-----
From: Chen, Marc W
Sent: Tuesday, August 6, 2019 4:57 PM
To: devel@edk2.groups.io; lersek@redhat.com; Ni, Ray
<ray.ni@intel.com>;
Gao, Liming <liming.gao@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

Hi Liming and Ray

Do you also agree?

Thanks,
Marc

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Laszlo
Ersek
Sent: Friday, August 2, 2019 10:14 AM
To: devel@edk2.groups.io; Chen, Marc W
<marc.w.chen@intel.com>;
Ni,
Ray <ray.ni@intel.com>; Gao, Liming <liming.gao@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

On 08/01/19 12:15, Marc W Chen wrote:
Yes, my purpose is to avoid platform code update if the package is
allowed
to use MdeModulePkg like OvmfPkg.
For those packages that cannot depend on MdeModulePkg must
change
to
use new definition.

Agreed.

Thanks!
Laszlo

-----Original Message-----
From: Ni, Ray
Sent: Thursday, August 1, 2019 6:04 PM
To: Chen, Marc W <marc.w.chen@intel.com>; Gao, Liming
<liming.gao@intel.com>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

Marc,
Is the purpose to avoid platform code update with the wrapper
header
file in
MdeModulePkg?
Certain platforms they may require to not depend on
MdeModulePkg
but just depend on MdePkg.
Having SmmAccess.h in MdeModulePkg doesn't help.

It also bring confusing.

Thanks,
Ray

-----Original Message-----
From: Chen, Marc W
Sent: Thursday, August 1, 2019 4:53 PM
To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray
<ray.ni@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

Hi Liming

Another thought, do you think it is ok to keep SmmAccess.h in
MdeModulePkg? We can use #define and typedef to convert
MmAccess
to
SmmAccess, just like current SmmAccess Protocol in MdePkg.

Thanks,
Marc

-----Original Message-----
From: Chen, Marc W
Sent: Thursday, August 1, 2019 4:48 PM
To: Gao, Liming <liming.gao@intel.com>;
devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray
<ray.ni@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

Hi Liming

Since there are multiple packages are still using SmmAccess.h,
we
need to change all packages to use MmAccess.h instead of
SmmAccess.h first, then clean up SmmAccess.h from
MdeModulePkg
will be the last step.
Below are the packages that has "include <Ppi/SmmAccess.h>"
for
your reference.
* Edk2 repo:
o MdeModulePkg
o OvmfPkg
o UefiCpuPkg
* Edk2Platform repo:
o MinPlatformPkg
o QuarkSocPkg

Thanks,
Marc

-----Original Message-----
From: Gao, Liming
Sent: Thursday, August 1, 2019 4:42 PM
To: devel@edk2.groups.io; Chen, Marc W
<marc.w.chen@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray
<ray.ni@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess
and
MmControl
definition.

Marc:
The change is good. Reviewed-by: Liming Gao
<liming.gao@intel.com>

Besides, I see BZ also mention to remove the one in
MdeModulePkg.
Have you the following patches for the change in
MdeModulePkg?

Thanks
Liming
-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
On
Behalf
Of Marc W Chen
Sent: Monday, July 29, 2019 12:26 PM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao,
Liming
<liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

EFI MmAccess and MmControl PPIs are defined in the PI 1.5
specification.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2023
Signed-off-by: Marc W Chen <marc.w.chen@intel.com>
---
MdePkg/Include/Ppi/MmAccess.h | 155
+++++++++++++++++++++++++++++++++++++++++
MdePkg/Include/Ppi/MmControl.h | 90
++++++++++++++++++++++++
MdePkg/MdePkg.dec | 6 ++
3 files changed, 251 insertions(+) create mode 100644
MdePkg/Include/Ppi/MmAccess.h create mode 100644
MdePkg/Include/Ppi/MmControl.h

diff --git a/MdePkg/Include/Ppi/MmAccess.h
b/MdePkg/Include/Ppi/MmAccess.h new file mode 100644
index
0000000000..636e7288a0
--- /dev/null
+++ b/MdePkg/Include/Ppi/MmAccess.h
@@ -0,0 +1,155 @@
+/** @file
+ EFI MM Access PPI definition.
+
+ This PPI is used to control the visibility of the MMRAM on
+ the
platform.
+ The EFI_PEI_MM_ACCESS_PPI abstracts the location and
+ characteristics
of
MMRAM. The
+ principal functionality found in the memory controller
+ includes the
following:
+ - Exposing the MMRAM to all non-MM agents, or the
"open"
+ state
+ - Shrouding the MMRAM to all but the MM agents, or the
"closed"
+ state
+ - Preserving the system integrity, or "locking" the MMRAM,
+ such that
the
settings cannot be
+ perturbed by either boot service or runtime agents
+
+ Copyright (c) 2019, Intel Corporation. All rights
+ reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+ @par Revision Reference:
+ This PPI is introduced in PI Version 1.5.
+
+**/
+
+#ifndef _MM_ACCESS_PPI_H_
+#define _MM_ACCESS_PPI_H_
+
+#define EFI_PEI_MM_ACCESS_PPI_GUID \
+ { 0x268f33a9, 0xcccd, 0x48be, { 0x88, 0x17, 0x86, 0x5, 0x3a,
+0xc3, 0x2e,
0xd6 }}
+
+typedef struct _EFI_PEI_MM_ACCESS_PPI
EFI_PEI_MM_ACCESS_PPI;
+
+/**
+ Opens the MMRAM area to be accessible by a PEIM.
+
+ This function "opens" MMRAM so that it is visible while
not
+ inside of
MM.
The function should
+ return EFI_UNSUPPORTED if the hardware does not
support
+ hiding of
MMRAM. The function
+ should return EFI_DEVICE_ERROR if the MMRAM
configuration
is
locked.
+
+ @param PeiServices An indirect pointer to the PEI
Services
Table
published by the PEI Foundation.
+ @param This The EFI_PEI_MM_ACCESS_PPI
instance.
+ @param DescriptorIndex The region of MMRAM to
Open.
+
+ @retval EFI_SUCCESS The operation was successful.
+ @retval EFI_UNSUPPORTED The system does not
support
opening
and
closing of MMRAM.
+ @retval EFI_DEVICE_ERROR MMRAM cannot be
opened,
perhaps
because it is locked.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_PEI_MM_OPEN)(
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_MM_ACCESS_PPI *This,
+ IN UINTN DescriptorIndex
+ );
+
+/**
+ Inhibits access to the MMRAM.
+
+ This function "closes" MMRAM so that it is not visible
while
+ outside of
MM.
The function should
+ return EFI_UNSUPPORTED if the hardware does not
support
+ hiding of
MMRAM.
+
+ @param PeiServices An indirect pointer to the PEI
Services
Table
published by the PEI Foundation.
+ @param This The EFI_PEI_MM_ACCESS_PPI
instance.
+ @param DescriptorIndex The region of MMRAM to
Close.
+
+ @retval EFI_SUCCESS The operation was successful.
+ @retval EFI_UNSUPPORTED The system does not
support
opening
and
closing of MMRAM.
+ @retval EFI_DEVICE_ERROR MMRAM cannot be closed.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_PEI_MM_CLOSE)(
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_MM_ACCESS_PPI *This,
+ IN UINTN DescriptorIndex
+ );
+
+/**
+ This function prohibits access to the MMRAM region. This
+function is
usually
implemented such
+ that it is a write-once operation.
+
+ @param PeiServices An indirect pointer to the PEI
Services
Table
published by the PEI Foundation.
+ @param This The EFI_PEI_MM_ACCESS_PPI
instance.
+ @param DescriptorIndex The region of MMRAM to
Lock.
+
+ @retval EFI_SUCCESS The operation was successful.
+ @retval EFI_UNSUPPORTED The system does not
support
opening
and
closing of MMRAM.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_PEI_MM_LOCK)(
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_MM_ACCESS_PPI *This,
+ IN UINTN DescriptorIndex
+ );
+
+/**
+ Queries the memory controller for the possible regions
that
+will support
MMRAM.
+
+ This function describes the MMRAM regions.
+ This data structure forms the contract between the
MM_ACCESS
and
MM_IPL drivers. There is an
+ ambiguity when any MMRAM region is remapped. For
example,
on
some
chipsets, some MMRAM
+ regions can be initialized at one physical address but is
+ later accessed at
another processor address.
+ There is currently no way for the MM IPL driver to know
that
+ it must use
two different addresses
+ depending on what it is trying to do. As a result, initial
+ configuration and
loading can use the
+ physical address PhysicalStart while MMRAM is open.
However,
+ once
the
region has been
+ closed and needs to be accessed by agents in MM, the
+ CpuStart address
must be used.
+ This PPI publishes the available memory that the chipset
can
+ shroud for
the
use of installing code.
+ These regions serve the dual purpose of describing which
+ regions have
been open, closed, or locked.
+ In addition, these regions may include overlapping
memory
+ ranges,
depending on the chipset
+ implementation. The latter might include a chipset that
+ supports T-SEG,
where memory near the top
+ of the physical DRAM can be allocated for MMRAM too.
+ The key thing to note is that the regions that are described
+ by the PPI
are
a
subset of the capabilities
+ of the hardware.
+
+ @param PeiServices An indirect pointer to the PEI
Services
Table
published by the PEI Foundation.
+ @param This The EFI_PEI_MM_ACCESS_PPI
instance.
+ @param MmramMapSize A pointer to the size, in
bytes,
of
the
MmramMemoryMap buffer. On input, this value is
+ the size of the buffer that
+ is allocated by the caller. On
output, it is the size of the
+ buffer that was returned by
+ the firmware if the buffer
was
large enough, or, if the
+ buffer was too small, the
+ size of the buffer that is
needed to
contain the map.
+ @param MmramMap A pointer to the buffer in
which
firmware
places the current memory map. The map is
+ an array of
+ EFI_MMRAM_DESCRIPTORs
+
+ @retval EFI_SUCCESS The chipset supported the
given
resource.
+ @retval EFI_BUFFER_TOO_SMALL The MmramMap
parameter
was
too
small. The current
+ buffer size needed to hold
+ the memory map is
returned
in
+ MmramMapSize.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_PEI_MM_CAPABILITIES)(
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_MM_ACCESS_PPI *This,
+ IN OUT UINTN *MmramMapSize,
+ IN OUT EFI_MMRAM_DESCRIPTOR *MmramMap
+ );
+
+///
+/// EFI MM Access PPI is used to control the visibility of
+the MMRAM on
the
platform.
+/// It abstracts the location and characteristics of MMRAM.
+The platform
should report
+/// all MMRAM via EFI_PEI_MM_ACCESS_PPI. The
expectation
is
that
+the
north bridge or
+/// memory controller would publish this PPI.
+///
+struct _EFI_PEI_MM_ACCESS_PPI {
+ EFI_PEI_MM_OPEN Open;
+ EFI_PEI_MM_CLOSE Close;
+ EFI_PEI_MM_LOCK Lock;
+ EFI_PEI_MM_CAPABILITIES GetCapabilities;
+ BOOLEAN LockState;
+ BOOLEAN OpenState;
+};
+
+extern EFI_GUID gEfiPeiMmAccessPpiGuid;
+
+#endif
diff --git a/MdePkg/Include/Ppi/MmControl.h
b/MdePkg/Include/Ppi/MmControl.h new file mode 100644
index
0000000000..983ed95cd5
--- /dev/null
+++ b/MdePkg/Include/Ppi/MmControl.h
@@ -0,0 +1,90 @@
+/** @file
+ EFI MM Control PPI definition.
+
+ This PPI is used initiate synchronous MMI activations. This
+ PPI could be
published by a processor
+ driver to abstract the MMI IPI or a driver which abstracts
+ the ASIC that
is
supporting the APM port.
+ Because of the possibility of performing MMI IPI
+ transactions, the ability
to
generate this event
+ from a platform chipset agent is an optional capability for
+ both
+ IA-32
and
x64-based systems.
+
+ Copyright (c) 2019, Intel Corporation. All rights
+ reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+ @par Revision Reference:
+ This PPI is introduced in PI Version 1.5.
+
+**/
+
+
+#ifndef _MM_CONTROL_PPI_H_
+#define _MM_CONTROL_PPI_H_
+
+#define EFI_PEI_MM_CONTROL_PPI_GUID \
+ { 0x61c68702, 0x4d7e, 0x4f43, 0x8d, 0xef, 0xa7, 0x43, 0x5,
+0xce, 0x74,
0xc5 }
+
+typedef struct _EFI_PEI_MM_CONTROL_PPI
EFI_PEI_MM_CONTROL_PPI;
+
+/**
+ Invokes PPI activation from the PI PEI environment.
+
+ @param PeiServices An indirect pointer to the PEI
Services
Table
published by the PEI Foundation.
+ @param This The PEI_MM_CONTROL_PPI
instance.
+ @param ArgumentBuffer The value passed to the
MMI
handler.
This
value corresponds to the
+ SwMmiInputValue in the
+ RegisterContext parameter
for
the
Register()
+ function in the
+ EFI_MM_SW_DISPATCH_PROTOCOL
and
in
the Context parameter
+ in the call to the DispatchFunction
+ @param ArgumentBufferSize The size of the data
passed
in
ArgumentBuffer or NULL if ArgumentBuffer is NULL.
+ @param Periodic An optional mechanism to
periodically
repeat
activation.
+ @param ActivationInterval An optional parameter to
repeat
at
this
period
one
+ time or, if the Periodic Boolean is set,
periodically.
+
+ @retval EFI_SUCCESS The MMI has been engendered.
+ @retval EFI_DEVICE_ERROR The timing is unsupported.
+ @retval EFI_INVALID_PARAMETER The activation period is
unsupported.
+ @retval EFI_NOT_STARTED The MM base service has
not
been
initialized.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_PEI_MM_ACTIVATE) (
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_MM_CONTROL_PPI * This,
+ IN OUT INT8 *ArgumentBuffer
OPTIONAL,
+ IN OUT UINTN *ArgumentBufferSize
OPTIONAL,
+ IN BOOLEAN Periodic OPTIONAL,
+ IN UINTN ActivationInterval
OPTIONAL
+ );
+
+/**
+ Clears any system state that was created in response to
the
Trigger()
call.
+
+ @param PeiServices General purpose services
available
to
every
PEIM.
+ @param This The PEI_MM_CONTROL_PPI
instance.
+ @param Periodic Optional parameter to repeat at
this
period
one
+ time or, if the Periodic Boolean is set,
periodically.
+
+ @retval EFI_SUCCESS The MMI has been engendered.
+ @retval EFI_DEVICE_ERROR The source could not be
cleared.
+ @retval EFI_INVALID_PARAMETER The service did not
support
+ the
Periodic
input argument.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *PEI_MM_DEACTIVATE) (
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_MM_CONTROL_PPI * This,
+ IN BOOLEAN Periodic OPTIONAL
+ );
+
+///
+/// The EFI_PEI_MM_CONTROL_PPI is produced by a PEIM.
It
provides
an
abstraction of the
+/// platform hardware that generates an MMI. There are
often
+I/O ports
that, when accessed, will
+/// generate the MMI. Also, the hardware optionally
supports
+the
periodic
generation of these signals.
+///
+struct _PEI_MM_CONTROL_PPI {
+ PEI_MM_ACTIVATE Trigger;
+ PEI_MM_DEACTIVATE Clear;
+};
+
+extern EFI_GUID gEfiPeiMmControlPpiGuid;
+
+#endif
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index
b382efd578..34e0f39395 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -925,6 +925,12 @@
## Include/Ppi/SecHobData.h
gEfiSecHobDataPpiGuid = { 0x3ebdaf20, 0x6667, 0x40d8,
{0xb4,
0xee,
0xf5,
0x99, 0x9a, 0xc1, 0xb7, 0x1f } }

+ ## Include/Ppi/MmAccess.h
+ gEfiPeiMmAccessPpiGuid = { 0x268f33a9, 0xcccd,
0x48be,
{ 0x88,
0x17,
0x86, 0x5, 0x3a, 0xc3, 0x2e, 0xd6 }}
+
+ ## Include/Ppi/MmControl.h
+ gEfiPeiMmControlPpiGuid = { 0x61c68702, 0x4d7e,
0x4f43,
{ 0x8d,
0xef,
0xa7, 0x43, 0x5, 0xce, 0x74, 0xc5 }}
+
#
# PPIs defined in PI 1.7.
#
--
2.16.2.windows.1








Re: [PATCH] MdePkg: Add MmAccess and MmControl definition.

Ni, Ray
 

I don't have strong preference on removing SmmAccess.h from MdeModulePkg.
If people except me all agreed to keep that header file for compatibility, I am ok.

-----Original Message-----
From: Chen, Marc W
Sent: Wednesday, August 14, 2019 5:54 PM
To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; lersek@redhat.com; Ni, Ray <ray.ni@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and MmControl definition.

Thanks Liming for pushing the code.

Can we have a conclusion for the SmmAccess.h removal from MdeModulePkg? Who can make the decision? Or do we need any
further discussion?

Thanks,
Marc

-----Original Message-----
From: Gao, Liming
Sent: Tuesday, August 13, 2019 5:20 PM
To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Chen, Marc
W <marc.w.chen@intel.com>; lersek@redhat.com; Ni, Ray
<ray.ni@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and MmControl
definition.

Push @6f33f7a262314af35e2b99c849e08928ea49aa55

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Liming Gao
Sent: Saturday, August 10, 2019 11:34 AM
To: Chen, Marc W <marc.w.chen@intel.com>; devel@edk2.groups.io;
lersek@redhat.com; Ni, Ray <ray.ni@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH] MdePkg: Add MmAccess and MmControl
definition.

I agree. The patch to add them in MdePkg is clear. I will push it next Monday.

Thanks
Liming
-----Original Message-----
From: Chen, Marc W
Sent: Saturday, August 10, 2019 11:33 AM
To: devel@edk2.groups.io; lersek@redhat.com; Ni, Ray
<ray.ni@intel.com>;
Gao, Liming <liming.gao@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

Hi Liming

Since the SmmAccess.h of MdeModulePkg removal is still under discussion,
can we push the MmAccess.h and MmControl.h to MdePkg
first? I have other tasks pending by this.
We can keep discussing the SmmAccess after push the code.

Thanks,
Marc

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Laszlo
Ersek
Sent: Thursday, August 8, 2019 12:11 AM
To: Ni, Ray <ray.ni@intel.com>; Chen, Marc W
<marc.w.chen@intel.com>;
devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

On 08/06/19 12:08, Ni, Ray wrote:
Laszlo,
Do you want to avoid touching the OvmfPkg due the file removal in
MdeModulePkg?

My main problem with this *specific* update proposal is that the
identifiers (type names and such) that would be subject to removal were
publicly specified in earlier PI spec releases.

I don't suggest that we maintain two parallel sets of typedefs and such
in edk2. It's fine to keep the new "MM" nomenclature the main one. But
we should keep the "SMM" set of terms too, as a thin wrapper, if that's
technically feasible.

If we remove "SMM", that will not only affect OvmfPkg unpleasantly, but
a bunch of other, out-of-tree code. And the reason I suddenly care
about
out-of-tree code is that such code was written against a public industry
spec (= earlier versions of the PI spec).

I don't think that the simplicity that would come from removing "SMM"
altogether, is well-balanced against the pain that it would cause for
platforms.

Earlier this was solved in the following commits:
- 07c6a47e70ba ("MdePkg: Add new definitions for Management
Mode.",
2017-08-29)
- 2f208e59e4b9 ("MdePkg: Reference new definitions for Management
Mode.", 2017-08-29)

Thanks
Laszlo

I prefer to remove the files in MdeModulePkg to avoid future
confusion.

Thanks,
Ray

-----Original Message-----
From: Chen, Marc W
Sent: Tuesday, August 6, 2019 4:57 PM
To: devel@edk2.groups.io; lersek@redhat.com; Ni, Ray
<ray.ni@intel.com>;
Gao, Liming <liming.gao@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

Hi Liming and Ray

Do you also agree?

Thanks,
Marc

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Laszlo
Ersek
Sent: Friday, August 2, 2019 10:14 AM
To: devel@edk2.groups.io; Chen, Marc W
<marc.w.chen@intel.com>;
Ni,
Ray <ray.ni@intel.com>; Gao, Liming <liming.gao@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

On 08/01/19 12:15, Marc W Chen wrote:
Yes, my purpose is to avoid platform code update if the package is
allowed
to use MdeModulePkg like OvmfPkg.
For those packages that cannot depend on MdeModulePkg must
change
to
use new definition.

Agreed.

Thanks!
Laszlo

-----Original Message-----
From: Ni, Ray
Sent: Thursday, August 1, 2019 6:04 PM
To: Chen, Marc W <marc.w.chen@intel.com>; Gao, Liming
<liming.gao@intel.com>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

Marc,
Is the purpose to avoid platform code update with the wrapper
header
file in
MdeModulePkg?
Certain platforms they may require to not depend on
MdeModulePkg
but just depend on MdePkg.
Having SmmAccess.h in MdeModulePkg doesn't help.

It also bring confusing.

Thanks,
Ray

-----Original Message-----
From: Chen, Marc W
Sent: Thursday, August 1, 2019 4:53 PM
To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray
<ray.ni@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

Hi Liming

Another thought, do you think it is ok to keep SmmAccess.h in
MdeModulePkg? We can use #define and typedef to convert
MmAccess
to
SmmAccess, just like current SmmAccess Protocol in MdePkg.

Thanks,
Marc

-----Original Message-----
From: Chen, Marc W
Sent: Thursday, August 1, 2019 4:48 PM
To: Gao, Liming <liming.gao@intel.com>;
devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray
<ray.ni@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

Hi Liming

Since there are multiple packages are still using SmmAccess.h,
we
need to change all packages to use MmAccess.h instead of
SmmAccess.h first, then clean up SmmAccess.h from
MdeModulePkg
will be the last step.
Below are the packages that has "include <Ppi/SmmAccess.h>"
for
your reference.
* Edk2 repo:
o MdeModulePkg
o OvmfPkg
o UefiCpuPkg
* Edk2Platform repo:
o MinPlatformPkg
o QuarkSocPkg

Thanks,
Marc

-----Original Message-----
From: Gao, Liming
Sent: Thursday, August 1, 2019 4:42 PM
To: devel@edk2.groups.io; Chen, Marc W
<marc.w.chen@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray
<ray.ni@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdePkg: Add MmAccess
and
MmControl
definition.

Marc:
The change is good. Reviewed-by: Liming Gao
<liming.gao@intel.com>

Besides, I see BZ also mention to remove the one in
MdeModulePkg.
Have you the following patches for the change in
MdeModulePkg?

Thanks
Liming
-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
On
Behalf
Of Marc W Chen
Sent: Monday, July 29, 2019 12:26 PM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao,
Liming
<liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: [edk2-devel] [PATCH] MdePkg: Add MmAccess and
MmControl
definition.

EFI MmAccess and MmControl PPIs are defined in the PI 1.5
specification.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2023
Signed-off-by: Marc W Chen <marc.w.chen@intel.com>
---
MdePkg/Include/Ppi/MmAccess.h | 155
+++++++++++++++++++++++++++++++++++++++++
MdePkg/Include/Ppi/MmControl.h | 90
++++++++++++++++++++++++
MdePkg/MdePkg.dec | 6 ++
3 files changed, 251 insertions(+) create mode 100644
MdePkg/Include/Ppi/MmAccess.h create mode 100644
MdePkg/Include/Ppi/MmControl.h

diff --git a/MdePkg/Include/Ppi/MmAccess.h
b/MdePkg/Include/Ppi/MmAccess.h new file mode 100644
index
0000000000..636e7288a0
--- /dev/null
+++ b/MdePkg/Include/Ppi/MmAccess.h
@@ -0,0 +1,155 @@
+/** @file
+ EFI MM Access PPI definition.
+
+ This PPI is used to control the visibility of the MMRAM on
+ the
platform.
+ The EFI_PEI_MM_ACCESS_PPI abstracts the location and
+ characteristics
of
MMRAM. The
+ principal functionality found in the memory controller
+ includes the
following:
+ - Exposing the MMRAM to all non-MM agents, or the
"open"
+ state
+ - Shrouding the MMRAM to all but the MM agents, or the
"closed"
+ state
+ - Preserving the system integrity, or "locking" the MMRAM,
+ such that
the
settings cannot be
+ perturbed by either boot service or runtime agents
+
+ Copyright (c) 2019, Intel Corporation. All rights
+ reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+ @par Revision Reference:
+ This PPI is introduced in PI Version 1.5.
+
+**/
+
+#ifndef _MM_ACCESS_PPI_H_
+#define _MM_ACCESS_PPI_H_
+
+#define EFI_PEI_MM_ACCESS_PPI_GUID \
+ { 0x268f33a9, 0xcccd, 0x48be, { 0x88, 0x17, 0x86, 0x5, 0x3a,
+0xc3, 0x2e,
0xd6 }}
+
+typedef struct _EFI_PEI_MM_ACCESS_PPI
EFI_PEI_MM_ACCESS_PPI;
+
+/**
+ Opens the MMRAM area to be accessible by a PEIM.
+
+ This function "opens" MMRAM so that it is visible while
not
+ inside of
MM.
The function should
+ return EFI_UNSUPPORTED if the hardware does not
support
+ hiding of
MMRAM. The function
+ should return EFI_DEVICE_ERROR if the MMRAM
configuration
is
locked.
+
+ @param PeiServices An indirect pointer to the PEI
Services
Table
published by the PEI Foundation.
+ @param This The EFI_PEI_MM_ACCESS_PPI
instance.
+ @param DescriptorIndex The region of MMRAM to
Open.
+
+ @retval EFI_SUCCESS The operation was successful.
+ @retval EFI_UNSUPPORTED The system does not
support
opening
and
closing of MMRAM.
+ @retval EFI_DEVICE_ERROR MMRAM cannot be
opened,
perhaps
because it is locked.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_PEI_MM_OPEN)(
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_MM_ACCESS_PPI *This,
+ IN UINTN DescriptorIndex
+ );
+
+/**
+ Inhibits access to the MMRAM.
+
+ This function "closes" MMRAM so that it is not visible
while
+ outside of
MM.
The function should
+ return EFI_UNSUPPORTED if the hardware does not
support
+ hiding of
MMRAM.
+
+ @param PeiServices An indirect pointer to the PEI
Services
Table
published by the PEI Foundation.
+ @param This The EFI_PEI_MM_ACCESS_PPI
instance.
+ @param DescriptorIndex The region of MMRAM to
Close.
+
+ @retval EFI_SUCCESS The operation was successful.
+ @retval EFI_UNSUPPORTED The system does not
support
opening
and
closing of MMRAM.
+ @retval EFI_DEVICE_ERROR MMRAM cannot be closed.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_PEI_MM_CLOSE)(
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_MM_ACCESS_PPI *This,
+ IN UINTN DescriptorIndex
+ );
+
+/**
+ This function prohibits access to the MMRAM region. This
+function is
usually
implemented such
+ that it is a write-once operation.
+
+ @param PeiServices An indirect pointer to the PEI
Services
Table
published by the PEI Foundation.
+ @param This The EFI_PEI_MM_ACCESS_PPI
instance.
+ @param DescriptorIndex The region of MMRAM to
Lock.
+
+ @retval EFI_SUCCESS The operation was successful.
+ @retval EFI_UNSUPPORTED The system does not
support
opening
and
closing of MMRAM.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_PEI_MM_LOCK)(
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_MM_ACCESS_PPI *This,
+ IN UINTN DescriptorIndex
+ );
+
+/**
+ Queries the memory controller for the possible regions
that
+will support
MMRAM.
+
+ This function describes the MMRAM regions.
+ This data structure forms the contract between the
MM_ACCESS
and
MM_IPL drivers. There is an
+ ambiguity when any MMRAM region is remapped. For
example,
on
some
chipsets, some MMRAM
+ regions can be initialized at one physical address but is
+ later accessed at
another processor address.
+ There is currently no way for the MM IPL driver to know
that
+ it must use
two different addresses
+ depending on what it is trying to do. As a result, initial
+ configuration and
loading can use the
+ physical address PhysicalStart while MMRAM is open.
However,
+ once
the
region has been
+ closed and needs to be accessed by agents in MM, the
+ CpuStart address
must be used.
+ This PPI publishes the available memory that the chipset
can
+ shroud for
the
use of installing code.
+ These regions serve the dual purpose of describing which
+ regions have
been open, closed, or locked.
+ In addition, these regions may include overlapping
memory
+ ranges,
depending on the chipset
+ implementation. The latter might include a chipset that
+ supports T-SEG,
where memory near the top
+ of the physical DRAM can be allocated for MMRAM too.
+ The key thing to note is that the regions that are described
+ by the PPI
are
a
subset of the capabilities
+ of the hardware.
+
+ @param PeiServices An indirect pointer to the PEI
Services
Table
published by the PEI Foundation.
+ @param This The EFI_PEI_MM_ACCESS_PPI
instance.
+ @param MmramMapSize A pointer to the size, in
bytes,
of
the
MmramMemoryMap buffer. On input, this value is
+ the size of the buffer that
+ is allocated by the caller. On
output, it is the size of the
+ buffer that was returned by
+ the firmware if the buffer
was
large enough, or, if the
+ buffer was too small, the
+ size of the buffer that is
needed to
contain the map.
+ @param MmramMap A pointer to the buffer in
which
firmware
places the current memory map. The map is
+ an array of
+ EFI_MMRAM_DESCRIPTORs
+
+ @retval EFI_SUCCESS The chipset supported the
given
resource.
+ @retval EFI_BUFFER_TOO_SMALL The MmramMap
parameter
was
too
small. The current
+ buffer size needed to hold
+ the memory map is
returned
in
+ MmramMapSize.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_PEI_MM_CAPABILITIES)(
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_MM_ACCESS_PPI *This,
+ IN OUT UINTN *MmramMapSize,
+ IN OUT EFI_MMRAM_DESCRIPTOR *MmramMap
+ );
+
+///
+/// EFI MM Access PPI is used to control the visibility of
+the MMRAM on
the
platform.
+/// It abstracts the location and characteristics of MMRAM.
+The platform
should report
+/// all MMRAM via EFI_PEI_MM_ACCESS_PPI. The
expectation
is
that
+the
north bridge or
+/// memory controller would publish this PPI.
+///
+struct _EFI_PEI_MM_ACCESS_PPI {
+ EFI_PEI_MM_OPEN Open;
+ EFI_PEI_MM_CLOSE Close;
+ EFI_PEI_MM_LOCK Lock;
+ EFI_PEI_MM_CAPABILITIES GetCapabilities;
+ BOOLEAN LockState;
+ BOOLEAN OpenState;
+};
+
+extern EFI_GUID gEfiPeiMmAccessPpiGuid;
+
+#endif
diff --git a/MdePkg/Include/Ppi/MmControl.h
b/MdePkg/Include/Ppi/MmControl.h new file mode 100644
index
0000000000..983ed95cd5
--- /dev/null
+++ b/MdePkg/Include/Ppi/MmControl.h
@@ -0,0 +1,90 @@
+/** @file
+ EFI MM Control PPI definition.
+
+ This PPI is used initiate synchronous MMI activations. This
+ PPI could be
published by a processor
+ driver to abstract the MMI IPI or a driver which abstracts
+ the ASIC that
is
supporting the APM port.
+ Because of the possibility of performing MMI IPI
+ transactions, the ability
to
generate this event
+ from a platform chipset agent is an optional capability for
+ both
+ IA-32
and
x64-based systems.
+
+ Copyright (c) 2019, Intel Corporation. All rights
+ reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+ @par Revision Reference:
+ This PPI is introduced in PI Version 1.5.
+
+**/
+
+
+#ifndef _MM_CONTROL_PPI_H_
+#define _MM_CONTROL_PPI_H_
+
+#define EFI_PEI_MM_CONTROL_PPI_GUID \
+ { 0x61c68702, 0x4d7e, 0x4f43, 0x8d, 0xef, 0xa7, 0x43, 0x5,
+0xce, 0x74,
0xc5 }
+
+typedef struct _EFI_PEI_MM_CONTROL_PPI
EFI_PEI_MM_CONTROL_PPI;
+
+/**
+ Invokes PPI activation from the PI PEI environment.
+
+ @param PeiServices An indirect pointer to the PEI
Services
Table
published by the PEI Foundation.
+ @param This The PEI_MM_CONTROL_PPI
instance.
+ @param ArgumentBuffer The value passed to the
MMI
handler.
This
value corresponds to the
+ SwMmiInputValue in the
+ RegisterContext parameter
for
the
Register()
+ function in the
+ EFI_MM_SW_DISPATCH_PROTOCOL
and
in
the Context parameter
+ in the call to the DispatchFunction
+ @param ArgumentBufferSize The size of the data
passed
in
ArgumentBuffer or NULL if ArgumentBuffer is NULL.
+ @param Periodic An optional mechanism to
periodically
repeat
activation.
+ @param ActivationInterval An optional parameter to
repeat
at
this
period
one
+ time or, if the Periodic Boolean is set,
periodically.
+
+ @retval EFI_SUCCESS The MMI has been engendered.
+ @retval EFI_DEVICE_ERROR The timing is unsupported.
+ @retval EFI_INVALID_PARAMETER The activation period is
unsupported.
+ @retval EFI_NOT_STARTED The MM base service has
not
been
initialized.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_PEI_MM_ACTIVATE) (
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_MM_CONTROL_PPI * This,
+ IN OUT INT8 *ArgumentBuffer
OPTIONAL,
+ IN OUT UINTN *ArgumentBufferSize
OPTIONAL,
+ IN BOOLEAN Periodic OPTIONAL,
+ IN UINTN ActivationInterval
OPTIONAL
+ );
+
+/**
+ Clears any system state that was created in response to
the
Trigger()
call.
+
+ @param PeiServices General purpose services
available
to
every
PEIM.
+ @param This The PEI_MM_CONTROL_PPI
instance.
+ @param Periodic Optional parameter to repeat at
this
period
one
+ time or, if the Periodic Boolean is set,
periodically.
+
+ @retval EFI_SUCCESS The MMI has been engendered.
+ @retval EFI_DEVICE_ERROR The source could not be
cleared.
+ @retval EFI_INVALID_PARAMETER The service did not
support
+ the
Periodic
input argument.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *PEI_MM_DEACTIVATE) (
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_MM_CONTROL_PPI * This,
+ IN BOOLEAN Periodic OPTIONAL
+ );
+
+///
+/// The EFI_PEI_MM_CONTROL_PPI is produced by a PEIM.
It
provides
an
abstraction of the
+/// platform hardware that generates an MMI. There are
often
+I/O ports
that, when accessed, will
+/// generate the MMI. Also, the hardware optionally
supports
+the
periodic
generation of these signals.
+///
+struct _PEI_MM_CONTROL_PPI {
+ PEI_MM_ACTIVATE Trigger;
+ PEI_MM_DEACTIVATE Clear;
+};
+
+extern EFI_GUID gEfiPeiMmControlPpiGuid;
+
+#endif
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index
b382efd578..34e0f39395 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -925,6 +925,12 @@
## Include/Ppi/SecHobData.h
gEfiSecHobDataPpiGuid = { 0x3ebdaf20, 0x6667, 0x40d8,
{0xb4,
0xee,
0xf5,
0x99, 0x9a, 0xc1, 0xb7, 0x1f } }

+ ## Include/Ppi/MmAccess.h
+ gEfiPeiMmAccessPpiGuid = { 0x268f33a9, 0xcccd,
0x48be,
{ 0x88,
0x17,
0x86, 0x5, 0x3a, 0xc3, 0x2e, 0xd6 }}
+
+ ## Include/Ppi/MmControl.h
+ gEfiPeiMmControlPpiGuid = { 0x61c68702, 0x4d7e,
0x4f43,
{ 0x8d,
0xef,
0xa7, 0x43, 0x5, 0xce, 0x74, 0xc5 }}
+
#
# PPIs defined in PI 1.7.
#
--
2.16.2.windows.1








Upcoming Event: TianoCore Design Meeting - APAC/NAMO - Thu, 08/22/2019 6:30pm-7:30pm #cal-reminder

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

Reminder: TianoCore Design Meeting - APAC/NAMO

When: Thursday, 22 August 2019, 6:30pm to 7:30pm, (GMT-07:00) America/Los Angeles

Where:https://zoom.us/j/969264410

View Event

Organizer: Stephano Cetola stephano.cetola@...

Description:

Join Zoom Meeting

https://zoom.us/j/969264410

 

One tap mobile

+16465588656,,969264410# US (New York)

+17207072699,,969264410# US

 

Dial by your location

        +1 646 558 8656 US (New York)

        +1 720 707 2699 US

Meeting ID: 969 264 410

Find your local number: https://zoom.us/u/abOtdJckxL