Re: [PATCH for-7.1 v6 25/51] target/nios2: Clean up handling of tlbmisc in do_exception

[Peter Maydell]

On Thu, 17 Mar 2022 at 05:23, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The 4 lower bits, D, PERM, BAD, DBL, are unconditionally set on any
> exception with EH=0, or so says Table 42 (Processor Status After
> Taking Exception).
>
> We currently do not set PERM or BAD at all, and only set/clear
> DBL for tlb miss, and do not clear DBL for any other exception.
>
> It is a bit confusing to set D in tlb_fill and the rest during
> do_interrupt, so move the setting of D to do_interrupt as well.
> To do this, split EXP_TLBD into two cases, EXCP_TLB_X and EXCP_TLB_D,
> which allows us to distinguish them during do_interrupt.  Choose
> a value for EXCP_TLB_D such that when truncated it produces the
> correct value for exception.CAUSE.
>
> Rename EXCP_TLB[RWX] to EXCP_PERM_[RWX], to emphasize that the
> exception is permissions related.  Rename EXCP_SUPER[AD] to
> EXCP_SUPERA_[DX] to emphasize that they are both "supervisor
> address" exceptions, data and execute.
>
> Retain the setting of tlbmisc.WE for the fast-tlb-miss path, as it
> is being relied upon, but remove it from the permission path.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/nios2/cpu.h    |  13 +++---
>  target/nios2/helper.c | 102 +++++++++++++++++++++++++++++-------------
>  2 files changed, 77 insertions(+), 38 deletions(-)
>
> diff --git a/target/nios2/cpu.h b/target/nios2/cpu.h
> index d003af5afc..c925cdd8e3 100644
> --- a/target/nios2/cpu.h
> +++ b/target/nios2/cpu.h
> @@ -166,13 +166,14 @@ FIELD(CR_TLBMISC, EE, 24, 1)
>  #define EXCP_UNALIGN  6
>  #define EXCP_UNALIGND 7
>  #define EXCP_DIV      8
> -#define EXCP_SUPERA   9
> +#define EXCP_SUPERA_X 9
>  #define EXCP_SUPERI   10
> -#define EXCP_SUPERD   11
> -#define EXCP_TLBD     12
> -#define EXCP_TLBX     13
> -#define EXCP_TLBR     14
> -#define EXCP_TLBW     15
> +#define EXCP_SUPERA_D 11
> +#define EXCP_TLB_X    12
> +#define EXCP_TLB_D    (0x1000 | EXCP_TLB_X)
> +#define EXCP_PERM_X   13
> +#define EXCP_PERM_R   14
> +#define EXCP_PERM_W   15
>  #define EXCP_MPUI     16
>  #define EXCP_MPUD     17
>
> diff --git a/target/nios2/helper.c b/target/nios2/helper.c
> index afbafd1fdc..8b69918ba3 100644
> --- a/target/nios2/helper.c
> +++ b/target/nios2/helper.c
> @@ -49,7 +49,8 @@ void nios2_cpu_record_sigsegv(CPUState *cs, vaddr addr,
>
>  #else /* !CONFIG_USER_ONLY */
>
> -static void do_exception(Nios2CPU *cpu, uint32_t exception_addr, bool 
> is_break)
> +static void do_exception(Nios2CPU *cpu, uint32_t exception_addr,
> +                         uint32_t tlbmisc_set, bool is_break)
>  {
>      CPUNios2State *env = &cpu->env;
>      CPUState *cs = CPU(cpu);
> @@ -68,6 +69,16 @@ static void do_exception(Nios2CPU *cpu, uint32_t 
> exception_addr, bool is_break)
>
>          if (cpu->mmu_present) {
>              new_status |= CR_STATUS_EH;
> +
> +            /*
> +             * There are 4 bits that are always written.
> +             * Explicitly clear them, to be set via the argument.
> +             */
> +            env->ctrl[CR_TLBMISC] &= ~(CR_TLBMISC_D |
> +                                       CR_TLBMISC_PERM |
> +                                       CR_TLBMISC_BAD |
> +                                       CR_TLBMISC_DBL);
> +            env->ctrl[CR_TLBMISC] |= tlbmisc_set;
>          }
>      }
>
> @@ -83,13 +94,14 @@ static void do_exception(Nios2CPU *cpu, uint32_t 
> exception_addr, bool is_break)
>
>  static void do_iic_irq(Nios2CPU *cpu)
>  {
> -    do_exception(cpu, cpu->exception_addr, false);
> +    do_exception(cpu, cpu->exception_addr, 0, false);
>  }
>
>  void nios2_cpu_do_interrupt(CPUState *cs)
>  {
>      Nios2CPU *cpu = NIOS2_CPU(cs);
>      CPUNios2State *env = &cpu->env;
> +    uint32_t tlbmisc_set = 0;
>
>      if (qemu_loglevel_mask(CPU_LOG_INT)) {
>          const char *name = NULL;
> @@ -98,20 +110,21 @@ void nios2_cpu_do_interrupt(CPUState *cs)
>          case EXCP_IRQ:
>              name = "interrupt";
>              break;
> -        case EXCP_TLBD:
> +        case EXCP_TLB_X:
> +        case EXCP_TLB_D:
>              if (env->ctrl[CR_STATUS] & CR_STATUS_EH) {
>                  name = "TLB MISS (double)";
>              } else {
>                  name = "TLB MISS (fast)";
>              }
>              break;
> -        case EXCP_TLBR:
> -        case EXCP_TLBW:
> -        case EXCP_TLBX:
> +        case EXCP_PERM_R:
> +        case EXCP_PERM_W:
> +        case EXCP_PERM_X:
>              name = "TLB PERM";
>              break;
> -        case EXCP_SUPERA:
> -        case EXCP_SUPERD:
> +        case EXCP_SUPERA_X:
> +        case EXCP_SUPERA_D:
>              name = "SUPERVISOR (address)";
>              break;
>          case EXCP_SUPERI:
> @@ -149,38 +162,60 @@ void nios2_cpu_do_interrupt(CPUState *cs)
>          do_iic_irq(cpu);
>          break;
>
> -    case EXCP_TLBD:
> -        if ((env->ctrl[CR_STATUS] & CR_STATUS_EH) == 0) {
> -            env->ctrl[CR_TLBMISC] &= ~CR_TLBMISC_DBL;
> -            env->ctrl[CR_TLBMISC] |= CR_TLBMISC_WE;
> -            do_exception(cpu, cpu->fast_tlb_miss_addr, false);
> +    case EXCP_TLB_D:
> +        tlbmisc_set = CR_TLBMISC_D;
> +        /* fall through */
> +    case EXCP_TLB_X:
> +        if (env->ctrl[CR_STATUS] & CR_STATUS_EH) {
> +            tlbmisc_set |= CR_TLBMISC_DBL;
> +            /*
> +             * Normally, we don't write to tlbmisc unless !EH,
> +             * so do it manually for the double-tlb miss exception.
> +             */
> +            env->ctrl[CR_TLBMISC] &= ~(CR_TLBMISC_D |
> +                                       CR_TLBMISC_PERM |
> +                                       CR_TLBMISC_BAD);
> +            env->ctrl[CR_TLBMISC] |= tlbmisc_set;
> +            do_exception(cpu, cpu->exception_addr, 0, false);
>          } else {
> -            env->ctrl[CR_TLBMISC] |= CR_TLBMISC_DBL;
> -            do_exception(cpu, cpu->exception_addr, false);
> +            /*
> +             * ??? Implicitly setting tlbmisc.WE for the fast-tlb-miss
> +             * handler appears to be out of spec.  But, the linux kernel
> +             * handler relies on it, writing to tlbacc without first
> +             * setting tlbmisc.WE.
> +             */

On page 3-21 of
https://www.intel.com/content/dam/www/programmable/us/en/pdfs/literature/hb/nios2/n2cpu_nii5v1_01.pdf
the description of the WE flag says
"Hardware sets the WE flag to one on a TLB permission violation
exception, and on a TLB miss exception when status.EH = 0."

This is the second of those two cases, isn't it ?

> +            tlbmisc_set |= CR_TLBMISC_WE;
> +            do_exception(cpu, cpu->fast_tlb_miss_addr, tlbmisc_set, false);
>          }
>          break;

> -    case EXCP_TLBR:
> -    case EXCP_TLBW:
> -    case EXCP_TLBX:
> -        if ((env->ctrl[CR_STATUS] & CR_STATUS_EH) == 0) {
> -            env->ctrl[CR_TLBMISC] |= CR_TLBMISC_WE;
> -        }
> -        do_exception(cpu, cpu->exception_addr, false);
> +    case EXCP_PERM_R:
> +    case EXCP_PERM_W:
> +        tlbmisc_set = CR_TLBMISC_D;
> +        /* fall through */
> +    case EXCP_PERM_X:
> +        tlbmisc_set |= CR_TLBMISC_PERM;
> +        do_exception(cpu, cpu->exception_addr, tlbmisc_set, false);

And this change is incorrectly dropping the "sets WE on
TLB permission violation exception" part.

Other than that,

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM