date:20190130

Re: [edk2] [RFC] Proposal to split Pkgs

2019-01-30 Thread Ard Biesheuvel

On Thu, 31 Jan 2019 at 08:09, Ni, Ray  wrote:
>
> On 1/29/2019 10:21 PM, Leif Lindholm wrote:
> > Hi Ray,
> >
> > First of all, thank you for restarting this discussion, and putting
> > the effort in for a POC.
> >
> > On Tue, Jan 29, 2019 at 05:59:35AM +, Ni, Ray wrote:
> >> Hello,
> >> I'd like to propose to split today's BIG packages in following ways:
> >>
> >> ==Overview =
> >>
> >> 1. Separate Industry standard definitions from UEFI and PI interfaces.
> >> 2. Separate UEFI and PI interfaces from implementations.
> >>  a. Separate UEFI and PI interfaces to different packages
> >>  b. Separate PI PEI, DXE and MM phase interfaces to different packages
> >> 3. Separate different features into feature packages.
> >>  Feature interface may be in the feature package to provide minimal
> >>  common interface packages.
> >>
> >> The POC code is in https://github.com/jyao1/edk2/tree/ReOrg.
> >> It basically split the EDKII common code to three directories:
> >> Core/, Device/, and Feature/.
> >
> > I'm not sure I like the 'Device' name, but that's not that important
> > at this stage - I like that this is split up the way it is.
> >
> > I also think a lot of things currently under Feture/misc could be
> > broken out into additional subdirectories under Feature/.
> >
> >> The code is in very early POC phase and only code in Core/ directory
> >> can pass the build.
> >> I would like to gather feedbacks through this RFC to see whether
> >> this splitting direction makes sense.
> >> Is there any other better ways of splitting?
> >> Or perhaps do not split in such a small granularity?
> >
> > I think this would be my only negative(ish) feedback on the POC.
> >
> > While it has the benefit of modules requiring to specify more
> > precisely which functionality they are pulling in...
> > ...it means that they now need to list *everything*.
> >
> > And with the mechanism by which I have seen Intel engineers make use
> > of PACKAGES_PATH in the past*, we end up with a bazillion packages
> > that need to be added individually to this variable for each build.
>
> No. "PACKAGES_PATH" only contains paths pointing to the *parent*
> directory of *Pkg. In this case, it equals to:
> /work/edk2/Core:/work/edk2/Device:/work/edk2/Feature/security:/work/edk2/Feature/network:/work/edk2/Feature/misc
>

Could we *please* not do this? PACKAGES_PATH is very useful for
combining packages from different repositories, but that does not mean
we should split up the core repository in this way.

What we will end up with is platforms that use

  Core/Package/Foo.inf

and platforms that use

  Package/Foo.inf

which creates an aliasing mess where the same file is reachable in
different ways. Also, it encourages the behavior where you need to
fetch version X of repo A and version Y of repo B, and the whole point
of open sourcing is to have all code co-exist in the same source tree.





>
> >
> > * (arguably how it was designed to be used - just not how I am
> >interested in using it)
> >
> > It also means that we need *tons* of dummy .dsc files in order to run
> > through simple build tests.
>
> Yes each small package needs its own DSC and DEC.
> DSC to list all components.
> DEC to list all interfaces.
>
> OK maybe the "need" in your wording means the build test script file
> needs to reference all DSC files.
> That maybe can be solved by an advanced script to call "build" for each
> DSC files inside a directory that matches a specific pattern. So the
> build test script to build PiPeiPkg/PiDxePkg/PiMmPkg will be like:
>build-any-dsc /work/edk2/Core/ --pattern="*Pi*"
>
> >
> > My preference would be to push the packages back up to the top-level.
> > The split still makes sense, and allocating maintainers to sub-parts
> > can happen without a full-out redesign of the Maintainers.txt format.
> > (Which I will try to resurrect with a proposal over the next few weeks
> > anyway.)
>
> Combining all packages inside the Device and Feature directory to still
> one single big package but with another directory layout so that only
> single DSC/DEC is needed?
>
> >
> >> Or perhaps Mike's work to move lib-c content to edk2-libc repo,
> >> to move real platform code to edk2-platform repo is enough for
> >> now?
> >
> > I think we need this further restructuring, and renaming. Not just for
> > splitting up maintainership duties (which as I mentioned in email to
> > Laszlo, we will still need to do with this change). But to make the
> > codebase more approachable.
> >
>
> "more approachable" means more friendly to new comers? Just try to
> understand everyone's real concern to today's codebase.
>
> >> ==More explanations =
> >>
> >> There are 9 packages inside Core/ directory:
> >> 1. BasePkg
> >> Contains industry standard definitions (exclude UEFI and PI) and base
> >> libraries that non-UEFI and non-PI development can depend on.
> >> UEFI or PI development can also

Re: [edk2] [PATCH v5] MdePkg/BaseLib: Add Base64Encode() and Base64Decode()

2019-01-30 Thread Ni, Ray




Even ECC doesn't complain, I still suggest to change 
Encoding_table/Decoding_table to a proper name, e.g.:

EncodingTable/DecodingTable.

With the rename applied in V5 patch,
Reviewed-by: Ray Ni 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/2] IntelSiliconPkg\Include\IndustryStandard: Update IGD_OPREGION_MBOX3 Structure

2019-01-30 Thread Ni, Ray


On 1/31/2019 2:55 PM, Ni, Ray wrote:

On 1/31/2019 1:00 PM, Digant H Solanki wrote:

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1454
Based on latest IGD OpRegion Spec, IGD_OPREGION_MBOX3 needs to be 
updated with two new members : Physical Address of Raw VBT Data (RVDA) 
and Size of Raw VBT Data (RVDS)


Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Digant H Solanki 
Cc: Liming Gao 
Cc: Chaganty, Rangasai V 
Cc: Ni, Ray 
---
  IntelSiliconPkg/Include/IndustryStandard/IgdOpRegion.h | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/IntelSiliconPkg/Include/IndustryStandard/IgdOpRegion.h 
b/IntelSiliconPkg/Include/IndustryStandard/IgdOpRegion.h

index 5ce80a5be8..1c83efee40 100644
--- a/IntelSiliconPkg/Include/IndustryStandard/IgdOpRegion.h
+++ b/IntelSiliconPkg/Include/IndustryStandard/IgdOpRegion.h
@@ -4,9 +4,7 @@

https://01.org/sites/default/files/documentation/skl_opregion_rev0p5.pdf
-  @note Fixed bug in the spec Mailbox3 - RM31 size from 0x45(69) to 
0x46(70)

-
-  Copyright (c) 2016, Intel Corporation. All rights reserved.
+  Copyright (c) 2016 - 2019, Intel Corporation. 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

@@ -118,7 +116,9 @@ typedef struct {
    UINT64 FDSS;  ///< Offset 0x3AA DSS Buffer address 
allocated for IFFS feature

    UINT32 FDSP;  ///< Offset 0x3B2 Size of DSS buffer
    UINT32 STAT;  ///< Offset 0x3B6 State Indicator
-  UINT8  RM31[0x46];    ///< Offset 0x3BA - 0x3FF  Reserved Must be 
zero. Bug in spec 0x45(69)
+  UINT64 RVDA;  ///< Offset 0x3BA Physical address of Raw VBT 
data. Added from Spec Version 0.90 to support VBT greater than 6KB.
+  UINT32 RVDS;  ///< Offset 0x3C2 Size of Raw VBT data. Added 
from Spec Version 0.90 to support VBT greater than 6KB.
+  UINT8  RM31[0x3A];    ///< Offset 0x3C6 - 0x3FF  Reserved Must be 
zero.

  } IGD_OPREGION_MBOX3;
  ///






Sorry seems my comment was missed in previous mail.
Is it possible that an existing consumer is already using
RM31 to access RVDA and RVDS? For example,
using " * (UINT64 *) RM31[0]" to access RVDA.
If it's possible, I suggest we rename "RM31" to a different name.
Otherwise, the original code that modifies RVDA using RM may
incorrectly modifies bytes at offset 0x3C6.
A build failure instead of a silent failure can be seen with the rename.

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

[edk2] [PATCH v5] MdePkg/BaseLib: Add Base64Encode() and Base64Decode()

2019-01-30 Thread Shenglei Zhang

From: Mike Turner 

Introduce public functions Base64Encode and Base64Decode.
https://bugzilla.tianocore.org/show_bug.cgi?id=1370

v2:1.Remove some white space.
   2.Add unit test with test vectors in RFC 4648.
 https://github.com/shenglei10/edk2/tree/encode_test
 https://github.com/shenglei10/edk2/tree/decode_test

v3:1.Align white space.
   2.Update comments of Base64Encode and Base64Decode.
   3.Change the use of macro RETURN_DEVICE_ERROR to
 RETURN_INVALID_PARAMETER in string.c.

v4:Change parameters' names.

v5:1.Update usage of variables.
   2.Remove debug message in Base64Decode().

Cc: Michael D Kinney 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Shenglei Zhang 
---
 MdePkg/Include/Library/BaseLib.h |  56 ++
 MdePkg/Library/BaseLib/String.c  | 331 +++
 2 files changed, 387 insertions(+)

diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 1eb842384e..03173def58 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -2720,6 +2720,62 @@ AsciiStrnToUnicodeStrS (
   OUT UINTN *DestinationLength
   );
 
+/**
+  Convert binary data to a Base64 encoded ascii string based on RFC4648.
+
+  Produce a Null-terminated Ascii string in the output buffer specified by 
Destination and DestinationSize.
+  The Ascii string is produced by converting the data string specified by 
Source and SourceLength.
+
+  @param Source   Input UINT8 data
+  @param SourceLength Number of UINT8 bytes of data
+  @param Destination  Pointer to output string buffer
+  @param DestinationSize  Size of ascii buffer. Set to 0 to get the size 
needed.
+  Caller is responsible for passing in buffer of 
DestinationSize
+
+  @retval RETURN_SUCCESS When ascii buffer is filled in.
+  @retval RETURN_INVALID_PARAMETER   If Source is NULL or DestinationSize is 
NULL.
+  @retval RETURN_INVALID_PARAMETER   If SourceLength or DestinationSize is 
bigger than (MAX_ADDRESS - (UINTN)Destination).
+  @retval RETURN_BUFFER_TOO_SMALLIf SourceLength is 0 and DestinationSize 
is <1.
+  @retval RETURN_BUFFER_TOO_SMALLIf Destination is NULL or DestinationSize 
is smaller than required buffersize.
+
+**/
+RETURN_STATUS
+EFIAPI
+Base64Encode (
+  IN  CONST UINT8  *Source,
+  INUINTN   SourceLength,
+  OUT   CHAR8  *Destination  OPTIONAL,
+  IN OUTUINTN  *DestinationSize
+  );
+
+/**
+  Convert Base64 ascii string to binary data based on RFC4648.
+
+  Produce Null-terminated binary data in the output buffer specified by 
Destination and DestinationSize.
+  The binary data is produced by converting the Base64 ascii string specified 
by Source and SourceLength.
+
+  @param Source  Input ASCII characters
+  @param SourceLengthNumber of ASCII characters
+  @param Destination Pointer to output buffer
+  @param DestinationSize Caller is responsible for passing in buffer of at 
least DestinationSize.
+ Set 0 to get the size needed. Set to bytes stored on 
return.
+
+  @retval RETURN_SUCCESS When binary buffer is filled in.
+  @retval RETURN_INVALID_PARAMETER   If Source is NULL or DestinationSize is 
NULL.
+  @retval RETURN_INVALID_PARAMETER   If SourceLength or DestinationSize is 
bigger than (MAX_ADDRESS -(UINTN)Destination ).
+  @retval RETURN_INVALID_PARAMETER   If there is any invalid character in 
input stream.
+  @retval RETURN_BUFFER_TOO_SMALLIf buffer length is smaller than required 
buffer size.
+
+ **/
+RETURN_STATUS
+EFIAPI
+Base64Decode (
+  IN  CONST CHAR8  *Source,
+  INUINTN   SourceLength,
+  OUT   UINT8  *Destination  OPTIONAL,
+  IN OUTUINTN  *DestinationSize
+  );
+
 /**
   Converts an 8-bit value to an 8-bit BCD value.
 
diff --git a/MdePkg/Library/BaseLib/String.c b/MdePkg/Library/BaseLib/String.c
index e6df12797d..87d0a9d1dc 100644
--- a/MdePkg/Library/BaseLib/String.c
+++ b/MdePkg/Library/BaseLib/String.c
@@ -1763,6 +1763,337 @@ AsciiStrToUnicodeStr (
 
 #endif
 
+//
+// The basis for Base64 encoding is RFC 4686 
https://tools.ietf.org/html/rfc4648
+//
+// RFC 4686 has a number of MAY and SHOULD cases.  This implementation chooses
+// the more restrictive versions for security concerns (see RFC 4686 section 
3.3).
+//
+// A invalid character, if encountered during the decode operation, causes the 
data
+// to be rejected. In addition, the '=' padding character is only allowed at 
the end
+// of the Base64 encoded string.
+//
+#define BAD_V  99
+
+STATIC CHAR8 Encoding_table[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+"abcdefghijklmnopqrstuvwxyz"
+"0123456789+/";
+
+STATIC UINT8 Decoding_table[] = {
+  //
+  // Valid characters 
ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/
+  // Also, set '=' as a zero for decoding
+  // 0  ,1,   2,

Re: [edk2] [RFC] Proposal to split Pkgs

2019-01-30 Thread Ni, Ray

On 1/29/2019 10:21 PM, Leif Lindholm wrote:

Hi Ray,

First of all, thank you for restarting this discussion, and putting
the effort in for a POC.

On Tue, Jan 29, 2019 at 05:59:35AM +, Ni, Ray wrote:

Hello,
I'd like to propose to split today's BIG packages in following ways:

==Overview =

1. Separate Industry standard definitions from UEFI and PI interfaces.
2. Separate UEFI and PI interfaces from implementations.
a. Separate UEFI and PI interfaces to different packages
b. Separate PI PEI, DXE and MM phase interfaces to different packages
3. Separate different features into feature packages.
Feature interface may be in the feature package to provide minimal
common interface packages.

The POC code is in https://github.com/jyao1/edk2/tree/ReOrg.
It basically split the EDKII common code to three directories:
Core/, Device/, and Feature/.

I'm not sure I like the 'Device' name, but that's not that important
at this stage - I like that this is split up the way it is.

I also think a lot of things currently under Feture/misc could be
broken out into additional subdirectories under Feature/.

The code is in very early POC phase and only code in Core/ directory
can pass the build.
I would like to gather feedbacks through this RFC to see whether
this splitting direction makes sense.
Is there any other better ways of splitting?
Or perhaps do not split in such a small granularity?

I think this would be my only negative(ish) feedback on the POC.

While it has the benefit of modules requiring to specify more
precisely which functionality they are pulling in...
...it means that they now need to list *everything*.

And with the mechanism by which I have seen Intel engineers make use
of PACKAGES_PATH in the past*, we end up with a bazillion packages
that need to be added individually to this variable for each build.

No. "PACKAGES_PATH" only contains paths pointing to the *parent*
directory of *Pkg. In this case, it equals to:

/work/edk2/Core:/work/edk2/Device:/work/edk2/Feature/security:/work/edk2/Feature/network:/work/edk2/Feature/misc

* (arguably how it was designed to be used - just not how I am
interested in using it)

It also means that we need *tons* of dummy .dsc files in order to run
through simple build tests.

Yes each small package needs its own DSC and DEC.
DSC to list all components.
DEC to list all interfaces.

OK maybe the "need" in your wording means the build test script file
needs to reference all DSC files.
That maybe can be solved by an advanced script to call "build" for each
DSC files inside a directory that matches a specific pattern. So the
build test script to build PiPeiPkg/PiDxePkg/PiMmPkg will be like:

build-any-dsc /work/edk2/Core/ --pattern="*Pi*"

My preference would be to push the packages back up to the top-level.
The split still makes sense, and allocating maintainers to sub-parts
can happen without a full-out redesign of the Maintainers.txt format.
(Which I will try to resurrect with a proposal over the next few weeks
anyway.)

Combining all packages inside the Device and Feature directory to still
one single big package but with another directory layout so that only
single DSC/DEC is needed?

Or perhaps Mike's work to move lib-c content to edk2-libc repo,
to move real platform code to edk2-platform repo is enough for
now?

I think we need this further restructuring, and renaming. Not just for
splitting up maintainership duties (which as I mentioned in email to
Laszlo, we will still need to do with this change). But to make the
codebase more approachable.

"more approachable" means more friendly to new comers? Just try to
understand everyone's real concern to today's codebase.

==More explanations =

There are 9 packages inside Core/ directory:
1. BasePkg
Contains industry standard definitions (exclude UEFI and PI) and base
libraries that non-UEFI and non-PI development can depend on.
UEFI or PI development can also depend on this package.
2. UefiPkg
Contains UEFI interfaces and libraries that UEFI driver-model driver
development can depend on.
3. PiPeiPkg
Contains PI interfaces and libraries for PEI phase that PEI module
development can depend on.
4. PiDxePkg
Contains PI interfaces and libraries for DXE phase that DXE module
development can depend on.

Really happy with the above split.

5. PiMmPkg
Contains PI interfaces and libraries for MM phase that MM/SMM
module development can depend on.

How would/should this work relative to StandaloneMmPkg?

The "Mm" is to reflect to the same word in PI spec.
But I didn't check whether "StandaloneMmPkg" can work with the this.

6. MdeModulePkg (TianoPkg? Name is still TBD)

Yes, this needs to be renamed. I don't object to the Tiano naming.

Thanks.

Contains Tiano implementation specific interfaces and libraries.
Developing modules for pure UEFI or PI should not depend on this package.
7. PeiFoundationPkg

1 2 >

1 - 100 of 120 matches

Mail list logo