Re: [edk2-devel] [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Ard Biesheuvel]

On Fri, 19 Apr 2024 at 18:36, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 19 Apr 2024 at 18:09, Jonathan Cameron via groups.io
> <jonathan.cameron=huawei.com@groups.io> wrote:
> >
> > On Fri, 19 Apr 2024 13:52:07 +0200
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > >   Hi,
> > >
> > > > Gerd, any ideas?  Maybe I needs something subtly different in my
> > > > edk2 build?  I've not looked at this bit of the qemu infrastructure
> > > > before - is there a document on how that image is built?
> > >
> > > There is roms/Makefile for that.
> > >
> > > make -C roms help
> > > make -C roms efi
> > >
> > > So easiest would be to just update the edk2 submodule to what you
> > > need, then rebuild.
> > >
> > > The build is handled by the roms/edk2-build.py script,
> > > with the build configuration being in roms/edk2-build.config.
> > > That is usable outside the qemu source tree too, i.e. like this:
> > >
> > >   python3 /path/to/qemu.git/roms/edk2-build.py \
> > >     --config /path/to/qemu.git/roms/edk2-build.config \
> > >     --core /path/to/edk2.git \
> > >     --match armvirt \
> > >     --silent --no-logs
> > >
> > > That'll try to place the images build in "../pc-bios", so maybe better
> > > work with a copy of the config file where you adjust this.
> > >
> > > HTH,
> > >   Gerd
> > >
> >
> > Thanks Gerd!
> >
> > So the builds are very similar via the two method...
> > However - the QEMU build sets -D CAVIUM_ERRATUM_27456=TRUE
> >
> > And that's the difference - with that set for my other builds the alignment
> > problems go away...
> >
> > Any idea why we have that set in roms/edk2-build.config?
> > Superficially it seems rather unlikely anyone cares about thunderx1
> > (if they do we need to get them some new hardware with fresh bugs)
> > bugs now and this config file was only added last year.
> >
> >
> > However, the last comment in Ard's commit message below seems
> > highly likely to be relevant!
> >
> > Chasing through Ard's patch it has the side effect of dropping
> > an override of a requirement for strict alignment.
> > So with out the errata
> > DEFINE GCC_AARCH64_CC_XIPFLAGS     = -mstrict-align -mgeneral-regs-only
> > is replaced with
> >  [BuildOptions]
> > +!if $(CAVIUM_ERRATUM_27456) == TRUE^M
> > +  GCC:*_*_AARCH64_PP_FLAGS = -DCAVIUM_ERRATUM_27456^M
> > +!else^M
> >    GCC:*_*_AARCH64_CC_XIPFLAGS ==
> > +!endif^M
> >
> > The edk2 commit that added this was the following +CC Ard.
> >
> > Given I wasn't sure of the syntax of that file I set it
> > manually to the original value and indeed it works.
> >
> >
> > commit ec54ce1f1ab41b92782b37ae59e752fff0ef9c41
> > Author: Ard Biesheuvel <ardb@kernel.org>
> > Date:   Wed Jan 4 16:51:35 2023 +0100
> >
> >     ArmVirtPkg/ArmVirtQemu: Avoid early ID map on ThunderX
> >
> >     The early ID map used by ArmVirtQemu uses ASID scoped non-global
> >     mappings, as this allows us to switch to the permanent ID map seamlessly
> >     without the need for explicit TLB maintenance.
> >
> >     However, this triggers a known erratum on ThunderX, which does not
> >     tolerate non-global mappings that are executable at EL1, as this appears
> >     to result in I-cache corruption. (Linux disables the KPTI based Meltdown
> >     mitigation on ThunderX for the same reason)
> >
> >     So work around this, by detecting the CPU implementor and part number,
> >     and proceeding without the early ID map if a ThunderX CPU is detected.
> >
> >     Note that this requires the C code to be built with strict alignment
> >     again, as we may end up executing it with the MMU and caches off.
> >
> >     Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >     Acked-by: Laszlo Ersek <lersek@redhat.com>
> >     Tested-by: dann frazier <dann.frazier@canonical.com>
> >
> > Test case is
> > qemu-system-aarch64 -M virt,virtualization=true, -m 4g -cpu cortex-a76 \
> > -bios QEMU_EFI.fd -d int
> >
> > Which gets alignment faults since:
> > https://lore.kernel.org/all/20240301204110.656742-6-richard.henderson@linaro.org/
> >
> > So my feeling here is EDK2 should either have yet another config for QEMU 
> > as a host
> > or should always set the alignment without needing to pick the CAVIUM 27456 
> > errata
> > which I suspect will get dropped soonish anyway if anyone ever cleans up
> > old errata.
> >
>
> This code was never really intended for execution at EL2, but it
> happened to work, partially because TCG's lack of strict alignment
> checking when the MMU is off.
>
> Those assumptions no longer hold, so yes, let's get this fixed properly.
>
> Given VHE and nested virt (which will likely imply VHE in practice), I
> would like to extend this functionality (i.e., the use of preliminary
> page tables in NOR flash) to EL2 as well, but with VHE enabled. This
> means we can still elide TLB maintenance (and BBM checks) by using
> different ASIDs, and otherwise, fall back to entering with the MMU off
> if VHE is not available. In that case, we should enforce strict
> alignment too, so that needs to be fixed regardless.
>
> I'll try to code something up and send it round. In the mean time,
> feel free to propose a minimal patch that reinstates the strict
> alignment if you are pressed for time, and I'll merge it right away.

Actually, let's just so that first - I'll send out a patch.