diff mbox series

[v4,12/33] target/nios2: Use hw/registerfields.h for CR_EXCEPTION fields

Message ID 20220308072005.307955-13-richard.henderson@linaro.org
State New
Headers show
Series target/nios2: Shadow register set, EIC and VIC | expand

Commit Message

Richard Henderson March 8, 2022, 7:19 a.m. UTC 
Sink the set of env->exception to the end of nios2_cpu_do_interrupt.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/nios2/cpu.h    |  4 ++++
 target/nios2/helper.c | 24 +++---------------------
 2 files changed, 7 insertions(+), 21 deletions(-)

Comments

Peter Maydell March 8, 2022, 10:12 a.m. UTC | #1 
On Tue, 8 Mar 2022 at 07:20, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Sink the set of env->exception to the end of nios2_cpu_do_interrupt.

This splits the two things the patch is doing between the subject line
and the commit message body; the subject is supposed to be a summary
and the body should be able to stand alone without the subject.

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

>      }
> +
> +    env->exception = FIELD_DP32(env->exception, CR_EXCEPTION, CAUSE,
> +                                cs->exception_index);
>  }

This is a behaviour change in the semihosting case, which
previously did not change env->exception and now does.
Since that's guest visible I think it's not correct.
You could fix that by making the semihosting handling end
with 'return' rather than 'break'.

thanks
-- PMM
Richard Henderson March 8, 2022, 7:36 p.m. UTC | #2 
On 3/8/22 00:12, Peter Maydell wrote:
> On Tue, 8 Mar 2022 at 07:20, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Sink the set of env->exception to the end of nios2_cpu_do_interrupt.
> 
> This splits the two things the patch is doing between the subject line
> and the commit message body; the subject is supposed to be a summary
> and the body should be able to stand alone without the subject.

Yea, I should merge this sink to patch 28, which does other cleanup to this function.

>> +    env->exception = FIELD_DP32(env->exception, CR_EXCEPTION, CAUSE,
>> +                                cs->exception_index);
>>   }
> 
> This is a behaviour change in the semihosting case, which
> previously did not change env->exception and now does.
> Since that's guest visible I think it's not correct.
> You could fix that by making the semihosting handling end
> with 'return' rather than 'break'.

Yea, poor patch ordering, as you discovered.


r~
diff mbox series

Patch

diff --git a/target/nios2/cpu.h b/target/nios2/cpu.h
index 26618baa70..35b4d88859 100644
--- a/target/nios2/cpu.h
+++ b/target/nios2/cpu.h
@@ -105,6 +105,10 @@  FIELD(CR_STATUS, NMI, 22, 1)
 #define CR_CPUID         5
 #define CR_CTL6          6
 #define CR_EXCEPTION     7
+
+FIELD(CR_EXCEPTION, CAUSE, 2, 5)
+FIELD(CR_EXCEPTION, ECCFTL, 31, 1)
+
 #define CR_PTEADDR       8
 #define   CR_PTEADDR_PTBASE_SHIFT 22
 #define   CR_PTEADDR_PTBASE_MASK  (0x3FF << CR_PTEADDR_PTBASE_SHIFT)
diff --git a/target/nios2/helper.c b/target/nios2/helper.c
index 3c49b0cfbf..eb354f78e2 100644
--- a/target/nios2/helper.c
+++ b/target/nios2/helper.c
@@ -64,9 +64,6 @@  void nios2_cpu_do_interrupt(CPUState *cs)
         env->status |= CR_STATUS_IH;
         env->status &= ~(CR_STATUS_PIE | CR_STATUS_U);
 
-        env->exception &= ~(0x1F << 2);
-        env->exception |= (cs->exception_index & 0x1F) << 2;
-
         env->regs[R_EA] = env->pc + 4;
         env->pc = cpu->exception_addr;
         break;
@@ -83,9 +80,6 @@  void nios2_cpu_do_interrupt(CPUState *cs)
             env->status |= CR_STATUS_EH;
             env->status &= ~(CR_STATUS_PIE | CR_STATUS_U);
 
-            env->exception &= ~(0x1F << 2);
-            env->exception |= (cs->exception_index & 0x1F) << 2;
-
             env->tlbmisc &= ~CR_TLBMISC_DBL;
             env->tlbmisc |= CR_TLBMISC_WR;
 
@@ -98,9 +92,6 @@  void nios2_cpu_do_interrupt(CPUState *cs)
             env->status |= CR_STATUS_EH;
             env->status &= ~(CR_STATUS_PIE | CR_STATUS_U);
 
-            env->exception &= ~(0x1F << 2);
-            env->exception |= (cs->exception_index & 0x1F) << 2;
-
             env->tlbmisc |= CR_TLBMISC_DBL;
 
             env->pc = cpu->exception_addr;
@@ -116,9 +107,6 @@  void nios2_cpu_do_interrupt(CPUState *cs)
         env->status |= CR_STATUS_EH;
         env->status &= ~(CR_STATUS_PIE | CR_STATUS_U);
 
-        env->exception &= ~(0x1F << 2);
-        env->exception |= (cs->exception_index & 0x1F) << 2;
-
         if ((env->status & CR_STATUS_EH) == 0) {
             env->tlbmisc |= CR_TLBMISC_WR;
         }
@@ -140,9 +128,6 @@  void nios2_cpu_do_interrupt(CPUState *cs)
         env->status |= CR_STATUS_EH;
         env->status &= ~(CR_STATUS_PIE | CR_STATUS_U);
 
-        env->exception &= ~(0x1F << 2);
-        env->exception |= (cs->exception_index & 0x1F) << 2;
-
         env->pc = cpu->exception_addr;
         break;
 
@@ -158,9 +143,6 @@  void nios2_cpu_do_interrupt(CPUState *cs)
         env->status |= CR_STATUS_EH;
         env->status &= ~(CR_STATUS_PIE | CR_STATUS_U);
 
-        env->exception &= ~(0x1F << 2);
-        env->exception |= (cs->exception_index & 0x1F) << 2;
-
         env->pc = cpu->exception_addr;
         break;
 
@@ -183,9 +165,6 @@  void nios2_cpu_do_interrupt(CPUState *cs)
         env->status |= CR_STATUS_EH;
         env->status &= ~(CR_STATUS_PIE | CR_STATUS_U);
 
-        env->exception &= ~(0x1F << 2);
-        env->exception |= (cs->exception_index & 0x1F) << 2;
-
         env->pc = cpu->exception_addr;
         break;
 
@@ -194,6 +173,9 @@  void nios2_cpu_do_interrupt(CPUState *cs)
                   cs->exception_index);
         break;
     }
+
+    env->exception = FIELD_DP32(env->exception, CR_EXCEPTION, CAUSE,
+                                cs->exception_index);
 }
 
 hwaddr nios2_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)