[PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Ard Biesheuvel]

The UEFI spec revision 2.7 errata A section 8.4 has the following to
say about the virtual memory runtime services:

  "This section contains function definitions for the virtual memory
  support that may be optionally used by an operating system at runtime.
  If an operating system chooses to make EFI runtime service calls in a
  virtual addressing mode instead of the flat physical mode, then the
  operating system must use the services in this section to switch the
  EFI runtime services from flat physical addressing to virtual
  addressing."

So it is pretty clear that calling SetVirtualAddressMap() is entirely
optional, and so there is no point in doing so unless it achieves
anything useful for us.

This is not the case for 64-bit ARM. The native mapping used by the OS
is arbitrarily converted into another permutation of userland addresses
(i.e., bits [63:48] cleared), and the runtime code could easily deal
with the original layout in exactly the same way as it deals with the
converted layout. However, due to constraints related to page size
differences if the OS is not running with 4k pages, and related to
systems that may expose the individual sections of PE/COFF runtime
modules as different memory regions, creating the virtual layout is a
bit fiddly, and requires us to sort the memory map and reason about
adjacent regions with identical memory types etc etc.

So the obvious fix is to stop calling SetVirtualAddressMap() altogether
on arm64 systems. However, to avoid surprises, which are notoriously
hard to diagnose when it comes to OS<->firmware interactions, let's
start by making it an opt-out feature, and implement support for the
'efi=novamap' kernel command line parameter on ARM and arm64 systems.

(Note that 32-bit ARM generally does require SetVirtualAddressMap() to be
used, given that the physical memory map and the kernel virtual address
map are not guaranteed to be non-overlapping like on arm64. However,
having support for efi=novamap,noruntime on 32-bit ARM, combined with
the recently proposed support for earlycon=efi, is likely to be useful
to diagnose boot issues on such systems if they have no accessible serial
port)

Cc: Alexander Graf <agraf@suse.de>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/libstub/arm-stub.c        |  5 +++++
 drivers/firmware/efi/libstub/efi-stub-helper.c | 10 ++++++++++
 drivers/firmware/efi/libstub/efistub.h         |  1 +
 drivers/firmware/efi/libstub/fdt.c             |  3 +++
 4 files changed, 19 insertions(+)

diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index eee42d5e25ee..626ec4b4a664 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -370,6 +370,11 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
 		paddr = in->phys_addr;
 		size = in->num_pages * EFI_PAGE_SIZE;
 
+		if (novamap()) {
+			in->virt_addr = in->phys_addr;
+			continue;
+		}
+
 		/*
 		 * Make the mapping compatible with 64k pages: this allows
 		 * a 4k page size kernel to kexec a 64k page size kernel and
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index e94975f4655b..442f51c2a53d 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -34,6 +34,7 @@ static unsigned long __chunk_size = EFI_READ_CHUNK_SIZE;
 
 static int __section(.data) __nokaslr;
 static int __section(.data) __quiet;
+static int __section(.data) __novamap;
 
 int __pure nokaslr(void)
 {
@@ -43,6 +44,10 @@ int __pure is_quiet(void)
 {
 	return __quiet;
 }
+int __pure novamap(void)
+{
+	return __novamap;
+}
 
 #define EFI_MMAP_NR_SLACK_SLOTS	8
 
@@ -482,6 +487,11 @@ efi_status_t efi_parse_options(char const *cmdline)
 			__chunk_size = -1UL;
 		}
 
+		if (!strncmp(str, "novamap", 7)) {
+			str += strlen("novamap");
+			__novamap = 1;
+		}
+
 		/* Group words together, delimited by "," */
 		while (*str && *str != ' ' && *str != ',')
 			str++;
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 32799cf039ef..337b52c4702c 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -27,6 +27,7 @@
 
 extern int __pure nokaslr(void);
 extern int __pure is_quiet(void);
+extern int __pure novamap(void);
 
 #define pr_efi(sys_table, msg)		do {				\
 	if (!is_quiet()) efi_printk(sys_table, "EFI stub: "msg);	\
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 0dc7b4987cc2..f8f89f995e9d 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -327,6 +327,9 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	if (status == EFI_SUCCESS) {
 		efi_set_virtual_address_map_t *svam;
 
+		if (novamap())
+			return EFI_SUCCESS;
+
 		/* Install the new virtual address map */
 		svam = sys_table->runtime->set_virtual_address_map;
 		status = svam(runtime_entry_count * desc_size, desc_size,
-- 
2.17.1


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

Re: [PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Heinrich Schuchardt]

On 1/26/19 11:22 AM, Ard Biesheuvel wrote:
> The UEFI spec revision 2.7 errata A section 8.4 has the following to
> say about the virtual memory runtime services:
> 
>   "This section contains function definitions for the virtual memory
>   support that may be optionally used by an operating system at runtime.
>   If an operating system chooses to make EFI runtime service calls in a
>   virtual addressing mode instead of the flat physical mode, then the
>   operating system must use the services in this section to switch the
>   EFI runtime services from flat physical addressing to virtual
>   addressing."
> 
> So it is pretty clear that calling SetVirtualAddressMap() is entirely
> optional, and so there is no point in doing so unless it achieves
> anything useful for us.
> 
> This is not the case for 64-bit ARM. The native mapping used by the OS
> is arbitrarily converted into another permutation of userland addresses
> (i.e., bits [63:48] cleared), and the runtime code could easily deal
> with the original layout in exactly the same way as it deals with the
> converted layout. However, due to constraints related to page size
> differences if the OS is not running with 4k pages, and related to
> systems that may expose the individual sections of PE/COFF runtime
> modules as different memory regions, creating the virtual layout is a
> bit fiddly, and requires us to sort the memory map and reason about
> adjacent regions with identical memory types etc etc.
> 
> So the obvious fix is to stop calling SetVirtualAddressMap() altogether
> on arm64 systems. However, to avoid surprises, which are notoriously
> hard to diagnose when it comes to OS<->firmware interactions, let's
> start by making it an opt-out feature, and implement support for the
> 'efi=novamap' kernel command line parameter on ARM and arm64 systems.
> 
> (Note that 32-bit ARM generally does require SetVirtualAddressMap() to be
> used, given that the physical memory map and the kernel virtual address
> map are not guaranteed to be non-overlapping like on arm64. However,
> having support for efi=novamap,noruntime on 32-bit ARM, combined with
> the recently proposed support for earlycon=efi, is likely to be useful
> to diagnose boot issues on such systems if they have no accessible serial
> port)
> 

NAK

This patch breaks EFI booting with any known U-Boot release.

You are a contributor to an alternative open source firmware (EDK2).
Could you, please, share test results for EDK2.

Best regards

Heinrich

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

Re: [PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Ard Biesheuvel]

On Sat, 26 Jan 2019 at 13:27, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 1/26/19 11:22 AM, Ard Biesheuvel wrote:
> > The UEFI spec revision 2.7 errata A section 8.4 has the following to
> > say about the virtual memory runtime services:
> >
> >   "This section contains function definitions for the virtual memory
> >   support that may be optionally used by an operating system at runtime.
> >   If an operating system chooses to make EFI runtime service calls in a
> >   virtual addressing mode instead of the flat physical mode, then the
> >   operating system must use the services in this section to switch the
> >   EFI runtime services from flat physical addressing to virtual
> >   addressing."
> >
> > So it is pretty clear that calling SetVirtualAddressMap() is entirely
> > optional, and so there is no point in doing so unless it achieves
> > anything useful for us.
> >
> > This is not the case for 64-bit ARM. The native mapping used by the OS
> > is arbitrarily converted into another permutation of userland addresses
> > (i.e., bits [63:48] cleared), and the runtime code could easily deal
> > with the original layout in exactly the same way as it deals with the
> > converted layout. However, due to constraints related to page size
> > differences if the OS is not running with 4k pages, and related to
> > systems that may expose the individual sections of PE/COFF runtime
> > modules as different memory regions, creating the virtual layout is a
> > bit fiddly, and requires us to sort the memory map and reason about
> > adjacent regions with identical memory types etc etc.
> >
> > So the obvious fix is to stop calling SetVirtualAddressMap() altogether
> > on arm64 systems. However, to avoid surprises, which are notoriously
> > hard to diagnose when it comes to OS<->firmware interactions, let's
> > start by making it an opt-out feature, and implement support for the
> > 'efi=novamap' kernel command line parameter on ARM and arm64 systems.
> >
> > (Note that 32-bit ARM generally does require SetVirtualAddressMap() to be
> > used, given that the physical memory map and the kernel virtual address
> > map are not guaranteed to be non-overlapping like on arm64. However,
> > having support for efi=novamap,noruntime on 32-bit ARM, combined with
> > the recently proposed support for earlycon=efi, is likely to be useful
> > to diagnose boot issues on such systems if they have no accessible serial
> > port)
> >
>
> NAK
>

Excuse me?

> This patch breaks EFI booting with any known U-Boot release.
>

It does if you pass 'efi=novmap'. Otherwise, it works fine.

> You are a contributor to an alternative open source firmware (EDK2).
> Could you, please, share test results for EDK2.
>

It works fine with EDK2.

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

Re: [PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Alexander Graf]

> Am 26.01.2019 um 13:28 schrieb Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> 
>> On Sat, 26 Jan 2019 at 13:27, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> 
>>> On 1/26/19 11:22 AM, Ard Biesheuvel wrote:
>>> The UEFI spec revision 2.7 errata A section 8.4 has the following to
>>> say about the virtual memory runtime services:
>>> 
>>>  "This section contains function definitions for the virtual memory
>>>  support that may be optionally used by an operating system at runtime.
>>>  If an operating system chooses to make EFI runtime service calls in a
>>>  virtual addressing mode instead of the flat physical mode, then the
>>>  operating system must use the services in this section to switch the
>>>  EFI runtime services from flat physical addressing to virtual
>>>  addressing."
>>> 
>>> So it is pretty clear that calling SetVirtualAddressMap() is entirely
>>> optional, and so there is no point in doing so unless it achieves
>>> anything useful for us.
>>> 
>>> This is not the case for 64-bit ARM. The native mapping used by the OS
>>> is arbitrarily converted into another permutation of userland addresses
>>> (i.e., bits [63:48] cleared), and the runtime code could easily deal
>>> with the original layout in exactly the same way as it deals with the
>>> converted layout. However, due to constraints related to page size
>>> differences if the OS is not running with 4k pages, and related to
>>> systems that may expose the individual sections of PE/COFF runtime
>>> modules as different memory regions, creating the virtual layout is a
>>> bit fiddly, and requires us to sort the memory map and reason about
>>> adjacent regions with identical memory types etc etc.
>>> 
>>> So the obvious fix is to stop calling SetVirtualAddressMap() altogether
>>> on arm64 systems. However, to avoid surprises, which are notoriously
>>> hard to diagnose when it comes to OS<->firmware interactions, let's
>>> start by making it an opt-out feature, and implement support for the
>>> 'efi=novamap' kernel command line parameter on ARM and arm64 systems.
>>> 
>>> (Note that 32-bit ARM generally does require SetVirtualAddressMap() to be
>>> used, given that the physical memory map and the kernel virtual address
>>> map are not guaranteed to be non-overlapping like on arm64. However,
>>> having support for efi=novamap,noruntime on 32-bit ARM, combined with
>>> the recently proposed support for earlycon=efi, is likely to be useful
>>> to diagnose boot issues on such systems if they have no accessible serial
>>> port)
>>> 
>> 
>> NAK
>> 
> 
> Excuse me?
> 
>> This patch breaks EFI booting with any known U-Boot release.
>> 
> 
> It does if you pass 'efi=novmap'. Otherwise, it works fine.

Even then it doesn't break the entire boot, only runtime services (which are guarded anyway). I would claim that in most cases, we do not break even.

Alex



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

Re: [PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Heinrich Schuchardt]

On 1/26/19 1:34 PM, Alexander Graf wrote:
> 
> 
>> Am 26.01.2019 um 13:28 schrieb Ard Biesheuvel <ard.biesheuvel@linaro.org>:
>>
>>> On Sat, 26 Jan 2019 at 13:27, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>
>>>> On 1/26/19 11:22 AM, Ard Biesheuvel wrote:
>>>> The UEFI spec revision 2.7 errata A section 8.4 has the following to
>>>> say about the virtual memory runtime services:
>>>>
>>>>  "This section contains function definitions for the virtual memory
>>>>  support that may be optionally used by an operating system at runtime.
>>>>  If an operating system chooses to make EFI runtime service calls in a
>>>>  virtual addressing mode instead of the flat physical mode, then the
>>>>  operating system must use the services in this section to switch the
>>>>  EFI runtime services from flat physical addressing to virtual
>>>>  addressing."
>>>>
>>>> So it is pretty clear that calling SetVirtualAddressMap() is entirely
>>>> optional, and so there is no point in doing so unless it achieves
>>>> anything useful for us.
>>>>
>>>> This is not the case for 64-bit ARM. The native mapping used by the OS
>>>> is arbitrarily converted into another permutation of userland addresses
>>>> (i.e., bits [63:48] cleared), and the runtime code could easily deal
>>>> with the original layout in exactly the same way as it deals with the
>>>> converted layout. However, due to constraints related to page size
>>>> differences if the OS is not running with 4k pages, and related to
>>>> systems that may expose the individual sections of PE/COFF runtime
>>>> modules as different memory regions, creating the virtual layout is a
>>>> bit fiddly, and requires us to sort the memory map and reason about
>>>> adjacent regions with identical memory types etc etc.
>>>>
>>>> So the obvious fix is to stop calling SetVirtualAddressMap() altogether
>>>> on arm64 systems. However, to avoid surprises, which are notoriously
>>>> hard to diagnose when it comes to OS<->firmware interactions, let's
>>>> start by making it an opt-out feature, and implement support for the
>>>> 'efi=novamap' kernel command line parameter on ARM and arm64 systems.
>>>>
>>>> (Note that 32-bit ARM generally does require SetVirtualAddressMap() to be
>>>> used, given that the physical memory map and the kernel virtual address
>>>> map are not guaranteed to be non-overlapping like on arm64. However,
>>>> having support for efi=novamap,noruntime on 32-bit ARM, combined with
>>>> the recently proposed support for earlycon=efi, is likely to be useful
>>>> to diagnose boot issues on such systems if they have no accessible serial
>>>> port)
>>>>
>>>
>>> NAK
>>>
>>
>> Excuse me?
>>
>>> This patch breaks EFI booting with any known U-Boot release.
>>>
>>
>> It does if you pass 'efi=novmap'. Otherwise, it works fine.

It think it would be helpful to add this information to the commit message.

If it is strictly opt-in, I have no concern.

Best regards

Heinrich

> 
> Even then it doesn't break the entire boot, only runtime services (which are guarded anyway). I would claim that in most cases, we do not break even.
> 
> Alex
> 
> 
> 


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

Re: [PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Ard Biesheuvel]

On Sat, 26 Jan 2019 at 15:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 1/26/19 1:34 PM, Alexander Graf wrote:
> >
> >
> >> Am 26.01.2019 um 13:28 schrieb Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> >>
> >>> On Sat, 26 Jan 2019 at 13:27, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>
> >>>> On 1/26/19 11:22 AM, Ard Biesheuvel wrote:
> >>>> The UEFI spec revision 2.7 errata A section 8.4 has the following to
> >>>> say about the virtual memory runtime services:
> >>>>
> >>>>  "This section contains function definitions for the virtual memory
> >>>>  support that may be optionally used by an operating system at runtime.
> >>>>  If an operating system chooses to make EFI runtime service calls in a
> >>>>  virtual addressing mode instead of the flat physical mode, then the
> >>>>  operating system must use the services in this section to switch the
> >>>>  EFI runtime services from flat physical addressing to virtual
> >>>>  addressing."
> >>>>
> >>>> So it is pretty clear that calling SetVirtualAddressMap() is entirely
> >>>> optional, and so there is no point in doing so unless it achieves
> >>>> anything useful for us.
> >>>>
> >>>> This is not the case for 64-bit ARM. The native mapping used by the OS
> >>>> is arbitrarily converted into another permutation of userland addresses
> >>>> (i.e., bits [63:48] cleared), and the runtime code could easily deal
> >>>> with the original layout in exactly the same way as it deals with the
> >>>> converted layout. However, due to constraints related to page size
> >>>> differences if the OS is not running with 4k pages, and related to
> >>>> systems that may expose the individual sections of PE/COFF runtime
> >>>> modules as different memory regions, creating the virtual layout is a
> >>>> bit fiddly, and requires us to sort the memory map and reason about
> >>>> adjacent regions with identical memory types etc etc.
> >>>>
> >>>> So the obvious fix is to stop calling SetVirtualAddressMap() altogether
> >>>> on arm64 systems. However, to avoid surprises, which are notoriously
> >>>> hard to diagnose when it comes to OS<->firmware interactions, let's
> >>>> start by making it an opt-out feature, and implement support for the
> >>>> 'efi=novamap' kernel command line parameter on ARM and arm64 systems.
> >>>>
> >>>> (Note that 32-bit ARM generally does require SetVirtualAddressMap() to be
> >>>> used, given that the physical memory map and the kernel virtual address
> >>>> map are not guaranteed to be non-overlapping like on arm64. However,
> >>>> having support for efi=novamap,noruntime on 32-bit ARM, combined with
> >>>> the recently proposed support for earlycon=efi, is likely to be useful
> >>>> to diagnose boot issues on such systems if they have no accessible serial
> >>>> port)
> >>>>
> >>>
> >>> NAK
> >>>
> >>
> >> Excuse me?
> >>
> >>> This patch breaks EFI booting with any known U-Boot release.
> >>>
> >>
> >> It does if you pass 'efi=novmap'. Otherwise, it works fine.
>
> It think it would be helpful to add this information to the commit message.
>

Did you read the commit message?

> If it is strictly opt-in, I have no concern.
>

Thanks.

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

Re: [PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Ard Biesheuvel]

On Sat, 26 Jan 2019 at 16:03, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Sat, 26 Jan 2019 at 15:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 1/26/19 1:34 PM, Alexander Graf wrote:
> > >
> > >
> > >> Am 26.01.2019 um 13:28 schrieb Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> > >>
> > >>> On Sat, 26 Jan 2019 at 13:27, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >>>
> > >>>> On 1/26/19 11:22 AM, Ard Biesheuvel wrote:
> > >>>> The UEFI spec revision 2.7 errata A section 8.4 has the following to
> > >>>> say about the virtual memory runtime services:
> > >>>>
> > >>>>  "This section contains function definitions for the virtual memory
> > >>>>  support that may be optionally used by an operating system at runtime.
> > >>>>  If an operating system chooses to make EFI runtime service calls in a
> > >>>>  virtual addressing mode instead of the flat physical mode, then the
> > >>>>  operating system must use the services in this section to switch the
> > >>>>  EFI runtime services from flat physical addressing to virtual
> > >>>>  addressing."
> > >>>>
> > >>>> So it is pretty clear that calling SetVirtualAddressMap() is entirely
> > >>>> optional, and so there is no point in doing so unless it achieves
> > >>>> anything useful for us.
> > >>>>
> > >>>> This is not the case for 64-bit ARM. The native mapping used by the OS
> > >>>> is arbitrarily converted into another permutation of userland addresses
> > >>>> (i.e., bits [63:48] cleared), and the runtime code could easily deal
> > >>>> with the original layout in exactly the same way as it deals with the
> > >>>> converted layout. However, due to constraints related to page size
> > >>>> differences if the OS is not running with 4k pages, and related to
> > >>>> systems that may expose the individual sections of PE/COFF runtime
> > >>>> modules as different memory regions, creating the virtual layout is a
> > >>>> bit fiddly, and requires us to sort the memory map and reason about
> > >>>> adjacent regions with identical memory types etc etc.
> > >>>>
> > >>>> So the obvious fix is to stop calling SetVirtualAddressMap() altogether
> > >>>> on arm64 systems. However, to avoid surprises, which are notoriously
> > >>>> hard to diagnose when it comes to OS<->firmware interactions, let's
> > >>>> start by making it an opt-out feature, and implement support for the
> > >>>> 'efi=novamap' kernel command line parameter on ARM and arm64 systems.
> > >>>>
> > >>>> (Note that 32-bit ARM generally does require SetVirtualAddressMap() to be
> > >>>> used, given that the physical memory map and the kernel virtual address
> > >>>> map are not guaranteed to be non-overlapping like on arm64. However,
> > >>>> having support for efi=novamap,noruntime on 32-bit ARM, combined with
> > >>>> the recently proposed support for earlycon=efi, is likely to be useful
> > >>>> to diagnose boot issues on such systems if they have no accessible serial
> > >>>> port)
> > >>>>
> > >>>
> > >>> NAK
> > >>>
> > >>
> > >> Excuse me?
> > >>
> > >>> This patch breaks EFI booting with any known U-Boot release.
> > >>>
> > >>
> > >> It does if you pass 'efi=novmap'. Otherwise, it works fine.
> >
> > It think it would be helpful to add this information to the commit message.
> >
>
> Did you read the commit message?
>
> > If it is strictly opt-in, I have no concern.
> >
>
> Thanks.

Below is the boot log of a EDK2 based SynQuacer UEFI/ACPI machine
running with this change enabled. The dump of the memory map has been
modified to show the virtual address between ()

https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc2-3-g4f78f37fc6eb/arm64/defconfig/lab-mhart/boot-synquacer-acpi.html

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

Re: [PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Jeffrey Hugo]

On 1/26/2019 3:22 AM, Ard Biesheuvel wrote:
> The UEFI spec revision 2.7 errata A section 8.4 has the following to
> say about the virtual memory runtime services:
> 
>    "This section contains function definitions for the virtual memory
>    support that may be optionally used by an operating system at runtime.
>    If an operating system chooses to make EFI runtime service calls in a
>    virtual addressing mode instead of the flat physical mode, then the
>    operating system must use the services in this section to switch the
>    EFI runtime services from flat physical addressing to virtual
>    addressing."
> 
> So it is pretty clear that calling SetVirtualAddressMap() is entirely
> optional, and so there is no point in doing so unless it achieves
> anything useful for us.
> 
> This is not the case for 64-bit ARM. The native mapping used by the OS
> is arbitrarily converted into another permutation of userland addresses
> (i.e., bits [63:48] cleared), and the runtime code could easily deal
> with the original layout in exactly the same way as it deals with the
> converted layout. However, due to constraints related to page size
> differences if the OS is not running with 4k pages, and related to
> systems that may expose the individual sections of PE/COFF runtime
> modules as different memory regions, creating the virtual layout is a
> bit fiddly, and requires us to sort the memory map and reason about
> adjacent regions with identical memory types etc etc.
> 
> So the obvious fix is to stop calling SetVirtualAddressMap() altogether
> on arm64 systems. However, to avoid surprises, which are notoriously
> hard to diagnose when it comes to OS<->firmware interactions, let's
> start by making it an opt-out feature, and implement support for the
> 'efi=novamap' kernel command line parameter on ARM and arm64 systems.
> 
> (Note that 32-bit ARM generally does require SetVirtualAddressMap() to be
> used, given that the physical memory map and the kernel virtual address
> map are not guaranteed to be non-overlapping like on arm64. However,
> having support for efi=novamap,noruntime on 32-bit ARM, combined with
> the recently proposed support for earlycon=efi, is likely to be useful
> to diagnose boot issues on such systems if they have no accessible serial
> port)
> 
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---

I threw this at msm8998 (arm64), and it seem to work fine as far as I 
can tell.

For what it's worth:
Tested-by: Jeffrey Hugo <jhugo@codeaurora.org>

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

Re: [PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Ard Biesheuvel]

On Mon, 28 Jan 2019 at 19:04, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>
> On 1/26/2019 3:22 AM, Ard Biesheuvel wrote:
> > The UEFI spec revision 2.7 errata A section 8.4 has the following to
> > say about the virtual memory runtime services:
> >
> >    "This section contains function definitions for the virtual memory
> >    support that may be optionally used by an operating system at runtime.
> >    If an operating system chooses to make EFI runtime service calls in a
> >    virtual addressing mode instead of the flat physical mode, then the
> >    operating system must use the services in this section to switch the
> >    EFI runtime services from flat physical addressing to virtual
> >    addressing."
> >
> > So it is pretty clear that calling SetVirtualAddressMap() is entirely
> > optional, and so there is no point in doing so unless it achieves
> > anything useful for us.
> >
> > This is not the case for 64-bit ARM. The native mapping used by the OS
> > is arbitrarily converted into another permutation of userland addresses
> > (i.e., bits [63:48] cleared), and the runtime code could easily deal
> > with the original layout in exactly the same way as it deals with the
> > converted layout. However, due to constraints related to page size
> > differences if the OS is not running with 4k pages, and related to
> > systems that may expose the individual sections of PE/COFF runtime
> > modules as different memory regions, creating the virtual layout is a
> > bit fiddly, and requires us to sort the memory map and reason about
> > adjacent regions with identical memory types etc etc.
> >
> > So the obvious fix is to stop calling SetVirtualAddressMap() altogether
> > on arm64 systems. However, to avoid surprises, which are notoriously
> > hard to diagnose when it comes to OS<->firmware interactions, let's
> > start by making it an opt-out feature, and implement support for the
> > 'efi=novamap' kernel command line parameter on ARM and arm64 systems.
> >
> > (Note that 32-bit ARM generally does require SetVirtualAddressMap() to be
> > used, given that the physical memory map and the kernel virtual address
> > map are not guaranteed to be non-overlapping like on arm64. However,
> > having support for efi=novamap,noruntime on 32-bit ARM, combined with
> > the recently proposed support for earlycon=efi, is likely to be useful
> > to diagnose boot issues on such systems if they have no accessible serial
> > port)
> >
> > Cc: Alexander Graf <agraf@suse.de>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
>
> I threw this at msm8998 (arm64), and it seem to work fine as far as I
> can tell.
>
> For what it's worth:
> Tested-by: Jeffrey Hugo <jhugo@codeaurora.org>
>

Thanks a lot.

Unless anyone has any objections, I'll get this queued up for v5.1 by
the end of the week.

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

Re: [PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Bjorn Andersson]

On Sat 26 Jan 02:22 PST 2019, Ard Biesheuvel wrote:

> The UEFI spec revision 2.7 errata A section 8.4 has the following to
> say about the virtual memory runtime services:
> 
>   "This section contains function definitions for the virtual memory
>   support that may be optionally used by an operating system at runtime.
>   If an operating system chooses to make EFI runtime service calls in a
>   virtual addressing mode instead of the flat physical mode, then the
>   operating system must use the services in this section to switch the
>   EFI runtime services from flat physical addressing to virtual
>   addressing."
> 
> So it is pretty clear that calling SetVirtualAddressMap() is entirely
> optional, and so there is no point in doing so unless it achieves
> anything useful for us.
> 
> This is not the case for 64-bit ARM. The native mapping used by the OS
> is arbitrarily converted into another permutation of userland addresses
> (i.e., bits [63:48] cleared), and the runtime code could easily deal
> with the original layout in exactly the same way as it deals with the
> converted layout. However, due to constraints related to page size
> differences if the OS is not running with 4k pages, and related to
> systems that may expose the individual sections of PE/COFF runtime
> modules as different memory regions, creating the virtual layout is a
> bit fiddly, and requires us to sort the memory map and reason about
> adjacent regions with identical memory types etc etc.
> 
> So the obvious fix is to stop calling SetVirtualAddressMap() altogether
> on arm64 systems. However, to avoid surprises, which are notoriously
> hard to diagnose when it comes to OS<->firmware interactions, let's
> start by making it an opt-out feature, and implement support for the
> 'efi=novamap' kernel command line parameter on ARM and arm64 systems.
> 
> (Note that 32-bit ARM generally does require SetVirtualAddressMap() to be
> used, given that the physical memory map and the kernel virtual address
> map are not guaranteed to be non-overlapping like on arm64. However,
> having support for efi=novamap,noruntime on 32-bit ARM, combined with
> the recently proposed support for earlycon=efi, is likely to be useful
> to diagnose boot issues on such systems if they have no accessible serial
> port)
> 
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

This makes the SDM850 bootable, thanks.

Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/firmware/efi/libstub/arm-stub.c        |  5 +++++
>  drivers/firmware/efi/libstub/efi-stub-helper.c | 10 ++++++++++
>  drivers/firmware/efi/libstub/efistub.h         |  1 +
>  drivers/firmware/efi/libstub/fdt.c             |  3 +++
>  4 files changed, 19 insertions(+)
> 
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index eee42d5e25ee..626ec4b4a664 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -370,6 +370,11 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
>  		paddr = in->phys_addr;
>  		size = in->num_pages * EFI_PAGE_SIZE;
>  
> +		if (novamap()) {
> +			in->virt_addr = in->phys_addr;
> +			continue;
> +		}
> +
>  		/*
>  		 * Make the mapping compatible with 64k pages: this allows
>  		 * a 4k page size kernel to kexec a 64k page size kernel and
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index e94975f4655b..442f51c2a53d 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -34,6 +34,7 @@ static unsigned long __chunk_size = EFI_READ_CHUNK_SIZE;
>  
>  static int __section(.data) __nokaslr;
>  static int __section(.data) __quiet;
> +static int __section(.data) __novamap;
>  
>  int __pure nokaslr(void)
>  {
> @@ -43,6 +44,10 @@ int __pure is_quiet(void)
>  {
>  	return __quiet;
>  }
> +int __pure novamap(void)
> +{
> +	return __novamap;
> +}
>  
>  #define EFI_MMAP_NR_SLACK_SLOTS	8
>  
> @@ -482,6 +487,11 @@ efi_status_t efi_parse_options(char const *cmdline)
>  			__chunk_size = -1UL;
>  		}
>  
> +		if (!strncmp(str, "novamap", 7)) {
> +			str += strlen("novamap");
> +			__novamap = 1;
> +		}
> +
>  		/* Group words together, delimited by "," */
>  		while (*str && *str != ' ' && *str != ',')
>  			str++;
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 32799cf039ef..337b52c4702c 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -27,6 +27,7 @@
>  
>  extern int __pure nokaslr(void);
>  extern int __pure is_quiet(void);
> +extern int __pure novamap(void);
>  
>  #define pr_efi(sys_table, msg)		do {				\
>  	if (!is_quiet()) efi_printk(sys_table, "EFI stub: "msg);	\
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 0dc7b4987cc2..f8f89f995e9d 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -327,6 +327,9 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>  	if (status == EFI_SUCCESS) {
>  		efi_set_virtual_address_map_t *svam;
>  
> +		if (novamap())
> +			return EFI_SUCCESS;
> +
>  		/* Install the new virtual address map */
>  		svam = sys_table->runtime->set_virtual_address_map;
>  		status = svam(runtime_entry_count * desc_size, desc_size,
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

Re: [PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Ard Biesheuvel]

On Wed, 30 Jan 2019 at 01:06, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Sat 26 Jan 02:22 PST 2019, Ard Biesheuvel wrote:
>
> > The UEFI spec revision 2.7 errata A section 8.4 has the following to
> > say about the virtual memory runtime services:
> >
> >   "This section contains function definitions for the virtual memory
> >   support that may be optionally used by an operating system at runtime.
> >   If an operating system chooses to make EFI runtime service calls in a
> >   virtual addressing mode instead of the flat physical mode, then the
> >   operating system must use the services in this section to switch the
> >   EFI runtime services from flat physical addressing to virtual
> >   addressing."
> >
> > So it is pretty clear that calling SetVirtualAddressMap() is entirely
> > optional, and so there is no point in doing so unless it achieves
> > anything useful for us.
> >
> > This is not the case for 64-bit ARM. The native mapping used by the OS
> > is arbitrarily converted into another permutation of userland addresses
> > (i.e., bits [63:48] cleared), and the runtime code could easily deal
> > with the original layout in exactly the same way as it deals with the
> > converted layout. However, due to constraints related to page size
> > differences if the OS is not running with 4k pages, and related to
> > systems that may expose the individual sections of PE/COFF runtime
> > modules as different memory regions, creating the virtual layout is a
> > bit fiddly, and requires us to sort the memory map and reason about
> > adjacent regions with identical memory types etc etc.
> >
> > So the obvious fix is to stop calling SetVirtualAddressMap() altogether
> > on arm64 systems. However, to avoid surprises, which are notoriously
> > hard to diagnose when it comes to OS<->firmware interactions, let's
> > start by making it an opt-out feature, and implement support for the
> > 'efi=novamap' kernel command line parameter on ARM and arm64 systems.
> >
> > (Note that 32-bit ARM generally does require SetVirtualAddressMap() to be
> > used, given that the physical memory map and the kernel virtual address
> > map are not guaranteed to be non-overlapping like on arm64. However,
> > having support for efi=novamap,noruntime on 32-bit ARM, combined with
> > the recently proposed support for earlycon=efi, is likely to be useful
> > to diagnose boot issues on such systems if they have no accessible serial
> > port)
> >
> > Cc: Alexander Graf <agraf@suse.de>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> This makes the SDM850 bootable, thanks.
>
> Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>

Thanks Bjorn

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

Re: [PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Will Deacon]

Hi Ard,

On Sat, Jan 26, 2019 at 11:22:07AM +0100, Ard Biesheuvel wrote:
> The UEFI spec revision 2.7 errata A section 8.4 has the following to
> say about the virtual memory runtime services:
> 
>   "This section contains function definitions for the virtual memory
>   support that may be optionally used by an operating system at runtime.
>   If an operating system chooses to make EFI runtime service calls in a
>   virtual addressing mode instead of the flat physical mode, then the
>   operating system must use the services in this section to switch the
>   EFI runtime services from flat physical addressing to virtual
>   addressing."

I should probably go RTFM, but what does UEFI say about the attributes of
"flat physical addressing"? The wording above implies to me that it should
act as though the stage-1 MMU is disabled because it's described as an
alternative to virtual addressing.

If we move in a direction where we avoid calling SetVirtualAddressMap() by
default on arm64, isn't there a real threat that this firmware call will no
longer be validated? Do we need to worry about that?

Finally, Bjorn said that SDM850 is unbootable without this change. Why is
that?

Will

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

Re: [PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Ard Biesheuvel]

On Wed, 30 Jan 2019 at 19:19, Will Deacon <will.deacon@arm.com> wrote:
>
> Hi Ard,
>

Hi Will,

> On Sat, Jan 26, 2019 at 11:22:07AM +0100, Ard Biesheuvel wrote:
> > The UEFI spec revision 2.7 errata A section 8.4 has the following to
> > say about the virtual memory runtime services:
> >
> >   "This section contains function definitions for the virtual memory
> >   support that may be optionally used by an operating system at runtime.
> >   If an operating system chooses to make EFI runtime service calls in a
> >   virtual addressing mode instead of the flat physical mode, then the
> >   operating system must use the services in this section to switch the
> >   EFI runtime services from flat physical addressing to virtual
> >   addressing."
>
> I should probably go RTFM, but what does UEFI say about the attributes of
> "flat physical addressing"? The wording above implies to me that it should
> act as though the stage-1 MMU is disabled because it's described as an
> alternative to virtual addressing.
>

No. Flat physical addressing applies to the translation, not to the
memory attributes. The UEFI spec is pretty detailed when it comes to
how the MMU should be configured and which memory attributes should be
used while running in the firmware, and while running under the OS,
and that is orthogonal.

> If we move in a direction where we avoid calling SetVirtualAddressMap() by
> default on arm64, isn't there a real threat that this firmware call will no
> longer be validated? Do we need to worry about that?
>

Yes. From a validation pov, it is useful to have both options, since
optional functionality should work as specified, but it should also be
truly optional, and with this change, we can at least validate that.

Whether this will ever become the default or not is something we can
debate at a later date.

> Finally, Bjorn said that SDM850 is unbootable without this change. Why is
> that?
>

sdm835 and sdm850 are used in Windows on ARM laptops, and Windows
behaves differently in this regard. The main issue is that the virtual
mapping that Windows installs (which is a kernel mapping) is already
active when it makes the call to SetVirtualAddressMap(), which means
drivers can get away with accessing both the old and the new mapping
in the context of that call. On Linux, the new VA mapping is not
mapped yet, and so the code crashes.

Another problem is that the PE/COFF relocation processing requires
that the code sections are mapped read-write after ExitBootServices(),
and the QC firmware is relying on Windows to clear the r/o
permissions, rather than clear the r/o bits itself.

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

Re: [PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Lee Jones]

On Sat, 26 Jan 2019, Ard Biesheuvel wrote:

> The UEFI spec revision 2.7 errata A section 8.4 has the following to
> say about the virtual memory runtime services:
> 
>   "This section contains function definitions for the virtual memory
>   support that may be optionally used by an operating system at runtime.
>   If an operating system chooses to make EFI runtime service calls in a
>   virtual addressing mode instead of the flat physical mode, then the
>   operating system must use the services in this section to switch the
>   EFI runtime services from flat physical addressing to virtual
>   addressing."
> 
> So it is pretty clear that calling SetVirtualAddressMap() is entirely
> optional, and so there is no point in doing so unless it achieves
> anything useful for us.
> 
> This is not the case for 64-bit ARM. The native mapping used by the OS
> is arbitrarily converted into another permutation of userland addresses
> (i.e., bits [63:48] cleared), and the runtime code could easily deal
> with the original layout in exactly the same way as it deals with the
> converted layout. However, due to constraints related to page size
> differences if the OS is not running with 4k pages, and related to
> systems that may expose the individual sections of PE/COFF runtime
> modules as different memory regions, creating the virtual layout is a
> bit fiddly, and requires us to sort the memory map and reason about
> adjacent regions with identical memory types etc etc.
> 
> So the obvious fix is to stop calling SetVirtualAddressMap() altogether
> on arm64 systems. However, to avoid surprises, which are notoriously
> hard to diagnose when it comes to OS<->firmware interactions, let's
> start by making it an opt-out feature, and implement support for the
> 'efi=novamap' kernel command line parameter on ARM and arm64 systems.
> 
> (Note that 32-bit ARM generally does require SetVirtualAddressMap() to be
> used, given that the physical memory map and the kernel virtual address
> map are not guaranteed to be non-overlapping like on arm64. However,
> having support for efi=novamap,noruntime on 32-bit ARM, combined with
> the recently proposed support for earlycon=efi, is likely to be useful
> to diagnose boot issues on such systems if they have no accessible serial
> port)
> 
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Tested-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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