On Tue, 4 Jun 2024 at 20:08, Jean-Philippe Brucker
<jean-phili...@linaro.org> wrote:
>
> On Fri, May 31, 2024 at 05:24:44PM +0200, Ard Biesheuvel wrote:
> > > I'm able to reproduce this even without RME. This code was introduced
> > > recently by c98f7f755089 ("ArmVirtPkg: Use dynamic PCD to set the SMCCC
> > > conduit"). Maybe Ard (Cc'd) knows what could be going wrong here.
> > >
> > > A slightly reduced reproducer:
> > >
> > > $ cd edk2/
> > > $ build -b DEBUG -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc
> > > $ cd ..
> > >
> > > $ git clone https://github.com/ARM-software/arm-trusted-firmware.git tf-a
> > > $ cd tf-a/
> > > $ make -j CROSS_COMPILE=aarch64-linux-gnu- PLAT=qemu DEBUG=1 LOG_LEVEL=40
> > > QEMU_USE_GIC_DRIVER=QEMU_GICV3
> > > BL33=../edk2/Build/ArmVirtQemuKernel-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd
> > > all fip && \
> > > dd if=build/qemu/debug/bl1.bin of=flash.bin && \
> > > dd if=build/qemu/debug/fip.bin of=flash.bin seek=64 bs=4096
> > > $ qemu-system-aarch64 -M virt,virtualization=on,secure=on,gic-version=3
> > > -cpu max -m 2G -smp 8 -monitor none -serial mon:stdio -nographic -bios
> > > flash.bin
> > >
> >
> > Hmm, this is not something I anticipated.
> >
> > The problem here is that ArmVirtQemuKernel does not actually support
> > dynamic PCDs, so instead, the PCD here is 'patchable', which means
> > that the underlying value is just overwritten in the binary image, and
> > does not propagate to the rest of the firmware. I assume the write
> > ends up targettng a location that does not tolerate this.
>
> Yes, the QemuVirtMemInfoLib declares this region read-only, so we end up
> with a permission fault
>
> // Map the FV region as normal executable memory
> VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
> VirtualMemoryTable[2].VirtualBase = VirtualMemoryTable[2].PhysicalBase;
> VirtualMemoryTable[2].Length = FixedPcdGet32 (PcdFvSize);
> VirtualMemoryTable[2].Attributes =
> ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO;
>
> Making it writable doesn't seem sufficient, since I then get a "HVC issued
> at EL2" fault. I'll keep debugging.
>
That is expected, sadly. As I said, this code was never intended to run at EL2.
The dynamic PCD will propagate to other boot stages. However, the
'patchable' PCD that we use in ArmVirtQemuKernel is local to the
driver, and other users of the PCD will see the default value of
'HVC'. Which would be fine if we only executed at EL1.
So I know exactly what is wrong and have an idea how to fix it - I
just need to find the time for it.
