[U-Boot] [PATCH v2 0/2] efi_loader: Patch RTS at ExitBootServices
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(-)
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) {
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) {
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
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
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
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
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
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
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
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
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
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
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
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
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) {
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
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-cont...
Shouldn't we do it the same way as the Kernel does it?
Best regards
Heinrich
Alex
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-cont...
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
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); }
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(-)
participants (3)
-
Alexander Graf -
Heinrich Schuchardt -
Simon Glass