Date   

Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Fix dwc2 reset on raspberry pi boards

Samer El-Haj-Mahmoud
 

Thanks for the patch!

+ Andrei and Jeremey for review

I think this may be a side effect of https://github.com/tianocore/edk2-platforms/commit/f8958b86e8863432b815a132a0f0fe82950c6dd1

Previously, the DwHcReset() function did not check for valid Attributes passed in as an argument. So if you pass in 0, the function will still happily reset the controller. That caused UEFI SCT issues (since the function will take in garbage Attributes without checking for their validity, per UEFI spec). The change was to verify the Attributes are valid, and return EFI_UNSUPPORTED if they are not. The only valid attributes for resetting are EFI_USB_HC_RESET_GLOBAL and EFI_USB_HC_RESET_HOST_CONTROLLER.

I think your change makes sense. But I would like to run more tests.

Acked-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of René
Treffer via groups.io
Sent: Thursday, March 11, 2021 7:40 AM
To: devel@edk2.groups.io
Cc: Pete Batard <pete@akeo.ie>; Leif Lindholm <leif@nuviainc.com>; Ard
Biesheuvel <ardb+tianocore@kernel.org>
Subject: [edk2-devel] [edk2-platforms][PATCH 1/1] Platform/RaspberryPi:
Fix dwc2 reset on raspberry pi boards

DwHcReset expects attributes as the second argument. A reset is performed
if the passed attribute is valid. However 0 is not a valid attribute and will thus
never cause a controller reset.

Passing EFI_USB_HC_RESET_HOST_CONTROLLER will reset the dwc2
controller as expected.

This enables the USB 2.0 port of the raspberry compute module 4.
---
Platform/RaspberryPi/Drivers/DwUsbHostDxe/DriverBinding.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/Drivers/DwUsbHostDxe/DriverBinding.c
b/Platform/RaspberryPi/Drivers/DwUsbHostDxe/DriverBinding.c
index bada13a6cd..bb228e62d9 100644
--- a/Platform/RaspberryPi/Drivers/DwUsbHostDxe/DriverBinding.c
+++ b/Platform/RaspberryPi/Drivers/DwUsbHostDxe/DriverBinding.c
@@ -140,7 +140,7 @@ DriverStart (
* UsbBusDxe as of b4e96b82b4e2e47e95014b51787ba5b43abac784 expects
* the HCD to do this. There is no agent invoking DwHcReset anymore.
*/
- DwHcReset (&DwHc->DwUsbOtgHc, 0);
+ DwHcReset (&DwHc->DwUsbOtgHc,
EFI_USB_HC_RESET_HOST_CONTROLLER);
DwHcSetState (&DwHc->DwUsbOtgHc, EfiUsbHcStateOperational);

Status = gBS->InstallMultipleProtocolInterfaces (
--
2.27.0




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


Re: [PATCH v1] ArmPkg/ArmPkg.dec: New pcd defined for GICv3 ITS

Ard Biesheuvel
 

Hello Shashi,

On Thu, 11 Mar 2021 at 21:20, Shashi Mallela <shashi.mallela@linaro.org> wrote:

To enable detection of GICv3 Interrupt Translation Service capability
in the ACPI MADT,a new pcd setting has been created in edk2.This pcd
setting would be referenced by edk2-platform code to advertise the ITS
physical base address within GIC ITS structure of MADT.
This does not explain why the PCD in question should be defined in
ArmPkg. UEFI itself does not use interrupts other than the timer one
in the first place, so ITS, LPI, MSI etc are also irrelevant to it.

I think it would be better to find a home for this PCD in edk2-platforms itself.


Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
ArmPkg/ArmPkg.dec | 1 +
1 file changed, 1 insertion(+)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index a8a22c649f..c22b7d0c42 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -266,6 +266,7 @@
# Base address for the GIC Redistributor region that contains the boot CPU
gArmTokenSpaceGuid.PcdGicRedistributorsBase|0|UINT64|0x0000000E
gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0|UINT64|0x0000000D
+ gArmTokenSpaceGuid.PcdGicItsBase|0|UINT64|0x0000000F
gArmTokenSpaceGuid.PcdGicSgiIntId|0|UINT32|0x00000025

#
--
2.27.0


[PATCH v1] ArmPkg/ArmPkg.dec: New pcd defined for GICv3 ITS

Shashi Mallela
 

To enable detection of GICv3 Interrupt Translation Service capability
in the ACPI MADT,a new pcd setting has been created in edk2.This pcd
setting would be referenced by edk2-platform code to advertise the ITS
physical base address within GIC ITS structure of MADT.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
ArmPkg/ArmPkg.dec | 1 +
1 file changed, 1 insertion(+)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index a8a22c649f..c22b7d0c42 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -266,6 +266,7 @@
# Base address for the GIC Redistributor region that contains the boot CPU
gArmTokenSpaceGuid.PcdGicRedistributorsBase|0|UINT64|0x0000000E
gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0|UINT64|0x0000000D
+ gArmTokenSpaceGuid.PcdGicItsBase|0|UINT64|0x0000000F
gArmTokenSpaceGuid.PcdGicSgiIntId|0|UINT32|0x00000025

#
--
2.27.0


[PATCH v1 2/2] Silicon/Qemu: Update MADT with GICv3 ITS info

Shashi Mallela
 

For Qemu sbsa-ref platforms,to enable detection of GICv3 Interrupt
Translation Service capability in the ACPI MADT,the GIC ITS structure
is created with the relevant values for each of its fields.The
existing MADT functionality is extended to include GIC ITS structure
presence as well.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Graeme Gregory <graeme@nuviainc.com>
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf | 1 +
Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf | 1 +
Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h | 10 ++++++++++
Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c | 10 +++++++++-
4 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf b/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf
index 9be34488eb7a..de58987b0044 100644
--- a/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf
+++ b/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf
@@ -34,6 +34,7 @@ [Packages]
[FixedPcd]
gArmTokenSpaceGuid.PcdGicDistributorBase
gArmTokenSpaceGuid.PcdGicRedistributorsBase
+ gArmTokenSpaceGuid.PcdGicItsBase

gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum
gArmTokenSpaceGuid.PcdArmArchTimerIntrNum
diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
index c6de685bd2c4..adf682fac564 100644
--- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
+++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
@@ -59,6 +59,7 @@ [FixedPcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision
gArmTokenSpaceGuid.PcdGicDistributorBase
gArmTokenSpaceGuid.PcdGicRedistributorsBase
+ gArmTokenSpaceGuid.PcdGicItsBase

gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorId
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision
diff --git a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h
index 4d5b05ba17c6..5f9e9477bf6a 100644
--- a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h
+++ b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h
@@ -37,6 +37,16 @@
SBSAQEMU_MADT_GICR_SIZE /* DiscoveryRangeLength */ \
}

+// Macro for MADT GIC ITS Structure
+#define SBSAQEMU_MADT_GIC_ITS_INIT() { \
+ EFI_ACPI_6_0_GIC_ITS, /* Type */ \
+ sizeof (EFI_ACPI_6_0_GIC_ITS_STRUCTURE), /* Length */ \
+ EFI_ACPI_RESERVED_WORD, /* Reserved */ \
+ 0, /* GicItsId */ \
+ FixedPcdGet64 (PcdGicItsBase), /* PhysicalBaseAddress */ \
+ EFI_ACPI_RESERVED_DWORD /* Reserved */ \
+ }
+
#define SBSAQEMU_ACPI_SCOPE_OP_MAX_LENGTH 5

#define SBSAQEMU_ACPI_SCOPE_NAME { '_', 'S', 'B', '_' }
diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
index b8901030ecd0..4e0d24ed6608 100644
--- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
+++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
@@ -91,6 +91,9 @@ AddMadtTable (
// Initialize GIC Redistributor Structure
EFI_ACPI_6_0_GICR_STRUCTURE Gicr = SBSAQEMU_MADT_GICR_INIT();

+ // Initialize GIC ITS Structure
+ EFI_ACPI_6_0_GIC_ITS_STRUCTURE Gic_Its = SBSAQEMU_MADT_GIC_ITS_INIT();
+
// Get CoreCount which was determined eariler after parsing device tree
NumCores = PcdGet32 (PcdCoreCount);

@@ -98,7 +101,8 @@ AddMadtTable (
TableSize = sizeof (EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER) +
(sizeof (EFI_ACPI_6_0_GIC_STRUCTURE) * NumCores) +
sizeof (EFI_ACPI_6_0_GIC_DISTRIBUTOR_STRUCTURE) +
- sizeof (EFI_ACPI_6_0_GICR_STRUCTURE);
+ sizeof (EFI_ACPI_6_0_GICR_STRUCTURE) +
+ sizeof (EFI_ACPI_6_0_GIC_ITS_STRUCTURE);

Status = gBS->AllocatePages (
AllocateAnyPages,
@@ -138,6 +142,10 @@ AddMadtTable (
CopyMem (New, &Gicr, sizeof (EFI_ACPI_6_0_GICR_STRUCTURE));
New += sizeof (EFI_ACPI_6_0_GICR_STRUCTURE);

+ // GIC ITS Structure
+ CopyMem (New, &Gic_Its, sizeof (EFI_ACPI_6_0_GIC_ITS_STRUCTURE));
+ New += sizeof (EFI_ACPI_6_0_GIC_ITS_STRUCTURE);
+
AcpiPlatformChecksum ((UINT8*) PageAddress, TableSize);

Status = AcpiTable->InstallAcpiTable (
--
2.27.0


[PATCH v1 1/2] Platform/Qemu/SbsaQemu/SbsaQemu.dsc: define GICv3 ITS base address

Shashi Mallela
 

Update the new pcd setting (defined in edk2 ArmPkg) with the base
address of GICv3 Interrupt Translation Service.For Qemu sbsa-ref
platforms,this enables the detection of GIC ITS capability within
the GIC ITS structure of ACPI MADT.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Graeme Gregory <graeme@nuviainc.com>
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
Platform/Qemu/SbsaQemu/SbsaQemu.dsc | 1 +
1 file changed, 1 insertion(+)

diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
index c1f8a4696560..58da89232800 100644
--- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
+++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
@@ -404,6 +404,7 @@ [PcdsFixedAtBuild.common]
#
gArmTokenSpaceGuid.PcdGicDistributorBase|0x40060000
gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x40080000
+ gArmTokenSpaceGuid.PcdGicItsBase|0x44090000

## Default Terminal Type
## 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
--
2.27.0


[PATCH v1 0/2] Add GIC ITS entry to MADT

Shashi Mallela
 

This patchset implements ACPI MADT functionality extension to include
the GICv3 ITS functionality. This enables devices to use message
signalled LPI interrupts in addition to SPIs,PPIs supported on ARM
SBSA reference qemu platforms.

Shashi Mallela (2):
Platform/Qemu/SbsaQemu/SbsaQemu.dsc: define GICv3 ITS base address
Silicon/Qemu: Update MADT with GICv3 ITS info

Platform/Qemu/SbsaQemu/SbsaQemu.dsc | 1 +
Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf | 1 +
Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf | 1 +
Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h | 10 ++++++++++
Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c | 10 +++++++++-
5 files changed, 22 insertions(+), 1 deletion(-)

--
2.27.0


Re: [edk2-platforms] [PATCH v1 0/9] IpmiFeaturePkg: Add IPMI transport drivers

Chaganty, Rangasai V
 

Overall structure looks good. For the series:
Reviewed-by: Sai Chaganty <rangasai.v.chaganty@intel.com>

There are few, rather general comments which I think can be queued up as on-going improvement after the initial check-in.
1. Copyright year for newly added files should begin with the current year // this is probably should be taken care before the first checkin
2. Ensure new GUIDs are created for newly added modules and interfaces, to avoid any potential conflicts.
3. Null instance libraries (e.g. IpmiBaseLibNull.inf) has classes defined that are not necessarily in use (e.g DebugLib). While it may not be a functional failure, it's a good practice to declare only those classes that are being used in the implementation.
4. ReadMe needs to be filled out completely.
5. Function description are found to be placed after the function (e.g. UpdateErrorStatus(), IpmiSendCommandToBmc() in IpmiBmc.c). The description comment block should precede the function.
6. Found instances where return status is marked as "Status" in description. Suggest to add possible return values in the function description
7. Found cases where we have two APIs with different names but with similar implementation - e.g. IpmiSendCommandToBmc() and PeiIpmiSendCommandToBmc(). Can these be converged?
8. Please check and address relative path issue in . inf (e.g. GenericIpmi.inf)
9. In IpmiInit.c, Please remove the following, if not applicable
#ifdef FAST_VIDEO_SUPPORT
#include <Protocol/VideoPrint.h>
#endif
10. Consider adding .fdf definition under \Include that lists the IPMI modules to be dispatched. This will simplify the integration of this feature on a platform.
11. Need to make sure package build is working.

Regards,
Sai

-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: Monday, March 01, 2021 6:28 PM
To: devel@edk2.groups.io
Cc: Oram, Isaac W <isaac.w.oram@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Michael Kubacki <michael.kubacki@microsoft.com>
Subject: [edk2-platforms] [PATCH v1 0/9] IpmiFeaturePkg: Add IPMI transport drivers

From: Isaac Oram <isaac.w.oram@intel.com>

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

Implement an open source generic IPMI transport driver.
Provides the ability to communicate with a BMC over IPMI in MinPlatform board packages.

New changes:
1. Added GenericIpmi
2. Added IPMI base libs
3. Added transport PPI and protocol
4. Updated IPMI command request and response data size from
UINT8 to UINT32 in IPMI transport layer to be compatible
with EDK2 industry standard IPMI commands.
6. Added the completion code in the first byte of all IPMI
response data to be compatible with EDK2 industry standard
IPMI commands.

Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>
Signed-off-by: Isaac Oram <isaac.w.oram@intel.com>
Co-authored-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

Isaac Oram (9):
IpmiFeaturePkg: Add IPMI driver Include headers
IpmiFeaturePkg: Add IpmiBaseLib and IpmiCommandLib
IpmiFeaturePkg: Add IpmiInit drivers
IpmiFeaturePkg: Add GenericIpmi driver common code
IpmiFeaturePkg: Add GenericIpmi PEIM
IpmiFeaturePkg: Add GenericIpmi DXE Driver
IpmiFeaturePkg: Add GenericIpmi SMM Driver
IpmiFeaturePkg: Add IPMI driver build files
Maintainers.txt: Add IpmiFeaturePkg maintainers

.../GenericIpmi/Common/IpmiBmc.c | 297 +++++++++++
.../GenericIpmi/Common/IpmiBmc.h | 44 ++
.../GenericIpmi/Common/IpmiBmcCommon.h | 179 +++++++
.../GenericIpmi/Common/IpmiHooks.c | 96 ++++
.../GenericIpmi/Common/IpmiHooks.h | 81 +++
.../GenericIpmi/Common/IpmiPhysicalLayer.h | 29 ++
.../GenericIpmi/Common/KcsBmc.c | 483 ++++++++++++++++++
.../GenericIpmi/Common/KcsBmc.h | 236 +++++++++
.../GenericIpmi/Dxe/GenericIpmi.c | 46 ++
.../GenericIpmi/Dxe/GenericIpmi.inf | 66 +++
.../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 452 ++++++++++++++++
.../GenericIpmi/Pei/PeiGenericIpmi.c | 362 +++++++++++++
.../GenericIpmi/Pei/PeiGenericIpmi.h | 138 +++++
.../GenericIpmi/Pei/PeiGenericIpmi.inf | 58 +++
.../GenericIpmi/Pei/PeiIpmiBmc.c | 277 ++++++++++
.../GenericIpmi/Pei/PeiIpmiBmc.h | 38 ++
.../GenericIpmi/Pei/PeiIpmiBmcDef.h | 156 ++++++
.../GenericIpmi/Smm/SmmGenericIpmi.c | 208 ++++++++
.../GenericIpmi/Smm/SmmGenericIpmi.inf | 53 ++
.../IpmiFeaturePkg/Include/IpmiFeature.dsc | 9 +-
.../Include/Library/IpmiBaseLib.h | 50 ++
.../Include/Library/IpmiCommandLib.h | 19 +-
.../Include/Ppi/IpmiTransportPpi.h | 68 +++
.../Include/Protocol/IpmiTransportProtocol.h | 75 +++ .../IpmiFeaturePkg/Include/ServerManagement.h | 337 ++++++++++++
.../IpmiFeaturePkg/Include/SmStatusCodes.h | 98 ++++
.../IpmiFeaturePkg/IpmiFeaturePkg.dec | 22 +-
.../IpmiFeaturePkg/IpmiInit/DxeIpmiInit.c | 4 +-
.../IpmiFeaturePkg/IpmiInit/DxeIpmiInit.inf | 6 +-
.../IpmiFeaturePkg/IpmiInit/PeiIpmiInit.c | 4 +-
.../IpmiFeaturePkg/IpmiInit/PeiIpmiInit.inf | 4 +-
.../Library/IpmiBaseLib/IpmiBaseLib.c | 155 ++++++
.../Library/IpmiBaseLib/IpmiBaseLib.inf | 28 +
.../Library/IpmiBaseLibNull/IpmiBaseLibNull.c | 76 +++
.../IpmiBaseLibNull/IpmiBaseLibNull.inf | 36 ++
.../Library/IpmiCommandLib/IpmiCommandLib.inf | 4 +-
.../IpmiCommandLib/IpmiCommandLibNetFnApp.c | 7 +-
.../IpmiCommandLibNetFnChassis.c | 51 +-
.../IpmiCommandLibNetFnStorage.c | 7 +-
.../IpmiCommandLibNetFnTransport.c | 7 +-
.../Library/PeiIpmiBaseLib/PeiIpmiBaseLib.c | 111 ++++
.../Library/PeiIpmiBaseLib/PeiIpmiBaseLib.inf | 30 ++
.../Library/SmmIpmiBaseLib/SmmIpmiBaseLib.c | 180 +++++++
.../Library/SmmIpmiBaseLib/SmmIpmiBaseLib.inf | 29 ++
.../IpmiFeaturePkg/Readme.md | 4 +-
Maintainers.txt | 6 +
46 files changed, 4694 insertions(+), 32 deletions(-) create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiPhysicalLayer.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/KcsBmc.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/KcsBmc.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.inf
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.inf
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmcDef.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.inf
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/Library/IpmiBaseLib.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/Ppi/IpmiTransportPpi.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/Protocol/IpmiTransportProtocol.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/ServerManagement.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/SmStatusCodes.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLib/IpmiBaseLib.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLib/IpmiBaseLib.inf
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLibNull/IpmiBaseLibNull.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLibNull/IpmiBaseLibNull.inf
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/PeiIpmiBaseLib/PeiIpmiBaseLib.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/PeiIpmiBaseLib/PeiIpmiBaseLib.inf
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/SmmIpmiBaseLib/SmmIpmiBaseLib.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/SmmIpmiBaseLib/SmmIpmiBaseLib.inf

--
2.27.0.windows.1


Re: [PATCH 1/3 v6] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver

Achin Gupta <achin.gupta@...>
 

Hi,

CIL...

On Thu, Mar 11, 2021 at 06:37:30PM +0200, Ilias Apalodimas wrote:
(+cc Achin)

On Wed, 10 Mar 2021 at 14:45, Ilias Apalodimas via groups.io <ilias.apalodimas=
linaro.org@groups.io> wrote:

Hi Pierre,

On Wed, Mar 10, 2021 at 09:58:19AM +0000, Pierre wrote:
> Hi Ilias,
> Just minor coding style nit picking:
>
> * Drivers/OpTee/OpteeRpmbPkg/FixupPcd.c:FixPcdMemory() I think the
> error codes are missing in the function header

Ah you mean the return values of locate protocol?

> * Thanks for adding the IN/OUT function parameter descriptors. Is it
> possible to also add them in the function headers [1] ?

Sure, I'll send a v7 anyways since I managed to mess up the maintainers
patch
somehow...

I hope I haven't missed any of your other requests.

>
> About the FFA/SVC call:
>
> >> If this is an FFA call, is it possible to:
> >> - put a reference in the header to the spec (it should be similar to
the
> >> one at
> >> edk2/ArmPkg/Library/StandaloneMmMmuLib/AArch64/
ArmMmuStandaloneMmLib.c)
> >> - check the return status of the SVC call against the ones available
at
> >> edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
> >> - if possible, remove the dependency to <IndustryStandard/ArmMmSvc.h>
> >
> > The call is technically an FFA one but at the moment OP-TEE returns the
> StMM
> > return code which is defined in the last header you mention.
> > The relevant code is in ./core/arch/arm/kernel/stmm_sp.c function
> > tee2stmm_ret_val().
> > So unless we redefine that in OP-TEE or (better imho), wait for a full
FFA
> > mechanism to be in place, I'd prefer leaving it as is.
> > Keep in mind that adding the full FFA will also get rid of the
hardcoded
> IDs
> > on the beginning of the file.
>
> The description of a FFA_MSG_SEND_DIRECT_REQ (s10.2 of [2]) doesn't seem
to
> return the same error codes as the ones optee returns (in
> optee_os/core/arch/arm/kernel/stmm_sp.c:tee2stmm_ret_val()). I am not
sure a
> new FFA specification will change these error codes.
> Another thing is that I think the mMemMgrId variable described in
> edk2-platforms/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c is currently
> defined as edk2/ArmPkg/Include/IndustryStandard/
ArmFfaSvc.h:ARM_FFA_DESTINATION_ENDPOINT_ID
> (the name seems to be misleading).
> I think it would be better to:
>
> * align the optee error codes with what is in the FFA spec
> * handle these error codes in edk2 with what is in
> edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h and remove the
> dependendy to edk2/ArmPkg/Include/IndustryStandard/ArmMmSvc.h if
> possible
> * rename
> edk2/ArmPkg/Include/IndustryStandard/
ArmFfaSvc.h:ARM_FFA_DESTINATION_ENDPOINT_ID
> define to a proper name, according to what is in
> optee_os/core/arch/arm/kernel/stmm_sp.c, and add one define for
> 'mem_mgr_id'
> * remove the mMemMgrId and mStorageId variables from
> edk2-platforms/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c and use the
> newly created defines from
> edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
>
> This would allow to be aligned with the current FFA spec and when a new
one
> comes, these endpoints IDs (mMemMgrId/mStorageId) can just be removed
from
> one location (as you said).
> In the end it seems you and Sami will maintain this, so I will let you
> decide what is best.

I get the whole point, and for the record you are technically right.

But there's something (once again) that's 'weird' here.
This StandAloneMM that's compiling over here is only used by OP-TEE.
In order to use that you need to call an SMC into OP-TEE (not FFA) from
the non-secure world to initiate it.
There's an OP-TEE PTA (pseudo-TA), that then converts the message to MM and
sends it over to StandAloneMM. There are no FFA manifests yet, that's why
the
get/set memory attributes code is still running, to set up page
permissions
as well.

The FFA mechanism you are seeing right now, is just the internal contract
between OP-TEE and this driver. We did some of the calls depend on FFA
since
this was a good way to start introducing FFA code into EDK2 (which will
eventually be needed), without being too intrusive.

In the long run OP-TEE will be fully converted into FFA the changes you are
talking about make sense. In fact there's a ./core/arch/arm/kernel/
secure_partition.c
in OP-TEE doing exactly that but it's not yet complete.
I tried to describe the entire situation here [1].

If anyone feels really strong about this, we can go and change it. The
changes
aren't too big to begin with. That being said I'd prefer keeping it as is,
since this will naturally evolve to a complete FFA spec, but the mechanisms
are still missing from OP-TEE. Last but not least when OP-TEE gets that's
FFA
support you won't bundle StandAloneMM with the driver right? You'd have 2
discrete Secure partitions, one dealing with variables and one dealing with
storage.
Just to second Ilias's explanation above...

The plan is to incorporate the ABIs to get and set memory attributes into the
FF-A v1.1 specification. This way, the memory manager service will not be a
protocol that uses FF-A DIRECT_REQUEST and DIRECT_RESP as the
transport. Instead, it will be natively implemented by OP-TEE. The error codes
etc will align at this point.

I hope this helps clarify. Do shout if you need more information.

cheers,
Achin


[1] https://apalos.github.io/
Protected%20UEFI%20variables%20with%20U-Boot.html#
Protected%20UEFI%20variables%20with%20U-Boot


Thanks
/Ilias
>
> [1] https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/
6_documenting_software/610_special_commands#
6-10-4-param-in-out-parameter-description
> [2] https://developer.arm.com/documentation/den0077/a
>
> Regards,
> Pierre






Re: [PATCH] Platform/RaspberryPi: Dynamically build UARTs info in ACPI

Mario Bălănică
 

+=0D
+ PinFunction (Exclusive, PullDown, BCM_ALT3, "\\_SB.GDV0.GPI0", 0, Reso=
urceConsumer, , ) { 32, 33 }=0D
+=0D
+ // fake the CTS signal as we don't support HW flow control yet=0D
+ // BCM_ALT2 is set as output (low) by default=0D
+ PinFunction (Exclusive, PullNone, BCM_ALT2, "\\_SB.GDV0.GPI0", 0, Reso=
urceConsumer, , ) { 31 }=0D
})=0D
I think it's better to do the pin muxing in ConfigDxe, depending on which UART is free to be used for Bluetooth.


How does EDK2 detect virtio-blk-pci device as a boot device

Xiaohe Yang <xh_young@...>
 

Hello, I am using EDK2  OvmfPkgX64 at commit 37568365, and have a problem. The following is the detail.


The Image disk that I use has EFI system partition, and is configured as a virtio-blk-pci device. Bootorder file-entry is not added in fw_cfg device. The log of EDK2 shows that

(1)  VirtioBlkDxe.efi is successfully loaded at DXE stage. BootScriptExecutorDxe drivers "was discovered but not loaded".

(2) PCI enumeration has finished at the start of BDS stage, virtio-blk-pci device is discovered. And one Option Rom's address is 0/0.

(3) Before entering Shell, log shows "map: No mapping found".

(4) After entering Shell, I choose “Boot From File”, the virtio-blk-pci device is not displayed.


So I wonder what are the requirements for UEFI to detect the virtio-blk-pci device as a boot device, except having EFI system partition?


Thanks,

Xiaohe.


Help/Advice needed to Dump display MMIO registers from EFI Shell

Rao G
 

Hi All,

Could dump graphics display registers from OS with intel_gpu_tools, but not from EFI Shell


Interested to dump the above MMIO registers in the pdf for BayTrail platfrm from EFI shell, Kindly provide some clues


BAR's available

  Bus 0, Device 2, Function 0: 

1. IOBAR – IO access window for internal graphics. Though this window address/data register pair, using I/O semantics, the IGD and internal graphics instruction port registers can be accessed. Note, this allows accessing the same registers as GTTMMADR. The IOBAR can be used to issue writes to the GTTMMADR or the GTT table. 

2. GMADR – Internal graphics translation window (128MB, 256MB, 512MB window). 

3. GTTMMADR – This register requests a 4MB allocation for combined Graphics Translation Table Modification Range and Memory Mapped Range. GTTADR will be at GTTMMADR + 2MB while the MMIO base address will be the same as GTTMMADR.

the register dump from OS are attached to this email


[edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Fix dwc2 reset on raspberry pi boards

treffer+groups.io@...
 

DwHcReset expects attributes as the second argument. A reset is
performed if the passed attribute is valid. However 0 is not a valid
attribute and will thus never cause a controller reset.

Passing EFI_USB_HC_RESET_HOST_CONTROLLER will reset the dwc2 controller
as expected.

This enables the USB 2.0 port of the raspberry compute module 4.
---
 Platform/RaspberryPi/Drivers/DwUsbHostDxe/DriverBinding.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/Drivers/DwUsbHostDxe/DriverBinding.c
b/Platform/RaspberryPi/Drivers/DwUsbHostDxe/DriverBinding.c
index bada13a6cd..bb228e62d9 100644
--- a/Platform/RaspberryPi/Drivers/DwUsbHostDxe/DriverBinding.c
+++ b/Platform/RaspberryPi/Drivers/DwUsbHostDxe/DriverBinding.c
@@ -140,7 +140,7 @@ DriverStart (
    * UsbBusDxe as of b4e96b82b4e2e47e95014b51787ba5b43abac784 expects
    * the HCD to do this. There is no agent invoking DwHcReset anymore.
    */
-  DwHcReset (&DwHc->DwUsbOtgHc, 0);
+  DwHcReset (&DwHc->DwUsbOtgHc, EFI_USB_HC_RESET_HOST_CONTROLLER);
   DwHcSetState (&DwHc->DwUsbOtgHc, EfiUsbHcStateOperational);
 
   Status = gBS->InstallMultipleProtocolInterfaces (
--
2.27.0


GSoC 2021

KAAIRA GUPTA
 

Hey everyone! My name is Kaaira Gupta. I am a junior at IIT Roorkee, India. I am experienced in C, C++ and Linux kernel. I have gone through the ideas list and I find the idea of building MinPlatform board port for Qemu very interesting. Currently I am reading the information of this task posted with the idea list. Please let me know if I should know anything else or if I am missing something. Also, please let me know whom/where I should contact for doubts in this Idea. Looking forward contributing!

Thanks,
Kaaira Gupta


Re: 回复: [edk2-devel] [edk2-staging][PATCH V1 2/5] MinPlatformPkg: Core Include Files: Added Tcg2Acpi driver after separation

Kun Qin
 

Thanks, Liming. Will use [edk2-platform] next time contributing to this repo.

Regards,
Kun 


Re: [PATCH] MdeModulePkg: Initialize local variable value before they are used

Laszlo Ersek
 

Hi Liming,

On 03/09/21 02:24, gaoliming wrote:
This change is to fix the compiler error on GCC49 release build.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Liming Gao <gaoliming@byosoft.com.cn>
---
MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c | 1 +
.../Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c | 1 +
2 files changed, 2 insertions(+)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
index e99a812a44..0779f94f9e 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
@@ -1122,6 +1122,7 @@ AhciDmaTransfer (

Map = NULL;
PciIo = Instance->PciIo;
+ Status = EFI_SUCCESS;

if (PciIo == NULL) {
return EFI_INVALID_PARAMETER;
diff --git a/MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c b/MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c
index 0c9299c8b0..7822cbf4bb 100644
--- a/MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c
+++ b/MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c
@@ -117,6 +117,7 @@ CreateBasicVariablePolicy (

// Now we've gotta determine the total size of the buffer required for
// the VariablePolicy structure.
+ NameSize = 0;
TotalSize = sizeof( VARIABLE_POLICY_ENTRY );
if (Name != NULL) {
NameSize = StrnSizeS( Name, MAX_UINT16 );
(1) we have a bugzilla ticket related to this (or more precisely,
overlapping AtaAtapiPassThru):

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

I think mentioning the BZ in the commit message might make sense.

(2) Recently we have used a special comment format for such assignments.
Namely:

//
// Set Status to suppress incorrect compiler/analyzer warnings
//
Status = EFI_SUCCESS;

Hao already requested that we should document that we only suppress
compiler false positives with these assignments -- they are not needed
functionally. However, I think saying so in the commit message *only* is
not sufficient. I think we should stick with the above code-comment
format (which we've used recently in several places).

Thanks!
Laszlo


Re: [PATCH 0/2] Maintainers: create the "OvmfPkg: Confidential Computing" subsystem

James Bottomley
 

On Thu, 2021-03-11 at 18:13 +0100, Laszlo Ersek wrote:
On 03/10/21 19:56, Laszlo Ersek wrote:
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3249

Generalize the current OVMF SEV subsystem entry, so that we can use
it for Intel TDX in the future, ensuring proper patch circulation
for reviews.

Cc: Andrew Fish <afish@apple.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>

Thanks
Laszlo

Laszlo Ersek (2):
Maintainers: refresh the OVMF SEV subsystem after TianoCore #2198
and #3077
Maintainers: rename the OVMF SEV subsystem to "Confidential
Computing"

Maintainers.txt | 28 +++++++++++++-------
1 file changed, 18 insertions(+), 10 deletions(-)


base-commit: edd46cd407ea4a0adaa8d6ca86f550c2a4d5c507
For merging this series, I still need ACKs from James and Min Xu,
please.
Acked-by: James Bottomley <jejb@linux.ibm.com>

James


Re: [PATCH 0/2] Maintainers: create the "OvmfPkg: Confidential Computing" subsystem

Laszlo Ersek
 

On 03/10/21 19:56, Laszlo Ersek wrote:
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3249

Generalize the current OVMF SEV subsystem entry, so that we can use it
for Intel TDX in the future, ensuring proper patch circulation for
reviews.

Cc: Andrew Fish <afish@apple.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>

Thanks
Laszlo

Laszlo Ersek (2):
Maintainers: refresh the OVMF SEV subsystem after TianoCore #2198 and
#3077
Maintainers: rename the OVMF SEV subsystem to "Confidential Computing"

Maintainers.txt | 28 +++++++++++++-------
1 file changed, 18 insertions(+), 10 deletions(-)


base-commit: edd46cd407ea4a0adaa8d6ca86f550c2a4d5c507
For merging this series, I still need ACKs from James and Min Xu, please.

Thanks
Laszlo


Re: [PATCH 1/3 v6] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver

Ilias Apalodimas
 

(+cc Achin)


On Wed, 10 Mar 2021 at 14:45, Ilias Apalodimas via groups.io <ilias.apalodimas=linaro.org@groups.io> wrote:
Hi Pierre,

On Wed, Mar 10, 2021 at 09:58:19AM +0000, Pierre wrote:
> Hi Ilias,
> Just minor coding style nit picking:
>
>  * Drivers/OpTee/OpteeRpmbPkg/FixupPcd.c:FixPcdMemory() I think the
>    error codes are missing in the function header

Ah you mean the return values of locate protocol?

>  * Thanks for adding the IN/OUT function parameter descriptors. Is it
>    possible to also add them in the function headers [1] ?

Sure, I'll send a v7 anyways since I managed to mess up the maintainers patch
somehow...

I hope I haven't missed any of your other requests.

>
> About the FFA/SVC call:
>
> >> If this is an FFA call, is it possible to:
> >>  - put a reference in the header to the spec (it should be similar to the
> >> one at
> >> edk2/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c)
> >>  - check the return status of the SVC call against the ones available at
> >> edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
> >>  - if possible, remove the dependency to <IndustryStandard/ArmMmSvc.h>
> >
> > The call is technically an FFA one but at the moment OP-TEE returns the
> StMM
> > return code which is defined in the last header you mention.
> > The relevant code is in ./core/arch/arm/kernel/stmm_sp.c function
> > tee2stmm_ret_val().
> > So unless we redefine that in OP-TEE or (better imho), wait for a full FFA
> > mechanism to be in place, I'd prefer leaving it as is.
> > Keep in mind that adding the full FFA will also get rid of the hardcoded
> IDs
> > on the beginning of the file.
>
> The description of a FFA_MSG_SEND_DIRECT_REQ (s10.2 of [2]) doesn't seem to
> return the same error codes as the ones optee returns (in
> optee_os/core/arch/arm/kernel/stmm_sp.c:tee2stmm_ret_val()). I am not sure a
> new FFA specification will change these error codes.
> Another thing is that I think the mMemMgrId variable described in
> edk2-platforms/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c is currently
> defined as edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h:ARM_FFA_DESTINATION_ENDPOINT_ID
> (the name seems to be misleading).
> I think it would be better to:
>
>  * align the optee error codes with what is in the FFA spec
>  * handle these error codes in edk2 with what is in
>    edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h and remove the
>    dependendy to edk2/ArmPkg/Include/IndustryStandard/ArmMmSvc.h if
>    possible
>  * rename
>    edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h:ARM_FFA_DESTINATION_ENDPOINT_ID
>    define to a proper name, according to what is in
>    optee_os/core/arch/arm/kernel/stmm_sp.c, and add one define for
>    'mem_mgr_id'
>  * remove the mMemMgrId and mStorageId variables from
>    edk2-platforms/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c and use the
>    newly created defines from
>    edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
>
> This would allow to be aligned with the current FFA spec and when a new one
> comes, these endpoints IDs (mMemMgrId/mStorageId) can just be removed from
> one location (as you said).
> In the end it seems you and Sami will maintain this, so I will let you
> decide what is best.

I get the whole point, and for the record you are technically right.

But there's something (once again) that's 'weird' here.
This StandAloneMM that's compiling over here is only used by OP-TEE.
In order to use that you need to call an SMC into OP-TEE (not FFA) from
the non-secure world to initiate it.
There's an OP-TEE PTA (pseudo-TA), that then converts the message to MM and
sends it over to StandAloneMM.  There are no FFA manifests yet, that's why the
get/set memory attributes code is still running,  to set up page permissions
as well.

The FFA mechanism you are seeing right now, is just the internal contract
between OP-TEE and this driver. We did some of the calls depend on FFA since
this was a good way to start introducing FFA code into EDK2 (which will
eventually be needed), without being too intrusive.

In the long run OP-TEE will be fully converted into FFA the changes you are
talking about make sense. In fact there's a ./core/arch/arm/kernel/secure_partition.c
in OP-TEE doing exactly that but it's not yet complete.
I tried to describe the entire situation here [1].

If anyone feels really strong about this, we can go and change it. The changes
aren't too big to begin with. That being said I'd prefer keeping it as is,
since this will naturally evolve to a complete FFA spec, but the mechanisms
are still missing from OP-TEE. Last but not least when OP-TEE gets that's FFA
support you won't bundle StandAloneMM with the driver right? You'd have 2
discrete Secure partitions, one dealing with variables and one dealing with
storage.

[1] https://apalos.github.io/Protected%20UEFI%20variables%20with%20U-Boot.html#Protected%20UEFI%20variables%20with%20U-Boot


Thanks
/Ilias
>
> [1] https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/6_documenting_software/610_special_commands#6-10-4-param-in-out-parameter-description
> [2] https://developer.arm.com/documentation/den0077/a
>
> Regards,
> Pierre






Re: Conflicting virtual addresses causing Runtime Services issues

Jon Nettleton
 

On Thu, Mar 11, 2021 at 3:51 PM Laszlo Ersek <lersek@redhat.com> wrote:

On 03/11/21 10:48, Jon Nettleton wrote:
On Thu, Mar 11, 2021 at 7:54 AM Jon Nettleton via groups.io
<jon=solid-run.com@groups.io> wrote:

On Wed, Mar 10, 2021 at 3:52 PM Laszlo Ersek <lersek@redhat.com> wrote:

On 03/10/21 09:04, Jon Nettleton wrote:
I am debugging a failure that I am seeing while using the HoneyComb's
spi-nor flash for runtime variable storage. I am hoping someone on
the list can give me some insight as what may be the problem.

The problem showed up when we switched the MMIO region for the fspi
flash device to be marked as non executable. reading variables is
fine, however writes began throwing an error.

[ 556.709828] Unable to handle kernel execute from non-executable
memory at virtual address 00000000206a3968

I have patched the kernel and removed the X86 requirement and enabled
the sysfs runtime mappings kernel config so I can get an easy view of
the mappings the kernel carries for runtime services. I then track
that virtual address to the MMIO region of nor flash, which makes
sense that region is marked as non executable. The question is why is
code being executed from this address range

attribute
::::::::::::::
0x8000000000000001
::::::::::::::
num_pages
::::::::::::::
0x40
::::::::::::::
phys_addr
::::::::::::::
0x20500000
::::::::::::::
type
::::::::::::::
0xb
::::::::::::::
virt_addr
::::::::::::::
0x20680000

So then I patched the PL011 serial driver to be able to log to the
console in runtime and I track down the access to Status =
Fvb->GetPhysicalAddress(Fvb, &FvVolHdr); in UpdateVariableStore().
What I don't understand is why EfiConvertPointer is mapping that
pointer into the Virtual address space occupied by the runtime mmio of
the flash. The pointer is being properly remapped. Here are the
pointer addresses in EFI and Kernel Runtime

EFI:
UpdateVariableStore:156 ECE33968
FvbGetPhysicalAddress(BaseAddress=0x20000000)

KERNEL:
UpdateVariableStore:156 206A3968
[ 556.709828] Unable to handle kernel execute from non-executable
memory at virtual address 00000000206a3968

Any insight that anyone could provide would be much appreciated.
Your platform appears misconfigured -- the flash MMIO range appears to
overlap runtime services code even before SetVirtualAddressMap. The
virtual address conflict is likely the result of the original physical
address conflict.
There is no physical address conflict. Running in physical mode the pointer
for GetPhysicalAddress is at 0xECE33968 and the MMIO physical
region is 0x20000000 - 0x2FFFFFFF. Shown as the BaseAddress above.
Obviously I don't use that full MMIO region because our flash is not
256MB.


After the virtual address updates, the EfiMemoryMappedIO (0xb) type
range is mapped at virtual address range [0x20680000, 0x206C0000). The
GetPhysicalAddress function seems to be located at offset 0x23968 in
that range (with 0x1C698 bytes to go in the range). That's inexplicable.
Yes, exactly why I am reaching out.


What is the physical base address of the flash? What are the PCDs used
in "ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c"?
We don't use the ArmPlatformPkg

The code is available here.
https://github.com/SolidRun/edk2-platforms/tree/LX2160_UEFI_ACPI_EAR3-lx2160cex7/Silicon/NXP/Drivers/SpiNorFlashDxe

Thanks
Found the root of my problem and fixed it. As used from other devices
they are relocating the individual pointer for each function in FvbProtocol
of the DXE. This is done in ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c,
as well as lots of vendor drivers. I was able to dump the addresses being used
in RuntimeDriverConvertPointer and could locate the pointer to
GetPhysicalAddress.

Convert 0xEC8C1648:0xEC8B0000:0x203D0000
New virtual address is then 203E1648 which is fine.

However then later down in the remapping I found
Convert 0x203E1648:0x20000000:0x20800000

And this is where the pointer gets remapped again and into the
MMIO space of the nor flash. If I remove the calls to
ConvertPointer for the FvbProtocol I am still seeing those addresses
getting remapped but only once and runtime works as expected.

I am seeing that in MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
&mVariableModuleGlobal->FvbInstance->* are all being converted. It is
possible this is a long standing bug and it just so happens that our
configuration
has caused a conflict and exposed it.
Yes, this is curious, I noticed it too yesterday, trying to see where
the FVB protocol member function pointers were converted. I found that
OVMF's flash driver (OvmfPkg/QemuFlashFvbServicesRuntimeDxe) didn't do
it, but MdeModulePkg/Universal/Variable/RuntimeDxe did. That was
certainly strange, as the variable driver is a consumer of the protocol
(not the producer thereof), so I'd say it has no business poking new
values into the protocol interface structure.

So the issue (IIUC) on your platform is that the function pointers are
doubly-converted, once by your flash driver (which is arguably the right
thing), and another time by the variable driver.
Correct, that seems to be what is happening


I recommend filing a bug about this, at
<https://bugzilla.tianocore.org/>, against "VariableDxe.c".
Sure


I suggest CC'ing the "UEFI Variable modules" reviewers on the BZ as
well, from "Maintainers.txt" -- FWIW, I'm CC'ing Hao and Liming on this
email, now.

... It feels as if the Platform Init specification should speak to the
FVB[2] protocol's runtime aspects... But (as of v1.7), I don't see
anything runtime-related in that section. In fact I've checked the whole
PI v1.7 spec for "runtime", and the closest matches are under
EFI_VARIABLE_WRITE_ARCH_PROTOCOL. FVB2 is not spelled out as a
(potential) runtime dependency of EFI_VARIABLE_WRITE_ARCH_PROTOCOL. The
only explicitly runtime protocol (unrelated to runtime services) is
"Status Code Runtime Protocol".

So what we're seeing here is an edk2-ism, I believe.
I checked the commits and the code that does that conversion is quite old.


I tried seeing if one of the "tour beyond BIOS" edk2 whitepapers said
anything on FVB -- but the only reference I found (in "Implementing
UEFI Authenticated Variables in SMM with EDKII", version 2) mentions
only *SMM* FVB.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-white-papers

I think we should have a TianoCore BZ about this, even if we don' change
the code as a result -- which driver is responsible for updating the
runtime FVB protocol member functions should be spelled out somewhere
"searchable".

... Strangely, the other flash (FVB) driver in edk2,
ArmPlatformPkg/Drivers/NorFlashDxe, *does* perform the conversion
itself! See NorFlashVirtualNotifyEvent().

I don't understand that. Is it possible that, with
"ArmPlatformPkg/Drivers/NorFlashDxe" too, the conversion happens
*twice*, but (at least) one of those mappings is "identity"?
I believe all the drivers that are doing the conversion are getting
double mappings. It just so happens our second mapping got poked
into an area that we had marked non-executable which started the
debugging and led to the issue.


Thanks
Laszlo






Re: Conflicting virtual addresses causing Runtime Services issues

Laszlo Ersek
 

On 03/11/21 10:48, Jon Nettleton wrote:
On Thu, Mar 11, 2021 at 7:54 AM Jon Nettleton via groups.io
<jon=solid-run.com@groups.io> wrote:

On Wed, Mar 10, 2021 at 3:52 PM Laszlo Ersek <lersek@redhat.com> wrote:

On 03/10/21 09:04, Jon Nettleton wrote:
I am debugging a failure that I am seeing while using the HoneyComb's
spi-nor flash for runtime variable storage. I am hoping someone on
the list can give me some insight as what may be the problem.

The problem showed up when we switched the MMIO region for the fspi
flash device to be marked as non executable. reading variables is
fine, however writes began throwing an error.

[ 556.709828] Unable to handle kernel execute from non-executable
memory at virtual address 00000000206a3968

I have patched the kernel and removed the X86 requirement and enabled
the sysfs runtime mappings kernel config so I can get an easy view of
the mappings the kernel carries for runtime services. I then track
that virtual address to the MMIO region of nor flash, which makes
sense that region is marked as non executable. The question is why is
code being executed from this address range

attribute
::::::::::::::
0x8000000000000001
::::::::::::::
num_pages
::::::::::::::
0x40
::::::::::::::
phys_addr
::::::::::::::
0x20500000
::::::::::::::
type
::::::::::::::
0xb
::::::::::::::
virt_addr
::::::::::::::
0x20680000

So then I patched the PL011 serial driver to be able to log to the
console in runtime and I track down the access to Status =
Fvb->GetPhysicalAddress(Fvb, &FvVolHdr); in UpdateVariableStore().
What I don't understand is why EfiConvertPointer is mapping that
pointer into the Virtual address space occupied by the runtime mmio of
the flash. The pointer is being properly remapped. Here are the
pointer addresses in EFI and Kernel Runtime

EFI:
UpdateVariableStore:156 ECE33968
FvbGetPhysicalAddress(BaseAddress=0x20000000)

KERNEL:
UpdateVariableStore:156 206A3968
[ 556.709828] Unable to handle kernel execute from non-executable
memory at virtual address 00000000206a3968

Any insight that anyone could provide would be much appreciated.
Your platform appears misconfigured -- the flash MMIO range appears to
overlap runtime services code even before SetVirtualAddressMap. The
virtual address conflict is likely the result of the original physical
address conflict.
There is no physical address conflict. Running in physical mode the pointer
for GetPhysicalAddress is at 0xECE33968 and the MMIO physical
region is 0x20000000 - 0x2FFFFFFF. Shown as the BaseAddress above.
Obviously I don't use that full MMIO region because our flash is not
256MB.


After the virtual address updates, the EfiMemoryMappedIO (0xb) type
range is mapped at virtual address range [0x20680000, 0x206C0000). The
GetPhysicalAddress function seems to be located at offset 0x23968 in
that range (with 0x1C698 bytes to go in the range). That's inexplicable.
Yes, exactly why I am reaching out.


What is the physical base address of the flash? What are the PCDs used
in "ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c"?
We don't use the ArmPlatformPkg

The code is available here.
https://github.com/SolidRun/edk2-platforms/tree/LX2160_UEFI_ACPI_EAR3-lx2160cex7/Silicon/NXP/Drivers/SpiNorFlashDxe

Thanks
Found the root of my problem and fixed it. As used from other devices
they are relocating the individual pointer for each function in FvbProtocol
of the DXE. This is done in ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c,
as well as lots of vendor drivers. I was able to dump the addresses being used
in RuntimeDriverConvertPointer and could locate the pointer to
GetPhysicalAddress.

Convert 0xEC8C1648:0xEC8B0000:0x203D0000
New virtual address is then 203E1648 which is fine.

However then later down in the remapping I found
Convert 0x203E1648:0x20000000:0x20800000

And this is where the pointer gets remapped again and into the
MMIO space of the nor flash. If I remove the calls to
ConvertPointer for the FvbProtocol I am still seeing those addresses
getting remapped but only once and runtime works as expected.

I am seeing that in MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
&mVariableModuleGlobal->FvbInstance->* are all being converted. It is
possible this is a long standing bug and it just so happens that our
configuration
has caused a conflict and exposed it.
Yes, this is curious, I noticed it too yesterday, trying to see where
the FVB protocol member function pointers were converted. I found that
OVMF's flash driver (OvmfPkg/QemuFlashFvbServicesRuntimeDxe) didn't do
it, but MdeModulePkg/Universal/Variable/RuntimeDxe did. That was
certainly strange, as the variable driver is a consumer of the protocol
(not the producer thereof), so I'd say it has no business poking new
values into the protocol interface structure.

So the issue (IIUC) on your platform is that the function pointers are
doubly-converted, once by your flash driver (which is arguably the right
thing), and another time by the variable driver.

I recommend filing a bug about this, at
<https://bugzilla.tianocore.org/>, against "VariableDxe.c".

I suggest CC'ing the "UEFI Variable modules" reviewers on the BZ as
well, from "Maintainers.txt" -- FWIW, I'm CC'ing Hao and Liming on this
email, now.

... It feels as if the Platform Init specification should speak to the
FVB[2] protocol's runtime aspects... But (as of v1.7), I don't see
anything runtime-related in that section. In fact I've checked the whole
PI v1.7 spec for "runtime", and the closest matches are under
EFI_VARIABLE_WRITE_ARCH_PROTOCOL. FVB2 is not spelled out as a
(potential) runtime dependency of EFI_VARIABLE_WRITE_ARCH_PROTOCOL. The
only explicitly runtime protocol (unrelated to runtime services) is
"Status Code Runtime Protocol".

So what we're seeing here is an edk2-ism, I believe.

I tried seeing if one of the "tour beyond BIOS" edk2 whitepapers said
anything on FVB -- but the only reference I found (in "Implementing
UEFI Authenticated Variables in SMM with EDKII", version 2) mentions
only *SMM* FVB.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-white-papers

I think we should have a TianoCore BZ about this, even if we don' change
the code as a result -- which driver is responsible for updating the
runtime FVB protocol member functions should be spelled out somewhere
"searchable".

... Strangely, the other flash (FVB) driver in edk2,
ArmPlatformPkg/Drivers/NorFlashDxe, *does* perform the conversion
itself! See NorFlashVirtualNotifyEvent().

I don't understand that. Is it possible that, with
"ArmPlatformPkg/Drivers/NorFlashDxe" too, the conversion happens
*twice*, but (at least) one of those mappings is "identity"?

Thanks
Laszlo

17241 - 17260 of 89854