[PATCH v2 0/3] Allow building MdeModulePkg on non-x86

[PATCH v2 0/3] Allow building MdeModulePkg on non-x86

[Daniel Schaefer]

Here's the second round of patches to allow building MdeModulePkg on non x86 architectures.
We need this to successfully run the CI for RISC-V.
The commits are also here: https://github.com/changab/edk2-staging-riscv/commits/MdeModulePkg-cross-for-edk2
Yes, it's in our staging repo but the patches and branch are based on edk2 master.

By the way, how hasn't this caused any problems for ARM?

I've built this with the X64 and RISCV64 GCC toolchains:
  build -a X64 -p MdeModulePkg/MdeModulePkg.dsc -t GCC5
  build -a RISCV64 -p MdeModulePkg/MdeModulePkg.dsc -t GCC5

Building on RISCV64 needs Abners patches: https://edk2.groups.io/g/devel/message/54725

Daniel Schaefer (3):
  MdeModulePkg: Restrict libraries using SMM to x86
  MdeModulePkg: Set PcdDxeIplSwitchToLongMode false on non-x86
  MdeModulePkg: Use CopyMem instead of GUID assignment

 MdeModulePkg/MdeModulePkg.dec                           | 3 +++
 MdeModulePkg/MdeModulePkg.dsc                           | 9 ++++++---
 MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
 3 files changed, 10 insertions(+), 4 deletions(-)

Cc: Abner Chang <abner.chang@hpe.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Gilbert Chen <gilbert.chen@hpe.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>

-- 
2.25.0

[PATCH v2 1/3] MdeModulePkg: Restrict libraries using SMM to x86

[Daniel Schaefer]

The modules:

  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf
  MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf
  MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf

seem like they are independent of SMM but they actually do requires SMM's
LockBoxLib. Ideally they would be rewritten to not require SMM on non x86.

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

Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com>
Cc: Abner Chang <abner.chang@hpe.com>
Cc: Gilbert Chen <gilbert.chen@hpe.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
---

Notes:
    v2:
    - Fix IA86 -> IA32 [Mike]

 MdeModulePkg/MdeModulePkg.dsc | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
index f7dbb27ce25d..5a20722a4270 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -109,6 +109,8 @@ [LibraryClasses.common.PEIM]
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
   ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedSectionLib/PeiExtractGuidedSectionLib.inf
+
+[LibraryClasses.IA32.PEIM, LibraryClasses.X64.PEIM]
   LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.inf
 
 [LibraryClasses.common.DXE_CORE]
@@ -228,7 +230,6 @@ [Components]
   MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
   MdeModulePkg/Bus/Pci/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupportDxe.inf
   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
-  MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf
   MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
   MdeModulePkg/Bus/Pci/SdMmcPciHcPei/SdMmcPciHcPei.inf
   MdeModulePkg/Bus/Sd/EmmcBlockIoPei/EmmcBlockIoPei.inf
@@ -251,7 +252,6 @@ [Components]
   MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
   MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
-  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf
   MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
   MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBusDxe.inf
@@ -395,7 +395,6 @@ [Components]
     <LibraryClasses>
       LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf
   }
-  MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf
   MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
   MdeModulePkg/Universal/SectionExtractionDxe/SectionExtractionDxe.inf {
     <LibraryClasses>
@@ -447,6 +446,8 @@ [Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
 [Components.IA32, Components.X64]
   MdeModulePkg/Universal/DebugSupportDxe/DebugSupportDxe.inf
   MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.inf
+  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf
+  MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf
   MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
   MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf {
@@ -478,6 +479,7 @@ [Components.IA32, Components.X64]
   MdeModulePkg/Library/SmmSmiHandlerProfileLib/SmmSmiHandlerProfileLib.inf
   MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaArchCustomDecompressLib.inf
   MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf
+  MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf
   MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf
   MdeModulePkg/Universal/Acpi/SmmS3SaveState/SmmS3SaveState.inf
   MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableSmm/FirmwarePerformanceSmm.inf
@@ -487,6 +489,7 @@ [Components.IA32, Components.X64]
   MdeModulePkg/Universal/SmmCommunicationBufferDxe/SmmCommunicationBufferDxe.inf
   MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
 
+
 [Components.X64]
   MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf
 
-- 
2.25.0

[PATCH v2 2/3] MdeModulePkg: Set PcdDxeIplSwitchToLongMode false on non-x86

[Daniel Schaefer]

Otherwise the PCD isn't defined on other architectures.

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

Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com>
Cc: Abner Chang <abner.chang@hpe.com>
Cc: Gilbert Chen <gilbert.chen@hpe.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
---
 MdeModulePkg/MdeModulePkg.dec | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 91a3c608231c..59645c860148 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -879,6 +879,9 @@ [PcdsFeatureFlag]
 [PcdsFeatureFlag.IA32, PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdPciDegradeResourceForOptionRom|FALSE|BOOLEAN|0x0001003a
 
+[PcdsFeatureFlag.EBC, PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|FALSE|BOOLEAN|0x0001003b
+
 [PcdsFeatureFlag.IA32, PcdsFeatureFlag.X64]
   ## Indicates if DxeIpl should switch to long mode to enter DXE phase.
   #  It is assumed that 64-bit DxeCore is built in firmware if it is true; otherwise 32-bit DxeCore
-- 
2.25.0

[PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Daniel Schaefer]

GCC translates a simple assignment to memcpy, which EDKII doesn't provide.
See: https://www.mail-archive.com/edk2-devel@lists.01.org/msg11928.html

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

Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com>
Cc: Abner Chang <abner.chang@hpe.com>
Cc: Gilbert Chen <gilbert.chen@hpe.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
---

Notes:
    v2:
      - Use CopyMem instead of CopyGuid [Dandan]

 MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
index 5cc527679a78..0540e6fa8a44 100644
--- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
+++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
@@ -619,7 +619,7 @@ CreateDeviceManagerForm(
         TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
         FreePool (String);
 
-        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
+        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid, sizeof (EFI_GUID));
 
         //
         // Network device process
-- 
2.25.0

Re: [edk2-devel] [PATCH v2 1/3] MdeModulePkg: Restrict libraries using SMM to x86

[Liming Gao]

Daniel:
  Those modules consumes LockBox for S3 boot path. If the different ARCH doesn't depend on LockBox for S3 boot, it can use LockBoxNullLib.inf library instance. For EBC arch, LockBoxNullLib.inf has been specified. You can append more ARCHs with LockBoxNullLib library instance. 

[LibraryClasses.EBC]
  LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Daniel Schaefer
> Sent: Monday, March 2, 2020 6:33 PM
> To: devel@edk2.groups.io
> Cc: Abner Chang <abner.chang@hpe.com>; Gilbert Chen <gilbert.chen@hpe.com>; Leif Lindholm <leif@nuviainc.com>; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Gao,
> Liming <liming.gao@intel.com>
> Subject: [edk2-devel] [PATCH v2 1/3] MdeModulePkg: Restrict libraries using SMM to x86
> 
> The modules:
> 
>   MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf
>   MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf
>   MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf
> 
> seem like they are independent of SMM but they actually do requires SMM's
> LockBoxLib. Ideally they would be rewritten to not require SMM on non x86.
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2549
> 
> Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com>
> Cc: Abner Chang <abner.chang@hpe.com>
> Cc: Gilbert Chen <gilbert.chen@hpe.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> ---
> 
> Notes:
>     v2:
>     - Fix IA86 -> IA32 [Mike]
> 
>  MdeModulePkg/MdeModulePkg.dsc | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
> index f7dbb27ce25d..5a20722a4270 100644
> --- a/MdeModulePkg/MdeModulePkg.dsc
> +++ b/MdeModulePkg/MdeModulePkg.dsc
> @@ -109,6 +109,8 @@ [LibraryClasses.common.PEIM]
>    HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
> 
> 
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
> 
> 
>    ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedSectionLib/PeiExtractGuidedSectionLib.inf
> 
> 
> +
> 
> 
> +[LibraryClasses.IA32.PEIM, LibraryClasses.X64.PEIM]
> 
> 
>    LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.inf
> 
> 
> 
> 
> 
>  [LibraryClasses.common.DXE_CORE]
> 
> 
> @@ -228,7 +230,6 @@ [Components]
>    MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> 
> 
>    MdeModulePkg/Bus/Pci/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupportDxe.inf
> 
> 
>    MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> 
> 
> -  MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf
> 
> 
>    MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
> 
> 
>    MdeModulePkg/Bus/Pci/SdMmcPciHcPei/SdMmcPciHcPei.inf
> 
> 
>    MdeModulePkg/Bus/Sd/EmmcBlockIoPei/EmmcBlockIoPei.inf
> 
> 
> @@ -251,7 +252,6 @@ [Components]
>    MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
> 
> 
>    MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
> 
> 
>    MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> 
> 
> -  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf
> 
> 
>    MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> 
> 
>    MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
> 
> 
>    MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBusDxe.inf
> 
> 
> @@ -395,7 +395,6 @@ [Components]
>      <LibraryClasses>
> 
> 
>        LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf
> 
> 
>    }
> 
> 
> -  MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf
> 
> 
>    MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
> 
> 
>    MdeModulePkg/Universal/SectionExtractionDxe/SectionExtractionDxe.inf {
> 
> 
>      <LibraryClasses>
> 
> 
> @@ -447,6 +446,8 @@ [Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
>  [Components.IA32, Components.X64]
> 
> 
>    MdeModulePkg/Universal/DebugSupportDxe/DebugSupportDxe.inf
> 
> 
>    MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.inf
> 
> 
> +  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf
> 
> 
> +  MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf
> 
> 
>    MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> 
> 
>    MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> 
> 
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf {
> 
> 
> @@ -478,6 +479,7 @@ [Components.IA32, Components.X64]
>    MdeModulePkg/Library/SmmSmiHandlerProfileLib/SmmSmiHandlerProfileLib.inf
> 
> 
>    MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaArchCustomDecompressLib.inf
> 
> 
>    MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf
> 
> 
> +  MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf
> 
> 
>    MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf
> 
> 
>    MdeModulePkg/Universal/Acpi/SmmS3SaveState/SmmS3SaveState.inf
> 
> 
>    MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableSmm/FirmwarePerformanceSmm.inf
> 
> 
> @@ -487,6 +489,7 @@ [Components.IA32, Components.X64]
>    MdeModulePkg/Universal/SmmCommunicationBufferDxe/SmmCommunicationBufferDxe.inf
> 
> 
>    MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
> 
> 
> 
> 
> 
> +
> 
> 
>  [Components.X64]
> 
> 
>    MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf
> 
> 
> 
> 
> 
> --
> 2.25.0
> 
> 
> 

Re: [PATCH v2 2/3] MdeModulePkg: Set PcdDxeIplSwitchToLongMode false on non-x86

[Liming Gao]

Daniel:
  PcdDxeIplSwitchToLongMode is only used in IA32 arch. It is also specified in [FeaturePcd.IA32] section of DxeIpl.inf. So, I don't understand why defines this PCD in other ARCH in MdeModulePkg.dec. 

Thanks
Liming
> -----Original Message-----
> From: Daniel Schaefer <daniel.schaefer@hpe.com>
> Sent: Monday, March 2, 2020 6:33 PM
> To: devel@edk2.groups.io
> Cc: Abner Chang <abner.chang@hpe.com>; Gilbert Chen <gilbert.chen@hpe.com>; Leif Lindholm <leif@nuviainc.com>; Bi, Dandan
> <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [PATCH v2 2/3] MdeModulePkg: Set PcdDxeIplSwitchToLongMode false on non-x86
> 
> Otherwise the PCD isn't defined on other architectures.
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2548
> 
> Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com>
> Cc: Abner Chang <abner.chang@hpe.com>
> Cc: Gilbert Chen <gilbert.chen@hpe.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> ---
>  MdeModulePkg/MdeModulePkg.dec | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 91a3c608231c..59645c860148 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -879,6 +879,9 @@ [PcdsFeatureFlag]
>  [PcdsFeatureFlag.IA32, PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64]
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPciDegradeResourceForOptionRom|FALSE|BOOLEAN|0x0001003a
> 
> 
> 
> +[PcdsFeatureFlag.EBC, PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64]
> 
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|FALSE|BOOLEAN|0x0001003b
> 
> +
> 
>  [PcdsFeatureFlag.IA32, PcdsFeatureFlag.X64]
> 
>    ## Indicates if DxeIpl should switch to long mode to enter DXE phase.
> 
>    #  It is assumed that 64-bit DxeCore is built in firmware if it is true; otherwise 32-bit DxeCore
> 
> --
> 2.25.0

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Liming Gao]

Daniel:
  I agree this fix. But, I don't meet with this failure with GCC on IA32/X64 arch. So, I don't fix it early. Reviewed-by: Liming Gao <liming.gao@intel.com>

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Daniel Schaefer
> Sent: Monday, March 2, 2020 6:33 PM
> To: devel@edk2.groups.io
> Cc: Abner Chang <abner.chang@hpe.com>; Gilbert Chen <gilbert.chen@hpe.com>; Leif Lindholm <leif@nuviainc.com>; Bi, Dandan
> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment
> 
> GCC translates a simple assignment to memcpy, which EDKII doesn't provide.
> See: https://www.mail-archive.com/edk2-devel@lists.01.org/msg11928.html
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2547
> 
> Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com>
> Cc: Abner Chang <abner.chang@hpe.com>
> Cc: Gilbert Chen <gilbert.chen@hpe.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> ---
> 
> Notes:
>     v2:
>       - Use CopyMem instead of CopyGuid [Dandan]
> 
>  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> index 5cc527679a78..0540e6fa8a44 100644
> --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> @@ -619,7 +619,7 @@ CreateDeviceManagerForm(
>          TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
> 
> 
>          FreePool (String);
> 
> 
> 
> 
> 
> -        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
> 
> 
> +        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid, sizeof (EFI_GUID));
> 
> 
> 
> 
> 
>          //
> 
> 
>          // Network device process
> 
> 
> --
> 2.25.0
> 
> 
> 

Re: [PATCH v2 2/3] MdeModulePkg: Set PcdDxeIplSwitchToLongMode false on non-x86

[Daniel Schaefer]

[-- Attachment #1: Type: text/plain, Size: 2781 bytes --]

Hi Liming,

You're right, it doesn't make sense to define this PCD on other architectures and it is not necessary.
I wrote this patch before patch 1 (MdeModulePkg: Restrict libraries using SMM to x86).
With the other patch, this PCD is not used on other architectures, so *we can drop this patch*.

Thanks,
Daniel
________________________________
From: Gao, Liming <liming.gao@intel.com>
Sent: Monday, March 2, 2020 14:19
To: Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>; Chen, Gilbert <gilbert.chen@hpe.com>; Leif Lindholm <leif@nuviainc.com>; Bi, Dandan <dandan.bi@intel.com>
Subject: RE: [PATCH v2 2/3] MdeModulePkg: Set PcdDxeIplSwitchToLongMode false on non-x86

Daniel:
  PcdDxeIplSwitchToLongMode is only used in IA32 arch. It is also specified in [FeaturePcd.IA32] section of DxeIpl.inf. So, I don't understand why defines this PCD in other ARCH in MdeModulePkg.dec.

Thanks
Liming
> -----Original Message-----
> From: Daniel Schaefer <daniel.schaefer@hpe.com>
> Sent: Monday, March 2, 2020 6:33 PM
> To: devel@edk2.groups.io
> Cc: Abner Chang <abner.chang@hpe.com>; Gilbert Chen <gilbert.chen@hpe.com>; Leif Lindholm <leif@nuviainc.com>; Bi, Dandan
> <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [PATCH v2 2/3] MdeModulePkg: Set PcdDxeIplSwitchToLongMode false on non-x86
>
> Otherwise the PCD isn't defined on other architectures.
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2548
>
> Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com>
> Cc: Abner Chang <abner.chang@hpe.com>
> Cc: Gilbert Chen <gilbert.chen@hpe.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> ---
>  MdeModulePkg/MdeModulePkg.dec | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 91a3c608231c..59645c860148 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -879,6 +879,9 @@ [PcdsFeatureFlag]
>  [PcdsFeatureFlag.IA32, PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64]
>
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPciDegradeResourceForOptionRom|FALSE|BOOLEAN|0x0001003a
>
>
>
> +[PcdsFeatureFlag.EBC, PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64]
>
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|FALSE|BOOLEAN|0x0001003b
>
> +
>
>  [PcdsFeatureFlag.IA32, PcdsFeatureFlag.X64]
>
>    ## Indicates if DxeIpl should switch to long mode to enter DXE phase.
>
>    #  It is assumed that 64-bit DxeCore is built in firmware if it is true; otherwise 32-bit DxeCore
>
> --
> 2.25.0


[-- Attachment #2: Type: text/html, Size: 4903 bytes --]

Re: [edk2-devel] [PATCH v2 1/3] MdeModulePkg: Restrict libraries using SMM to x86

[Daniel Schaefer]

[-- Attachment #1: Type: text/plain, Size: 6499 bytes --]

Hi Liming,

thanks for your review!
Oh, I missed this, yes indeed, I can just include LockBoxNullLib and it works on RISCV64, too.
So we can drop this patch, too.

Cheers,
Daniel
________________________________
From: Gao, Liming <liming.gao@intel.com>
Sent: Monday, March 2, 2020 14:18
To: devel@edk2.groups.io <devel@edk2.groups.io>; Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com>
Cc: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>; Chen, Gilbert <gilbert.chen@hpe.com>; Leif Lindholm <leif@nuviainc.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>
Subject: RE: [edk2-devel] [PATCH v2 1/3] MdeModulePkg: Restrict libraries using SMM to x86

Daniel:
  Those modules consumes LockBox for S3 boot path. If the different ARCH doesn't depend on LockBox for S3 boot, it can use LockBoxNullLib.inf library instance. For EBC arch, LockBoxNullLib.inf has been specified. You can append more ARCHs with LockBoxNullLib library instance.

[LibraryClasses.EBC]
  LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Daniel Schaefer
> Sent: Monday, March 2, 2020 6:33 PM
> To: devel@edk2.groups.io
> Cc: Abner Chang <abner.chang@hpe.com>; Gilbert Chen <gilbert.chen@hpe.com>; Leif Lindholm <leif@nuviainc.com>; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Gao,
> Liming <liming.gao@intel.com>
> Subject: [edk2-devel] [PATCH v2 1/3] MdeModulePkg: Restrict libraries using SMM to x86
>
> The modules:
>
>   MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf
>   MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf
>   MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf
>
> seem like they are independent of SMM but they actually do requires SMM's
> LockBoxLib. Ideally they would be rewritten to not require SMM on non x86.
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2549
>
> Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com>
> Cc: Abner Chang <abner.chang@hpe.com>
> Cc: Gilbert Chen <gilbert.chen@hpe.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> ---
>
> Notes:
>     v2:
>     - Fix IA86 -> IA32 [Mike]
>
>  MdeModulePkg/MdeModulePkg.dsc | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
> index f7dbb27ce25d..5a20722a4270 100644
> --- a/MdeModulePkg/MdeModulePkg.dsc
> +++ b/MdeModulePkg/MdeModulePkg.dsc
> @@ -109,6 +109,8 @@ [LibraryClasses.common.PEIM]
>    HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
>
>
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
>
>
>    ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedSectionLib/PeiExtractGuidedSectionLib.inf
>
>
> +
>
>
> +[LibraryClasses.IA32.PEIM, LibraryClasses.X64.PEIM]
>
>
>    LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.inf
>
>
>
>
>
>  [LibraryClasses.common.DXE_CORE]
>
>
> @@ -228,7 +230,6 @@ [Components]
>    MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>
>
>    MdeModulePkg/Bus/Pci/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupportDxe.inf
>
>
>    MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
>
>
> -  MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf
>
>
>    MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
>
>
>    MdeModulePkg/Bus/Pci/SdMmcPciHcPei/SdMmcPciHcPei.inf
>
>
>    MdeModulePkg/Bus/Sd/EmmcBlockIoPei/EmmcBlockIoPei.inf
>
>
> @@ -251,7 +252,6 @@ [Components]
>    MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
>
>
>    MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
>
>
>    MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
>
>
> -  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf
>
>
>    MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
>
>
>    MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
>
>
>    MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBusDxe.inf
>
>
> @@ -395,7 +395,6 @@ [Components]
>      <LibraryClasses>
>
>
>        LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf
>
>
>    }
>
>
> -  MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf
>
>
>    MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
>
>
>    MdeModulePkg/Universal/SectionExtractionDxe/SectionExtractionDxe.inf {
>
>
>      <LibraryClasses>
>
>
> @@ -447,6 +446,8 @@ [Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
>  [Components.IA32, Components.X64]
>
>
>    MdeModulePkg/Universal/DebugSupportDxe/DebugSupportDxe.inf
>
>
>    MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.inf
>
>
> +  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf
>
>
> +  MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf
>
>
>    MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
>
>
>    MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
>
>
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf {
>
>
> @@ -478,6 +479,7 @@ [Components.IA32, Components.X64]
>    MdeModulePkg/Library/SmmSmiHandlerProfileLib/SmmSmiHandlerProfileLib.inf
>
>
>    MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaArchCustomDecompressLib.inf
>
>
>    MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf
>
>
> +  MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf
>
>
>    MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf
>
>
>    MdeModulePkg/Universal/Acpi/SmmS3SaveState/SmmS3SaveState.inf
>
>
>    MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableSmm/FirmwarePerformanceSmm.inf
>
>
> @@ -487,6 +489,7 @@ [Components.IA32, Components.X64]
>    MdeModulePkg/Universal/SmmCommunicationBufferDxe/SmmCommunicationBufferDxe.inf
>
>
>    MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
>
>
>
>
>
> +
>
>
>  [Components.X64]
>
>
>    MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf
>
>
>
>
>
> --
> 2.25.0
>
>
> 


[-- Attachment #2: Type: text/html, Size: 10658 bytes --]

Re: [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Dandan Bi]

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


Thanks,
Dandan
> -----Original Message-----
> From: Daniel Schaefer [mailto:daniel.schaefer@hpe.com]
> Sent: Monday, March 2, 2020 6:33 PM
> To: devel@edk2.groups.io
> Cc: Abner Chang <abner.chang@hpe.com>; Gilbert Chen
> <gilbert.chen@hpe.com>; Leif Lindholm <leif@nuviainc.com>; Bi, Dandan
> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID
> assignment
> 
> GCC translates a simple assignment to memcpy, which EDKII doesn't provide.
> See: https://www.mail-archive.com/edk2-devel@lists.01.org/msg11928.html
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2547
> 
> Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com>
> Cc: Abner Chang <abner.chang@hpe.com>
> Cc: Gilbert Chen <gilbert.chen@hpe.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> ---
> 
> Notes:
>     v2:
>       - Use CopyMem instead of CopyGuid [Dandan]
> 
>  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> index 5cc527679a78..0540e6fa8a44 100644
> --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> @@ -619,7 +619,7 @@ CreateDeviceManagerForm(
>          TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
> 
>          FreePool (String);
> 
> 
> 
> -        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
> 
> +        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid, sizeof
> (EFI_GUID));
> 
> 
> 
>          //
> 
>          // Network device process
> 
> --
> 2.25.0

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Wang, Jian J]

Pushed @ 64a228f5f89320fd632bb6c55e154961f2410680

Regards,
Jian

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dandan Bi
> Sent: Thursday, March 05, 2020 8:40 AM
> To: Daniel Schaefer <daniel.schaefer@hpe.com>; devel@edk2.groups.io
> Cc: Abner Chang <abner.chang@hpe.com>; Gilbert Chen
> <gilbert.chen@hpe.com>; Leif Lindholm <leif@nuviainc.com>; Dong, Eric
> <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> instead of GUID assignment
> 
> Reviewed-by: Dandan Bi <dandan.bi@intel.com>
> 
> 
> Thanks,
> Dandan
> > -----Original Message-----
> > From: Daniel Schaefer [mailto:daniel.schaefer@hpe.com]
> > Sent: Monday, March 2, 2020 6:33 PM
> > To: devel@edk2.groups.io
> > Cc: Abner Chang <abner.chang@hpe.com>; Gilbert Chen
> > <gilbert.chen@hpe.com>; Leif Lindholm <leif@nuviainc.com>; Bi, Dandan
> > <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>
> > Subject: [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID
> > assignment
> >
> > GCC translates a simple assignment to memcpy, which EDKII doesn't provide.
> > See: https://www.mail-archive.com/edk2-devel@lists.01.org/msg11928.html
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2547
> >
> > Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com>
> > Cc: Abner Chang <abner.chang@hpe.com>
> > Cc: Gilbert Chen <gilbert.chen@hpe.com>
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Cc: Dandan Bi <dandan.bi@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > ---
> >
> > Notes:
> >     v2:
> >       - Use CopyMem instead of CopyGuid [Dandan]
> >
> >  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > index 5cc527679a78..0540e6fa8a44 100644
> > --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > @@ -619,7 +619,7 @@ CreateDeviceManagerForm(
> >          TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
> >
> >          FreePool (String);
> >
> >
> >
> > -        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
> >
> > +        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid, sizeof
> > (EFI_GUID));
> >
> >
> >
> >          //
> >
> >          // Network device process
> >
> > --
> > 2.25.0
> 
> 
> 

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Abner Chang]

[-- Attachment #1: Type: text/plain, Size: 3162 bytes --]

Appreciated, Jian J.

Get Outlook for Android<https://aka.ms/ghei36>
________________________________
From: Wang, Jian J <jian.j.wang@intel.com>
Sent: Thursday, March 12, 2020 1:58:05 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Bi, Dandan <dandan.bi@intel.com>; Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com>
Cc: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>; Chen, Gilbert <gilbert.chen@hpe.com>; Leif Lindholm <leif@nuviainc.com>; Dong, Eric <eric.dong@intel.com>
Subject: RE: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

Pushed @ 64a228f5f89320fd632bb6c55e154961f2410680

Regards,
Jian

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dandan Bi
> Sent: Thursday, March 05, 2020 8:40 AM
> To: Daniel Schaefer <daniel.schaefer@hpe.com>; devel@edk2.groups.io
> Cc: Abner Chang <abner.chang@hpe.com>; Gilbert Chen
> <gilbert.chen@hpe.com>; Leif Lindholm <leif@nuviainc.com>; Dong, Eric
> <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> instead of GUID assignment
>
> Reviewed-by: Dandan Bi <dandan.bi@intel.com>
>
>
> Thanks,
> Dandan
> > -----Original Message-----
> > From: Daniel Schaefer [mailto:daniel.schaefer@hpe.com]
> > Sent: Monday, March 2, 2020 6:33 PM
> > To: devel@edk2.groups.io
> > Cc: Abner Chang <abner.chang@hpe.com>; Gilbert Chen
> > <gilbert.chen@hpe.com>; Leif Lindholm <leif@nuviainc.com>; Bi, Dandan
> > <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>
> > Subject: [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID
> > assignment
> >
> > GCC translates a simple assignment to memcpy, which EDKII doesn't provide.
> > See: https://www.mail-archive.com/edk2-devel@lists.01.org/msg11928.html
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2547
> >
> > Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com>
> > Cc: Abner Chang <abner.chang@hpe.com>
> > Cc: Gilbert Chen <gilbert.chen@hpe.com>
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Cc: Dandan Bi <dandan.bi@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > ---
> >
> > Notes:
> >     v2:
> >       - Use CopyMem instead of CopyGuid [Dandan]
> >
> >  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > index 5cc527679a78..0540e6fa8a44 100644
> > --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > @@ -619,7 +619,7 @@ CreateDeviceManagerForm(
> >          TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
> >
> >          FreePool (String);
> >
> >
> >
> > -        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
> >
> > +        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid, sizeof
> > (EFI_GUID));
> >
> >
> >
> >          //
> >
> >          // Network device process
> >
> > --
> > 2.25.0
>
>
> 


[-- Attachment #2: Type: text/html, Size: 5408 bytes --]

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Leif Lindholm]

Hi Daniel,

There is nothing wrong with this patch that just went in (and I should
have called out sooner if I wanted to stop it), but I think a better
solution is to implement a RISC-V variant of
ArmPkg/Library/CompilerIntrinsicsLib/.

It is perfectly valid for the compiler to generate memcpy calls in
response to struct operations that are perfectly valid C.

In fact, we could consider moving the ArmPkg one over into
MdeModulePkg. I have a feeling that including a
  NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
in your current build would be an alternative solution to your
compilation error.

/
    Leif

On Mon, Mar 02, 2020 at 11:32:38 +0100, Daniel Schaefer wrote:
> GCC translates a simple assignment to memcpy, which EDKII doesn't provide.
> See: https://www.mail-archive.com/edk2-devel@lists.01.org/msg11928.html
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2547
> 
> Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com>
> Cc: Abner Chang <abner.chang@hpe.com>
> Cc: Gilbert Chen <gilbert.chen@hpe.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> ---
> 
> Notes:
>     v2:
>       - Use CopyMem instead of CopyGuid [Dandan]
> 
>  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> index 5cc527679a78..0540e6fa8a44 100644
> --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> @@ -619,7 +619,7 @@ CreateDeviceManagerForm(
>          TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
>          FreePool (String);
>  
> -        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
> +        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid, sizeof (EFI_GUID));
>  
>          //
>          // Network device process
> -- 
> 2.25.0
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Ni, Ray]

Leif,
Does CompilerIntrinsicsLib exist because GCC doesn't support to disable intrinsic?
How does the linker find the CompilerInstrinsicsLib in linking phase?

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif
> Lindholm
> Sent: Thursday, March 12, 2020 6:55 PM
> To: devel@edk2.groups.io; daniel.schaefer@hpe.com
> Cc: Abner Chang <abner.chang@hpe.com>; Gilbert Chen
> <gilbert.chen@hpe.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
> <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> instead of GUID assignment
> 
> Hi Daniel,
> 
> There is nothing wrong with this patch that just went in (and I should
> have called out sooner if I wanted to stop it), but I think a better
> solution is to implement a RISC-V variant of
> ArmPkg/Library/CompilerIntrinsicsLib/.
> 
> It is perfectly valid for the compiler to generate memcpy calls in
> response to struct operations that are perfectly valid C.
> 
> In fact, we could consider moving the ArmPkg one over into
> MdeModulePkg. I have a feeling that including a
>   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> in your current build would be an alternative solution to your
> compilation error.
> 
> /
>     Leif
> 
> On Mon, Mar 02, 2020 at 11:32:38 +0100, Daniel Schaefer wrote:
> > GCC translates a simple assignment to memcpy, which EDKII doesn't
> provide.
> > See: https://www.mail-archive.com/edk2-
> devel@lists.01.org/msg11928.html
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2547
> >
> > Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com>
> > Cc: Abner Chang <abner.chang@hpe.com>
> > Cc: Gilbert Chen <gilbert.chen@hpe.com>
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Cc: Dandan Bi <dandan.bi@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > ---
> >
> > Notes:
> >     v2:
> >       - Use CopyMem instead of CopyGuid [Dandan]
> >
> >  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > index 5cc527679a78..0540e6fa8a44 100644
> > --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > @@ -619,7 +619,7 @@ CreateDeviceManagerForm(
> >          TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
> >          FreePool (String);
> >
> > -        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
> > +        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid,
> sizeof (EFI_GUID));
> >
> >          //
> >          // Network device process
> > --
> > 2.25.0
> >
> >
> >
> >
> 
> 

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Daniel Schaefer]

[-- Attachment #1: Type: text/plain, Size: 3541 bytes --]

Hi Leif,

you're right. If I revert my commit and include
  NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
without making any changes to it, the build succeeds.

What do others think? (cc Michael, Pete, Andrew, Ard, who have made changes to this module)
Is this a big hack or should we use it in RISC-V, too and move the module to MdeModulePkg?
Why isn't this a problem on x86? Was it fine on Itanium?

- Daniel

________________________________
From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io><mailto:devel@edk2.groups.io> on behalf of Leif Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
Sent: Thursday, March 12, 2020 11:55
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io><mailto:devel@edk2.groups.io>; Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
Cc: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com><mailto:abner.chang@hpe.com>; Chen, Gilbert <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>; Dandan Bi <dandan.bi@intel.com><mailto:dandan.bi@intel.com>; Eric Dong <eric.dong@intel.com><mailto:eric.dong@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

Hi Daniel,

There is nothing wrong with this patch that just went in (and I should
have called out sooner if I wanted to stop it), but I think a better
solution is to implement a RISC-V variant of
ArmPkg/Library/CompilerIntrinsicsLib/.

It is perfectly valid for the compiler to generate memcpy calls in
response to struct operations that are perfectly valid C.

In fact, we could consider moving the ArmPkg one over into
MdeModulePkg. I have a feeling that including a
  NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
in your current build would be an alternative solution to your
compilation error.

/
    Leif

On Mon, Mar 02, 2020 at 11:32:38 +0100, Daniel Schaefer wrote:
> GCC translates a simple assignment to memcpy, which EDKII doesn't provide.
> See: https://www.mail-archive.com/edk2-devel@lists.01.org/msg11928.html
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2547
>
> Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
> Cc: Abner Chang <abner.chang@hpe.com><mailto:abner.chang@hpe.com>
> Cc: Gilbert Chen <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>
> Cc: Leif Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
> Cc: Dandan Bi <dandan.bi@intel.com><mailto:dandan.bi@intel.com>
> Cc: Eric Dong <eric.dong@intel.com><mailto:eric.dong@intel.com>
> ---
>
> Notes:
>     v2:
>       - Use CopyMem instead of CopyGuid [Dandan]
>
>  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> index 5cc527679a78..0540e6fa8a44 100644
> --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> @@ -619,7 +619,7 @@ CreateDeviceManagerForm(
>          TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
>          FreePool (String);
>
> -        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
> +        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid, sizeof (EFI_GUID));
>
>          //
>          // Network device process
> --
> 2.25.0
>
>
>
>




[-- Attachment #2: Type: text/html, Size: 7282 bytes --]

Re: [EXTERNAL] RE: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Leif Lindholm]

[-- Attachment #1: Type: text/plain, Size: 591 bytes --]

Hi Ray,

On Thu, Mar 12, 2020 at 12:21 PM Ni, Ray <ray.ni@intel.com> wrote:
> Does CompilerIntrinsicsLib exist because GCC doesn't support to disable
intrinsic?

It is not possible to disable all intrinsics on 32-bit ARM, and it's not
necessarily desirable to do so for AArch64 (so I don't think it's
implemented).

Note that this is somewhat true for x86 too - leading to things like
DivU64x32 in BaseLib.

> How does the linker find the CompilerInstrinsicsLib in linking phase?

For example:
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/MdeModulePkg.dsc#L172

Regards,

Leif

[-- Attachment #2: Type: text/html, Size: 990 bytes --]

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Leif Lindholm]

+Ard, Laszlo.

I think it would make sense to move it to MdeModulePkg (or MdePkg) and
rename it BaseCompilerIntrinsicsLib (it *is* a BASE library).

As I alluded in my reply to Ray - x86 also have this problem, but to a
lesser extent, and ended up creating library functions to call instead
of using plain C for certain operations.
Which was probably the right decision when it was restricted to a
very few corner cases.

/
    Leif

On Thu, Mar 12, 2020 at 13:24:30 +0000, Schaefer, Daniel (DualStudy) wrote:
> Hi Leif,
> 
> you're right. If I revert my commit and include
>   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> without making any changes to it, the build succeeds.
> 
> What do others think? (cc Michael, Pete, Andrew, Ard, who have made changes to this module)
> Is this a big hack or should we use it in RISC-V, too and move the module to MdeModulePkg?
> Why isn't this a problem on x86? Was it fine on Itanium?
> 
> - Daniel
> 
> ________________________________
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io><mailto:devel@edk2.groups.io> on behalf of Leif Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
> Sent: Thursday, March 12, 2020 11:55
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io><mailto:devel@edk2.groups.io>; Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
> Cc: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com><mailto:abner.chang@hpe.com>; Chen, Gilbert <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>; Dandan Bi <dandan.bi@intel.com><mailto:dandan.bi@intel.com>; Eric Dong <eric.dong@intel.com><mailto:eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment
> 
> Hi Daniel,
> 
> There is nothing wrong with this patch that just went in (and I should
> have called out sooner if I wanted to stop it), but I think a better
> solution is to implement a RISC-V variant of
> ArmPkg/Library/CompilerIntrinsicsLib/.
> 
> It is perfectly valid for the compiler to generate memcpy calls in
> response to struct operations that are perfectly valid C.
> 
> In fact, we could consider moving the ArmPkg one over into
> MdeModulePkg. I have a feeling that including a
>   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> in your current build would be an alternative solution to your
> compilation error.
> 
> /
>     Leif
> 
> On Mon, Mar 02, 2020 at 11:32:38 +0100, Daniel Schaefer wrote:
> > GCC translates a simple assignment to memcpy, which EDKII doesn't provide.
> > See: https://www.mail-archive.com/edk2-devel@lists.01.org/msg11928.html
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2547
> >
> > Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
> > Cc: Abner Chang <abner.chang@hpe.com><mailto:abner.chang@hpe.com>
> > Cc: Gilbert Chen <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>
> > Cc: Leif Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
> > Cc: Dandan Bi <dandan.bi@intel.com><mailto:dandan.bi@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com><mailto:eric.dong@intel.com>
> > ---
> >
> > Notes:
> >     v2:
> >       - Use CopyMem instead of CopyGuid [Dandan]
> >
> >  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > index 5cc527679a78..0540e6fa8a44 100644
> > --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > @@ -619,7 +619,7 @@ CreateDeviceManagerForm(
> >          TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
> >          FreePool (String);
> >
> > -        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
> > +        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid, sizeof (EFI_GUID));
> >
> >          //
> >          // Network device process
> > --
> > 2.25.0
> >
> >
> >
> >
> 
> 
> 

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Abner Chang]

I don't prefer to do this at this moment, Leif. I have no problem if we create a BZ for this and create BaseCompilerIntrinsicsLib in MdeModulePkg, but please don't bind it with RISC-V EDK2 port. You know my concern pretty well, we can't hold those RISC-V patches on hands like forever and address the code structure optimization. We can still work on BaseCompilerIntrinsicsLib but not part of this RISC-V submission. We can implement RISC-V variant if necessary after RISC-V edk2 get in edk2 master.

Abner

> -----Original Message-----
> From: Leif Lindholm [mailto:leif@nuviainc.com]
> Sent: Thursday, March 12, 2020 10:03 PM
> To: Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com>
> Cc: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> <abner.chang@hpe.com>; Chen, Gilbert <gilbert.chen@hpe.com>;
> afish@apple.com; michael.d.kinney@intel.com; pete@akeo.ie; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> instead of GUID assignment
> 
> +Ard, Laszlo.
> 
> I think it would make sense to move it to MdeModulePkg (or MdePkg) and
> rename it BaseCompilerIntrinsicsLib (it *is* a BASE library).
> 
> As I alluded in my reply to Ray - x86 also have this problem, but to a lesser
> extent, and ended up creating library functions to call instead of using plain C
> for certain operations.
> Which was probably the right decision when it was restricted to a very few
> corner cases.
> 
> /
>     Leif
> 
> On Thu, Mar 12, 2020 at 13:24:30 +0000, Schaefer, Daniel (DualStudy) wrote:
> > Hi Leif,
> >
> > you're right. If I revert my commit and include
> >   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> > without making any changes to it, the build succeeds.
> >
> > What do others think? (cc Michael, Pete, Andrew, Ard, who have made
> > changes to this module) Is this a big hack or should we use it in RISC-V, too
> and move the module to MdeModulePkg?
> > Why isn't this a problem on x86? Was it fine on Itanium?
> >
> > - Daniel
> >
> > ________________________________
> > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > <devel@edk2.groups.io><mailto:devel@edk2.groups.io> on behalf of Leif
> > Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
> > Sent: Thursday, March 12, 2020 11:55
> > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > <devel@edk2.groups.io><mailto:devel@edk2.groups.io>; Schaefer, Daniel
> > (DualStudy)
> <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
> > Cc: Chang, Abner (HPS SW/FW Technologist)
> > <abner.chang@hpe.com><mailto:abner.chang@hpe.com>; Chen, Gilbert
> > <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>; Dandan Bi
> > <dandan.bi@intel.com><mailto:dandan.bi@intel.com>; Eric Dong
> > <eric.dong@intel.com><mailto:eric.dong@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> > instead of GUID assignment
> >
> > Hi Daniel,
> >
> > There is nothing wrong with this patch that just went in (and I should
> > have called out sooner if I wanted to stop it), but I think a better
> > solution is to implement a RISC-V variant of
> > ArmPkg/Library/CompilerIntrinsicsLib/.
> >
> > It is perfectly valid for the compiler to generate memcpy calls in
> > response to struct operations that are perfectly valid C.
> >
> > In fact, we could consider moving the ArmPkg one over into
> > MdeModulePkg. I have a feeling that including a
> >   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> > in your current build would be an alternative solution to your
> > compilation error.
> >
> > /
> >     Leif
> >
> > On Mon, Mar 02, 2020 at 11:32:38 +0100, Daniel Schaefer wrote:
> > > GCC translates a simple assignment to memcpy, which EDKII doesn't
> provide.
> > > See:
> > > INVALID URI REMOVED
> 2Darch
> > > ive.com_edk2-2Ddevel-
> 40lists.01.org_msg11928.html&d=DwIBAg&c=C5b8zRQ
> > >
> O1miGmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3
> E&m=wjlf
> > >
> QYZsXfc5WWDFmywLOEYlCKvrpYrnaXMIpJcLK7U&s=426yv7VvgHTtgtYaR0f0X
> NiaEA
> > > LgqZkxektjywEwPco&e=
> > >
> > > REF:INVALID URI REMOVED
> > > anocore.org_show-5Fbug.cgi-3Fid-
> 3D2547&d=DwIBAg&c=C5b8zRQO1miGmBeVZ2
> > >
> LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=wjlfQYZsX
> fc5WWD
> > > FmywLOEYlCKvrpYrnaXMIpJcLK7U&s=PjC_mIwh0GhNy5np2h7K8l4l-
> pLLBWt1FRuLB
> > > UbULpc&e=
> > >
> > > Signed-off-by: Daniel Schaefer
> > > <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
> > > Cc: Abner Chang
> <abner.chang@hpe.com><mailto:abner.chang@hpe.com>
> > > Cc: Gilbert Chen
> <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>
> > > Cc: Leif Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
> > > Cc: Dandan Bi <dandan.bi@intel.com><mailto:dandan.bi@intel.com>
> > > Cc: Eric Dong <eric.dong@intel.com><mailto:eric.dong@intel.com>
> > > ---
> > >
> > > Notes:
> > >     v2:
> > >       - Use CopyMem instead of CopyGuid [Dandan]
> > >
> > >  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git
> a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > index 5cc527679a78..0540e6fa8a44 100644
> > > --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > @@ -619,7 +619,7 @@ CreateDeviceManagerForm(
> > >          TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
> > >          FreePool (String);
> > >
> > > -        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
> > > +        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid,
> > > + sizeof (EFI_GUID));
> > >
> > >          //
> > >          // Network device process
> > > --
> > > 2.25.0
> > >
> > >
> > >
> > >
> >
> > 
> >

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Leif Lindholm]

And what would you propose we do the next time the RISC-V toolchain
generates a memcpy call based on some other completely valid change to
core code?

Doing this de-risks the RISC-V upstreaming effort.
It's also a trivial move/rename opetation - the only question is where
the library should live and be called.

/
    Leif

On Thu, Mar 12, 2020 at 14:33:27 +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
> I don't prefer to do this at this moment, Leif. I have no problem if
> we create a BZ for this and create BaseCompilerIntrinsicsLib in
> MdeModulePkg, but please don't bind it with RISC-V EDK2 port. You
> know my concern pretty well, we can't hold those RISC-V patches on
> hands like forever and address the code structure optimization. We
> can still work on BaseCompilerIntrinsicsLib but not part of this
> RISC-V submission. We can implement RISC-V variant if necessary
> after RISC-V edk2 get in edk2 master.
> 
> Abner
> 
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif@nuviainc.com]
> > Sent: Thursday, March 12, 2020 10:03 PM
> > To: Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com>
> > Cc: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> > <abner.chang@hpe.com>; Chen, Gilbert <gilbert.chen@hpe.com>;
> > afish@apple.com; michael.d.kinney@intel.com; pete@akeo.ie; Ard
> > Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>
> > Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> > instead of GUID assignment
> > 
> > +Ard, Laszlo.
> > 
> > I think it would make sense to move it to MdeModulePkg (or MdePkg) and
> > rename it BaseCompilerIntrinsicsLib (it *is* a BASE library).
> > 
> > As I alluded in my reply to Ray - x86 also have this problem, but to a lesser
> > extent, and ended up creating library functions to call instead of using plain C
> > for certain operations.
> > Which was probably the right decision when it was restricted to a very few
> > corner cases.
> > 
> > /
> >     Leif
> > 
> > On Thu, Mar 12, 2020 at 13:24:30 +0000, Schaefer, Daniel (DualStudy) wrote:
> > > Hi Leif,
> > >
> > > you're right. If I revert my commit and include
> > >   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> > > without making any changes to it, the build succeeds.
> > >
> > > What do others think? (cc Michael, Pete, Andrew, Ard, who have made
> > > changes to this module) Is this a big hack or should we use it in RISC-V, too
> > and move the module to MdeModulePkg?
> > > Why isn't this a problem on x86? Was it fine on Itanium?
> > >
> > > - Daniel
> > >
> > > ________________________________
> > > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > > <devel@edk2.groups.io><mailto:devel@edk2.groups.io> on behalf of Leif
> > > Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
> > > Sent: Thursday, March 12, 2020 11:55
> > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > > <devel@edk2.groups.io><mailto:devel@edk2.groups.io>; Schaefer, Daniel
> > > (DualStudy)
> > <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
> > > Cc: Chang, Abner (HPS SW/FW Technologist)
> > > <abner.chang@hpe.com><mailto:abner.chang@hpe.com>; Chen, Gilbert
> > > <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>; Dandan Bi
> > > <dandan.bi@intel.com><mailto:dandan.bi@intel.com>; Eric Dong
> > > <eric.dong@intel.com><mailto:eric.dong@intel.com>
> > > Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> > > instead of GUID assignment
> > >
> > > Hi Daniel,
> > >
> > > There is nothing wrong with this patch that just went in (and I should
> > > have called out sooner if I wanted to stop it), but I think a better
> > > solution is to implement a RISC-V variant of
> > > ArmPkg/Library/CompilerIntrinsicsLib/.
> > >
> > > It is perfectly valid for the compiler to generate memcpy calls in
> > > response to struct operations that are perfectly valid C.
> > >
> > > In fact, we could consider moving the ArmPkg one over into
> > > MdeModulePkg. I have a feeling that including a
> > >   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> > > in your current build would be an alternative solution to your
> > > compilation error.
> > >
> > > /
> > >     Leif
> > >
> > > On Mon, Mar 02, 2020 at 11:32:38 +0100, Daniel Schaefer wrote:
> > > > GCC translates a simple assignment to memcpy, which EDKII doesn't
> > provide.
> > > > See:
> > > > INVALID URI REMOVED
> > 2Darch
> > > > ive.com_edk2-2Ddevel-
> > 40lists.01.org_msg11928.html&d=DwIBAg&c=C5b8zRQ
> > > >
> > O1miGmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3
> > E&m=wjlf
> > > >
> > QYZsXfc5WWDFmywLOEYlCKvrpYrnaXMIpJcLK7U&s=426yv7VvgHTtgtYaR0f0X
> > NiaEA
> > > > LgqZkxektjywEwPco&e=
> > > >
> > > > REF:INVALID URI REMOVED
> > > > anocore.org_show-5Fbug.cgi-3Fid-
> > 3D2547&d=DwIBAg&c=C5b8zRQO1miGmBeVZ2
> > > >
> > LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=wjlfQYZsX
> > fc5WWD
> > > > FmywLOEYlCKvrpYrnaXMIpJcLK7U&s=PjC_mIwh0GhNy5np2h7K8l4l-
> > pLLBWt1FRuLB
> > > > UbULpc&e=
> > > >
> > > > Signed-off-by: Daniel Schaefer
> > > > <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
> > > > Cc: Abner Chang
> > <abner.chang@hpe.com><mailto:abner.chang@hpe.com>
> > > > Cc: Gilbert Chen
> > <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>
> > > > Cc: Leif Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
> > > > Cc: Dandan Bi <dandan.bi@intel.com><mailto:dandan.bi@intel.com>
> > > > Cc: Eric Dong <eric.dong@intel.com><mailto:eric.dong@intel.com>
> > > > ---
> > > >
> > > > Notes:
> > > >     v2:
> > > >       - Use CopyMem instead of CopyGuid [Dandan]
> > > >
> > > >  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git
> > a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > > b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > > index 5cc527679a78..0540e6fa8a44 100644
> > > > --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > > +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > > @@ -619,7 +619,7 @@ CreateDeviceManagerForm(
> > > >          TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
> > > >          FreePool (String);
> > > >
> > > > -        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
> > > > +        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid,
> > > > + sizeof (EFI_GUID));
> > > >
> > > >          //
> > > >          // Network device process
> > > > --
> > > > 2.25.0
> > > >
> > > >
> > > >
> > > >
> > >
> > > 
> > >

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Leif Lindholm]

For clarity, I'm suggesting *I* take care of moving this into a
generic Intrinsics handling lib, and that HPE take care of including
and using it.

I'm also not suggesting we revert the CopyMem patch before that is
complete.

On Thu, Mar 12, 2020 at 14:44:52 +0000, Leif Lindholm wrote:
> And what would you propose we do the next time the RISC-V toolchain
> generates a memcpy call based on some other completely valid change to
> core code?
> 
> Doing this de-risks the RISC-V upstreaming effort.
> It's also a trivial move/rename opetation - the only question is where
> the library should live and be called.
> 
> /
>     Leif
> 
> On Thu, Mar 12, 2020 at 14:33:27 +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
> > I don't prefer to do this at this moment, Leif. I have no problem if
> > we create a BZ for this and create BaseCompilerIntrinsicsLib in
> > MdeModulePkg, but please don't bind it with RISC-V EDK2 port. You
> > know my concern pretty well, we can't hold those RISC-V patches on
> > hands like forever and address the code structure optimization. We
> > can still work on BaseCompilerIntrinsicsLib but not part of this
> > RISC-V submission. We can implement RISC-V variant if necessary
> > after RISC-V edk2 get in edk2 master.
> > 
> > Abner
> > 
> > > -----Original Message-----
> > > From: Leif Lindholm [mailto:leif@nuviainc.com]
> > > Sent: Thursday, March 12, 2020 10:03 PM
> > > To: Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com>
> > > Cc: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> > > <abner.chang@hpe.com>; Chen, Gilbert <gilbert.chen@hpe.com>;
> > > afish@apple.com; michael.d.kinney@intel.com; pete@akeo.ie; Ard
> > > Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>
> > > Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> > > instead of GUID assignment
> > > 
> > > +Ard, Laszlo.
> > > 
> > > I think it would make sense to move it to MdeModulePkg (or MdePkg) and
> > > rename it BaseCompilerIntrinsicsLib (it *is* a BASE library).
> > > 
> > > As I alluded in my reply to Ray - x86 also have this problem, but to a lesser
> > > extent, and ended up creating library functions to call instead of using plain C
> > > for certain operations.
> > > Which was probably the right decision when it was restricted to a very few
> > > corner cases.
> > > 
> > > /
> > >     Leif
> > > 
> > > On Thu, Mar 12, 2020 at 13:24:30 +0000, Schaefer, Daniel (DualStudy) wrote:
> > > > Hi Leif,
> > > >
> > > > you're right. If I revert my commit and include
> > > >   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> > > > without making any changes to it, the build succeeds.
> > > >
> > > > What do others think? (cc Michael, Pete, Andrew, Ard, who have made
> > > > changes to this module) Is this a big hack or should we use it in RISC-V, too
> > > and move the module to MdeModulePkg?
> > > > Why isn't this a problem on x86? Was it fine on Itanium?
> > > >
> > > > - Daniel
> > > >
> > > > ________________________________
> > > > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > > > <devel@edk2.groups.io><mailto:devel@edk2.groups.io> on behalf of Leif
> > > > Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
> > > > Sent: Thursday, March 12, 2020 11:55
> > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > > > <devel@edk2.groups.io><mailto:devel@edk2.groups.io>; Schaefer, Daniel
> > > > (DualStudy)
> > > <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
> > > > Cc: Chang, Abner (HPS SW/FW Technologist)
> > > > <abner.chang@hpe.com><mailto:abner.chang@hpe.com>; Chen, Gilbert
> > > > <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>; Dandan Bi
> > > > <dandan.bi@intel.com><mailto:dandan.bi@intel.com>; Eric Dong
> > > > <eric.dong@intel.com><mailto:eric.dong@intel.com>
> > > > Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> > > > instead of GUID assignment
> > > >
> > > > Hi Daniel,
> > > >
> > > > There is nothing wrong with this patch that just went in (and I should
> > > > have called out sooner if I wanted to stop it), but I think a better
> > > > solution is to implement a RISC-V variant of
> > > > ArmPkg/Library/CompilerIntrinsicsLib/.
> > > >
> > > > It is perfectly valid for the compiler to generate memcpy calls in
> > > > response to struct operations that are perfectly valid C.
> > > >
> > > > In fact, we could consider moving the ArmPkg one over into
> > > > MdeModulePkg. I have a feeling that including a
> > > >   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> > > > in your current build would be an alternative solution to your
> > > > compilation error.
> > > >
> > > > /
> > > >     Leif
> > > >
> > > > On Mon, Mar 02, 2020 at 11:32:38 +0100, Daniel Schaefer wrote:
> > > > > GCC translates a simple assignment to memcpy, which EDKII doesn't
> > > provide.
> > > > > See:
> > > > > INVALID URI REMOVED
> > > 2Darch
> > > > > ive.com_edk2-2Ddevel-
> > > 40lists.01.org_msg11928.html&d=DwIBAg&c=C5b8zRQ
> > > > >
> > > O1miGmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3
> > > E&m=wjlf
> > > > >
> > > QYZsXfc5WWDFmywLOEYlCKvrpYrnaXMIpJcLK7U&s=426yv7VvgHTtgtYaR0f0X
> > > NiaEA
> > > > > LgqZkxektjywEwPco&e=
> > > > >
> > > > > REF:INVALID URI REMOVED
> > > > > anocore.org_show-5Fbug.cgi-3Fid-
> > > 3D2547&d=DwIBAg&c=C5b8zRQO1miGmBeVZ2
> > > > >
> > > LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=wjlfQYZsX
> > > fc5WWD
> > > > > FmywLOEYlCKvrpYrnaXMIpJcLK7U&s=PjC_mIwh0GhNy5np2h7K8l4l-
> > > pLLBWt1FRuLB
> > > > > UbULpc&e=
> > > > >
> > > > > Signed-off-by: Daniel Schaefer
> > > > > <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
> > > > > Cc: Abner Chang
> > > <abner.chang@hpe.com><mailto:abner.chang@hpe.com>
> > > > > Cc: Gilbert Chen
> > > <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>
> > > > > Cc: Leif Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
> > > > > Cc: Dandan Bi <dandan.bi@intel.com><mailto:dandan.bi@intel.com>
> > > > > Cc: Eric Dong <eric.dong@intel.com><mailto:eric.dong@intel.com>
> > > > > ---
> > > > >
> > > > > Notes:
> > > > >     v2:
> > > > >       - Use CopyMem instead of CopyGuid [Dandan]
> > > > >
> > > > >  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git
> > > a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > > > b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > > > index 5cc527679a78..0540e6fa8a44 100644
> > > > > --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > > > +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > > > @@ -619,7 +619,7 @@ CreateDeviceManagerForm(
> > > > >          TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
> > > > >          FreePool (String);
> > > > >
> > > > > -        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
> > > > > +        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid,
> > > > > + sizeof (EFI_GUID));
> > > > >
> > > > >          //
> > > > >          // Network device process
> > > > > --
> > > > > 2.25.0
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > > 
> > > >

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Laszlo Ersek]

On 03/12/20 15:03, Leif Lindholm wrote:
> +Ard, Laszlo.
> 
> I think it would make sense to move it to MdeModulePkg (or MdePkg) and
> rename it BaseCompilerIntrinsicsLib (it *is* a BASE library).
> 
> As I alluded in my reply to Ray - x86 also have this problem, but to a
> lesser extent, and ended up creating library functions to call instead
> of using plain C for certain operations.
> Which was probably the right decision when it was restricted to a
> very few corner cases.

I think people that are interested in IA32/X64 are happier with explicit
CopyMem() calls that are optimized (via one of the several BaseMemoryLib
instances, such as SSE2, REP + string instructions, MMX, "smart"
(chunked) C code, etc), than with a naive loop, such as the one in
"ArmPkg/Library/CompilerIntrinsicsLib/memcpy.c", that gets silently
plugged into an intrinsic (such as a structure assignment).

I mean, compiler intrinsics exist in the first place because they
implement language features in a fast / performant manner, behind the
scenes. If we replace the internals of an intrinsic with a slow / naive
implementation, then the intrinsic has no more right to exist, the
compiler could / should just generate the normal naive code.

I don't mind the code movement, but I'd like to avoid using
BaseCompilerIntrinsicsLib on IA32/X64. On those arches, I think it would
only be an improvement if it had a configurable backend, similarly how
CopyMem() is currently configurable.

... I guess I've gotten very used to calling CopyMem(), in place of
structure assignment.

Thanks
Laszlo

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Laszlo Ersek]

On 03/12/20 15:44, Leif Lindholm wrote:
> And what would you propose we do the next time the RISC-V toolchain
> generates a memcpy call based on some other completely valid change to
> core code?

We could choose to enable the intrinsics library for RISC-V at that point.

IIUC, the CreateDeviceManagerForm() code in question did break an edk2
rule ("don't use structure assignment") *prior* to commit 64a228f5f893.
The rule violation was in commit 32465d9ae7ee; RISC-V only exposed it.
This doesn't seem uncharted territory.

Thanks
Laszlo

> Doing this de-risks the RISC-V upstreaming effort.
> It's also a trivial move/rename opetation - the only question is where
> the library should live and be called.
> 
> /
>     Leif
> 
> On Thu, Mar 12, 2020 at 14:33:27 +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
>> I don't prefer to do this at this moment, Leif. I have no problem if
>> we create a BZ for this and create BaseCompilerIntrinsicsLib in
>> MdeModulePkg, but please don't bind it with RISC-V EDK2 port. You
>> know my concern pretty well, we can't hold those RISC-V patches on
>> hands like forever and address the code structure optimization. We
>> can still work on BaseCompilerIntrinsicsLib but not part of this
>> RISC-V submission. We can implement RISC-V variant if necessary
>> after RISC-V edk2 get in edk2 master.
>>
>> Abner
>>
>>> -----Original Message-----
>>> From: Leif Lindholm [mailto:leif@nuviainc.com]
>>> Sent: Thursday, March 12, 2020 10:03 PM
>>> To: Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com>
>>> Cc: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
>>> <abner.chang@hpe.com>; Chen, Gilbert <gilbert.chen@hpe.com>;
>>> afish@apple.com; michael.d.kinney@intel.com; pete@akeo.ie; Ard
>>> Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>
>>> Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
>>> instead of GUID assignment
>>>
>>> +Ard, Laszlo.
>>>
>>> I think it would make sense to move it to MdeModulePkg (or MdePkg) and
>>> rename it BaseCompilerIntrinsicsLib (it *is* a BASE library).
>>>
>>> As I alluded in my reply to Ray - x86 also have this problem, but to a lesser
>>> extent, and ended up creating library functions to call instead of using plain C
>>> for certain operations.
>>> Which was probably the right decision when it was restricted to a very few
>>> corner cases.
>>>
>>> /
>>>     Leif
>>>
>>> On Thu, Mar 12, 2020 at 13:24:30 +0000, Schaefer, Daniel (DualStudy) wrote:
>>>> Hi Leif,
>>>>
>>>> you're right. If I revert my commit and include
>>>>   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
>>>> without making any changes to it, the build succeeds.
>>>>
>>>> What do others think? (cc Michael, Pete, Andrew, Ard, who have made
>>>> changes to this module) Is this a big hack or should we use it in RISC-V, too
>>> and move the module to MdeModulePkg?
>>>> Why isn't this a problem on x86? Was it fine on Itanium?
>>>>
>>>> - Daniel
>>>>
>>>> ________________________________
>>>> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
>>>> <devel@edk2.groups.io><mailto:devel@edk2.groups.io> on behalf of Leif
>>>> Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
>>>> Sent: Thursday, March 12, 2020 11:55
>>>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
>>>> <devel@edk2.groups.io><mailto:devel@edk2.groups.io>; Schaefer, Daniel
>>>> (DualStudy)
>>> <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
>>>> Cc: Chang, Abner (HPS SW/FW Technologist)
>>>> <abner.chang@hpe.com><mailto:abner.chang@hpe.com>; Chen, Gilbert
>>>> <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>; Dandan Bi
>>>> <dandan.bi@intel.com><mailto:dandan.bi@intel.com>; Eric Dong
>>>> <eric.dong@intel.com><mailto:eric.dong@intel.com>
>>>> Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
>>>> instead of GUID assignment
>>>>
>>>> Hi Daniel,
>>>>
>>>> There is nothing wrong with this patch that just went in (and I should
>>>> have called out sooner if I wanted to stop it), but I think a better
>>>> solution is to implement a RISC-V variant of
>>>> ArmPkg/Library/CompilerIntrinsicsLib/.
>>>>
>>>> It is perfectly valid for the compiler to generate memcpy calls in
>>>> response to struct operations that are perfectly valid C.
>>>>
>>>> In fact, we could consider moving the ArmPkg one over into
>>>> MdeModulePkg. I have a feeling that including a
>>>>   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
>>>> in your current build would be an alternative solution to your
>>>> compilation error.
>>>>
>>>> /
>>>>     Leif
>>>>
>>>> On Mon, Mar 02, 2020 at 11:32:38 +0100, Daniel Schaefer wrote:
>>>>> GCC translates a simple assignment to memcpy, which EDKII doesn't
>>> provide.
>>>>> See:
>>>>> INVALID URI REMOVED
>>> 2Darch
>>>>> ive.com_edk2-2Ddevel-
>>> 40lists.01.org_msg11928.html&d=DwIBAg&c=C5b8zRQ
>>>>>
>>> O1miGmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3
>>> E&m=wjlf
>>>>>
>>> QYZsXfc5WWDFmywLOEYlCKvrpYrnaXMIpJcLK7U&s=426yv7VvgHTtgtYaR0f0X
>>> NiaEA
>>>>> LgqZkxektjywEwPco&e=
>>>>>
>>>>> REF:INVALID URI REMOVED
>>>>> anocore.org_show-5Fbug.cgi-3Fid-
>>> 3D2547&d=DwIBAg&c=C5b8zRQO1miGmBeVZ2
>>>>>
>>> LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=wjlfQYZsX
>>> fc5WWD
>>>>> FmywLOEYlCKvrpYrnaXMIpJcLK7U&s=PjC_mIwh0GhNy5np2h7K8l4l-
>>> pLLBWt1FRuLB
>>>>> UbULpc&e=
>>>>>
>>>>> Signed-off-by: Daniel Schaefer
>>>>> <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
>>>>> Cc: Abner Chang
>>> <abner.chang@hpe.com><mailto:abner.chang@hpe.com>
>>>>> Cc: Gilbert Chen
>>> <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>
>>>>> Cc: Leif Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
>>>>> Cc: Dandan Bi <dandan.bi@intel.com><mailto:dandan.bi@intel.com>
>>>>> Cc: Eric Dong <eric.dong@intel.com><mailto:eric.dong@intel.com>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>>     v2:
>>>>>       - Use CopyMem instead of CopyGuid [Dandan]
>>>>>
>>>>>  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git
>>> a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
>>>>> b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
>>>>> index 5cc527679a78..0540e6fa8a44 100644
>>>>> --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
>>>>> +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
>>>>> @@ -619,7 +619,7 @@ CreateDeviceManagerForm(
>>>>>          TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
>>>>>          FreePool (String);
>>>>>
>>>>> -        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
>>>>> +        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid,
>>>>> + sizeof (EFI_GUID));
>>>>>
>>>>>          //
>>>>>          // Network device process
>>>>> --
>>>>> 2.25.0
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>> 
>>>>
> 

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Andrew Fish]

> On Mar 12, 2020, at 12:36 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 03/12/20 15:03, Leif Lindholm wrote:
>> +Ard, Laszlo.
>> 
>> I think it would make sense to move it to MdeModulePkg (or MdePkg) and
>> rename it BaseCompilerIntrinsicsLib (it *is* a BASE library).
>> 
>> As I alluded in my reply to Ray - x86 also have this problem, but to a
>> lesser extent, and ended up creating library functions to call instead
>> of using plain C for certain operations.
>> Which was probably the right decision when it was restricted to a
>> very few corner cases.
> 
> I think people that are interested in IA32/X64 are happier with explicit
> CopyMem() calls that are optimized (via one of the several BaseMemoryLib
> instances, such as SSE2, REP + string instructions, MMX, "smart"
> (chunked) C code, etc), than with a naive loop, such as the one in
> "ArmPkg/Library/CompilerIntrinsicsLib/memcpy.c", that gets silently
> plugged into an intrinsic (such as a structure assignment).
> 
> I mean, compiler intrinsics exist in the first place because they
> implement language features in a fast / performant manner, behind the
> scenes. If we replace the internals of an intrinsic with a slow / naive
> implementation, then the intrinsic has no more right to exist, the
> compiler could / should just generate the normal naive code.
> 
> I don't mind the code movement, but I'd like to avoid using
> BaseCompilerIntrinsicsLib on IA32/X64. On those arches, I think it would
> only be an improvement if it had a configurable backend, similarly how
> CopyMem() is currently configurable.
> 
> ... I guess I've gotten very used to calling CopyMem(), in place of
> structure assignment.
> 

Laszlo,

My brain has flipped too. 

For x86 I find smaller structure assignments can cause the optimizer to optimize away the memcpy/memset and you only see issue issues on a NOOPT build since DEBUG builds tend to be size optimized. I tend to hit  this issue when I try to turn off optimizations to try and walk vendor code in the debugger. So code review is the 1st line of defense. 

Thanks,

Andrew Fish

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Leif Lindholm]

On Thu, Mar 12, 2020 at 20:36:04 +0100, Laszlo Ersek wrote:
> On 03/12/20 15:03, Leif Lindholm wrote:
> > +Ard, Laszlo.
> > 
> > I think it would make sense to move it to MdeModulePkg (or MdePkg) and
> > rename it BaseCompilerIntrinsicsLib (it *is* a BASE library).
> > 
> > As I alluded in my reply to Ray - x86 also have this problem, but to a
> > lesser extent, and ended up creating library functions to call instead
> > of using plain C for certain operations.
> > Which was probably the right decision when it was restricted to a
> > very few corner cases.
> 
> I think people that are interested in IA32/X64 are happier with explicit
> CopyMem() calls that are optimized (via one of the several BaseMemoryLib
> instances, such as SSE2, REP + string instructions, MMX, "smart"
> (chunked) C code, etc), than with a naive loop, such as the one in
> "ArmPkg/Library/CompilerIntrinsicsLib/memcpy.c", that gets silently
> plugged into an intrinsic (such as a structure assignment).
> 
> I mean, compiler intrinsics exist in the first place because they
> implement language features in a fast / performant manner, behind the
> scenes.

That may have been the original intent. The end result is we *have*
them, so we must do something about them.

> If we replace the internals of an intrinsic with a slow / naive
> implementation, then the intrinsic has no more right to exist, the
> compiler could / should just generate the normal naive code.

Sure. That's a toolchain issue, which we don't always control.

In the case of CopyGuid() I would take some convincing that there was
a significant difference in performance across 16 bytes of cached
memory, performed occasionally.

But, if there was, we could do

--- a/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
+++ b/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
@@ -86,3 +86,4 @@ [Packages]
[BuildOptions]
   MSFT:*_*_*_CC_FLAGS = /GL-
   MSFT:*_*_ARM_ASM_FLAGS = /oldit
+  GCC:RELEASE_*_*_CC_FLAGS = -O3

and let idiom recognition take care of inserting the optimised
versions in place of the naive ones.

> I don't mind the code movement, but I'd like to avoid using
> BaseCompilerIntrinsicsLib on IA32/X64. On those arches, I think it would
> only be an improvement if it had a configurable backend, similarly how
> CopyMem() is currently configurable.

With less than making CompilerIntrinsicLib *not* a BASE library
(urgh), we can't have *it* depending on platform-specific optimised
versions.

The comment about IA32/X64 was more about the few instances we've seen
where new code has broken them due to things like dividing 64-bit
values withouy function calls, and how in the future it might make
sense catching that in other ways.

> ... I guess I've gotten very used to calling CopyMem(), in place of
> structure assignment.

Basically, I don't think having to learn a new dialect of C is
consistent with lowering the barrier of entry to the project.

Regards,

Leif

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Leif Lindholm]

On Thu, Mar 12, 2020 at 20:42:52 +0100, Laszlo Ersek wrote:
> On 03/12/20 15:44, Leif Lindholm wrote:
> > And what would you propose we do the next time the RISC-V toolchain
> > generates a memcpy call based on some other completely valid change to
> > core code?
> 
> We could choose to enable the intrinsics library for RISC-V at that point.

We could. And have no time left for resolving any issues that may be
triggered by that without slipping the next stable tag. I would prefer
de-risking it.

> IIUC, the CreateDeviceManagerForm() code in question did break an edk2
> rule ("don't use structure assignment") *prior* to commit 64a228f5f893.
> The rule violation was in commit 32465d9ae7ee; RISC-V only exposed it.
> This doesn't seem uncharted territory.

I don't understand, I've already said I'm not pushing to revert that
patch, I have suggested that we don't put RISC-V on less stable ground
than ARM/AARCH64.

But continuing on the unrelated topic:
If the rule is "no structure assignments", then fine, that's part of
the C dialect you need to learn in order to contribute to TianoCore.
I can separately start arguing for changing that rule.
However, I can't easily find that in the coding style - could you give
me a pointer?

/
    Leif

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Abner Chang]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Leif Lindholm
> Sent: Thursday, March 12, 2020 10:57 PM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>
> Cc: Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com>;
> devel@edk2.groups.io; Chen, Gilbert <gilbert.chen@hpe.com>;
> afish@apple.com; michael.d.kinney@intel.com; pete@akeo.ie; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> instead of GUID assignment
> 
> For clarity, I'm suggesting *I* take care of moving this into a generic Intrinsics
> handling lib, and that HPE take care of including and using it.
Great to hear this.
> 
> I'm also not suggesting we revert the CopyMem patch before that is
> complete.
> 
> On Thu, Mar 12, 2020 at 14:44:52 +0000, Leif Lindholm wrote:
> > And what would you propose we do the next time the RISC-V toolchain
> > generates a memcpy call based on some other completely valid change to
> > core code?
> >
> > Doing this de-risks the RISC-V upstreaming effort.
> > It's also a trivial move/rename opetation - the only question is where
> > the library should live and be called.
> >
> > /
> >     Leif
> >
> > On Thu, Mar 12, 2020 at 14:33:27 +0000, Chang, Abner (HPS SW/FW
> Technologist) wrote:
> > > I don't prefer to do this at this moment, Leif. I have no problem if
> > > we create a BZ for this and create BaseCompilerIntrinsicsLib in
> > > MdeModulePkg, but please don't bind it with RISC-V EDK2 port. You
> > > know my concern pretty well, we can't hold those RISC-V patches on
> > > hands like forever and address the code structure optimization. We
> > > can still work on BaseCompilerIntrinsicsLib but not part of this
> > > RISC-V submission. We can implement RISC-V variant if necessary
> > > after RISC-V edk2 get in edk2 master.
> > >
> > > Abner
> > >
> > > > -----Original Message-----
> > > > From: Leif Lindholm [mailto:leif@nuviainc.com]
> > > > Sent: Thursday, March 12, 2020 10:03 PM
> > > > To: Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com>
> > > > Cc: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> > > > <abner.chang@hpe.com>; Chen, Gilbert <gilbert.chen@hpe.com>;
> > > > afish@apple.com; michael.d.kinney@intel.com; pete@akeo.ie; Ard
> > > > Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek
> > > > <lersek@redhat.com>
> > > > Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use
> CopyMem
> > > > instead of GUID assignment
> > > >
> > > > +Ard, Laszlo.
> > > >
> > > > I think it would make sense to move it to MdeModulePkg (or MdePkg)
> > > > and rename it BaseCompilerIntrinsicsLib (it *is* a BASE library).
> > > >
> > > > As I alluded in my reply to Ray - x86 also have this problem, but
> > > > to a lesser extent, and ended up creating library functions to
> > > > call instead of using plain C for certain operations.
> > > > Which was probably the right decision when it was restricted to a
> > > > very few corner cases.
> > > >
> > > > /
> > > >     Leif
> > > >
> > > > On Thu, Mar 12, 2020 at 13:24:30 +0000, Schaefer, Daniel (DualStudy)
> wrote:
> > > > > Hi Leif,
> > > > >
> > > > > you're right. If I revert my commit and include
> > > > >
> > > > > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.
> > > > > inf without making any changes to it, the build succeeds.
> > > > >
> > > > > What do others think? (cc Michael, Pete, Andrew, Ard, who have
> > > > > made changes to this module) Is this a big hack or should we use
> > > > > it in RISC-V, too
> > > > and move the module to MdeModulePkg?
> > > > > Why isn't this a problem on x86? Was it fine on Itanium?
> > > > >
> > > > > - Daniel
> > > > >
> > > > > ________________________________
> > > > > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > > > > <devel@edk2.groups.io><mailto:devel@edk2.groups.io> on behalf
> of
> > > > > Leif Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
> > > > > Sent: Thursday, March 12, 2020 11:55
> > > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > > > > <devel@edk2.groups.io><mailto:devel@edk2.groups.io>; Schaefer,
> > > > > Daniel
> > > > > (DualStudy)
> > > > <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
> > > > > Cc: Chang, Abner (HPS SW/FW Technologist)
> > > > > <abner.chang@hpe.com><mailto:abner.chang@hpe.com>; Chen,
> Gilbert
> > > > > <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>; Dandan Bi
> > > > > <dandan.bi@intel.com><mailto:dandan.bi@intel.com>; Eric Dong
> > > > > <eric.dong@intel.com><mailto:eric.dong@intel.com>
> > > > > Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use
> > > > > CopyMem instead of GUID assignment
> > > > >
> > > > > Hi Daniel,
> > > > >
> > > > > There is nothing wrong with this patch that just went in (and I
> > > > > should have called out sooner if I wanted to stop it), but I
> > > > > think a better solution is to implement a RISC-V variant of
> > > > > ArmPkg/Library/CompilerIntrinsicsLib/.
> > > > >
> > > > > It is perfectly valid for the compiler to generate memcpy calls
> > > > > in response to struct operations that are perfectly valid C.
> > > > >
> > > > > In fact, we could consider moving the ArmPkg one over into
> > > > > MdeModulePkg. I have a feeling that including a
> > > > >
> > > > > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.
> > > > > inf in your current build would be an alternative solution to
> > > > > your compilation error.
> > > > >
> > > > > /
> > > > >     Leif
> > > > >
> > > > > On Mon, Mar 02, 2020 at 11:32:38 +0100, Daniel Schaefer wrote:
> > > > > > GCC translates a simple assignment to memcpy, which EDKII
> > > > > > doesn't
> > > > provide.
> > > > > > See:
> > > > > > INVALID URI REMOVED
> > > > 2Darch
> > > > > > ive.com_edk2-2Ddevel-
> > > > 40lists.01.org_msg11928.html&d=DwIBAg&c=C5b8zRQ
> > > > > >
> > > >
> O1miGmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3
> > > > E&m=wjlf
> > > > > >
> > > >
> QYZsXfc5WWDFmywLOEYlCKvrpYrnaXMIpJcLK7U&s=426yv7VvgHTtgtYaR0f0X
> > > > NiaEA
> > > > > > LgqZkxektjywEwPco&e=
> > > > > >
> > > > > > REF:INVALID URI REMOVED
> > > > > > anocore.org_show-5Fbug.cgi-3Fid-
> > > > 3D2547&d=DwIBAg&c=C5b8zRQO1miGmBeVZ2
> > > > > >
> > > >
> LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=wjlfQYZsX
> > > > fc5WWD
> > > > > > FmywLOEYlCKvrpYrnaXMIpJcLK7U&s=PjC_mIwh0GhNy5np2h7K8l4l-
> > > > pLLBWt1FRuLB
> > > > > > UbULpc&e=
> > > > > >
> > > > > > Signed-off-by: Daniel Schaefer
> > > > > > <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
> > > > > > Cc: Abner Chang
> > > > <abner.chang@hpe.com><mailto:abner.chang@hpe.com>
> > > > > > Cc: Gilbert Chen
> > > > <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>
> > > > > > Cc: Leif Lindholm
> > > > > > <leif@nuviainc.com><mailto:leif@nuviainc.com>
> > > > > > Cc: Dandan Bi
> > > > > > <dandan.bi@intel.com><mailto:dandan.bi@intel.com>
> > > > > > Cc: Eric Dong
> > > > > > <eric.dong@intel.com><mailto:eric.dong@intel.com>
> > > > > > ---
> > > > > >
> > > > > > Notes:
> > > > > >     v2:
> > > > > >       - Use CopyMem instead of CopyGuid [Dandan]
> > > > > >
> > > > > >  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2
> > > > > > +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git
> > > > a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > > > > b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > > > > index 5cc527679a78..0540e6fa8a44 100644
> > > > > > ---
> a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > > > > +++
> b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > > > > @@ -619,7 +619,7 @@ CreateDeviceManagerForm(
> > > > > >          TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
> > > > > >          FreePool (String);
> > > > > >
> > > > > > -        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
> > > > > > +        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *)
> > > > > > + Ptr)->Guid, sizeof (EFI_GUID));
> > > > > >
> > > > > >          //
> > > > > >          // Network device process
> > > > > > --
> > > > > > 2.25.0
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> 
> 

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Abner Chang]

> -----Original Message-----
> From: Leif Lindholm [mailto:leif@nuviainc.com]
> Sent: Friday, March 13, 2020 5:20 AM
> To: devel@edk2.groups.io; lersek@redhat.com
> Cc: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com>; Chen, Gilbert
> <gilbert.chen@hpe.com>; afish@apple.com; michael.d.kinney@intel.com;
> pete@akeo.ie; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> instead of GUID assignment

The current NULL instance of CompilerIntrinsicsLib is applied on every modules, this means it's not flexible for overwriting memcpy (for example) with the faster algorithm (such as SSEx instructions) for the specific module in the same DSC, right? That says we can't assign a special version of memcpy to just one particular module. 

> 
> On Thu, Mar 12, 2020 at 20:42:52 +0100, Laszlo Ersek wrote:
> > On 03/12/20 15:44, Leif Lindholm wrote:
> > > And what would you propose we do the next time the RISC-V toolchain
> > > generates a memcpy call based on some other completely valid change
> > > to core code?
> >
> > We could choose to enable the intrinsics library for RISC-V at that point.
and I would like to see the flexibility of overwriting memory library functions for particular modules. There is no special algorithm of memory manipulation so far in RISC-V spec, however, the working group of Vector extension does propose the new instruction sets.

> 
> We could. And have no time left for resolving any issues that may be
> triggered by that without slipping the next stable tag. I would prefer de-
> risking it.
> 
> > IIUC, the CreateDeviceManagerForm() code in question did break an edk2
> > rule ("don't use structure assignment") *prior* to commit 64a228f5f893.
> > The rule violation was in commit 32465d9ae7ee; RISC-V only exposed it.
> > This doesn't seem uncharted territory.
> 
> I don't understand, I've already said I'm not pushing to revert that patch, I
> have suggested that we don't put RISC-V on less stable ground than
> ARM/AARCH64.
> 
> But continuing on the unrelated topic:
> If the rule is "no structure assignments", then fine, that's part of the C dialect
> you need to learn in order to contribute to TianoCore.
> I can separately start arguing for changing that rule.
> However, I can't easily find that in the coding style - could you give me a
> pointer?
> 
> /
>     Leif

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Leif Lindholm]

On Fri, Mar 13, 2020 at 04:08:12 +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif@nuviainc.com]
> > Sent: Friday, March 13, 2020 5:20 AM
> > To: devel@edk2.groups.io; lersek@redhat.com
> > Cc: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com>; Chen, Gilbert
> > <gilbert.chen@hpe.com>; afish@apple.com; michael.d.kinney@intel.com;
> > pete@akeo.ie; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> > instead of GUID assignment
> 
> The current NULL instance of CompilerIntrinsicsLib is applied on
> every modules, this means it's not flexible for overwriting memcpy
> (for example) with the faster algorithm (such as SSEx instructions)
> for the specific module in the same DSC, right? That says we can't
> assign a special version of memcpy to just one particular module.

All true.
But ... are you planning to contribute lots of code that performs
large (certainly larger than GUIDs in order for effects to be
noticeable) struct assignments on critical paths?

Even if that is the case, as commented on the other fork of this
thread: if we add the -O3 jiggle, GCC will automatically insert what
it considers to be the optimal implementation for the specified target
when encountering the naïve implementation.

> > On Thu, Mar 12, 2020 at 20:42:52 +0100, Laszlo Ersek wrote:
> > > On 03/12/20 15:44, Leif Lindholm wrote:
> > > > And what would you propose we do the next time the RISC-V toolchain
> > > > generates a memcpy call based on some other completely valid change
> > > > to core code?
> > >
> > > We could choose to enable the intrinsics library for RISC-V at that point.
>
> and I would like to see the flexibility of overwriting memory
> library functions for particular modules. There is no special
> algorithm of memory manipulation so far in RISC-V spec, however, the
> working group of Vector extension does propose the new instruction
> sets.

Use of CompilerIntrinsicsLib has no effect on explicit CopyMem & co
operations, only on memcpy/memset generated by the compiler. Which
will generate build failures if not handled.

/
    Leif

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Laszlo Ersek]

On 03/12/20 22:19, Leif Lindholm wrote:
> On Thu, Mar 12, 2020 at 20:42:52 +0100, Laszlo Ersek wrote:
>> On 03/12/20 15:44, Leif Lindholm wrote:
>>> And what would you propose we do the next time the RISC-V toolchain
>>> generates a memcpy call based on some other completely valid change to
>>> core code?
>>
>> We could choose to enable the intrinsics library for RISC-V at that point.
> 
> We could. And have no time left for resolving any issues that may be
> triggered by that without slipping the next stable tag. I would prefer
> de-risking it.

OK.

>> IIUC, the CreateDeviceManagerForm() code in question did break an edk2
>> rule ("don't use structure assignment") *prior* to commit 64a228f5f893.
>> The rule violation was in commit 32465d9ae7ee; RISC-V only exposed it.
>> This doesn't seem uncharted territory.
> 
> I don't understand, I've already said I'm not pushing to revert that
> patch, I have suggested that we don't put RISC-V on less stable ground
> than ARM/AARCH64.

Apologies, I was unclear. By "not uncharted territory", I meant that
it's not uncommon for new code (regardless of architecture) to expose
dormant issues in old code. In particular the "no struct assignment"
rule is not being broken for the first time.

Anyway, I'm not opposing your suggestion.

> But continuing on the unrelated topic:
> If the rule is "no structure assignments", then fine, that's part of
> the C dialect you need to learn in order to contribute to TianoCore.
> I can separately start arguing for changing that rule.

*That* would be awesome, if it can be pulled off universally
(arch-independently), and hopefully without performance loss. (I do
agree it's unlikely that we'll have tight loops with CopyGuid() etc.)

> However, I can't easily find that in the coding style - could you give
> me a pointer?

I'm not even going to look now: I believe I tried that earlier, and it's
not in the edk2 CCS (AFAIR). It's by word of mouth.

Laszlo

Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Abner Chang]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Leif Lindholm
> Sent: Friday, March 13, 2020 6:11 PM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>
> Cc: devel@edk2.groups.io; lersek@redhat.com; Schaefer, Daniel (DualStudy)
> <daniel.schaefer@hpe.com>; Chen, Gilbert <gilbert.chen@hpe.com>;
> afish@apple.com; michael.d.kinney@intel.com; pete@akeo.ie; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> instead of GUID assignment
> 
> On Fri, Mar 13, 2020 at 04:08:12 +0000, Chang, Abner (HPS SW/FW
> Technologist) wrote:
> > > -----Original Message-----
> > > From: Leif Lindholm [mailto:leif@nuviainc.com]
> > > Sent: Friday, March 13, 2020 5:20 AM
> > > To: devel@edk2.groups.io; lersek@redhat.com
> > > Cc: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > > Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com>; Chen,
> > > Gilbert <gilbert.chen@hpe.com>; afish@apple.com;
> > > michael.d.kinney@intel.com; pete@akeo.ie; Ard Biesheuvel
> > > <ard.biesheuvel@linaro.org>
> > > Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use
> CopyMem
> > > instead of GUID assignment
> >
> > The current NULL instance of CompilerIntrinsicsLib is applied on every
> > modules, this means it's not flexible for overwriting memcpy (for
> > example) with the faster algorithm (such as SSEx instructions) for the
> > specific module in the same DSC, right? That says we can't assign a
> > special version of memcpy to just one particular module.
> 
> All true.
> But ... are you planning to contribute lots of code that performs large
> (certainly larger than GUIDs in order for effects to be
> noticeable) struct assignments on critical paths?
A huge block of memory copy or set indeed a requirement of HPE server BIOS for graphic mode BIOS setup without VGA H/W accelerator and others such as memory test, but it is x86 system though. . Respond to your feedback below, that should be fine if new code just use CopyMem for the high performance memory operations. RISC-V has no problem with using intrinsic memcpy, there is no strong performance requirements for memory operations so far...maybe later. 
> 
> Even if that is the case, as commented on the other fork of this
> thread: if we add the -O3 jiggle, GCC will automatically insert what it
> considers to be the optimal implementation for the specified target when
> encountering the naïve implementation.
> 
> > > On Thu, Mar 12, 2020 at 20:42:52 +0100, Laszlo Ersek wrote:
> > > > On 03/12/20 15:44, Leif Lindholm wrote:
> > > > > And what would you propose we do the next time the RISC-V
> > > > > toolchain generates a memcpy call based on some other completely
> > > > > valid change to core code?
> > > >
> > > > We could choose to enable the intrinsics library for RISC-V at that point.
> >
> > and I would like to see the flexibility of overwriting memory library
> > functions for particular modules. There is no special algorithm of
> > memory manipulation so far in RISC-V spec, however, the working group
> > of Vector extension does propose the new instruction sets.
> 
> Use of CompilerIntrinsicsLib has no effect on explicit CopyMem & co
> operations, only on memcpy/memset generated by the compiler. Which will
> generate build failures if not handled.
> 
> /
>     Leif
> 
> 

Re: [EXTERNAL] RE: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]

Leif,
I didn’t realized tool supports a global NULL library class usage. I now found this https://bugzilla.tianocore.org/show_bug.cgi?id=645.

Thanks for the education😊

Thanks,
Ray


From: Leif Lindholm <leif@nuviainc.com>
Sent: Thursday, March 12, 2020 9:54 PM
To: Ni, Ray <ray.ni@intel.com>
Cc: devel@edk2.groups.io; daniel.schaefer@hpe.com; Abner Chang <abner.chang@hpe.com>; Gilbert Chen <gilbert.chen@hpe.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: Re: [EXTERNAL] RE: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

Hi Ray,

On Thu, Mar 12, 2020 at 12:21 PM Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>> wrote:
> Does CompilerIntrinsicsLib exist because GCC doesn't support to disable intrinsic?

It is not possible to disable all intrinsics on 32-bit ARM, and it's not necessarily desirable to do so for AArch64 (so I don't think it's implemented).

Note that this is somewhat true for x86 too - leading to things like DivU64x32 in BaseLib.

> How does the linker find the CompilerInstrinsicsLib in linking phase?

For example:
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/MdeModulePkg.dsc#L172

Regards,

Leif


[-- Attachment #2: Type: text/html, Size: 5053 bytes --]