Re: [RFT PATCH 0/2] arm64: efi: Call SetVaMap() with a 1:1 mapping

[Darren Hart <darren@os.amperecomputing.com>]

On Wed, Jan 18, 2023 at 04:33:29PM +0100, Ard Biesheuvel wrote:
> On Wed, 18 Jan 2023 at 16:26, Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > On Wed, Jan 18, 2023 at 08:56:54AM +0100, Ard Biesheuvel wrote:
> > > On Tue, 17 Jan 2023 at 18:30, Nathan Chancellor <nathan@kernel.org> wrote:
> > > >
> > > > On Tue, Jan 17, 2023 at 04:20:09PM +0100, Ard Biesheuvel wrote:
> > > > > On Tue, 17 Jan 2023 at 15:27, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > >
> > > > > > Linux on arm64 is now in the same boat as x86, where supporting laptops
> > > > > > that were built to run Windows and never tested beyond what is required
> > > > > > for the Windows Logo certification need workarounds for all kinds of
> > > > > > bizarre behaviors.
> > > > > >
> > > > > > On Snapdragon laptops, we cannot call SetVirtualAddressMap() from the
> > > > > > stub, because the firmware will crash while trying to access memory via
> > > > > > the virtual addresses being installed, which is explicitly unsupported
> > > > > > by the EFI spec.
> > > > > >
> > > > > > However, not calling SetVirtualAddressMap() results in other problems:
> > > > > > on Ampere Altra, it causes SetTime() to crash. On Surface and Flex5g
> > > > > > Windows-on-ARM laptops, it causes ResetSystem() to crash.
> > > > > >
> > > > > > So let's try to work around this while not making too much of a mess.
> > > > > >
> > > > > > First of all, install a 1:1 mapping instead of avoiding SetVaMap()
> > > > > > altogether - from the EFI spec pov, this should amount to the same
> > > > > > thing.
> > > > > >
> > > > > > Then, given that we already use a SMBIOS based hack for Altra to force
> > > > > > the use of SetVirtualAddressMap(), let's check for Surface systems in
> > > > > > the same way.
> > > > > >
> > > > > > Please test, and please report the SMBIOS type 1 family field for which
> > > > > > this workaround is needed.
> > > > > >
> > > > > > Also, note that these changes will not make a difference if the
> > > > > > EFI_RT_PROPERTIES_TABLE lists SetVirtualAddressMap() as not implemented.
> > > > > >
> > > > > > Nathan, I would appreciate it if you could give this a spin on your
> > > > > > Altra box (only patch #1 should make a difference), and for good
> > > > > > measure, double check that hwclock still works as it should.
> > > > > >
> > > > > > Cc: Johan Hovold <johan+linaro@kernel.org>
> > > > > > Cc: Maximilian Luz <luzmaximilian@gmail.com>
> > > > > > Cc: Nathan Chancellor <nathan@kernel.org>
> > > > > > Cc: Steev Klimaszewski <steev@kali.org>
> > > > > > Cc: Shawn Guo <shawn.guo@linaro.org>
> > > > > >
> > > > > > Ard Biesheuvel (2):
> > > > > >   arm64: efi: Prefer a flat virtual mapping of the runtime services
> > > > > >   arm64: efi: Force use of SetVirtualAddressMap() on MS Surface
> > > > > >
> > > > >
> > > > > Bah this does not even work on Yoga C630, so this is not going to help us.
> > > > >
> > > > > If we want ResetSystem() on these machines, we'll have to retain other
> > > > > memory ranges and map the in the EFI runtime map. Yuck.
> > > > >
> > > > > Nathan - still interested in whether patch #1 works on Altra,
> > > >
> > > > I applied patch 1 on top of commit 6e50979a9c87 ("Merge tag
> > > > 'mm-hotfixes-stable-2023-01-16-15-23' of
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm") in Linus' tree
> > > > and everything still appears to be okay with hwclock. If there is any
> > > > more specific testing that I should do, please let me know. Feel free to
> > > > add
> > > >
> > > >     Tested-by: Nathan Chancellor <nathan@kernel.org>
> > > >
> > > > to patch 1 in future revisions, and I am happy to test anything else
> > > > that you might need in this series or future ones.
> > > >
> > > > Mainline:
> > > >
> > > > # uname -mr
> > > > 6.2.0-rc4-00031-g6e50979a9c87 aarch64
> > > >
> > > > # hwclock
> > > > 2023-01-17 09:04:58.845411-07:00
> > > >
> > > > Patch:
> > > >
> > > > # uname -mr
> > > > 6.2.0-rc4-00032-g20165e83052e aarch64
> > > >
> > > > # hwclock
> > > > 2023-01-17 10:25:38.843788-07:00
> > > >
> > >
> > > Thanks Nathan,
> > >
> > > Forgot to mention, though: it is SetTime() not GetTime() that is
> > > problematic on this platform. Could you please double check whether
> > > setting the RTC using hwclock works too?

The other runtime service that triggers this is SetVariable, which I have been
testing like this. The tests still pass on systems with the affected firmware
with the SMBIOS string mitigation in place and this flat va mapping applied.

#!/bin/sh

function set_timeout() {
	TO=$1

	re='^[0-9]$'
	if ! [[ "$TO" =~ $re ]]; then
		echo "Timeout must be 0-9"
		exit 1
	fi

	efibootmgr -t $TO > /dev/null
	efivar -n 8be4df61-93ca-11d2-aa0d-00e098032b8c-Timeout | grep -q "00000000  0$TO 00"
	if [ $? -ne 0 ]; then
		echo "ERROR: timeout $TO not reflected in efivar"
		exit 1
	fi
	echo "SUCCESS: timeout $TO reflected in efivar"
	return 0
}

set_timeout 4
set_timeout 5
exit 0

Thanks,

Darren

> >
> > Ah, okay, makes sense! As far as I can tell, that works too.
> >
> > Mainline:
> >
> > # uname -mr
> > 6.2.0-rc4-00031-g6e50979a9c87 aarch64
> >
> > # hwclock --systohc
> >
> > # echo $status
> > 0
> >
> > Patch:
> >
> > # uname -mr
> > 6.2.0-rc4-00032-g20165e83052e aarch64
> >
> > # hwclock --systohc
> >
> > # echo $status
> > 0
> >
> 
> Thanks again!

-- 
Darren Hart
Ampere Computing / OS and Kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel