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

[U-Boot] [PATCH v2 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

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, 46 insertions(+), 9 deletions(-)

-- 
2.12.3

[U-Boot] [PATCH v2 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.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
index 20b958cfd4..efed45ccb7 100644
--- a/drivers/sysreset/sysreset_x86.c
+++ b/drivers/sysreset/sysreset_x86.c
@@ -10,6 +10,29 @@
 #include <sysreset.h>
 #include <asm/io.h>
 #include <asm/processor.h>
+#include <efi_loader.h>
+
+#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)
+{
+	u32 value = 0;
+
+	if (reset_type == EFI_RESET_COLD)
+		value = SYS_RST | RST_CPU | FULL_RST;
+	else if (reset_type == EFI_RESET_WARM)
+		value = SYS_RST | RST_CPU;
+
+	/* TODO EFI_RESET_PLATFORM_SPECIFIC and EFI_RESET_SHUTDOWN */
+
+	if (value)
+		outb(value, IO_PORT_RESET);
+
+	while (1) { }
+}
+#endif
 
 static int x86_sysreset_request(struct udevice *dev, enum sysreset_t type)
 {
-- 
2.12.3

[U-Boot] [PATCH v2 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.

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.

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

---

v1 -> v2:

  - Add missing icache invalidation
---
 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 v2 0/2] efi_loader: Patch RTS at ExitBootServices

[Heinrich Schuchardt]

On 1/28/19 4:42 PM, Alexander Graf wrote:
> 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.

This patch series is related to a parallel patch in Linux:

efi: arm/arm64: allow SetVirtualAddressMap() to be omitted
https://www.spinics.net/lists/linux-efi/msg15457.html

Please, mention it in the next version of the patch series.

The Python test_efi_selftest() has to be adjusted:
23     m = u_boot_console.p.expect(['resetting', 'U-Boot'])

Please, add the missing patch to the series.

Best regards

Heinrich

> 
> ---
> 
> v1 -> v2:
> 
>   - Add missing icache invalidation
>   - New patch: x86: Add efi runtime reset
> 
> 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, 46 insertions(+), 9 deletions(-)
> 

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

[Heinrich Schuchardt]

On 1/28/19 4:42 PM, 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.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
> index 20b958cfd4..efed45ccb7 100644
> --- a/drivers/sysreset/sysreset_x86.c
> +++ b/drivers/sysreset/sysreset_x86.c
> @@ -10,6 +10,29 @@
>  #include <sysreset.h>
>  #include <asm/io.h>
>  #include <asm/processor.h>
> +#include <efi_loader.h>
> +
> +#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)
> +{
> +	u32 value = 0;
> +
> +	if (reset_type == EFI_RESET_COLD)
> +		value = SYS_RST | RST_CPU | FULL_RST;
> +	else if (reset_type == EFI_RESET_WARM)
> +		value = SYS_RST | RST_CPU;

It would preferable to do a reset for EFI_RESET_PLATFORM_SPECIFIC
instead of falling in to a loop.

For EFI_RESET_SHUTDOWN this patch only puts a single CPU core into an
endless loop but does not do anything for the remaining cores. Resetting
the whole CPU would be an alternative.

> +
> +	/* TODO EFI_RESET_PLATFORM_SPECIFIC and EFI_RESET_SHUTDOWN */
> +
> +	if (value)
> +		outb(value, IO_PORT_RESET);
Best regards

Heinrich

> +
> +	while (1) { }
> +}
> +#endif
>  
>  static int x86_sysreset_request(struct udevice *dev, enum sysreset_t type)
>  {
> 

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

[Simon Glass]

Hi,

On Mon, 28 Jan 2019 at 08:42, Alexander Graf <agraf@suse.de> 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.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
> index 20b958cfd4..efed45ccb7 100644
> --- a/drivers/sysreset/sysreset_x86.c
> +++ b/drivers/sysreset/sysreset_x86.c
> @@ -10,6 +10,29 @@
>  #include <sysreset.h>
>  #include <asm/io.h>
>  #include <asm/processor.h>
> +#include <efi_loader.h>
> +
> +#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)
> +{
> +       u32 value = 0;
> +
> +       if (reset_type == EFI_RESET_COLD)
> +               value = SYS_RST | RST_CPU | FULL_RST;
> +       else if (reset_type == EFI_RESET_WARM)
> +               value = SYS_RST | RST_CPU;

The EFI should use the sysreset driver and sysreset_walk() or similar,
rather than having a function called directly.

> +
> +       /* TODO EFI_RESET_PLATFORM_SPECIFIC and EFI_RESET_SHUTDOWN */
> +
> +       if (value)
> +               outb(value, IO_PORT_RESET);
> +
> +       while (1) { }
> +}
> +#endif
>
>  static int x86_sysreset_request(struct udevice *dev, enum sysreset_t type)
>  {
> --
> 2.12.3
>

Regards,
Simon

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

[Alexander Graf]

On 28.01.19 20:13, Simon Glass wrote:
> Hi,
> 
> On Mon, 28 Jan 2019 at 08:42, Alexander Graf <agraf@suse.de> 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.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
>> index 20b958cfd4..efed45ccb7 100644
>> --- a/drivers/sysreset/sysreset_x86.c
>> +++ b/drivers/sysreset/sysreset_x86.c
>> @@ -10,6 +10,29 @@
>>  #include <sysreset.h>
>>  #include <asm/io.h>
>>  #include <asm/processor.h>
>> +#include <efi_loader.h>
>> +
>> +#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)
>> +{
>> +       u32 value = 0;
>> +
>> +       if (reset_type == EFI_RESET_COLD)
>> +               value = SYS_RST | RST_CPU | FULL_RST;
>> +       else if (reset_type == EFI_RESET_WARM)
>> +               value = SYS_RST | RST_CPU;
> 
> The EFI should use the sysreset driver and sysreset_walk() or similar,
> rather than having a function called directly.

It can't. At this point all of DM is long gone. We're in runtime space here.


Alex

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

[Simon Glass]

Hi Alex,

On Mon, 28 Jan 2019 at 12:15, Alexander Graf <agraf@suse.de> wrote:
>
>
>
> On 28.01.19 20:13, Simon Glass wrote:
> > Hi,
> >
> > On Mon, 28 Jan 2019 at 08:42, Alexander Graf <agraf@suse.de> 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.
> >>
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >> ---
> >>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
> >>  1 file changed, 23 insertions(+)
> >>
> >> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
> >> index 20b958cfd4..efed45ccb7 100644
> >> --- a/drivers/sysreset/sysreset_x86.c
> >> +++ b/drivers/sysreset/sysreset_x86.c
> >> @@ -10,6 +10,29 @@
> >>  #include <sysreset.h>
> >>  #include <asm/io.h>
> >>  #include <asm/processor.h>
> >> +#include <efi_loader.h>
> >> +
> >> +#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)
> >> +{
> >> +       u32 value = 0;
> >> +
> >> +       if (reset_type == EFI_RESET_COLD)
> >> +               value = SYS_RST | RST_CPU | FULL_RST;
> >> +       else if (reset_type == EFI_RESET_WARM)
> >> +               value = SYS_RST | RST_CPU;
> >
> > The EFI should use the sysreset driver and sysreset_walk() or similar,
> > rather than having a function called directly.
>
> It can't. At this point all of DM is long gone. We're in runtime space here.

This has come up before. We'll end up with a lot of duplication if we
cannot solve this. I think the run-time code will need to be built and
linked separately so that all necessary code is pulled in.

Regards,
Simon

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

[Alexander Graf]

On 28.01.19 20:24, Simon Glass wrote:
> Hi Alex,
> 
> On Mon, 28 Jan 2019 at 12:15, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>>
>> On 28.01.19 20:13, Simon Glass wrote:
>>> Hi,
>>>
>>> On Mon, 28 Jan 2019 at 08:42, Alexander Graf <agraf@suse.de> 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.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
>>>>  1 file changed, 23 insertions(+)
>>>>
>>>> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
>>>> index 20b958cfd4..efed45ccb7 100644
>>>> --- a/drivers/sysreset/sysreset_x86.c
>>>> +++ b/drivers/sysreset/sysreset_x86.c
>>>> @@ -10,6 +10,29 @@
>>>>  #include <sysreset.h>
>>>>  #include <asm/io.h>
>>>>  #include <asm/processor.h>
>>>> +#include <efi_loader.h>
>>>> +
>>>> +#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)
>>>> +{
>>>> +       u32 value = 0;
>>>> +
>>>> +       if (reset_type == EFI_RESET_COLD)
>>>> +               value = SYS_RST | RST_CPU | FULL_RST;
>>>> +       else if (reset_type == EFI_RESET_WARM)
>>>> +               value = SYS_RST | RST_CPU;
>>>
>>> The EFI should use the sysreset driver and sysreset_walk() or similar,
>>> rather than having a function called directly.
>>
>> It can't. At this point all of DM is long gone. We're in runtime space here.
> 
> This has come up before. We'll end up with a lot of duplication if we
> cannot solve this. I think the run-time code will need to be built and
> linked separately so that all necessary code is pulled in.

It's mostly a question of size. We can even transform all of U-Boot into
huge runtime services if we keep the relocation tables around and apply
relocations manually for everything.

The problem is that if we do that, we'll become even bigger than we are
now, no?


Alex

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

[Simon Glass]

Hi Alex,

On Mon, 28 Jan 2019 at 12:27, Alexander Graf <agraf@suse.de> wrote:
>
>
>
> On 28.01.19 20:24, Simon Glass wrote:
> > Hi Alex,
> >
> > On Mon, 28 Jan 2019 at 12:15, Alexander Graf <agraf@suse.de> wrote:
> >>
> >>
> >>
> >> On 28.01.19 20:13, Simon Glass wrote:
> >>> Hi,
> >>>
> >>> On Mon, 28 Jan 2019 at 08:42, Alexander Graf <agraf@suse.de> 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.
> >>>>
> >>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>> ---
> >>>>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
> >>>>  1 file changed, 23 insertions(+)
> >>>>
> >>>> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
> >>>> index 20b958cfd4..efed45ccb7 100644
> >>>> --- a/drivers/sysreset/sysreset_x86.c
> >>>> +++ b/drivers/sysreset/sysreset_x86.c
> >>>> @@ -10,6 +10,29 @@
> >>>>  #include <sysreset.h>
> >>>>  #include <asm/io.h>
> >>>>  #include <asm/processor.h>
> >>>> +#include <efi_loader.h>
> >>>> +
> >>>> +#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)
> >>>> +{
> >>>> +       u32 value = 0;
> >>>> +
> >>>> +       if (reset_type == EFI_RESET_COLD)
> >>>> +               value = SYS_RST | RST_CPU | FULL_RST;
> >>>> +       else if (reset_type == EFI_RESET_WARM)
> >>>> +               value = SYS_RST | RST_CPU;
> >>>
> >>> The EFI should use the sysreset driver and sysreset_walk() or similar,
> >>> rather than having a function called directly.
> >>
> >> It can't. At this point all of DM is long gone. We're in runtime space here.
> >
> > This has come up before. We'll end up with a lot of duplication if we
> > cannot solve this. I think the run-time code will need to be built and
> > linked separately so that all necessary code is pulled in.
>
> It's mostly a question of size. We can even transform all of U-Boot into
> huge runtime services if we keep the relocation tables around and apply
> relocations manually for everything.
>
> The problem is that if we do that, we'll become even bigger than we are
> now, no?

I did a change to build U-Boot as a library, perhaps it could build on
that. The 'run-time U-Boot' won't be any bigger than the code that is
actually used. Also I don't think memory usage is a concern in systems
that use UEFI :-)

The problem with the current approach is that everything becomes
parallel to DM, duplicating existing code, and that is complicated
too.

Regards,
Simon

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

[Alexander Graf]

On 28.01.19 20:31, Simon Glass wrote:
> Hi Alex,
> 
> On Mon, 28 Jan 2019 at 12:27, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>>
>> On 28.01.19 20:24, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On Mon, 28 Jan 2019 at 12:15, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>>
>>>> On 28.01.19 20:13, Simon Glass wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, 28 Jan 2019 at 08:42, Alexander Graf <agraf@suse.de> 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.
>>>>>>
>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>> ---
>>>>>>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
>>>>>>  1 file changed, 23 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
>>>>>> index 20b958cfd4..efed45ccb7 100644
>>>>>> --- a/drivers/sysreset/sysreset_x86.c
>>>>>> +++ b/drivers/sysreset/sysreset_x86.c
>>>>>> @@ -10,6 +10,29 @@
>>>>>>  #include <sysreset.h>
>>>>>>  #include <asm/io.h>
>>>>>>  #include <asm/processor.h>
>>>>>> +#include <efi_loader.h>
>>>>>> +
>>>>>> +#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)
>>>>>> +{
>>>>>> +       u32 value = 0;
>>>>>> +
>>>>>> +       if (reset_type == EFI_RESET_COLD)
>>>>>> +               value = SYS_RST | RST_CPU | FULL_RST;
>>>>>> +       else if (reset_type == EFI_RESET_WARM)
>>>>>> +               value = SYS_RST | RST_CPU;
>>>>>
>>>>> The EFI should use the sysreset driver and sysreset_walk() or similar,
>>>>> rather than having a function called directly.
>>>>
>>>> It can't. At this point all of DM is long gone. We're in runtime space here.
>>>
>>> This has come up before. We'll end up with a lot of duplication if we
>>> cannot solve this. I think the run-time code will need to be built and
>>> linked separately so that all necessary code is pulled in.
>>
>> It's mostly a question of size. We can even transform all of U-Boot into
>> huge runtime services if we keep the relocation tables around and apply
>> relocations manually for everything.
>>
>> The problem is that if we do that, we'll become even bigger than we are
>> now, no?
> 
> I did a change to build U-Boot as a library, perhaps it could build on
> that. The 'run-time U-Boot' won't be any bigger than the code that is
> actually used. Also I don't think memory usage is a concern in systems
> that use UEFI :-)

It is, we're already exploding some constraints today. Furthermore, by
moving all of U-Boot into RTS you obviously waste a few MB of RAM while
Linux is up. And it's much harder to review that the code is only doing
what you want it to do.

> The problem with the current approach is that everything becomes
> parallel to DM, duplicating existing code, and that is complicated
> too.

No, not everything. Only reset/shutdown. Well, and maybe variable
services. Which may use SPI. Meh.


Alex

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

[Simon Glass]

Hi Alex,

On Mon, 28 Jan 2019 at 12:34, Alexander Graf <agraf@suse.de> wrote:
>
>
>
> On 28.01.19 20:31, Simon Glass wrote:
> > Hi Alex,
> >
> > On Mon, 28 Jan 2019 at 12:27, Alexander Graf <agraf@suse.de> wrote:
> >>
> >>
> >>
> >> On 28.01.19 20:24, Simon Glass wrote:
> >>> Hi Alex,
> >>>
> >>> On Mon, 28 Jan 2019 at 12:15, Alexander Graf <agraf@suse.de> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 28.01.19 20:13, Simon Glass wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Mon, 28 Jan 2019 at 08:42, Alexander Graf <agraf@suse.de> 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.
> >>>>>>
> >>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>>>> ---
> >>>>>>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
> >>>>>>  1 file changed, 23 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
> >>>>>> index 20b958cfd4..efed45ccb7 100644
> >>>>>> --- a/drivers/sysreset/sysreset_x86.c
> >>>>>> +++ b/drivers/sysreset/sysreset_x86.c
> >>>>>> @@ -10,6 +10,29 @@
> >>>>>>  #include <sysreset.h>
> >>>>>>  #include <asm/io.h>
> >>>>>>  #include <asm/processor.h>
> >>>>>> +#include <efi_loader.h>
> >>>>>> +
> >>>>>> +#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)
> >>>>>> +{
> >>>>>> +       u32 value = 0;
> >>>>>> +
> >>>>>> +       if (reset_type == EFI_RESET_COLD)
> >>>>>> +               value = SYS_RST | RST_CPU | FULL_RST;
> >>>>>> +       else if (reset_type == EFI_RESET_WARM)
> >>>>>> +               value = SYS_RST | RST_CPU;
> >>>>>
> >>>>> The EFI should use the sysreset driver and sysreset_walk() or similar,
> >>>>> rather than having a function called directly.
> >>>>
> >>>> It can't. At this point all of DM is long gone. We're in runtime space here.
> >>>
> >>> This has come up before. We'll end up with a lot of duplication if we
> >>> cannot solve this. I think the run-time code will need to be built and
> >>> linked separately so that all necessary code is pulled in.
> >>
> >> It's mostly a question of size. We can even transform all of U-Boot into
> >> huge runtime services if we keep the relocation tables around and apply
> >> relocations manually for everything.
> >>
> >> The problem is that if we do that, we'll become even bigger than we are
> >> now, no?
> >
> > I did a change to build U-Boot as a library, perhaps it could build on
> > that. The 'run-time U-Boot' won't be any bigger than the code that is
> > actually used. Also I don't think memory usage is a concern in systems
> > that use UEFI :-)
>
> It is, we're already exploding some constraints today. Furthermore, by
> moving all of U-Boot into RTS you obviously waste a few MB of RAM while
> Linux is up. And it's much harder to review that the code is only doing
> what you want it to do.

My suggest is not to move all of U-Boot into RTS. It is to build an
RTS version of U-Boot with just those things that are needed, a bit
like TPL or SPL.

I did not invent the run-time aspect of EFI, but I feel we are trying
to support it with a hack.

>
> > The problem with the current approach is that everything becomes
> > parallel to DM, duplicating existing code, and that is complicated
> > too.
>
> No, not everything. Only reset/shutdown. Well, and maybe variable
> services. Which may use SPI. Meh.

That should be a fairly small build, then.

Regards,
Simon

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

[Alexander Graf]

On 28.01.19 20:36, Simon Glass wrote:
> Hi Alex,
> 
> On Mon, 28 Jan 2019 at 12:34, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>>
>> On 28.01.19 20:31, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On Mon, 28 Jan 2019 at 12:27, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>>
>>>> On 28.01.19 20:24, Simon Glass wrote:
>>>>> Hi Alex,
>>>>>
>>>>> On Mon, 28 Jan 2019 at 12:15, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 28.01.19 20:13, Simon Glass wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Mon, 28 Jan 2019 at 08:42, Alexander Graf <agraf@suse.de> 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.
>>>>>>>>
>>>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>>>> ---
>>>>>>>>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
>>>>>>>>  1 file changed, 23 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
>>>>>>>> index 20b958cfd4..efed45ccb7 100644
>>>>>>>> --- a/drivers/sysreset/sysreset_x86.c
>>>>>>>> +++ b/drivers/sysreset/sysreset_x86.c
>>>>>>>> @@ -10,6 +10,29 @@
>>>>>>>>  #include <sysreset.h>
>>>>>>>>  #include <asm/io.h>
>>>>>>>>  #include <asm/processor.h>
>>>>>>>> +#include <efi_loader.h>
>>>>>>>> +
>>>>>>>> +#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)
>>>>>>>> +{
>>>>>>>> +       u32 value = 0;
>>>>>>>> +
>>>>>>>> +       if (reset_type == EFI_RESET_COLD)
>>>>>>>> +               value = SYS_RST | RST_CPU | FULL_RST;
>>>>>>>> +       else if (reset_type == EFI_RESET_WARM)
>>>>>>>> +               value = SYS_RST | RST_CPU;
>>>>>>>
>>>>>>> The EFI should use the sysreset driver and sysreset_walk() or similar,
>>>>>>> rather than having a function called directly.
>>>>>>
>>>>>> It can't. At this point all of DM is long gone. We're in runtime space here.
>>>>>
>>>>> This has come up before. We'll end up with a lot of duplication if we
>>>>> cannot solve this. I think the run-time code will need to be built and
>>>>> linked separately so that all necessary code is pulled in.
>>>>
>>>> It's mostly a question of size. We can even transform all of U-Boot into
>>>> huge runtime services if we keep the relocation tables around and apply
>>>> relocations manually for everything.
>>>>
>>>> The problem is that if we do that, we'll become even bigger than we are
>>>> now, no?
>>>
>>> I did a change to build U-Boot as a library, perhaps it could build on
>>> that. The 'run-time U-Boot' won't be any bigger than the code that is
>>> actually used. Also I don't think memory usage is a concern in systems
>>> that use UEFI :-)
>>
>> It is, we're already exploding some constraints today. Furthermore, by
>> moving all of U-Boot into RTS you obviously waste a few MB of RAM while
>> Linux is up. And it's much harder to review that the code is only doing
>> what you want it to do.
> 
> My suggest is not to move all of U-Boot into RTS. It is to build an
> RTS version of U-Boot with just those things that are needed, a bit
> like TPL or SPL.

Yes, but the "loaded binary" to boot the system is then BTS + RTS. So if
U-Boot is 1MB and the RTS U-Boot build is 0.5MB, you need to store and
load 1.5MB from SD card or SPI or wherever your U-Boot is stored.

By morphing all of U-Boot over, the runtime cost is higher (1MB rather
than 0.5MB used while Linux is up), but the boot time cost is smaller
(only 1MB on storage).

> I did not invent the run-time aspect of EFI, but I feel we are trying
> to support it with a hack.

Well, it is a hack in edk2 as well. RTS are a terrible idea in my book.
But they're there and we need to support at least reset/shutdown and
*maybe* some sort of runtime variables.

>>> The problem with the current approach is that everything becomes
>>> parallel to DM, duplicating existing code, and that is complicated
>>> too.
>>
>> No, not everything. Only reset/shutdown. Well, and maybe variable
>> services. Which may use SPI. Meh.
> 
> That should be a fairly small build, then.

It still adds to the binary size.


Alex

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

[Simon Glass]

Hi Alex,

On Mon, 28 Jan 2019 at 12:39, Alexander Graf <agraf@suse.de> wrote:
>
>
>
> On 28.01.19 20:36, Simon Glass wrote:
> > Hi Alex,
> >
> > On Mon, 28 Jan 2019 at 12:34, Alexander Graf <agraf@suse.de> wrote:
> >>
> >>
> >>
> >> On 28.01.19 20:31, Simon Glass wrote:
> >>> Hi Alex,
> >>>
> >>> On Mon, 28 Jan 2019 at 12:27, Alexander Graf <agraf@suse.de> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 28.01.19 20:24, Simon Glass wrote:
> >>>>> Hi Alex,
> >>>>>
> >>>>> On Mon, 28 Jan 2019 at 12:15, Alexander Graf <agraf@suse.de> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 28.01.19 20:13, Simon Glass wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> On Mon, 28 Jan 2019 at 08:42, Alexander Graf <agraf@suse.de> 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.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>>>>>> ---
> >>>>>>>>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
> >>>>>>>>  1 file changed, 23 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
> >>>>>>>> index 20b958cfd4..efed45ccb7 100644
> >>>>>>>> --- a/drivers/sysreset/sysreset_x86.c
> >>>>>>>> +++ b/drivers/sysreset/sysreset_x86.c
> >>>>>>>> @@ -10,6 +10,29 @@
> >>>>>>>>  #include <sysreset.h>
> >>>>>>>>  #include <asm/io.h>
> >>>>>>>>  #include <asm/processor.h>
> >>>>>>>> +#include <efi_loader.h>
> >>>>>>>> +
> >>>>>>>> +#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)
> >>>>>>>> +{
> >>>>>>>> +       u32 value = 0;
> >>>>>>>> +
> >>>>>>>> +       if (reset_type == EFI_RESET_COLD)
> >>>>>>>> +               value = SYS_RST | RST_CPU | FULL_RST;
> >>>>>>>> +       else if (reset_type == EFI_RESET_WARM)
> >>>>>>>> +               value = SYS_RST | RST_CPU;
> >>>>>>>
> >>>>>>> The EFI should use the sysreset driver and sysreset_walk() or similar,
> >>>>>>> rather than having a function called directly.
> >>>>>>
> >>>>>> It can't. At this point all of DM is long gone. We're in runtime space here.
> >>>>>
> >>>>> This has come up before. We'll end up with a lot of duplication if we
> >>>>> cannot solve this. I think the run-time code will need to be built and
> >>>>> linked separately so that all necessary code is pulled in.
> >>>>
> >>>> It's mostly a question of size. We can even transform all of U-Boot into
> >>>> huge runtime services if we keep the relocation tables around and apply
> >>>> relocations manually for everything.
> >>>>
> >>>> The problem is that if we do that, we'll become even bigger than we are
> >>>> now, no?
> >>>
> >>> I did a change to build U-Boot as a library, perhaps it could build on
> >>> that. The 'run-time U-Boot' won't be any bigger than the code that is
> >>> actually used. Also I don't think memory usage is a concern in systems
> >>> that use UEFI :-)
> >>
> >> It is, we're already exploding some constraints today. Furthermore, by
> >> moving all of U-Boot into RTS you obviously waste a few MB of RAM while
> >> Linux is up. And it's much harder to review that the code is only doing
> >> what you want it to do.
> >
> > My suggest is not to move all of U-Boot into RTS. It is to build an
> > RTS version of U-Boot with just those things that are needed, a bit
> > like TPL or SPL.
>
> Yes, but the "loaded binary" to boot the system is then BTS + RTS. So if
> U-Boot is 1MB and the RTS U-Boot build is 0.5MB, you need to store and
> load 1.5MB from SD card or SPI or wherever your U-Boot is stored.

Yes that's right. Although I suspect it is only about half that. E.g.
Raspberry Pi 2 is 431KB. I'd hope that RTS would be very small?

>
>
> By morphing all of U-Boot over, the runtime cost is higher (1MB rather
> than 0.5MB used while Linux is up), but the boot time cost is smaller
> (only 1MB on storage).
>
> > I did not invent the run-time aspect of EFI, but I feel we are trying
> > to support it with a hack.
>
> Well, it is a hack in edk2 as well. RTS are a terrible idea in my book.
> But they're there and we need to support at least reset/shutdown and
> *maybe* some sort of runtime variables.

Yes, but at some point this all becomes unmanageable. I think we
should adjust the approach now.

>
>
> >>> The problem with the current approach is that everything becomes
> >>> parallel to DM, duplicating existing code, and that is complicated
> >>> too.
> >>
> >> No, not everything. Only reset/shutdown. Well, and maybe variable
> >> services. Which may use SPI. Meh.
> >
> > That should be a fairly small build, then.
>
> It still adds to the binary size.

Yes. In fact EFI_LOADER adds about 50KB on the same rpi_2. This is a
fact of life with EFI. I really don't think EFI is optimised for code
size.

I think we need a little build like SPL which pulls in just what is
needed, but does use DM.

Regards,
Simon

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

[Alexander Graf]

On 28.01.19 20:47, Simon Glass wrote:
> Hi Alex,
> 
> On Mon, 28 Jan 2019 at 12:39, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>>
>> On 28.01.19 20:36, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On Mon, 28 Jan 2019 at 12:34, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>>
>>>> On 28.01.19 20:31, Simon Glass wrote:
>>>>> Hi Alex,
>>>>>
>>>>> On Mon, 28 Jan 2019 at 12:27, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 28.01.19 20:24, Simon Glass wrote:
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> On Mon, 28 Jan 2019 at 12:15, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 28.01.19 20:13, Simon Glass wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On Mon, 28 Jan 2019 at 08:42, Alexander Graf <agraf@suse.de> 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.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>>>>>> ---
>>>>>>>>>>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
>>>>>>>>>>  1 file changed, 23 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
>>>>>>>>>> index 20b958cfd4..efed45ccb7 100644
>>>>>>>>>> --- a/drivers/sysreset/sysreset_x86.c
>>>>>>>>>> +++ b/drivers/sysreset/sysreset_x86.c
>>>>>>>>>> @@ -10,6 +10,29 @@
>>>>>>>>>>  #include <sysreset.h>
>>>>>>>>>>  #include <asm/io.h>
>>>>>>>>>>  #include <asm/processor.h>
>>>>>>>>>> +#include <efi_loader.h>
>>>>>>>>>> +
>>>>>>>>>> +#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)
>>>>>>>>>> +{
>>>>>>>>>> +       u32 value = 0;
>>>>>>>>>> +
>>>>>>>>>> +       if (reset_type == EFI_RESET_COLD)
>>>>>>>>>> +               value = SYS_RST | RST_CPU | FULL_RST;
>>>>>>>>>> +       else if (reset_type == EFI_RESET_WARM)
>>>>>>>>>> +               value = SYS_RST | RST_CPU;
>>>>>>>>>
>>>>>>>>> The EFI should use the sysreset driver and sysreset_walk() or similar,
>>>>>>>>> rather than having a function called directly.
>>>>>>>>
>>>>>>>> It can't. At this point all of DM is long gone. We're in runtime space here.
>>>>>>>
>>>>>>> This has come up before. We'll end up with a lot of duplication if we
>>>>>>> cannot solve this. I think the run-time code will need to be built and
>>>>>>> linked separately so that all necessary code is pulled in.
>>>>>>
>>>>>> It's mostly a question of size. We can even transform all of U-Boot into
>>>>>> huge runtime services if we keep the relocation tables around and apply
>>>>>> relocations manually for everything.
>>>>>>
>>>>>> The problem is that if we do that, we'll become even bigger than we are
>>>>>> now, no?
>>>>>
>>>>> I did a change to build U-Boot as a library, perhaps it could build on
>>>>> that. The 'run-time U-Boot' won't be any bigger than the code that is
>>>>> actually used. Also I don't think memory usage is a concern in systems
>>>>> that use UEFI :-)
>>>>
>>>> It is, we're already exploding some constraints today. Furthermore, by
>>>> moving all of U-Boot into RTS you obviously waste a few MB of RAM while
>>>> Linux is up. And it's much harder to review that the code is only doing
>>>> what you want it to do.
>>>
>>> My suggest is not to move all of U-Boot into RTS. It is to build an
>>> RTS version of U-Boot with just those things that are needed, a bit
>>> like TPL or SPL.
>>
>> Yes, but the "loaded binary" to boot the system is then BTS + RTS. So if
>> U-Boot is 1MB and the RTS U-Boot build is 0.5MB, you need to store and
>> load 1.5MB from SD card or SPI or wherever your U-Boot is stored.
> 
> Yes that's right. Although I suspect it is only about half that. E.g.
> Raspberry Pi 2 is 431KB. I'd hope that RTS would be very small?
> 
>>
>>
>> By morphing all of U-Boot over, the runtime cost is higher (1MB rather
>> than 0.5MB used while Linux is up), but the boot time cost is smaller
>> (only 1MB on storage).
>>
>>> I did not invent the run-time aspect of EFI, but I feel we are trying
>>> to support it with a hack.
>>
>> Well, it is a hack in edk2 as well. RTS are a terrible idea in my book.
>> But they're there and we need to support at least reset/shutdown and
>> *maybe* some sort of runtime variables.
> 
> Yes, but at some point this all becomes unmanageable. I think we
> should adjust the approach now.

So how would we transfer device information from one piece to the next?
How would we ensure that we don't reinitialize everything?

Adding another runtime payload really doesn't sound terribly appealing
to me. It just adds even more complexity to a problem that shouldn't
exist in an ideal world.

> 
>>
>>
>>>>> The problem with the current approach is that everything becomes
>>>>> parallel to DM, duplicating existing code, and that is complicated
>>>>> too.
>>>>
>>>> No, not everything. Only reset/shutdown. Well, and maybe variable
>>>> services. Which may use SPI. Meh.
>>>
>>> That should be a fairly small build, then.
>>
>> It still adds to the binary size.
> 
> Yes. In fact EFI_LOADER adds about 50KB on the same rpi_2. This is a
> fact of life with EFI. I really don't think EFI is optimised for code
> size.

It's not, but 50kb is already on the verge. If we add 20kb more, people
will start to disable EFI support for their boards. And that's exactly
contrary to what we want.

> I think we need a little build like SPL which pulls in just what is
> needed, but does use DM.

Again, I don't think that will work very well. Transferring state from
one to the other is going to be tricky. Sure - you could reinitialize -
but then the boot process will be even slower.

So if we need to play tricks, maybe we're better off adding some Kconfig
magic that indicates to the compiler that .text is now called
.text.runtime. which then automatically pulls everything from specific
files into runtime.

We could apply such logic to all DM files and everything we need for
specific hardware at runtime. Then we can add a runtime flag to devices
and limit device enumeration to only runtime enabled ones after the
switch-over.


Alex

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

[Simon Glass]

Hi Alex,

On Mon, 28 Jan 2019 at 13:08, Alexander Graf <agraf@suse.de> wrote:
>
>
>
> On 28.01.19 20:47, Simon Glass wrote:
> > Hi Alex,
> >
> > On Mon, 28 Jan 2019 at 12:39, Alexander Graf <agraf@suse.de> wrote:
> >>
> >>
> >>
> >> On 28.01.19 20:36, Simon Glass wrote:
> >>> Hi Alex,
> >>>
> >>> On Mon, 28 Jan 2019 at 12:34, Alexander Graf <agraf@suse.de> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 28.01.19 20:31, Simon Glass wrote:
> >>>>> Hi Alex,
> >>>>>
> >>>>> On Mon, 28 Jan 2019 at 12:27, Alexander Graf <agraf@suse.de> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 28.01.19 20:24, Simon Glass wrote:
> >>>>>>> Hi Alex,
> >>>>>>>
> >>>>>>> On Mon, 28 Jan 2019 at 12:15, Alexander Graf <agraf@suse.de> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 28.01.19 20:13, Simon Glass wrote:
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> On Mon, 28 Jan 2019 at 08:42, Alexander Graf <agraf@suse.de> 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.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>>>>>>>> ---
> >>>>>>>>>>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
> >>>>>>>>>>  1 file changed, 23 insertions(+)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
> >>>>>>>>>> index 20b958cfd4..efed45ccb7 100644
> >>>>>>>>>> --- a/drivers/sysreset/sysreset_x86.c
> >>>>>>>>>> +++ b/drivers/sysreset/sysreset_x86.c
> >>>>>>>>>> @@ -10,6 +10,29 @@
> >>>>>>>>>>  #include <sysreset.h>
> >>>>>>>>>>  #include <asm/io.h>
> >>>>>>>>>>  #include <asm/processor.h>
> >>>>>>>>>> +#include <efi_loader.h>
> >>>>>>>>>> +
> >>>>>>>>>> +#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)
> >>>>>>>>>> +{
> >>>>>>>>>> +       u32 value = 0;
> >>>>>>>>>> +
> >>>>>>>>>> +       if (reset_type == EFI_RESET_COLD)
> >>>>>>>>>> +               value = SYS_RST | RST_CPU | FULL_RST;
> >>>>>>>>>> +       else if (reset_type == EFI_RESET_WARM)
> >>>>>>>>>> +               value = SYS_RST | RST_CPU;
> >>>>>>>>>
> >>>>>>>>> The EFI should use the sysreset driver and sysreset_walk() or similar,
> >>>>>>>>> rather than having a function called directly.
> >>>>>>>>
> >>>>>>>> It can't. At this point all of DM is long gone. We're in runtime space here.
> >>>>>>>
> >>>>>>> This has come up before. We'll end up with a lot of duplication if we
> >>>>>>> cannot solve this. I think the run-time code will need to be built and
> >>>>>>> linked separately so that all necessary code is pulled in.
> >>>>>>
> >>>>>> It's mostly a question of size. We can even transform all of U-Boot into
> >>>>>> huge runtime services if we keep the relocation tables around and apply
> >>>>>> relocations manually for everything.
> >>>>>>
> >>>>>> The problem is that if we do that, we'll become even bigger than we are
> >>>>>> now, no?
> >>>>>
> >>>>> I did a change to build U-Boot as a library, perhaps it could build on
> >>>>> that. The 'run-time U-Boot' won't be any bigger than the code that is
> >>>>> actually used. Also I don't think memory usage is a concern in systems
> >>>>> that use UEFI :-)
> >>>>
> >>>> It is, we're already exploding some constraints today. Furthermore, by
> >>>> moving all of U-Boot into RTS you obviously waste a few MB of RAM while
> >>>> Linux is up. And it's much harder to review that the code is only doing
> >>>> what you want it to do.
> >>>
> >>> My suggest is not to move all of U-Boot into RTS. It is to build an
> >>> RTS version of U-Boot with just those things that are needed, a bit
> >>> like TPL or SPL.
> >>
> >> Yes, but the "loaded binary" to boot the system is then BTS + RTS. So if
> >> U-Boot is 1MB and the RTS U-Boot build is 0.5MB, you need to store and
> >> load 1.5MB from SD card or SPI or wherever your U-Boot is stored.
> >
> > Yes that's right. Although I suspect it is only about half that. E.g.
> > Raspberry Pi 2 is 431KB. I'd hope that RTS would be very small?
> >
> >>
> >>
> >> By morphing all of U-Boot over, the runtime cost is higher (1MB rather
> >> than 0.5MB used while Linux is up), but the boot time cost is smaller
> >> (only 1MB on storage).
> >>
> >>> I did not invent the run-time aspect of EFI, but I feel we are trying
> >>> to support it with a hack.
> >>
> >> Well, it is a hack in edk2 as well. RTS are a terrible idea in my book.
> >> But they're there and we need to support at least reset/shutdown and
> >> *maybe* some sort of runtime variables.
> >
> > Yes, but at some point this all becomes unmanageable. I think we
> > should adjust the approach now.
>
> So how would we transfer device information from one piece to the next?
> How would we ensure that we don't reinitialize everything?

Well if we only need a few devices (sysreset, variables, SPI and SPI
flash) then it doesn't really matter.

>
> Adding another runtime payload really doesn't sound terribly appealing
> to me. It just adds even more complexity to a problem that shouldn't
> exist in an ideal world.

Is there any way to avoid the run-time stuff completely? If not, then
I suppose we have to address it.

>
> >
> >>
> >>
> >>>>> The problem with the current approach is that everything becomes
> >>>>> parallel to DM, duplicating existing code, and that is complicated
> >>>>> too.
> >>>>
> >>>> No, not everything. Only reset/shutdown. Well, and maybe variable
> >>>> services. Which may use SPI. Meh.
> >>>
> >>> That should be a fairly small build, then.
> >>
> >> It still adds to the binary size.
> >
> > Yes. In fact EFI_LOADER adds about 50KB on the same rpi_2. This is a
> > fact of life with EFI. I really don't think EFI is optimised for code
> > size.
>
> It's not, but 50kb is already on the verge. If we add 20kb more, people
> will start to disable EFI support for their boards. And that's exactly
> contrary to what we want.
>
> > I think we need a little build like SPL which pulls in just what is
> > needed, but does use DM.
>
> Again, I don't think that will work very well. Transferring state from
> one to the other is going to be tricky. Sure - you could reinitialize -
> but then the boot process will be even slower.

What state? We do have bloblist now for 'automatic' state transfer.
But what state?

>
> So if we need to play tricks, maybe we're better off adding some Kconfig
> magic that indicates to the compiler that .text is now called
> .text.runtime. which then automatically pulls everything from specific
> files into runtime.
>
> We could apply such logic to all DM files and everything we need for
> specific hardware at runtime. Then we can add a runtime flag to devices
> and limit device enumeration to only runtime enabled ones after the
> switch-over.

That might work. I suppose we need a test which zeroes out all the
non-runtime code and then calls all the runtime functions in 'fake'
mode?

Regards,
Simon

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

[Alexander Graf]

On 01/29/2019 01:49 AM, Simon Glass wrote:
> Hi Alex,
>
> On Mon, 28 Jan 2019 at 13:08, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 28.01.19 20:47, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On Mon, 28 Jan 2019 at 12:39, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>> On 28.01.19 20:36, Simon Glass wrote:
>>>>> Hi Alex,
>>>>>
>>>>> On Mon, 28 Jan 2019 at 12:34, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>
>>>>>> On 28.01.19 20:31, Simon Glass wrote:
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> On Mon, 28 Jan 2019 at 12:27, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 28.01.19 20:24, Simon Glass wrote:
>>>>>>>>> Hi Alex,
>>>>>>>>>
>>>>>>>>> On Mon, 28 Jan 2019 at 12:15, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 28.01.19 20:13, Simon Glass wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On Mon, 28 Jan 2019 at 08:42, Alexander Graf <agraf@suse.de> 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.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>>>>>>>> ---
>>>>>>>>>>>>   drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
>>>>>>>>>>>>   1 file changed, 23 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
>>>>>>>>>>>> index 20b958cfd4..efed45ccb7 100644
>>>>>>>>>>>> --- a/drivers/sysreset/sysreset_x86.c
>>>>>>>>>>>> +++ b/drivers/sysreset/sysreset_x86.c
>>>>>>>>>>>> @@ -10,6 +10,29 @@
>>>>>>>>>>>>   #include <sysreset.h>
>>>>>>>>>>>>   #include <asm/io.h>
>>>>>>>>>>>>   #include <asm/processor.h>
>>>>>>>>>>>> +#include <efi_loader.h>
>>>>>>>>>>>> +
>>>>>>>>>>>> +#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)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +       u32 value = 0;
>>>>>>>>>>>> +
>>>>>>>>>>>> +       if (reset_type == EFI_RESET_COLD)
>>>>>>>>>>>> +               value = SYS_RST | RST_CPU | FULL_RST;
>>>>>>>>>>>> +       else if (reset_type == EFI_RESET_WARM)
>>>>>>>>>>>> +               value = SYS_RST | RST_CPU;
>>>>>>>>>>> The EFI should use the sysreset driver and sysreset_walk() or similar,
>>>>>>>>>>> rather than having a function called directly.
>>>>>>>>>> It can't. At this point all of DM is long gone. We're in runtime space here.
>>>>>>>>> This has come up before. We'll end up with a lot of duplication if we
>>>>>>>>> cannot solve this. I think the run-time code will need to be built and
>>>>>>>>> linked separately so that all necessary code is pulled in.
>>>>>>>> It's mostly a question of size. We can even transform all of U-Boot into
>>>>>>>> huge runtime services if we keep the relocation tables around and apply
>>>>>>>> relocations manually for everything.
>>>>>>>>
>>>>>>>> The problem is that if we do that, we'll become even bigger than we are
>>>>>>>> now, no?
>>>>>>> I did a change to build U-Boot as a library, perhaps it could build on
>>>>>>> that. The 'run-time U-Boot' won't be any bigger than the code that is
>>>>>>> actually used. Also I don't think memory usage is a concern in systems
>>>>>>> that use UEFI :-)
>>>>>> It is, we're already exploding some constraints today. Furthermore, by
>>>>>> moving all of U-Boot into RTS you obviously waste a few MB of RAM while
>>>>>> Linux is up. And it's much harder to review that the code is only doing
>>>>>> what you want it to do.
>>>>> My suggest is not to move all of U-Boot into RTS. It is to build an
>>>>> RTS version of U-Boot with just those things that are needed, a bit
>>>>> like TPL or SPL.
>>>> Yes, but the "loaded binary" to boot the system is then BTS + RTS. So if
>>>> U-Boot is 1MB and the RTS U-Boot build is 0.5MB, you need to store and
>>>> load 1.5MB from SD card or SPI or wherever your U-Boot is stored.
>>> Yes that's right. Although I suspect it is only about half that. E.g.
>>> Raspberry Pi 2 is 431KB. I'd hope that RTS would be very small?
>>>
>>>>
>>>> By morphing all of U-Boot over, the runtime cost is higher (1MB rather
>>>> than 0.5MB used while Linux is up), but the boot time cost is smaller
>>>> (only 1MB on storage).
>>>>
>>>>> I did not invent the run-time aspect of EFI, but I feel we are trying
>>>>> to support it with a hack.
>>>> Well, it is a hack in edk2 as well. RTS are a terrible idea in my book.
>>>> But they're there and we need to support at least reset/shutdown and
>>>> *maybe* some sort of runtime variables.
>>> Yes, but at some point this all becomes unmanageable. I think we
>>> should adjust the approach now.
>> So how would we transfer device information from one piece to the next?
>> How would we ensure that we don't reinitialize everything?
> Well if we only need a few devices (sysreset, variables, SPI and SPI
> flash) then it doesn't really matter.
>
>> Adding another runtime payload really doesn't sound terribly appealing
>> to me. It just adds even more complexity to a problem that shouldn't
>> exist in an ideal world.
> Is there any way to avoid the run-time stuff completely? If not, then
> I suppose we have to address it.

Depends on what you want. There is the possibility now to just not 
support (or rather, error out on all) runtime services, yes. But if you 
want their functionality, you also need to implement them.

>
>>>>
>>>>>>> The problem with the current approach is that everything becomes
>>>>>>> parallel to DM, duplicating existing code, and that is complicated
>>>>>>> too.
>>>>>> No, not everything. Only reset/shutdown. Well, and maybe variable
>>>>>> services. Which may use SPI. Meh.
>>>>> That should be a fairly small build, then.
>>>> It still adds to the binary size.
>>> Yes. In fact EFI_LOADER adds about 50KB on the same rpi_2. This is a
>>> fact of life with EFI. I really don't think EFI is optimised for code
>>> size.
>> It's not, but 50kb is already on the verge. If we add 20kb more, people
>> will start to disable EFI support for their boards. And that's exactly
>> contrary to what we want.
>>
>>> I think we need a little build like SPL which pulls in just what is
>>> needed, but does use DM.
>> Again, I don't think that will work very well. Transferring state from
>> one to the other is going to be tricky. Sure - you could reinitialize -
>> but then the boot process will be even slower.
> What state? We do have bloblist now for 'automatic' state transfer.
> But what state?

State such as "environment in SPI flash is here". I guess a lot of it 
can be compile time. Hm.

>
>> So if we need to play tricks, maybe we're better off adding some Kconfig
>> magic that indicates to the compiler that .text is now called
>> .text.runtime. which then automatically pulls everything from specific
>> files into runtime.
>>
>> We could apply such logic to all DM files and everything we need for
>> specific hardware at runtime. Then we can add a runtime flag to devices
>> and limit device enumeration to only runtime enabled ones after the
>> switch-over.
> That might work. I suppose we need a test which zeroes out all the
> non-runtime code and then calls all the runtime functions in 'fake'
> mode?

Yeah, probably. And an allocator that allocates from runtime memory.


Alex

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

[Heinrich Schuchardt]

On 1/28/19 4:42 PM, 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.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
> index 20b958cfd4..efed45ccb7 100644
> --- a/drivers/sysreset/sysreset_x86.c
> +++ b/drivers/sysreset/sysreset_x86.c
> @@ -10,6 +10,29 @@
>  #include <sysreset.h>
>  #include <asm/io.h>
>  #include <asm/processor.h>
> +#include <efi_loader.h>
> +
> +#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)
> +{
> +	u32 value = 0;
> +
> +	if (reset_type == EFI_RESET_COLD)
> +		value = SYS_RST | RST_CPU | FULL_RST;
> +	else if (reset_type == EFI_RESET_WARM)
> +		value = SYS_RST | RST_CPU;
> +
> +	/* TODO EFI_RESET_PLATFORM_SPECIFIC and EFI_RESET_SHUTDOWN */
> +
> +	if (value)
> +		outb(value, IO_PORT_RESET);

This does not look ACPI compliant. Shouldn't we read the ACPI table to
identify the reset register?

When we do the reset CPUs in several sockets may be running multiple
cores each. Are all of these stopped via this register? Or do we first
have to halt all but one core before doing the reset?

Best regards

Heinrich


> +
> +	while (1) { }
> +}
> +#endif
>  
>  static int x86_sysreset_request(struct udevice *dev, enum sysreset_t type)
>  {
> 

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

[Alexander Graf]

On 30.01.19 00:08, Heinrich Schuchardt wrote:
> On 1/28/19 4:42 PM, 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.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
>> index 20b958cfd4..efed45ccb7 100644
>> --- a/drivers/sysreset/sysreset_x86.c
>> +++ b/drivers/sysreset/sysreset_x86.c
>> @@ -10,6 +10,29 @@
>>  #include <sysreset.h>
>>  #include <asm/io.h>
>>  #include <asm/processor.h>
>> +#include <efi_loader.h>
>> +
>> +#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)
>> +{
>> +	u32 value = 0;
>> +
>> +	if (reset_type == EFI_RESET_COLD)
>> +		value = SYS_RST | RST_CPU | FULL_RST;
>> +	else if (reset_type == EFI_RESET_WARM)
>> +		value = SYS_RST | RST_CPU;
>> +
>> +	/* TODO EFI_RESET_PLATFORM_SPECIFIC and EFI_RESET_SHUTDOWN */
>> +
>> +	if (value)
>> +		outb(value, IO_PORT_RESET);
> 
> This does not look ACPI compliant. Shouldn't we read the ACPI table to
> identify the reset register?

There are about 500 different ways to reset the system, CPU, something
on x86. I don't think U-Boot should do anything with ACPI. It's an ACPI
producer, not an ACPI consumer.

> When we do the reset CPUs in several sockets may be running multiple
> cores each. Are all of these stopped via this register? Or do we first
> have to halt all but one core before doing the reset?

I don't know what the reset protocol dictates here. But it probably also
heavily depends on the target platform we're looking at. IIUC this
particular reset is the keyboard controller one which just pulls the
reset line of the primary CPU socket.

I am not aware of any multi-socket targets that U-Boot supports, so it
should work in all of today's cases?

Realistically, I would not expect anyone to care too much about U-Boot
on multi-socket (x86) systems :).


Alex

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

[Heinrich Schuchardt]

On 1/30/19 12:15 AM, Alexander Graf wrote:
> 
> 
> On 30.01.19 00:08, Heinrich Schuchardt wrote:
>> On 1/28/19 4:42 PM, 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.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
>>>  1 file changed, 23 insertions(+)
>>>
>>> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
>>> index 20b958cfd4..efed45ccb7 100644
>>> --- a/drivers/sysreset/sysreset_x86.c
>>> +++ b/drivers/sysreset/sysreset_x86.c
>>> @@ -10,6 +10,29 @@
>>>  #include <sysreset.h>
>>>  #include <asm/io.h>
>>>  #include <asm/processor.h>
>>> +#include <efi_loader.h>
>>> +
>>> +#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)
>>> +{
>>> +	u32 value = 0;
>>> +
>>> +	if (reset_type == EFI_RESET_COLD)
>>> +		value = SYS_RST | RST_CPU | FULL_RST;
>>> +	else if (reset_type == EFI_RESET_WARM)
>>> +		value = SYS_RST | RST_CPU;
>>> +
>>> +	/* TODO EFI_RESET_PLATFORM_SPECIFIC and EFI_RESET_SHUTDOWN */
>>> +
>>> +	if (value)
>>> +		outb(value, IO_PORT_RESET);
>>
>> This does not look ACPI compliant. Shouldn't we read the ACPI table to
>> identify the reset register?
> 
> There are about 500 different ways to reset the system, CPU, something
> on x86. I don't think U-Boot should do anything with ACPI. It's an ACPI
> producer, not an ACPI consumer.
> 
>> When we do the reset CPUs in several sockets may be running multiple
>> cores each. Are all of these stopped via this register? Or do we first
>> have to halt all but one core before doing the reset?
> 
> I don't know what the reset protocol dictates here. But it probably also
> heavily depends on the target platform we're looking at. IIUC this
> particular reset is the keyboard controller one which just pulls the
> reset line of the primary CPU socket.
> 
> I am not aware of any multi-socket targets that U-Boot supports, so it
> should work in all of today's cases?
> 
> Realistically, I would not expect anyone to care too much about U-Boot
> on multi-socket (x86) systems :).

Your code resembles loosely 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

> 
> 
> Alex
> 

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

[Alexander Graf]

On 30.01.19 00:48, Heinrich Schuchardt wrote:
> On 1/30/19 12:15 AM, Alexander Graf wrote:
>>
>>
>> On 30.01.19 00:08, Heinrich Schuchardt wrote:
>>> On 1/28/19 4:42 PM, 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.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>>  drivers/sysreset/sysreset_x86.c | 23 +++++++++++++++++++++++
>>>>  1 file changed, 23 insertions(+)
>>>>
>>>> diff --git a/drivers/sysreset/sysreset_x86.c b/drivers/sysreset/sysreset_x86.c
>>>> index 20b958cfd4..efed45ccb7 100644
>>>> --- a/drivers/sysreset/sysreset_x86.c
>>>> +++ b/drivers/sysreset/sysreset_x86.c
>>>> @@ -10,6 +10,29 @@
>>>>  #include <sysreset.h>
>>>>  #include <asm/io.h>
>>>>  #include <asm/processor.h>
>>>> +#include <efi_loader.h>
>>>> +
>>>> +#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)
>>>> +{
>>>> +	u32 value = 0;
>>>> +
>>>> +	if (reset_type == EFI_RESET_COLD)
>>>> +		value = SYS_RST | RST_CPU | FULL_RST;
>>>> +	else if (reset_type == EFI_RESET_WARM)
>>>> +		value = SYS_RST | RST_CPU;
>>>> +
>>>> +	/* TODO EFI_RESET_PLATFORM_SPECIFIC and EFI_RESET_SHUTDOWN */
>>>> +
>>>> +	if (value)
>>>> +		outb(value, IO_PORT_RESET);
>>>
>>> This does not look ACPI compliant. Shouldn't we read the ACPI table to
>>> identify the reset register?
>>
>> There are about 500 different ways to reset the system, CPU, something
>> on x86. I don't think U-Boot should do anything with ACPI. It's an ACPI
>> producer, not an ACPI consumer.
>>
>>> When we do the reset CPUs in several sockets may be running multiple
>>> cores each. Are all of these stopped via this register? Or do we first
>>> have to halt all but one core before doing the reset?
>>
>> I don't know what the reset protocol dictates here. But it probably also
>> heavily depends on the target platform we're looking at. IIUC this
>> particular reset is the keyboard controller one which just pulls the
>> reset line of the primary CPU socket.
>>
>> I am not aware of any multi-socket targets that U-Boot supports, so it
>> should work in all of today's cases?
>>
>> Realistically, I would not expect anyone to care too much about U-Boot
>> on multi-socket (x86) systems :).
> 
> Your code resembles loosely 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?

Not sure, I really just copied the existing code ;). I guess I should
make that more obvious by not duplicating, but calling it.


Alex