MIME-Version: 1.0
References: <2ab9645789707f31fd37c49435e476d4b5c38a0a.1675901828.git.darren@os.amperecomputing.com>
 <DBBPR08MB4538C586B721C8209B03AEEEF7D99@DBBPR08MB4538.eurprd08.prod.outlook.com>
In-Reply-To: <DBBPR08MB4538C586B721C8209B03AEEEF7D99@DBBPR08MB4538.eurprd08.prod.outlook.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Thu, 9 Feb 2023 16:30:57 +0100
Message-ID: <CAMj1kXG8TY9eKJxtSWYPCu_8qs7W3FWwDSZ+teuwhHb1BHUf7g@mail.gmail.com>
Subject: Re: [PATCH v2] arm64: efi: Force the use of SetVirtualAddressMap() on
 eMAG and Altra Max machines
To:     Justin He <Justin.He@arm.com>
Cc:     Darren Hart <darren@os.amperecomputing.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>,
        "linux-efi@vger.kernel.org" <linux-efi@vger.kernel.org>,
        Alexandru Elisei <alexandru.elisei@gmail.com>, nd <nd@arm.com>,
        Nathan Chancellor <nathan@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

(cc Nathan, another happy Ampere customer)

On Thu, 9 Feb 2023 at 05:26, Justin He <Justin.He@arm.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Darren Hart <darren@os.amperecomputing.com>
> > Sent: Thursday, February 9, 2023 8:28 AM
> > To: LKML <linux-kernel@vger.kernel.org>
> > Cc: stable@vger.kernel.org; linux-efi@vger.kernel.org; Alexandru Elisei
> > <alexandru.elisei@gmail.com>; Justin He <Justin.He@arm.com>; Huacai Chen
> > <chenhuacai@kernel.org>; Jason A. Donenfeld <Jason@zx2c4.com>; Ard
> > Biesheuvel <ardb@kernel.org>
> > Subject: [PATCH v2] arm64: efi: Force the use of SetVirtualAddressMap() on
> > eMAG and Altra Max machines
> >
> > Commit 550b33cfd445 ("arm64: efi: Force the use of SetVirtualAddressMap()
> > on Altra machines") identifies the Altra family via the family field in the type#1
> > SMBIOS record. eMAG and Altra Max machines are similarly affected but not
> > detected with the strict strcmp test.
> >
> > The type1_family smbios string is not an entirely reliable means of identifying
> > systems with this issue as OEMs can, and do, use their own strings for these
> > fields. However, until we have a better solution, capture the bulk of these
> > systems by adding strcmp matching for "eMAG"
> > and "Altra Max".
> >
> > Fixes: 550b33cfd445 ("arm64: efi: Force the use of SetVirtualAddressMap() on
> > Altra machines")
> > Cc: <stable@vger.kernel.org> # 6.1.x
> > Cc: <linux-efi@vger.kernel.org>
> > Cc: Alexandru Elisei <alexandru.elisei@gmail.com>
> > Cc: Justin He <Justin.He@arm.com>
> > Cc: Huacai Chen <chenhuacai@kernel.org>
> > Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Signed-off-by: Darren Hart <darren@os.amperecomputing.com>
> Tested-by: justin.he@arm.com
> > ---
> > V1 -> V2: include eMAG
> >
> >  drivers/firmware/efi/libstub/arm64.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/arm64.c
> > b/drivers/firmware/efi/libstub/arm64.c
> > index ff2d18c42ee7..4501652e11ab 100644
> > --- a/drivers/firmware/efi/libstub/arm64.c
> > +++ b/drivers/firmware/efi/libstub/arm64.c
> > @@ -19,10 +19,13 @@ static bool system_needs_vamap(void)
> >       const u8 *type1_family = efi_get_smbios_string(1, family);
> >
> >       /*
> > -      * Ampere Altra machines crash in SetTime() if SetVirtualAddressMap()
> > -      * has not been called prior.
> > +      * Ampere eMAG, Altra, and Altra Max machines crash in SetTime() if
> > +      * SetVirtualAddressMap() has not been called prior.
> >        */
> > -     if (!type1_family || strcmp(type1_family, "Altra"))
> > +     if (!type1_family || (
> > +         strcmp(type1_family, "eMAG") &&
> > +         strcmp(type1_family, "Altra") &&
> > +         strcmp(type1_family, "Altra Max")))
> In terms of resolving the boot hang issue, it looks good to me. And I've verified the
> "eMAG" part check.
> So please feel free to add:
> Tested-by: Justin He <justin.he@arm.com>
>

Thanks. I've queued this up now.

> But I have some other concerns:
> 1. On an Altra server, the type1_family returns "Server". I don't know whether it
> is a smbios or server firmware bug.

This is not really a bug. OEMs are free to put whatever they want into
those fields, although that is a great example of a sloppy vendor that
just puts random junk in there.

We could plumb in the type 4 smbios record too, and check the version
for *Altra* - however, it would be nice to get an idea of how many
more we will end up needing to handle here.

Also, is anyone looking to get this fixed? There is Altra code in the
public EDK2 repo, but it is very hard to get someone to care about
these things, and fix their firmware.


> 2. On an eMAG server, I once successfully run efibootmgr -t 10 to call the
> Set_variable rts, but currently I always met the error, even with above patch:
> # efibootmgr -t 9; efibootmgr -t 5;
> Could not set Timeout: Input/output error
> Could not set Timeout: Input/output error
>

Did this work before? Can you bisect?

> Meanwhile, on the Altra server, it works:
> # efibootmgr -t 9; efibootmgr -t 5;
> BootCurrent: 0007
> Timeout: 9 seconds
> BootOrder: 0007,0005,0006,0001
> Boot0001* UEFI: Built-in EFI Shell
> Boot0005* UEFI: PXE IPv4 Intel(R) I350 Gigabit Network Connection
> Boot0006* UEFI: PXE IPv4 Intel(R) I350 Gigabit Network Connection
> Boot0007* ubuntu
> BootCurrent: 0007
> Timeout: 5 seconds
> BootOrder: 0007,0005,0006,0001
> Boot0001* UEFI: Built-in EFI Shell
> Boot0005* UEFI: PXE IPv4 Intel(R) I350 Gigabit Network Connection
> Boot0006* UEFI: PXE IPv4 Intel(R) I350 Gigabit Network Connection
> Boot0007* ubuntu
>
>
>
> --
> Cheers,
> Justin (Jia He)
>
>