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

[Laszlo Ersek]

On 04/21/21 01:13, Lendacky, Thomas wrote:
> On 4/20/21 5:54 PM, Lendacky, Thomas via groups.io wrote:
>> From: Tom Lendacky 
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
>>
>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>> guest will fail attempting to perform MMIO to an encrypted address.

(1) The subject says SEV, not SEV-ES, and the code in the patch too
suggests SEV, not SEV-ES. If that's correct, can you please update the
commit message?

>>
>> Read the PcdTpmBaseAddress and mark the specification defined range
>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>> the MMIO requests.
>>
>> Cc: Laszlo Ersek 
>> Cc: Ard Biesheuvel 
>> Cc: Jordan Justen 
>> Cc: Brijesh Singh 
>> Cc: James Bottomley 
>> Cc: Jiewen Yao 
>> Cc: Min Xu 
>> Signed-off-by: Tom Lendacky 
>> ---
>>  OvmfPkg/PlatformPei/PlatformPei.inf |  1 +
>>  OvmfPkg/PlatformPei/AmdSev.c| 19 +++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf 
>> b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index 6ef77ba7bb21..de60332e9390 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -113,6 +113,7 @@ [Pcd]
>>
>>  [FixedPcd]
>>gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>>gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>>gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>>gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>> index dddffdebda4b..d524929f9e10 100644
>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>>)
>>  {
>>UINT64EncryptionMask;
>> +  UINT64TpmBaseAddress;
>>RETURN_STATUS PcdStatus;
>>
>>//
>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>>  }
>>}
>>
>> +  //
>> +  // PEI TPM support will perform MMIO accesses, be sure this range is not
>> +  // marked encrypted.
>> +  //
>> +  TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>> +  if (TpmBaseAddress != 0) {
>> +RETURN_STATUS  DecryptStatus;
>> +
>> +DecryptStatus = MemEncryptSevClearPageEncMask (
>> +  0,
>> +  TpmBaseAddress,
>> +  EFI_SIZE_TO_PAGES (0x5000),
>> +  FALSE
>> +  );
>> +
>> +ASSERT_RETURN_ERROR (DecryptStatus);
>> +  }
>> +
>
> Laszlo, I'm not sure if this is the best way to approach this. It is
> simple and straight forward and the TCG/TPM support is one of the few
> (only?) pieces of code that does actual MMIO during PEI that is bitten
> by not having the address marked as shared/unencrypted.

In SEC, I think we have MMIO access too (LAPIC --
InitializeApicTimer()); why does that work?

Hmm... Is that because we're immediately in x2apic mode, and that means
CPUID plus MSR accesses, and not MMIO? (I'm reminded of commit
decb365b0016 ("OvmfPkg: select LocalApicLib instance with x2apic
support", 2015-11-30).) And, we have #VC handling in SEC too.

Anyway: I think the TPM (MMIO) access you see comes from this PEIM:

  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf

The driver uses the following library instance:

  SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf

This library instance is what depends on "PcdTpmBaseAddress".

And it's not just that decrypting the TPM MMIO range in PlatformPei
"looks awkward", but I don't even see it immediately why PlatformPei is
guaranteed to be dispatched before Tcg2ConfigPei. The effective depex of
Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according to the
build report file. If Tcg2ConfigPei runs first, whatever we do in
PlatformPei is too late.

I also don't like that, with this patch, we'd decrypt the TPM range even
if OVMF weren't built with "-D TPM_ENABLE". Namely, OVMF uses
"PcdTpmBaseAddress" as fixed (not dynamic), inheriting the nonzero
default from "SecurityPkg.dec". (In ArmVirtQemu, PcdTpmBaseAddress is
set dynamically, which is why Tcg2ConfigPei has an ARM-specific depex
too.)


(2) So, can you please try the following, in the
"OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" module:

> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf 
> b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> index 6776ec931ce0..0d0572b83599 100644
> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> @@ -20,13 +20,16 @@ [Defines]
>ENTRY_POINT= Tcg2ConfigPeimEntryPoint
>
>  [Sources]
> +  MemEncrypt.h
>Tcg2ConfigPeim.c
>Tpm12Support.h
>
>  [Sources.IA32, Sources.X64]
> +  MemEncryptSev.c
>Tpm12Support.c
>
>  [Sources.ARM, Sources.AARCH64]
> +  MemEncryptNull.c
>

[edk2-devel] [PATCH v2] BaseTools: Add support for version 3 of FMP Image Header structure

[Sughosh Ganu]

Add support for the ImageCapsuleSupport field, introduced in version 3
of the EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER structure. This
structure member is used to indicate if the corresponding payload has
support for authentication and dependency.

Signed-off-by: Sughosh Ganu 
---

Changes since v1:
- Reword the patch header to get rid of the PatchCheck warning
- Make passing of ImageCapsuleSupport parameter to the AddPayload
  function as an optional parameter to maintain backward compatibility
- Declare the values of CAPSULE_SUPPORT_DEPENDENCY and
  CAPSULE_SUPPORT_AUTHENTICATION in the FmpCapsuleHeaderClass and use
  those in the GenerateCapsule script

 .../Source/Python/Capsule/GenerateCapsule.py  |  5 +++-
 .../Common/Uefi/Capsule/FmpCapsuleHeader.py   | 28 +--
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/BaseTools/Source/Python/Capsule/GenerateCapsule.py 
b/BaseTools/Source/Python/Capsule/GenerateCapsule.py
index a8de988253..b8039db878 100644
--- a/BaseTools/Source/Python/Capsule/GenerateCapsule.py
+++ b/BaseTools/Source/Python/Capsule/GenerateCapsule.py
@@ -561,6 +561,7 @@ if __name__ == '__main__':
 print ('GenerateCapsule: error:' + str(Msg))
 sys.exit (1)
 for SinglePayloadDescriptor in PayloadDescriptorList:
+ImageCapsuleSupport = 0x
 Result = SinglePayloadDescriptor.Payload
 try:
 FmpPayloadHeader.FwVersion  = 
SinglePayloadDescriptor.FwVersion
@@ -575,6 +576,7 @@ if __name__ == '__main__':
 if SinglePayloadDescriptor.UseDependency:
 CapsuleDependency.Payload = Result
 CapsuleDependency.DepexExp = SinglePayloadDescriptor.DepexExp
+ImageCapsuleSupport|= 
FmpCapsuleHeader.CAPSULE_SUPPORT_DEPENDENCY
 Result = CapsuleDependency.Encode ()
 if args.Verbose:
 CapsuleDependency.DumpInfo ()
@@ -607,13 +609,14 @@ if __name__ == '__main__':
 FmpAuthHeader.MonotonicCount = 
SinglePayloadDescriptor.MonotonicCount
 FmpAuthHeader.CertData   = CertData
 FmpAuthHeader.Payload= Result
+ImageCapsuleSupport  |= 
FmpCapsuleHeader.CAPSULE_SUPPORT_AUTHENTICATION
 Result = FmpAuthHeader.Encode ()
 if args.Verbose:
 FmpAuthHeader.DumpInfo ()
 except:
 print ('GenerateCapsule: error: can not encode FMP Auth 
Header')
 sys.exit (1)
-FmpCapsuleHeader.AddPayload (SinglePayloadDescriptor.Guid, Result, 
HardwareInstance = SinglePayloadDescriptor.HardwareInstance, UpdateImageIndex = 
SinglePayloadDescriptor.UpdateImageIndex)
+FmpCapsuleHeader.AddPayload (SinglePayloadDescriptor.Guid, Result, 
HardwareInstance = SinglePayloadDescriptor.HardwareInstance, UpdateImageIndex = 
SinglePayloadDescriptor.UpdateImageIndex, CapsuleSupport = ImageCapsuleSupport)
 try:
 for EmbeddedDriver in EmbeddedDriverDescriptorList:
 FmpCapsuleHeader.AddEmbeddedDriver(EmbeddedDriver)
diff --git a/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py 
b/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py
index 91d24919c4..8abb449c6f 100644
--- a/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py
+++ b/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py
@@ -47,14 +47,19 @@ class FmpCapsuleImageHeaderClass (object):
 #   /// therefore can be modified without changing the Auth data.
 #   ///
 #   UINT64   UpdateHardwareInstance;
+#
+#   ///
+#   /// Bits which indicate authentication and depex information for the 
image that follows this structure
+#   ///
+#   UINT64   ImageCapsuleSupport
 # } EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER;
 #
-#  #define EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION 
0x0002
+#  #define EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION 
0x0003
 
-_StructFormat = '= len (self._FmpCapsuleImageHeaderList):
@@ -198,13 +209,14 @@ class FmpCapsuleHeaderClass (object):
 self._ItemOffsetList.append (Offset)
 Offset = Offset + len (EmbeddedDriver)
 Index = 1
-for (UpdateImageTypeId, Payload, VendorCodeBytes, HardwareInstance, 
UpdateImageIndex) in self._PayloadList:
+for (UpdateImageTypeId, Payload, VendorCodeBytes, HardwareInstance, 
UpdateImageIndex, CapsuleSupport) in self._PayloadList:
 FmpCapsuleImageHeader = FmpCapsuleImageHeaderClass ()
 FmpCapsuleImageHeader.UpdateImageTypeId  = UpdateImageTypeId
 FmpCapsuleImageHeader.UpdateImageIndex   = UpdateImageIndex
 FmpCapsuleImageHeader.Payload= Payload
 FmpCapsuleImageHeader.

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

[Laszlo Ersek]

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

> Anyway: I think the TPM (MMIO) access you see comes from this PEIM:
>
>   OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>
> The driver uses the following library instance:
>
>   SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
>
> This library instance is what depends on "PcdTpmBaseAddress".
>
> And it's not just that decrypting the TPM MMIO range in PlatformPei
> "looks awkward", but I don't even see it immediately why PlatformPei
> is guaranteed to be dispatched before Tcg2ConfigPei. The effective
> depex of Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according
> to the build report file. If Tcg2ConfigPei runs first, whatever we do
> in PlatformPei is too late.
>
> I also don't like that, with this patch, we'd decrypt the TPM range
> even if OVMF weren't built with "-D TPM_ENABLE". Namely, OVMF uses
> "PcdTpmBaseAddress" as fixed (not dynamic), inheriting the nonzero
> default from "SecurityPkg.dec". (In ArmVirtQemu, PcdTpmBaseAddress is
> set dynamically, which is why Tcg2ConfigPei has an ARM-specific depex
> too.)
>
>
> (2) So, can you please try the following, in the
> "OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" module:
>
>> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf 
>> b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> index 6776ec931ce0..0d0572b83599 100644
>> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> @@ -20,13 +20,16 @@ [Defines]
>>ENTRY_POINT= Tcg2ConfigPeimEntryPoint
>>
>>  [Sources]
>> +  MemEncrypt.h
>>Tcg2ConfigPeim.c
>>Tpm12Support.h
>>
>>  [Sources.IA32, Sources.X64]
>> +  MemEncryptSev.c
>>Tpm12Support.c
>>
>>  [Sources.ARM, Sources.AARCH64]
>> +  MemEncryptNull.c
>>Tpm12SupportNull.c
>>
>>  [Packages]
>> @@ -43,6 +46,7 @@ [LibraryClasses]
>>
>>  [LibraryClasses.IA32, LibraryClasses.X64]
>>BaseLib
>> +  MemEncryptSevLib
>>Tpm12DeviceLib
>>
>>  [Guids]
>> @@ -56,6 +60,9 @@ [Ppis]
>>  [Pcd]
>>gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## 
>> PRODUCES
>>
>> +[Pcd.IA32, Pcd.X64]
>> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## 
>> SOMETIMES_CONSUMES
>> +
>>  [Depex.IA32, Depex.X64]
>>TRUE
>>
>
> In the "MemEncrypt.h" file, declare a function called
> InternalTpmDecryptAddressRange(). The function definition in
> "MemEncryptNull.c" should do nothing, while the one in
> "MemEncryptSev.c" should check MemEncryptSevIsEnabled(), and then make
> the above-seen MemEncryptSevClearPageEncMask() call.
>
> 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().
>
> (An alternative approach would be to call MemEncryptSevIsEnabled() and
> MemEncryptSevClearPageEncMask() regardless of architecture, i.e., also
> on ARM / AARCH64. In addition to that, we'd have to implement a Null
> instance of MemEncryptSevLib, and resolve MemEncryptSevLib to the Null
> instance in the ArmVirtPkg DSC files. But I don't like that: the
> library *class* carries SEV in the name, which is inherently
> X64-specific, thus I wouldn't even like the lib *class* to leak into
> ArmVirtPkg.)

Here's another thing. Above, I mention that nothing guarantees that
Tcg2ConfigPei runs before PlatformPei. That raises a problem even if we
use approach (2).

In approach (2), we massage page table entries, and ultimately use

  OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf

for that. But that library instance can allocate full pages, in case
page table splitting is needed (from 1GB to 2MB to 4KB).

I can't tell off-hand if such page table splitting will actually occur
when we decrypt the TPM MMIO address range -- but even if it does not,
for whatever reason, I wouldn't like to rely on that particular reason.
And I definitely don't want such page allocations to be satisfied from
the temporary SEC/PEI heap, before we migrate to permanent PEI RAM. See
how "PcdOvmfSecPeiTempRamBase" and "PcdOvmfSecPeiTempRamSize" are set in
the FDF files, and see the OVMF log too:

> Temp Stack : BaseAddress=0x818000 Length=0x8000
> Temp Heap  : BaseAddress=0x81 Length=0x8000
> Total temporary memory:65536 bytes.
>   temporary memory stack ever used:   30128 bytes.
>   temporary memory heap used for HobList: 7208 bytes.
>   temporary memory heap occupied by memory pages: 0 bytes.

What I'm saying is that we've probably been missing the following hunk
for a long time now:

> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf 
> b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> index 03a78c32df28..1b3808305415 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> @@ -55,3 +5

Re: [edk2-devel] [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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74351): https://edk2.groups.io/g/devel/message/74351
Mute This Topic: https://groups.io/mt/82248382/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] 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 
Date: Tuesday, 20 April 2021 at 21:04
To: Sami Mujawar , devel@edk2.groups.io 
, Samer El-Haj-Mahmoud , 
Ard Biesheuvel , l...@nuviainc.com 
Cc: r...@edk2.groups.io , Jiewen Yao 
, Rahul Kumar , nd 
, 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  On Behalf Of Rebecca Cran 
> via groups.io
> Sent: 14 January 2021 09:05 PM
> To: Sami Mujawar ; devel@edk2.groups.io; Samer 
> El-Haj-Mahmoud ; Ard Biesheuvel 
> ; l...@nuviainc.com
> Cc: r...@edk2.groups.io; Jiewen Yao ; Rahul Kumar 
> ; 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?
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74352): https://edk2.groups.io/g/devel/message/74352
Mute This Topic: https://groups.io/mt/78823009/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sami Mujawar]

Hi Pierre,

Thank you for this patch.

Reviewed-by: Sami Mujawar 

Regards,

Sami Mujawar

From: pierre.gond...@arm.com 
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io , Sami Mujawar 
, l...@nuviainc.com , 
ardb+tianoc...@kernel.org , 
sean.bro...@microsoft.com , 
bret.barke...@microsoft.com 
Subject: [PATCH v1 01/12] ArmPkg: Fix Ecc error 8003
From: 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 
---
 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 
 #include 
@@ -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 

@@ -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 @@

 **/

-#

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

[Sami Mujawar]

Hi Pierre,

Thank you for this patch.

Reviewed-by: Sami Mujawar 

Regards,

Sami Mujawar
From: pierre.gond...@arm.com 
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io , Sami Mujawar 
, l...@nuviainc.com , 
ardb+tianoc...@kernel.org , 
sean.bro...@microsoft.com , 
bret.barke...@microsoft.com 
Subject: [PATCH v1 02/12] ArmPkg: Fix Ecc error 3002 in StandaloneMmMmuLib
From: Pierre Gondois 

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

Signed-off-by: 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.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74354): https://edk2.groups.io/g/devel/message/74354
Mute This Topic: https://groups.io/mt/82258501/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [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 

Regards,

Sami Mujawar

From: pierre.gond...@arm.com 
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io , Sami Mujawar 
, l...@nuviainc.com , 
ardb+tianoc...@kernel.org , 
sean.bro...@microsoft.com , 
bret.barke...@microsoft.com 
Subject: [PATCH v1 03/12] ArmPkg: Add missing library headers to ArmPkg.dec
From: 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 
---
 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.
-# 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.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74355): https://edk2.groups.io/g/devel/message/74355
Mute This Topic: https://groups.io/mt/82258504/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sami Mujawar]

Hi Pierre,

Thank you for this patch.

Reviewed-by: Sami Mujawar 

Regards,

Sami Mujawar

From: pierre.gond...@arm.com 
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io , Sami Mujawar 
, l...@nuviainc.com , 
ardb+tianoc...@kernel.org , 
sean.bro...@microsoft.com , 
bret.barke...@microsoft.com 
Subject: [PATCH v1 04/12] ArmPkg: Add OemMiscLibNull library to ArmPkg.dsc
From: 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 
---
 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.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74356): https://edk2.groups.io/g/devel/message/74356
Mute This Topic: https://groups.io/mt/82258505/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sami Mujawar]

Hi Pierre,

This patch looks good to me.

Reviewed-by: Sami Mujawar 

Regards,

Sami Mujawar


From: pierre.gond...@arm.com 
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io , Sami Mujawar 
, l...@nuviainc.com , 
ardb+tianoc...@kernel.org , 
sean.bro...@microsoft.com , 
bret.barke...@microsoft.com 
Subject: [PATCH v1 05/12] ArmPkg: Correct small typos
From: Pierre Gondois 

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

Signed-off-by: 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_PATHGuid;
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  NumberOfBytesNumber of output bytes which are cached in Buffer.

   @retval 0Write 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  NumberOfBytesNumber of output bytes which are cached in Buffer.

   @retval 0Read 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_MEMOR

Re: [edk2-devel] [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 

Regards,

Sami Mujawar


From: pierre.gond...@arm.com 
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io , Sami Mujawar 
, l...@nuviainc.com , 
ardb+tianoc...@kernel.org , 
sean.bro...@microsoft.com , 
bret.barke...@microsoft.com 
Subject: [PATCH v1 06/12] ArmPkg: Add ArmPkg.ci.yaml
From: Pierre Gondois 

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

Signed-off-by: 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 ..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.
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+{
+## options defined .pytool/Plugin/LicenseCheck
+"LicenseCheck": {
+"IgnoreFiles": []
+},
+
+"EccCheck": {
+## Exception sample looks like below:
+## "ExceptionList": [
+## "", ""
+## ]
+"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",
+   

Re: [edk2-devel] [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 mailto:sami.muja...@arm.com>>

Regards,

Sami Mujawar


From: pierre.gond...@arm.com 
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io , Sami Mujawar 
, l...@nuviainc.com , 
ardb+tianoc...@kernel.org , 
sean.bro...@microsoft.com , 
bret.barke...@microsoft.com 
Subject: [PATCH v1 07/12] ArmPlatformPkg: Add ArmPlatformPkg.ci.yaml
From: Pierre Gondois 

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

Signed-off-by: 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 ..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.
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+{
+## options defined .pytool/Plugin/LicenseCheck
+"LicenseCheck": {
+"IgnoreFiles": []
+},
+
+"EccCheck": {
+## Exception sample looks like below:
+## "ExceptionList": [
+## "", ""
+## ]
+"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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74359): https://edk2.groups.io/g/devel/message/74359
Mute This Topic: https://groups.io/mt/82258510/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sami Mujawar]

Hi Pierre,

This patch looks good to me.

Reviewed-by: Sami Mujawar mailto:sami.muja...@arm.com>>

Regards,

Sami Mujawar

From: pierre.gond...@arm.com 
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io , Sami Mujawar 
, l...@nuviainc.com , 
ardb+tianoc...@kernel.org , 
sean.bro...@microsoft.com , 
bret.barke...@microsoft.com 
Subject: [PATCH v1 08/12] .pytool: Enable CI for ArmPkg
From: Pierre Gondois 

Enable the CI for the ArmPkg.

Signed-off-by: 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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74360): https://edk2.groups.io/g/devel/message/74360
Mute This Topic: https://groups.io/mt/82258512/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sami Mujawar]

Hi Pierre,

This patch looks good to me.

Reviewed-by: Sami Mujawar mailto:sami.muja...@arm.com>>

Regards,

Sami Mujawar



From: pierre.gond...@arm.com 
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io , Sami Mujawar 
, l...@nuviainc.com , 
ardb+tianoc...@kernel.org , 
sean.bro...@microsoft.com , 
bret.barke...@microsoft.com 
Subject: [PATCH v1 09/12] .pytool: Enable CI for ArmPlatformPkg
From: Pierre Gondois 

Enable the CI for the ArmPlatformPkg.

Signed-off-by: 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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74361): https://edk2.groups.io/g/devel/message/74361
Mute This Topic: https://groups.io/mt/82258513/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sami Mujawar]

Hi Pierre,

Thank you for this patch.

Reviewed-by: Sami Mujawar mailto:sami.muja...@arm.com>>

Regards,

Sami Mujawar

From: pierre.gond...@arm.com 
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io , Sami Mujawar 
, l...@nuviainc.com , 
ardb+tianoc...@kernel.org , 
sean.bro...@microsoft.com , 
bret.barke...@microsoft.com 
Subject: [PATCH v1 10/12] .pytool: Document LicenseCheck and EccCheck
From: Pierre Gondois 

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

Signed-off-by: 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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74362): https://edk2.groups.io/g/devel/message/74362
Mute This Topic: https://groups.io/mt/82258514/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sami Mujawar]

Hi Pierre,

Thank you for this patch.

Reviewed-by: Sami Mujawar mailto:sami.muja...@arm.com>>

Regards,

Sami Mujawar

From: pierre.gond...@arm.com 
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io , Sami Mujawar 
, l...@nuviainc.com , 
ardb+tianoc...@kernel.org , 
sean.bro...@microsoft.com , 
bret.barke...@microsoft.com 
Subject: [PATCH v1 11/12] AzurePipelines: Add support for ArmPkg
From: 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 
---
 .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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74363): https://edk2.groups.io/g/devel/message/74363
Mute This Topic: https://groups.io/mt/82258516/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sami Mujawar]

Hi Pierre,

 Thank you for this patch.

Reviewed-by: Sami Mujawar mailto:sami.muja...@arm.com>>

Regards,

Sami Mujawar



From: pierre.gond...@arm.com 
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io , Sami Mujawar 
, l...@nuviainc.com , 
ardb+tianoc...@kernel.org , 
sean.bro...@microsoft.com , 
bret.barke...@microsoft.com 
Subject: [PATCH v1 12/12] AzurePipelines: Add support for ArmPlatformPkg
From: 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 
---
 .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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74364): https://edk2.groups.io/g/devel/message/74364
Mute This Topic: https://groups.io/mt/82258517/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [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 
>>
>> 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&sdata=1EmUDf%2FfuCuu%2BkXPZijzatfliplMhKEQH8kiZ9Z8ZF0%3D&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 
>> Cc: Ard Biesheuvel 
>> Cc: Jordan Justen 
>> Cc: Brijesh Singh 
>> Cc: James Bottomley 
>> Cc: Jiewen Yao 
>> Cc: Min Xu 
>> Signed-off-by: Tom Lendacky 
>> ---
>>  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 
> 
> 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;
>>  
>>
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74365): https://edk2.groups.io/g/devel/message/74365
Mute This Topic: https://groups.io/mt/82247952/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [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 
> Cc: Ard Biesheuvel 
> Cc: Leif Lindholm 
> Signed-off-by: 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;
. 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 ..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 ..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 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +// 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   0x0800
> +#define MACH_VIRT_PERIPH_SIZE   SIZE_128MB
> +
> +//
> +// in cloud-hypervisor, 0x0 ~ 0x800 is reserved as normal memory for UEFI
> +//
> +#define CLH_UEFI_MEM_BASE   0x0

Re: [edk2-devel] [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 
nore...@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 
Date: Thursday, 22 April 2021 at 14:57
To: Jianyong Wu , edk2-devel-groups-io 

Cc: Justin He , Ard Biesheuvel , 
Leif Lindholm , 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 
> Cc: Ard Biesheuvel 
> Cc: Leif Lindholm 
> Signed-off-by: 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;
. 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 ..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 ..829d7d7aa259
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
> @@ -0,0 +1,94 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, Linaro Limit

Re: [edk2-devel] [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 
>>
>> 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&sdata=znSezOvpnItW7mHAJkr%2FtJtkQNFc2H0dG9STpmOpVqU%3D&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 
>> Cc: Ard Biesheuvel 
>> Cc: Jordan Justen 
>> Cc: Brijesh Singh 
>> Cc: James Bottomley 
>> Cc: Jiewen Yao 
>> Cc: Min Xu 
>> Signed-off-by: Tom Lendacky 
>> ---
>>  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;
>> +}
>> 

Re: [edk2-devel] [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  
Sent: Tuesday, April 20, 2021 6:29 PM
To: Agrawal, Sachin ; devel@edk2.groups.io
Cc: Wang, Jian J ; Lu, XiaoyuX ; 
Jiang, Guomin 
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 
> Sent: Tuesday, April 20, 2021 11:20 PM
> To: Yao, Jiewen ; devel@edk2.groups.io
> Cc: Wang, Jian J ; Lu, XiaoyuX 
> ; Jiang, Guomin 
> 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 
> Sent: Tuesday, April 20, 2021 2:12 AM
> To: Agrawal, Sachin ; devel@edk2.groups.io
> Cc: Wang, Jian J ; Lu, XiaoyuX 
> ; Jiang, Guomin 
> 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 
> > Sent: Tuesday, April 20, 2021 3:19 PM
> > To: Yao, Jiewen ; devel@edk2.groups.io
> > Cc: Wang, Jian J ; Lu, XiaoyuX 
> > ; Jiang, Guomin 
> > 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

Re: [edk2-devel] 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  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 
 Sent: Friday, April 16, 2021 3:43 AM
 To: Paolo Bonzini 
 Cc: devel@edk2.groups.io; j...@linux.ibm.com; Yao, Jiewen
 ; dgilb...@redhat.com; Laszlo Ersek
 ; Xu, Min M ;
 thomas.lenda...@amd.com; Brijesh Singh ; Justen,
 Jordan L ; Ard Biesheuvel
 ; Nathaniel McCallum
 ; Ning Yang 
 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 
 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 re

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

[Lendacky, Thomas]

On 4/22/21 2:34 AM, Laszlo Ersek wrote:
> On 04/21/21 01:13, Lendacky, Thomas wrote:
>> On 4/20/21 5:54 PM, Lendacky, Thomas via groups.io wrote:
>>> From: Tom Lendacky 
>>>
>>> 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%7C6b8da1f9a3bf4fb5f01e08d905613998%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546737416495415%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=5vPlHPzGlS2%2Bqu3U4RPMITpyY%2F2ZxKJlaVYfFZItONQ%3D&reserved=0
>>>
>>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>>> guest will fail attempting to perform MMIO to an encrypted address.
> 
> (1) The subject says SEV, not SEV-ES, and the code in the patch too
> suggests SEV, not SEV-ES. If that's correct, can you please update the
> commit message?

Yes, I'll update the commit message. The action is correct for all SEV
guests in general, but it is only with SEV-ES, where the tighter MMIO
checks can be performed, that an actual issue shows up.

> 
>>>
>>> Read the PcdTpmBaseAddress and mark the specification defined range
>>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>>> the MMIO requests.
>>>
>>> Cc: Laszlo Ersek 
>>> Cc: Ard Biesheuvel 
>>> Cc: Jordan Justen 
>>> Cc: Brijesh Singh 
>>> Cc: James Bottomley 
>>> Cc: Jiewen Yao 
>>> Cc: Min Xu 
>>> Signed-off-by: Tom Lendacky 
>>> ---
>>>  OvmfPkg/PlatformPei/PlatformPei.inf |  1 +
>>>  OvmfPkg/PlatformPei/AmdSev.c| 19 +++
>>>  2 files changed, 20 insertions(+)
>>>
>>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf 
>>> b/OvmfPkg/PlatformPei/PlatformPei.inf
>>> index 6ef77ba7bb21..de60332e9390 100644
>>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>>> @@ -113,6 +113,7 @@ [Pcd]
>>>
>>>  [FixedPcd]
>>>gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>>>gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>>>gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>>>gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>>> index dddffdebda4b..d524929f9e10 100644
>>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>>>)
>>>  {
>>>UINT64EncryptionMask;
>>> +  UINT64TpmBaseAddress;
>>>RETURN_STATUS PcdStatus;
>>>
>>>//
>>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>>>  }
>>>}
>>>
>>> +  //
>>> +  // PEI TPM support will perform MMIO accesses, be sure this range is not
>>> +  // marked encrypted.
>>> +  //
>>> +  TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>>> +  if (TpmBaseAddress != 0) {
>>> +RETURN_STATUS  DecryptStatus;
>>> +
>>> +DecryptStatus = MemEncryptSevClearPageEncMask (
>>> +  0,
>>> +  TpmBaseAddress,
>>> +  EFI_SIZE_TO_PAGES (0x5000),
>>> +  FALSE
>>> +  );
>>> +
>>> +ASSERT_RETURN_ERROR (DecryptStatus);
>>> +  }
>>> +
>>
>> Laszlo, I'm not sure if this is the best way to approach this. It is
>> simple and straight forward and the TCG/TPM support is one of the few
>> (only?) pieces of code that does actual MMIO during PEI that is bitten
>> by not having the address marked as shared/unencrypted.
> 
> In SEC, I think we have MMIO access too (LAPIC --
> InitializeApicTimer()); why does that work?
> 
> Hmm... Is that because we're immediately in x2apic mode, and that means
> CPUID plus MSR accesses, and not MMIO? (I'm reminded of commit
> decb365b0016 ("OvmfPkg: select LocalApicLib instance with x2apic
> support", 2015-11-30).) And, we have #VC handling in SEC too.
> 
> Anyway: I think the TPM (MMIO) access you see comes from this PEIM:
> 
>   OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> 
> The driver uses the following library instance:
> 
>   SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> 
> This library instance is what depends on "PcdTpmBaseAddress".
> 
> And it's not just that decrypting the TPM MMIO range in PlatformPei
> "looks awkward", but I don't even see it immediately why PlatformPei is
> guaranteed to be dispatched before Tcg2ConfigPei. The effective depex of
> Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according to the
> build report file. If Tcg2ConfigPei runs first, whatever we do in
> PlatformPei is too late.
> 
> I also don't like that, with this patch, we'd decrypt the TPM range even
> if OVMF weren't built with "-D TPM_ENABLE". Namely, OVMF uses
> "PcdTpmBaseAddress" as fixed (not dynamic), inheriting the nonzero
> default from "SecurityPkg.dec". (I

[edk2-devel] [PATCH] Maintainers.txt: Add 'Erdem Aktas' to Confidential Computing reviewers

[Erdem Aktas via groups.io]

Add 'Erdem Aktas' as a reviewer for OvmfPkg/Confidential Computing.

Signed-off-by: Erdem Aktas 
---
 Maintainers.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index fda3df5de2..cafe6b1ab8 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -458,6 +458,7 @@ F: OvmfPkg/PlatformPei/AmdSev.c
 F: OvmfPkg/ResetVector/
 F: OvmfPkg/Sec/
 R: Brijesh Singh 
+R: Erdem Aktas 
 R: James Bottomley 
 R: Jiewen Yao 
 R: Min Xu 
-- 
2.31.1.498.g6c1eba8ee3d-goog



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74372): https://edk2.groups.io/g/devel/message/74372
Mute This Topic: https://groups.io/mt/82287761/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Yao, Jiewen]

I think we have some EDKII tool will use the Signing capability, but it is not 
needed during BIOS boot. That is why Signing function is in Ext.c, while verify 
function in in Basic.c

Please also add crypto unit test for both API - 
https://github.com/tianocore/edk2/tree/master/CryptoPkg/Test

Thank you
Yao Jiewen

> -Original Message-
> From: Agrawal, Sachin 
> Sent: Thursday, April 22, 2021 10:16 PM
> To: Yao, Jiewen ; devel@edk2.groups.io
> Cc: Wang, Jian J ; Lu, XiaoyuX
> ; Jiang, Guomin 
> Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS verify
> support
> 
> 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 
> Sent: Tuesday, April 20, 2021 6:29 PM
> To: Agrawal, Sachin ; devel@edk2.groups.io
> Cc: Wang, Jian J ; Lu, XiaoyuX
> ; Jiang, Guomin 
> 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 
> > Sent: Tuesday, April 20, 2021 11:20 PM
> > To: Yao, Jiewen ; devel@edk2.groups.io
> > Cc: Wang, Jian J ; Lu, XiaoyuX
> > ; Jiang, Guomin 
> > 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 
> > Sent: Tuesday, April 20, 2021 2:12 AM
> > To: Agrawal, Sachin ; devel@edk2.groups.io
> > Cc: Wang, Jian J ; Lu, XiaoyuX
> > ; Jiang, Guomin 
> > Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
> > verify support

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

[Lendacky, Thomas]

On 4/22/21 9:15 AM, Tom Lendacky wrote:
> 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 
>>>
>>> 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&sdata=znSezOvpnItW7mHAJkr%2FtJtkQNFc2H0dG9STpmOpVqU%3D&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 
>>> Cc: Ard Biesheuvel 
>>> Cc: Jordan Justen 
>>> Cc: Brijesh Singh 
>>> Cc: James Bottomley 
>>> Cc: Jiewen Yao 
>>> Cc: Min Xu 
>>> Signed-off-by: Tom Lendacky 
>>> ---
>>>  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.

"Address" is a type UINTN, but since this is X64 only code, an 8-byte copy
isn't an issue. Should I add a comment about that above the setting of
"Address"? Or should I convert "Address" to a UINT64 - although
ValidateMmioMemory expects a UINTN...  Thoughts?

Thanks,
Tom

> 
>>
>>> +
>>> +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) {

Re: [edk2-devel] [PATCH] Maintainers.txt: Add 'Erdem Aktas' to Confidential Computing reviewers

[Yao, Jiewen]

Acked-by: jiewen@intel.com

thank you!
Yao, Jiewen


> 在 2021年4月22日，下午11:05，Erdem Aktas via groups.io 
>  写道：
> 
> Add 'Erdem Aktas' as a reviewer for OvmfPkg/Confidential Computing.
> 
> Signed-off-by: Erdem Aktas 
> ---
> Maintainers.txt | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index fda3df5de2..cafe6b1ab8 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -458,6 +458,7 @@ F: OvmfPkg/PlatformPei/AmdSev.c
> F: OvmfPkg/ResetVector/
> F: OvmfPkg/Sec/
> R: Brijesh Singh 
> +R: Erdem Aktas 
> R: James Bottomley 
> R: Jiewen Yao 
> R: Min Xu 
> -- 
> 2.31.1.498.g6c1eba8ee3d-goog
> 
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74375): https://edk2.groups.io/g/devel/message/74375
Mute This Topic: https://groups.io/mt/82287761/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Lendacky, Thomas]

On 4/22/21 9:51 AM, Tom Lendacky wrote:
> On 4/22/21 2:34 AM, Laszlo Ersek wrote:
>> On 04/21/21 01:13, Lendacky, Thomas wrote:
>>> On 4/20/21 5:54 PM, Lendacky, Thomas via groups.io wrote:
 From: Tom Lendacky 

 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%7C6b8da1f9a3bf4fb5f01e08d905613998%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546737416495415%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=5vPlHPzGlS2%2Bqu3U4RPMITpyY%2F2ZxKJlaVYfFZItONQ%3D&reserved=0

 The TPM support in OVMF performs MMIO accesses during the PEI phase. At
 this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
 guest will fail attempting to perform MMIO to an encrypted address.
>>
>> (1) The subject says SEV, not SEV-ES, and the code in the patch too
>> suggests SEV, not SEV-ES. If that's correct, can you please update the
>> commit message?
> 
> Yes, I'll update the commit message. The action is correct for all SEV
> guests in general, but it is only with SEV-ES, where the tighter MMIO
> checks can be performed, that an actual issue shows up.
> 
>>

 Read the PcdTpmBaseAddress and mark the specification defined range
 (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
 the MMIO requests.

 Cc: Laszlo Ersek 
 Cc: Ard Biesheuvel 
 Cc: Jordan Justen 
 Cc: Brijesh Singh 
 Cc: James Bottomley 
 Cc: Jiewen Yao 
 Cc: Min Xu 
 Signed-off-by: Tom Lendacky 
 ---
  OvmfPkg/PlatformPei/PlatformPei.inf |  1 +
  OvmfPkg/PlatformPei/AmdSev.c| 19 +++
  2 files changed, 20 insertions(+)

 diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf 
 b/OvmfPkg/PlatformPei/PlatformPei.inf
 index 6ef77ba7bb21..de60332e9390 100644
 --- a/OvmfPkg/PlatformPei/PlatformPei.inf
 +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
 @@ -113,6 +113,7 @@ [Pcd]

  [FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
 +  gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
 diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
 index dddffdebda4b..d524929f9e10 100644
 --- a/OvmfPkg/PlatformPei/AmdSev.c
 +++ b/OvmfPkg/PlatformPei/AmdSev.c
 @@ -141,6 +141,7 @@ AmdSevInitialize (
)
  {
UINT64EncryptionMask;
 +  UINT64TpmBaseAddress;
RETURN_STATUS PcdStatus;

//
 @@ -206,6 +207,24 @@ AmdSevInitialize (
  }
}

 +  //
 +  // PEI TPM support will perform MMIO accesses, be sure this range is not
 +  // marked encrypted.
 +  //
 +  TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
 +  if (TpmBaseAddress != 0) {
 +RETURN_STATUS  DecryptStatus;
 +
 +DecryptStatus = MemEncryptSevClearPageEncMask (
 +  0,
 +  TpmBaseAddress,
 +  EFI_SIZE_TO_PAGES (0x5000),
 +  FALSE
 +  );
 +
 +ASSERT_RETURN_ERROR (DecryptStatus);
 +  }
 +
>>>
>>> Laszlo, I'm not sure if this is the best way to approach this. It is
>>> simple and straight forward and the TCG/TPM support is one of the few
>>> (only?) pieces of code that does actual MMIO during PEI that is bitten
>>> by not having the address marked as shared/unencrypted.
>>
>> In SEC, I think we have MMIO access too (LAPIC --
>> InitializeApicTimer()); why does that work?
>>
>> Hmm... Is that because we're immediately in x2apic mode, and that means
>> CPUID plus MSR accesses, and not MMIO? (I'm reminded of commit
>> decb365b0016 ("OvmfPkg: select LocalApicLib instance with x2apic
>> support", 2015-11-30).) And, we have #VC handling in SEC too.

Missed this question in my earlier reply...  LAPIC access has a dedicated
check in ValidateMmioMemory() to allow access in this case.

Thanks,
Tom

>>
>> Anyway: I think the TPM (MMIO) access you see comes from this PEIM:
>>
>>   OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>
>> The driver uses the following library instance:
>>
>>   SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
>>
>> This library instance is what depends on "PcdTpmBaseAddress".
>>
>> And it's not just that decrypting the TPM MMIO range in PlatformPei
>> "looks awkward", but I don't even see it immediately why PlatformPei is
>> guaranteed to be dispatched before Tcg2ConfigPei. The effective depex of
>> Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according to the
>> build report file. If Tcg2ConfigPei runs

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

[Lendacky, Thomas]

On 4/22/21 3:39 AM, Laszlo Ersek wrote:
> 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().
> 

Unfortunately, this method doesn't work. The OVMF Tcg2ConfigPei.inf file
uses the SecurityPkg Tpm2DeviceLib library. The SecurityPkg Tpm2DeviceLib
library's constructor is called before the OVMF Tcg2ConfigPei constructor.
The Tpm2DeviceLib constructor performs MMIO to the TPM base address and
fails because the pages haven't been marked unencrypted yet by OVMF
Tcg2ConfigPei. Some debug output:

Loading PEIM at 0x0007F793000 EntryPoint=0x0007F794E4F Tcg2ConfigPei.efi
*** DEBUG: InternalTpm2DeviceLibDTpmCommonConstructor:55
*** DEBUG: Tpm2GetPtpInterface:425
*** DEBUG: Tpm2IsPtpPresence:51
MMIO using encrypted memory: FED4
 X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID -  


Thanks,
Tom

> 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
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74377): https://edk2.groups.io/g/devel/message/74377
Mute This Topic: https://groups.io/mt/82248382/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] Maintainers.txt: Add 'Erdem Aktas' to Confidential Computing reviewers

[Min Xu]

Acked-by: min.m...@intel.com

Thanks!
Xu, Min

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Erdem
> Aktas via groups.io
> Sent: Thursday, April 22, 2021 11:05 PM
> To: devel@edk2.groups.io; Laszlo Ersek 
> Cc: y...@google.com; Yao, Jiewen ; Paolo Bonzini
> ; jejb @ linux . ibm . com ;
> dgilbert @ redhat . com ; x...@google.com; Xu, Min M
> ; thomas . lendacky @ amd . com
> ; Brijesh Singh ;
> jus...@google.com; Justen, Jordan L ; Ard
> Biesheuvel ; Nathaniel McCallum
> ; Ning Yang ; Erdem
> Aktas 
> Subject: [edk2-devel] [PATCH] Maintainers.txt: Add 'Erdem Aktas' to
> Confidential Computing reviewers
> 
> Add 'Erdem Aktas' as a reviewer for OvmfPkg/Confidential Computing.
> 
> Signed-off-by: Erdem Aktas 
> ---
>  Maintainers.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt index fda3df5de2..cafe6b1ab8
> 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -458,6 +458,7 @@ F: OvmfPkg/PlatformPei/AmdSev.c
>  F: OvmfPkg/ResetVector/
>  F: OvmfPkg/Sec/
>  R: Brijesh Singh 
> +R: Erdem Aktas 
>  R: James Bottomley 
>  R: Jiewen Yao 
>  R: Min Xu 
> --
> 2.31.1.498.g6c1eba8ee3d-goog
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74378): https://edk2.groups.io/g/devel/message/74378
Mute This Topic: https://groups.io/mt/82287761/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] 回复: [PATCH] * MdePkg/SmBios.h: Updated newly added socket info from smbios 3.4.

[gaoliming]

Reviewed-by: Liming Gao 

> -邮件原件-
> 发件人: Chaganty, Rangasai V 
> 发送时间: 2021年4月21日 14:53
> 收件人: Bhargava, Avinash ;
> devel@edk2.groups.io
> 抄送: Ni, Ray ; Liming Gao ;
> Kumar, Chandana C 
> 主题: RE: [PATCH] * MdePkg/SmBios.h: Updated newly added socket info
> from smbios 3.4.
> 
> Please also update SmbiosView library in ShellPkg with the new socket
types.
> With that:
> Reviewed-by: Sai Chaganty 
> 
> -Original Message-
> From: Bhargava, Avinash 
> Sent: Tuesday, April 20, 2021 7:39 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray ; Chaganty; Chaganty, Rangasai V
> ; Liming Gao ;
> Kumar; Kumar, Chandana C 
> Subject: [PATCH] * MdePkg/SmBios.h: Updated newly added socket info from
> smbios 3.4.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3140
> Add theses socket type to MdePkg\Include\IndustryStandard\SmBios.h
> -LGA1200
> -LGA4189
> 
> Signed-off-by: Avinash Bhargava 
> Cc: Ray Ni 
> Cc: Chaganty, Rangasai V 
> Cc: Liming Gao 
> Cc: Kumar, Chandana C 
> ---
>  MdePkg/Include/IndustryStandard/SmBios.h | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Include/IndustryStandard/SmBios.h
> b/MdePkg/Include/IndustryStandard/SmBios.h
> index cc023b7369..39dfe4e137 100644
> --- a/MdePkg/Include/IndustryStandard/SmBios.h
> +++ b/MdePkg/Include/IndustryStandard/SmBios.h
> @@ -1,7 +1,7 @@
>  /** @file
> 
>Industry Standard Definitions of SMBIOS Table Specification v3.3.0.
> 
> 
> 
> -Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
> 
> +Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.
> 
>  (C) Copyright 2015-2017 Hewlett Packard Enterprise Development LP
> 
>  (C) Copyright 2015 - 2019 Hewlett Packard Enterprise Development LP
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -810,7 +810,9 @@ typedef enum {
>ProcessorUpgradeSocketLGA2066   = 0x39,
> 
>ProcessorUpgradeSocketBGA1392   = 0x3A,
> 
>ProcessorUpgradeSocketBGA1510   = 0x3B,
> 
> -  ProcessorUpgradeSocketBGA1528   = 0x3C
> 
> +  ProcessorUpgradeSocketBGA1528   = 0x3C,
> 
> +  ProcessorUpgradeSocketLGA4189   = 0x3D,
> 
> +  ProcessorUpgradeSocketLGA1200   = 0x3E
> 
>  } PROCESSOR_UPGRADE;
> 
> 
> 
>  ///
> 
> --
> 2.28.0.windows.1





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74379): https://edk2.groups.io/g/devel/message/74379
Mute This Topic: https://groups.io/mt/82300665/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] 回复: [PATCH v1 1/2] MdePkg:Update IndustryStandard/Nvme.h with Nvme amdin controller data

[gaoliming]

Cheng Zhou:
  Please update the commit message to highlight this change based on NVME1.3
spec. 

  With this update, Reviewed-by: Liming Gao 

Thanks
Liming
> -邮件原件-
> 发件人: zhoucheng 
> 发送时间: 2021年4月22日 10:11
> 收件人: devel@edk2.groups.io
> 抄送: Michael D Kinney ; Liming Gao
> 
> 主题: [PATCH v1 1/2] MdePkg:Update IndustryStandard/Nvme.h with Nvme
> amdin controller data
> 
> Add definition that describes the structure of Nvme admin controller data.
> 
> Signed-off-by: Cheng Zhou 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Reviewed-by: Zhiguang Liu 
> ---
>  MdePkg/Include/IndustryStandard/Nvme.h | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/MdePkg/Include/IndustryStandard/Nvme.h
> b/MdePkg/Include/IndustryStandard/Nvme.h
> index 9b19a2074b0d..61bf0fadc05c 100644
> --- a/MdePkg/Include/IndustryStandard/Nvme.h
> +++ b/MdePkg/Include/IndustryStandard/Nvme.h
> @@ -353,7 +353,13 @@ typedef struct {
>UINT8  Npss;/* Number of Power States Support */
>UINT8  Avscc;   /* Admin Vendor Specific Command
> Configuration */
>UINT8  Apsta;   /* Autonomous Power State Transition
> Attributes */
> -  UINT8  Rsvd2[246];  /* Reserved as of Nvm Express 1.1 Spec
> */
> +  UINT16 Wctemp;  /* Warning Composite Temperature
> Threshold */
> +  UINT16 Cctemp;  /* Critical Composite Temperature
> Threshold */
> +  UINT16 Mtfa;/* Maximum Time for Firmware
> Activation */
> +  UINT8  Hmpre[4];/* Host Memory Buffer Preferred Size
> */
> +  UINT8  Hmmin[4];/* Host Memory Buffer Minimum Size
> */
> +  UINT8  Tnvmcap[16]; /* Total NVM Capacity */
> +  UINT8  Rsvd2[216];  /* Reserved as of Nvm Express 1.3 Spec
> */
>//
>// NVM Command Set Attributes
>//
> --
> 1.9.1





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74380): https://edk2.groups.io/g/devel/message/74380
Mute This Topic: https://groups.io/mt/82300740/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] SecurityPkg: Add constraints on PK strength

[Min Xu]

This patch is good to me.
Reviewed-by: Min Xu 

> -Original Message-
> From: Gao, Jiaqi 
> Sent: Monday, April 19, 2021 9:31 AM
> To: Xu, Min M ; devel@edk2.groups.io
> Cc: Yao, Jiewen 
> Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength
> 
> Hi,
> 
> The patch has been built and tested with several toolchains:
> 1. GCC5 on Linux, both DEBUG and RELEASE.
> 2. VS2017 on Windows, both DEBUG and RELEASE.
> 3. VS2019 on Windows, both DEBUG and RELEASE.
> 
> To make sure the program can cope with various input, test cases consist of
> different PK certificate enrollment , which are:
> 1. Platform Keys (PKs) with RSA public key length less than 2048 bits, include
> RSA-512 and RSA-1024, etc. These kind of certificates were rejected during 
> user
> enrollment.
> 2. PKs with RSA public key length equal to or greater than 2048 bits, include 
> RSA-
> 2048, RSA-3072 and RSA-4096, etc. These kind of certificates were successfully
> enrolled.
> 3. PKs which are not DER encoded, such as PEM encoded certificates
> with .cer/.der/.crt file suffix.
> 4. Empty PKs.
> 5. Empty inputs.
> 
> All the test cases were performed as expected. Test cases with unqualified key
> strength pop up the prompt of unqualified key, and the others with unsupported
> encode format or illegal input act as previous program.
> 
> 
> Best Regards,
> Jiaqi
> 
> -Original Message-
> From: Xu, Min M 
> Sent: Monday, April 19, 2021 7:52 AM
> To: Gao, Jiaqi ; devel@edk2.groups.io
> Cc: Yao, Jiewen 
> Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength
> 
> Have you tested the patch? Would you please post the test result in the mail
> thread?
> Thanks.
> 
> > -Original Message-
> > From: Gao, Jiaqi 
> > Sent: Friday, April 16, 2021 3:56 PM
> > To: devel@edk2.groups.io
> > Cc: Gao, Jiaqi ; Xu, Min M ;
> > Yao, Jiewen 
> > Subject: [PATCH] SecurityPkg: Add constraints on PK strength
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3293
> >
> > Add constraints on the key strength of enrolled platform key(PK),
> > which must be greater than or equal to 2048 bit.PK key strength is
> > required by Intel SDL and MSFT, etc. This limitation prevents user from 
> > using
> weak keys as PK.
> >
> > The original code to check the certificate file type is placed in a
> > new function CheckX509Certificate(), which checks if the X.509
> > certificate meets the requirements of encode type, RSA-Key strengh, etc.
> >
> > Cc: Min Xu 
> > Cc: Jiewen Yao 
> > Signed-off-by: Jiaqi Gao 
> > ---
> >  .../SecureBootConfigImpl.c| 165 +++---
> >  .../SecureBootConfigImpl.h|  21 +++
> >  2 files changed, 160 insertions(+), 26 deletions(-)
> >
> > diff --git
> > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > igI
> > mpl.c
> > b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > igI
> > mpl.c
> > index 4f01a2ed67..1304e21266 100644
> > ---
> > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > igI
> > mpl.c
> > +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot
> > +++ Co
> > +++ nfigImpl.c
> > @@ -90,6 +90,22 @@ CHAR16* mDerEncodedSuffix[] = {  };
> >  CHAR16* mSupportX509Suffix = L"*.cer/der/crt";
> >
> > +//
> > +// Prompt strings during certificate enrollment.
> > +//
> > +CHAR16* mX509EnrollPromptTitle[] = {
> > +  L"",
> > +  L"ERROR: Unsupported file type!",
> > +  L"ERROR: Unsupported certificate!",
> > +  NULL
> > +};
> > +CHAR16* mX509EnrollPromptString[] = {
> > +  L"",
> > +  L"Only DER encoded certificate file (*.cer/der/crt) is supported.",
> > +  L"Public key length should be equal to or greater than 2048 bits.",
> > +  NULL
> > +};
> > +
> >  SECUREBOOT_CONFIG_PRIVATE_DATA  *gSecureBootPrivateData = NULL;
> >
> >  /**
> > @@ -383,6 +399,102 @@ SetSecureBootMode (
> >  );
> >  }
> >
> > +/**
> > +  This code checks if the encode type and key strength of X.509
> > +  certificate is qualified.
> > +
> > +  @param[in]  X509FileContext FileContext of X.509 certificate storing
> > +  file.
> > +  @param[out] Error   Error type checked in the certificate.
> > +
> > +  @return EFI_SUCCESS The certificate checked successfully.
> > +  @return EFI_INVALID_PARAMETER   The parameter is invalid.
> > +  @return EFI_OUT_OF_RESOURCESMemory allocation failed.
> > +
> > +**/
> > +EFI_STATUS
> > +CheckX509Certificate (
> > +  INSECUREBOOT_FILE_CONTEXT*X509FileContext,
> > +  OUT   ENROLL_KEY_ERROR*   Error
> > +)
> > +{
> > +  EFI_STATUS Status;
> > +  UINT16*FilePostFix;
> > +  UINTN  NameLength;
> > +  UINT8* X509Data;
> > +  UINTN  X509DataSize;
> > +  void*  X509PubKey;
> > +  UINTN  PubKeyModSize;
> > +
> > +  if (X509FileContext->FileName == NULL) {
> > +*Error = Unsupported_Type;
> > +return EFI_INVALID_PARAMETER;
> > +

Re: [edk2-devel] 回复: [PATCH v1 1/2] MdePkg:Update IndustryStandard/Nvme.h with Nvme amdin controller data

[Wu, Hao A]

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of
> gaoliming
> Sent: Friday, April 23, 2021 8:53 AM
> To: 'zhoucheng' ; devel@edk2.groups.io
> Cc: Kinney, Michael D 
> Subject: [edk2-devel] 回复: [PATCH v1 1/2] MdePkg:Update
> IndustryStandard/Nvme.h with Nvme amdin controller data
> 
> Cheng Zhou:
>   Please update the commit message to highlight this change based on
> NVME1.3 spec.
> 
>   With this update, Reviewed-by: Liming Gao 
> 
> Thanks
> Liming
> > -邮件原件-
> > 发件人: zhoucheng 
> > 发送时间: 2021年4月22日 10:11
> > 收件人: devel@edk2.groups.io
> > 抄送: Michael D Kinney ; Liming Gao
> > 
> > 主题: [PATCH v1 1/2] MdePkg:Update IndustryStandard/Nvme.h with
> Nvme
> > amdin controller data
> >
> > Add definition that describes the structure of Nvme admin controller data.
> >
> > Signed-off-by: Cheng Zhou 
> > Cc: Michael D Kinney 
> > Cc: Liming Gao 
> > Reviewed-by: Zhiguang Liu 
> > ---
> >  MdePkg/Include/IndustryStandard/Nvme.h | 8 +++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdePkg/Include/IndustryStandard/Nvme.h
> > b/MdePkg/Include/IndustryStandard/Nvme.h
> > index 9b19a2074b0d..61bf0fadc05c 100644
> > --- a/MdePkg/Include/IndustryStandard/Nvme.h
> > +++ b/MdePkg/Include/IndustryStandard/Nvme.h
> > @@ -353,7 +353,13 @@ typedef struct {
> >UINT8  Npss;/* Number of Power States Support */
> >UINT8  Avscc;   /* Admin Vendor Specific Command
> > Configuration */
> >UINT8  Apsta;   /* Autonomous Power State Transition
> > Attributes */
> > -  UINT8  Rsvd2[246];  /* Reserved as of Nvm Express 1.1 Spec
> > */
> > +  UINT16 Wctemp;  /* Warning Composite Temperature
> > Threshold */
> > +  UINT16 Cctemp;  /* Critical Composite Temperature
> > Threshold */
> > +  UINT16 Mtfa;/* Maximum Time for Firmware
> > Activation */
> > +  UINT8  Hmpre[4];/* Host Memory Buffer Preferred Size
> > */
> > +  UINT8  Hmmin[4];/* Host Memory Buffer Minimum Size
> > */


I did not see the origin patch was delivered to the mailing list. (Not sure if 
there is something wrong with the mail client on my side.)

One minor comment:
Do you think we can use UINT32 for definition 'Hmpre' & 'Hmmin'?

Best Regards,
Hao Wu


> > +  UINT8  Tnvmcap[16]; /* Total NVM Capacity */
> > +  UINT8  Rsvd2[216];  /* Reserved as of Nvm Express 1.3 Spec
> > */
> >//
> >// NVM Command Set Attributes
> >//
> > --
> > 1.9.1
> 
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74382): https://edk2.groups.io/g/devel/message/74382
Mute This Topic: https://groups.io/mt/82300740/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] SecurityPkg: Add constraints on PK strength

[Yao, Jiewen]

Acked-by: Jiewen Yao 

> -Original Message-
> From: Xu, Min M 
> Sent: Friday, April 23, 2021 9:36 AM
> To: Gao, Jiaqi ; devel@edk2.groups.io
> Cc: Yao, Jiewen 
> Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength
> 
> This patch is good to me.
> Reviewed-by: Min Xu 
> 
> > -Original Message-
> > From: Gao, Jiaqi 
> > Sent: Monday, April 19, 2021 9:31 AM
> > To: Xu, Min M ; devel@edk2.groups.io
> > Cc: Yao, Jiewen 
> > Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength
> >
> > Hi,
> >
> > The patch has been built and tested with several toolchains:
> > 1. GCC5 on Linux, both DEBUG and RELEASE.
> > 2. VS2017 on Windows, both DEBUG and RELEASE.
> > 3. VS2019 on Windows, both DEBUG and RELEASE.
> >
> > To make sure the program can cope with various input, test cases consist of
> > different PK certificate enrollment , which are:
> > 1. Platform Keys (PKs) with RSA public key length less than 2048 bits, 
> > include
> > RSA-512 and RSA-1024, etc. These kind of certificates were rejected during
> user
> > enrollment.
> > 2. PKs with RSA public key length equal to or greater than 2048 bits, 
> > include
> RSA-
> > 2048, RSA-3072 and RSA-4096, etc. These kind of certificates were
> successfully
> > enrolled.
> > 3. PKs which are not DER encoded, such as PEM encoded certificates
> > with .cer/.der/.crt file suffix.
> > 4. Empty PKs.
> > 5. Empty inputs.
> >
> > All the test cases were performed as expected. Test cases with unqualified
> key
> > strength pop up the prompt of unqualified key, and the others with
> unsupported
> > encode format or illegal input act as previous program.
> >
> >
> > Best Regards,
> > Jiaqi
> >
> > -Original Message-
> > From: Xu, Min M 
> > Sent: Monday, April 19, 2021 7:52 AM
> > To: Gao, Jiaqi ; devel@edk2.groups.io
> > Cc: Yao, Jiewen 
> > Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength
> >
> > Have you tested the patch? Would you please post the test result in the
> mail
> > thread?
> > Thanks.
> >
> > > -Original Message-
> > > From: Gao, Jiaqi 
> > > Sent: Friday, April 16, 2021 3:56 PM
> > > To: devel@edk2.groups.io
> > > Cc: Gao, Jiaqi ; Xu, Min M ;
> > > Yao, Jiewen 
> > > Subject: [PATCH] SecurityPkg: Add constraints on PK strength
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3293
> > >
> > > Add constraints on the key strength of enrolled platform key(PK),
> > > which must be greater than or equal to 2048 bit.PK key strength is
> > > required by Intel SDL and MSFT, etc. This limitation prevents user from
> using
> > weak keys as PK.
> > >
> > > The original code to check the certificate file type is placed in a
> > > new function CheckX509Certificate(), which checks if the X.509
> > > certificate meets the requirements of encode type, RSA-Key strengh, etc.
> > >
> > > Cc: Min Xu 
> > > Cc: Jiewen Yao 
> > > Signed-off-by: Jiaqi Gao 
> > > ---
> > >  .../SecureBootConfigImpl.c| 165 +++---
> > >  .../SecureBootConfigImpl.h|  21 +++
> > >  2 files changed, 160 insertions(+), 26 deletions(-)
> > >
> > > diff --git
> > >
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > > igI
> > > mpl.c
> > >
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > > igI
> > > mpl.c
> > > index 4f01a2ed67..1304e21266 100644
> > > ---
> > >
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > > igI
> > > mpl.c
> > > +++
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot
> > > +++ Co
> > > +++ nfigImpl.c
> > > @@ -90,6 +90,22 @@ CHAR16* mDerEncodedSuffix[] = {  };
> > >  CHAR16* mSupportX509Suffix = L"*.cer/der/crt";
> > >
> > > +//
> > > +// Prompt strings during certificate enrollment.
> > > +//
> > > +CHAR16* mX509EnrollPromptTitle[] = {
> > > +  L"",
> > > +  L"ERROR: Unsupported file type!",
> > > +  L"ERROR: Unsupported certificate!",
> > > +  NULL
> > > +};
> > > +CHAR16* mX509EnrollPromptString[] = {
> > > +  L"",
> > > +  L"Only DER encoded certificate file (*.cer/der/crt) is supported.",
> > > +  L"Public key length should be equal to or greater than 2048 bits.",
> > > +  NULL
> > > +};
> > > +
> > >  SECUREBOOT_CONFIG_PRIVATE_DATA  *gSecureBootPrivateData = NULL;
> > >
> > >  /**
> > > @@ -383,6 +399,102 @@ SetSecureBootMode (
> > >  );
> > >  }
> > >
> > > +/**
> > > +  This code checks if the encode type and key strength of X.509
> > > +  certificate is qualified.
> > > +
> > > +  @param[in]  X509FileContext FileContext of X.509 certificate 
> > > storing
> > > +  file.
> > > +  @param[out] Error   Error type checked in the certificate.
> > > +
> > > +  @return EFI_SUCCESS The certificate checked successfully.
> > > +  @return EFI_INVALID_PARAMETER   The parameter is invalid.
> > > +  @return EFI_OUT_OF_RESOURCESMemory allocation failed.
> > > +
> > > +**/
> > > +EFI_STATUS