Date   

Re: separate OVMF binary for TDX? [was: OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest]

Laszlo Ersek
 

On 04/21/21 19:07, Erdem Aktas wrote:
Hi Laszlo,

I am sorry to hear that it sounded like we are dictating a certain
approach. Although I can see why it sounded that way, it certainly was not
my intention.
We want to work with the EDK2 community to have a solution that is
beneficial for everyone and we appreciate the inputs that we got from you
and Paolo. Code quality is always a high priority for us. Therefore, if,
at some point, things get too hacky to impact the
quality/maintainability of the upstream code, we will consider making
adjustments on our side.

With the current discussion, I was just trying to describe our use case and
the importance of having a single binary where there is no absolute need
for architectural differences. As far as I know, the only problematic area
is modifying the reset vector to be compatible with TDX and it seems like
Intel has a solution for it without impacting the overall quality of the
upstream code. I just want to reiterate that we are open for discussion and
what we ask should not be considered "at all cost" and please let us know
that if edk2 community/maintainers are still thinking that what Intel is
proposing is not feasible.
OK.

It's not lost on me that we're talking about ~3 instructions.

Let's keep a close eye on further "polymorphisms" that would require hacks.


Can Google at least propose a designated reviewer ("R") for the
"OvmfPkg: Confidential Computing" section of "Maintainers.txt", in a
patch?
Sure I would be happy too.
Yes, please do that. It can be included in the TDX patch set from Min Xu
that modifies the beginning of reset vector as discussed above.

Thanks!
Laszlo


-Erdem

On Wed, Apr 21, 2021 at 3:44 AM Laszlo Ersek <lersek@redhat.com> wrote:

On 04/21/21 02:38, Yao, Jiewen wrote:
Hello
Do we have some conclusion on this topic?

Do we agree the one-binary solution in OVMF or we need more discussion?
Well it's not technically impossible to do, just very ugly and brittle.
And I'm doubtful that this is a unique problem ("just fix the reset
vector") the likes of which will supposedly never return during the
integration of SEV and TDX. Once we make this promise ("one firmware
binary at all costs"), the hacks we accept for its sake will only
accumulate over time, and we'll have more and more precedent to justify
the next hack. Technical debt is not exactly what we don't have enough
of, in edk2.

I won't make a secret out of the fact that I'm slightly annoyed that
this approach is being dictated by Google (as far as I understand, at
this point, anyway). I don't see or recall a lot of Google contributions
in the edk2 history or the bug tracker. I'm not enthusiastic about
complexity without explicit commitment / investment on the beneficiary's
side.

I won't nack the approach personally, but I'm quite unhappy about it.
Can Google at least propose a designated reviewer ("R") for the
"OvmfPkg: Confidential Computing" section of "Maintainers.txt", in a patch?

Thanks
Laszlo



Thank you
Yao Jiewen

-----Original Message-----
From: Erdem Aktas <erdemaktas@google.com>
Sent: Friday, April 16, 2021 3:43 AM
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: devel@edk2.groups.io; jejb@linux.ibm.com; Yao, Jiewen
<jiewen.yao@intel.com>; dgilbert@redhat.com; Laszlo Ersek
<lersek@redhat.com>; Xu, Min M <min.m.xu@intel.com>;
thomas.lendacky@amd.com; Brijesh Singh <brijesh.singh@amd.com>; Justen,
Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Nathaniel McCallum
<npmccallum@redhat.com>; Ning Yang <ningyang@google.com>
Subject: Re: [edk2-devel] separate OVMF binary for TDX? [was: OvmfPkg:
Reserve the Secrets and Cpuid page for the SEV-SNP guest]

Thanks Paolo.

On Thu, Apr 15, 2021 at 12:59 AM Paolo Bonzini <pbonzini@redhat.com>
wrote:

On 15/04/21 01:34, Erdem Aktas wrote:
We do not want to generate different binaries for AMD, Intel, Intel
with TDX, AMD with SEV/SNP etc
My question is why the user would want a single binary for VMs with and
without TDX/SNP. I know there is attestation, but why would you even
want the _possibility_ that your guest starts running without TDX or
SNP
protection, and only find out later via attestation?
There might be multiple reasons why customers want it but we need this
requirement for a couple of other reasons too.

We do not only have hardware based confidential VMs. We might have
some other solutions which measure the initial image before boot.
Ultimately we might want to use a common attestation interface where
customers might be running different kinds of VMs. Using a single
binary will make it easier to manage/verify measurements for both of
us and the customers. I am not a PM so I cannot give more context on
customer use cases.

Another reason is how we deploy and manage guest firmware. We have a
lot of optimization and customization to speed up firmware loading
time and also reduce the time to deploy new builds on the whole fleet
uniformly. Adding a new firmware binary is a big challenge for us to
enable these features. On the top of integration challenges, it will
create maintainability issues in the long run for us when we provide
tools to verify/reproduce the hashes in the attestation report.

want the _possibility_ that your guest starts running without TDX or
SNP
protection, and only find out later via attestation?
I am missing the point here. Customers should rely on only the
attestation report to establish the trust.
-If firmware does not support TDX and TDX is enabled, that firmware
will crash at some point.
-If firmware is generic firmware that supports TDX and SNP and others,
and TDX is enabled or not, still the customer needs to verify the TDX
enablement through attestation.
-If firmware is a customized binary compiled to support TDX,
irrelevant of TDX being enabled or not, still the customer needs to
verify the TDX enablement through attestation.


For a similar reason, OVMF already supports shipping a binary that
fails
to boot if SMM is not available to the firmware, because then secure
boot would be trivially circumvented.

I can understand having a single binary for both TDX or SNP. That's
not
a problem since you can set up the SEV startup VMSA to 32-bit protected
mode just like TDX wants.
I agree that this is doable but I am not sure if we need to also
modify the reset vector for AMD SNP in that case. Also it will not
solve our problem. If we start to generate a new firmware for every
feature , it will not end well for us, I think. Both TDX and SNP are
still new features in the same architecture, and it seems to me that
they are sharing a lot of common/similar code. AMD has already made
some of their patches in (SEV and SEV-ES) which works very nicely for
our use case and integration. Looks like Intel just has an issue on
how to fix their reset vector problem. Once they solve it and upstream
accepts the changes, I do not see any other big blocker. OVMF was
doing a great job on abstracting differences and providing a common
interface without creating multiple binaries. I do not see why it
should not do the same thing here.

therefore we were expecting the TDX
changes to be part of the upstream code.
Having 1 or more binaries should be unrelated to the changes being
upstream (or more likely, I am misunderstanding you).
You are right, it is my bad for not clarifying it. What I mean is we
want it to be part of the upstream so it can be easier for us to pull
the changes and they are compatible with the changes that SNP is doing
but we also do not want to use different configuration files to
generate different binaries for each use case.


Thanks,

Paolo






Re: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS verify support

Agrawal, Sachin
 

Hi Jiewen,

Thanks for sharing these references.

We are currently using Salt Length of digest length.
I will add the test for new API in the unit test framework in the next version of the patch.

In reference to adding support for RsaPssSign() API : This maybe due to my ignorance, but I am unaware of usages where BIOS is involved in doing asymmetric signing during run time. I do see that CryptoPkg also contains TLS interface and that would involve asymmetric signing, but that will directly use the OpenSSL's TLS interface for signing. And, therefore I was skeptical about adding RsaPssSign interface.

Thanks
Sachin

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Tuesday, April 20, 2021 6:29 PM
To: Agrawal, Sachin <sachin.agrawal@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS verify support

HI Sachin
Sorry, I forget to add link for the reference.

1) TPM2 Library Specification, part 2 structure (https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p64_Part2_Structures_15may2021.pdf) describes the PSS salt length.

For the TPM_ALG_RSAPSS signing scheme, ...
.... The salt size is
always the largest salt value that will fit into the available space.


2) NIST FIPS 186-5 draft (https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-5-draft.pdf) and NIST FIPS 186-4 (https://doi.org/10.6028/NIST.FIPS.186-4) says:

For RSASSA-PSS,
the length (in bytes) of the salt (sLen) shall satisfy 0 ≤ sLen ≤ hLen

3) TCG FIPS 140-2 Guidance for TPM2 (https://trustedcomputinggroup.org/resource/tcg-fips-140-2-guidance-for-tpm-2-0/) mentions:

Language in [1] Part 1 Appendix B.7 RSASSA_PSS indicates:
"For both restricted and unrestricted signing keys, the random salt length will be the largest
size allowed by the key size and message digest size.
NOTE If the TPM implementation is required to be compliant with FIPS 186-4, then the random
salt length will be the largest size allowed by that specification."

4) TLS1.3 - RFC8446 (https://datatracker.ietf.org/doc/rfc8446/) has below.

RSASSA-PSS PSS algorithms:
The length of the Salt MUST be equal to the length of the digest
algorithm.


My view is that, TLS 1.3 and TPM FIPS mode require salt length == hash length, explicitly.

May I know that in your production, which salt length you choose in signing?
If you also choose salt length == hash length, then I would recommend make the default behavior to be HASH_LEN instead of AUTO.

Also, may I recommend we add RsaPssSign API as well?

Please also add the new API to the crypto test unit test.


I notice that crypto implementation (such as openssl, mbedtls) has API to let caller indicate what is the expected salt length. The caller may want AUTO or MAX in their special environment. I am OK to add another API later (such as RsaPssVerifyEx) to satisfy that need, if there is real use case.




-----Original Message-----
From: Agrawal, Sachin <sachin.agrawal@intel.com>
Sent: Tuesday, April 20, 2021 11:20 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

Hi Jiewen,

I reviewed RFC 8017 and I could not find any specific
'recommendations' on salt length to be used during signing with PSS encoding scheme.
However, in Section D.5.2.2.1(Notes 2) of IEEE 1363a-2004, it is
recommended to use salt length atleast equal to the hash digest length.

We can modify the current API to take a additional parameter as salt
length and ONLY pursue verification operation if Salt length is
atleast equal to digest length.
This will act as a hardening mechanism for Edk2 as it will accept
signatures only with 'appropriate' salt lengths.

Let me know if this is fine and I will push a corresponding patch.

Thx
Sachin


-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Tuesday, April 20, 2021 2:12 AM
To: Agrawal, Sachin <sachin.agrawal@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

Right. That has PROs and CONs.

On one hand, that allows maximum compatibility, salt could be
HASH_SIZE or MAX, or even 0 ?

On the other hand, what if the consumer only wants to accept a
specific length? E.g. TPM in FIPS mode and TLS requires SaltLength==HashLength.

Thank you
Yao Jiewen


-----Original Message-----
From: Agrawal, Sachin <sachin.agrawal@intel.com>
Sent: Tuesday, April 20, 2021 3:19 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

Hi Jiewen,

From Section 9.1 in RFC 8017:
" Note that the verification operation follows reverse steps to recover
salt and then forward steps to recompute and compare H."

Therefore, salt length can be inferred from the PSS block structure
during verification operation.

I opted for 'RSA_PSS_SALTLEN_AUTO' as it will allow Edk2 to verify
PSS signatures of any salt lengths.

Thanks
Sachin

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Monday, April 19, 2021 7:30 PM
To: Agrawal, Sachin <sachin.agrawal@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

Hi Sachin
May I know why you hardcode PSS salt length to be
RSA_PSS_SALTLEN_AUTO ?

Thank you
Yao Jiewen


-----Original Message-----
From: Agrawal, Sachin <sachin.agrawal@intel.com>
Sent: Tuesday, April 20, 2021 10:02 AM
To: devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>;
Jiang, Guomin <guomin.jiang@intel.com>; Agrawal, Sachin
<sachin.agrawal@intel.com>
Subject: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

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

This patch uses Openssl's EVP API's to perform RSASSA-PSS
verification of a binary blob.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Cc: Guomin Jiang <guomin.jiang@intel.com>

Signed-off-by: Sachin Agrawal <sachin.agrawal@intel.com>
---
CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c | 139
++++++++++++++++++++
CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssNull.c | 43 ++++++
CryptoPkg/Include/Library/BaseCryptLib.h | 27 ++++
CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf | 1 +
7 files changed, 213 insertions(+)

diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
new file mode 100644
index 000000000000..acf5eb689cd8
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
@@ -0,0 +1,139 @@
+/** @file
+ RSA Asymmetric Cipher Wrapper Implementation over OpenSSL.
+
+ This file implements following APIs which provide basic
+ capabilities for
RSA:
+ 1) RsaPssVerify
+
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+#include <openssl/bn.h>
+#include <openssl/rsa.h>
+#include <openssl/objects.h>
+#include <openssl/evp.h>
+
+
+/**
+ Retrieve a pointer to EVP message digest object.
+
+ @param[in] DigestLen Length of the message digest.
+
+**/
+static
+EVP_MD*
+GetEvpMD (
+ IN UINT16 DigestLen
+ )
+{
+ switch (DigestLen){
+ case SHA256_DIGEST_SIZE:
+ return EVP_sha256();
+ break;
+ case SHA384_DIGEST_SIZE:
+ return EVP_sha384();
+ break;
+ case SHA512_DIGEST_SIZE:
+ return EVP_sha512();
+ break;
+ default:
+ return NULL;
+ }
+}
+
+
+/**
+ Verifies the RSA signature with RSASSA-PSS signature scheme
+defined in RFC
8017.
+ Implementation determines salt length automatically from the
+ signature
encoding.
+ Mask generation function is the same as the message digest algorithm.
+
+ @param[in] RsaContext Pointer to RSA context for signature
verification.
+ @param[in] Message Pointer to octet message to be verified.
+ @param[in] MsgSize Size of the message in bytes.
+ @param[in] Signature Pointer to RSASSA-PSS signature to be verified.
+ @param[in] SigSize Size of signature in bytes.
+ @param[in] DigestLen Length of digest for RSA operation.
+
+ @retval TRUE Valid signature encoded in RSASSA-PSS.
+ @retval FALSE Invalid signature or invalid RSA context.
+
+**/
+BOOLEAN
+EFIAPI
+RsaPssVerify (
+ IN VOID *RsaContext,
+ IN CONST UINT8 *Message,
+ IN UINTN MsgSize,
+ IN CONST UINT8 *Signature,
+ IN UINTN SigSize,
+ IN UINT16 DigestLen
+ )
+{
+ BOOLEAN Result;
+ EVP_PKEY *pEvpRsaKey = NULL;
+ EVP_MD_CTX *pEvpVerifyCtx = NULL;
+ EVP_PKEY_CTX *pKeyCtx = NULL;
+ CONST EVP_MD *HashAlg = NULL;
+
+ if (RsaContext == NULL) {
+ return FALSE;
+ }
+ if (Message == NULL || MsgSize == 0 || MsgSize > INT_MAX) {
+ return FALSE;
+ }
+ if (Signature == NULL || SigSize == 0 || SigSize > INT_MAX) {
+ return FALSE;
+ }
+
+ HashAlg = GetEvpMD(DigestLen);
+
+ if (HashAlg == NULL) {
+ return FALSE;
+ }
+
+ pEvpRsaKey = EVP_PKEY_new();
+ if (pEvpRsaKey == NULL) {
+ goto _Exit;
+ }
+
+ EVP_PKEY_set1_RSA(pEvpRsaKey, RsaContext);
+
+ pEvpVerifyCtx = EVP_MD_CTX_create(); if (pEvpVerifyCtx == NULL) {
+ goto _Exit;
+ }
+
+ Result = EVP_DigestVerifyInit(pEvpVerifyCtx, &pKeyCtx, HashAlg,
+ NULL,
pEvpRsaKey) > 0;
+ if (pKeyCtx == NULL) {
+ goto _Exit;
+ }
+
+ if (Result) {
+ Result = EVP_PKEY_CTX_set_rsa_padding(pKeyCtx,
RSA_PKCS1_PSS_PADDING) > 0;
+ }
+ if (Result) {
+ Result = EVP_PKEY_CTX_set_rsa_pss_saltlen(pKeyCtx,
RSA_PSS_SALTLEN_AUTO) > 0;
+ }
+ if (Result) {
+ Result = EVP_PKEY_CTX_set_rsa_mgf1_md(pKeyCtx, HashAlg) > 0;
+ } if (Result) {
+ Result = EVP_DigestVerifyUpdate(pEvpVerifyCtx, Message,
(UINT32)MsgSize) > 0;
+ }
+ if (Result) {
+ Result = EVP_DigestVerifyFinal(pEvpVerifyCtx, Signature,
+ (UINT32)SigSize) > 0; }
+
+_Exit :
+ if (pEvpRsaKey) {
+ EVP_PKEY_free(pEvpRsaKey);
+ }
+ if (pEvpVerifyCtx) {
+ EVP_MD_CTX_destroy(pEvpVerifyCtx);
+ }
+
+ return Result;
+}
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssNull.c
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssNull.c
new file mode 100644
index 000000000000..8d84b4c1426c
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssNull.c
@@ -0,0 +1,43 @@
+/** @file
+ RSA-PSS Asymmetric Cipher Wrapper Implementation over OpenSSL.
+
+ This file does not provide real capabilities for following APIs
+ in RSA
handling:
+ 1) RsaPssVerify
+
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+ Verifies the RSA signature with RSASSA-PSS signature scheme
+defined in RFC
8017.
+ Implementation determines salt length automatically from the
+ signature
encoding.
+ Mask generation function is the same as the message digest algorithm.
+
+ @param[in] RsaContext Pointer to RSA context for signature
verification.
+ @param[in] Message Pointer to octet message to be verified.
+ @param[in] MsgSize Size of the message in bytes.
+ @param[in] Signature Pointer to RSASSA-PSS signature to be verified.
+ @param[in] SigSize Size of signature in bytes.
+ @param[in] DigestLen Length of digest for RSA operation.
+
+ @retval TRUE Valid signature encoded in RSASSA-PSS.
+ @retval FALSE Invalid signature or invalid RSA context.
+
+**/
+BOOLEAN
+EFIAPI
+RsaPssVerify (
+ IN VOID *RsaContext,
+ IN CONST UINT8 *Message,
+ IN UINTN MsgSize,
+ IN CONST UINT8 *Signature,
+ IN UINTN SigSize,
+ IN UINT16 DigestLen
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
b/CryptoPkg/Include/Library/BaseCryptLib.h
index 496121e6a4ed..36d560b8d691 100644
--- a/CryptoPkg/Include/Library/BaseCryptLib.h
+++ b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -1363,6 +1363,33 @@ RsaPkcs1Verify (
IN UINTN SigSize
);

+/**
+ Verifies the RSA signature with RSASSA-PSS signature scheme
+defined in RFC
8017.
+ Implementation determines salt length automatically from the
+ signature
encoding.
+ Mask generation function is the same as the message digest algorithm.
+
+ @param[in] RsaContext Pointer to RSA context for signature
verification.
+ @param[in] Message Pointer to octet message to be verified.
+ @param[in] MsgSize Size of the message in bytes.
+ @param[in] Signature Pointer to RSASSA-PSS signature to be verified.
+ @param[in] SigSize Size of signature in bytes.
+ @param[in] DigestLen Length of digest for RSA operation.
+
+ @retval TRUE Valid signature encoded in RSASSA-PSS.
+ @retval FALSE Invalid signature or invalid RSA context.
+
+**/
+BOOLEAN
+EFIAPI
+RsaPssVerify (
+ IN VOID *RsaContext,
+ IN CONST UINT8 *Message,
+ IN UINTN MsgSize,
+ IN CONST UINT8 *Signature,
+ IN UINTN SigSize,
+ IN UINT16 DigestLen
+ );
+
/**
Retrieve the RSA Private Key from the password-protected PEM key data.


Re: [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes

Lendacky, Thomas
 

On 4/22/21 12:50 AM, Laszlo Ersek via groups.io wrote:
On 04/21/21 00:54, Lendacky, Thomas wrote:
From: Tom Lendacky <thomas.lendacky@amd.com>

BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&;data=04%7C01%7Cthomas.lendacky%40amd.com%7C19a7d97e2a7b461830ed08d905528472%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546674232278910%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=znSezOvpnItW7mHAJkr%2FtJtkQNFc2H0dG9STpmOpVqU%3D&amp;reserved=0

Enabling TPM support results in guest termination of an SEV-ES guest
because it uses MMIO opcodes that are not currently supported.

Add support for the new MMIO opcodes (0xA0 - 0xA3), MOV instructions which
use a memory offset directly encoded in the instruction. Also, add a DEBUG
statement to identify an unsupported MMIO opcode being used.

Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 99 +++++++++++++++++++
1 file changed, 99 insertions(+)

diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 273f36499988..f9660b757d8e 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -678,6 +678,7 @@ MmioExit (
UINTN Bytes;
UINT64 *Register;
UINT8 OpCode, SignByte;
+ UINTN Address;

Bytes = 0;

@@ -727,6 +728,51 @@ MmioExit (
}
break;

+ //
+ // MMIO write (MOV moffsetX, aX)
+ //
+ case 0xA2:
+ Bytes = 1;
+ //
+ // fall through
+ //
+ case 0xA3:
+ Bytes = ((Bytes != 0) ? Bytes :
+ (InstructionData->DataSize == Size16Bits) ? 2 :
+ (InstructionData->DataSize == Size32Bits) ? 4 :
+ (InstructionData->DataSize == Size64Bits) ? 8 :
+ 0);
+
+ InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
+ InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
+
+ if (InstructionData->AddrSize == Size8Bits) {
+ Address = *(UINT8 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size16Bits) {
+ Address = *(UINT16 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size32Bits) {
+ Address = *(UINT32 *) InstructionData->Immediate;
+ } else {
+ Address = *(UINTN *) InstructionData->Immediate;
+ }
(1) Can we simplify this as follows?

InstructionData->ImmediateSize = 1 << InstructionData->AddrSize;
InstructionData->End += InstructionData->ImmediateSize;
Address = 0;
CopyMem (&Address, InstructionData->Immediate,
InstructionData->ImmediateSize);
Yup, that can be done.


+
+ Status = ValidateMmioMemory (Ghcb, Address, Bytes);
+ if (Status != 0) {
+ return Status;
+ }
+
+ ExitInfo1 = Address;
+ ExitInfo2 = Bytes;
+ CopyMem (Ghcb->SharedBuffer, &Regs->Rax, Bytes);
+
+ Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+ VmgSetOffsetValid (Ghcb, GhcbSwScratch);
+ Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
+ if (Status != 0) {
+ return Status;
+ }
+ break;
+
//
// MMIO write (MOV reg/memX, immX)
//
@@ -809,6 +855,58 @@ MmioExit (
CopyMem (Register, Ghcb->SharedBuffer, Bytes);
break;

+ //
+ // MMIO read (MOV aX, moffsetX)
+ //
+ case 0xA0:
+ Bytes = 1;
+ //
+ // fall through
+ //
+ case 0xA1:
+ Bytes = ((Bytes != 0) ? Bytes :
+ (InstructionData->DataSize == Size16Bits) ? 2 :
+ (InstructionData->DataSize == Size32Bits) ? 4 :
+ (InstructionData->DataSize == Size64Bits) ? 8 :
+ 0);
+
+ InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
+ InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
+
+ if (InstructionData->AddrSize == Size8Bits) {
+ Address = *(UINT8 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size16Bits) {
+ Address = *(UINT16 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size32Bits) {
+ Address = *(UINT32 *) InstructionData->Immediate;
+ } else {
+ Address = *(UINTN *) InstructionData->Immediate;
+ }
(2) Similar question as (1).
Will do.


+
+ Status = ValidateMmioMemory (Ghcb, Address, Bytes);
+ if (Status != 0) {
+ return Status;
+ }
+
+ ExitInfo1 = Address;
+ ExitInfo2 = Bytes;
+
+ Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+ VmgSetOffsetValid (Ghcb, GhcbSwScratch);
+ Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
+ if (Status != 0) {
+ return Status;
+ }
+
+ if (Bytes == 4) {
+ //
+ // Zero-extend for 32-bit operation
+ //
+ Regs->Rax = 0;
+ }
(3) This is also seen with opcode 0x8B, but can you remind me please why
we ignore (Bytes == 1) and (Bytes == 2) for zero extension?
That comes from the APM Vol 3, Table B-1, that says, in 64-bit mode, for a
32-bit operand size the 32-bit register results are zero-extended to 64-bits.


+ CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
+ break;
+
//
// MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
//
@@ -886,6 +984,7 @@ MmioExit (
break;

default:
+ DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
Status = GP_EXCEPTION;
ASSERT (FALSE);
}
(4) We should use the DEBUG_ERROR log mask here.
Will change.

Thanks,
Tom


Thanks
Laszlo






Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

Sami Mujawar
 

Hi Jianyong,

 

You need to check if you're subscribed to the EDK II development mailing list. Otherwise, your patch email will get rejected. You can subscribe here: https://edk2.groups.io/g/devel.

Make sure that you reply to the email with subscription confirmation sent from noreply@groups.io. Unless you do it, you won't become a member of the mailing list.

I would also recommend that you wait for a day after confirming, as I believe the edk2.groups.io admin will need to approve your membership.

 

Regards,

 

Sami Mujawar

 

From: Laszlo Ersek <lersek@...>
Date: Thursday, 22 April 2021 at 14:57
To: Jianyong Wu <Jianyong.Wu@...>, edk2-devel-groups-io <devel@edk2.groups.io>
Cc: Justin He <Justin.He@...>, Ard Biesheuvel <ardb+tianocore@...>, Leif Lindholm <leif@...>, Sami Mujawar <Sami.Mujawar@...>
Subject: Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

Hi Jianyong,

On 04/22/21 10:24, Jianyong Wu wrote:
> Cloud Hypervisor is kvm based VMM implemented in rust.
>
> This library populates the system memory map for the
> Cloud Hypervisor virtual platform.
>
> Cc: Laszlo Ersek <lersek@...>
> Cc: Ard Biesheuvel <ardb+tianocore@...>
> Cc: Leif Lindholm <leif@...>
> Signed-off-by: Jianyong Wu <jianyong.wu@...>
> ---
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf          |  47 +++++++++
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c               |  94 ++++++++++++++++++
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c | 100 ++++++++++++++++++++
>  3 files changed, 241 insertions(+)

let's sort out the meta-problems first:

(1) you need a Feature Request BZ for this;
<https://bugzilla.tianocore.org/>. The commit messages should reference
the specific bugzilla ticket URL.

(2) "Clh" is a catastrophically bad abbreviation. The whole point of
your work is to add Cloud Hypervisor support, so why trash the most
relevant information in the file names with an inane abbreviation?

(Not to mention that the name "Cloud Hypervisor" itself is as
nondescript as possible. :/)

(3) I have not received a cover letter (0/4). Not sure if you sent one.

(4) I don't see the messages in my edk2-devel folder, or in the mailing
list archives, or in the messages held for moderation at the groups.io
WebUI.

(5) "Cloud Hypervisor" is not something that I can justifiably spend
much time on. I'm willing to review this series at the level at which
I've reviewed (for example) XenPVH or Bhyve in the past, mainly focusing
on style and potential regressions. However, that's not enough for the
long term: someone from ARM (or elsewhere) will have to step up for
permanent reviewership. Please add a patch for extending
"Maintainers.txt" appropriately. Example subsystems:

- ArmVirtPkg: modules used on Xen
- ArmVirtPkg: Kvmtool emulated platform support
- OvmfPkg: bhyve-related modules
- OvmfPkg: Xen-related modules

Please keep the subsystem titles alphabetically sorted in the file.

Please resend.

(I'm posting these comments at once because they are understandable to
the community even in the absence of your patches on the list.)

Thanks
Laszlo

>
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
> new file mode 100644
> index 000000000000..04cb1f2a581a
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
> @@ -0,0 +1,47 @@
> +#/* @file
> +#
> +#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#*/
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = ClhVirtMemInfoPeiLib
> +  FILE_GUID                      = 3E29D940-0591-EE6A-CAD4-223A9CF55E75
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ArmVirtMemInfoLib|PEIM
> +  CONSTRUCTOR                    = ClhVirtMemInfoPeiLibConstructor
> +
> +[Sources]
> +  ClhVirtMemInfoLib.c
> +  ClhVirtMemInfoPeiLibConstructor.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmVirtPkg/ArmVirtPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  ArmLib
> +  BaseMemoryLib
> +  DebugLib
> +  FdtLib
> +  PcdLib
> +  MemoryAllocationLib
> +
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdFdBaseAddress
> +  gArmTokenSpaceGuid.PcdFvBaseAddress
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdFdSize
> +  gArmTokenSpaceGuid.PcdFvSize
> +  gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
> new file mode 100644
> index 000000000000..829d7d7aa259
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
> @@ -0,0 +1,94 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Library/ArmLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +
> +// Number of Virtual Memory Map Descriptors
> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          5
> +
> +//
> +// mach-virt's core peripherals such as the UART, the GIC and the RTC are
> +// all mapped in the 'miscellaneous device I/O' region, which we just map
> +// in its entirety rather than device by device. Note that it does not
> +// cover any of the NOR flash banks or PCI resource windows.
> +//
> +#define MACH_VIRT_PERIPH_BASE       0x08000000
> +#define MACH_VIRT_PERIPH_SIZE       SIZE_128MB
> +
> +//
> +// in cloud-hypervisor, 0x0 ~ 0x8000000 is reserved as normal memory for UEFI
> +//
> +#define CLH_UEFI_MEM_BASE       0x0
> +#define CLH_UEFI_MEM_SIZE       0x08000000
> +
> +/**
> +  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. The allocated memory
> +                                    will not be freed.
> +
> +**/
> +VOID
> +ArmVirtGetMemoryMap (
> +  OUT ARM_MEMORY_REGION_DESCRIPTOR   **VirtualMemoryMap
> +  )
> +{
> +  ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
> +
> +  ASSERT (VirtualMemoryMap != NULL);
> +
> +  VirtualMemoryTable = AllocatePool (sizeof (ARM_MEMORY_REGION_DESCRIPTOR) *
> +                                     MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
> +
> +  if (VirtualMemoryTable == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: Error: Failed AllocatePool()\n", __FUNCTION__));
> +    return;
> +  }
> +
> +  // System DRAM
> +  VirtualMemoryTable[0].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
> +  VirtualMemoryTable[0].VirtualBase  = VirtualMemoryTable[0].PhysicalBase;
> +  VirtualMemoryTable[0].Length       = PcdGet64 (PcdSystemMemorySize);
> +  VirtualMemoryTable[0].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +  DEBUG ((DEBUG_INFO, "%a: Dumping System DRAM Memory Map:\n"
> +      "\tPhysicalBase: 0x%lX\n"
> +      "\tVirtualBase: 0x%lX\n"
> +      "\tLength: 0x%lX\n",
> +      __FUNCTION__,
> +      VirtualMemoryTable[0].PhysicalBase,
> +      VirtualMemoryTable[0].VirtualBase,
> +      VirtualMemoryTable[0].Length));
> +
> +  // Memory mapped peripherals (UART, RTC, GIC, virtio-mmio, etc)
> +  VirtualMemoryTable[1].PhysicalBase = MACH_VIRT_PERIPH_BASE;
> +  VirtualMemoryTable[1].VirtualBase  = MACH_VIRT_PERIPH_BASE;
> +  VirtualMemoryTable[1].Length       = MACH_VIRT_PERIPH_SIZE;
> +  VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +
> +  // Map the FV region as normal executable memory
> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFvSize);
> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +  // End of Table
> +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
> +
> +  *VirtualMemoryMap = VirtualMemoryTable;
> +}
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
> new file mode 100644
> index 000000000000..5f89b70df990
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
> @@ -0,0 +1,100 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <libfdt.h>
> +
> +RETURN_STATUS
> +EFIAPI
> +ClhVirtMemInfoPeiLibConstructor (
> +  VOID
> +  )
> +{
> +  VOID          *DeviceTreeBase;
> +  INT32         Node, Prev;
> +  UINT64        NewBase, CurBase;
> +  UINT64        NewSize, CurSize;
> +  CONST CHAR8   *Type;
> +  INT32         Len;
> +  CONST UINT64  *RegProp;
> +  RETURN_STATUS PcdStatus;
> +
> +  NewBase = 0;
> +  NewSize = 0;
> +
> +  DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
> +  ASSERT (DeviceTreeBase != NULL);
> +
> +  //
> +  // Make sure we have a valid device tree blob
> +  //
> +  ASSERT (fdt_check_header (DeviceTreeBase) == 0);
> +
> +  //
> +  // Look for the lowest memory node
> +  //
> +  for (Prev = 0;; Prev = Node) {
> +    Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
> +    if (Node < 0) {
> +      break;
> +    }
> +
> +    //
> +    // Check for memory node
> +    //
> +    Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
> +    if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
> +      //
> +      // Get the 'reg' property of this node. For now, we will assume
> +      // two 8 byte quantities for base and size, respectively.
> +      //
> +      RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
> +      if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
> +
> +        CurBase = fdt64_to_cpu (ReadUnaligned64 (RegProp));
> +        CurSize = fdt64_to_cpu (ReadUnaligned64 (RegProp + 1));
> +
> +        DEBUG ((DEBUG_INFO, "%a: System RAM @ 0x%lx - 0x%lx\n",
> +          __FUNCTION__, CurBase, CurBase + CurSize - 1));
> +
> +        if (NewBase > CurBase || NewBase == 0) {
> +          NewBase = CurBase;
> +          NewSize = CurSize;
> +        }
> +      } else {
> +        DEBUG ((DEBUG_ERROR, "%a: Failed to parse FDT memory node\n",
> +          __FUNCTION__));
> +      }
> +    }
> +  }
> +
> +  //
> +  // Make sure the start of DRAM matches our expectation
> +  //
> +  ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
> +  PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +
> +  //
> +  // We need to make sure that the machine we are running on has at least
> +  // 128 MB of memory configured, and is currently executing this binary from
> +  // NOR flash. This prevents a device tree image in DRAM from getting
> +  // clobbered when our caller installs permanent PEI RAM, before we have a
> +  // chance of marking its location as reserved or copy it to a freshly
> +  // allocated block in the permanent PEI RAM in the platform PEIM.
> +  //
> +  ASSERT (NewSize >= SIZE_128MB);
> +  ASSERT (
> +    (((UINT64)PcdGet64 (PcdFdBaseAddress) +
> +      (UINT64)PcdGet32 (PcdFdSize)) <= NewBase) ||
> +    ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize)));
> +
> +  return RETURN_SUCCESS;
> +}
>


Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

Laszlo Ersek
 

Hi Jianyong,

On 04/22/21 10:24, Jianyong Wu wrote:
Cloud Hypervisor is kvm based VMM implemented in rust.

This library populates the system memory map for the
Cloud Hypervisor virtual platform.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Leif Lindholm <leif@nuviainc.com>
Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf | 47 +++++++++
ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c | 94 ++++++++++++++++++
ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c | 100 ++++++++++++++++++++
3 files changed, 241 insertions(+)
let's sort out the meta-problems first:

(1) you need a Feature Request BZ for this;
<https://bugzilla.tianocore.org/>. The commit messages should reference
the specific bugzilla ticket URL.

(2) "Clh" is a catastrophically bad abbreviation. The whole point of
your work is to add Cloud Hypervisor support, so why trash the most
relevant information in the file names with an inane abbreviation?

(Not to mention that the name "Cloud Hypervisor" itself is as
nondescript as possible. :/)

(3) I have not received a cover letter (0/4). Not sure if you sent one.

(4) I don't see the messages in my edk2-devel folder, or in the mailing
list archives, or in the messages held for moderation at the groups.io
WebUI.

(5) "Cloud Hypervisor" is not something that I can justifiably spend
much time on. I'm willing to review this series at the level at which
I've reviewed (for example) XenPVH or Bhyve in the past, mainly focusing
on style and potential regressions. However, that's not enough for the
long term: someone from ARM (or elsewhere) will have to step up for
permanent reviewership. Please add a patch for extending
"Maintainers.txt" appropriately. Example subsystems:

- ArmVirtPkg: modules used on Xen
- ArmVirtPkg: Kvmtool emulated platform support
- OvmfPkg: bhyve-related modules
- OvmfPkg: Xen-related modules

Please keep the subsystem titles alphabetically sorted in the file.

Please resend.

(I'm posting these comments at once because they are understandable to
the community even in the absence of your patches on the list.)

Thanks
Laszlo


diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
new file mode 100644
index 000000000000..04cb1f2a581a
--- /dev/null
+++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
@@ -0,0 +1,47 @@
+#/* @file
+#
+# Copyright (c) 2011-2015, ARM Limited. All rights reserved.
+# Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#*/
+
+[Defines]
+ INF_VERSION = 0x0001001A
+ BASE_NAME = ClhVirtMemInfoPeiLib
+ FILE_GUID = 3E29D940-0591-EE6A-CAD4-223A9CF55E75
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = ArmVirtMemInfoLib|PEIM
+ CONSTRUCTOR = ClhVirtMemInfoPeiLibConstructor
+
+[Sources]
+ ClhVirtMemInfoLib.c
+ ClhVirtMemInfoPeiLibConstructor.c
+
+[Packages]
+ ArmPkg/ArmPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec
+ EmbeddedPkg/EmbeddedPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+
+[LibraryClasses]
+ ArmLib
+ BaseMemoryLib
+ DebugLib
+ FdtLib
+ PcdLib
+ MemoryAllocationLib
+
+[Pcd]
+ gArmTokenSpaceGuid.PcdFdBaseAddress
+ gArmTokenSpaceGuid.PcdFvBaseAddress
+ gArmTokenSpaceGuid.PcdSystemMemoryBase
+ gArmTokenSpaceGuid.PcdSystemMemorySize
+
+[FixedPcd]
+ gArmTokenSpaceGuid.PcdFdSize
+ gArmTokenSpaceGuid.PcdFvSize
+ gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
new file mode 100644
index 000000000000..829d7d7aa259
--- /dev/null
+++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
@@ -0,0 +1,94 @@
+/** @file
+
+ Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Base.h>
+#include <Library/ArmLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+
+// Number of Virtual Memory Map Descriptors
+#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 5
+
+//
+// mach-virt's core peripherals such as the UART, the GIC and the RTC are
+// all mapped in the 'miscellaneous device I/O' region, which we just map
+// in its entirety rather than device by device. Note that it does not
+// cover any of the NOR flash banks or PCI resource windows.
+//
+#define MACH_VIRT_PERIPH_BASE 0x08000000
+#define MACH_VIRT_PERIPH_SIZE SIZE_128MB
+
+//
+// in cloud-hypervisor, 0x0 ~ 0x8000000 is reserved as normal memory for UEFI
+//
+#define CLH_UEFI_MEM_BASE 0x0
+#define CLH_UEFI_MEM_SIZE 0x08000000
+
+/**
+ 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. The allocated memory
+ will not be freed.
+
+**/
+VOID
+ArmVirtGetMemoryMap (
+ OUT ARM_MEMORY_REGION_DESCRIPTOR **VirtualMemoryMap
+ )
+{
+ ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable;
+
+ ASSERT (VirtualMemoryMap != NULL);
+
+ VirtualMemoryTable = AllocatePool (sizeof (ARM_MEMORY_REGION_DESCRIPTOR) *
+ MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
+
+ if (VirtualMemoryTable == NULL) {
+ DEBUG ((DEBUG_ERROR, "%a: Error: Failed AllocatePool()\n", __FUNCTION__));
+ return;
+ }
+
+ // System DRAM
+ VirtualMemoryTable[0].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
+ VirtualMemoryTable[0].VirtualBase = VirtualMemoryTable[0].PhysicalBase;
+ VirtualMemoryTable[0].Length = PcdGet64 (PcdSystemMemorySize);
+ VirtualMemoryTable[0].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+
+ DEBUG ((DEBUG_INFO, "%a: Dumping System DRAM Memory Map:\n"
+ "\tPhysicalBase: 0x%lX\n"
+ "\tVirtualBase: 0x%lX\n"
+ "\tLength: 0x%lX\n",
+ __FUNCTION__,
+ VirtualMemoryTable[0].PhysicalBase,
+ VirtualMemoryTable[0].VirtualBase,
+ VirtualMemoryTable[0].Length));
+
+ // Memory mapped peripherals (UART, RTC, GIC, virtio-mmio, etc)
+ VirtualMemoryTable[1].PhysicalBase = MACH_VIRT_PERIPH_BASE;
+ VirtualMemoryTable[1].VirtualBase = MACH_VIRT_PERIPH_BASE;
+ VirtualMemoryTable[1].Length = MACH_VIRT_PERIPH_SIZE;
+ VirtualMemoryTable[1].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+ // Map the FV region as normal executable memory
+ VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
+ VirtualMemoryTable[2].VirtualBase = VirtualMemoryTable[2].PhysicalBase;
+ VirtualMemoryTable[2].Length = FixedPcdGet32 (PcdFvSize);
+ VirtualMemoryTable[2].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+
+ // End of Table
+ ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
+
+ *VirtualMemoryMap = VirtualMemoryTable;
+}
diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
new file mode 100644
index 000000000000..5f89b70df990
--- /dev/null
+++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
@@ -0,0 +1,100 @@
+/** @file
+
+ Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Base.h>
+#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+#include <libfdt.h>
+
+RETURN_STATUS
+EFIAPI
+ClhVirtMemInfoPeiLibConstructor (
+ VOID
+ )
+{
+ VOID *DeviceTreeBase;
+ INT32 Node, Prev;
+ UINT64 NewBase, CurBase;
+ UINT64 NewSize, CurSize;
+ CONST CHAR8 *Type;
+ INT32 Len;
+ CONST UINT64 *RegProp;
+ RETURN_STATUS PcdStatus;
+
+ NewBase = 0;
+ NewSize = 0;
+
+ DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
+ ASSERT (DeviceTreeBase != NULL);
+
+ //
+ // Make sure we have a valid device tree blob
+ //
+ ASSERT (fdt_check_header (DeviceTreeBase) == 0);
+
+ //
+ // Look for the lowest memory node
+ //
+ for (Prev = 0;; Prev = Node) {
+ Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
+ if (Node < 0) {
+ break;
+ }
+
+ //
+ // Check for memory node
+ //
+ Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
+ if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
+ //
+ // Get the 'reg' property of this node. For now, we will assume
+ // two 8 byte quantities for base and size, respectively.
+ //
+ RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
+ if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
+
+ CurBase = fdt64_to_cpu (ReadUnaligned64 (RegProp));
+ CurSize = fdt64_to_cpu (ReadUnaligned64 (RegProp + 1));
+
+ DEBUG ((DEBUG_INFO, "%a: System RAM @ 0x%lx - 0x%lx\n",
+ __FUNCTION__, CurBase, CurBase + CurSize - 1));
+
+ if (NewBase > CurBase || NewBase == 0) {
+ NewBase = CurBase;
+ NewSize = CurSize;
+ }
+ } else {
+ DEBUG ((DEBUG_ERROR, "%a: Failed to parse FDT memory node\n",
+ __FUNCTION__));
+ }
+ }
+ }
+
+ //
+ // Make sure the start of DRAM matches our expectation
+ //
+ ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
+ PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);
+ ASSERT_RETURN_ERROR (PcdStatus);
+
+ //
+ // We need to make sure that the machine we are running on has at least
+ // 128 MB of memory configured, and is currently executing this binary from
+ // NOR flash. This prevents a device tree image in DRAM from getting
+ // clobbered when our caller installs permanent PEI RAM, before we have a
+ // chance of marking its location as reserved or copy it to a freshly
+ // allocated block in the permanent PEI RAM in the platform PEIM.
+ //
+ ASSERT (NewSize >= SIZE_128MB);
+ ASSERT (
+ (((UINT64)PcdGet64 (PcdFdBaseAddress) +
+ (UINT64)PcdGet32 (PcdFdSize)) <= NewBase) ||
+ ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize)));
+
+ return RETURN_SUCCESS;
+}


Re: [PATCH 1/3] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes

Lendacky, Thomas
 

On 4/22/21 12:28 AM, Laszlo Ersek wrote:
On 04/21/21 00:54, Lendacky, Thomas wrote:
From: Tom Lendacky <thomas.lendacky@amd.com>

BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&;data=04%7C01%7Cthomas.lendacky%40amd.com%7C22bf3a3ae9cb4421e93208d9054f79c8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546661229697941%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=1EmUDf%2FfuCuu%2BkXPZijzatfliplMhKEQH8kiZ9Z8ZF0%3D&amp;reserved=0

The MOVZX and MOVSX instructions use the ModRM byte in the instruction,
but the instruction decoding support was not decoding it. This resulted
in invalid decoding and failing of the MMIO operation. Also, when
performing the zero-extend or sign-extend operation, the memory operation
should be using the size, and not the size enumeration value.

Add the ModRM byte decoding for the MOVZX and MOVSX opcodes and use the
true data size to perform the extend operations. Additionally, add a
DEBUG statement identifying the MMIO address being flagged as encrypted
during the MMIO address validation.

Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 24259060fd65..273f36499988 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -643,6 +643,7 @@ ValidateMmioMemory (
//
// Any state other than unencrypted is an error, issue a #GP.
//
+ DEBUG ((DEBUG_INFO, "MMIO using encrypted memory: %lx\n", MemoryAddress));
GpEvent.Uint64 = 0;
GpEvent.Elements.Vector = GP_EXCEPTION;
GpEvent.Elements.Type = GHCB_EVENT_INJECTION_TYPE_EXCEPTION;
(1) This can potentially generate a large number of debug messages;
please use the DEBUG_VERBOSE log mask.
Actually, you will see this only once since the code will propagate a GP
and the guest will terminate in this situation.


(2) "MemoryAddress" has type UINTN, but %lx takes UINT64. Given that
this is X64-only code, functionally there is no bug, but it's still
cleaner to pass "(UINT64)MemoryAddress" to %lx.
Will do.

Thanks,
Tom


With that:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo


@@ -817,6 +818,7 @@ MmioExit (
// fall through
//
case 0xB7:
+ DecodeModRm (Regs, InstructionData);
Bytes = (Bytes != 0) ? Bytes : 2;

Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes);
@@ -835,7 +837,7 @@ MmioExit (
}

Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
- SetMem (Register, InstructionData->DataSize, 0);
+ SetMem (Register, (UINTN) (1 << InstructionData->DataSize), 0);
CopyMem (Register, Ghcb->SharedBuffer, Bytes);
break;

@@ -848,6 +850,7 @@ MmioExit (
// fall through
//
case 0xBF:
+ DecodeModRm (Regs, InstructionData);
Bytes = (Bytes != 0) ? Bytes : 2;

Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes);
@@ -878,7 +881,7 @@ MmioExit (
}

Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
- SetMem (Register, InstructionData->DataSize, SignByte);
+ SetMem (Register, (UINTN) (1 << InstructionData->DataSize), SignByte);
CopyMem (Register, Ghcb->SharedBuffer, Bytes);
break;


Re: [PATCH v1 12/12] AzurePipelines: Add support for ArmPlatformPkg

Sami Mujawar
 

Hi Pierre,

 

 Thank you for this patch.

 

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

 

Regards,

 

Sami Mujawar

 

 

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 12/12] AzurePipelines: Add support for ArmPlatformPkg

From: Pierre Gondois <Pierre.Gondois@...>

Add an entry to build the ArmPlatformPkg in the CI.

Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=3349
Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 .azurepipelines/templates/pr-gate-build-job.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.azurepipelines/templates/pr-gate-build-job.yml b/.azurepipelines/templates/pr-gate-build-job.yml
index 837079e7bd97..16d197cde913 100644
--- a/.azurepipelines/templates/pr-gate-build-job.yml
+++ b/.azurepipelines/templates/pr-gate-build-job.yml
@@ -22,7 +22,7 @@ jobs:
   strategy:
     matrix:
       TARGET_ARM:
-        Build.Pkgs: 'ArmPkg'
+        Build.Pkgs: 'ArmPkg,ArmPlatformPkg'
         Build.Targets: 'DEBUG,RELEASE,NO-TARGET,NOOPT'
       TARGET_MDE_CPU:
         Build.Pkgs: 'MdePkg,UefiCpuPkg'
--
2.17.1


Re: [PATCH v1 11/12] AzurePipelines: Add support for ArmPkg

Sami Mujawar
 

Hi Pierre,

 

Thank you for this patch.

 

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

 

Regards,

 

Sami Mujawar

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 11/12] AzurePipelines: Add support for ArmPkg

From: Pierre Gondois <Pierre.Gondois@...>

Add an entry to build the ArmPkg in the CI.

Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=3348
Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 .azurepipelines/templates/pr-gate-build-job.yml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/.azurepipelines/templates/pr-gate-build-job.yml b/.azurepipelines/templates/pr-gate-build-job.yml
index 3e6d275b1b9a..837079e7bd97 100644
--- a/.azurepipelines/templates/pr-gate-build-job.yml
+++ b/.azurepipelines/templates/pr-gate-build-job.yml
@@ -21,6 +21,9 @@ jobs:
   #Use matrix to speed up the build process
   strategy:
     matrix:
+      TARGET_ARM:
+        Build.Pkgs: 'ArmPkg'
+        Build.Targets: 'DEBUG,RELEASE,NO-TARGET,NOOPT'
       TARGET_MDE_CPU:
         Build.Pkgs: 'MdePkg,UefiCpuPkg'
         Build.Targets: 'DEBUG,RELEASE,NO-TARGET,NOOPT'
--
2.17.1


Re: [PATCH v1 10/12] .pytool: Document LicenseCheck and EccCheck

Sami Mujawar
 

Hi Pierre,

 

Thank you for this patch.

 

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

 

Regards,

 

Sami Mujawar

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 10/12] .pytool: Document LicenseCheck and EccCheck

From: Pierre Gondois <Pierre.Gondois@...>

Add an entry in the documentation for the LicenseCheck and
EccCheck plugins.

Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 .pytool/Readme.md | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/.pytool/Readme.md b/.pytool/Readme.md
index eca86c6a822d..f6505507966a 100644
--- a/.pytool/Readme.md
+++ b/.pytool/Readme.md
@@ -254,6 +254,16 @@ Install
 
   More cspell info: https://github.com/streetsidesoftware/cspell
 
+### License Checking - LicenseCheck
+
+Scans all new added files in a package to make sure code is contributed under
+BSD-2-Clause-Patent.
+
+### Ecc tool - EccCheck
+
+Run the Ecc tool on the package. The Ecc tool is available in the BaseTools
+package. It checks that the code complies to the EDKII coding standard.
+
 ## PyTool Scopes
 
 Scopes are how the PyTool ext_dep, path_env, and plugins are activated.  Meaning
--
2.17.1


Re: [PATCH v1 09/12] .pytool: Enable CI for ArmPlatformPkg

Sami Mujawar
 

Hi Pierre,

 

This patch looks good to me.

 

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

 

Regards,

 

Sami Mujawar

 

 

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 09/12] .pytool: Enable CI for ArmPlatformPkg

From: Pierre Gondois <Pierre.Gondois@...>

Enable the CI for the ArmPlatformPkg.

Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 .pytool/CISettings.py | 1 +
 .pytool/Readme.md     | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/.pytool/CISettings.py b/.pytool/CISettings.py
index 6f7daeca076b..96e6baa5190d 100644
--- a/.pytool/CISettings.py
+++ b/.pytool/CISettings.py
@@ -50,6 +50,7 @@ class Settings(CiBuildSettingsManager, UpdateSettingsManager, SetupSettingsManag
         These should be edk2 workspace relative paths '''
 
         return ("ArmPkg",
+                "ArmPlatformPkg",
                 "ArmVirtPkg",
                 "DynamicTablesPkg",
                 "EmulatorPkg",
diff --git a/.pytool/Readme.md b/.pytool/Readme.md
index cbce1f6cd54a..eca86c6a822d 100644
--- a/.pytool/Readme.md
+++ b/.pytool/Readme.md
@@ -5,7 +5,7 @@
 | Package              | Windows VS2019 (IA32/X64)| Ubuntu GCC (IA32/X64/ARM/AARCH64) | Known Issues |
 | :----                | :-----                   | :----                             | :---         |
 | ArmPkg               |                    | :heavy_check_mark: |
-| ArmPlatformPkg       |
+| ArmPlatformPkg       |                    | :heavy_check_mark: |
 | ArmVirtPkg           | SEE PACKAGE README | SEE PACKAGE README |
 | CryptoPkg            | :heavy_check_mark: | :heavy_check_mark: | Spell checking in audit mode
 | DynamicTablesPkg     |                    | :heavy_check_mark: |
--
2.17.1


Re: [PATCH v1 08/12] .pytool: Enable CI for ArmPkg

Sami Mujawar
 

Hi Pierre,

 

This patch looks good to me.

 

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

 

Regards,

 

Sami Mujawar

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 08/12] .pytool: Enable CI for ArmPkg

From: Pierre Gondois <Pierre.Gondois@...>

Enable the CI for the ArmPkg.

Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 .pytool/CISettings.py | 3 ++-
 .pytool/Readme.md     | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/.pytool/CISettings.py b/.pytool/CISettings.py
index 5f71eca1992e..6f7daeca076b 100644
--- a/.pytool/CISettings.py
+++ b/.pytool/CISettings.py
@@ -49,7 +49,8 @@ class Settings(CiBuildSettingsManager, UpdateSettingsManager, SetupSettingsManag
         ''' return iterable of edk2 packages supported by this build.
         These should be edk2 workspace relative paths '''
 
-        return ("ArmVirtPkg",
+        return ("ArmPkg",
+                "ArmVirtPkg",
                 "DynamicTablesPkg",
                 "EmulatorPkg",
                 "MdePkg",
diff --git a/.pytool/Readme.md b/.pytool/Readme.md
index e158b2b81a34..cbce1f6cd54a 100644
--- a/.pytool/Readme.md
+++ b/.pytool/Readme.md
@@ -4,7 +4,7 @@
 
 | Package              | Windows VS2019 (IA32/X64)| Ubuntu GCC (IA32/X64/ARM/AARCH64) | Known Issues |
 | :----                | :-----                   | :----                             | :---         |
-| ArmPkg               |
+| ArmPkg               |                    | :heavy_check_mark: |
 | ArmPlatformPkg       |
 | ArmVirtPkg           | SEE PACKAGE README | SEE PACKAGE README |
 | CryptoPkg            | :heavy_check_mark: | :heavy_check_mark: | Spell checking in audit mode
--
2.17.1


Re: [PATCH v1 07/12] ArmPlatformPkg: Add ArmPlatformPkg.ci.yaml

Sami Mujawar
 

Hi Pierre,

 

I have a minor comment marked inline as [SAMI].

With that changed.

 

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

 

Regards,

 

Sami Mujawar

 

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 07/12] ArmPlatformPkg: Add ArmPlatformPkg.ci.yaml

From: Pierre Gondois <Pierre.Gondois@...>

Add ArmPlatformPkg.ci.yaml to configure the CI for the
ArmPlatformPkg.

Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 ArmPlatformPkg/ArmPlatformPkg.ci.yaml | 100 ++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 ArmPlatformPkg/ArmPlatformPkg.ci.yaml

diff --git a/ArmPlatformPkg/ArmPlatformPkg.ci.yaml b/ArmPlatformPkg/ArmPlatformPkg.ci.yaml
new file mode 100644
index 000000000000..1abaa2f6870c
--- /dev/null
+++ b/ArmPlatformPkg/ArmPlatformPkg.ci.yaml
@@ -0,0 +1,100 @@
+## @file
+# CI configuration for ArmPlatformPkg
+#
+# Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+{
+    ## options defined .pytool/Plugin/LicenseCheck
+    "LicenseCheck": {
+        "IgnoreFiles": []
+    },
+
+    "EccCheck": {
+        ## Exception sample looks like below:
+        ## "ExceptionList": [
+        ##     "<ErrorID>", "<KeyWord>"
+        ## ]
+        "ExceptionList": [
+        ],
+        ## Both file path and directory path are accepted.
+        "IgnoreFiles": [
+            "Scripts/Ds5/"
+        ]
+    },
+
+    ## options defined .pytool/Plugin/CompilerPlugin
+    "CompilerPlugin": {
+        "DscPath": "ArmPlatformPkg.dsc"
+    },
+
+    ## options defined .pytool/Plugin/HostUnitTestCompilerPlugin
+    "HostUnitTestCompilerPlugin": {
+        "DscPath": "" # Don't support this test
+    },
+
+    ## options defined .pytool/Plugin/CharEncodingCheck
+    "CharEncodingCheck": {
+        "IgnoreFiles": []
+    },
+
+    ## options defined .pytool/Plugin/DependencyCheck
+    "DependencyCheck": {
+        "AcceptableDependencies": [
+            "ArmPlatformPkg/ArmPlatformPkg.dec",
+            "ArmPkg/ArmPkg.dec",

[SAMI] Can this list be sorted in alphabetical order, please?

[/SAMI]
+            "EmbeddedPkg/EmbeddedPkg.dec",
+            "MdeModulePkg/MdeModulePkg.dec",
+            "MdePkg/MdePkg.dec"
+        ],
+        # For host based unit tests
+        "AcceptableDependencies-HOST_APPLICATION":[
+            "UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec"
+        ],
+        # For UEFI shell based apps
+        "AcceptableDependencies-UEFI_APPLICATION":[],
+        "IgnoreInf": []
+    },
+
+    ## options defined .pytool/Plugin/DscCompleteCheck
+    "DscCompleteCheck": {
+        "IgnoreInf": [],
+        "DscPath": "ArmPlatformPkg.dsc"
+    },
+
+    ## options defined .pytool/Plugin/HostUnitTestDscCompleteCheck
+    "HostUnitTestDscCompleteCheck": {
+        "IgnoreInf": [""],
+        "DscPath": "" # Don't support this test
+    },
+
+    ## options defined .pytool/Plugin/GuidCheck
+    "GuidCheck": {
+        "IgnoreGuidName": [],
+        "IgnoreGuidValue": [],
+        "IgnoreFoldersAndFiles": [],
+        "IgnoreDuplicates": [],
+    },
+
+    ## options defined .pytool/Plugin/LibraryClassCheck
+    "LibraryClassCheck": {
+        "IgnoreHeaderFile": []
+    },
+
+    ## options defined .pytool/Plugin/SpellCheck
+    "SpellCheck": {
+        "AuditOnly": False,
+        "IgnoreFiles": [],           # use gitignore syntax to ignore errors
+                                     # in matching files
+        "ExtendWords": [
+            "hdlcd",
+            "icdsgir",
+            "primecells"
+           ],           # words to extend to the dictionary for this package
+        "IgnoreStandardPaths": [    # Standard Plugin defined paths that
+            "*.asm", "*.s"          # should be ignore
+        ],
+        "AdditionalIncludePaths": [] # Additional paths to spell check
+                                     # (wildcards supported)
+    }
+}
--
2.17.1


Re: [PATCH v1 06/12] ArmPkg: Add ArmPkg.ci.yaml

Sami Mujawar
 

Hi Pierre,

 

I have a few minor comments marked inline as [SAMI].

With those changed.

 

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

 

Regards,

 

Sami Mujawar

 

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 06/12] ArmPkg: Add ArmPkg.ci.yaml

From: Pierre Gondois <Pierre.Gondois@...>

Add ArmPkg.ci.yaml to configure the CI for the
ArmPkg.

Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 ArmPkg/ArmPkg.ci.yaml | 222 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 222 insertions(+)
 create mode 100644 ArmPkg/ArmPkg.ci.yaml

diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
new file mode 100644
index 000000000000..ba502cd647c9
--- /dev/null
+++ b/ArmPkg/ArmPkg.ci.yaml
@@ -0,0 +1,222 @@
+## @file
+# CI configuration for ArmPkg
+#
+# Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+{
+    ## options defined .pytool/Plugin/LicenseCheck
+    "LicenseCheck": {
+        "IgnoreFiles": []
+    },
+
+    "EccCheck": {
+        ## Exception sample looks like below:
+        ## "ExceptionList": [
+        ##     "<ErrorID>", "<KeyWord>"
+        ## ]
+        "ExceptionList": [
+        ],
+        ## Both file path and directory path are accepted.
+        "IgnoreFiles": [
+            "Library/ArmSoftFloatLib/berkeley-softfloat-3"
+        ]
+    },
+
+    ## options defined .pytool/Plugin/CompilerPlugin
+    "CompilerPlugin": {
+        "DscPath": "ArmPkg.dsc"
+    },
+
+    ## options defined .pytool/Plugin/HostUnitTestCompilerPlugin
+    "HostUnitTestCompilerPlugin": {
+        "DscPath": "" # Don't support this test
+    },
+
+    ## options defined .pytool/Plugin/CharEncodingCheck
+    "CharEncodingCheck": {
+        "IgnoreFiles": []
+    },
+
+    ## options defined .pytool/Plugin/DependencyCheck
+    "DependencyCheck": {
+        "AcceptableDependencies": [
+            "ArmPlatformPkg/ArmPlatformPkg.dec",
+            "ArmPkg/ArmPkg.dec",
+            "EmbeddedPkg/EmbeddedPkg.dec",
+            "MdeModulePkg/MdeModulePkg.dec",
+            "MdePkg/MdePkg.dec",
+            "ShellPkg/ShellPkg.dec"

[SAMI] Can this list be sorted in alphabetical order, please?

[/SAMI]
+        ],
+        # For host based unit tests
+        "AcceptableDependencies-HOST_APPLICATION":[
+            "UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec"
+        ],
+        # For UEFI shell based apps
+        "AcceptableDependencies-UEFI_APPLICATION":[],
+        "IgnoreInf": []
+    },
+
+    ## options defined .pytool/Plugin/DscCompleteCheck
+    "DscCompleteCheck": {
+        "IgnoreInf": [],
+        "DscPath": "ArmPkg.dsc"
+    },
+
+    ## options defined .pytool/Plugin/HostUnitTestDscCompleteCheck
+    "HostUnitTestDscCompleteCheck": {
+        "IgnoreInf": [""],
+        "DscPath": "" # Don't support this test
+    },
+
+    ## options defined .pytool/Plugin/GuidCheck
+    "GuidCheck": {
+        "IgnoreGuidName": [],
+        "IgnoreGuidValue": [],
+        "IgnoreFoldersAndFiles": [],
+        "IgnoreDuplicates": [],
+    },
+
+    ## options defined .pytool/Plugin/LibraryClassCheck
+    "LibraryClassCheck": {
+        "IgnoreHeaderFile": []
+    },
+
+    ## options defined .pytool/Plugin/SpellCheck
+    "SpellCheck": {
+        "AuditOnly": False,
+        "IgnoreFiles": [
+            "Library/ArmSoftFloatLib/berkeley-softfloat-3/**"
+        ],                           # use gitignore syntax to ignore errors
+                                     # in matching files
+        "ExtendWords": [
+          "api's",
+          "ackintid",

[SAMI] Can this list be sorted in alphabetical order, please?

[/SAMI]
+          "actlr",
+          "aeabi",
+          "ashldi",
+          "ashrdi",
+          "ccidx",
+          "ccsidr",
+          "clidr",
+          "clrex",
+          "clzsi",
+          "cpuactlr",
+          "csselr",
+          "ctzsi",
+          "cygdrive",
+          "cygpaths",
+          "datas",
+          "dcmpeq",
+          "dcmpge",
+          "dcmpgt",
+          "dcmple",
+          "dcmplt",
+          "ddisable",
+          "divdi",
+          "divsi",
+          "dmdepkg",
+          "drsub",
+          "eoi'ed",

[SAMI] I don’t think there is such a word. Should the original text be fixed?

[/SAMI]
+          "fcmpeq",
+          "fcmpge",
+          "fcmpgt",
+          "fcmple",
+          "fcmplt",
+          "ffreestanding",
+          "frsub",
+          "hisilicon",
+          "iccbpr",
+          "icciar",
+          "iccicr",
+          "icciidr",
+          "iccpmr",
+          "icdicer",
+          "icdicfr",
+          "icdictr",
+          "icdiser",
+          "icdisr",
+          "icdsgir",
+          "icenabler",
+          "intid",
+          "ipriority",
+          "irouter",
+          "isenabler",
+          "istatus",
+          "itargets",
+          "lable",
+          "ldivmod",
+          "ldmdb",
+          "ldmia",
+          "ldrbt",
+          "ldrex",
+          "ldrexb",
+          "ldrexd",
+          "ldrexh",
+          "ldrhbt",
+          "ldrht",
+          "ldrsb",
+          "ldrsbt",
+          "ldrsh",
+          "lshrdi",
+          "moddi",
+          "modsi",
+          "mpidr",
+          "muldi",
+          "mullu",
+          "nonshareable",
+          "nsacr",
+          "nsasedis",
+          "nuvia",
+          "oldit",
+          "readc",
+          "revsh",
+          "rfedb",
+          "sctlr",
+          "smccc",
+          "smlabb",
+          "smlabt",
+          "smlad",
+          "smladx",
+          "smlatb",
+          "smlatt",
+          "smlawb",
+          "smlawt",
+          "smlsd",
+          "smlsdx",
+          "smmla",
+          "smmlar",
+          "smmls",
+          "smmlsr",
+          "sourcery",
+          "srsdb",
+          "stmdb",
+          "stmia",
+          "strbt",
+          "strexb",
+          "strexd",
+          "strexh",
+          "strht",
+          "switchu",
+          "tpidrurw",
+          "ttbcr",
+          "typer",
+          "ucmpdi",
+          "udivdi",
+          "udivmoddi",
+          "udivsi",
+          "uefi's",
+          "uldiv",
+          "umoddi",
+          "umodsi",
+          "usada",
+          "vlpis",
+          "writec"
+        ],                          # words to extend to the dictionary for this package
+        "IgnoreStandardPaths": [    # Standard Plugin defined paths that
+            "*.asm", "*.s"          # should be ignore
+        ],
+        "AdditionalIncludePaths": [] # Additional paths to spell check
+                                     # (wildcards supported)
+    }
+}
--
2.17.1

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


Re: [PATCH v1 05/12] ArmPkg: Correct small typos

Sami Mujawar
 

Hi Pierre,

 

This patch looks good to me.

 

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

 

Regards,

 

Sami Mujawar

 

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 05/12] ArmPkg: Correct small typos

From: Pierre Gondois <Pierre.Gondois@...>

The 'cspell' CI test detected some small typos in ArmPkg.
Correct them.

Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                             | 2 +-
 ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c               | 2 +-
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c              | 4 ++--
 ArmPkg/Library/SemiHostingSerialPortLib/SerialPortLib.c     | 6 +++---
 .../StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c      | 2 +-
 .../SmbiosMiscDxe/Type03/MiscChassisManufacturerData.c      | 2 +-
 .../Type13/MiscNumberOfInstallableLanguagesFunction.c       | 6 +++---
 7 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 6c58d2b49317..54fad23cb42d 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -345,7 +345,7 @@ EfiAttributeToArmAttribute (
       break;
 
     case EFI_MEMORY_WC:
-      // Map to normal non-cachable
+      // Map to normal non-cacheable
       ArmAttributes = TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
       break;
 
diff --git a/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c b/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
index 6a06b38ab949..c5036b7b5c70 100644
--- a/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
+++ b/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
@@ -51,7 +51,7 @@ EFI_FILE gSemihostFsFile = {
 };
 
 //
-// Device path for semi-hosting. It contains our autogened Caller ID GUID.
+// Device path for semi-hosting. It contains our auto-generated Caller ID GUID.
 //
 typedef struct {
   VENDOR_DEVICE_PATH        Guid;
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
index 940e4bc797f2..6b9d7eba90b9 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
@@ -124,7 +124,7 @@ UpdatePageEntries (
   } else if ((Attributes & EFI_MEMORY_WC) != 0) {
     // modify cacheability attributes
     EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
-    // map to normal non-cachable
+    // map to normal non-cacheable
     EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
   } else if ((Attributes & EFI_MEMORY_WT) != 0) {
     // modify cacheability attributes
@@ -254,7 +254,7 @@ UpdateSectionEntries (
   } else if ((Attributes & EFI_MEMORY_WC) != 0) {
     // modify cacheability attributes
     EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
-    // map to normal non-cachable
+    // map to normal non-cacheable
     EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
   } else if ((Attributes & EFI_MEMORY_WT) != 0) {
     // modify cacheability attributes
diff --git a/ArmPkg/Library/SemiHostingSerialPortLib/SerialPortLib.c b/ArmPkg/Library/SemiHostingSerialPortLib/SerialPortLib.c
index e35bcee38098..b6a07dd46608 100644
--- a/ArmPkg/Library/SemiHostingSerialPortLib/SerialPortLib.c
+++ b/ArmPkg/Library/SemiHostingSerialPortLib/SerialPortLib.c
@@ -37,11 +37,11 @@ SerialPortInitialize (
 /**
   Write data to serial device.
 
-  @param  Buffer           Point of data buffer which need to be writed.
+  @param  Buffer           Point of data buffer which need to be written.
   @param  NumberOfBytes    Number of output bytes which are cached in Buffer.
 
   @retval 0                Write data failed.
-  @retval !0               Actual number of bytes writed to serial device.
+  @retval !0               Actual number of bytes written to serial device.
 
 **/
 
@@ -103,7 +103,7 @@ SerialPortWrite (
 /**
   Read data from serial device and save the datas in buffer.
 
-  @param  Buffer           Point of data buffer which need to be writed.
+  @param  Buffer           Point of data buffer which need to be written.
   @param  NumberOfBytes    Number of output bytes which are cached in Buffer.
 
   @retval 0                Read data failed.
diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
index 31672ae5cf4d..dd014beec873 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
+++ b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
@@ -102,7 +102,7 @@ SendMemoryPermissionRequest (
 
   // Check error response from Callee.
   if ((*RetVal & BIT31) != 0) {
-    // Bit 31 set means there is an error retured
+    // Bit 31 set means there is an error returned
     // See [1], Section 13.5.5.1 MM_SP_MEMORY_ATTRIBUTES_GET_AARCH64 and
     // Section 13.5.5.2 MM_SP_MEMORY_ATTRIBUTES_SET_AARCH64.
     switch (*RetVal) {
diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerData.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerData.c
index 137bd941d0b1..29449b871902 100644
--- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerData.c
+++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerData.c
@@ -23,7 +23,7 @@ SMBIOS_MISC_TABLE_DATA(SMBIOS_TABLE_TYPE3, MiscChassisManufacturer) = {
     0,                                                    // Length,
     0                                                     // Handle
   },
-  1,                                                      // Manufactrurer
+  1,                                                      // Manufacturer
   MiscChassisTypeMainServerChassis,                       // Type
   2,                                                      // Version
   3,                                                      // SerialNumber
diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type13/MiscNumberOfInstallableLanguagesFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type13/MiscNumberOfInstallableLanguagesFunction.c
index 19b60ed71f8c..7c941b5c0709 100644
--- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type13/MiscNumberOfInstallableLanguagesFunction.c
+++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type13/MiscNumberOfInstallableLanguagesFunction.c
@@ -23,9 +23,9 @@
 /**
   Get next language from language code list (with separator ';').
 
-  @param  LangCode       Input: point to first language in the list. On
-                         Otput: point to next language in the list, or
-                                NULL if no more language in the list.
+  @param  LangCode       Input:  point to first language in the list. On
+                         Output: point to next language in the list, or
+                                 NULL if no more language in the list.
   @param  Lang           The first language in the list.
 
 **/
--
2.17.1

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


Re: [PATCH v1 04/12] ArmPkg: Add OemMiscLibNull library to ArmPkg.dsc

Sami Mujawar
 

Hi Pierre,

 

Thank you for this patch.

 

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

 

Regards,

 

Sami Mujawar

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 04/12] ArmPkg: Add OemMiscLibNull library to ArmPkg.dsc

From: Pierre Gondois <Pierre.Gondois@...>

Add the OemMiscLibNull library to the [Components] section of
ArmPkg.dsc, allowing to complete the 'DscCompleteCheck' CI test.

According to .pytool/Readme about the 'DscCompleteCheck' test:
The test considers it an error if any INF does not appear in the
`Components` section of the package-level DSC.

Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=3258
Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 ArmPkg/ArmPkg.dsc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ArmPkg/ArmPkg.dsc b/ArmPkg/ArmPkg.dsc
index 282950b953a8..926986cf7fbb 100644
--- a/ArmPkg/ArmPkg.dsc
+++ b/ArmPkg/ArmPkg.dsc
@@ -156,6 +156,7 @@ [Components.common]
 
   ArmPkg/Universal/Smbios/ProcessorSubClassDxe/ProcessorSubClassDxe.inf
   ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
+  ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLibNull.inf
 
 [Components.AARCH64]
   ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
--
2.17.1

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


Re: [PATCH v1 03/12] ArmPkg: Add missing library headers to ArmPkg.dec

Sami Mujawar
 

Hi Pierre,

 

Thank you for this patch.

 

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

 

Regards,

 

Sami Mujawar

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 03/12] ArmPkg: Add missing library headers to ArmPkg.dec

From: Pierre Gondois <Pierre.Gondois@...>

Some library headers are missing/incorrect in ArmPkg.dec.
This makes the 'LibraryClassCheck' CI test fail. This patch
adds/corrects them.

According to .pytool/Readme about the 'LibraryClassCheck' test:
This test scans at all library header files found in the
`Library` folders in all of the package's declared include
directories and ensures that all files have a matching
LibraryClass declaration in the DEC file for the package.

Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=3254
Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=3258
Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 ArmPkg/ArmPkg.dec | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index a8a22c649ff8..496f588bd0ca 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -2,7 +2,7 @@
 # ARM processor package.
 #
 # Copyright (c) 2009 - 2010, Apple Inc. All rights reserved.<BR>
-# Copyright (c) 2011 - 2018, ARM Limited. All rights reserved.
+# Copyright (c) 2011 - 2021, ARM Limited. All rights reserved.
 #
 #    SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -29,14 +29,20 @@ [Includes.common]
 [LibraryClasses.common]
   ArmLib|Include/Library/ArmLib.h
   ArmMmuLib|Include/Library/ArmMmuLib.h
-  SemihostLib|Include/Library/Semihosting.h
+  SemihostLib|Include/Library/SemihostLib.h
   DefaultExceptionHandlerLib|Include/Library/DefaultExceptionHandlerLib.h
   ArmDisassemblerLib|Include/Library/ArmDisassemblerLib.h
   ArmGicArchLib|Include/Library/ArmGicArchLib.h
-  ArmMtlLib|ArmPlatformPkg/Include/Library/ArmMtlLib.h
+  ArmMtlLib|Include/Library/ArmMtlLib.h
   ArmSvcLib|Include/Library/ArmSvcLib.h
   OpteeLib|Include/Library/OpteeLib.h
   StandaloneMmMmuLib|Include/Library/StandaloneMmMmuLib.h
+  ArmGenericTimerCounterLib|Include/Library/ArmGenericTimerCounterLib.h
+  ArmGicLib|Include/Library/ArmGicLib.h
+  ArmHvcLib|Include/Library/ArmHvcLib.h
+  OemMiscLib|Include/Library/OemMiscLib.h
+  ArmSmcLib|Include/Library/ArmSmcLib.h
+
 
 [Guids.common]
   gArmTokenSpaceGuid       = { 0xBB11ECFE, 0x820F, 0x4968, { 0xBB, 0xA6, 0xF7, 0x6A, 0xFE, 0x30, 0x25, 0x96 } }
--
2.17.1

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


Re: [PATCH v1 02/12] ArmPkg: Fix Ecc error 3002 in StandaloneMmMmuLib

Sami Mujawar
 

Hi Pierre,

 

Thank you for this patch.

 

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

 

Regards,

 

Sami Mujawar

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 02/12] ArmPkg: Fix Ecc error 3002 in StandaloneMmMmuLib

From: Pierre Gondois <Pierre.Gondois@...>

This patch fixes the following Ecc reported error:
Non-Boolean comparisons should use a compare operator
(==, !=, >, < >=, <=)

Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 .../Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c  | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
index 5f453d18e415..31672ae5cf4d 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
+++ b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
@@ -101,7 +101,7 @@ SendMemoryPermissionRequest (
   }
 
   // Check error response from Callee.
-  if (*RetVal & BIT31) {
+  if ((*RetVal & BIT31) != 0) {
     // Bit 31 set means there is an error retured
     // See [1], Section 13.5.5.1 MM_SP_MEMORY_ATTRIBUTES_GET_AARCH64 and
     // Section 13.5.5.2 MM_SP_MEMORY_ATTRIBUTES_SET_AARCH64.
--
2.17.1

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


Re: [PATCH v1 01/12] ArmPkg: Fix Ecc error 8003

Sami Mujawar
 

Hi Pierre,

 

Thank you for this patch.

 

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

 

Regards,

 

Sami Mujawar

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 01/12] ArmPkg: Fix Ecc error 8003

From: Pierre Gondois <Pierre.Gondois@...>

This patch fixes the following Ecc reported error:
The #ifndef at the start of an include file should have
one postfix underscore, and no prefix underscore character

Some include guards have been modified to match the name of the
header file. Some comments have also been added on the closing
'#endif'.

Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 ArmPkg/Drivers/ArmGic/ArmGicDxe.h                   | 6 +++---
 ArmPkg/Drivers/CpuDxe/CpuDxe.h                      | 6 +++---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h | 6 +++---
 ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.h       | 6 +++---
 ArmPkg/Include/AsmMacroIoLib.h                      | 6 +++---
 ArmPkg/Include/AsmMacroIoLibV8.h                    | 6 +++---
 ArmPkg/Include/Chipset/AArch64.h                    | 6 +++---
 ArmPkg/Include/Chipset/AArch64Mmu.h                 | 6 +++---
 ArmPkg/Include/Chipset/ArmCortexA9.h                | 6 +++---
 ArmPkg/Include/Chipset/ArmV7.h                      | 6 +++---
 ArmPkg/Include/Chipset/ArmV7Mmu.h                   | 6 +++---
 ArmPkg/Include/Guid/ArmMpCoreInfo.h                 | 6 +++---
 ArmPkg/Include/IndustryStandard/ArmMmSvc.h          | 6 +++---
 ArmPkg/Include/IndustryStandard/ArmStdSmc.h         | 6 +++---
 ArmPkg/Include/Library/ArmDisassemblerLib.h         | 6 +++---
 ArmPkg/Include/Library/ArmGenericTimerCounterLib.h  | 6 +++---
 ArmPkg/Include/Library/ArmGicArchLib.h              | 6 +++---
 ArmPkg/Include/Library/ArmHvcLib.h                  | 6 +++---
 ArmPkg/Include/Library/ArmLib.h                     | 6 +++---
 ArmPkg/Include/Library/ArmMmuLib.h                  | 6 +++---
 ArmPkg/Include/Library/ArmSmcLib.h                  | 6 +++---
 ArmPkg/Include/Library/ArmSvcLib.h                  | 6 +++---
 ArmPkg/Include/Library/DefaultExceptionHandlerLib.h | 6 +++---
 ArmPkg/Include/Library/OpteeLib.h                   | 6 +++---
 ArmPkg/Include/Library/SemihostLib.h                | 6 +++---
 ArmPkg/Include/Library/StandaloneMmMmuLib.h         | 6 +++---
 ArmPkg/Include/Ppi/ArmMpCoreInfo.h                  | 6 +++---
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h          | 6 +++---
 ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h                | 6 +++---
 ArmPkg/Library/ArmLib/ArmLibPrivate.h               | 6 +++---
 ArmPkg/Library/OpteeLib/OpteeSmc.h                  | 6 +++---
 ArmPkg/Library/PlatformBootManagerLib/PlatformBm.h  | 6 +++---
 ArmPkg/Library/SemihostLib/SemihostPrivate.h        | 6 +++---
 33 files changed, 99 insertions(+), 99 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
index bf067ae03e08..c78b788ac012 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
+++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
@@ -6,8 +6,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 --*/
 
-#ifndef __ARM_GIC_DXE_H__
-#define __ARM_GIC_DXE_H__
+#ifndef ARM_GIC_DXE_H_
+#define ARM_GIC_DXE_H_
 
 #include <Library/ArmGicLib.h>
 #include <Library/ArmLib.h>
@@ -76,4 +76,4 @@ GicGetDistributorIcfgBaseAndBit (
   OUT UINTN                               *Config1Bit
   );
 
-#endif
+#endif // ARM_GIC_DXE_H_
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index 3fe5c24d5e5b..4cf3ab258c24 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -7,8 +7,8 @@
 
 **/
 
-#ifndef __CPU_DXE_ARM_EXCEPTION_H__
-#define __CPU_DXE_ARM_EXCEPTION_H__
+#ifndef CPU_DXE_H_
+#define CPU_DXE_H_
 
 #include <Uefi.h>
 
@@ -143,4 +143,4 @@ SetGcdMemorySpaceAttributes (
   IN UINT64                              Attributes
   );
 
-#endif // __CPU_DXE_ARM_EXCEPTION_H__
+#endif // CPU_DXE_H_
diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
index c64bc5c4627d..28db57e07bdf 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
@@ -5,8 +5,8 @@
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
 **/
-#ifndef __GENERIC_WATCHDOG_H__
-#define __GENERIC_WATCHDOG_H__
+#ifndef GENERIC_WATCHDOG_H_
+#define GENERIC_WATCHDOG_H_
 
 // Refresh Frame:
 #define GENERIC_WDOG_REFRESH_REG              ((UINTN)FixedPcdGet64 (PcdGenericWatchdogRefreshBase) + 0x000)
@@ -21,4 +21,4 @@
 #define GENERIC_WDOG_ENABLED          1
 #define GENERIC_WDOG_DISABLED         0
 
-#endif  // __GENERIC_WATCHDOG_H__
+#endif  // GENERIC_WATCHDOG_H_
diff --git a/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.h b/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.h
index ce92fe9f1b91..5fe7c5f4d4e3 100644
--- a/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.h
+++ b/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.h
@@ -7,8 +7,8 @@
 
 **/
 
-#ifndef __SEMIHOST_FS_H__
-#define __SEMIHOST_FS_H__
+#ifndef SEMIHOST_FS_H_
+#define SEMIHOST_FS_H_
 
 EFI_STATUS
 VolumeOpen (
@@ -242,5 +242,5 @@ FileFlush (
   IN EFI_FILE *File
   );
 
-#endif // __SEMIHOST_FS_H__
+#endif // SEMIHOST_FS_H_
 
diff --git a/ArmPkg/Include/AsmMacroIoLib.h b/ArmPkg/Include/AsmMacroIoLib.h
index e3576c8beb6e..6c901ac3871b 100644
--- a/ArmPkg/Include/AsmMacroIoLib.h
+++ b/ArmPkg/Include/AsmMacroIoLib.h
@@ -10,8 +10,8 @@
 **/
 
 
-#ifndef __MACRO_IO_LIB_H__
-#define __MACRO_IO_LIB_H__
+#ifndef ASM_MACRO_IO_LIB_H_
+#define ASM_MACRO_IO_LIB_H_
 
 #define _ASM_FUNC(Name, Section)    \
   .global   Name                  ; \
@@ -36,4 +36,4 @@
   movt      Reg, #:upper16:(Sym) - (. + 12) ; \
   ldr       Reg, [pc, Reg]
 
-#endif
+#endif // ASM_MACRO_IO_LIB_H_
diff --git a/ArmPkg/Include/AsmMacroIoLibV8.h b/ArmPkg/Include/AsmMacroIoLibV8.h
index bcc0d8dafe0c..337d9ae0168e 100644
--- a/ArmPkg/Include/AsmMacroIoLibV8.h
+++ b/ArmPkg/Include/AsmMacroIoLibV8.h
@@ -10,8 +10,8 @@
 **/
 
 
-#ifndef __MACRO_IO_LIBV8_H__
-#define __MACRO_IO_LIBV8_H__
+#ifndef ASM_MACRO_IO_LIBV8_H_
+#define ASM_MACRO_IO_LIBV8_H_
 
 // CurrentEL : 0xC = EL3; 8 = EL2; 4 = EL1
 // This only selects between EL1 and EL2, else we die.
@@ -54,4 +54,4 @@
   movk      Reg, ((Val) >> 16) & 0xffff, lsl #16  ; \
   movk      Reg, (Val) & 0xffff
 
-#endif // __MACRO_IO_LIBV8_H__
+#endif // ASM_MACRO_IO_LIBV8_H_
diff --git a/ArmPkg/Include/Chipset/AArch64.h b/ArmPkg/Include/Chipset/AArch64.h
index 09d4cfe28da7..10aeb9a15ad8 100644
--- a/ArmPkg/Include/Chipset/AArch64.h
+++ b/ArmPkg/Include/Chipset/AArch64.h
@@ -7,8 +7,8 @@
 
 **/
 
-#ifndef __AARCH64_H__
-#define __AARCH64_H__
+#ifndef AARCH64_H_
+#define AARCH64_H_
 
 #include <Chipset/AArch64Mmu.h>
 
@@ -238,4 +238,4 @@ ArmWriteCntHctl (
   IN UINT32 CntHctl
   );
 
-#endif // __AARCH64_H__
+#endif // AARCH64_H_
diff --git a/ArmPkg/Include/Chipset/AArch64Mmu.h b/ArmPkg/Include/Chipset/AArch64Mmu.h
index 6c7ada16b18a..fe38ba1c50ce 100644
--- a/ArmPkg/Include/Chipset/AArch64Mmu.h
+++ b/ArmPkg/Include/Chipset/AArch64Mmu.h
@@ -6,8 +6,8 @@
 *
 **/
 
-#ifndef __AARCH64_MMU_H_
-#define __AARCH64_MMU_H_
+#ifndef AARCH64_MMU_H_
+#define AARCH64_MMU_H_
 
 //
 // Memory Attribute Indirection register Definitions
@@ -194,5 +194,5 @@
 
 // Uses LPAE Page Table format
 
-#endif // __AARCH64_MMU_H_
+#endif // AARCH64_MMU_H_
 
diff --git a/ArmPkg/Include/Chipset/ArmCortexA9.h b/ArmPkg/Include/Chipset/ArmCortexA9.h
index 13d18e5893dd..cb937ebc8c8b 100644
--- a/ArmPkg/Include/Chipset/ArmCortexA9.h
+++ b/ArmPkg/Include/Chipset/ArmCortexA9.h
@@ -6,8 +6,8 @@
 
 **/
 
-#ifndef __ARM_CORTEX_A9_H__
-#define __ARM_CORTEX_A9_H__
+#ifndef ARM_CORTEX_A9_H_
+#define ARM_CORTEX_A9_H_
 
 #include <Chipset/ArmV7.h>
 
@@ -55,5 +55,5 @@ ArmGetScuBaseAddress (
   VOID
   );
 
-#endif
+#endif // ARM_CORTEX_A9_H_
 
diff --git a/ArmPkg/Include/Chipset/ArmV7.h b/ArmPkg/Include/Chipset/ArmV7.h
index 025f87a56d16..6b20b988e364 100644
--- a/ArmPkg/Include/Chipset/ArmV7.h
+++ b/ArmPkg/Include/Chipset/ArmV7.h
@@ -7,8 +7,8 @@
 
 **/
 
-#ifndef __ARM_V7_H__
-#define __ARM_V7_H__
+#ifndef ARM_V7_H_
+#define ARM_V7_H_
 
 #include <Chipset/ArmV7Mmu.h>
 
@@ -120,4 +120,4 @@ ArmWriteNsacr (
   IN  UINT32   Nsacr
   );
 
-#endif // __ARM_V7_H__
+#endif // ARM_V7_H_
diff --git a/ArmPkg/Include/Chipset/ArmV7Mmu.h b/ArmPkg/Include/Chipset/ArmV7Mmu.h
index 25d82d029795..87c443df3fce 100644
--- a/ArmPkg/Include/Chipset/ArmV7Mmu.h
+++ b/ArmPkg/Include/Chipset/ArmV7Mmu.h
@@ -6,8 +6,8 @@
 *
 **/
 
-#ifndef __ARMV7_MMU_H_
-#define __ARMV7_MMU_H_
+#ifndef ARMV7_MMU_H_
+#define ARMV7_MMU_H_
 
 #define TTBR_NOT_OUTER_SHAREABLE             BIT5
 #define TTBR_RGN_OUTER_NON_CACHEABLE         0
@@ -235,4 +235,4 @@ ConvertSectionAttributesToPageAttributes (
   IN BOOLEAN  IsLargePage
   );
 
-#endif
+#endif // ARMV7_MMU_H_
diff --git a/ArmPkg/Include/Guid/ArmMpCoreInfo.h b/ArmPkg/Include/Guid/ArmMpCoreInfo.h
index 3f9d17bb72c0..b810767879ae 100644
--- a/ArmPkg/Include/Guid/ArmMpCoreInfo.h
+++ b/ArmPkg/Include/Guid/ArmMpCoreInfo.h
@@ -6,8 +6,8 @@
 *
 **/
 
-#ifndef __ARM_MP_CORE_INFO_GUID_H_
-#define __ARM_MP_CORE_INFO_GUID_H_
+#ifndef ARM_MP_CORE_INFO_GUID_H_
+#define ARM_MP_CORE_INFO_GUID_H_
 
 #define MAX_CPUS_PER_MPCORE_SYSTEM    0x04
 #define SCU_CONFIG_REG_OFFSET         0x04
@@ -57,4 +57,4 @@ typedef struct {
 
 extern EFI_GUID gArmMpCoreInfoGuid;
 
-#endif /* MPCOREINFO_H_ */
+#endif /* ARM_MP_CORE_INFO_GUID_H_ */
diff --git a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
index 71a5398558b8..33d60ccf17bc 100644
--- a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
@@ -6,8 +6,8 @@
 *
 **/
 
-#ifndef __ARM_MM_SVC_H__
-#define __ARM_MM_SVC_H__
+#ifndef ARM_MM_SVC_H_
+#define ARM_MM_SVC_H_
 
 /*
  * SVC IDs to allow the MM secure partition to initialise itself, handle
@@ -44,4 +44,4 @@
 #define SPM_MAJOR_VERSION                     0
 #define SPM_MINOR_VERSION                     1
 
-#endif
+#endif // ARM_MM_SVC_H_
diff --git a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
index 9e0a3a3960d5..67afb0ea2d3d 100644
--- a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
@@ -10,8 +10,8 @@
 *    (https://developer.arm.com/documentation/den0028/c/?lang=en)
 **/
 
-#ifndef __ARM_STD_SMC_H__
-#define __ARM_STD_SMC_H__
+#ifndef ARM_STD_SMC_H_
+#define ARM_STD_SMC_H_
 
 /*
  * SMC function IDs for Standard Service queries
@@ -129,4 +129,4 @@
 /*                                    0xbf00ff02 is reserved */
 #define ARM_SMC_ID_TOS_REVISION       0xbf00ff03
 
-#endif
+#endif // ARM_STD_SMC_H_
diff --git a/ArmPkg/Include/Library/ArmDisassemblerLib.h b/ArmPkg/Include/Library/ArmDisassemblerLib.h
index c103b72e8127..d8c7af029dd3 100644
--- a/ArmPkg/Include/Library/ArmDisassemblerLib.h
+++ b/ArmPkg/Include/Library/ArmDisassemblerLib.h
@@ -6,8 +6,8 @@
 
 **/
 
-#ifndef __ARM_DISASSEBLER_LIB_H__
-#define __ARM_DISASSEBLER_LIB_H__
+#ifndef ARM_DISASSEMBLER_LIB_H_
+#define ARM_DISASSEMBLER_LIB_H_
 
 /**
   Place a disassembly of **OpCodePtr into buffer, and update OpCodePtr to
@@ -34,4 +34,4 @@ DisassembleInstruction (
   OUT UINTN     Size
   );
 
-#endif
+#endif // ARM_DISASSEMBLER_LIB_H_
diff --git a/ArmPkg/Include/Library/ArmGenericTimerCounterLib.h b/ArmPkg/Include/Library/ArmGenericTimerCounterLib.h
index d3051be30f2d..96bdffbf1ee3 100644
--- a/ArmPkg/Include/Library/ArmGenericTimerCounterLib.h
+++ b/ArmPkg/Include/Library/ArmGenericTimerCounterLib.h
@@ -7,8 +7,8 @@
 
 **/
 
-#ifndef __ARM_GENERIC_TIMER_COUNTER_LIB_H__
-#define __ARM_GENERIC_TIMER_COUNTER_LIB_H__
+#ifndef ARM_GENERIC_TIMER_COUNTER_LIB_H_
+#define ARM_GENERIC_TIMER_COUNTER_LIB_H_
 
 VOID
 EFIAPI
@@ -82,4 +82,4 @@ ArmGenericTimerSetCompareVal (
   IN   UINT64   Value
   );
 
-#endif
+#endif // ARM_GENERIC_TIMER_COUNTER_LIB_H_
diff --git a/ArmPkg/Include/Library/ArmGicArchLib.h b/ArmPkg/Include/Library/ArmGicArchLib.h
index 264322f1d0df..b3635d226866 100644
--- a/ArmPkg/Include/Library/ArmGicArchLib.h
+++ b/ArmPkg/Include/Library/ArmGicArchLib.h
@@ -6,8 +6,8 @@
 *
 **/
 
-#ifndef __ARM_GIC_ARCH_LIB_H__
-#define __ARM_GIC_ARCH_LIB_H__
+#ifndef ARM_GIC_ARCH_LIB_H_
+#define ARM_GIC_ARCH_LIB_H_
 
 //
 // GIC definitions
@@ -24,4 +24,4 @@ ArmGicGetSupportedArchRevision (
   VOID
   );
 
-#endif
+#endif // ARM_GIC_ARCH_LIB_H_
diff --git a/ArmPkg/Include/Library/ArmHvcLib.h b/ArmPkg/Include/Library/ArmHvcLib.h
index d26f0cff31eb..d202c2af6ee3 100644
--- a/ArmPkg/Include/Library/ArmHvcLib.h
+++ b/ArmPkg/Include/Library/ArmHvcLib.h
@@ -6,8 +6,8 @@
 *
 **/
 
-#ifndef __ARM_HVC_LIB__
-#define __ARM_HVC_LIB__
+#ifndef ARM_HVC_LIB_H_
+#define ARM_HVC_LIB_H_
 
 /**
  * The size of the HVC arguments are different between AArch64 and AArch32.
@@ -37,4 +37,4 @@ ArmCallHvc (
   IN OUT ARM_HVC_ARGS *Args
   );
 
-#endif
+#endif // ARM_HVC_LIB_H_
diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index 70b9d816b74c..5c232d779c83 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -8,8 +8,8 @@
 
 **/
 
-#ifndef __ARM_LIB__
-#define __ARM_LIB__
+#ifndef ARM_LIB_H_
+#define ARM_LIB_H_
 
 #include <Uefi/UefiBaseType.h>
 
@@ -753,4 +753,4 @@ ArmHasSecurityExtensions (
   );
 #endif // MDE_CPU_ARM
 
-#endif // __ARM_LIB__
+#endif // ARM_LIB_H_
diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h
index 23e89a0c6584..410f06ce373c 100644
--- a/ArmPkg/Include/Library/ArmMmuLib.h
+++ b/ArmPkg/Include/Library/ArmMmuLib.h
@@ -6,8 +6,8 @@
 
 **/
 
-#ifndef __ARM_MMU_LIB__
-#define __ARM_MMU_LIB__
+#ifndef ARM_MMU_LIB_H_
+#define ARM_MMU_LIB_H_
 
 #include <Uefi/UefiBaseType.h>
 
@@ -64,4 +64,4 @@ ArmSetMemoryAttributes (
   IN UINT64                    Attributes
   );
 
-#endif
+#endif // ARM_MMU_LIB_H_
diff --git a/ArmPkg/Include/Library/ArmSmcLib.h b/ArmPkg/Include/Library/ArmSmcLib.h
index 835d6788e0b9..ced60b3c1147 100644
--- a/ArmPkg/Include/Library/ArmSmcLib.h
+++ b/ArmPkg/Include/Library/ArmSmcLib.h
@@ -6,8 +6,8 @@
 *
 **/
 
-#ifndef __ARM_SMC_LIB__
-#define __ARM_SMC_LIB__
+#ifndef ARM_SMC_LIB_H_
+#define ARM_SMC_LIB_H_
 
 /**
  * The size of the SMC arguments are different between AArch64 and AArch32.
@@ -37,4 +37,4 @@ ArmCallSmc (
   IN OUT ARM_SMC_ARGS *Args
   );
 
-#endif
+#endif // ARM_SMC_LIB_H_
diff --git a/ArmPkg/Include/Library/ArmSvcLib.h b/ArmPkg/Include/Library/ArmSvcLib.h
index a4414270f3b9..d4a1a8f11863 100644
--- a/ArmPkg/Include/Library/ArmSvcLib.h
+++ b/ArmPkg/Include/Library/ArmSvcLib.h
@@ -6,8 +6,8 @@
 *
 **/
 
-#ifndef __ARM_SVC_LIB__
-#define __ARM_SVC_LIB__
+#ifndef ARM_SVC_LIB_H_
+#define ARM_SVC_LIB_H_
 
 /**
  * The size of the SVC arguments are different between AArch64 and AArch32.
@@ -43,4 +43,4 @@ ArmCallSvc (
   IN OUT ARM_SVC_ARGS *Args
   );
 
-#endif
+#endif // ARM_SVC_LIB_H_
diff --git a/ArmPkg/Include/Library/DefaultExceptionHandlerLib.h b/ArmPkg/Include/Library/DefaultExceptionHandlerLib.h
index bbcf30f85752..57dc555e1332 100644
--- a/ArmPkg/Include/Library/DefaultExceptionHandlerLib.h
+++ b/ArmPkg/Include/Library/DefaultExceptionHandlerLib.h
@@ -6,8 +6,8 @@
 
 **/
 
-#ifndef __DEFAULT_EXCEPTION_HANDLER_LIB_H__
-#define __DEFAULT_EXCEPTION_HANDLER_LIB_H__
+#ifndef DEFAULT_EXCEPTION_HANDLER_LIB_H_
+#define DEFAULT_EXCEPTION_HANDLER_LIB_H_
 
 /**
   This is the default action to take on an unexpected exception
@@ -22,4 +22,4 @@ DefaultExceptionHandler (
   IN OUT EFI_SYSTEM_CONTEXT           SystemContext
   );
 
-#endif
+#endif // DEFAULT_EXCEPTION_HANDLER_LIB_H_
diff --git a/ArmPkg/Include/Library/OpteeLib.h b/ArmPkg/Include/Library/OpteeLib.h
index 8ceab117d132..b9399d2e1810 100644
--- a/ArmPkg/Include/Library/OpteeLib.h
+++ b/ArmPkg/Include/Library/OpteeLib.h
@@ -8,8 +8,8 @@
 
 **/
 
-#ifndef _OPTEE_H_
-#define _OPTEE_H_
+#ifndef OPTEE_LIB_H_
+#define OPTEE_LIB_H_
 
 /*
  * The 'Trusted OS Call UID' is supposed to return the following UUID for
@@ -117,4 +117,4 @@ OpteeInvokeFunction (
   IN OUT OPTEE_INVOKE_FUNCTION_ARG       *InvokeFunctionArg
   );
 
-#endif
+#endif // OPTEE_LIB_H_
diff --git a/ArmPkg/Include/Library/SemihostLib.h b/ArmPkg/Include/Library/SemihostLib.h
index ce08b2778d72..590728c804f4 100644
--- a/ArmPkg/Include/Library/SemihostLib.h
+++ b/ArmPkg/Include/Library/SemihostLib.h
@@ -7,8 +7,8 @@
 
 **/
 
-#ifndef __SEMIHOSTING_H__
-#define __SEMIHOSTING_H__
+#ifndef SEMIHOSTING_LIB_H_
+#define SEMIHOSTING_LIB_H_
 
 /*
  *
@@ -129,4 +129,4 @@ SemihostSystem (
   IN CHAR8 *CommandLine
   );
 
-#endif // __SEMIHOSTING_H__
+#endif // SEMIHOSTING_LIB_H_
diff --git a/ArmPkg/Include/Library/StandaloneMmMmuLib.h b/ArmPkg/Include/Library/StandaloneMmMmuLib.h
index 1d2a0e0c2bac..ccc016d0350a 100644
--- a/ArmPkg/Include/Library/StandaloneMmMmuLib.h
+++ b/ArmPkg/Include/Library/StandaloneMmMmuLib.h
@@ -6,8 +6,8 @@
 
 **/
 
-#ifndef __STANDALONEMM_MMU_LIB__
-#define __STANDALONEMM_MMU_LIB__
+#ifndef STANDALONE_MM_MMU_LIB_
+#define STANDALONE_MM_MMU_LIB_
 
 EFI_STATUS
 ArmSetMemoryRegionNoExec (
@@ -33,4 +33,4 @@ ArmClearMemoryRegionReadOnly (
   IN  UINT64                    Length
   );
 
-#endif /* __STANDALONEMM_MMU_LIB__ */
+#endif /* STANDALONE_MM_MMU_LIB_ */
diff --git a/ArmPkg/Include/Ppi/ArmMpCoreInfo.h b/ArmPkg/Include/Ppi/ArmMpCoreInfo.h
index 871119bde2f0..b1e404ce1364 100644
--- a/ArmPkg/Include/Ppi/ArmMpCoreInfo.h
+++ b/ArmPkg/Include/Ppi/ArmMpCoreInfo.h
@@ -6,8 +6,8 @@
 *
 **/
 
-#ifndef __ARM_MP_CORE_INFO_PPI_H__
-#define __ARM_MP_CORE_INFO_PPI_H__
+#ifndef ARM_MP_CORE_INFO_PPI_H_
+#define ARM_MP_CORE_INFO_PPI_H_
 
 #include <Guid/ArmMpCoreInfo.h>
 
@@ -49,4 +49,4 @@ typedef struct {
 extern EFI_GUID gArmMpCoreInfoPpiGuid;
 extern EFI_GUID gArmMpCoreInfoGuid;
 
-#endif
+#endif // ARM_MP_CORE_INFO_PPI_H_
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
index cfc0c878a415..318020277b24 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
@@ -8,8 +8,8 @@
 
 **/
 
-#ifndef __AARCH64_LIB_H__
-#define __AARCH64_LIB_H__
+#ifndef AARCH64_LIB_H_
+#define AARCH64_LIB_H_
 
 typedef VOID (*AARCH64_CACHE_OPERATION)(UINTN);
 
@@ -52,5 +52,5 @@ ArmReadIdAA64Mmfr2 (
   VOID
   );
 
-#endif // __AARCH64_LIB_H__
+#endif // AARCH64_LIB_H_
 
diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h
index dcf6723b803b..5a92ade2b313 100644
--- a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h
+++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h
@@ -6,8 +6,8 @@
 
 **/
 
-#ifndef __ARM_V7_LIB_H__
-#define __ARM_V7_LIB_H__
+#ifndef ARM_V7_LIB_H_
+#define ARM_V7_LIB_H_
 
 #define ID_MMFR0_SHARELVL_SHIFT       12
 #define ID_MMFR0_SHARELVL_MASK       0xf
@@ -64,5 +64,5 @@ ArmReadIdPfr1 (
   VOID
   );
 
-#endif // __ARM_V7_LIB_H__
+#endif // ARM_V7_LIB_H_
 
diff --git a/ArmPkg/Library/ArmLib/ArmLibPrivate.h b/ArmPkg/Library/ArmLib/ArmLibPrivate.h
index 1818a1994dc3..5db83d620bfc 100644
--- a/ArmPkg/Library/ArmLib/ArmLibPrivate.h
+++ b/ArmPkg/Library/ArmLib/ArmLibPrivate.h
@@ -8,8 +8,8 @@
 
 **/
 
-#ifndef __ARM_LIB_PRIVATE_H__
-#define __ARM_LIB_PRIVATE_H__
+#ifndef ARM_LIB_PRIVATE_H_
+#define ARM_LIB_PRIVATE_H_
 
 #define CACHE_SIZE_4_KB             (3UL)
 #define CACHE_SIZE_8_KB             (4UL)
@@ -186,4 +186,4 @@ ReadCLIDR (
   VOID
   );
 
-#endif // __ARM_LIB_PRIVATE_H__
+#endif // ARM_LIB_PRIVATE_H_
diff --git a/ArmPkg/Library/OpteeLib/OpteeSmc.h b/ArmPkg/Library/OpteeLib/OpteeSmc.h
index 62319d718dc5..b760ec8f8227 100644
--- a/ArmPkg/Library/OpteeLib/OpteeSmc.h
+++ b/ArmPkg/Library/OpteeLib/OpteeSmc.h
@@ -7,8 +7,8 @@
 
 **/
 
-#ifndef _OPTEE_SMC_H_
-#define _OPTEE_SMC_H_
+#ifndef OPTEE_SMC_H_
+#define OPTEE_SMC_H_
 
 /* Returned in Arg0 only from Trusted OS functions */
 #define OPTEE_SMC_RETURN_OK                     0x0
@@ -47,4 +47,4 @@ typedef struct {
   UINT8   Data4[8];
 } RFC4122_UUID;
 
-#endif
+#endif // OPTEE_SMC_H_
diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.h b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.h
index 0bb3645ddc47..a40a2ff5cb4f 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.h
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.h
@@ -9,8 +9,8 @@
 
 **/
 
-#ifndef _PLATFORM_BM_H_
-#define _PLATFORM_BM_H_
+#ifndef PLATFORM_BM_H_
+#define PLATFORM_BM_H_
 
 #include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
@@ -50,4 +50,4 @@ DisableQuietBoot (
   VOID
   );
 
-#endif // _PLATFORM_BM_H_
+#endif // PLATFORM_BM_H_
diff --git a/ArmPkg/Library/SemihostLib/SemihostPrivate.h b/ArmPkg/Library/SemihostLib/SemihostPrivate.h
index 30103b04b53f..886472611623 100644
--- a/ArmPkg/Library/SemihostLib/SemihostPrivate.h
+++ b/ArmPkg/Library/SemihostLib/SemihostPrivate.h
@@ -7,8 +7,8 @@
 
 **/
 
-#ifndef __SEMIHOST_PRIVATE_H__
-#define __SEMIHOST_PRIVATE_H__
+#ifndef SEMIHOST_PRIVATE_H_
+#define SEMIHOST_PRIVATE_H_
 
 typedef struct {
   CHAR8   *FileName;
@@ -209,4 +209,4 @@ GccSemihostCall (
 
 #endif // __CC_ARM
 
-#endif //__SEMIHOST_PRIVATE_H__
+#endif // SEMIHOST_PRIVATE_H_
--
2.17.1

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


Re: RFC: Adding support for ARM (RNDR etc.) to RngDxe

Sami Mujawar
 

Hi Rebecca,

 

I have been working on the following modules (See slide 11 in “EDKII - Proposed update to RNG implementation.pdf”):

  1. TrngLib|FwTrnglib (Arm Firmware TRNG)
  2. DrbgLib stack – with support for DrbgAlgorithmLib|CRT_DRBG & AesLib|ArmAesInstructionLib.

 

I plan to post patches for (a) in the next fortnight. Following this I plan to update the proposal with the interface definitions for the various library interfaces in the DrbgLib Stack.

 

I have not looked at RngLib|RNDR as I believe you were interested in implementing the part. Kindly let me know if you plan to implement this and the platform you would be using for testing. It looks like the FVP_Base_AEMv8A-AEMv8A and the FVP-RevC models support RNDR, so these could be used for testing as well. Please feel free to get in touch should you need any help with the model parameters or if you face any issues.

 

Regards,

 

Sami Mujawar

 

From: Rebecca Cran <rebecca@...>
Date: Tuesday, 20 April 2021 at 21:04
To: Sami Mujawar <Sami.Mujawar@...>, devel@edk2.groups.io <devel@edk2.groups.io>, Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>, Ard Biesheuvel <Ard.Biesheuvel@...>, leif@... <leif@...>
Cc: rfc@edk2.groups.io <rfc@edk2.groups.io>, Jiewen Yao <jiewen.yao@...>, Rahul Kumar <rahul1.kumar@...>, nd <nd@...>, Jose Marinho <Jose.Marinho@...>
Subject: Re: [edk2-devel] RFC: Adding support for ARM (RNDR etc.) to RngDxe

Hi Sami,

I was wondering if you're still collecting feedback on the design, or if
you have a plan and schedule for the implementation?

--
Rebecca Cran

On 1/15/21 7:51 PM, Sami Mujawar wrote:
> Hi All,
>
> I have shared some initial thoughts on the RNG implementation updates at https://edk2.groups.io/g/devel/files/Designs/2021/0116/EDKII%20-%20Proposed%20update%20to%20RNG%20implementation.pdf
>
> Kindly let me know your feedback or if you have any queries.
>
> Regards,
>
> Sami Mujawar
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Rebecca Cran via groups.io
> Sent: 14 January 2021 09:05 PM
> To: Sami Mujawar <Sami.Mujawar@...>; devel@edk2.groups.io; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>; Ard Biesheuvel <Ard.Biesheuvel@...>; leif@...
> Cc: rfc@edk2.groups.io; Jiewen Yao <jiewen.yao@...>; Rahul Kumar <rahul1.kumar@...>; nd <nd@...>
> Subject: Re: [edk2-devel] RFC: Adding support for ARM (RNDR etc.) to RngDxe
>
> On 12/10/20 4:26 AM, Sami Mujawar wrote:
>
>> I am working on the TRNG FW API interface and will share more details
>> for the discussion soon.
>>
>> We had some thoughts about streamlining the RngDxe implementations and
>> would like to share some diagrams for the discussion.
>>
>> My diagrams are in Visio that I can export as JPG images. However, I am
>> open to switching to any other suggested tool.
>
> Hi Sami,
>
> I don't see any further discussions on this. Have you made any progress
> with sharing the design documents or scheduling a review?
>


Re: [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV

Laszlo Ersek
 

On 04/22/21 09:34, Laszlo Ersek wrote:

The new InternalTpmDecryptAddressRange() function should be called
from Tcg2ConfigPeimEntryPoint(), before the latter calls
InternalTpm12Detect(). Regarding error checking... if
InternalTpmDecryptAddressRange() fails, I think we can log an error
message, and hang with CpuDeadLoop().
Sorry, another point:

(6) where we determine that no TPM is available:

//
// If no TPM2 was detected, we still need to install
// TpmInitializationDonePpi. Namely, Tcg2Pei will exit early upon seeing
// the default (all-bits-zero) contents of PcdTpmInstanceGuid, thus we have
// to install the PPI in its place, in order to unblock any dependent
// PEIMs.
//
Status = PeiServicesInstallPpi (&mTpmInitializationDonePpiList);

we should re-encrypt the address range, as if nothing had happened.

For this, we'll likely need a similarly polymorphic function called
InternalTpmEncryptAddressRange().

(

For some background on this particular branch of the code, please refer
to commit 6cf1880fb5b6 ("OvmfPkg: add customized Tcg2ConfigPei clone",
2018-03-09):

- Check the QEMU hardware for TPM2 availability only

- If found, set the dynamic PCD "PcdTpmInstanceGuid" to
&gEfiTpmDeviceInstanceTpm20DtpmGuid. This is what informs the rest of
the firmware about the TPM type.

- Install the gEfiTpmDeviceSelectedGuid PPI. This action permits the
PEI_CORE to dispatch the Tcg2Pei module, which consumes the above PCD.
In effect, the gEfiTpmDeviceSelectedGuid PPI serializes the setting
and the consumption of the "TPM type" PCD.

- If no TPM2 was found, install gPeiTpmInitializationDonePpiGuid.
(Normally this is performed by Tcg2Pei, but Tcg2Pei doesn't do it if
no TPM2 is available. So in that case our Tcg2ConfigPei must do it.)

)

Thanks
Laszlo

8421 - 8440 of 82716