[edk2-devel] [PATCH] Platform/ARM/VExpressPkg: Fix unused but set

2021-04-23 Thread Adrián Herrera 
Remove unused but set variables in GetArmNameSpaceObject. These caused a
build crash due to -Werror=unused-but-set-variable.

Signed-off-by: Adrián Herrera 
---
 .../ConfigurationManagerDxe/ConfigurationManager.c  | 6 --
 1 file changed, 6 deletions(-)

diff --git 
a/Platform/ARM/VExpressPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
 
b/Platform/ARM/VExpressPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
index e99fbb654f..d169cd2c5d 100644
--- 
a/Platform/ARM/VExpressPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
+++ 
b/Platform/ARM/VExpressPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
@@ -721,8 +721,6 @@ GetArmNameSpaceObject (
   UINTN Smmuv3Count;

   UINTN ItsGroupCount;

   UINTN ItsIdentifierArrayCount;

-  UINTN RootComplexCount;

-  UINTN DeviceIdMappingArrayCount;

   UINTN PciConfigSpaceCount;

 

   if ((This == NULL) || (CmObject == NULL)) {

@@ -739,15 +737,11 @@ GetArmNameSpaceObject (
 Smmuv3Count = 1;

 ItsGroupCount = 1;

 ItsIdentifierArrayCount = ARRAY_SIZE (PlatformRepo->ItsIdentifierArray);

-RootComplexCount = 1;

-DeviceIdMappingArrayCount = ARRAY_SIZE (PlatformRepo->DeviceIdMapping);

 PciConfigSpaceCount = 1;

   } else {

 Smmuv3Count = 0;

 ItsGroupCount = 0;

 ItsIdentifierArrayCount = 0;

-RootComplexCount = 0;

-DeviceIdMappingArrayCount = 0;

 PciConfigSpaceCount = 0;

   }

 

-- 
2.30.0



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

[edk2-devel] [PATCH] ArmPkg/ArmGicLib: fix maximum interrupts supported

2021-04-23 Thread Adrián Herrera 
The maximum number of interrupts supported is determined as
32 * (GICD_TYPER.ITLinesNumber + 1).

When GICD_TYPER.ITLinesNumber = 0b1, the maximum number of
interrupts supported is 1024. However, both GICv2 and GICv3 reserve
INTIDs 1020-1023 for special purposes.

This results in runtime crashes because:
  (1) ArmGicLib functions do not guard against special interrupts.
  (2) ArmGicGetMaxNumInterrupts number includes special interrupts.
  (2) ArmGicV*Dxe relies on ArmGicGetMaxNumInterrupts, and thus
  programs special interrupts through ArmGicLib.

ArmGicGetMaxNumInterrupts now does not include special interrupts, that
is, it reports 1020 instead of 1024 when GICD_TYPER.ITLinesNumber = 0b1.
This avoids the overhead of guarding ArmGicLib functions while not
requiring to modify the drivers code.

Signed-off-by: Adrián Herrera 
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c 
b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 6b01c88206..dff1401e9c 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -120,7 +120,10 @@ ArmGicGetMaxNumInterrupts (
   IN  INTN  GicDistributorBase

   )

 {

-  return 32 * ((MmioRead32 (GicDistributorBase + ARM_GIC_ICDICTR) & 0x1F) + 1);

+  UINT32 ITLinesNumber;

+

+  ITLinesNumber = MmioRead32 (GicDistributorBase + ARM_GIC_ICDICTR) & 0x1F;

+  return MIN (32 * (ITLinesNumber+ 1), 1020);

 }

 

 VOID

-- 
2.30.0



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

Re: [edk2-devel] [edk2-platforms PATCH v2 1/1] SbsaQemu: Add OemMiscLib boot information and chassis status functions

2021-04-23 Thread Rebecca Cran 
v2 has no changes other than adding a newline at the end of 
Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c and reverting the change 
to SbsaQemu.dec.


--
Rebecca Cran

On 4/23/21 3:57 PM, Rebecca Cran wrote:

Add new SMBIOS Type 32 boot information and Type 3 chassis status
functions that have been added to OemMiscLib in ArmPkg.

Since this is a virtual platform, return fixed values for the chassis
statuses.

Signed-off-by: Rebecca Cran 
---
  Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c | 91 
  1 file changed, 91 insertions(+)

diff --git a/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c 
b/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c
index eb405b259848..326bb56bcfa3 100644
--- a/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c
+++ b/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c
@@ -239,3 +239,94 @@ OemUpdateSmbiosInfo (
  HiiSetString (HiiHandle, TokenToUpdate, String, NULL);
}
  }
+
+/** Fetches the Type 32 boot information status.
+
+  @return Boot status.
+**/
+MISC_BOOT_INFORMATION_STATUS_DATA_TYPE
+EFIAPI
+OemGetBootStatus (
+  VOID
+  )
+{
+  return BootInformationStatusNoError;
+}
+
+/** Fetches the chassis status when it was last booted.
+
+ @return Chassis status.
+**/
+MISC_CHASSIS_STATE
+EFIAPI
+OemGetChassisBootupState (
+  VOID
+  )
+{
+  return ChassisStateSafe;
+}
+
+/** Fetches the chassis power supply/supplies status when last booted.
+
+ @return Chassis power supply/supplies status.
+**/
+MISC_CHASSIS_STATE
+EFIAPI
+OemGetChassisPowerSupplyState (
+  VOID
+  )
+{
+  return ChassisStateSafe;
+}
+
+/** Fetches the chassis thermal status when last booted.
+
+ @return Chassis thermal status.
+**/
+MISC_CHASSIS_STATE
+EFIAPI
+OemGetChassisThermalState (
+  VOID
+  )
+{
+  return ChassisStateSafe;
+}
+
+/** Fetches the chassis security status when last booted.
+
+ @return Chassis security status.
+**/
+MISC_CHASSIS_SECURITY_STATE
+EFIAPI
+OemGetChassisSecurityStatus (
+  VOID
+  )
+{
+  return ChassisSecurityStatusNone;
+}
+
+/** Fetches the chassis height in RMUs (Rack Mount Units).
+
+  @return The height of the chassis.
+**/
+UINT8
+EFIAPI
+OemGetChassisHeight (
+  VOID
+  )
+{
+  return 1U;
+}
+
+/** Fetches the number of power cords.
+
+  @return The number of power cords.
+**/
+UINT8
+EFIAPI
+OemGetChassisNumPowerCords (
+  VOID
+  )
+{
+  return 1;
+}





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

[edk2-devel] [edk2-platforms PATCH v2 1/1] SbsaQemu: Add OemMiscLib boot information and chassis status functions

2021-04-23 Thread Rebecca Cran 
Add new SMBIOS Type 32 boot information and Type 3 chassis status
functions that have been added to OemMiscLib in ArmPkg.

Since this is a virtual platform, return fixed values for the chassis
statuses.

Signed-off-by: Rebecca Cran 
---
 Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c | 91 
 1 file changed, 91 insertions(+)

diff --git a/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c 
b/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c
index eb405b259848..326bb56bcfa3 100644
--- a/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c
+++ b/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c
@@ -239,3 +239,94 @@ OemUpdateSmbiosInfo (
 HiiSetString (HiiHandle, TokenToUpdate, String, NULL);
   }
 }
+
+/** Fetches the Type 32 boot information status.
+
+  @return Boot status.
+**/
+MISC_BOOT_INFORMATION_STATUS_DATA_TYPE
+EFIAPI
+OemGetBootStatus (
+  VOID
+  )
+{
+  return BootInformationStatusNoError;
+}
+
+/** Fetches the chassis status when it was last booted.
+
+ @return Chassis status.
+**/
+MISC_CHASSIS_STATE
+EFIAPI
+OemGetChassisBootupState (
+  VOID
+  )
+{
+  return ChassisStateSafe;
+}
+
+/** Fetches the chassis power supply/supplies status when last booted.
+
+ @return Chassis power supply/supplies status.
+**/
+MISC_CHASSIS_STATE
+EFIAPI
+OemGetChassisPowerSupplyState (
+  VOID
+  )
+{
+  return ChassisStateSafe;
+}
+
+/** Fetches the chassis thermal status when last booted.
+
+ @return Chassis thermal status.
+**/
+MISC_CHASSIS_STATE
+EFIAPI
+OemGetChassisThermalState (
+  VOID
+  )
+{
+  return ChassisStateSafe;
+}
+
+/** Fetches the chassis security status when last booted.
+
+ @return Chassis security status.
+**/
+MISC_CHASSIS_SECURITY_STATE
+EFIAPI
+OemGetChassisSecurityStatus (
+  VOID
+  )
+{
+  return ChassisSecurityStatusNone;
+}
+
+/** Fetches the chassis height in RMUs (Rack Mount Units).
+
+  @return The height of the chassis.
+**/
+UINT8
+EFIAPI
+OemGetChassisHeight (
+  VOID
+  )
+{
+  return 1U;
+}
+
+/** Fetches the number of power cords.
+
+  @return The number of power cords.
+**/
+UINT8
+EFIAPI
+OemGetChassisNumPowerCords (
+  VOID
+  )
+{
+  return 1;
+}
-- 
2.26.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74399): https://edk2.groups.io/g/devel/message/74399
Mute This Topic: https://groups.io/mt/82322803/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

2021-04-23 Thread Lendacky, Thomas 
On 4/23/21 12:41 PM, Tom Lendacky wrote:
> On 4/23/21 8:04 AM, Laszlo Ersek wrote:
>> On 04/23/21 12:26, Laszlo Ersek wrote:
>>> review#2 from scratch:
>>>
>>> On 04/21/21 00:54, Tom Lendacky 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%7Ca24cbb3f52404e466fe808d906585034%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637547798659640707%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=o2rAydWG7jenRhqbZx3oDffZhlCUimJL9f9Tpmy4etk%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) As discussed, please update the commit message, for more clarify
>>> about SEV vs. SEV-ES.
>>>

 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) {
>>>
>>> It's OK to keep this as a sanity check, yes.
>>>
 +RETURN_STATUS  DecryptStatus;
 +
 +DecryptStatus = MemEncryptSevClearPageEncMask (
 +  0,
 +  TpmBaseAddress,
 +  EFI_SIZE_TO_PAGES (0x5000),
>>>
>>> (2) Should be (UINTN)0x5000, as discussed earlier.
>>>
 +  FALSE
 +  );
 +
 +ASSERT_RETURN_ERROR (DecryptStatus);
>>>
>>> (3) So this is where the mess begins.
>>>
>>> The idea is to delay the dispatch of Tcg2ConfigPei until after
>>> PlatformPei determines if SEV is active, and (in case SEV is active)
>>> PlatformPei decrypts the MMIO range of the TPM.
>>>
>>> For this, we need to change the IA32 / X64 DEPEX of Tcg2ConfigPei, from
>>> the current TRUE, to some PPI GUID.
>>>
>>> There are two choices for that PPI:
>>>
>>> (a) gEfiPeiMemoryDiscoveredPpiGuid
>>>
>>> Advantages:
>>>
>>> - no new PPI definition needed,
>>>
>>> - no new PPI installation needed,
>>>
>>> - OvmfPkg/Bhyve/PlatformPei needs no separate change
>>>
>>> Disadvantages:
>>>
>>> - total abuse of gEfiPeiMemoryDiscoveredPpiGuid
>>>
>>>
>>> (b) gOvmfTpmMmioAccessiblePpiGuid
>>>
>>> Disadvantages:
>>>
>>> - this new GUID must be defined in "OvmfPkg.dec", in the [Ppis] section,
>>> in a separate patch; its comment should say "this PPI signals that
>>> accessing the MMIO range of the TPM is possible in the PEI phase,
>>> regardless of memory encryption". The PPI definitions should be kept
>>> alphabetically ordered.
>>>
>>> - OvmfPkg/PlatformPei must install this new PPI, with a NULL interface.
>>> (See "mPpiBootMode" as a technical example.) OvmfPkg/PlatformPei must
>>> install this new PPI either when the SEV check at the top of
>>> AmdSevInitialize() fails, or when MemEncryptSevClearPageEncMask() succeeds.
>>>
>>> - OvmfPkg/Bhyve/PlatformPei must receive the same update, in a separate
>>> patch. That's because "OvmfPkg/Bhyve/BhyveX64.fdf" includes the same
>>> "Tcg2ConfigPei", but consumes "OvmfPkg/Bhyve/PlatformPei" rather than
>>> "OvmfPkg/Plat

Re: [edk2-devel] [edk2-platforms PATCH 1/1] SbsaQemu: Add OemMiscLib boot information and chassis status functions

2021-04-23 Thread Leif Lindholm 
On Fri, Apr 16, 2021 at 16:26:49 -0600, Rebecca Cran wrote:
> Add new SMBIOS Type 32 boot information and Type 3 chassis status
> functions that have been added to OemMiscLib in ArmPkg.
> 
> Since this is a virtual platform, return fixed values for the chassis
> statuses.
> 
> Signed-off-by: Rebecca Cran 
> ---
>  Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c | 91 
>  Silicon/Qemu/SbsaQemu/SbsaQemu.dec |  2 +-
>  2 files changed, 92 insertions(+), 1 deletion(-)
> 
> diff --git a/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c 
> b/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c
> index eb405b259848..b543045de3b7 100644
> --- a/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c
> +++ b/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c
> @@ -239,3 +239,94 @@ OemUpdateSmbiosInfo (
>  HiiSetString (HiiHandle, TokenToUpdate, String, NULL);
>}
>  }
> +
> +/** Fetches the Type 32 boot information status.
> +
> +  @return Boot status.
> +**/
> +MISC_BOOT_INFORMATION_STATUS_DATA_TYPE
> +EFIAPI
> +OemGetBootStatus (
> +  VOID
> +  )
> +{
> +  return BootInformationStatusNoError;
> +}
> +
> +/** Fetches the chassis status when it was last booted.
> +
> + @return Chassis status.
> +**/
> +MISC_CHASSIS_STATE
> +EFIAPI
> +OemGetChassisBootupState (
> +  VOID
> +  )
> +{
> +  return ChassisStateSafe;
> +}
> +
> +/** Fetches the chassis power supply/supplies status when last booted.
> +
> + @return Chassis power supply/supplies status.
> +**/
> +MISC_CHASSIS_STATE
> +EFIAPI
> +OemGetChassisPowerSupplyState (
> +  VOID
> +  )
> +{
> +  return ChassisStateSafe;
> +}
> +
> +/** Fetches the chassis thermal status when last booted.
> +
> + @return Chassis thermal status.
> +**/
> +MISC_CHASSIS_STATE
> +EFIAPI
> +OemGetChassisThermalState (
> +  VOID
> +  )
> +{
> +  return ChassisStateSafe;
> +}
> +
> +/** Fetches the chassis security status when last booted.
> +
> + @return Chassis security status.
> +**/
> +MISC_CHASSIS_SECURITY_STATE
> +EFIAPI
> +OemGetChassisSecurityStatus (
> +  VOID
> +  )
> +{
> +  return ChassisSecurityStatusNone;
> +}
> +
> +/** Fetches the chassis height in RMUs (Rack Mount Units).
> +
> +  @return The height of the chassis.
> +**/
> +UINT8
> +EFIAPI
> +OemGetChassisHeight (
> +  VOID
> +  )
> +{
> +  return 1U;
> +}
> +
> +/** Fetches the number of power cords.
> +
> +  @return The number of power cords.
> +**/
> +UINT8
> +EFIAPI
> +OemGetChassisNumPowerCords (
> +  VOID
> +  )
> +{
> +  return 1;
> +}
> \ No newline at end of file
> diff --git a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec 
> b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> index 9448852967b6..6051fabd683a 100644
> --- a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> +++ b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> @@ -66,4 +66,4 @@
>
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisVersion|L""|VOID*|0x011A
>
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisManufacturer|L""|VOID*|0x011B
>
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisAssetTag|L""|VOID*|0x011C
> -  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisSKU|L""|VOID*|0x011D
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisSKU|L""|VOID*|0x011D
> \ No newline at end of file

Code looks fine, but can you please resend with the two 'no newline at
end of file's fixed?

/
Leif

> -- 
> 2.26.2
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74397): https://edk2.groups.io/g/devel/message/74397
Mute This Topic: https://groups.io/mt/82154496/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

2021-04-23 Thread Lendacky, Thomas 
On 4/23/21 8:04 AM, Laszlo Ersek wrote:
> On 04/23/21 12:26, Laszlo Ersek wrote:
>> review#2 from scratch:
>>
>> On 04/21/21 00:54, Tom Lendacky 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%7Ca24cbb3f52404e466fe808d906585034%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637547798659640707%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=o2rAydWG7jenRhqbZx3oDffZhlCUimJL9f9Tpmy4etk%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) As discussed, please update the commit message, for more clarify
>> about SEV vs. SEV-ES.
>>
>>>
>>> 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) {
>>
>> It's OK to keep this as a sanity check, yes.
>>
>>> +RETURN_STATUS  DecryptStatus;
>>> +
>>> +DecryptStatus = MemEncryptSevClearPageEncMask (
>>> +  0,
>>> +  TpmBaseAddress,
>>> +  EFI_SIZE_TO_PAGES (0x5000),
>>
>> (2) Should be (UINTN)0x5000, as discussed earlier.
>>
>>> +  FALSE
>>> +  );
>>> +
>>> +ASSERT_RETURN_ERROR (DecryptStatus);
>>
>> (3) So this is where the mess begins.
>>
>> The idea is to delay the dispatch of Tcg2ConfigPei until after
>> PlatformPei determines if SEV is active, and (in case SEV is active)
>> PlatformPei decrypts the MMIO range of the TPM.
>>
>> For this, we need to change the IA32 / X64 DEPEX of Tcg2ConfigPei, from
>> the current TRUE, to some PPI GUID.
>>
>> There are two choices for that PPI:
>>
>> (a) gEfiPeiMemoryDiscoveredPpiGuid
>>
>> Advantages:
>>
>> - no new PPI definition needed,
>>
>> - no new PPI installation needed,
>>
>> - OvmfPkg/Bhyve/PlatformPei needs no separate change
>>
>> Disadvantages:
>>
>> - total abuse of gEfiPeiMemoryDiscoveredPpiGuid
>>
>>
>> (b) gOvmfTpmMmioAccessiblePpiGuid
>>
>> Disadvantages:
>>
>> - this new GUID must be defined in "OvmfPkg.dec", in the [Ppis] section,
>> in a separate patch; its comment should say "this PPI signals that
>> accessing the MMIO range of the TPM is possible in the PEI phase,
>> regardless of memory encryption". The PPI definitions should be kept
>> alphabetically ordered.
>>
>> - OvmfPkg/PlatformPei must install this new PPI, with a NULL interface.
>> (See "mPpiBootMode" as a technical example.) OvmfPkg/PlatformPei must
>> install this new PPI either when the SEV check at the top of
>> AmdSevInitialize() fails, or when MemEncryptSevClearPageEncMask() succeeds.
>>
>> - OvmfPkg/Bhyve/PlatformPei must receive the same update, in a separate
>> patch. That's because "OvmfPkg/Bhyve/BhyveX64.fdf" includes the same
>> "Tcg2ConfigPei", but consumes "OvmfPkg/Bhyve/PlatformPei" rather than
>> "OvmfPkg/PlatformPei". Tcg2ConfigPei will receive the same
>> stricter-than-before depex, so something on the bhyve platform too must
>> produce the new PPI.
>>
>> Advantages:
>>
>> - mor

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

2021-04-23 Thread Lendacky, Thomas 
On 4/23/21 4:10 AM, Laszlo Ersek wrote:
> On 04/22/21 17:42, Tom Lendacky wrote:
>> 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%7Ca0ff778cdbb146a6186508d90637ba60%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637547658699539157%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=vMmmGGXXT8MtM4WOB4gkwtkr0cwwkx98LoOXYg6fclQ%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?
> 
> Yes, I had the exact same thought process :)
> 
> The comment looks good, but how about expressing it as a STATIC_ASSERT,
> with sizeof (UINTN) and sizeof (UINT64) being equal? (Alternatively,
> about MAX_UINT64 being equal to MAX_UINTN.)

I like the STATIC_ASSERT idea, I'll do that.

Thanks,
Tom

> 
> If you find that too verbose, a comment is good enough too, of course.
> 
> Thanks!
> Laszlo
> 
>>
>> 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)

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

2021-04-23 Thread Laszlo Ersek 
On 04/23/21 15:04, Laszlo Ersek wrote:

> There are several advantages to such a separate PEIM:
> 
> - For Bhyve, the update is minimal. Just include one line in each of the
> FDF and the DSC files. No need to customize an existent
> platform-specific PEIM, no code duplication between two PlatformPei modules.

OTOH, you'd have to update OvmfPkg/AmdSev/AmdSevX64.* too.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74394): https://edk2.groups.io/g/devel/message/74394
Mute This Topic: https://groups.io/mt/82247968/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

2021-04-23 Thread Laszlo Ersek 
On 04/23/21 12:26, Laszlo Ersek wrote:
> review#2 from scratch:
> 
> On 04/21/21 00:54, Tom Lendacky 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) As discussed, please update the commit message, for more clarify
> about SEV vs. SEV-ES.
> 
>>
>> 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) {
> 
> It's OK to keep this as a sanity check, yes.
> 
>> +RETURN_STATUS  DecryptStatus;
>> +
>> +DecryptStatus = MemEncryptSevClearPageEncMask (
>> +  0,
>> +  TpmBaseAddress,
>> +  EFI_SIZE_TO_PAGES (0x5000),
> 
> (2) Should be (UINTN)0x5000, as discussed earlier.
> 
>> +  FALSE
>> +  );
>> +
>> +ASSERT_RETURN_ERROR (DecryptStatus);
> 
> (3) So this is where the mess begins.
> 
> The idea is to delay the dispatch of Tcg2ConfigPei until after
> PlatformPei determines if SEV is active, and (in case SEV is active)
> PlatformPei decrypts the MMIO range of the TPM.
> 
> For this, we need to change the IA32 / X64 DEPEX of Tcg2ConfigPei, from
> the current TRUE, to some PPI GUID.
> 
> There are two choices for that PPI:
> 
> (a) gEfiPeiMemoryDiscoveredPpiGuid
> 
> Advantages:
> 
> - no new PPI definition needed,
> 
> - no new PPI installation needed,
> 
> - OvmfPkg/Bhyve/PlatformPei needs no separate change
> 
> Disadvantages:
> 
> - total abuse of gEfiPeiMemoryDiscoveredPpiGuid
> 
> 
> (b) gOvmfTpmMmioAccessiblePpiGuid
> 
> Disadvantages:
> 
> - this new GUID must be defined in "OvmfPkg.dec", in the [Ppis] section,
> in a separate patch; its comment should say "this PPI signals that
> accessing the MMIO range of the TPM is possible in the PEI phase,
> regardless of memory encryption". The PPI definitions should be kept
> alphabetically ordered.
> 
> - OvmfPkg/PlatformPei must install this new PPI, with a NULL interface.
> (See "mPpiBootMode" as a technical example.) OvmfPkg/PlatformPei must
> install this new PPI either when the SEV check at the top of
> AmdSevInitialize() fails, or when MemEncryptSevClearPageEncMask() succeeds.
> 
> - OvmfPkg/Bhyve/PlatformPei must receive the same update, in a separate
> patch. That's because "OvmfPkg/Bhyve/BhyveX64.fdf" includes the same
> "Tcg2ConfigPei", but consumes "OvmfPkg/Bhyve/PlatformPei" rather than
> "OvmfPkg/PlatformPei". Tcg2ConfigPei will receive the same
> stricter-than-before depex, so something on the bhyve platform too must
> produce the new PPI.
> 
> Advantages:
> 
> - more or less palatable as a concept, with the new PPI precisely
> expressing the dependency we have.
> 
> 
> In approach (b), the "OvmfPkg/Bhyve/PlatformPei" patch needs to be CC'd
> to the Bhyve reviewers. If the Bhyve reviewers determine that such an
> update is actually unnecessary, because on Bhyve, there is no TPM
> support and/or no SEV support in fact, then *first* we have to create an
> independent Bhyve cleanup series, that rips out the TPM and/or SEV
> remnants from the OvmfPkg/Bhyve sub-tree.
> 
> 
> I prefer

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

2021-04-23 Thread Laszlo Ersek 
On 04/22/21 17:05, Erdem Aktas via groups.io wrote:
> 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 
> 

Reviewed-by: Laszlo Ersek 

Merged as commit 61680cac5e43, via
.

Thanks,
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74392): https://edk2.groups.io/g/devel/message/74392
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/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

2021-04-23 Thread Laszlo Ersek 
Hi Jianyong,

On 04/22/21 15:56, Laszlo Ersek wrote:

> (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. :/)

In an attempt to approach this constructively, I've given it more
thought. Does "CloudHv" sound acceptable to the community? I've seen
"hv" stand for "hypervisor" frequently.


I have another high-level note. I could delay it until after you post
v2, but I figure I could save you some time by sharing my observation
with you right now.

I think that the ACPI platform stuff, in patch#2, does not belong in
OvmfPkg/AcpiPlatformDxe. What's more, I don't think it belongs in
OvmfPkg, even.

The CloudHvAcpiPlatformDxe and CloudHvPlatformHasAcpiDtDxe drivers
should exist as stand-alone, self-contained drivers; they should be as
minimal as possible. This is already a given for
"CloudHvPlatformHasAcpiDtDxe", but it should also be possible for
"CloudHvAcpiPlatformDxe". OvmfPkg/AcpiPlatformDxe is a complex driver,
and the overlap between what OvmfPkg/AcpiPlatformDxe currently does, and
what CloudHvAcpiPlatformDxe actually *needs*, is virtually nil.

And so, the series shouldn't touch OvmfPkg at all.

Ultimately I suggest following the Xen pattern that can be seen under
ArmVirtPkg already. In detail, the following files and directories
should contain the new platform:

  ArmVirtPkg/ArmVirtCloudHv.dsc
  ArmVirtPkg/ArmVirtCloudHv.fdf
  ArmVirtPkg/CloudHvAcpiPlatformDxe/
  ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/
  ArmVirtPkg/Library/CloudHvVirtMemInfoLib/

(And I don't really see the point of an FDF include file.)

Thanks!
Laszlo



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

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

2021-04-23 Thread 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

2021-04-23 Thread Laszlo Ersek 
review#2 from scratch:

On 04/21/21 00:54, Tom Lendacky 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) As discussed, please update the commit message, for more clarify
about SEV vs. SEV-ES.

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

It's OK to keep this as a sanity check, yes.

> +RETURN_STATUS  DecryptStatus;
> +
> +DecryptStatus = MemEncryptSevClearPageEncMask (
> +  0,
> +  TpmBaseAddress,
> +  EFI_SIZE_TO_PAGES (0x5000),

(2) Should be (UINTN)0x5000, as discussed earlier.

> +  FALSE
> +  );
> +
> +ASSERT_RETURN_ERROR (DecryptStatus);

(3) So this is where the mess begins.

The idea is to delay the dispatch of Tcg2ConfigPei until after
PlatformPei determines if SEV is active, and (in case SEV is active)
PlatformPei decrypts the MMIO range of the TPM.

For this, we need to change the IA32 / X64 DEPEX of Tcg2ConfigPei, from
the current TRUE, to some PPI GUID.

There are two choices for that PPI:

(a) gEfiPeiMemoryDiscoveredPpiGuid

Advantages:

- no new PPI definition needed,

- no new PPI installation needed,

- OvmfPkg/Bhyve/PlatformPei needs no separate change

Disadvantages:

- total abuse of gEfiPeiMemoryDiscoveredPpiGuid


(b) gOvmfTpmMmioAccessiblePpiGuid

Disadvantages:

- this new GUID must be defined in "OvmfPkg.dec", in the [Ppis] section,
in a separate patch; its comment should say "this PPI signals that
accessing the MMIO range of the TPM is possible in the PEI phase,
regardless of memory encryption". The PPI definitions should be kept
alphabetically ordered.

- OvmfPkg/PlatformPei must install this new PPI, with a NULL interface.
(See "mPpiBootMode" as a technical example.) OvmfPkg/PlatformPei must
install this new PPI either when the SEV check at the top of
AmdSevInitialize() fails, or when MemEncryptSevClearPageEncMask() succeeds.

- OvmfPkg/Bhyve/PlatformPei must receive the same update, in a separate
patch. That's because "OvmfPkg/Bhyve/BhyveX64.fdf" includes the same
"Tcg2ConfigPei", but consumes "OvmfPkg/Bhyve/PlatformPei" rather than
"OvmfPkg/PlatformPei". Tcg2ConfigPei will receive the same
stricter-than-before depex, so something on the bhyve platform too must
produce the new PPI.

Advantages:

- more or less palatable as a concept, with the new PPI precisely
expressing the dependency we have.


In approach (b), the "OvmfPkg/Bhyve/PlatformPei" patch needs to be CC'd
to the Bhyve reviewers. If the Bhyve reviewers determine that such an
update is actually unnecessary, because on Bhyve, there is no TPM
support and/or no SEV support in fact, then *first* we have to create an
independent Bhyve cleanup series, that rips out the TPM and/or SEV
remnants from the OvmfPkg/Bhyve sub-tree.


I prefer approach (b). I'm sorry that it means extra work wrt. Bhyve,
but I strongly believe in keeping all platforms in the tree, and that
means we need to spend time on such changes.

I'm not CC'ing Rebecca and Peter on this message -- we're deep into this
patch r

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

2021-04-23 Thread Laszlo Ersek 
On 04/22/21 21:10, Tom Lendacky wrote:
> 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 - 
>  

Thank you for checking this approach.

Let me re-review this patch from scratch.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74388): https://edk2.groups.io/g/devel/message/74388
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 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes

2021-04-23 Thread Laszlo Ersek 
On 04/22/21 17:42, Tom Lendacky wrote:
> 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?

Yes, I had the exact same thought process :)

The comment looks good, but how about expressing it as a STATIC_ASSERT,
with sizeof (UINTN) and sizeof (UINT64) being equal? (Alternatively,
about MAX_UINT64 being equal to MAX_UINTN.)

If you find that too verbose, a comment is good enough too, of course.

Thanks!
Laszlo

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

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

2021-04-23 Thread Laszlo Ersek 
On 04/22/21 15:35, Tom Lendacky wrote:
> 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.

Ugh, sorry, I must have completely lost track of the context here. I
apologize.

In that case however, it should be DEBUG_ERROR.

Thanks,
Laszlo

> 
>>
>> (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 (#74386): https://edk2.groups.io/g/devel/message/74386
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/2] BaseTools: Make undefined VFR macro an error (GCC)

2021-04-23 Thread Daniel Schaefer 
Ok sure, let's make only undef an error, not all other warnings. Then the 
behaviour will also be the same as on MSVC.

From: devel@edk2.groups.io  on behalf of Daniel Schaefer 

Sent: Monday, March 8, 2021 11:44
To: Feng, Bob C ; devel@edk2.groups.io 

Cc: Liming Gao ; Chen, Christine 
; Lin, Derek (HPS SW) 
Subject: Re: [edk2-devel] [PATCH v1 1/2] BaseTools: Make undefined VFR macro an 
error (GCC)

It didn't cause any other errors for the huge HPE codebase. Only undefined 
macros. I don't believe the preprocessor has so many warnings anyways.
So -Werror should be fine.


From: Feng, Bob C 
Sent: Monday, March 8, 2021 09:05
To: devel@edk2.groups.io ; Schaefer, Daniel 

Cc: Liming Gao ; Chen, Christine 
; Lin, Derek (HPS SW) 
Subject: RE: [edk2-devel] [PATCH v1 1/2] BaseTools: Make undefined VFR macro an 
error (GCC)

Hi Derek,

-Werror.  Make all warnings into errors.

Should here be that only treat undef warning as error?

Thanks,
Bob

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Daniel Schaefer
Sent: Tuesday, March 2, 2021 4:22 PM
To: devel@edk2.groups.io
Cc: Feng, Bob C ; Liming Gao ; 
Chen, Christine ; Derek Lin 
Subject: [edk2-devel] [PATCH v1 1/2] BaseTools: Make undefined VFR macro an 
error (GCC)

VFR successfully compiles if we forget to include a header that defines a 
macro. In that case the HII option was hidden when it shouldn't be just because 
the macro was used but not defined.

The behaviour is totally intended by the C/PP standard. When a macro is 
undefined it evaluates to 0.
GCC, MSVC and Clang have warnings to catch this type of mistake. With this 
commit we enable this warning and make it a compiler error.

Cc: Bob Feng 
Cc: Liming Gao 
Cc: Yuwei Chen 
Cc: Derek Lin 
---
 BaseTools/Conf/tools_def.template | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Conf/tools_def.template 
b/BaseTools/Conf/tools_def.template
index 933b3160fd2b..728c1d3119e4 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -3,7 +3,7 @@
 #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.  #  
Portions copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.  #  
Copyright (c) 2015, Hewlett-Packard Development Company, L.P. -#  (C) 
Copyright 2020, Hewlett Packard Enterprise Development LP
+#  (C) Copyright 2020-2021, Hewlett Packard Enterprise Development
+LP
 #  Copyright (c) Microsoft Corporation
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent @@ -1938,7 +1938,7 @@ DEFINE 
GCC_AARCH64_ASLDLINK_FLAGS  = DEF(GCC_AARCH64_DLINK_FLAGS) -Wl,--entry,Re
 DEFINE GCC_IA32_X64_DLINK_FLAGS= DEF(GCC_IA32_X64_DLINK_COMMON) --entry 
_$(IMAGE_ENTRY_POINT) --file-alignment 0x20 --section-alignment 0x20 -Map 
$(DEST_DIR_DEBUG)/$(BASE_NAME).map
 DEFINE GCC_ASM_FLAGS   = -c -x assembler -imacros AutoGen.h
 DEFINE GCC_PP_FLAGS= -E -x assembler-with-cpp -include 
AutoGen.h
-DEFINE GCC_VFRPP_FLAGS = -x c -E -P -DVFRCOMPILE --include 
$(MODULE_NAME)StrDefs.h
+DEFINE GCC_VFRPP_FLAGS = -x c -E -P -DVFRCOMPILE --include 
$(MODULE_NAME)StrDefs.h -Wundef -Werror
 DEFINE GCC_ASLPP_FLAGS = -x c -E -include AutoGen.h
 DEFINE GCC_ASLCC_FLAGS = -x c
 DEFINE GCC_WINDRES_FLAGS   = -J rc -O coff
--
2.30.0









-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74385): https://edk2.groups.io/g/devel/message/74385
Mute This Topic: https://groups.io/mt/82306124/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 2/2] BaseTools: Make undefined VFR macro an error (MSVC)

2021-04-23 Thread Daniel Schaefer 
Ok, I'll send a new series without EBC. Can't find anything about it and we 
don't use it.

From: Schaefer, Daniel 
Sent: Thursday, March 4, 2021 10:46
To: gaoliming ; devel@edk2.groups.io 

Cc: 'Bob Feng' ; 'Yuwei Chen' ; 
Lin, Derek (HPS SW) 
Subject: Re: 回复: [PATCH v1 2/2] BaseTools: Make undefined VFR macro an error 
(MSVC)

Hi Liming,

as stated in the coverletter, "I only tested GCC5, CLANPDB and VS2015 
toolchains."

Clang support is documented here: 
https://clang.llvm.org/docs/DiagnosticsReference.html#wundef
GCC support is documented here: 
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
MSVC support is documented here: 
https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4668?view=msvc-160

I'm sorry, I'm unable to find documentation for, or even the EBC compiler 
itself.
Can you please help me with this?

Thanks,
Daniel

On 3/4/21 10:12 AM, gaoliming wrote:
> Do you check whether EBC compiler supports this warning?
>
> And, do you evaluate CLANG compiler support for this warning?
>
> Thanks
> Liming
>> -邮件原件-
>> 发件人: Daniel Schaefer 
>> 发送时间: 2021年3月2日 16:22
>> 收件人: devel@edk2.groups.io
>> 抄送: Bob Feng ; Liming Gao
>> ; Yuwei Chen ; Derek
>> Lin 
>> 主题: [PATCH v1 2/2] BaseTools: Make undefined VFR macro an error (MSVC)
>>
>> VFR successfully compiles if we forget to include a header that defines
>> a macro. In that case the HII option was hidden when it shouldn't be
>> just because the macro was used but not defined.
>>
>> The behaviour is totally intended by the C/PP standard. When a macro is
>> undefined it evaluates to 0.
>> GCC, MSVC and Clang have warnings to catch this type of mistake. With
>> this commit we enable this warning and make it a compiler error.
>>
>> Cc: Bob Feng 
>> Cc: Liming Gao 
>> Cc: Yuwei Chen 
>> Cc: Derek Lin 
>> ---
>>   BaseTools/Conf/tools_def.template | 46 ++--
>>   1 file changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/BaseTools/Conf/tools_def.template
>> b/BaseTools/Conf/tools_def.template
>> index 728c1d3119e4..56c7bd13f157 100755
>> --- a/BaseTools/Conf/tools_def.template
>> +++ b/BaseTools/Conf/tools_def.template
>> @@ -422,7 +422,7 @@ DEFINE DTC_BIN =
>> ENV(DTC_PREFIX)dtc
>>   *_VS2008_*_SLINK_FLAGS= /NOLOGO /LTCG
>>
>>   *_VS2008_*_APP_FLAGS  = /nologo /E /TC
>>
>>   *_VS2008_*_PP_FLAGS   = /nologo /E /TC /FIAutoGen.h
>>
>> -*_VS2008_*_VFRPP_FLAGS= /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2008_*_VFRPP_FLAGS= /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2008_*_DEPS_FLAGS= DEF(MSFT_DEPS_FLAGS)
>>
>>   *_VS2008_*_ASM16_PATH = DEF(VS2008_BIN)\ml.exe
>>
>>
>>
>> @@ -518,7 +518,7 @@ NOOPT_VS2008_X64_DLINK_FLAGS  = /NOLOGO
>> /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT
>>   *_VS2008_EBC_MAKE_FLAGS  = /nologo
>>
>>   *_VS2008_EBC_PP_FLAGS= /nologo /E /TC /FIAutoGen.h
>>
>>   *_VS2008_EBC_CC_FLAGS= /nologo /c /WX /W3
>> /FIAutoGen.h /D$(MODULE_ENTRY_POINT)=$(ARCH_ENTRY_POINT)
>>
>> -*_VS2008_EBC_VFRPP_FLAGS = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2008_EBC_VFRPP_FLAGS = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2008_EBC_SLINK_FLAGS = /lib /NOLOGO /MACHINE:EBC
>>
>>   *_VS2008_EBC_DLINK_FLAGS = "C:\Program
>> Files\Intel\EBC\Lib\EbcLib.lib" /NOLOGO /NODEFAULTLIB /MACHINE:EBC
>> /OPT:REF /ENTRY:$(IMAGE_ENTRY_POINT)
>> /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /MAP /ALIGN:32 /DRIVER
>>
>>
>>
>> @@ -538,7 +538,7 @@ NOOPT_VS2008_X64_DLINK_FLAGS  = /NOLOGO
>> /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT
>>   *_VS2008x86_*_SLINK_FLAGS = /NOLOGO /LTCG
>>
>>   *_VS2008x86_*_APP_FLAGS   = /nologo /E /TC
>>
>>   *_VS2008x86_*_PP_FLAGS= /nologo /E /TC /FIAutoGen.h
>>
>> -*_VS2008x86_*_VFRPP_FLAGS = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2008x86_*_VFRPP_FLAGS = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2008x86_*_DEPS_FLAGS  = DEF(MSFT_DEPS_FLAGS)
>>
>>   *_VS2008x86_*_ASM16_PATH  = DEF(VS2008x86_BIN)\ml.exe
>>
>>
>>
>> @@ -633,7 +633,7 @@ NOOPT_VS2008x86_X64_DLINK_FLAGS=
>> /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF
>>   *_VS2008x86_EBC_MAKE_FLAGS  = /nologo
>>
>>   *_VS2008x86_EBC_PP_FLAGS= /nologo /E /TC /FIAutoGen.h
>>
>>   *_VS2008x86_EBC_CC_FLAGS= /nologo /c /WX /W3
>> /FIAutoGen.h /D$(MODULE_ENTRY_POINT)=$(ARCH_ENTRY_POINT)
>>
>> -*_VS2008x86_EBC_VFRPP_FLAGS = /nologo /E /TC
>> /DVFRCOMPILE /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2008x86_EBC_VFRPP_FLAGS = /nologo /E /TC
>> /DVFRCOMPILE /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2008x86_EBC_SLINK_FLAGS = /lib /NOLOGO
>> /MACHINE:EBC
>>
>>   *_VS2008x86_EBC_DLINK_FLAGS