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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55164): https://edk2.groups.io/g/devel/message/55164
Mute This Topic: https://groups.io/mt/71671270/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55193): https://edk2.groups.io/g/devel/message/55193 Mute This Topic: https://groups.io/mt/71671270/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55463): https://edk2.groups.io/g/devel/message/55463 Mute This Topic: https://groups.io/mt/71671270/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55790): https://edk2.groups.io/g/devel/message/55790 Mute This Topic: https://groups.io/mt/71671270/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55791): https://edk2.groups.io/g/devel/message/55791 Mute This Topic: https://groups.io/mt/71671270/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55798): https://edk2.groups.io/g/devel/message/55798
Mute This Topic: https://groups.io/mt/71671270/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55799): https://edk2.groups.io/g/devel/message/55799 Mute This Topic: https://groups.io/mt/71671270/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55810): https://edk2.groups.io/g/devel/message/55810 Mute This Topic: https://groups.io/mt/71902327/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56052): https://edk2.groups.io/g/devel/message/56052 Mute This Topic: https://groups.io/mt/71902327/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55800): https://edk2.groups.io/g/devel/message/55800
Mute This Topic: https://groups.io/mt/71671270/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
+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
> >
> >
> >
> >
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55811): https://edk2.groups.io/g/devel/message/55811
Mute This Topic: https://groups.io/mt/71671270/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55828): https://edk2.groups.io/g/devel/message/55828 Mute This Topic: https://groups.io/mt/71671270/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55830): https://edk2.groups.io/g/devel/message/55830 Mute This Topic: https://groups.io/mt/71671270/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55831): https://edk2.groups.io/g/devel/message/55831 Mute This Topic: https://groups.io/mt/71671270/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55814): https://edk2.groups.io/g/devel/message/55814 Mute This Topic: https://groups.io/mt/71671270/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55824): https://edk2.groups.io/g/devel/message/55824
Mute This Topic: https://groups.io/mt/71671270/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55825): https://edk2.groups.io/g/devel/message/55825 Mute This Topic: https://groups.io/mt/71671270/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> -----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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55842): https://edk2.groups.io/g/devel/message/55842 Mute This Topic: https://groups.io/mt/71671270/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55829): https://edk2.groups.io/g/devel/message/55829
Mute This Topic: https://groups.io/mt/71671270/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55833): https://edk2.groups.io/g/devel/message/55833
Mute This Topic: https://groups.io/mt/71671270/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55843): https://edk2.groups.io/g/devel/message/55843
Mute This Topic: https://groups.io/mt/71671270/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55845): https://edk2.groups.io/g/devel/message/55845
Mute This Topic: https://groups.io/mt/71671270/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----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 > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55855): https://edk2.groups.io/g/devel/message/55855 Mute This Topic: https://groups.io/mt/71671270/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55847): https://edk2.groups.io/g/devel/message/55847
Mute This Topic: https://groups.io/mt/71671270/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.