Re: [edk2-devel] [PATCH v5 2/6] CryptoPkg/CryptoPkg.ci.yaml: Allow dependency upon ArmPkg

["Leif Lindholm" <quic_llindhol@quicinc.com>]

On Tue, Nov 21, 2023 at 14:46:05 +0000, Yao, Jiewen wrote:
> This Bugzilla is filed in 2022-10-26. Now it is 2023-11-21.

Oh, I'm sure I voiced the same opinion for many years before someone
(rightly) told me to go gile that bugzilla.

> I agree with you that it is a big task. May I know what is the plan?
> E.g. who is working on that? When do you expect it will be done?

On my list of "big items" to deal with, this comes after github PR
migration and line-ending conversion.

> According to the dependency rule, what we need is only *interface*
> definition, but not *implementation*.
> That means the really requirement here is to move *interface* from
> ArmPkg to MdePkg, you can still keep the library implementation in
> ArmPkg. (It is just a subset of this Bugzilla)

That ... is an option I had not previously considered.
Long-term we would still like to smash ArmLib into BaseLib, but if
MdePkg maintainers would be OK with moving ArmLib.h into MdePkg...

> Also, I don’t think CPUID check really matters here - since it is only implementation.
> As long as, you have interface in MdePkg, then your INF can declare that interface.
> You can still put real implementation in ArmPkg - no requirement to move.
> That benefit is that you don’t need to add ArmPkg dependency in yaml.

I can spin up a patch for that to get merged shortly after stable tag
to give plenty of time to catch any issues that may arise from moving
such a fundamental file. (These would likely be bugs, but
nevertheless...)

Thanks!

/
    Leif


> Thank you
> Yao, Jiewen
> 
> > -----Original Message-----
> > From: Leif Lindholm <quic_llindhol@quicinc.com>
> > Sent: Tuesday, November 21, 2023 10:26 PM
> > To: Yao, Jiewen <jiewen.yao@intel.com>
> > Cc: Pierre Gondois <pierre.gondois@arm.com>; devel@edk2.groups.io; Li, Yi1
> > <yi1.li@intel.com>; Lu, Xiaoyu1 <xiaoyu1.lu@intel.com>; Jiang, Guomin
> > <guomin.jiang@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami
> > Mujawar <sami.mujawar@arm.com>; Gerd Hoffmann <kraxel@redhat.com>
> > Subject: Re: [PATCH v5 2/6] CryptoPkg/CryptoPkg.ci.yaml: Allow dependency
> > upon ArmPkg
> > 
> > Hi Jiewen,
> > 
> > On Tue, Nov 21, 2023 at 13:41:21 +0000, Yao, Jiewen wrote:
> > > Thanks to let me know the background.
> > >
> > > Please be aware that there is fundamental difference between
> > > dependency in INF and dependency in DSC.
> > >
> > > What we have previously in the ArmPkg in *DSC*. We don’t need add
> > > ArmPkg in yaml.
> > > However, what you try to introduce is ArmPkg in *INF*, e.g. your
> > > patch v5 5/6. Then we have to add ArmPkg in yaml.
> > >
> > > Personally, I don’t think it is a good idea to add ArmPkg to yaml,
> > > because it means that you have to pull ArmPkg when you build
> > > CryptoPkg,.
> > >
> > > As long as what you add is industry standard, it is OK to add to
> > > MdePkg, like what you did in v2. I would like to suggest this
> > > approach.
> > 
> > Ultimately, all of ArmPkg needs to migrate to MdePkg.
> > See https://bugzilla.tianocore.org/show_bug.cgi?id=4121
> > But this is a BIG task.
> > 
> > The reason I asked Pierre to add this functionality in ArmPkg rather
> > than MdePkg is because that is where the existing related discovery
> > code lives. (Think of it as CPUID.)
> > 
> > For historical reasons, predating mine and Ard's involvement with
> > edk2, this functionality (as well as other critical Arm functionality)
> > lives in a library called ArmLib, under ArmPkg.
> > For Ia32/X64, all such support lives in BaseLib, under MdePkg.
> > 
> > This is why I referred to ArmPkg as an exclave of MdePkg in my
> > original reply to Pierre. And until someone untangles this, it's not
> > realistic to treat ArmPkg as anything else.
> > 
> > And I don't think it's fair to expect Pierre to untangle this as part
> > of this series. But I also don't think "Arm architectures need to
> > duplicate their basic support code across multiple packages" is a
> > solution.
> > 
> > Regards,
> > 
> > Leif
> > 
> > > But I would like to have ARM expert to check if those are really ARM
> > > standard, and also have MdePkg owner check if it is acceptable.
> > >
> > > Thank you
> > > Yao, Jiewen
> > >
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Pierre Gondois <pierre.gondois@arm.com>
> > > > Sent: Tuesday, November 21, 2023 8:59 PM
> > > > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Leif
> > Lindholm
> > > > <quic_llindhol@quicinc.com>
> > > > Cc: Li, Yi1 <yi1.li@intel.com>; Lu, Xiaoyu1 <xiaoyu1.lu@intel.com>; Jiang,
> > Guomin
> > > > <guomin.jiang@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> > Sami
> > > > Mujawar <sami.mujawar@arm.com>; Gerd Hoffmann <kraxel@redhat.com>
> > > > Subject: Re: [PATCH v5 2/6] CryptoPkg/CryptoPkg.ci.yaml: Allow dependency
> > > > upon ArmPkg
> > > >
> > > > Hello Jiewen,
> > > >
> > > > On 11/21/23 12:27, Yao, Jiewen wrote:
> > > > > Why CryptoPkg needs to depend on ArmPkg?
> > > > >
> > > > > Can we move content to MdePkg?
> > > >
> > > > The OpensslLib needs to discover the native instruction supported by the
> > > > underlying platform before using them. This could also be done through the
> > > > MdePkg as you suggested. The v2 is implemented that way:
> > > > https://edk2.groups.io/g/devel/message/110953
> > > >
> > > > Also, as noted by Leif, it seems there is already a dependency over ArmPkg:
> > > > # git grep ArmPkg CryptoPkg/
> > > > CryptoPkg/CryptoPkg.dsc:  ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf
> > > > CryptoPkg/CryptoPkg.dsc:
> > > > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> > > > CryptoPkg/CryptoPkg.dsc:
> > > > ArmSoftFloatLib|ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf
> > > > CryptoPkg/CryptoPkgMbedTls.dsc:
> > > > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> > > > CryptoPkg/CryptoPkgMbedTls.dsc:
> > > > ArmSoftFloatLib|ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf
> > > > CryptoPkg/CryptoPkgMbedTls.dsc:
> > > >
> > PeiServicesTablePointerLib|ArmPkg/Library/PeiServicesTablePointerLib/PeiServic
> > > > esTablePointerLib.inf
> > > >
> > > > Both solutions suit me (discovering capabilities through ArmPkg or MdePkg),
> > > > I just need to know which one is preferred,
> > > >
> > > > Regards,
> > > > Pierre
> > > >
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: Pierre Gondois <pierre.gondois@arm.com>
> > > > >> Sent: Tuesday, November 21, 2023 4:47 PM
> > > > >> To: devel@edk2.groups.io
> > > > >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Li, Yi1 <yi1.li@intel.com>; Lu,
> > > > Xiaoyu1
> > > > >> <xiaoyu1.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>; Leif
> > > > Lindholm
> > > > >> <quic_llindhol@quicinc.com>; Ard Biesheuvel
> > <ardb+tianocore@kernel.org>;
> > > > >> Sami Mujawar <sami.mujawar@arm.com>; Gerd Hoffmann
> > > > >> <kraxel@redhat.com>
> > > > >> Subject: [PATCH v5 2/6] CryptoPkg/CryptoPkg.ci.yaml: Allow dependency
> > upon
> > > > >> ArmPkg
> > > > >>
> > > > >> Allow dependency upon ArmPkg to pass the dependency Check.
> > > > >>
> > > > >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> > > > >> ---
> > > > >>   CryptoPkg/CryptoPkg.ci.yaml | 1 +
> > > > >>   1 file changed, 1 insertion(+)
> > > > >>
> > > > >> diff --git a/CryptoPkg/CryptoPkg.ci.yaml b/CryptoPkg/CryptoPkg.ci.yaml
> > > > >> index f961d85927c0..3bbb220d3224 100644
> > > > >> --- a/CryptoPkg/CryptoPkg.ci.yaml
> > > > >> +++ b/CryptoPkg/CryptoPkg.ci.yaml
> > > > >> @@ -69,6 +69,7 @@
> > > > >>       },
> > > > >>
> > > > >>       "DependencyCheck": {
> > > > >>
> > > > >>           "AcceptableDependencies": [
> > > > >>
> > > > >> +            "ArmPkg/ArmPkg.dec",
> > > > >>
> > > > >>               "MdePkg/MdePkg.dec",
> > > > >>
> > > > >>               "MdeModulePkg/MdeModulePkg.dec",
> > > > >>
> > > > >>               "CryptoPkg/CryptoPkg.dec",
> > > > >>
> > > > >> --
> > > > >> 2.25.1
> > > > >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111555): https://edk2.groups.io/g/devel/message/111555
Mute This Topic: https://groups.io/mt/102725178/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-