Date   

Re: [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

Dandan Bi
 

Reviewed-by: Dandan Bi <dandan.bi@intel.com>



Thanks,
Dandan

-----Original Message-----
From: Ma, Hua <hua.ma@intel.com>
Sent: Tuesday, October 12, 2021 4:34 PM
To: devel@edk2.groups.io
Cc: Ma, Hua <hua.ma@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
Liming Gao <gaoliming@byosoft.com.cn>; Bi, Dandan <dandan.bi@intel.com>;
Ni, Ray <ray.ni@intel.com>
Subject: [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating
gHandleList

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

This patch fixes the following issue:

The global variable gHandleList is a linked list.
This list is locked when a entry is added or removed from the list, but there is no
lock when iterating this list in function CoreValidateHandle().
It can lead to "Handle.c (76): CR has Bad Signature" assertion if the iterated
entry in the list is just removed by other task during iterating.
Locking the list when iterating can fix this issue.

v2 changes:
- Add lock check and comments in CoreGetProtocolInterface() before calling
CoreValidateHandle()
- Update the comments in CoreValidateHandle() header file

v1: https://edk2.groups.io/g/devel/topic/86233569

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Hua Ma <hua.ma@intel.com>
---
MdeModulePkg/Core/Dxe/Hand/DriverSupport.c | 10 ++--
MdeModulePkg/Core/Dxe/Hand/Handle.c | 56 +++++++++++++++-------
MdeModulePkg/Core/Dxe/Hand/Handle.h | 5 +-
MdeModulePkg/Core/Dxe/Hand/Notify.c | 2 +-
4 files changed, 50 insertions(+), 23 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
index feabf12faf..eb8a765d2c 100644
--- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
+++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
@@ -68,7 +68,7 @@ CoreConnectController (
//
// Make sure ControllerHandle is valid
//
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -154,7 +154,7 @@ CoreConnectController (
//
// Make sure the DriverBindingHandle is valid
//
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, TRUE);
if (EFI_ERROR (Status)) {
//
// Release the protocol lock on the handle database @@ -268,7 +268,7 @@
AddSortedDriverBindingProtocol (
//
// Make sure the DriverBindingHandle is valid
//
- Status = CoreValidateHandle (DriverBindingHandle);
+ Status = CoreValidateHandle (DriverBindingHandle, FALSE);
if (EFI_ERROR (Status)) {
return;
}
@@ -746,7 +746,7 @@ CoreDisconnectController (
//
// Make sure ControllerHandle is valid
//
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -755,7 +755,7 @@ CoreDisconnectController (
// Make sure ChildHandle is valid if it is not NULL
//
if (ChildHandle != NULL) {
- Status = CoreValidateHandle (ChildHandle);
+ Status = CoreValidateHandle (ChildHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c
b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 6eccb41ecb..46f67d3d6a 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -55,31 +55,46 @@ CoreReleaseProtocolLock (
Check whether a handle is a valid EFI_HANDLE

@param UserHandle The handle to check
+ @param IsLocked The protocol lock is acquried or not

@retval EFI_INVALID_PARAMETER The handle is NULL or not a valid
EFI_HANDLE.
+ @retval EFI_NOT_FOUND The handle is not found in the handle
database.
@retval EFI_SUCCESS The handle is valid EFI_HANDLE.

**/
EFI_STATUS
CoreValidateHandle (
- IN EFI_HANDLE UserHandle
+ IN EFI_HANDLE UserHandle,
+ IN BOOLEAN IsLocked
)
{
IHANDLE *Handle;
LIST_ENTRY *Link;
+ EFI_STATUS Status;

if (UserHandle == NULL) {
return EFI_INVALID_PARAMETER;
}

+ if (IsLocked) {
+ ASSERT_LOCKED(&gProtocolDatabaseLock);
+ } else {
+ CoreAcquireProtocolLock ();
+ }
+
+ Status = EFI_NOT_FOUND;
for (Link = gHandleList.BackLink; Link != &gHandleList; Link = Link->BackLink) {
Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
if (Handle == (IHANDLE *) UserHandle) {
- return EFI_SUCCESS;
+ Status = EFI_SUCCESS;
+ break;
}
}

- return EFI_INVALID_PARAMETER;
+ if (!IsLocked) {
+ CoreReleaseProtocolLock ();
+ }
+ return Status;
}


@@ -428,7 +443,7 @@ CoreInstallProtocolInterfaceNotify (
//
InsertTailList (&gHandleList, &Handle->AllHandles);
} else {
- Status = CoreValidateHandle (Handle);
+ Status = CoreValidateHandle (Handle, TRUE);
if (EFI_ERROR (Status)) {
DEBUG((DEBUG_ERROR, "InstallProtocolInterface: input handle at 0x%x is
invalid\n", Handle));
goto Done;
@@ -723,7 +738,7 @@ CoreUninstallProtocolInterface (
//
// Check that UserHandle is a valid handle
//
- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -899,7 +914,16 @@ CoreGetProtocolInterface (
IHANDLE *Handle;
LIST_ENTRY *Link;

- Status = CoreValidateHandle (UserHandle);
+ //
+ // The gProtocolDatabaseLock must be owned //
+ ASSERT_LOCKED(&gProtocolDatabaseLock);
+
+ //
+ // The gProtocolDatabaseLock is owned // Call CoreValidateHandle ()
+ with IsLocked == TRUE // Status = CoreValidateHandle (UserHandle,
+ TRUE);
if (EFI_ERROR (Status)) {
return NULL;
}
@@ -1013,7 +1037,7 @@ CoreOpenProtocol (
//
// Check for invalid UserHandle
//
- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -1023,11 +1047,11 @@ CoreOpenProtocol (
//
switch (Attributes) {
case EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER :
- Status = CoreValidateHandle (ImageHandle);
+ Status = CoreValidateHandle (ImageHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -1037,17 +1061,17 @@ CoreOpenProtocol (
break;
case EFI_OPEN_PROTOCOL_BY_DRIVER :
case EFI_OPEN_PROTOCOL_BY_DRIVER | EFI_OPEN_PROTOCOL_EXCLUSIVE :
- Status = CoreValidateHandle (ImageHandle);
+ Status = CoreValidateHandle (ImageHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
break;
case EFI_OPEN_PROTOCOL_EXCLUSIVE :
- Status = CoreValidateHandle (ImageHandle);
+ Status = CoreValidateHandle (ImageHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -1249,16 +1273,16 @@ CoreCloseProtocol (
//
// Check for invalid parameters
//
- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
- Status = CoreValidateHandle (AgentHandle);
+ Status = CoreValidateHandle (AgentHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
if (ControllerHandle != NULL) {
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -1443,7 +1467,7 @@ CoreProtocolsPerHandle (
UINTN ProtocolCount;
EFI_GUID **Buffer;

- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.h
b/MdeModulePkg/Core/Dxe/Hand/Handle.h
index 83eb2b9f3a..20d886beca 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.h
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.h
@@ -244,14 +244,17 @@ CoreReleaseProtocolLock (
Check whether a handle is a valid EFI_HANDLE

@param UserHandle The handle to check
+ @param IsLocked The protocol lock is acquried or not

@retval EFI_INVALID_PARAMETER The handle is NULL or not a valid
EFI_HANDLE.
+ @retval EFI_NOT_FOUND The handle is not found in the handle
database.
@retval EFI_SUCCESS The handle is valid EFI_HANDLE.

**/
EFI_STATUS
CoreValidateHandle (
- IN EFI_HANDLE UserHandle
+ IN EFI_HANDLE UserHandle,
+ IN BOOLEAN IsLocked
);

//
diff --git a/MdeModulePkg/Core/Dxe/Hand/Notify.c
b/MdeModulePkg/Core/Dxe/Hand/Notify.c
index 553413a350..f4e7c01e96 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Notify.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Notify.c
@@ -188,7 +188,7 @@ CoreReinstallProtocolInterface (
PROTOCOL_INTERFACE *Prot;
PROTOCOL_ENTRY *ProtEntry;

- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
--
2.32.0.windows.2


Re: [PATCH 2/2] Allow wildcards in hostname

Yao, Jiewen
 

It seems the Bugzilla only describes the ECC, but no much info on why we need allow wildcards in hostname.

The git log in mu is also unclear to me - "This enables certain local network recovery stories. May re-evaluate as those stories change. "

I am OK with ECC change, and give R-B.

But I would like to understand more on why we need allow wildcards in general. What are the stories?

If this is only for "recovery stories", should we also allow wildcards in recovery boot path?

For example, should we have a PCD to platform owner make decision? E.g. normal boot - NO. recovery boot - YES ?

Thank you
Yao Jiewen

-----Original Message-----
From: Vineel Kovvuri <vineel.kovvuri@gmail.com>
Sent: Tuesday, October 12, 2021 1:38 PM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>;
sean.brogan@microsoft.com; bret.barkelew@microsoft.com;
Michael.Turner@microsoft.com
Cc: Vineel Kovvuri <vineelko@microsoft.com>
Subject: [PATCH 2/2] Allow wildcards in hostname

This PR is cherry-picked from
https://github.com/microsoft/mu_basecore/commit/d0c7733400c35722499ee
dcd4279042a9bcb0eb4

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

Signed-off-by: Vineel Kovvuri <vineelko@microsoft.com>
---
NetworkPkg/HttpDxe/HttpsSupport.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/NetworkPkg/HttpDxe/HttpsSupport.c
b/NetworkPkg/HttpDxe/HttpsSupport.c
index 7e0bf85c3c..0f28ae9447 100644
--- a/NetworkPkg/HttpDxe/HttpsSupport.c
+++ b/NetworkPkg/HttpDxe/HttpsSupport.c
@@ -625,7 +625,7 @@ TlsConfigureSession (
//
HttpInstance->TlsConfigData.ConnectionEnd = EfiTlsClient;
HttpInstance->TlsConfigData.VerifyMethod = EFI_TLS_VERIFY_PEER;
- HttpInstance->TlsConfigData.VerifyHost.Flags =
EFI_TLS_VERIFY_FLAG_NO_WILDCARDS;
+ HttpInstance->TlsConfigData.VerifyHost.Flags =
EFI_TLS_VERIFY_FLAG_NONE;
HttpInstance->TlsConfigData.VerifyHost.HostName = HttpInstance-
RemoteHost;
HttpInstance->TlsConfigData.SessionState = EfiTlsSessionNotStarted;

--
2.17.1


Re: [PATCH 1/2] Reconfigure OpensslLib to add elliptic curve chipher algorithms

Yao, Jiewen
 

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

-----Original Message-----
From: Vineel Kovvuri <vineel.kovvuri@gmail.com>
Sent: Tuesday, October 12, 2021 1:38 PM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>;
sean.brogan@microsoft.com; bret.barkelew@microsoft.com;
Michael.Turner@microsoft.com
Cc: Vineel Kovvuri <vineelko@microsoft.com>
Subject: [PATCH 1/2] Reconfigure OpensslLib to add elliptic curve chipher
algorithms

This commit is a cherry pick of project mu's commit
https://github.com/microsoft/mu_tiano_plus/commit/1f3b135ddc821718a78c3
52316197889c5d3e0c2

Reconfigure OpensslLib to add elliptic curve chipher algorithms.
The only file manually changed is process_files.pl.
Running the script changes the other three files.

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

Signed-off-by: Vineel Kovvuri <vineelko@microsoft.com>
---
.../Library/Include/openssl/opensslconf.h | 25 ++--------
CryptoPkg/Library/OpensslLib/OpensslLib.inf | 50 +++++++++++++++++++
.../Library/OpensslLib/OpensslLibCrypto.inf | 50 +++++++++++++++++++
CryptoPkg/Library/OpensslLib/process_files.pl | 1 -
4 files changed, 105 insertions(+), 21 deletions(-)

diff --git a/CryptoPkg/Library/Include/openssl/opensslconf.h
b/CryptoPkg/Library/Include/openssl/opensslconf.h
index b8d59aebe8..09a6641ffc 100644
--- a/CryptoPkg/Library/Include/openssl/opensslconf.h
+++ b/CryptoPkg/Library/Include/openssl/opensslconf.h
@@ -55,9 +55,6 @@ extern "C" {
#ifndef OPENSSL_NO_DSA
# define OPENSSL_NO_DSA
#endif
-#ifndef OPENSSL_NO_EC
-# define OPENSSL_NO_EC
-#endif
#ifndef OPENSSL_NO_IDEA
# define OPENSSL_NO_IDEA
#endif
@@ -88,9 +85,6 @@ extern "C" {
#ifndef OPENSSL_NO_SEED
# define OPENSSL_NO_SEED
#endif
-#ifndef OPENSSL_NO_SM2
-# define OPENSSL_NO_SM2
-#endif
#ifndef OPENSSL_NO_SRP
# define OPENSSL_NO_SRP
#endif
@@ -154,12 +148,6 @@ extern "C" {
#ifndef OPENSSL_NO_EC_NISTP_64_GCC_128
# define OPENSSL_NO_EC_NISTP_64_GCC_128
#endif
-#ifndef OPENSSL_NO_ECDH
-# define OPENSSL_NO_ECDH
-#endif
-#ifndef OPENSSL_NO_ECDSA
-# define OPENSSL_NO_ECDSA
-#endif
#ifndef OPENSSL_NO_EGD
# define OPENSSL_NO_EGD
#endif
@@ -226,9 +214,6 @@ extern "C" {
#ifndef OPENSSL_NO_TESTS
# define OPENSSL_NO_TESTS
#endif
-#ifndef OPENSSL_NO_TLS1_3
-# define OPENSSL_NO_TLS1_3
-#endif
#ifndef OPENSSL_NO_UBSAN
# define OPENSSL_NO_UBSAN
#endif
@@ -265,11 +250,11 @@ extern "C" {
# undef DECLARE_DEPRECATED
# define DECLARE_DEPRECATED(f) f __attribute__ ((deprecated));
# endif
-#elif defined(__SUNPRO_C)
-#if (__SUNPRO_C >= 0x5130)
-#undef DECLARE_DEPRECATED
-#define DECLARE_DEPRECATED(f) f __attribute__ ((deprecated));
-#endif
+# elif defined(__SUNPRO_C)
+# if (__SUNPRO_C >= 0x5130)
+# undef DECLARE_DEPRECATED
+# define DECLARE_DEPRECATED(f) f __attribute__ ((deprecated));
+# endif
# endif
#endif

diff --git a/CryptoPkg/Library/OpensslLib/OpensslLib.inf
b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
index d84bde056a..bd3d9cc90f 100644
--- a/CryptoPkg/Library/OpensslLib/OpensslLib.inf
+++ b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
@@ -199,6 +199,43 @@
$(OPENSSL_PATH)/crypto/dso/dso_vms.c
$(OPENSSL_PATH)/crypto/dso/dso_win32.c
$(OPENSSL_PATH)/crypto/ebcdic.c
+ $(OPENSSL_PATH)/crypto/ec/curve25519.c
+ $(OPENSSL_PATH)/crypto/ec/curve448/arch_32/f_impl.c
+ $(OPENSSL_PATH)/crypto/ec/curve448/curve448.c
+ $(OPENSSL_PATH)/crypto/ec/curve448/curve448_tables.c
+ $(OPENSSL_PATH)/crypto/ec/curve448/eddsa.c
+ $(OPENSSL_PATH)/crypto/ec/curve448/f_generic.c
+ $(OPENSSL_PATH)/crypto/ec/curve448/scalar.c
+ $(OPENSSL_PATH)/crypto/ec/ec2_oct.c
+ $(OPENSSL_PATH)/crypto/ec/ec2_smpl.c
+ $(OPENSSL_PATH)/crypto/ec/ec_ameth.c
+ $(OPENSSL_PATH)/crypto/ec/ec_asn1.c
+ $(OPENSSL_PATH)/crypto/ec/ec_check.c
+ $(OPENSSL_PATH)/crypto/ec/ec_curve.c
+ $(OPENSSL_PATH)/crypto/ec/ec_cvt.c
+ $(OPENSSL_PATH)/crypto/ec/ec_err.c
+ $(OPENSSL_PATH)/crypto/ec/ec_key.c
+ $(OPENSSL_PATH)/crypto/ec/ec_kmeth.c
+ $(OPENSSL_PATH)/crypto/ec/ec_lib.c
+ $(OPENSSL_PATH)/crypto/ec/ec_mult.c
+ $(OPENSSL_PATH)/crypto/ec/ec_oct.c
+ $(OPENSSL_PATH)/crypto/ec/ec_pmeth.c
+ $(OPENSSL_PATH)/crypto/ec/ec_print.c
+ $(OPENSSL_PATH)/crypto/ec/ecdh_kdf.c
+ $(OPENSSL_PATH)/crypto/ec/ecdh_ossl.c
+ $(OPENSSL_PATH)/crypto/ec/ecdsa_ossl.c
+ $(OPENSSL_PATH)/crypto/ec/ecdsa_sign.c
+ $(OPENSSL_PATH)/crypto/ec/ecdsa_vrf.c
+ $(OPENSSL_PATH)/crypto/ec/eck_prn.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_mont.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_nist.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_nistp224.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_nistp256.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_nistp521.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_nistputil.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_oct.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_smpl.c
+ $(OPENSSL_PATH)/crypto/ec/ecx_meth.c
$(OPENSSL_PATH)/crypto/err/err.c
$(OPENSSL_PATH)/crypto/err/err_prn.c
$(OPENSSL_PATH)/crypto/evp/bio_b64.c
@@ -384,6 +421,10 @@
$(OPENSSL_PATH)/crypto/siphash/siphash.c
$(OPENSSL_PATH)/crypto/siphash/siphash_ameth.c
$(OPENSSL_PATH)/crypto/siphash/siphash_pmeth.c
+ $(OPENSSL_PATH)/crypto/sm2/sm2_crypt.c
+ $(OPENSSL_PATH)/crypto/sm2/sm2_err.c
+ $(OPENSSL_PATH)/crypto/sm2/sm2_pmeth.c
+ $(OPENSSL_PATH)/crypto/sm2/sm2_sign.c
$(OPENSSL_PATH)/crypto/sm3/m_sm3.c
$(OPENSSL_PATH)/crypto/sm3/sm3.c
$(OPENSSL_PATH)/crypto/sm4/sm4.c
@@ -496,6 +537,15 @@
$(OPENSSL_PATH)/crypto/conf/conf_local.h
$(OPENSSL_PATH)/crypto/dh/dh_local.h
$(OPENSSL_PATH)/crypto/dso/dso_local.h
+ $(OPENSSL_PATH)/crypto/ec/ec_local.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/curve448_local.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/curve448utils.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/ed448.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/field.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/point_448.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/word.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/arch_32/arch_intrinsics.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/arch_32/f_impl.h
$(OPENSSL_PATH)/crypto/evp/evp_local.h
$(OPENSSL_PATH)/crypto/hmac/hmac_local.h
$(OPENSSL_PATH)/crypto/lhash/lhash_local.h
diff --git a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
index cdeed0d073..38ccf1a5b6 100644
--- a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
+++ b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
@@ -199,6 +199,43 @@
$(OPENSSL_PATH)/crypto/dso/dso_vms.c
$(OPENSSL_PATH)/crypto/dso/dso_win32.c
$(OPENSSL_PATH)/crypto/ebcdic.c
+ $(OPENSSL_PATH)/crypto/ec/curve25519.c
+ $(OPENSSL_PATH)/crypto/ec/curve448/arch_32/f_impl.c
+ $(OPENSSL_PATH)/crypto/ec/curve448/curve448.c
+ $(OPENSSL_PATH)/crypto/ec/curve448/curve448_tables.c
+ $(OPENSSL_PATH)/crypto/ec/curve448/eddsa.c
+ $(OPENSSL_PATH)/crypto/ec/curve448/f_generic.c
+ $(OPENSSL_PATH)/crypto/ec/curve448/scalar.c
+ $(OPENSSL_PATH)/crypto/ec/ec2_oct.c
+ $(OPENSSL_PATH)/crypto/ec/ec2_smpl.c
+ $(OPENSSL_PATH)/crypto/ec/ec_ameth.c
+ $(OPENSSL_PATH)/crypto/ec/ec_asn1.c
+ $(OPENSSL_PATH)/crypto/ec/ec_check.c
+ $(OPENSSL_PATH)/crypto/ec/ec_curve.c
+ $(OPENSSL_PATH)/crypto/ec/ec_cvt.c
+ $(OPENSSL_PATH)/crypto/ec/ec_err.c
+ $(OPENSSL_PATH)/crypto/ec/ec_key.c
+ $(OPENSSL_PATH)/crypto/ec/ec_kmeth.c
+ $(OPENSSL_PATH)/crypto/ec/ec_lib.c
+ $(OPENSSL_PATH)/crypto/ec/ec_mult.c
+ $(OPENSSL_PATH)/crypto/ec/ec_oct.c
+ $(OPENSSL_PATH)/crypto/ec/ec_pmeth.c
+ $(OPENSSL_PATH)/crypto/ec/ec_print.c
+ $(OPENSSL_PATH)/crypto/ec/ecdh_kdf.c
+ $(OPENSSL_PATH)/crypto/ec/ecdh_ossl.c
+ $(OPENSSL_PATH)/crypto/ec/ecdsa_ossl.c
+ $(OPENSSL_PATH)/crypto/ec/ecdsa_sign.c
+ $(OPENSSL_PATH)/crypto/ec/ecdsa_vrf.c
+ $(OPENSSL_PATH)/crypto/ec/eck_prn.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_mont.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_nist.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_nistp224.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_nistp256.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_nistp521.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_nistputil.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_oct.c
+ $(OPENSSL_PATH)/crypto/ec/ecp_smpl.c
+ $(OPENSSL_PATH)/crypto/ec/ecx_meth.c
$(OPENSSL_PATH)/crypto/err/err.c
$(OPENSSL_PATH)/crypto/err/err_prn.c
$(OPENSSL_PATH)/crypto/evp/bio_b64.c
@@ -384,6 +421,10 @@
$(OPENSSL_PATH)/crypto/siphash/siphash.c
$(OPENSSL_PATH)/crypto/siphash/siphash_ameth.c
$(OPENSSL_PATH)/crypto/siphash/siphash_pmeth.c
+ $(OPENSSL_PATH)/crypto/sm2/sm2_crypt.c
+ $(OPENSSL_PATH)/crypto/sm2/sm2_err.c
+ $(OPENSSL_PATH)/crypto/sm2/sm2_pmeth.c
+ $(OPENSSL_PATH)/crypto/sm2/sm2_sign.c
$(OPENSSL_PATH)/crypto/sm3/m_sm3.c
$(OPENSSL_PATH)/crypto/sm3/sm3.c
$(OPENSSL_PATH)/crypto/sm4/sm4.c
@@ -496,6 +537,15 @@
$(OPENSSL_PATH)/crypto/conf/conf_local.h
$(OPENSSL_PATH)/crypto/dh/dh_local.h
$(OPENSSL_PATH)/crypto/dso/dso_local.h
+ $(OPENSSL_PATH)/crypto/ec/ec_local.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/curve448_local.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/curve448utils.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/ed448.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/field.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/point_448.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/word.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/arch_32/arch_intrinsics.h
+ $(OPENSSL_PATH)/crypto/ec/curve448/arch_32/f_impl.h
$(OPENSSL_PATH)/crypto/evp/evp_local.h
$(OPENSSL_PATH)/crypto/hmac/hmac_local.h
$(OPENSSL_PATH)/crypto/lhash/lhash_local.h
diff --git a/CryptoPkg/Library/OpensslLib/process_files.pl
b/CryptoPkg/Library/OpensslLib/process_files.pl
index 42bff05fa6..2ebfbbbca0 100755
--- a/CryptoPkg/Library/OpensslLib/process_files.pl
+++ b/CryptoPkg/Library/OpensslLib/process_files.pl
@@ -169,7 +169,6 @@ BEGIN {
"no-dgram",
"no-dsa",
"no-dynamic-engine",
- "no-ec",
"no-ec2m",
"no-engine",
"no-err",
--
2.17.1


Re: [PATCH] UserAuthFeaturePkg/UserAuthenticationDxeSmm: The SMI to handle the user authentication should be unregister before booting to OS

Dandan Bi
 

Patch is submitted via commit 23ca68c23dd600973e961de4368abacf4db8c5c0
https://github.com/tianocore/edk2-platforms/commit/23ca68c23dd600973e961de4368abacf4db8c5c0



Thanks,
Dandan

-----Original Message-----
From: Bi, Dandan
Sent: Friday, October 8, 2021 9:44 AM
To: Shi, Hao <hao.shi@intel.com>; devel@edk2.groups.io
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Subject: RE: [PATCH] UserAuthFeaturePkg/UserAuthenticationDxeSmm: The
SMI to handle the user authentication should be unregister before booting to OS

Reviewed-by: Dandan Bi <dandan.bi@intel.com>



Thanks,
Dandan

-----Original Message-----
From: Shi, Hao <hao.shi@intel.com>
Sent: Tuesday, September 28, 2021 10:09 AM
To: devel@edk2.groups.io
Cc: Shi, Hao <hao.shi@intel.com>; Bi, Dandan <dandan.bi@intel.com>;
Liming Gao <gaoliming@byosoft.com.cn>
Subject: [PATCH] UserAuthFeaturePkg/UserAuthenticationDxeSmm: The SMI
to handle the user authentication should be unregister before booting
to OS

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

Register SmmExitBootServices and SmmLegacyBoot callback function to
unregister this handler.

Signed-off-by: Hao Shi <hao.shi@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
---
.../UserAuthenticationSmm.c | 39 +++++++++++++++++--
.../UserAuthenticationSmm.inf | 2 +
2 files changed, 38 insertions(+), 3 deletions(-)

diff --git
a/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDx
eSmm/UserAuthenticationSmm.c
b/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDx
eSmm/UserAuthenticationSmm.c
index 07e834eb..3d66010b 100644
---
a/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDx
eSmm/UserAuthenticationSmm.c
+++
b/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthentication
+++ DxeSmm/UserAuthenticationSmm.c
@@ -13,6 +13,7 @@ UINTN mAdminPasswordTryCount = 0;

BOOLEAN mNeedReVerify = TRUE;
BOOLEAN mPasswordVerified = FALSE;
+EFI_HANDLE mSmmHandle = NULL;

/**
Verify if the password is correct.
@@ -612,6 +613,30 @@ EXIT:
return EFI_SUCCESS;
}

+/**
+ Performs Exit Boot Services UserAuthentication actions
+
+ @param[in] Protocol Points to the protocol's unique identifier.
+ @param[in] Interface Points to the interface instance.
+ @param[in] Handle The handle on which the interface was installed.
+
+ @retval EFI_SUCCESS Notification runs successfully.
+**/
+EFI_STATUS
+EFIAPI
+UaExitBootServices (
+ IN CONST EFI_GUID *Protocol,
+ IN VOID *Interface,
+ IN EFI_HANDLE Handle
+ )
+{
+ DEBUG ((DEBUG_INFO, "Unregister User Authentication Smi\n"));
+
+ gSmst->SmiHandlerUnRegister(mSmmHandle);
+
+ return EFI_SUCCESS;
+}
+
/**
Main entry for this driver.

@@ -629,10 +654,11 @@ PasswordSmmInit (
)
{
EFI_STATUS Status;
- EFI_HANDLE SmmHandle;
EDKII_VARIABLE_LOCK_PROTOCOL *VariableLock;
CHAR16
PasswordHistoryName[sizeof(USER_AUTHENTICATION_VAR_NAME)/sizeof(
CHAR16) + 5];
UINTN Index;
+ EFI_EVENT ExitBootServicesEvent;
+ EFI_EVENT LegacyBootEvent;

ASSERT (PASSWORD_HASH_SIZE == SHA256_DIGEST_SIZE);
ASSERT (PASSWORD_HISTORY_CHECK_COUNT < 0xFFFF); @@ -657,13
+683,20 @@ PasswordSmmInit (
ASSERT_EFI_ERROR (Status);
}

- SmmHandle = NULL;
- Status = gSmst->SmiHandlerRegister (SmmPasswordHandler,
&gUserAuthenticationGuid, &SmmHandle);
+ Status = gSmst->SmiHandlerRegister (SmmPasswordHandler,
+ &gUserAuthenticationGuid, &mSmmHandle);
ASSERT_EFI_ERROR (Status);
if (EFI_ERROR (Status)) {
return Status;
}

+ //
+ // Register for SmmExitBootServices and SmmLegacyBoot notification.
+ //
+ Status = gSmst->SmmRegisterProtocolNotify
+ (&gEdkiiSmmExitBootServicesProtocolGuid, UaExitBootServices,
+ &ExitBootServicesEvent); ASSERT_EFI_ERROR (Status); Status =
+ gSmst->SmmRegisterProtocolNotify (&gEdkiiSmmLegacyBootProtocolGuid,
+ UaExitBootServices, &LegacyBootEvent); ASSERT_EFI_ERROR (Status);
+
if (IsPasswordCleared()) {
DEBUG ((DEBUG_INFO, "IsPasswordCleared\n"));
SavePasswordToVariable (&gUserAuthenticationGuid, NULL, 0); diff
--git
a/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDx
eSmm/UserAuthenticationSmm.inf
b/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDx
eSmm/UserAuthenticationSmm.inf
index 0b33b194..d73a2fe2 100644
---
a/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDx
eSmm/UserAuthenticationSmm.inf
+++
b/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthentication
+++ DxeSmm/UserAuthenticationSmm.inf
@@ -48,6 +48,8 @@
[Protocols]
gEdkiiVariableLockProtocolGuid ## CONSUMES
gEfiSmmVariableProtocolGuid ## CONSUMES
+ gEdkiiSmmExitBootServicesProtocolGuid ## CONSUMES
+ gEdkiiSmmLegacyBootProtocolGuid ## CONSUMES

[Depex]
gEfiSmmVariableProtocolGuid AND gEfiVariableWriteArchProtocolGuid
--
2.31.1.windows.1


Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal

Jeff Fan
 

OVMF did a similare change on Time Driver, please refer to https://github.com/tianocore/edk2/commit/239b50a863704f7960525799eda82de061c7c458 

I do not think this will be apply for ArmPkg/TimerDxe. 

If one real issue happened on platform, it seems that interrupt was reenabled by reigstered timer event functions.

Jeff

 
Date: 2021-10-12 23:38
Subject: Re: [edk2-devel] [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal
+ Shaker

Get Outlook for iOS

From: Ashish Singhal <ashishsingha@...>
Sent: Tuesday, October 12, 2021 8:56:58 AM
To: Marc Zyngier <maz@...>; Ard Biesheuvel <ardb@...>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@...>; Ard Biesheuvel <ardb+tianocore@...>
Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal
 
Marc,

In the document ARM062-1010708621-30 (AArch64 Programmer's Guides Generic Timer), towards the end of section 3.4 it says: "When writing an interrupt handler for the timers, it is important that software clears the interrupt before deactivating the interrupt in the GIC. Otherwise, the GIC will re-signal the same interrupt again."

My change was in accordance with this. We only clear the interrupt when we update the compare value but were signaling EOI before that going against the guidance in the document.

Thanks
Ashish

From: Marc Zyngier <maz@...>
Sent: Tuesday, October 12, 2021 2:57 AM
To: Ard Biesheuvel <ardb@...>; Ashish Singhal <ashishsingha@...>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@...>; Ard Biesheuvel <ardb+tianocore@...>
Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal
 
External email: Use caution opening links or attachments


On Mon, 11 Oct 2021 23:24:38 +0100,
Ard Biesheuvel <ardb@...> wrote:
>
> (+ Marc)
>
> On Mon, 11 Oct 2021 at 23:40, Ashish Singhal <ashishsingha@...> wrote:
> >
> > In an interrupt handler for the timers, it is important that
> > software clears the interrupt before deactivating the interrupt
> > in the GIC. Otherwise the GIC will re-signal the same interrupt
> > again.
> >
> > Signed-off-by: Ashish Singhal <ashishsingha@...>
>
> Please don't spam us with almost identical versions of the same patch
> without even documenting what the difference is.
>
>
> > ---
> >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > index 0370620fae..46e5bbf287 100644
> > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > @@ -300,10 +300,6 @@ TimerInterruptHandler (
> >    //
> >    OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> >
> > -  // Signal end of interrupt early to help avoid losing subsequent ticks
> > -  // from long duration handlers
> > -  gInterrupt->EndOfInterrupt (gInterrupt, Source);
> > -
> >    // Check if the timer interrupt is active
> >    if ((ArmGenericTimerGetTimerCtrlReg () ) & ARM_ARCH_TIMER_ISTATUS) {
> >
> > @@ -335,6 +331,11 @@ TimerInterruptHandler (
> >      ArmInstructionSynchronizationBarrier ();
> >    }
> >
> > +  // In an interrupt handler for the timers, it is important that software clears the interrupt
> > +  // before deactivating the interrupt in the GIC. Otherwise the GIC will re-signal the same
> > +  // interrupt again.
> > +  gInterrupt->EndOfInterrupt (gInterrupt, Source);
> > +
> >    gBS->RestoreTPL (OriginalTPL);
> >  }
> >

This isn't a requirement if you haven't re-enabled interrupts in
PSTATE (and the TPL being raised seems to indicate that interrupts are
actively masked here).

The timer is a level interrupt, and lowering the level takes time. If
your GIC implementation is good, it will notice that the lowering of
the level quickly, before you can reach the point where you re-enable
interrupts. If it is slow (or badly emulated if this is actually done
in a hypervisor), you'll get a spurious interrupt.

In any case, moving the EOI around doesn't make things any better. You
are just moving the goal post, without additional guarantees that the
level has been retired.

As a consequence, I don't think this patch makes much sense.

        M.

--
Without deviation from the norm, progress is not possible.


Re: [PATCH EDK2 v1 1/1] EmbeddedPkg:Fix compiler warning

Abner Chang
 

Acked-by: Abner Chang <abner.chang@hpe.com>

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
wenyi,xie via groups.io
Sent: Tuesday, October 12, 2021 4:16 PM
To: devel@edk2.groups.io; leif@nuviainc.com; ardb+tianocore@kernel.org;
Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
Schaefer, Daniel <daniel.schaefer@hpe.com>
Cc: songdongkuang@huawei.com; xiewenyi2@huawei.com
Subject: [edk2-devel] [PATCH EDK2 v1 1/1] EmbeddedPkg:Fix compiler
warning

Fixes the following compiler warning in VS2019.

edk2\EmbeddedPkg\Library\GdbSerialDebugPortLib\GdbSerialDebugPortLib
.c(127):
error C2220: the following warning is treated as an error
edk2\EmbeddedPkg\Library\GdbSerialDebugPortLib\GdbSerialDebugPortLib
.c(127):
warning C4244: 'function': conversion from 'UINTN' to 'UINT32', possible
loss of data

edk2\EmbeddedPkg\Library\PrePiLib\FwVol.c(347): error C2220: the
following
warning is treated as an error
edk2\EmbeddedPkg\Library\PrePiLib\FwVol.c(347): warning C4244:
'function':
conversion from 'UINTN' to 'UINT32', possible loss of data

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Abner Chang <abner.chang@hpe.com>
Cc: Daniel Schaefer <daniel.schaefer@hpe.com>
Signed-off-by: Wenyi Xie <xiewenyi2@huawei.com>
---
EmbeddedPkg/Library/GdbSerialDebugPortLib/GdbSerialDebugPortLib.c | 2
+-
EmbeddedPkg/Library/PrePiLib/FwVol.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git
a/EmbeddedPkg/Library/GdbSerialDebugPortLib/GdbSerialDebugPortLib.c
b/EmbeddedPkg/Library/GdbSerialDebugPortLib/GdbSerialDebugPortLib.c
index d2bafcf69b60..0f50a8b64191 100644
---
a/EmbeddedPkg/Library/GdbSerialDebugPortLib/GdbSerialDebugPortLib.c
+++
b/EmbeddedPkg/Library/GdbSerialDebugPortLib/GdbSerialDebugPortLib.c
@@ -18,7 +18,7 @@


EFI_DEBUGPORT_PROTOCOL *gDebugPort = NULL;
-UINTN gTimeOut = 0;
+UINT32 gTimeOut = 0;

/**
The constructor function initializes the UART.
diff --git a/EmbeddedPkg/Library/PrePiLib/FwVol.c
b/EmbeddedPkg/Library/PrePiLib/FwVol.c
index 881506edddaf..46ea5f733f60 100644
--- a/EmbeddedPkg/Library/PrePiLib/FwVol.c
+++ b/EmbeddedPkg/Library/PrePiLib/FwVol.c
@@ -298,7 +298,7 @@ FfsProcessSection (
UINT16 SectionAttribute;
UINT32 AuthenticationStatus;
CHAR8 *CompressedData;
- UINTN CompressedDataLength;
+ UINT32 CompressedDataLength;


*OutputBuffer = NULL;
--
2.20.1.windows.1





Re: [PATCH v2] CryptoPkg/BaseCryptLib: Eliminate extra buffer copy in Pkcs7Verify()

Yao, Jiewen
 

-----Original Message-----
From: Yao, Jiewen
Sent: Saturday, September 11, 2021 11:30 PM
To: Bob Morgan <bobm@nvidia.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 v2] CryptoPkg/BaseCryptLib: Eliminate extra buffer copy in
Pkcs7Verify()

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

-----Original Message-----
From: Bob Morgan <bobm@nvidia.com>
Sent: Saturday, September 11, 2021 5:34 AM
To: devel@edk2.groups.io
Cc: Bob Morgan <bobm@nvidia.com>; 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>
Subject: [PATCH v2] CryptoPkg/BaseCryptLib: Eliminate extra buffer copy in
Pkcs7Verify()

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

Create a read-only openSSL BIO wrapper for the existing input
buffer passed to Pkcs7Verify() instead of copying the buffer
into an empty writable BIO which causes memory allocations
within openSSL.

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: Bob Morgan <bobm@nvidia.com>
---
CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c
index d99597d181..8eda98f7b2 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c
@@ -864,15 +864,11 @@ Pkcs7Verify (
// For generic PKCS#7 handling, InData may be NULL if the content is present
// in PKCS#7 structure. So ignore NULL checking here.
//
- DataBio = BIO_new (BIO_s_mem ());
+ DataBio = BIO_new_mem_buf (InData, (int) DataLength);
if (DataBio == NULL) {
goto _Exit;
}

- if (BIO_write (DataBio, InData, (int) DataLength) <= 0) {
- goto _Exit;
- }
-
//
// Allow partial certificate chains, terminated by a non-self-signed but
// still trusted intermediate certificate. Also disable time checks.
--
2.17.1


Event: TianoCore Bug Triage - APAC / NAMO - 10/12/2021 #cal-reminder

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

Reminder: TianoCore Bug Triage - APAC / NAMO

When:
10/12/2021
6:30pm to 7:30pm
(UTC-07:00) America/Los Angeles

Where:
https://teams.microsoft.com/l/meetup-join/19%3ameeting_OTUyZTg2NjgtNDhlNS00ODVlLTllYTUtYzg1OTNjNjdiZjFh%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%22b286b53a-1218-4db3-bfc9-3d4c5aa7669e%22%7d

Organizer: Liming Gao gaoliming@...

View Event

Description:

TianoCore Bug Triage - APAC / NAMO

Hosted by Liming Gao

 

________________________________________________________________________________

Microsoft Teams meeting

Join on your computer or mobile app

Click here to join the meeting

Join with a video conferencing device

teams@...

Video Conference ID: 116 062 094 0

Alternate VTC dialing instructions

Or call in (audio only)

+1 916-245-6934,,77463821#   United States, Sacramento

Phone Conference ID: 774 638 21#

Find a local number | Reset PIN

Learn More | Meeting options


回复: [PATCH v9 1/1] MdePkg/BaseLib: Add QuickSort function on BaseLib

gaoliming
 

I have no other comments. Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

-----邮件原件-----
发件人: Kuo, IanX <ianx.kuo@intel.com>
发送时间: 2021年10月12日 11:51
收件人: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Kinney, Michael
D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>
抄送: Chan, Amy <amy.chan@intel.com>; Liu, Zhiguang
<zhiguang.liu@intel.com>
主题: RE: [PATCH v9 1/1] MdePkg/BaseLib: Add QuickSort function on
BaseLib

@Liming Gao and @Kinney, Michael D

May I get one of yours help for the reviewed from MdePkg maintainer side ?
Have any concern, please also share for me.

Thanks,
Ian Kuo

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, October 12, 2021 10:22 AM
To: Kuo, IanX <ianx.kuo@intel.com>; devel@edk2.groups.io
Cc: Chan, Amy <amy.chan@intel.com>; Kinney, Michael D
<michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Liu,
Zhiguang <zhiguang.liu@intel.com>
Subject: RE: [PATCH v9 1/1] MdePkg/BaseLib: Add QuickSort function on
BaseLib

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

Ian, please take the approval from maintainers of MdePkg as the formal
approval.

Thanks,
Ray

-----Original Message-----
From: Kuo, IanX <ianx.kuo@intel.com>
Sent: Tuesday, October 12, 2021 8:06 AM
To: devel@edk2.groups.io
Cc: Chan, Amy <amy.chan@intel.com>; Ni, Ray <ray.ni@intel.com>; Kuo,
IanX <ianx.kuo@intel.com>; Kinney, Michael D
<michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
Liu, Zhiguang <zhiguang.liu@intel.com>
Subject: [PATCH v9 1/1] MdePkg/BaseLib: Add QuickSort function on
BaseLib

From: IanX Kuo <ianx.kuo@intel.com>

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

Add QuickSort function into BaseLib

Cc: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Signed-off-by: IanX Kuo <ianx.kuo@intel.com>
---
MdePkg/Include/Library/BaseLib.h | 49 ++++++++
MdePkg/Library/BaseLib/BaseLib.inf | 1 +
MdePkg/Library/BaseLib/QuickSort.c | 116
++++++++++++++++++
.../Library/BaseLib/UnitTestHostBaseLib.inf | 3 +-
4 files changed, 168 insertions(+), 1 deletion(-) create mode 100644
MdePkg/Library/BaseLib/QuickSort.c

diff --git a/MdePkg/Include/Library/BaseLib.h
b/MdePkg/Include/Library/BaseLib.h
index 2452c1d92e..0ae0f4e6af 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -2856,6 +2856,55 @@ RemoveEntryList ( //

// Math Services

//

+/**

+ Prototype for comparison function for any two element types.

+

+ @param[in] Buffer1 The pointer to first buffer.

+ @param[in] Buffer2 The pointer to second buffer.

+

+ @retval 0 Buffer1 equal to Buffer2.

+ @return <0 Buffer1 is less than Buffer2.

+ @return >0 Buffer1 is greater than
Buffer2.

+**/

+typedef

+INTN

+(EFIAPI *BASE_SORT_COMPARE)(

+ IN CONST VOID *Buffer1,

+ IN CONST VOID *Buffer2

+ );

+

+/**

+ This function is identical to perform QuickSort,

+ except that is uses the pre-allocated buffer so the in place
+ sorting does not need to

+ allocate and free buffers constantly.

+

+ Each element must be equal sized.

+

+ if BufferToSort is NULL, then ASSERT.

+ if CompareFunction is NULL, then ASSERT.

+ if BufferOneElement is NULL, then ASSERT.

+ if ElementSize is < 1, then ASSERT.

+

+ if Count is < 2 then perform no action.

+

+ @param[in, out] BufferToSort on call a Buffer of (possibly sorted)
elements

+ on return a buffer of sorted
+ elements

+ @param[in] Count the number of elements in the
buffer to sort

+ @param[in] ElementSize Size of an element in bytes

+ @param[in] CompareFunction The function to call to perform the
comparison

+ of any 2 elements

+ @param[out] BufferOneElement Caller provided buffer whose size
equals to ElementSize.

+ It's used by QuickSort() for
swapping in sorting.

+**/

+VOID

+EFIAPI

+QuickSort (

+ IN OUT VOID *BufferToSort,

+ IN CONST UINTN Count,

+ IN CONST UINTN ElementSize,

+ IN BASE_SORT_COMPARE CompareFunction,

+ OUT VOID *BufferOneElement

+ );



/**

Shifts a 64-bit integer left between 0 and 63 bits. The low bits
are filled

diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
b/MdePkg/Library/BaseLib/BaseLib.inf
index 6efa5315b6..cebda3b210 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -32,6 +32,7 @@
SwapBytes16.c

LongJump.c

SetJump.c

+ QuickSort.c

RShiftU64.c

RRotU64.c

RRotU32.c

diff --git a/MdePkg/Library/BaseLib/QuickSort.c
b/MdePkg/Library/BaseLib/QuickSort.c
new file mode 100644
index 0000000000..a825c072b0
--- /dev/null
+++ b/MdePkg/Library/BaseLib/QuickSort.c
@@ -0,0 +1,116 @@
+/** @file

+ Math worker functions.

+

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

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

+

+**/

+

+#include "BaseLibInternals.h"

+

+/**

+ This function is identical to perform QuickSort,

+ except that is uses the pre-allocated buffer so the in place
+ sorting does not need to

+ allocate and free buffers constantly.

+

+ Each element must be equal sized.

+

+ if BufferToSort is NULL, then ASSERT.

+ if CompareFunction is NULL, then ASSERT.

+ if BufferOneElement is NULL, then ASSERT.

+ if ElementSize is < 1, then ASSERT.

+

+ if Count is < 2 then perform no action.

+

+ @param[in, out] BufferToSort on call a Buffer of (possibly sorted)
elements

+ on return a buffer of sorted
+ elements

+ @param[in] Count the number of elements in the
buffer to sort

+ @param[in] ElementSize Size of an element in bytes

+ @param[in] CompareFunction The function to call to perform the
comparison

+ of any 2 elements

+ @param[out] BufferOneElement Caller provided buffer whose size
equals to ElementSize.

+ It's used by QuickSort() for
swapping in sorting.

+**/

+VOID

+EFIAPI

+QuickSort (

+ IN OUT VOID *BufferToSort,

+ IN CONST UINTN Count,

+ IN CONST UINTN ElementSize,

+ IN BASE_SORT_COMPARE CompareFunction,

+ OUT VOID *BufferOneElement

+ )

+{

+ VOID *Pivot;

+ UINTN LoopCount;

+ UINTN NextSwapLocation;

+

+ ASSERT (BufferToSort != NULL);

+ ASSERT (CompareFunction != NULL);

+ ASSERT (BufferOneElement != NULL);

+ ASSERT (ElementSize >= 1);

+

+ if (Count < 2) {

+ return;

+ }

+

+ NextSwapLocation = 0;

+

+ //

+ // pick a pivot (we choose last element)

+ //

+ Pivot = ((UINT8*) BufferToSort + ((Count - 1) * ElementSize));

+

+ //

+ // Now get the pivot such that all on "left" are below it

+ // and everything "right" are above it

+ //

+ for (LoopCount = 0; LoopCount < Count -1; LoopCount++) {

+ //

+ // if the element is less than or equal to the pivot

+ //

+ if (CompareFunction ((VOID*) ((UINT8*) BufferToSort +
+ ((LoopCount) * ElementSize)), Pivot) <= 0){

+ //

+ // swap

+ //

+ CopyMem (BufferOneElement, (UINT8*) BufferToSort +
+ (NextSwapLocation * ElementSize), ElementSize);

+ CopyMem ((UINT8*) BufferToSort + (NextSwapLocation *
+ ElementSize), (UINT8*) BufferToSort + ((LoopCount) *
ElementSize), ElementSize);

+ CopyMem ((UINT8*) BufferToSort + ((LoopCount)*ElementSize),
+ BufferOneElement, ElementSize);

+

+ //

+ // increment NextSwapLocation

+ //

+ NextSwapLocation++;

+ }

+ }

+ //

+ // swap pivot to it's final position (NextSwapLocation)

+ //

+ CopyMem (BufferOneElement, Pivot, ElementSize);

+ CopyMem (Pivot, (UINT8*) BufferToSort + (NextSwapLocation *
+ ElementSize), ElementSize);

+ CopyMem ((UINT8*) BufferToSort + (NextSwapLocation * ElementSize),
+ BufferOneElement, ElementSize);

+

+ //

+ // Now recurse on 2 partial lists. neither of these will have the
+ 'pivot' element

+ // IE list is sorted left half, pivot element, sorted right half...

+ //

+ if (NextSwapLocation >= 2) {

+ QuickSort (

+ BufferToSort,

+ NextSwapLocation,

+ ElementSize,

+ CompareFunction,

+ BufferOneElement

+ );

+ }

+

+ if ((Count - NextSwapLocation - 1) >= 2) {

+ QuickSort (

+ (UINT8 *)BufferToSort + (NextSwapLocation + 1) * ElementSize,

+ Count - NextSwapLocation - 1,

+ ElementSize,

+ CompareFunction,

+ BufferOneElement

+ );

+ }

+}

diff --git a/MdePkg/Library/BaseLib/UnitTestHostBaseLib.inf
b/MdePkg/Library/BaseLib/UnitTestHostBaseLib.inf
index eae1a7158d..d09bd12bef 100644
--- a/MdePkg/Library/BaseLib/UnitTestHostBaseLib.inf
+++ b/MdePkg/Library/BaseLib/UnitTestHostBaseLib.inf
@@ -1,7 +1,7 @@
## @file

# Base Library implementation for use with host based unit tests.

#

-# Copyright (c) 2007 - 2020, Intel Corporation. All rights
reserved.<BR>

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

# Portions copyright (c) 2008 - 2009, Apple Inc. All rights
reserved.<BR>

# Portions copyright (c) 2011 - 2013, ARM Ltd. All rights
reserved.<BR>

# Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All
rights reserved.<BR>

@@ -33,6 +33,7 @@
SwapBytes16.c

LongJump.c

SetJump.c

+ QuickSort.c

RShiftU64.c

RRotU64.c

RRotU32.c

--
2.30.0.windows.1


Re: [PATCH V9 2/4] OvmfPkg: Clear WORK_AREA_GUEST_TYPE in Main.asm

Min Xu
 

On October 12, 2021 9:23 PM, Lendacky Thomas wrote:
On 10/11/21 9:37 PM, Min Xu wrote:
diff --git a/OvmfPkg/ResetVector/Main.asm
b/OvmfPkg/ResetVector/Main.asm index ae90a148fce7..a501fbe880f2
100644
--- a/OvmfPkg/ResetVector/Main.asm
+++ b/OvmfPkg/ResetVector/Main.asm
@@ -36,6 +36,14 @@ Main16:

BITS 32

+%ifdef ARCH_X64
A regular SEV guest can be built in the hybrid IA32 and X64 configuration, so this
will break existing SEV firmwares built in that manner. Only SEV-ES and SEV-SNP
require the full X64 confguration.
WORK_AREA_GUEST_TYPE is defined in https://github.com/tianocore/edk2/blob/master/OvmfPkg/ResetVector/ResetVector.nasmb#L75 and previously it was cleared in https://github.com/tianocore/edk2/blob/master/OvmfPkg/ResetVector/Ia32/PageTables64.asm#L47. These 2 lines are both surrounded by ARCH_X64.
So in this commit, the clearance of WORK_AREA_GUEST_TYPE in Main.asm is surrounded by ARCH_X64 too.
Brijesh, what's your comments on this change?

Thanks!
Min


Re: [PATCH V9 1/4] OvmfPkg: Copy Main.asm from UefiCpuPkg to OvmfPkg's ResetVector

Min Xu
 

On October 12, 2021 3:02 PM, Gerd Hoffmann wrote:
On Tue, Oct 12, 2021 at 10:37:47AM +0800, Min Xu wrote:
RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

Previously OvmfPkg/ResetVector uses the Main.asm in
UefiCpuPkg/ReseteVector/Vtf0. In this Main.asm there is only Main16
entry point.

This patch-set is to introduce Intel TDX into Ovmf. Main32 entry point
is needed in Main.asm by Intel TDX. To reduce the complexity of
Main.asm in UefiCpuPkg, OvmfPkg create its own Main.asm to meet the
requirement of Intel TDX.

UefiCpuPkg/ResetVector/Vtf0/main.asm ->
OvmfPkg/ResetVector/Main.asm

You should mention that this is an unmodified copy (so no functional
change) and the actual changes for tdx come as incremental patches.
Thanks for reminder. It will be updated in next version.

With that:
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Thanks!
Min


Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal

Ashish Singhal
 

From: Ashish Singhal <ashishsingha@nvidia.com>
Sent: Tuesday, October 12, 2021 10:32 AM
To: Marc Zyngier <maz@kernel.org>
Cc: Shanker Donthineni <sdonthineni@nvidia.com>; Ard Biesheuvel <ardb@kernel.org>; edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal
 



From: Marc Zyngier <maz@kernel.org>
Sent: Tuesday, October 12, 2021 10:27 AM
To: Ashish Singhal <ashishsingha@nvidia.com>
Cc: Shanker Donthineni <sdonthineni@nvidia.com>; Ard Biesheuvel <ardb@kernel.org>; edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal
 
External email: Use caution opening links or attachments


On Tue, 12 Oct 2021 17:11:36 +0100,
Ashish Singhal <ashishsingha@nvidia.com> wrote:

Marc,

What do you suggest should be the proper fix for getting timer
interrupts even when ISTATUS bit is not set? Should we ignore them
the way it is in current implementation? I am OK to file a bug for
this if you think that is a better way to discuss this.
I don't think there is anything to fix.

Yes, the order in EDKII is odd. No, changing the order doesn't give
any extra guarantee. Spurious interrupts can always happen. Broken (or
slow) HW and bad emulation are more susceptible to it.

Now, how often do you see that? On which HW?

        M.

--
Without deviation from the norm, progress is not possible.

Marc,

We see at least one spurious interrupt after every valid timer interrupt. While both valid and spurious interrupt has the correct source, spurious interrupt does not have ISTATUS bit set. We are seeing this on Silicon and not on the emulation platform. Delaying EOI signal to GIC does take the spurious interrupt out as with the new flow we clear the interrupt before signaling EOI so that next time only a valid interrupt can be triggered and not the old interrupt which was still not cleared while signaling EOI to GIC.

Thanks
Ashish

Thanks
Ashish

Marc,

I can confirm that with the current code on edk2, we get 1 spurious interrupt for every 1 valid interrupt from GIC. With the change I proposed, we do not get the spurious interrupt at all.

Thanks
Ashish


[PATCH 1/1] SecurityPkg/DxeImageVerificationLib: Set Action for failed signed image

Joseph Hemann
 

If the image is signed but not allowed by DB and the hash of
image is not found in DB/DBX, then the EFI_IMAGE_INFO_ACTION
of the load of said image should be set to,
EFI_IMAGE_EXECUTION_AUTH_SIG_NOT_FOUND, rather then being left
unset as EFI_IMAGE_EXECUTION_AUTH_UNTESTED.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Min Xu <min.m.xu@intel.com>

Signed-off-by: Joseph Hemann <joseph.hemann@arm.com>
Change-Id: I6b2777bd7aeb57773b8876e44c2179ea7501bc8c
---
.../Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index c48861cd6496..0a804af2162f 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1957,6 +1957,7 @@ DxeImageVerificationHandler (
if (!EFI_ERROR (DbStatus) && IsFound) {
IsVerified = TRUE;
} else {
+ Action = EFI_IMAGE_EXECUTION_AUTH_SIG_NOT_FOUND;
DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but signature is not allowed by DB and %s hash of image is not found in DB/DBX.\n", mHashTypeStr));
}
}
--
2.17.1


[PATCH 1/1] SecurityPkg/DxeImageVerificationLib: Set Action for failed unsigned image

Joseph Hemann
 

If the image is not signed and the hash of image is not found
in DB/DBX, then the EFI_IMAGE_INFO_ACTION of the load of said
image should be set to,
EFI_IMAGE_EXECUTION_AUTH_SIG_NOT_FOUND, rather then being left
unset as EFI_IMAGE_EXECUTION_AUTH_UNTESTED.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Min Xu <min.m.xu@intel.com>

Signed-off-by: Joseph Hemann <Joseph.Hemann@arm.com>
Change-Id: Ia432ebf4ec811e36d67b80bc438a6aff60bc9b67
---
.../Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 0a804af2162f..e5fae732bb1f 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1848,6 +1848,7 @@ DxeImageVerificationHandler (
//
// Image Hash is not found in both forbidden and allowed database.
//
+ Action = EFI_IMAGE_EXECUTION_AUTH_SIG_NOT_FOUND;
DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed and %s hash of image is not found in DB/DBX.\n", mHashTypeStr));
goto Failed;
}
--
2.17.1


Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal

Ashish Singhal
 

From: Marc Zyngier <maz@kernel.org>
Sent: Tuesday, October 12, 2021 10:27 AM
To: Ashish Singhal <ashishsingha@nvidia.com>
Cc: Shanker Donthineni <sdonthineni@nvidia.com>; Ard Biesheuvel <ardb@kernel.org>; edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal
 
External email: Use caution opening links or attachments


On Tue, 12 Oct 2021 17:11:36 +0100,
Ashish Singhal <ashishsingha@nvidia.com> wrote:

Marc,

What do you suggest should be the proper fix for getting timer
interrupts even when ISTATUS bit is not set? Should we ignore them
the way it is in current implementation? I am OK to file a bug for
this if you think that is a better way to discuss this.
I don't think there is anything to fix.

Yes, the order in EDKII is odd. No, changing the order doesn't give
any extra guarantee. Spurious interrupts can always happen. Broken (or
slow) HW and bad emulation are more susceptible to it.

Now, how often do you see that? On which HW?

        M.

--
Without deviation from the norm, progress is not possible.

Marc,

We see at least one spurious interrupt after every valid timer interrupt. While both valid and spurious interrupt has the correct source, spurious interrupt does not have ISTATUS bit set. We are seeing this on Silicon and not on the emulation platform. Delaying EOI signal to GIC does take the spurious interrupt out as with the new flow we clear the interrupt before signaling EOI so that next time only a valid interrupt can be triggered and not the old interrupt which was still not cleared while signaling EOI to GIC.

Thanks
Ashish

Thanks
Ashish


Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal

Ashish Singhal
 

From: Marc Zyngier <maz@kernel.org>
Sent: Tuesday, October 12, 2021 9:44 AM
To: Ashish Singhal <ashishsingha@nvidia.com>
Cc: Ard Biesheuvel <ardb@kernel.org>; edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal
 
External email: Use caution opening links or attachments


Ashish,

Please don't top post, and please use plain text.

On Tue, 12 Oct 2021 15:56:58 +0100,
Ashish Singhal <ashishsingha@nvidia.com> wrote:

Marc,

In the document ARM062-1010708621-30 (AArch64 Programmer's Guides
Generic Timer), towards the end of section 3.4 it says: "When
writing an interrupt handler for the timers, it is important that
software clears the interrupt before deactivating the interrupt in
the GIC. Otherwise, the GIC will re-signal the same interrupt
again."
This document is a waste of valuable bits, unfortunately, and isn't an
architecture reference.

My change was in accordance with this. We only clear the interrupt
when we update the compare value but were signaling EOI before that
going against the guidance in the document.
There is no such requirement in the GIC architecture, as it makes no
guarantee on how much time it takes for a change of level to be
observed. Given that, this change is pretty much immaterial.

        M.

--
Without deviation from the norm, progress is not possible.

Marc,

What do you suggest should be the proper fix for getting timer interrupts even when ISTATUS bit is not set? Should we ignore them the way it is in current implementation? I am OK to file a bug for this if you think that is a better way to discuss this.

Shanker,

Any thoughts on this?

Thanks
Ashish


Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal

Ashish Singhal
 

+ Shaker

Get Outlook for iOS


From: Ashish Singhal <ashishsingha@...>
Sent: Tuesday, October 12, 2021 8:56:58 AM
To: Marc Zyngier <maz@...>; Ard Biesheuvel <ardb@...>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@...>; Ard Biesheuvel <ardb+tianocore@...>
Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal
 
Marc,

In the document ARM062-1010708621-30 (AArch64 Programmer's Guides Generic Timer), towards the end of section 3.4 it says: "When writing an interrupt handler for the timers, it is important that software clears the interrupt before deactivating the interrupt in the GIC. Otherwise, the GIC will re-signal the same interrupt again."

My change was in accordance with this. We only clear the interrupt when we update the compare value but were signaling EOI before that going against the guidance in the document.

Thanks
Ashish

From: Marc Zyngier <maz@...>
Sent: Tuesday, October 12, 2021 2:57 AM
To: Ard Biesheuvel <ardb@...>; Ashish Singhal <ashishsingha@...>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@...>; Ard Biesheuvel <ardb+tianocore@...>
Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal
 
External email: Use caution opening links or attachments


On Mon, 11 Oct 2021 23:24:38 +0100,
Ard Biesheuvel <ardb@...> wrote:
>
> (+ Marc)
>
> On Mon, 11 Oct 2021 at 23:40, Ashish Singhal <ashishsingha@...> wrote:
> >
> > In an interrupt handler for the timers, it is important that
> > software clears the interrupt before deactivating the interrupt
> > in the GIC. Otherwise the GIC will re-signal the same interrupt
> > again.
> >
> > Signed-off-by: Ashish Singhal <ashishsingha@...>
>
> Please don't spam us with almost identical versions of the same patch
> without even documenting what the difference is.
>
>
> > ---
> >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > index 0370620fae..46e5bbf287 100644
> > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > @@ -300,10 +300,6 @@ TimerInterruptHandler (
> >    //
> >    OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> >
> > -  // Signal end of interrupt early to help avoid losing subsequent ticks
> > -  // from long duration handlers
> > -  gInterrupt->EndOfInterrupt (gInterrupt, Source);
> > -
> >    // Check if the timer interrupt is active
> >    if ((ArmGenericTimerGetTimerCtrlReg () ) & ARM_ARCH_TIMER_ISTATUS) {
> >
> > @@ -335,6 +331,11 @@ TimerInterruptHandler (
> >      ArmInstructionSynchronizationBarrier ();
> >    }
> >
> > +  // In an interrupt handler for the timers, it is important that software clears the interrupt
> > +  // before deactivating the interrupt in the GIC. Otherwise the GIC will re-signal the same
> > +  // interrupt again.
> > +  gInterrupt->EndOfInterrupt (gInterrupt, Source);
> > +
> >    gBS->RestoreTPL (OriginalTPL);
> >  }
> >

This isn't a requirement if you haven't re-enabled interrupts in
PSTATE (and the TPL being raised seems to indicate that interrupts are
actively masked here).

The timer is a level interrupt, and lowering the level takes time. If
your GIC implementation is good, it will notice that the lowering of
the level quickly, before you can reach the point where you re-enable
interrupts. If it is slow (or badly emulated if this is actually done
in a hypervisor), you'll get a spurious interrupt.

In any case, moving the EOI around doesn't make things any better. You
are just moving the goal post, without additional guarantees that the
level has been retired.

As a consequence, I don't think this patch makes much sense.

        M.

--
Without deviation from the norm, progress is not possible.


Re: [PATCH V2 0/3] Introduce TdProtocol into EDK2

Sami Mujawar
 

Hi Min,

Thank you for this patch.

I think it would greatly help if the EFI_TD_PROTOCOL is changed to something more architecture neutral. As I understand, this patch series is removing the dependency on TPM for measurement and is instead providing a lightweight interface for extending measurements for Confidential Compute Architecture (CCA) guests.

Considering this, it would be good to generalise EFI_TD_PROTOCOL as a Confidential Compute Architecture Measurement (CCAM) protocol.
In fact, your v2 series demonstrates this need with the introduction of MEASURE_BOOT_PROTOCOLS in "[PATCH V2 2/3] SecurityPkg: Support TdProtocol in DxeTpm2MeasureBootLib [https://edk2.groups.io/g/devel/message/81651]".

As it stands, I feel most of the code can be reused/common. Some interfaces may need to use an architecture specific library, and some configuration options would need to be defined using PCDs.

Kindly let me know your thoughts.

Regards,

Sami Mujawar

On 08/10/2021, 06:24, "devel@edk2.groups.io on behalf of Min Xu via groups.io" <devel@edk2.groups.io on behalf of min.m.xu=intel.com@groups.io> wrote:

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

If TD-Guest firmware supports measurement and an event is created,
TD-Guest firmware is designed to report the event log with the same data
structure in TCG-Platform-Firmware-Profile specification with
EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format.

The TD-Guest firmware supports measurement, the TD Guest Firmware is
designed to produce EFI_TD_PROTOCOL with new GUID EFI_TD_PROTOCOL_GUID
to report event log and provides hash capability.

https://software.intel.com/content/dam/develop/external/us/en/documents/
intel-tdx-guest-hypervisor-communication-interface-1.0-344426-002.pdf
Section 4.3.2 includes the EFI_TD_PROTOCOL.

Patch #1:
Introduce the TD Protocol definition into MdePkg

Patch #2:
Update DxeTpm2MeasureBootLib to support TD based measure boot.

Patch #3:
Update DxeTpmMeasurementLib to support TD based measurement.

Code is at https://github.com/mxu9/edk2/tree/td_protocol.v2

v2 changes:
- TD based measure boot is implemented in DxeTpm2MeasureBootLib.
This minimize the code changes.
- TD based measurement is added. It is implemented in
DxeTpmMeasurementLib.
- Fix the typo in comments.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Ken Lu <ken.lu@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>

Min Xu (3):
MdePkg: Introduce TdProtocol for TD-Guest firmware
SecurityPkg: Support TdProtocol in DxeTpm2MeasureBootLib
SecurityPkg: Support TdProtocol in DxeTpmMeasurementLib

MdePkg/Include/Protocol/TdProtocol.h | 305 +++++++++++++++
MdePkg/MdePkg.dec | 3 +
.../DxeTpm2MeasureBootLib.c | 346 ++++++++++++++----
.../DxeTpm2MeasureBootLib.inf | 1 +
.../DxeTpmMeasurementLib.c | 87 ++++-
.../DxeTpmMeasurementLib.inf | 1 +
6 files changed, 672 insertions(+), 71 deletions(-)
create mode 100644 MdePkg/Include/Protocol/TdProtocol.h

--
2.29.2.windows.2


Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal

Ashish Singhal
 

Marc,

In the document ARM062-1010708621-30 (AArch64 Programmer's Guides Generic Timer), towards the end of section 3.4 it says: "When writing an interrupt handler for the timers, it is important that software clears the interrupt before deactivating the interrupt in the GIC. Otherwise, the GIC will re-signal the same interrupt again."

My change was in accordance with this. We only clear the interrupt when we update the compare value but were signaling EOI before that going against the guidance in the document.

Thanks
Ashish


From: Marc Zyngier <maz@...>
Sent: Tuesday, October 12, 2021 2:57 AM
To: Ard Biesheuvel <ardb@...>; Ashish Singhal <ashishsingha@...>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@...>; Ard Biesheuvel <ardb+tianocore@...>
Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal
 
External email: Use caution opening links or attachments


On Mon, 11 Oct 2021 23:24:38 +0100,
Ard Biesheuvel <ardb@...> wrote:
>
> (+ Marc)
>
> On Mon, 11 Oct 2021 at 23:40, Ashish Singhal <ashishsingha@...> wrote:
> >
> > In an interrupt handler for the timers, it is important that
> > software clears the interrupt before deactivating the interrupt
> > in the GIC. Otherwise the GIC will re-signal the same interrupt
> > again.
> >
> > Signed-off-by: Ashish Singhal <ashishsingha@...>
>
> Please don't spam us with almost identical versions of the same patch
> without even documenting what the difference is.
>
>
> > ---
> >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > index 0370620fae..46e5bbf287 100644
> > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > @@ -300,10 +300,6 @@ TimerInterruptHandler (
> >    //
> >    OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> >
> > -  // Signal end of interrupt early to help avoid losing subsequent ticks
> > -  // from long duration handlers
> > -  gInterrupt->EndOfInterrupt (gInterrupt, Source);
> > -
> >    // Check if the timer interrupt is active
> >    if ((ArmGenericTimerGetTimerCtrlReg () ) & ARM_ARCH_TIMER_ISTATUS) {
> >
> > @@ -335,6 +331,11 @@ TimerInterruptHandler (
> >      ArmInstructionSynchronizationBarrier ();
> >    }
> >
> > +  // In an interrupt handler for the timers, it is important that software clears the interrupt
> > +  // before deactivating the interrupt in the GIC. Otherwise the GIC will re-signal the same
> > +  // interrupt again.
> > +  gInterrupt->EndOfInterrupt (gInterrupt, Source);
> > +
> >    gBS->RestoreTPL (OriginalTPL);
> >  }
> >

This isn't a requirement if you haven't re-enabled interrupts in
PSTATE (and the TPL being raised seems to indicate that interrupts are
actively masked here).

The timer is a level interrupt, and lowering the level takes time. If
your GIC implementation is good, it will notice that the lowering of
the level quickly, before you can reach the point where you re-enable
interrupts. If it is slow (or badly emulated if this is actually done
in a hypervisor), you'll get a spurious interrupt.

In any case, moving the EOI around doesn't make things any better. You
are just moving the goal post, without additional guarantees that the
level has been retired.

As a consequence, I don't think this patch makes much sense.

        M.

--
Without deviation from the norm, progress is not possible.


Re: [PATCH V9 2/4] OvmfPkg: Clear WORK_AREA_GUEST_TYPE in Main.asm

Lendacky, Thomas
 

On 10/11/21 9:37 PM, Min Xu wrote:
RFC: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3429&;data=04%7C01%7Cthomas.lendacky%40amd.com%7Cc4c4ac9654a940ada92308d98d2994d0%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637696032012206979%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=1SVRKXztfFcaVVer1AYOhLIhs6sVW%2BwtYQNxuuHHbTE%3D&amp;reserved=0
Previously WORK_AREA_GUEST_TYPE was cleared in SetCr3ForPageTables64.
This is workable for Legacy guest and SEV guest. But it doesn't work
after Intel TDX is introduced. It is because all TDX CPUs (BSP and APs)
start to run from 0xfffffff0, thus WORK_AREA_GUEST_TYPE will be cleared
multi-times if it is TDX guest. So the clearance of WORK_AREA_GUEST_TYPE
is moved to Main16 entry point in Main.asm.
Note: WORK_AREA_GUEST_TYPE is only defined for ARCH_X64.
For Intel TDX, its corresponding entry point is Main32 (which will be
introduced in next commit in this patch-set). WORK_AREA_GUEST_TYPE will
be cleared there.
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 4 ----
OvmfPkg/ResetVector/Main.asm | 8 ++++++++
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index 07b6ca070909..02528221e560 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -42,10 +42,6 @@ BITS 32
;
SetCr3ForPageTables64:
- ; Clear the WorkArea header. The SEV probe routines will populate the
- ; work area when detected.
- mov byte[WORK_AREA_GUEST_TYPE], 0
-
; Check whether the SEV is active and populate the SevEsWorkArea
OneTimeCall CheckSevFeatures
diff --git a/OvmfPkg/ResetVector/Main.asm b/OvmfPkg/ResetVector/Main.asm
index ae90a148fce7..a501fbe880f2 100644
--- a/OvmfPkg/ResetVector/Main.asm
+++ b/OvmfPkg/ResetVector/Main.asm
@@ -36,6 +36,14 @@ Main16:
BITS 32
+%ifdef ARCH_X64
A regular SEV guest can be built in the hybrid IA32 and X64 configuration, so this will break existing SEV firmwares built in that manner. Only SEV-ES and SEV-SNP require the full X64 confguration.

Thanks,
Tom

+
+ ; Clear the WorkArea header. The SEV probe routines will populate the
+ ; work area when detected.
+ mov byte[WORK_AREA_GUEST_TYPE], 0
+
+%endif
+
;
; Search for the Boot Firmware Volume (BFV)
;

4341 - 4360 of 86113