[U-Boot] [PATCH v3 0/2] efi_loader: Patch RTS at ExitBootServices

[U-Boot] [PATCH v3 0/2] efi_loader: Patch RTS at ExitBootServices

[Alexander Graf]

While discussing something compeltely different, Ard pointed out
that it might be legal to omit calling SetVirtualAddressMap altogether.

While that sounds great, we currently rely on that call to remove
all function pointers to code that we do not support outside of
boot services.

So let's patch out those bits already on the call to ExitBootServices,
so that we can successfully run even when an OS chooses to omit
any call to SetVirtualAddressMap.

---

v1 -> v2:

  - Add missing icache invalidation
  - New patch: x86: Add efi runtime reset

v2 -> v3:

  - Add link to upstream Linux patch
  - support EFI_RESET_PLATFORM_SPECIFIC
  - reuse existing x86_sysreset_request() function

Alexander Graf (2):
  x86: Add efi runtime reset
  efi_loader: Patch non-runtime code out at ExitBootServices already

 drivers/sysreset/sysreset_x86.c | 23 ++++++++++++++++++++++-
 include/efi_loader.h            |  2 ++
 lib/efi_loader/efi_boottime.c   |  1 +
 lib/efi_loader/efi_runtime.c    | 29 ++++++++++++++++++++---------
 4 files changed, 45 insertions(+), 10 deletions(-)

-- 
2.12.3

[U-Boot] [PATCH v3 1/2] x86: Add efi runtime reset

[Alexander Graf]

Our selftest will soon test the actual runtime reset function rather than
the boot time one. For this, we need to ensure that the runtime version
actually succeeds on x86 to keep our travis tests work.

So this patch implements an x86 runtime reset function. It is missing
shutdown functionality today, but OSs usually implement that via ACPI
and this function does more than the stub from before, so it's at least
an improvement.

Eventually we will want to have full DM functionality in runtime services.
But this fixes a travis failure and doesn't clutter the code too heavily, so
we should pull it in without the amazing new RTS DM framework.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v2 -> v3:

  - support EFI_RESET_PLATFORM_SPECIFIC
  - reuse existing x86_sysreset_request() function
---
 drivers/sysreset/sysreset_x86.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
index 20b958cfd4..009f376602 100644
--- a/drivers/sysreset/sysreset_x86.c
+++ b/drivers/sysreset/sysreset_x86.c
@@ -10,8 +10,10 @@
 #include <sysreset.h>
 #include <asm/io.h>
 #include <asm/processor.h>
+#include <efi_loader.h>
 
-static int x86_sysreset_request(struct udevice *dev, enum sysreset_t type)
+static __efi_runtime int x86_sysreset_request(struct udevice *dev,
+					      enum sysreset_t type)
 {
 	int value;
 
@@ -31,6 +33,25 @@ static int x86_sysreset_request(struct udevice *dev, enum sysreset_t type)
 	return -EINPROGRESS;
 }
 
+#ifdef CONFIG_EFI_LOADER
+void __efi_runtime EFIAPI efi_reset_system(
+			enum efi_reset_type reset_type,
+			efi_status_t reset_status,
+			unsigned long data_size, void *reset_data)
+{
+	if (reset_type == EFI_RESET_COLD ||
+		 reset_type == EFI_RESET_PLATFORM_SPECIFIC)
+		x86_sysreset_request(NULL, SYSRESET_COLD);
+	else if (reset_type == EFI_RESET_WARM)
+		x86_sysreset_request(NULL, SYSRESET_WARM);
+
+	/* TODO EFI_RESET_SHUTDOWN */
+
+	while (1) { }
+}
+#endif
+
+
 static const struct udevice_id x86_sysreset_ids[] = {
 	{ .compatible = "x86,reset" },
 	{ }
-- 
2.12.3

[U-Boot] [PATCH v3 2/2] efi_loader: Patch non-runtime code out at ExitBootServices already

[Alexander Graf]

While discussing something compeltely different, Ard pointed out
that it might be legal to omit calling SetVirtualAddressMap altogether.

There is even a patch on the Linux Kernel Mailing List that implements
such behavior by now:

  https://patchwork.kernel.org/patch/10782393/

While that sounds great, we currently rely on the SetVirtualAddressMap
call to remove all references to code that would not workoutside of
boot services.

So let's patch out those bits already on the call to ExitBootServices,
so that we can successfully run even when an OS chooses to omit
any call to SetVirtualAddressMap.

Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - Add missing icache invalidation

v2 -> v3:

  - Add link to upstream Linux patch
---
 include/efi_loader.h          |  2 ++
 lib/efi_loader/efi_boottime.c |  1 +
 lib/efi_loader/efi_runtime.c  | 29 ++++++++++++++++++++---------
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 9dd933dae7..2a40b09693 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -310,6 +310,8 @@ void efi_save_gd(void);
 void efi_restore_gd(void);
 /* Call this to relocate the runtime section to an address space */
 void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map);
+/* Call this when we start to live in a runtime only world */
+void efi_runtime_detach(ulong offset);
 /* Call this to set the current device name */
 void efi_set_bootdev(const char *dev, const char *devnr, const char *path);
 /* Add a new object to the object list. */
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index fc26d6adc1..8cb2979bab 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1917,6 +1917,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
 	bootm_disable_interrupts();
 
 	/* Disable boot time services */
+	efi_runtime_detach((ulong)gd->relocaddr);
 	systab.con_in_handle = NULL;
 	systab.con_in = NULL;
 	systab.con_out_handle = NULL;
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 636dfdab39..17d22d429e 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -276,16 +276,12 @@ struct efi_runtime_detach_list_struct {
 	void *patchto;
 };
 
-static const struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = {
+static struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = {
 	{
 		/* do_reset is gone */
 		.ptr = &efi_runtime_services.reset_system,
 		.patchto = efi_reset_system,
 	}, {
-		/* invalidate_*cache_all are gone */
-		.ptr = &efi_runtime_services.set_virtual_address_map,
-		.patchto = &efi_unimplemented,
-	}, {
 		/* RTC accessors are gone */
 		.ptr = &efi_runtime_services.get_time,
 		.patchto = &efi_get_time,
@@ -328,7 +324,15 @@ static bool efi_runtime_tobedetached(void *p)
 	return false;
 }
 
-static void efi_runtime_detach(ulong offset)
+/**
+ * efi_runtime_detach() - Remove any dependency on non-runtime sections
+ *
+ * This function patches all remaining code to be self-sufficient inside
+ * runtime sections. Any calls to non-runtime will be removed after this.
+ *
+ * @offset:		relocaddr for pre-set_v_a_space, offset to VA after
+ */
+__efi_runtime void efi_runtime_detach(ulong offset)
 {
 	int i;
 	ulong patchoff = offset - (ulong)gd->relocaddr;
@@ -344,6 +348,8 @@ static void efi_runtime_detach(ulong offset)
 
 	/* Update CRC32 */
 	efi_update_table_header_crc32(&efi_runtime_services.hdr);
+
+        invalidate_icache_all();
 }
 
 /* Relocate EFI runtime to uboot_reloc_base = offset */
@@ -430,19 +436,25 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map)
  * @virtmap:		virtual address mapping information
  * Return:		status code
  */
-static efi_status_t EFIAPI efi_set_virtual_address_map(
+static __efi_runtime efi_status_t EFIAPI efi_set_virtual_address_map(
 			unsigned long memory_map_size,
 			unsigned long descriptor_size,
 			uint32_t descriptor_version,
 			struct efi_mem_desc *virtmap)
 {
+	static __efi_runtime_data bool is_patched;
 	int n = memory_map_size / descriptor_size;
 	int i;
 	int rt_code_sections = 0;
 
+	if (is_patched)
+		return EFI_INVALID_PARAMETER;
+
 	EFI_ENTRY("%lx %lx %x %p", memory_map_size, descriptor_size,
 		  descriptor_version, virtmap);
 
+	is_patched = true;
+
 	/*
 	 * TODO:
 	 * Further down we are cheating. While really we should implement
@@ -514,8 +526,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(
 					   map->physical_start + gd->relocaddr;
 
 			efi_runtime_relocate(new_offset, map);
-			/* Once we're virtual, we can no longer handle
-			   complex callbacks */
+			/* We need to repatch callbacks for their new VA */
 			efi_runtime_detach(new_offset);
 			return EFI_EXIT(EFI_SUCCESS);
 		}
-- 
2.12.3

[U-Boot] [PATCH v3 2/2] efi_loader: Patch non-runtime code out at ExitBootServices already

[Heinrich Schuchardt]

On 1/30/19 11:46 AM, Alexander Graf wrote:
> While discussing something compeltely different, Ard pointed out
> that it might be legal to omit calling SetVirtualAddressMap altogether.
> 
> There is even a patch on the Linux Kernel Mailing List that implements
> such behavior by now:
> 
>   https://patchwork.kernel.org/patch/10782393/
> 
> While that sounds great, we currently rely on the SetVirtualAddressMap
> call to remove all references to code that would not workoutside of
> boot services.
> 
> So let's patch out those bits already on the call to ExitBootServices,
> so that we can successfully run even when an OS chooses to omit
> any call to SetVirtualAddressMap.
> 
> Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> ---
> 

This patch series leads to a Python test error on qemu_arm64_defconfig:

        if m != 0:
>               raise Exception('Reset failed during the EFI selftest')
E     Exception: Reset failed during the EFI selftest

test/py/tests/test_efi_selftest.py:25: Exception

<snip />

The output looks like:

Preparing for reset. Press any key...




U-Boot 2019.01-00458-gada5205aef (Jan 30 2019 - 19:47:02 +0100)



DRAM:

<snip />

Please, adjust test_efi_selftest().

Best regards

Heinrich

> v1 -> v2:
> 
>   - Add missing icache invalidation
> 
> v2 -> v3:
> 
>   - Add link to upstream Linux patch
> ---
>  include/efi_loader.h          |  2 ++
>  lib/efi_loader/efi_boottime.c |  1 +
>  lib/efi_loader/efi_runtime.c  | 29 ++++++++++++++++++++---------
>  3 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 9dd933dae7..2a40b09693 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -310,6 +310,8 @@ void efi_save_gd(void);
>  void efi_restore_gd(void);
>  /* Call this to relocate the runtime section to an address space */
>  void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map);
> +/* Call this when we start to live in a runtime only world */
> +void efi_runtime_detach(ulong offset);
>  /* Call this to set the current device name */
>  void efi_set_bootdev(const char *dev, const char *devnr, const char *path);
>  /* Add a new object to the object list. */
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index fc26d6adc1..8cb2979bab 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1917,6 +1917,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
>  	bootm_disable_interrupts();
>  
>  	/* Disable boot time services */
> +	efi_runtime_detach((ulong)gd->relocaddr);
>  	systab.con_in_handle = NULL;
>  	systab.con_in = NULL;
>  	systab.con_out_handle = NULL;
> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> index 636dfdab39..17d22d429e 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -276,16 +276,12 @@ struct efi_runtime_detach_list_struct {
>  	void *patchto;
>  };
>  
> -static const struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = {
> +static struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = {
>  	{
>  		/* do_reset is gone */
>  		.ptr = &efi_runtime_services.reset_system,
>  		.patchto = efi_reset_system,
>  	}, {
> -		/* invalidate_*cache_all are gone */
> -		.ptr = &efi_runtime_services.set_virtual_address_map,
> -		.patchto = &efi_unimplemented,
> -	}, {
>  		/* RTC accessors are gone */
>  		.ptr = &efi_runtime_services.get_time,
>  		.patchto = &efi_get_time,
> @@ -328,7 +324,15 @@ static bool efi_runtime_tobedetached(void *p)
>  	return false;
>  }
>  
> -static void efi_runtime_detach(ulong offset)
> +/**
> + * efi_runtime_detach() - Remove any dependency on non-runtime sections
> + *
> + * This function patches all remaining code to be self-sufficient inside
> + * runtime sections. Any calls to non-runtime will be removed after this.
> + *
> + * @offset:		relocaddr for pre-set_v_a_space, offset to VA after
> + */
> +__efi_runtime void efi_runtime_detach(ulong offset)
>  {
>  	int i;
>  	ulong patchoff = offset - (ulong)gd->relocaddr;
> @@ -344,6 +348,8 @@ static void efi_runtime_detach(ulong offset)
>  
>  	/* Update CRC32 */
>  	efi_update_table_header_crc32(&efi_runtime_services.hdr);
> +
> +        invalidate_icache_all();
>  }
>  
>  /* Relocate EFI runtime to uboot_reloc_base = offset */
> @@ -430,19 +436,25 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map)
>   * @virtmap:		virtual address mapping information
>   * Return:		status code
>   */
> -static efi_status_t EFIAPI efi_set_virtual_address_map(
> +static __efi_runtime efi_status_t EFIAPI efi_set_virtual_address_map(
>  			unsigned long memory_map_size,
>  			unsigned long descriptor_size,
>  			uint32_t descriptor_version,
>  			struct efi_mem_desc *virtmap)
>  {
> +	static __efi_runtime_data bool is_patched;
>  	int n = memory_map_size / descriptor_size;
>  	int i;
>  	int rt_code_sections = 0;
>  
> +	if (is_patched)
> +		return EFI_INVALID_PARAMETER;
> +
>  	EFI_ENTRY("%lx %lx %x %p", memory_map_size, descriptor_size,
>  		  descriptor_version, virtmap);
>  
> +	is_patched = true;
> +
>  	/*
>  	 * TODO:
>  	 * Further down we are cheating. While really we should implement
> @@ -514,8 +526,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(
>  					   map->physical_start + gd->relocaddr;
>  
>  			efi_runtime_relocate(new_offset, map);
> -			/* Once we're virtual, we can no longer handle
> -			   complex callbacks */
> +			/* We need to repatch callbacks for their new VA */
>  			efi_runtime_detach(new_offset);
>  			return EFI_EXIT(EFI_SUCCESS);
>  		}
> 

[U-Boot] [PATCH v3 1/2] x86: Add efi runtime reset

[Heinrich Schuchardt]

On 1/30/19 11:46 AM, Alexander Graf wrote:
> Our selftest will soon test the actual runtime reset function rather than
> the boot time one. For this, we need to ensure that the runtime version
> actually succeeds on x86 to keep our travis tests work.
> 
> So this patch implements an x86 runtime reset function. It is missing
> shutdown functionality today, but OSs usually implement that via ACPI
> and this function does more than the stub from before, so it's at least
> an improvement.
> 
> Eventually we will want to have full DM functionality in runtime services.
> But this fixes a travis failure and doesn't clutter the code too heavily, so
> we should pull it in without the amazing new RTS DM framework.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> ---
> 
> v2 -> v3:
> 
>   - support EFI_RESET_PLATFORM_SPECIFIC
>   - reuse existing x86_sysreset_request() function

The v2->v3 update does not answer the question if the reset is correctly
implemented. We would not want to call a function we do not trust.

@Simon, Bin:
x86_sysreset_request() loosely resembles BOOT_CF9_SAFE in
native_machine_emergency_restart() in Linux arch/x86/kernel/reboot.c
which is tried before using the keyboard controller as last resort.

u8 reboot_code = reboot_mode == REBOOT_WARM ?  0x06 : 0x0E;
u8 cf9 = inb(0xcf9) & ~reboot_code;
outb(cf9|2, 0xcf9); /* Request hard reset */
udelay(50);
/* Actually do the reset */
outb(cf9|reboot_code, 0xcf9);
udelay(50);

So the Kernel first switches bit 2 off and bit 1 on, waits, and then
switches bit 2 on, cf.
http://smackerelofopinion.blogspot.com/2011/02/resetting-pc-using-reset-control.html

Shouldn't we do it the same way as the Kernel does it?

Best regards

Heinrich

> ---
>  drivers/sysreset/sysreset_x86.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
> index 20b958cfd4..009f376602 100644
> --- a/drivers/sysreset/sysreset_x86.c
> +++ b/drivers/sysreset/sysreset_x86.c
> @@ -10,8 +10,10 @@
>  #include <sysreset.h>
>  #include <asm/io.h>
>  #include <asm/processor.h>
> +#include <efi_loader.h>
>  
> -static int x86_sysreset_request(struct udevice *dev, enum sysreset_t type)
> +static __efi_runtime int x86_sysreset_request(struct udevice *dev,
> +					      enum sysreset_t type)
>  {
>  	int value;
>  
> @@ -31,6 +33,25 @@ static int x86_sysreset_request(struct udevice *dev, enum sysreset_t type)
>  	return -EINPROGRESS;
>  }
>  
> +#ifdef CONFIG_EFI_LOADER
> +void __efi_runtime EFIAPI efi_reset_system(
> +			enum efi_reset_type reset_type,
> +			efi_status_t reset_status,
> +			unsigned long data_size, void *reset_data)
> +{
> +	if (reset_type == EFI_RESET_COLD ||
> +		 reset_type == EFI_RESET_PLATFORM_SPECIFIC)
> +		x86_sysreset_request(NULL, SYSRESET_COLD);
> +	else if (reset_type == EFI_RESET_WARM)
> +		x86_sysreset_request(NULL, SYSRESET_WARM);
> +
> +	/* TODO EFI_RESET_SHUTDOWN */
> +
> +	while (1) { }
> +}
> +#endif
> +
> +
>  static const struct udevice_id x86_sysreset_ids[] = {
>  	{ .compatible = "x86,reset" },
>  	{ }
> 

[U-Boot] [PATCH v3 1/2] x86: Add efi runtime reset

[Simon Glass]

Hi,

On Wed, 30 Jan 2019 at 12:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 1/30/19 11:46 AM, Alexander Graf wrote:
> > Our selftest will soon test the actual runtime reset function rather than
> > the boot time one. For this, we need to ensure that the runtime version
> > actually succeeds on x86 to keep our travis tests work.
> >
> > So this patch implements an x86 runtime reset function. It is missing
> > shutdown functionality today, but OSs usually implement that via ACPI
> > and this function does more than the stub from before, so it's at least
> > an improvement.
> >
> > Eventually we will want to have full DM functionality in runtime services.
> > But this fixes a travis failure and doesn't clutter the code too heavily, so
> > we should pull it in without the amazing new RTS DM framework.
> >
> > Signed-off-by: Alexander Graf <agraf@suse.de>
> >
> > ---
> >
> > v2 -> v3:
> >
> >   - support EFI_RESET_PLATFORM_SPECIFIC
> >   - reuse existing x86_sysreset_request() function
>
> The v2->v3 update does not answer the question if the reset is correctly
> implemented. We would not want to call a function we do not trust.
>
> @Simon, Bin:
> x86_sysreset_request() loosely resembles BOOT_CF9_SAFE in
> native_machine_emergency_restart() in Linux arch/x86/kernel/reboot.c
> which is tried before using the keyboard controller as last resort.
>
> u8 reboot_code = reboot_mode == REBOOT_WARM ?  0x06 : 0x0E;
> u8 cf9 = inb(0xcf9) & ~reboot_code;
> outb(cf9|2, 0xcf9); /* Request hard reset */
> udelay(50);
> /* Actually do the reset */
> outb(cf9|reboot_code, 0xcf9);
> udelay(50);
>
> So the Kernel first switches bit 2 off and bit 1 on, waits, and then
> switches bit 2 on, cf.
> http://smackerelofopinion.blogspot.com/2011/02/resetting-pc-using-reset-control.html
>
> Shouldn't we do it the same way as the Kernel does it?

I suspect so, but Bin is the expert.

As to this patch, it perpetuates the current EFI run-time approach in
U-Boot so I'm not sure this is the right path.

Regards,
Simon

[U-Boot] [PATCH v3 1/2] x86: Add efi runtime reset

[Alexander Graf]

> Am 31.01.2019 um 02:24 schrieb Simon Glass <sjg@chromium.org>:
> 
> Hi,
> 
>> On Wed, 30 Jan 2019 at 12:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> 
>>> On 1/30/19 11:46 AM, Alexander Graf wrote:
>>> Our selftest will soon test the actual runtime reset function rather than
>>> the boot time one. For this, we need to ensure that the runtime version
>>> actually succeeds on x86 to keep our travis tests work.
>>> 
>>> So this patch implements an x86 runtime reset function. It is missing
>>> shutdown functionality today, but OSs usually implement that via ACPI
>>> and this function does more than the stub from before, so it's at least
>>> an improvement.
>>> 
>>> Eventually we will want to have full DM functionality in runtime services.
>>> But this fixes a travis failure and doesn't clutter the code too heavily, so
>>> we should pull it in without the amazing new RTS DM framework.
>>> 
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> 
>>> ---
>>> 
>>> v2 -> v3:
>>> 
>>>  - support EFI_RESET_PLATFORM_SPECIFIC
>>>  - reuse existing x86_sysreset_request() function
>> 
>> The v2->v3 update does not answer the question if the reset is correctly
>> implemented. We would not want to call a function we do not trust.
>> 
>> @Simon, Bin:
>> x86_sysreset_request() loosely resembles BOOT_CF9_SAFE in
>> native_machine_emergency_restart() in Linux arch/x86/kernel/reboot.c
>> which is tried before using the keyboard controller as last resort.
>> 
>> u8 reboot_code = reboot_mode == REBOOT_WARM ?  0x06 : 0x0E;
>> u8 cf9 = inb(0xcf9) & ~reboot_code;
>> outb(cf9|2, 0xcf9); /* Request hard reset */
>> udelay(50);
>> /* Actually do the reset */
>> outb(cf9|reboot_code, 0xcf9);
>> udelay(50);
>> 
>> So the Kernel first switches bit 2 off and bit 1 on, waits, and then
>> switches bit 2 on, cf.
>> http://smackerelofopinion.blogspot.com/2011/02/resetting-pc-using-reset-control.html
>> 
>> Shouldn't we do it the same way as the Kernel does it?
> 
> I suspect so, but Bin is the expert.
> 
> As to this patch, it perpetuates the current EFI run-time approach in
> U-Boot so I'm not sure this is the right path.

It is the right short- to mid-term plan. I rather have a shed I can live in today than a palace in 5 years :). Well, not true, I probably want both :).


Alex

[U-Boot] [PATCH v3 1/2] x86: Add efi runtime reset

[Bin Meng]

On Thu, Jan 31, 2019 at 9:24 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On Wed, 30 Jan 2019 at 12:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 1/30/19 11:46 AM, Alexander Graf wrote:
> > > Our selftest will soon test the actual runtime reset function rather than
> > > the boot time one. For this, we need to ensure that the runtime version
> > > actually succeeds on x86 to keep our travis tests work.
> > >
> > > So this patch implements an x86 runtime reset function. It is missing
> > > shutdown functionality today, but OSs usually implement that via ACPI
> > > and this function does more than the stub from before, so it's at least
> > > an improvement.
> > >
> > > Eventually we will want to have full DM functionality in runtime services.
> > > But this fixes a travis failure and doesn't clutter the code too heavily, so
> > > we should pull it in without the amazing new RTS DM framework.
> > >
> > > Signed-off-by: Alexander Graf <agraf@suse.de>
> > >
> > > ---
> > >
> > > v2 -> v3:
> > >
> > >   - support EFI_RESET_PLATFORM_SPECIFIC
> > >   - reuse existing x86_sysreset_request() function
> >
> > The v2->v3 update does not answer the question if the reset is correctly
> > implemented. We would not want to call a function we do not trust.
> >
> > @Simon, Bin:
> > x86_sysreset_request() loosely resembles BOOT_CF9_SAFE in
> > native_machine_emergency_restart() in Linux arch/x86/kernel/reboot.c
> > which is tried before using the keyboard controller as last resort.
> >
> > u8 reboot_code = reboot_mode == REBOOT_WARM ?  0x06 : 0x0E;
> > u8 cf9 = inb(0xcf9) & ~reboot_code;
> > outb(cf9|2, 0xcf9); /* Request hard reset */
> > udelay(50);
> > /* Actually do the reset */
> > outb(cf9|reboot_code, 0xcf9);
> > udelay(50);
> >
> > So the Kernel first switches bit 2 off and bit 1 on, waits, and then
> > switches bit 2 on, cf.
> > http://smackerelofopinion.blogspot.com/2011/02/resetting-pc-using-reset-control.html
> >
> > Shouldn't we do it the same way as the Kernel does it?
>
> I suspect so, but Bin is the expert.
>

What U-Boot does is essentially the same as Linux but a simplified
version, because bit 2 is 0 any way. If it were 0, the system should
have been reset already then there is no chance to execute the reset
sequence at all.

> As to this patch, it perpetuates the current EFI run-time approach in
> U-Boot so I'm not sure this is the right path.
>

Regards,
Bin

[U-Boot] [PATCH v3 1/2] x86: Add efi runtime reset

[Bin Meng]

On Thu, Jan 31, 2019 at 3:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Jan 31, 2019 at 9:24 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, 30 Jan 2019 at 12:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > > On 1/30/19 11:46 AM, Alexander Graf wrote:
> > > > Our selftest will soon test the actual runtime reset function rather than
> > > > the boot time one. For this, we need to ensure that the runtime version
> > > > actually succeeds on x86 to keep our travis tests work.
> > > >
> > > > So this patch implements an x86 runtime reset function. It is missing
> > > > shutdown functionality today, but OSs usually implement that via ACPI
> > > > and this function does more than the stub from before, so it's at least
> > > > an improvement.
> > > >
> > > > Eventually we will want to have full DM functionality in runtime services.
> > > > But this fixes a travis failure and doesn't clutter the code too heavily, so
> > > > we should pull it in without the amazing new RTS DM framework.
> > > >
> > > > Signed-off-by: Alexander Graf <agraf@suse.de>
> > > >
> > > > ---
> > > >
> > > > v2 -> v3:
> > > >
> > > >   - support EFI_RESET_PLATFORM_SPECIFIC
> > > >   - reuse existing x86_sysreset_request() function
> > >
> > > The v2->v3 update does not answer the question if the reset is correctly
> > > implemented. We would not want to call a function we do not trust.
> > >
> > > @Simon, Bin:
> > > x86_sysreset_request() loosely resembles BOOT_CF9_SAFE in
> > > native_machine_emergency_restart() in Linux arch/x86/kernel/reboot.c
> > > which is tried before using the keyboard controller as last resort.
> > >
> > > u8 reboot_code = reboot_mode == REBOOT_WARM ?  0x06 : 0x0E;
> > > u8 cf9 = inb(0xcf9) & ~reboot_code;
> > > outb(cf9|2, 0xcf9); /* Request hard reset */
> > > udelay(50);
> > > /* Actually do the reset */
> > > outb(cf9|reboot_code, 0xcf9);
> > > udelay(50);
> > >
> > > So the Kernel first switches bit 2 off and bit 1 on, waits, and then
> > > switches bit 2 on, cf.
> > > http://smackerelofopinion.blogspot.com/2011/02/resetting-pc-using-reset-control.html
> > >
> > > Shouldn't we do it the same way as the Kernel does it?
> >
> > I suspect so, but Bin is the expert.
> >
>
> What U-Boot does is essentially the same as Linux but a simplified
> version, because bit 2 is 0 any way. If it were 0, the system should

Sorry, a typo: If it were "1"

> have been reset already then there is no chance to execute the reset
> sequence at all.
>
> > As to this patch, it perpetuates the current EFI run-time approach in
> > U-Boot so I'm not sure this is the right path.
> >
>

Regards,
Bin

[U-Boot] [PATCH v3 1/2] x86: Add efi runtime reset

[Heinrich Schuchardt]

On 1/31/19 8:31 AM, Bin Meng wrote:
> On Thu, Jan 31, 2019 at 3:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> On Thu, Jan 31, 2019 at 9:24 AM Simon Glass <sjg@chromium.org> wrote:
>>>
>>> Hi,
>>>
>>> On Wed, 30 Jan 2019 at 12:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On 1/30/19 11:46 AM, Alexander Graf wrote:
>>>>> Our selftest will soon test the actual runtime reset function rather than
>>>>> the boot time one. For this, we need to ensure that the runtime version
>>>>> actually succeeds on x86 to keep our travis tests work.
>>>>>
>>>>> So this patch implements an x86 runtime reset function. It is missing
>>>>> shutdown functionality today, but OSs usually implement that via ACPI
>>>>> and this function does more than the stub from before, so it's at least
>>>>> an improvement.
>>>>>
>>>>> Eventually we will want to have full DM functionality in runtime services.
>>>>> But this fixes a travis failure and doesn't clutter the code too heavily, so
>>>>> we should pull it in without the amazing new RTS DM framework.
>>>>>
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>
>>>>> ---
>>>>>
>>>>> v2 -> v3:
>>>>>
>>>>>   - support EFI_RESET_PLATFORM_SPECIFIC
>>>>>   - reuse existing x86_sysreset_request() function
>>>>
>>>> The v2->v3 update does not answer the question if the reset is correctly
>>>> implemented. We would not want to call a function we do not trust.
>>>>
>>>> @Simon, Bin:
>>>> x86_sysreset_request() loosely resembles BOOT_CF9_SAFE in
>>>> native_machine_emergency_restart() in Linux arch/x86/kernel/reboot.c
>>>> which is tried before using the keyboard controller as last resort.
>>>>
>>>> u8 reboot_code = reboot_mode == REBOOT_WARM ?  0x06 : 0x0E;
>>>> u8 cf9 = inb(0xcf9) & ~reboot_code;
>>>> outb(cf9|2, 0xcf9); /* Request hard reset */
>>>> udelay(50);
>>>> /* Actually do the reset */
>>>> outb(cf9|reboot_code, 0xcf9);
>>>> udelay(50);
>>>>
>>>> So the Kernel first switches bit 2 off and bit 1 on, waits, and then
>>>> switches bit 2 on, cf.
>>>> http://smackerelofopinion.blogspot.com/2011/02/resetting-pc-using-reset-control.html
>>>>
>>>> Shouldn't we do it the same way as the Kernel does it?
>>>
>>> I suspect so, but Bin is the expert.
>>>
>>
>> What U-Boot does is essentially the same as Linux but a simplified
>> version, because bit 2 is 0 any way. If it were 0, the system should
> 
> Sorry, a typo: If it were "1"

What happens if we reset a board multiple times?

Best regards

Heinrich

> 
>> have been reset already then there is no chance to execute the reset
>> sequence at all.
>>
>>> As to this patch, it perpetuates the current EFI run-time approach in
>>> U-Boot so I'm not sure this is the right path.
>>>
>>
> 
> Regards,
> Bin
> 

[U-Boot] [PATCH v3 1/2] x86: Add efi runtime reset

[Bin Meng]

On Thu, Jan 31, 2019 at 7:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 1/31/19 8:31 AM, Bin Meng wrote:
> > On Thu, Jan 31, 2019 at 3:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>
> >> On Thu, Jan 31, 2019 at 9:24 AM Simon Glass <sjg@chromium.org> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On Wed, 30 Jan 2019 at 12:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>
> >>>> On 1/30/19 11:46 AM, Alexander Graf wrote:
> >>>>> Our selftest will soon test the actual runtime reset function rather than
> >>>>> the boot time one. For this, we need to ensure that the runtime version
> >>>>> actually succeeds on x86 to keep our travis tests work.
> >>>>>
> >>>>> So this patch implements an x86 runtime reset function. It is missing
> >>>>> shutdown functionality today, but OSs usually implement that via ACPI
> >>>>> and this function does more than the stub from before, so it's at least
> >>>>> an improvement.
> >>>>>
> >>>>> Eventually we will want to have full DM functionality in runtime services.
> >>>>> But this fixes a travis failure and doesn't clutter the code too heavily, so
> >>>>> we should pull it in without the amazing new RTS DM framework.
> >>>>>
> >>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>>>
> >>>>> ---
> >>>>>
> >>>>> v2 -> v3:
> >>>>>
> >>>>>   - support EFI_RESET_PLATFORM_SPECIFIC
> >>>>>   - reuse existing x86_sysreset_request() function
> >>>>
> >>>> The v2->v3 update does not answer the question if the reset is correctly
> >>>> implemented. We would not want to call a function we do not trust.
> >>>>
> >>>> @Simon, Bin:
> >>>> x86_sysreset_request() loosely resembles BOOT_CF9_SAFE in
> >>>> native_machine_emergency_restart() in Linux arch/x86/kernel/reboot.c
> >>>> which is tried before using the keyboard controller as last resort.
> >>>>
> >>>> u8 reboot_code = reboot_mode == REBOOT_WARM ?  0x06 : 0x0E;
> >>>> u8 cf9 = inb(0xcf9) & ~reboot_code;
> >>>> outb(cf9|2, 0xcf9); /* Request hard reset */
> >>>> udelay(50);
> >>>> /* Actually do the reset */
> >>>> outb(cf9|reboot_code, 0xcf9);
> >>>> udelay(50);
> >>>>
> >>>> So the Kernel first switches bit 2 off and bit 1 on, waits, and then
> >>>> switches bit 2 on, cf.
> >>>> http://smackerelofopinion.blogspot.com/2011/02/resetting-pc-using-reset-control.html
> >>>>
> >>>> Shouldn't we do it the same way as the Kernel does it?
> >>>
> >>> I suspect so, but Bin is the expert.
> >>>
> >>
> >> What U-Boot does is essentially the same as Linux but a simplified
> >> version, because bit 2 is 0 any way. If it were 0, the system should
> >
> > Sorry, a typo: If it were "1"
>
> What happens if we reset a board multiple times?
>

This is not a sticky bit so it is reset to default 0 after power up.

Regards,
Bin