Re: [edk2-devel] [PATCH 3/6] ArmPkg/ArmMmuLib ARM: cache-invalidate initial page table entries

["Leif Lindholm" <leif@nuviainc.com>]

On Mon, Mar 02, 2020 at 14:15:58 +0100, Ard Biesheuvel wrote:
> > > > > @@ -257,7 +259,11 @@ FillTranslationTable (
> > > > >          RemainLength >= TT_DESCRIPTOR_SECTION_SIZE) {
> > > > >        // Case: Physical address aligned on the Section Size (1MB) && the length
> > > > >        // is greater than the Section Size
> > > > > -      *SectionEntry++ = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(PhysicalBase) | Attributes;
> > > > > +      *SectionEntry = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(PhysicalBase) | Attributes;
> > > > > +
> > > > > +      ArmDataSynchronizationBarrier ();
> > > > > +      ArmInvalidateDataCacheEntryByMVA ((UINTN)SectionEntry++);
> > > > > +
> > > >
> > > > Since the sequence is somewhat conterintuitive, could we add a comment
> > > > to the extent that // Force subsequent acces to fetch from main memory?
> > >
> > > The barrier is there to ensure that the write made it to meain memory,
> > > so we could actually relax this to a DMB.
> >
> > If there's no risk there could be a stale entry for that line (i.e.,
> > D-cache has not been enabled since reset). Otherwise, I *think* there
> > could be a potential race condition in v7.
> 
> I don't follow. Getting rid of stale, clean cachelines is the whole
> point. But to ensure that a speculative fetch doesn't fetch the same
> stale value again, we need to order the invalidate wrt the store.

What I'm getting at is that *some* v7 platforms don't use TF-A, and
some of those could have run with caches enabled before ever going
into EDK2.

Since v7 architecture says:
---
When a cache is disabled for a type of access, for a particular
translation regime:
- it is IMPLEMENTATION DEFINED whether a cache hit occurs if a location
  that is held in the cache is accessed by that type of access.
- any location that is not held in the cache is not brought into the
  cache as a result of a memory access of that type.
---

Which I *think* means that if there *was* a stale entry in the D-cache
for that location, we need to ensure that entry is gone before we do
the write, or we risk it going missing.

Race condition was an inaccurate way to describe this, but I had
forgotten about the second bullet point.

> > > > Obnoxious question: do we need another DSB here? Or are we reasonably
> > > > guaranteed that one will appear in the instruction stream between here
> > > > and anything else that would touch the same line?
> > >
> > > The MMU enable will issue a DSB to ensure that all the cache
> > > invalidations have completed.
> >
> > And that happens on our return path from here?
> > If so, fine.
> 
> It happens later. But remember that all of this code runs with the MMU
> and caches off. We are just ensuring that our page table changes are
> not shadowed in the PTW's view by clean but stale cachelines when we
> turn on the MMU a bit later.

Second bullet point again. Ignore me.

/
    Leif