Change in ...coreboot[master]: riscv: fix run_payload

[Xiang Wang (Code Review)]

Xiang Wang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33462

Change subject: riscv: fix run_payload
......................................................................

riscv: fix run_payload

1. Trap vector base address point to the payload
2. Disable Interrupt
3. Fix bug in setting MPP field of mstatus

Change-Id: Iaab595f916949c57104ec00f8b06ea047fe76bba
Signed-off-by: Xiang Wang 
---
M src/arch/riscv/payload.c
1 file changed, 20 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/33462/1

diff --git a/src/arch/riscv/payload.c b/src/arch/riscv/payload.c
index f3ed5a4..903e8a6 100644
--- a/src/arch/riscv/payload.c
+++ b/src/arch/riscv/payload.c
@@ -19,23 +19,38 @@
 #include <arch/boot.h>
 #include <arch/encoding.h>
 #include <console/console.h>
+#include <vm.h>

void run_payload(struct prog *prog, void *fdt, int payload_mode)
 {
    void (*doit)(int hart_id, void *fdt) = prog_entry(prog);
    int hart_id = read_csr(mhartid);
    uintptr_t status = read_csr(mstatus);
-	status &= ~MSTATUS_MPIE;
-	status &= ~MSTATUS_MPP;
+	status = INSERT_FIELD(status, MSTATUS_MPIE, 0);
    switch (payload_mode) {
    case RISCV_PAYLOAD_MODE_U:
+		status = INSERT_FIELD(status, MSTATUS_MPP, PRV_U);
+		/* Trap vector base address point to the payload */
+		write_csr(utvec, doit);
+		/* disable U-Mode interrupt */
+		write_csr(uie, 0);
    	break;
    case RISCV_PAYLOAD_MODE_S:
-		status |= MSTATUS_SPP;
+		status = INSERT_FIELD(status, MSTATUS_MPP, PRV_S);
+		/* Trap vector base address point to the payload */
+		write_csr(stvec, doit);
+		/* disable S-Mode interrupt */
+		write_csr(sie, 0);
+		/* disable MMU */
+		write_csr(satp, 0);
    	break;
    case RISCV_PAYLOAD_MODE_M:
-		doit(hart_id, fdt);
-		return;
+		status = INSERT_FIELD(status, MSTATUS_MPP, PRV_M);
+		/* Trap vector base address point to the payload */
+		write_csr(mtvec, doit);
+		/* disable M-Mode interrupt */
+		write_csr(mie, 0);
+		break;
    default:
    	die("wrong privilege level for payload");
    	break;

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33462 )

Change subject: riscv: fix run_payload
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/33462/2/src/arch/riscv/payload.c 
File src/arch/riscv/payload.c:

https://review.coreboot.org/#/c/33462/2/src/arch/riscv/payload.c@44 
PS2, Line 44: 		/* disable MMU */
why only disable MMU for S-Mode?
Shouldn't this always be done?

[Xiang Wang (Code Review)]

Xiang Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33462 )

Change subject: riscv: fix run_payload
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/33462/2/src/arch/riscv/payload.c 
File src/arch/riscv/payload.c:

https://review.coreboot.org/#/c/33462/2/src/arch/riscv/payload.c@44 
PS2, Line 44: 		/* disable MMU */
> why only disable MMU for S-Mode? […]

satp has no effect on M-Mode, so there is no need to consider returning M-Mode. If return to U-Mode, the system should not have S-Mode, so don't consider it.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33462 )

Change subject: riscv: fix run_payload
......................................................................

Patch Set 2:

(1 comment)

I just wanted to test the commit, but it doesn't compile with GCC 6.3, trying GCC 8.3 now

https://review.coreboot.org/#/c/33462/2/src/arch/riscv/payload.c 
File src/arch/riscv/payload.c:

https://review.coreboot.org/#/c/33462/2/src/arch/riscv/payload.c@44 
PS2, Line 44: 		/* disable MMU */
> satp has no effect on M-Mode, so there is no need to consider returning M-Mode. […]

OK, thanks for explaining

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33462 )

Change subject: riscv: fix run_payload
......................................................................

Patch Set 2: Code-Review+2

[Philipp Hug (Code Review)]

Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33462 )

Change subject: riscv: fix run_payload
......................................................................

Patch Set 2: Code-Review+1

Thanks!
Please change "riscv: fix run_payload" to something more specific.
Also mention how the change has been tested.

[Xiang Wang (Code Review)]

Xiang Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33462 )

Change subject: riscv: fix run_payload
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review+1
>
> Thanks!
> Please change "riscv: fix run_payload" to something more specific.

Do you have suggest?
> Also mention how the change has been tested.

I read this code and found a logic error when setting the MPP field of mstatus.
If you do not return via mret, MPIE will not work.
I have only tested the situation of return to M-Mode.

[Philipp Hug (Code Review)]

Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33462 )

Change subject: riscv: fix run_payload
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2: Code-Review+1
> >
> > Thanks!
> > Please change "riscv: fix run_payload" to something more specific.
>
> Do you have suggest?
> > Also mention how the change has been tested.
>
> I read this code and found a logic error when setting the MPP field of mstatus.
> If you do not return via mret, MPIE will not work.
> I have only tested the situation of return to M-Mode.

e.g.

riscv: use mret to invoke M-mode payload and disable interrupts

Fixes a logic error that sets MPIE, but didn't use mret to return to the payload. This left MIE set to an undefined value.

Now all modes are handled the same way:
* Trap vector base address point to the payload
* Disable Interrupt
* Return to payload using mret

TEST=Run an M-mode payload

Change in ...coreboot[master]: riscv: use mret to invoke M-mode payload and disable interrupts

[Xiang Wang (Code Review)]

Hello Patrick Rudolph, Patrick Rudolph, build bot (Jenkins), Philipp Hug,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33462

to look at the new patch set (#3).

Change subject: riscv: use mret to invoke M-mode payload and disable interrupts
......................................................................

riscv: use mret to invoke M-mode payload and disable interrupts

Fixes a logic error that sets MPIE, but didn't use mret to return to the payload.
This left MIE set to an undefined value.

Now all modes are handled the same way:
- Trap vector base address point to the payload
- Disable Interrupt
- Return to payload using mret

TEST=Run an M-mode payload

Change-Id: Iaab595f916949c57104ec00f8b06ea047fe76bba
Signed-off-by: Xiang Wang 
---
M src/arch/riscv/payload.c
1 file changed, 20 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/33462/3

Change in ...coreboot[master]: riscv: use mret to invoke M-mode payload and disable interrupts

[Xiang Wang (Code Review)]

Xiang Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33462 )

Change subject: riscv: use mret to invoke M-mode payload and disable interrupts
......................................................................

Patch Set 3:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2: Code-Review+1
> > >
> > > Thanks!
> > > Please change "riscv: fix run_payload" to something more specific.
> >
> > Do you have suggest?
> > > Also mention how the change has been tested.
> >
> > I read this code and found a logic error when setting the MPP field of mstatus.
> > If you do not return via mret, MPIE will not work.
> > I have only tested the situation of return to M-Mode.
>
> e.g.
>
> riscv: use mret to invoke M-mode payload and disable interrupts
>
> Fixes a logic error that sets MPIE, but didn't use mret to return to the payload. This left MIE set to an undefined value.
>
> Now all modes are handled the same way:
> Trap vector base address point to the payload
> Disable Interrupt
> Return to payload using mret
>
> TEST=Run an M-mode payload

Thank you very much!

Change in ...coreboot[master]: riscv: use mret to invoke M-mode payload and disable interrupts

[Philipp Hug (Code Review)]

Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33462 )

Change subject: riscv: use mret to invoke M-mode payload and disable interrupts
......................................................................

Patch Set 3: Code-Review+2

Thanks

Change in ...coreboot[master]: riscv: use mret to invoke M-mode payload and disable interrupts

[Patrick Rudolph (Code Review)]

Patrick Rudolph has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33462 )

Change subject: riscv: use mret to invoke M-mode payload and disable interrupts
......................................................................

riscv: use mret to invoke M-mode payload and disable interrupts

Fixes a logic error that sets MPIE, but didn't use mret to return to the payload.
This left MIE set to an undefined value.

Now all modes are handled the same way:
- Trap vector base address point to the payload
- Disable Interrupt
- Return to payload using mret

TEST=Run an M-mode payload

Change-Id: Iaab595f916949c57104ec00f8b06ea047fe76bba
Signed-off-by: Xiang Wang 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33462
Reviewed-by: Philipp Hug 
Reviewed-by: Patrick Rudolph 
Reviewed-by: Patrick Rudolph 
Tested-by: build bot (Jenkins) 
---
M src/arch/riscv/payload.c
1 file changed, 20 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Rudolph: Looks good to me, approved
  Patrick Rudolph: Looks good to me, but someone else must approve
  Philipp Hug: Looks good to me, approved

diff --git a/src/arch/riscv/payload.c b/src/arch/riscv/payload.c
index f3ed5a4..903e8a6 100644
--- a/src/arch/riscv/payload.c
+++ b/src/arch/riscv/payload.c
@@ -19,23 +19,38 @@
 #include <arch/boot.h>
 #include <arch/encoding.h>
 #include <console/console.h>
+#include <vm.h>

void run_payload(struct prog *prog, void *fdt, int payload_mode)
 {
    void (*doit)(int hart_id, void *fdt) = prog_entry(prog);
    int hart_id = read_csr(mhartid);
    uintptr_t status = read_csr(mstatus);
-	status &= ~MSTATUS_MPIE;
-	status &= ~MSTATUS_MPP;
+	status = INSERT_FIELD(status, MSTATUS_MPIE, 0);
    switch (payload_mode) {
    case RISCV_PAYLOAD_MODE_U:
+		status = INSERT_FIELD(status, MSTATUS_MPP, PRV_U);
+		/* Trap vector base address point to the payload */
+		write_csr(utvec, doit);
+		/* disable U-Mode interrupt */
+		write_csr(uie, 0);
    	break;
    case RISCV_PAYLOAD_MODE_S:
-		status |= MSTATUS_SPP;
+		status = INSERT_FIELD(status, MSTATUS_MPP, PRV_S);
+		/* Trap vector base address point to the payload */
+		write_csr(stvec, doit);
+		/* disable S-Mode interrupt */
+		write_csr(sie, 0);
+		/* disable MMU */
+		write_csr(satp, 0);
    	break;
    case RISCV_PAYLOAD_MODE_M:
-		doit(hart_id, fdt);
-		return;
+		status = INSERT_FIELD(status, MSTATUS_MPP, PRV_M);
+		/* Trap vector base address point to the payload */
+		write_csr(mtvec, doit);
+		/* disable M-Mode interrupt */
+		write_csr(mie, 0);
+		break;
    default:
    	die("wrong privilege level for payload");
    	break;