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

["Abner Chang" <abner.chang@hpe.com>]

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