Re: [edk2] [PATCH] ArmPkg: update InvalidateInstructionCacheRange to flush only to PoU
On Mon, Jan 28, 2019 at 09:09:26PM +0800, Tan Xiaojun wrote: > On 2019/1/28 19:54, Laszlo Ersek wrote: > > On 01/28/19 11:46, Mark Rutland wrote: > >> On Wed, Jan 23, 2019 at 10:54:56AM +0100, Laszlo Ersek wrote: > >>> And even on the original (unspecified) hardware, the same binary works > >>> frequently. My understanding is that there are five VMs executing reboot > >>> loops in parallel, on the same host, and 4 out of 5 may hit the issue in > >>> a reasonable time period (300 reboots or so). > >> > >> Interesting. > >> > >> Do you happen to know how many VMID bits the host has? If it has an 8-bit > >> VMID, > >> this could be indicative of some problem upon overflow. > > > > I'll let Tan Xiaojun (CC'd) answer this questions. > > > >> Can you point us at the host kernel? > > > > In the report, Tan Xiaojun wrote "4.18.0-48.el8.aarch64"; I guess that > > information is mostly useless in an upstream discussion. Unfortunately, > > I couldn't reproduce the symptom at all (I know nothing about the > > hardware in question), so I can't myself retest with an upstream host > > kernel. > > I don't understand, what do you want me to do? What is the specific problem? Could you let us know which CPU/system you've seen this issue with? ... and what the value of ID_AA64MMFR1_EL1.VMIDBits is? Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg: update InvalidateInstructionCacheRange to flush only to PoU
On Wed, Jan 23, 2019 at 10:54:56AM +0100, Laszlo Ersek wrote: > On 01/23/19 10:26, Ard Biesheuvel wrote: > > On Wed, 23 Jan 2019 at 10:14, Laszlo Ersek wrote: > >> On 01/22/19 16:37, Ard Biesheuvel wrote: > > >>> Is SetUefiImageMemoryAttributes() being > >>> called to remap the memory R-X ? > >> > >> No, it is not; the grub binary in question doesn't have the required > >> section alignment (... I hope at least that that's what your question > >> refers to): > >> > >>> ProtectUefiImageCommon - 0x3E6C54C0 > >>> - 0x00013BEEF000 - 0x00030600 > >>> ProtectUefiImageCommon - Section Alignment(0x200) is > >> incorrect > > > > This is puzzling, given that the exact same binary works on Mustang. > > And even on the original (unspecified) hardware, the same binary works > frequently. My understanding is that there are five VMs executing reboot > loops in parallel, on the same host, and 4 out of 5 may hit the issue in > a reasonable time period (300 reboots or so). Interesting. Do you happen to know how many VMID bits the host has? If it has an 8-bit VMID, this could be indicative of some problem upon overflow. Can you point us at the host kernel? Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg: update InvalidateInstructionCacheRange to flush only to PoU
On Wed, Jan 23, 2019 at 03:02:14PM +0100, Ard Biesheuvel wrote:
> On Wed, 23 Jan 2019 at 10:55, Laszlo Ersek wrote:
> >
> > On 01/23/19 10:26, Ard Biesheuvel wrote:
> > > On Wed, 23 Jan 2019 at 10:14, Laszlo Ersek wrote:
> > >> On 01/22/19 16:37, Ard Biesheuvel wrote:
> >
> > >>> Is SetUefiImageMemoryAttributes() being
> > >>> called to remap the memory R-X ?
> > >>
> > >> No, it is not; the grub binary in question doesn't have the required
> > >> section alignment (... I hope at least that that's what your question
> > >> refers to):
> > >>
> > >>> ProtectUefiImageCommon - 0x3E6C54C0
> > >>> - 0x00013BEEF000 - 0x00030600
> > >>> ProtectUefiImageCommon - Section Alignment(0x200) is
> > >> incorrect
> > >>
> > >
> > > This is puzzling, given that the exact same binary works on Mustang.
> >
> > And even on the original (unspecified) hardware, the same binary works
> > frequently. My understanding is that there are five VMs executing reboot
> > loops in parallel, on the same host, and 4 out of 5 may hit the issue in
> > a reasonable time period (300 reboots or so).
> >
> > > So when loaded, GRUB should cover the following regions:
> > >
> > > 0x13beef - 0x13bf00 (0x11000)
> > > 0x13bf00 - 0x13bf01f600 (0x1f600)
> > >
> > > where neither covers a 2 MB block fully, which means that the TLB
> > > entry that we are hitting is stale.
> > >
> > > Since ProtectUefiImageCommon() does not do anything in this case, the
> > > stale translation must be the result of
> > > PcdDxeNxMemoryProtectionPolicy, which either sets the wrong
> > > permissions for EfiLoaderCode (relying on ProtectUefiImageCommon), or
> > > we don't flush the TLBs correctly after updating the permissions when
> > > converting the memory from EfiConventionalMemory to EfiLoaderCode
> > >
> > > Are you using the default value for PcdDxeNxMemoryProtectionPolicy?
> >
> > Yes, we have
> >
> > ArmVirtPkg/ArmVirt.dsc.inc:
> > gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC0007FD1
> >
> > from commit 1acd7c54a724 ("ArmVirtPkg AARCH64: enable NX memory
> > protection for all platforms", 2017-03-01).
> >
> > The binary is from the RPM
> > "edk2-aarch64-20180508gitee3198e672e2-5.el8+1789+f0947240.noarch", which
> > is basically upstream ee3198e672e2 plus a small number of backports and
> > downstream customizations.
> >
>
> This might help:
>
> diff --git a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> index b7173e00b039..4c0b4b4efbd5 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> +++ b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> @@ -138,7 +138,7 @@ ASM_FUNC(ArmUpdateTranslationTableEntry)
>
> ASM_FUNC(ArmInvalidateTlb)
> EL1_OR_EL2_OR_EL3(x0)
> -1: tlbi vmalle1
> +1: tlbi vmalle1is
> b 4f
> 2: tlbi alle2
> b 4f
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> index 90192df24f55..d54b1c19accf 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> @@ -34,7 +34,7 @@
>
>// flush the TLBs
>.if \el == 1
> - tlbi vmalle1
> + tlbi vmalle1is
>.else
>tlbi alle\el
>.endif
Assuming that hardware is working correctly, this change shouldn't be
necessary.
KVM sets HCR_EL2.FB, so all TLBI ops will behave as their *IS variant.
Likewise it sets HCR_EL2.BSU, so barriers apply to the inner shareable domain
too.
On bare-metal, NSH should be sufficient.
Thanks,
Mark.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH edk2-platforms v2 1/1] Silicon/SynQuacer: add optional OP-TEE DT node
On Thu, Jul 26, 2018 at 02:12:04PM +0530, Sumit Garg wrote:
> On Thu, 26 Jul 2018 at 13:20, Daniel Thompson
> wrote:
> > I guess it could implement a secure monitor call to provide it. In
> > fact I find it a rather pleasing approach. However I think it still loops
> > us round to pretty much the same question as before. Does TF-A "protec
> > " a normal world that makes an SMC to an OP-TEE that isn't there by
> > failing the call in a nice way?
>
> TF-A returns SMC call for OP-TEE as unknown (error code: -1 in "x0"
> register) if OP-TEE is not present.
Be careful here; you can't use an arbitrary SMC since that could be
implemented by another trusted OS (with a completely different meaning).
Assuming you know the system provides SMCCC, you can use the "Call UID
Query" in the trusted OS range, and check that returned value matches
OP-TEE's UID.
i.e
uid = smccc_uid_query(OPTEE_RANGE);
if (uid == OPTEEE_SMCCC_UID) {
[ OP-TEE present ]
} else {
[ unknown/no trusted OS present ]
}
Thanks,
Mark.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmVirtPkg/PlatformHasAcpiDtDxe: allow manual override for DT installation
Hi, On Wed, Mar 29, 2017 at 06:55:26PM +0200, Laszlo Ersek wrote: > On 03/29/17 18:17, Ard Biesheuvel wrote: > > On 29 March 2017 at 17:09, Jon Masters wrote: > >> Thanks Laszlo. A quick note from me that regardless of this > >> discussion I will be pushing to ensure the version Red Hat ships > >> makes ACPI the default with it being extremely painful to use DT. > >> It is time the ecosystem got with the program we all agreed upon > >> and there will be no more excuses. > > > > This has *nothing* to do with the ecosystem. This has to do with > > existing users of ArmVirtQemu (admittedly, a small fraction) that will > > be forced to compile their UEFI images from source because we made a > > backwards incompatible change. > > > > I am 100% on board with making ACPI and DT mutually exclusive. But I > > don't believe for a second that 'everybody just exposes ACPI and DT at > > the same time' if this gets merged. > > That's where we disagree, 100%. > > > That happens because people are clueless, not because they are > > deliberately spending time and effort on producing two hardware > > descriptions. > > If this were true, then the kernel's preference would have been changed > to ACPI aeons ago (assuming both DT and ACPI were present). As has been covered elsewhere, unilaterally changing the default breaks existing systems, regardless of what vendors do from now on. > ACPI is superior to DT (cue again Grant Likely's blog post), yet > kernel people resist it. In several respects, sure, though let's not be under the illusion that it is not free of novel technical problems. The big deal with ACPI is that there are other major OS vendors who will support ACPI, but not DT. To avoid a fragmented market, commodity servers must use ACPI. > That's not cluelessness. If the kernel's DT camp has any > influence on platform vendors (and that's rather more a "given that" > than an "if"), when they find out about this loophole, I expect them to > actively recommend it as a way to perpetuate the status quo. Who is this "DT camp"? As a DT binding maintainer, I would be livid were people recommending using this in generic OS images. Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg/ArmGenericTimerVirtCounterLib: deal with broken generic timers
[Adding Marc Zyngier]
On Fri, Jan 20, 2017 at 02:20:43PM +, Ard Biesheuvel wrote:
> Users of ArmGenericTimerVirtCounterLib may execute under virtualization,
> which implies that they may be affected by core errata of the host.
>
> Some implementations of the ARM Generic Timer are affected by errata where
> reads of the counter and reads or writes to the timer value may execute
> incorrectly when issued around the time the counter is incremented by
> the hardware.
So far, there are at least two slightly different errata I've seen in
this area: Freescale erratum A-008585, and Hisilicon erratum #161010101.
The first has a workaround in upstream Linux, whereas the latter
apparently requires a different workaround, and is currently under
review.
There may not be a one-size-fits-all solution, here. To that end, I
would strongly suggest that we document precisely which errata we are
trying to handle here.
> Since we can easily work around this without affecting performance too
> much, implement an unconditional workaround that compares two subsequent
> reads of the counter to ensure the value is correct. Note that the number
> for attempts should be limited to avoid breaking platforms such as QEMU
> with TCG emulation, since that has been observed never to return the same
> value from back to back reads of the counter register.
Even on real HW it's possible for back-to-back counter reads to not
(ever) see the same value. That may depend on the relative frequency of
the CPU and counter clocks, e.g. consider FPGAs, or the read of the
counter might always take at least one counter cycle.
Thanks,
Mark.
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel
> ---
>
> Note that this patch applies on top of the patch 'ArmPkg/ArmLib: remove
> indirection layer from timer register accessors' that I send out earlier
> today.
>
> ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c
> | 51 ++--
> 1 file changed, 48 insertions(+), 3 deletions(-)
>
> diff --git
> a/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c
>
> b/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c
> index 69a4ceb62db6..9fe673e8222c 100644
> ---
> a/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c
> +++
> b/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c
> @@ -70,13 +70,36 @@ ArmGenericTimerGetTimerFreq (
>return ArmReadCntFrq ();
> }
>
> +//
> +// The virtual counter may be used under virtualization on a host that
> +// is affected by one of the various errata where reads to the counter
> +// register may return incorrect values when the access occurs at the exact
> +// time that the counter is incremented by the hardware. This affects the
> +// timer as well as the counter.
> +// So repeat the read until we get the same value twice. Unfortunately,
> +// platforms such as QEMU with TCG emulation (i.e., non-virtualized) appear
> +// never to return the same value twice, so we need to set a retry limit.
> +//
> +#define MAX_RETRIES 200
> +
> UINTN
> EFIAPI
> ArmGenericTimerGetTimerVal (
>VOID
>)
> {
> - return ArmReadCntvTval ();
> + UINTN Result;
> + UINTN Tries;
> +
> + Tries = 0;
> + do {
> +//
> +// Keep reading until we see the same value twice in a row. See above.
> +//
> +Result = ArmReadCntvTval ();
> + } while (Result != ArmReadCntvTval () && ++Tries < MAX_RETRIES);
> +
> + return Result;
> }
>
>
> @@ -86,7 +109,18 @@ ArmGenericTimerSetTimerVal (
>IN UINTN Value
>)
> {
> - ArmWriteCntvTval (Value);
> + UINTN CounterVal;
> + UINTN Tries;
> +
> + Tries = 0;
> + do {
> +//
> +// Read the counter before and after the write to TVAL, to ensure that
> +// the write to TVAL did not involve a corrupted sample of the counter.
> +//
> +CounterVal = ArmReadCntvCt ();
> +ArmWriteCntvTval (Value);
> + } while (CounterVal != ArmReadCntvCt () && ++Tries < MAX_RETRIES);
> }
>
> UINT64
> @@ -95,7 +129,18 @@ ArmGenericTimerGetSystemCount (
>VOID
>)
> {
> - return ArmReadCntvCt ();
> + UINT64 Result;
> + UINTN Tries;
> +
> + Tries = 0;
> + do {
> +//
> +// Keep reading until we see the same value twice in a row. See above.
> +//
> +Result = ArmReadCntvCt ();
> + } while (Result != ArmReadCntvCt () && ++Tries < MAX_RETRIES);
> +
> + return Result;
> }
>
> UINTN
> --
> 2.7.4
>
> ___
> 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
Re: [edk2] [PATCH] ArmPkg/ArmMmuLib: avoid type promotion in TCR_EL1 assignment
On Wed, Jul 13, 2016 at 09:28:06AM +0200, Ard Biesheuvel wrote:
> Commit fafb7e9c110e ("ArmPkg: correct TTBR1_EL1 settings in TCR_EL1")
> introduced a symbolic constant TCR_TG1_4KB which resolves to (2 << 30),
> and ORs it into the value to be written into TCR_EL1 (if executing at
> EL1). Since the constant is implicitly typed as signed int, and has the
> sign bit set, the promotion that occurs when casting to UINT64 results
> in a TCR value that has bits [63:32] all set, which includes mostly
> RES0 bits but also the TBIn, AS and IPS fields.
>
> So explicitly redefine all TCR related constants as 'unsigned long'
> types, using the UL suffix. To avoid confusion in the future, the
> inappropriately named VTCR_EL23_xxx constants have the leading V
> removed, and the actual VTCR_EL2 related constants are dropped, given
> that we never configure stage 2 translation in UEFI.
>
> Reported-by: Vishal Oliyil Kunnil
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel
Apologies for this. FWIW:
Acked-by: Mark Rutland
Mark.
> ---
> ArmPkg/Include/Chipset/AArch64Mmu.h | 140 +---
> 1 file changed, 62 insertions(+), 78 deletions(-)
>
> diff --git a/ArmPkg/Include/Chipset/AArch64Mmu.h
> b/ArmPkg/Include/Chipset/AArch64Mmu.h
> index f660e65aac33..ff77b16b25c0 100644
> --- a/ArmPkg/Include/Chipset/AArch64Mmu.h
> +++ b/ArmPkg/Include/Chipset/AArch64Mmu.h
> @@ -98,17 +98,17 @@
> //
> // Translation Control Register
> //
> -#define TCR_T0SZ_MASK 0x3F
> +#define TCR_T0SZ_MASK 0x3FUL
>
> -#define TCR_PS_4GB (0 << 16)
> -#define TCR_PS_64GB (1 << 16)
> -#define TCR_PS_1TB (2 << 16)
> -#define TCR_PS_4TB (3 << 16)
> -#define TCR_PS_16TB (4 << 16)
> -#define TCR_PS_256TB(5 << 16)
> +#define TCR_PS_4GB (0UL << 16)
> +#define TCR_PS_64GB (1UL << 16)
> +#define TCR_PS_1TB (2UL << 16)
> +#define TCR_PS_4TB (3UL << 16)
> +#define TCR_PS_16TB (4UL << 16)
> +#define TCR_PS_256TB(5UL << 16)
>
> -#define TCR_TG0_4KB (0 << 14)
> -#define TCR_TG1_4KB (2 << 30)
> +#define TCR_TG0_4KB (0UL << 14)
> +#define TCR_TG1_4KB (2UL << 30)
>
> #define TCR_IPS_4GB (0ULL << 32)
> #define TCR_IPS_64GB(1ULL << 32)
> @@ -117,7 +117,7 @@
> #define TCR_IPS_16TB(4ULL << 32)
> #define TCR_IPS_256TB (5ULL << 32)
>
> -#define TCR_EPD1(1 << 23)
> +#define TCR_EPD1(1UL << 23)
>
> #define TTBR_ASID_FIELD (48)
> #define TTBR_ASID_MASK (0xFF << TTBR_ASID_FIELD)
> @@ -140,75 +140,59 @@
> #define TCR_EL1_AS_FIELD (36)
> #define TCR_EL1_TBI0_FIELD (37)
> #define TCR_EL1_TBI1_FIELD (38)
> -#define TCR_EL1_T0SZ_MASK(0x1F << TCR_EL1_T0SZ_FIELD)
> -#define TCR_EL1_EPD0_MASK(0x1 << TCR_EL1_EPD0_FIELD)
> -#define TCR_EL1_IRGN0_MASK (0x3 << TCR_EL1_IRGN0_FIELD)
> -#define TCR_EL1_ORGN0_MASK (0x3 << TCR_EL1_ORGN0_FIELD)
> -#define TCR_EL1_SH0_MASK (0x3 << TCR_EL1_SH0_FIELD)
> -#define TCR_EL1_TG0_MASK (0x1 << TCR_EL1_TG0_FIELD)
> -#define TCR_EL1_T1SZ_MASK(0x1F << TCR_EL1_T1SZ_FIELD)
> -#define TCR_EL1_A1_MASK (0x1 << TCR_EL1_A1_FIELD)
> -#define TCR_EL1_EPD1_MASK(0x1 << TCR_EL1_EPD1_FIELD)
> -#define TCR_EL1_IRGN1_MASK (0x3 << TCR_EL1_IRGN1_FIELD)
> -#define TCR_EL1_ORGN1_MASK (0x3 << TCR_EL1_ORGN1_FIELD)
> -#define TCR_EL1_SH1_MASK (0x3 << TCR_EL1_SH1_FIELD)
> -#define TCR_EL1_TG1_MASK (0x1 << TCR_EL1_TG1_FIELD)
> -#define TCR_EL1_IPS_MASK (0x7 << TCR_EL1_IPS_FIELD)
> -#define TCR_EL1_AS_MASK (0x1 << TCR_EL1_AS_FIELD)
> -#de
Re: [edk2] [PATCH 2/2] ArmPkg/CpuDxe: unmask SErrors in DEBUG builds
On Fri, Jul 01, 2016 at 12:53:13PM +0200, Ard Biesheuvel wrote: > SErrors (formerly called asynchronous aborts) are a distinct class of > exceptions that are not closely tied to the currently executing > instruction. Since execution may be able to proceed in such a condition, > this class of exception is masked by default, and software needs to unmask > it explicitly if it is prepared to handle such exceptions. > > On DEBUG builds, we are well equipped to report the CPU context to the user > and it makes sense to report an SError as soon as it occurs rather than to > wait for the OS to take it when it unmasks them, especially since the current > arm64/Linux implementation simply panics in that case. So unmask them when > ArmCpuDxe loads. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > ArmPkg/Drivers/CpuDxe/Exception.c | 9 + > 1 file changed, 9 insertions(+) These look sensible to me. FWIW, for both patches: Acked-by: Mark Rutland Mark. > > diff --git a/ArmPkg/Drivers/CpuDxe/Exception.c > b/ArmPkg/Drivers/CpuDxe/Exception.c > index c3107cd4a6bc..d806a5fdf910 100644 > --- a/ArmPkg/Drivers/CpuDxe/Exception.c > +++ b/ArmPkg/Drivers/CpuDxe/Exception.c > @@ -62,6 +62,15 @@ InitializeExceptions ( > Status = Cpu->EnableInterrupt (Cpu); >} > > + // > + // On a DEBUG build, unmask SErrors so they are delivered right away rather > + // than when the OS unmasks them. This gives us a better chance of figuring > + // out the cause. > + // > + DEBUG_CODE ( > +ArmEnableAsynchronousAbort (); > + ); > + >return Status; > } > > -- > 2.7.4 > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] ArmPkg/ArmLib: avoid cache maintenance in PEIMs when executing in place
On Mon, Jun 13, 2016 at 05:51:23PM +0200, Ard Biesheuvel wrote: > On 13 June 2016 at 17:45, Mark Rutland wrote: > > On Mon, Jun 13, 2016 at 05:26:07PM +0200, Ard Biesheuvel wrote: > >> On some platforms, performing cache maintenance on regions that are backed > >> by NOR flash result in SErrors. Since cache maintenance is unnecessary in > >> that case, create a PEIM specific version that only performs said cache > >> maintenance in its constructor if the module is shadowed in RAM. To avoid > >> performing the cache maintenance if the MMU code is not used to begin with, > >> check that explicitly in the constructor. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Ard Biesheuvel > >> --- > >> > >> As discussed in the thread dedicated to this subject, the preferred way of > >> addressing this to split off the MMU manipulation code from ArmLib into a > >> separate ArmMmuLib, but this affects other packages and platforms. So in > >> the > >> mean time, let's merge this patch so that D02 can use the upstream ArmLib > >> unmodified. > > > > I'm missing something here (and couldn't figure out which thread covered > > this earlier), and this feels pretty suspect. > > > > Why does cache maintenance to these regions result in SError in the > > first place? > > > > Is the SRAM not accepting some writeback or fetch that occurs as a > > result? How are we protected against natural evictions and/or fetches? > > > > Does the SRAM respond badly to a CMO itself for some reason? > > > > It's NOR flash, not SRAM, and it is basically a quirk of the D02 > non-DRAM memory bus implementation. Ok. I take it that means the CMO itself is the issue (i.e. it gets forwarded to the memory controller endpoint, which barfs), rather than asynchronous writebacks or fetches being a potential issue. If Heyi can confirm that, then this looks fine to me. For the sake of future reference, it would be nice to note the specific issue in the commit message. > I will let Heyi respond with more details, but this patch is basically > a stop gap solution until we split off the MMU code from ArmLib, which > will allow us to deal with this more elegantly (Currently, the only > PEI phase module that manipulates the page tables with the MMU on, and > is likely to split entries is DxeIpl, which maps the DXE phase stack > non-executable. The only other user is the module that creates the > page tables in the first place, and so does not require the extra > caution when splitting block entries) Understood! My only concern was whether this was masking an issue with asynchronous behaviour. Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] ArmPkg/ArmLib: avoid cache maintenance in PEIMs when executing in place
On Mon, Jun 13, 2016 at 05:26:07PM +0200, Ard Biesheuvel wrote:
> On some platforms, performing cache maintenance on regions that are backed
> by NOR flash result in SErrors. Since cache maintenance is unnecessary in
> that case, create a PEIM specific version that only performs said cache
> maintenance in its constructor if the module is shadowed in RAM. To avoid
> performing the cache maintenance if the MMU code is not used to begin with,
> check that explicitly in the constructor.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel
> ---
>
> As discussed in the thread dedicated to this subject, the preferred way of
> addressing this to split off the MMU manipulation code from ArmLib into a
> separate ArmMmuLib, but this affects other packages and platforms. So in the
> mean time, let's merge this patch so that D02 can use the upstream ArmLib
> unmodified.
I'm missing something here (and couldn't figure out which thread covered
this earlier), and this feels pretty suspect.
Why does cache maintenance to these regions result in SError in the
first place?
Is the SRAM not accepting some writeback or fetch that occurs as a
result? How are we protected against natural evictions and/or fetches?
Does the SRAM respond badly to a CMO itself for some reason?
Thanks,
Mark.
> ArmPkg/Library/ArmLib/AArch64/{AArch64LibConstructor.c =>
> AArch64BaseLibConstructor.c} | 0
> ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf
> | 2 +-
> ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf
> | 43 +++
> ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> | 2 +
> ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c
> | 75
> 5 files changed, 121 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c
> b/ArmPkg/Library/ArmLib/AArch64/AArch64BaseLibConstructor.c
> similarity index 100%
> rename from ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c
> rename to ArmPkg/Library/ArmLib/AArch64/AArch64BaseLibConstructor.c
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf
> b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf
> index 58684e8492f2..ef9d261b910d 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf
> @@ -32,7 +32,7 @@ [Sources.AARCH64]
>
>../Common/AArch64/ArmLibSupport.S
>../Common/ArmLib.c
> - AArch64LibConstructor.c
> + AArch64BaseLibConstructor.c
>
> [Packages]
>ArmPkg/ArmPkg.dec
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf
> b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf
> new file mode 100644
> index ..c8f0b97750d4
> --- /dev/null
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf
> @@ -0,0 +1,43 @@
> +#/** @file
> +#
> +# Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.
> +# Portions copyright (c) 2011 - 2014, ARM Limited. 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
> +# http://opensource.org/licenses/bsd-license.php
> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> IMPLIED.
> +#
> +#
> +#**/
> +
> +[Defines]
> + INF_VERSION= 0x00010005
> + BASE_NAME = AArch64Lib
> + FILE_GUID = ef20ddf5-b334-47b3-94cf-52ff44c29138
> + MODULE_TYPE= PEIM
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = ArmLib|PEIM PEI_CORE
> + CONSTRUCTOR= AArch64LibConstructor
> +
> +[Sources.AARCH64]
> + AArch64Lib.c
> + AArch64Mmu.c
> + AArch64ArchTimer.c
> + ArmLibSupportV8.S
> + AArch64Support.S
> + AArch64ArchTimerSupport.S
> +
> + ../Common/AArch64/ArmLibSupport.S
> + ../Common/ArmLib.c
> + AArch64PeiLibConstructor.c
> +
> +[Packages]
> + ArmPkg/ArmPkg.dec
> + MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> + MemoryAllocationLib
> + CacheMaintenanceLib
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> index cf9b7222b47b..07864bac28e6 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> @@ -26,6 +26,8 @@
> // We use this index definition to define an invalid block entry
> #define TT_ATTR_INDX_INVALID((UINT32)~0)
>
> +INT32 HaveMmuRoutines = 1;
> +
> STATIC
> UINT64
> ArmMemoryAttributeToPageAttribute (
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c
> b/ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c
> new file mode 100
Re: [edk2] [PATCH] ArmPkg/ArmLib: don't invalidate entire I-cache on range operation
On Wed, May 11, 2016 at 12:23:58PM +0200, Ard Biesheuvel wrote:
> On 11 May 2016 at 12:22, Mark Rutland wrote:
> > On Wed, May 11, 2016 at 12:07:51PM +0200, Ard Biesheuvel wrote:
> >> On 11 May 2016 at 11:35, Achin Gupta wrote:
> >> >> diff --git
> >> >> a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> >> >> b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> >> >> index 1045f9068f4d..cc7555061428 100644
> >> >> --- a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> >> >> +++ b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> >> >> @@ -65,7 +65,8 @@ InvalidateInstructionCacheRange (
> >> >>)
> >> >> {
> >> >>CacheRangeOperation (Address, Length,
> >> >> ArmCleanDataCacheEntryToPoUByMVA);
> >> >> - ArmInvalidateInstructionCache ();
> >> >> + CacheRangeOperation (Address, Length,
> >> >> +ArmInvalidateInstructionCacheEntryToPoUByMVA);
> >> >
> >> > Afaics, CacheRangeOperation() uses the data cache line length by
> >> > default. This
> >> > will match the I$ line length in most cases. But, it would be better to
> >> > use the
> >> > ArmInstructionCacheLineLength() function instead. I suggest that we add
> >> > a cache
> >> > line length parameter to CacheRangeOperation() to allow the distinction.
> >> >
> >>
> >> Good point.
> >>
> >> > Also, from my reading of the ARMv8 ARM B2.6.5 & E2.6.5, we need an ISB
> >> > at the
> >> > end of all the operations.
> >> >
> >>
> >> I would assume that a single call to
> >> ArmInstructionSynchronizationBarrier() at the end of
> >> InvalidateInstructionCacheRange () should suffice?
> >
> > Almost. You also need DSBs to complete the maintenance ops. e.g. a
> > sequence:
> >
> > d-cache maintenance ops
> > DSB
> > i-cache maintenance ops
> > DSB
> > ISB
> >
>
> Yes, but the DSB is already performed by CacheRangeOperation (). So
> adding the ISB in InvalidateInstructionCacheRange () results in
> exactly the above.
Ah, sorry, I managed to miss that when scanning the source code.
I guess I'm just saying "yes" then. :)
THanks,
Mark.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg/ArmLib: don't invalidate entire I-cache on range operation
On Wed, May 11, 2016 at 12:07:51PM +0200, Ard Biesheuvel wrote:
> On 11 May 2016 at 11:35, Achin Gupta wrote:
> >> diff --git
> >> a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> >> b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> >> index 1045f9068f4d..cc7555061428 100644
> >> --- a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> >> +++ b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> >> @@ -65,7 +65,8 @@ InvalidateInstructionCacheRange (
> >>)
> >> {
> >>CacheRangeOperation (Address, Length, ArmCleanDataCacheEntryToPoUByMVA);
> >> - ArmInvalidateInstructionCache ();
> >> + CacheRangeOperation (Address, Length,
> >> +ArmInvalidateInstructionCacheEntryToPoUByMVA);
> >
> > Afaics, CacheRangeOperation() uses the data cache line length by default.
> > This
> > will match the I$ line length in most cases. But, it would be better to use
> > the
> > ArmInstructionCacheLineLength() function instead. I suggest that we add a
> > cache
> > line length parameter to CacheRangeOperation() to allow the distinction.
> >
>
> Good point.
>
> > Also, from my reading of the ARMv8 ARM B2.6.5 & E2.6.5, we need an ISB at
> > the
> > end of all the operations.
> >
>
> I would assume that a single call to
> ArmInstructionSynchronizationBarrier() at the end of
> InvalidateInstructionCacheRange () should suffice?
Almost. You also need DSBs to complete the maintenance ops. e.g. a
sequence:
d-cache maintenance ops
DSB
i-cache maintenance ops
DSB
ISB
Thanks,
Mark.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/4] EmbeddedPkg/Lan9118Dxe MMIO fixes
On Mon, May 09, 2016 at 07:02:16PM +0100, Ryan Harkin wrote: > On 9 May 2016 at 11:07, Ryan Harkin wrote: > > On 9 May 2016 at 10:22, Mark Rutland wrote: > >> On Sat, May 07, 2016 at 10:43:45AM +0200, Ard Biesheuvel wrote: > >>> On 6 May 2016 at 19:19, Mark Rutland wrote: > >>> > The LAN9118 driver uses memory fences in a novel but erroneous fashion, > >>> > due to > >>> > a misunderstanding of some under-commented code. This series fixes these > >>> > erroneous uses, documenting the unusual requirements of the LAN9118 > >>> > chip that > >>> > lead us to this situation, and introduces new helpers to handle this in > >>> > a more > >>> > consistent fashion. > >>> > > >>> > The LAN9118 datasheet is publicly available at: > >>> > > >>> > http://www.microchip.com/wwwproducts/en/LAN9118 > >>> > > >>> > >>> Thanks a lot for getting to the bottom of this! I particularly like > >>> the way how you folded the required delays into the MMIO read/write > >>> functions, which makes the top level code a lot cleaner. > >>> > >>> I can't test this, but the code looks fine to me. > >>> > > > > I'll test it later today on TC2 and Juno R0/1/2. But I like the look > > of it, it seems like a huge improvement. > > > > This seems to work on TC2 and Juno R0, R1 and R2, although I'm not > 100% sure it's reliable, so I need to do more testing. > > When attempting to install debian over the network, my Juno R1 has > just reported: > > LAN9118: There was an error transmitting. TxStatus=0x8401:- No carrier > - Packet tx was deferred > > The 2nd attempt seems to be working fine. Juno R0 and R2 seem happy enough. I suspect you're hitting the latent, intermittent issue with full duplex operation that I mentioned in my other reply. This occurs without these patches, and was what I was trying to fix when I noticed the suspicious barrier usage. If you early return from AutoNegotiate (e.g. because the timeout is short enough), or remove the full duplex bits from the Feature register, this works, this issue does not manifest (at least in my experience), as the HW will auto-negotiate a half-duplex connection anyway. Evidently, we don't have a good test case for this. I've found that it's fairly reproducable when using TFTP from GRUB (using SNP). Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/4] EmbeddedPkg/Lan9118Dxe MMIO fixes
On Sat, May 07, 2016 at 10:43:45AM +0200, Ard Biesheuvel wrote: > On 6 May 2016 at 19:19, Mark Rutland wrote: > > The LAN9118 driver uses memory fences in a novel but erroneous fashion, due > > to > > a misunderstanding of some under-commented code. This series fixes these > > erroneous uses, documenting the unusual requirements of the LAN9118 chip > > that > > lead us to this situation, and introduces new helpers to handle this in a > > more > > consistent fashion. > > > > The LAN9118 datasheet is publicly available at: > > > > http://www.microchip.com/wwwproducts/en/LAN9118 > > > > Thanks a lot for getting to the bottom of this! I particularly like > the way how you folded the required delays into the MMIO read/write > functions, which makes the top level code a lot cleaner. > > I can't test this, but the code looks fine to me. > > Reviewed-by: Ard Biesheuvel Cheers! FWIW, I've tested each patch on Juno R1, and I haven't seen any regression as a result of this. That said, I haven't been able to trigger issues even without this series. There's another latent bug that this doesn't solve, in that if the PHY negotiates full-duplex operation (at 100Mb/s or 10Mb/s), but that appears to be unrelated. Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 4/4] EmbeddedPkg/Lan9118Dxe: remove redundant stalls
Now that the LAN9118-specific MMIO accessors provide the required
delays, remove the redundant stalls.
Stalls in delay loops are kept, as these give time for work to happen
beyond synchronisation of the device register file.
Cc: Ard Biesheuvel
Cc: Leif Lindholm
Cc: Ryan Harkin
Signed-off-by: Mark Rutland
Contributed-under: TianoCore Contribution Agreement 1.0
---
EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 5 -
EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c | 16
2 files changed, 21 deletions(-)
diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
index bef34c2..8af23df 100644
--- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
+++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
@@ -307,8 +307,6 @@ SnpInitialize (
// Write the current configuration to the register
Lan9118MmioWrite32 (LAN9118_PMT_CTRL, PmConf);
- gBS->Stall (LAN9118_STALL);
- gBS->Stall (LAN9118_STALL);
// Configure GPIO and HW
Status = ConfigureHardware (HW_CONF_USE_LEDS, Snp);
@@ -431,7 +429,6 @@ SnpReset (
// Write the current configuration to the register
Lan9118MmioWrite32 (LAN9118_PMT_CTRL, PmConf);
- gBS->Stall (LAN9118_STALL);
// Reactivate the LEDs
Status = ConfigureHardware (HW_CONF_USE_LEDS, Snp);
@@ -446,7 +443,6 @@ SnpReset (
HwConf |= HW_CFG_TX_FIFO_SIZE(gTxBuffer);// assign size chosen in
SnpInitialize
Lan9118MmioWrite32 (LAN9118_HW_CFG, HwConf);// Write the conf
-gBS->Stall (LAN9118_STALL);
}
// Enable the receiver and transmitter and clear their contents
@@ -701,7 +697,6 @@ SnpReceiveFilters (
// Write the options to the MAC_CSR
//
IndirectMACWrite32 (INDIRECT_MAC_INDEX_CR, MacCSRValue);
- gBS->Stall (LAN9118_STALL);
//
// If we have to retrieve something, start packet reception.
diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c
b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c
index 61f11b6..50c004d 100644
--- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c
+++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c
@@ -284,7 +284,6 @@ IndirectEEPROMRead32 (
// Write to Eeprom command register
Lan9118MmioWrite32 (LAN9118_E2P_CMD, EepromCmd);
- gBS->Stall (LAN9118_STALL);
// Wait until operation has completed
while (Lan9118MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY);
@@ -332,7 +331,6 @@ IndirectEEPROMWrite32 (
// Write to Eeprom command register
Lan9118MmioWrite32 (LAN9118_E2P_CMD, EepromCmd);
- gBS->Stall (LAN9118_STALL);
// Wait until operation has completed
while (Lan9118MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY);
@@ -410,7 +408,6 @@ Lan9118Initialize (
if (((Lan9118MmioRead32 (LAN9118_PMT_CTRL) & MPTCTRL_PM_MODE_MASK) >> 12) !=
0) {
DEBUG ((DEBUG_NET, "Waking from reduced power state.\n"));
Lan9118MmioWrite32 (LAN9118_BYTE_TEST, 0x);
-gBS->Stall (LAN9118_STALL);
}
// Check that device is active
@@ -495,7 +492,6 @@ SoftReset (
// Write the configuration
Lan9118MmioWrite32 (LAN9118_HW_CFG, HwConf);
- gBS->Stall (LAN9118_STALL);
// Wait for reset to complete
while (Lan9118MmioRead32 (LAN9118_HW_CFG) & HWCFG_SRST) {
@@ -590,7 +586,6 @@ ConfigureHardware (
// Write the configuration
Lan9118MmioWrite32 (LAN9118_GPIO_CFG, GpioConf);
-gBS->Stall (LAN9118_STALL);
}
return EFI_SUCCESS;
@@ -719,7 +714,6 @@ StopTx (
TxCfg = Lan9118MmioRead32 (LAN9118_TX_CFG);
TxCfg |= TXCFG_TXS_DUMP | TXCFG_TXD_DUMP;
Lan9118MmioWrite32 (LAN9118_TX_CFG, TxCfg);
-gBS->Stall (LAN9118_STALL);
}
// Check if already stopped
@@ -738,7 +732,6 @@ StopTx (
if (TxCfg & TXCFG_TX_ON) {
TxCfg |= TXCFG_STOP_TX;
Lan9118MmioWrite32 (LAN9118_TX_CFG, TxCfg);
- gBS->Stall (LAN9118_STALL);
// Wait for Tx to finish transmitting
while (Lan9118MmioRead32 (LAN9118_TX_CFG) & TXCFG_STOP_TX);
@@ -773,7 +766,6 @@ StopRx (
RxCfg = Lan9118MmioRead32 (LAN9118_RX_CFG);
RxCfg |= RXCFG_RX_DUMP;
Lan9118MmioWrite32 (LAN9118_RX_CFG, RxCfg);
-gBS->Stall (LAN9118_STALL);
while (Lan9118MmioRead32 (LAN9118_RX_CFG) & RXCFG_RX_DUMP);
}
@@ -799,28 +791,23 @@ StartTx (
TxCfg = Lan9118MmioRead32 (LAN9118_TX_CFG);
TxCfg |= TXCFG_TXS_DUMP | TXCFG_TXD_DUMP;
Lan9118MmioWrite32 (LAN9118_TX_CFG, TxCfg);
-gBS->Stall (LAN9118_STALL);
}
// Check if tx was started from MAC and enable if not
if (Flags & START_TX_MAC) {
MacCsr = IndirectMACRead32 (INDIRECT_MAC_INDEX_CR);
-gBS->Stall (LAN9118_STALL);
if ((MacCsr & MACCR_TX_EN) == 0) {
MacCsr |= MACCR_TX_EN;
IndirectMACWrite32 (INDIRECT_MAC_INDEX_CR, MacCsr);
- gBS->Stall (LAN9118_STALL);
}
}
// Check if tx was started from TX_CFG and enable if not
if (Flags & START_TX_CFG) {
TxCfg =
[edk2] [PATCH 3/4] EmbeddedPkg/Lan9118Dxe: Use LAN9118 MMIO wrappers
Migrate the existing code to use the new LAN9118 MMIO wrappers, ensuring
that timing requirements are respected.
The newly redundant stalls will be removed in a subsequent patch.
Cc: Ard Biesheuvel
Cc: Leif Lindholm
Cc: Ryan Harkin
Signed-off-by: Mark Rutland
Contributed-under: TianoCore Contribution Agreement 1.0
---
EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 78 +++---
EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeHw.h | 4 +-
EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c | 132
3 files changed, 107 insertions(+), 107 deletions(-)
diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
index 50644e7..bef34c2 100644
--- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
+++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
@@ -297,7 +297,7 @@ SnpInitialize (
}
// Read the PM register
- PmConf = MmioRead32 (LAN9118_PMT_CTRL);
+ PmConf = Lan9118MmioRead32 (LAN9118_PMT_CTRL);
// MPTCTRL_WOL_EN: Allow Wake-On-Lan to detect wake up frames or magic
packets
// MPTCTRL_ED_EN: Allow energy detection to allow lowest power consumption
mode
@@ -306,7 +306,7 @@ SnpInitialize (
PmConf |= (MPTCTRL_WOL_EN | MPTCTRL_ED_EN | MPTCTRL_PME_EN);
// Write the current configuration to the register
- MmioWrite32 (LAN9118_PMT_CTRL, PmConf);
+ Lan9118MmioWrite32 (LAN9118_PMT_CTRL, PmConf);
gBS->Stall (LAN9118_STALL);
gBS->Stall (LAN9118_STALL);
@@ -359,7 +359,7 @@ SnpInitialize (
}
// Now acknowledge all interrupts
- MmioWrite32 (LAN9118_INT_STS, ~0);
+ Lan9118MmioWrite32 (LAN9118_INT_STS, ~0);
// Declare the driver as initialized
Snp->Mode->State = EfiSimpleNetworkInitialized;
@@ -422,7 +422,7 @@ SnpReset (
}
// Read the PM register
- PmConf = MmioRead32 (LAN9118_PMT_CTRL);
+ PmConf = Lan9118MmioRead32 (LAN9118_PMT_CTRL);
// MPTCTRL_WOL_EN: Allow Wake-On-Lan to detect wake up frames or magic
packets
// MPTCTRL_ED_EN: Allow energy detection to allow lowest power consumption
mode
@@ -430,7 +430,7 @@ SnpReset (
PmConf |= (MPTCTRL_WOL_EN | MPTCTRL_ED_EN | MPTCTRL_PME_EN);
// Write the current configuration to the register
- MmioWrite32 (LAN9118_PMT_CTRL, PmConf);
+ Lan9118MmioWrite32 (LAN9118_PMT_CTRL, PmConf);
gBS->Stall (LAN9118_STALL);
// Reactivate the LEDs
@@ -441,11 +441,11 @@ SnpReset (
// Check that a buffer size was specified in SnpInitialize
if (gTxBuffer != 0) {
-HwConf = MmioRead32 (LAN9118_HW_CFG);// Read the HW register
+HwConf = Lan9118MmioRead32 (LAN9118_HW_CFG);// Read the HW register
HwConf &= ~HW_CFG_TX_FIFO_SIZE_MASK; // Clear buffer bits first
HwConf |= HW_CFG_TX_FIFO_SIZE(gTxBuffer);// assign size chosen in
SnpInitialize
-MmioWrite32 (LAN9118_HW_CFG, HwConf);// Write the conf
+Lan9118MmioWrite32 (LAN9118_HW_CFG, HwConf);// Write the conf
gBS->Stall (LAN9118_STALL);
}
@@ -454,7 +454,7 @@ SnpReset (
StartTx (START_TX_MAC | START_TX_CFG | START_TX_CLEAR, Snp);
// Now acknowledge all interrupts
- MmioWrite32 (LAN9118_INT_STS, ~0);
+ Lan9118MmioWrite32 (LAN9118_INT_STS, ~0);
return EFI_SUCCESS;
}
@@ -996,12 +996,12 @@ SnpGetStatus (
// consumer of SNP does not call GetStatus.)
// TODO will we lose TxStatuses if this happens? Maybe in SnpTransmit we
// should check for it and dump the TX Status FIFO.
- FifoInt = MmioRead32 (LAN9118_FIFO_INT);
+ FifoInt = Lan9118MmioRead32 (LAN9118_FIFO_INT);
// Clear the TX Status FIFO Overflow
if ((FifoInt & INSTS_TXSO) == 0) {
FifoInt |= INSTS_TXSO;
-MmioWrite32 (LAN9118_FIFO_INT, FifoInt);
+Lan9118MmioWrite32 (LAN9118_FIFO_INT, FifoInt);
}
// Read interrupt status if IrqStat is not NULL
@@ -1009,30 +1009,30 @@ SnpGetStatus (
*IrqStat = 0;
// Check for receive interrupt
-if (MmioRead32 (LAN9118_INT_STS) & INSTS_RSFL) { // Data moved from rx FIFO
+if (Lan9118MmioRead32 (LAN9118_INT_STS) & INSTS_RSFL) { // Data moved from
rx FIFO
*IrqStat |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
- MmioWrite32 (LAN9118_INT_STS,INSTS_RSFL);
+ Lan9118MmioWrite32 (LAN9118_INT_STS,INSTS_RSFL);
}
// Check for transmit interrupt
-if (MmioRead32 (LAN9118_INT_STS) & INSTS_TSFL) {
+if (Lan9118MmioRead32 (LAN9118_INT_STS) & INSTS_TSFL) {
*IrqStat |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
- MmioWrite32 (LAN9118_INT_STS,INSTS_TSFL);
+ Lan9118MmioWrite32 (LAN9118_INT_STS,INSTS_TSFL);
}
// Check for software interrupt
-if (MmioRead32 (LAN9118_INT_STS) & INSTS_SW_INT) {
+if (Lan9118MmioRead32 (LAN9118_INT_STS) & INSTS_SW_INT) {
*IrqStat |= EFI_SIMPLE_NETWORK_SOFTWARE_INTERRUPT;
- MmioWrite32 (LAN9118_INT_STS,INSTS_SW_INT);
+ Lan9118MmioWrite32 (LAN9118_INT_STS,INSTS_SW_INT);
}
}
// Check Status of transmitted packets
[edk2] [PATCH 2/4] EmbeddedPkg/Lan9118Dxe: add LAN9118 MMIO wrappers
As described in the LAN9118 datasheet, delays are necessary after some reads and writes in order to ensure subsequent reads do not see stale data. This patch adds helpers to provide these delays automatically, by performing dummy reads of the BYTE_TEST register (as recommended in the LAN9118 datasheet). This approach allows the device register file itself to provide the required delay, avoiding issues with early write acknowledgement, or re-ordering of MMIO accesses aganist other instructions (e.g. the delay loop). Cc: Ard Biesheuvel Cc: Leif Lindholm Cc: Ryan Harkin Signed-off-by: Mark Rutland Contributed-under: TianoCore Contribution Agreement 1.0 --- EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeHw.h | 71 + EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c | 48 + EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.h | 17 ++ 3 files changed, 136 insertions(+) diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeHw.h b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeHw.h index 9e89d27..1189584 100644 --- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeHw.h +++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeHw.h @@ -57,6 +57,77 @@ #define LAN9118_E2P_CMD (0x00B0 + LAN9118_BA)// EEPROM Command #define LAN9118_E2P_DATA (0x00B4 + LAN9118_BA)// EEPROM Data +/* + * Required delays following write cycles (number of BYTE_TEST reads) + * Taken from Table 6.1 in Revision 1.5 (07-11-08) of the LAN9118 datasheet. + * Where no delay listed, 0 has been assumed. + */ +#define LAN9118_RX_DATA_WR_DELAY 0 +#define LAN9118_RX_STATUS_WR_DELAY0 +#define LAN9118_RX_STATUS_PEEK_WR_DELAY 0 +#define LAN9118_TX_DATA_WR_DELAY 0 +#define LAN9118_TX_STATUS_WR_DELAY0 +#define LAN9118_TX_STATUS_PEEK_WR_DELAY 0 +#define LAN9118_ID_REV_WR_DELAY 0 +#define LAN9118_IRQ_CFG_WR_DELAY 3 +#define LAN9118_INT_STS_WR_DELAY 2 +#define LAN9118_INT_EN_WR_DELAY 1 +#define LAN9118_BYTE_TEST_WR_DELAY0 +#define LAN9118_FIFO_INT_WR_DELAY 1 +#define LAN9118_RX_CFG_WR_DELAY 1 +#define LAN9118_TX_CFG_WR_DELAY 1 +#define LAN9118_HW_CFG_WR_DELAY 1 +#define LAN9118_RX_DP_CTL_WR_DELAY1 +#define LAN9118_RX_FIFO_INF_WR_DELAY 0 +#define LAN9118_TX_FIFO_INF_WR_DELAY 3 +#define LAN9118_PMT_CTRL_WR_DELAY 7 +#define LAN9118_GPIO_CFG_WR_DELAY 1 +#define LAN9118_GPT_CFG_WR_DELAY 1 +#define LAN9118_GPT_CNT_WR_DELAY 3 +#define LAN9118_WORD_SWAP_WR_DELAY1 +#define LAN9118_FREE_RUN_WR_DELAY 4 +#define LAN9118_RX_DROP_WR_DELAY 0 +#define LAN9118_MAC_CSR_CMD_WR_DELAY 1 +#define LAN9118_MAC_CSR_DATA_WR_DELAY 1 +#define LAN9118_AFC_CFG_WR_DELAY 1 +#define LAN9118_E2P_CMD_WR_DELAY 1 +#define LAN9118_E2P_DATA_WR_DELAY 1 + +/* + * Required delays following read cycles (number of BYTE_TEST reads) + * Taken from Table 6.2 in Revision 1.5 (07-11-08) of the LAN9118 datasheet. + * Where no delay listed, 0 has been assumed. + */ +#define LAN9118_RX_DATA_RD_DELAY 3 +#define LAN9118_RX_STATUS_RD_DELAY3 +#define LAN9118_RX_STATUS_PEEK_RD_DELAY 0 +#define LAN9118_TX_DATA_RD_DELAY 0 +#define LAN9118_TX_STATUS_RD_DELAY3 +#define LAN9118_TX_STATUS_PEEK_RD_DELAY 0 +#define LAN9118_ID_REV_RD_DELAY 0 +#define LAN9118_IRQ_CFG_RD_DELAY 0 +#define LAN9118_INT_STS_RD_DELAY 0 +#define LAN9118_INT_EN_RD_DELAY 0 +#define LAN9118_BYTE_TEST_RD_DELAY0 +#define LAN9118_FIFO_INT_RD_DELAY 0 +#define LAN9118_RX_CFG_RD_DELAY 0 +#define LAN9118_TX_CFG_RD_DELAY 0 +#define LAN9118_HW_CFG_RD_DELAY 0 +#define LAN9118_RX_DP_CTL_RD_DELAY0 +#define LAN9118_RX_FIFO_INF_RD_DELAY 0 +#define LAN9118_TX_FIFO_INF_RD_DELAY 0 +#define LAN9118_PMT_CTRL_RD_DELAY 0 +#define LAN9118_GPIO_CFG_RD_DELAY 0 +#define LAN9118_GPT_CFG_RD_DELAY 0 +#define LAN9118_GPT_CNT_RD_DELAY 0 +#define LAN9118_WORD_SWAP_RD_DELAY0 +#define LAN9118_FREE_RUN_RD_DELAY 0 +#define LAN9118_RX_DROP_RD_DELAY 4 +#define LAN9118_MAC_CSR_CMD_RD_DELAY 0 +#define LAN9118_MAC_CSR_DATA_RD_DELAY 0 +#define LAN9118_AFC_CFG_RD_DELAY 0 +#define LAN9118_E2P_CMD_RD_DELAY 0 +#define LAN9118_E2P_DATA_RD_DELAY 0 // Receiver Status bits #define RXSTATUS_CRC_ERRORBIT1 // Cyclic Redundancy Check Error diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c index bd20eeb..002ea20 100644
[edk2] [PATCH 0/4] EmbeddedPkg/Lan9118Dxe MMIO fixes
The LAN9118 driver uses memory fences in a novel but erroneous fashion, due to a misunderstanding of some under-commented code. This series fixes these erroneous uses, documenting the unusual requirements of the LAN9118 chip that lead us to this situation, and introduces new helpers to handle this in a more consistent fashion. The LAN9118 datasheet is publicly available at: http://www.microchip.com/wwwproducts/en/LAN9118 Thanks, Mark. Mark Rutland (4): Revert "EmbeddedPkg/Lan9118Dxe: use MemoryFence" EmbeddedPkg/Lan9118Dxe: add LAN9118 MMIO wrappers EmbeddedPkg/Lan9118Dxe: Use LAN9118 MMIO wrappers EmbeddedPkg/Lan9118Dxe: remove redundant stalls EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 82 +- EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeHw.h | 75 - EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c | 200 ++-- EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.h | 17 ++ 4 files changed, 243 insertions(+), 131 deletions(-) -- 1.9.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 1/4] Revert "EmbeddedPkg/Lan9118Dxe: use MemoryFence"
Commit a4626006bbf86113 ("EmbeddedPkg/Lan9118Dxe: use MemoryFence")
replaced some stalls with memory fences, on the presumption that these
were erroneously being used to order memory accesses. However, this was
not the case.
LAN9118 devices require a timing delay between state-changing
reads/writes and subsequent reads, as updates to the register file are
asynchronous and the effects of state-changes are not immediately
visible to subsequent reads.
This delay cannot be ensured through the use of memory barriers, which
only enforce observable ordering, and not timing. Thus, converting these
stalls to memory fences was erroneous, and may result in stale values
being read.
This reverts commit a4626006bbf86113453aeb7920895e66cdd04737.
Cc: Ard Biesheuvel
Cc: Leif Lindholm
Cc: Ryan Harkin
Signed-off-by: Mark Rutland
Contributed-under: TianoCore Contribution Agreement 1.0
---
EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 9 +++---
EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c | 40 +++--
2 files changed, 23 insertions(+), 26 deletions(-)
diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
index d0bf7be..50644e7 100644
--- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
+++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
@@ -307,7 +307,8 @@ SnpInitialize (
// Write the current configuration to the register
MmioWrite32 (LAN9118_PMT_CTRL, PmConf);
- MemoryFence();
+ gBS->Stall (LAN9118_STALL);
+ gBS->Stall (LAN9118_STALL);
// Configure GPIO and HW
Status = ConfigureHardware (HW_CONF_USE_LEDS, Snp);
@@ -430,7 +431,7 @@ SnpReset (
// Write the current configuration to the register
MmioWrite32 (LAN9118_PMT_CTRL, PmConf);
- MemoryFence();
+ gBS->Stall (LAN9118_STALL);
// Reactivate the LEDs
Status = ConfigureHardware (HW_CONF_USE_LEDS, Snp);
@@ -445,7 +446,7 @@ SnpReset (
HwConf |= HW_CFG_TX_FIFO_SIZE(gTxBuffer);// assign size chosen in
SnpInitialize
MmioWrite32 (LAN9118_HW_CFG, HwConf);// Write the conf
-MemoryFence();
+gBS->Stall (LAN9118_STALL);
}
// Enable the receiver and transmitter and clear their contents
@@ -700,7 +701,7 @@ SnpReceiveFilters (
// Write the options to the MAC_CSR
//
IndirectMACWrite32 (INDIRECT_MAC_INDEX_CR, MacCSRValue);
- MemoryFence();
+ gBS->Stall (LAN9118_STALL);
//
// If we have to retrieve something, start packet reception.
diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c
b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c
index 3ef98ef..bd20eeb 100644
--- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c
+++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c
@@ -236,7 +236,7 @@ IndirectEEPROMRead32 (
// Write to Eeprom command register
MmioWrite32 (LAN9118_E2P_CMD, EepromCmd);
- MemoryFence();
+ gBS->Stall (LAN9118_STALL);
// Wait until operation has completed
while (MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY);
@@ -284,7 +284,7 @@ IndirectEEPROMWrite32 (
// Write to Eeprom command register
MmioWrite32 (LAN9118_E2P_CMD, EepromCmd);
- MemoryFence();
+ gBS->Stall (LAN9118_STALL);
// Wait until operation has completed
while (MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY);
@@ -362,14 +362,13 @@ Lan9118Initialize (
if (((MmioRead32 (LAN9118_PMT_CTRL) & MPTCTRL_PM_MODE_MASK) >> 12) != 0) {
DEBUG ((DEBUG_NET, "Waking from reduced power state.\n"));
MmioWrite32 (LAN9118_BYTE_TEST, 0x);
-MemoryFence();
+gBS->Stall (LAN9118_STALL);
}
// Check that device is active
Retries = 20;
while ((MmioRead32 (LAN9118_PMT_CTRL) & MPTCTRL_READY) == 0 && --Retries) {
gBS->Stall (LAN9118_STALL);
-MemoryFence();
}
if (!Retries) {
return EFI_TIMEOUT;
@@ -379,7 +378,6 @@ Lan9118Initialize (
Retries = 20;
while ((MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY) && --Retries){
gBS->Stall (LAN9118_STALL);
-MemoryFence();
}
if (!Retries) {
return EFI_TIMEOUT;
@@ -449,12 +447,11 @@ SoftReset (
// Write the configuration
MmioWrite32 (LAN9118_HW_CFG, HwConf);
- MemoryFence();
+ gBS->Stall (LAN9118_STALL);
// Wait for reset to complete
while (MmioRead32 (LAN9118_HW_CFG) & HWCFG_SRST) {
-MemoryFence();
gBS->Stall (LAN9118_STALL);
ResetTime += 1;
@@ -503,7 +500,7 @@ PhySoftReset (
// Wait for completion
while (MmioRead32 (LAN9118_PMT_CTRL) & MPTCTRL_PHY_RST) {
- MemoryFence();
+ gBS->Stall (LAN9118_STALL);
}
// PHY Basic Control Register reset
} else if (Flags & PHY_RESET_BCR) {
@@ -511,7 +508,7 @@ PhySoftReset (
// Wait for completion
while (IndirectPHYRead32 (PHY_INDEX_BASIC_CTRL) & PHYCR_RESET) {
- MemoryFence();
+ gBS->Stall (LAN9118_STALL);
}
}
@@ -545,7 +542,7 @@ ConfigureHardware (
// W
Re: [edk2] [PATCH v4] ArmPkg/AArch64Mmu: disable MMU during page table manipulations
On Thu, Apr 14, 2016 at 05:48:27PM +0200, Ard Biesheuvel wrote:
> On ARM, manipulating live page tables is cumbersome since the architecture
> mandates the use of break-before-make, i.e., replacing a block entry with
> a table entry requires an intermediate step via an invalid entry, or TLB
> conflicts may occur.
>
> Since it is not generally feasible to decide in the page table manipulation
> routines whether such an invalid entry will result in those routines
> themselves to become unavailable, use a function that is callable with
> the MMU off (i.e., a leaf function that does not access the stack) to
> perform the change of a block entry into a table entry.
>
> Note that the opposite should never occur, i.e., table entries are never
> coalesced into block entries.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel
Acked-by: Mark Rutland
Mark.
> ---
> ArmPkg/Include/Library/ArmLib.h | 6 ++
> ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf | 5 +-
> ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c | 36
> ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c| 17 +-
> ArmPkg/Library/ArmLib/AArch64/AArch64Support.S| 62
>
> 5 files changed, 124 insertions(+), 2 deletions(-)
>
> diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
> index 15f610d82e1d..1689f0072db6 100644
> --- a/ArmPkg/Include/Library/ArmLib.h
> +++ b/ArmPkg/Include/Library/ArmLib.h
> @@ -613,4 +613,10 @@ ArmClearMemoryRegionReadOnly (
>IN UINT64Length
>);
>
> +VOID
> +ArmReplaceLiveTranslationEntry (
> + IN UINT64 *Entry,
> + IN UINT64 Value
> + );
> +
> #endif // __ARM_LIB__
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf
> b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf
> index dd585dea91fb..58684e8492f2 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf
> @@ -17,9 +17,10 @@ [Defines]
>INF_VERSION= 0x00010005
>BASE_NAME = AArch64Lib
>FILE_GUID = ef20ddf5-b334-47b3-94cf-52ff44c29138
> - MODULE_TYPE= DXE_DRIVER
> + MODULE_TYPE= BASE
>VERSION_STRING = 1.0
>LIBRARY_CLASS = ArmLib
> + CONSTRUCTOR= AArch64LibConstructor
>
> [Sources.AARCH64]
>AArch64Lib.c
> @@ -31,6 +32,7 @@ [Sources.AARCH64]
>
>../Common/AArch64/ArmLibSupport.S
>../Common/ArmLib.c
> + AArch64LibConstructor.c
>
> [Packages]
>ArmPkg/ArmPkg.dec
> @@ -38,6 +40,7 @@ [Packages]
>
> [LibraryClasses]
>MemoryAllocationLib
> + CacheMaintenanceLib
>
> [Protocols]
>gEfiCpuArchProtocolGuid
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c
> b/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c
> new file mode 100644
> index ..d2d0d3c15ee3
> --- /dev/null
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c
> @@ -0,0 +1,36 @@
> +#/* @file
> +#
> +# Copyright (c) 2016, Linaro Limited. 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
> +# http://opensource.org/licenses/bsd-license.php
> +#
> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> IMPLIED.
> +#
> +#*/
> +
> +#include
> +
> +#include
> +#include
> +
> +RETURN_STATUS
> +EFIAPI
> +AArch64LibConstructor (
> + VOID
> + )
> +{
> + extern UINT32 ArmReplaceLiveTranslationEntrySize;
> +
> + //
> + // The ArmReplaceLiveTranslationEntry () helper function may be invoked
> + // with the MMU off so we have to ensure that it gets cleaned to the PoC
> + //
> + WriteBackDataCacheRange (ArmReplaceLiveTranslationEntry,
> +ArmReplaceLiveTranslationEntrySize);
> +
> + return RETURN_SUCCESS;
> +}
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> index b7d23c6f3286..2cc6fc45aecf 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> @@ -169,6 +169,20 @@ GetRootTranslationTableInfo (
>
> STATIC
> VOID
> +ReplaceLiveEntry (
> + IN UINT64 *Entry,
> + IN UINT64 Value
> + )
&g
Re: [edk2] [PATCH v3] ArmPkg/AArch64Mmu: disable MMU during page table manipulations
On Thu, Apr 14, 2016 at 01:36:00PM +0200, Ard Biesheuvel wrote: > On 14 April 2016 at 13:17, Mark Rutland wrote: > > On Tue, Apr 12, 2016 at 08:57:46AM +0200, Ard Biesheuvel wrote: > > I take it that ArmReplaceLiveTranslationEntry isn't part of the static > > image (e.g. it gets relocated or copied after the MMU is enabled for the > > first time)? > > Yes, the various DXE modules are loaded from a compressed firmware > volume by a PE/COFF loader which may clean to the PoU only. Ok. > >> + // write an entry and invalidate it in the caches > >> + .macro __set_entry, reg > >> + str \reg, [x0] > >> + dmb sy > >> + dcivac, x0 > >> + dsb sy > >> + .endm > >> + > >> + .macro __replace_entry, el > >> + mrs x8, sctlr_el\el > >> + bic x9, x8, #CTRL_M_BIT // Clear MMU enable bit > >> + msr sctlr_el\el, x9 > >> + isb > >> + > >> + __set_entry xzr// write invalid entry > > > > The MMU is off, and thus there are no TLB walks, so I don't see the > > point in storing+invalidating the invalid entry here. > > Do you mean storing is sufficient? Or that we don't need bbm in the > first place when disabling the MMU? With the MMU off we don't need to follow a strict BBM sequence. > Because it seems to me that one implies the other: if we can flush the > TLBs without the risk of them refetching the stale entries, we can > just write the new value here and be done with it. Yes, provided we invalidate the TLBs while the MMU is off, and have the appropriate cache maintenance. Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3] ArmPkg/AArch64Mmu: disable MMU during page table manipulations
On Tue, Apr 12, 2016 at 08:57:46AM +0200, Ard Biesheuvel wrote:
> On ARM, manipulating live page tables is cumbersome since the architecture
> mandates the use of break-before-make, i.e., replacing a block entry with
> a table entry requires an intermediate step via an invalid entry, or TLB
> conflicts may occur.
>
> Since it is not generally feasible to decide in the page table manipulation
> routines whether such an invalid entry will result in those routines
> themselves to become unavailable, use a function that is callable with
> the MMU off (i.e., a leaf function that does not access the stack) to
> perform the change of a block entry into a table entry.
>
> Note that the opposite should never occur, i.e., table entries are never
> coalesced into block entries.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel
> ---
> ArmPkg/Include/Library/ArmLib.h | 6 +++
> ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf | 5 +-
> ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c | 36 +
> ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c| 17 +-
> ArmPkg/Library/ArmLib/AArch64/AArch64Support.S| 57
>
> 5 files changed, 119 insertions(+), 2 deletions(-)
> +RETURN_STATUS
> +EFIAPI
> +AArch64LibConstructor (
> + VOID
> + )
> +{
> + extern UINT32 ArmReplaceLiveTranslationEntrySize;
> +
> + //
> + // The ArmReplaceLiveTranslationEntry () helper function may be invoked
> + // with the MMU off so we have to ensure that it gets cleaned to the PoC
> + //
> + WriteBackDataCacheRange (ArmReplaceLiveTranslationEntry,
> +ArmReplaceLiveTranslationEntrySize);
> +
> + return RETURN_SUCCESS;
> +}
I take it that ArmReplaceLiveTranslationEntry isn't part of the static
image (e.g. it gets relocated or copied after the MMU is enabled for the
first time)?
Otherwise I'm not following why the code wouldn't be clean to the PoC.
> + // write an entry and invalidate it in the caches
> + .macro __set_entry, reg
> + str \reg, [x0]
> + dmb sy
> + dcivac, x0
> + dsb sy
> + .endm
> +
> + .macro __replace_entry, el
> + mrs x8, sctlr_el\el
> + bic x9, x8, #CTRL_M_BIT // Clear MMU enable bit
> + msr sctlr_el\el, x9
> + isb
> +
> + __set_entry xzr// write invalid entry
The MMU is off, and thus there are no TLB walks, so I don't see the
point in storing+invalidating the invalid entry here.
It might be better to hoist the DC CIVAC from
ArmReplaceLiveTranslationEntry to here, to keep the maintenance
together, and avoid the redundant invalidate.
With the __set_entry macro folded in here, it would be a little clearer,
too.
Other than that, the general strategey seems sound to me.
Thanks,
Mark.
> + .if \el == 1
> + tlbi vmalle1
> + .else
> + tlbi alle\el
> + .endif
> + dsb sy
> + __set_entry x1 // write updated entry
> + msr sctlr_el\el, x8
> + isb
> + .endm
> +
> +//VOID
> +//ArmReplaceLiveTranslationEntry (
> +// IN UINT64 *Entry,
> +// IN UINT64 Value
> +// )
> +ASM_PFX(ArmReplaceLiveTranslationEntry):
> +
> + // disable interrupts
> + mrs x2, daif
> + msr daifset, #0xf
> + isb
> +
> + // clean and invalidate first so that we don't clobber adjacent entries
> + // that are dirty in the caches
> + dccivac, x0
> + dsb ish
> +
> + EL1_OR_EL2_OR_EL3(x3)
> +1:__replace_entry 1
> + b 4f
> +2:__replace_entry 2
> + b 4f
> +3:__replace_entry 3
> +4:msr daif, x2
> + ret
> +
> +ASM_PFX(ArmReplaceLiveTranslationEntrySize):
> + .long. - ArmReplaceLiveTranslationEntry
> +
> ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> --
> 2.5.0
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] ArmPkg/AArch64Mmu: disable MMU during page table manipulations
On Mon, Apr 11, 2016 at 06:50:06PM +0200, Ard Biesheuvel wrote: > On 11 April 2016 at 18:47, Mark Rutland wrote: > > On Mon, Apr 11, 2016 at 06:08:29PM +0200, Ard Biesheuvel wrote: > >> +//VOID > >> +//ArmReplaceLiveTranslationEntry ( > >> +// IN UINT64 *Entry, > >> +// IN UINT64 Value > >> +// ) > >> +ASM_PFX(ArmReplaceLiveTranslationEntry): > >> + .macro __replace_entry, el > >> + mrs x8, sctlr_el\el > >> + and x9, x8, #~CTRL_M_BIT // Clear MMU enable bit > >> + msr sctlr_el\el, x9 > >> + isb > >> + > >> + // write an invalid entry and invalidate it in the caches > >> + str xzr, [x0] > >> + dmb sy > >> + dcivac, x0 > > > > Is it definitely the case that the line is not potentially dirty? > > > > If it is a possiblity, you need to invalidate first. > > > > Yes, that happens before this macro is invoked > > >> + .if \el == 1 > >> + tlbi vmalle1 > >> + .else > >> + tlbi alle\el > >> + .endif > >> + dsb sy > >> + msr sctlr_el\el, x8 > >> + isb > > > > We never did str x1, [x0], so we re-enable the MMU with the entry > > invalid. If that's safe, then there's no point turning the MMU off in > > the first place; this is just a convoluted BBM sequence > > > > I thought the problem was that the entry might be in active use by this > > code (either mapping code or data). For that, you also need to create > > the new entry before re-enabling the MMU. > > > > Indeed. I had spotted that myself, and mentioned it in my reply to self > > > So long as speculation isn't a problem, you only need the prior > > invalidate. Though for KVM you're at the mercy of the host's cacheable > > alias regardless... > > > > Could you elaborate? The KVM host has a linear alias of RAM, with the usual attributes we expect (Normal, Inner Write-Back, Outer Write-Back, Inner Shareable). It's possible for cache allocations to occur as a result of this mapping, regardless of the state of the guest (e.g. even if the MMU is off from its PoV). Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] ArmPkg/AArch64Mmu: disable MMU during page table manipulations
On Mon, Apr 11, 2016 at 06:08:29PM +0200, Ard Biesheuvel wrote: > +//VOID > +//ArmReplaceLiveTranslationEntry ( > +// IN UINT64 *Entry, > +// IN UINT64 Value > +// ) > +ASM_PFX(ArmReplaceLiveTranslationEntry): > + .macro __replace_entry, el > + mrs x8, sctlr_el\el > + and x9, x8, #~CTRL_M_BIT // Clear MMU enable bit > + msr sctlr_el\el, x9 > + isb > + > + // write an invalid entry and invalidate it in the caches > + str xzr, [x0] > + dmb sy > + dcivac, x0 Is it definitely the case that the line is not potentially dirty? If it is a possiblity, you need to invalidate first. > + .if \el == 1 > + tlbi vmalle1 > + .else > + tlbi alle\el > + .endif > + dsb sy > + msr sctlr_el\el, x8 > + isb We never did str x1, [x0], so we re-enable the MMU with the entry invalid. If that's safe, then there's no point turning the MMU off in the first place; this is just a convoluted BBM sequence I thought the problem was that the entry might be in active use by this code (either mapping code or data). For that, you also need to create the new entry before re-enabling the MMU. So long as speculation isn't a problem, you only need the prior invalidate. Though for KVM you're at the mercy of the host's cacheable alias regardless... > + .endm > + > + // disable interrupts > + mrs x2, daif > + msr daifset, #0xf > + isb > + > + // clean and invalidate first so that we don't clobber adjacent entries > + // that are dirty in the caches > + dccivac, x0 > + dsb ish > + > + EL1_OR_EL2_OR_EL3(x3) > +1:__replace_entry 1 > + b 4f > +2:__replace_entry 2 > + b 4f > +3:__replace_entry 3 > +4:msr daif, x2 > + str x1, [x0] As above, this is too late if the entry is in active use. Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg/AArch64Mmu: disable MMU during page table manipulations
Hi Ard,
On Mon, Apr 11, 2016 at 03:57:15PM +0200, Ard Biesheuvel wrote:
> On ARM, manipulating live page tables is cumbersome since the architecture
> mandates the use of break-before-make, i.e., replacing a block entry with
> a table entry requires an intermediate step via an invalid entry, or TLB
> conflicts may occur.
>
> Since it is not generally feasible to decide in the page table manipulation
> routines whether such an invalid entry will result in those routines
> themselves to become unavailable, and since UEFI uses an identity mapping
> anyway, it is far simpler to just disable the MMU, perform the page table
> manipulations, flush the TLBs and re-enable the MMU.
Perhaps I am missing something, but I suspect that this isn't so simple.
More on that below.
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel
> ---
> ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 39
> 1 file changed, 39 insertions(+)
>
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> index b7d23c6f3286..2f8f05df8aec 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> @@ -286,6 +286,11 @@ GetBlockEntryListFromAddress (
>}
> } else if ((*BlockEntry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) {
>// If we are not at the last level then we need to split this
> BlockEntry
> + // Since splitting live block entries may cause TLB conflicts, we need
> to
> + // enter this function with the MMU disabled and flush the TLBs before
> + // re-enabling it. This is the responsibility of the caller.
> + ASSERT (!ArmMmuEnabled ());
> +
>if (IndexLevel != PageLevel) {
> // Retrieve the attributes from the block entry
> Attributes = *BlockEntry & TT_ATTRIBUTES_MASK;
> @@ -453,6 +458,8 @@ SetMemoryAttributes (
>RETURN_STATUSStatus;
>ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion;
>UINT64 *TranslationTable;
> + UINTNSavedInterruptState;
> + BOOLEAN SavedMmuState;
>
>MemoryRegion.PhysicalBase = BaseAddress;
>MemoryRegion.VirtualBase = BaseAddress;
> @@ -461,6 +468,15 @@ SetMemoryAttributes (
>
>TranslationTable = ArmGetTTBR0BaseAddress ();
>
> + SavedMmuState = ArmMmuEnabled ();
> + if (SavedMmuState) {
> +SavedInterruptState = ArmGetInterruptState ();
> +if (SavedInterruptState) {
> + ArmDisableInterrupts();
> +}
> +ArmDisableMmu();
> + }
Unless ArmDisableMmu() performs a Clean+Invalidate of all memory in use
by EDK2 (after the MMU is disabled), this is not safe.
For example, the latest version of your stack (which may be dirty in
cache) is not guaranteed to be visible after the MMU is disabled. If any
of that is dirty it may be naturally evicted at any time,
masking/corrupting data written with the MMU off. Any implicit memory
accesses generated by the compiler may go wrong.
A similar problem applies to anything previously mapped with cacheable
attributes.
> +
>Status = FillTranslationTable (TranslationTable, &MemoryRegion);
>if (RETURN_ERROR (Status)) {
> return Status;
> @@ -468,6 +484,12 @@ SetMemoryAttributes (
>
>// Invalidate all TLB entries so changes are synced
>ArmInvalidateTlb ();
> + if (SavedMmuState) {
> +ArmEnableMmu();
> +if (SavedInterruptState) {
> + ArmEnableInterrupts ();
> +}
> + }
Likewise, now everything written with the MMU off is not guaranteed to
be visible to subsequent cacheable accesses, due to stale lines
allocated before the MMU was disabled. That includes the modifications
to the page tables, which may become eventually become visible to
cacheable accesses in an unpredictable fashion (e.g. you might still end
up with the TLBs filling with conflicting entries).
>return RETURN_SUCCESS;
> }
> @@ -483,9 +505,20 @@ SetMemoryRegionAttribute (
> {
>RETURN_STATUSStatus;
>UINT64 *RootTable;
> + UINTNSavedInterruptState;
> + BOOLEAN SavedMmuState;
>
>RootTable = ArmGetTTBR0BaseAddress ();
>
> + SavedMmuState = ArmMmuEnabled ();
> + if (SavedMmuState) {
> +SavedInterruptState = ArmGetInterruptState ();
> +if (SavedInterruptState) {
> + ArmDisableInterrupts();
> +}
> +ArmDisableMmu();
> + }
> +
>Status = UpdateRegionMapping (RootTable, BaseAddress, Length, Attributes,
> BlockEntryMask);
>if (RETURN_ERROR (Status)) {
> return Status;
> @@ -493,6 +526,12 @@ SetMemoryRegionAttribute (
>
>// Invalidate all TLB entries so changes are synced
>ArmInvalidateTlb ();
> + if (SavedMmuState) {
> +ArmEnableMmu();
> +if (SavedInterruptState) {
> + ArmEnableInterrupts ();
> +}
> + }
The same problems apply to both of these.
Thanks,
Mark.
_
Re: [edk2] [PATCH] ArmPkg: rewrite vector table population macros
On Wed, Dec 16, 2015 at 12:24:30PM +0100, Ard Biesheuvel wrote: > On 16 December 2015 at 12:18, Mark Rutland wrote: > > On Wed, Dec 16, 2015 at 10:37:39AM +0100, Ard Biesheuvel wrote: > >> diff --git a/ArmPkg/Include/Chipset/AArch64.h > >> b/ArmPkg/Include/Chipset/AArch64.h > >> index 5e1653bcb69d..7be18546799c 100644 > >> --- a/ArmPkg/Include/Chipset/AArch64.h > >> +++ b/ArmPkg/Include/Chipset/AArch64.h > >> @@ -119,15 +119,18 @@ > >> #define ARM_VECTOR_LOW_A32_SERR 0x780 > >> > >> #define VECTOR_BASE(tbl) \ > >> + .section .text.##tbl##,"aw";\ > > > > Shouldn't this have "x", too? > > > > Do we overwrite this? If not, it could just be "ax" rather than "awx". > > > > Ah yes, I meant "ax" not "aw" Ok. With that: Acked-by: Mark Rutland Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg: rewrite vector table population macros
On Wed, Dec 16, 2015 at 10:37:39AM +0100, Ard Biesheuvel wrote: > Unfortunately, Clang does not support the use of symbol references in .org > directives, and bails with the following error message when it encounters > them: > > <...>:error: expected assembly-time absolute expression > .org DebugAgentVectorTable + 0x000 Ah. Sorry for that breakage. > So replace the .org arguments with absolute values, and move the whole > vector table into a subsection with the appropriate alignment, and > starting at .org 0x0. This gives the same protection with respect to > entries that exceed 128 bytes, in a way that Clang supports as well. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > > Surprisingly, this worked fine in Clang-3.5 but not in 3.6 or 3.7 Have you queried the clang developers about that? This might not be deliberate, and they'd probably want to know, even if we have to work around it here. > ArmPkg/Include/Chipset/AArch64.h | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/ArmPkg/Include/Chipset/AArch64.h > b/ArmPkg/Include/Chipset/AArch64.h > index 5e1653bcb69d..7be18546799c 100644 > --- a/ArmPkg/Include/Chipset/AArch64.h > +++ b/ArmPkg/Include/Chipset/AArch64.h > @@ -119,15 +119,18 @@ > #define ARM_VECTOR_LOW_A32_SERR 0x780 > > #define VECTOR_BASE(tbl) \ > + .section .text.##tbl##,"aw";\ Shouldn't this have "x", too? Do we overwrite this? If not, it could just be "ax" rather than "awx". Otherwise this looks good to me. Thanks, Mark. >.align 11; \ > + .org 0x0; \ >GCC_ASM_EXPORT(tbl);\ >ASM_PFX(tbl): \ > > #define VECTOR_ENTRY(tbl, off)\ > - .org ASM_PFX(tbl) + off > + .org off > > #define VECTOR_END(tbl) \ > - .org ASM_PFX(tbl) + 0x800 > + .org 0x800; \ > + .previous > > VOID > EFIAPI > -- > 2.5.0 > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPlatformPkg/PrePeiCore: add missing entries to AArch64 vector table
On Thu, Nov 26, 2015 at 04:06:40PM +0100, Ard Biesheuvel wrote:
> On 20 November 2015 at 13:46, Mark Rutland wrote:
> > On Fri, Nov 20, 2015 at 01:39:26PM +0100, Ard Biesheuvel wrote:
> >> The PrePeiCore vector table for AArch64 mode is only half populated.
> >> However unlikely, if exceptions from lower exception levels are ever
> >> taken, they should be reported correctly, rather than causing a
> >> recursive undefined instruction fault on the zero padding that was
> >> introduced by commit SVN r18904 ("ArmPkg/ArmPlatformPkg: position
> >> vectors relative to base"). So add the missing entries, and wire
> >> them up to the default handler.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel
> >
> > Acked-by: Mark Rutland
> >
> > Mark.
> >
>
> Ping?
Applied locally.
Thanks,
Mark.
p.s. I assume you're asking Leif to apply this upstream?
>
> >> ---
> >> ArmPlatformPkg/PrePeiCore/AArch64/Exception.S | 40
> >> 1 file changed, 40 insertions(+)
> >>
> >> diff --git a/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S
> >> b/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S
> >> index b31854ced256..75cd98ff4863 100644
> >> --- a/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S
> >> +++ b/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S
> >> @@ -77,4 +77,44 @@ _DefaultSError_h:
> >>mov x0, #EXCEPT_AARCH64_SERROR
> >>TO_HANDLER
> >>
> >> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A64_SYNC)
> >> +_DefaultSyncExceptHandler_LowerA64:
> >> + mov x0, #EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS
> >> + TO_HANDLER
> >> +
> >> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A64_IRQ)
> >> +_DefaultIrq_LowerA64:
> >> + mov x0, #EXCEPT_AARCH64_IRQ
> >> + TO_HANDLER
> >> +
> >> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A64_FIQ)
> >> +_DefaultFiq_LowerA64:
> >> + mov x0, #EXCEPT_AARCH64_FIQ
> >> + TO_HANDLER
> >> +
> >> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A64_SERR)
> >> +_DefaultSError_LowerA64:
> >> + mov x0, #EXCEPT_AARCH64_SERROR
> >> + TO_HANDLER
> >> +
> >> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A32_SYNC)
> >> +_DefaultSyncExceptHandler_LowerA32:
> >> + mov x0, #EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS
> >> + TO_HANDLER
> >> +
> >> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A32_IRQ)
> >> +_DefaultIrq_LowerA32:
> >> + mov x0, #EXCEPT_AARCH64_IRQ
> >> + TO_HANDLER
> >> +
> >> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A32_FIQ)
> >> +_DefaultFiq_LowerA32:
> >> + mov x0, #EXCEPT_AARCH64_FIQ
> >> + TO_HANDLER
> >> +
> >> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A32_SERR)
> >> +_DefaultSError_LowerA32:
> >> + mov x0, #EXCEPT_AARCH64_SERROR
> >> + TO_HANDLER
> >> +
> >> VECTOR_END(PeiVectorTable)
> >> --
> >> 1.9.1
> >>
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPlatformPkg/PrePeiCore: add missing entries to AArch64 vector table
On Fri, Nov 20, 2015 at 01:39:26PM +0100, Ard Biesheuvel wrote:
> The PrePeiCore vector table for AArch64 mode is only half populated.
> However unlikely, if exceptions from lower exception levels are ever
> taken, they should be reported correctly, rather than causing a
> recursive undefined instruction fault on the zero padding that was
> introduced by commit SVN r18904 ("ArmPkg/ArmPlatformPkg: position
> vectors relative to base"). So add the missing entries, and wire
> them up to the default handler.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel
Acked-by: Mark Rutland
Mark.
> ---
> ArmPlatformPkg/PrePeiCore/AArch64/Exception.S | 40
> 1 file changed, 40 insertions(+)
>
> diff --git a/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S
> b/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S
> index b31854ced256..75cd98ff4863 100644
> --- a/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S
> +++ b/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S
> @@ -77,4 +77,44 @@ _DefaultSError_h:
>mov x0, #EXCEPT_AARCH64_SERROR
>TO_HANDLER
>
> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A64_SYNC)
> +_DefaultSyncExceptHandler_LowerA64:
> + mov x0, #EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS
> + TO_HANDLER
> +
> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A64_IRQ)
> +_DefaultIrq_LowerA64:
> + mov x0, #EXCEPT_AARCH64_IRQ
> + TO_HANDLER
> +
> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A64_FIQ)
> +_DefaultFiq_LowerA64:
> + mov x0, #EXCEPT_AARCH64_FIQ
> + TO_HANDLER
> +
> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A64_SERR)
> +_DefaultSError_LowerA64:
> + mov x0, #EXCEPT_AARCH64_SERROR
> + TO_HANDLER
> +
> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A32_SYNC)
> +_DefaultSyncExceptHandler_LowerA32:
> + mov x0, #EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS
> + TO_HANDLER
> +
> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A32_IRQ)
> +_DefaultIrq_LowerA32:
> + mov x0, #EXCEPT_AARCH64_IRQ
> + TO_HANDLER
> +
> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A32_FIQ)
> +_DefaultFiq_LowerA32:
> + mov x0, #EXCEPT_AARCH64_FIQ
> + TO_HANDLER
> +
> +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_LOW_A32_SERR)
> +_DefaultSError_LowerA32:
> + mov x0, #EXCEPT_AARCH64_SERROR
> + TO_HANDLER
> +
> VECTOR_END(PeiVectorTable)
> --
> 1.9.1
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg: ArmLib: purge incorrect ArmDrainWriteBuffer () alias
On Thu, Nov 19, 2015 at 04:25:31PM +, Leif Lindholm wrote:
> In ArmLib, there exists an alias for ArmDataSynchronizationBarrier,
> named after one of several names for the pre-ARMv6 cp15 operation that
> was formalised into the Data Synchronization Barrier in ARMv6.
>
> This alias is also the one called from within ArmLib, in preference of
> the correct name. Through the power of code reuse, this name slipped
> into the AArch64 variant as well.
>
> Expunge it from the codebase.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leif Lindholm
Looks sensible to me. FWIW:
Acked-by: Mark Rutland
Mark.
> ---
> ArmPkg/Include/Library/ArmLib.h| 6 --
> ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c | 8
> ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 2 --
> ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c | 8
> ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S | 2 --
> ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm | 2 --
> 6 files changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
> index a328146..9622444 100644
> --- a/ArmPkg/Include/Library/ArmLib.h
> +++ b/ArmPkg/Include/Library/ArmLib.h
> @@ -393,12 +393,6 @@ ArmSetHighVectors (
>
> VOID
> EFIAPI
> -ArmDrainWriteBuffer (
> - VOID
> - );
> -
> -VOID
> -EFIAPI
> ArmDataMemoryBarrier (
>VOID
>);
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> index dec125f..ec35097 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> @@ -33,7 +33,7 @@ AArch64DataCacheOperation (
>
>AArch64AllDataCachesOperation (DataCacheOperation);
>
> - ArmDrainWriteBuffer ();
> + ArmDataSynchronizationBarrier ();
>
>if (SavedInterruptState) {
> ArmEnableInterrupts ();
> @@ -46,7 +46,7 @@ ArmInvalidateDataCache (
>VOID
>)
> {
> - ArmDrainWriteBuffer ();
> + ArmDataSynchronizationBarrier ();
>AArch64DataCacheOperation (ArmInvalidateDataCacheEntryBySetWay);
> }
>
> @@ -56,7 +56,7 @@ ArmCleanInvalidateDataCache (
>VOID
>)
> {
> - ArmDrainWriteBuffer ();
> + ArmDataSynchronizationBarrier ();
>AArch64DataCacheOperation (ArmCleanInvalidateDataCacheEntryBySetWay);
> }
>
> @@ -66,6 +66,6 @@ ArmCleanDataCache (
>VOID
>)
> {
> - ArmDrainWriteBuffer ();
> + ArmDataSynchronizationBarrier ();
>AArch64DataCacheOperation (ArmCleanDataCacheEntryBySetWay);
> }
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> index df2dc93..c530d19 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> @@ -26,7 +26,6 @@ GCC_ASM_EXPORT (ArmCleanInvalidateDataCacheEntryByMVA)
> GCC_ASM_EXPORT (ArmInvalidateDataCacheEntryBySetWay)
> GCC_ASM_EXPORT (ArmCleanDataCacheEntryBySetWay)
> GCC_ASM_EXPORT (ArmCleanInvalidateDataCacheEntryBySetWay)
> -GCC_ASM_EXPORT (ArmDrainWriteBuffer)
> GCC_ASM_EXPORT (ArmEnableMmu)
> GCC_ASM_EXPORT (ArmDisableMmu)
> GCC_ASM_EXPORT (ArmDisableCachesAndMmu)
> @@ -364,7 +363,6 @@ ASM_PFX(ArmDataMemoryBarrier):
>
>
> ASM_PFX(ArmDataSynchronizationBarrier):
> -ASM_PFX(ArmDrainWriteBuffer):
>dsb sy
>ret
>
> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
> index b53f455..23a7f2f 100644
> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
> @@ -32,7 +32,7 @@ ArmV7DataCacheOperation (
>
>ArmV7AllDataCachesOperation (DataCacheOperation);
>
> - ArmDrainWriteBuffer ();
> + ArmDataSynchronizationBarrier ();
>
>if (SavedInterruptState) {
> ArmEnableInterrupts ();
> @@ -45,7 +45,7 @@ ArmInvalidateDataCache (
>VOID
>)
> {
> - ArmDrainWriteBuffer ();
> + ArmDataSynchronizationBarrier ();
>ArmV7DataCacheOperation (ArmInvalidateDataCacheEntryBySetWay);
> }
>
> @@ -55,7 +55,7 @@ ArmCleanInvalidateDataCache (
>VOID
>)
> {
> - ArmDrainWriteBuffer ();
> + ArmDataSynchronizationBarrier ();
>ArmV7DataCacheOperation (ArmCleanInvalidateDataCacheEntryBySetWay);
> }
>
> @@ -65,6 +65,6 @@ ArmCleanDataCache (
>VOID
>)
> {
> - ArmDrainWriteBuffer ();
> + ArmDataSynchronizationBarrier ();
>ArmV7DataCacheOperation (ArmCleanDataCacheEntryBySetWay);
> }
> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S
> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Su
Re: [edk2] [PATCH] ArmPkg/ArmPlatformPkg: position vectors relative to base
On Thu, Nov 19, 2015 at 03:42:51PM +0100, Ard Biesheuvel wrote: > On 19 November 2015 at 14:44, Mark Rutland wrote: > > We currently rely on .align directives to ensure that each exception > > vector entry is the appropriate offset from the vector base address. > > > > This is slightly fragile, as were an entry to become too large (greater > > than 32 A64 instructions), all following entries would be silently > > shifted until they meet the next alignment boundary. Thus we might > > execute the wrong code in response to an exception. > > > > To prevent this, introduce a new macro, VECTOR_ENTRY, that uses .org > > directives to position each entry at the precise required offset from > > the base of a vector. A vector entry which is too large will trigger a > > build failure rather than a runtime failure which is difficult to debug. > > > > For consistency, the base and end of each vector is similarly annotated, > > with VECTOR_BASE and VECTOR_END, which provide the necessary alignment > > and symbol exports. The now redundant directives and labels are removed. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Mark Rutland > > Cc: Ard Biesheuvel > > Cc: Leif Lindholm > > --- > > ArmPkg/Drivers/CpuDxe/AArch64/ExceptionSupport.S | 38 > > ++--- > > ArmPkg/Include/Chipset/AArch64.h | 32 ++ > > .../AArch64/DebugAgentException.S | 39 > > -- > > ArmPlatformPkg/PrePeiCore/AArch64/Exception.S | 27 --- > > 4 files changed, 85 insertions(+), 51 deletions(-) > > > > This is obviously an improvement, although very little exception or > interrupt handling actually takes place in EDK2. > > Reviewed-by: Ard Biesheuvel > > I needed to fix up some more tabs, and two instances of the incorrect > 'Lower EL using AArch32 : 0x0 - 0x180' which were not new but we may > just as well change while we are at it. Cheers, thanks for fixing those up. I'll try to do a better job w.r.t. whitespace in future. > Committed as SVN r18904 > > However, any idea what the deal is with half a vector table in the > last file? (ArmPlatformPkg/PrePeiCore/AArch64/Exception.S) Whoops, I meant to ask about that myself. I don't have a good idea, but I suspect the assumption was that there would be no exceptions from a lower EL at that point in time as it wasn't expected that we take any exceptions at this point in time (and thus we had no reason to eret). The .org in VECTOR_END will ensure that the first instruction of each of the missing vectors are 0x. As that's currently unallocated, that will currently trigger one of the current EL vectors. However, 0x isn't guaranteed by the architecture to be permanently unallocated. We should probably add the missing entries. Mark. > > diff --git a/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S > > b/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S > > index cdc8d92..b31854c 100644 > > --- a/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S > > +++ b/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S > > @@ -11,22 +11,17 @@ > > # > > # > > > > +#include > > #include > > #include > > #include > > > > .text > > -.align 11 > > - > > -GCC_ASM_EXPORT(PeiVectorTable) > > > > // > > //Default Exception Handlers > > // > > > > -ASM_PFX(PeiVectorTable): > > - > > - > > #define TO_HANDLER \ > > EL1_OR_EL2(x1) \ > > 1: mrs x1, elr_el1/* EL1 Exception Link Register */ ;\ > > @@ -40,42 +35,46 @@ ASM_PFX(PeiVectorTable): > > // No context saving at all. > > // > > > > -.align 7 > > +VECTOR_BASE(PeiVectorTable) > > + > > +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_CUR_SP0_SYNC) > > _DefaultSyncExceptHandler_t: > >mov x0, #EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS > >TO_HANDLER > > > > -.align 7 > > +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_CUR_SP0_IRQ) > > _DefaultIrq_t: > >mov x0, #EXCEPT_AARCH64_IRQ > >TO_HANDLER > > > > -.align 7 > > +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_CUR_SP0_FIQ) > > _DefaultFiq_t: > >mov x0, #EXCEPT_AARCH64_FIQ > >TO_HANDLER > > > > -.align 7 > > +VECTOR_ENTRY(PeiVectorTable, ARM_VECTOR_CUR_SP0_
[edk2] [PATCH] ArmPkg/ArmPlatformPkg: position vectors relative to base
We currently rely on .align directives to ensure that each exception vector entry is the appropriate offset from the vector base address. This is slightly fragile, as were an entry to become too large (greater than 32 A64 instructions), all following entries would be silently shifted until they meet the next alignment boundary. Thus we might execute the wrong code in response to an exception. To prevent this, introduce a new macro, VECTOR_ENTRY, that uses .org directives to position each entry at the precise required offset from the base of a vector. A vector entry which is too large will trigger a build failure rather than a runtime failure which is difficult to debug. For consistency, the base and end of each vector is similarly annotated, with VECTOR_BASE and VECTOR_END, which provide the necessary alignment and symbol exports. The now redundant directives and labels are removed. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Mark Rutland Cc: Ard Biesheuvel Cc: Leif Lindholm --- ArmPkg/Drivers/CpuDxe/AArch64/ExceptionSupport.S | 38 ++--- ArmPkg/Include/Chipset/AArch64.h | 32 ++ .../AArch64/DebugAgentException.S | 39 -- ArmPlatformPkg/PrePeiCore/AArch64/Exception.S | 27 --- 4 files changed, 85 insertions(+), 51 deletions(-) diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/ExceptionSupport.S b/ArmPkg/Drivers/CpuDxe/AArch64/ExceptionSupport.S index ca6c9a1..ea4c87a 100644 --- a/ArmPkg/Drivers/CpuDxe/AArch64/ExceptionSupport.S +++ b/ArmPkg/Drivers/CpuDxe/AArch64/ExceptionSupport.S @@ -12,6 +12,7 @@ // //-- +#include #include #include @@ -95,14 +96,12 @@ UINT64 Padding;0x328 // Required for stack alignment */ -GCC_ASM_EXPORT(ExceptionHandlersStart) GCC_ASM_EXPORT(ExceptionHandlersEnd) GCC_ASM_EXPORT(CommonExceptionEntry) GCC_ASM_EXPORT(AsmCommonExceptionEntry) GCC_ASM_EXPORT(CommonCExceptionHandler) .text -.align 11 #define GP_CONTEXT_SIZE(32 * 8) #define FP_CONTEXT_SIZE(32 * 16) @@ -160,84 +159,85 @@ GCC_ASM_EXPORT(CommonCExceptionHandler) // This code gets copied to the ARM vector table // VectorTableStart - VectorTableEnd gets copied // -ASM_PFX(ExceptionHandlersStart): +VECTOR_BASE(ExceptionHandlersStart) // // Current EL with SP0 : 0x0 - 0x180 // -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SP0_SYNC) ASM_PFX(SynchronousExceptionSP0): b ASM_PFX(SynchronousExceptionEntry) -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SP0_IRQ) ASM_PFX(IrqSP0): b ASM_PFX(IrqEntry) -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SP0_FIQ) ASM_PFX(FiqSP0): b ASM_PFX(FiqEntry) -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SP0_SERR) ASM_PFX(SErrorSP0): b ASM_PFX(SErrorEntry) // // Current EL with SPx: 0x200 - 0x380 // -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_SYNC) ASM_PFX(SynchronousExceptionSPx): b ASM_PFX(SynchronousExceptionEntry) -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_IRQ) ASM_PFX(IrqSPx): b ASM_PFX(IrqEntry) -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_FIQ) ASM_PFX(FiqSPx): b ASM_PFX(FiqEntry) -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_SERR) ASM_PFX(SErrorSPx): b ASM_PFX(SErrorEntry) // // Lower EL using AArch64 : 0x400 - 0x580 // -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_LOW_A64_SYNC) ASM_PFX(SynchronousExceptionA64): b ASM_PFX(SynchronousExceptionEntry) -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_LOW_A64_IRQ) ASM_PFX(IrqA64): b ASM_PFX(IrqEntry) -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_LOW_A64_FIQ) ASM_PFX(FiqA64): b ASM_PFX(FiqEntry) -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_LOW_A64_SERR) ASM_PFX(SErrorA64): b ASM_PFX(SErrorEntry) // // Lower EL using AArch32 : 0x0 - 0x180 // -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_LOW_A32_SYNC) ASM_PFX(SynchronousExceptionA32): b ASM_PFX(SynchronousExceptionEntry) -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_LOW_A32_IRQ) ASM_PFX(IrqA32): b ASM_PFX(IrqEntry) -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_LOW_A32_FIQ) ASM_PFX(FiqA32): b ASM_PFX(FiqEntry) -.align 7 +VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_LOW_A32_SERR) ASM_PFX(SErrorA32): b ASM_PFX(SErrorEntry) +VECTOR_END(ExceptionHandlersStart) #undef REG_PAIR #undef REG_ONE diff --git a/ArmPkg/Include/Chipset/AArch64.h b/ArmPkg/Include/Chipset/AArch64.h index 47993ec..5b6496f 100644 --- a/ArmPkg/Include/Chipset/AArch64.h +++ b/ArmPkg/Include/Chipset/AArch64.h @@ -97,6 +97,38 @@ #define ARM_VECTOR_TABLE_ALIGNMENT ((1 << 11)-1) +// Vector table
[edk2] [PATCH] ArmPkg: correct TCR_EL1 TTBR1_EL1 settings
As EDK2 runs in an idmap, we do not use TTBR1_EL1, nor do we configure
it. TTBR1_EL1 may contain UNKNOWN values if it is not programmed since
reset.
Prior to enabling the MMU, we do not set TCR_EL1.EPD1, and hence the CPU
may make page table walks via TTBR1_EL1 at any time, potentially using
UNKNOWN values. This can result in a number of potential problems (e.g.
the CPU may load from MMIO registers as part of a page table walk).
Additionally, in the presence of Cortex-A57 erratum #87, we must
program TCR_EL1.TG1 == 0b1x (e.g. 4KB granule) regardless of the value
of TCR_EL1.EPD1, to ensure that EDK2 can make forward progress under a
hypervisor which makes use of PAR_EL1.
This patch ensures that we program TCR_EL1.EPD1 and TCR_EL1.TG1 as above
to avoid these issues. TCR_EL1.TG1 is set to 4K for all targets, as any
CPU capable of running EDK2 must support this granule, and given
TCR_EL1.EPD1, programming the field is not detrimental in the absence of
the erratum.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Mark Rutland
Cc: Leif Lindholm
Cc: Ard Biesheuvel
---
ArmPkg/Include/Chipset/AArch64Mmu.h| 2 ++
ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 3 ++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/ArmPkg/Include/Chipset/AArch64Mmu.h
b/ArmPkg/Include/Chipset/AArch64Mmu.h
index 3c3df6d..c0b875c 100644
--- a/ArmPkg/Include/Chipset/AArch64Mmu.h
+++ b/ArmPkg/Include/Chipset/AArch64Mmu.h
@@ -108,6 +108,7 @@
#define TCR_PS_256TB(5 << 16)
#define TCR_TG0_4KB (0 << 14)
+#define TCR_TG1_4KB (2 << 30)
#define TCR_IPS_4GB (0ULL << 32)
#define TCR_IPS_64GB(1ULL << 32)
@@ -116,6 +117,7 @@
#define TCR_IPS_16TB(4ULL << 32)
#define TCR_IPS_256TB (5ULL << 32)
+#define TCR_EPD1 (1 << 23)
#define TTBR_ASID_FIELD (48)
#define TTBR_ASID_MASK (0xFF << TTBR_ASID_FIELD)
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
index 8829c62..12db3ee 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
@@ -667,7 +667,8 @@ ArmConfigureMmu (
return RETURN_UNSUPPORTED;
}
} else if (ArmReadCurrentEL () == AARCH64_EL1) {
-TCR = T0SZ | TCR_TG0_4KB;
+// Due to Cortex-A57 erratum #87 we must set TG1[1] == 1, regardless
of EPD1.
+TCR = T0SZ | TCR_TG0_4KB | TCR_TG1_4KB | TCR_EPD1;
// Set the Physical Address Size using MaxAddress
if (MaxAddress < SIZE_4GB) {
--
1.9.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH] ArmPkg: ensure DebugAgentVectorTable is 2K-aligned
We force alignment to 2K after generating the DebugAgentVectorTable symbol, and hence DebugAgentVectorTable itself may not be 2K-aligned, and table entries may not be at the correct offset from the DebugAgentVectorTable base address. Fix this by forcing alignment before generating the DebugAgentVectorTable symbol. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Mark Rutland Cc: Leif Lindholm Cc: Ard Biesheuvel --- ArmPkg/Library/DebugAgentSymbolsBaseLib/AArch64/DebugAgentException.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ArmPkg/Library/DebugAgentSymbolsBaseLib/AArch64/DebugAgentException.S b/ArmPkg/Library/DebugAgentSymbolsBaseLib/AArch64/DebugAgentException.S index 022e279..3fc090b 100644 --- a/ArmPkg/Library/DebugAgentSymbolsBaseLib/AArch64/DebugAgentException.S +++ b/ArmPkg/Library/DebugAgentSymbolsBaseLib/AArch64/DebugAgentException.S @@ -16,12 +16,12 @@ GCC_ASM_EXPORT(DebugAgentVectorTable) GCC_ASM_IMPORT(DefaultExceptionHandler) .text +.align 11 ASM_PFX(DebugAgentVectorTable): // // Current EL with SP0 : 0x0 - 0x180 // -.align 11 ASM_PFX(SynchronousExceptionSP0): b ASM_PFX(SynchronousExceptionSP0) -- 1.9.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] Armv8 64bit: System error booting linux from the UEFI
> > Is the SError taken directly to EL2? I understood from your previous > > reply that the EDK2 exception handler was invoked at this point, so is > > there anything at EL3 which is trying to catch exceptions and then > > re-inject them down to EL2? > None that I would be aware of. Ok. > > The fact that we're clearly able to take the SError via the table > > confuses me; it makes it sound like the table is fine. > > > > Is that exception taken via the old vector table, or the new one we're > > trying to install with this write to VBAR_EL2? > The exception is taken from the NEW vector table. Interesting! > > > What would you advise? > > > > My hunch is that we have a pending SError that gets "flushed" by an ISB. > > > > You should be able to add ArmInstructionSynchronizationBarrier() calls > > in the general vicinity of InitializeDebugAgent() (e.g. before the call > > to ArmWriteVBar(), in the caller of InitializeDebugAgent(), before it > > and the previous few functions are called). > > > > That may cause the SError to be taken closer to its trigger, and if so > > it would be possible to bisect down to the trigger. > Did that. Regardless of ArmInstructionSycnhronizationBarrier() and subsequent > enabling of the async abort > SError happens right in the ArmWriteVBar. Ok. Two theories: * When we take an exception, SError is masked. So perhaps we take another exception immediately after writing to VBAR_EL2, and that exception handler triggers the SError. I imagine that must be an asynchronous exception (i.e. one we can mask with DAIF). We should be able to see if that's the case if we change the ArmWriteVbar logic for EL2 to something like: mrs x1, daif msr daifset, #(0xb << 6) // D_IF masked mrs vbar_el2, x0 isb nop mrs daif, x1 nop If that is the case, I'd expect to take the SError immediately after the write back to daif. Ard, do we ever expect to take (non-fatal) synchronous exceptions? * As Ard originally suggested, the VBAR_EL2 address is close to some memory region we shouldn't speculatively fetch from, but for some reason do. Can you take a look at the new VBAR_EL2 value and your memory map, and see if it's close to anything that shouldn't be fetched from? Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] Armv8 64bit: System error booting linux from the UEFI
On Mon, Nov 16, 2015 at 08:16:26PM +0100, Ard Biesheuvel wrote: > On 16 November 2015 at 20:13, Mark Rutland wrote: > > On Mon, Nov 16, 2015 at 08:08:18PM +0100, Ard Biesheuvel wrote: > >> On 16 November 2015 at 19:41, Vladimir Olovyannikov > >> wrote: > >> >> -Original Message- > >> >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > >> >> Sent: Monday, November 16, 2015 10:28 AM > >> >> To: Vladimir Olovyannikov > >> >> Cc: Mark Rutland; edk2-devel@lists.01.org > >> >> Subject: Re: [edk2] Armv8 64bit: System error booting linux from the > >> >> UEFI > >> >> > >> > [...] > >> >> > > >> >> > Async abort occurs in ArmWriteVBar() called by InitializeDebugAgent(), > >> >> DebugAgentSymbolsBaseLib.c. > >> >> > Prior to this call I can easily enable async aborts with no "bad" > >> >> consequences. > >> >> > > >> >> > Here is the exact instruction causing the SError in the > >> >> > ArmWriteVBar(): > >> >> > 2: msr vbar_el2, x0// Set the Address of the EL2 Vector > >> >> > Table in the > >> >> VBAR register > >> >> > >> >> Are you using a release build? If so, you should check whether x0 is > >> >> correctly aligned to 2 KB. The ASSERT() tries to establish that, but > >> >> it is only active in DEBUG builds. > >> >> > >> > Ard, it is a DEBUG build. > >> > >> OK. So that should confirm that x0 is aligned to 2 KB. > >> > >> Perhaps the write to VBAR_EL2 enables the delivery in some way. Could > >> you check the A bit in ESR_EL1 before and after? > > > > Do you mean in DAIF / PSTATE? I'm not sure what ESR_ELx would have to do > > with this. > > > > If I am reading the ARM ARM correctly, ISR_EL1.A will be 1 if an > SError interrupt is pending. Is that gated by DAIF / PSTATE in some > way? Ah. You mentioned ESR_EL1 previously rather than ISR_EL1, hence my confusion. That shouldn't be gated by DAIF / PSTATE, but could be out of date if the core can speculatively execute the read, which I suspect it may be able to (though I haven't looked at the ARM ARM and could be wrong). Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] Armv8 64bit: System error booting linux from the UEFI
On Mon, Nov 16, 2015 at 08:08:18PM +0100, Ard Biesheuvel wrote: > On 16 November 2015 at 19:41, Vladimir Olovyannikov > wrote: > >> -Original Message- > >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > >> Sent: Monday, November 16, 2015 10:28 AM > >> To: Vladimir Olovyannikov > >> Cc: Mark Rutland; edk2-devel@lists.01.org > >> Subject: Re: [edk2] Armv8 64bit: System error booting linux from the UEFI > >> > > [...] > >> > > >> > Async abort occurs in ArmWriteVBar() called by InitializeDebugAgent(), > >> DebugAgentSymbolsBaseLib.c. > >> > Prior to this call I can easily enable async aborts with no "bad" > >> consequences. > >> > > >> > Here is the exact instruction causing the SError in the ArmWriteVBar(): > >> > 2: msr vbar_el2, x0// Set the Address of the EL2 Vector > >> > Table in the > >> VBAR register > >> > >> Are you using a release build? If so, you should check whether x0 is > >> correctly aligned to 2 KB. The ASSERT() tries to establish that, but > >> it is only active in DEBUG builds. > >> > > Ard, it is a DEBUG build. > > OK. So that should confirm that x0 is aligned to 2 KB. > > Perhaps the write to VBAR_EL2 enables the delivery in some way. Could > you check the A bit in ESR_EL1 before and after? Do you mean in DAIF / PSTATE? I'm not sure what ESR_ELx would have to do with this. Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] Armv8 64bit: System error booting linux from the UEFI
On Mon, Nov 16, 2015 at 06:23:20PM +, Vladimir Olovyannikov wrote: > > -Original Message- > > From: Mark Rutland [mailto:mark.rutl...@arm.com] [...] > > What is the earliest point in EDK2 that you have unmasked SError? > > > > Are you doing this in PrePeiCoreEntryPoint.S, or later? > Thanks for the pointers Mark. > > Async abort occurs in ArmWriteVBar() called by InitializeDebugAgent(), > DebugAgentSymbolsBaseLib.c. Interesting. Is the SError taken directly to EL2? I understood from your previous reply that the EDK2 exception handler was invoked at this point, so is there anything at EL3 which is trying to catch exceptions and then re-inject them down to EL2? The fact that we're clearly able to take the SError via the table confuses me; it makes it sound like the table is fine. Is that exception taken via the old vector table, or the new one we're trying to install with this write to VBAR_EL2? > Prior to this call I can easily enable async aborts with no "bad" > consequences. > > Here is the exact instruction causing the SError in the ArmWriteVBar(): > 2: msr vbar_el2, x0// Set the Address of the EL2 Vector Table > in the VBAR register As SError is asynchronous, it's not necessarily the case that the write to VBAR_EL2 is the trigger. I note that in ArmPkg/Library/ArmLib/AArch64/AArch64Support.S, there is an ISB at the end of ArmWriteVBar(). That might happen to "flush" a pending SError on the CPU and cause it to be taken. > Could it mean that I do not have enough privileges in the UEFI for this > operation? There are no traps or enables affecting VBAR_EL2. So long as EDK2 is running at EL2 or higher it should be sufficiently privileged to read or write VBAR_EL2. > What would you advise? My hunch is that we have a pending SError that gets "flushed" by an ISB. You should be able to add ArmInstructionSynchronizationBarrier() calls in the general vicinity of InitializeDebugAgent() (e.g. before the call to ArmWriteVBar(), in the caller of InitializeDebugAgent(), before it and the previous few functions are called). That may cause the SError to be taken closer to its trigger, and if so it would be possible to bisect down to the trigger. Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] Armv8 64bit: System error booting linux from the UEFI
On Fri, Nov 13, 2015 at 10:39:34PM +, Vladimir Olovyannikov wrote: > > > > -Original Message- > > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > > Sent: Friday, November 13, 2015 3:00 AM > > To: Vladimir Olovyannikov > > Cc: edk2-devel@lists.01.org > > Subject: Re: Armv8 64bit: System error booting linux from the UEFI > > > (removed extra text) > > > >> This is a firmware problem, not a kernel problem. The kernel is > > >> entered with an pending asynchronous external abort, which may have > > >> several causes. You need to instrument the firmware (i.e., unmask the > > >> aborts earlier and run in the debugger) to get a feeling for what is > > >> going on there. It could be something like speculative accesses to > > >> unassigned physical ranges that are mapped cacheable inadvertently, > > >> but it could be lots of other things as well. > > > > > > > > > > FYI http://article.gmane.org/gmane.comp.bios.edk2.devel/4246 > > > > This fixes a bug that could potentially result in speculative reads to > > device regions. We have also made some other changes regarding > > cacheability and shareability recently (and the patch above will > > likely go in today) so it is probably worthwhile to make sure you are > > based on the latest upstream. > > Thank you Ard, I got the latest upstream. Same issue... It is not related to > the cache. > > I investigated and found that as soon as I do > msr DAIFClr, #4 (or call ArmEnableAsynchronousAbort()) anywhere in the UEFI, > I immediately get this exception. This implies that either code very early in EDK2, or code prior to EDK2 is the problem, given that when masked, SError will pend until it is unmasked (whereupon it should be taken). What is the earliest point in EDK2 that you have unmasked SError? Are you doing this in PrePeiCoreEntryPoint.S, or later? > The boot sequence is BL2->BL3.1->UEFI (plus grub.efi app)->Linux I take it per the naming that you are running ARM Trusted Firmware. Are you able to unmask SError during BL2 or BL3.1, and if so, does it fire prior to entering EDK2? [...] > UEFI is running in EL2. > Maybe I am missing some initialization in the UEFI and that is the reason any > msr DAIFClr,#4 causes this SError exception? It is possible that some initialisation is missing at an early stage in the boot sequence, and that this contributes ot the SError. As an asynchronous exception, SError pends until it is unmasked, and should trigger as soon as it is unmasked. The DAIFClr itself is vanishingly unlikely to be the root cause of the SError, but rather makes it apparent. The earlier you are able to unmask SError, the closer you will be able to get to the root cause. Are you able to capture it were it earlier in the boot sequence (e.g. can you register a handler earlier, or capture the exception with a hardware debugger)? Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg/ArmLib: mark all cached mappings as (inner) shareable
On Thu, Nov 12, 2015 at 11:35:28AM +, Leif Lindholm wrote:
> Hi Ard,
>
> On Mon, Nov 09, 2015 at 02:18:58PM +0100, Ard Biesheuvel wrote:
> > Mark all cached memory mappings as shareable (or inner shareable on
> > AArch64) so that our view of memory is kept coherent by the hardware.
> >
> > This is primarily relevant under virtualization (where a guest may migrate
> > to another core) but in general, since UEFI on ARM is mostly used in a
> > context where the secure firmware and possibly a secure OS are already up
> > and running, it is best to refrain from using any non-shareable mappings.
>
> Thanks, this is both an important correctness fix and nice code
> cleanup.
>
> I ran into an issue while testing this, which is why I haven't
> responded to this, but I've bisected it down to r18503/3a05b13, and am
> looking into what causes an issue with Linux booting.
FWIW, I had issues with that which Ard worked around for virtual
machines in r18533/2f71ad11d6eaa2af ("ArmVirtPkg: reduce preallocation
of boot services data pages").
You may be suffering a similar issue.
Thanks,
Mark.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] Filesystem issues since "OvmfPkg: enable SATA controller"
I've been trying to test an EFI application on x86_64 using Ubuntu 14.04's QEMU 2.0.0. I have a directory 'foo' containing the application, and I get QEMU to create a virtual FAT device: $ qemu-system-x86_64 -nographic \ -bios src/edk2/Build/OvmfX64/RELEASE_GCC48/FV/OVMF.fd \ -hda fat:foo However, I'm unable to access the root filesystem: UEFI Interactive Shell v2.1 EDK II UEFI v2.50 (EDK II, 0x0001) Mapping table FS0: Alias(s):HD7a1:;BLK3: PciRoot(0x0)/Pci(0x1,0x1)/Ata(0x0)/HD(1,MBR,0xBE1AFDFA,0x3F,0xFBFC1) BLK2: Alias(s): PciRoot(0x0)/Pci(0x1,0x1)/Ata(0x0) BLK4: Alias(s): PciRoot(0x0)/Pci(0x1,0x1)/Ata(0x0) BLK0: Alias(s): PciRoot(0x0)/Pci(0x1,0x0)/Floppy(0x0) BLK1: Alias(s): PciRoot(0x0)/Pci(0x1,0x0)/Floppy(0x1) Press ESC in 1 seconds to skip startup.nsh or any other key to continue. Shell> fs0: FS0:\> dir ls: File Not Found - 'FS0:\' With git, I bisected this down to the commit "OvmfPkg: enable SATA controller". Any ideas as to what the problem could be? I don't have a newer qemu-system-x86_64 lying around, so I haven't been able to test with that. Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 10/10] ArmPkg/ArmDmaLib: use the cache writeback granularity for alignment
On Fri, Nov 06, 2015 at 03:03:58PM +0100, Ard Biesheuvel wrote: > When allocating memory to perform non-coherent DMA, use the cache > writeback granularity rather than the data cache linesize for > alignment. This prevents the explicit cache management from corrupting > unrelated adjacent data if the cache writeback granularity exceeds > the cache linesize. > > Reported-by: Mark Rutland > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > index 12b194046ae7..7e352e665a30 100755 > --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > @@ -277,7 +277,7 @@ ArmDmaLibConstructor ( >Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID > **)&gCpu); >ASSERT_EFI_ERROR(Status); > > - gCacheAlignment = ArmDataCacheLineLength (); > + gCacheAlignment = ArmCacheWritebackGranularity (); I assume that per my comment on the previous patch, the function name might change here. Regardless, this makes the logic look correct to me: Reviewed-by: Mark Rutland Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 09/10] ArmPkg/ArmLib: add accessor function for Cache Writeback Granularity
On Fri, Nov 06, 2015 at 03:03:57PM +0100, Ard Biesheuvel wrote:
> Add a function to ArmLib that provides access to the Cache Writeback
> Granularity (CWG) field in CTR_EL0. This information is required when
> performing non-coherent DMA.
Nit: s/Granularity/Granule/
Likewise for the patch body.
[...]
> +UINTN
> +EFIAPI
> +ArmCacheWritebackGranularity (
> + VOID
> + )
> +{
> + UINTN CWG;
> +
> + CWG = (ArmCacheInfo () >> 24) & 0xf; // CTR_EL0.CWG
> +
> + if (CWG == 0) {
> +return SIZE_512KB;
> + }
This should be SIZE_2KB, no?
The ARM ARM says the architectural maximum is 512 /words/ (i.e. 2KB).
With those changes:
Reviewed-by: Mark Rutland
Thanks,
Mark.
> +
> + return 4 << CWG;
> +}
> --
> 1.9.1
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 08/10] ArmCacheMaintenanceLib: disallow whole D-cache maintenance operations
On Wed, Nov 04, 2015 at 04:24:20PM +0100, Laszlo Ersek wrote: > On 11/04/15 16:19, Ard Biesheuvel wrote: [...] > > The problem remains that VA and set/way ops are completely different > > things. Each by-VA operation handles all copies of the same cacheline > > throughout the cache hierarchy at once. The set/way ops, on the other > > hand, traverse each cache level in turn, leaving a time window between > > the completion of the L1 maintenance and the completion of the L2 > > maintenance where speculative accesses resulting in L1 misses are > > happily served from the L2 and allocated in L1 -- unless your MMU and > > I-cache are off. This means the logic that promotes from VA to set/way > > ops should take MMU and I-cache state into account as well, and I am > > not sure you would want to go there. > > Thanks for this great summary. > > So, why doesn't ARM provide instructions for flushing / invalidating the > complete cache? Just curious. I don't have a an official answer, but there are several factors that spring to mind. Because of system-level coherency, dirty cacheline migration, and so on, performing an operation on the "complete cache" would mean performing the operation on every cache in the system (e.g. CPUs, system L3, coherent devices, etc). That's an enormous amount of data and associated state to keep track of, and it could take an extremely long time for such maintenance to complete. It's almost never necessary to clean or invalidate the entire cache hierarchy in a system. In practically every scenario, maintenance by VA can be used to provide the necessary guarantees, so the complexity of implementing the above can be avoided. Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 08/10] ArmCacheMaintenanceLib: disallow whole D-cache maintenance operations
On Wed, Nov 04, 2015 at 12:17:16PM +, Cohen, Eugene wrote: > > The set/way operations are really only suitable for managing the caches > > themselves > > This makes sense to me and I agree that the majority of developers > should only be dealing with managing buffers and should only use the > VA/address-range interface. > > > there are examples of architecturally compliant systems with system > > level caches where cleaned data has been observed to only make it to > > main memory if you clean by VA. > > I fully expect this given the nature of the ARM architecture - the > scope architectural specifications stop at the interconnect so if > somebody wants to implement wacky hardware then there's nothing to > stop them. It's worth noting that the (ARMv8) architecture defines some properties of system-level caches (e.g. that they must respect maintenance by VA), which ensures that the architected mechanism for managing caches is portable (i.e. maintenance by VA works everywhere). If someone builds a system cache that does not respect maintenance by VA then they have violated the (ARMv8) architecture. Personally, I expect that system-level caches are likely to become the common case in server systems, assuming that there are coherent masters other than CPUs in the system. I don't think we can consider them a "wacky" niche case. > Presumably for whole-cache management some extra stuff is required on > these systems to make it work - which is fine although not that > helpful for interoperability purposes. > > > But seriously, I understand that there is a performance concern > > here, but 'promoting' a clean by VA operation to clean by set/way > > really makes no sense at all. > > Defaulting to safe/correct for all architectures makes sense to me. > But it would be nice for those of us that understand our architectures > (no system level caches, SMP config understood) to choose an option to > turn on thresholding through a PCD setting or library class selection. I am not necessarily opposed to this, however I am a little concerned that as with other performance options, this is something people may turn on blindly, regardless of whether such maintenance can actually provide useful guarantees for their system. Do we actually see a measurable performance advantage to enabling such thresholding on systems where it is safe to do so? What saving do we see, and in what paths? It's also worth noting that a Set/Way sequence may not be sufficient on all platforms even in the absence of system-level caches, and other IMPLEMENTATION DEFINED sequences may be necessary. In the general case. supporting thresholding would need the platform code to provide its particular mechanism. Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 09/10] ArmVirtPkg/PrePi: do not invalidate the entire data cache at startup
On Tue, Nov 03, 2015 at 11:16:34AM +0100, Ard Biesheuvel wrote: > Drop the call to ArmInvalidateDataCache () from the PrePi startup > sequence. This kind of data cache maintenance should not be performed > at the UEFI firmware level. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > ArmVirtPkg/PrePi/PrePi.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c > index f9ad37427217..fe7612cec74b 100755 > --- a/ArmVirtPkg/PrePi/PrePi.c > +++ b/ArmVirtPkg/PrePi/PrePi.c > @@ -194,8 +194,6 @@ CEntryPoint ( > >// Data Cache enabled on Primary core when MMU is enabled. >ArmDisableDataCache (); > - // Invalidate Data cache > - ArmInvalidateDataCache (); >// Invalidate instruction cache >ArmInvalidateInstructionCache (); >// Enable Instruction Caches on all cores. I'm not sure the I-cache maintenance is necessary either, given that we're already executing code that must have been fetched into the I-cache, but that really depends on what EDK's requirements are w.r.t. the state of the system. For this patch as is: Acked-by: Mark Rutland Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 08/10] ArmCacheMaintenanceLib: disallow whole D-cache maintenance operations
On Tue, Nov 03, 2015 at 01:51:46PM +, Cohen, Eugene wrote:
> Please don't remove this functionality. At times we do want to use
> this library to turn off the cache in preparation for going to another
> environment (say, loading an OS) and this is useful.
Could you elaborate on your use-case?
Given the unreliability of Set/Way operations, they may not achieve the
guarantees that you require. They are not guaranted to push data to the
PoC (i.e. memory), cannot drain system caches (e.g. external L3s), etc.
Mark.
> Eugene
>
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard
> Biesheuvel
> Sent: Tuesday, November 03, 2015 3:17 AM
> To: edk2-devel@lists.01.org; leif.lindh...@linaro.org; mark.rutl...@arm.com
> Cc: ler...@redhat.com; Ard Biesheuvel
> Subject: [edk2] [PATCH 08/10] ArmCacheMaintenanceLib: disallow whole D-cache
> maintenance operations
>
> The ARM architecture provides no reliable way to clean or invalidate the
> entire data cache at runtime. The reason is that such maintenance requires
> the use of set/way maintenance operations, which are suitable only for the
> kind of maintenance that is carried out when the cache is taken offline
> entirely.
>
> So ASSERT () when any of the CacheMaintenanceLib whole data cache routines
> are invoked rather than pretending we can do anything meaningful here.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel
> ---
> ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> index 175d29496c32..8cc1990b3266 100644
> --- a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> +++ b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> @@ -14,6 +14,7 @@
> **/
> #include
> #include
> +#include
> #include
>
> VOID
> @@ -44,7 +45,6 @@ InvalidateInstructionCache (
>VOID
>)
> {
> - ArmCleanDataCache();
>ArmInvalidateInstructionCache();
> }
>
> @@ -54,7 +54,7 @@ InvalidateDataCache (
>VOID
>)
> {
> - ArmInvalidateDataCache();
> + ASSERT (FALSE);
> }
>
> VOID *
> @@ -76,7 +76,7 @@ WriteBackInvalidateDataCache (
>VOID
>)
> {
> - ArmCleanInvalidateDataCache();
> + ASSERT (FALSE);
> }
>
> VOID *
> @@ -96,7 +96,7 @@ WriteBackDataCache (
>VOID
>)
> {
> - ArmCleanDataCache();
> + ASSERT (FALSE);
> }
>
> VOID *
> --
> 1.9.1
>
> ___
> 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
Re: [edk2] [PATCH 07/10] ArmPkg/ArmLib: move cache maintenance sync barriers out of loop
On Tue, Nov 03, 2015 at 11:16:32AM +0100, Ard Biesheuvel wrote:
> There is no need to issue a full data synchronization barrier and an
> instruction synchronization barrier after each and every set/way or
> MVA cache maintenance operation. For the set/way case, we can simply
> remove them, since the set/way outer loop already issues the required
> barriers after completing its traversal over all the cache levels.
>
> For the MVA case, move the data synchronization barrier out of the
> loop, and add the instruction synchronization barrier to the I-cache
> invalidation by MVA routine.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel
> ---
> ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c | 2 ++
> ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 12
>
> ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S | 12
>
> ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm | 12
>
> 4 files changed, 2 insertions(+), 36 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> index d8e53df6096e..175d29496c32 100644
> --- a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> +++ b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
> @@ -35,6 +35,7 @@ CacheRangeOperation (
> LineOperation(AlignedAddress);
> AlignedAddress += ArmCacheLineLength;
>}
> + ArmDataSynchronizationBarrier ();
> }
>
> VOID
> @@ -65,6 +66,7 @@ InvalidateInstructionCacheRange (
> {
>CacheRangeOperation (Address, Length, ArmCleanDataCacheEntryByMVA);
>ArmInvalidateInstructionCache ();
> + ArmInstructionSynchronizationBarrier ();
>return Address;
> }
I've just spotted that the ArmInvalidateInstructionCache implementations
already have the requisite DSB; ISB sequence, so we didn't actually need
to introduce an ISB here.
I don't know if it's best to remove the ISB here, or move both the DSB
and ISB here for consistency across all the cache maintenance
primitives.
Either way, with that fixed up:
Reviewed-by: Mark Rutland
Mark.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 06/10] ArmPkg/ArmLib: retrieve cache line length from CTR not CCSIDR
Hi,
On Tue, Nov 03, 2015 at 11:16:31AM +0100, Ard Biesheuvel wrote:
> The stride used by the cache maintenance by MVA instructions should
> be retrieved from CTR_EL0.DminLine and CTR_EL0.IminLine, whose values
> reflect the actual geometry of the caches. Using CCSIDR for this purpose
> violates the architecture.
Looking around, I think that ArmDataCacheLineLength/CTR_EL0.DminLine is
also mis-used by DMA code to align/pad data for non-coherent DMA, where
I would expect to see CTR_EL0.CWG (effectively the maximum D-cache line
length) being used instead.
That requires some investigation.
> Also, move the line length accessors to common code, since there is no
> need to keep them separate between ARMv7 and AArch64.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel
Regardless of the above, this is most definitely an improvement.
Reviewed-by: Mark Rutland
Mark.
> ---
> ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c | 25 --
> ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c | 27
> ArmPkg/Library/ArmLib/Common/ArmLib.c | 18 +
> 3 files changed, 18 insertions(+), 52 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> index 4bea20356fa3..dec125f248cd 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> @@ -21,31 +21,6 @@
> #include "AArch64Lib.h"
> #include "ArmLibPrivate.h"
>
> -UINTN
> -EFIAPI
> -ArmDataCacheLineLength (
> - VOID
> - )
> -{
> - UINT32 CCSIDR = ReadCCSIDR (0) & 7;
> -
> - // * 4 converts to bytes
> - return (1 << (CCSIDR + 2)) * 4;
> -}
> -
> -UINTN
> -EFIAPI
> -ArmInstructionCacheLineLength (
> - VOID
> - )
> -{
> - UINT32 CCSIDR = ReadCCSIDR (1) & 7;
> -
> - // * 4 converts to bytes
> - return (1 << (CCSIDR + 2)) * 4;
> -}
> -
> -
> VOID
> AArch64DataCacheOperation (
>IN AARCH64_CACHE_OPERATION DataCacheOperation
> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
> index 44edff869eae..b53f455bfad2 100644
> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
> @@ -20,33 +20,6 @@
> #include "ArmV7Lib.h"
> #include "ArmLibPrivate.h"
>
> -UINTN
> -EFIAPI
> -ArmDataCacheLineLength (
> - VOID
> - )
> -{
> - UINT32 CCSIDR = ReadCCSIDR (0) & 7;
> -
> - // * 4 converts to bytes
> - return (1 << (CCSIDR + 2)) * 4;
> -}
> -
> -UINTN
> -EFIAPI
> -ArmInstructionCacheLineLength (
> - VOID
> - )
> -{
> - UINT32 CCSIDR = ReadCCSIDR (1) & 7;
> -
> - // * 4 converts to bytes
> - return (1 << (CCSIDR + 2)) * 4;
> -
> -// return 64;
> -}
> -
> -
> VOID
> ArmV7DataCacheOperation (
>IN ARM_V7_CACHE_OPERATION DataCacheOperation
> diff --git a/ArmPkg/Library/ArmLib/Common/ArmLib.c
> b/ArmPkg/Library/ArmLib/Common/ArmLib.c
> index 4febc45220a3..ad0a265e9f59 100644
> --- a/ArmPkg/Library/ArmLib/Common/ArmLib.c
> +++ b/ArmPkg/Library/ArmLib/Common/ArmLib.c
> @@ -70,3 +70,21 @@ ArmUnsetCpuActlrBit (
>Value &= ~Bits;
>ArmWriteCpuActlr (Value);
> }
> +
> +UINTN
> +EFIAPI
> +ArmDataCacheLineLength (
> + VOID
> + )
> +{
> + return 4 << ((ArmCacheInfo () >> 16) & 0xf); // CTR_EL0.DminLine
> +}
> +
> +UINTN
> +EFIAPI
> +ArmInstructionCacheLineLength (
> + VOID
> + )
> +{
> + return 4 << (ArmCacheInfo () & 0xf); // CTR_EL0.IminLine
> +}
> --
> 1.9.1
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 04/10] ArmPkg/ArmLib: remove unused ArmCleanDataCacheToPoU()
On Tue, Nov 03, 2015 at 11:16:29AM +0100, Ard Biesheuvel wrote:
> The function ArmCleanDataCacheToPoU() has no users, and its purpose
> is unclear, since it uses cache maintenance by set/way to perform
> the clean to PoU, which is a dubious practice to begin with. So
> remove the declaration and all definitions.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel
Acked-by: Mark Rutland
Mark.
> ---
> ArmPkg/Include/Library/ArmLib.h| 6 ---
> ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c | 30
> ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h | 6 ---
> ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 14 --
> ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c | 30
> ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.h | 6 ---
> ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S | 50
> ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm | 50
> 8 files changed, 192 deletions(-)
>
> diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
> index 58116663b28d..f1de303d952d 100644
> --- a/ArmPkg/Include/Library/ArmLib.h
> +++ b/ArmPkg/Include/Library/ArmLib.h
> @@ -243,12 +243,6 @@ ArmCleanDataCache (
>
> VOID
> EFIAPI
> -ArmCleanDataCacheToPoU (
> - VOID
> - );
> -
> -VOID
> -EFIAPI
> ArmInvalidateInstructionCache (
>VOID
>);
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> index a4e1f20ad910..f795a2f896b2 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> @@ -206,26 +206,6 @@ AArch64DataCacheOperation (
>}
> }
>
> -
> -VOID
> -AArch64PoUDataCacheOperation (
> - IN AARCH64_CACHE_OPERATION DataCacheOperation
> - )
> -{
> - UINTN SavedInterruptState;
> -
> - SavedInterruptState = ArmGetInterruptState ();
> - ArmDisableInterrupts ();
> -
> - AArch64PerformPoUDataCacheOperation (DataCacheOperation);
> -
> - ArmDrainWriteBuffer ();
> -
> - if (SavedInterruptState) {
> -ArmEnableInterrupts ();
> - }
> -}
> -
> VOID
> EFIAPI
> ArmInvalidateDataCache (
> @@ -255,13 +235,3 @@ ArmCleanDataCache (
>ArmDrainWriteBuffer ();
>AArch64DataCacheOperation (ArmCleanDataCacheEntryBySetWay);
> }
> -
> -VOID
> -EFIAPI
> -ArmCleanDataCacheToPoU (
> - VOID
> - )
> -{
> - ArmDrainWriteBuffer ();
> - AArch64PoUDataCacheOperation (ArmCleanDataCacheEntryBySetWay);
> -}
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
> b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
> index c8bb84365bb6..7b9b9c371531 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
> @@ -18,12 +18,6 @@
>
> typedef VOID (*AARCH64_CACHE_OPERATION)(UINTN);
>
> -
> -VOID
> -AArch64PerformPoUDataCacheOperation (
> - IN AARCH64_CACHE_OPERATION DataCacheOperation
> - );
> -
> VOID
> AArch64AllDataCachesOperation (
>IN AARCH64_CACHE_OPERATION DataCacheOperation
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> index 8b5e0fb6e7fe..f973a35c21d6 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> @@ -40,7 +40,6 @@ GCC_ASM_EXPORT (ArmEnableAlignmentCheck)
> GCC_ASM_EXPORT (ArmEnableBranchPrediction)
> GCC_ASM_EXPORT (ArmDisableBranchPrediction)
> GCC_ASM_EXPORT (AArch64AllDataCachesOperation)
> -GCC_ASM_EXPORT (AArch64PerformPoUDataCacheOperation)
> GCC_ASM_EXPORT (ArmDataMemoryBarrier)
> GCC_ASM_EXPORT (ArmDataSynchronizationBarrier)
> GCC_ASM_EXPORT (ArmInstructionSynchronizationBarrier)
> @@ -324,19 +323,6 @@ ASM_PFX(AArch64AllDataCachesOperation):
> // right to ease the access to CSSELR and
> the Set/Way operation.
>cbz x3, L_Finished // No need to clean if LoC is 0
>mov x10, #0 // Start clean at cache level 0
> - b Loop1
> -
> -ASM_PFX(AArch64PerformPoUDataCacheOperation):
> -// We can use regs 0-7 and 9-15 without having to save/restore.
> -// Save our link register on the stack. - The stack must always be quad-word
> aligned
> - str x30, [sp, #-16]!
> - mov x1, x0 // Save Function call in x1
> - mrs x6, clidr_el1 // Read EL1 CLIDR
> - and x3, x6, #0x3800 // Mask out all but Point of Unification
> (PoU)
> - lsr x3, x3, #26 // Left align cache level value - the level
> is shifted by 1 to the
> -
Re: [edk2] [PATCH 02/10] ArmPkg BeagleBoardPkg Omap35xxPkg: fix typo 'ArmDataSyncronizationBarrier'
On Tue, Nov 03, 2015 at 12:29:34PM +0100, Ard Biesheuvel wrote: > On 3 November 2015 at 12:28, Mark Rutland wrote: > > On Tue, Nov 03, 2015 at 11:16:27AM +0100, Ard Biesheuvel wrote: > >> Replace all instances of ArmDataSyncronizationBarrier with > >> ArmDataSynchronizationBarrier. > > > > There are a few instances left in the ARM9 support: > > > > ArmPkg/Library/ArmLib/Arm9/Arm9Support.S:GCC_ASM_EXPORT(ArmDataSyncronizationBarrier) > > ArmPkg/Library/ArmLib/Arm9/Arm9Support.S:ASM_PFX(ArmDataSyncronizationBarrier): > > ArmPkg/Library/ArmLib/Arm9/Arm9Support.asm:EXPORT > > ArmDataSyncronizationBarrier > > ArmPkg/Library/ArmLib/Arm9/Arm9Support.asm:ASM_PFX(ArmDataSyncronizationBarrier): > > > > Any reason to not fix those up? > > > > There wasn't until I reshuffled the patches :-) Ah, now I see ;) > > Otherwise this looks sensible to me. Acked-by: Mark Rutland Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 02/10] ArmPkg BeagleBoardPkg Omap35xxPkg: fix typo 'ArmDataSyncronizationBarrier'
On Tue, Nov 03, 2015 at 11:16:27AM +0100, Ard Biesheuvel wrote: > Replace all instances of ArmDataSyncronizationBarrier with > ArmDataSynchronizationBarrier. There are a few instances left in the ARM9 support: ArmPkg/Library/ArmLib/Arm9/Arm9Support.S:GCC_ASM_EXPORT(ArmDataSyncronizationBarrier) ArmPkg/Library/ArmLib/Arm9/Arm9Support.S:ASM_PFX(ArmDataSyncronizationBarrier): ArmPkg/Library/ArmLib/Arm9/Arm9Support.asm:EXPORT ArmDataSyncronizationBarrier ArmPkg/Library/ArmLib/Arm9/Arm9Support.asm:ASM_PFX(ArmDataSyncronizationBarrier): Any reason to not fix those up? Otherwise this looks sensible to me. Mark. > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > ArmPkg/Include/Library/ArmLib.h | 2 +- > ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 4 ++-- > ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S | 4 ++-- > ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm| 4 ++-- > BeagleBoardPkg/Library/BeagleBoardLib/BeagleBoard.c | 4 ++-- > Omap35xxPkg/InterruptDxe/HardwareInterrupt.c| 6 +++--- > Omap35xxPkg/Library/DebugAgentTimerLib/DebugAgentTimerLib.c | 2 +- > 7 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h > index b4768841bd9d..58116663b28d 100644 > --- a/ArmPkg/Include/Library/ArmLib.h > +++ b/ArmPkg/Include/Library/ArmLib.h > @@ -483,7 +483,7 @@ ArmDataMemoryBarrier ( > > VOID > EFIAPI > -ArmDataSyncronizationBarrier ( > +ArmDataSynchronizationBarrier ( >VOID >); > > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > index 28cf27fbd1b6..8b5e0fb6e7fe 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > @@ -42,7 +42,7 @@ GCC_ASM_EXPORT (ArmDisableBranchPrediction) > GCC_ASM_EXPORT (AArch64AllDataCachesOperation) > GCC_ASM_EXPORT (AArch64PerformPoUDataCacheOperation) > GCC_ASM_EXPORT (ArmDataMemoryBarrier) > -GCC_ASM_EXPORT (ArmDataSyncronizationBarrier) > +GCC_ASM_EXPORT (ArmDataSynchronizationBarrier) > GCC_ASM_EXPORT (ArmInstructionSynchronizationBarrier) > GCC_ASM_EXPORT (ArmWriteVBar) > GCC_ASM_EXPORT (ArmReadVBar) > @@ -389,7 +389,7 @@ ASM_PFX(ArmDataMemoryBarrier): >ret > > > -ASM_PFX(ArmDataSyncronizationBarrier): > +ASM_PFX(ArmDataSynchronizationBarrier): > ASM_PFX(ArmDrainWriteBuffer): >dsb sy >ret > diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S > b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S > index af5ec23a1a4f..f59cd5f32e6b 100644 > --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S > +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S > @@ -40,7 +40,7 @@ GCC_ASM_EXPORT (ArmSetHighVectors) > GCC_ASM_EXPORT (ArmV7AllDataCachesOperation) > GCC_ASM_EXPORT (ArmV7PerformPoUDataCacheOperation) > GCC_ASM_EXPORT (ArmDataMemoryBarrier) > -GCC_ASM_EXPORT (ArmDataSyncronizationBarrier) > +GCC_ASM_EXPORT (ArmDataSynchronizationBarrier) > GCC_ASM_EXPORT (ArmInstructionSynchronizationBarrier) > GCC_ASM_EXPORT (ArmReadVBar) > GCC_ASM_EXPORT (ArmWriteVBar) > @@ -321,7 +321,7 @@ ASM_PFX(ArmDataMemoryBarrier): >dmb >bx LR > > -ASM_PFX(ArmDataSyncronizationBarrier): > +ASM_PFX(ArmDataSynchronizationBarrier): > ASM_PFX(ArmDrainWriteBuffer): >dsb >bx LR > diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm > b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm > index 2b13811dc6cf..07ff1ae15a6a 100644 > --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm > +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm > @@ -37,7 +37,7 @@ > EXPORT ArmV7AllDataCachesOperation > EXPORT ArmV7PerformPoUDataCacheOperation > EXPORT ArmDataMemoryBarrier > -EXPORT ArmDataSyncronizationBarrier > +EXPORT ArmDataSynchronizationBarrier > EXPORT ArmInstructionSynchronizationBarrier > EXPORT ArmReadVBar > EXPORT ArmWriteVBar > @@ -315,7 +315,7 @@ ArmDataMemoryBarrier >dmb >bx LR > > -ArmDataSyncronizationBarrier > +ArmDataSynchronizationBarrier > ArmDrainWriteBuffer >dsb >bx LR > diff --git a/BeagleBoardPkg/Library/BeagleBoardLib/BeagleBoard.c > b/BeagleBoardPkg/Library/BeagleBoardLib/BeagleBoard.c > index dbbe68a7a08e..3b0244004853 100755 > --- a/BeagleBoardPkg/Library/BeagleBoardLib/BeagleBoard.c > +++ b/BeagleBoardPkg/Library/BeagleBoardLib/BeagleBoard.c > @@ -92,11 +92,11 @@ ArmPlatformInitialize ( > >// Turn off the functional clock for Timer 3 >MmioAnd32 (CM_FCLKEN_PER, 0x ^ CM_ICLKEN_PER_EN_GPT3_ENABLE ); > - ArmDataSyncronizationBarrier (); > + ArmDataSynchronizationBarrier (); > >// Clear IRQs >MmioWrite32 (INTCPS_CONTROL, INTCPS_CONTROL_NEWIRQAGR); > - ArmDataSyncronizationBarrier (); > + ArmDataSynchronizationBarrier (); > >return RETURN_SUCCESS; > } > diff --git a/Omap35xxPkg/Inte
[edk2] [PATCH] ArmVirtPkg: fix barriers in ArmEnableMmu
The ARM architecture requires a DSB to complete TLB maintenance, with a subsequent ISB being required to synchronize subsequent items in the current instruction stream against the completed TLB maintenance. The ArmEnableMmu function is currently missing the DSB, and hence the TLB maintenance is not guaranteed to have completed at the point the MMU is enabled. This may result in unpredictable behaviour. The DSB subsequent to the write to SCTLR_EL1 is unnecessary; the ISB alone is sufficient to complete all prior instructions and to synchronise the new context with any subsequent instructions. This patch adds missing DSBs to complete TLB maintenance, and removes the unnecessary trailing DSB. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Mark Rutland Cc: Ard Biesheuvel Cc: Laszlo Ersek Cc: Leif Lindholm --- ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S index bdede48..28cf27f 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S @@ -123,18 +123,20 @@ ASM_PFX(ArmEnableMmu): 4: orr x0, x0, #CTRL_M_BIT // Set MMU enable bit EL1_OR_EL2_OR_EL3(x1) 1: tlbivmalle1 + dsb nsh isb msr sctlr_el1, x0 // Write back b 4f 2: tlbialle2 + dsb nsh isb msr sctlr_el2, x0 // Write back b 4f 3: tlbialle3 + dsb nsh isb msr sctlr_el3, x0 // Write back -4: dsb sy - isb +4: isb ret -- 1.9.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg/ArmLib: remove pointless sync barriers after each cache op
Hi Ard, On Mon, Nov 02, 2015 at 02:24:18PM +0100, Ard Biesheuvel wrote: > There is no need to issue a full data synchronization barrier and an > instruction synchronization barrier after each and every set/way or > MVA cache maintenance operation. So remove them. So long as the loops calling these have the relevant trailing barriers, these are indeed unnecessary and only serve to waste time. In ArmCacheMaintenanceLib, CacheRangeOperation is missing the requisite DSB, and InvalidateInstructionCacheRange is missing the requisite ISB. I think those need to be added too. I think the Set/Way ops need to be ripped out entirely, they should really only be used by low-level CPU power management code. They're not portable and don't guarantee data is pushed to the PoU or PoC. Thanks, Mark. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 12 > 1 file changed, 12 deletions(-) > > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > index bdede48724e6..aaccd38dc998 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > @@ -66,43 +66,31 @@ GCC_ASM_EXPORT (ArmReadCurrentEL) > > ASM_PFX(ArmInvalidateDataCacheEntryByMVA): >dc ivac, x0// Invalidate single data cache line > - dsb sy > - isb >ret > > > ASM_PFX(ArmCleanDataCacheEntryByMVA): >dc cvac, x0// Clean single data cache line > - dsb sy > - isb >ret > > > ASM_PFX(ArmCleanInvalidateDataCacheEntryByMVA): >dc civac, x0 // Clean and invalidate single data cache line > - dsb sy > - isb >ret > > > ASM_PFX(ArmInvalidateDataCacheEntryBySetWay): >dc isw, x0 // Invalidate this line > - dsb sy > - isb >ret > > > ASM_PFX(ArmCleanInvalidateDataCacheEntryBySetWay): >dc cisw, x0// Clean and Invalidate this line > - dsb sy > - isb >ret > > > ASM_PFX(ArmCleanDataCacheEntryBySetWay): >dc csw, x0 // Clean this line > - dsb sy > - isb >ret > > > -- > 1.9.1 > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCHv2] ArmVirtPkg: include BaseStackCheckLib also for AARCH64
Some AArch64 toolchains also invoke the software stack checker functions on certain code - so include BaseStackCheckLib for AARCH64 as well as for ARM. Since this was the only difference between [LibraryClasses.ARM] and [LibraryClasses.AARCH64], merge the two into the existing [LibraryClasses.common]. At the same time, fix the grammar for the related comments. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Mark Rutland Cc: Ard Biesheuvel Cc: Laszlo Ersek Cc: Leif Lindholm --- ArmVirtPkg/ArmVirt.dsc.inc | 24 +--- 1 file changed, 9 insertions(+), 15 deletions(-) Since v1: * Fold into existing [LibraryClasses.common] section. * Correct grammar. * Drop redundant comment line. diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc index b75499f..8626919 100644 --- a/ArmVirtPkg/ArmVirt.dsc.inc +++ b/ArmVirtPkg/ArmVirt.dsc.inc @@ -70,6 +70,15 @@ UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf + # + # It is not possible to prevent the ARM compiler from inserting calls to intrinsic functions. + # This library provides the instrinsic functions such a compiler may generate calls to. + # + NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf + + # Add support for GCC stack protector + NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf + # ARM Architectural Libraries CacheMaintenanceLib|ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.inf DefaultExceptionHandlerLib|ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf @@ -229,21 +238,6 @@ BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf !endif -[LibraryClasses.ARM] - # - # It is not possible to prevent the ARM compiler for generic intrinsic functions. - # This library provides the instrinsic functions generate by a given compiler. - # [LibraryClasses.ARM] and NULL mean link this library into all ARM images. - # - NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf - - # Add support for GCC stack protector - NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf - -[LibraryClasses.AARCH64] - NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf - - [BuildOptions] RVCT:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG -- 1.9.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] [PATCH] ArmVirtPkg: include BaseStackCheckLib also for AARCH64
On Fri, Oct 16, 2015 at 12:48:37PM +0200, Laszlo Ersek wrote: > On 10/16/15 12:15, Mark Rutland wrote: > > Some AArch64 toolchains also invoke the software stack checker > > functions on certain code - so include BaseStackCheckLib for > > AARCH64 as well as for ARM. Since this was the only difference > > between [LibraryClasses.ARM] and [LibraryClasses.AARCH64], > > merge the two into a single [LibraryClasses.common]. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Mark Rutland > > Cc: Ard Biesheuvel > > Cc: Laszlo Ersek > > Cc: Leif Lindholm > > --- > > ArmVirtPkg/ArmVirt.dsc.inc | 8 ++-- > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc > > index b75499f..a14d0d3 100644 > > --- a/ArmVirtPkg/ArmVirt.dsc.inc > > +++ b/ArmVirtPkg/ArmVirt.dsc.inc > > @@ -229,21 +229,17 @@ > >BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf > > !endif > > > > -[LibraryClasses.ARM] > > +[LibraryClasses.common] > ># > ># It is not possible to prevent the ARM compiler for generic intrinsic > > functions. > ># This library provides the instrinsic functions generate by a given > > compiler. > > - # [LibraryClasses.ARM] and NULL mean link this library into all ARM > > images. > > + # [LibraryClasses.common] and NULL mean link this library into all > > images. > ># > >NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf > > > ># Add support for GCC stack protector > >NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf > > > > -[LibraryClasses.AARCH64] > > - NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf > > - > > - > > [BuildOptions] > >RVCT:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG > > > > > > I'll leave it to Ard & Leif to judge whether BaseStackCheckLib is > appropriate for AARCH64 modules. > > If it is, then the idea to move it to [LibraryClasses.common] is sane of > course. I was encountering build issues without it, but I'll defer to Ard and Leif's judgement as to whether the above is the correct fix. > One request though: this file already has a section called > [LibraryClasses.common]; can you please move both CompilerIntrinsicsLib > and BaseStackCheckLib up there? Sure. I've tested that locally and I have working release and debug builds. > While at it, can you please fix up the English grammar in the comments? > As in "prevent ... *from inserting*", "*generated* by". Done. Expect a v2 shortly. Thanks, Mark. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH] [PATCH] ArmVirtPkg: include BaseStackCheckLib also for AARCH64
Some AArch64 toolchains also invoke the software stack checker functions on certain code - so include BaseStackCheckLib for AARCH64 as well as for ARM. Since this was the only difference between [LibraryClasses.ARM] and [LibraryClasses.AARCH64], merge the two into a single [LibraryClasses.common]. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Mark Rutland Cc: Ard Biesheuvel Cc: Laszlo Ersek Cc: Leif Lindholm --- ArmVirtPkg/ArmVirt.dsc.inc | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc index b75499f..a14d0d3 100644 --- a/ArmVirtPkg/ArmVirt.dsc.inc +++ b/ArmVirtPkg/ArmVirt.dsc.inc @@ -229,21 +229,17 @@ BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf !endif -[LibraryClasses.ARM] +[LibraryClasses.common] # # It is not possible to prevent the ARM compiler for generic intrinsic functions. # This library provides the instrinsic functions generate by a given compiler. - # [LibraryClasses.ARM] and NULL mean link this library into all ARM images. + # [LibraryClasses.common] and NULL mean link this library into all images. # NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf # Add support for GCC stack protector NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf -[LibraryClasses.AARCH64] - NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf - - [BuildOptions] RVCT:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG -- 1.9.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
