[PATCH v2] arm64: efi: Force the use of SetVirtualAddressMap() on eMAG and Altra Max machines

[PATCH v2] arm64: efi: Force the use of SetVirtualAddressMap() on eMAG and Altra Max machines

[Darren Hart]

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>
---
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")))
 		return false;
 
 	efi_warn("Working around broken SetVirtualAddressMap()\n");
-- 
2.34.3

RE: [PATCH v2] arm64: efi: Force the use of SetVirtualAddressMap() on eMAG and Altra Max machines

[Justin He]

> -----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>

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.
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

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)

Re: [PATCH v2] arm64: efi: Force the use of SetVirtualAddressMap() on eMAG and Altra Max machines

[Ard Biesheuvel]

(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)
>
>

Re: [PATCH v2] arm64: efi: Force the use of SetVirtualAddressMap() on eMAG and Altra Max machines

[Darren Hart]

On Thu, Feb 09, 2023 at 04:30:57PM +0100, Ard Biesheuvel wrote:
> (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 use the type1 "product name" and have a unique identifier, but that
doesn't scale well either. Ampere partners with many OEMs, and we should expect
this to increase in time.

> 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.

If we don't get this fixed in firmware, I think we have two kernel side
maintainable options:

1) Depend on the type4 string for the SoCs with impacted EDK2 firmware. This is
suboptimal as OEMs should be able to update the firmware they ship and control
how the kernel interacts with their platform. This effectively removes that
option in order to avoid the individual listing of "product name".

2) Revert the earlier commit changing the default to not calling
SetVirtualAddressMap(). Obviously undesirable for the reasons that patch went
in, and it affects all arm64 platforms. The only argument here is it used to
work and now it doesn't.

> 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.

Yes, this is a conversation I'm having internally with the firmware teams, and
the preferred approach provided it can be effectively rolled out to eventually
capture all Altra* and future platforms.

Thanks,

-- 
Darren Hart
Ampere Computing / OS and Kernel

RE: [PATCH v2] arm64: efi: Force the use of SetVirtualAddressMap() on eMAG and Altra Max machines

[Justin He]

Hi Ard,

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Thursday, February 9, 2023 11:31 PM
> To: Justin He <Justin.He@arm.com>
> Cc: Darren Hart <darren@os.amperecomputing.com>; LKML
> <linux-kernel@vger.kernel.org>; stable@vger.kernel.org;
> linux-efi@vger.kernel.org; Alexandru Elisei <alexandru.elisei@gmail.com>; nd
> <nd@arm.com>; Nathan Chancellor <nathan@kernel.org>
> Subject: Re: [PATCH v2] arm64: efi: Force the use of SetVirtualAddressMap() on
> eMAG and Altra Max machines
> 
> (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?
Yes, it worked before, as the log I attached in previous thread.
That kernel is v5.19+.
But very strange, now I tried different kernel (v6.x, v5.19,v4.19),
It didn't work now.

Bisect doesn’t help in this case.

--
Cheers,
Justin (Jia He)

RE: [PATCH v2] arm64: efi: Force the use of SetVirtualAddressMap() on eMAG and Altra Max machines

[Justin He]

Hi Darren and Ard

> -----Original Message-----
> From: Darren Hart <darren@os.amperecomputing.com>
> Sent: Friday, February 10, 2023 5:07 AM
> To: Ard Biesheuvel <ardb@kernel.org>
> Cc: Justin He <Justin.He@arm.com>; LKML <linux-kernel@vger.kernel.org>;
> stable@vger.kernel.org; linux-efi@vger.kernel.org; Alexandru Elisei
> <alexandru.elisei@gmail.com>; nd <nd@arm.com>; Nathan Chancellor
> <nathan@kernel.org>
> Subject: Re: [PATCH v2] arm64: efi: Force the use of SetVirtualAddressMap() on
> eMAG and Altra Max machines
> 
> On Thu, Feb 09, 2023 at 04:30:57PM +0100, Ard Biesheuvel wrote:
> > (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 use the type1 "product name" and have a unique identifier, but that
> doesn't scale well either. Ampere partners with many OEMs, and we should
> expect this to increase in time.
> 
> > 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.
> 
> If we don't get this fixed in firmware, I think we have two kernel side
> maintainable options:
> 
> 1) Depend on the type4 string for the SoCs with impacted EDK2 firmware. This
> is suboptimal as OEMs should be able to update the firmware they ship and
> control how the kernel interacts with their platform. This effectively removes
> that option in order to avoid the individual listing of "product name".
> 
Whether could we introduce a system_force_vamap or similar one to force the 
system_needs_vamap() to return true. I think kernel should allow those
buggy firmware to boot instead of hanging. And the forcible boot parameter is
at least better than the complicated workaround patches.

What do you think of it?

--
Cheers,
Justin (Jia He)

> 2) Revert the earlier commit changing the default to not calling
> SetVirtualAddressMap(). Obviously undesirable for the reasons that patch went
> in, and it affects all arm64 platforms. The only argument here is it used to
> work and now it doesn't.
> 
> > 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.
> 
> Yes, this is a conversation I'm having internally with the firmware teams, and
> the preferred approach provided it can be effectively rolled out to eventually
> capture all Altra* and future platforms.
>