On 31 July 2015 at 13:28, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote:
> On 31 July 2015 at 14:21, Ryan Harkin <ryan.har...@linaro.org> wrote:
> >
> >
> > On 31 July 2015 at 13:13, Ard Biesheuvel <ard.biesheu...@linaro.org>
> wrote:
> >>
> >> On 31 July 2015 at 14:06, Ryan Harkin <ryan.har...@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.har...@linaro.org>
> >> > ---
> >> > .../Library/ArmGicArchLib/AArch64/ArmGicArchLib.c | 26
> >> > +++++++++++++++-------
> >> > ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c | 24
> >> > ++++++++++++++------
> >> > 2 files changed, 35 insertions(+), 15 deletions(-)
> >> >
> >> > diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
> >> > b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
> >> > index 9853c7b..b092e3a 100644
> >> > --- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
> >> > +++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
> >> > @@ -15,7 +15,22 @@
> >> > #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;
> >> > +
> >> > +RETURN_STATUS
> >> > +EFIAPI
> >> > +ArmGicArchLibHasGicv3 (
> >> > + 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
> >> > +}
> >> >
> >>
> >> Officially, the bit indicates whether the GIC system register
> >> interface is supported at the current exception level, and from that
> >> we infer that the system most likely has a GICv3.
> >> So if we don't have the SRE enabled, we try to drive it as a GICv2
> >> which only happens to work if this particular GICv3 has the GICv2
> >> compatibility mode implemented. (This is also mentioned in the
> >> comments)
> >>
> >> So the function name is a bit misleading here, although I won't insist
> >> that you change it.
> >
> >
> > I'm happy to change it if you want to recommend a better name, this
> seems a
> > but unwieldy:
> >
> > HasGicv3 -> GicSystemRegistersSupported
> >
> > ... but is perhaps more correct.
> >
>
> Well, the function and the caller are so close together that you
> shouldn't be able to miss the comment.
>
I do recommend STATIC for the function though.
>
Sure, good point. I'll add it for v2.
>
> >>
> >>
> >> Reviewed-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> >
> >
> > Thanks for the quick review turnaround :-)
>
> Too quick, as it seems. Spotted an inadvertent white space change below :-)
>
Drat, I would have got away with it if it wasn't for you meddling kids :-)
>
> >>
> >>
> >> > RETURN_STATUS
> >> > EFIAPI
> >> > @@ -31,7 +46,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 (ArmGicArchLibHasGicv3()) {
> >> > // 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
> >> > @@ -41,18 +56,13 @@ ArmGicArchLibInitialize (
> >> > // It is the OS responsibility to set this bit.
> >> > IccSre = ArmGicV3GetControlSystemRegisterEnable ();
> >> > if (!(IccSre & ICC_SRE_EL2_SRE)) {
> >> > - ArmGicV3SetControlSystemRegisterEnable (IccSre |
> >> > ICC_SRE_EL2_SRE);
> >> > + ArmGicV3SetControlSystemRegisterEnable (IccSre|
> ICC_SRE_EL2_SRE);
>
> here ^^^
>
I sort-of did that on purpose to make the two files identical. The space
vanished when the original copy/paste/hack operation happened and it crept
back in when I made the Arm and Aarch64 files identical.
I know it's not important, but if I omit that whitespace change, then the
two files won't actually be identical before moving to a single common
source.
Now, do you *really* want me to drop this hunk for v2??
> >> > IccSre = ArmGicV3GetControlSystemRegisterEnable ();
> >> > }
> >> > 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..b092e3a 100644
> >> > --- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
> >> > +++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
> >> > @@ -15,7 +15,22 @@
> >> > #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;
> >> > +
> >> > +RETURN_STATUS
> >> > +EFIAPI
> >> > +ArmGicArchLibHasGicv3 (
> >> > + 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 +46,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 (ArmGicArchLibHasGicv3()) {
> >> > // 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 +61,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
