[v1] Support FDT library.

[edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Benny Lin 1 year, 1 month ago

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4392
Add FDT support in EDK2 by submodule 3rd party libfdt
(https://github.com/devicetree-org/pylibfdt/tree/main/libfdt)

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Signed-off-by: Benny Lin <benny.lin@intel.com>

Benny Lin (2):
  MdePkg: Support FDT library.
  .pytool: Support FDT library.

 .gitmodules                               |   3 +
 .pytool/CISettings.py                     |   2 +
 MdePkg/Include/Library/FdtLib.h           | 300 ++++++++++++++++++++++
 MdePkg/Library/BaseFdtLib/BaseFdtLib.inf  |  62 +++++
 MdePkg/Library/BaseFdtLib/BaseFdtLib.uni  |  14 +
 MdePkg/Library/BaseFdtLib/FdtLib.c        | 284 ++++++++++++++++++++
 MdePkg/Library/BaseFdtLib/LibFdtSupport.h | 102 ++++++++
 MdePkg/Library/BaseFdtLib/LibFdtWrapper.c | 138 ++++++++++
 MdePkg/Library/BaseFdtLib/libfdt          |   1 +
 MdePkg/Library/BaseFdtLib/limits.h        |  10 +
 MdePkg/Library/BaseFdtLib/stdbool.h       |  10 +
 MdePkg/Library/BaseFdtLib/stddef.h        |  10 +
 MdePkg/Library/BaseFdtLib/stdint.h        |  10 +
 MdePkg/Library/BaseFdtLib/stdlib.h        |  10 +
 MdePkg/Library/BaseFdtLib/string.h        |  10 +
 MdePkg/MdePkg.ci.yaml                     |  17 +-
 MdePkg/MdePkg.dec                         |   4 +
 MdePkg/MdePkg.dsc                         |   2 +
 ReadMe.rst                                |   1 +
 19 files changed, 988 insertions(+), 2 deletions(-)
 create mode 100644 MdePkg/Include/Library/FdtLib.h
 create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.inf
 create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.uni
 create mode 100644 MdePkg/Library/BaseFdtLib/FdtLib.c
 create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtSupport.h
 create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
 create mode 160000 MdePkg/Library/BaseFdtLib/libfdt
 create mode 100644 MdePkg/Library/BaseFdtLib/limits.h
 create mode 100644 MdePkg/Library/BaseFdtLib/stdbool.h
 create mode 100644 MdePkg/Library/BaseFdtLib/stddef.h
 create mode 100644 MdePkg/Library/BaseFdtLib/stdint.h
 create mode 100644 MdePkg/Library/BaseFdtLib/stdlib.h
 create mode 100644 MdePkg/Library/BaseFdtLib/string.h

-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102200): https://edk2.groups.io/g/devel/message/102200
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Andrei Warkentin 1 year, 1 month ago

How does this relate to the existing EmbeddedPkg/Library/FdtLib code? Is there a specific plan to move away from this in existing components?

I did look in the BZ (https://bugzilla.tianocore.org/show_bug.cgi?id=4392) but this doesn't seem to acknowledge that there is existing Fdt support in EmbeddedPkg.

A

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Benny
> Lin
> Sent: Thursday, March 30, 2023 11:52 AM
> To: devel@edk2.groups.io
> Cc: Lin, Benny <benny.lin@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> Liu, Zhiguang <zhiguang.liu@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>; Michael Kubacki
> <mikuback@linux.microsoft.com>
> Subject: [edk2-devel] [PATCH 0/2] Support FDT library.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4392
> Add FDT support in EDK2 by submodule 3rd party libfdt
> (https://github.com/devicetree-org/pylibfdt/tree/main/libfdt)
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Signed-off-by: Benny Lin <benny.lin@intel.com>
> 
> Benny Lin (2):
>   MdePkg: Support FDT library.
>   .pytool: Support FDT library.
> 
>  .gitmodules                               |   3 +
>  .pytool/CISettings.py                     |   2 +
>  MdePkg/Include/Library/FdtLib.h           | 300 ++++++++++++++++++++++
>  MdePkg/Library/BaseFdtLib/BaseFdtLib.inf  |  62 +++++
> MdePkg/Library/BaseFdtLib/BaseFdtLib.uni  |  14 +
>  MdePkg/Library/BaseFdtLib/FdtLib.c        | 284 ++++++++++++++++++++
>  MdePkg/Library/BaseFdtLib/LibFdtSupport.h | 102 ++++++++
> MdePkg/Library/BaseFdtLib/LibFdtWrapper.c | 138 ++++++++++
>  MdePkg/Library/BaseFdtLib/libfdt          |   1 +
>  MdePkg/Library/BaseFdtLib/limits.h        |  10 +
>  MdePkg/Library/BaseFdtLib/stdbool.h       |  10 +
>  MdePkg/Library/BaseFdtLib/stddef.h        |  10 +
>  MdePkg/Library/BaseFdtLib/stdint.h        |  10 +
>  MdePkg/Library/BaseFdtLib/stdlib.h        |  10 +
>  MdePkg/Library/BaseFdtLib/string.h        |  10 +
>  MdePkg/MdePkg.ci.yaml                     |  17 +-
>  MdePkg/MdePkg.dec                         |   4 +
>  MdePkg/MdePkg.dsc                         |   2 +
>  ReadMe.rst                                |   1 +
>  19 files changed, 988 insertions(+), 2 deletions(-)  create mode 100644
> MdePkg/Include/Library/FdtLib.h  create mode 100644
> MdePkg/Library/BaseFdtLib/BaseFdtLib.inf
>  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.uni
>  create mode 100644 MdePkg/Library/BaseFdtLib/FdtLib.c
>  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtSupport.h
>  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
>  create mode 160000 MdePkg/Library/BaseFdtLib/libfdt  create mode 100644
> MdePkg/Library/BaseFdtLib/limits.h
>  create mode 100644 MdePkg/Library/BaseFdtLib/stdbool.h
>  create mode 100644 MdePkg/Library/BaseFdtLib/stddef.h
>  create mode 100644 MdePkg/Library/BaseFdtLib/stdint.h
>  create mode 100644 MdePkg/Library/BaseFdtLib/stdlib.h
>  create mode 100644 MdePkg/Library/BaseFdtLib/string.h
> 
> --
> 2.39.1.windows.1
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102344): https://edk2.groups.io/g/devel/message/102344
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Sheng Lean Tan 1 year ago

Thanks for the nice feedback Pedro, Gerd and Andrei! Yeah it seems like a
valid concern here as Mik mentioned on edk2-libc, and it seems to fits edk2
long term interest on this.
Can we file this as an issue in Bugzilla for tracking or something? Since
this will take some time to work on this as it involves a bigger
discussion, personally I think we could get this FDT patch in first
meanwhile, and also remove the FDT from Embedded Pkg as next step, per
discussion with Leif?
What do you think?




On Sat, 1 Apr 2023 at 03:30, Andrei Warkentin <andrei.warkentin@intel.com>
wrote:

> How does this relate to the existing EmbeddedPkg/Library/FdtLib code? Is
> there a specific plan to move away from this in existing components?
>
> I did look in the BZ (https://bugzilla.tianocore.org/show_bug.cgi?id=4392)
> but this doesn't seem to acknowledge that there is existing Fdt support in
> EmbeddedPkg.
>
> A
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Benny
> > Lin
> > Sent: Thursday, March 30, 2023 11:52 AM
> > To: devel@edk2.groups.io
> > Cc: Lin, Benny <benny.lin@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> > Liu, Zhiguang <zhiguang.liu@intel.com>; Sean Brogan
> > <sean.brogan@microsoft.com>; Michael Kubacki
> > <mikuback@linux.microsoft.com>
> > Subject: [edk2-devel] [PATCH 0/2] Support FDT library.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4392
> > Add FDT support in EDK2 by submodule 3rd party libfdt
> > (https://github.com/devicetree-org/pylibfdt/tree/main/libfdt)
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> > Signed-off-by: Benny Lin <benny.lin@intel.com>
> >
> > Benny Lin (2):
> >   MdePkg: Support FDT library.
> >   .pytool: Support FDT library.
> >
> >  .gitmodules                               |   3 +
> >  .pytool/CISettings.py                     |   2 +
> >  MdePkg/Include/Library/FdtLib.h           | 300 ++++++++++++++++++++++
> >  MdePkg/Library/BaseFdtLib/BaseFdtLib.inf  |  62 +++++
> > MdePkg/Library/BaseFdtLib/BaseFdtLib.uni  |  14 +
> >  MdePkg/Library/BaseFdtLib/FdtLib.c        | 284 ++++++++++++++++++++
> >  MdePkg/Library/BaseFdtLib/LibFdtSupport.h | 102 ++++++++
> > MdePkg/Library/BaseFdtLib/LibFdtWrapper.c | 138 ++++++++++
> >  MdePkg/Library/BaseFdtLib/libfdt          |   1 +
> >  MdePkg/Library/BaseFdtLib/limits.h        |  10 +
> >  MdePkg/Library/BaseFdtLib/stdbool.h       |  10 +
> >  MdePkg/Library/BaseFdtLib/stddef.h        |  10 +
> >  MdePkg/Library/BaseFdtLib/stdint.h        |  10 +
> >  MdePkg/Library/BaseFdtLib/stdlib.h        |  10 +
> >  MdePkg/Library/BaseFdtLib/string.h        |  10 +
> >  MdePkg/MdePkg.ci.yaml                     |  17 +-
> >  MdePkg/MdePkg.dec                         |   4 +
> >  MdePkg/MdePkg.dsc                         |   2 +
> >  ReadMe.rst                                |   1 +
> >  19 files changed, 988 insertions(+), 2 deletions(-)  create mode 100644
> > MdePkg/Include/Library/FdtLib.h  create mode 100644
> > MdePkg/Library/BaseFdtLib/BaseFdtLib.inf
> >  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.uni
> >  create mode 100644 MdePkg/Library/BaseFdtLib/FdtLib.c
> >  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtSupport.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
> >  create mode 160000 MdePkg/Library/BaseFdtLib/libfdt  create mode 100644
> > MdePkg/Library/BaseFdtLib/limits.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/stdbool.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/stddef.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/stdint.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/stdlib.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/string.h
> >
> > --
> > 2.39.1.windows.1
> >
> >
> >
> >
> >
>
>
>
> 
>
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102625): https://edk2.groups.io/g/devel/message/102625
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Pedro Falcato 1 year ago

On Thu, Apr 6, 2023 at 5:34 PM Sheng Lean Tan <sheng.tan@9elements.com> wrote:
>
> Thanks for the nice feedback Pedro, Gerd and Andrei! Yeah it seems like a valid concern here as Mik mentioned on edk2-libc, and it seems to fits edk2 long term interest on this.
> Can we file this as an issue in Bugzilla for tracking or something? Since this will take some time to work on this as it involves a bigger discussion, personally I think we could get this FDT patch in first meanwhile, and also remove the FDT from Embedded Pkg as next step, per discussion with Leif?
> What do you think?

I'm all for not merging this without a proper solution in that regard
(I even presented a quick RFC solution which wasn't tested by anyone
involved in this patch, yet).

But if there really is an urgent need for this lib, I'm O-K with
merging this given that all my concerns are addressed (minus libc
duplication).

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102698): https://edk2.groups.io/g/devel/message/102698
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Andrei Warkentin 1 year ago

I think in general it would be nice to understand the long term picture of a change, esp. since there is already FDT support in EDK2 in various forms (with libraries and drivers depending on the existing FdtLib). So it would really of confusing to see another FDT library in MdePkg, without a clear reasoning for the work (this isn't reflected in the BZ) and a clear action plan to end up with just one FDT library in MdePkg in some identified time frame.

I do think FDT lib *does* belong in MdePkg, but it seems the shortest path to get there is to simply move the existing EmbeddedPkg one (and update all users). Subsequent cleanup can be incremental. And regardless, every existing FdtLib user ought to be updated to use the new one, so there need to be more patches (we're not just throwing the code over the wall, right?)

A

> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Friday, April 7, 2023 8:24 AM
> To: devel@edk2.groups.io; Tan, Lean Sheng <sheng.tan@9elements.com>
> Cc: Warkentin, Andrei <andrei.warkentin@intel.com>; Lin, Benny
> <benny.lin@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> <zhiguang.liu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> Michael Kubacki <mikuback@linux.microsoft.com>
> Subject: Re: [edk2-devel] [PATCH 0/2] Support FDT library.
> 
> On Thu, Apr 6, 2023 at 5:34 PM Sheng Lean Tan
> <sheng.tan@9elements.com> wrote:
> >
> > Thanks for the nice feedback Pedro, Gerd and Andrei! Yeah it seems like a
> valid concern here as Mik mentioned on edk2-libc, and it seems to fits edk2
> long term interest on this.
> > Can we file this as an issue in Bugzilla for tracking or something? Since this
> will take some time to work on this as it involves a bigger discussion,
> personally I think we could get this FDT patch in first meanwhile, and also
> remove the FDT from Embedded Pkg as next step, per discussion with Leif?
> > What do you think?
> 
> I'm all for not merging this without a proper solution in that regard (I even
> presented a quick RFC solution which wasn't tested by anyone involved in
> this patch, yet).
> 
> But if there really is an urgent need for this lib, I'm O-K with merging this
> given that all my concerns are addressed (minus libc duplication).
> 
> --
> Pedro

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102737): https://edk2.groups.io/g/devel/message/102737
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Michael D Kinney 1 year ago

The main advantage of the new lib is that it depends on a submodule for the FDT
content so we can easily move to new releases for bug fixes or features.

The FDT content in the EmbeddedPkg is a snap-shot of the code from a long time
ago.  We have been discouraging that approach and trying to move to released
content from a well support submodule if one is available.

Once this new better supported version is accepted, we can then incrementally
remove the duplicate content with the existing consumers moving from use of
EmbeddedPkg version to the MdePkg version and finally the removal of the 
duplicate content in the EmbeddedPkg.

Mike

> -----Original Message-----
> From: Warkentin, Andrei <andrei.warkentin@intel.com>
> Sent: Friday, April 7, 2023 3:36 PM
> To: Pedro Falcato <pedro.falcato@gmail.com>; devel@edk2.groups.io; Tan, Lean Sheng <sheng.tan@9elements.com>
> Cc: Lin, Benny <benny.lin@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> Liu, Zhiguang <zhiguang.liu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Michael Kubacki <mikuback@linux.microsoft.com>
> Subject: RE: [edk2-devel] [PATCH 0/2] Support FDT library.
> 
> I think in general it would be nice to understand the long term picture of a change, esp. since there is already FDT support in
> EDK2 in various forms (with libraries and drivers depending on the existing FdtLib). So it would really of confusing to see
> another FDT library in MdePkg, without a clear reasoning for the work (this isn't reflected in the BZ) and a clear action plan
> to end up with just one FDT library in MdePkg in some identified time frame.
> 
> I do think FDT lib *does* belong in MdePkg, but it seems the shortest path to get there is to simply move the existing
> EmbeddedPkg one (and update all users). Subsequent cleanup can be incremental. And regardless, every existing FdtLib user ought
> to be updated to use the new one, so there need to be more patches (we're not just throwing the code over the wall, right?)
> 
> A
> 
> > -----Original Message-----
> > From: Pedro Falcato <pedro.falcato@gmail.com>
> > Sent: Friday, April 7, 2023 8:24 AM
> > To: devel@edk2.groups.io; Tan, Lean Sheng <sheng.tan@9elements.com>
> > Cc: Warkentin, Andrei <andrei.warkentin@intel.com>; Lin, Benny
> > <benny.lin@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> > Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> > <zhiguang.liu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> > Michael Kubacki <mikuback@linux.microsoft.com>
> > Subject: Re: [edk2-devel] [PATCH 0/2] Support FDT library.
> >
> > On Thu, Apr 6, 2023 at 5:34 PM Sheng Lean Tan
> > <sheng.tan@9elements.com> wrote:
> > >
> > > Thanks for the nice feedback Pedro, Gerd and Andrei! Yeah it seems like a
> > valid concern here as Mik mentioned on edk2-libc, and it seems to fits edk2
> > long term interest on this.
> > > Can we file this as an issue in Bugzilla for tracking or something? Since this
> > will take some time to work on this as it involves a bigger discussion,
> > personally I think we could get this FDT patch in first meanwhile, and also
> > remove the FDT from Embedded Pkg as next step, per discussion with Leif?
> > > What do you think?
> >
> > I'm all for not merging this without a proper solution in that regard (I even
> > presented a quick RFC solution which wasn't tested by anyone involved in
> > this patch, yet).
> >
> > But if there really is an urgent need for this lib, I'm O-K with merging this
> > given that all my concerns are addressed (minus libc duplication).
> >
> > --
> > Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102740): https://edk2.groups.io/g/devel/message/102740
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Sheng Lean Tan 1 year ago

Thanks Mike for the proposal layout!
It sounds good to me :)

*Hi Pedro,*
I went through the email chain again, basically these are 2 of your main
concerns (correct me if I'm wrong):
1. a good idea to at least ditch that specific copy (current FDT in
Embedded Pkg) for a git submodule.
2. Rework to remove/reduce libc Implementation as there is a problem with
both libc fragments and compiler intrinsic fragments all over edk2. Should
unify standards between crypto, libfdt, etc, could we try here

I guess Mike has provided a plan to answer your first question, and the 2nd
question would require a broader discussion with a few key owners.

So it seems like we could get the current patchset from Benny Lin in for
now? Any minor clean up needed for the current patch?



On Sat, 8 Apr 2023 at 01:04, Kinney, Michael D <michael.d.kinney@intel.com>
wrote:

> The main advantage of the new lib is that it depends on a submodule for
> the FDT
> content so we can easily move to new releases for bug fixes or features.
>
> The FDT content in the EmbeddedPkg is a snap-shot of the code from a long
> time
> ago.  We have been discouraging that approach and trying to move to
> released
> content from a well support submodule if one is available.
>
> Once this new better supported version is accepted, we can then
> incrementally
> remove the duplicate content with the existing consumers moving from use of
> EmbeddedPkg version to the MdePkg version and finally the removal of the
> duplicate content in the EmbeddedPkg.
>
> Mike
>
> > -----Original Message-----
> > From: Warkentin, Andrei <andrei.warkentin@intel.com>
> > Sent: Friday, April 7, 2023 3:36 PM
> > To: Pedro Falcato <pedro.falcato@gmail.com>; devel@edk2.groups.io; Tan,
> Lean Sheng <sheng.tan@9elements.com>
> > Cc: Lin, Benny <benny.lin@intel.com>; Kinney, Michael D <
> michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> > Liu, Zhiguang <zhiguang.liu@intel.com>; Sean Brogan <
> sean.brogan@microsoft.com>; Michael Kubacki <mikuback@linux.microsoft.com>
> > Subject: RE: [edk2-devel] [PATCH 0/2] Support FDT library.
> >
> > I think in general it would be nice to understand the long term picture
> of a change, esp. since there is already FDT support in
> > EDK2 in various forms (with libraries and drivers depending on the
> existing FdtLib). So it would really of confusing to see
> > another FDT library in MdePkg, without a clear reasoning for the work
> (this isn't reflected in the BZ) and a clear action plan
> > to end up with just one FDT library in MdePkg in some identified time
> frame.
> >
> > I do think FDT lib *does* belong in MdePkg, but it seems the shortest
> path to get there is to simply move the existing
> > EmbeddedPkg one (and update all users). Subsequent cleanup can be
> incremental. And regardless, every existing FdtLib user ought
> > to be updated to use the new one, so there need to be more patches
> (we're not just throwing the code over the wall, right?)
> >
> > A
> >
> > > -----Original Message-----
> > > From: Pedro Falcato <pedro.falcato@gmail.com>
> > > Sent: Friday, April 7, 2023 8:24 AM
> > > To: devel@edk2.groups.io; Tan, Lean Sheng <sheng.tan@9elements.com>
> > > Cc: Warkentin, Andrei <andrei.warkentin@intel.com>; Lin, Benny
> > > <benny.lin@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> > > Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> > > <zhiguang.liu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> > > Michael Kubacki <mikuback@linux.microsoft.com>
> > > Subject: Re: [edk2-devel] [PATCH 0/2] Support FDT library.
> > >
> > > On Thu, Apr 6, 2023 at 5:34 PM Sheng Lean Tan
> > > <sheng.tan@9elements.com> wrote:
> > > >
> > > > Thanks for the nice feedback Pedro, Gerd and Andrei! Yeah it seems
> like a
> > > valid concern here as Mik mentioned on edk2-libc, and it seems to fits
> edk2
> > > long term interest on this.
> > > > Can we file this as an issue in Bugzilla for tracking or something?
> Since this
> > > will take some time to work on this as it involves a bigger discussion,
> > > personally I think we could get this FDT patch in first meanwhile, and
> also
> > > remove the FDT from Embedded Pkg as next step, per discussion with
> Leif?
> > > > What do you think?
> > >
> > > I'm all for not merging this without a proper solution in that regard
> (I even
> > > presented a quick RFC solution which wasn't tested by anyone involved
> in
> > > this patch, yet).
> > >
> > > But if there really is an urgent need for this lib, I'm O-K with
> merging this
> > > given that all my concerns are addressed (minus libc duplication).
> > >
> > > --
> > > Pedro
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102830): https://edk2.groups.io/g/devel/message/102830
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Pedro Falcato 1 year ago

On Tue, Apr 11, 2023 at 2:20 PM Lean Sheng Tan <sheng.tan@9elements.com> wrote:
>
> Thanks Mike for the proposal layout!
> It sounds good to me :)
>
> Hi Pedro,
> I went through the email chain again, basically these are 2 of your main concerns (correct me if I'm wrong):
> 1. a good idea to at least ditch that specific copy (current FDT in Embedded Pkg) for a git submodule.
> 2. Rework to remove/reduce libc Implementation as there is a problem with both libc fragments and compiler intrinsic fragments all over edk2. Should unify standards between crypto, libfdt, etc, could we try here
>
> I guess Mike has provided a plan to answer your first question, and the 2nd question would require a broader discussion with a few key owners.
>
> So it seems like we could get the current patchset from Benny Lin in for now? Any minor clean up needed for the current patch?
>

No.

3. Lots of questions and comments on the actual patch set regarding
the quality of the libc implementation. Which should be fixed,
regardless of centralizing a libc implementation.
    Also questions on the FdtLib itself (why are we wrapping pure
libfdt functions with FluffyIdentifierNames and SCARY_TYPEDEFS?).
    I also sent out an RFC for a central libc for GCC/clang based
toolchains, which should cover the libc usage of libfdt. Asked for
testing, got ignored.

So a NAK from me, in its current state.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102845): https://edk2.groups.io/g/devel/message/102845
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Michael D Kinney 1 year ago


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pedro Falcato
> Sent: Tuesday, April 11, 2023 9:07 AM
> To: Tan, Lean Sheng <sheng.tan@9elements.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Warkentin, Andrei <andrei.warkentin@intel.com>; devel@edk2.groups.io;
> Lin, Benny <benny.lin@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Sean
> Brogan <sean.brogan@microsoft.com>; Michael Kubacki <mikuback@linux.microsoft.com>
> Subject: Re: [edk2-devel] [PATCH 0/2] Support FDT library.
> 
> On Tue, Apr 11, 2023 at 2:20 PM Lean Sheng Tan <sheng.tan@9elements.com> wrote:
> >
> > Thanks Mike for the proposal layout!
> > It sounds good to me :)
> >
> > Hi Pedro,
> > I went through the email chain again, basically these are 2 of your main concerns (correct me if I'm wrong):
> > 1. a good idea to at least ditch that specific copy (current FDT in Embedded Pkg) for a git submodule.
> > 2. Rework to remove/reduce libc Implementation as there is a problem with both libc fragments and compiler intrinsic
> fragments all over edk2. Should unify standards between crypto, libfdt, etc, could we try here
> >
> > I guess Mike has provided a plan to answer your first question, and the 2nd question would require a broader discussion
> with a few key owners.
> >
> > So it seems like we could get the current patchset from Benny Lin in for now? Any minor clean up needed for the current
> patch?
> >
> 
> No.
> 
> 3. Lots of questions and comments on the actual patch set regarding
> the quality of the libc implementation. Which should be fixed,
> regardless of centralizing a libc implementation.
>     Also questions on the FdtLib itself (why are we wrapping pure
> libfdt functions with FluffyIdentifierNames and SCARY_TYPEDEFS?).

This is done to accommodate potential future changes to the submodule APIs
and types.  The wrapper lib is a common practice in EDK II use of submodules.
The calling code can use the APIs/types defined in EDK II lib class.  If
there are changes to the submodule APIs/types, we only have to update one
location. Can also provide flexibility to move to a different submodule or
implementation if there is a better option in the future.

Look at CryptoPkg CryptoLibs as an example. We have an implementation that
layers on top of openssl.  We also have experiments looking at mbedtls. 
No changes to calling code that uses CrytoLibs.

>     I also sent out an RFC for a central libc for GCC/clang based
> toolchains, which should cover the libc usage of libfdt. Asked for
> testing, got ignored.

I provided feedback on your work on a central libc and asked if you are
able to test the libc uses currently checked into edk2.  Are you able to
help with that testing?

> 
> So a NAK from me, in its current state.
> 
> --
> Pedro
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102846): https://edk2.groups.io/g/devel/message/102846
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Pedro Falcato 1 year, 1 month ago

On Thu, Mar 30, 2023 at 6:13 PM Benny Lin <benny.lin@intel.com> wrote:
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4392
> Add FDT support in EDK2 by submodule 3rd party libfdt
> (https://github.com/devicetree-org/pylibfdt/tree/main/libfdt)
>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Signed-off-by: Benny Lin <benny.lin@intel.com>
>
> Benny Lin (2):
>   MdePkg: Support FDT library.
>   .pytool: Support FDT library.
>
>  .gitmodules                               |   3 +
>  .pytool/CISettings.py                     |   2 +
>  MdePkg/Include/Library/FdtLib.h           | 300 ++++++++++++++++++++++
>  MdePkg/Library/BaseFdtLib/BaseFdtLib.inf  |  62 +++++
>  MdePkg/Library/BaseFdtLib/BaseFdtLib.uni  |  14 +
>  MdePkg/Library/BaseFdtLib/FdtLib.c        | 284 ++++++++++++++++++++
>  MdePkg/Library/BaseFdtLib/LibFdtSupport.h | 102 ++++++++
>  MdePkg/Library/BaseFdtLib/LibFdtWrapper.c | 138 ++++++++++
>  MdePkg/Library/BaseFdtLib/libfdt          |   1 +
>  MdePkg/Library/BaseFdtLib/limits.h        |  10 +
>  MdePkg/Library/BaseFdtLib/stdbool.h       |  10 +
>  MdePkg/Library/BaseFdtLib/stddef.h        |  10 +
>  MdePkg/Library/BaseFdtLib/stdint.h        |  10 +
>  MdePkg/Library/BaseFdtLib/stdlib.h        |  10 +
>  MdePkg/Library/BaseFdtLib/string.h        |  10 +
>  MdePkg/MdePkg.ci.yaml                     |  17 +-
>  MdePkg/MdePkg.dec                         |   4 +
>  MdePkg/MdePkg.dsc                         |   2 +
>  ReadMe.rst                                |   1 +
>  19 files changed, 988 insertions(+), 2 deletions(-)
>  create mode 100644 MdePkg/Include/Library/FdtLib.h
>  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.inf
>  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.uni
>  create mode 100644 MdePkg/Library/BaseFdtLib/FdtLib.c
>  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtSupport.h
>  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
>  create mode 160000 MdePkg/Library/BaseFdtLib/libfdt
>  create mode 100644 MdePkg/Library/BaseFdtLib/limits.h
>  create mode 100644 MdePkg/Library/BaseFdtLib/stdbool.h
>  create mode 100644 MdePkg/Library/BaseFdtLib/stddef.h
>  create mode 100644 MdePkg/Library/BaseFdtLib/stdint.h
>  create mode 100644 MdePkg/Library/BaseFdtLib/stdlib.h
>  create mode 100644 MdePkg/Library/BaseFdtLib/string.h
>
> --
> 2.39.1.windows.1

There's already a copy of libfdt plus "FdtLib" at
https://github.com/tianocore/edk2/tree/master/EmbeddedPkg/Library/FdtLib.
Please figure out what to do with it.
It's an old copy and has been accidentally uncrustify'd so it's
probably a good idea to at least ditch that specific copy for a git
submodule.

Also, NAK to Yet Another libc Implementation (and not a particularly
good one at that).

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102218): https://edk2.groups.io/g/devel/message/102218
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Leif Lindholm 1 year, 1 month ago

On Thu, Mar 30, 2023 at 22:50:18 +0100, Pedro Falcato wrote:
> On Thu, Mar 30, 2023 at 6:13 PM Benny Lin <benny.lin@intel.com> wrote:
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4392
> > Add FDT support in EDK2 by submodule 3rd party libfdt
> > (https://github.com/devicetree-org/pylibfdt/tree/main/libfdt)
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> > Signed-off-by: Benny Lin <benny.lin@intel.com>
> >
> > Benny Lin (2):
> >   MdePkg: Support FDT library.
> >   .pytool: Support FDT library.
> >
> >  .gitmodules                               |   3 +
> >  .pytool/CISettings.py                     |   2 +
> >  MdePkg/Include/Library/FdtLib.h           | 300 ++++++++++++++++++++++
> >  MdePkg/Library/BaseFdtLib/BaseFdtLib.inf  |  62 +++++
> >  MdePkg/Library/BaseFdtLib/BaseFdtLib.uni  |  14 +
> >  MdePkg/Library/BaseFdtLib/FdtLib.c        | 284 ++++++++++++++++++++
> >  MdePkg/Library/BaseFdtLib/LibFdtSupport.h | 102 ++++++++
> >  MdePkg/Library/BaseFdtLib/LibFdtWrapper.c | 138 ++++++++++
> >  MdePkg/Library/BaseFdtLib/libfdt          |   1 +
> >  MdePkg/Library/BaseFdtLib/limits.h        |  10 +
> >  MdePkg/Library/BaseFdtLib/stdbool.h       |  10 +
> >  MdePkg/Library/BaseFdtLib/stddef.h        |  10 +
> >  MdePkg/Library/BaseFdtLib/stdint.h        |  10 +
> >  MdePkg/Library/BaseFdtLib/stdlib.h        |  10 +
> >  MdePkg/Library/BaseFdtLib/string.h        |  10 +
> >  MdePkg/MdePkg.ci.yaml                     |  17 +-
> >  MdePkg/MdePkg.dec                         |   4 +
> >  MdePkg/MdePkg.dsc                         |   2 +
> >  ReadMe.rst                                |   1 +
> >  19 files changed, 988 insertions(+), 2 deletions(-)
> >  create mode 100644 MdePkg/Include/Library/FdtLib.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.inf
> >  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.uni
> >  create mode 100644 MdePkg/Library/BaseFdtLib/FdtLib.c
> >  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtSupport.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
> >  create mode 160000 MdePkg/Library/BaseFdtLib/libfdt
> >  create mode 100644 MdePkg/Library/BaseFdtLib/limits.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/stdbool.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/stddef.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/stdint.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/stdlib.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/string.h
> >
> > --
> > 2.39.1.windows.1
> 
> There's already a copy of libfdt plus "FdtLib" at
> https://github.com/tianocore/edk2/tree/master/EmbeddedPkg/Library/FdtLib.
> Please figure out what to do with it.

Like much of EmbeddedPkg, it's a halfway-house minimal effort legacy thing.
As soon as the actively maintained platforms have moved away from it,
I will delete it.

> It's an old copy and has been accidentally uncrustify'd so it's
> probably a good idea to at least ditch that specific copy for a git
> submodule.

Yes, but a migration path for existing users is preferable to breaking
the world.

/
    Leif


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102306): https://edk2.groups.io/g/devel/message/102306
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Michael D Kinney 1 year, 1 month ago


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pedro Falcato
> Sent: Thursday, March 30, 2023 2:50 PM
> To: devel@edk2.groups.io; Lin, Benny <benny.lin@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> <zhiguang.liu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Michael Kubacki <mikuback@linux.microsoft.com>
> Subject: Re: [edk2-devel] [PATCH 0/2] Support FDT library.
> 
> On Thu, Mar 30, 2023 at 6:13 PM Benny Lin <benny.lin@intel.com> wrote:
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4392
> > Add FDT support in EDK2 by submodule 3rd party libfdt
> > (https://github.com/devicetree-org/pylibfdt/tree/main/libfdt)
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> > Signed-off-by: Benny Lin <benny.lin@intel.com>
> >
> > Benny Lin (2):
> >   MdePkg: Support FDT library.
> >   .pytool: Support FDT library.
> >
> >  .gitmodules                               |   3 +
> >  .pytool/CISettings.py                     |   2 +
> >  MdePkg/Include/Library/FdtLib.h           | 300 ++++++++++++++++++++++
> >  MdePkg/Library/BaseFdtLib/BaseFdtLib.inf  |  62 +++++
> >  MdePkg/Library/BaseFdtLib/BaseFdtLib.uni  |  14 +
> >  MdePkg/Library/BaseFdtLib/FdtLib.c        | 284 ++++++++++++++++++++
> >  MdePkg/Library/BaseFdtLib/LibFdtSupport.h | 102 ++++++++
> >  MdePkg/Library/BaseFdtLib/LibFdtWrapper.c | 138 ++++++++++
> >  MdePkg/Library/BaseFdtLib/libfdt          |   1 +
> >  MdePkg/Library/BaseFdtLib/limits.h        |  10 +
> >  MdePkg/Library/BaseFdtLib/stdbool.h       |  10 +
> >  MdePkg/Library/BaseFdtLib/stddef.h        |  10 +
> >  MdePkg/Library/BaseFdtLib/stdint.h        |  10 +
> >  MdePkg/Library/BaseFdtLib/stdlib.h        |  10 +
> >  MdePkg/Library/BaseFdtLib/string.h        |  10 +
> >  MdePkg/MdePkg.ci.yaml                     |  17 +-
> >  MdePkg/MdePkg.dec                         |   4 +
> >  MdePkg/MdePkg.dsc                         |   2 +
> >  ReadMe.rst                                |   1 +
> >  19 files changed, 988 insertions(+), 2 deletions(-)
> >  create mode 100644 MdePkg/Include/Library/FdtLib.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.inf
> >  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.uni
> >  create mode 100644 MdePkg/Library/BaseFdtLib/FdtLib.c
> >  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtSupport.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
> >  create mode 160000 MdePkg/Library/BaseFdtLib/libfdt
> >  create mode 100644 MdePkg/Library/BaseFdtLib/limits.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/stdbool.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/stddef.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/stdint.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/stdlib.h
> >  create mode 100644 MdePkg/Library/BaseFdtLib/string.h
> >
> > --
> > 2.39.1.windows.1
> 
> There's already a copy of libfdt plus "FdtLib" at
> https://github.com/tianocore/edk2/tree/master/EmbeddedPkg/Library/FdtLib.
> Please figure out what to do with it.
> It's an old copy and has been accidentally uncrustify'd so it's
> probably a good idea to at least ditch that specific copy for a git
> submodule.

I have discussed this overlap with Leif. After this patch series is 
added, the EmbeddedPkg maintainers can convert that package to use
this lib and delete the duplicate sources.

> 
> Also, NAK to Yet Another libc Implementation (and not a particularly
> good one at that).

Please provide constructive feedback on what is not good about this specific libc
Implementation so that appropriate code updates could be made for this patch series.

This is following the same pattern as other libs that are consuming a submodule
that requires some amount of libc support.

Getting down to one libc wrapper would be great.  Do you want to provide a proposed
implementation?  That could be handled as a separate task.

> 
> --
> Pedro
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102221): https://edk2.groups.io/g/devel/message/102221
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Pedro Falcato 1 year, 1 month ago

On Thu, Mar 30, 2023 at 11:59 PM Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pedro Falcato
> > Sent: Thursday, March 30, 2023 2:50 PM
> > To: devel@edk2.groups.io; Lin, Benny <benny.lin@intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> > <zhiguang.liu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Michael Kubacki <mikuback@linux.microsoft.com>
> > Subject: Re: [edk2-devel] [PATCH 0/2] Support FDT library.
> >
> > On Thu, Mar 30, 2023 at 6:13 PM Benny Lin <benny.lin@intel.com> wrote:
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4392
> > > Add FDT support in EDK2 by submodule 3rd party libfdt
> > > (https://github.com/devicetree-org/pylibfdt/tree/main/libfdt)
> > >
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> > > Signed-off-by: Benny Lin <benny.lin@intel.com>
> > >
> > > Benny Lin (2):
> > >   MdePkg: Support FDT library.
> > >   .pytool: Support FDT library.
> > >
> > >  .gitmodules                               |   3 +
> > >  .pytool/CISettings.py                     |   2 +
> > >  MdePkg/Include/Library/FdtLib.h           | 300 ++++++++++++++++++++++
> > >  MdePkg/Library/BaseFdtLib/BaseFdtLib.inf  |  62 +++++
> > >  MdePkg/Library/BaseFdtLib/BaseFdtLib.uni  |  14 +
> > >  MdePkg/Library/BaseFdtLib/FdtLib.c        | 284 ++++++++++++++++++++
> > >  MdePkg/Library/BaseFdtLib/LibFdtSupport.h | 102 ++++++++
> > >  MdePkg/Library/BaseFdtLib/LibFdtWrapper.c | 138 ++++++++++
> > >  MdePkg/Library/BaseFdtLib/libfdt          |   1 +
> > >  MdePkg/Library/BaseFdtLib/limits.h        |  10 +
> > >  MdePkg/Library/BaseFdtLib/stdbool.h       |  10 +
> > >  MdePkg/Library/BaseFdtLib/stddef.h        |  10 +
> > >  MdePkg/Library/BaseFdtLib/stdint.h        |  10 +
> > >  MdePkg/Library/BaseFdtLib/stdlib.h        |  10 +
> > >  MdePkg/Library/BaseFdtLib/string.h        |  10 +
> > >  MdePkg/MdePkg.ci.yaml                     |  17 +-
> > >  MdePkg/MdePkg.dec                         |   4 +
> > >  MdePkg/MdePkg.dsc                         |   2 +
> > >  ReadMe.rst                                |   1 +
> > >  19 files changed, 988 insertions(+), 2 deletions(-)
> > >  create mode 100644 MdePkg/Include/Library/FdtLib.h
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.inf
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.uni
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/FdtLib.c
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtSupport.h
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
> > >  create mode 160000 MdePkg/Library/BaseFdtLib/libfdt
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/limits.h
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/stdbool.h
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/stddef.h
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/stdint.h
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/stdlib.h
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/string.h
> > >
> > > --
> > > 2.39.1.windows.1
> >
> > There's already a copy of libfdt plus "FdtLib" at
> > https://github.com/tianocore/edk2/tree/master/EmbeddedPkg/Library/FdtLib.
> > Please figure out what to do with it.
> > It's an old copy and has been accidentally uncrustify'd so it's
> > probably a good idea to at least ditch that specific copy for a git
> > submodule.
>
> I have discussed this overlap with Leif. After this patch series is
> added, the EmbeddedPkg maintainers can convert that package to use
> this lib and delete the duplicate sources.
Ok, SGTM.
>
> >
> > Also, NAK to Yet Another libc Implementation (and not a particularly
> > good one at that).
>
> Please provide constructive feedback on what is not good about this specific libc
> Implementation so that appropriate code updates could be made for this patch series.

Done.
> This is following the same pattern as other libs that are consuming a submodule
> that requires some amount of libc support.
>
> Getting down to one libc wrapper would be great.  Do you want to provide a proposed
> implementation?  That could be handled as a separate task.

I would like it if we could stop contributing to that problem. We very
much *know* there is a problem with both libc fragments and compiler
intrinsic fragments all over edk2.
A proper, correct C standard library is not trivial to implement
(hence the multiple problems I did find from a quick read of the
patch).
We also have a whole libc implementation in edk2-libc that seems to be
permanently gathering dust until Intel touches it for Python purposes
from time to time. So between crypto, libfdt, etc, could we try to
unify things here a bit?

Thanks,
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102223): https://edk2.groups.io/g/devel/message/102223
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Leif Lindholm 1 year, 1 month ago

On Fri, Mar 31, 2023 at 00:26:37 +0100, Pedro Falcato wrote:
> We also have a whole libc implementation in edk2-libc that seems to be
> permanently gathering dust until Intel touches it for Python purposes
> from time to time. So between crypto, libfdt, etc, could we try to
> unify things here a bit?

Personally, I'd quite like to nuke edk2-libc. The only effect I'm
seeing from it is that it keeps misleading people into believing it's
sensible to expect full POSIX compliance under UEFI, then embed those
expectations into their organisational workflows.

Meanwhile, it receives (next to) no security fixes.

/
    Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102307): https://edk2.groups.io/g/devel/message/102307
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Gerd Hoffmann 1 year, 1 month ago

  Hi,

> > Getting down to one libc wrapper would be great.  Do you want to provide a proposed
> > implementation?  That could be handled as a separate task.
> 
> I would like it if we could stop contributing to that problem. We very
> much *know* there is a problem with both libc fragments and compiler
> intrinsic fragments all over edk2.

I'd suggest to start with what we already have in the tree.  Which is
(not fully sure the list is actually complete):

 - CryptoPkg/Library/Include/ carrying the bits needed to make openssl
   compile, and
 - CryptoPkg/Library/IntrinsicLib (again, for openssl, mostly IA32, some
   X64) and,
 - ArmPkg/Library/CompilerIntrinsicsLib (mostly ARM, some AARCH64).

Can we move that over to MdePkg so everyone can easily share it instead
of adding more copies of the same to the tree?

I have an old branch gathering dust doing that for the intrinsics, I can
try rebasing it to latest master next week ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102303): https://edk2.groups.io/g/devel/message/102303
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Pedro Falcato 1 year, 1 month ago

On Fri, Mar 31, 2023 at 12:39 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > Getting down to one libc wrapper would be great.  Do you want to provide a proposed
> > > implementation?  That could be handled as a separate task.
> >
> > I would like it if we could stop contributing to that problem. We very
> > much *know* there is a problem with both libc fragments and compiler
> > intrinsic fragments all over edk2.
>
> I'd suggest to start with what we already have in the tree.  Which is
> (not fully sure the list is actually complete):
>
>  - CryptoPkg/Library/Include/ carrying the bits needed to make openssl
>    compile, and
>  - CryptoPkg/Library/IntrinsicLib (again, for openssl, mostly IA32, some
>    X64) and,
>  - ArmPkg/Library/CompilerIntrinsicsLib (mostly ARM, some AARCH64).
>
> Can we move that over to MdePkg so everyone can easily share it instead
> of adding more copies of the same to the tree?

Yes please. The problem with starting with what's in the tree is that
it's very messy and sometimes of super dubious quality.
For instance, on CryptoPkg there's the same lack of rigour in the
headers and CrtWrapper.c that I would rather avoid, as to make this a
relatively proper thing

(did you know if OpenSSL strcpy's over 4KiB it silently fails?).

> I have an old branch gathering dust doing that for the intrinsics, I can
> try rebasing it to latest master next week ...

Yes please! I did think about bringing in compiler-rt from LLVM for
high quality relatively self-contained intrinsics a good while ago.
There's the open question that GCC and clang require intrinsics and a
very small set of libc string functions to properly generate code.
Could BaseTools implicitly link this in? If we do it correctly,
there's no reason why the binaries would grow beyond what it requires
from the intrinsics set.
And I actually did see problems with lacking memcpy when trying out
tree-wide GCC12/clang -ftrivial-auto-var-init...

Anyway, if you want help with this or want to sync up efforts, do ping
me off-list :)

-- 
Pedro

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102308): https://edk2.groups.io/g/devel/message/102308
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Michael D Kinney 1 year, 1 month ago


> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Thursday, March 30, 2023 4:27 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Lin, Benny <benny.lin@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> <zhiguang.liu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Leif
> Lindholm <llindhol@qti.qualcomm.com>
> Subject: Re: [edk2-devel] [PATCH 0/2] Support FDT library.
> 
> On Thu, Mar 30, 2023 at 11:59 PM Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pedro Falcato
> > > Sent: Thursday, March 30, 2023 2:50 PM
> > > To: devel@edk2.groups.io; Lin, Benny <benny.lin@intel.com>
> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> > > <zhiguang.liu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Michael Kubacki <mikuback@linux.microsoft.com>
> > > Subject: Re: [edk2-devel] [PATCH 0/2] Support FDT library.
> > >
> > > On Thu, Mar 30, 2023 at 6:13 PM Benny Lin <benny.lin@intel.com> wrote:
> > > >
> > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4392
> > > > Add FDT support in EDK2 by submodule 3rd party libfdt
> > > > (https://github.com/devicetree-org/pylibfdt/tree/main/libfdt)
> > > >
> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > > Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> > > > Signed-off-by: Benny Lin <benny.lin@intel.com>
> > > >
> > > > Benny Lin (2):
> > > >   MdePkg: Support FDT library.
> > > >   .pytool: Support FDT library.
> > > >
> > > >  .gitmodules                               |   3 +
> > > >  .pytool/CISettings.py                     |   2 +
> > > >  MdePkg/Include/Library/FdtLib.h           | 300 ++++++++++++++++++++++
> > > >  MdePkg/Library/BaseFdtLib/BaseFdtLib.inf  |  62 +++++
> > > >  MdePkg/Library/BaseFdtLib/BaseFdtLib.uni  |  14 +
> > > >  MdePkg/Library/BaseFdtLib/FdtLib.c        | 284 ++++++++++++++++++++
> > > >  MdePkg/Library/BaseFdtLib/LibFdtSupport.h | 102 ++++++++
> > > >  MdePkg/Library/BaseFdtLib/LibFdtWrapper.c | 138 ++++++++++
> > > >  MdePkg/Library/BaseFdtLib/libfdt          |   1 +
> > > >  MdePkg/Library/BaseFdtLib/limits.h        |  10 +
> > > >  MdePkg/Library/BaseFdtLib/stdbool.h       |  10 +
> > > >  MdePkg/Library/BaseFdtLib/stddef.h        |  10 +
> > > >  MdePkg/Library/BaseFdtLib/stdint.h        |  10 +
> > > >  MdePkg/Library/BaseFdtLib/stdlib.h        |  10 +
> > > >  MdePkg/Library/BaseFdtLib/string.h        |  10 +
> > > >  MdePkg/MdePkg.ci.yaml                     |  17 +-
> > > >  MdePkg/MdePkg.dec                         |   4 +
> > > >  MdePkg/MdePkg.dsc                         |   2 +
> > > >  ReadMe.rst                                |   1 +
> > > >  19 files changed, 988 insertions(+), 2 deletions(-)
> > > >  create mode 100644 MdePkg/Include/Library/FdtLib.h
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.inf
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.uni
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/FdtLib.c
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtSupport.h
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
> > > >  create mode 160000 MdePkg/Library/BaseFdtLib/libfdt
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/limits.h
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/stdbool.h
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/stddef.h
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/stdint.h
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/stdlib.h
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/string.h
> > > >
> > > > --
> > > > 2.39.1.windows.1
> > >
> > > There's already a copy of libfdt plus "FdtLib" at
> > > https://github.com/tianocore/edk2/tree/master/EmbeddedPkg/Library/FdtLib.
> > > Please figure out what to do with it.
> > > It's an old copy and has been accidentally uncrustify'd so it's
> > > probably a good idea to at least ditch that specific copy for a git
> > > submodule.
> >
> > I have discussed this overlap with Leif. After this patch series is
> > added, the EmbeddedPkg maintainers can convert that package to use
> > this lib and delete the duplicate sources.
> Ok, SGTM.
> >
> > >
> > > Also, NAK to Yet Another libc Implementation (and not a particularly
> > > good one at that).
> >
> > Please provide constructive feedback on what is not good about this specific libc
> > Implementation so that appropriate code updates could be made for this patch series.
> 
> Done.
> > This is following the same pattern as other libs that are consuming a submodule
> > that requires some amount of libc support.
> >
> > Getting down to one libc wrapper would be great.  Do you want to provide a proposed
> > implementation?  That could be handled as a separate task.
> 
> I would like it if we could stop contributing to that problem. We very
> much *know* there is a problem with both libc fragments and compiler
> intrinsic fragments all over edk2.
> A proper, correct C standard library is not trivial to implement
> (hence the multiple problems I did find from a quick read of the
> patch).

Appreciate the feedback.  Agree that any libc API that is implemented in a
wrapper should conform to the standard.

> We also have a whole libc implementation in edk2-libc that seems to be
> permanently gathering dust until Intel touches it for Python purposes
> from time to time. So between crypto, libfdt, etc, could we try to
> unify things here a bit?

edk2-libc to too large for FW components and it is out of date.

Would you like to volunteer to lead a design and implementation that
is right-sized for FW modules and could be the one wrapper that works
for all current use cases and could be extended in the future as 
needed to support additional use cases?  Don’t need all of libc.  Just
enough to support the APIs used by the submodules used so far.

> 
> Thanks,
> Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102224): https://edk2.groups.io/g/devel/message/102224
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Support FDT library.

Posted by Pedro Falcato 1 year, 1 month ago

On Fri, Mar 31, 2023 at 12:32 AM Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
> Appreciate the feedback.  Agree that any libc API that is implemented in a
> wrapper should conform to the standard.
>
> > We also have a whole libc implementation in edk2-libc that seems to be
> > permanently gathering dust until Intel touches it for Python purposes
> > from time to time. So between crypto, libfdt, etc, could we try to
> > unify things here a bit?
>
> edk2-libc to too large for FW components and it is out of date.
>
> Would you like to volunteer to lead a design and implementation that
> is right-sized for FW modules and could be the one wrapper that works
> for all current use cases and could be extended in the future as
> needed to support additional use cases?  Don’t need all of libc.  Just
> enough to support the APIs used by the submodules used so far.
>

Mike,

I wrote up a quick RFC patch for a bunch of libc bits that you needed
in this case (BaseFdtLib and libfdt).
It's very much a WIP and only supports GCC/clang. MSVC needs some
support when it comes to limits.h (because of LP64 vs Windows's
LLP64), but nothing too hard certainly.
See https://edk2.groups.io/g/devel/topic/rfc_patch_1_1_mdepkg_add_a/97965830?p=,,,20,0,0,0::recentpostdate/sticky,,,20,2,0,97965830,previd%3D1680229851681438282,nextid%3D1680190220621190228&previd=1680229851681438282&nextid=1680190220621190228

Comments welcome.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102229): https://edk2.groups.io/g/devel/message/102229
Mute This Topic: https://groups.io/mt/97955736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-