On 31 Jul 2015 21:21, "Cohen, Eugene" <eugene@hp.com> wrote:
>
> The .inf/source file approach is the way to be consistent with the rest of edk2. I think it would be messy for the rest of edk2 to use .inf binding and for Arm packages to use a combination of #ifdef and .inf techniques. As someone who has ported UEFI to a new processor architecture I appreciate the .inf approach. When you try to build the component for a new architecture and you're missing the source file you I get nice linker errors, a virtual 'todo' list for the port
I'm not in favour of over engineering code for the sake of it. A #error exactly at the line of code you need to examine is precise. Linker errors are never nice.
If someone wishes to expand this code beyond Arm, they will need to do more than create a simple source file to provide a helper function. The entire package will need reworking.
>
> There's a right way to do this - refactor to minimize the difference to the smallest possible chunk of code and avoid the temptation to copy and paste the existing function / source file.
That's exactly what my patch achieves.
>
> Eugene
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Andrew Fish
> Sent: Friday, July 31, 2015 10:54 AM
> To: Ryan Harkin <ryan.harkin@linaro.org>
> Cc: edk2-devel@lists.01.org; Linaro UEFI Mailman List <linaro-uefi@lists.linaro.org>; Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH v2 1/2] ArmPkg/ArmGicLibArchLib: common check for GICv3
>
>
> > On Jul 31, 2015, at 9:37 AM, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> >
> > On 31 July 2015 at 17:16, Andrew Fish <afish@apple.com> wrote:
> >
> >>
> >> On Jul 31, 2015, at 7:49 AM, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> >>
> >> Make Arm and Aarch64 both use the same code, conditionally compiled, to
> >> check if the platform has GICv3.
> >>
> >> Both source files for Arm and Aarch64 are now identical and the next
> >> step is to move to a common source file.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ryan Harkin <ryan.harkin@linaro.org>
> >> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >> .../Library/ArmGicArchLib/AArch64/ArmGicArchLib.c | 25
> >> ++++++++++++++++------
> >> ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c | 25
> >> ++++++++++++++++------
> >> 2 files changed, 36 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
> >> b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
> >> index 9853c7b..36dbe41 100644
> >> --- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
> >> +++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
> >> @@ -15,7 +15,23 @@
> >> #include <Library/ArmLib.h>
> >> #include <Library/ArmGicLib.h>
> >>
> >> -STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
> >> +STATIC ARM_GIC_ARCH_REVISION mGicArchRevision =
> >> ARM_GIC_ARCH_REVISION_2;
> >> +
> >> +STATIC
> >> +RETURN_STATUS
> >> +EFIAPI
> >> +GicSystemRegistersSupported (
> >> + VOID
> >> + )
> >> +{
> >> +#if defined (MDE_CPU_ARM)
> >> + return (ArmReadIdPfr1 () & ARM_PFR1_GIC);
> >> +#elif defined(MDE_CPU_AARCH64)
> >> + return (ArmReadIdPfr0 () & AARCH64_PFR0_GIC);
> >> +#else
> >> + #error "Unknown chipset."
> >> +#endif
> >> +}
> >>
> >>
> >> We usually don’t use processor specific #ifdef’s in code like this in the
> >> edk2.
> >>
> >
> > That's probably why the code is filled with vast swathes of
> > copy/paste/hack-one-line happening all over Arm*Pkg.
> >
>
> The previous code was a bad porting job. it should have been:
>
> [Sources]
> CommonCode.c
>
> [Sources.ARM]
> Arm/ArmSpecific.c
>
> [Sources.AARCH64]
> AArch64/AArch64Specific.c
>
>
> > Although, I could split the only GicSystemRegistersSupported function into
> > it's own arch specific files to fit within that regime.
> >
>
> This is a good form as porting to a new processor architecture is quite easy. The process specific code is in a single file.
>
> The #ifdefs, and possible including processor specific includes can get really unwieldy in the general edk2 case that supports 6 CPU architectures. This form also makes porting to new processor architectures a lot easier.
>
> > Seems over the top to me. So unless it's a hard requirement, I'd resist it.
> >
> >
>
> Well the original issue was assumptions about what would happen in the code moving forward. What happens if some SOC comes along and uses a GIC for another CPU arch, or UEFI adds another CPU architecture?
>
> I’m kind of 50/50 on this specific case. I would definitely reject if it was generic code. But for me if its 50/50 I vote against the code with #ifdefs.
>
> Thanks,
>
> Andrew Fish
>
> >
> >>
> >> We usually isolate the code to a file that is processor specific, and use
> >> the INF file to point at the correct one.
> >>
> >> Here is an example:
> >> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
> >>
> >> [Sources]
> >> BasePeCoffLibInternals.h
> >> BasePeCoff.c
> >>
> >> [Sources.IA32, Sources.X64, Sources.EBC]
> >> PeCoffLoaderEx.c
> >>
> >> [Sources.IPF]
> >> Ipf/PeCoffLoaderEx.c
> >>
> >> [Sources.ARM]
> >> Arm/PeCoffLoaderEx.c
> >>
> >> [Sources.AARCH64]
> >> AArch64/PeCoffLoaderEx.c
> >>
> >>
> > This is a good example of where the arch specific sources are not
> > copy/paste/hack, but split out for good reason.
> >
> >
> > Thanks,
> >>
> >> Andrew Fish
> >>
> >> RETURN_STATUS
> >> EFIAPI
> >> @@ -31,7 +47,7 @@ ArmGicArchLibInitialize (
> >> // feature is implemented on the CPU. This is also convenient as our
> >> GICv3
> >> // driver requires SRE. If only Memory mapped access is available we try
> >> to
> >> // drive the GIC as a v2.
> >> - if (ArmReadIdPfr0 () & AARCH64_PFR0_GIC) {
> >> + if (GicSystemRegistersSupported()) {
> >> // Make sure System Register access is enabled (SRE). This depends on
> >> the
> >> // higher privilege level giving us permission, otherwise we will
> >> either
> >> // cause an exception here, or the write doesn't stick in which case
> >> we need
> >> @@ -46,13 +62,8 @@ ArmGicArchLibInitialize (
> >> }
> >> if (IccSre & ICC_SRE_EL2_SRE) {
> >> mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
> >> - goto Done;
> >> }
> >> }
> >> -
> >> - mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
> >> -
> >> -Done:
> >> return RETURN_SUCCESS;
> >> }
> >>
> >> diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
> >> b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
> >> index f8822a2..75ba156 100644
> >> --- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
> >> +++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
> >> @@ -15,7 +15,23 @@
> >> #include <Library/ArmLib.h>
> >> #include <Library/ArmGicLib.h>
> >>
> >> -STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
> >> +STATIC ARM_GIC_ARCH_REVISION mGicArchRevision =
> >> ARM_GIC_ARCH_REVISION_2;
> >> +
> >> +STATIC
> >> +RETURN_STATUS
> >> +EFIAPI
> >> +GicSystemRegistersSupported (
> >> + VOID
> >> + )
> >> +{
> >> +#if defined (MDE_CPU_ARM)
> >> + return (ArmReadIdPfr1 () & ARM_PFR1_GIC);
> >> +#elif defined(MDE_CPU_AARCH64)
> >> + return (ArmReadIdPfr0 () & AARCH64_PFR0_GIC);
> >> +#else
> >> + #error "Unknown chipset."
> >> +#endif
> >> +}
> >>
> >> RETURN_STATUS
> >> EFIAPI
> >> @@ -31,7 +47,7 @@ ArmGicArchLibInitialize (
> >> // feature is implemented on the CPU. This is also convenient as our
> >> GICv3
> >> // driver requires SRE. If only Memory mapped access is available we try
> >> to
> >> // drive the GIC as a v2.
> >> - if (ArmReadIdPfr1 () & ARM_PFR1_GIC) {
> >> + if (GicSystemRegistersSupported()) {
> >> // Make sure System Register access is enabled (SRE). This depends on
> >> the
> >> // higher privilege level giving us permission, otherwise we will
> >> either
> >> // cause an exception here, or the write doesn't stick in which case
> >> we need
> >> @@ -46,13 +62,8 @@ ArmGicArchLibInitialize (
> >> }
> >> if (IccSre & ICC_SRE_EL2_SRE) {
> >> mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
> >> - goto Done;
> >> }
> >> }
> >> -
> >> - mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
> >> -
> >> -Done:
> >> return RETURN_SUCCESS;
> >> }
> >>
> >> --
> >> 2.1.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
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel