Date   

Re: [PATCH 5/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP

Philippe Mathieu-Daudé
 

On 6/8/21 3:06 PM, Laszlo Ersek wrote:
Insert a SHA256 CHAP_HASH structure at the start of "mChapHash".

Update ISCSI_CHAP_MAX_DIGEST_SIZE to SHA256_DIGEST_SIZE (32).

This enables the initiator and the target to negotiate SHA256 for CHAP, in
preference to MD5.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3355
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.h | 3 ++-
NetworkPkg/IScsiDxe/IScsiCHAP.c | 12 ++++++++++++
2 files changed, 14 insertions(+), 1 deletion(-)
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>


Re: [PATCH 2/6] NetworkPkg/IScsiDxe: add horizontal whitespace to IScsiCHAP files

Philippe Mathieu-Daudé
 

On 6/8/21 3:06 PM, Laszlo Ersek wrote:
In the next patches, we'll need more room for various macro and parameter
names. For maintaining the current visual alignments, insert some
horizontal whitespace in preparation. "git show -b" produces no output for
this patch; the patch introduces no functional changes.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3355
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.h | 24 ++++++++++----------
NetworkPkg/IScsiDxe/IScsiCHAP.c | 12 +++++-----
2 files changed, 18 insertions(+), 18 deletions(-)
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>


Re: [PATCH 0/2] Quo vadis virtio-mmio?

Philippe Mathieu-Daudé
 

On 6/9/21 7:05 AM, Gerd Hoffmann wrote:
virtio-mmio support in ovmf seems to be the unloved child. The final
virto-1.0 specification was published five(!) years ago, nevertheless
the mmio transport doesn't support it yet ...

Some people argue that it has been obsoleted by virtio-pci. Which is a
valid argument. But IMHO isn't a good reason to just let virtio-mmio
bitrot. We should either remove it from the tree, or support it.

So, opening the discussion with this little patch series. It does the
latter and adds virtio 1.0 support. For the mmio transport the
difference between 0.9.5 and 1.0 is rather small (when compared to the
pci transport), it is just a bunch of new registers for the changed
virtio queue initialization. So the patch series is small too ...

take care,
Gerd

Gerd Hoffmann (2):
OvmfPkg/Virtio10: Add virtio-mmio 1.0 defines
OvmfPkg/VirtioMmioDeviceLib: Add virtio 1.0 support.

OvmfPkg/Include/IndustryStandard/Virtio10.h | 12 +++++++
.../VirtioMmioDeviceLib/VirtioMmioDevice.h | 1 +
.../VirtioMmioDeviceLib/VirtioMmioDevice.c | 17 +++++++---
.../VirtioMmioDeviceFunctions.c | 31 +++++++++++++++++--
4 files changed, 54 insertions(+), 7 deletions(-)
Series:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>


[PATCH 1/1] BaseTools GenFw: Keep read only alloc section as text section when convert ELF

gaoliming
 

This is the fix of the regression issue at c6b872c6.
Based on ELF spec, readonly alloc section is .rodata section. It is requried.
This fix is to add back original check logic for ELF section. Now,
the readonly alloc section and execute alloc section are regarded as .text section.

Signed-off-by: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
---
With this fix, previous fix commit ec1cffd9 is not required. But, the checker added
by commit ec1cffd9 is correct for ACPI data conversion. So, I don't plan to revert it.

BaseTools/Source/C/GenFw/Elf32Convert.c | 3 ++-
BaseTools/Source/C/GenFw/Elf64Convert.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c b/BaseTools/Source/C/GenFw/Elf32Convert.c
index 314f8233234d..d917a444c82d 100644
--- a/BaseTools/Source/C/GenFw/Elf32Convert.c
+++ b/BaseTools/Source/C/GenFw/Elf32Convert.c
@@ -238,7 +238,8 @@ IsTextShdr (
Elf_Shdr *Shdr
)
{
- return (BOOLEAN) ((Shdr->sh_flags & (SHF_EXECINSTR | SHF_ALLOC)) == (SHF_EXECINSTR | SHF_ALLOC));
+ return (BOOLEAN) (((Shdr->sh_flags & (SHF_EXECINSTR | SHF_ALLOC)) == (SHF_EXECINSTR | SHF_ALLOC)) ||
+ ((Shdr->sh_flags & (SHF_WRITE | SHF_ALLOC)) == SHF_ALLOC));
}

STATIC
diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c b/BaseTools/Source/C/GenFw/Elf64Convert.c
index 8b09db7b690b..33031ec8f6e7 100644
--- a/BaseTools/Source/C/GenFw/Elf64Convert.c
+++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
@@ -246,7 +246,8 @@ IsTextShdr (
Elf_Shdr *Shdr
)
{
- return (BOOLEAN) ((Shdr->sh_flags & (SHF_EXECINSTR | SHF_ALLOC)) == (SHF_EXECINSTR | SHF_ALLOC));
+ return (BOOLEAN) (((Shdr->sh_flags & (SHF_EXECINSTR | SHF_ALLOC)) == (SHF_EXECINSTR | SHF_ALLOC)) ||
+ ((Shdr->sh_flags & (SHF_WRITE | SHF_ALLOC)) == SHF_ALLOC));
}

STATIC
--
2.27.0.windows.1


Re: [PATCH v2 2/3] UefiPayloadPkg: Add PayloadLoaderPeim which can load ELF payload

Marvin Häuser
 

On 09.06.21 11:49, Ni, Ray wrote:
Thank you for your quick reply, comments inline.
I have to be quick because my project depends on the check-in of this code😊
Sure, so thanks a lot for taking the time!
One non-trivial comment at the bottom.


+ for ( Index = 0
+ ; RelaEntrySize * Index < RelaSize
Overflow?
Will change from:
RelaEntrySize * Index < RelaSize
to:
Index < RelaSize / RelaEntrySize
imo add ASSERT for RelaEntrySize > 0 then.
Sure. To avoid dividend by zero error.

Basically the alignment of any offset with which a pointer to
non-trivially-aligned (i.e. requirement > 1 Byte) data can be forged
must be checked.

Examples from our new PE loader:
https://github.com/mhaeuser/ISPRASOpen-
SecurePE/tree/6a7abcd8647cf6faa733082f6d8dcc9adc141d7e/src/PeCoffInit.
c#L1226
->
https://github.com/mhaeuser/ISPRASOpen-
SecurePE/tree/6a7abcd8647cf6faa733082f6d8dcc9adc141d7e/src/PeCoffInit.
c#L1242
https://github.com/mhaeuser/ISPRASOpen-
SecurePE/tree/6a7abcd8647cf6faa733082f6d8dcc9adc141d7e/src/PeCoffInit.
c#L1389
->
https://github.com/mhaeuser/ISPRASOpen-
SecurePE/tree/6a7abcd8647cf6faa733082f6d8dcc9adc141d7e/src/PeCoffInit.
c#L148

The idea here is that the base pointer (i.e. image header) is "maximally
aligned" (i.e. can hold data of any platform data alignment). For the 8
Bytes AllocatePool() guarantees (file data), this is sufficient for any
primitive and composite data type. For the 4 KB AllocatePages()
guarantees (destination), this is sufficient of that, and for advanced
things like AVX (however not needed here). If the base is maximally
aligned, Base + X is guaranteed aligned for A if X is aligned for A,
i.e. X % _Alignof(A) = 0. Failing to verify this can cause exceptions on
platforms which don't support or have disabled the capability to perform
unaligned memory accesses.
I understand now. I remember that X86 contains a control flag that can trigger
CPU exception as well when unaligned access happens.

But adding such check in all places might require a huge change to today's code.
Can you accept that I ignore such check for now and add it later?
Of course, I mean, it needs some EDK II wide concept first anyway. Just the overall situation is similar (but a lot worse in severity) with the PE loader and now it's not easy to address the issues. :)
So if there are plans to address it, that's great!


+ //
+ // It's abnormal a DYN ELF doesn't contain a dynamic section.
+ //
+ ASSERT (DynShdr != NULL);
+ if (DynShdr == NULL) {
+ return EFI_UNSUPPORTED;
+ }
+ ASSERT (DynShdr->sh_type == SHT_DYNAMIC);
+ ASSERT (DynShdr->sh_entsize >= sizeof (*Dyn));
Abnormalities in unknown/untrusted data must be filtered with a runtime
check, not with an ASSERT.
Sure. I will add if-check below the assertion so assertion-enabled build can
report the errors earlier.
I have started this discussion under another patch, maybe I should write
a broader-scope mail to the list for comments. Basically using ASSERTs
for anything but *impossible* (*not* assuming the input data is
well-formed) situations significantly reduces the efficacy of dynamic
analysis. When doing fuzzing for example, you want to keep the ASSERTs
enabled to be made aware of any internal invariant violations. But if
ASSERTs happen on possible conditions, they will kill the fuzzing
process for no good reason. Turning them off will not analyse your
ASSERTs for possible code defects.

Maybe fuzzing would be a good idea for this library? :)
I understand now. I am ok to remove assertion for external inputs.
Thank you.



+ for ( Index = 0, Dyn = (Elf32_Dyn *) (ElfCt->FileBase + DynShdr-
sh_offset)
+ ; Index < DynShdr->sh_size / DynShdr->sh_entsize
Is "sh_entsize" checked for 0?
No need. Because code above makes sure sh_entsize >= sizeof (*Dyn).
When you turn it into a runtime check as discussed above, yes.


+ ASSERT (RelShdr->sh_type == RelaType);
+ ASSERT (RelShdr->sh_entsize == RelaEntrySize);
See above.
Agree. Will add if-checks.


+ DEBUG ((DEBUG_INFO, "DYN ELF: Relocate using dynamic
sections...\n"));
+ Status = RelocateElf32Dynamic (ElfCt);
+ ASSERT_EFI_ERROR (Status);
Why cannot this fail?
A DYN type ELF image should contain dynamic section. So only an abnormal
ELF image can fail.

Same ASSERT point as above, "cannot" refers to both well-formed and
ill-formed images.
Sure. Will remove assertion.


+ return (Elf64_Phdr *)(ImageBase + Ehdr->e_phoff + Index * Ehdr-
e_phentsize);
Alignment checks? Bounds checks?
For the alignment checks, do you suggest that I should make sure the
segment should be placed
in the address that meets the alignment requirement?
It could be implemented, PE code does it, but I meant pointer alignment
as discussed above somewhere. I don't think ELFs would likely request
more than page alignment, but abort + DEBUG message sounds like a good
idea.

ELF spec just requires below for Elf64_Phdr.p_align:
loadable process segments must have congruent values for p_vaddr and
p_offset, modulo the page size.
I can add such check in ParseElfImage().

+ ProcessRelocation64 (
+ (Elf64_Rela *) (ElfCt->FileBase + RelShdr->sh_offset),
Alignment? :) I know there is no real concept in EDK II yet, but it
really is needed.
Can you explain a bit more on the alignment?
Done above, sorry.


+
+/**
+ Check if the ELF image is valid.
+
+ @param[in] ImageBase Memory address of an image.
+
+ @retval TRUE if valid.
+
+**/
+BOOLEAN
+IsElfFormat (
+ IN CONST UINT8 *ImageBase
You cannot safely inspect untrusted/unknown data without a size field,
also needs checks below.
Agree. Original idea was to add a ELF loader that can load the ELF assuming
the ELF image is well-formatted.
I get that idea, but the reality is that people will start using it for
external images once it is needed. :)
Sorry for being pedantic.

But with your help, I am glad to enhance the logic a bit more to expand
the support of external ELF images.

Will add a "UINTN ImageSize" parameter.

+ )
+{
+ Elf32_Ehdr *Elf32Hdr;
+ Elf64_Ehdr *Elf64Hdr;
+
+ ASSERT (ImageBase != NULL);
+
+ Elf32Hdr = (Elf32_Ehdr *)ImageBase;
+
+ //
+ // Start with correct signature "\7fELF"
+ //
+ if ((Elf32Hdr->e_ident[EI_MAG0] != ELFMAG0) ||
+ (Elf32Hdr->e_ident[EI_MAG1] != ELFMAG1) ||
+ (Elf32Hdr->e_ident[EI_MAG1] != ELFMAG1) ||
+ (Elf32Hdr->e_ident[EI_MAG2] != ELFMAG2)
+ ) {
+ return FALSE;
+ }
+
+ //
+ // Support little-endian only
+ //
+ if (Elf32Hdr->e_ident[EI_DATA] != ELFDATA2LSB) {
+ return FALSE;
+ }
+
+ //
+ // Check 32/64-bit architecture
+ //
+ if (Elf32Hdr->e_ident[EI_CLASS] == ELFCLASS64) {
+ Elf64Hdr = (Elf64_Ehdr *)Elf32Hdr;
+ Elf32Hdr = NULL;
+ } else if (Elf32Hdr->e_ident[EI_CLASS] == ELFCLASS32) {
+ Elf64Hdr = NULL;
+ } else {
+ return FALSE;
+ }
Why are the branches above and below separated when they map
basically 1:1?
Indeed. Thanks for catching this.
Will merge the separate "if" together.

+
+ if (Elf64Hdr != NULL) {
+ //
+ // Support intel architecture only for now
+ //
+ if (Elf64Hdr->e_machine != EM_X86_64) {
+ return FALSE;
+ }
+
+ // Use last section as end of file
+ Status = GetElfSectionPos (ElfCt, ElfCt->ShNum - 1, &Offset, &Size);
What if ShNum is 0?
Agree. The logic to calculate file size might not be needed.
Let me confirm and try to remove the entire function.


+ if (ElfCt->EiClass == ELFCLASS32) {
+ Elf32Hdr = (Elf32_Ehdr *)ElfCt->FileBase;
+ FileSize2 = Elf32Hdr->e_shoff + Elf32Hdr->e_shentsize * Elf32Hdr-
e_shnum;
+ } else if (ElfCt->EiClass == ELFCLASS64) {
+ Elf64Hdr = (Elf64_Ehdr *)ElfCt->FileBase;
+ FileSize2 = (UINTN)(Elf64Hdr->e_shoff + Elf64Hdr->e_shentsize *
Elf64Hdr->e_shnum);
+ }
Overflows?
Integer overflow?
Yes, sorry.

Will add integer overflow check if this file size calculation logic is still needed.


+
+ if (ElfCt == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
As this is function contract, I'd replace this with an ASSERT, or at
least have both.
I will put "ASSERT (ElfCt != NULL)" above the if.


+ ZeroMem (ElfCt, sizeof(ELF_IMAGE_CONTEXT));
+
+ if (ImageBase == NULL) {
+ return (ElfCt->ParseStatus = EFI_INVALID_PARAMETER);
If I see it correctly, all instances that can assign ParseStatus also
return it. Why is the member needed at all?
I expect that caller needs to call ParseElfImage() to get the ParseStatus
properly assigned before calling LoadElfImage().
But it just throws back the error without doing anything as far as I can
see. For the new PE loader, there are "PeCoffInitializeContext" (more or
less "ParseElfImage") and "PeCoffLoadImage" (more or less
"LoadElfImage"), and there is a precondition to not call latter when
former error'd.
A minimal caller cal look like this:

Status = PeCoffInitializeContext (&Context, FileBuffer, FileSize);
if (RETURN_ERROR (Status)) {
return Status;
}

// [ ... hash image, allocate destination, and so on ... ]

PeCoffLoadImage (Context, Destination, DestinationSize);

The load function is never invoked if the init function fails. This
gives an intuitive and easy-to-comprehend control flow. The old PE lib
also has a status member in the context, and it was one of the first
things I went away with. Callers should not read from the context, and
callees have clear contracts.
Without the ParseStatus field, callee cannot know whether ParseElfImage() is called.
It can by function contracts, the caller guarantees it. I.e. with the PE library I linked, no other function must be called before the init function.
Your "ParseElfImage" function is very similar. The context is initialized by it, i.e. it is trash if it is not called, i.e. it must be called before other functions.
If it is called, which we know, the caller has the return status. For PE, it means the caller must not proceed with any further PE processing and abort immediately.
Is there any scenario where this does not work for ELF? Sorry if I missed something.

Best regards,
Marvin

There are several APIs which all depend on the well format of ELF image.
For example:
GetElfSectionName
GetElfSectionPos
LoadElfImage

If the ParseStatus is removed, all above API implementations need to call
ParseElfImage() again internally to make sure the ELF image is well formatted.

Caller doesn't need to read the ParseStatus. It just need to check the return
status of API calls.

The member ParseStatus is checked in LoadElfImage() later.
Today it's just PayloadLoaderPeim that calls the ElfLib functions.
But I expect that the ElfLib functions can be public lib APIs in future
if needs appear.


+ Name = (CHAR8 *)(ElfCt->FileBase + ElfCt->ShStrOff + Elf64Shdr-
sh_name);
0-termination checks, or return size?
I will validate the string section in ParseElfImage(). The validation logic will:
1. Verify that each section name is pointed from the e_shstrndx
2. Verify that section name strings don't occupy spaces outside of the string
section.

+
+ ZeroMem (&Context, sizeof (Context));
This is done by the callee already.
Indeed. Will remove this.
Rest looks good, thanks a lot!

If you have some time, please consider checking the rest for similar
issues. I maybe could help with it if you would like that, but not right
now, sorry. :)

Best regards,
Marvin



Re: [PATCH] MdeModulePkg: Fix device path when the boot manager menu is from different FV

Ni, Ray
 

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

-----Original Message-----
From: Liu, Zhiguang <zhiguang.liu@intel.com>
Sent: Wednesday, June 9, 2021 5:37 PM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: [PATCH] MdeModulePkg: Fix device path when the boot manager
menu is from different FV

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

When the boot manager menu is from different FV, the current logic still use
the
device path of the FV as the module links to this library

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 28 +++-------------
------------
1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index bef41ae102..95d185b639 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -2405,13 +2405,9 @@ BmRegisterBootManagerMenu (
CHAR16 *Description;

UINTN DescriptionLength;

EFI_DEVICE_PATH_PROTOCOL *DevicePath;

- EFI_LOADED_IMAGE_PROTOCOL *LoadedImage;

- MEDIA_FW_VOL_FILEPATH_DEVICE_PATH FileNode;

UINTN HandleCount;

EFI_HANDLE *Handles;

UINTN Index;

- VOID *Data;

- UINTN DataSize;



DevicePath = NULL;

Description = NULL;

@@ -2437,22 +2433,17 @@ BmRegisterBootManagerMenu (
}



if (DevicePath == NULL) {

- Data = NULL;

- Status = GetSectionFromAnyFv (

+ Status = GetFileDevicePathFromAnyFv (

PcdGetPtr (PcdBootManagerMenuFile),

EFI_SECTION_PE32,

0,

- (VOID **) &Data,

- &DataSize

+ &DevicePath

);

- if (Data != NULL) {

- FreePool (Data);

- }

if (EFI_ERROR (Status)) {

DEBUG ((EFI_D_WARN, "[Bds]BootManagerMenu FFS section can not be
found, skip its boot option registration\n"));

return EFI_NOT_FOUND;

}

-

+ ASSERT (DevicePath != NULL);

//

// Get BootManagerMenu application's description from EFI User
Interface Section.

//

@@ -2466,19 +2457,6 @@ BmRegisterBootManagerMenu (
if (EFI_ERROR (Status)) {

Description = NULL;

}

-

- EfiInitializeFwVolDevicepathNode (&FileNode, PcdGetPtr
(PcdBootManagerMenuFile));

- Status = gBS->HandleProtocol (

- gImageHandle,

- &gEfiLoadedImageProtocolGuid,

- (VOID **) &LoadedImage

- );

- ASSERT_EFI_ERROR (Status);

- DevicePath = AppendDevicePathNode (

- DevicePathFromHandle (LoadedImage->DeviceHandle),

- (EFI_DEVICE_PATH_PROTOCOL *) &FileNode

- );

- ASSERT (DevicePath != NULL);

}



Status = EfiBootManagerInitializeLoadOption (

--
2.30.0.windows.2


Re: [PATCH v2 2/3] UefiPayloadPkg: Add PayloadLoaderPeim which can load ELF payload

Ni, Ray
 

Thank you for your quick reply, comments inline.
I have to be quick because my project depends on the check-in of this code😊

+ for ( Index = 0
+ ; RelaEntrySize * Index < RelaSize
Overflow?
Will change from:
RelaEntrySize * Index < RelaSize
to:
Index < RelaSize / RelaEntrySize
imo add ASSERT for RelaEntrySize > 0 then.
Sure. To avoid dividend by zero error.

Basically the alignment of any offset with which a pointer to
non-trivially-aligned (i.e. requirement > 1 Byte) data can be forged
must be checked.

Examples from our new PE loader:
https://github.com/mhaeuser/ISPRASOpen-
SecurePE/tree/6a7abcd8647cf6faa733082f6d8dcc9adc141d7e/src/PeCoffInit.
c#L1226
->
https://github.com/mhaeuser/ISPRASOpen-
SecurePE/tree/6a7abcd8647cf6faa733082f6d8dcc9adc141d7e/src/PeCoffInit.
c#L1242
https://github.com/mhaeuser/ISPRASOpen-
SecurePE/tree/6a7abcd8647cf6faa733082f6d8dcc9adc141d7e/src/PeCoffInit.
c#L1389
->
https://github.com/mhaeuser/ISPRASOpen-
SecurePE/tree/6a7abcd8647cf6faa733082f6d8dcc9adc141d7e/src/PeCoffInit.
c#L148

The idea here is that the base pointer (i.e. image header) is "maximally
aligned" (i.e. can hold data of any platform data alignment). For the 8
Bytes AllocatePool() guarantees (file data), this is sufficient for any
primitive and composite data type. For the 4 KB AllocatePages()
guarantees (destination), this is sufficient of that, and for advanced
things like AVX (however not needed here). If the base is maximally
aligned, Base + X is guaranteed aligned for A if X is aligned for A,
i.e. X % _Alignof(A) = 0. Failing to verify this can cause exceptions on
platforms which don't support or have disabled the capability to perform
unaligned memory accesses.
I understand now. I remember that X86 contains a control flag that can trigger
CPU exception as well when unaligned access happens.

But adding such check in all places might require a huge change to today's code.
Can you accept that I ignore such check for now and add it later?



+ //
+ // It's abnormal a DYN ELF doesn't contain a dynamic section.
+ //
+ ASSERT (DynShdr != NULL);
+ if (DynShdr == NULL) {
+ return EFI_UNSUPPORTED;
+ }
+ ASSERT (DynShdr->sh_type == SHT_DYNAMIC);
+ ASSERT (DynShdr->sh_entsize >= sizeof (*Dyn));
Abnormalities in unknown/untrusted data must be filtered with a runtime
check, not with an ASSERT.
Sure. I will add if-check below the assertion so assertion-enabled build can
report the errors earlier.
I have started this discussion under another patch, maybe I should write
a broader-scope mail to the list for comments. Basically using ASSERTs
for anything but *impossible* (*not* assuming the input data is
well-formed) situations significantly reduces the efficacy of dynamic
analysis. When doing fuzzing for example, you want to keep the ASSERTs
enabled to be made aware of any internal invariant violations. But if
ASSERTs happen on possible conditions, they will kill the fuzzing
process for no good reason. Turning them off will not analyse your
ASSERTs for possible code defects.

Maybe fuzzing would be a good idea for this library? :)
I understand now. I am ok to remove assertion for external inputs.



+ for ( Index = 0, Dyn = (Elf32_Dyn *) (ElfCt->FileBase + DynShdr-
sh_offset)
+ ; Index < DynShdr->sh_size / DynShdr->sh_entsize
Is "sh_entsize" checked for 0?
No need. Because code above makes sure sh_entsize >= sizeof (*Dyn).
When you turn it into a runtime check as discussed above, yes.



+ ASSERT (RelShdr->sh_type == RelaType);
+ ASSERT (RelShdr->sh_entsize == RelaEntrySize);
See above.
Agree. Will add if-checks.


+ DEBUG ((DEBUG_INFO, "DYN ELF: Relocate using dynamic
sections...\n"));
+ Status = RelocateElf32Dynamic (ElfCt);
+ ASSERT_EFI_ERROR (Status);
Why cannot this fail?
A DYN type ELF image should contain dynamic section. So only an abnormal
ELF image can fail.

Same ASSERT point as above, "cannot" refers to both well-formed and
ill-formed images.
Sure. Will remove assertion.



+ return (Elf64_Phdr *)(ImageBase + Ehdr->e_phoff + Index * Ehdr-
e_phentsize);
Alignment checks? Bounds checks?
For the alignment checks, do you suggest that I should make sure the
segment should be placed
in the address that meets the alignment requirement?
It could be implemented, PE code does it, but I meant pointer alignment
as discussed above somewhere. I don't think ELFs would likely request
more than page alignment, but abort + DEBUG message sounds like a good
idea.

ELF spec just requires below for Elf64_Phdr.p_align:
loadable process segments must have congruent values for p_vaddr and
p_offset, modulo the page size.

I can add such check in ParseElfImage().

+ ProcessRelocation64 (
+ (Elf64_Rela *) (ElfCt->FileBase + RelShdr->sh_offset),
Alignment? :) I know there is no real concept in EDK II yet, but it
really is needed.
Can you explain a bit more on the alignment?
Done above, sorry.



+
+/**
+ Check if the ELF image is valid.
+
+ @param[in] ImageBase Memory address of an image.
+
+ @retval TRUE if valid.
+
+**/
+BOOLEAN
+IsElfFormat (
+ IN CONST UINT8 *ImageBase
You cannot safely inspect untrusted/unknown data without a size field,
also needs checks below.
Agree. Original idea was to add a ELF loader that can load the ELF assuming
the ELF image is well-formatted.
I get that idea, but the reality is that people will start using it for
external images once it is needed. :)
Sorry for being pedantic.


But with your help, I am glad to enhance the logic a bit more to expand
the support of external ELF images.

Will add a "UINTN ImageSize" parameter.

+ )
+{
+ Elf32_Ehdr *Elf32Hdr;
+ Elf64_Ehdr *Elf64Hdr;
+
+ ASSERT (ImageBase != NULL);
+
+ Elf32Hdr = (Elf32_Ehdr *)ImageBase;
+
+ //
+ // Start with correct signature "\7fELF"
+ //
+ if ((Elf32Hdr->e_ident[EI_MAG0] != ELFMAG0) ||
+ (Elf32Hdr->e_ident[EI_MAG1] != ELFMAG1) ||
+ (Elf32Hdr->e_ident[EI_MAG1] != ELFMAG1) ||
+ (Elf32Hdr->e_ident[EI_MAG2] != ELFMAG2)
+ ) {
+ return FALSE;
+ }
+
+ //
+ // Support little-endian only
+ //
+ if (Elf32Hdr->e_ident[EI_DATA] != ELFDATA2LSB) {
+ return FALSE;
+ }
+
+ //
+ // Check 32/64-bit architecture
+ //
+ if (Elf32Hdr->e_ident[EI_CLASS] == ELFCLASS64) {
+ Elf64Hdr = (Elf64_Ehdr *)Elf32Hdr;
+ Elf32Hdr = NULL;
+ } else if (Elf32Hdr->e_ident[EI_CLASS] == ELFCLASS32) {
+ Elf64Hdr = NULL;
+ } else {
+ return FALSE;
+ }
Why are the branches above and below separated when they map
basically 1:1?
Indeed. Thanks for catching this.
Will merge the separate "if" together.

+
+ if (Elf64Hdr != NULL) {
+ //
+ // Support intel architecture only for now
+ //
+ if (Elf64Hdr->e_machine != EM_X86_64) {
+ return FALSE;
+ }
+
+ // Use last section as end of file
+ Status = GetElfSectionPos (ElfCt, ElfCt->ShNum - 1, &Offset, &Size);
What if ShNum is 0?
Agree. The logic to calculate file size might not be needed.
Let me confirm and try to remove the entire function.


+ if (ElfCt->EiClass == ELFCLASS32) {
+ Elf32Hdr = (Elf32_Ehdr *)ElfCt->FileBase;
+ FileSize2 = Elf32Hdr->e_shoff + Elf32Hdr->e_shentsize * Elf32Hdr-
e_shnum;
+ } else if (ElfCt->EiClass == ELFCLASS64) {
+ Elf64Hdr = (Elf64_Ehdr *)ElfCt->FileBase;
+ FileSize2 = (UINTN)(Elf64Hdr->e_shoff + Elf64Hdr->e_shentsize *
Elf64Hdr->e_shnum);
+ }
Overflows?
Integer overflow?
Yes, sorry.

Will add integer overflow check if this file size calculation logic is still needed.


+
+ if (ElfCt == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
As this is function contract, I'd replace this with an ASSERT, or at
least have both.
I will put "ASSERT (ElfCt != NULL)" above the if.


+ ZeroMem (ElfCt, sizeof(ELF_IMAGE_CONTEXT));
+
+ if (ImageBase == NULL) {
+ return (ElfCt->ParseStatus = EFI_INVALID_PARAMETER);
If I see it correctly, all instances that can assign ParseStatus also
return it. Why is the member needed at all?
I expect that caller needs to call ParseElfImage() to get the ParseStatus
properly assigned before calling LoadElfImage().
But it just throws back the error without doing anything as far as I can
see. For the new PE loader, there are "PeCoffInitializeContext" (more or
less "ParseElfImage") and "PeCoffLoadImage" (more or less
"LoadElfImage"), and there is a precondition to not call latter when
former error'd.
A minimal caller cal look like this:

Status = PeCoffInitializeContext (&Context, FileBuffer, FileSize);
if (RETURN_ERROR (Status)) {
return Status;
}

// [ ... hash image, allocate destination, and so on ... ]

PeCoffLoadImage (Context, Destination, DestinationSize);

The load function is never invoked if the init function fails. This
gives an intuitive and easy-to-comprehend control flow. The old PE lib
also has a status member in the context, and it was one of the first
things I went away with. Callers should not read from the context, and
callees have clear contracts.
Without the ParseStatus field, callee cannot know whether ParseElfImage() is called.
There are several APIs which all depend on the well format of ELF image.
For example:
GetElfSectionName
GetElfSectionPos
LoadElfImage

If the ParseStatus is removed, all above API implementations need to call
ParseElfImage() again internally to make sure the ELF image is well formatted.

Caller doesn't need to read the ParseStatus. It just need to check the return
status of API calls.


The member ParseStatus is checked in LoadElfImage() later.
Today it's just PayloadLoaderPeim that calls the ElfLib functions.
But I expect that the ElfLib functions can be public lib APIs in future
if needs appear.


+ Name = (CHAR8 *)(ElfCt->FileBase + ElfCt->ShStrOff + Elf64Shdr-
sh_name);
0-termination checks, or return size?
I will validate the string section in ParseElfImage(). The validation logic will:
1. Verify that each section name is pointed from the e_shstrndx
2. Verify that section name strings don't occupy spaces outside of the string
section.


+
+ ZeroMem (&Context, sizeof (Context));
This is done by the callee already.
Indeed. Will remove this.
Rest looks good, thanks a lot!

If you have some time, please consider checking the rest for similar
issues. I maybe could help with it if you would like that, but not right
now, sorry. :)

Best regards,
Marvin


[PATCH] MdeModulePkg: Fix device path when the boot manager menu is from different FV

Zhiguang Liu
 

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

When the boot manager menu is from different FV, the current logic still us=
e the
device path of the FV as the module links to this library

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 28 +++-----------------=
--------
1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePk=
g/Library/UefiBootManagerLib/BmBoot.c
index bef41ae102..95d185b639 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -2405,13 +2405,9 @@ BmRegisterBootManagerMenu (
CHAR16 *Description;=0D
UINTN DescriptionLength;=0D
EFI_DEVICE_PATH_PROTOCOL *DevicePath;=0D
- EFI_LOADED_IMAGE_PROTOCOL *LoadedImage;=0D
- MEDIA_FW_VOL_FILEPATH_DEVICE_PATH FileNode;=0D
UINTN HandleCount;=0D
EFI_HANDLE *Handles;=0D
UINTN Index;=0D
- VOID *Data;=0D
- UINTN DataSize;=0D
=0D
DevicePath =3D NULL;=0D
Description =3D NULL;=0D
@@ -2437,22 +2433,17 @@ BmRegisterBootManagerMenu (
}=0D
=0D
if (DevicePath =3D=3D NULL) {=0D
- Data =3D NULL;=0D
- Status =3D GetSectionFromAnyFv (=0D
+ Status =3D GetFileDevicePathFromAnyFv (=0D
PcdGetPtr (PcdBootManagerMenuFile),=0D
EFI_SECTION_PE32,=0D
0,=0D
- (VOID **) &Data,=0D
- &DataSize=0D
+ &DevicePath=0D
);=0D
- if (Data !=3D NULL) {=0D
- FreePool (Data);=0D
- }=0D
if (EFI_ERROR (Status)) {=0D
DEBUG ((EFI_D_WARN, "[Bds]BootManagerMenu FFS section can not be fou=
nd, skip its boot option registration\n"));=0D
return EFI_NOT_FOUND;=0D
}=0D
-=0D
+ ASSERT (DevicePath !=3D NULL);=0D
//=0D
// Get BootManagerMenu application's description from EFI User Interfa=
ce Section.=0D
//=0D
@@ -2466,19 +2457,6 @@ BmRegisterBootManagerMenu (
if (EFI_ERROR (Status)) {=0D
Description =3D NULL;=0D
}=0D
-=0D
- EfiInitializeFwVolDevicepathNode (&FileNode, PcdGetPtr (PcdBootManager=
MenuFile));=0D
- Status =3D gBS->HandleProtocol (=0D
- gImageHandle,=0D
- &gEfiLoadedImageProtocolGuid,=0D
- (VOID **) &LoadedImage=0D
- );=0D
- ASSERT_EFI_ERROR (Status);=0D
- DevicePath =3D AppendDevicePathNode (=0D
- DevicePathFromHandle (LoadedImage->DeviceHandle),=0D
- (EFI_DEVICE_PATH_PROTOCOL *) &FileNode=0D
- );=0D
- ASSERT (DevicePath !=3D NULL);=0D
}=0D
=0D
Status =3D EfiBootManagerInitializeLoadOption (=0D
--=20
2.30.0.windows.2


Re: [Patch] StandaloneMmPkg: Fixed communicating from TF-A failed issue

Ard Biesheuvel
 

On Tue, 8 Jun 2021 at 16:21, Ming Huang <huangming@linux.alibaba.com> wrote:

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

For AArch64, when SPM enable in TF-A, TF-A may communicate to MM
with buffer address (PLAT_SPM_BUF_BASE). The address is different
from PcdMmBufferBase which use in edk2.
Then why do we have PcdMmBufferBase?

Is it possible to set PcdMmBufferBase to the correct value?

Checking address will let TF-A communicate failed to MM. So remove
below checking code:
if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
return EFI_ACCESS_DENIED;
}

Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
---
StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c | 4 ----
1 file changed, 4 deletions(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
index 63fbe26642..fe98d3181d 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
@@ -103,10 +103,6 @@ PiMmStandaloneArmTfCpuDriverEntry (
return EFI_INVALID_PARAMETER;
}

- if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
- return EFI_ACCESS_DENIED;
- }
-
if ((NsCommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
(mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
return EFI_INVALID_PARAMETER;
--
2.17.1


Re: [Patch V3 1/9] MdeModulePkg: Add Universal Payload general defination header file

Zhiguang Liu
 

Hi all,

Thanks for the comments.
I agree with the change.
I will update the term in next version of the patch set.

Thanks
Zhiguang

-----Original Message-----
From: Ma, Maurice <maurice.ma@intel.com>
Sent: Wednesday, June 9, 2021 11:08 AM
To: Ni, Ray <ray.ni@intel.com>; Dong, Guo <guo.dong@intel.com>;
Rangarajan, Ravi P <ravi.p.rangarajan@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>;
Liu, Zhiguang <zhiguang.liu@intel.com>; Zimmer, Vincent
<vincent.zimmer@intel.com>
Subject: RE: [edk2-devel] [Patch V3 1/9] MdeModulePkg: Add Universal
Payload general defination header file

Hi, Ray,

Yes, I agree. PLD might cause confusion sometimes.
Maybe we just use the full name UNIVERSAL_PAYLOAD instead ?

Thanks
Maurice
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, June 8, 2021 19:58
To: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
<guo.dong@intel.com>; Rangarajan, Ravi P <ravi.p.rangarajan@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>;
Liu, Zhiguang <zhiguang.liu@intel.com>; Zimmer, Vincent
<vincent.zimmer@intel.com>
Subject: RE: [edk2-devel] [Patch V3 1/9] MdeModulePkg: Add Universal
Payload general defination header file

Mike,
Thanks for the recommendation.
Just check https://en.wikipedia.org/wiki/Programmable_logic_device and
it seems to me PLD is a very common term in EE world. FPGA is a kind of
PLD.

Maurice, Ravi, Guo, comments?

Thanks,
Ray

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Michael
D Kinney
Sent: Wednesday, June 9, 2021 12:26 AM
To: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io;
Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: Re: [edk2-devel] [Patch V3 1/9] MdeModulePkg: Add Universal
Payload general defination header file

I see use of the abbreviation PLD in this series.

PLD is sometimes interpreted as Programmable Logic Device.

Given this is for Universal Payload, I recommend using
UNIVERSAL_PAYLOAD or PAYLOAD as appropriate.

Mike

-----Original Message-----
From: Liu, Zhiguang <zhiguang.liu@intel.com>
Sent: Friday, June 4, 2021 2:42 AM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
<gaoliming@byosoft.com.cn>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: [Patch V3 1/9] MdeModulePkg: Add Universal Payload
general
defination header file

V1:
Add Universal Payload general defination header file according to
Universal Payload’s documentation
V2:
Add a macro funtion to check the Revision

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>

Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdeModulePkg/Include/UniversalPayload/UniversalPayload.h | 33
+++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)

diff --git
a/MdeModulePkg/Include/UniversalPayload/UniversalPayload.h
b/MdeModulePkg/Include/UniversalPayload/UniversalPayload.h
new file mode 100644
index 0000000000..627b9e880e
--- /dev/null
+++ b/MdeModulePkg/Include/UniversalPayload/UniversalPayload.h
@@ -0,0 +1,33 @@
+/** @file

+ Universal Payload general definations.

+

+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>

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

+

+**/

+

+#ifndef __UNIVERSAL_PAYLOAD_H__

+#define __UNIVERSAL_PAYLOAD_H__

+

+#pragma pack(1)

+

+typedef struct {

+ UINT8 Revision;

+ UINT8 Reserved;

+ UINT16 Length;

+} PLD_GENERIC_HEADER;

+

+#pragma pack()

+

+/**

+ Returns the size of a structure of known type, up through and
+ including a
specified field.

+

+ @param TYPE The name of the data structure that contains the
field
specified by Field.

+ @param Field The name of the field in the data structure.

+

+ @return size, in bytes.

+

+**/

+#define PLD_SIZEOF_THROUGH_FIELD(TYPE, Field) (OFFSET_OF(TYPE,
Field) + sizeof (((TYPE *) 0)->Field))

+

+#endif // __UNIVERSAL_PAYLOAD_H__

--
2.30.0.windows.2




Re: [edk2-platforms][PATCH v2 16/32] AmpereAltraPkg: Add PciHostBridge driver

Ard Biesheuvel
 

On Wed, 26 May 2021 at 12:12, Nhi Pham <nhi@os.amperecomputing.com> wrote:

From: Vu Nguyen <vunguyen@os.amperecomputing.com>

The roles of this driver:
* Consume PcieCoreLib to initialize all enable PCIe controllers.
* Produce neccessary protocols like RootBridgeIo an ResourceAllocation
which will be used later by PciBus.

Cc: Thang Nguyen <thang@os.amperecomputing.com>
Cc: Chuong Tran <chuong@os.amperecomputing.com>
Cc: Phong Vo <phong@os.amperecomputing.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>

Signed-off-by: Vu Nguyen <vunguyen@os.amperecomputing.com>
Why do you need a re-implementation of PciHostBridgeDxe for any of
this? There is very little h/w specific code there, and it is all
customizable using PciHostBridgeLib and PciSegmentLib (among others)

There are a couple of examples of this in edk2-platforms - please take
a look at those, and if that does not give you enough wiggle room,
let's see if we can accommodate your needs in PciHostBridgeDxe itself.

--
Ard.


[PATCH 2/2] OvmfPkg/VirtioMmioDeviceLib: Add virtio 1.0 support.

Gerd Hoffmann
 

Add support for virtio 1.0 to the mmio transport. virtio 0.9.5 uses
page size, page frame number and a fixed layout for the ring. virtio
1.0 uses the physical addresses for base address, used bits and
available bits instead.

The ring layout is not changed, so a 0.9.5 compatible layout is used in
both 0.9.5 and 1.0 mode to to keep the code differences as small as
possible.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
.../VirtioMmioDeviceLib/VirtioMmioDevice.h | 1 +
.../VirtioMmioDeviceLib/VirtioMmioDevice.c | 17 +++++++---
.../VirtioMmioDeviceFunctions.c | 31 +++++++++++++++++--
3 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
index ab53b90d51c9..8b19996b716f 100644
--- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
+++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
@@ -26,6 +26,7 @@

typedef struct {
UINT32 Signature;
+ UINT32 Version;
VIRTIO_DEVICE_PROTOCOL VirtioDevice;
PHYSICAL_ADDRESS BaseAddress;
} VIRTIO_MMIO_DEVICE;
diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
index 6dbbba008c75..a8f78a50861b 100644
--- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
+++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
@@ -58,7 +58,6 @@ VirtioMmioInit (
)
{
UINT32 MagicValue;
- UINT32 Version;

//
// Initialize VirtIo Mmio Device
@@ -66,7 +65,6 @@ VirtioMmioInit (
CopyMem (&Device->VirtioDevice, &mMmioDeviceProtocolTemplate,
sizeof (VIRTIO_DEVICE_PROTOCOL));
Device->BaseAddress = BaseAddress;
- Device->VirtioDevice.Revision = VIRTIO_SPEC_REVISION (0, 9, 5);
Device->VirtioDevice.SubSystemDeviceId =
MmioRead32 (BaseAddress + VIRTIO_MMIO_OFFSET_DEVICE_ID);

@@ -78,8 +76,19 @@ VirtioMmioInit (
return EFI_UNSUPPORTED;
}

- Version = VIRTIO_CFG_READ (Device, VIRTIO_MMIO_OFFSET_VERSION);
- if (Version != 1) {
+ Device->Version = VIRTIO_CFG_READ (Device, VIRTIO_MMIO_OFFSET_VERSION);
+ switch (Device->Version) {
+ case 1:
+ DEBUG ((DEBUG_INFO, "%a virtio 0.9.5, id %d\n", __FUNCTION__,
+ Device->VirtioDevice.SubSystemDeviceId));
+ Device->VirtioDevice.Revision = VIRTIO_SPEC_REVISION (0, 9, 5);
+ break;
+ case 2:
+ DEBUG ((DEBUG_INFO, "%a virtio 1.0, id %d\n", __FUNCTION__,
+ Device->VirtioDevice.SubSystemDeviceId));
+ Device->VirtioDevice.Revision = VIRTIO_SPEC_REVISION (1, 0, 0);
+ break;
+ default:
return EFI_UNSUPPORTED;
}

diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
index b0d75fb1dd24..bf8523a6fb3b 100644
--- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
+++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
@@ -151,7 +151,9 @@ VirtioMmioSetPageSize (

Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);

- VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_GUEST_PAGE_SIZE, PageSize);
+ if (Device->Version == 1) {
+ VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_GUEST_PAGE_SIZE, PageSize);
+ }

return EFI_SUCCESS;
}
@@ -181,13 +183,36 @@ VirtioMmioSetQueueAddress (
)
{
VIRTIO_MMIO_DEVICE *Device;
+ UINT64 Address;

ASSERT (RingBaseShift == 0);

Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);

- VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_PFN,
- (UINT32)((UINTN)Ring->Base >> EFI_PAGE_SHIFT));
+ if (Device->Version == 1) {
+ VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_PFN,
+ (UINT32)((UINTN)Ring->Base >> EFI_PAGE_SHIFT));
+ } else {
+ Address = (UINT64)Ring->Base;
+ VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_DESC_LO,
+ (UINT32)Address);
+ VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_DESC_HI,
+ (UINT32)RShiftU64(Address, 32));
+
+ Address = (UINT64)Ring->Avail.Flags;
+ VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_AVAIL_LO,
+ (UINT32)Address);
+ VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_AVAIL_HI,
+ (UINT32)RShiftU64(Address, 32));
+
+ Address = (UINT64)Ring->Used.Flags;
+ VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_USED_LO,
+ (UINT32)Address);
+ VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_USED_HI,
+ (UINT32)RShiftU64(Address, 32));
+
+ VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_READY, 1);
+ }

return EFI_SUCCESS;
}
--
2.31.1


[PATCH 1/2] OvmfPkg/Virtio10: Add virtio-mmio 1.0 defines

Gerd Hoffmann
 

Add defines for the config space offsets for virtio 1.0 mmio transport.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
OvmfPkg/Include/IndustryStandard/Virtio10.h | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/OvmfPkg/Include/IndustryStandard/Virtio10.h b/OvmfPkg/Include/IndustryStandard/Virtio10.h
index 2c60be2b7c0c..a1712247e054 100644
--- a/OvmfPkg/Include/IndustryStandard/Virtio10.h
+++ b/OvmfPkg/Include/IndustryStandard/Virtio10.h
@@ -81,4 +81,16 @@ typedef struct {
#define VIRTIO_F_VERSION_1 BIT32
#define VIRTIO_F_IOMMU_PLATFORM BIT33

+//
+// MMIO VirtIo Header Offsets
+//
+#define VIRTIO_MMIO_OFFSET_QUEUE_READY 0x44
+#define VIRTIO_MMIO_OFFSET_QUEUE_DESC_LO 0x80
+#define VIRTIO_MMIO_OFFSET_QUEUE_DESC_HI 0x84
+#define VIRTIO_MMIO_OFFSET_QUEUE_AVAIL_LO 0x90
+#define VIRTIO_MMIO_OFFSET_QUEUE_AVAIL_HI 0x94
+#define VIRTIO_MMIO_OFFSET_QUEUE_USED_LO 0xa0
+#define VIRTIO_MMIO_OFFSET_QUEUE_USED_HI 0xa4
+#define VIRTIO_MMIO_OFFSET_CONFIG_GENERATION 0xfc
+
#endif // _VIRTIO_1_0_H_
--
2.31.1


[PATCH 0/2] Quo vadis virtio-mmio?

Gerd Hoffmann
 

virtio-mmio support in ovmf seems to be the unloved child. The final
virto-1.0 specification was published five(!) years ago, nevertheless
the mmio transport doesn't support it yet ...

Some people argue that it has been obsoleted by virtio-pci. Which is a
valid argument. But IMHO isn't a good reason to just let virtio-mmio
bitrot. We should either remove it from the tree, or support it.

So, opening the discussion with this little patch series. It does the
latter and adds virtio 1.0 support. For the mmio transport the
difference between 0.9.5 and 1.0 is rather small (when compared to the
pci transport), it is just a bunch of new registers for the changed
virtio queue initialization. So the patch series is small too ...

take care,
Gerd

Gerd Hoffmann (2):
OvmfPkg/Virtio10: Add virtio-mmio 1.0 defines
OvmfPkg/VirtioMmioDeviceLib: Add virtio 1.0 support.

OvmfPkg/Include/IndustryStandard/Virtio10.h | 12 +++++++
.../VirtioMmioDeviceLib/VirtioMmioDevice.h | 1 +
.../VirtioMmioDeviceLib/VirtioMmioDevice.c | 17 +++++++---
.../VirtioMmioDeviceFunctions.c | 31 +++++++++++++++++--
4 files changed, 54 insertions(+), 7 deletions(-)

--
2.31.1


Re: [edk2-platforms][PATCH v2 01/32] Ampere: Initial support for Ampere Altra processor and Mt. Jade platform

Nhi Pham
 

On 6/5/21 06:04, Leif Lindholm wrote:
On Wed, May 26, 2021 at 17:06:52 +0700, Nhi Pham wrote:
From: Vu Nguyen <vunguyen@...>

This commit adds the support for Ampere’s Altra processor-based Mt. Jade
platform that provides up to 160 processor cores in a dual socket
configuration. The essential modules are wired up enough to boot system
to EDK2 UiApp.

Cc: Thang Nguyen <thang@...>
Cc: Chuong Tran <chuong@...>
Cc: Phong Vo <phong@...>
Cc: Leif Lindholm <leif@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Nate DeSimone <nathaniel.l.desimone@...>

Signed-off-by: Vu Nguyen <vunguyen@...>
---
 Platform/Ampere/AmperePlatformPkg/AmperePlatformPkg.dec                                         |  28 +
 Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dec                                                |  42 ++
 Silicon/Ampere/AmpereSiliconPkg/AmpereSiliconPkg.dec                                            |  46 ++
 Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc                                            | 674 +++++++++++++++++++
 Platform/Ampere/JadePkg/Jade.dsc                                                                | 100 +++
 Platform/Ampere/JadePkg/Jade.fdf                                                                | 225 +++++++
 Silicon/Ampere/AmpereAltraPkg/Drivers/ATFHobPei/ATFHobPeim.inf                                  |  41 ++
 Silicon/Ampere/AmpereAltraPkg/Drivers/MemoryInitPeim/MemoryInitPeim.inf                         |  64 ++
 Silicon/Ampere/AmpereAltraPkg/Library/AmpereCpuLib/AmpereCpuLib.inf                             |  44 ++
 Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.inf                         |  57 ++
 Silicon/Ampere/AmpereAltraPkg/Library/MailboxInterfaceLib/MailboxInterfaceLib.inf               |  37 +
 Silicon/Ampere/AmpereAltraPkg/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf                     |  63 ++
 Silicon/Ampere/AmpereAltraPkg/Library/MmCommunicationLib/MmCommunicationLib.inf                 |  35 +
 Silicon/Ampere/AmpereAltraPkg/Library/NVParamLib/NVParamLib.inf                                 |  32 +
 Silicon/Ampere/AmpereAltraPkg/Library/PlatformPeiLib/PlatformPeiLib.inf                         |  42 ++
 Silicon/Ampere/AmpereAltraPkg/Library/RngLib/RngLib.inf                                         |  29 +
 Silicon/Ampere/AmpereAltraPkg/Library/SystemFirmwareInterfaceLib/SystemFirmwareInterfaceLib.inf |  30 +
 Silicon/Ampere/AmpereAltraPkg/Library/TrngLib/TrngLib.inf                                       |  29 +
 Silicon/Ampere/AmpereAltraPkg/Include/Guid/PlatformInfoHobGuid.h                                |  17 +
 Silicon/Ampere/AmpereAltraPkg/Include/Library/AmpereCpuLib.h                                    | 282 ++++++++
 Silicon/Ampere/AmpereAltraPkg/Include/Library/MailboxInterfaceLib.h                             | 172 +++++
 Silicon/Ampere/AmpereAltraPkg/Include/Library/MmCommunicationLib.h                              |  19 +
 Silicon/Ampere/AmpereAltraPkg/Include/Library/NVParamLib.h                                      | 133 ++++
 Silicon/Ampere/AmpereAltraPkg/Include/Library/SystemFirmwareInterfaceLib.h                      | 282 ++++++++
 Silicon/Ampere/AmpereAltraPkg/Include/Library/TrngLib.h                                         |  31 +
 Silicon/Ampere/AmpereAltraPkg/Include/MmLib.h                                                   |  79 +++
 Silicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h                                              | 515 ++++++++++++++
 Silicon/Ampere/AmpereAltraPkg/Include/Platform/Ac01.h                                           | 146 ++++
 Silicon/Ampere/AmpereAltraPkg/Include/PlatformInfoHob.h                                         | 182 +++++
 Silicon/Ampere/AmpereAltraPkg/Drivers/ATFHobPei/ATFHobPeim.c                                    |  52 ++
 Silicon/Ampere/AmpereAltraPkg/Drivers/MemoryInitPeim/MemoryInitPeim.c                           | 151 +++++
 Silicon/Ampere/AmpereAltraPkg/Library/AmpereCpuLib/AmpereCpuLib.c                               | 706 ++++++++++++++++++++
 Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c                           | 169 +++++
 Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLibMemory.c                     | 399 +++++++++++
 Silicon/Ampere/AmpereAltraPkg/Library/MailboxInterfaceLib/MailboxInterfaceLib.c                 | 282 ++++++++
 Silicon/Ampere/AmpereAltraPkg/Library/MemoryInitPeiLib/MemoryInitPeiLib.c                       |  93 +++
 Silicon/Ampere/AmpereAltraPkg/Library/MmCommunicationLib/MmCommunicationLib.c                   | 184 +++++
 Silicon/Ampere/AmpereAltraPkg/Library/NVParamLib/NVParamLib.c                                   | 202 ++++++
 Silicon/Ampere/AmpereAltraPkg/Library/PlatformPeiLib/PlatformPeiLib.c                           |  40 ++
 Silicon/Ampere/AmpereAltraPkg/Library/RngLib/RngLib.c                                           | 141 ++++
 Silicon/Ampere/AmpereAltraPkg/Library/SystemFirmwareInterfaceLib/SystemFirmwareInterfaceLib.c   | 328 +++++++++
 Silicon/Ampere/AmpereAltraPkg/Library/TrngLib/TrngLib.c                                         |  63 ++
 Platform/Ampere/AmperePlatformPkg/FvRules.fdf.inc                                               | 176 +++++
 Platform/Ampere/JadePkg/JadeBoardSetting.cfg                                                    | 209 ++++++
 Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformHelper.S                        |  45 ++
 Silicon/Ampere/AmpereAltraPkg/Library/RngLib/RngLib.uni                                         |  13 +
 46 files changed, 6729 insertions(+)


      
diff --git a/Platform/Ampere/JadePkg/Jade.dsc b/Platform/Ampere/JadePkg/Jade.dsc
new file mode 100755
index 000000000000..f68af24a0d78
--- /dev/null
+++ b/Platform/Ampere/JadePkg/Jade.dsc
@@ -0,0 +1,100 @@
+## @file
+#
+# Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+################################################################################
+#
+# Defines Section - statements that will be processed to create a Makefile.
+#
+################################################################################
+[Defines]
+  PLATFORM_NAME                  = Jade
+  PLATFORM_GUID                  = 7BDD00C0-68F3-4CC1-8775-F0F00572019F
+  PLATFORM_VERSION               = 0.1
+  DSC_SPECIFICATION              = 0x0001001B
+  OUTPUT_DIRECTORY               = Build/Jade
+  SUPPORTED_ARCHITECTURES        = AARCH64
+  BUILD_TARGETS                  = DEBUG|RELEASE|NOOPT
+  SKUID_IDENTIFIER               = DEFAULT
+  FLASH_DEFINITION               = Platform/Ampere/JadePkg/Jade.fdf
+
+  #
+  # Defines for default states. These can be changed on the command line.
+  # -D FLAG=VALUE
+  #
+
+  #  DEBUG_INIT      0x00000001  // Initialization
+  #  DEBUG_WARN      0x00000002  // Warnings
+  #  DEBUG_LOAD      0x00000004  // Load events
+  #  DEBUG_FS        0x00000008  // EFI File system
+  #  DEBUG_POOL      0x00000010  // Alloc & Free (pool)
+  #  DEBUG_PAGE      0x00000020  // Alloc & Free (page)
+  #  DEBUG_INFO      0x00000040  // Informational debug messages
+  #  DEBUG_DISPATCH  0x00000080  // PEI/DXE/SMM Dispatchers
+  #  DEBUG_VARIABLE  0x00000100  // Variable
+  #  DEBUG_BM        0x00000400  // Boot Manager
+  #  DEBUG_BLKIO     0x00001000  // BlkIo Driver
+  #  DEBUG_NET       0x00004000  // SNP Driver
+  #  DEBUG_UNDI      0x00010000  // UNDI Driver
+  #  DEBUG_LOADFILE  0x00020000  // LoadFile
+  #  DEBUG_EVENT     0x00080000  // Event messages
+  #  DEBUG_GCD       0x00100000  // Global Coherency Database changes
+  #  DEBUG_CACHE     0x00200000  // Memory range cachability changes
+  #  DEBUG_VERBOSE   0x00400000  // Detailed debug messages that may
+  #                              // significantly impact boot performance
+  #  DEBUG_ERROR     0x80000000  // Error
+  DEFINE DEBUG_PRINT_ERROR_LEVEL = 0x8000000F
+  DEFINE FIRMWARE_VER            = 0.01.001
+  DEFINE EDK2_SKIP_PEICORE       = TRUE
+  DEFINE SECURE_BOOT_ENABLE      = FALSE
+  DEFINE INCLUDE_TFTP_COMMAND    = TRUE
+
+  #
+  # Network definition
+  #
+  DEFINE NETWORK_IP6_ENABLE                  = FALSE
+  DEFINE NETWORK_HTTP_BOOT_ENABLE            = TRUE
+  DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS      = TRUE
+  DEFINE NETWORK_TLS_ENABLE                  = FALSE
+
You'll want to include that !include MdePkg/MdeLibs.dsc.inc bit
somewhere here.

+# Include default Ampere Platform DSC file
+!include Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc
+
+################################################################################
+#
+# Specific Platform Library
+#
+################################################################################
+[LibraryClasses]
+  #
+  # RTC Library: Common RTC
+  #
+  RealTimeClockLib|EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf
+
+################################################################################
+#
+# Specific Platform Pcds
+#
+################################################################################
+[PcdsFeatureFlag.common]
+[PcdsFixedAtBuild.common]
+
+!if $(SECURE_BOOT_ENABLE) == TRUE
+  # Override the default values from SecurityPkg to ensure images
+  # from all sources are verified in secure boot
+  gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x04
+  gEfiSecurityPkgTokenSpaceGuid.PcdFixedMediaImageVerificationPolicy|0x04
+  gEfiSecurityPkgTokenSpaceGuid.PcdRemovableMediaImageVerificationPolicy|0x04
+!endif
+
+
+################################################################################
+#
+# Specific Platform Component
+#
+################################################################################
+[Components.common]


diff --git a/Silicon/Ampere/AmpereAltraPkg/Include/Library/AmpereCpuLib.h b/Silicon/Ampere/AmpereAltraPkg/Include/Library/AmpereCpuLib.h
new file mode 100644
index 000000000000..de576474fb48
--- /dev/null
+++ b/Silicon/Ampere/AmpereAltraPkg/Include/Library/AmpereCpuLib.h
@@ -0,0 +1,282 @@
+/** @file
+
+  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef AMPERE_CPU_LIB_H_
+#define AMPERE_CPU_LIB_H_
+
+/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
+#define CLIDR_CTYPE_SHIFT(Level)    (3 * (Level - 1))
+#define CLIDR_CTYPE_MASK(Level)     (7 << CLIDR_CTYPE_SHIFT(Level))
+#define CLIDR_CTYPE(Clidr, Level) \
+  (((Clidr) & CLIDR_CTYPE_MASK(Level)) >> CLIDR_CTYPE_SHIFT(Level))
+
+#define CCSIDR_NUMSETS_SHIFT        13
+#define CCSIDR_NUMSETS_MASK         0xFFFE000
+#define CCSIDR_NUMSETS(Ccsidr) \
+  (((Ccsidr) & CCSIDR_NUMSETS_MASK) >> CCSIDR_NUMSETS_SHIFT)
+#define CCSIDR_ASSOCIATIVITY_SHIFT  3
+#define CCSIDR_ASSOCIATIVITY_MASK   0x1FF8
+#define CCSIDR_ASSOCIATIVITY(Ccsidr) \
+  (((Ccsidr) & CCSIDR_ASSOCIATIVITY_MASK) >> CCSIDR_ASSOCIATIVITY_SHIFT)
+#define CCSIDR_LINE_SIZE_SHIFT      0
+#define CCSIDR_LINE_SIZE_MASK       0x7
+#define CCSIDR_LINE_SIZE(Ccsidr) \
+  (((Ccsidr) & CCSIDR_LINE_SIZE_MASK) >> CCSIDR_LINE_SIZE_SHIFT)
All of the above (CLIDR/CCSIDR) are architectural.
We've also developed some new accessors and stuff for these, as part
of ArmPkg/Universal/SmbiosDxe work that's gone on since v1 of this set.
We may need to clean some interfaces up, but ideally I'd prefer if you
could use some accessors from ArmLib or such.

+
+#define SUBNUMA_MODE_MONOLITHIC        0
+#define SUBNUMA_MODE_HEMISPHERE        1
+#define SUBNUMA_MODE_QUADRANT          2
+
+#define MONOLITIC_NUM_OF_REGION        1
+#define HEMISPHERE_NUM_OF_REGION       2
+#define QUADRANT_NUM_OF_REGION         4
+#define SUBNUMA_CPM_REGION_SIZE        4
+#define NUM_OF_CPM_PER_MESH_ROW        8
+
+#define CPM_PER_ROW_OFFSET(CpmId)      ((CpmId) % NUM_OF_CPM_PER_MESH_ROW)
+#define CPM_ROW_NUMBER(CpmId)          ((CpmId) / NUM_OF_CPM_PER_MESH_ROW)
+
+#define SOCKET_ID(CpuId)               ((CpuId) / (PLATFORM_CPU_MAX_CPM * PLATFORM_CPU_NUM_CORES_PER_CPM))
+#define CLUSTER_ID(CpuId)              (((CpuId) / PLATFORM_CPU_NUM_CORES_PER_CPM) % PLATFORM_CPU_MAX_CPM)
+
+
+/**
+  Get the SubNUMA mode.
+
+  @return   UINT8      The SubNUMA mode.
+
+**/
+UINT8
+EFIAPI
+CpuGetSubNumaMode (
+  VOID
+  );
+
+/**
+  Get the number of SubNUMA region.
+
+  @return   UINT8      The number of SubNUMA region.
+
+**/
+UINT8
+EFIAPI
+CpuGetNumberOfSubNumaRegion (
+  VOID
+  );
+
+/**
+  Get the SubNUMA node of a CPM.
+
+  @param    SocketId    Socket index.
+  @param    Cpm         CPM index.
+  @return   UINT8       The SubNUMA node of a CPM.
+
+**/
+UINT8
+EFIAPI
+CpuGetSubNumNode (
+  UINT8  Socket,
+  UINT16 Cpm
+  );
+
+/**
+  Get the associativity of cache.
+
+  @param    Level       Cache level.
+  @return   UINT32      Associativity of cache.
+
+**/
+UINT32
+EFIAPI
+CpuGetAssociativity (
+  UINT32 Level
+  );
+
+/**
+  Get the cache size.
+
+  @param    Level       Cache level.
+  @return   UINT32      Cache size.
+
+**/
+UINT32
+EFIAPI
+CpuGetCacheSize (
+  UINT32 Level
+  );
+
+/**
+  Get the number of supported socket.
+
+  @return   UINT8      Number of supported socket.
+
+**/
+UINT8
+EFIAPI
+GetNumberOfSupportedSockets (
+  VOID
+  );
+
+/**
+  Get the number of active socket.
+
+  @return   UINT8      Number of active socket.
+
+**/
+UINT8
+EFIAPI
+GetNumberOfActiveSockets (
+  VOID
+  );
+
+/**
+  Get the number of active CPM per socket.
+
+  @param    SocketId    Socket index.
+  @return   UINT16      Number of CPM.
+
+**/
+UINT16
+EFIAPI
+GetNumberOfActiveCPMsPerSocket (
+  UINT8 SocketId
+  );
+
+/**
+  Get the number of configured CPM per socket.
+
+  @param    SocketId    Socket index.
+  @return   UINT16      Number of configured CPM.
+
+**/
+UINT16
+EFIAPI
+GetNumberOfConfiguredCPMs (
+  UINT8 SocketId
+  );
+
+/**
+  Set the number of configured CPM per socket.
+
+  @param    SocketId        Socket index.
+  @param    NumberOfCPMs    Number of CPM to be configured.
+  @return   EFI_SUCCESS     Operation succeeded.
+  @return   Others          An error has occurred.
+
+**/
+EFI_STATUS
+EFIAPI
+SetNumberOfConfiguredCPMs (
+  UINT8  SocketId,
+  UINT16 NumberOfCPMs
+  );
+
+/**
+  Get the maximum number of core per socket. This number
+  should be the same for all sockets.
+
+  @return   UINT16      Maximum number of core.
+
+**/
+UINT16
+EFIAPI
+GetMaximumNumberOfCores (
+  VOID
+  );
+
+/**
+  Get the maximum number of CPM per socket. This number
+  should be the same for all sockets.
+
+  @return   UINT32      Maximum number of CPM.
+
+**/
+UINT16
+EFIAPI
+GetMaximumNumberOfCPMs (
+  VOID
+  );
+
+/**
+  Get the number of active cores of a sockets.
+
+  @return   UINT16      Number of active core.
+
+**/
+UINT16
+EFIAPI
+GetNumberOfActiveCoresPerSocket (
+  UINT8 SocketId
+  );
+
+/**
+  Get the number of active cores of all socket.
+
+  @return   UINT16      Number of active core.
+
+**/
+UINT16
+EFIAPI
+GetNumberOfActiveCores (
+  VOID
+  );
+
+/**
+  Check if the logical CPU is enabled or not.
+
+  @param    CpuId       The logical Cpu ID. Started from 0.
+  @return   BOOLEAN     TRUE if the Cpu enabled
+                        FALSE if the Cpu disabled.
+
+**/
+BOOLEAN
+EFIAPI
+IsCpuEnabled (
+  UINT16 CpuId
+  );
+
+
+/**
+  Check if the slave socket is present
+
+  @return   BOOLEAN     TRUE if the Slave Cpu is present
+                        FALSE if the Slave Cpu is not present
+
+**/
+BOOLEAN
+EFIAPI
+IsSlaveSocketPresent (
+  VOID
+  );
+
+/**
+  Check if the slave socket is active
+
+  @return   BOOLEAN     TRUE if the Slave CPU Socket is active.
+                        FALSE if the Slave CPU Socket is not active.
+
+**/
+BOOLEAN
+EFIAPI
+IsSlaveSocketActive (
+  VOID
+  );
+
+/**
+  Check if the CPU product ID is Ac01
+  @return   BOOLEAN     TRUE if the Product ID is Ac01
+                        FALSE otherwise.
+
+**/
+BOOLEAN
+EFIAPI
+IsAc01Processor (
+  VOID
+  );
+
+#endif /* AMPERE_CPU_LIB_H_ */

diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/AmpereCpuLib/AmpereCpuLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/AmpereCpuLib/AmpereCpuLib.c
new file mode 100644
index 000000000000..8da698e0b855
--- /dev/null
+++ b/Silicon/Ampere/AmpereAltraPkg/Library/AmpereCpuLib/AmpereCpuLib.c
@@ -0,0 +1,706 @@
+/** @file
+
+  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <PiPei.h>
+#include <Uefi.h>
+
+#include <Guid/PlatformInfoHobGuid.h>
+#include <Library/AmpereCpuLib.h>
+#include <Library/ArmLib/ArmLibPrivate.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/HobLib.h>
+#include <Library/IoLib.h>
+#include <Library/NVParamLib.h>
+#include <NVParamDef.h>
+#include <Platform/Ac01.h>
+#include <PlatformInfoHob.h>
+
+STATIC PLATFORM_INFO_HOB *mPlatformInfoHob = NULL;
+
+PLATFORM_INFO_HOB *
+GetPlatformHob (
+  VOID
+  )
+{
+  VOID *Hob;
+
+  if (mPlatformInfoHob == NULL) {
+    Hob = GetNextGuidHob (
+            &gPlatformHobGuid,
+            (CONST VOID *)FixedPcdGet64 (PcdSystemMemoryBase)
+            );
+    ASSERT (Hob != NULL);
+    if (Hob == NULL) {
+      DEBUG ((DEBUG_ERROR, "%a: Failed to get gPlatformHobGuid!\n", __FUNCTION__));
+      return NULL;
+    }
+
+    mPlatformInfoHob = (PLATFORM_INFO_HOB *)GET_GUID_HOB_DATA (Hob);
+  }
+
+  return mPlatformInfoHob;
+}
+
+/**
+  Get the SubNUMA mode.
+
+  @return   UINT8      The SubNUMA mode.
+
+**/
+UINT8
+EFIAPI
+CpuGetSubNumaMode (
+  VOID
+  )
+{
+  PLATFORM_INFO_HOB  *PlatformHob;
+
+  PlatformHob = GetPlatformHob ();
+  if (PlatformHob == NULL) {
+    return SUBNUMA_MODE_MONOLITHIC;
+  }
+
+  return PlatformHob->SubNumaMode[0];
+}
+
+/**
+  Get the number of SubNUMA region.
+
+  @return   UINT8      The number of SubNUMA region.
+
+**/
+UINT8
+EFIAPI
+CpuGetNumberOfSubNumaRegion (
+  VOID
+  )
+{
+  UINT8 SubNumaMode;
+  UINT8 NumberOfSubNumaRegion;
+
+  SubNumaMode = CpuGetSubNumaMode ();
+  ASSERT (SubNumaMode <= SUBNUMA_MODE_QUADRANT);
+
+  switch (SubNumaMode) {
+  case SUBNUMA_MODE_MONOLITHIC:
+    NumberOfSubNumaRegion = MONOLITIC_NUM_OF_REGION;
+    break;
+
+  case SUBNUMA_MODE_HEMISPHERE:
+    NumberOfSubNumaRegion = HEMISPHERE_NUM_OF_REGION;
+    break;
+
+  case SUBNUMA_MODE_QUADRANT:
+    NumberOfSubNumaRegion = QUADRANT_NUM_OF_REGION;
+    break;
+
+  default:
+    // Should never reach there.
+    ASSERT (FALSE);
+    break;
+  }
+
+  return NumberOfSubNumaRegion;
+}
+
+/**
+  Get the SubNUMA node of a CPM.
+
+  @param    SocketId    Socket index.
+  @param    Cpm         CPM index.
+  @return   UINT8       The SubNUMA node of a CPM.
+
+**/
+UINT8
+EFIAPI
+CpuGetSubNumNode (
+  UINT8  SocketId,
+  UINT16 Cpm
+  )
+{
+  BOOLEAN IsAsymMesh;
+  UINT8   SubNumaNode;
+  UINT16  MaxNumberOfCPM;
+  UINT8   MiddleRow;
+  UINT8   QuadrantHigherRowNodeNumber[NUM_OF_CPM_PER_MESH_ROW] = {1, 1, 1, 1, 3, 3, 3, 3};
+  UINT8   QuadrantLowerRowNodeNumber[NUM_OF_CPM_PER_MESH_ROW]  = {0, 0, 0, 0, 2, 2, 2, 2};
+  UINT8   QuadrantMiddleRowNodeNumber[NUM_OF_CPM_PER_MESH_ROW] = {0, 0, 1, 1, 3, 3, 2, 2};
+  UINT8   SubNumaMode;
+
+  MaxNumberOfCPM = GetMaximumNumberOfCPMs ();
+  SubNumaMode = CpuGetSubNumaMode ();
+  ASSERT (SubNumaMode <= SUBNUMA_MODE_QUADRANT);
+
+  switch (SubNumaMode) {
+  case SUBNUMA_MODE_MONOLITHIC:
+    SubNumaNode = (SocketId == 0) ? 0 : 1;
+    break;
+
+  case SUBNUMA_MODE_HEMISPHERE:
+    if (CPM_PER_ROW_OFFSET (Cpm) >= SUBNUMA_CPM_REGION_SIZE) {
+      SubNumaNode = 1;
+    } else {
+      SubNumaNode = 0;
+    }
+
+    if (SocketId == 1) {
+      SubNumaNode += HEMISPHERE_NUM_OF_REGION;
+    }
+    break;
+
+  case SUBNUMA_MODE_QUADRANT:
+    //
+    // CPM Mesh Rows
+    //
+    // |---------------------------------------|
+    // | 00 ----------- 03 | 04 ----------- 07 | Row 0
+    // |-------------------|-------------------|
+    // | 08 ----------- 11 | 12 ----------- 15 | Row 1
+    // |-------------------|-------------------|
+    // | 16 - 17 | 18 - 19 | 20 - 21 | 22 - 23 | Middle Row
+    // |-------------------|-------------------|
+    // | 24 ----------- 27 | 28 ----------- 31 | Row 3
+    // |-------------------|-------------------|
+    // | 32 ----------- 35 | 36 ----------- 39 | Row 4
+    // |---------------------------------------|
+    //
+
+    IsAsymMesh = (BOOLEAN)(CPM_ROW_NUMBER (MaxNumberOfCPM) % 2 != 0);
+    MiddleRow = CPM_ROW_NUMBER (MaxNumberOfCPM) / 2;
+    if (IsAsymMesh
+        && CPM_ROW_NUMBER (Cpm) == MiddleRow)
+    {
+      SubNumaNode = QuadrantMiddleRowNodeNumber[CPM_PER_ROW_OFFSET (Cpm)];
+
+    } else if (CPM_ROW_NUMBER (Cpm) >= MiddleRow) {
+      SubNumaNode = QuadrantHigherRowNodeNumber[CPM_PER_ROW_OFFSET (Cpm)];
+
+    } else {
+      SubNumaNode = QuadrantLowerRowNodeNumber[CPM_PER_ROW_OFFSET (Cpm)];
+    }
+
+    if (SocketId == 1) {
+      SubNumaNode += QUADRANT_NUM_OF_REGION;
+    }
+    break;
+
+  default:
+    // Should never reach there.
+    ASSERT (FALSE);
+    break;
+  }
+
+  return SubNumaNode;
+}
+
+/**
+  Get the associativity of cache.
+
+  @param    Level       Cache level.
+  @return   UINT32      Associativity of cache.
+
+**/
+UINT32
+EFIAPI
+CpuGetAssociativity (
+  UINT32 Level
+  )
+{
I do feel some of this stuff could be replaced by the common functions
we now have in ArmLib (that we didn't when v1 went out).

+  UINT64 CacheCCSIDR;
+  UINT64 CacheCLIDR;
+  UINT32 Value = 0x2; /* Unknown Set-Associativity */
+
+  CacheCLIDR = ReadCLIDR ();
+  if (!CLIDR_CTYPE (CacheCLIDR, Level)) {
+    return Value;
+  }
+
+  CacheCCSIDR = ReadCCSIDR (Level);
+  switch (CCSIDR_ASSOCIATIVITY (CacheCCSIDR)) {
+  case 0:
+    /* Direct mapped */
+    Value = 0x3;
+    break;
+
+  case 1:
+    /* 2-way Set-Associativity */
+    Value = 0x4;
+    break;
+
+  case 3:
+    /* 4-way Set-Associativity */
+    Value = 0x5;
+    break;
+
+  case 7:
+    /* 8-way Set-Associativity */
+    Value = 0x7;
+    break;
+
+  case 15:
+    /* 16-way Set-Associativity */
+    Value = 0x8;
+    break;
+
+  case 11:
+    /* 12-way Set-Associativity */
+    Value = 0x9;
+    break;
+
+  case 23:
+    /* 24-way Set-Associativity */
+    Value = 0xA;
+    break;
+
+  case 31:
+    /* 32-way Set-Associativity */
+    Value = 0xB;
+    break;
+
+  case 47:
+    /* 48-way Set-Associativity */
+    Value = 0xC;
+    break;
+
+  case 63:
+    /* 64-way Set-Associativity */
+    Value = 0xD;
+    break;
+
+  case 19:
+    /* 20-way Set-Associativity */
+    Value = 0xE;
+    break;
+  }
+
+  return Value;
+}
+
+/**
+  Get the cache size.
+
+  @param    Level       Cache level.
+  @return   UINT32      Cache size.
+
+**/
+UINT32
+EFIAPI
+CpuGetCacheSize (
+  UINT32 Level
+  )
+{
+  UINT32 CacheLineSize;
+  UINT32 Count;
+  UINT64 CacheCCSIDR;
+  UINT64 CacheCLIDR;
+
+  CacheCLIDR = ReadCLIDR ();
+  if (!CLIDR_CTYPE (CacheCLIDR, Level)) {
+    return 0;
+  }
+
+  CacheCCSIDR = ReadCCSIDR (Level);
+  CacheLineSize = 1;
+  Count = CCSIDR_LINE_SIZE (CacheCCSIDR) + 4;
+  while (Count-- > 0) {
+    CacheLineSize *= 2;
+  }
+
+  return ((CCSIDR_NUMSETS (CacheCCSIDR) + 1) *
+          (CCSIDR_ASSOCIATIVITY (CacheCCSIDR) + 1) *
+          CacheLineSize);
+}
+
+/**
+  Get the number of supported socket.
+
+  @return   UINT8      Number of supported socket.
+
+**/
+UINT8
+EFIAPI
+GetNumberOfSupportedSockets (
+  VOID
+  )
+{
+  PLATFORM_INFO_HOB  *PlatformHob;
+
+  PlatformHob = GetPlatformHob ();
+  if (PlatformHob == NULL) {
+    //
+    // By default, the number of supported sockets is 1.
+    //
+    return 1;
+  }
+
+  return (sizeof (PlatformHob->ClusterEn) / sizeof (PLATFORM_CLUSTER_EN));
+}
+
+/**
+  Get the number of active socket.
+
+  @return   UINT8      Number of active socket.
+
+**/
+UINT8
+EFIAPI
+GetNumberOfActiveSockets (
+  VOID
+  )
+{
+  UINT8               NumberOfActiveSockets, Count, Index, Index1;
+  PLATFORM_CLUSTER_EN *Socket;
+  PLATFORM_INFO_HOB   *PlatformHob;
+
+  PlatformHob = GetPlatformHob ();
+  if (PlatformHob == NULL) {
+    //
+    // By default, the number of active sockets is 1.
+    //
+    return 1;
+  }
+
+  NumberOfActiveSockets = 0;
+
+  for (Index = 0; Index < GetNumberOfSupportedSockets (); Index++) {
+    Socket = &PlatformHob->ClusterEn[Index];
+    Count = ARRAY_SIZE (Socket->EnableMask);
+    for (Index1 = 0; Index1 < Count; Index1++) {
+      if (Socket->EnableMask[Index1] != 0) {
+        NumberOfActiveSockets++;
+        break;
+      }
+    }
+  }
+
+  return NumberOfActiveSockets;
+}
+
+/**
+  Get the number of active CPM per socket.
+
+  @param    SocketId    Socket index.
+  @return   UINT16      Number of CPM.
+
+**/
+UINT16
+EFIAPI
+GetNumberOfActiveCPMsPerSocket (
+  UINT8 SocketId
+  )
+{
+  UINT16              NumberOfCPMs, Count, Index;
+  UINT32              Val32;
+  PLATFORM_CLUSTER_EN *Socket;
+  PLATFORM_INFO_HOB   *PlatformHob;
+
+  PlatformHob = GetPlatformHob ();
+  if (PlatformHob == NULL) {
+    return 0;
+  }
+
+  if (SocketId >= GetNumberOfActiveSockets ()) {
+    return 0;
+  }
+
+  NumberOfCPMs = 0;
+  Socket = &PlatformHob->ClusterEn[SocketId];
+  Count = ARRAY_SIZE (Socket->EnableMask);
+  for (Index = 0; Index < Count; Index++) {
+    Val32 = Socket->EnableMask[Index];
+    while (Val32 > 0) {
+      if ((Val32 & 0x1) != 0) {
+        NumberOfCPMs++;
+      }
+      Val32 >>= 1;
+    }
+  }
+
+  return NumberOfCPMs;
+}
+
+/**
+  Get the number of configured CPM per socket. This number
+  should be the same for all sockets.
+
+  @param    SocketId    Socket index.
+  @return   UINT8       Number of configured CPM.
+
+**/
+UINT16
+EFIAPI
+GetNumberOfConfiguredCPMs (
+  UINT8 SocketId
+  )
+{
+  EFI_STATUS Status;
+  UINT32     Value;
+  UINT32     Param, ParamStart, ParamEnd;
+  UINT16     Count;
+
+  Count = 0;
+  ParamStart = NV_SI_S0_PCP_ACTIVECPM_0_31 + SocketId * NV_PARAM_ENTRYSIZE * (PLATFORM_CPU_MAX_CPM / 32);
+  ParamEnd = ParamStart + NV_PARAM_ENTRYSIZE * (PLATFORM_CPU_MAX_CPM / 32);
+  for (Param = ParamStart; Param < ParamEnd; Param += NV_PARAM_ENTRYSIZE) {
+    Status = NVParamGet (
+               Param,
+               NV_PERM_ATF | NV_PERM_BIOS | NV_PERM_MANU | NV_PERM_BMC,
+               &Value
+               );
+    if (EFI_ERROR (Status)) {
+      break;
+    }
+    while (Value != 0) {
+      if ((Value & 0x01) != 0) {
+        Count++;
+      }
+      Value >>= 1;
+    }
+  }
+
+  return Count;
+}
+
+/**
+  Set the number of configured CPM per socket.
+
+  @param    SocketId        Socket index.
+  @param    NumberOfCPMs    Number of CPM to be configured.
+  @return   EFI_SUCCESS     Operation succeeded.
+  @return   Others          An error has occurred.
+
+**/
+EFI_STATUS
+EFIAPI
+SetNumberOfConfiguredCPMs (
+  UINT8  SocketId,
+  UINT16 NumberOfCPMs
+  )
+{
+  EFI_STATUS Status;
+  UINT32     Value;
+  UINT32     Param, ParamStart, ParamEnd;
+  BOOLEAN    IsClear;
+
+  IsClear = FALSE;
+  if (NumberOfCPMs == 0) {
+    IsClear = TRUE;
+  }
+
+  Status = EFI_SUCCESS;
+
+  ParamStart = NV_SI_S0_PCP_ACTIVECPM_0_31 + SocketId * NV_PARAM_ENTRYSIZE * (PLATFORM_CPU_MAX_CPM / 32);
+  ParamEnd = ParamStart + NV_PARAM_ENTRYSIZE * (PLATFORM_CPU_MAX_CPM / 32);
+  for (Param = ParamStart; Param < ParamEnd; Param += NV_PARAM_ENTRYSIZE) {
+    if (NumberOfCPMs >= 32) {
+      Value = 0xffffffff;
+      NumberOfCPMs -= 32;
+    } else {
+      Value = 0;
+      while (NumberOfCPMs > 0) {
+        Value |= (1 << (--NumberOfCPMs));
+      }
+    }
+    if (IsClear) {
+      /* Clear this param */
+      Status = NVParamClr (
+                 Param,
+                 NV_PERM_BIOS | NV_PERM_MANU
+                 );
+    } else {
+      Status = NVParamSet (
+                 Param,
+                 NV_PERM_ATF | NV_PERM_BIOS | NV_PERM_MANU | NV_PERM_BMC,
+                 NV_PERM_BIOS | NV_PERM_MANU,
+                 Value
+                 );
+    }
+  }
+
+  return Status;
+}
+
+/**
+  Get the maximum number of core per socket.
+
+  @return   UINT16      Maximum number of core.
+
+**/
+UINT16
+EFIAPI
+GetMaximumNumberOfCores (
+  VOID
+  )
+{
+  PLATFORM_INFO_HOB  *PlatformHob;
+
+  PlatformHob = GetPlatformHob ();
+  if (PlatformHob == NULL) {
+    return 0;
+  }
+
+  return PlatformHob->MaxNumOfCore[0];
+}
+
+/**
+  Get the maximum number of CPM per socket. This number
+  should be the same for all sockets.
+
+  @return   UINT16      Maximum number of CPM.
+
+**/
+UINT16
+EFIAPI
+GetMaximumNumberOfCPMs (
+  VOID
+  )
+{
+  return GetMaximumNumberOfCores () / PLATFORM_CPU_NUM_CORES_PER_CPM;
+}
+
+/**
+  Get the number of active cores of a sockets.
+
+  @param    SocketId    Socket Index.
+  @return   UINT16      Number of active core.
+
+**/
+UINT16
+EFIAPI
+GetNumberOfActiveCoresPerSocket (
+  UINT8 SocketId
+  )
+{
+  return GetNumberOfActiveCPMsPerSocket (SocketId) * PLATFORM_CPU_NUM_CORES_PER_CPM;
+}
+
+/**
+  Get the number of active cores of all sockets.
+
+  @return   UINT16      Number of active core.
+
+**/
+UINT16
+EFIAPI
+GetNumberOfActiveCores (
+  VOID
+  )
+{
+  UINT16 NumberOfActiveCores;
+  UINT8  Index;
+
+  NumberOfActiveCores = 0;
+
+  for (Index = 0; Index < GetNumberOfActiveSockets (); Index++) {
+    NumberOfActiveCores += GetNumberOfActiveCoresPerSocket (Index);
+  }
+
+  return NumberOfActiveCores;
+}
+
+/**
+  Check if the logical CPU is enabled or not.
+
+  @param    CpuId       The logical Cpu ID. Started from 0.
+  @return   BOOLEAN     TRUE if the Cpu enabled
+                        FALSE if the Cpu disabled.
+
+**/
+BOOLEAN
+EFIAPI
+IsCpuEnabled (
+  UINT16 CpuId
+  )
+{
+  PLATFORM_CLUSTER_EN *Socket;
+  PLATFORM_INFO_HOB   *PlatformHob;
+  UINT8               SocketId;
+  UINT16              ClusterId;
+
+  SocketId = SOCKET_ID (CpuId);
+  ClusterId = CLUSTER_ID (CpuId);
+
+  PlatformHob = GetPlatformHob ();
+  if (PlatformHob == NULL) {
+    return FALSE;
+  }
+
+  if (SocketId >= GetNumberOfActiveSockets ()) {
+    return FALSE;
+  }
+
+  Socket = &PlatformHob->ClusterEn[SocketId];
+  if ((Socket->EnableMask[ClusterId / 32] & (1 << (ClusterId % 32))) != 0) {
+    return TRUE;
+  }
+
+  return FALSE;
+}
+
+/**
+  Check if the slave socket is present
+
+  @return   BOOLEAN     TRUE if the Slave Cpu is present
+                        FALSE if the Slave Cpu is not present
+
+**/
+BOOLEAN
+EFIAPI
+IsSlaveSocketPresent (
+  VOID
+  )
+{
+  UINT32 Value;
+
+  Value = MmioRead32 (SMPRO_EFUSE_SHADOW0 + CFG2P_OFFSET);
+
+  return ((Value & SLAVE_PRESENT_N) != 0) ? FALSE : TRUE;
+}
+
+/**
+  Check if the slave socket is active
+
+  @return   BOOLEAN     TRUE if the Slave CPU Socket is active.
+                        FALSE if the Slave CPU Socket is not active.
+
+**/
+BOOLEAN
+EFIAPI
+IsSlaveSocketActive (
+  VOID
+  )
+{
+  return (GetNumberOfActiveSockets () > 1) ? TRUE : FALSE;
+}
+
+/**
+  Check if the CPU product ID is Ac01
+  @return   BOOLEAN     TRUE if the Product ID is Ac01
+                        FALSE otherwise.
+
+**/
+BOOLEAN
+EFIAPI
+IsAc01Processor (
+  VOID
+  )
+{
+  PLATFORM_INFO_HOB  *PlatformHob;
+
+  PlatformHob = GetPlatformHob ();
+  ASSERT (PlatformHob != NULL);
+
+  if (PlatformHob != NULL) {
+    if ((PlatformHob->ScuProductId[0] & 0xFF) == 0x01) {
+      return TRUE;
+    }
+  }
+
+  return FALSE;
+}


diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
new file mode 100644
index 000000000000..8c1eb93f00fd
--- /dev/null
+++ b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
@@ -0,0 +1,169 @@
+/** @file
+
+  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi.h>
+
+#include <Guid/PlatformInfoHobGuid.h>
+#include <Library/AmpereCpuLib.h>
+#include <Library/ArmLib.h>
+#include <Library/ArmPlatformLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/HobLib.h>
+#include <Library/IoLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/PcdLib.h>
+#include <Library/PL011UartLib.h>
+#include <Library/SerialPortLib.h>
+#include <Platform/Ac01.h>
+#include <PlatformInfoHob.h>
+#include <Ppi/ArmMpCoreInfo.h>
+#include <Ppi/TemporaryRamSupport.h>
+
+ARM_CORE_INFO mArmPlatformMpCoreInfoTable[PLATFORM_CPU_MAX_NUM_CORES];
+
+/**
+  Return the current Boot Mode
+
+  This function returns the boot reason on the platform
+
+  @return   Return the current Boot Mode of the platform
+
+**/
+EFI_BOOT_MODE
+ArmPlatformGetBootMode (
+  VOID
+  )
+{
+  return BOOT_WITH_FULL_CONFIGURATION;
+}
+
+/**
+  Initialize controllers that must setup in the normal world
+
+  This function is called by the ArmPlatformPkg/PrePi or ArmPlatformPkg/PlatformPei
+  in the PEI phase.
+
+**/
+EFI_STATUS
+ArmPlatformInitialize (
+  IN UINTN MpId
+  )
+{
+  RETURN_STATUS      Status;
+  UINT64             BaudRate;
+  UINT32             ReceiveFifoDepth;
+  EFI_PARITY_TYPE    Parity;
+  UINT8              DataBits;
+  EFI_STOP_BITS_TYPE StopBits;
+
+  Status = EFI_SUCCESS;
+
+  if (FixedPcdGet64 (PcdSerialRegisterBase) != 0) {
+    /* Debug port should use the same parameters with console */
+    BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);
+    ReceiveFifoDepth = FixedPcdGet32 (PcdUartDefaultReceiveFifoDepth);
+    Parity = (EFI_PARITY_TYPE)FixedPcdGet8 (PcdUartDefaultParity);
+    DataBits = FixedPcdGet8 (PcdUartDefaultDataBits);
+    StopBits = (EFI_STOP_BITS_TYPE)FixedPcdGet8 (PcdUartDefaultStopBits);
+
+    /* Initialize uart debug port */
+    Status = PL011UartInitializePort (
+               (UINTN)FixedPcdGet64 (PcdSerialRegisterBase),
+               FixedPcdGet32 (PL011UartClkInHz),
+               &BaudRate,
+               &ReceiveFifoDepth,
+               &Parity,
+               &DataBits,
+               &StopBits
+               );
+  }
+
+  return Status;
+}
+
+EFI_STATUS
+PrePeiCoreGetMpCoreInfo (
+  OUT UINTN         *CoreCount,
+  OUT ARM_CORE_INFO **ArmCoreTable
+  )
+{
+  UINTN              mArmPlatformCoreCount;
+  UINTN              ClusterId;
+  UINTN              SocketId;
+  UINTN              Index;
+
+  ASSERT (CoreCount != NULL);
+  ASSERT (ArmCoreTable != NULL);
+  ASSERT (*ArmCoreTable != NULL);
+
+  mArmPlatformCoreCount = 0;
+  for  (Index = 0; Index < PLATFORM_CPU_MAX_NUM_CORES; Index++) {
+    if (!IsCpuEnabled (Index)) {
+      continue;
+    }
+    SocketId = SOCKET_ID (Index);
+    ClusterId = CLUSTER_ID (Index);
+    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].ClusterId = SocketId;
+    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].CoreId =
+      (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM);
+    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].MailboxClearAddress = 0;
+    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].MailboxClearValue = 0;
+    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].MailboxGetAddress = 0;
+    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].MailboxSetAddress = 0;
+    mArmPlatformCoreCount++;
+  }
+
+  *CoreCount = mArmPlatformCoreCount;
+
+  *ArmCoreTable = mArmPlatformMpCoreInfoTable;
+  ASSERT (*ArmCoreTable != NULL);
+
+  return EFI_SUCCESS;
+}
+
+// Needs to be declared in the file. Otherwise gArmMpCoreInfoPpiGuid is undefined in the contect of PrePeiCore
+EFI_GUID             mArmMpCoreInfoPpiGuid = ARM_MP_CORE_INFO_PPI_GUID;
+ARM_MP_CORE_INFO_PPI mMpCoreInfoPpi = { PrePeiCoreGetMpCoreInfo };
+
+EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
+  {
+    EFI_PEI_PPI_DESCRIPTOR_PPI,
+    &mArmMpCoreInfoPpiGuid,
+    &mMpCoreInfoPpi
+  },
+};
+
+/**
+  Return the Platform specific PPIs
+
+  This function exposes the Platform Specific PPIs. They can be used by any PrePi modules or passed
+  to the PeiCore by PrePeiCore.
+
+  @param[out]   PpiListSize         Size in Bytes of the Platform PPI List
+  @param[out]   PpiList             Platform PPI List
+
+**/
+VOID
+ArmPlatformGetPlatformPpiList (
+  OUT UINTN                  *PpiListSize,
+  OUT EFI_PEI_PPI_DESCRIPTOR **PpiList
+  )
+{
+  ASSERT (PpiListSize != NULL);
+  ASSERT (PpiList != NULL);
+  ASSERT (*PpiList != NULL);
+
+  if (ArmIsMpCore ()) {
+    *PpiListSize = sizeof (gPlatformPpiTable);
+    *PpiList = gPlatformPpiTable;
+  } else {
+    *PpiListSize = 0;
+    *PpiList = NULL;
+  }
+}
diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLibMemory.c b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLibMemory.c
new file mode 100644
index 000000000000..117c9cc56ac2
--- /dev/null
+++ b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLibMemory.c
@@ -0,0 +1,399 @@
+/** @file
+
+  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi.h>
+
+#include <Guid/PlatformInfoHobGuid.h>
+#include <Library/AmpereCpuLib.h>
+#include <Library/ArmPlatformLib.h>
+#include <Library/DebugLib.h>
+#include <Library/HobLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/PcdLib.h>
+#include <PlatformInfoHob.h>
+
+/* Number of Virtual Memory Map Descriptors */
+#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          50
+
+/* DDR attributes */
+#define DDR_ATTRIBUTES_CACHED           ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
+#define DDR_ATTRIBUTES_UNCACHED         ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED
+
+/**
+  Return the Virtual Memory Map of your platform
+
+  This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU on your platform.
+
+  @param[out]   VirtualMemoryMap    Array of ARM_MEMORY_REGION_DESCRIPTOR describing a Physical-to-
+                                    Virtual Memory mapping. This array must be ended by a zero-filled
+                                    entry
+
+**/
+VOID
+ArmPlatformGetVirtualMemoryMap (
+  OUT ARM_MEMORY_REGION_DESCRIPTOR **VirtualMemoryMap
+  )
+{
+  UINTN                        Index = 0;
+  ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable;
+  UINT32                       NumRegion;
+  UINTN                        Count;
+  VOID                         *Hob;
+  PLATFORM_INFO_HOB            *PlatformHob;
+
+  Hob = GetFirstGuidHob (&gPlatformHobGuid);
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+    return;
+  }
+
+  PlatformHob = (PLATFORM_INFO_HOB *)GET_GUID_HOB_DATA (Hob);
+
+  ASSERT (VirtualMemoryMap != NULL);
+
+  VirtualMemoryTable = (ARM_MEMORY_REGION_DESCRIPTOR *)AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ARM_MEMORY_REGION_DESCRIPTOR) * MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS));
+  if (VirtualMemoryTable == NULL) {
+    return;
+  }
+
+  /* For Address space 0x1000_0000_0000 to 0x1001_00FF_FFFF
+   *  - Device memory
+   */
+  VirtualMemoryTable[Index].PhysicalBase = 0x100000000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x100000000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x102000000ULL;
Please move all of these live-coded addresses/sizes to a private .h
and use symbolic names here.

+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /* For Address space 0x5000_0000_0000 to 0x5001_00FF_FFFF
+   *  - Device memory
+   */
+  if (IsSlaveSocketActive ())
+  {
+    VirtualMemoryTable[++Index].PhysicalBase = 0x500000000000ULL;
+    VirtualMemoryTable[Index].VirtualBase  = 0x500000000000ULL;
+    VirtualMemoryTable[Index].Length       = 0x101000000ULL;
+    VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+  }
+
+  /*
+   *  - PCIe RCA0 Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x300000000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x300000000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket0 RCA0 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCB2 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x20000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x20000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - PCIe RCA1 Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x340000000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x340000000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket0 RCA1 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCB2 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x28000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x28000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - PCIe RCA2 Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x380000000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x380000000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket0 RCA2 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCB3 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x30000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x30000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - PCIe RCA3 Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x3C0000000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x3C0000000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket0 RCA3 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCB3 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x38000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x38000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - PCIe RCB0 Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x200000000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x200000000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket0 RCB0 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCB0 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x00000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x00000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - PCIe RCB1 Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x240000000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x240000000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket0 RCB1 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCB0 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x08000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x08000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - PCIe RCB2 Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x280000000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x280000000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket0 RCB2 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCB1 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x10000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x10000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - PCIe RCB3 Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x2C0000000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x2C0000000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket0 RCB3 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCB1 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x18000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x18000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  if (IsSlaveSocketActive ()) {
+    // Slave socket exist
+    /*
+     *  - PCIe RCA0 Device memory
+     */
+    VirtualMemoryTable[++Index].PhysicalBase = 0x700000000000ULL;
+    VirtualMemoryTable[Index].VirtualBase  = 0x700000000000ULL;
+    VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+    VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+    /*
+     *  - PCIe RCA1 Device memory
+     */
+    VirtualMemoryTable[++Index].PhysicalBase = 0x740000000000ULL;
+    VirtualMemoryTable[Index].VirtualBase  = 0x740000000000ULL;
+    VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+    VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+    /*
+     *  - PCIe RCA2 Device memory
+     */
+    VirtualMemoryTable[++Index].PhysicalBase = 0x780000000000ULL;
+    VirtualMemoryTable[Index].VirtualBase  = 0x780000000000ULL;
+    VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+    VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+    /*
+     *  - PCIe RCA3 Device memory
+     */
+    VirtualMemoryTable[++Index].PhysicalBase = 0x7C0000000000ULL;
+    VirtualMemoryTable[Index].VirtualBase  = 0x7C0000000000ULL;
+    VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+    VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+    /*
+     *  - PCIe RCB0 Device memory
+     */
+    VirtualMemoryTable[++Index].PhysicalBase = 0x600000000000ULL;
+    VirtualMemoryTable[Index].VirtualBase  = 0x600000000000ULL;
+    VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+    VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+    /*
+     *  - PCIe RCB1 Device memory
+     */
+    VirtualMemoryTable[++Index].PhysicalBase = 0x640000000000ULL;
+    VirtualMemoryTable[Index].VirtualBase  = 0x640000000000ULL;
+    VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+    VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+    /*
+     *  - PCIe RCB2 Device memory
+     */
+    VirtualMemoryTable[++Index].PhysicalBase = 0x680000000000ULL;
+    VirtualMemoryTable[Index].VirtualBase  = 0x680000000000ULL;
+    VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+    VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+    /*
+     *  - PCIe RCB3 Device memory
+     */
+    VirtualMemoryTable[++Index].PhysicalBase = 0x6C0000000000ULL;
+    VirtualMemoryTable[Index].VirtualBase  = 0x6C0000000000ULL;
+    VirtualMemoryTable[Index].Length       = 0x40000000000ULL;
+    VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+  }
+
+  /*
+   *  - 2P/PCIe Socket1 RCA0 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCA2 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x60000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x60000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket1 RCA1 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCA2 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x68000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x68000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket1 RCA2 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCA3 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x70000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x70000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket1 RCA3 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCA3 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x78000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x78000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket1 RCB0 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCA0 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x40000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x40000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket1 RCB1 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCA0 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x48000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x48000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket1 RCB2 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCA1 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x50000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x50000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - 2P/PCIe Socket1 RCB3 32-bit Device memory
+   *  - 1P/PCIe consolidated to RCA1 32-bit Device memory
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x58000000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x58000000ULL;
+  VirtualMemoryTable[Index].Length       = 0x8000000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - BERT memory region
+   */
+  VirtualMemoryTable[++Index].PhysicalBase = 0x88230000ULL;
+  VirtualMemoryTable[Index].VirtualBase  = 0x88230000ULL;
+  VirtualMemoryTable[Index].Length       = 0x50000ULL;
+  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+  /*
+   *  - DDR memory region
+   */
+  NumRegion = PlatformHob->DramInfo.NumRegion;
+  Count = 0;
+  while (NumRegion-- > 0) {
+    if (PlatformHob->DramInfo.NvdRegion[Count]) { /* Skip NVDIMM Region */
+      Count++;
+      continue;
+    }
+
+    VirtualMemoryTable[++Index].PhysicalBase = PlatformHob->DramInfo.Base[Count];
+    VirtualMemoryTable[Index].VirtualBase  = PlatformHob->DramInfo.Base[Count];
+    VirtualMemoryTable[Index].Length       = PlatformHob->DramInfo.Size[Count];
+    VirtualMemoryTable[Index].Attributes   = DDR_ATTRIBUTES_CACHED;
+    Count++;
+  }
+
+  /* SPM MM NS Buffer for MmCommunicateDxe */
+  VirtualMemoryTable[++Index].PhysicalBase = PcdGet64 (PcdMmBufferBase);
+  VirtualMemoryTable[Index].VirtualBase  = PcdGet64 (PcdMmBufferBase);
+  VirtualMemoryTable[Index].Length       = PcdGet64 (PcdMmBufferSize);
+  VirtualMemoryTable[Index].Attributes   = DDR_ATTRIBUTES_CACHED;
+
+  /* End of Table */
+  VirtualMemoryTable[++Index].PhysicalBase = 0;
+  VirtualMemoryTable[Index].VirtualBase  = 0;
+  VirtualMemoryTable[Index].Length       = 0;
+  VirtualMemoryTable[Index].Attributes   = (ARM_MEMORY_REGION_ATTRIBUTES)0;
+
+  ASSERT ((Index + 1) <= MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
+
+  *VirtualMemoryMap = VirtualMemoryTable;
+}


diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/MailboxInterfaceLib/MailboxInterfaceLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/MailboxInterfaceLib/MailboxInterfaceLib.c
new file mode 100644
index 000000000000..0da1f90699f6
--- /dev/null
+++ b/Silicon/Ampere/AmpereAltraPkg/Library/MailboxInterfaceLib/MailboxInterfaceLib.c
@@ -0,0 +1,282 @@
+/** @file
+  The library implements the hardware Mailbox (Doorbell) interface for communication
+  between the Application Processor (ARMv8) and the System Control Processors (SMpro/PMpro).
+
+  Copyright (c) 2021, Ampere Computing LLC. All rights reserved.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi.h>
+
+#include <Library/AmpereCpuLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MailboxInterfaceLib.h>
+#include <Library/TimerLib.h>
+#include <Library/IoLib.h>
+
+//
+// Hardware Doorbells
+//
+#define SMPRO_DB0_IRQ_OFST               40
+#define SMPRO_DB0_BASE_ADDRESS           (FixedPcdGet64 (PcdSmproDbBaseReg))
+
+#define PMPRO_DB0_IRQ_OFST               56
+#define PMPRO_DB0_BASE_ADDRESS           (FixedPcdGet64 (PcdPmproDbBaseReg))
+
+#define SLAVE_SOCKET_BASE_ADDRESS_OFFSET 0x400000000000
+
+//
+// The base SPI interrupt number of the Slave socket
+//
+#define SLAVE_SOCKET_SPI_INTERRUPT 352
+
+#define SLAVE_SOCKET_DOORBELL_INTERRUPT_BASE(Socket) ((Socket) * SLAVE_SOCKET_SPI_INTERRUPT - 32)
+
+//
+// Doorbell base register stride size
+//
+#define DB_BASE_REG_STRIDE 0x00001000
+
+#define SMPRO_DBx_ADDRESS(socket, db) \
+        ((socket) * SLAVE_SOCKET_BASE_ADDRESS_OFFSET + SMPRO_DB0_BASE_ADDRESS + DB_BASE_REG_STRIDE * (db))
+
+#define PMPRO_DBx_ADDRESS(socket, db) \
+        ((socket) * SLAVE_SOCKET_BASE_ADDRESS_OFFSET + PMPRO_DB0_BASE_ADDRESS + DB_BASE_REG_STRIDE * (db))
+
+//
+// Doorbell Status Bits
+//
+#define DB_STATUS_AVAIL_BIT       BIT16
+#define DB_STATUS_ACK_BIT         BIT0
+
+/**
+  Get the base address of a doorbell.
+
+  @param[in]  Socket            Active socket index.
+  @param[in]  Doorbell          Doorbell channel for communication with the SMpro/PMpro.
+
+  @retval UINT32                The base address of the doorbell.
+                                The returned value is 0 indicate that the input parameters are invalid.
+
+**/
+UINTN
+EFIAPI
+MailboxGetDoorbellAddress (
+  IN UINT8             Socket,
+  IN DOORBELL_CHANNELS Doorbell
+  )
+{
+  UINTN DoorbellAddress;
+
+  if (Socket >= GetNumberOfActiveSockets ()
+      || Doorbell >= NUMBER_OF_DOORBELLS_PER_SOCKET)
+  {
+    return 0;
+  }
Coding style is
 if () {
 }

This file gets it consistently wrong.

I agree that it should be consistent. But that coding style conforms to "5.2.1.6 Each sub-expression of a complex predicate expression must be on a separate line" in EDK II Coding Standards Spec (https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-1-6-each-sub-expression-of-a-complex-predicate-expression-must-be-on-a-separate-line).

"Predicate expressions containing multiple operators with sub-expressions joined by && or || must have each sub-expression on a separate line. The opening brace, '{' of the body shall be on a line by itself and aligned in the starting column of the associated keyword.

while ( ( Code == MEETS_STANDARD)
  && ( Code == FUNCTIONAL))
{
  ShipIt();
}

"

However, I'm OK to change it as your suggestion.


Thanks,

Nhi


+
+  if (Doorbell >= SMproDoorbellChannel0) {
+    DoorbellAddress = SMPRO_DBx_ADDRESS (Socket, (UINT8)(Doorbell - SMproDoorbellChannel0));
+  } else {
+    DoorbellAddress = PMPRO_DBx_ADDRESS (Socket, (UINT8)Doorbell);
+  }
+
+  return DoorbellAddress;
+}
+
+/**
+  Get the interrupt number of a doorbell.
+
+  @param[in]  Socket            Active socket index.
+  @param[in]  Doorbell          Doorbell channel for communication with the SMpro/PMpro.
+
+  @retval UINT32                The interrupt number.
+                                The returned value is 0 indicate that the input parameters are invalid.
+
+**/
+UINT32
+EFIAPI
+MailboxGetDoorbellInterruptNumber (
+  IN UINT8             Socket,
+  IN DOORBELL_CHANNELS Doorbell
+  )
+{
+  UINT32 DoorbellInterruptNumber;
+
+  if (Socket >= GetNumberOfActiveSockets ()
+      || Doorbell >= NUMBER_OF_DOORBELLS_PER_SOCKET)
+  {
+    return 0;
+  }
+
+  DoorbellInterruptNumber = 0;
+
+  if (Socket > 0) {
+    DoorbellInterruptNumber = SLAVE_SOCKET_DOORBELL_INTERRUPT_BASE (Socket);
+  }
+
+  if (Doorbell >= SMproDoorbellChannel0) {
+    DoorbellInterruptNumber += SMPRO_DB0_IRQ_OFST + (UINT8)(Doorbell - SMproDoorbellChannel0);
+  } else {
+    DoorbellInterruptNumber += PMPRO_DB0_IRQ_OFST + (UINT8)Doorbell;
+  }
+
+  return DoorbellInterruptNumber;
+}
+
+/**
+  Read a message via the hardware Doorbell interface.
+
+  @param[in]  Socket            Active socket index.
+  @param[in]  Doorbell          Doorbell channel for communication with the SMpro/PMpro.
+  @param[out] Message           Pointer to the Mailbox message.
+
+  @retval EFI_SUCCESS           Read the message successfully.
+  @retval EFI_TIMEOUT           Timeout occurred when waiting for available message in the mailbox.
+  @retval EFI_INVALID_PARAMETER A parameter is invalid.
+**/
+EFI_STATUS
+EFIAPI
+MailboxRead (
+  IN  UINT8                Socket,
+  IN  DOORBELL_CHANNELS    Doorbell,
+  OUT MAILBOX_MESSAGE_DATA *Message
+  )
+{
+  UINTN TimeoutCount;
+  UINTN DoorbellAddress;
+
+  if (Socket >= GetNumberOfActiveSockets ()
+      || Doorbell >= NUMBER_OF_DOORBELLS_PER_SOCKET
+      || Message == NULL)
+  {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  TimeoutCount = MAILBOX_POLL_COUNT;
+
+  DoorbellAddress = MailboxGetDoorbellAddress (Socket, Doorbell);
+  ASSERT (DoorbellAddress != 0);
+
+  //
+  // Polling Doorbell status
+  //
+  while ((MmioRead32 ((DoorbellAddress + DB_STATUS_REG_OFST)) & DB_STATUS_AVAIL_BIT) == 0) {
+    MicroSecondDelay (MAILBOX_POLL_INTERVAL_US);
+    if (--TimeoutCount == 0) {
+      return EFI_TIMEOUT;
+    }
+  }
+
+  Message->ExtendedData[0] = MmioRead32 (DoorbellAddress + DB_DIN0_REG_OFST);
+  Message->ExtendedData[1] = MmioRead32 (DoorbellAddress + DB_DIN1_REG_OFST);
+  Message->Data = MmioRead32 (DoorbellAddress + DB_IN_REG_OFST);
+
+  //
+  // Write 1 to clear the AVAIL status
+  //
+  MmioWrite32 (DoorbellAddress + DB_STATUS_REG_OFST, DB_STATUS_AVAIL_BIT);
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Write a message via the hardware Doorbell interface.
+
+  @param[in]  Socket            Active socket index.
+  @param[in]  Doorbell          Doorbel channel for communication with the SMpro/PMpro.
+  @param[in]  Message           Pointer to the Mailbox message.
+
+  @retval EFI_SUCCESS           Write the message successfully.
+  @retval EFI_TIMEOUT           Timeout occurred when waiting for acknowledge signal from the mailbox.
+  @retval EFI_INVALID_PARAMETER A parameter is invalid.
+**/
+EFI_STATUS
+EFIAPI
+MailboxWrite (
+  IN UINT8                Socket,
+  IN DOORBELL_CHANNELS    Doorbell,
+  IN MAILBOX_MESSAGE_DATA *Message
+  )
+{
+  UINTN TimeoutCount;
+  UINTN DoorbellAddress;
+
+  if (Socket >= GetNumberOfActiveSockets ()
+      || Doorbell >= NUMBER_OF_DOORBELLS_PER_SOCKET
+      || Message == NULL)
+  {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  TimeoutCount = MAILBOX_POLL_COUNT;
+
+  DoorbellAddress = MailboxGetDoorbellAddress (Socket, Doorbell);
+  ASSERT (DoorbellAddress != 0);
+
+  //
+  // Clear previous pending ack if any
+  //
+  if ((MmioRead32 (DoorbellAddress + DB_STATUS_REG_OFST) & DB_STATUS_ACK_BIT) != 0) {
+    MmioWrite32 (DoorbellAddress + DB_STATUS_REG_OFST, DB_STATUS_ACK_BIT);
+  }
+
+  //
+  // Send message
+  //
+  MmioWrite32 (DoorbellAddress + DB_DOUT0_REG_OFST, Message->ExtendedData[0]);
+  MmioWrite32 (DoorbellAddress + DB_DOUT1_REG_OFST, Message->ExtendedData[1]);
+  MmioWrite32 (DoorbellAddress + DB_OUT_REG_OFST, Message->Data);
+
+  //
+  // Wait for ACK
+  //
+  while ((MmioRead32 (DoorbellAddress + DB_STATUS_REG_OFST) & DB_STATUS_ACK_BIT) == 0) {
+    MicroSecondDelay (MAILBOX_POLL_INTERVAL_US);
+    if (--TimeoutCount == 0) {
+      return EFI_TIMEOUT;
+    }
+  }
+
+  //
+  // Write 1 to clear the ACK status
+  //
+  MmioWrite32 (DoorbellAddress + DB_STATUS_REG_OFST, DB_STATUS_ACK_BIT);
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Unmask the Doorbell interrupt status.
+
+  @param  Socket    Active socket index.
+  @param  Doorbell  Doorbel channel for communication with the SMpro/PMpro.
+
+  @retval EFI_SUCCESS            Unmask the Doorbell interrupt successfully.
+  @retval EFI_INVALID_PARAMETER  A parameter is invalid.
+
+**/
+EFI_STATUS
+EFIAPI
+MailboxUnmaskInterrupt (
+  IN UINT8  Socket,
+  IN UINT16 Doorbell
+  )
+{
+  UINTN DoorbellAddress;
+
+  if (Socket >= GetNumberOfActiveSockets ()
+      || Doorbell >= NUMBER_OF_DOORBELLS_PER_SOCKET)
+  {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  DoorbellAddress = MailboxGetDoorbellAddress (Socket, Doorbell);
+  ASSERT (DoorbellAddress != 0);
+
+  MmioWrite32 (DoorbellAddress + DB_STATUS_MASK_REG_OFST, ~DB_STATUS_AVAIL_BIT);
+
+  return EFI_SUCCESS;
+}

diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/MmCommunicationLib/MmCommunicationLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/MmCommunicationLib/MmCommunicationLib.c
new file mode 100644
index 000000000000..bf400ec0a835
--- /dev/null
+++ b/Silicon/Ampere/AmpereAltraPkg/Library/MmCommunicationLib/MmCommunicationLib.c
@@ -0,0 +1,184 @@
+/** @file
+
+  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <IndustryStandard/ArmStdSmc.h>
+#include <Library/ArmLib.h>
+#include <Library/ArmSmcLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MmCommunicationLib.h>
+#include <Library/PcdLib.h>
+#include <Protocol/MmCommunication.h>
+
+//
+// Address, Length of the pre-allocated buffer for communication with the secure
+// world.
+//
+STATIC ARM_MEMORY_REGION_DESCRIPTOR mNsCommBuffMemRegion;
+
+EFI_STATUS
+EFIAPI
+MmCommunicationLibConstructor (
+  VOID
+  )
+{
+  mNsCommBuffMemRegion.PhysicalBase = PcdGet64 (PcdMmBufferBase);
+  // During boot , Virtual and Physical are same
Ideally, use UEFI-defined terms. "During boot" is quite ambiguous.


+  mNsCommBuffMemRegion.VirtualBase = mNsCommBuffMemRegion.PhysicalBase;
+  mNsCommBuffMemRegion.Length = PcdGet64 (PcdMmBufferSize);
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Communicates with a registered handler.
+
+  This function provides an interface to send and receive messages to the
+  Standalone MM environment in UEFI PEI phase.
+
+  @param[in, out] CommBuffer          A pointer to the buffer to convey
+                                      into MMRAM.
+  @param[in, out] CommSize            The size of the data buffer being
+                                      passed in. This is optional.
+
+  @retval EFI_SUCCESS                 The message was successfully posted.
+  @retval EFI_INVALID_PARAMETER       The CommBuffer was NULL.
+  @retval EFI_BAD_BUFFER_SIZE         The buffer size is incorrect for the MM
+                                      implementation. If this error is
+                                      returned, the MessageLength field in
+                                      the CommBuffer header or the integer
+                                      pointed by CommSize are updated to reflect
+                                      the maximum payload size the
+                                      implementation can accommodate.
+  @retval EFI_ACCESS_DENIED           The CommunicateBuffer parameter
+                                      or CommSize parameter, if not omitted,
+                                      are in address range that cannot be
+                                      accessed by the MM environment
+**/
+EFI_STATUS
+EFIAPI
+MmCommunicationCommunicate (
+  IN OUT VOID  *CommBuffer,
+  IN OUT UINTN *CommSize OPTIONAL
+  )
+{
+  EFI_MM_COMMUNICATE_HEADER *CommunicateHeader;
+  ARM_SMC_ARGS              CommunicateSmcArgs;
+  EFI_STATUS                Status;
+  UINTN                     BufferSize;
+
+  Status = EFI_ACCESS_DENIED;
+  BufferSize = 0;
+
+  ZeroMem (&CommunicateSmcArgs, sizeof (ARM_SMC_ARGS));
+
+  //
+  // Check parameters
+  //
+  if (CommBuffer == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  CommunicateHeader = CommBuffer;
+  // CommBuffer is a mandatory parameter. Hence, Rely on
+  // MessageLength + Header to ascertain the
+  // total size of the communication payload rather than
+  // rely on optional CommSize parameter
+  BufferSize = CommunicateHeader->MessageLength +
+               sizeof (CommunicateHeader->HeaderGuid) +
+               sizeof (CommunicateHeader->MessageLength);
+
+  // If the length of the CommBuffer is 0 then return the expected length.
+  if (CommSize != NULL) {
+    // This case can be used by the consumer of this driver to find out the
+    // max size that can be used for allocating CommBuffer.
+    if ((*CommSize == 0) ||
+        (*CommSize > mNsCommBuffMemRegion.Length))
+    {
{ at end of preceding line.
Please address throughout this file.

+      *CommSize = mNsCommBuffMemRegion.Length;
+      return EFI_BAD_BUFFER_SIZE;
+    }
+    //
+    // CommSize must match MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
+    //
+    if (*CommSize != BufferSize) {
+      return EFI_INVALID_PARAMETER;
+    }
+  }
+
+  //
+  // If the buffer size is 0 or greater than what can be tolerated by the MM
+  // environment then return the expected size.
+  //
+  if ((BufferSize == 0) ||
+      (BufferSize > mNsCommBuffMemRegion.Length))
+  {
+    CommunicateHeader->MessageLength = mNsCommBuffMemRegion.Length -
+                                       sizeof (CommunicateHeader->HeaderGuid) -
+                                       sizeof (CommunicateHeader->MessageLength);
+    return EFI_BAD_BUFFER_SIZE;
+  }
+
+  // SMC Function ID
+  CommunicateSmcArgs.Arg0 = ARM_SMC_ID_MM_COMMUNICATE_AARCH64;
+
+  // Cookie
+  CommunicateSmcArgs.Arg1 = 0;
+
+  // Copy Communication Payload
+  CopyMem ((VOID *)mNsCommBuffMemRegion.VirtualBase, CommBuffer, BufferSize);
+
+  // comm_buffer_address (64-bit physical address)
+  CommunicateSmcArgs.Arg2 = (UINTN)mNsCommBuffMemRegion.PhysicalBase;
+
+  // comm_size_address (not used, indicated by setting to zero)
+  CommunicateSmcArgs.Arg3 = 0;
+
+  // Call the Standalone MM environment.
+  ArmCallSmc (&CommunicateSmcArgs);
+
+  switch (CommunicateSmcArgs.Arg0) {
+  case ARM_SMC_MM_RET_SUCCESS:
+    ZeroMem (CommBuffer, BufferSize);
+    // On successful return, the size of data being returned is inferred from
+    // MessageLength + Header.
+    CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER *)mNsCommBuffMemRegion.VirtualBase;
+    BufferSize = CommunicateHeader->MessageLength +
+                 sizeof (CommunicateHeader->HeaderGuid) +
+                 sizeof (CommunicateHeader->MessageLength);
+
+    CopyMem (
+      CommBuffer,
+      (VOID *)mNsCommBuffMemRegion.VirtualBase,
+      BufferSize
+      );
+    Status = EFI_SUCCESS;
+    break;
+
+  case ARM_SMC_MM_RET_INVALID_PARAMS:
+    Status = EFI_INVALID_PARAMETER;
+    break;
+
+  case ARM_SMC_MM_RET_DENIED:
+    Status = EFI_ACCESS_DENIED;
+    break;
+
+  case ARM_SMC_MM_RET_NO_MEMORY:
+    // Unexpected error since the CommSize was checked for zero length
+    // prior to issuing the SMC
+    Status = EFI_OUT_OF_RESOURCES;
+    ASSERT (0);
+    break;
+
+  default:
+    Status = EFI_ACCESS_DENIED;
+    ASSERT (0);
+  }
+
+  return Status;
+}

      
diff --git a/Platform/Ampere/JadePkg/JadeBoardSetting.cfg b/Platform/Ampere/JadePkg/JadeBoardSetting.cfg
new file mode 100644
index 000000000000..5a67e8fc6a75
--- /dev/null
+++ b/Platform/Ampere/JadePkg/JadeBoardSetting.cfg
@@ -0,0 +1,209 @@
+# Sample board setting
+#
+# This is a sample board setting as used for the
+# Ampere Altra reference design.
What is a board setting?

+#
+# Name, offset (hex), value
+# value can be hex or decimal
+#
+
+NV_SI_RO_BOARD_VENDOR, 0x0000, 0x0000CD3A
+NV_SI_RO_BOARD_TYPE, 0x0008, 0x00000000
+NV_SI_RO_BOARD_REV, 0x0010, 0x00000000
+NV_SI_RO_BOARD_CFG, 0x0018, 0x00000000
+NV_SI_RO_BOARD_S0_DIMM_AVAIL, 0x0020, 0x0000FFFF
+NV_SI_RO_BOARD_S1_DIMM_AVAIL, 0x0028, 0x0000FFFF
+NV_SI_RO_BOARD_SPI0CS0_FREQ_KHZ, 0x0030, 0x000080E8
+NV_SI_RO_BOARD_SPI0CS1_FREQ_KHZ, 0x0038, 0x000080E8
+NV_SI_RO_BOARD_SPI1CS0_FREQ_KHZ, 0x0040, 0x00002710
+NV_SI_RO_BOARD_SPI1CS1_FREQ_KHZ, 0x0048, 0x00002710
+NV_SI_RO_BOARD_TPM_LOC, 0x0050, 0x00000000
+NV_SI_RO_BOARD_I2C0_FREQ_KHZ, 0x0058, 0x00000190
+NV_SI_RO_BOARD_I2C1_FREQ_KHZ, 0x0060, 0x00000190
+NV_SI_RO_BOARD_I2C2_10_FREQ_KHZ, 0x0068, 0x00000190
+NV_SI_RO_BOARD_I2C3_FREQ_KHZ, 0x0070, 0x00000190
+NV_SI_RO_BOARD_I2C9_FREQ_KHZ, 0x0078, 0x00000190
+NV_SI_RO_BOARD_2P_CFG, 0x0080, 0xFFFFFF01
+NV_SI_RO_BOARD_S0_RCA0_CFG, 0x0088, 0x00000000
+NV_SI_RO_BOARD_S0_RCA1_CFG, 0x0090, 0x00000000
+NV_SI_RO_BOARD_S0_RCA2_CFG, 0x0098, 0x00000004
+NV_SI_RO_BOARD_S0_RCA3_CFG, 0x00A0, 0x00000004
+NV_SI_RO_BOARD_S0_RCB0_LO_CFG, 0x00A8, 0x00020002
+NV_SI_RO_BOARD_S0_RCB0_HI_CFG, 0x00B0, 0x00020002
+NV_SI_RO_BOARD_S0_RCB1_LO_CFG, 0x00B8, 0x00020002
+NV_SI_RO_BOARD_S0_RCB1_HI_CFG, 0x00C0, 0x00020002
+NV_SI_RO_BOARD_S0_RCB2_LO_CFG, 0x00C8, 0x00020002
+NV_SI_RO_BOARD_S0_RCB2_HI_CFG, 0x00D0, 0x00000003
+NV_SI_RO_BOARD_S0_RCB3_LO_CFG, 0x00D8, 0x00000003
+NV_SI_RO_BOARD_S0_RCB3_HI_CFG, 0x00E0, 0x00020002
+NV_SI_RO_BOARD_S1_RCA0_CFG, 0x00E8, 0x00000000
+NV_SI_RO_BOARD_S1_RCA1_CFG, 0x00F0, 0x00000000
+NV_SI_RO_BOARD_S1_RCA2_CFG, 0x00F8, 0x02020202
+NV_SI_RO_BOARD_S1_RCA3_CFG, 0x0100, 0x00030003
+NV_SI_RO_BOARD_S1_RCB0_LO_CFG, 0x0108, 0x00000003
+NV_SI_RO_BOARD_S1_RCB0_HI_CFG, 0x0110, 0x00020002
+NV_SI_RO_BOARD_S1_RCB1_LO_CFG, 0x0118, 0x00020002
+NV_SI_RO_BOARD_S1_RCB1_HI_CFG, 0x0120, 0x00000003
+NV_SI_RO_BOARD_S1_RCB2_LO_CFG, 0x0128, 0x00020002
+NV_SI_RO_BOARD_S1_RCB2_HI_CFG, 0x0130, 0x00020002
+NV_SI_RO_BOARD_S1_RCB3_LO_CFG, 0x0138, 0x00020002
+NV_SI_RO_BOARD_S1_RCB3_HI_CFG, 0x0140, 0x00020002
+NV_SI_RO_BOARD_T_LTLM_DELTA_P0, 0x0148, 0x00000001
+NV_SI_RO_BOARD_T_LTLM_DELTA_P1, 0x0150, 0x00000002
+NV_SI_RO_BOARD_T_LTLM_DELTA_P2, 0x0158, 0x00000003
+NV_SI_RO_BOARD_T_LTLM_DELTA_P3, 0x0160, 0x00000004
+NV_SI_RO_BOARD_T_LTLM_DELTA_M1, 0x0168, 0xFFFFFFFF
+NV_SI_RO_BOARD_T_LTLM_DELTA_M2, 0x0170, 0xFFFFFFFE
+NV_SI_RO_BOARD_T_LTLM_DELTA_M3, 0x0178, 0xFFFFFFFD
+NV_SI_RO_BOARD_P_LM_PID_P, 0x0180, 0x00000000
+NV_SI_RO_BOARD_P_LM_PID_I, 0x0188, 0x00000000
+NV_SI_RO_BOARD_P_LM_PID_I_L_THOLD, 0x0190, 0x00000000
+NV_SI_RO_BOARD_P_LM_PID_I_H_THOLD, 0x0198, 0x00000000
+NV_SI_RO_BOARD_P_LM_PID_D, 0x01A0, 0x00000000
+NV_SI_RO_BOARD_P_LM_EXP_SMOOTH_CONST, 0x01A8, 0x00000000
+NV_SI_RO_BOARD_TPM_ALG_ID, 0x01B0, 0x00000002
+NV_SI_RO_BOARD_DDR_SPEED_GRADE, 0x01B8, 0x00000C80
+NV_SI_RO_BOARD_DDR_S0_RTT_WR, 0x01C0, 0x00020000
+NV_SI_RO_BOARD_DDR_S1_RTT_WR, 0x01C8, 0x00020000
+NV_SI_RO_BOARD_DDR_S0_RTT_NOM, 0x01D0, 0xFF060177
+NV_SI_RO_BOARD_DDR_S1_RTT_NOM, 0x01D8, 0xFF060177
+NV_SI_RO_BOARD_DDR_S0_RTT_PARK, 0x01E0, 0x00060070
+NV_SI_RO_BOARD_DDR_S1_RTT_PARK, 0x01E8, 0x00060070
+NV_SI_RO_BOARD_DDR_CS0_RDODT_MASK_1DPC, 0x01F0, 0x00000000
+NV_SI_RO_BOARD_DDR_CS1_RDODT_MASK_1DPC, 0x01F8, 0x00000000
+NV_SI_RO_BOARD_DDR_CS2_RDODT_MASK_1DPC, 0x0200, 0x00000000
+NV_SI_RO_BOARD_DDR_CS3_RDODT_MASK_1DPC, 0x0208, 0x00000000
+NV_SI_RO_BOARD_DDR_CS0_RDODT_MASK_2DPC, 0x0210, 0x000C0CCC
+NV_SI_RO_BOARD_DDR_CS1_RDODT_MASK_2DPC, 0x0218, 0x000C0CCC
+NV_SI_RO_BOARD_DDR_CS2_RDODT_MASK_2DPC, 0x0220, 0x00030333
+NV_SI_RO_BOARD_DDR_CS3_RDODT_MASK_2DPC, 0x0228, 0x00030333
+NV_SI_RO_BOARD_DDR_CS0_WRODT_MASK_1DPC, 0x0230, 0x00030333
+NV_SI_RO_BOARD_DDR_CS1_WRODT_MASK_1DPC, 0x0238, 0x00030333
+NV_SI_RO_BOARD_DDR_CS2_WRODT_MASK_1DPC, 0x0240, 0x00030333
+NV_SI_RO_BOARD_DDR_CS3_WRODT_MASK_1DPC, 0x0248, 0x00030333
+NV_SI_RO_BOARD_DDR_CS0_WRODT_MASK_2DPC, 0x0250, 0x000EDEED
+NV_SI_RO_BOARD_DDR_CS1_WRODT_MASK_2DPC, 0x0258, 0x000DEDDE
+NV_SI_RO_BOARD_DDR_CS2_WRODT_MASK_2DPC, 0x0260, 0x000B7BB7
+NV_SI_RO_BOARD_DDR_CS3_WRODT_MASK_2DPC, 0x0268, 0x0007B77B
+NV_SI_RO_BOARD_DDR_PHY_TERM_DQ_CTRL_1DPC, 0x0270, 0x00000005
+NV_SI_RO_BOARD_DDR_PHY_TERM_DQ_VAL_1DPC, 0x0278, 0x0090DD90
+NV_SI_RO_BOARD_DDR_PHY_TERM_DQS_CTRL_1DPC, 0x0280, 0x00000005
+NV_SI_RO_BOARD_DDR_PHY_TERM_DQS_VAL_1DPC, 0x0288, 0x0090DD90
+NV_SI_RO_BOARD_DDR_PHY_TERM_DQ_CTRL_2DPC, 0x0290, 0x00000005
+NV_SI_RO_BOARD_DDR_PHY_TERM_DQ_VAL_2DPC, 0x0298, 0x0090DD90
+NV_SI_RO_BOARD_DDR_PHY_TERM_DQS_CTRL_2DPC, 0x02A0, 0x00000005
+NV_SI_RO_BOARD_DDR_PHY_TERM_DQS_VAL_2DPC, 0x02A8, 0x0090DD90
+NV_SI_RO_BOARD_DDR_PHY_VREFDQ_RANGE_VAL_1DPC, 0x02B0, 0x00000024
+NV_SI_RO_BOARD_DDR_DRAM_VREFDQ_RANGE_VAL_1DPC, 0x02B8, 0x0000001A
+NV_SI_RO_BOARD_DDR_PHY_VREFDQ_RANGE_VAL_2DPC, 0x02C0, 0x00000050
+NV_SI_RO_BOARD_DDR_DRAM_VREFDQ_RANGE_VAL_2DPC, 0x02C8, 0x00000020
+NV_SI_RO_BOARD_DDR_CLK_WRDQ_DLY_DEFAULT, 0x02D0, 0x02800280
+NV_SI_RO_BOARD_DDR_RDDQS_DQ_DLY_DEFAULT, 0x02D8, 0x90909090
+NV_SI_RO_BOARD_DDR_WRDQS_SHIFT_DEFAULT, 0x02E0, 0x00000000
+NV_SI_RO_BOARD_DDR_ADCMD_DLY_DEFAULT, 0x02E8, 0x00C000C0
+NV_SI_RO_BOARD_DDR_CLK_WRDQ_DLY_ADJ, 0x02F0, 0x00000000
+NV_SI_RO_BOARD_DDR_RDDQS_DQ_DLY_ADJ, 0x02F8, 0x00000000
+NV_SI_RO_BOARD_DDR_PHY_VREF_ADJ, 0x0300, 0x00000000
+NV_SI_RO_BOARD_DDR_DRAM_VREF_ADJ, 0x0308, 0x00000000
+NV_SI_RO_BOARD_DDR_WR_PREAMBLE_CYCLE, 0x0310, 0x02010201
+NV_SI_RO_BOARD_DDR_ADCMD_2T_MODE, 0x0318, 0x00000000
+NV_SI_RO_BOARD_I2C_VRD_CONFIG_INFO, 0x0320, 0x00000000
+NV_SI_RO_BOARD_DDR_PHY_FEATURE_CTRL, 0x0328, 0x00000000
+NV_SI_RO_BOARD_BMC_HANDSHAKE_SPI_ACCESS, 0x0330, 0x01050106
+NV_SI_RO_BOARD_DIMM_TEMP_THRESHOLD, 0x0338, 0x000005F4
+NV_SI_RO_BOARD_DIMM_SPD_COMPARE_DISABLE, 0x0340, 0x00000000
+NV_SI_RO_BOARD_S0_PCIE_CLK_CFG, 0x0348, 0x00000000
+NV_SI_RO_BOARD_S0_RCA4_CFG, 0x0350, 0x02020202
+NV_SI_RO_BOARD_S0_RCA5_CFG, 0x0358, 0x02020202
+NV_SI_RO_BOARD_S0_RCA6_CFG, 0x0360, 0x02020202
+NV_SI_RO_BOARD_S0_RCA7_CFG, 0x0368, 0x02020003
+NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET, 0x0370, 0x00000000
+NV_SI_RO_BOARD_S0_RCA1_TXRX_G3PRESET, 0x0378, 0x00000000
+NV_SI_RO_BOARD_S0_RCA2_TXRX_G3PRESET, 0x0380, 0x00000000
+NV_SI_RO_BOARD_S0_RCA3_TXRX_G3PRESET, 0x0388, 0x00000000
+NV_SI_RO_BOARD_S0_RCB0A_TXRX_G3PRESET, 0x0390, 0x00000000
+NV_SI_RO_BOARD_S0_RCB0B_TXRX_G3PRESET, 0x0398, 0x00000000
+NV_SI_RO_BOARD_S0_RCB1A_TXRX_G3PRESET, 0x03A0, 0x00000000
+NV_SI_RO_BOARD_S0_RCB1B_TXRX_G3PRESET, 0x03A8, 0x00000000
+NV_SI_RO_BOARD_S0_RCB2A_TXRX_G3PRESET, 0x03B0, 0x00000000
+NV_SI_RO_BOARD_S0_RCB2B_TXRX_G3PRESET, 0x03B8, 0x00000000
+NV_SI_RO_BOARD_S0_RCB3A_TXRX_G3PRESET, 0x03C0, 0x00000000
+NV_SI_RO_BOARD_S0_RCB3B_TXRX_G3PRESET, 0x03C8, 0x00000000
+NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET, 0x03D0, 0x00000000
+NV_SI_RO_BOARD_S0_RCA5_TXRX_G3PRESET, 0x03D8, 0x00000000
+NV_SI_RO_BOARD_S0_RCA6_TXRX_G3PRESET, 0x03E0, 0x00000000
+NV_SI_RO_BOARD_S0_RCA7_TXRX_G3PRESET, 0x03E8, 0x00000000
+NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET, 0x03F0, 0x57575757
+NV_SI_RO_BOARD_S0_RCA1_TXRX_G4PRESET, 0x03F8, 0x57575757
+NV_SI_RO_BOARD_S0_RCA2_TXRX_G4PRESET, 0x0400, 0x57575757
+NV_SI_RO_BOARD_S0_RCA3_TXRX_G4PRESET, 0x0408, 0x57575757
+NV_SI_RO_BOARD_S0_RCB0A_TXRX_G4PRESET, 0x0410, 0x57575757
+NV_SI_RO_BOARD_S0_RCB0B_TXRX_G4PRESET, 0x0418, 0x57575757
+NV_SI_RO_BOARD_S0_RCB1A_TXRX_G4PRESET, 0x0420, 0x57575757
+NV_SI_RO_BOARD_S0_RCB1B_TXRX_G4PRESET, 0x0428, 0x57575757
+NV_SI_RO_BOARD_S0_RCB2A_TXRX_G4PRESET, 0x0430, 0x57575757
+NV_SI_RO_BOARD_S0_RCB2B_TXRX_G4PRESET, 0x0438, 0x57575757
+NV_SI_RO_BOARD_S0_RCB3A_TXRX_G4PRESET, 0x0440, 0x57575757
+NV_SI_RO_BOARD_S0_RCB3B_TXRX_G4PRESET, 0x0448, 0x57575757
+NV_SI_RO_BOARD_S0_RCA4_TXRX_G4PRESET, 0x0450, 0x57575757
+NV_SI_RO_BOARD_S0_RCA5_TXRX_G4PRESET, 0x0458, 0x57575757
+NV_SI_RO_BOARD_S0_RCA6_TXRX_G4PRESET, 0x0460, 0x57575757
+NV_SI_RO_BOARD_S0_RCA7_TXRX_G4PRESET, 0x0468, 0x57575757
+NV_SI_RO_BOARD_S1_PCIE_CLK_CFG, 0x0470, 0x00000000
+NV_SI_RO_BOARD_S1_RCA4_CFG, 0x0478, 0x02020202
+NV_SI_RO_BOARD_S1_RCA5_CFG, 0x0480, 0x02020202
+NV_SI_RO_BOARD_S1_RCA6_CFG, 0x0488, 0x02020202
+NV_SI_RO_BOARD_S1_RCA7_CFG, 0x0490, 0x02020003
+NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET, 0x0498, 0x00000000
+NV_SI_RO_BOARD_S1_RCA3_TXRX_G3PRESET, 0x04A0, 0x00000000
+NV_SI_RO_BOARD_S1_RCB0A_TXRX_G3PRESET, 0x04A8, 0x00000000
+NV_SI_RO_BOARD_S1_RCB0B_TXRX_G3PRESET, 0x04B0, 0x00000000
+NV_SI_RO_BOARD_S1_RCB1A_TXRX_G3PRESET, 0x04B8, 0x00000000
+NV_SI_RO_BOARD_S1_RCB1B_TXRX_G3PRESET, 0x04C0, 0x00000000
+NV_SI_RO_BOARD_S1_RCB2A_TXRX_G3PRESET, 0x04C8, 0x00000000
+NV_SI_RO_BOARD_S1_RCB2B_TXRX_G3PRESET, 0x04D0, 0x00000000
+NV_SI_RO_BOARD_S1_RCB3A_TXRX_G3PRESET, 0x04D8, 0x00000000
+NV_SI_RO_BOARD_S1_RCB3B_TXRX_G3PRESET, 0x04E0, 0x00000000
+NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET, 0x04E8, 0x00000000
+NV_SI_RO_BOARD_S1_RCA5_TXRX_G3PRESET, 0x04F0, 0x00000000
+NV_SI_RO_BOARD_S1_RCA6_TXRX_G3PRESET, 0x04F8, 0x00000000
+NV_SI_RO_BOARD_S1_RCA7_TXRX_G3PRESET, 0x0500, 0x00000000
+NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET, 0x0508, 0x57575757
+NV_SI_RO_BOARD_S1_RCA3_TXRX_G4PRESET, 0x0510, 0x57575757
+NV_SI_RO_BOARD_S1_RCB0A_TXRX_G4PRESET, 0x0518, 0x57575757
+NV_SI_RO_BOARD_S1_RCB0B_TXRX_G4PRESET, 0x0520, 0x57575757
+NV_SI_RO_BOARD_S1_RCB1A_TXRX_G4PRESET, 0x0528, 0x57575757
+NV_SI_RO_BOARD_S1_RCB1B_TXRX_G4PRESET, 0x0530, 0x57575757
+NV_SI_RO_BOARD_S1_RCB2A_TXRX_G4PRESET, 0x0538, 0x57575757
+NV_SI_RO_BOARD_S1_RCB2B_TXRX_G4PRESET, 0x0540, 0x57575757
+NV_SI_RO_BOARD_S1_RCB3A_TXRX_G4PRESET, 0x0548, 0x57575757
+NV_SI_RO_BOARD_S1_RCB3B_TXRX_G4PRESET, 0x0550, 0x57575757
+NV_SI_RO_BOARD_S1_RCA4_TXRX_G4PRESET, 0x0558, 0x57575757
+NV_SI_RO_BOARD_S1_RCA5_TXRX_G4PRESET, 0x0560, 0x57575757
+NV_SI_RO_BOARD_S1_RCA6_TXRX_G4PRESET, 0x0568, 0x57575757
+NV_SI_RO_BOARD_S1_RCA7_TXRX_G4PRESET, 0x0570, 0x57575757
+NV_SI_RO_BOARD_2P_CE_MASK_THRESHOLD, 0x0578, 0x00000003
+NV_SI_RO_BOARD_2P_CE_MASK_INTERVAL, 0x0580, 0x000001A4
+NV_SI_RO_BOARD_SX_PHY_CFG_SETTING, 0x0588, 0x00000000
+NV_SI_RO_BOARD_DDR_PHY_DC_CLK, 0x0590, 0x00018000
+NV_SI_RO_BOARD_DDR_PHY_DC_DATA, 0x0598, 0x80018000
+NV_SI_RO_BOARD_SX_RCA0_TXRX_20GPRESET, 0x05A0, 0x00000000
+NV_SI_RO_BOARD_SX_RCA1_TXRX_20GPRESET, 0x05A8, 0x00000000
+NV_SI_RO_BOARD_SX_RCA2_TXRX_20GPRESET, 0x05B0, 0x00000000
+NV_SI_RO_BOARD_SX_RCA3_TXRX_20GPRESET, 0x05B8, 0x00000000
+NV_SI_RO_BOARD_SX_RCA0_TXRX_25GPRESET, 0x05C0, 0x00000000
+NV_SI_RO_BOARD_SX_RCA1_TXRX_25GPRESET, 0x05C8, 0x00000000
+NV_SI_RO_BOARD_SX_RCA2_TXRX_25GPRESET, 0x05D0, 0x00000000
+NV_SI_RO_BOARD_SX_RCA3_TXRX_25GPRESET, 0x05D8, 0x00000000
+NV_SI_RO_BOARD_DDR_2X_REFRESH_TEMP_THRESHOLD, 0x05E0, 0x00550055
+NV_SI_RO_BOARD_PCP_VRD_VOUT_WAIT_US, 0x05E8, 0x00000064
+NV_SI_RO_BOARD_PCP_VRD_VOUT_RESOLUTION_MV, 0x05F0, 0x00000005
+NV_SI_RO_BOARD_DVFS_VOLT_READ_BACK_EN, 0x05F8, 0x00000001
+NV_SI_RO_BOARD_DVFS_VOLT_READ_BACK_TIME, 0x0600, 0x00000002
+NV_SI_RO_BOARD_DVFS_VOUT_20MV_RAMP_TIME_US, 0x0608, 0x00000005
+NV_SI_RO_BOARD_PCIE_AER_FW_FIRST, 0x0610, 0x00000000
+NV_SI_RO_BOARD_RTC_GPI_LOCK_BYPASS, 0x0618, 0x00000000
+NV_SI_RO_BOARD_TPM_DISABLE, 0x0620, 0x00000000
+NV_SI_RO_BOARD_MESH_S0_CXG_RC_STRONG_ORDERING_EN, 0x0628, 0x00000000
+NV_SI_RO_BOARD_MESH_S1_CXG_RC_STRONG_ORDERING_EN, 0x0630, 0x00000000
+NV_SI_RO_BOARD_GPIO_SW_WATCHDOG_EN, 0x0638, 0x00000000
There was also a few things in this patch where I felt names had
insufficient namespace - such as starting with TRNG_ or NV_.
I'm not going to insist on adding prefixes to those, I'm just going to
say I have warned you, and those might get you in trouble in the
future :)

Best Regards,

Leif


Re: [Patch V3 1/9] MdeModulePkg: Add Universal Payload general defination header file

Ma, Maurice
 

Hi, Ray,

Yes, I agree. PLD might cause confusion sometimes.
Maybe we just use the full name UNIVERSAL_PAYLOAD instead ?

Thanks
Maurice

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, June 8, 2021 19:58
To: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
<guo.dong@intel.com>; Rangarajan, Ravi P <ravi.p.rangarajan@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>;
Liu, Zhiguang <zhiguang.liu@intel.com>; Zimmer, Vincent
<vincent.zimmer@intel.com>
Subject: RE: [edk2-devel] [Patch V3 1/9] MdeModulePkg: Add Universal
Payload general defination header file

Mike,
Thanks for the recommendation.
Just check https://en.wikipedia.org/wiki/Programmable_logic_device and it
seems to me PLD is a very common term in EE world. FPGA is a kind of PLD.

Maurice, Ravi, Guo, comments?

Thanks,
Ray

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Michael
D Kinney
Sent: Wednesday, June 9, 2021 12:26 AM
To: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io;
Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: Re: [edk2-devel] [Patch V3 1/9] MdeModulePkg: Add Universal
Payload general defination header file

I see use of the abbreviation PLD in this series.

PLD is sometimes interpreted as Programmable Logic Device.

Given this is for Universal Payload, I recommend using
UNIVERSAL_PAYLOAD or PAYLOAD as appropriate.

Mike

-----Original Message-----
From: Liu, Zhiguang <zhiguang.liu@intel.com>
Sent: Friday, June 4, 2021 2:42 AM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
<gaoliming@byosoft.com.cn>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: [Patch V3 1/9] MdeModulePkg: Add Universal Payload general
defination header file

V1:
Add Universal Payload general defination header file according to
Universal Payload’s documentation
V2:
Add a macro funtion to check the Revision

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>

Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdeModulePkg/Include/UniversalPayload/UniversalPayload.h | 33
+++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)

diff --git
a/MdeModulePkg/Include/UniversalPayload/UniversalPayload.h
b/MdeModulePkg/Include/UniversalPayload/UniversalPayload.h
new file mode 100644
index 0000000000..627b9e880e
--- /dev/null
+++ b/MdeModulePkg/Include/UniversalPayload/UniversalPayload.h
@@ -0,0 +1,33 @@
+/** @file

+ Universal Payload general definations.

+

+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>

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

+

+**/

+

+#ifndef __UNIVERSAL_PAYLOAD_H__

+#define __UNIVERSAL_PAYLOAD_H__

+

+#pragma pack(1)

+

+typedef struct {

+ UINT8 Revision;

+ UINT8 Reserved;

+ UINT16 Length;

+} PLD_GENERIC_HEADER;

+

+#pragma pack()

+

+/**

+ Returns the size of a structure of known type, up through and
+ including a
specified field.

+

+ @param TYPE The name of the data structure that contains the field
specified by Field.

+ @param Field The name of the field in the data structure.

+

+ @return size, in bytes.

+

+**/

+#define PLD_SIZEOF_THROUGH_FIELD(TYPE, Field) (OFFSET_OF(TYPE,
Field) + sizeof (((TYPE *) 0)->Field))

+

+#endif // __UNIVERSAL_PAYLOAD_H__

--
2.30.0.windows.2




Re: [PATCHV2] CryptoPkg/BaseCryptLib: Enabled CryptSha512 for Smm/Runtime drivers

Yao, Jiewen
 

Thank you! Shengfeng

Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

I recommend to wait for *1 week*, to see if anyone has concern on size change.

Thank you
Yao Jiewen

-----Original Message-----
From: xueshengfeng <xueshengfeng@byosoft.com.cn>
Sent: Tuesday, June 8, 2021 12:31 PM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>
Cc: Xue, ShengfengX <shengfengx.xue@intel.com>
Subject: [PATCHV2] CryptoPkg/BaseCryptLib: Enabled CryptSha512 for
Smm/Runtime drivers

Intel Platform utility Syscfg/sysfwupdt will trigger SMI
to enter BIOS interface. then BIOS invoke EncodePassword
in SMM mode to check password.
it's need sha384(in CryptSha512.c) in SMM mode.

the origin SmmCryptLib.lib size is 1389KB,
after changed, the size is 1391KB.

the origin RuntimeCryptLib.lib size is 911KB,
after changed,the size is 913KB.

in SmmCryptLib.inf and RuntimeCryptLib.inf,
change CryptSha512NULL.c to CryptSha512.c.

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

Signed-off-by: xueshengfeng <xueshengfeng@byosoft.com.cn>
---
CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 6 +++---
CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
index 3d3a6fb94a..fdbb6edfd2 100644
--- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
@@ -11,8 +11,8 @@
# functions, PKCS#7 SignedData sign functions, Diffie-Hellman functions, and
# authenticode signature verification functions are not supported in this
instance.
#
-# Copyright (c) 2009 - 2020, Intel Corporation. All rights reserved.<BR>
-# Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights
reserved.<BR>
+# Copyright (c) 2009 - 2021, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2021, Hewlett Packard Enterprise Development LP. All rights
reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -39,7 +39,7 @@
Hash/CryptSha1.c
Hash/CryptSha256.c
Hash/CryptSm3.c
- Hash/CryptSha512Null.c
+ Hash/CryptSha512.c
Hmac/CryptHmacSha256.c
Kdf/CryptHkdf.c
Cipher/CryptAes.c
diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
index 07c376ce04..e6470d7a21 100644
--- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
@@ -10,7 +10,7 @@
# RSA external functions, PKCS#7 SignedData sign functions, Diffie-Hellman
functions, and
# authenticode signature verification functions are not supported in this
instance.
#
-# Copyright (c) 2010 - 2020, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2010 - 2021, Intel Corporation. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -37,7 +37,7 @@
Hash/CryptSha1.c
Hash/CryptSha256.c
Hash/CryptSm3.c
- Hash/CryptSha512Null.c
+ Hash/CryptSha512.c
Hmac/CryptHmacSha256.c
Kdf/CryptHkdfNull.c
Cipher/CryptAes.c
--
2.31.1.windows.1


Re: [Patch V3 1/9] MdeModulePkg: Add Universal Payload general defination header file

Ni, Ray
 

Mike,
Thanks for the recommendation.
Just check https://en.wikipedia.org/wiki/Programmable_logic_device and it seems to me PLD is a very common term in EE world. FPGA is a kind of PLD.

Maurice, Ravi, Guo, comments?

Thanks,
Ray

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
D Kinney
Sent: Wednesday, June 9, 2021 12:26 AM
To: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io; Kinney,
Michael D <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: Re: [edk2-devel] [Patch V3 1/9] MdeModulePkg: Add Universal
Payload general defination header file

I see use of the abbreviation PLD in this series.

PLD is sometimes interpreted as Programmable Logic Device.

Given this is for Universal Payload, I recommend using UNIVERSAL_PAYLOAD
or PAYLOAD as appropriate.

Mike

-----Original Message-----
From: Liu, Zhiguang <zhiguang.liu@intel.com>
Sent: Friday, June 4, 2021 2:42 AM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
<gaoliming@byosoft.com.cn>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: [Patch V3 1/9] MdeModulePkg: Add Universal Payload general
defination header file

V1:
Add Universal Payload general defination header file according to
Universal Payload’s documentation
V2:
Add a macro funtion to check the Revision

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>

Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdeModulePkg/Include/UniversalPayload/UniversalPayload.h | 33
+++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)

diff --git a/MdeModulePkg/Include/UniversalPayload/UniversalPayload.h
b/MdeModulePkg/Include/UniversalPayload/UniversalPayload.h
new file mode 100644
index 0000000000..627b9e880e
--- /dev/null
+++ b/MdeModulePkg/Include/UniversalPayload/UniversalPayload.h
@@ -0,0 +1,33 @@
+/** @file

+ Universal Payload general definations.

+

+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>

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

+

+**/

+

+#ifndef __UNIVERSAL_PAYLOAD_H__

+#define __UNIVERSAL_PAYLOAD_H__

+

+#pragma pack(1)

+

+typedef struct {

+ UINT8 Revision;

+ UINT8 Reserved;

+ UINT16 Length;

+} PLD_GENERIC_HEADER;

+

+#pragma pack()

+

+/**

+ Returns the size of a structure of known type, up through and including a
specified field.

+

+ @param TYPE The name of the data structure that contains the field
specified by Field.

+ @param Field The name of the field in the data structure.

+

+ @return size, in bytes.

+

+**/

+#define PLD_SIZEOF_THROUGH_FIELD(TYPE, Field) (OFFSET_OF(TYPE,
Field) + sizeof (((TYPE *) 0)->Field))

+

+#endif // __UNIVERSAL_PAYLOAD_H__

--
2.30.0.windows.2




Re: [PATCH v1 1/1] MdeModulePkg/BdsDxe: Update BdsEntry to use Variable Policy

Gao, Zhichao
 

Add Liming to review as it is a change related to variable.

Thanks,
Zhichao

-----Original Message-----
From: kenlautner3@gmail.com <kenlautner3@gmail.com>
Sent: Saturday, June 5, 2021 4:13 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: [PATCH v1 1/1] MdeModulePkg/BdsDxe: Update BdsEntry to use
Variable Policy

From: Ken Lautner <klautner@microsoft.com>

Changed BdsEntry.c to use Variable Policy instead of Variable Lock as Variable
Lock will be Deprecated eventually

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Kenneth Lautner <kenlautner3@gmail.com>
---
MdeModulePkg/Universal/BdsDxe/BdsDxe.inf | 4 +++-
MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 20 +++++++++++++++-----
2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
index 9310b4dccb18..76ff6a0f5fc3 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
+++ b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
@@ -50,6 +50,8 @@
BaseMemoryLib DebugLib UefiBootManagerLib+ VariablePolicyLib+
VariablePolicyHelperLib PlatformBootManagerLib PcdLib PrintLib@@ -77,7
+79,7 @@
[Protocols] gEfiBdsArchProtocolGuid ## PRODUCES
gEfiSimpleTextInputExProtocolGuid ## CONSUMES-
gEdkiiVariableLockProtocolGuid ## SOMETIMES_CONSUMES+
gEdkiiVariablePolicyProtocolGuid ## SOMETIMES_CONSUMES
gEfiDeferredImageLoadProtocolGuid ## CONSUMES [FeaturePcd]diff -
-git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index 83b773a2fa5f..13723bee299b 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -15,6 +15,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include "Bds.h" #include "Language.h" #include
"HwErrRecSupport.h"+#include "library/VariablePolicyHelperLib.h" #define
SET_BOOT_OPTION_SUPPORT_KEY_COUNT(a, c) { \ (a) = ((a) &
~EFI_BOOT_OPTION_SUPPORT_COUNT) | (((c) << LowBitSet32
(EFI_BOOT_OPTION_SUPPORT_COUNT)) &
EFI_BOOT_OPTION_SUPPORT_COUNT); \@@ -670,7 +671,7 @@ BdsEntry (
EFI_STATUS Status; UINT32 BootOptionSupport;
UINT16 BootTimeOut;- EDKII_VARIABLE_LOCK_PROTOCOL
*VariableLock;+ EDKII_VARIABLE_POLICY_PROTOCOL *VariablePolicy;
UINTN Index; EFI_BOOT_MANAGER_LOAD_OPTION
LoadOption; UINT16 *BootNext;@@ -716,12 +717,21 @@
BdsEntry (
// // Mark the read-only variables if the Variable Lock protocol exists //-
Status = gBS->LocateProtocol (&gEdkiiVariableLockProtocolGuid, NULL,
(VOID **) &VariableLock);- DEBUG ((EFI_D_INFO, "[BdsDxe] Locate Variable
Lock protocol - %r\n", Status));+ Status = gBS-
LocateProtocol(&gEdkiiVariablePolicyProtocolGuid, NULL,
(VOID**)&VariablePolicy);+ DEBUG((DEBUG_INFO, "[BdsDxe] Locate
Variable Policy protocol - %r\n", Status)); if (!EFI_ERROR (Status)) { for
(Index = 0; Index < ARRAY_SIZE (mReadOnlyVariables); Index++) {- Status
= VariableLock->RequestToLock (VariableLock, mReadOnlyVariables[Index],
&gEfiGlobalVariableGuid);- ASSERT_EFI_ERROR (Status);+ Status =
RegisterBasicVariablePolicy(+ VariablePolicy,+
&gEfiGlobalVariableGuid,+ mReadOnlyVariables[Index],+
VARIABLE_POLICY_NO_MIN_SIZE,+
VARIABLE_POLICY_NO_MAX_SIZE,+
VARIABLE_POLICY_NO_MUST_ATTR,+
VARIABLE_POLICY_NO_CANT_ATTR,+
VARIABLE_POLICY_TYPE_LOCK_NOW+ );+
ASSERT_EFI_ERROR(Status); } } --
2.25.1.windows.1


Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

Min Xu
 

On 06/09/2021 12:01 AM, James Bottomley wrote:
It looks like I'll be travelling that day, but should be able to attend at least the
first 45 minutes of the design review from the airport lounge.
Thanks much James!

On TdMailbox and TdHob, we already have two SEV pages in the MEMFD and
since TDX and SEV is an either/or, could we simply not rename both pages and
use them for either boot depending on what CPU type is detected, so we only
have two MEMFD pages, not four?
Agree. A good idea.

On your slide 13 Question: "Open: How will the QEMU find the metadata
location?" can't you just use the mechanism for SEV that's already upstream in
both QEMU and OVMF?
I have answered this comments in my last answer to Laszlo.


On slide 19, the mucking with the reset vector really worries me because we
don't have that much space to play with. Given that you're starting in 32 bit
mode and can thus enter anywhere in the lower 4GB, why not simply use a
different and TDX specific entry point?
If TDVF has a separate DSF/FDF, this is not a problem.

I once think about below solution, that different mode goes to its specific entry point.
For example:
real-mode goes to 0xfffffff0,
protected-mode goes to 0xffffffe0,
long-mode goes to 0xffffffd0.

Let's wait for a conclusion to the *one binary* solution.


I'm not quite sure why you don't have a PEI phase, since TdxStartupLib seems
effectively to be PEI.
I will answer this comments in my next mail. Thanks

On all the Tcg2 changes: what about installing a vTPM driver that simply
translates to your MSRs? That way we can use all the standard TCG code as is?
Plus then we could do SEV-SNP measurement through an actual vTPM running
at higher VMPL or something.
I don’t quite understand "installing a vTPM driver". Can you explain more about
the vTpm driver? Do you mean TDX specific RTMR extending is implemented in the
"vTpm driver" ?


Slide 41: IOMMU operation. The implication is that you only transition to
unencrypted memory for DMA during the actual operation, so do I have it
correct that the guest writes DMA to encrypted memory, then the iommu
marks the region as unencrypted and transforms the memory to be in the clear
and then transforms it back after the DMA operation completes? Given that
SEV operates quite happily with always in the clear DMA buffers, this seems to
have the potential to be a performance problem, but what security does it gain?
See my comments in my last answer to Laszlo.

Thanks!
Min

4601 - 4620 of 80786