Re: [PATCH 0/3] ArmPkg: introduce IsZeroGuid() and IsZeroBuffer()

[Laszlo Ersek <lersek@redhat.com>]

On 09/02/16 15:01, Leif Lindholm wrote:
> On Fri, Sep 02, 2016 at 01:33:21PM +0200, Laszlo Ersek wrote:
>>> Maybe this is a good point at which to move these into MdePkg, in the
>>> hope that the ARM versions won't be overlooked in future API
>>> revisions?
>>
>> I strongly suggest / request that your (good) suggestion be implemented
>> as a separate endeavor. Moving this stuff into MdePkg is definitely
>> justified, but it will almost certainly take a good chunk of time.
>> Meanwhile the ArmVirtQemu builds remain broken.
>>
>> I suggest to go ahead and commit patches #2 and #3 as well, and swiftly
>> at that. And, in order to keep ourselves honest about the longer term
>> goal, I propose to file a bug for the code movement in our Bugzilla
>> instance. (The affected packages should be MdePkg + ArmPkg.)
> 
> Surely if we're doing a panic fix, that should be to revert
> 313831d-72388f9?

I preferred a quick fix that was also not destructive. Patches #2 and #3
here can be considered the initial, basic implementation of the new
interfaces for ARM/AARCH64, which could (and should) have been part of
the original series (that introduced the breakage by missing them). IOW,
#2 and #3 could be construed as the continuation of that initial series,
for ARM/AARCH64.

> If we're not doing that, let's do the thing that will reduce the
> likelihood of this breaking again.
> In the interest of speeding this up, I would propose to wait with
> Ard's latest set (which deserves and is likely to see some more
> discussion) and go ahead with:
> - Nuking BaseMemoryLibVstm
> - Moving BaseMemoryLibStm to MdePkg
>   - Updating platforms in edk2 to reflect new location of
>     BaseMemoryLibStm
> - Add the new functions to BaseMemoryLibStm
> 
> This will interfere with nothing else under MdePkg, so could hopefully
> be merged today anyway.
> 
> Would that be an acceptable compromise?

I'd just like the tree to resume building. Doing anything at all under
MdePkg will require MdePkg reviewers to ACK the changes, which will
introduce delays. (Not because those reviewers are very slow, but
because MdePkg changes are always scrutinized, rightfully, with a fine
toothed comb.)

I wouldn't mind reverting the initial series either, except that it'd
require reviews for MdePkg and SecurityPkg changes... Same problem.

I'm quite sad that Hao missed grepping the tree for all INF files with
'LIBRARY_CLASS *= *BaseMemoryLib', when posting the original stuff :(

$ git grep -l 'LIBRARY_CLASS *= *BaseMemoryLib' -- '*inf'

ArmPkg/Library/BaseMemoryLibStm/BaseMemoryLibStm.inf <---------
MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
MdePkg/Library/BaseMemoryLibMmx/BaseMemoryLibMmx.inf
MdePkg/Library/BaseMemoryLibOptDxe/BaseMemoryLibOptDxe.inf
MdePkg/Library/BaseMemoryLibOptPei/BaseMemoryLibOptPei.inf
MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
MdePkg/Library/BaseMemoryLibSse2/BaseMemoryLibSse2.inf
MdePkg/Library/PeiMemoryLib/PeiMemoryLib.inf
MdePkg/Library/UefiMemoryLib/UefiMemoryLib.inf

Sigh.

Thanks
Laszlo

> 
> Regards,
> 
> Leif
> 
>> For patches #2 and #3:
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Can we please commit these patches today?
>>
>> Thanks,
>> Laszlo
>>
>>>
>>> /
>>>     Leif
>>>
>>>> Ard Biesheuvel (3):
>>>>   ArmPkg: remove BaseMemoryLibVstm implementation of BaseMemoryLib
>>>>   ArmPkg/BaseMemoryLibStm: implement new IsZeroGuid() API function
>>>>   ArmPkg/BaseMemoryLibStm: implement new IsZeroBuffer() API function
>>>>
>>>>  ArmPkg/ArmPkg.dsc                                                                             |   2 -
>>>>  ArmPkg/Library/BaseMemoryLibStm/BaseMemoryLibStm.inf                                          |   1 +
>>>>  ArmPkg/Library/{BaseMemoryLibVstm/ZeroMemWrapper.c => BaseMemoryLibStm/IsZeroBufferWrapper.c} |  28 ++-
>>>>  ArmPkg/Library/BaseMemoryLibStm/MemLibGeneric.c                                               |  29 +++
>>>>  ArmPkg/Library/BaseMemoryLibStm/MemLibGuid.c                                                  |  29 +++
>>>>  ArmPkg/Library/BaseMemoryLibStm/MemLibInternals.h                                             |  17 ++
>>>>  ArmPkg/Library/BaseMemoryLibVstm/Arm/CopyMem.S                                                | 112 ---------
>>>>  ArmPkg/Library/BaseMemoryLibVstm/Arm/CopyMem.asm                                              | 114 ---------
>>>>  ArmPkg/Library/BaseMemoryLibVstm/Arm/SetMem.S                                                 |  76 ------
>>>>  ArmPkg/Library/BaseMemoryLibVstm/Arm/SetMem.asm                                               |  78 ------
>>>>  ArmPkg/Library/BaseMemoryLibVstm/BaseMemoryLibVstm.inf                                        |  70 ------
>>>>  ArmPkg/Library/BaseMemoryLibVstm/CompareMemWrapper.c                                          |  66 -----
>>>>  ArmPkg/Library/BaseMemoryLibVstm/CopyMem.c                                                    |  62 -----
>>>>  ArmPkg/Library/BaseMemoryLibVstm/CopyMemWrapper.c                                             |  63 -----
>>>>  ArmPkg/Library/BaseMemoryLibVstm/MemLibGeneric.c                                              | 264 --------------------
>>>>  ArmPkg/Library/BaseMemoryLibVstm/MemLibGuid.c                                                 | 132 ----------
>>>>  ArmPkg/Library/BaseMemoryLibVstm/MemLibInternals.h                                            | 234 -----------------
>>>>  ArmPkg/Library/BaseMemoryLibVstm/ScanMem16Wrapper.c                                           |  67 -----
>>>>  ArmPkg/Library/BaseMemoryLibVstm/ScanMem32Wrapper.c                                           |  66 -----
>>>>  ArmPkg/Library/BaseMemoryLibVstm/ScanMem64Wrapper.c                                           |  67 -----
>>>>  ArmPkg/Library/BaseMemoryLibVstm/ScanMem8Wrapper.c                                            |  99 --------
>>>>  ArmPkg/Library/BaseMemoryLibVstm/SetMem.c                                                     |  53 ----
>>>>  ArmPkg/Library/BaseMemoryLibVstm/SetMem16Wrapper.c                                            |  64 -----
>>>>  ArmPkg/Library/BaseMemoryLibVstm/SetMem32Wrapper.c                                            |  64 -----
>>>>  ArmPkg/Library/BaseMemoryLibVstm/SetMem64Wrapper.c                                            |  64 -----
>>>>  ArmPkg/Library/BaseMemoryLibVstm/SetMemWrapper.c                                              |  91 -------
>>>>  26 files changed, 91 insertions(+), 1921 deletions(-)
>>>>  rename ArmPkg/Library/{BaseMemoryLibVstm/ZeroMemWrapper.c => BaseMemoryLibStm/IsZeroBufferWrapper.c} (53%)
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/Arm/CopyMem.S
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/Arm/CopyMem.asm
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/Arm/SetMem.S
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/Arm/SetMem.asm
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/BaseMemoryLibVstm.inf
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/CompareMemWrapper.c
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/CopyMem.c
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/CopyMemWrapper.c
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/MemLibGeneric.c
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/MemLibGuid.c
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/MemLibInternals.h
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/ScanMem16Wrapper.c
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/ScanMem32Wrapper.c
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/ScanMem64Wrapper.c
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/ScanMem8Wrapper.c
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/SetMem.c
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/SetMem16Wrapper.c
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/SetMem32Wrapper.c
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/SetMem64Wrapper.c
>>>>  delete mode 100644 ArmPkg/Library/BaseMemoryLibVstm/SetMemWrapper.c
>>>>
>>>> -- 
>>>> 2.7.4
>>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>>