Date   

Re: [edk2-plaforms PATCH 2/3] RISC-V/PlatformPkg: Revise Readme.md

Leif Lindholm
 

On Thu, Aug 27, 2020 at 20:03:04 +0800, Abner Chang wrote:
Update RISC-V PlatformPkg Readme.md to align with the latest implementation.

Signed-off-by: Abner Chang <abner.chang@...>
Co-authored-by: Daniel Schaefer <daniel.schaefer@...>

Cc: Daniel Schaefer <daniel.schaefer@...>
---
Platform/RISC-V/PlatformPkg/Readme.md | 72 ++++++++++++++-------------
1 file changed, 37 insertions(+), 35 deletions(-)

diff --git a/Platform/RISC-V/PlatformPkg/Readme.md b/Platform/RISC-V/PlatformPkg/Readme.md
index 2632ebeb28..bd3b823fb4 100644
--- a/Platform/RISC-V/PlatformPkg/Readme.md
+++ b/Platform/RISC-V/PlatformPkg/Readme.md
@@ -1,49 +1,48 @@
-# Introduction
+# Introduction of EDK2 RISC-V Port
This is edk2-platforms: any introduction of edk2 portions should be in edk2.


-## EDK2 RISC-V Platform Packages
-RISC-V platform package provides the generic and common modules for RISC-V
-platforms. RISC-V platform package could include RiscPlatformPkg.dec to
-use the common drivers, libraries, definitions, PCDs and etc. for the
-platform development.
+## EDK2 RISC-V Project
+The edk2 build architecture which is supported and verified on edk2 code base for RISC-V platforms is `RISCV64`.
+The toolchain is on RISC-V GitHub (https://github.com/riscv/riscv-gnu-toolchain) for building edk2 RISC-V binary.
+The corresponding edk2 Toolchain tag for building RISC-V platform is "GCC5" declared in `tools_def.txt`.
Please wrap long lines, like in the text being replaced.
The point of markdown/rst etc is that it can be rendered into
auto-reflowed HTML text *or* read directly in a terminal. Wrapping it
properly for the latter won't impact the former.


-There are two packages to support RISC-V:
+There are two packages to support RISC-V edk2 platforms:
- `edk2-platforms/Silicon/RISC-V/ProcessorPkg/RiscVProcessorPkg.dec`
- `edk2-platforms/Platform/RISC-V/PlatformPkg/RiscVPlatformPkg.dec`
(I would say edk2-platforms can be left out when referring to the
current repository.)


-`RiscVPlatformPkg` provides SEC phase and NULL libs.
-`RiscVProcessorPkg` provides many libraries, PEIMs and DXE drivers.
+`RiscVPlatformPkg` currently provides the generic SEC driver for all RISC-V platforms, and some platform level libraries.
+`RiscVProcessorPkg` currently provides RISC-V processor related libraries, PEI modules, DXE drivers and industrial
+standard header files.

-### Download the sources ###
+## EDK2 RISC-V Platform Package
edk2-platforms?

+RISC-V platform package provides the common modules for RISC-V platforms. RISC-V platform vendors could include
+RiscPlatformPkg.dec to use the common drivers, libraries, definitions, PCDs and etc. for the
+RISC-V platform development.
+
+### Download the Source Code ###
```
git clone https://github.com/tianocore/edk2.git
+git clone https://github.com/tianocore/edk2-platforms.git

-git clone https://github.com/changab/edk2-platforms.git
-# Check out branch: riscv-smode-lib
```

-To build it, you have to follow the regular steps for EDK2 and additionally set
-an environmen variable to point to your RISC-V toolchain installation,
-including the binary prefixes:
-
+You have to follow the build steps for EDK2 (https://github.com/tianocore/tianocore.github.io/wiki/Getting-Started-with-EDK-II)
+and additionally set an environment variable to point to your RISC-V toolchain binaries for building RISC-V
+platforms,
```
+# e.g. If the toolchain binaries are under /riscv-gnu-toolchain-binaries/bin
export GCC5_RISCV64_PREFIX=/riscv-gnu-toolchain-binaries/bin/riscv64-unknown-elf-
```
Look, I realise you guys aren't building natively yet, but I
*strongly* recomment that you start seeing that as something normal
sooner rather than later. There's nothing wrong with describing cross
compilation as well, and in these early days even point to specific
"known good" toolchains, but treating it as the only valid way of
building feeds complacency.

And even while I *did* push for native-is-normal for arm64 from the
earliest days, we still get occasional comments about people using
some archaologic specific linaro build and think the sky will fall on
their heads because it's an ancient build for i686 and no longer runs
on current Linux distros.

/
Leif


-Then you can build the image for the SiFive HifiveUnleashed platform:
+Then you can build the edk2 firmware image for RISC-V platforms.

```
+# e.g. For building SiFive Hifive Unleashed platform:
build -a RISCV64 -t GCC5 -p Platform/SiFive/U5SeriesPkg/FreedomU540HiFiveUnleashedBoard/U540.dsc
```

-### EDK2 project
-All changes in edk2 are upstream, however, most of the RISC-V code is in
-edk2-platforms. Therefore you have to check out the branch `riscv-smode-lib` on
-`github.com/changab/edk2-platforms`.
-
-The build architecture which is supported and verified so far is `RISCV64`.
-The latest master of the RISC-V toolchain https://github.com/riscv/riscv-gnu-toolchain
-should work but the latest verified commit is `b468107e701433e1caca3dbc8aef8d40`.
-Toolchain tag is "GCC5" declared in `tools_def.txt`
+## RISC-V OpenSBI Library
+RISC-V [OpenSBI](https://github.com/riscv/opensbi) is the implementation of [RISC-V SBI (Supervisor Binary Interface) specification](https://github.com/riscv/riscv-sbi-doc). For EDK2 UEFI firmware solution, RISC-V OpenSBI is integrated as a library [(submoudule)](Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/opensbi) in EDK2 RISC-V Processor Package. The RISC-V OpenSBI library is built in SEC driver
+without any modifications and provides the interfaces for supervisor mode execution environment to execute privileged operations.

## RISC-V Platform PCD settings
### EDK2 Firmware Volume Settings
@@ -54,9 +53,9 @@ EDK2 Firmware volume related PCDs which declared in platform FDF file.
|PcdRiscVSecFvBase| The base address of SEC Firmware Volume|
|PcdRiscVSecFvSize| The size of SEC Firmware Volume|
|PcdRiscVPeiFvBase| The base address of PEI Firmware Volume|
-|PcdRiscVPeiFvSize| The size of SEC Firmware Volume|
+|PcdRiscVPeiFvSize| The size of PEI Firmware Volume|
|PcdRiscVDxeFvBase| The base address of DXE Firmware Volume|
-|PcdRiscVDxeFvSize| The size of SEC Firmware Volume|
+|PcdRiscVDxeFvSize| The size of DXE Firmware Volume|

### EDK2 EFI Variable Region Settings
The PCD settings regard to EFI Variable
@@ -84,21 +83,24 @@ Below PCDs could be set in platform FDF file.
|--------------|---------|
|PcdHartCount| Number of RISC-V HARTs, the value is processor-implementation specific|
|PcdBootHartId| The ID of RISC-V HART to execute main fimrware code and boot system to OS|
+|PcdBootableHartNumber|The bootable HART number, which is incorporate with RISC-V OpenSBI platform hart_index2id value|

### RISC-V OpenSBI Settings

| **PCD name** |**Usage**|
|--------------|---------|
-|PcdScratchRamBase| The base address of OpenSBI scratch buffer for all RISC-V HARTs|
-|PcdScratchRamSize| The total size of OpenSBI scratch buffer for all RISC-V HARTs|
-|PcdOpenSbiStackSize| The size of initial stack of each RISC-V HART for booting system use OpenSBI|
+|PcdScratchRamBase| The base address of RISC-V OpenSBI scratch buffer for all RISC-V HARTs|
+|PcdScratchRamSize| The total size of RISC-V OpenSBI scratch buffer for all RISC-V HARTs|
+|PcdOpenSbiStackSize| The size of initial stack of each RISC-V HART for booting system use RISC-V OpenSBI|
|PcdTemporaryRamBase| The base address of temporary memory for PEI phase|
|PcdTemporaryRamSize| The temporary memory size for PEI phase|
+|PcdPeiCorePrivilegeMode|The target RISC-V privilege mode for edk2 PEI phase|

## Supported Operating Systems
-Only support to boot to EFI Shell so far.
-
-Porting GRUB2 and Linux EFISTUB is in progress.
+Currently support boot to EFI Shell and Linux kernel.
+Refer to below link for more information,
+https://github.com/riscv/riscv-uefi-edk2-docs

## Known Issues and Limitations
-Only RISC-V RV64 is verified.
+Only RISC-V RV64 is verified on edk2.
+
--
2.25.0




[edk2-plaforms PATCH 1/1] SbsaQemu: Fix CPUID generation in SSDT

Graeme Gregory <graeme@...>
 

The SBSAQEMU_ACPI_ITOA contains a typo that puts invalid characters in
the ASL for any number of CPUs > 10.

Signed-off-by: Graeme Gregory <graeme@...>
---
Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h
index 1a7d9dda2b99..0f79d8a9c3c8 100644
--- a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h
+++ b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h
@@ -51,7 +51,7 @@
#define SBSAQEMU_ACPI_CPU_DEV_NAME { 'C', '0', '0', '0' }

// Macro to convert Integer to Character
-#define SBSAQEMU_ACPI_ITOA(Byte) (0x30 + (Byte > 9 ? (Byte + 1) : Byte))
+#define SBSAQEMU_ACPI_ITOA(Byte) (0x30 + (Byte > 9 ? (Byte + 7) : Byte))

#define SBSAQEMU_ACPI_CPU_HID { \
AML_NAME_OP, AML_NAME_CHAR__, 'H', 'I', 'D', \
--
2.25.1


Re: [edk2-plaforms PATCH 1/3] edk2-platforms: Revise Readme.md

Abner Chang
 

Oops!!! It always happens to me. I will correct it.
Thanks :)

-----Original Message-----
From: Leif Lindholm [mailto:leif@...]
Sent: Thursday, August 27, 2020 9:01 PM
To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>
Cc: devel@edk2.groups.io; Schaefer, Daniel <daniel.schaefer@...>;
Michael D Kinney <michael.d.kinney@...>
Subject: Re: [edk2-plaforms PATCH 1/3] edk2-platforms: Revise Readme.md

You have a typo in the list name inside [] above.
If that's in your static config, please fix it.
If it's not, please make one :)

For this actual patch (unaffected by the above):
Reviewed-by: Leif Lindholm <leif@...>

On Thu, Aug 27, 2020 at 20:03:03 +0800, Abner Chang wrote:
Mention to update submodule under for edk2-platforms. It is for RISC-V
OpenSBI library.

Signed-off-by: Abner Chang <abner.chang@...>
Co-authored-by: Daniel Schaefer <daniel.schaefer@...>

Cc: Leif Lindholm <leif@...>
Cc: Michael D Kinney <michael.d.kinney@...>
---
Readme.md | 1 +
1 file changed, 1 insertion(+)

diff --git a/Readme.md b/Readme.md
index 0675685fb7..d896b2c5fc 100644
--- a/Readme.md
+++ b/Readme.md
@@ -97,6 +97,7 @@ target-specific binutils. These are included with any
prepackaged GCC toolchain
$ git submodule update --init
...
$ git clone https://github.com/tianocore/edk2-platforms.git
+ $ git submodule update --init
...
$ git clone https://github.com/tianocore/edk2-non-osi.git
```
--
2.25.0


Re: [edk2-plaforms PATCH 1/3] edk2-platforms: Revise Readme.md

Leif Lindholm
 

You have a typo in the list name inside [] above.
If that's in your static config, please fix it.
If it's not, please make one :)

For this actual patch (unaffected by the above):
Reviewed-by: Leif Lindholm <leif@...>

On Thu, Aug 27, 2020 at 20:03:03 +0800, Abner Chang wrote:
Mention to update submodule under for edk2-platforms. It is for RISC-V OpenSBI library.

Signed-off-by: Abner Chang <abner.chang@...>
Co-authored-by: Daniel Schaefer <daniel.schaefer@...>

Cc: Leif Lindholm <leif@...>
Cc: Michael D Kinney <michael.d.kinney@...>
---
Readme.md | 1 +
1 file changed, 1 insertion(+)

diff --git a/Readme.md b/Readme.md
index 0675685fb7..d896b2c5fc 100644
--- a/Readme.md
+++ b/Readme.md
@@ -97,6 +97,7 @@ target-specific binutils. These are included with any prepackaged GCC toolchain
$ git submodule update --init
...
$ git clone https://github.com/tianocore/edk2-platforms.git
+ $ git submodule update --init
...
$ git clone https://github.com/tianocore/edk2-non-osi.git
```
--
2.25.0


[edk2-plaforms PATCH 3/3] Platform/U5SeriesPkg: Revise Readme.md

Abner Chang
 


[edk2-plaforms PATCH 2/3] RISC-V/PlatformPkg: Revise Readme.md

Abner Chang
 

Update RISC-V PlatformPkg Readme.md to align with the latest implementation.

Signed-off-by: Abner Chang <abner.chang@...>
Co-authored-by: Daniel Schaefer <daniel.schaefer@...>

Cc: Daniel Schaefer <daniel.schaefer@...>
---
Platform/RISC-V/PlatformPkg/Readme.md | 72 ++++++++++++++-------------
1 file changed, 37 insertions(+), 35 deletions(-)

diff --git a/Platform/RISC-V/PlatformPkg/Readme.md b/Platform/RISC-V/Platfo=
rmPkg/Readme.md
index 2632ebeb28..bd3b823fb4 100644
--- a/Platform/RISC-V/PlatformPkg/Readme.md
+++ b/Platform/RISC-V/PlatformPkg/Readme.md
@@ -1,49 +1,48 @@
-# Introduction=0D
+# Introduction of EDK2 RISC-V Port=0D
=0D
-## EDK2 RISC-V Platform Packages=0D
-RISC-V platform package provides the generic and common modules for RISC-V=
=0D
-platforms. RISC-V platform package could include RiscPlatformPkg.dec to=0D
-use the common drivers, libraries, definitions, PCDs and etc. for the=0D
-platform development.=0D
+## EDK2 RISC-V Project=0D
+The edk2 build architecture which is supported and verified on edk2 code b=
ase for RISC-V platforms is `RISCV64`.=0D
+The toolchain is on RISC-V GitHub (https://github.com/riscv/riscv-gnu-tool=
chain) for building edk2 RISC-V binary.=0D
+The corresponding edk2 Toolchain tag for building RISC-V platform is "GCC5=
" declared in `tools_def.txt`.=0D
=0D
-There are two packages to support RISC-V:=0D
+There are two packages to support RISC-V edk2 platforms:=0D
- `edk2-platforms/Silicon/RISC-V/ProcessorPkg/RiscVProcessorPkg.dec`=0D
- `edk2-platforms/Platform/RISC-V/PlatformPkg/RiscVPlatformPkg.dec`=0D
=0D
-`RiscVPlatformPkg` provides SEC phase and NULL libs.=0D
-`RiscVProcessorPkg` provides many libraries, PEIMs and DXE drivers.=0D
+`RiscVPlatformPkg` currently provides the generic SEC driver for all RISC-=
V platforms, and some platform level libraries.=0D
+`RiscVProcessorPkg` currently provides RISC-V processor related libraries,=
PEI modules, DXE drivers and industrial=0D
+standard header files.=0D
=0D
-### Download the sources ###=0D
+## EDK2 RISC-V Platform Package=0D
+RISC-V platform package provides the common modules for RISC-V platforms. =
RISC-V platform vendors could include=0D
+RiscPlatformPkg.dec to use the common drivers, libraries, definitions, PCD=
s and etc. for the=0D
+RISC-V platform development.=0D
+=0D
+### Download the Source Code ###=0D
```=0D
git clone https://github.com/tianocore/edk2.git=0D
+git clone https://github.com/tianocore/edk2-platforms.git=0D
=0D
-git clone https://github.com/changab/edk2-platforms.git=0D
-# Check out branch: riscv-smode-lib=0D
```=0D
=0D
-To build it, you have to follow the regular steps for EDK2 and additionall=
y set=0D
-an environmen variable to point to your RISC-V toolchain installation,=0D
-including the binary prefixes:=0D
-=0D
+You have to follow the build steps for EDK2 (https://github.com/tianocore/=
tianocore.github.io/wiki/Getting-Started-with-EDK-II)=0D
+and additionally set an environment variable to point to your RISC-V toolc=
hain binaries for building RISC-V=0D
+platforms,=0D
```=0D
+# e.g. If the toolchain binaries are under /riscv-gnu-toolchain-binaries/b=
in=0D
export GCC5_RISCV64_PREFIX=3D/riscv-gnu-toolchain-binaries/bin/riscv64-unk=
nown-elf-=0D
```=0D
=0D
-Then you can build the image for the SiFive HifiveUnleashed platform:=0D
+Then you can build the edk2 firmware image for RISC-V platforms.=0D
=0D
```=0D
+# e.g. For building SiFive Hifive Unleashed platform:=0D
build -a RISCV64 -t GCC5 -p Platform/SiFive/U5SeriesPkg/FreedomU540HiFiveU=
nleashedBoard/U540.dsc=0D
```=0D
=0D
-### EDK2 project=0D
-All changes in edk2 are upstream, however, most of the RISC-V code is in=0D
-edk2-platforms. Therefore you have to check out the branch `riscv-smode-li=
b` on=0D
-`github.com/changab/edk2-platforms`.=0D
-=0D
-The build architecture which is supported and verified so far is `RISCV64`=
.=0D
-The latest master of the RISC-V toolchain https://github.com/riscv/riscv-g=
nu-toolchain=0D
-should work but the latest verified commit is `b468107e701433e1caca3dbc8ae=
f8d40`.=0D
-Toolchain tag is "GCC5" declared in `tools_def.txt`=0D
+## RISC-V OpenSBI Library=0D
+RISC-V [OpenSBI](https://github.com/riscv/opensbi) is the implementation o=
f [RISC-V SBI (Supervisor Binary Interface) specification](https://github.c=
om/riscv/riscv-sbi-doc). For EDK2 UEFI firmware solution, RISC-V OpenSBI is=
integrated as a library [(submoudule)](Silicon/RISC-V/ProcessorPkg/Library=
/RiscVOpensbiLib/opensbi) in EDK2 RISC-V Processor Package. The RISC-V Open=
SBI library is built in SEC driver=0D
+without any modifications and provides the interfaces for supervisor mode =
execution environment to execute privileged operations.=0D
=0D
## RISC-V Platform PCD settings=0D
### EDK2 Firmware Volume Settings=0D
@@ -54,9 +53,9 @@ EDK2 Firmware volume related PCDs which declared in platf=
orm FDF file.
|PcdRiscVSecFvBase| The base address of SEC Firmware Volume|=0D
|PcdRiscVSecFvSize| The size of SEC Firmware Volume|=0D
|PcdRiscVPeiFvBase| The base address of PEI Firmware Volume|=0D
-|PcdRiscVPeiFvSize| The size of SEC Firmware Volume|=0D
+|PcdRiscVPeiFvSize| The size of PEI Firmware Volume|=0D
|PcdRiscVDxeFvBase| The base address of DXE Firmware Volume|=0D
-|PcdRiscVDxeFvSize| The size of SEC Firmware Volume|=0D
+|PcdRiscVDxeFvSize| The size of DXE Firmware Volume|=0D
=0D
### EDK2 EFI Variable Region Settings=0D
The PCD settings regard to EFI Variable=0D
@@ -84,21 +83,24 @@ Below PCDs could be set in platform FDF file.
|--------------|---------|=0D
|PcdHartCount| Number of RISC-V HARTs, the value is processor-implementati=
on specific|=0D
|PcdBootHartId| The ID of RISC-V HART to execute main fimrware code and bo=
ot system to OS|=0D
+|PcdBootableHartNumber|The bootable HART number, which is incorporate with=
RISC-V OpenSBI platform hart_index2id value|=0D
=0D
### RISC-V OpenSBI Settings=0D
=0D
| **PCD name** |**Usage**|=0D
|--------------|---------|=0D
-|PcdScratchRamBase| The base address of OpenSBI scratch buffer for all RIS=
C-V HARTs|=0D
-|PcdScratchRamSize| The total size of OpenSBI scratch buffer for all RISC-=
V HARTs|=0D
-|PcdOpenSbiStackSize| The size of initial stack of each RISC-V HART for bo=
oting system use OpenSBI|=0D
+|PcdScratchRamBase| The base address of RISC-V OpenSBI scratch buffer for =
all RISC-V HARTs|=0D
+|PcdScratchRamSize| The total size of RISC-V OpenSBI scratch buffer for al=
l RISC-V HARTs|=0D
+|PcdOpenSbiStackSize| The size of initial stack of each RISC-V HART for bo=
oting system use RISC-V OpenSBI|=0D
|PcdTemporaryRamBase| The base address of temporary memory for PEI phase|=
=0D
|PcdTemporaryRamSize| The temporary memory size for PEI phase|=0D
+|PcdPeiCorePrivilegeMode|The target RISC-V privilege mode for edk2 PEI pha=
se|=0D
=0D
## Supported Operating Systems=0D
-Only support to boot to EFI Shell so far.=0D
-=0D
-Porting GRUB2 and Linux EFISTUB is in progress.=0D
+Currently support boot to EFI Shell and Linux kernel.=0D
+Refer to below link for more information,=0D
+https://github.com/riscv/riscv-uefi-edk2-docs=0D
=0D
## Known Issues and Limitations=0D
-Only RISC-V RV64 is verified.=0D
+Only RISC-V RV64 is verified on edk2.=0D
+=0D
--=20
2.25.0


[edk2-plaforms PATCH 1/3] edk2-platforms: Revise Readme.md

Abner Chang
 

Mention to update submodule under for edk2-platforms. It is for RISC-V Open=
SBI library.

Signed-off-by: Abner Chang <abner.chang@...>
Co-authored-by: Daniel Schaefer <daniel.schaefer@...>

Cc: Leif Lindholm <leif@...>
Cc: Michael D Kinney <michael.d.kinney@...>
---
Readme.md | 1 +
1 file changed, 1 insertion(+)

diff --git a/Readme.md b/Readme.md
index 0675685fb7..d896b2c5fc 100644
--- a/Readme.md
+++ b/Readme.md
@@ -97,6 +97,7 @@ target-specific binutils. These are included with any pre=
packaged GCC toolchain
$ git submodule update --init=0D
...=0D
$ git clone https://github.com/tianocore/edk2-platforms.git=0D
+ $ git submodule update --init=0D
...=0D
$ git clone https://github.com/tianocore/edk2-non-osi.git=0D
```=0D
--=20
2.25.0


[edk2-plaforms PATCH 0/3] Revise Readme for RISC-V updates

Abner Chang
 

Update below Readme.md,=0D
- edk2-platforms/Readme.md=0D
- Platform/RISC-V/PlatformPkg/Readme.md=0D
- Platform/SiFive/U5SeriesPkg/Readme.md=0D
=0D
Abner Chang (3):=0D
edk2-platforms: Revise Readme.md=0D
RISC-V/PlatformPkg: Revise Readme.md=0D
Platform/U5SeriesPkg: Revise Readme.md=0D
=0D
Platform/RISC-V/PlatformPkg/Readme.md | 72 +++++++++--------=0D
Platform/SiFive/U5SeriesPkg/Readme.md | 112 +++++++++++++-------------=0D
Readme.md | 1 +=0D
3 files changed, 96 insertions(+), 89 deletions(-)=0D
=0D
-- =0D
2.25.0=0D
=0D


Re: [PATCH v3 edk2-platforms 5/8] SbsaQemu: AcpiDxe: Create MADT table at runtime

Graeme Gregory <graeme@...>
 

On Thu, Aug 27, 2020 at 08:26:21AM +0530, Tanmay Jagdale wrote:
Hi Graeme,


On Thu, 27 Aug 2020 at 04:18, Graeme Gregory <[1]graeme@...> wrote:

On Wed, Aug 26, 2020 at 04:35:22PM +0100, Graeme Gregory wrote:
> On Tue, Aug 25, 2020 at 07:09:55PM +0530, Tanmay Jagdale wrote:
> > - Add support to create MADT table at runtime.
> > - Included a macro for GIC Redistributor structure initialisation.
> >
> > Signed-off-by: Tanmay Jagdale <[2]tanmay.jagdale@...>
> > ---
> >  Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf | 
20 ++-
> >  Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h     | 
15 ++
> >  Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c   |
156 ++++++++++++++++++++
> >  3 files changed, 190 insertions(+), 1 deletion(-)
> >
> > diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/
SbsaQemuAcpiDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/
SbsaQemuAcpiDxe.inf
> > index 3795a7e11639..8125e8ba7553 100644
> > --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> > +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> > @@ -41,9 +41,27 @@ [LibraryClasses]
> >    UefiRuntimeServicesTableLib
> > 
> >  [Pcd]
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile
> >    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCoreCount
> >    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdClusterCount
> >    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdDeviceTreeBaseAddress
> > 
> >  [Depex]
> > -  TRUE
> > +  gEfiAcpiTableProtocolGuid                       ## CONSUMES
> > +
> > +[Guids]
> > +  gEdkiiPlatformHasAcpiGuid
> > +
> > +[Protocols]
> > +  gEfiAcpiTableProtocolGuid                       ## CONSUMES
> > +
> > +[FixedPcd]
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision
> > +  gArmTokenSpaceGuid.PcdGicDistributorBase
> > +  gArmTokenSpaceGuid.PcdGicRedistributorsBase
> > +
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorId
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemId
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemTableId
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision
> > diff --git a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/
SbsaQemuAcpi.h b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/
SbsaQemuAcpi.h
> > index eac195b0585c..7a9a0061675f 100644
> > --- a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h
> > +++ b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h
> > @@ -22,6 +22,21 @@
> >      FixedPcdGet32 (PcdAcpiDefaultCreatorRevision)/* UINT32 
CreatorRevision */ \
> >    }
> > 
> > +// Defines for MADT
> > +#define SBSAQEMU_MADT_GIC_VBASE          0x2c020000
> > +#define SBSAQEMU_MADT_GIC_HBASE          0x2c010000
> > +#define SBSAQEMU_MADT_GIC_PMU_IRQ        23
> > +#define SBSAQEMU_MADT_GICR_SIZE          0x4000000
> > +
> > +// Macro for MADT GIC Redistributor Structure
> > +#define SBSAQEMU_MADT_GICR_INIT() {                                   
        \
> > +   EFI_ACPI_6_0_GICR,                        /* Type */               
        \
> > +   sizeof (EFI_ACPI_6_0_GICR_STRUCTURE),     /* Length */             
        \
> > +   EFI_ACPI_RESERVED_WORD,                   /* Reserved */           
        \
> > +   FixedPcdGet32 (PcdGicRedistributorsBase), /*
DiscoveryRangeBaseAddress */   \
> > +   SBSAQEMU_MADT_GICR_SIZE                   /* DiscoveryRangeLength *
/        \
> > +   }
> > +
> >  #define SBSAQEMU_UART0_BASE              0x60000000
> > 
> >  #define SBSAQEMU_PCI_SEG0_CONFIG_BASE    0xf0000000
> > diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/
SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/
SbsaQemuAcpiDxe.c
> > index 75abdae3b8ce..16cb4e904e6f 100644
> > --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> > +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> > @@ -6,11 +6,17 @@
> >  *  SPDX-License-Identifier: BSD-2-Clause-Patent
> >  *
> >  **/
> > +#include <IndustryStandard/Acpi.h>
> > +#include <IndustryStandard/SbsaQemuAcpi.h>
> > +#include <Library/AcpiLib.h>
> > +#include <Library/BaseMemoryLib.h>
> >  #include <Library/DebugLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> >  #include <Library/PcdLib.h>
> >  #include <Library/UefiBootServicesTableLib.h>
> >  #include <Library/UefiDriverEntryPoint.h>
> >  #include <Library/UefiLib.h>
> > +#include <Protocol/AcpiTable.h>
> >  #include <Protocol/FdtClient.h>
> >  #include <libfdt.h>
> > 
> > @@ -61,6 +67,137 @@ CountCpusFromFdt (
> >    ASSERT_RETURN_ERROR (PcdStatus);
> >  }
> > 
> > +/*
> > + * A Function to Compute the ACPI Table Checksum
> > + */
> > +VOID
> > +AcpiPlatformChecksum (
> > +  IN UINT8      *Buffer,
> > +  IN UINTN      Size
> > +  )
> > +{
> > +  UINTN ChecksumOffset;
> > +
> > +  ChecksumOffset = OFFSET_OF (EFI_ACPI_DESCRIPTION_HEADER, Checksum);
> > +
> > +  // Set checksum field to 0 since it is used as part of the
calculation
> > +  Buffer[ChecksumOffset] = 0;
> > +
> > +  Buffer[ChecksumOffset] = CalculateCheckSum8(Buffer, Size);
> > +}
> > +
> > +/*
> > + * A function that add the MADT ACPI table.
> > +  IN EFI_ACPI_COMMON_HEADER    *CurrentTable
> > + */
> > +EFI_STATUS
> > +AddMadtTable (
> > +  IN EFI_ACPI_TABLE_PROTOCOL   *AcpiTable
> > +  )
> > +{
> > +  EFI_STATUS            Status;
> > +  UINTN                 TableHandle;
> > +  UINT32                TableSize;
> > +  EFI_PHYSICAL_ADDRESS  PageAddress;
> > +  UINT8                 *New;
> > +  UINT32                NumCores;
> > +
> > +  // Initialize MADT ACPI Header
> > +  EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER Header = {
> > +     SBSAQEMU_ACPI_HEADER
(EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,
> > +                         
 EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER,
> > +                         
 EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_REVISION),
> > +      0, 0 };
> > +
> > +  // Initialize GICC Structure
> > +  EFI_ACPI_6_0_GIC_STRUCTURE Gicc = EFI_ACPI_6_0_GICC_STRUCTURE_INIT (
> > +    0,                                     /* GicID */
> > +    0,                                     /* AcpiCpuUid */
> > +    0,                                     /* Mpidr */
> > +    EFI_ACPI_6_0_GIC_ENABLED,              /* Flags */
> > +    SBSAQEMU_MADT_GIC_PMU_IRQ,             /* PMU Irq */
> > +    FixedPcdGet32 (PcdGicDistributorBase), /* PhysicalBaseAddress */
> > +    SBSAQEMU_MADT_GIC_VBASE,               /* GicVBase */
> > +    SBSAQEMU_MADT_GIC_HBASE,               /* GicHBase */
> > +    25,                                    /* GsivId */
> > +    0,                                     /* GicRBase */
> > +    0                                      /* Efficiency */
> > +    );
> > +
> > +  // Initialize GIC Distributor Structure
> > +  EFI_ACPI_6_0_GIC_DISTRIBUTOR_STRUCTURE Gicd =
> > +    EFI_ACPI_6_0_GIC_DISTRIBUTOR_INIT (
> > +      0,
> > +      FixedPcdGet32 (PcdGicDistributorBase),
> > +      0,
> > +      3 /* GicVersion */
> > +    );
> > +
> > + // Initialize GIC Redistributor Structure
> > +  EFI_ACPI_6_0_GICR_STRUCTURE Gicr = SBSAQEMU_MADT_GICR_INIT();
> > +
> > +  // Get CoreCount which was determined eariler after parsing device
tree
> > +  NumCores = PcdGet32 (PcdCoreCount);
> > +
> > +  // Calculate the new table size based on the number of cores
> > +  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);
> > +
> > +  Status = gBS->AllocatePages (
> > +                  AllocateAnyPages,
> > +                  EfiACPIReclaimMemory,
> > +                  EFI_SIZE_TO_PAGES (TableSize),
> > +                  &PageAddress
> > +                  );
> > +  if (EFI_ERROR(Status)) {
> > +    DEBUG ((DEBUG_ERROR, "Failed to allocate pages for MADT table\
n"));
> > +    return EFI_OUT_OF_RESOURCES;
> > +  }
> > +
> > +  New = (UINT8 *)(UINTN) PageAddress;
> > +  ZeroMem (New, TableSize);
> > +
> > +  // Add the  ACPI Description table header
> > +  CopyMem (New, &Header, sizeof
(EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER));
> > +  ((EFI_ACPI_DESCRIPTION_HEADER*) New)->Length = TableSize;
> > +  New += sizeof (EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER);
> > +
> > +  // Add new GICC structures for the Cores
> > +  for (NumCores = 0; NumCores < PcdGet32 (PcdCoreCount); NumCores++) {
> > +    EFI_ACPI_6_0_GIC_STRUCTURE *GiccPtr;
> > +
> > +    CopyMem (New, &Gicc, sizeof (EFI_ACPI_6_0_GIC_STRUCTURE));
> > +    GiccPtr = (EFI_ACPI_6_0_GIC_STRUCTURE *) New;
> > +    GiccPtr->AcpiProcessorUid = NumCores;
> > +    GiccPtr->MPIDR = NumCores;
>
> This does not seem to be quite correct, if I dump the MPIDRs from ARM-TF
when
> booting with 12 cpus this is what I get.
>
> NOTICE:  MPIDR 0
> NOTICE:  MPIDR 1
> NOTICE:  MPIDR 2
> NOTICE:  MPIDR 3
> NOTICE:  MPIDR 4
> NOTICE:  MPIDR 5
> NOTICE:  MPIDR 6
> NOTICE:  MPIDR 7
> NOTICE:  MPIDR 100
> NOTICE:  MPIDR 101
> NOTICE:  MPIDR 102
> NOTICE:  MPIDR 103
>
> I think this will make PSCI operations from CPU8 onwards fail.
>

I can confirm this is wrong

I did a quick hack

GiccPtr->MPIDR = ((NumCores / 8) << 8) | (NumCores % 8);


Thanks for the fix.
 

and I can boot 128 cores (MPIDR also needs fixed in SSDT I think)

[   12.637579] GICv3: CPU127: found redistributor f07 region
0:0x0000000041060000
[   12.640185] CPU127: Booted secondary processor 0x0000000f07
[0x411fd070]
[   12.676961] smp: Brought up 1 node, 128 CPUs

Considering this patch is in the works

[3]https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg06223.html

which add MPIDR to the DT, i think you will when that is accepted be
able to use the MPIDR from there and then you are protected for the
CPU topology changing.


So shall I push an interim patch that uses the aforementioned formula
to derive the MPIDR ? Or wait for the qemu patch to get merged and
then read MPIDR directly from there ?
IMHO wait for to see progress on Leif's patch, then issue an update to
read the MPIDR from the FDT. Obviously you can start the
development/review process for that before patch lands.

The formula only works because I happen to know what qemu generates today.
This might change with a qemu change in future though.

Graeme


Re: [PATCH V2 2/2] BaseTools: Factorize GCC flags

PierreGondois
 

Hello Laszlo,
I thought Leif wanted to revert this modification. Should I apply your requested changes, or should this patch be reverted?

Regards,
Pierre

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Wednesday, August 26, 2020 5:43 PM
To: Pierre Gondois <Pierre.Gondois@...>
Cc: devel@edk2.groups.io; bob.c.feng@...; liming.gao@...; Tomas Pilar <Tomas.Pilar@...>; nd <nd@...>; Leif Lindholm (Nuvia address) <leif@...>; Ard Biesheuvel <Ard.Biesheuvel@...>
Subject: Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

On 07/22/20 13:03, Laszlo Ersek wrote:
Hi Pierre,

On 07/07/20 10:35, PierreGondois wrote:
From: Pierre Gondois <pierre.gondois@...>

GCC48_ALL_CC_FLAGS has no dependency on GCC_ALL_CC_FLAGS.
By definition, there should be such dependency.

The outcomes of this patch is that GCC48_ALL_CC_FLAGS and other
dependent configurations will inherit from the additional "-Os" flag.
The "-Os" flag optimizes a build in size, not breaking any build. In
a gcc command line, the last optimization flag has precedence. This
means that this "-Os" flag will be overriden by a more specific
optimization configuration, provided that this more specific flag is
appended at the end of the CC_FLAGS.

Signed-off-by: Pierre Gondois <pierre.gondois@...>
Suggested-by: Tomas Pilar <Tomas.Pilar@...>
---

The changes can be seen at:
https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2

Notes:
v2:
- Make GCC48_ALL_CC_FLAGS dependent on
GCC_ALL_CC_FLAGS. [Tomas]

BaseTools/Conf/tools_def.template | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Conf/tools_def.template
b/BaseTools/Conf/tools_def.template
index
397b011ba38f97f81f314f8641ac8bb95d5a2197..a1fd27b1adba8769949b7d628d7
fbed49fe24267 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -1952,7 +1952,7 @@ DEFINE GCC_RISCV64_RC_FLAGS = -I binary -O elf64-littleriscv -B riscv
# GCC Build Flag for included header file list generation
DEFINE GCC_DEPS_FLAGS = -MMD -MF $@.deps

-DEFINE GCC48_ALL_CC_FLAGS = -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings
+DEFINE GCC48_ALL_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -ffunction-sections -fdata-sections -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings
DEFINE GCC48_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20
DEFINE GCC48_IA32_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m32 -march=i586 -malign-double -fno-stack-protector -D EFI32 -fno-asynchronous-unwind-tables -Wno-address
DEFINE GCC48_X64_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables -Wno-address
As the commit message states, this change makes GCC48_ALL_CC_FLAGS inherit "-Os".

It is true that all the NOOPT_GCC flags override "-Os" with "-O0":

NOOPT_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -O0
NOOPT_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -O0
NOOPT_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -O0
NOOPT_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -O0
NOOPT_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -O0
NOOPT_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -O0
NOOPT_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -O0
NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
NOOPT_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -O0
NOOPT_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -O0
NOOPT_GCC5_ARM_CC_FLAGS = DEF(GCC5_ARM_CC_FLAGS) -O0
NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0

However, *some* of the DEBUG and RELEASE flags now have two "-Os" flags:

DEBUG_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os
RELEASE_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os -Wno-unused-but-set-variable
DEBUG_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os
RELEASE_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os -Wno-unused-but-set-variable
DEBUG_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os
RELEASE_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os -Wno-unused-but-set-variable -Wno-unused-const-variable
DEBUG_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os
RELEASE_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os -Wno-unused-but-set-variable -Wno-unused-const-variable
DEBUG_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os
RELEASE_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os -Wno-unused-but-set-variable -Wno-unused-const-variable
DEBUG_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os
RELEASE_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os -Wno-unused-but-set-variable -Wno-unused-const-variable

(The ARM and AARCH64 DEBUG/RELEASE GCC options don't seem to be
affected, as they have relied on inherited -- not open-coded -- "-Os"
options from much earlier. So now they do not suffer from this
duplication.)

The point of this patch was a kind of "normalization", so I think the work isn't complete until the duplication is undone, i.e., the explicit "-Os" flag is removed from the last twelve defines.

Can you submit a follow-up patch please?
I have not received an answer, and I'm not aware of a follow-up patch being on the list; so now I've filed:

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

Thanks
Laszlo


Re: [PATCH v2 1/4] Platform/RaspberryPi4: Add a basic thermal zone

Ard Biesheuvel
 

On 8/20/20 4:41 PM, Jeremy Linton wrote:
Hi,
On 8/20/20 2:59 AM, Ard Biesheuvel wrote:
On 8/20/20 6:42 AM, Jeremy Linton wrote:
Rather than exporting the temp sensor or mailbox
in ACPI land we can wrap them in AML and use the default
ACPI drivers provided by the OS. This enables the use of
"sensors" in linux to report the SOC temp.

This commit also adds a basic passive cooling ACPI thermalzone
with trip points for passive cooling (throttling) handled
by the vc firmware, hibernate and critical shutdown. The
vc apparently kicks in at ~80C, so the hibernate and critical
set points are set at +5 and +10 of that. In the future
CPPC should be able to monitor the thermal throttling.

Cc: Leif Lindholm <leif@...>
Cc: Pete Batard <pete@...>
Cc: Andrei Warkentin <awarkentin@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>
Signed-off-by: Jeremy Linton <jeremy.linton@...>
Reviewed-by: Pete Batard <@pbatard>
This looks ok to me but I did not receive patch #4 or the cover letter.
I appear to have only CCed the groups.io ML on the cover letter and 4th patch (which is just whitespace).
Can you find them them there? If not I will just resend the whole series as v3.
Mind resending them? Grabbing and applying patches from the web UI is not really feasible with the groups.io interface.




---
  Platform/RaspberryPi/AcpiTables/Dsdt.asl           | 31 ++++++++++++++++++++++
  .../Bcm27xx/Include/IndustryStandard/Bcm2711.h     |  2 ++
  2 files changed, 33 insertions(+)

diff --git a/Platform/RaspberryPi/AcpiTables/Dsdt.asl b/Platform/RaspberryPi/AcpiTables/Dsdt.asl
index 353af2d876..73067aefd2 100644
--- a/Platform/RaspberryPi/AcpiTables/Dsdt.asl
+++ b/Platform/RaspberryPi/AcpiTables/Dsdt.asl
@@ -252,6 +252,37 @@ DefinitionBlock ("Dsdt.aml", "DSDT", 5, "RPIFDN", "RPI", 2)
          }
        })
      }
+
+    // Define a simple thermal zone. The idea here is we compute the SOC temp
+    // via a register we can read, and give it to the OS. This enables basic
+    // reports from the "sensors" utility, and the OS can then poll and take
+    // actions if that temp exceeds any of the given thresholds.
+    Device (EC0)
+    {
+      Name (_HID, EISAID ("PNP0C06"))
+      Name (_CCA, 0x0)
+
+      // all temps in are tenths of K (aka 2732 is the min temps in Linux (aka 0C))
+      ThermalZone (TZ0) {
+        Method (_TMP, 0, Serialized) {
+          OperationRegion (TEMS, SystemMemory, THERM_SENSOR, 0x8)
+          Field (TEMS, DWordAcc, NoLock, Preserve) {
+            TMPS, 32
+          }
+          return (((419949 - ((TMPS & 0x3ff) * 487)) / 100) + 2732);
+        }
+        Method (_SCP, 3) { }               // receive cooling policy from OS
+
+        Method (_CRT) { Return (3632) }    // (90C) Critical temp point (immediate power-off)
+        Method (_HOT) { Return (3582) }    // (85C) HOT state where OS should hibernate
+        Method (_PSV) { Return (3532) }    // (80C) Passive cooling (CPU throttling) trip point
+
+        // SSDT inserts _AC0/_AL0 @60C here, if a FAN is configured
+
+        Name (_TZP, 10)                   //The OSPM must poll this device every 1 seconds
+        Name (_PSL, Package () { \_SB_.CPU0, \_SB_.CPU1, \_SB_.CPU2, \_SB_.CPU3 })
+      }
+    }
  #endif
    }
diff --git a/Silicon/Broadcom/Bcm27xx/Include/IndustryStandard/Bcm2711.h b/Silicon/Broadcom/Bcm27xx/Include/IndustryStandard/Bcm2711.h
index e9c81cafa1..86906b2438 100644
--- a/Silicon/Broadcom/Bcm27xx/Include/IndustryStandard/Bcm2711.h
+++ b/Silicon/Broadcom/Bcm27xx/Include/IndustryStandard/Bcm2711.h
@@ -86,4 +86,6 @@
  #define GENET_BASE_ADDRESS         FixedPcdGet64 (PcdBcmGenetRegistersAddress)
  #define GENET_LENGTH               0x00010000
+#define THERM_SENSOR               0xfd5d2200
+
  #endif /* BCM2711_H__ */


Re: [edk2-platform][PATCH v1 1/1] Platforms/RaspberryPi: Fix build error in DisplayDxe

Ard Biesheuvel
 

On 8/27/20 1:20 AM, Samer El-Haj-Mahmoud wrote:
Commit 0c2af04985f0bf152ac3edc70d9c6d9fe884cdcb added mDriverBinding
extern module global, but did not remove the STATIC declaration, which
caused the build to break. Fix the build error by removing STATIC for
that module global variable.
Cc: Leif Lindholm <leif@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>
Cc: Pete Batard <pete@...>
Cc: Andrei Warkentin <awarkentin@...>
Signed-off-by: Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@...>
Pushed as 0cb34c0de123..5955ac2f10af

Thanks all

---
Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.c b/Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.c
index ae4b2735820c..3eba98e5aa87 100644
--- a/Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.c
+++ b/Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.c
@@ -87,7 +87,7 @@ DisplayBlt (
IN UINTN Delta OPTIONAL
);
-STATIC EFI_DRIVER_BINDING_PROTOCOL mDriverBinding = {
+EFI_DRIVER_BINDING_PROTOCOL mDriverBinding = {
DriverSupported,
DriverStart,
DriverStop,


Re: [PATCH 0/2] OvmfPkg: fix race conditions in VCPU hotplug (for edk2-stable202008)

Ard Biesheuvel
 

On 8/27/20 12:21 AM, Laszlo Ersek wrote:
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2929
Repo: https://pagure.io/lersek/edk2.git
Branch: cpu_hotplug_races_bz_2929
Proposing these bugfixes for edk2-stable202008. We should either include
them in the release, or revert commit 5ba203b54e59
("OvmfPkg/SmmControl2Dxe: negotiate ICH9_LPC_SMI_F_CPU_HOTPLUG",
2020-08-24). Please see the BZ for more details.
Cc: Ard Biesheuvel <ard.biesheuvel@...>
Cc: Igor Mammedov <imammedo@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Philippe Mathieu-Daudé <philmd@...>

For the series,

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@...>

I have no objections to incorporating these changes into the upcoming stable tag.


Re: [edk2-platform][PATCH v1 1/1] Platforms/RaspberryPi: Fix build error in DisplayDxe

Andrei Warkentin
 

Reviewed-by: Andrei Warkentin <andrey.warkentin@...>


From: Pete Batard <pete@...>
Sent: Wednesday, August 26, 2020 6:23 PM
To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Leif Lindholm <leif@...>; Ard Biesheuvel <ard.biesheuvel@...>; Andrei Warkentin <awarkentin@...>
Subject: Re: [edk2-platform][PATCH v1 1/1] Platforms/RaspberryPi: Fix build error in DisplayDxe
 
On 2020.08.27 00:20, Samer El-Haj-Mahmoud wrote:
> Commit 0c2af04985f0bf152ac3edc70d9c6d9fe884cdcb added mDriverBinding
> extern module global, but did not remove the STATIC declaration, which
> caused the build to break. Fix the build error by removing STATIC for
> that module global variable.
>
> Cc: Leif Lindholm <leif@...>
> Cc: Ard Biesheuvel <ard.biesheuvel@...>
> Cc: Pete Batard <pete@...>
> Cc: Andrei Warkentin <awarkentin@...>
> Signed-off-by: Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@...>
> ---
>   Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.c b/Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.c
> index ae4b2735820c..3eba98e5aa87 100644
> --- a/Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.c
> +++ b/Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.c
> @@ -87,7 +87,7 @@ DisplayBlt (
>     IN  UINTN                                   Delta         OPTIONAL
>     );
>  
> -STATIC EFI_DRIVER_BINDING_PROTOCOL mDriverBinding = {
> +EFI_DRIVER_BINDING_PROTOCOL mDriverBinding = {
>     DriverSupported,
>     DriverStart,
>     DriverStop,
>

Reviewed-by: Pete Batard <pete@...>


Re: [PATCH v3 edk2-platforms 5/8] SbsaQemu: AcpiDxe: Create MADT table at runtime

Tanmay Jagdale
 

Hi Graeme,


On Thu, 27 Aug 2020 at 04:18, Graeme Gregory <graeme@...> wrote:
On Wed, Aug 26, 2020 at 04:35:22PM +0100, Graeme Gregory wrote:
> On Tue, Aug 25, 2020 at 07:09:55PM +0530, Tanmay Jagdale wrote:
> > - Add support to create MADT table at runtime.
> > - Included a macro for GIC Redistributor structure initialisation.
> >
> > Signed-off-by: Tanmay Jagdale <tanmay.jagdale@...>
> > ---
> >  Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf |  20 ++-
> >  Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h     |  15 ++
> >  Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c   | 156 ++++++++++++++++++++
> >  3 files changed, 190 insertions(+), 1 deletion(-)
> >
> > diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> > index 3795a7e11639..8125e8ba7553 100644
> > --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> > +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> > @@ -41,9 +41,27 @@ [LibraryClasses]
> >    UefiRuntimeServicesTableLib
> > 
> >  [Pcd]
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile
> >    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCoreCount
> >    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdClusterCount
> >    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdDeviceTreeBaseAddress
> > 
> >  [Depex]
> > -  TRUE
> > +  gEfiAcpiTableProtocolGuid                       ## CONSUMES
> > +
> > +[Guids]
> > +  gEdkiiPlatformHasAcpiGuid
> > +
> > +[Protocols]
> > +  gEfiAcpiTableProtocolGuid                       ## CONSUMES
> > +
> > +[FixedPcd]
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision
> > +  gArmTokenSpaceGuid.PcdGicDistributorBase
> > +  gArmTokenSpaceGuid.PcdGicRedistributorsBase
> > +
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorId
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemId
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemTableId
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision
> > diff --git a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h
> > index eac195b0585c..7a9a0061675f 100644
> > --- a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h
> > +++ b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h
> > @@ -22,6 +22,21 @@
> >      FixedPcdGet32 (PcdAcpiDefaultCreatorRevision)/* UINT32  CreatorRevision */ \
> >    }
> > 
> > +// Defines for MADT
> > +#define SBSAQEMU_MADT_GIC_VBASE          0x2c020000
> > +#define SBSAQEMU_MADT_GIC_HBASE          0x2c010000
> > +#define SBSAQEMU_MADT_GIC_PMU_IRQ        23
> > +#define SBSAQEMU_MADT_GICR_SIZE          0x4000000
> > +
> > +// Macro for MADT GIC Redistributor Structure
> > +#define SBSAQEMU_MADT_GICR_INIT() {                                            \
> > +   EFI_ACPI_6_0_GICR,                        /* Type */                        \
> > +   sizeof (EFI_ACPI_6_0_GICR_STRUCTURE),     /* Length */                      \
> > +   EFI_ACPI_RESERVED_WORD,                   /* Reserved */                    \
> > +   FixedPcdGet32 (PcdGicRedistributorsBase), /* DiscoveryRangeBaseAddress */   \
> > +   SBSAQEMU_MADT_GICR_SIZE                   /* DiscoveryRangeLength */        \
> > +   }
> > +
> >  #define SBSAQEMU_UART0_BASE              0x60000000
> > 
> >  #define SBSAQEMU_PCI_SEG0_CONFIG_BASE    0xf0000000
> > diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> > index 75abdae3b8ce..16cb4e904e6f 100644
> > --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> > +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> > @@ -6,11 +6,17 @@
> >  *  SPDX-License-Identifier: BSD-2-Clause-Patent
> >  *
> >  **/
> > +#include <IndustryStandard/Acpi.h>
> > +#include <IndustryStandard/SbsaQemuAcpi.h>
> > +#include <Library/AcpiLib.h>
> > +#include <Library/BaseMemoryLib.h>
> >  #include <Library/DebugLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> >  #include <Library/PcdLib.h>
> >  #include <Library/UefiBootServicesTableLib.h>
> >  #include <Library/UefiDriverEntryPoint.h>
> >  #include <Library/UefiLib.h>
> > +#include <Protocol/AcpiTable.h>
> >  #include <Protocol/FdtClient.h>
> >  #include <libfdt.h>
> > 
> > @@ -61,6 +67,137 @@ CountCpusFromFdt (
> >    ASSERT_RETURN_ERROR (PcdStatus);
> >  }
> > 
> > +/*
> > + * A Function to Compute the ACPI Table Checksum
> > + */
> > +VOID
> > +AcpiPlatformChecksum (
> > +  IN UINT8      *Buffer,
> > +  IN UINTN      Size
> > +  )
> > +{
> > +  UINTN ChecksumOffset;
> > +
> > +  ChecksumOffset = OFFSET_OF (EFI_ACPI_DESCRIPTION_HEADER, Checksum);
> > +
> > +  // Set checksum field to 0 since it is used as part of the calculation
> > +  Buffer[ChecksumOffset] = 0;
> > +
> > +  Buffer[ChecksumOffset] = CalculateCheckSum8(Buffer, Size);
> > +}
> > +
> > +/*
> > + * A function that add the MADT ACPI table.
> > +  IN EFI_ACPI_COMMON_HEADER    *CurrentTable
> > + */
> > +EFI_STATUS
> > +AddMadtTable (
> > +  IN EFI_ACPI_TABLE_PROTOCOL   *AcpiTable
> > +  )
> > +{
> > +  EFI_STATUS            Status;
> > +  UINTN                 TableHandle;
> > +  UINT32                TableSize;
> > +  EFI_PHYSICAL_ADDRESS  PageAddress;
> > +  UINT8                 *New;
> > +  UINT32                NumCores;
> > +
> > +  // Initialize MADT ACPI Header
> > +  EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER Header = {
> > +     SBSAQEMU_ACPI_HEADER (EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,
> > +                           EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER,
> > +                           EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_REVISION),
> > +      0, 0 };
> > +
> > +  // Initialize GICC Structure
> > +  EFI_ACPI_6_0_GIC_STRUCTURE Gicc = EFI_ACPI_6_0_GICC_STRUCTURE_INIT (
> > +    0,                                     /* GicID */
> > +    0,                                     /* AcpiCpuUid */
> > +    0,                                     /* Mpidr */
> > +    EFI_ACPI_6_0_GIC_ENABLED,              /* Flags */
> > +    SBSAQEMU_MADT_GIC_PMU_IRQ,             /* PMU Irq */
> > +    FixedPcdGet32 (PcdGicDistributorBase), /* PhysicalBaseAddress */
> > +    SBSAQEMU_MADT_GIC_VBASE,               /* GicVBase */
> > +    SBSAQEMU_MADT_GIC_HBASE,               /* GicHBase */
> > +    25,                                    /* GsivId */
> > +    0,                                     /* GicRBase */
> > +    0                                      /* Efficiency */
> > +    );
> > +
> > +  // Initialize GIC Distributor Structure
> > +  EFI_ACPI_6_0_GIC_DISTRIBUTOR_STRUCTURE Gicd =
> > +    EFI_ACPI_6_0_GIC_DISTRIBUTOR_INIT (
> > +      0,
> > +      FixedPcdGet32 (PcdGicDistributorBase),
> > +      0,
> > +      3 /* GicVersion */
> > +    );
> > +
> > + // Initialize GIC Redistributor Structure
> > +  EFI_ACPI_6_0_GICR_STRUCTURE Gicr = SBSAQEMU_MADT_GICR_INIT();
> > +
> > +  // Get CoreCount which was determined eariler after parsing device tree
> > +  NumCores = PcdGet32 (PcdCoreCount);
> > +
> > +  // Calculate the new table size based on the number of cores
> > +  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);
> > +
> > +  Status = gBS->AllocatePages (
> > +                  AllocateAnyPages,
> > +                  EfiACPIReclaimMemory,
> > +                  EFI_SIZE_TO_PAGES (TableSize),
> > +                  &PageAddress
> > +                  );
> > +  if (EFI_ERROR(Status)) {
> > +    DEBUG ((DEBUG_ERROR, "Failed to allocate pages for MADT table\n"));
> > +    return EFI_OUT_OF_RESOURCES;
> > +  }
> > +
> > +  New = (UINT8 *)(UINTN) PageAddress;
> > +  ZeroMem (New, TableSize);
> > +
> > +  // Add the  ACPI Description table header
> > +  CopyMem (New, &Header, sizeof (EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER));
> > +  ((EFI_ACPI_DESCRIPTION_HEADER*) New)->Length = TableSize;
> > +  New += sizeof (EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER);
> > +
> > +  // Add new GICC structures for the Cores
> > +  for (NumCores = 0; NumCores < PcdGet32 (PcdCoreCount); NumCores++) {
> > +    EFI_ACPI_6_0_GIC_STRUCTURE *GiccPtr;
> > +
> > +    CopyMem (New, &Gicc, sizeof (EFI_ACPI_6_0_GIC_STRUCTURE));
> > +    GiccPtr = (EFI_ACPI_6_0_GIC_STRUCTURE *) New;
> > +    GiccPtr->AcpiProcessorUid = NumCores;
> > +    GiccPtr->MPIDR = NumCores;
>
> This does not seem to be quite correct, if I dump the MPIDRs from ARM-TF when
> booting with 12 cpus this is what I get.
>
> NOTICE:  MPIDR 0
> NOTICE:  MPIDR 1
> NOTICE:  MPIDR 2
> NOTICE:  MPIDR 3
> NOTICE:  MPIDR 4
> NOTICE:  MPIDR 5
> NOTICE:  MPIDR 6
> NOTICE:  MPIDR 7
> NOTICE:  MPIDR 100
> NOTICE:  MPIDR 101
> NOTICE:  MPIDR 102
> NOTICE:  MPIDR 103
>
> I think this will make PSCI operations from CPU8 onwards fail.
>

I can confirm this is wrong

I did a quick hack

GiccPtr->MPIDR = ((NumCores / 8) << 8) | (NumCores % 8);

Thanks for the fix.
 
and I can boot 128 cores (MPIDR also needs fixed in SSDT I think)

[   12.637579] GICv3: CPU127: found redistributor f07 region
0:0x0000000041060000
[   12.640185] CPU127: Booted secondary processor 0x0000000f07
[0x411fd070]
[   12.676961] smp: Brought up 1 node, 128 CPUs

Considering this patch is in the works

https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg06223.html

which add MPIDR to the DT, i think you will when that is accepted be
able to use the MPIDR from there and then you are protected for the
CPU topology changing.

So shall I push an interim patch that uses the aforementioned formula
to derive the MPIDR ? Or wait for the qemu patch to get merged and
then read MPIDR directly from there ?

Thanks,
Tanmay

Thanks

Graeme


> Thanks
>
> Graeme
>
>
> > +    New += sizeof (EFI_ACPI_6_0_GIC_STRUCTURE);
> > +  }
> > +
> > +  // GIC Distributor Structure
> > +  CopyMem (New, &Gicd, sizeof (EFI_ACPI_6_0_GIC_DISTRIBUTOR_STRUCTURE));
> > +  New += sizeof (EFI_ACPI_6_0_GIC_DISTRIBUTOR_STRUCTURE);
> > +
> > +  // GIC ReDistributor Structure
> > +  CopyMem (New, &Gicr, sizeof (EFI_ACPI_6_0_GICR_STRUCTURE));
> > +  New += sizeof (EFI_ACPI_6_0_GICR_STRUCTURE);
> > +
> > +  AcpiPlatformChecksum ((UINT8*) PageAddress, TableSize);
> > +
> > +  Status = AcpiTable->InstallAcpiTable (
> > +                        AcpiTable,
> > +                        (EFI_ACPI_COMMON_HEADER *)PageAddress,
> > +                        TableSize,
> > +                        &TableHandle
> > +                        );
> > +  if (EFI_ERROR(Status)) {
> > +    DEBUG ((DEBUG_ERROR, "Failed to install MADT table\n"));
> > +  }
> > +
> > +  return Status;
> > +}
> > +
> >  EFI_STATUS
> >  EFIAPI
> >  InitializeSbsaQemuAcpiDxe (
> > @@ -68,8 +205,27 @@ InitializeSbsaQemuAcpiDxe (
> >    IN EFI_SYSTEM_TABLE     *SystemTable
> >    )
> >  {
> > +  EFI_STATUS                     Status;
> > +  EFI_ACPI_TABLE_PROTOCOL        *AcpiTable;
> > +
> >    // Parse the device tree and get the number of CPUs
> >    CountCpusFromFdt ();
> > 
> > +  // Check if ACPI Table Protocol has been installed
> > +  Status = gBS->LocateProtocol (
> > +                  &gEfiAcpiTableProtocolGuid,
> > +                  NULL,
> > +                  (VOID **)&AcpiTable
> > +                  );
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "Failed to locate ACPI Table Protocol\n"));
> > +    return Status;
> > +  }
> > +
> > +  Status = AddMadtTable (AcpiTable);
> > +  if (EFI_ERROR(Status)) {
> > +     DEBUG ((DEBUG_ERROR, "Failed to add MADT table\n"));
> > +  }
> > +
> >    return EFI_SUCCESS;
> >  }
> > --
> > 2.28.0
> >


Re: question about EDKII_IOMMU_PROTOCOL

Tiger Liu(BJ-RD)
 

Hi, Laszlo:
Got it.

Thanks

-----邮件原件-----
发件人: Laszlo Ersek <lersek@...>
发送时间: 2020年8月26日 18:35
收件人: devel@edk2.groups.io; Tiger Liu(BJ-RD) <TigerLiu@...>
主题: Re: [edk2-devel] question about EDKII_IOMMU_PROTOCOL

On 08/25/20 09:39, Tiger Liu(BJ-RD) wrote:
Hi, Experts:
I find EDKII_IOMMU_PROTOCOL definition in MdeModulePkg\Include\Protocol\IoMmu.h .

But I didn't find its define in UEFI Spec or PI Spec.
So, is it a private protocol definition?
Yes -- that's why the protocol name starts with the "EDKII_" prefix, and why the header file is under MdeModulePkg and not MdePkg.

It's an "implementation detail" in edk2.

Thanks,
Laszlo



保密声明:
本邮件含有保密或专有信息,仅供指定收件人使用。严禁对本邮件或其内容做任何未经授权的查阅、使用、复制或转发。
CONFIDENTIAL NOTE:
This email contains confidential or legally privileged information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or forwarding of this email or the content of this email is strictly prohibited.


回复: [edk2-devel] gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask and ExitBootServices

gaoliming
 

Fish:
If PcdNullPointerDetectionPropertyMask BIT7 is set, it will be disabled on EndOfDxe. Do you mean to always turn off it on ExitBootServices? But, after ExitBootSerivces, there should be no memory allocation.

Thanks
Liming

-----邮件原件-----
发件人: bounce+27952+64665+4905953+8761045@groups.io
<bounce+27952+64665+4905953+8761045@groups.io> 代表 Andrew Fish
via groups.io
发送时间: 2020年8月27日 8:34
收件人: edk2-devel-groups-io <devel@edk2.groups.io>
主题: [edk2-devel]
gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
and ExitBootServices

I think I might have found a case when grub calls ExitBootServices,
PcdNullPointerDetectionPropertyMask is TRUE, and grub decided to allocate
memory in the guard area….

Is there any code to turn off PcdNullPointerDetectionPropertyMask at
ExitBootServices time?

Seems like MemoryProtectionExitBootServicesCallback() should also turn off
PcdNullPointerDetectionPropertyMask?

Thanks,

Andrew Fish


回复: [edk2-devel] [PATCH v9 2/5] MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe

gaoliming
 

Matthew:

-----邮件原件-----
发件人: bounce+27952+64654+4905953+8761045@groups.io
<bounce+27952+64654+4905953+8761045@groups.io> 代表 Matthew
Carlson
发送时间: 2020年8月27日 4:55
收件人: devel@edk2.groups.io
抄送: Ard Biesheuvel <ard.biesheuvel@...>; Michael D Kinney
<michael.d.kinney@...>; Liming Gao <liming.gao@...>;
Zhiguang Liu <zhiguang.liu@...>; Matthew Carlson
<matthewfcarlson@...>
主题: [edk2-devel] [PATCH v9 2/5] MdePkg: BaseRngLibDxe: Add RngLib that
uses RngDxe

From: Matthew Carlson <macarl@...>

This adds a RngLib that uses the RngProtocol to provide randomness.
This means that the RngLib is meant to be used with DXE_DRIVERS.

Ref: https://github.com/tianocore/edk2/pull/845
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1871

Cc: Ard Biesheuvel <ard.biesheuvel@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <liming.gao@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Signed-off-by: Matthew Carlson <matthewfcarlson@...>
---
MdePkg/Library/DxeRngLib/DxeRngLib.c | 199 ++++++++++++++++++++
MdePkg/Library/DxeRngLib/DxeRngLib.inf | 38 ++++
MdePkg/Library/DxeRngLib/DxeRngLib.uni | 15 ++
MdePkg/MdePkg.dsc | 4 +-
4 files changed, 255 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Library/DxeRngLib/DxeRngLib.c
b/MdePkg/Library/DxeRngLib/DxeRngLib.c
new file mode 100644
index 000000000000..8d4e05e52d57
--- /dev/null
+++ b/MdePkg/Library/DxeRngLib/DxeRngLib.c
@@ -0,0 +1,199 @@
+/** @file

+ Provides an implementation of the library class RngLib that uses the Rng
protocol.

+

+ Copyright (c) Microsoft Corporation. All rights reserved.

+ SPDX-License-Identifier: BSD-2-Clause-Patent

+

+**/

+#include <Uefi.h>

+#include <Library/UefiBootServicesTableLib.h>

+#include <Library/DebugLib.h>

+#include <Library/RngLib.h>

+#include <Protocol/Rng.h>

+

+/**

+ Routine Description:

+

+ Generates a random number via the NIST

+ 800-9A algorithm. Refer to

+ http://csrc.nist.gov/groups/STM/cavp/documents/drbg/DRBGVS.pdf

+ for more information.

+

+ @param[out] Buffer Buffer to receive the random number.

+ @param[in] BufferSize Number of bytes in Buffer.

+

+ @retval EFI_SUCCESS or underlying failure code.

+**/

+STATIC

+EFI_STATUS

+GenerateRandomNumberViaNist800Algorithm (

+ OUT UINT8 *Buffer,

+ IN UINTN BufferSize

+ )

+{

+ EFI_STATUS Status;

+ EFI_RNG_PROTOCOL *RngProtocol;

+

+ RngProtocol = NULL;

+

+ if (Buffer == NULL) {

+ DEBUG((DEBUG_ERROR, "%a: Buffer == NULL.\n",
__FUNCTION__));

+ return EFI_INVALID_PARAMETER;

+ }

+

+ Status = gBS->LocateProtocol (&gEfiRngProtocolGuid, NULL, (VOID
**)&RngProtocol);

+ if (EFI_ERROR (Status) || RngProtocol == NULL) {

+ DEBUG((DEBUG_ERROR, "%a: Could not locate RNG prototocol,
Status = %r\n", __FUNCTION__, Status));

+ return Status;

+ }

+

+ Status = RngProtocol->GetRNG (RngProtocol,
&gEfiRngAlgorithmSp80090Ctr256Guid, BufferSize, Buffer);

+ DEBUG((DEBUG_INFO, "%a: GetRNG algorithm CTR-256 - Status = %r\n",
__FUNCTION__, Status));

+ if (!EFI_ERROR (Status)) {

+ return Status;

+ }

+

+ Status = RngProtocol->GetRNG (RngProtocol,
&gEfiRngAlgorithmSp80090Hmac256Guid, BufferSize, Buffer);

+ DEBUG((DEBUG_INFO, "%a: GetRNG algorithm HMAC-256 - Status
= %r\n", __FUNCTION__, Status));

+ if (!EFI_ERROR (Status)) {

+ return Status;

+ }

+

+ Status = RngProtocol->GetRNG (RngProtocol,
&gEfiRngAlgorithmSp80090Hash256Guid, BufferSize, Buffer);

+ DEBUG((DEBUG_INFO, "%a: GetRNG algorithm Hash-256 - Status = %r\n",
__FUNCTION__, Status));

+ if (!EFI_ERROR (Status)) {

+ return Status;

+ }

+ // If all the other methods have failed, use the default method from
the
RngProtocol

+ Status = RngProtocol->GetRNG (RngProtocol, NULL, BufferSize, Buffer);

+ DEBUG((DEBUG_INFO, "%a: GetRNG algorithm Hash-256 - Status = %r\n",
__FUNCTION__, Status));

+ if (!EFI_ERROR (Status)) {

+ return Status;

+ }

+ // If we get to this point, we have failed

+ DEBUG((DEBUG_ERROR, "%a: GetRNG() failed, staus = %r\n",
__FUNCTION__, Status));

+

+ return Status;

+}// GenerateRandomNumberViaNist800Algorithm()

+

+

+/**

+ Generates a 16-bit random number.

+

+ if Rand is NULL, return FALSE.

+

+ @param[out] Rand Buffer pointer to store the 16-bit random value.

+

+ @retval TRUE Random number generated successfully.

+ @retval FALSE Failed to generate the random number.

+

+**/

+BOOLEAN

+EFIAPI

+GetRandomNumber16 (

+ OUT UINT16 *Rand

+ )

+{

+ EFI_STATUS Status;

+

+ if (Rand == NULL)

+ {

+ return FALSE;

+ }

+

+ Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand,
2);
Here, how about use sizeof (UINT16) to replace hardcode 2? It will be
meaningful.
The same comments are for the following 4, 8, 16. 16 is 2 * sizeof (UINT64).


Thanks
Liming

+ if (EFI_ERROR (Status)) {

+ return FALSE;

+ }

+ return TRUE;

+}

+

+/**

+ Generates a 32-bit random number.

+

+ if Rand is NULL, return FALSE.

+

+ @param[out] Rand Buffer pointer to store the 32-bit random value.

+

+ @retval TRUE Random number generated successfully.

+ @retval FALSE Failed to generate the random number.

+

+**/

+BOOLEAN

+EFIAPI

+GetRandomNumber32 (

+ OUT UINT32 *Rand

+ )

+{

+ EFI_STATUS Status;

+

+ if (Rand == NULL) {

+ return FALSE;

+ }

+

+ Status = GenerateRandomNumberViaNist800Algorithm ((UINT8*)Rand,
4);

+ if (EFI_ERROR (Status)) {

+ return FALSE;

+ }

+ return TRUE;

+}

+

+/**

+ Generates a 64-bit random number.

+

+ if Rand is NULL, return FALSE.

+

+ @param[out] Rand Buffer pointer to store the 64-bit random value.

+

+ @retval TRUE Random number generated successfully.

+ @retval FALSE Failed to generate the random number.

+

+**/

+BOOLEAN

+EFIAPI

+GetRandomNumber64 (

+ OUT UINT64 *Rand

+ )

+{

+ EFI_STATUS Status;

+

+ if (Rand == NULL) {

+ return FALSE;

+ }

+

+ Status = GenerateRandomNumberViaNist800Algorithm ((UINT8*)Rand,
8);

+ if (EFI_ERROR (Status)) {

+ return FALSE;

+ }

+ return TRUE;

+}

+

+/**

+ Generates a 128-bit random number.

+

+ if Rand is NULL, return FALSE.

+

+ @param[out] Rand Buffer pointer to store the 128-bit random
value.

+

+ @retval TRUE Random number generated successfully.

+ @retval FALSE Failed to generate the random number.

+

+**/

+BOOLEAN

+EFIAPI

+GetRandomNumber128 (

+ OUT UINT64 *Rand

+ )

+{

+ EFI_STATUS Status;

+

+ if (Rand == NULL) {

+ return FALSE;

+ }

+

+ Status = GenerateRandomNumberViaNist800Algorithm ((UINT8*)Rand,
16);

+ if (EFI_ERROR (Status)) {

+ return FALSE;

+ }

+ return TRUE;

+}

diff --git a/MdePkg/Library/DxeRngLib/DxeRngLib.inf
b/MdePkg/Library/DxeRngLib/DxeRngLib.inf
new file mode 100644
index 000000000000..68554ad21146
--- /dev/null
+++ b/MdePkg/Library/DxeRngLib/DxeRngLib.inf
@@ -0,0 +1,38 @@
+# @file

+# Provides implementation of the library class RngLib that uses the
RngProtocol

+#

+# @copyright

+# Copyright (c) Microsoft Corporation. All rights reserved.

+# SPDX-License-Identifier: BSD-2-Clause-Patent

+#

+##

+

+[Defines]

+ INF_VERSION = 1.27

+ BASE_NAME = DxeRngLib

+ MODULE_UNI_FILE = DxeRngLib.uni

+ FILE_GUID = FF9F84C5-A33E-44E3-9BB5-0D654B2D4149

+ MODULE_TYPE = DXE_DRIVER

+ VERSION_STRING = 1.0

+ LIBRARY_CLASS = RngLib|DXE_DRIVER UEFI_APPLICATION
UEFI_DRIVER

+

+[Packages]

+ MdePkg/MdePkg.dec

+

+[Sources]

+ DxeRngLib.c

+

+[LibraryClasses]

+ DebugLib

+ UefiBootServicesTableLib

+

+[Protocols]

+ gEfiRngProtocolGuid ## CONSUMES

+

+[Depex]

+ gEfiRngProtocolGuid

+

+[Guids]

+ gEfiRngAlgorithmSp80090Ctr256Guid

+ gEfiRngAlgorithmSp80090Hash256Guid

+ gEfiRngAlgorithmSp80090Hmac256Guid

diff --git a/MdePkg/Library/DxeRngLib/DxeRngLib.uni
b/MdePkg/Library/DxeRngLib/DxeRngLib.uni
new file mode 100644
index 000000000000..c904e54b6fb0
--- /dev/null
+++ b/MdePkg/Library/DxeRngLib/DxeRngLib.uni
@@ -0,0 +1,15 @@
+// @file

+// Instance of RNG (Random Number Generator) Library.

+//

+// RngLib that uses the Rng Protocol to provide random numbers.

+//

+// Copyright (c) Microsoft Corporation.

+//

+// SPDX-License-Identifier: BSD-2-Clause-Patent

+//

+

+

+#string STR_MODULE_ABSTRACT #language en-US "Instance of RNG
Library"

+

+#string STR_MODULE_DESCRIPTION #language en-US "BaseRng Library
that uses the Rng Protocol to provide random numbers"

+

diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc
index d7ba3a730909..2c3b7966b086 100644
--- a/MdePkg/MdePkg.dsc
+++ b/MdePkg/MdePkg.dsc
@@ -62,8 +62,10 @@
MdePkg/Library/BasePostCodeLibPort80/BasePostCodeLibPort80.inf

MdePkg/Library/BasePrintLib/BasePrintLib.inf


MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull
.inf

- MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf

+ MdePkg/Library/DxeRngLib/DxeRngLib.inf

MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf

+ MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf

+

MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf

MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf


MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf

--
2.28.0.windows.1


-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#64654): https://edk2.groups.io/g/devel/message/64654
Mute This Topic: https://groups.io/mt/76437902/4905953
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub
[gaoliming@...]
-=-=-=-=-=-=


回复: [edk2-devel] [PATCH v9 1/5] MdePkg: TimerRngLib: Added RngLib that uses TimerLib

gaoliming
 

Matthew:

-----邮件原件-----
发件人: bounce+27952+64653+4905953+8761045@groups.io
<bounce+27952+64653+4905953+8761045@groups.io> 代表 Matthew
Carlson
发送时间: 2020年8月27日 4:55
收件人: devel@edk2.groups.io
抄送: Ard Biesheuvel <ard.biesheuvel@...>; Michael D Kinney
<michael.d.kinney@...>; Liming Gao <liming.gao@...>;
Zhiguang Liu <zhiguang.liu@...>; Matthew Carlson
<matthewfcarlson@...>
主题: [edk2-devel] [PATCH v9 1/5] MdePkg: TimerRngLib: Added RngLib that
uses TimerLib

From: Matthew Carlson <macarl@...>

Added a new RngLib that provides random numbers from the TimerLib
using the performance counter. This is meant to be used for OpenSSL
to replicate past behavior. This should not be used in production as
a real source of entropy.

Ref: https://github.com/tianocore/edk2/pull/845
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1871

Cc: Ard Biesheuvel <ard.biesheuvel@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <liming.gao@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Signed-off-by: Matthew Carlson <matthewfcarlson@...>
---
MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c | 187
++++++++++++++++++++
MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf | 36 ++++
MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni | 15 ++
MdePkg/MdePkg.dsc |
3 +-
4 files changed, 240 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c
b/MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c
new file mode 100644
index 000000000000..aecaa427bb3f
--- /dev/null
+++ b/MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c
@@ -0,0 +1,187 @@
+/** @file

+ BaseRng Library that uses the TimerLib to provide reasonably random
numbers.

+ Do not use this on a production system.

+

+ Copyright (c) Microsoft Corporation.

+ SPDX-License-Identifier: BSD-2-Clause-Patent

+**/

+

+#include <Base.h>

+#include <Library/BaseLib.h>

+#include <Library/DebugLib.h>

+#include <Library/TimerLib.h>

+

+/**

+ Using the TimerLib GetPerformanceCounterProperties() we delay

+ for enough time for the PerformanceCounter to increment.

+

+ If the return value from GetPerformanceCounterProperties (TimerLib)

+ is zero, this function will return 10 and attempt to assert.

+ **/

+STATIC

+UINT32

+CalculateMinimumDecentDelayInMicroseconds (

+ VOID

+ )

+{

+ UINT64 CounterHz;

+

+ // Get the counter properties

+ CounterHz = GetPerformanceCounterProperties (NULL, NULL);

+ // Make sure we won't divide by zero

+ if (CounterHz == 0) {

+ ASSERT(CounterHz != 0); // Assert so the developer knows something is
wrong

+ return 10; // return 10 microseconds by default
How about define one macro for the default value?


+ }

+ // Calculate the minimum delay based on 1.5 microseconds divided by the
hertz.

+ // We calculate the length of a cycle (1/CounterHz) and multiply it by
1.5
microseconds

+ // This ensures that the performance counter has increased by at least
one

+ return (UINT32)(MAX (DivU64x64Remainder (1500000,CounterHz, NULL),
1));

+}

+

+

+/**

+ Generates a 16-bit random number.

+

+ if Rand is NULL, then ASSERT().

+

+ @param[out] Rand Buffer pointer to store the 16-bit random value.

+

+ @retval TRUE Random number generated successfully.

+ @retval FALSE Failed to generate the random number.

+

+**/

+BOOLEAN

+EFIAPI

+GetRandomNumber16 (

+ OUT UINT16 *Rand

+ )

+{

+ UINT32 Index;

+ UINT8 *RandPtr;

+ UINT32 DelayInMicroSeconds;

+

+ ASSERT (Rand != NULL);

+

+ if (Rand == NULL) {

+ return FALSE;

+ }

+ DelayInMicroSeconds = CalculateMinimumDecentDelayInMicroseconds ();

+ RandPtr = (UINT8*)Rand;

+ // Get 2 bytes of random ish data

+ for (Index = 0; Index < 2; Index ++) {

+ *RandPtr = (UINT8)(GetPerformanceCounter () & 0xFF);

+ // Delay to give the performance counter a chance to change

+ MicroSecondDelay (DelayInMicroSeconds);

+ RandPtr++;

+ }

+ return TRUE;

+}

+

+/**

+ Generates a 32-bit random number.

+

+ if Rand is NULL, then ASSERT().

+

+ @param[out] Rand Buffer pointer to store the 32-bit random value.

+

+ @retval TRUE Random number generated successfully.

+ @retval FALSE Failed to generate the random number.

+

+**/

+BOOLEAN

+EFIAPI

+GetRandomNumber32 (

+ OUT UINT32 *Rand

+ )

+{

+ UINT32 Index;

+ UINT8 *RandPtr;

+ UINT32 DelayInMicroSeconds;

+

+ ASSERT (Rand != NULL);

+

+ if (NULL == Rand) {

+ return FALSE;

+ }

+

+ RandPtr = (UINT8 *) Rand;

+ DelayInMicroSeconds = CalculateMinimumDecentDelayInMicroseconds ();

+ // Get 4 bytes of random ish data

+ for (Index = 0; Index < 4; Index ++) {

+ *RandPtr = (UINT8)(GetPerformanceCounter () & 0xFF);

+ // Delay to give the performance counter a chance to change

+ MicroSecondDelay (DelayInMicroSeconds);

+ RandPtr++;

+ }

+ return TRUE;

+}

+

+/**

+ Generates a 64-bit random number.

+

+ if Rand is NULL, then ASSERT().

+

+ @param[out] Rand Buffer pointer to store the 64-bit random value.

+

+ @retval TRUE Random number generated successfully.

+ @retval FALSE Failed to generate the random number.

+

+**/

+BOOLEAN

+EFIAPI

+GetRandomNumber64 (

+ OUT UINT64 *Rand

+ )

+{

+ UINT32 Index;

+ UINT8 *RandPtr;

+ UINT32 DelayInMicroSeconds;

+

+ ASSERT (Rand != NULL);

+

+ if (NULL == Rand) {

+ return FALSE;

+ }

+

+ RandPtr = (UINT8 *)Rand;

+ DelayInMicroSeconds = CalculateMinimumDecentDelayInMicroseconds ();

+ // Get 8 bytes of random ish data

+ for (Index = 0; Index < 8; Index ++) {

+ *RandPtr = (UINT8)(GetPerformanceCounter () & 0xFF);

+ // Delay to give the performance counter a chance to change

+ MicroSecondDelay (DelayInMicroSeconds);

+ RandPtr++;

+ }

+

+ return TRUE;

+}

+

+/**

+ Generates a 128-bit random number.

+

+ if Rand is NULL, then ASSERT().

+

+ @param[out] Rand Buffer pointer to store the 128-bit random
value.

+

+ @retval TRUE Random number generated successfully.

+ @retval FALSE Failed to generate the random number.

+

+**/

+BOOLEAN

+EFIAPI

+GetRandomNumber128 (

+ OUT UINT64 *Rand

+ )

+{

+ ASSERT (Rand != NULL);

+ // This should take around 80ms

+

+ // Read first 64 bits

+ if (!GetRandomNumber64 (Rand)) {

+ return FALSE;

+ }

+

+ // Read second 64 bits

+ return GetRandomNumber64 (++Rand);

+}

diff --git a/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
b/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
new file mode 100644
index 000000000000..c499e5327351
--- /dev/null
+++ b/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
@@ -0,0 +1,36 @@
+## @file

+# Instance of RNG (Random Number Generator) Library.

+#

+# BaseRng Library that uses the TimerLib to provide reasonably random
numbers.

+# Do NOT use this on a production system as this uses the system
performance

+# counter rather than a true source of random in addition to having a
weak

+# random algorithm. This is provided primarily as a source of entropy
for

+# OpenSSL for platforms that do not have a good built in RngLib as this

+# emulates what was done before (though it isn't perfect).

+#

+# Copyright (c) Microsoft Corporation. All rights reserved.<BR>

+#

+# SPDX-License-Identifier: BSD-2-Clause-Patent

+#

+#

+##

+

+[Defines]

+ INF_VERSION = 1.27

+ BASE_NAME = BaseRngLibTimerLib

+ MODULE_UNI_FILE = BaseRngLibTimerLib.uni

+ FILE_GUID =
74950C45-10FC-4AB5-B114-49C87C17409B

+ MODULE_TYPE = BASE

+ VERSION_STRING = 1.0

+ LIBRARY_CLASS = RngLib

+ CONSTRUCTOR = BaseRngLibConstructor
Please remove CONSTRUCTOR, this library instance has no constructor.

+

+[Sources]

+ RngLibTimer.c

+

+[Packages]

+ MdePkg/MdePkg.dec

+

+[LibraryClasses]

+ BaseLib

+ TimerLib
Please add DebugLib here, this library instance also depends on DebugLib.

Thanks
Liming

diff --git a/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni
b/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni
new file mode 100644
index 000000000000..fde24b9f0107
--- /dev/null
+++ b/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni
@@ -0,0 +1,15 @@
+// @file

+// Instance of RNG (Random Number Generator) Library.

+//

+// RngLib that uses TimerLib's performance counter to provide random
numbers.

+//

+// Copyright (c) Microsoft Corporation.

+//

+// SPDX-License-Identifier: BSD-2-Clause-Patent

+//

+

+

+#string STR_MODULE_ABSTRACT #language en-US "Instance of RNG
Library"

+

+#string STR_MODULE_DESCRIPTION #language en-US "BaseRng Library
that uses the TimerLib to provide low-entropy random numbers"

+

diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc
index 472fa3777412..d7ba3a730909 100644
--- a/MdePkg/MdePkg.dsc
+++ b/MdePkg/MdePkg.dsc
@@ -62,6 +62,8 @@
MdePkg/Library/BasePostCodeLibPort80/BasePostCodeLibPort80.inf

MdePkg/Library/BasePrintLib/BasePrintLib.inf


MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull
.inf

+ MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf

+ MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf

MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf

MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf


MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf

@@ -69,7 +71,6 @@

MdePkg/Library/BaseUefiDecompressLib/BaseUefiTianoCustomDecompressL
ib.inf

MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf

MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf

- MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf



MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf

MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf

--
2.28.0.windows.1


-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#64653): https://edk2.groups.io/g/devel/message/64653
Mute This Topic: https://groups.io/mt/76437900/4905953
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub
[gaoliming@...]
-=-=-=-=-=-=


Re: [PATCH] MdeModulePkg/UsbBusDxe: some USB PenDisk fails enumeration.

Feng Libo <lbfeng@...>
 

Hello, 

How about the progress of the Patch Review?

Thanks

--
Best Regards

Feng Libo
ZD Technology (Beijing) Co., Ltd

发件人:"丰立波" <lbfeng@...>
发送日期:2020-08-13 10:49:33
收件人:"Jiang, Guomin" <guomin.jiang@...>
抄送人:"devel@edk2.groups.io" <devel@edk2.groups.io>,"jeremy.linton@..." <jeremy.linton@...>,"Wu, Hao A" <hao.a.wu@...>,"Ni, Ray" <ray.ni@...>,"张超" <czhang@...>
主题:Re:RE: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: some USB PenDisk fails enumeration.

Hello, Mr. Jiang,

Most USB PenDisk work fine with the original enumeration sequence. A few can't.
 
In Microsoft Post, they explained the cause :
In the early days of USB some USB devices would become confused by a second request for the Device Descriptor if they did not return the complete Device Descriptor for the first request.  To allow these devices to enumerate successfully it was necessary to reset the port between the first and second requests for the Device Descriptor.

In our experience, only three Pendisk with Innostor USB controller chip (VID=0x1F75, PID=0x917, USB3.1) fail the enumeration. We can only observed the Pendisk not responding the second Device Descriptor Request for a full Descriptor. The first Device Descriptor Request could impact some Pendisk or as Microsoft said "confuse".  

So, We add a second port reset after the MaxPacketSize0 request. and this second reset can clear the address that is already assigned. Then, we move the MaxPacketSize request before the address assignation.

That is all patch.

Furthermore, the USB Spec states the MaxPacketSize Request could be read through the default Pipe, the address 0. So I think it implies the MaxPacketSize Request should be prior to the address assignation, just as the enumeration sequence in the Microsoft Post.

Thanks

--
Best Regards

Feng Libo
ZD Technology (Beijing) Co., Ltd

发件人:"Jiang, Guomin" <guomin.jiang@...>
发送日期:2020-08-12 16:03:42
收件人:"devel@edk2.groups.io" <devel@edk2.groups.io>,"Jiang, Guomin" <guomin.jiang@...>,"lbfeng@..." <lbfeng@...>
抄送人:"jeremy.linton@..." <jeremy.linton@...>,"Wu, Hao A" <hao.a.wu@...>,"Ni, Ray" <ray.ni@...>
主题:RE: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: some USB PenDisk fails enumeration.

Hi Libo,

 

I review the USB Spec 2.0 and have some confusion when check the spec as below:

From the USB 2.0 spec:

 

Section 5.5.3 Control Transfer Packet Size Constraints

...

In order to determine the maximum packet size for the Default Control Pipe, the USB System Software

reads the device descriptor. The host will read the first eight bytes of the device descriptor. The device

always responds with at least these initial bytes in a single packet. After the host reads the initial part of the

device descriptor, it is guaranteed to have read this default pipe’s wMaxPacketSize field (byte 7 of the

device descriptor). It will then allow the correct size for all subsequent transactions. For all other control

endpoints, the maximum data payload size is known after configuration so that the USB System Software

can ensure that no data payload will be sent to the endpoint that is larger than the supported size.

...

 

Also,

Section 9.1.2 Bus Enumeration

...

3. Now that the host knows the port to which the new device has been attached, the host then waits for at

least 100 ms to allow completion of an insertion process and for power at the device to become stable.

The host then issues a port enable and reset command to that port. Refer to Section 7.1.7.5 for

sequence of events and timings of connection through device reset.

4. The hub performs the required reset processing for that port (see Section 11.5.1.5). When the reset

signal is released, the port has been enabled. The USB device is now in the Default state and can draw

no more than 100 mA from VBUS. All of its registers and state have been reset and it answers to the

default address.

5. The host assigns a unique address to the USB device, moving the device to the Address state.

6. Before the USB device receives a unique address, its Default Control Pipe is still accessible via the

default address. The host reads the device descriptor to determine what actual maximum data payload

size this USB device’s default pipe can use.

7. The host reads the configuration information from the device by reading each configuration zero to

n-1, where n is the number of configurations. This process may take several milliseconds to complete.

...

 

It seem that the original behavior follow the spec, but I don’t know why the device will not response and must reset it.

 

I notice that  you obtain the patch from https://techcommunity.microsoft.com/t5/microsoft-usb-blog/how-does-usb-stack-enumerate-a-device/ba-p/270685#:~:text=%20How%20does%20USB%20stack%20enumerate%20a%20device%3F,a%20request%20for%20the%20USB%20Device...%20More%20.

 

Do you know the device behavior from the device side when the issue happened?

 

Thanks.

Guomin

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin Jiang
Sent: Tuesday, August 11, 2020 7:17 PM
To: devel@edk2.groups.io; lbfeng@...
Cc: jeremy.linton@...; Wu, Hao A <hao.a.wu@...>; Ni, Ray <ray.ni@...>
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: some USB PenDisk fails enumeration.

 

+Hao, Ray,

 

Hi Libo, thanks for your explanation.

 

So I think the patch is improvement for current logic.

 

Hi Hao and Ray,

 

Can you give some comments for the change.

 

Hi Jeremy,

 

It may be helpful for the ASSERT issue https://edk2.groups.io/g/devel/message/62651,can you try it?

 

Best Regards

Guomin

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Feng Libo
Sent: Tuesday, August 11, 2020 5:50 PM
To: Jiang, Guomin <guomin.jiang@...>
Cc: devel@edk2.groups.io; jeremy.linton@...
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: some USB PenDisk fails enumeration.

 

Hello, Mr. Jiang,

 

Thank for the review.

 

The original enumeration steps in the function of UsbEnumerateNewDev of file UsbEnumer.c: 1 reset the port, 2 set the usb device address, 3 get the Max Packet Size, 4 get the full device descriptor. However, when plugging a USB PenDisk with Innostor USB

controller chip (VID=0x1F75, PID=0x917, USB3.1), the fourth step always fails, trace as below:

 

========

XhcCheckUrbResult: TRANSACTION_ERROR! Completecode = 4 XhcControlTransfer: error - Device Error, transfer - 40 UsbGetOneConfig: failed to get full descript Device Error UsbBuildDescTable: failed to get configure (index 0) UsbEnumerateNewDev: failed to build descriptor table - Device Error

=======

 

The host controller need to get the full device descriptor, but this moment, the Pendisk device doesn't response any more. Then timeout. and UsbEnumerateNewDev complains : failed to build descriptor.

 

We have three Pendisks from different manufacturers, all with Innostor USB controller chip. they all can't be enumerated all. And we observed the problem on both Huawei KunPeng(华为鲲鹏)and Loognson(龙芯)platforms.

 

The three Pendisks always fail the USB enumeration. Other USB 2.0 and USB 3.0 on hand can work well.

 

With the patch, the three pendisks and other pendisks can all work well.

 

THanks

 

--

Best Regards

 

Feng Libo

ZD Technology (Beijing) Co., Ltd


发件人:"Jiang, Guomin" <guomin.jiang@...>
发送日期:2020-08-11 08:21:10
收件人:"devel@edk2.groups.io" <devel@edk2.groups.io>,"Jiang, Guomin" <guomin.jiang@...>,"lbfeng@..." <lbfeng@...>
抄送人:"jeremy.linton@..." <jeremy.linton@...>
主题:RE: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: some USB PenDisk fails enumeration.


+Jeremy,

 

I review the patch and think it is reasonable, but I want to know some more detail information

  1. Can you provide the detail debug log about USB?
  2. The symptom always can be seen or have fail rate?

 

Best Regards

Guomin

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin Jiang
Sent: Thursday, August 6, 2020 12:29 PM
To: devel@edk2.groups.io; lbfeng@...
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: some USB PenDisk fails enumeration.

 

I will review it by next weekend(8/14).

 

Thanks.

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Feng Libo
Sent: Thursday, August 6, 2020 9:25 AM
To: Feng Libo <lbfeng@...>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: some USB PenDisk fails enumeration.

 

Hello, 

could anyone review this PATCH?

We encountered the USB enumeration problem and the patch is based on the Microsoft post as below.

https://techcommunity.microsoft.com/t5/microsoft-usb-blog/how-does-usb-stack-enumerate-a-device/ba-p/270685#:~:text=%20How%20does%20USB%20stack%20enumerate%20a%20device%3F,a%20request%20for%20the%20USB%20Device...%20More%20

Thanks

Best Regards

Feng Libo