Re: [edk2] [Patch 0/3] Update LZMA SDK version to the latest 18.05

[Ard Biesheuvel]

On 9 October 2018 at 08:23, Gao, Liming  wrote:
> Ard:
>   I verify GCC 5.5 X64. It can pass build. Which version of GCC can trig this 
> failure?
>

$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/aarch64-linux-gnu/6/lto-wrapper
Target: aarch64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian
6.3.0-18+deb9u1' --with-bugurl=file:///usr/share/doc/gcc-6/README.Bugs
--enable-languages=c,ada,c++,java,go,d,fortran,objc,obj-c++
--prefix=/usr --program-suffix=-6 --program-prefix=aarch64-linux-gnu-
--enable-shared --enable-linker-build-id --libexecdir=/usr/lib
--without-included-gettext --enable-threads=posix --libdir=/usr/lib
--enable-nls --with-sysroot=/ --enable-clocale=gnu
--enable-libstdcxx-debug --enable-libstdcxx-time=yes
--with-default-libstdcxx-abi=new --enable-gnu-unique-object
--disable-libquadmath --enable-plugin --enable-default-pie
--with-system-zlib --disable-browser-plugin --enable-java-awt=gtk
--enable-gtk-cairo
--with-java-home=/usr/lib/jvm/java-1.5.0-gcj-6-arm64/jre
--enable-java-home
--with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-6-arm64
--with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-6-arm64
--with-arch-directory=aarch64
--with-ecj-jar=/usr/share/java/eclipse-ecj.jar --enable-multiarch
--enable-fix-cortex-a53-843419 --enable-checking=release
--build=aarch64-linux-gnu --host=aarch64-linux-gnu
--target=aarch64-linux-gnu
Thread model: posix
gcc version 6.3.0 20170516 (Debian 6.3.0-18+deb9u1)

> Thanks
> Liming
>> -Original Message-
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: Tuesday, October 9, 2018 2:07 PM
>> To: Zeng, Star 
>> Cc: Gao, Liming ; edk2-devel@lists.01.org
>> Subject: Re: [edk2] [Patch 0/3] Update LZMA SDK version to the latest 18.05
>>
>> On 8 October 2018 at 03:36, Zeng, Star  wrote:
>> > Reviewed-by: Star Zeng 
>> >
>>
>> This change is breaking the build on GCC
>>
>> Sdk/C/LzmaEnc.c: In function ‘GetOptimum’:
>> Sdk/C/LzmaEnc.c:1467:9: error: this ‘for’ clause does not guard...
>> [-Werror=misleading-indentation]
>>  for (len = 3; len < limit && data[len] == data2[len]; len++);
>>
>>
>> > -Original Message-
>> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>> > Liming Gao
>> > Sent: Wednesday, August 29, 2018 8:51 AM
>> > To: edk2-devel@lists.01.org
>> > Subject: [edk2] [Patch 0/3] Update LZMA SDK version to the latest 18.05
>> >
>> > Liming Gao (3):
>> >   IntelFrameworkModulePkg Lzma: Update LZMA SDK version to 18.05
>> >   MdeModulePkg Lzma: Update LZMA SDK version to 18.05
>> >   BaseTools Lzma: Update LZMA SDK version to 18.05
>> >
>> >  .../Source/C/LzmaCompress/LZMA-SDK-README.txt  |4 +-
>> >  BaseTools/Source/C/LzmaCompress/LzmaCompress.c |8 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/7zFile.c |   28 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/7zFile.h |8 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/7zStream.c   |  111 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/7zTypes.h|  220 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/7zVersion.h  |   22 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/Alloc.c  |  399 +++-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/Alloc.h  |   20 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/Bra86.c  |4 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/Compiler.h   |3 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/CpuArch.h|  166 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzFind.c |  163 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzFind.h |   12 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzFindMt.c   |   67 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzFindMt.h   |6 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaDec.c|  401 ++--
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaDec.h|   47 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaEnc.c| 2334 
>> > 
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaEnc.h|   48 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/Threads.c|   14 +-
>> >  BaseTools/Source/C/LzmaCompress/Sdk/C/Threads.h|5 +-
>> >  .../Source/C/LzmaCompress/Sdk/DOC/lzma-history.txt |   63 +-
>> >  .../Source/C/LzmaCompress/Sdk/DOC/lzma-sdk.txt |2 +-
>> >  .../LzmaCustomDecompressLib/LZMA-SDK-README.txt|6 +-
>> >  .../LzmaArchCustomDecompressLib.inf|6 +-
>> >  .../LzmaCustomDecompressLib.inf|6 +-
>> >  .../LzmaCustomDecompressLib/LzmaDecompress.c   |4 +-
>> >  .../LzmaCustomDecompressLib/Sdk/C/7zTypes.h|  220 +-
>> >  .../LzmaCustomDecompressLib/Sdk/C/7zVersion.h  |   22 +-
>> >  .../Library/LzmaCustomDecompressLib/Sdk/C/Bra86.c  |4 +-
>> >  .../LzmaCustomDecompressLib/Sdk/C/Compiler.h   |3 +-
>> >  .../LzmaCustomDecompressLib/Sdk/C/CpuArch.h|  166 +-
>> >  .../Library/LzmaCustomDecompressLib/Sdk/C/LzFind.c |  163 +-
>> >  .../Library/LzmaCustomDecompressLib/Sdk

Re: [edk2] [Patch 0/3] Update LZMA SDK version to the latest 18.05

[Gao, Liming]

Ard:
  I verify GCC 5.5 X64. It can pass build. Which version of GCC can trig this 
failure?

Thanks
Liming
> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Tuesday, October 9, 2018 2:07 PM
> To: Zeng, Star 
> Cc: Gao, Liming ; edk2-devel@lists.01.org
> Subject: Re: [edk2] [Patch 0/3] Update LZMA SDK version to the latest 18.05
> 
> On 8 October 2018 at 03:36, Zeng, Star  wrote:
> > Reviewed-by: Star Zeng 
> >
> 
> This change is breaking the build on GCC
> 
> Sdk/C/LzmaEnc.c: In function ‘GetOptimum’:
> Sdk/C/LzmaEnc.c:1467:9: error: this ‘for’ clause does not guard...
> [-Werror=misleading-indentation]
>  for (len = 3; len < limit && data[len] == data2[len]; len++);
> 
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> > Liming Gao
> > Sent: Wednesday, August 29, 2018 8:51 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [Patch 0/3] Update LZMA SDK version to the latest 18.05
> >
> > Liming Gao (3):
> >   IntelFrameworkModulePkg Lzma: Update LZMA SDK version to 18.05
> >   MdeModulePkg Lzma: Update LZMA SDK version to 18.05
> >   BaseTools Lzma: Update LZMA SDK version to 18.05
> >
> >  .../Source/C/LzmaCompress/LZMA-SDK-README.txt  |4 +-
> >  BaseTools/Source/C/LzmaCompress/LzmaCompress.c |8 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/7zFile.c |   28 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/7zFile.h |8 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/7zStream.c   |  111 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/7zTypes.h|  220 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/7zVersion.h  |   22 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/Alloc.c  |  399 +++-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/Alloc.h  |   20 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/Bra86.c  |4 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/Compiler.h   |3 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/CpuArch.h|  166 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzFind.c |  163 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzFind.h |   12 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzFindMt.c   |   67 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzFindMt.h   |6 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaDec.c|  401 ++--
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaDec.h|   47 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaEnc.c| 2334 
> > 
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaEnc.h|   48 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/Threads.c|   14 +-
> >  BaseTools/Source/C/LzmaCompress/Sdk/C/Threads.h|5 +-
> >  .../Source/C/LzmaCompress/Sdk/DOC/lzma-history.txt |   63 +-
> >  .../Source/C/LzmaCompress/Sdk/DOC/lzma-sdk.txt |2 +-
> >  .../LzmaCustomDecompressLib/LZMA-SDK-README.txt|6 +-
> >  .../LzmaArchCustomDecompressLib.inf|6 +-
> >  .../LzmaCustomDecompressLib.inf|6 +-
> >  .../LzmaCustomDecompressLib/LzmaDecompress.c   |4 +-
> >  .../LzmaCustomDecompressLib/Sdk/C/7zTypes.h|  220 +-
> >  .../LzmaCustomDecompressLib/Sdk/C/7zVersion.h  |   22 +-
> >  .../Library/LzmaCustomDecompressLib/Sdk/C/Bra86.c  |4 +-
> >  .../LzmaCustomDecompressLib/Sdk/C/Compiler.h   |3 +-
> >  .../LzmaCustomDecompressLib/Sdk/C/CpuArch.h|  166 +-
> >  .../Library/LzmaCustomDecompressLib/Sdk/C/LzFind.c |  163 +-
> >  .../Library/LzmaCustomDecompressLib/Sdk/C/LzFind.h |   12 +-
> >  .../LzmaCustomDecompressLib/Sdk/C/LzmaDec.c|  401 ++--
> >  .../LzmaCustomDecompressLib/Sdk/C/LzmaDec.h|   47 +-
> >  .../Sdk/DOC/lzma-history.txt   |   63 +-
> >  .../LzmaCustomDecompressLib/Sdk/DOC/lzma-sdk.txt   |2 +-
> >  .../LzmaCustomDecompressLib/LZMA-SDK-README.txt|6 +-
> >  .../LzmaArchCustomDecompressLib.inf|6 +-
> >  .../LzmaCustomDecompressLib.inf|6 +-
> >  .../LzmaCustomDecompressLib/LzmaDecompress.c   |4 +-
> >  .../LzmaCustomDecompressLib/Sdk/C/7zTypes.h|  220 +-
> >  .../LzmaCustomDecompressLib/Sdk/C/7zVersion.h  |   22 +-
> >  .../Library/LzmaCustomDecompressLib/Sdk/C/Bra86.c  |4 +-
> >  .../LzmaCustomDecompressLib/Sdk/C/Compiler.h   |3 +-
> >  .../LzmaCustomDecompressLib/Sdk/C/CpuArch.h|  166 +-
> >  .../Library/LzmaCustomDecompressLib/Sdk/C/LzFind.c |  163 +-
> >  .../Library/LzmaCustomDecompressLib/Sdk/C/LzFind.h |   12 +-
> >  .../LzmaCustomDecompressLib/Sdk/C/LzmaDec.c|  401 ++--
> >  .../LzmaCustomDecompressLib/Sdk/C/LzmaDec.h|   47 +-
> >  .../Sdk/DOC/lzma-history.txt   |   63 +-
> >  .../LzmaCustomDecompressLib/Sdk/DOC/lzma-sdk.txt   |2 +-
> >  54 files changed, 4230 insertions(+), 2175 deletions(-)
> >
> > --
> > 2.10.0.windows.1
> >
> > ___
> > edk2-devel mailing list
> > e

Re: [edk2] [Patch 0/3] Update LZMA SDK version to the latest 18.05

[Ard Biesheuvel]

On 8 October 2018 at 03:36, Zeng, Star  wrote:
> Reviewed-by: Star Zeng 
>

This change is breaking the build on GCC

Sdk/C/LzmaEnc.c: In function ‘GetOptimum’:
Sdk/C/LzmaEnc.c:1467:9: error: this ‘for’ clause does not guard...
[-Werror=misleading-indentation]
 for (len = 3; len < limit && data[len] == data2[len]; len++);


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Liming 
> Gao
> Sent: Wednesday, August 29, 2018 8:51 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [Patch 0/3] Update LZMA SDK version to the latest 18.05
>
> Liming Gao (3):
>   IntelFrameworkModulePkg Lzma: Update LZMA SDK version to 18.05
>   MdeModulePkg Lzma: Update LZMA SDK version to 18.05
>   BaseTools Lzma: Update LZMA SDK version to 18.05
>
>  .../Source/C/LzmaCompress/LZMA-SDK-README.txt  |4 +-
>  BaseTools/Source/C/LzmaCompress/LzmaCompress.c |8 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/7zFile.c |   28 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/7zFile.h |8 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/7zStream.c   |  111 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/7zTypes.h|  220 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/7zVersion.h  |   22 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/Alloc.c  |  399 +++-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/Alloc.h  |   20 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/Bra86.c  |4 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/Compiler.h   |3 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/CpuArch.h|  166 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/LzFind.c |  163 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/LzFind.h |   12 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/LzFindMt.c   |   67 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/LzFindMt.h   |6 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaDec.c|  401 ++--
>  BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaDec.h|   47 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaEnc.c| 2334 
> 
>  BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaEnc.h|   48 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/Threads.c|   14 +-
>  BaseTools/Source/C/LzmaCompress/Sdk/C/Threads.h|5 +-
>  .../Source/C/LzmaCompress/Sdk/DOC/lzma-history.txt |   63 +-
>  .../Source/C/LzmaCompress/Sdk/DOC/lzma-sdk.txt |2 +-
>  .../LzmaCustomDecompressLib/LZMA-SDK-README.txt|6 +-
>  .../LzmaArchCustomDecompressLib.inf|6 +-
>  .../LzmaCustomDecompressLib.inf|6 +-
>  .../LzmaCustomDecompressLib/LzmaDecompress.c   |4 +-
>  .../LzmaCustomDecompressLib/Sdk/C/7zTypes.h|  220 +-
>  .../LzmaCustomDecompressLib/Sdk/C/7zVersion.h  |   22 +-
>  .../Library/LzmaCustomDecompressLib/Sdk/C/Bra86.c  |4 +-
>  .../LzmaCustomDecompressLib/Sdk/C/Compiler.h   |3 +-
>  .../LzmaCustomDecompressLib/Sdk/C/CpuArch.h|  166 +-
>  .../Library/LzmaCustomDecompressLib/Sdk/C/LzFind.c |  163 +-
>  .../Library/LzmaCustomDecompressLib/Sdk/C/LzFind.h |   12 +-
>  .../LzmaCustomDecompressLib/Sdk/C/LzmaDec.c|  401 ++--
>  .../LzmaCustomDecompressLib/Sdk/C/LzmaDec.h|   47 +-
>  .../Sdk/DOC/lzma-history.txt   |   63 +-
>  .../LzmaCustomDecompressLib/Sdk/DOC/lzma-sdk.txt   |2 +-
>  .../LzmaCustomDecompressLib/LZMA-SDK-README.txt|6 +-
>  .../LzmaArchCustomDecompressLib.inf|6 +-
>  .../LzmaCustomDecompressLib.inf|6 +-
>  .../LzmaCustomDecompressLib/LzmaDecompress.c   |4 +-
>  .../LzmaCustomDecompressLib/Sdk/C/7zTypes.h|  220 +-
>  .../LzmaCustomDecompressLib/Sdk/C/7zVersion.h  |   22 +-
>  .../Library/LzmaCustomDecompressLib/Sdk/C/Bra86.c  |4 +-
>  .../LzmaCustomDecompressLib/Sdk/C/Compiler.h   |3 +-
>  .../LzmaCustomDecompressLib/Sdk/C/CpuArch.h|  166 +-
>  .../Library/LzmaCustomDecompressLib/Sdk/C/LzFind.c |  163 +-
>  .../Library/LzmaCustomDecompressLib/Sdk/C/LzFind.h |   12 +-
>  .../LzmaCustomDecompressLib/Sdk/C/LzmaDec.c|  401 ++--
>  .../LzmaCustomDecompressLib/Sdk/C/LzmaDec.h|   47 +-
>  .../Sdk/DOC/lzma-history.txt   |   63 +-
>  .../LzmaCustomDecompressLib/Sdk/DOC/lzma-sdk.txt   |2 +-
>  54 files changed, 4230 insertions(+), 2175 deletions(-)
>
> --
> 2.10.0.windows.1
>
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE

[Sumit Garg]

On Mon, 8 Oct 2018 at 19:35, Leif Lindholm  wrote:
>
> On Mon, Oct 08, 2018 at 03:20:27PM +0530, Sumit Garg wrote:
> > On Fri, 5 Oct 2018 at 20:33, Leif Lindholm  wrote:
> > >
> > > On Wed, Oct 03, 2018 at 11:33:01AM +0200, Ard Biesheuvel wrote:
> > > > On 3 October 2018 at 09:43, Sumit Garg  wrote:
> > > > > Add following APIs to communicate with OP-TEE pseudo/early TAs:
> > > > > 1. OpteeInit
> > > > > 2. OpteeOpenSession
> > > > > 3. OpteeCloseSession
> > > > > 4. OpteeInvokeFunc
> > > > >
> > > > > Cc: Ard Biesheuvel 
> > > > > Cc: Leif Lindholm 
> > > > > Cc: Michael D Kinney 
> > > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > > Signed-off-by: Sumit Garg 
> > > >
> > > > Given the outcome of the GP discussion, I'm fine with this approach. 
> > > > Leif?
> > >
> > > Apologies for the delay, I needed some time to think it over.
> > >
> > > I'm not super happy about this approach, but I'm happier with this
> > > than I would be with leaving the functionality out of the upstream
> > > tree. I really hope we can get that license either changed or dropped
> > > completely.
> > >
> >
> > Thanks Leif for this go ahead.
> >
> > > I do have a few comments below.
> > >
> > > > > ---
> > > > >  ArmPkg/Include/Library/OpteeLib.h|  90 +
> > > > >  ArmPkg/Library/OpteeLib/Optee.c  | 357 
> > > > > +++
> > > > >  ArmPkg/Library/OpteeLib/OpteeLib.inf |   2 +
> > > > >  ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +
> > >
> > > Could you follow the instructions in
> > > https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
> > > when generating future patches?
> > > The --stat* effects aren't apparent here, but the -O ones are.
> > >
> >
> > Sure.
> >
> > > > >  4 files changed, 492 insertions(+)
> > > > >  create mode 100644 ArmPkg/Library/OpteeLib/OpteeSmc.h
> > > > >
> > > > > diff --git a/ArmPkg/Include/Library/OpteeLib.h 
> > > > > b/ArmPkg/Include/Library/OpteeLib.h
> > > > > index f65d8674d9b8..2d1c60632dfe 100644
> > > > > --- a/ArmPkg/Include/Library/OpteeLib.h
> > > > > +++ b/ArmPkg/Include/Library/OpteeLib.h
> > > > > @@ -25,10 +25,100 @@
> > > > >  #define OPTEE_OS_UID2  0xaf630002
> > > > >  #define OPTEE_OS_UID3  0xa5d5c51b
> > > > >
> > > > > +#define OPTEE_MSG_ATTR_TYPE_NONE0x0
> > > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INPUT 0x1
> > > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT0x2
> > > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INOUT 0x3
> > > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_INPUT   0x9
> > > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT  0xa
> > > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_INOUT   0xb
> > > > > +
> > > > > +#define OPTEE_MSG_ATTR_TYPE_MASK0xff
> > > > > +
> > > > > +#define OPTEE_ORIGIN_COMMS  0x0002
> > > > > +#define OPTEE_ERROR_COMMS   0x000E
> > > > > +
> > > > > +typedef struct {
> > > > > +  UINT64BufPtr;
> > >
> > > If it's a pointer, it has a *.
> > > Otherwise it's an address.
> > >
> >
> > Will rather use BufferAddress.
> >
> > > > > +  UINT64Size;
> > > > > +  UINT64ShmRef;
> > >
> > > Abbreviations in function, variable or type names (other than the ones
> > > defined in [1] are not permitted unless they are explicitly added to a
> > > glossary section of the source file header. Where possible, just write
> > > out the name in full.
> > >
> > > [1] 
> > > https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/v/release/2.20/4_naming_conventions/#table-2-efi-supported-abbreviations
> > >
> > > BufPtr (as a name) gets a pass since it's unambiguous and we already
> > > have a bunch of those in the codebase. ShmRef needs to be clear.
> > >
> >
> > Will replace it with SharedMemoryReference.
> >
> > > (This comment also applies to a lot of things below, I won't point
> > > them all out unless asked to.)
> > >
> >
> > I will try to replace most of them with full names. But I have
> > confusion regarding usage of following not included in [1]:
> >
> > 1. MSG/Msg
> > 2. MEM/Mem
> > 3. INFO/Info
> > 4. FUNC/Func
> > 5. RET/Ret
> > 6. ATTR/Attr
> > 7. CMD/Cmd
> >
> > But I do see their usage in the codebase. Please help to clarify.
>
> It's a horrible secret, but not all of the codebase conforms to the
> coding style. We're continuously trying to improve it.
>
> > Also can I use 1-2 letter variable or struct member names like Mp, Ip,
> > Op, U etc.?
>
> Ideally not. I'm not a huge fan of ReallyLongCamelCase coding styles,
> but the benefit it does bring is that you don't need to stop and think
> about what a variable may be for. And correspondingly, mixing between
> abbreviations an non-abbreviations make for hard reading.
>
> Another completely unscientific observation I would make is that once
> you force yourself to write a full variable name out, it becomes a

[edk2] [Patch v3] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.

[Eric Dong]

V3 changes:
No need to change inf file. Also update commit message to include regression 
info.

V2 changes:
Only disable paging in 32 bit mode, no matter it is enable or not.

V1 changes:
PEI Stack Guard needs to enable paging. This might cause #GP if code
trying to write CR3 register with PML4 page table while the processor
is enabled with PAE paging.

Simply disabling paging before updating CR3 can solve this conflict.

It's an regression caused by change: 0a0d5296e448fc350de1594c49b9c0deff7fad60

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1232

Cc: Ruiyu Ni 
Cc: Laszlo Ersek 
Cc: Jian J Wang 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by:Eric Dong 
---
 UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c 
b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
index f164c1713b..53ed76c6e6 100644
--- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
+++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
@@ -1105,6 +1105,14 @@ S3RestoreConfig2 (
   //
   SetInterruptState (InterruptStatus);
 
+  if (sizeof(UINTN) == sizeof(UINT32)) {
+//
+// Paging maybe enabled. If current mode is 32 bit mode and code try to
+// enable 64 bit mode page table, it will cause GP fault.
+// To avoid conflict configuration, disable paging first anyway.
+//
+AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
+  }
   AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
 
   //
-- 
2.15.0.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment variable

[Ni, Ruiyu]

On 10/5/2018 8:19 PM, Tim Lewis wrote:

Jaben --

Following on this: shouldn't this be a spec issue? If you are asking people
to depend on the behavior.


I agree. So I suggest submit a Spec ECR for this change.
Since it was checked in, we can revert it if the ECR is rejected.



Thanks,
Tim

-Original Message-
From: edk2-devel  On Behalf Of Laszlo Ersek
Sent: Friday, October 5, 2018 4:33 AM
To: Carsey, Jaben ; Andrew Fish 
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment
variable

On 10/04/18 22:54, Carsey, Jaben wrote:

Laszlo,

The leading "_" was required for out of spec, but built in, commands.  The

spec has no restrictions on environment variables except some have special
meaning and may be read only.


I can certainly work on slowing down the process.  I have been complaining

about that same thing and should have been more aware.  I would like to have
a community minimum amount of time before commits are done that we all agree
to.  Something like 1 full day would be nice I think.

Good idea! I believe 24 hours should be tolerable on all ends. It also gives
a chance to people in other time zones to comment.

I think there should be one exception: grave regressions -- build failures,
or total boot failures -- should be possible to revert (or
fix) as soon as there's one review.

Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel




--
Thanks,
Ray
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] BaseTools/edksetup.sh: Handle the return value from grep

[Zhu, Yonghong]

Looks good to me. I will push this to Python3 branch.
Reviewed-by: Yonghong Zhu  

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Gary Lin
Sent: Tuesday, October 2, 2018 11:36 AM
To: edk2-devel@lists.01.org
Cc: Gao, Liming 
Subject: [edk2] [PATCH] BaseTools/edksetup.sh: Handle the return value from grep

When SetupPython3() parses the possible python binary paths, it uses `grep 
"[[:digit:]]$"` to filter the python versions in the file name.
Since grep would return 1 when the last character of the file name is not a 
digit, OvmfPkg/build.sh would fail immediately when the function is handling 
some cases, e.g. "python3:".

This commit adds `|| true` to the grep command to make sure the command never 
returns 1 in any condition.

[NOTE: For the python3 branch]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Gary Lin 
Cc: Liming Gao 
Cc: Yonghong Zhu 
---
 edksetup.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/edksetup.sh b/edksetup.sh
index 5ff3be19fb2f..d4e577e60781 100755
--- a/edksetup.sh
+++ b/edksetup.sh
@@ -115,7 +115,7 @@ function SetupPython3()  {
   for python in $(whereis python3)
   do
-python=$(echo $python | grep "[[:digit:]]$")
+python=$(echo $python | grep "[[:digit:]]$" || true)
 python_version=${python##*python}
 if [ -z "${python_version}" ];then
   continue
--
2.18.0

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH V2] IntelFsp2Pkg/GenCfgOpt.py: Support PCD input from command line

[Zhu, Yonghong]

Reviewed-by: Yonghong Zhu  

Best Regards,
Zhu Yonghong

-Original Message-
From: Chiu, Chasel 
Sent: Monday, October 08, 2018 4:11 PM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Gao, Liming ; 
Zhu, Yonghong ; Chiu, Chasel 
Subject: [PATCH V2] IntelFsp2Pkg/GenCfgOpt.py: Support PCD input from command 
line

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

Build system already support override PCD value by command line so add this 
support to GenCfgOpt.py Also update revision to 0.53

Test: Verified UPD header files generated can reflect different
  PCD values from --pcd build command input

Cc: Jiewen Yao 
Cc: Gao Liming 
Cc: Zhu Yonghong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chasel Chiu 
---
 IntelFsp2Pkg/Tools/GenCfgOpt.py | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py b/IntelFsp2Pkg/Tools/GenCfgOpt.py 
index 059cfcb7e4..15d33582ef 100644
--- a/IntelFsp2Pkg/Tools/GenCfgOpt.py
+++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py
@@ -88,6 +88,8 @@ are permitted provided that the following conditions are met:
 **/
 """
 
+BuildOptionPcd = []
+
 class CLogicalExpression:
 def __init__(self):
 self.index= 0
@@ -561,6 +563,12 @@ EndList
 self._PcdsDict[Match.group(1)] = Match.group(2)
 if self.Debug:
 print "INFO : PCD %s = [ %s ]" % (Match.group(1), 
Match.group(2))
+i = 0
+while i < len(BuildOptionPcd):
+Match = re.match("\s*([\w\.]+)\s*\=\s*(\w+)", 
BuildOptionPcd[i])
+if Match:
+self._PcdsDict[Match.group(1)] = Match.group(2)
+i += 1
 else:
 Match = re.match("^\s*#\s+(!BSF|@Bsf|!HDR)\s+(.+)", DscLine)
 if Match:
@@ -1462,7 +1470,7 @@ EndList
 
 
 def Usage():
-print "GenCfgOpt Version 0.52"
+print "GenCfgOpt Version 0.53"
 print "Usage:"
 print "GenCfgOpt  UPDTXT  PlatformDscFile BuildFvDir 
[-D Macros]"
 print "GenCfgOpt  HEADER  PlatformDscFile BuildFvDir  InputHFile 
[-D Macros]"
@@ -1472,7 +1480,14 @@ def Main():
 #
 # Parse the options and args
 #
+i = 1
+
 GenCfgOpt = CGenCfgOpt()
+while i < len(sys.argv):
+if sys.argv[i].strip().lower() == "--pcd":
+BuildOptionPcd.append(sys.argv[i+1])
+i += 1
+i += 1
 argc = len(sys.argv)
 if argc < 4:
 Usage()
--
2.13.3.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."

[Ni, Ruiyu]

On 10/8/2018 9:23 PM, jim.dai...@dell.com wrote:

-Original Message-


diff --git a/MdePkg/Library/BaseLib/FilePaths.c 
b/MdePkg/Library/BaseLib/FilePaths.c
index d6f3758ecb..5d3de01894 100644
--- a/MdePkg/Library/BaseLib/FilePaths.c
+++ b/MdePkg/Library/BaseLib/FilePaths.c
@@ -2,6 +2,7 @@
 Defines file-path manipulation functions.
   
 Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.

+  Copyright (c) 2018, Dell Technologies. All rights reserved.
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD 
License
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -103,7 +104,9 @@ PathCleanUpDirectories(
   ) {
   *(TempString + 1) = CHAR_NULL;
   PathRemoveLastItem(Path);
-CopyMem (Path + StrLen (Path), TempString + 3, StrSize (TempString + 3));
+if (*(TempString + 3)) {
+  CopyMem (Path + StrLen (Path), TempString + 4, StrSize (TempString + 4));
+}
I just setup a debugging environment to trace the code. Finally I 
understand the issue.
I agree to change "TempString + 3" to "TempString + 4". This can 
eliminate double slash so in next same loop, the pattern "\\..\\" or 
"\\..\0" can be detected and "last item" can be correctly removed by 
PathRemoveLastItem().


Can you change
 "if (*(TempString + 3))"
 to
 "if (*(TempString + 3) != CHAR_NULL)"?

With the above change, Reviewed-by: Ruiyu Ni 
If you agree, I can modify your patch and push it for you.


 }
   
 //



Jim,
Are you fixing a corner case bug introduced by following commit:


SHA-1: bb99e3282c9e69fbd6365d117c58d15589e34c5d

* MdePkg/BaseLib: Fix PathCleanUpDirectories to correctly handle "\.\"


The old code incorrectly cleans path like "fs0:\abc\.\.." to
"fs0:\abc", instead of "fs0:\"



The patch fixes this bug.


Honestly, because of the scope of bb99e328, I cannot say for sure, but
my inclination is to answer "No."

The current fix is strictly related to handling paths that include two
or more references to the previous directory, whether or not one of them
abuts a reference to the current directory (as in the bb99e328 fix).

Regards,
Jim




--
Thanks,
Ray
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.

[Dong, Eric]

Hi Jian,

Not aware this comments before I send out V2 patch, will include this info in 
commit message when I check in the code.

Thanks,
Eric
> -Original Message-
> From: Wang, Jian J
> Sent: Tuesday, October 9, 2018 10:03 AM
> To: edk2-devel ; Dong, Eric
> ; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu ; Laszlo Ersek 
> Subject: RE: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
> creating new page table.
> 
> Forgot one thing: this issue is caused by introducing of PEI stack guard so
> please add the revision id of related check in for references in message.
> 
> Regards,
> Jian
> 
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
> > Sent: Tuesday, October 09, 2018 9:59 AM
> > To: Dong, Eric ; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu ; Laszlo Ersek 
> > Subject: Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging
> > before creating new page table.
> >
> > Thanks for fixing this issue.
> >
> > Reviewed-by: Jian J Wang 
> >
> > > -Original Message-
> > > From: Dong, Eric
> > > Sent: Tuesday, October 09, 2018 9:51 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Ni, Ruiyu ; Laszlo Ersek
> > > ; Wang, Jian J 
> > > Subject: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
> > > creating
> > new
> > > page table.
> > >
> > > PEI Stack Guard needs to enable paging. This might cause #GP in the
> > > transition from 32-bit PEI to 64-bit SMM due to the code trying to
> > > write CR3 register with PML4 page table while the processor is
> > > enabled with PAE paging.
> > >
> > > Simply disabling paging before updating CR3 can solve this conflict.
> > >
> > > Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
> > > Cc: Ruiyu Ni 
> > > Cc: Laszlo Ersek 
> > > Cc: Jian J Wang 
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by:Eric Dong 
> > > Signed-off-by: Eric Dong 
> > > ---
> > >  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c   | 7 +++
> > >  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 1 +
> > >  2 files changed, 8 insertions(+)
> > >
> > > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > > b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > > index f164c1713b..b3bf56e13d 100644
> > > --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > > +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > > @@ -1105,6 +1105,13 @@ S3RestoreConfig2 (
> > >//
> > >SetInterruptState (InterruptStatus);
> > >
> > > +  if (PcdGetBool (PcdCpuStackGuard)) {
> > > +//
> > > +// Paging already been enabled, to avoid conflict configuration,
> > > +// disable paging first anyway.
> > > +//
> > > +AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
> > > +  }
> > >AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
> > >
> > >//
> > > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > > b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > > index 6ce1bf944c..0f131d19df 100644
> > > --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > > +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > > @@ -90,6 +90,7 @@
> > >  [Pcd]
> > >gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable  ##
> > > SOMETIMES_CONSUMES
> > >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
> ask
> > > ## CONSUMES
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard   
> > > ##
> > > CONSUMES
> > >
> > >  [Depex]
> > >TRUE
> > > --
> > > 2.15.0.windows.1
> >
> > ___
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch v2] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.

[Eric Dong]

V2 changes:
Only disable paging in 32 bit mode, no matter it is enable or not.

V1 changes:
PEI Stack Guard needs to enable paging. This might cause #GP in the
transition from 32-bit PEI to 64-bit SMM due to the code trying to
write CR3 register with PML4 page table while the processor is enabled
with PAE paging.

Simply disabling paging before updating CR3 can solve this conflict.

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1232

Cc: Ruiyu Ni 
Cc: Laszlo Ersek 
Cc: Jian J Wang 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by:Eric Dong 
Signed-off-by: Eric Dong 
---
 UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c   | 8 
 UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 1 +
 2 files changed, 9 insertions(+)

diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c 
b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
index f164c1713b..53ed76c6e6 100644
--- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
+++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
@@ -1105,6 +1105,14 @@ S3RestoreConfig2 (
   //
   SetInterruptState (InterruptStatus);
 
+  if (sizeof(UINTN) == sizeof(UINT32)) {
+//
+// Paging maybe enabled. If current mode is 32 bit mode and code try to
+// enable 64 bit mode page table, it will cause GP fault.
+// To avoid conflict configuration, disable paging first anyway.
+//
+AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
+  }
   AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
 
   //
diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf 
b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
index 6ce1bf944c..0f131d19df 100644
--- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
+++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
@@ -90,6 +90,7 @@
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable  ## SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard   ## 
CONSUMES
 
 [Depex]
   TRUE
-- 
2.15.0.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.

[Ni, Ruiyu]

On 10/9/2018 10:05 AM, Dong, Eric wrote:

Add BZ link for this issue: https://bugzilla.tianocore.org/show_bug.cgi?id=1232

Thanks,
Eric


-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
Eric Dong
Sent: Tuesday, October 9, 2018 9:51 AM
To: edk2-devel@lists.01.org
Cc: Ni, Ruiyu ; Laszlo Ersek 
Subject: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
creating new page table.

PEI Stack Guard needs to enable paging. This might cause #GP in the
transition from 32-bit PEI to 64-bit SMM due to the code trying to write CR3
register with PML4 page table while the processor is enabled with PAE paging.

Simply disabling paging before updating CR3 can solve this conflict.

Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
Cc: Ruiyu Ni 
Cc: Laszlo Ersek 
Cc: Jian J Wang 
Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-
by:Eric Dong 
Signed-off-by: Eric Dong 
---
  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c   | 7 +++
  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 1 +
  2 files changed, 8 insertions(+)

diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
index f164c1713b..b3bf56e13d 100644
--- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
+++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
@@ -1105,6 +1105,13 @@ S3RestoreConfig2 (
//
SetInterruptState (InterruptStatus);

+  if (PcdGetBool (PcdCpuStackGuard)) {
+//
+// Paging already been enabled, to avoid conflict configuration,
+// disable paging first anyway.
+//
+AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
+  }


Two comments:
1. We'd better not map the PcdCpuStackGuard to paging-enable. Maybe some 
other feature also enables the paging in PEI phase but the 
PcdCpuStackGuard is FALSE.
2. When PEI is in 64bit mode, disabling paging may not work because 
paging-enable is a must in 64bit mode.



AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);

//
diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
index 6ce1bf944c..0f131d19df 100644
--- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
+++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
@@ -90,6 +90,7 @@
  [Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable  ##
SOMETIMES_CONSUMES

gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
ask## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard   ##
CONSUMES

  [Depex]
TRUE
--
2.15.0.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel



--
Thanks,
Ray
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [patch] MdeModulePkg/HiiDB: Fix incorrect structure convention for checkbox

[Bi, Dandan]

> -Original Message-
> From: Ni, Ruiyu
> Sent: Tuesday, October 9, 2018 10:07 AM
> To: Laszlo Ersek ; Bi, Dandan ;
> edk2-devel@lists.01.org
> Cc: Gao, Liming ; Zeng, Star 
> Subject: Re: [edk2] [patch] MdeModulePkg/HiiDB: Fix incorrect structure
> convention for checkbox
> 
> On 10/8/2018 11:15 PM, Laszlo Ersek wrote:
> > On 10/08/18 16:32, Bi, Dandan wrote:
> >>>
> >>> what were the practical consequences (symptoms) of this issue? Did
> >>> some checkboxes not work? (I'm asking because SecureBootConfigDxe
> >>> uses some
> >>> checkboxes.)
> >>
> >> 1.  The bug is in function "UpdateDefaultSettingInFormPackage()" which is
> to update the default setting of some HII Questions in the IFR binary data. So
> it only has impact when platform overrides default setting in HII VarStore
> through DynamicHii PCD setting in Platform DSC file. If platform doesn't
> override default setting, it has no impact.
> >>
> >> 2. The implementation updates the "Flags" filed in the EFI_IFR_CHECKBOX
> structure to update the default setting of checkbox.
> >> If using "IfrCheckBox  = (EFI_IFR_CHECKBOX *) (IfrOpHdr + 1);" when
> wants to update the " Flags" filed in checkbox, but in fact it will update the
> opcode binary data(opcode binary length) behind checkbox binary.
> >> And then it will cause Browser can't parse the IFR binary data correctly.
> And then the possible symptom is that some HII Question and forms may be
> not parsed and then cannot be shown.
> >
> > Thanks! I've copied this into the BZ.
> 
> Has this patch been pushed? If not, maybe you could also copy the above
> description in the commit message.
> A commit message that describes what to be fixed is more meaningful.

Not yet. And I will copy the description in the commit message before push the 
patch.
> 
> >
> > Laszlo
> > ___
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >
> 
> 
> --
> Thanks,
> Ray
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.

[Dong, Eric]

Add BZ link for this issue: https://bugzilla.tianocore.org/show_bug.cgi?id=1232

Thanks,
Eric

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Eric Dong
> Sent: Tuesday, October 9, 2018 9:51 AM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu ; Laszlo Ersek 
> Subject: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
> creating new page table.
> 
> PEI Stack Guard needs to enable paging. This might cause #GP in the
> transition from 32-bit PEI to 64-bit SMM due to the code trying to write CR3
> register with PML4 page table while the processor is enabled with PAE paging.
> 
> Simply disabling paging before updating CR3 can solve this conflict.
> 
> Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
> Cc: Ruiyu Ni 
> Cc: Laszlo Ersek 
> Cc: Jian J Wang 
> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-
> by:Eric Dong 
> Signed-off-by: Eric Dong 
> ---
>  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c   | 7 +++
>  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> index f164c1713b..b3bf56e13d 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> @@ -1105,6 +1105,13 @@ S3RestoreConfig2 (
>//
>SetInterruptState (InterruptStatus);
> 
> +  if (PcdGetBool (PcdCpuStackGuard)) {
> +//
> +// Paging already been enabled, to avoid conflict configuration,
> +// disable paging first anyway.
> +//
> +AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
> +  }
>AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
> 
>//
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> index 6ce1bf944c..0f131d19df 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> @@ -90,6 +90,7 @@
>  [Pcd]
>gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable  ##
> SOMETIMES_CONSUMES
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
> ask## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard   ##
> CONSUMES
> 
>  [Depex]
>TRUE
> --
> 2.15.0.windows.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [patch] MdeModulePkg/HiiDB: Fix incorrect structure convention for checkbox

[Ni, Ruiyu]

On 10/8/2018 11:15 PM, Laszlo Ersek wrote:

On 10/08/18 16:32, Bi, Dandan wrote:


what were the practical consequences (symptoms) of this issue? Did some
checkboxes not work? (I'm asking because SecureBootConfigDxe uses some
checkboxes.)


1.  The bug is in function "UpdateDefaultSettingInFormPackage()" which is to 
update the default setting of some HII Questions in the IFR binary data. So it only has 
impact when platform overrides default setting in HII VarStore through DynamicHii PCD 
setting in Platform DSC file. If platform doesn't override default setting, it has no 
impact.

2. The implementation updates the "Flags" filed in the EFI_IFR_CHECKBOX 
structure to update the default setting of checkbox.
If using "IfrCheckBox  = (EFI_IFR_CHECKBOX *) (IfrOpHdr + 1);" when wants to update 
the " Flags" filed in checkbox, but in fact it will update the opcode binary data(opcode 
binary length) behind checkbox binary.
And then it will cause Browser can't parse the IFR binary data correctly. And 
then the possible symptom is that some HII Question and forms may be not parsed 
and then cannot be shown.


Thanks! I've copied this into the BZ.


Has this patch been pushed? If not, maybe you could also copy the above 
description in the commit message.

A commit message that describes what to be fixed is more meaningful.



Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel




--
Thanks,
Ray
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.

[Wang, Jian J]

Forgot one thing: this issue is caused by introducing of PEI stack guard so 
please
add the revision id of related check in for references in message.

Regards,
Jian


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
> Sent: Tuesday, October 09, 2018 9:59 AM
> To: Dong, Eric ; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu ; Laszlo Ersek 
> Subject: Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
> creating new page table.
> 
> Thanks for fixing this issue.
> 
> Reviewed-by: Jian J Wang 
> 
> > -Original Message-
> > From: Dong, Eric
> > Sent: Tuesday, October 09, 2018 9:51 AM
> > To: edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu ; Laszlo Ersek ; Wang,
> > Jian J 
> > Subject: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating
> new
> > page table.
> >
> > PEI Stack Guard needs to enable paging. This might cause #GP in the
> > transition from 32-bit PEI to 64-bit SMM due to the code trying to
> > write CR3 register with PML4 page table while the processor is enabled
> > with PAE paging.
> >
> > Simply disabling paging before updating CR3 can solve this conflict.
> >
> > Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
> > Cc: Ruiyu Ni 
> > Cc: Laszlo Ersek 
> > Cc: Jian J Wang 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by:Eric Dong 
> > Signed-off-by: Eric Dong 
> > ---
> >  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c   | 7 +++
> >  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 1 +
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > index f164c1713b..b3bf56e13d 100644
> > --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > @@ -1105,6 +1105,13 @@ S3RestoreConfig2 (
> >//
> >SetInterruptState (InterruptStatus);
> >
> > +  if (PcdGetBool (PcdCpuStackGuard)) {
> > +//
> > +// Paging already been enabled, to avoid conflict configuration,
> > +// disable paging first anyway.
> > +//
> > +AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
> > +  }
> >AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
> >
> >//
> > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > index 6ce1bf944c..0f131d19df 100644
> > --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > @@ -90,6 +90,7 @@
> >  [Pcd]
> >gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable  ##
> > SOMETIMES_CONSUMES
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> > ## CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard   ##
> > CONSUMES
> >
> >  [Depex]
> >TRUE
> > --
> > 2.15.0.windows.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.

[Wang, Jian J]

Thanks for fixing this issue.

Reviewed-by: Jian J Wang 

> -Original Message-
> From: Dong, Eric
> Sent: Tuesday, October 09, 2018 9:51 AM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu ; Laszlo Ersek ; Wang,
> Jian J 
> Subject: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new
> page table.
> 
> PEI Stack Guard needs to enable paging. This might cause #GP in the
> transition from 32-bit PEI to 64-bit SMM due to the code trying to
> write CR3 register with PML4 page table while the processor is enabled
> with PAE paging.
> 
> Simply disabling paging before updating CR3 can solve this conflict.
> 
> Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
> Cc: Ruiyu Ni 
> Cc: Laszlo Ersek 
> Cc: Jian J Wang 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by:Eric Dong 
> Signed-off-by: Eric Dong 
> ---
>  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c   | 7 +++
>  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> index f164c1713b..b3bf56e13d 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> @@ -1105,6 +1105,13 @@ S3RestoreConfig2 (
>//
>SetInterruptState (InterruptStatus);
> 
> +  if (PcdGetBool (PcdCpuStackGuard)) {
> +//
> +// Paging already been enabled, to avoid conflict configuration,
> +// disable paging first anyway.
> +//
> +AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
> +  }
>AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
> 
>//
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> index 6ce1bf944c..0f131d19df 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> @@ -90,6 +90,7 @@
>  [Pcd]
>gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable  ##
> SOMETIMES_CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard   ##
> CONSUMES
> 
>  [Depex]
>TRUE
> --
> 2.15.0.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.

[Eric Dong]

PEI Stack Guard needs to enable paging. This might cause #GP in the
transition from 32-bit PEI to 64-bit SMM due to the code trying to
write CR3 register with PML4 page table while the processor is enabled
with PAE paging.

Simply disabling paging before updating CR3 can solve this conflict.

Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
Cc: Ruiyu Ni 
Cc: Laszlo Ersek 
Cc: Jian J Wang 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by:Eric Dong 
Signed-off-by: Eric Dong 
---
 UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c   | 7 +++
 UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 1 +
 2 files changed, 8 insertions(+)

diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c 
b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
index f164c1713b..b3bf56e13d 100644
--- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
+++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
@@ -1105,6 +1105,13 @@ S3RestoreConfig2 (
   //
   SetInterruptState (InterruptStatus);
 
+  if (PcdGetBool (PcdCpuStackGuard)) {
+//
+// Paging already been enabled, to avoid conflict configuration,
+// disable paging first anyway.
+//
+AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
+  }
   AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
 
   //
diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf 
b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
index 6ce1bf944c..0f131d19df 100644
--- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
+++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
@@ -90,6 +90,7 @@
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable  ## SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard   ## 
CONSUMES
 
 [Depex]
   TRUE
-- 
2.15.0.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 0/2] Update BrotliCompress to the latest version 1.0.5

[Zeng, Star]

Reviewed-by: Star Zeng 

-Original Message-
From: Gao, Liming 
Sent: Monday, October 8, 2018 10:52 PM
To: Zeng, Star ; edk2-devel@lists.01.org
Subject: RE: [edk2] [Patch 0/2] Update BrotliCompress to the latest version 
1.0.5

Star:
  Thank you for review. I just find Brotli 1.0.6 has been released. Based on 
v1.0.5, there is the minor change in source file. They have no functionality 
impact. I have pushed the additional patches in 
https://github.com/lgao4/edk2/tree/Brotli to integrate 1.0.6 change. Could you 
help review them? After review, I will combine the patches to single serial to 
update BrotliCompress to the latest version 1.0.6. 

Thanks
Liming
> -Original Message-
> From: Zeng, Star
> Sent: Monday, October 8, 2018 9:37 AM
> To: Gao, Liming ; edk2-devel@lists.01.org
> Cc: Zeng, Star 
> Subject: RE: [edk2] [Patch 0/2] Update BrotliCompress to the latest 
> version 1.0.5
> 
> Reviewed-by: Star Zeng 
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Liming Gao
> Sent: Monday, September 10, 2018 8:37 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [Patch 0/2] Update BrotliCompress to the latest 
> version 1.0.5
> 
> Because the patch is too big, the change is placed in 
> https://github.com/lgao4/edk2/tree/Brotli
> 
> Update Brotli to the latest version 1.0.5 
> https://github.com/google/brotli Verify VS2017, GCC5 build.
> Verify Decompression boot functionality.
> 
> Liming Gao (2):
>   BaseTools: Update Brotli Compress to the latest one 1.0.5
>   MdeModulePkg: Update Brotli DecompressLib to the latest v1.0.5
> 
>  BaseTools/BinWrappers/PosixLike/BrotliCompress |13 +-
>  .../WindowsLike}/BrotliCompress.bat|12 +-
>  BaseTools/Source/C/BrotliCompress/GNUmakefile  | 8 +-
>  BaseTools/Source/C/BrotliCompress/Makefile |20 +-
>  BaseTools/Source/C/BrotliCompress/ReadMe.txt   | 2 +-
>  .../Source/C/BrotliCompress/common/constants.h |25 +-
>  .../Source/C/BrotliCompress/common}/context.h  |   356 +-
>  .../Source/C/BrotliCompress/common/dictionary.c| 15341 +++--
>  .../Source/C/BrotliCompress/common/dictionary.h|47 +-
>  .../Source/C/BrotliCompress/common/platform.h  |   509 +
>  BaseTools/Source/C/BrotliCompress/common/port.h|   107 -
>  .../Source/C/BrotliCompress/common/transform.c |   235 +
>  .../Source/C/BrotliCompress/common/transform.h |80 +
>  BaseTools/Source/C/BrotliCompress/common/types.h   |58 -
>  BaseTools/Source/C/BrotliCompress/common/version.h |26 +
>  BaseTools/Source/C/BrotliCompress/dec/bit_reader.c | 6 +-
>  BaseTools/Source/C/BrotliCompress/dec/bit_reader.h |   162 +-
>  BaseTools/Source/C/BrotliCompress/dec/decode.c |  1208 +-
>  BaseTools/Source/C/BrotliCompress/dec/decode.h |   188 -
>  BaseTools/Source/C/BrotliCompress/dec/huffman.c|59 +-
>  BaseTools/Source/C/BrotliCompress/dec/huffman.h|24 +-
>  BaseTools/Source/C/BrotliCompress/dec/port.h   |   159 -
>  BaseTools/Source/C/BrotliCompress/dec/prefix.h | 9 +-
>  BaseTools/Source/C/BrotliCompress/dec/state.c  |90 +-
>  BaseTools/Source/C/BrotliCompress/dec/state.h  |82 +-
>  BaseTools/Source/C/BrotliCompress/dec/transform.h  |   300 -
>  .../C/BrotliCompress/enc/backward_references.c |   822 +-
>  .../C/BrotliCompress/enc/backward_references.h |77 +-
>  .../C/BrotliCompress/enc/backward_references_hq.c  |   830 +
>  .../C/BrotliCompress/enc/backward_references_hq.h  |93 +
>  .../C/BrotliCompress/enc/backward_references_inc.h |70 +-
>  BaseTools/Source/C/BrotliCompress/enc/bit_cost.c   | 4 +-
>  BaseTools/Source/C/BrotliCompress/enc/bit_cost.h   |12 +-
>  .../C/BrotliCompress/enc/block_encoder_inc.h   |13 +-
>  .../Source/C/BrotliCompress/enc/block_splitter.c   |11 +-
>  .../Source/C/BrotliCompress/enc/block_splitter.h   | 4 +-
>  .../C/BrotliCompress/enc/block_splitter_inc.h  |35 +-
>  .../C/BrotliCompress/enc/brotli_bit_stream.c   |   223 +-
>  .../C/BrotliCompress/enc/brotli_bit_stream.h   |63 +-
>  BaseTools/Source/C/BrotliCompress/enc/cluster.c| 4 +-
>  BaseTools/Source/C/BrotliCompress/enc/cluster.h| 4 +-
>  .../Source/C/BrotliCompress/enc/cluster_inc.h  | 2 +
>  BaseTools/Source/C/BrotliCompress/enc/command.h|83 +-
>  .../C/BrotliCompress/enc/compress_fragment.c   |   189 +-
>  .../C/BrotliCompress/enc/compress_fragment.h   | 9 +-
>  .../enc/compress_fragment_two_pass.c   |   296 +-
>  .../enc/compress_fragment_two_pass.h   | 9 +-
>  BaseTools/Source/C/BrotliCompress/enc/compressor.h |   161 -
>  BaseTools/Source/C/BrotliCompress/enc/context.h|   184 -
>  .../Source/C/BrotliCompress/enc/dictionary_hash.c  |  1120 ++  
> .../Source/C/BrotliCompress/enc/dictionary_hash.h  |  4101 +
>  BaseTools/Source/C/BrotliCompress/enc/encode.c |

Re: [edk2] [PATCH] MdeModulePkg/RegularExpressionDxe:omit unused variable

[Guo, Dongao]

This code is based on oniguruma,a opensource regular expression project.
So I want to keep the origin code as much as possible.And I distinguish my 
changing by comment them out instead of removing.
Actually the function is unused.

Thanks,
Dongao.

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Gary Lin
Sent: Monday, October 8, 2018 5:52 PM
To: Guo, Dongao 
Cc: edk2-devel@lists.01.org; Gao, Liming 
Subject: Re: [edk2] [PATCH] MdeModulePkg/RegularExpressionDxe:omit unused 
variable

On Mon, Oct 08, 2018 at 03:56:48PM +0800, Dongao Guo wrote:
> 
> comment unused variable to avoid warning,and modify inf build option.
> 
Why not just remove the variables altogether instead of commenting them out?
Is it on purpose?

Gary Lin

> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dongao Guo 
> ---
>  .../RegularExpressionDxe/Oniguruma/regexec.c   | 28 
> +++---
>  .../RegularExpressionDxe/RegularExpressionDxe.inf  |  3 ---
>  2 files changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git 
> a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c 
> b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c
> index 7b0fda0..26e7a31 100644
> --- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c
> +++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c
> @@ -5603,11 +5603,11 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* 
> user_data)
>int r;
>int num;
>size_t tag_len;
> -  const UChar* start;
> -  const UChar* right;
> -  const UChar* current;
> -  const UChar* string;
> -  const UChar* strend;
> + // const UChar* start;
> + // const UChar* right;
> + // const UChar* current;
> + // const UChar* string;
> + // const UChar* strend;
>const UChar* tag_start;
>const UChar* tag_end;
>regex_t* reg;
> @@ -5615,9 +5615,9 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* 
> user_data)
>OnigType type;
>OnigValue val;
>char buf[20];
> -  FILE* fp;
> + // FILE* fp;
>  
> -  fp = OutFp;
> + // fp = OutFp;
>  
>r = onig_get_arg_by_callout_args(args, 0, &type, &val);
>if (r != ONIG_NORMAL) return r;
> @@ -5633,11 +5633,11 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* 
> user_data)
>}
>  
>num   = onig_get_callout_num_by_callout_args(args);
> -  start = onig_get_start_by_callout_args(args);
> -  right = onig_get_right_range_by_callout_args(args);
> -  current   = onig_get_current_by_callout_args(args);
> -  string= onig_get_string_by_callout_args(args);
> -  strend= onig_get_string_end_by_callout_args(args);
> + // start = onig_get_start_by_callout_args(args);
> + // right = onig_get_right_range_by_callout_args(args);
> + // current   = onig_get_current_by_callout_args(args);
> + // string= onig_get_string_by_callout_args(args);
> + // strend= onig_get_string_end_by_callout_args(args);
>reg   = onig_get_regex_by_callout_args(args);
>tag_start = onig_get_callout_tag_start(reg, num);
>tag_end   = onig_get_callout_tag_end(reg, num);
> @@ -5653,7 +5653,7 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* 
> user_data)
>  for (i = 0; i < tag_len; i++) buf[i] = tag_start[i];
>  buf[tag_len] = '\0';
>}
> -
> +/*
>fprintf(fp, "ONIG-MONITOR: %-4s %s at: %d [%d - %d] len: %d\n",
>buf,
>in == ONIG_CALLOUT_IN_PROGRESS ? "=>" : "<=", @@ -5662,7 
> +5662,7 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* user_data)
>(int )(right   - string),
>(int )(strend  - string));
>//fflush(fp);
> -
> +*/
>return ONIG_CALLOUT_SUCCESS;
>  }
>  
> diff --git 
> a/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf 
> b/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf
> index 16e91bd..98fb8db 100644
> --- 
> a/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf
> +++ b/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe
> +++ .inf
> @@ -106,6 +106,3 @@
>  
># Oniguruma: signed and unsigned mismatch/cast
>MSFT:*_*_*_CC_FLAGS = /wd4018 /wd4245 /wd4389
> -
> -  # Oniguruma: error: variable 'fp' set but not used
> -  GCC:*_*_*_CC_FLAGS = -Wno-error=unused-but-set-variable
> --
> 1.9.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] GenFds broken in latest basetools-win32

[Gao, Liming]

Yes.  The binary version edk2-BaseTools-win32 doesn't work now. It will not be 
maintained. 

If you still want to use the binary version BaseTools, you can switch to 
version dd00d6c9eed34b5b4d3fb0c5a5714ceeaed76165 of edk2-BaseTools-win32. It 
can work. 

And, I strongly suggest to run BaseTools Python from source in Windows. After 
you install python27 and set PYTHON_HOME, there is no extra step. 

Thanks
Liming
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Kurt Kennett
>Sent: Monday, October 08, 2018 11:28 PM
>To: Zhu, Yonghong ; edk2-devel@lists.01.org
>Subject: Re: [edk2] GenFds broken in latest basetools-win32
>
>Does this mean that I should expect the Win32 tools to not work any more
>"out of the box" for building images using GenFds?
>
>K2
>
>-Original Message-
>From: Zhu, Yonghong 
>Sent: Sunday, October 7, 2018 5:35 PM
>To: Kurt Kennett ; edk2-devel@lists.01.org
>Cc: Zhu, Yonghong 
>Subject: RE: GenFds broken in latest basetools-win32
>
>Hi,
>
>We planned to drop the support of BaseTools Python run from the freeze
>binary in Windows OS
>https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.
>org%2Fpipermail%2Fedk2-devel%2F2018-
>September%2F029436.html&data=02%7C01%7CKurt.Kennett%40micros
>oft.com%7C9a74694695f942c56c4608d62cb5ec03%7C72f988bf86f141af91ab2d
>7cd011db47%7C1%7C0%7C636745557205122493&sdata=FhEKPzqONdjpw
>5%2Bw4fZrGMfmiESE2M5IoO6YlT23mDs%3D&reserved=0
>
>Please run BaseTools Python from source in Windows. Here is the step wiki
>https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
>com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FWindows-
>systems&data=02%7C01%7CKurt.Kennett%40microsoft.com%7C9a74694
>695f942c56c4608d62cb5ec03%7C72f988bf86f141af91ab2d7cd011db47%7C1%7
>C0%7C636745557205122493&sdata=2MmoG395pbEHaQ1ybsLMNjepqe%
>2BwonqtbR%2BcGeDnqng%3D&reserved=0  Compile Tools section.
>
>Best Regards,
>Zhu Yonghong
>
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Kurt Kennett
>Sent: Tuesday, October 2, 2018 4:55 AM
>To: edk2-devel@lists.01.org
>Subject: [edk2] GenFds broken in latest basetools-win32
>
>"genfds" seems broken in latest tiano-win32
>
>Can anybody else repo this:
>
>C:\repo\tiano-win32>genfds
>Traceback (most recent call last):
>  File "C:\Python27\lib\site-packages\cx_Freeze\initscripts\Console.py", line
>27, in 
>  File "GenFds\GenFds.py", line 24, in 
>ValueError: Attempted relative import in non-package
>
>K2
>___
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.
>org%2Fmailman%2Flistinfo%2Fedk2-
>devel&data=02%7C01%7CKurt.Kennett%40microsoft.com%7C9a7469469
>5f942c56c4608d62cb5ec03%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0
>%7C636745557205122493&sdata=RWW9n3h6wznjqsLMhVeUAGTXrq4eU
>NoOeXY4GcpSx%2Bg%3D&reserved=0
>___
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."

[Laszlo Ersek]

(meta)

On 10/08/18 17:26, jim.dai...@dell.com wrote:
> Resending, as I never saw this email on the mailing list.

I did -- here's a link to the original in one of the archives too:

c8ccaad1befc4db4bcd3f835930c2508@ausx13mps335.AMER.DELL.COM">http://mid.mail-archive.com/c8ccaad1befc4db4bcd3f835930c2508@ausx13mps335.AMER.DELL.COM

(You were right to re-send, just confirming.)

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] GenFds broken in latest basetools-win32

[Kurt Kennett]

Does this mean that I should expect the Win32 tools to not work any more "out 
of the box" for building images using GenFds?

K2

-Original Message-
From: Zhu, Yonghong  
Sent: Sunday, October 7, 2018 5:35 PM
To: Kurt Kennett ; edk2-devel@lists.01.org
Cc: Zhu, Yonghong 
Subject: RE: GenFds broken in latest basetools-win32

Hi,

We planned to drop the support of BaseTools Python run from the freeze binary 
in Windows OS 
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.org%2Fpipermail%2Fedk2-devel%2F2018-September%2F029436.html&data=02%7C01%7CKurt.Kennett%40microsoft.com%7C9a74694695f942c56c4608d62cb5ec03%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636745557205122493&sdata=FhEKPzqONdjpw5%2Bw4fZrGMfmiESE2M5IoO6YlT23mDs%3D&reserved=0
 

Please run BaseTools Python from source in Windows. Here is the step wiki  
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FWindows-systems&data=02%7C01%7CKurt.Kennett%40microsoft.com%7C9a74694695f942c56c4608d62cb5ec03%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636745557205122493&sdata=2MmoG395pbEHaQ1ybsLMNjepqe%2BwonqtbR%2BcGeDnqng%3D&reserved=0
  Compile Tools section.

Best Regards,
Zhu Yonghong

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Kurt 
Kennett
Sent: Tuesday, October 2, 2018 4:55 AM
To: edk2-devel@lists.01.org
Subject: [edk2] GenFds broken in latest basetools-win32

"genfds" seems broken in latest tiano-win32

Can anybody else repo this:

C:\repo\tiano-win32>genfds
Traceback (most recent call last):
  File "C:\Python27\lib\site-packages\cx_Freeze\initscripts\Console.py", line 
27, in 
  File "GenFds\GenFds.py", line 24, in 
ValueError: Attempted relative import in non-package

K2
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.org%2Fmailman%2Flistinfo%2Fedk2-devel&data=02%7C01%7CKurt.Kennett%40microsoft.com%7C9a74694695f942c56c4608d62cb5ec03%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636745557205122493&sdata=RWW9n3h6wznjqsLMhVeUAGTXrq4eUNoOeXY4GcpSx%2Bg%3D&reserved=0
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."

[Jim.Dailey]

Resending, as I never saw this email on the mailing list.

>-Original Message-
>From: Ni, Ruiyu [mailto:ruiyu...@intel.com] 
>Sent: Monday, October 8, 2018 2:00 AM
>To: Dailey, Jim; edk2-devel@lists.01.org
>Cc: michael.d.kin...@intel.com; liming@intel.com
>Subject: Re: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() error 
>involving "\..\.."
>
>
>On 10/4/2018 11:03 PM, jim.dai...@dell.com wrote:
>> MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."
>> 
>> The loop that removes "\..\" errs when multiple "\.." sequences are
>> in the path.  Before this change the code would modify a path like
>> "FS0:\efi\tools\..\.." to "FS0:\efi\\.." and then to "FS0:\efi\", but
>> the correct path is "FS0:\".
>> 
>> You can test the effect of this change in the shell by setting the
>> current directory to something like FS0:\efi\boot and then executing
>> the command "ls ..\..".  Before the change you will see the files in
>> the FS0:\efi directory; after the change, you will see the files in
>> the root directory of FS0:.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jim Dailey 
>> ---
>>   MdePkg/Library/BaseLib/FilePaths.c | 5 -
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/MdePkg/Library/BaseLib/FilePaths.c 
>> b/MdePkg/Library/BaseLib/FilePaths.c
>> index d6f3758ecb..5d3de01894 100644
>> --- a/MdePkg/Library/BaseLib/FilePaths.c
>> +++ b/MdePkg/Library/BaseLib/FilePaths.c
>> @@ -2,6 +2,7 @@
>> Defines file-path manipulation functions.
>>   
>> Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.
>> +  Copyright (c) 2018, Dell Technologies. All rights reserved.
>> This program and the accompanying materials
>> are licensed and made available under the terms and conditions of the 
>> BSD License
>> which accompanies this distribution.  The full text of the license may 
>> be found at
>> @@ -103,7 +104,9 @@ PathCleanUpDirectories(
>>   ) {
>>   *(TempString + 1) = CHAR_NULL;
>>   PathRemoveLastItem(Path);
>> -CopyMem (Path + StrLen (Path), TempString + 3, StrSize (TempString + 
>> 3));
>> +if (*(TempString + 3)) {
>> +  CopyMem (Path + StrLen (Path), TempString + 4, StrSize (TempString + 
>> 4));
>> +}
>> }
>>   
>> //
>> 
>Jim,
>Are you fixing a corner case bug introduced by following commit:
>
> > SHA-1: bb99e3282c9e69fbd6365d117c58d15589e34c5d
>* MdePkg/BaseLib: Fix PathCleanUpDirectories to correctly handle "\.\"
>
> > The old code incorrectly cleans path like "fs0:\abc\.\.." to
> > "fs0:\abc", instead of "fs0:\"
>
> > The patch fixes this bug.

Honestly, because of the scope of bb99e328, I cannot say for sure, but
my inclination is to answer "No."

The current fix is strictly related to handling paths that include two
or more references to the previous directory, whether or not one of them
abuts a reference to the current directory (as in the bb99e328 fix).

Regards,
Jim
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [patch] MdeModulePkg/HiiDB: Fix incorrect structure convention for checkbox

[Laszlo Ersek]

On 10/08/18 16:32, Bi, Dandan wrote:
> Hi Laszlo,
> 
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Monday, October 08, 2018 8:19 PM
>> To: Bi, Dandan ; edk2-devel@lists.01.org
>> Cc: Zeng, Star ; Gao, Liming 
>> Subject: Re: [edk2] [patch] MdeModulePkg/HiiDB: Fix incorrect structure
>> convention for checkbox
>>
>> Hi Dandan,
>>
>> On 10/08/18 03:29, Dandan Bi wrote:
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1224
>>>
>>> When covert IFR binary to EFI_IFR_CHECKBOX structure, Current code has
>>> following incorrect code logic:
>>> IfrCheckBox = (EFI_IFR_CHECKBOX *) (IfrOpHdr + 1); The correct one
>>> should be:
>>> IfrCheckBox = (EFI_IFR_CHECKBOX *) IfrOpHdr;
>>>
>>> This patch is to fix this bug.
>>>
>>> Cc: Liming Gao 
>>> Cc: Star Zeng 
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Dandan Bi 
>>> ---
>>>  MdeModulePkg/Universal/HiiDatabaseDxe/Database.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
>>> b/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
>>> index 45448c5198..664687796f 100644
>>> --- a/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
>>> +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
>>> @@ -896,11 +896,11 @@ UpdateDefaultSettingInFormPackage (
>>>break;
>>>  case EFI_IFR_CHECKBOX_OP:
>>>IfrScope = IfrOpHdr->Scope;
>>>IfrQuestionType  = IfrOpHdr->OpCode;
>>>IfrQuestionHdr   = (EFI_IFR_QUESTION_HEADER *) (IfrOpHdr + 1);
>>> -  IfrCheckBox  = (EFI_IFR_CHECKBOX *) (IfrOpHdr + 1);
>>> +  IfrCheckBox  = (EFI_IFR_CHECKBOX *) IfrOpHdr;
>>>EfiVarStoreIndex = IsEfiVarStoreQuestion (IfrQuestionHdr,
>> EfiVarStoreList, EfiVarStoreNumber);
>>>Width= sizeof (BOOLEAN);
>>>if (EfiVarStoreIndex < EfiVarStoreNumber) {
>>>  for (Index = 0; Index < DefaultIdNumber; Index ++) {
>>>if (DefaultIdList[Index] == EFI_HII_DEFAULT_CLASS_STANDARD)
>>> {
>>>
>>
>> what were the practical consequences (symptoms) of this issue? Did some
>> checkboxes not work? (I'm asking because SecureBootConfigDxe uses some
>> checkboxes.)
> 
> 1.  The bug is in function "UpdateDefaultSettingInFormPackage()" which is to 
> update the default setting of some HII Questions in the IFR binary data. So 
> it only has impact when platform overrides default setting in HII VarStore 
> through DynamicHii PCD setting in Platform DSC file. If platform doesn't 
> override default setting, it has no impact.
> 
> 2. The implementation updates the "Flags" filed in the EFI_IFR_CHECKBOX 
> structure to update the default setting of checkbox.
> If using "IfrCheckBox  = (EFI_IFR_CHECKBOX *) (IfrOpHdr + 1);" when wants 
> to update the " Flags" filed in checkbox, but in fact it will update the 
> opcode binary data(opcode binary length) behind checkbox binary. 
> And then it will cause Browser can't parse the IFR binary data correctly. And 
> then the possible symptom is that some HII Question and forms may be not 
> parsed and then cannot be shown.

Thanks! I've copied this into the BZ.

Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol

[Ard Biesheuvel]

On 8 October 2018 at 16:52, Marcin Wojtas  wrote:
> pon., 8 paź 2018 o 15:43 Ard Biesheuvel  
> napisał(a):
>>
>> On 8 October 2018 at 15:37, Marcin Wojtas  wrote:
>> > pon., 8 paź 2018 o 15:27 Ard Biesheuvel  
>> > napisał(a):
>> >>
>> >> On 8 October 2018 at 15:17, Marcin Wojtas  wrote:
>> >> > pon., 8 paź 2018 o 15:07 Ard Biesheuvel  
>> >> > napisał(a):
>> >> >>
>> >> >> On 8 October 2018 at 14:59, Marcin Wojtas  wrote:
>> >> >> > Hi Ard,
>> >> >> >
>> >> >> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel  
>> >> >> > napisał(a):
>> >> >> >>
>> >> >> >> (add MdeModulePkg maintainers)
>> >> >> >>
>> >> >> >> On 5 October 2018 at 15:25, Marcin Wojtas  wrote:
>> >> >> >> > From: Tomasz Michalec 
>> >> >> >> >
>> >> >> >> > Some SD Host Controlers use different values in Host Control 2 
>> >> >> >> > Register
>> >> >> >> > to select UHS Mode. This patch adds a new UhsSignaling type 
>> >> >> >> > routine to
>> >> >> >> > the NotifyPhase of the SdMmcOverride protocol.
>> >> >> >> >
>> >> >> >> > UHS signaling configuration is moved to a common, default routine
>> >> >> >> > (SdMmcHcUhsSignaling), which is called when SdMmcOverride does not
>> >> >> >> > cover this functionality.
>> >> >> >> >
>> >> >> >> > Contributed-under: TianoCore Contribution Agreement 1.1
>> >> >> >> > Signed-off-by: Marcin Wojtas 
>> >> >> >> > ---
>> >> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  50 +++
>> >> >> >> >  MdeModulePkg/Include/Protocol/SdMmcOverride.h|   2 +
>> >> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 153 
>> >> >> >> > 
>> >> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c|  37 +++--
>> >> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c |  69 +
>> >> >> >> >  5 files changed, 243 insertions(+), 68 deletions(-)
>> >> >> >> >
>> >> >> >> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h 
>> >> >> >> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> >> >> >> > index e389d52..a03160d 100644
>> >> >> >> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> >> >> >> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> >> >> >> > @@ -63,6 +63,39 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY 
>> >> >> >> > KIND, EITHER EXPRESS OR IMPLIED.
>> >> >> >> >  #define SD_MMC_HC_CTRL_VER0xFE
>> >> >> >> >
>> >> >> >> >  //
>> >> >> >> > +// SD Host Controler bits to HOST_CTRL2 register
>> >> >> >> > +//
>> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_MASK   0x0007
>> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12  0x
>> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR25  0x0001
>> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR50  0x0002
>> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR104 0x0003
>> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_DDR50  0x0004
>> >> >> >> > +#define SD_MMC_HC_CTRL_MMC_DDR52  0x0004
>> >> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR50  0x0002
>> >> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR25  0x0001
>> >> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR12  0x
>> >> >> >> > +#define SD_MMC_HC_CTRL_HS200  0x0003
>> >> >> >> > +#define SD_MMC_HC_CTRL_HS400  0x0005
>> >> >> >> > +
>> >
>> > In case we move enums to SdMmcOverride.h, would it be desired, to move
>> > there register fields values as well? Or should I rather use Xenon
>> > macros for all of above locally?
>> >
>>
>> No, I think the macros should be kept locally.
>>
>> >> >> >> > +//
>> >> >> >> > +// Timing modes for uhs
>> >> >> >> > +//
>> >> >> >> > +typedef enum {
>> >> >> >> > +  SdMmcUhsSdr12,
>> >> >> >> > +  SdMmcUhsSdr25,
>> >> >> >> > +  SdMmcUhsSdr50,
>> >> >> >> > +  SdMmcUhsSdr104,
>> >> >> >> > +  SdMmcUhsDdr50,
>> >> >> >> > +  SdMmcMmcDdr52,
>> >> >> >> > +  SdMmcMmcSdr50,
>> >> >> >> > +  SdMmcMmcSdr25,
>> >> >> >> > +  SdMmcMmcSdr12,
>> >> >> >> > +  SdMmcMmcHs200,
>> >> >> >> > +  SdMmcMmcHs400,
>> >> >> >> > +} SD_MMC_UHS_TIMING;
>> >> >> >> > +
>> >> >> >>
>> >> >> >> Here, we end up with two sets of symbolic constants for the same
>> >> >> >> thing, and I suppose this enum will be duplicated in your
>> >> >> >> SdMmcOverride implementation?
>> >> >> >>
>> >> >> >
>> >> >> > Why duplicated? Macros are for generic UHS_MODE_SEL field values for
>> >> >> > SD and MMC in HostControl2Register.
>> >> >> >
>> >> >> > SD_MMC_UHS_TIMING is just a timing mode indicator, it can be used not
>> >> >> > only in UhsSignaling routine (actually the next patch, with
>> >> >> > SwitchClockFreqPost, use it...).
>> >> >> >
>> >> >> > In my SdMmcOverride implementation this enum is not duplicated,
>> >> >> > because this file (SdMmcPciHci.h) is included via
>> >> >> > Protocol/SdMmcOverride.h.
>> >> >> >
>> >> >>
>> >> >> Ah ok. Please don't expose internal headers of the SD/MMC driver via
>> >> >> Protocol/SdMmcOverride.h
>> >> >>
>> >> >
>> >> > OK.
>> >> >
>> >> >> I think it should be fine to add the enum definition to
>> >> >> Protocol/SdMmcOverride.h instead.
>> >> >>
>> >> >
>> >> > OK.
>

Re: [edk2] [PATCH] BaseTools: Allow multiple PciDeviceId in Fdf OptionROM override

[Carsey, Jaben]

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Gao, Liming
> Sent: Sunday, October 07, 2018 6:42 PM
> To: Tomas Pilar (tpilar) ; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] BaseTools: Allow multiple PciDeviceId in Fdf
> OptionROM override
> 
> Pilar:
>   The change is good. Could you also update INF and FDF spec for this usage?
> If you don't know how to update INF and FDF spec, please submit BZ. I will
> provide the spec patch.
> 
>   Reviewed-by: Liming Gao 
> 
> Thanks
> Liming
> >-Original Message-
> >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> >Tomas Pilar (tpilar)
> >Sent: Tuesday, October 02, 2018 10:46 PM
> >To: edk2-devel@lists.01.org
> >Subject: [edk2] [PATCH] BaseTools: Allow multiple PciDeviceId in Fdf
> >OptionROM override
> >
> >Contributed-under: TianoCore Contribution Agreement 1.1
> >Signed-off-by: Tomas Pilar 
> >---
> > BaseTools/Source/Python/GenFds/FdfParser.py | 11 ---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> >diff --git a/BaseTools/Source/Python/GenFds/FdfParser.py
> >b/BaseTools/Source/Python/GenFds/FdfParser.py
> >index 63687e98bb..a65f2cfd2d 100644
> >--- a/BaseTools/Source/Python/GenFds/FdfParser.py
> >+++ b/BaseTools/Source/Python/GenFds/FdfParser.py
> >@@ -4469,10 +4469,15 @@ class FdfParser:
> > if self.__IsKeyword( "PCI_DEVICE_ID"):
> > if not self.__IsToken( "="):
> > raise Warning("expected '='", self.FileName,
> >self.CurrentLineNumber)
> >-if not self.__GetNextHexNumber():
> >-raise Warning("expected Hex device id", 
> >self.FileName,
> >self.CurrentLineNumber)
> >
> >-Overrides.PciDeviceId = self.__Token
> >+# Get a list of PCI IDs
> >+Overrides.PciDeviceId = ""
> >+
> >+while self.__GetNextHexNumber():
> >+Overrides.PciDeviceId += " " + self.__Token

Can we change to minimize looping string concatenation here?  This in a loop 
will cause lots of memory allocation/deallocation and slow things down.

Maybe : 
Overrides.PciDeviceId = "{} {}".format(Overrides.PciDeviceId, self.__Token)


> >+
> >+if not Overrides.PciDeviceId:
> >+raise Warning("expected one or more Hex device ids",
> >self.FileName, self.CurrentLineNumber)
> > continue
> >
> > if self.__IsKeyword( "PCI_REVISION"):
> >--
> >2.17.1
> >
> >___
> >edk2-devel mailing list
> >edk2-devel@lists.01.org
> >https://lists.01.org/mailman/listinfo/edk2-devel
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ShellPkg-Cd: Ensure all valid cd targets are handled properly

[Carsey, Jaben]

Seems good to me.

Any thoughts Ray?

Reviewed-by: Jaben Carsey 

> -Original Message-
> From: jim.dai...@dell.com [mailto:jim.dai...@dell.com]
> Sent: Thursday, October 04, 2018 9:47 AM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben ; Ni, Ruiyu 
> Subject: [edk2] [PATCH] ShellPkg-Cd: Ensure all valid cd targets are handled
> properly
> Importance: High
> 
> ShellPkg-Cd: Ensure all valid cd targets are handled properly
> 
> Make sure that PathCleanUpDirectories() is called on all valid targets
> of the cd command.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jim Dailey 
> ---
>  ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c
> b/ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c
> index 79dd2096f4..1eb7056aee 100644
> --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c
> +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c
> @@ -4,6 +4,7 @@
>(C) Copyright 2016 Hewlett Packard Enterprise Development LP
>(C) Copyright 2015 Hewlett-Packard Development Company, L.P.
>Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
> +  Copyright (c) 2018, Dell Technologies. All rights reserved.
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD
> License
>which accompanies this distribution.  The full text of the license may be
> found at
> @@ -261,9 +262,6 @@ ShellCommandRunCd (
> 
>  if (Param1Copy != NULL && IsCurrentFileSystem (Param1Copy, Cwd)) {
>Status = ReplaceDriveWithCwd (&Param1Copy,Cwd);
> -  if (!EFI_ERROR (Status)) {
> -Param1Copy = PathCleanUpDirectories (Param1Copy);
> -  }
>  } else {
>//
>// Can't use cd command to change filesystem.
> @@ -302,13 +300,15 @@ ShellCommandRunCd (
>  StrCatS (TempBuffer, TotalSize / sizeof (CHAR16), 
> Param1Copy);
> 
>  FreePool (Param1Copy);
> -Param1Copy = PathCleanUpDirectories (TempBuffer);
> +Param1Copy = TempBuffer;
> +TempBuffer = NULL;
>}
>  }
>}
>  }
> 
>  if (!EFI_ERROR(Status)) {
> +  Param1Copy = PathCleanUpDirectories (Param1Copy);
>Status = ExtractDriveAndPath (Param1Copy, &Drive, &Path);
>  }
> 
> --
> 2.17.0.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol

[Marcin Wojtas]

pon., 8 paź 2018 o 15:43 Ard Biesheuvel  napisał(a):
>
> On 8 October 2018 at 15:37, Marcin Wojtas  wrote:
> > pon., 8 paź 2018 o 15:27 Ard Biesheuvel  
> > napisał(a):
> >>
> >> On 8 October 2018 at 15:17, Marcin Wojtas  wrote:
> >> > pon., 8 paź 2018 o 15:07 Ard Biesheuvel  
> >> > napisał(a):
> >> >>
> >> >> On 8 October 2018 at 14:59, Marcin Wojtas  wrote:
> >> >> > Hi Ard,
> >> >> >
> >> >> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel  
> >> >> > napisał(a):
> >> >> >>
> >> >> >> (add MdeModulePkg maintainers)
> >> >> >>
> >> >> >> On 5 October 2018 at 15:25, Marcin Wojtas  wrote:
> >> >> >> > From: Tomasz Michalec 
> >> >> >> >
> >> >> >> > Some SD Host Controlers use different values in Host Control 2 
> >> >> >> > Register
> >> >> >> > to select UHS Mode. This patch adds a new UhsSignaling type 
> >> >> >> > routine to
> >> >> >> > the NotifyPhase of the SdMmcOverride protocol.
> >> >> >> >
> >> >> >> > UHS signaling configuration is moved to a common, default routine
> >> >> >> > (SdMmcHcUhsSignaling), which is called when SdMmcOverride does not
> >> >> >> > cover this functionality.
> >> >> >> >
> >> >> >> > Contributed-under: TianoCore Contribution Agreement 1.1
> >> >> >> > Signed-off-by: Marcin Wojtas 
> >> >> >> > ---
> >> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  50 +++
> >> >> >> >  MdeModulePkg/Include/Protocol/SdMmcOverride.h|   2 +
> >> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 153 
> >> >> >> > 
> >> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c|  37 +++--
> >> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c |  69 +
> >> >> >> >  5 files changed, 243 insertions(+), 68 deletions(-)
> >> >> >> >
> >> >> >> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h 
> >> >> >> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> >> >> >> > index e389d52..a03160d 100644
> >> >> >> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> >> >> >> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> >> >> >> > @@ -63,6 +63,39 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY 
> >> >> >> > KIND, EITHER EXPRESS OR IMPLIED.
> >> >> >> >  #define SD_MMC_HC_CTRL_VER0xFE
> >> >> >> >
> >> >> >> >  //
> >> >> >> > +// SD Host Controler bits to HOST_CTRL2 register
> >> >> >> > +//
> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_MASK   0x0007
> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12  0x
> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR25  0x0001
> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR50  0x0002
> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR104 0x0003
> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_DDR50  0x0004
> >> >> >> > +#define SD_MMC_HC_CTRL_MMC_DDR52  0x0004
> >> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR50  0x0002
> >> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR25  0x0001
> >> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR12  0x
> >> >> >> > +#define SD_MMC_HC_CTRL_HS200  0x0003
> >> >> >> > +#define SD_MMC_HC_CTRL_HS400  0x0005
> >> >> >> > +
> >
> > In case we move enums to SdMmcOverride.h, would it be desired, to move
> > there register fields values as well? Or should I rather use Xenon
> > macros for all of above locally?
> >
>
> No, I think the macros should be kept locally.
>
> >> >> >> > +//
> >> >> >> > +// Timing modes for uhs
> >> >> >> > +//
> >> >> >> > +typedef enum {
> >> >> >> > +  SdMmcUhsSdr12,
> >> >> >> > +  SdMmcUhsSdr25,
> >> >> >> > +  SdMmcUhsSdr50,
> >> >> >> > +  SdMmcUhsSdr104,
> >> >> >> > +  SdMmcUhsDdr50,
> >> >> >> > +  SdMmcMmcDdr52,
> >> >> >> > +  SdMmcMmcSdr50,
> >> >> >> > +  SdMmcMmcSdr25,
> >> >> >> > +  SdMmcMmcSdr12,
> >> >> >> > +  SdMmcMmcHs200,
> >> >> >> > +  SdMmcMmcHs400,
> >> >> >> > +} SD_MMC_UHS_TIMING;
> >> >> >> > +
> >> >> >>
> >> >> >> Here, we end up with two sets of symbolic constants for the same
> >> >> >> thing, and I suppose this enum will be duplicated in your
> >> >> >> SdMmcOverride implementation?
> >> >> >>
> >> >> >
> >> >> > Why duplicated? Macros are for generic UHS_MODE_SEL field values for
> >> >> > SD and MMC in HostControl2Register.
> >> >> >
> >> >> > SD_MMC_UHS_TIMING is just a timing mode indicator, it can be used not
> >> >> > only in UhsSignaling routine (actually the next patch, with
> >> >> > SwitchClockFreqPost, use it...).
> >> >> >
> >> >> > In my SdMmcOverride implementation this enum is not duplicated,
> >> >> > because this file (SdMmcPciHci.h) is included via
> >> >> > Protocol/SdMmcOverride.h.
> >> >> >
> >> >>
> >> >> Ah ok. Please don't expose internal headers of the SD/MMC driver via
> >> >> Protocol/SdMmcOverride.h
> >> >>
> >> >
> >> > OK.
> >> >
> >> >> I think it should be fine to add the enum definition to
> >> >> Protocol/SdMmcOverride.h instead.
> >> >>
> >> >
> >> > OK.
> >> >
> >> >> But wouldn't it be much easier to have a hook for setting
> >> >> HostControl2Register that decodes the value and modifies it according
> >> >> to what the platf

Re: [edk2] [Patch 0/2] Update BrotliCompress to the latest version 1.0.5

[Gao, Liming]

Star:
  Thank you for review. I just find Brotli 1.0.6 has been released. Based on 
v1.0.5, there is the minor change in source file. They have no functionality 
impact. I have pushed the additional patches in 
https://github.com/lgao4/edk2/tree/Brotli to integrate 1.0.6 change. Could you 
help review them? After review, I will combine the patches to single serial to 
update BrotliCompress to the latest version 1.0.6. 

Thanks
Liming
> -Original Message-
> From: Zeng, Star
> Sent: Monday, October 8, 2018 9:37 AM
> To: Gao, Liming ; edk2-devel@lists.01.org
> Cc: Zeng, Star 
> Subject: RE: [edk2] [Patch 0/2] Update BrotliCompress to the latest version 
> 1.0.5
> 
> Reviewed-by: Star Zeng 
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Liming 
> Gao
> Sent: Monday, September 10, 2018 8:37 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [Patch 0/2] Update BrotliCompress to the latest version 1.0.5
> 
> Because the patch is too big, the change is placed in
> https://github.com/lgao4/edk2/tree/Brotli
> 
> Update Brotli to the latest version 1.0.5
> https://github.com/google/brotli
> Verify VS2017, GCC5 build.
> Verify Decompression boot functionality.
> 
> Liming Gao (2):
>   BaseTools: Update Brotli Compress to the latest one 1.0.5
>   MdeModulePkg: Update Brotli DecompressLib to the latest v1.0.5
> 
>  BaseTools/BinWrappers/PosixLike/BrotliCompress |13 +-
>  .../WindowsLike}/BrotliCompress.bat|12 +-
>  BaseTools/Source/C/BrotliCompress/GNUmakefile  | 8 +-
>  BaseTools/Source/C/BrotliCompress/Makefile |20 +-
>  BaseTools/Source/C/BrotliCompress/ReadMe.txt   | 2 +-
>  .../Source/C/BrotliCompress/common/constants.h |25 +-
>  .../Source/C/BrotliCompress/common}/context.h  |   356 +-
>  .../Source/C/BrotliCompress/common/dictionary.c| 15341 +++--
>  .../Source/C/BrotliCompress/common/dictionary.h|47 +-
>  .../Source/C/BrotliCompress/common/platform.h  |   509 +
>  BaseTools/Source/C/BrotliCompress/common/port.h|   107 -
>  .../Source/C/BrotliCompress/common/transform.c |   235 +
>  .../Source/C/BrotliCompress/common/transform.h |80 +
>  BaseTools/Source/C/BrotliCompress/common/types.h   |58 -
>  BaseTools/Source/C/BrotliCompress/common/version.h |26 +
>  BaseTools/Source/C/BrotliCompress/dec/bit_reader.c | 6 +-
>  BaseTools/Source/C/BrotliCompress/dec/bit_reader.h |   162 +-
>  BaseTools/Source/C/BrotliCompress/dec/decode.c |  1208 +-
>  BaseTools/Source/C/BrotliCompress/dec/decode.h |   188 -
>  BaseTools/Source/C/BrotliCompress/dec/huffman.c|59 +-
>  BaseTools/Source/C/BrotliCompress/dec/huffman.h|24 +-
>  BaseTools/Source/C/BrotliCompress/dec/port.h   |   159 -
>  BaseTools/Source/C/BrotliCompress/dec/prefix.h | 9 +-
>  BaseTools/Source/C/BrotliCompress/dec/state.c  |90 +-
>  BaseTools/Source/C/BrotliCompress/dec/state.h  |82 +-
>  BaseTools/Source/C/BrotliCompress/dec/transform.h  |   300 -
>  .../C/BrotliCompress/enc/backward_references.c |   822 +-
>  .../C/BrotliCompress/enc/backward_references.h |77 +-
>  .../C/BrotliCompress/enc/backward_references_hq.c  |   830 +
>  .../C/BrotliCompress/enc/backward_references_hq.h  |93 +
>  .../C/BrotliCompress/enc/backward_references_inc.h |70 +-
>  BaseTools/Source/C/BrotliCompress/enc/bit_cost.c   | 4 +-
>  BaseTools/Source/C/BrotliCompress/enc/bit_cost.h   |12 +-
>  .../C/BrotliCompress/enc/block_encoder_inc.h   |13 +-
>  .../Source/C/BrotliCompress/enc/block_splitter.c   |11 +-
>  .../Source/C/BrotliCompress/enc/block_splitter.h   | 4 +-
>  .../C/BrotliCompress/enc/block_splitter_inc.h  |35 +-
>  .../C/BrotliCompress/enc/brotli_bit_stream.c   |   223 +-
>  .../C/BrotliCompress/enc/brotli_bit_stream.h   |63 +-
>  BaseTools/Source/C/BrotliCompress/enc/cluster.c| 4 +-
>  BaseTools/Source/C/BrotliCompress/enc/cluster.h| 4 +-
>  .../Source/C/BrotliCompress/enc/cluster_inc.h  | 2 +
>  BaseTools/Source/C/BrotliCompress/enc/command.h|83 +-
>  .../C/BrotliCompress/enc/compress_fragment.c   |   189 +-
>  .../C/BrotliCompress/enc/compress_fragment.h   | 9 +-
>  .../enc/compress_fragment_two_pass.c   |   296 +-
>  .../enc/compress_fragment_two_pass.h   | 9 +-
>  BaseTools/Source/C/BrotliCompress/enc/compressor.h |   161 -
>  BaseTools/Source/C/BrotliCompress/enc/context.h|   184 -
>  .../Source/C/BrotliCompress/enc/dictionary_hash.c  |  1120 ++
>  .../Source/C/BrotliCompress/enc/dictionary_hash.h  |  4101 +
>  BaseTools/Source/C/BrotliCompress/enc/encode.c |  1050 +-
>  BaseTools/Source/C/BrotliCompress/enc/encode.h |   221 -
>  .../Source/C/BrotliCompress/enc/encode_parallel.h  |27 -
>  .../Source/C/BrotliCompress/enc/encoder_dict.c |32 +
>  .../Source/C/BrotliCompress/enc/en

Re: [edk2] [Patch 2/2] ShellPkg/TftpDynamicCommand: Correct comments to align with the input parameter.

[Carsey, Jaben]

Reviewed-by: Jaben Carsey 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Jiaxin Wu
> Sent: Sunday, October 07, 2018 8:03 PM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben ; Ye, Ting ;
> Fu, Siyuan ; Wu, Jiaxin ; Bi,
> Dandan 
> Subject: [edk2] [Patch 2/2] ShellPkg/TftpDynamicCommand: Correct
> comments to align with the input parameter.
> Importance: High
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1230
> 
> Cc: Carsey Jaben 
> Cc: Fu Siyuan 
> Cc: Ye Ting 
> Cc: Bi Dandan 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wu Jiaxin 
> ---
>  ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
> b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
> index d4391b9f33..ccf7abde42 100644
> --- a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
> +++ b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
> @@ -163,10 +163,11 @@ GetFileSize (
>@param[in]   Mtftp4 MTFTP4 protocol interface
>@param[in]   FilePath   Path of the file, Unicode encoded
>@param[in]   AsciiFilePath  Path of the file, ASCII encoded
>@param[in]   FileSize   Size of the file in number of bytes
>@param[in]   BlockSize  Value of the TFTP blksize option
> +  @param[in]   WindowSize Value of the TFTP window size option
>@param[out]  Data   Address where to store the address of the 
> buffer
>where the data of the file were downloaded in
>case of success.
> 
>@retval  EFI_SUCCESS   The file was downloaded.
> @@ -904,10 +905,11 @@ Error :
>@param[in]   Mtftp4 MTFTP4 protocol interface
>@param[in]   FilePath   Path of the file, Unicode encoded
>@param[in]   AsciiFilePath  Path of the file, ASCII encoded
>@param[in]   FileSize   Size of the file in number of bytes
>@param[in]   BlockSize  Value of the TFTP blksize option
> +  @param[in]   WindowSize Value of the TFTP window size option
>@param[out]  Data   Address where to store the address of the 
> buffer
>where the data of the file were downloaded in
>case of success.
> 
>@retval  EFI_SUCCESS   The file was downloaded.
> --
> 2.17.1.windows.2
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [patch 1/5] MdePkg: Correct the string expression of UTF8 vendor device path

[Bi, Dandan]

Hi Laszlo,

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Monday, October 08, 2018 7:55 PM
> To: Bi, Dandan ; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu ; Kinney, Michael D
> ; Gao, Liming 
> Subject: Re: [edk2] [patch 1/5] MdePkg: Correct the string expression of
> UTF8 vendor device path
> 
> Hi Dandan,
> 
> On 10/08/18 05:31, Dandan Bi wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1225
> >
> > According to UEFI spec, the string expression of UTF8 vendor device
> > node should be displayed as: VenUtf8(). Current code display it as:
> > VenUft8() by mistake when convert device path node to text.
> >
> > This commit is to fix this bug.
> >
> > Cc: Ruiyu Ni 
> > Cc: Michael D Kinney 
> > Cc: Liming Gao 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Dandan Bi 
> > ---
> >  MdePkg/Library/UefiDevicePathLib/DevicePathToText.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> > b/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> > index 7d8d304f6f..85f5e97131 100644
> > --- a/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> > +++ b/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> > @@ -193,11 +193,11 @@ DevPathToTextVendor (
> >  return ;
> >} else if (CompareGuid (&Vendor->Guid, &gEfiVT100PlusGuid)) {
> >  UefiDevicePathLibCatPrint (Str, L"VenVt100Plus()");
> >  return ;
> >} else if (CompareGuid (&Vendor->Guid, &gEfiVTUTF8Guid)) {
> > -UefiDevicePathLibCatPrint (Str, L"VenUft8()");
> > +UefiDevicePathLibCatPrint (Str, L"VenUtf8()");
> >  return ;
> >} else if (CompareGuid (&Vendor->Guid, &gEfiUartDevicePathGuid)) {
> >  FlowControlMap = (((UART_FLOW_CONTROL_DEVICE_PATH *)
> Vendor)->FlowControlMap);
> >  switch (FlowControlMap & 0x0003) {
> >  case 0:
> >
> 
> it makes sense to send a set of patches that are correlated in some fashion,
> even if they individually address different BZs and don't form a coherent
> "feature" or larger "bugfix". However, even in such cases, please send a
> common cover letter (0/5 in this case). Seeing a unified diffstat, and a few
> intro words (about the common theme of the patch
> set) is helpful.
> 
> (no need to repost, just for the future)
> 
Thanks for the reminder, I will pay more attention in the future.

Thanks,
Dandan

> Thanks
> Laszlo
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [patch] MdeModulePkg/HiiDB: Fix incorrect structure convention for checkbox

[Bi, Dandan]

Hi Laszlo,

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Monday, October 08, 2018 8:19 PM
> To: Bi, Dandan ; edk2-devel@lists.01.org
> Cc: Zeng, Star ; Gao, Liming 
> Subject: Re: [edk2] [patch] MdeModulePkg/HiiDB: Fix incorrect structure
> convention for checkbox
> 
> Hi Dandan,
> 
> On 10/08/18 03:29, Dandan Bi wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1224
> >
> > When covert IFR binary to EFI_IFR_CHECKBOX structure, Current code has
> > following incorrect code logic:
> > IfrCheckBox = (EFI_IFR_CHECKBOX *) (IfrOpHdr + 1); The correct one
> > should be:
> > IfrCheckBox = (EFI_IFR_CHECKBOX *) IfrOpHdr;
> >
> > This patch is to fix this bug.
> >
> > Cc: Liming Gao 
> > Cc: Star Zeng 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Dandan Bi 
> > ---
> >  MdeModulePkg/Universal/HiiDatabaseDxe/Database.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
> > b/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
> > index 45448c5198..664687796f 100644
> > --- a/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
> > +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
> > @@ -896,11 +896,11 @@ UpdateDefaultSettingInFormPackage (
> >break;
> >  case EFI_IFR_CHECKBOX_OP:
> >IfrScope = IfrOpHdr->Scope;
> >IfrQuestionType  = IfrOpHdr->OpCode;
> >IfrQuestionHdr   = (EFI_IFR_QUESTION_HEADER *) (IfrOpHdr + 1);
> > -  IfrCheckBox  = (EFI_IFR_CHECKBOX *) (IfrOpHdr + 1);
> > +  IfrCheckBox  = (EFI_IFR_CHECKBOX *) IfrOpHdr;
> >EfiVarStoreIndex = IsEfiVarStoreQuestion (IfrQuestionHdr,
> EfiVarStoreList, EfiVarStoreNumber);
> >Width= sizeof (BOOLEAN);
> >if (EfiVarStoreIndex < EfiVarStoreNumber) {
> >  for (Index = 0; Index < DefaultIdNumber; Index ++) {
> >if (DefaultIdList[Index] == EFI_HII_DEFAULT_CLASS_STANDARD)
> > {
> >
> 
> what were the practical consequences (symptoms) of this issue? Did some
> checkboxes not work? (I'm asking because SecureBootConfigDxe uses some
> checkboxes.)

1.  The bug is in function "UpdateDefaultSettingInFormPackage()" which is to 
update the default setting of some HII Questions in the IFR binary data. So it 
only has impact when platform overrides default setting in HII VarStore through 
DynamicHii PCD setting in Platform DSC file. If platform doesn't override 
default setting, it has no impact.

2. The implementation updates the "Flags" filed in the EFI_IFR_CHECKBOX 
structure to update the default setting of checkbox.
If using "IfrCheckBox  = (EFI_IFR_CHECKBOX *) (IfrOpHdr + 1);" when wants 
to update the " Flags" filed in checkbox, but in fact it will update the opcode 
binary data(opcode binary length) behind checkbox binary. 
And then it will cause Browser can't parse the IFR binary data correctly. And 
then the possible symptom is that some HII Question and forms may be not parsed 
and then cannot be shown.


Thanks,
Dandan

> 
> Thanks,
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE

[Leif Lindholm]

On Mon, Oct 08, 2018 at 03:20:27PM +0530, Sumit Garg wrote:
> On Fri, 5 Oct 2018 at 20:33, Leif Lindholm  wrote:
> >
> > On Wed, Oct 03, 2018 at 11:33:01AM +0200, Ard Biesheuvel wrote:
> > > On 3 October 2018 at 09:43, Sumit Garg  wrote:
> > > > Add following APIs to communicate with OP-TEE pseudo/early TAs:
> > > > 1. OpteeInit
> > > > 2. OpteeOpenSession
> > > > 3. OpteeCloseSession
> > > > 4. OpteeInvokeFunc
> > > >
> > > > Cc: Ard Biesheuvel 
> > > > Cc: Leif Lindholm 
> > > > Cc: Michael D Kinney 
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Sumit Garg 
> > >
> > > Given the outcome of the GP discussion, I'm fine with this approach. Leif?
> >
> > Apologies for the delay, I needed some time to think it over.
> >
> > I'm not super happy about this approach, but I'm happier with this
> > than I would be with leaving the functionality out of the upstream
> > tree. I really hope we can get that license either changed or dropped
> > completely.
> >
> 
> Thanks Leif for this go ahead.
> 
> > I do have a few comments below.
> >
> > > > ---
> > > >  ArmPkg/Include/Library/OpteeLib.h|  90 +
> > > >  ArmPkg/Library/OpteeLib/Optee.c  | 357 
> > > > +++
> > > >  ArmPkg/Library/OpteeLib/OpteeLib.inf |   2 +
> > > >  ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +
> >
> > Could you follow the instructions in
> > https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
> > when generating future patches?
> > The --stat* effects aren't apparent here, but the -O ones are.
> >
> 
> Sure.
> 
> > > >  4 files changed, 492 insertions(+)
> > > >  create mode 100644 ArmPkg/Library/OpteeLib/OpteeSmc.h
> > > >
> > > > diff --git a/ArmPkg/Include/Library/OpteeLib.h 
> > > > b/ArmPkg/Include/Library/OpteeLib.h
> > > > index f65d8674d9b8..2d1c60632dfe 100644
> > > > --- a/ArmPkg/Include/Library/OpteeLib.h
> > > > +++ b/ArmPkg/Include/Library/OpteeLib.h
> > > > @@ -25,10 +25,100 @@
> > > >  #define OPTEE_OS_UID2  0xaf630002
> > > >  #define OPTEE_OS_UID3  0xa5d5c51b
> > > >
> > > > +#define OPTEE_MSG_ATTR_TYPE_NONE0x0
> > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INPUT 0x1
> > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT0x2
> > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INOUT 0x3
> > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_INPUT   0x9
> > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT  0xa
> > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_INOUT   0xb
> > > > +
> > > > +#define OPTEE_MSG_ATTR_TYPE_MASK0xff
> > > > +
> > > > +#define OPTEE_ORIGIN_COMMS  0x0002
> > > > +#define OPTEE_ERROR_COMMS   0x000E
> > > > +
> > > > +typedef struct {
> > > > +  UINT64BufPtr;
> >
> > If it's a pointer, it has a *.
> > Otherwise it's an address.
> >
> 
> Will rather use BufferAddress.
> 
> > > > +  UINT64Size;
> > > > +  UINT64ShmRef;
> >
> > Abbreviations in function, variable or type names (other than the ones
> > defined in [1] are not permitted unless they are explicitly added to a
> > glossary section of the source file header. Where possible, just write
> > out the name in full.
> >
> > [1] 
> > https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/v/release/2.20/4_naming_conventions/#table-2-efi-supported-abbreviations
> >
> > BufPtr (as a name) gets a pass since it's unambiguous and we already
> > have a bunch of those in the codebase. ShmRef needs to be clear.
> >
> 
> Will replace it with SharedMemoryReference.
> 
> > (This comment also applies to a lot of things below, I won't point
> > them all out unless asked to.)
> >
> 
> I will try to replace most of them with full names. But I have
> confusion regarding usage of following not included in [1]:
> 
> 1. MSG/Msg
> 2. MEM/Mem
> 3. INFO/Info
> 4. FUNC/Func
> 5. RET/Ret
> 6. ATTR/Attr
> 7. CMD/Cmd
> 
> But I do see their usage in the codebase. Please help to clarify.

It's a horrible secret, but not all of the codebase conforms to the
coding style. We're continuously trying to improve it.

> Also can I use 1-2 letter variable or struct member names like Mp, Ip,
> Op, U etc.?

Ideally not. I'm not a huge fan of ReallyLongCamelCase coding styles,
but the benefit it does bring is that you don't need to stop and think
about what a variable may be for. And correspondingly, mixing between
abbreviations an non-abbreviations make for hard reading.

Another completely unscientific observation I would make is that once
you force yourself to write a full variable name out, it becomes a lot
more clear when you've left valuable semantic information out.
The codebase ends up with a bunch of fewer variables called only
'Msg', 'Mem', 'Info', Func', 'Ret', 'Attr' 'Cmd' (, or 'Data' ...).
(Alternatively, the mailing list ends up with a lot fewer emails
asking "what message", "what memory

Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment variable

[Jim.Dailey]

>-Original Message-
>From: Ni, Ruiyu [mailto:ruiyu...@intel.com]
>Sent: Monday, October 8, 2018 1:42 AM
>To: Dailey, Jim; edk2-devel@lists.01.org
>Cc: jaben.car...@intel.com
>Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment 
>variable
>
>
>On 10/4/2018 12:02 AM, jim.dai...@dell.com wrote:
>> Create a homefilesystem environment variable whose value is the file
>> system on which the executing shell is located. For example: "FS14:".
>>
>Jim,
>Creating spec-undefined "homefilesystem" ENV variable is not a good idea
>in my opinion.
>Can you submit a Shell Spec change and change the implementation once
>the spec change is approved?

Ray,

I think you and Jaben should get together on this:

>>-Original Message-
>>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
>>Ersek
>>Sent: Thursday, October 4, 2018 12:07 PM
>>To: Carsey, Jaben; Dailey, Jim; edk2-devel@lists.01.org
>>Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment 
>>variable
>>
>>Is it OK with the UEFI shell spec to define a shell variable called
>>"homefilesystem"? I seem to remember that edk2-specific options for
>>standard UEFI shell commands generally start with an underscore, to
>>avoid clashing with the standard namespace. Does that apply to shell
>>variables perhaps? (This is mostly for my own education.)
>>
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>>> Carsey, Jaben
>>> Sent: Thursday, October 4, 2018 3:54 PM
>>> To: Andrew Fish; Laszlo Ersek
>>> Cc: edk2-devel@lists.01.org
>>> Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment 
>>> variable
>>>
>>> Laszlo,
>>>
>>> The leading "_" was required for out of spec, but built in, commands.
>>> The spec has no restrictions on environment variables except some have
>>> special meaning and may be read only.
>>>

Besides, if I were you and didn't have a lot of free time, I wouldn't unleash
me on shell spec changes! :-)

>--
>Thanks,
>Ray

Regards,
Jim
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/5] MdePkg/BaseSynchronizationLib GCC: fixes, cleanups

[Laszlo Ersek]

On 09/30/18 00:23, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: inline_asm_rw_ops_1208
> 
> This series mainly fixes the operand constraints (missing input-output
> qualifications) in "BaseSynchronizationLib/*/GccInline.c".
> 
> (It would be better to remove these files altogether in favor of the
> already existing NASM implementation, but due to
> , we can't generally
> do that in libraries yet.)
> 
> Cc: Liming Gao 
> Cc: Michael D Kinney 
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (5):
>   MdePkg/BaseSynchronizationLib GCC: fix whitespace and comments
>   MdePkg/BaseSynchronizationLib GCC: fix InternalSyncCompareExchange16()
>   MdePkg/BaseSynchronizationLib GCC: fix InternalSyncCompareExchange32()
>   MdePkg/BaseSynchronizationLib GCC: fix X64
> InternalSyncCompareExchange64()
>   MdePkg/BaseSynchronizationLib GCC: simplify IA32
> InternalSyncCompareExchange64()
> 
>  MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c | 42 +++--
>  MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c  | 47 
> +++-
>  2 files changed, 34 insertions(+), 55 deletions(-)
> 

Ping :)

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol

[Ard Biesheuvel]

On 8 October 2018 at 15:37, Marcin Wojtas  wrote:
> pon., 8 paź 2018 o 15:27 Ard Biesheuvel  
> napisał(a):
>>
>> On 8 October 2018 at 15:17, Marcin Wojtas  wrote:
>> > pon., 8 paź 2018 o 15:07 Ard Biesheuvel  
>> > napisał(a):
>> >>
>> >> On 8 October 2018 at 14:59, Marcin Wojtas  wrote:
>> >> > Hi Ard,
>> >> >
>> >> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel  
>> >> > napisał(a):
>> >> >>
>> >> >> (add MdeModulePkg maintainers)
>> >> >>
>> >> >> On 5 October 2018 at 15:25, Marcin Wojtas  wrote:
>> >> >> > From: Tomasz Michalec 
>> >> >> >
>> >> >> > Some SD Host Controlers use different values in Host Control 2 
>> >> >> > Register
>> >> >> > to select UHS Mode. This patch adds a new UhsSignaling type routine 
>> >> >> > to
>> >> >> > the NotifyPhase of the SdMmcOverride protocol.
>> >> >> >
>> >> >> > UHS signaling configuration is moved to a common, default routine
>> >> >> > (SdMmcHcUhsSignaling), which is called when SdMmcOverride does not
>> >> >> > cover this functionality.
>> >> >> >
>> >> >> > Contributed-under: TianoCore Contribution Agreement 1.1
>> >> >> > Signed-off-by: Marcin Wojtas 
>> >> >> > ---
>> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  50 +++
>> >> >> >  MdeModulePkg/Include/Protocol/SdMmcOverride.h|   2 +
>> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 153 
>> >> >> > 
>> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c|  37 +++--
>> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c |  69 +
>> >> >> >  5 files changed, 243 insertions(+), 68 deletions(-)
>> >> >> >
>> >> >> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h 
>> >> >> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> >> >> > index e389d52..a03160d 100644
>> >> >> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> >> >> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> >> >> > @@ -63,6 +63,39 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY 
>> >> >> > KIND, EITHER EXPRESS OR IMPLIED.
>> >> >> >  #define SD_MMC_HC_CTRL_VER0xFE
>> >> >> >
>> >> >> >  //
>> >> >> > +// SD Host Controler bits to HOST_CTRL2 register
>> >> >> > +//
>> >> >> > +#define SD_MMC_HC_CTRL_UHS_MASK   0x0007
>> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12  0x
>> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR25  0x0001
>> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR50  0x0002
>> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR104 0x0003
>> >> >> > +#define SD_MMC_HC_CTRL_UHS_DDR50  0x0004
>> >> >> > +#define SD_MMC_HC_CTRL_MMC_DDR52  0x0004
>> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR50  0x0002
>> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR25  0x0001
>> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR12  0x
>> >> >> > +#define SD_MMC_HC_CTRL_HS200  0x0003
>> >> >> > +#define SD_MMC_HC_CTRL_HS400  0x0005
>> >> >> > +
>
> In case we move enums to SdMmcOverride.h, would it be desired, to move
> there register fields values as well? Or should I rather use Xenon
> macros for all of above locally?
>

No, I think the macros should be kept locally.

>> >> >> > +//
>> >> >> > +// Timing modes for uhs
>> >> >> > +//
>> >> >> > +typedef enum {
>> >> >> > +  SdMmcUhsSdr12,
>> >> >> > +  SdMmcUhsSdr25,
>> >> >> > +  SdMmcUhsSdr50,
>> >> >> > +  SdMmcUhsSdr104,
>> >> >> > +  SdMmcUhsDdr50,
>> >> >> > +  SdMmcMmcDdr52,
>> >> >> > +  SdMmcMmcSdr50,
>> >> >> > +  SdMmcMmcSdr25,
>> >> >> > +  SdMmcMmcSdr12,
>> >> >> > +  SdMmcMmcHs200,
>> >> >> > +  SdMmcMmcHs400,
>> >> >> > +} SD_MMC_UHS_TIMING;
>> >> >> > +
>> >> >>
>> >> >> Here, we end up with two sets of symbolic constants for the same
>> >> >> thing, and I suppose this enum will be duplicated in your
>> >> >> SdMmcOverride implementation?
>> >> >>
>> >> >
>> >> > Why duplicated? Macros are for generic UHS_MODE_SEL field values for
>> >> > SD and MMC in HostControl2Register.
>> >> >
>> >> > SD_MMC_UHS_TIMING is just a timing mode indicator, it can be used not
>> >> > only in UhsSignaling routine (actually the next patch, with
>> >> > SwitchClockFreqPost, use it...).
>> >> >
>> >> > In my SdMmcOverride implementation this enum is not duplicated,
>> >> > because this file (SdMmcPciHci.h) is included via
>> >> > Protocol/SdMmcOverride.h.
>> >> >
>> >>
>> >> Ah ok. Please don't expose internal headers of the SD/MMC driver via
>> >> Protocol/SdMmcOverride.h
>> >>
>> >
>> > OK.
>> >
>> >> I think it should be fine to add the enum definition to
>> >> Protocol/SdMmcOverride.h instead.
>> >>
>> >
>> > OK.
>> >
>> >> But wouldn't it be much easier to have a hook for setting
>> >> HostControl2Register that decodes the value and modifies it according
>> >> to what the platform requires?
>> >>
>> >
>> > Can you please explain, how it will be different from UhsSignaling in
>> > current shape (read required timing value and update UHS_MODE_SEL
>> > field)?
>> >
>>
>> Well, you decode the value, and if, e.g., the SD_MMC_HC_CTRL_HS200
>> bits are set, you substitute them 

Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol

[Marcin Wojtas]

pon., 8 paź 2018 o 15:27 Ard Biesheuvel  napisał(a):
>
> On 8 October 2018 at 15:17, Marcin Wojtas  wrote:
> > pon., 8 paź 2018 o 15:07 Ard Biesheuvel  
> > napisał(a):
> >>
> >> On 8 October 2018 at 14:59, Marcin Wojtas  wrote:
> >> > Hi Ard,
> >> >
> >> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel  
> >> > napisał(a):
> >> >>
> >> >> (add MdeModulePkg maintainers)
> >> >>
> >> >> On 5 October 2018 at 15:25, Marcin Wojtas  wrote:
> >> >> > From: Tomasz Michalec 
> >> >> >
> >> >> > Some SD Host Controlers use different values in Host Control 2 
> >> >> > Register
> >> >> > to select UHS Mode. This patch adds a new UhsSignaling type routine to
> >> >> > the NotifyPhase of the SdMmcOverride protocol.
> >> >> >
> >> >> > UHS signaling configuration is moved to a common, default routine
> >> >> > (SdMmcHcUhsSignaling), which is called when SdMmcOverride does not
> >> >> > cover this functionality.
> >> >> >
> >> >> > Contributed-under: TianoCore Contribution Agreement 1.1
> >> >> > Signed-off-by: Marcin Wojtas 
> >> >> > ---
> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  50 +++
> >> >> >  MdeModulePkg/Include/Protocol/SdMmcOverride.h|   2 +
> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 153 
> >> >> > 
> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c|  37 +++--
> >> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c |  69 +
> >> >> >  5 files changed, 243 insertions(+), 68 deletions(-)
> >> >> >
> >> >> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h 
> >> >> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> >> >> > index e389d52..a03160d 100644
> >> >> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> >> >> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> >> >> > @@ -63,6 +63,39 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, 
> >> >> > EITHER EXPRESS OR IMPLIED.
> >> >> >  #define SD_MMC_HC_CTRL_VER0xFE
> >> >> >
> >> >> >  //
> >> >> > +// SD Host Controler bits to HOST_CTRL2 register
> >> >> > +//
> >> >> > +#define SD_MMC_HC_CTRL_UHS_MASK   0x0007
> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12  0x
> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR25  0x0001
> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR50  0x0002
> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR104 0x0003
> >> >> > +#define SD_MMC_HC_CTRL_UHS_DDR50  0x0004
> >> >> > +#define SD_MMC_HC_CTRL_MMC_DDR52  0x0004
> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR50  0x0002
> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR25  0x0001
> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR12  0x
> >> >> > +#define SD_MMC_HC_CTRL_HS200  0x0003
> >> >> > +#define SD_MMC_HC_CTRL_HS400  0x0005
> >> >> > +

In case we move enums to SdMmcOverride.h, would it be desired, to move
there register fields values as well? Or should I rather use Xenon
macros for all of above locally?

> >> >> > +//
> >> >> > +// Timing modes for uhs
> >> >> > +//
> >> >> > +typedef enum {
> >> >> > +  SdMmcUhsSdr12,
> >> >> > +  SdMmcUhsSdr25,
> >> >> > +  SdMmcUhsSdr50,
> >> >> > +  SdMmcUhsSdr104,
> >> >> > +  SdMmcUhsDdr50,
> >> >> > +  SdMmcMmcDdr52,
> >> >> > +  SdMmcMmcSdr50,
> >> >> > +  SdMmcMmcSdr25,
> >> >> > +  SdMmcMmcSdr12,
> >> >> > +  SdMmcMmcHs200,
> >> >> > +  SdMmcMmcHs400,
> >> >> > +} SD_MMC_UHS_TIMING;
> >> >> > +
> >> >>
> >> >> Here, we end up with two sets of symbolic constants for the same
> >> >> thing, and I suppose this enum will be duplicated in your
> >> >> SdMmcOverride implementation?
> >> >>
> >> >
> >> > Why duplicated? Macros are for generic UHS_MODE_SEL field values for
> >> > SD and MMC in HostControl2Register.
> >> >
> >> > SD_MMC_UHS_TIMING is just a timing mode indicator, it can be used not
> >> > only in UhsSignaling routine (actually the next patch, with
> >> > SwitchClockFreqPost, use it...).
> >> >
> >> > In my SdMmcOverride implementation this enum is not duplicated,
> >> > because this file (SdMmcPciHci.h) is included via
> >> > Protocol/SdMmcOverride.h.
> >> >
> >>
> >> Ah ok. Please don't expose internal headers of the SD/MMC driver via
> >> Protocol/SdMmcOverride.h
> >>
> >
> > OK.
> >
> >> I think it should be fine to add the enum definition to
> >> Protocol/SdMmcOverride.h instead.
> >>
> >
> > OK.
> >
> >> But wouldn't it be much easier to have a hook for setting
> >> HostControl2Register that decodes the value and modifies it according
> >> to what the platform requires?
> >>
> >
> > Can you please explain, how it will be different from UhsSignaling in
> > current shape (read required timing value and update UHS_MODE_SEL
> > field)?
> >
>
> Well, you decode the value, and if, e.g., the SD_MMC_HC_CTRL_HS200
> bits are set, you substitute them with the appropriate xenon values.

Because values can be same for SD and MMC (e.g. UHS_104 and HS200),
from the controller driver perspective, how would I know, which mode
is requested?

>
> Also, how important is it to drive the SD/MMC at its

Re: [edk2] [platforms: PATCH v2 5/7] Marvell/Armada80x0Db: Introduce board description library

[Ard Biesheuvel]

On 8 October 2018 at 15:28, Marcin Wojtas  wrote:
> Hi Ard,
>
> pon., 8 paź 2018 o 14:52 Ard Biesheuvel  
> napisał(a):
>>
>> On 5 October 2018 at 15:26, Marcin Wojtas  wrote:
>> > From: Tomasz Michalec 
>> >
>> > This patch implements ArmadaBoarDescLib library for
>> > Armada8040 Development Board and add to it ArmadaBoardDescSdMmcGet
>> > function with description of connected Xenon host controllers.
>> >
>> > Contributed-under: TianoCore Contribution Agreement 1.1
>> > Signed-off-by: Marcin Wojtas 
>> > ---
>> >  Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
>> >   |  3 +
>> >  
>> > Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
>> >  | 34 ++
>> >  
>> > Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
>> >| 66 
>> >  3 files changed, 103 insertions(+)
>> >  create mode 100644 
>> > Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
>> >  create mode 100644 
>> > Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
>> >
>> > diff --git a/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc 
>> > b/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
>> > index 92e2dc8..42f7bd3 100644
>> > --- a/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
>> > +++ b/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
>> > @@ -54,6 +54,9 @@
>> >  [Components.AARCH64]
>> >Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0Db.inf
>> >
>> > +[LibraryClasses.common]
>> > +  
>> > ArmadaBoardDescLib|Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
>> > +
>> >  
>> > 
>> >  #
>> >  # Pcd Section - list of all EDK II PCD Entries defined by this Platform
>> > diff --git 
>> > a/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
>> >  
>> > b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
>> > new file mode 100644
>> > index 000..2d39d96
>> > --- /dev/null
>> > +++ 
>> > b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
>> > @@ -0,0 +1,34 @@
>> > +## @file
>> > +#
>> > +#  Copyright (C) 2018, Marvell International Ltd. and its affiliates
>> > +#
>> > +#  This program and the accompanying materials are licensed and made 
>> > available
>> > +#  under the terms and conditions of the BSD License which accompanies 
>> > this
>> > +#  distribution. The full text of the license may be found at
>> > +#  http://opensource.org/licenses/bsd-license.php
>> > +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> > +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
>> > +#  IMPLIED.
>> > +#
>> > +#
>> > +##
>> > +
>> > +[Defines]
>> > +  INF_VERSION= 0x0001001A
>> > +  BASE_NAME  = Armada80x0DbBoardDescLib
>> > +  FILE_GUID  = fee9e874-1481-4b4f-9882-966bd0d1310f
>> > +  MODULE_TYPE= BASE
>> > +  VERSION_STRING = 1.0
>> > +  LIBRARY_CLASS  = ArmadaBoardDescLib
>> > +
>> > +[Sources]
>> > +  Armada80x0DbBoardDescLib.c
>> > +
>> > +[Packages]
>> > +  MdeModulePkg/MdeModulePkg.dec
>> > +  MdePkg/MdePkg.dec
>> > +  Silicon/Marvell/Marvell.dec
>> > +
>> > +[LibraryClasses]
>> > +  DebugLib
>> > +  IoLib
>> > diff --git 
>> > a/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
>> >  
>> > b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
>> > new file mode 100644
>> > index 000..00d696d
>> > --- /dev/null
>> > +++ 
>> > b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
>> > @@ -0,0 +1,66 @@
>> > +/**
>> > +*
>> > +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
>> > +*
>> > +*  This program and the accompanying materials are licensed and made 
>> > available
>> > +*  under the terms and conditions of the BSD License which accompanies 
>> > this
>> > +*  distribution. The full text of the license may be found at
>> > +*  http://opensource.org/licenses/bsd-license.php
>> > +*
>> > +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> > +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
>> > IMPLIED.
>> > +*
>> > +**/
>> > +
>> > +#include 
>> > +
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +
>> > +//
>> > +// Order of devices in SdMmcDescTemplate has to be in par with 
>> > ArmadaSoCDescLib
>> > +//
>> > +STATIC
>> > +MV_BOARD_SDMMC_DESC mSdMmcDescTemplate[] = {
>> > +  { /* eMMC 0xF06E */
>> > +0, /* SOC will be filled by MvBoardDescDxe */
>> > +0, /* SdMmcDevCount will be filled by MvBoardDescDxe */
>> > +TRUE,  /* Xenon1v8Enabled */
>> > + 

Re: [edk2] [platforms: PATCH v2 5/7] Marvell/Armada80x0Db: Introduce board description library

[Marcin Wojtas]

Hi Ard,

pon., 8 paź 2018 o 14:52 Ard Biesheuvel  napisał(a):
>
> On 5 October 2018 at 15:26, Marcin Wojtas  wrote:
> > From: Tomasz Michalec 
> >
> > This patch implements ArmadaBoarDescLib library for
> > Armada8040 Development Board and add to it ArmadaBoardDescSdMmcGet
> > function with description of connected Xenon host controllers.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas 
> > ---
> >  Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc 
> >  |  3 +
> >  
> > Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
> >  | 34 ++
> >  
> > Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
> >| 66 
> >  3 files changed, 103 insertions(+)
> >  create mode 100644 
> > Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
> >  create mode 100644 
> > Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
> >
> > diff --git a/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc 
> > b/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
> > index 92e2dc8..42f7bd3 100644
> > --- a/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
> > +++ b/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
> > @@ -54,6 +54,9 @@
> >  [Components.AARCH64]
> >Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0Db.inf
> >
> > +[LibraryClasses.common]
> > +  
> > ArmadaBoardDescLib|Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
> > +
> >  
> > 
> >  #
> >  # Pcd Section - list of all EDK II PCD Entries defined by this Platform
> > diff --git 
> > a/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
> >  
> > b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
> > new file mode 100644
> > index 000..2d39d96
> > --- /dev/null
> > +++ 
> > b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
> > @@ -0,0 +1,34 @@
> > +## @file
> > +#
> > +#  Copyright (C) 2018, Marvell International Ltd. and its affiliates
> > +#
> > +#  This program and the accompanying materials are licensed and made 
> > available
> > +#  under the terms and conditions of the BSD License which accompanies this
> > +#  distribution. The full text of the license may be found at
> > +#  http://opensource.org/licenses/bsd-license.php
> > +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> > +#  IMPLIED.
> > +#
> > +#
> > +##
> > +
> > +[Defines]
> > +  INF_VERSION= 0x0001001A
> > +  BASE_NAME  = Armada80x0DbBoardDescLib
> > +  FILE_GUID  = fee9e874-1481-4b4f-9882-966bd0d1310f
> > +  MODULE_TYPE= BASE
> > +  VERSION_STRING = 1.0
> > +  LIBRARY_CLASS  = ArmadaBoardDescLib
> > +
> > +[Sources]
> > +  Armada80x0DbBoardDescLib.c
> > +
> > +[Packages]
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  MdePkg/MdePkg.dec
> > +  Silicon/Marvell/Marvell.dec
> > +
> > +[LibraryClasses]
> > +  DebugLib
> > +  IoLib
> > diff --git 
> > a/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
> >  
> > b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
> > new file mode 100644
> > index 000..00d696d
> > --- /dev/null
> > +++ 
> > b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
> > @@ -0,0 +1,66 @@
> > +/**
> > +*
> > +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> > +*
> > +*  This program and the accompanying materials are licensed and made 
> > available
> > +*  under the terms and conditions of the BSD License which accompanies this
> > +*  distribution. The full text of the license may be found at
> > +*  http://opensource.org/licenses/bsd-license.php
> > +*
> > +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> > IMPLIED.
> > +*
> > +**/
> > +
> > +#include 
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +//
> > +// Order of devices in SdMmcDescTemplate has to be in par with 
> > ArmadaSoCDescLib
> > +//
> > +STATIC
> > +MV_BOARD_SDMMC_DESC mSdMmcDescTemplate[] = {
> > +  { /* eMMC 0xF06E */
> > +0, /* SOC will be filled by MvBoardDescDxe */
> > +0, /* SdMmcDevCount will be filled by MvBoardDescDxe */
> > +TRUE,  /* Xenon1v8Enabled */
> > +TRUE,  /* Xenon8BitBusEnabled */
> > +TRUE,  /* XenonSlowModeEnabled */
> > +0x40,  /* XenonTuningStepDivisor */
> > +EmbeddedSlot /* SlotType */
> > +  },
> > +  { /* SD/MMC 0xF278 */

Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol

[Ard Biesheuvel]

On 8 October 2018 at 15:17, Marcin Wojtas  wrote:
> pon., 8 paź 2018 o 15:07 Ard Biesheuvel  
> napisał(a):
>>
>> On 8 October 2018 at 14:59, Marcin Wojtas  wrote:
>> > Hi Ard,
>> >
>> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel  
>> > napisał(a):
>> >>
>> >> (add MdeModulePkg maintainers)
>> >>
>> >> On 5 October 2018 at 15:25, Marcin Wojtas  wrote:
>> >> > From: Tomasz Michalec 
>> >> >
>> >> > Some SD Host Controlers use different values in Host Control 2 Register
>> >> > to select UHS Mode. This patch adds a new UhsSignaling type routine to
>> >> > the NotifyPhase of the SdMmcOverride protocol.
>> >> >
>> >> > UHS signaling configuration is moved to a common, default routine
>> >> > (SdMmcHcUhsSignaling), which is called when SdMmcOverride does not
>> >> > cover this functionality.
>> >> >
>> >> > Contributed-under: TianoCore Contribution Agreement 1.1
>> >> > Signed-off-by: Marcin Wojtas 
>> >> > ---
>> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  50 +++
>> >> >  MdeModulePkg/Include/Protocol/SdMmcOverride.h|   2 +
>> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 153 
>> >> > 
>> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c|  37 +++--
>> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c |  69 +
>> >> >  5 files changed, 243 insertions(+), 68 deletions(-)
>> >> >
>> >> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h 
>> >> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> >> > index e389d52..a03160d 100644
>> >> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> >> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> >> > @@ -63,6 +63,39 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, 
>> >> > EITHER EXPRESS OR IMPLIED.
>> >> >  #define SD_MMC_HC_CTRL_VER0xFE
>> >> >
>> >> >  //
>> >> > +// SD Host Controler bits to HOST_CTRL2 register
>> >> > +//
>> >> > +#define SD_MMC_HC_CTRL_UHS_MASK   0x0007
>> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12  0x
>> >> > +#define SD_MMC_HC_CTRL_UHS_SDR25  0x0001
>> >> > +#define SD_MMC_HC_CTRL_UHS_SDR50  0x0002
>> >> > +#define SD_MMC_HC_CTRL_UHS_SDR104 0x0003
>> >> > +#define SD_MMC_HC_CTRL_UHS_DDR50  0x0004
>> >> > +#define SD_MMC_HC_CTRL_MMC_DDR52  0x0004
>> >> > +#define SD_MMC_HC_CTRL_MMC_SDR50  0x0002
>> >> > +#define SD_MMC_HC_CTRL_MMC_SDR25  0x0001
>> >> > +#define SD_MMC_HC_CTRL_MMC_SDR12  0x
>> >> > +#define SD_MMC_HC_CTRL_HS200  0x0003
>> >> > +#define SD_MMC_HC_CTRL_HS400  0x0005
>> >> > +
>> >> > +//
>> >> > +// Timing modes for uhs
>> >> > +//
>> >> > +typedef enum {
>> >> > +  SdMmcUhsSdr12,
>> >> > +  SdMmcUhsSdr25,
>> >> > +  SdMmcUhsSdr50,
>> >> > +  SdMmcUhsSdr104,
>> >> > +  SdMmcUhsDdr50,
>> >> > +  SdMmcMmcDdr52,
>> >> > +  SdMmcMmcSdr50,
>> >> > +  SdMmcMmcSdr25,
>> >> > +  SdMmcMmcSdr12,
>> >> > +  SdMmcMmcHs200,
>> >> > +  SdMmcMmcHs400,
>> >> > +} SD_MMC_UHS_TIMING;
>> >> > +
>> >>
>> >> Here, we end up with two sets of symbolic constants for the same
>> >> thing, and I suppose this enum will be duplicated in your
>> >> SdMmcOverride implementation?
>> >>
>> >
>> > Why duplicated? Macros are for generic UHS_MODE_SEL field values for
>> > SD and MMC in HostControl2Register.
>> >
>> > SD_MMC_UHS_TIMING is just a timing mode indicator, it can be used not
>> > only in UhsSignaling routine (actually the next patch, with
>> > SwitchClockFreqPost, use it...).
>> >
>> > In my SdMmcOverride implementation this enum is not duplicated,
>> > because this file (SdMmcPciHci.h) is included via
>> > Protocol/SdMmcOverride.h.
>> >
>>
>> Ah ok. Please don't expose internal headers of the SD/MMC driver via
>> Protocol/SdMmcOverride.h
>>
>
> OK.
>
>> I think it should be fine to add the enum definition to
>> Protocol/SdMmcOverride.h instead.
>>
>
> OK.
>
>> But wouldn't it be much easier to have a hook for setting
>> HostControl2Register that decodes the value and modifies it according
>> to what the platform requires?
>>
>
> Can you please explain, how it will be different from UhsSignaling in
> current shape (read required timing value and update UHS_MODE_SEL
> field)?
>

Well, you decode the value, and if, e.g., the SD_MMC_HC_CTRL_HS200
bits are set, you substitute them with the appropriate xenon values.

Also, how important is it to drive the SD/MMC at its max rated speed
at boot time? On Synquacer, I just disable HS200 in the capability
struct so I can forget about all this stuff


>>
>>
>> >>
>> >>
>> >> > +//
>> >> >  // The transfer modes supported by SD Host Controller
>> >> >  // Simplified Spec 3.0 Table 1-2
>> >> >  //
>> >> > @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
>> >> >IN UINT8  Slot
>> >> >);
>> >> >
>> >> > +/**
>> >> > +  Set SD Host Controler control 2 registry according to selected speed.
>> >> > +
>> >> > +  @param[in] PciIo  The PCI IO protocol instance.
>> >> > +  @param[in] Slot   The slot number of the SD c

Re: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."

[Jim.Dailey]

>-Original Message-
>From: Ni, Ruiyu [mailto:ruiyu...@intel.com] 
>Sent: Monday, October 8, 2018 2:00 AM
>To: Dailey, Jim; edk2-devel@lists.01.org
>Cc: michael.d.kin...@intel.com; liming@intel.com
>Subject: Re: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() error 
>involving "\..\.."
>
>
>On 10/4/2018 11:03 PM, jim.dai...@dell.com wrote:
>> MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."
>> 
>> The loop that removes "\..\" errs when multiple "\.." sequences are
>> in the path.  Before this change the code would modify a path like
>> "FS0:\efi\tools\..\.." to "FS0:\efi\\.." and then to "FS0:\efi\", but
>> the correct path is "FS0:\".
>> 
>> You can test the effect of this change in the shell by setting the
>> current directory to something like FS0:\efi\boot and then executing
>> the command "ls ..\..".  Before the change you will see the files in
>> the FS0:\efi directory; after the change, you will see the files in
>> the root directory of FS0:.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jim Dailey 
>> ---
>>   MdePkg/Library/BaseLib/FilePaths.c | 5 -
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/MdePkg/Library/BaseLib/FilePaths.c 
>> b/MdePkg/Library/BaseLib/FilePaths.c
>> index d6f3758ecb..5d3de01894 100644
>> --- a/MdePkg/Library/BaseLib/FilePaths.c
>> +++ b/MdePkg/Library/BaseLib/FilePaths.c
>> @@ -2,6 +2,7 @@
>> Defines file-path manipulation functions.
>>   
>> Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.
>> +  Copyright (c) 2018, Dell Technologies. All rights reserved.
>> This program and the accompanying materials
>> are licensed and made available under the terms and conditions of the 
>> BSD License
>> which accompanies this distribution.  The full text of the license may 
>> be found at
>> @@ -103,7 +104,9 @@ PathCleanUpDirectories(
>>   ) {
>>   *(TempString + 1) = CHAR_NULL;
>>   PathRemoveLastItem(Path);
>> -CopyMem (Path + StrLen (Path), TempString + 3, StrSize (TempString + 
>> 3));
>> +if (*(TempString + 3)) {
>> +  CopyMem (Path + StrLen (Path), TempString + 4, StrSize (TempString + 
>> 4));
>> +}
>> }
>>   
>> //
>> 
>Jim,
>Are you fixing a corner case bug introduced by following commit:
>
> > SHA-1: bb99e3282c9e69fbd6365d117c58d15589e34c5d
>* MdePkg/BaseLib: Fix PathCleanUpDirectories to correctly handle "\.\"
>
> > The old code incorrectly cleans path like "fs0:\abc\.\.." to
> > "fs0:\abc", instead of "fs0:\"
>
> > The patch fixes this bug.

Honestly, because of the scope of bb99e328, I cannot say for sure, but
my inclination is to answer "No."

The current fix is strictly related to handling paths that include two
or more references to the previous directory, whether or not one of them
abuts a reference to the current directory (as in the bb99e328 fix).

Regards,
Jim
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol

[Marcin Wojtas]

pon., 8 paź 2018 o 15:07 Ard Biesheuvel  napisał(a):
>
> On 8 October 2018 at 14:59, Marcin Wojtas  wrote:
> > Hi Ard,
> >
> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel  
> > napisał(a):
> >>
> >> (add MdeModulePkg maintainers)
> >>
> >> On 5 October 2018 at 15:25, Marcin Wojtas  wrote:
> >> > From: Tomasz Michalec 
> >> >
> >> > Some SD Host Controlers use different values in Host Control 2 Register
> >> > to select UHS Mode. This patch adds a new UhsSignaling type routine to
> >> > the NotifyPhase of the SdMmcOverride protocol.
> >> >
> >> > UHS signaling configuration is moved to a common, default routine
> >> > (SdMmcHcUhsSignaling), which is called when SdMmcOverride does not
> >> > cover this functionality.
> >> >
> >> > Contributed-under: TianoCore Contribution Agreement 1.1
> >> > Signed-off-by: Marcin Wojtas 
> >> > ---
> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  50 +++
> >> >  MdeModulePkg/Include/Protocol/SdMmcOverride.h|   2 +
> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 153 
> >> > 
> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c|  37 +++--
> >> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c |  69 +
> >> >  5 files changed, 243 insertions(+), 68 deletions(-)
> >> >
> >> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h 
> >> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> >> > index e389d52..a03160d 100644
> >> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> >> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> >> > @@ -63,6 +63,39 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, 
> >> > EITHER EXPRESS OR IMPLIED.
> >> >  #define SD_MMC_HC_CTRL_VER0xFE
> >> >
> >> >  //
> >> > +// SD Host Controler bits to HOST_CTRL2 register
> >> > +//
> >> > +#define SD_MMC_HC_CTRL_UHS_MASK   0x0007
> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12  0x
> >> > +#define SD_MMC_HC_CTRL_UHS_SDR25  0x0001
> >> > +#define SD_MMC_HC_CTRL_UHS_SDR50  0x0002
> >> > +#define SD_MMC_HC_CTRL_UHS_SDR104 0x0003
> >> > +#define SD_MMC_HC_CTRL_UHS_DDR50  0x0004
> >> > +#define SD_MMC_HC_CTRL_MMC_DDR52  0x0004
> >> > +#define SD_MMC_HC_CTRL_MMC_SDR50  0x0002
> >> > +#define SD_MMC_HC_CTRL_MMC_SDR25  0x0001
> >> > +#define SD_MMC_HC_CTRL_MMC_SDR12  0x
> >> > +#define SD_MMC_HC_CTRL_HS200  0x0003
> >> > +#define SD_MMC_HC_CTRL_HS400  0x0005
> >> > +
> >> > +//
> >> > +// Timing modes for uhs
> >> > +//
> >> > +typedef enum {
> >> > +  SdMmcUhsSdr12,
> >> > +  SdMmcUhsSdr25,
> >> > +  SdMmcUhsSdr50,
> >> > +  SdMmcUhsSdr104,
> >> > +  SdMmcUhsDdr50,
> >> > +  SdMmcMmcDdr52,
> >> > +  SdMmcMmcSdr50,
> >> > +  SdMmcMmcSdr25,
> >> > +  SdMmcMmcSdr12,
> >> > +  SdMmcMmcHs200,
> >> > +  SdMmcMmcHs400,
> >> > +} SD_MMC_UHS_TIMING;
> >> > +
> >>
> >> Here, we end up with two sets of symbolic constants for the same
> >> thing, and I suppose this enum will be duplicated in your
> >> SdMmcOverride implementation?
> >>
> >
> > Why duplicated? Macros are for generic UHS_MODE_SEL field values for
> > SD and MMC in HostControl2Register.
> >
> > SD_MMC_UHS_TIMING is just a timing mode indicator, it can be used not
> > only in UhsSignaling routine (actually the next patch, with
> > SwitchClockFreqPost, use it...).
> >
> > In my SdMmcOverride implementation this enum is not duplicated,
> > because this file (SdMmcPciHci.h) is included via
> > Protocol/SdMmcOverride.h.
> >
>
> Ah ok. Please don't expose internal headers of the SD/MMC driver via
> Protocol/SdMmcOverride.h
>

OK.

> I think it should be fine to add the enum definition to
> Protocol/SdMmcOverride.h instead.
>

OK.

> But wouldn't it be much easier to have a hook for setting
> HostControl2Register that decodes the value and modifies it according
> to what the platform requires?
>

Can you please explain, how it will be different from UhsSignaling in
current shape (read required timing value and update UHS_MODE_SEL
field)?

Thanks,
Marcin

>
>
> >>
> >>
> >> > +//
> >> >  // The transfer modes supported by SD Host Controller
> >> >  // Simplified Spec 3.0 Table 1-2
> >> >  //
> >> > @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
> >> >IN UINT8  Slot
> >> >);
> >> >
> >> > +/**
> >> > +  Set SD Host Controler control 2 registry according to selected speed.
> >> > +
> >> > +  @param[in] PciIo  The PCI IO protocol instance.
> >> > +  @param[in] Slot   The slot number of the SD card to send the 
> >> > command to.
> >> > +  @param[in] Timing The timing to select.
> >> > +
> >> > +  @retval EFI_SUCCESS   The timing is set successfully.
> >> > +  @retval OthersThe timing isn't set successfully.
> >> > +**/
> >> > +EFI_STATUS
> >> > +SdMmcHcUhsSignaling (
> >> > +  IN EFI_PCI_IO_PROTOCOL*PciIo,
> >> > +  IN UINT8  Slot,
> >> > +  IN SD_MMC_UHS_TIMING  Timing
> >> > +  );
> >> > +
> >> >  #endif
> >> > diff --git a/MdeM

Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol

[Ard Biesheuvel]

On 8 October 2018 at 14:59, Marcin Wojtas  wrote:
> Hi Ard,
>
> pon., 8 paź 2018 o 14:41 Ard Biesheuvel  
> napisał(a):
>>
>> (add MdeModulePkg maintainers)
>>
>> On 5 October 2018 at 15:25, Marcin Wojtas  wrote:
>> > From: Tomasz Michalec 
>> >
>> > Some SD Host Controlers use different values in Host Control 2 Register
>> > to select UHS Mode. This patch adds a new UhsSignaling type routine to
>> > the NotifyPhase of the SdMmcOverride protocol.
>> >
>> > UHS signaling configuration is moved to a common, default routine
>> > (SdMmcHcUhsSignaling), which is called when SdMmcOverride does not
>> > cover this functionality.
>> >
>> > Contributed-under: TianoCore Contribution Agreement 1.1
>> > Signed-off-by: Marcin Wojtas 
>> > ---
>> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  50 +++
>> >  MdeModulePkg/Include/Protocol/SdMmcOverride.h|   2 +
>> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 153 
>> > 
>> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c|  37 +++--
>> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c |  69 +
>> >  5 files changed, 243 insertions(+), 68 deletions(-)
>> >
>> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h 
>> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> > index e389d52..a03160d 100644
>> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> > @@ -63,6 +63,39 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, 
>> > EITHER EXPRESS OR IMPLIED.
>> >  #define SD_MMC_HC_CTRL_VER0xFE
>> >
>> >  //
>> > +// SD Host Controler bits to HOST_CTRL2 register
>> > +//
>> > +#define SD_MMC_HC_CTRL_UHS_MASK   0x0007
>> > +#define SD_MMC_HC_CTRL_UHS_SDR12  0x
>> > +#define SD_MMC_HC_CTRL_UHS_SDR25  0x0001
>> > +#define SD_MMC_HC_CTRL_UHS_SDR50  0x0002
>> > +#define SD_MMC_HC_CTRL_UHS_SDR104 0x0003
>> > +#define SD_MMC_HC_CTRL_UHS_DDR50  0x0004
>> > +#define SD_MMC_HC_CTRL_MMC_DDR52  0x0004
>> > +#define SD_MMC_HC_CTRL_MMC_SDR50  0x0002
>> > +#define SD_MMC_HC_CTRL_MMC_SDR25  0x0001
>> > +#define SD_MMC_HC_CTRL_MMC_SDR12  0x
>> > +#define SD_MMC_HC_CTRL_HS200  0x0003
>> > +#define SD_MMC_HC_CTRL_HS400  0x0005
>> > +
>> > +//
>> > +// Timing modes for uhs
>> > +//
>> > +typedef enum {
>> > +  SdMmcUhsSdr12,
>> > +  SdMmcUhsSdr25,
>> > +  SdMmcUhsSdr50,
>> > +  SdMmcUhsSdr104,
>> > +  SdMmcUhsDdr50,
>> > +  SdMmcMmcDdr52,
>> > +  SdMmcMmcSdr50,
>> > +  SdMmcMmcSdr25,
>> > +  SdMmcMmcSdr12,
>> > +  SdMmcMmcHs200,
>> > +  SdMmcMmcHs400,
>> > +} SD_MMC_UHS_TIMING;
>> > +
>>
>> Here, we end up with two sets of symbolic constants for the same
>> thing, and I suppose this enum will be duplicated in your
>> SdMmcOverride implementation?
>>
>
> Why duplicated? Macros are for generic UHS_MODE_SEL field values for
> SD and MMC in HostControl2Register.
>
> SD_MMC_UHS_TIMING is just a timing mode indicator, it can be used not
> only in UhsSignaling routine (actually the next patch, with
> SwitchClockFreqPost, use it...).
>
> In my SdMmcOverride implementation this enum is not duplicated,
> because this file (SdMmcPciHci.h) is included via
> Protocol/SdMmcOverride.h.
>

Ah ok. Please don't expose internal headers of the SD/MMC driver via
Protocol/SdMmcOverride.h

I think it should be fine to add the enum definition to
Protocol/SdMmcOverride.h instead.

But wouldn't it be much easier to have a hook for setting
HostControl2Register that decodes the value and modifies it according
to what the platform requires?



>>
>>
>> > +//
>> >  // The transfer modes supported by SD Host Controller
>> >  // Simplified Spec 3.0 Table 1-2
>> >  //
>> > @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
>> >IN UINT8  Slot
>> >);
>> >
>> > +/**
>> > +  Set SD Host Controler control 2 registry according to selected speed.
>> > +
>> > +  @param[in] PciIo  The PCI IO protocol instance.
>> > +  @param[in] Slot   The slot number of the SD card to send the 
>> > command to.
>> > +  @param[in] Timing The timing to select.
>> > +
>> > +  @retval EFI_SUCCESS   The timing is set successfully.
>> > +  @retval OthersThe timing isn't set successfully.
>> > +**/
>> > +EFI_STATUS
>> > +SdMmcHcUhsSignaling (
>> > +  IN EFI_PCI_IO_PROTOCOL*PciIo,
>> > +  IN UINT8  Slot,
>> > +  IN SD_MMC_UHS_TIMING  Timing
>> > +  );
>> > +
>> >  #endif
>> > diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h 
>> > b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> > index 178945f..25db98a 100644
>> > --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> > +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> > @@ -17,6 +17,7 @@
>> >  #ifndef __SD_MMC_OVERRIDE_H__
>> >  #define __SD_MMC_OVERRIDE_H__
>> >
>> > +#include 
>> >  #include 
>> >
>> >  #define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
>> > @@ -31,6 +32,7 @@ typedef enum {
>> >EdkiiSd

Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol

[Marcin Wojtas]

Hi Ard,

pon., 8 paź 2018 o 14:41 Ard Biesheuvel  napisał(a):
>
> (add MdeModulePkg maintainers)
>
> On 5 October 2018 at 15:25, Marcin Wojtas  wrote:
> > From: Tomasz Michalec 
> >
> > Some SD Host Controlers use different values in Host Control 2 Register
> > to select UHS Mode. This patch adds a new UhsSignaling type routine to
> > the NotifyPhase of the SdMmcOverride protocol.
> >
> > UHS signaling configuration is moved to a common, default routine
> > (SdMmcHcUhsSignaling), which is called when SdMmcOverride does not
> > cover this functionality.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas 
> > ---
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  50 +++
> >  MdeModulePkg/Include/Protocol/SdMmcOverride.h|   2 +
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 153 
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c|  37 +++--
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c |  69 +
> >  5 files changed, 243 insertions(+), 68 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h 
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > index e389d52..a03160d 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > @@ -63,6 +63,39 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, 
> > EITHER EXPRESS OR IMPLIED.
> >  #define SD_MMC_HC_CTRL_VER0xFE
> >
> >  //
> > +// SD Host Controler bits to HOST_CTRL2 register
> > +//
> > +#define SD_MMC_HC_CTRL_UHS_MASK   0x0007
> > +#define SD_MMC_HC_CTRL_UHS_SDR12  0x
> > +#define SD_MMC_HC_CTRL_UHS_SDR25  0x0001
> > +#define SD_MMC_HC_CTRL_UHS_SDR50  0x0002
> > +#define SD_MMC_HC_CTRL_UHS_SDR104 0x0003
> > +#define SD_MMC_HC_CTRL_UHS_DDR50  0x0004
> > +#define SD_MMC_HC_CTRL_MMC_DDR52  0x0004
> > +#define SD_MMC_HC_CTRL_MMC_SDR50  0x0002
> > +#define SD_MMC_HC_CTRL_MMC_SDR25  0x0001
> > +#define SD_MMC_HC_CTRL_MMC_SDR12  0x
> > +#define SD_MMC_HC_CTRL_HS200  0x0003
> > +#define SD_MMC_HC_CTRL_HS400  0x0005
> > +
> > +//
> > +// Timing modes for uhs
> > +//
> > +typedef enum {
> > +  SdMmcUhsSdr12,
> > +  SdMmcUhsSdr25,
> > +  SdMmcUhsSdr50,
> > +  SdMmcUhsSdr104,
> > +  SdMmcUhsDdr50,
> > +  SdMmcMmcDdr52,
> > +  SdMmcMmcSdr50,
> > +  SdMmcMmcSdr25,
> > +  SdMmcMmcSdr12,
> > +  SdMmcMmcHs200,
> > +  SdMmcMmcHs400,
> > +} SD_MMC_UHS_TIMING;
> > +
>
> Here, we end up with two sets of symbolic constants for the same
> thing, and I suppose this enum will be duplicated in your
> SdMmcOverride implementation?
>

Why duplicated? Macros are for generic UHS_MODE_SEL field values for
SD and MMC in HostControl2Register.

SD_MMC_UHS_TIMING is just a timing mode indicator, it can be used not
only in UhsSignaling routine (actually the next patch, with
SwitchClockFreqPost, use it...).

In my SdMmcOverride implementation this enum is not duplicated,
because this file (SdMmcPciHci.h) is included via
Protocol/SdMmcOverride.h.

>
>
> > +//
> >  // The transfer modes supported by SD Host Controller
> >  // Simplified Spec 3.0 Table 1-2
> >  //
> > @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
> >IN UINT8  Slot
> >);
> >
> > +/**
> > +  Set SD Host Controler control 2 registry according to selected speed.
> > +
> > +  @param[in] PciIo  The PCI IO protocol instance.
> > +  @param[in] Slot   The slot number of the SD card to send the 
> > command to.
> > +  @param[in] Timing The timing to select.
> > +
> > +  @retval EFI_SUCCESS   The timing is set successfully.
> > +  @retval OthersThe timing isn't set successfully.
> > +**/
> > +EFI_STATUS
> > +SdMmcHcUhsSignaling (
> > +  IN EFI_PCI_IO_PROTOCOL*PciIo,
> > +  IN UINT8  Slot,
> > +  IN SD_MMC_UHS_TIMING  Timing
> > +  );
> > +
> >  #endif
> > diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h 
> > b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > index 178945f..25db98a 100644
> > --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > @@ -17,6 +17,7 @@
> >  #ifndef __SD_MMC_OVERRIDE_H__
> >  #define __SD_MMC_OVERRIDE_H__
> >
> > +#include 
> >  #include 
> >
> >  #define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
> > @@ -31,6 +32,7 @@ typedef enum {
> >EdkiiSdMmcResetPost,
> >EdkiiSdMmcInitHostPre,
> >EdkiiSdMmcInitHostPost,
> > +  EdkiiSdMmcUhsSignaling,
> >  } EDKII_SD_MMC_PHASE_TYPE;
> >
> >  /**
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c 
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > index c5fd214..05bd4a0 100755
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > @@ -740,10 +740,13 @@ EmmcSwitchToHighSpeed (
> >IN UINT8  BusWidth
> >)
> >  {
> > -  

Re: [edk2] [platforms: PATCH v2 5/7] Marvell/Armada80x0Db: Introduce board description library

[Ard Biesheuvel]

On 5 October 2018 at 15:26, Marcin Wojtas  wrote:
> From: Tomasz Michalec 
>
> This patch implements ArmadaBoarDescLib library for
> Armada8040 Development Board and add to it ArmadaBoardDescSdMmcGet
> function with description of connected Xenon host controllers.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc   
>|  3 +
>  
> Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
>  | 34 ++
>  
> Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
>| 66 
>  3 files changed, 103 insertions(+)
>  create mode 100644 
> Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
>  create mode 100644 
> Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
>
> diff --git a/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc 
> b/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
> index 92e2dc8..42f7bd3 100644
> --- a/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
> +++ b/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
> @@ -54,6 +54,9 @@
>  [Components.AARCH64]
>Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0Db.inf
>
> +[LibraryClasses.common]
> +  
> ArmadaBoardDescLib|Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
> +
>  
> 
>  #
>  # Pcd Section - list of all EDK II PCD Entries defined by this Platform
> diff --git 
> a/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
>  
> b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
> new file mode 100644
> index 000..2d39d96
> --- /dev/null
> +++ 
> b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
> @@ -0,0 +1,34 @@
> +## @file
> +#
> +#  Copyright (C) 2018, Marvell International Ltd. and its affiliates
> +#
> +#  This program and the accompanying materials are licensed and made 
> available
> +#  under the terms and conditions of the BSD License which accompanies this
> +#  distribution. The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> +#  IMPLIED.
> +#
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION= 0x0001001A
> +  BASE_NAME  = Armada80x0DbBoardDescLib
> +  FILE_GUID  = fee9e874-1481-4b4f-9882-966bd0d1310f
> +  MODULE_TYPE= BASE
> +  VERSION_STRING = 1.0
> +  LIBRARY_CLASS  = ArmadaBoardDescLib
> +
> +[Sources]
> +  Armada80x0DbBoardDescLib.c
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  IoLib
> diff --git 
> a/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
>  
> b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
> new file mode 100644
> index 000..00d696d
> --- /dev/null
> +++ 
> b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
> @@ -0,0 +1,66 @@
> +/**
> +*
> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> +*
> +*  This program and the accompanying materials are licensed and made 
> available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution. The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +//
> +// Order of devices in SdMmcDescTemplate has to be in par with 
> ArmadaSoCDescLib
> +//
> +STATIC
> +MV_BOARD_SDMMC_DESC mSdMmcDescTemplate[] = {
> +  { /* eMMC 0xF06E */
> +0, /* SOC will be filled by MvBoardDescDxe */
> +0, /* SdMmcDevCount will be filled by MvBoardDescDxe */
> +TRUE,  /* Xenon1v8Enabled */
> +TRUE,  /* Xenon8BitBusEnabled */
> +TRUE,  /* XenonSlowModeEnabled */
> +0x40,  /* XenonTuningStepDivisor */
> +EmbeddedSlot /* SlotType */
> +  },
> +  { /* SD/MMC 0xF278 */
> +0, /* SOC will be filled by MvBoardDescDxe */
> +0, /* SdMmcDevCount will be filled by MvBoardDescDxe */
> +FALSE, /* Xenon1v8Enabled */
> +FALSE, /* Xenon8BitBusEnabled */
> +FALSE, /* XenonSlowModeEnabled */
> +0x19,  /* XenonTuningStepDivisor */
> +EmbeddedSlot /* SlotType */

Isn't the SD remo

Re: [edk2] [platforms: PATCH v2 1/7] Silicon/SynQuacer/PlatformDxe: adjust to updated SdMmcOverride

[Ard Biesheuvel]

On 5 October 2018 at 15:26, Marcin Wojtas  wrote:
> The newest changes in the SdMmcOverride protocol added additional
> arguments to the NotifyPhase and Capability routines. Update
> according places in the Synquacer Emmc driver.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c 
> b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> index e0987c9..0a917a5 100644
> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> @@ -72,6 +72,8 @@ STATIC VOID *mEventRegistration;
>@param[in]  ControllerHandle  The EFI_HANDLE of the controller.
>@param[in]  Slot  The 0 based slot index.
>@param[in,out]  SdMmcHcSlotCapability The SDHCI capability structure.
> +  @param[in,out]  BaseClkFreq   The base clock frequency value that
> +optionally can be updated.
>
>@retval EFI_SUCCESS   The override function completed successfully.
>@retval EFI_NOT_FOUND The specified controller or slot does not 
> exist.
> @@ -84,7 +86,8 @@ EFIAPI
>  SynQuacerSdMmcCapability (
>IN  EFI_HANDLE  ControllerHandle,
>IN  UINT8   Slot,
> -  IN  OUT VOID*SdMmcHcSlotCapability
> +  IN OUT  VOID*SdMmcHcSlotCapability,
> +  IN OUT  UINT32  *BaseClkFreq
>)
>  {
>UINT64 Capability;
> @@ -117,6 +120,7 @@ SynQuacerSdMmcCapability (
>@param[in]  PhaseType The type of operation and whether the
>  hook is invoked right before (pre) or
>  right after (post)
> +  @param[in,out]  Data  The pointer to a phase-specific data.
>
>@retval EFI_SUCCESS   The override function completed successfully.
>@retval EFI_NOT_FOUND The specified controller or slot does not 
> exist.
> @@ -129,7 +133,8 @@ EFIAPI
>  SynQuacerSdMmcNotifyPhase (
>IN  EFI_HANDLE  ControllerHandle,
>IN  UINT8   Slot,
> -  IN  EDKII_SD_MMC_PHASE_TYPE PhaseType
> +  IN  EDKII_SD_MMC_PHASE_TYPE PhaseType,
> +  IN OUT  VOID*Data

Please align the name with the EDK2 changes.

>)
>  {
>if (ControllerHandle != mSdMmcControllerHandle) {
> --
> 2.7.4
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency

[Ard Biesheuvel]

(add MdeModulePkg maintainers)

On 5 October 2018 at 15:25, Marcin Wojtas  wrote:
> Some SdMmc host controllers are run by clocks with different
> frequency than it is reflected in Capabilities Register 1.
> It is allowed by SDHCI specification ver. 4.2 - if BaseClkFreq
> field value of the Capability Register 1 is zero, the clock
> frequency must be obtained via another method.
>
> Because the bitfield is only 8 bits wide, a maximum value
> that could be obtained from hardware is 255MHz.
> In case the actual frequency exceeds 255MHz, the 8-bit BaseClkFreq
> member of SD_MMC_HC_SLOT_CAP structure occurs to be not sufficient
> to be used for setting the clock speed in SdMmcHcClockSupply
> function.
>
> This patch adds new UINT32 array ('BaseClkFreq[]') to
> SD_MMC_HC_PRIVATE_DATA structure for specifying
> the input clock speed for each slot of the host controller.
> All routines that are used for clock configuration are
> updated accordingly.
>
> This patch also adds new IN OUT BaseClockFreq field
> in the Capability callback of the SdMmcOverride,
> protocol which allows to update BaseClkFreq value.
>
> The patch reuses original commit from edk2-platforms:
> 20f6f144d3a8 ("Marvell/Drivers: XenonDxe: Allow overriding base clock
> frequency")
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |  6 +
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   | 12 +
>  MdeModulePkg/Include/Protocol/SdMmcOverride.h  |  5 +++-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c|  4 +--
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c  |  4 +--
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 18 +++--
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 27 +++-
>  7 files changed, 52 insertions(+), 24 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h 
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> index c683600..8c1a589 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> @@ -118,6 +118,12 @@ typedef struct {
>UINT64  MaxCurrent[SD_MMC_HC_MAX_SLOT];
>
>UINT32  ControllerVersion;
> +
> +  //
> +  // Some controllers may require to override base clock frequency
> +  // value stored in Capabilities Register 1.
> +  //
> +  UINT32  BaseClkFreq[SD_MMC_HC_MAX_SLOT];
>  } SD_MMC_HC_PRIVATE_DATA;
>
>  #define SD_MMC_HC_TRB_SIG SIGNATURE_32 ('T', 'R', 'B', 'T')
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h 
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index a03160d..f01ba21 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -173,12 +173,14 @@ typedef struct {
>
>@param[in]  SlotThe slot number of the SD card to send the 
> command to.
>@param[in]  Capability  The buffer to store the capability data.
> +  @param[in]  BaseClkFreq The base clock frequency of host controller in 
> MHz.
>
>  **/
>  VOID
>  DumpCapabilityReg (
>IN UINT8Slot,
> -  IN SD_MMC_HC_SLOT_CAP   *Capability
> +  IN SD_MMC_HC_SLOT_CAP   *Capability,
> +  IN UINT32   BaseClkFreq
>);
>
>  /**
> @@ -431,7 +433,7 @@ SdMmcHcStopClock (
>@param[in] PciIo  The PCI IO protocol instance.
>@param[in] Slot   The slot number of the SD card to send the 
> command to.
>@param[in] ClockFreq  The max clock frequency to be set. The unit is 
> KHz.
> -  @param[in] Capability The capability of the slot.
> +  @param[in] BaseClkFreqThe base clock frequency of host controller in 
> MHz.
>
>@retval EFI_SUCCESS   The clock is supplied successfully.
>@retval OthersThe clock isn't supplied successfully.
> @@ -442,7 +444,7 @@ SdMmcHcClockSupply (
>IN EFI_PCI_IO_PROTOCOL*PciIo,
>IN UINT8  Slot,
>IN UINT64 ClockFreq,
> -  IN SD_MMC_HC_SLOT_CAP Capability
> +  IN UINT32 BaseClkFreq
>);
>
>  /**
> @@ -490,7 +492,7 @@ SdMmcHcSetBusWidth (
>
>@param[in] PciIo  The PCI IO protocol instance.
>@param[in] Slot   The slot number of the SD card to send the 
> command to.
> -  @param[in] Capability The capability of the slot.
> +  @param[in] BaseClkFreqThe base clock frequency of host controller in 
> MHz.
>
>@retval EFI_SUCCESS   The clock is supplied successfully.
>@retval OthersThe clock isn't supplied successfully.
> @@ -500,7 +502,7 @@ EFI_STATUS
>  SdMmcHcInitClockFreq (
>IN EFI_PCI_IO_PROTOCOL*PciIo,
>IN UINT8  Slot,
> -  IN SD_MMC_HC_SLOT_CAP Capability
> +  IN UINT32 BaseClkFreq
>);
>
>  /**
> diff --git a/MdeModuleP

Re: [edk2] [PATCH v2 3/4] MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride

[Ard Biesheuvel]

(add MdeModulePkg maintainers)

On 5 October 2018 at 15:25, Marcin Wojtas  wrote:
> From: Tomasz Michalec 
>
> Some SD Host Controlers need to do additional opperations after clock
> frequency switch.
>

controllers
operations

Otherwise, this looks fine to me

Reviewed-by: Ard Biesheuvel 

> This patch add new callback type to NotifyPhase of the SdMmcOverride
> protocol. It is called after EmmcSwitchClockFreq and SdMmcHcClockSupply.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  MdeModulePkg/Include/Protocol/SdMmcOverride.h   |  1 +
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 60 
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c   | 18 ++
>  3 files changed, 79 insertions(+)
>
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h 
> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> index 25db98a..d9daada 100644
> --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -33,6 +33,7 @@ typedef enum {
>EdkiiSdMmcInitHostPre,
>EdkiiSdMmcInitHostPost,
>EdkiiSdMmcUhsSignaling,
> +  EdkiiSdMmcSwitchClockFreqPost,
>  } EDKII_SD_MMC_PHASE_TYPE;
>
>  /**
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c 
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index 05bd4a0..7e75283 100755
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -796,6 +796,27 @@ EmmcSwitchToHighSpeed (
>
>HsTiming = 1;
>Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, 
> ClockFreq);
> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +Status = mOverride->NotifyPhase (
> +  Private->ControllerHandle,
> +  Slot,
> +  EdkiiSdMmcSwitchClockFreqPost,
> +  &Timing
> +  );
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((
> +DEBUG_ERROR,
> +"%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> +__FUNCTION__,
> +Status
> +));
> +  return Status;
> +}
> +  }
>
>return Status;
>  }
> @@ -905,6 +926,24 @@ EmmcSwitchToHS200 (
>  return Status;
>}
>
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +Status = mOverride->NotifyPhase (
> +  Private->ControllerHandle,
> +  Slot,
> +  EdkiiSdMmcSwitchClockFreqPost,
> +  &Timing
> +  );
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((
> +DEBUG_ERROR,
> +"%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> +__FUNCTION__,
> +Status
> +));
> +  return Status;
> +}
> +  }
> +
>Status = EmmcTuningClkForHs200 (PciIo, PassThru, Slot, BusWidth);
>
>return Status;
> @@ -989,6 +1028,27 @@ EmmcSwitchToHS400 (
>
>HsTiming = 3;
>Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, 
> ClockFreq);
> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +Status = mOverride->NotifyPhase (
> +  Private->ControllerHandle,
> +  Slot,
> +  EdkiiSdMmcSwitchClockFreqPost,
> +  &Timing
> +  );
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((
> +DEBUG_ERROR,
> +"%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> +__FUNCTION__,
> +Status
> +));
> +  return Status;
> +}
> +  }
>
>return Status;
>  }
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c 
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> index 5645a71..057a4e2 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> @@ -887,6 +887,24 @@ SdCardSetBusMode (
>  return Status;
>}
>
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +Status = mOverride->NotifyPhase (
> +  Private->ControllerHandle,
> +  Slot,
> +  EdkiiSdMmcSwitchClockFreqPost,
> +  &Timing
> +  );
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((
> +DEBUG_ERROR,
> +"%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> +__FUNCTION__,
> +Status
> +));
> +  return Status;
> +}
> +  }
> +
>if ((AccessMode == 3) || ((AccessMode == 2) && (Capability->TuningSDR50 != 
> 0))) {
>  Status = SdCardTuningClock (PciIo, PassThru, Slot);
>  if (EFI_ERROR (Status)) {
> --
> 2.7.4
>
__

Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol

[Ard Biesheuvel]

(add MdeModulePkg maintainers)

On 5 October 2018 at 15:25, Marcin Wojtas  wrote:
> From: Tomasz Michalec 
>
> Some SD Host Controlers use different values in Host Control 2 Register
> to select UHS Mode. This patch adds a new UhsSignaling type routine to
> the NotifyPhase of the SdMmcOverride protocol.
>
> UHS signaling configuration is moved to a common, default routine
> (SdMmcHcUhsSignaling), which is called when SdMmcOverride does not
> cover this functionality.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  50 +++
>  MdeModulePkg/Include/Protocol/SdMmcOverride.h|   2 +
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 153 
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c|  37 +++--
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c |  69 +
>  5 files changed, 243 insertions(+), 68 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h 
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index e389d52..a03160d 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -63,6 +63,39 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
> EXPRESS OR IMPLIED.
>  #define SD_MMC_HC_CTRL_VER0xFE
>
>  //
> +// SD Host Controler bits to HOST_CTRL2 register
> +//
> +#define SD_MMC_HC_CTRL_UHS_MASK   0x0007
> +#define SD_MMC_HC_CTRL_UHS_SDR12  0x
> +#define SD_MMC_HC_CTRL_UHS_SDR25  0x0001
> +#define SD_MMC_HC_CTRL_UHS_SDR50  0x0002
> +#define SD_MMC_HC_CTRL_UHS_SDR104 0x0003
> +#define SD_MMC_HC_CTRL_UHS_DDR50  0x0004
> +#define SD_MMC_HC_CTRL_MMC_DDR52  0x0004
> +#define SD_MMC_HC_CTRL_MMC_SDR50  0x0002
> +#define SD_MMC_HC_CTRL_MMC_SDR25  0x0001
> +#define SD_MMC_HC_CTRL_MMC_SDR12  0x
> +#define SD_MMC_HC_CTRL_HS200  0x0003
> +#define SD_MMC_HC_CTRL_HS400  0x0005
> +
> +//
> +// Timing modes for uhs
> +//
> +typedef enum {
> +  SdMmcUhsSdr12,
> +  SdMmcUhsSdr25,
> +  SdMmcUhsSdr50,
> +  SdMmcUhsSdr104,
> +  SdMmcUhsDdr50,
> +  SdMmcMmcDdr52,
> +  SdMmcMmcSdr50,
> +  SdMmcMmcSdr25,
> +  SdMmcMmcSdr12,
> +  SdMmcMmcHs200,
> +  SdMmcMmcHs400,
> +} SD_MMC_UHS_TIMING;
> +

Here, we end up with two sets of symbolic constants for the same
thing, and I suppose this enum will be duplicated in your
SdMmcOverride implementation?



> +//
>  // The transfer modes supported by SD Host Controller
>  // Simplified Spec 3.0 Table 1-2
>  //
> @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
>IN UINT8  Slot
>);
>
> +/**
> +  Set SD Host Controler control 2 registry according to selected speed.
> +
> +  @param[in] PciIo  The PCI IO protocol instance.
> +  @param[in] Slot   The slot number of the SD card to send the 
> command to.
> +  @param[in] Timing The timing to select.
> +
> +  @retval EFI_SUCCESS   The timing is set successfully.
> +  @retval OthersThe timing isn't set successfully.
> +**/
> +EFI_STATUS
> +SdMmcHcUhsSignaling (
> +  IN EFI_PCI_IO_PROTOCOL*PciIo,
> +  IN UINT8  Slot,
> +  IN SD_MMC_UHS_TIMING  Timing
> +  );
> +
>  #endif
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h 
> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> index 178945f..25db98a 100644
> --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -17,6 +17,7 @@
>  #ifndef __SD_MMC_OVERRIDE_H__
>  #define __SD_MMC_OVERRIDE_H__
>
> +#include 
>  #include 
>
>  #define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
> @@ -31,6 +32,7 @@ typedef enum {
>EdkiiSdMmcResetPost,
>EdkiiSdMmcInitHostPre,
>EdkiiSdMmcInitHostPost,
> +  EdkiiSdMmcUhsSignaling,
>  } EDKII_SD_MMC_PHASE_TYPE;
>
>  /**
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c 
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index c5fd214..05bd4a0 100755
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -740,10 +740,13 @@ EmmcSwitchToHighSpeed (
>IN UINT8  BusWidth
>)
>  {
> -  EFI_STATUS  Status;
> -  UINT8   HsTiming;
> -  UINT8   HostCtrl1;
> -  UINT8   HostCtrl2;
> +  EFI_STATUS  Status;
> +  UINT8   HsTiming;
> +  UINT8   HostCtrl1;
> +  SD_MMC_UHS_TIMING   Timing;
> +  SD_MMC_HC_PRIVATE_DATA  *Private;
> +
> +  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
>
>Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
>if (EFI_ERROR (Status)) {
> @@ -758,27 +761,37 @@ EmmcSwitchToHighSpeed (
>  return Status;
>}
>
> -  //
> -  // Clean UHS Mode Select field of Host Control 2 reigster before update
> -  //
> -  HostCtrl2 = (UINT8)~0x7;
> -  Status = SdMmcHcAndMmio (

Re: [edk2] [PATCH v2 1/4] MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in NotifyPhase

[Ard Biesheuvel]

(add MdeModulePkg maintainers)

On 5 October 2018 at 15:25, Marcin Wojtas  wrote:
> In order to ensure bigger flexibility in the NotifyPhase
> routine of the SdMmcOverride protocol, enable using an
> optional phase-specific data. This will allow to exchange
> more information between the protocol producer driver
> and SdMmcPciHcDxe in the newly added callbacks.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  MdeModulePkg/Include/Protocol/SdMmcOverride.h|  4 +++-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 12 
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h 
> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> index 0766252..178945f 100644
> --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -63,6 +63,7 @@ EFI_STATUS
>@param[in]  PhaseType The type of operation and whether the
>  hook is invoked right before (pre) or
>  right after (post)
> +  @param[in,out]  Data  The pointer to a phase-specific data.
>
>@retval EFI_SUCCESS   The override function completed successfully.
>@retval EFI_NOT_FOUND The specified controller or slot does not 
> exist.
> @@ -74,7 +75,8 @@ EFI_STATUS
>  (EFIAPI * EDKII_SD_MMC_NOTIFY_PHASE) (
>IN  EFI_HANDLE  ControllerHandle,
>IN  UINT8   Slot,
> -  IN  EDKII_SD_MMC_PHASE_TYPE PhaseType
> +  IN  EDKII_SD_MMC_PHASE_TYPE PhaseType,
> +  IN OUT  VOID*OptParam

Please use the same name in the comment block and in the actual prototype.

Also, could we use PhaseData as the name perhaps?

With those changes

Reviewed-by: Ard Biesheuvel 

>);
>
>  struct _EDKII_SD_MMC_OVERRIDE {
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c 
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index 25771dc..02eb4ad 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -444,7 +444,8 @@ SdMmcHcReset (
>  Status = mOverride->NotifyPhase (
>Private->ControllerHandle,
>Slot,
> -  EdkiiSdMmcResetPre);
> +  EdkiiSdMmcResetPre,
> +  NULL);
>  if (EFI_ERROR (Status)) {
>DEBUG ((DEBUG_WARN,
>  "%a: SD/MMC pre reset notifier callback failed - %r\n",
> @@ -494,7 +495,8 @@ SdMmcHcReset (
>  Status = mOverride->NotifyPhase (
>Private->ControllerHandle,
>Slot,
> -  EdkiiSdMmcResetPost);
> +  EdkiiSdMmcResetPost,
> +  NULL);
>  if (EFI_ERROR (Status)) {
>DEBUG ((DEBUG_WARN,
>  "%a: SD/MMC post reset notifier callback failed - %r\n",
> @@ -1087,7 +1089,8 @@ SdMmcHcInitHost (
>  Status = mOverride->NotifyPhase (
>Private->ControllerHandle,
>Slot,
> -  EdkiiSdMmcInitHostPre);
> +  EdkiiSdMmcInitHostPre,
> +  NULL);
>  if (EFI_ERROR (Status)) {
>DEBUG ((DEBUG_WARN,
>  "%a: SD/MMC pre init notifier callback failed - %r\n",
> @@ -1122,7 +1125,8 @@ SdMmcHcInitHost (
>  Status = mOverride->NotifyPhase (
>Private->ControllerHandle,
>Slot,
> -  EdkiiSdMmcInitHostPost);
> +  EdkiiSdMmcInitHostPost,
> +  NULL);
>  if (EFI_ERROR (Status)) {
>DEBUG ((DEBUG_WARN,
>  "%a: SD/MMC post init notifier callback failed - %r\n",
> --
> 2.7.4
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [patch] MdeModulePkg/HiiDB: Fix incorrect structure convention for checkbox

[Laszlo Ersek]

Hi Dandan,

On 10/08/18 03:29, Dandan Bi wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1224
> 
> When covert IFR binary to EFI_IFR_CHECKBOX structure,
> Current code has following incorrect code logic:
> IfrCheckBox = (EFI_IFR_CHECKBOX *) (IfrOpHdr + 1);
> The correct one should be:
> IfrCheckBox = (EFI_IFR_CHECKBOX *) IfrOpHdr;
> 
> This patch is to fix this bug.
> 
> Cc: Liming Gao 
> Cc: Star Zeng 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi 
> ---
>  MdeModulePkg/Universal/HiiDatabaseDxe/Database.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c 
> b/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
> index 45448c5198..664687796f 100644
> --- a/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
> +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
> @@ -896,11 +896,11 @@ UpdateDefaultSettingInFormPackage (
>break;
>  case EFI_IFR_CHECKBOX_OP:
>IfrScope = IfrOpHdr->Scope;
>IfrQuestionType  = IfrOpHdr->OpCode;
>IfrQuestionHdr   = (EFI_IFR_QUESTION_HEADER *) (IfrOpHdr + 1);
> -  IfrCheckBox  = (EFI_IFR_CHECKBOX *) (IfrOpHdr + 1);
> +  IfrCheckBox  = (EFI_IFR_CHECKBOX *) IfrOpHdr;
>EfiVarStoreIndex = IsEfiVarStoreQuestion (IfrQuestionHdr, 
> EfiVarStoreList, EfiVarStoreNumber);
>Width= sizeof (BOOLEAN);
>if (EfiVarStoreIndex < EfiVarStoreNumber) {
>  for (Index = 0; Index < DefaultIdNumber; Index ++) {
>if (DefaultIdList[Index] == EFI_HII_DEFAULT_CLASS_STANDARD) {
> 

what were the practical consequences (symptoms) of this issue? Did some
checkboxes not work? (I'm asking because SecureBootConfigDxe uses some
checkboxes.)

Thanks,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC MdeModulePkg/UefiBootManagerLib v1 1/1] MdeModulePkg/UefiBootManagerLib: Fix raid card repair fail issue

[Laszlo Ersek]

On 10/08/18 08:39, Ni, Ruiyu wrote:
> On 9/28/2018 2:18 PM, Ming Huang wrote:
>>
>>
>> On 9/26/2018 1:00 PM, Ni, Ruiyu wrote:
 @@ -507,12 +552,13 @@ BmRepairAllControllers (
  FormBrowser2,
  &HiiHandles[Index],
  1,
 -   PcdGetPtr
 (PcdDriverHealthConfigureForm),
 +   &gEfiHiiDriverHealthFormsetGuid,
>>>
>>> I still don't quite understand the changes.
>>> But the above specific change removes the
>>> PcdDriverHealthConfigureForm form pop up.
>>> Instead it pops up each driver health form one by one.
>>> Why?
>>
>> This change is refer to the below function:
>> ProcessSingleControllerHealth
>> (IntelFrameworkModulePkg/Universal/Bdsdxe/DeviceMngr/DeviceManager.c).
>> I don't real understand it also, but it works.
>>
> 
> I am sorry. I cannot review the changes if you cannot explain it:)
> Fix should be based on the thoroughly understanding to the issue.

Could it be that the RAID card driver is buggy?

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3 00/16] Removed unused PCDs

[Laszlo Ersek]

Hi Ray,

On 10/08/18 05:07, Ni, Ruiyu wrote:
>> -Original Message-
>> From: Laszlo Ersek 
>> Sent: Tuesday, October 2, 2018 6:14 PM
>> To: Ni, Ruiyu ; Zhang, Chao B 
>> Cc: Zhang, Shenglei ; edk2-devel@lists.01.org;
>> Kinney, Michael D 
>> Subject: Re: [edk2] [PATCH v3 00/16] Removed unused PCDs
>>
>> Ray, Chao,
>>
>> guys, you keep breaking the development process. Please fix your email
>> clients *now*.
>>
>> This is not the first time it has happened. If I remember correctly, Ray 
>> blamed
>> his email client last time (not showing message threads correctly, or
>> something similar).
>>
>> I'm sorry, but this is unacceptable. This is on-going, systemic disregard 
>> for the
>> project's other participants.
>>
>> Please fix your mail user agents *now*.
>>
>> Here's my promise. Next time, I'm going to revert such commits (assuming I
>> manage to catch them again). They do not represent the facts from the
>> mailing list.
> 
> Sorry about that. I can understand it.
> So this is the first mail I choose to reply after a long holiday.
> I am now using Mozilla Thunderbird to only receive mails from this mailing 
> list.
> This mail client can group the mails correctly so if I missed any R-b that's 
> absolutely
> my fault😊
> 
> There is a difference between help-to-push for Intel developers and non-Intel 
> developers.
> As the Intel developer, Shenglei is very kind to prepare the patch files with 
> R-b and
> send to me internally as attachments. All I need to do is pushing the patches.
> Before the pushing, I will check whether there is R-b but won't check whether 
> all R-bs
> are there.
> 
> For non-Intel developers, I will edit the patch to list all R-bs before 
> pushing.
> 
> Again, thanks for enforcing the process. We will follow the process more 
> strictly in future.

Thank you for following up.

Traditionally, going through the review feedback, and picking up R-b /
T-b and similar tags, are the maintainer's task, involving a git-rebase
with "reword" operations, and the final push. I'm not sure how it helps
to split this work, for any given /single/ series, between multiple
people. To me it seems difficult to coordinate.

It definitely makes sense to distribute /multiple/ series over a pool of
maintainers. I think it's the sub-series granularity that is difficult.

In edk2's case, if I remember correctly, there have been some series
even that modified multiple Packages, and were finally pushed by a
single maintainer (after everything had been fully reviewed). That's a
good approach.

If I can help with pushing a patch set at any time (because it is fully
reviewed, but it's hard to find the time to wrangle the patches), please
ping me, on-list or off-list, as you prefer.

(We might also end up updating the workflow so that it become less
error-prone when not using the git command line.)

Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [patch 1/5] MdePkg: Correct the string expression of UTF8 vendor device path

[Laszlo Ersek]

Hi Dandan,

On 10/08/18 05:31, Dandan Bi wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1225
> 
> According to UEFI spec, the string expression of UTF8 vendor
> device node should be displayed as: VenUtf8(). Current code
> display it as: VenUft8() by mistake when convert device
> path node to text.
> 
> This commit is to fix this bug.
> 
> Cc: Ruiyu Ni 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Dandan Bi 
> ---
>  MdePkg/Library/UefiDevicePathLib/DevicePathToText.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c 
> b/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> index 7d8d304f6f..85f5e97131 100644
> --- a/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> +++ b/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> @@ -193,11 +193,11 @@ DevPathToTextVendor (
>  return ;
>} else if (CompareGuid (&Vendor->Guid, &gEfiVT100PlusGuid)) {
>  UefiDevicePathLibCatPrint (Str, L"VenVt100Plus()");
>  return ;
>} else if (CompareGuid (&Vendor->Guid, &gEfiVTUTF8Guid)) {
> -UefiDevicePathLibCatPrint (Str, L"VenUft8()");
> +UefiDevicePathLibCatPrint (Str, L"VenUtf8()");
>  return ;
>} else if (CompareGuid (&Vendor->Guid, &gEfiUartDevicePathGuid)) {
>  FlowControlMap = (((UART_FLOW_CONTROL_DEVICE_PATH *) 
> Vendor)->FlowControlMap);
>  switch (FlowControlMap & 0x0003) {
>  case 0:
> 

it makes sense to send a set of patches that are correlated in some
fashion, even if they individually address different BZs and don't form
a coherent "feature" or larger "bugfix". However, even in such cases,
please send a common cover letter (0/5 in this case). Seeing a unified
diffstat, and a few intro words (about the common theme of the patch
set) is helpful.

(no need to repost, just for the future)

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Event Invitation: TianoCore Community Meeting - NAMO / EMEA

[Laszlo Ersek]

On 10/05/18 22:43, Rebecca Cran wrote:
> I don't see any information about time/place/etc. - should there be an
> attachment?

There is an attachment on the original invite email, called "invite.cs".
I see it on the instance of the invite that I got in my inbox directly,
not reflected from the list (because I was named personally on the
invite). However, the attachment is missing from the email reflected by
the list software.

This is not surprising: the edk2-devel list configuration strips all (or
most) attachments. If I remember correctly, I've complained about this
earlier -- such a list config is broken. It prevents people from sending
screenshots (well-compressed PNG screenshots of virtual machines are a
few tens of KB, they are perfectly suitable for mailing lists). It also
prevents people from sending compressed log files in attachments. IIRC
at one point I also had to base64-encode some binary file manually, and
embed it in the normal body of the email, because I wanted the mailing
list *archive* to preserve it.

At least with the TianoCore Bugzilla, we have another option for saving
binary artifacts necessary for debugging / bug reporting. The mailing
list config remains broken however.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg:disable wraning to pass gcc4.8 build

[Laszlo Ersek]

On 10/08/18 05:04, Gao, Liming wrote:
> Laszlo:
>We meet with this failure in GCC4.8 and GCC4.9, but not in GCC5.  We don't 
> verify earlier version than GCC4.8. 
>GCC48 is the specific tool chain for GCC4.8. But, GCC49 is the general 
> tool chain that can be used with GCC4.9 or the above GCC version for LTO 
> disable. So, even if we specify this warning for GCC48 and GCC49, it may be 
> applied for GCC5 version. To be simplified, I suggest to disable this warning 
> in the general style. I suggest to add more comments here to describe this 
> warning for GCC4.8, GCC4.9 version. Not for GCC5. 

OK, thank you.
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg/RegularExpressionDxe:omit unused variable

[Gary Lin]

On Mon, Oct 08, 2018 at 03:56:48PM +0800, Dongao Guo wrote:
> 
> comment unused variable to avoid warning,and modify inf build option.
> 
Why not just remove the variables altogether instead of commenting them out?
Is it on purpose?

Gary Lin

> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dongao Guo 
> ---
>  .../RegularExpressionDxe/Oniguruma/regexec.c   | 28 
> +++---
>  .../RegularExpressionDxe/RegularExpressionDxe.inf  |  3 ---
>  2 files changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c 
> b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c
> index 7b0fda0..26e7a31 100644
> --- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c
> +++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c
> @@ -5603,11 +5603,11 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* 
> user_data)
>int r;
>int num;
>size_t tag_len;
> -  const UChar* start;
> -  const UChar* right;
> -  const UChar* current;
> -  const UChar* string;
> -  const UChar* strend;
> + // const UChar* start;
> + // const UChar* right;
> + // const UChar* current;
> + // const UChar* string;
> + // const UChar* strend;
>const UChar* tag_start;
>const UChar* tag_end;
>regex_t* reg;
> @@ -5615,9 +5615,9 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* 
> user_data)
>OnigType type;
>OnigValue val;
>char buf[20];
> -  FILE* fp;
> + // FILE* fp;
>  
> -  fp = OutFp;
> + // fp = OutFp;
>  
>r = onig_get_arg_by_callout_args(args, 0, &type, &val);
>if (r != ONIG_NORMAL) return r;
> @@ -5633,11 +5633,11 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* 
> user_data)
>}
>  
>num   = onig_get_callout_num_by_callout_args(args);
> -  start = onig_get_start_by_callout_args(args);
> -  right = onig_get_right_range_by_callout_args(args);
> -  current   = onig_get_current_by_callout_args(args);
> -  string= onig_get_string_by_callout_args(args);
> -  strend= onig_get_string_end_by_callout_args(args);
> + // start = onig_get_start_by_callout_args(args);
> + // right = onig_get_right_range_by_callout_args(args);
> + // current   = onig_get_current_by_callout_args(args);
> + // string= onig_get_string_by_callout_args(args);
> + // strend= onig_get_string_end_by_callout_args(args);
>reg   = onig_get_regex_by_callout_args(args);
>tag_start = onig_get_callout_tag_start(reg, num);
>tag_end   = onig_get_callout_tag_end(reg, num);
> @@ -5653,7 +5653,7 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* 
> user_data)
>  for (i = 0; i < tag_len; i++) buf[i] = tag_start[i];
>  buf[tag_len] = '\0';
>}
> -
> +/*
>fprintf(fp, "ONIG-MONITOR: %-4s %s at: %d [%d - %d] len: %d\n",
>buf,
>in == ONIG_CALLOUT_IN_PROGRESS ? "=>" : "<=",
> @@ -5662,7 +5662,7 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* 
> user_data)
>(int )(right   - string),
>(int )(strend  - string));
>//fflush(fp);
> -
> +*/
>return ONIG_CALLOUT_SUCCESS;
>  }
>  
> diff --git 
> a/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf 
> b/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf
> index 16e91bd..98fb8db 100644
> --- a/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf
> +++ b/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf
> @@ -106,6 +106,3 @@
>  
># Oniguruma: signed and unsigned mismatch/cast
>MSFT:*_*_*_CC_FLAGS = /wd4018 /wd4245 /wd4389
> -
> -  # Oniguruma: error: variable 'fp' set but not used
> -  GCC:*_*_*_CC_FLAGS = -Wno-error=unused-but-set-variable
> -- 
> 1.9.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE

[Sumit Garg]

On Fri, 5 Oct 2018 at 20:33, Leif Lindholm  wrote:
>
> On Wed, Oct 03, 2018 at 11:33:01AM +0200, Ard Biesheuvel wrote:
> > On 3 October 2018 at 09:43, Sumit Garg  wrote:
> > > Add following APIs to communicate with OP-TEE pseudo/early TAs:
> > > 1. OpteeInit
> > > 2. OpteeOpenSession
> > > 3. OpteeCloseSession
> > > 4. OpteeInvokeFunc
> > >
> > > Cc: Ard Biesheuvel 
> > > Cc: Leif Lindholm 
> > > Cc: Michael D Kinney 
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Sumit Garg 
> >
> > Given the outcome of the GP discussion, I'm fine with this approach. Leif?
>
> Apologies for the delay, I needed some time to think it over.
>
> I'm not super happy about this approach, but I'm happier with this
> than I would be with leaving the functionality out of the upstream
> tree. I really hope we can get that license either changed or dropped
> completely.
>

Thanks Leif for this go ahead.

> I do have a few comments below.
>
> > > ---
> > >  ArmPkg/Include/Library/OpteeLib.h|  90 +
> > >  ArmPkg/Library/OpteeLib/Optee.c  | 357 
> > > +++
> > >  ArmPkg/Library/OpteeLib/OpteeLib.inf |   2 +
> > >  ArmPkg/Library/OpteeLib/OpteeSmc.h   |  43 +
>
> Could you follow the instructions in
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
> when generating future patches?
> The --stat* effects aren't apparent here, but the -O ones are.
>

Sure.

> > >  4 files changed, 492 insertions(+)
> > >  create mode 100644 ArmPkg/Library/OpteeLib/OpteeSmc.h
> > >
> > > diff --git a/ArmPkg/Include/Library/OpteeLib.h 
> > > b/ArmPkg/Include/Library/OpteeLib.h
> > > index f65d8674d9b8..2d1c60632dfe 100644
> > > --- a/ArmPkg/Include/Library/OpteeLib.h
> > > +++ b/ArmPkg/Include/Library/OpteeLib.h
> > > @@ -25,10 +25,100 @@
> > >  #define OPTEE_OS_UID2  0xaf630002
> > >  #define OPTEE_OS_UID3  0xa5d5c51b
> > >
> > > +#define OPTEE_MSG_ATTR_TYPE_NONE0x0
> > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INPUT 0x1
> > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT0x2
> > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INOUT 0x3
> > > +#define OPTEE_MSG_ATTR_TYPE_MEM_INPUT   0x9
> > > +#define OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT  0xa
> > > +#define OPTEE_MSG_ATTR_TYPE_MEM_INOUT   0xb
> > > +
> > > +#define OPTEE_MSG_ATTR_TYPE_MASK0xff
> > > +
> > > +#define OPTEE_ORIGIN_COMMS  0x0002
> > > +#define OPTEE_ERROR_COMMS   0x000E
> > > +
> > > +typedef struct {
> > > +  UINT64BufPtr;
>
> If it's a pointer, it has a *.
> Otherwise it's an address.
>

Will rather use BufferAddress.

> > > +  UINT64Size;
> > > +  UINT64ShmRef;
>
> Abbreviations in function, variable or type names (other than the ones
> defined in [1] are not permitted unless they are explicitly added to a
> glossary section of the source file header. Where possible, just write
> out the name in full.
>
> [1] 
> https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/v/release/2.20/4_naming_conventions/#table-2-efi-supported-abbreviations
>
> BufPtr (as a name) gets a pass since it's unambiguous and we already
> have a bunch of those in the codebase. ShmRef needs to be clear.
>

Will replace it with SharedMemoryReference.

> (This comment also applies to a lot of things below, I won't point
> them all out unless asked to.)
>

I will try to replace most of them with full names. But I have
confusion regarding usage of following not included in [1]:

1. MSG/Msg
2. MEM/Mem
3. INFO/Info
4. FUNC/Func
5. RET/Ret
6. ATTR/Attr
7. CMD/Cmd

But I do see their usage in the codebase. Please help to clarify.
Also can I use 1-2 letter variable or struct member names like Mp, Ip,
Op, U etc.?

> > > +} OPTEE_MSG_PARAM_MEM;
> > > +
> > > +typedef struct {
> > > +  UINT64A;
> > > +  UINT64B;
> > > +  UINT64C;
> > > +} OPTEE_MSG_PARAM_VALUE;
> > > +
> > > +typedef struct {
> > > +  UINT64 Attr;
> > > +  union {
> > > +OPTEE_MSG_PARAM_MEM  Mem;
> > > +OPTEE_MSG_PARAM_VALUEValue;
> > > +  } U;
> > > +} OPTEE_MSG_PARAM;
> > > +
> > > +#define MAX_PARAMS   4
>
> This is a very localised macro with a very globalised name.
> Suggest adding an OPTEE_ prefix, but also something describing what it
> is the maximum parameters for. CALL_?
>

Ok will rename it to OPTEE_CALL_MAX_PARAMS.

> > > +
> > > +typedef struct {
> > > +UINT32 Cmd;
> > > +UINT32 Func;
> > > +UINT32 Session;
> > > +UINT32 CancelId;
> > > +UINT32 Pad;
> > > +UINT32 Ret;
> > > +UINT32 RetOrigin;
> > > +UINT32 NumParams;
> > > +
> > > +// NumParams tells the actual number of element in Params
> > > +OPTEE_MSG_PARAMParams[MAX_PA

[edk2] [PATCH V2] IntelFsp2Pkg/GenCfgOpt.py: Support PCD input from command line

[Chasel, Chiu]

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

Build system already support override PCD value by command
line so add this support to GenCfgOpt.py
Also update revision to 0.53

Test: Verified UPD header files generated can reflect different
  PCD values from --pcd build command input

Cc: Jiewen Yao 
Cc: Gao Liming 
Cc: Zhu Yonghong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chasel Chiu 
---
 IntelFsp2Pkg/Tools/GenCfgOpt.py | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py b/IntelFsp2Pkg/Tools/GenCfgOpt.py
index 059cfcb7e4..15d33582ef 100644
--- a/IntelFsp2Pkg/Tools/GenCfgOpt.py
+++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py
@@ -88,6 +88,8 @@ are permitted provided that the following conditions are met:
 **/
 """
 
+BuildOptionPcd = []
+
 class CLogicalExpression:
 def __init__(self):
 self.index= 0
@@ -561,6 +563,12 @@ EndList
 self._PcdsDict[Match.group(1)] = Match.group(2)
 if self.Debug:
 print "INFO : PCD %s = [ %s ]" % (Match.group(1), 
Match.group(2))
+i = 0
+while i < len(BuildOptionPcd):
+Match = re.match("\s*([\w\.]+)\s*\=\s*(\w+)", 
BuildOptionPcd[i])
+if Match:
+self._PcdsDict[Match.group(1)] = Match.group(2)
+i += 1
 else:
 Match = re.match("^\s*#\s+(!BSF|@Bsf|!HDR)\s+(.+)", DscLine)
 if Match:
@@ -1462,7 +1470,7 @@ EndList
 
 
 def Usage():
-print "GenCfgOpt Version 0.52"
+print "GenCfgOpt Version 0.53"
 print "Usage:"
 print "GenCfgOpt  UPDTXT  PlatformDscFile BuildFvDir 
[-D Macros]"
 print "GenCfgOpt  HEADER  PlatformDscFile BuildFvDir  InputHFile 
[-D Macros]"
@@ -1472,7 +1480,14 @@ def Main():
 #
 # Parse the options and args
 #
+i = 1
+
 GenCfgOpt = CGenCfgOpt()
+while i < len(sys.argv):
+if sys.argv[i].strip().lower() == "--pcd":
+BuildOptionPcd.append(sys.argv[i+1])
+i += 1
+i += 1
 argc = len(sys.argv)
 if argc < 4:
 Usage()
-- 
2.13.3.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] MdeModulePkg/RegularExpressionDxe:omit unused variable

[Dongao Guo]

comment unused variable to avoid warning,and modify inf build option.

Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dongao Guo 
---
 .../RegularExpressionDxe/Oniguruma/regexec.c   | 28 +++---
 .../RegularExpressionDxe/RegularExpressionDxe.inf  |  3 ---
 2 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c 
b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c
index 7b0fda0..26e7a31 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c
@@ -5603,11 +5603,11 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* 
user_data)
   int r;
   int num;
   size_t tag_len;
-  const UChar* start;
-  const UChar* right;
-  const UChar* current;
-  const UChar* string;
-  const UChar* strend;
+ // const UChar* start;
+ // const UChar* right;
+ // const UChar* current;
+ // const UChar* string;
+ // const UChar* strend;
   const UChar* tag_start;
   const UChar* tag_end;
   regex_t* reg;
@@ -5615,9 +5615,9 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* 
user_data)
   OnigType type;
   OnigValue val;
   char buf[20];
-  FILE* fp;
+ // FILE* fp;
 
-  fp = OutFp;
+ // fp = OutFp;
 
   r = onig_get_arg_by_callout_args(args, 0, &type, &val);
   if (r != ONIG_NORMAL) return r;
@@ -5633,11 +5633,11 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* 
user_data)
   }
 
   num   = onig_get_callout_num_by_callout_args(args);
-  start = onig_get_start_by_callout_args(args);
-  right = onig_get_right_range_by_callout_args(args);
-  current   = onig_get_current_by_callout_args(args);
-  string= onig_get_string_by_callout_args(args);
-  strend= onig_get_string_end_by_callout_args(args);
+ // start = onig_get_start_by_callout_args(args);
+ // right = onig_get_right_range_by_callout_args(args);
+ // current   = onig_get_current_by_callout_args(args);
+ // string= onig_get_string_by_callout_args(args);
+ // strend= onig_get_string_end_by_callout_args(args);
   reg   = onig_get_regex_by_callout_args(args);
   tag_start = onig_get_callout_tag_start(reg, num);
   tag_end   = onig_get_callout_tag_end(reg, num);
@@ -5653,7 +5653,7 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* 
user_data)
 for (i = 0; i < tag_len; i++) buf[i] = tag_start[i];
 buf[tag_len] = '\0';
   }
-
+/*
   fprintf(fp, "ONIG-MONITOR: %-4s %s at: %d [%d - %d] len: %d\n",
   buf,
   in == ONIG_CALLOUT_IN_PROGRESS ? "=>" : "<=",
@@ -5662,7 +5662,7 @@ onig_builtin_monitor(OnigCalloutArgs* args, void* 
user_data)
   (int )(right   - string),
   (int )(strend  - string));
   //fflush(fp);
-
+*/
   return ONIG_CALLOUT_SUCCESS;
 }
 
diff --git 
a/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf 
b/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf
index 16e91bd..98fb8db 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf
@@ -106,6 +106,3 @@
 
   # Oniguruma: signed and unsigned mismatch/cast
   MSFT:*_*_*_CC_FLAGS = /wd4018 /wd4245 /wd4389
-
-  # Oniguruma: error: variable 'fp' set but not used
-  GCC:*_*_*_CC_FLAGS = -Wno-error=unused-but-set-variable
-- 
1.9.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] IntelFsp2Pkg/GenCfgOpt.py: Support PCD input from command line

[Chasel, Chiu]

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

Build system already support override PCD value by command
line so add this support to GenCfgOpt.py

Test: Verified UPD header files generated can reflect different
  PCD values from --pcd build command input

Cc: Jiewen Yao 
Cc: Gao Liming 
Cc: Zhu Yonghong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chasel Chiu 
---
 IntelFsp2Pkg/Tools/GenCfgOpt.py | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py b/IntelFsp2Pkg/Tools/GenCfgOpt.py
index 059cfcb7e4..efc2eea6ab 100644
--- a/IntelFsp2Pkg/Tools/GenCfgOpt.py
+++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py
@@ -88,6 +88,8 @@ are permitted provided that the following conditions are met:
 **/
 """
 
+BuildOptionPcd = []
+
 class CLogicalExpression:
 def __init__(self):
 self.index= 0
@@ -561,6 +563,12 @@ EndList
 self._PcdsDict[Match.group(1)] = Match.group(2)
 if self.Debug:
 print "INFO : PCD %s = [ %s ]" % (Match.group(1), 
Match.group(2))
+i = 0
+while i < len(BuildOptionPcd):
+Match = re.match("\s*([\w\.]+)\s*\=\s*(\w+)", 
BuildOptionPcd[i])
+if Match:
+self._PcdsDict[Match.group(1)] = Match.group(2)
+i += 1
 else:
 Match = re.match("^\s*#\s+(!BSF|@Bsf|!HDR)\s+(.+)", DscLine)
 if Match:
@@ -1472,7 +1480,14 @@ def Main():
 #
 # Parse the options and args
 #
+i = 1
+
 GenCfgOpt = CGenCfgOpt()
+while i < len(sys.argv):
+if sys.argv[i].strip().lower() == "--pcd":
+BuildOptionPcd.append(sys.argv[i+1])
+i += 1
+i += 1
 argc = len(sys.argv)
 if argc < 4:
 Usage()
-- 
2.13.3.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."

[Ni, Ruiyu]

On 10/4/2018 11:03 PM, jim.dai...@dell.com wrote:

MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."

The loop that removes "\..\" errs when multiple "\.." sequences are
in the path.  Before this change the code would modify a path like
"FS0:\efi\tools\..\.." to "FS0:\efi\\.." and then to "FS0:\efi\", but
the correct path is "FS0:\".

You can test the effect of this change in the shell by setting the
current directory to something like FS0:\efi\boot and then executing
the command "ls ..\..".  Before the change you will see the files in
the FS0:\efi directory; after the change, you will see the files in
the root directory of FS0:.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jim Dailey 
---
  MdePkg/Library/BaseLib/FilePaths.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Library/BaseLib/FilePaths.c 
b/MdePkg/Library/BaseLib/FilePaths.c
index d6f3758ecb..5d3de01894 100644
--- a/MdePkg/Library/BaseLib/FilePaths.c
+++ b/MdePkg/Library/BaseLib/FilePaths.c
@@ -2,6 +2,7 @@
Defines file-path manipulation functions.
  
Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.

+  Copyright (c) 2018, Dell Technologies. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD 
License
which accompanies this distribution.  The full text of the license may be 
found at
@@ -103,7 +104,9 @@ PathCleanUpDirectories(
  ) {
  *(TempString + 1) = CHAR_NULL;
  PathRemoveLastItem(Path);
-CopyMem (Path + StrLen (Path), TempString + 3, StrSize (TempString + 3));
+if (*(TempString + 3)) {
+  CopyMem (Path + StrLen (Path), TempString + 4, StrSize (TempString + 4));
+}
}
  
//



Jim,
Are you fixing a corner case bug introduced by following commit:

> SHA-1: bb99e3282c9e69fbd6365d117c58d15589e34c5d
* MdePkg/BaseLib: Fix PathCleanUpDirectories to correctly handle "\.\"

> The old code incorrectly cleans path like "fs0:\abc\.\.." to
> "fs0:\abc", instead of "fs0:\"

> The patch fixes this bug.


If yes, can you mention the above commit in your commit message?


--
Thanks,
Ray
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel