diff mbox series

[v2,1/1] riscv: restore global data pointer in trap handler

Message ID 20200927082411.30177-1-xypron.glpk@gmx.de
State Accepted
Commit c48e9f310b950e39a91cea74b6708dd4fe2eb39c
Delegated to: Andes
Headers show
Series [v2,1/1] riscv: restore global data pointer in trap handler | expand

Commit Message

Heinrich Schuchardt Sept. 27, 2020, 8:24 a.m. UTC 
The gp register is used to store U-Boot's global data pointer. We should
not assume that an UEFI application leaves the gp register unchanged as
the UEFI specifications does not define who is the owner of the gp and tp
registers.

So the following sequence should be followed in the trap handler:

* save the caller's gp register
* restore the global data pointer
* serve interrupts or print crash dump and reset
* restore the caller's gp register

Cc: Abner Chang <abner.chang@hpe.com>
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2:
	Saving and restoring the caller's x3 is already handled in mtrap.S.
	efi_loader.h provides an empty fallback efi_restore_gd() function
	if CONFIG_EFI_LOADER=n.
---
 arch/riscv/lib/interrupts.c | 3 +++
 1 file changed, 3 insertions(+)

--
2.28.0

Comments

Bin Meng Sept. 27, 2020, 8:40 a.m. UTC | #1 
On Sun, Sep 27, 2020 at 4:24 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> The gp register is used to store U-Boot's global data pointer. We should
> not assume that an UEFI application leaves the gp register unchanged as
> the UEFI specifications does not define who is the owner of the gp and tp
> registers.
>
> So the following sequence should be followed in the trap handler:
>
> * save the caller's gp register
> * restore the global data pointer
> * serve interrupts or print crash dump and reset
> * restore the caller's gp register
>
> Cc: Abner Chang <abner.chang@hpe.com>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2:
>         Saving and restoring the caller's x3 is already handled in mtrap.S.
>         efi_loader.h provides an empty fallback efi_restore_gd() function
>         if CONFIG_EFI_LOADER=n.
> ---
>  arch/riscv/lib/interrupts.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c
> index cd47e64487..8ff40f0f36 100644
> --- a/arch/riscv/lib/interrupts.c
> +++ b/arch/riscv/lib/interrupts.c
> @@ -111,6 +111,9 @@ ulong handle_trap(ulong cause, ulong epc, ulong tval, struct pt_regs *regs)
>  {
>         ulong is_irq, irq;
>
> +       /* An UEFI application may have changed gd. Restore U-Boot's gd. */
> +       efi_restore_gd();
> +
>         is_irq = (cause & MCAUSE_INT);
>         irq = (cause & ~MCAUSE_INT);
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>

Does other arch suffer the same issue?
Heinrich Schuchardt Sept. 27, 2020, 8:46 a.m. UTC | #2 
On 9/27/20 10:40 AM, Bin Meng wrote:
> On Sun, Sep 27, 2020 at 4:24 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> The gp register is used to store U-Boot's global data pointer. We should
>> not assume that an UEFI application leaves the gp register unchanged as
>> the UEFI specifications does not define who is the owner of the gp and tp
>> registers.
>>
>> So the following sequence should be followed in the trap handler:
>>
>> * save the caller's gp register
>> * restore the global data pointer
>> * serve interrupts or print crash dump and reset
>> * restore the caller's gp register
>>
>> Cc: Abner Chang <abner.chang@hpe.com>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> v2:
>>         Saving and restoring the caller's x3 is already handled in mtrap.S.
>>         efi_loader.h provides an empty fallback efi_restore_gd() function
>>         if CONFIG_EFI_LOADER=n.
>> ---
>>  arch/riscv/lib/interrupts.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c
>> index cd47e64487..8ff40f0f36 100644
>> --- a/arch/riscv/lib/interrupts.c
>> +++ b/arch/riscv/lib/interrupts.c
>> @@ -111,6 +111,9 @@ ulong handle_trap(ulong cause, ulong epc, ulong tval, struct pt_regs *regs)
>>  {
>>         ulong is_irq, irq;
>>
>> +       /* An UEFI application may have changed gd. Restore U-Boot's gd. */
>> +       efi_restore_gd();
>> +
>>         is_irq = (cause & MCAUSE_INT);
>>         irq = (cause & ~MCAUSE_INT);
>>
>
> Reviewed-by: Bin Meng <bin.meng@windriver.com>
>
> Does other arch suffer the same issue?
>

Hello Bin,

x86 does not use a register of gd.
arm has this statement already.
No further architectures support UEFI.

Best regards

Heinrich
Bin Meng Sept. 27, 2020, 8:51 a.m. UTC | #3 
Hi Heinrich,

On Sun, Sep 27, 2020 at 4:46 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 9/27/20 10:40 AM, Bin Meng wrote:
> > On Sun, Sep 27, 2020 at 4:24 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> The gp register is used to store U-Boot's global data pointer. We should
> >> not assume that an UEFI application leaves the gp register unchanged as
> >> the UEFI specifications does not define who is the owner of the gp and tp
> >> registers.
> >>
> >> So the following sequence should be followed in the trap handler:
> >>
> >> * save the caller's gp register
> >> * restore the global data pointer
> >> * serve interrupts or print crash dump and reset
> >> * restore the caller's gp register
> >>
> >> Cc: Abner Chang <abner.chang@hpe.com>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >> v2:
> >>         Saving and restoring the caller's x3 is already handled in mtrap.S.
> >>         efi_loader.h provides an empty fallback efi_restore_gd() function
> >>         if CONFIG_EFI_LOADER=n.
> >> ---
> >>  arch/riscv/lib/interrupts.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c
> >> index cd47e64487..8ff40f0f36 100644
> >> --- a/arch/riscv/lib/interrupts.c
> >> +++ b/arch/riscv/lib/interrupts.c
> >> @@ -111,6 +111,9 @@ ulong handle_trap(ulong cause, ulong epc, ulong tval, struct pt_regs *regs)
> >>  {
> >>         ulong is_irq, irq;
> >>
> >> +       /* An UEFI application may have changed gd. Restore U-Boot's gd. */
> >> +       efi_restore_gd();
> >> +
> >>         is_irq = (cause & MCAUSE_INT);
> >>         irq = (cause & ~MCAUSE_INT);
> >>
> >
> > Reviewed-by: Bin Meng <bin.meng@windriver.com>
> >
> > Does other arch suffer the same issue?
> >
>
> Hello Bin,
>
> x86 does not use a register of gd.

x86 uses fs segment register to point to gd. Does UEFI not use fs?

> arm has this statement already.
> No further architectures support UEFI.
>

Regards,
Bin
Heinrich Schuchardt Sept. 27, 2020, 12:17 p.m. UTC | #4 
On 9/27/20 10:51 AM, Bin Meng wrote:
> Hi Heinrich,
>
> On Sun, Sep 27, 2020 at 4:46 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 9/27/20 10:40 AM, Bin Meng wrote:
>>> On Sun, Sep 27, 2020 at 4:24 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> The gp register is used to store U-Boot's global data pointer. We should
>>>> not assume that an UEFI application leaves the gp register unchanged as
>>>> the UEFI specifications does not define who is the owner of the gp and tp
>>>> registers.
>>>>
>>>> So the following sequence should be followed in the trap handler:
>>>>
>>>> * save the caller's gp register
>>>> * restore the global data pointer
>>>> * serve interrupts or print crash dump and reset
>>>> * restore the caller's gp register
>>>>
>>>> Cc: Abner Chang <abner.chang@hpe.com>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>> v2:
>>>>         Saving and restoring the caller's x3 is already handled in mtrap.S.
>>>>         efi_loader.h provides an empty fallback efi_restore_gd() function
>>>>         if CONFIG_EFI_LOADER=n.
>>>> ---
>>>>  arch/riscv/lib/interrupts.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c
>>>> index cd47e64487..8ff40f0f36 100644
>>>> --- a/arch/riscv/lib/interrupts.c
>>>> +++ b/arch/riscv/lib/interrupts.c
>>>> @@ -111,6 +111,9 @@ ulong handle_trap(ulong cause, ulong epc, ulong tval, struct pt_regs *regs)
>>>>  {
>>>>         ulong is_irq, irq;
>>>>
>>>> +       /* An UEFI application may have changed gd. Restore U-Boot's gd. */
>>>> +       efi_restore_gd();
>>>> +
>>>>         is_irq = (cause & MCAUSE_INT);
>>>>         irq = (cause & ~MCAUSE_INT);
>>>>
>>>
>>> Reviewed-by: Bin Meng <bin.meng@windriver.com>
>>>
>>> Does other arch suffer the same issue?
>>>
>>
>> Hello Bin,
>>
>> x86 does not use a register of gd.
>
> x86 uses fs segment register to point to gd. Does UEFI not use fs?

The UEFI 2.8B specification is available at
https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf.

Chapter 2.3.2 "IA-32 Platform" and chapter 2.3.4 x64 "PlatformsAll" both
have this sentence: "Other general purpose flag registers are undefined."

I could find no provision that segment registers remain unchanged.

Chapter 2.3.4.3 "Enabling Paging or Alternate Translations" and 2.3.4.3
"Enabling Paging or Alternate Translations in an Application" both allow
UEFI payloads to have their own GDT and IDT. But it seems these have to
be restored by the payload before entering an API call: "An application
with translations enabled can restore firmware required mapping before
each UEFI call."

This implies we have to change multiple places in U-Boot. For ARM and
RISC-V we have routines

* efi_save_gd()
* efi_restore_gd()
* efi_set_gd()
* trace_save_gd();
* trace_swap_gd();

So what we need to do is:

* Save our value of fs in efi_save_gd() and trace_save_gd().
* On API entry from the caller save the value of the callers fs and
restore our value.
* Before returning restore the callers fs.

Best regards

Heinrich

>
>> arm has this statement already.
>> No further architectures support UEFI.
>>
>
> Regards,
> Bin
>
Rick Chen Sept. 28, 2020, 7:45 a.m. UTC | #5 
> From: Heinrich Schuchardt [mailto:xypron.glpk@gmx.de]
> Sent: Sunday, September 27, 2020 4:24 PM
> To: Rick Jian-Zhi Chen(陳建志)
> Cc: Simon Glass; Sean Anderson; Bin Meng; u-boot@lists.denx.de; Alexander Graf; Abner Chang; Heinrich Schuchardt
> Subject: [PATCH v2 1/1] riscv: restore global data pointer in trap handler
>
> The gp register is used to store U-Boot's global data pointer. We should
> not assume that an UEFI application leaves the gp register unchanged as
> the UEFI specifications does not define who is the owner of the gp and tp
> registers.
>
> So the following sequence should be followed in the trap handler:
>
> * save the caller's gp register
> * restore the global data pointer
> * serve interrupts or print crash dump and reset
> * restore the caller's gp register
>
> Cc: Abner Chang <abner.chang@hpe.com>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2:
>         Saving and restoring the caller's x3 is already handled in mtrap.S.
>         efi_loader.h provides an empty fallback efi_restore_gd() function
>         if CONFIG_EFI_LOADER=n.
> ---
>  arch/riscv/lib/interrupts.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Rick Chen <rick@andestech.com>
Heinrich Schuchardt Sept. 28, 2020, 9:01 a.m. UTC | #6 
On 28.09.20 09:45, Rick Chen wrote:
>> From: Heinrich Schuchardt [mailto:xypron.glpk@gmx.de]
>> Sent: Sunday, September 27, 2020 4:24 PM
>> To: Rick Jian-Zhi Chen(陳建志)
>> Cc: Simon Glass; Sean Anderson; Bin Meng; u-boot@lists.denx.de; Alexander Graf; Abner Chang; Heinrich Schuchardt
>> Subject: [PATCH v2 1/1] riscv: restore global data pointer in trap handler
>>
>> The gp register is used to store U-Boot's global data pointer. We should
>> not assume that an UEFI application leaves the gp register unchanged as
>> the UEFI specifications does not define who is the owner of the gp and tp
>> registers.
>>
>> So the following sequence should be followed in the trap handler:
>>
>> * save the caller's gp register
>> * restore the global data pointer
>> * serve interrupts or print crash dump and reset
>> * restore the caller's gp register
>>
>> Cc: Abner Chang <abner.chang@hpe.com>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> v2:
>>         Saving and restoring the caller's x3 is already handled in mtrap.S.
>>         efi_loader.h provides an empty fallback efi_restore_gd() function
>>         if CONFIG_EFI_LOADER=n.
>> ---
>>  arch/riscv/lib/interrupts.c | 3 +++
>>  1 file changed, 3 insertions(+)
>
> Reviewed-by: Rick Chen <rick@andestech.com>
>

Hello Rick,

I have two other corrections for the UEFI sub-system and would like to
add this patch to my pull-request for v2020.10. Is that ok with you?

Best regards

Heinrich
Rick Chen Sept. 29, 2020, 1:21 a.m. UTC | #7 
Hi Heinrich

> On 28.09.20 09:45, Rick Chen wrote:
> >> From: Heinrich Schuchardt [mailto:xypron.glpk@gmx.de]
> >> Sent: Sunday, September 27, 2020 4:24 PM
> >> To: Rick Jian-Zhi Chen(陳建志)
> >> Cc: Simon Glass; Sean Anderson; Bin Meng; u-boot@lists.denx.de; Alexander Graf; Abner Chang; Heinrich Schuchardt
> >> Subject: [PATCH v2 1/1] riscv: restore global data pointer in trap handler
> >>
> >> The gp register is used to store U-Boot's global data pointer. We should
> >> not assume that an UEFI application leaves the gp register unchanged as
> >> the UEFI specifications does not define who is the owner of the gp and tp
> >> registers.
> >>
> >> So the following sequence should be followed in the trap handler:
> >>
> >> * save the caller's gp register
> >> * restore the global data pointer
> >> * serve interrupts or print crash dump and reset
> >> * restore the caller's gp register
> >>
> >> Cc: Abner Chang <abner.chang@hpe.com>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >> v2:
> >>         Saving and restoring the caller's x3 is already handled in mtrap.S.
> >>         efi_loader.h provides an empty fallback efi_restore_gd() function
> >>         if CONFIG_EFI_LOADER=n.
> >> ---
> >>  arch/riscv/lib/interrupts.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >
> > Reviewed-by: Rick Chen <rick@andestech.com>
> >
>
> Hello Rick,
>
> I have two other corrections for the UEFI sub-system and would like to
> add this patch to my pull-request for v2020.10. Is that ok with you?

I am OK with it.

Thanks,
Rick

>
> Best regards
>
> Heinrich
diff mbox series

Patch

diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c
index cd47e64487..8ff40f0f36 100644
--- a/arch/riscv/lib/interrupts.c
+++ b/arch/riscv/lib/interrupts.c
@@ -111,6 +111,9 @@  ulong handle_trap(ulong cause, ulong epc, ulong tval, struct pt_regs *regs)
 {
 	ulong is_irq, irq;

+	/* An UEFI application may have changed gd. Restore U-Boot's gd. */
+	efi_restore_gd();
+
 	is_irq = (cause & MCAUSE_INT);
 	irq = (cause & ~MCAUSE_INT);