Re: [Qemu-ppc] [RFC NO-MERGE 04/12] target/ppc: Rework hash mmu page fault code and add defines for clarity

[David Gibson]

On Fri, Feb 17, 2017 at 04:08:04PM +1100, Suraj Jitindar Singh wrote:
> The hash mmu page fault handling code is responsible for generating ISIs
> and DSIs when access permissions cause an access to fail. Part of this
> involves setting the srr1 or dsisr registers to indicate what causes the
> access to fail. Add defines for the bit fields of these registers and
> rework the code to use these new defines in order to improve readability
> and code clarity.
> 
> While we're here, update what is logged when an access fails to include
> information as to what caused to access to fail for debug purposes.
> 
> Signed-off-by: Suraj Jitindar Singh <address@hidden>

Acked-by: David Gibson <address@hidden>

I'd love to apply it right now, but IIUC it will get conflicts without
the earlier patches in the series.

> ---
>  target/ppc/mmu-hash64.c | 30 +++++++++++++++---------------
>  target/ppc/mmu.h        | 13 +++++++++++--
>  2 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index a7ffedf..b5723d3 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -449,7 +449,7 @@ static int ppc_hash64_amr_prot(PowerPCCPU *cpu, 
> ppc_hash_pte64_t pte)
>      case POWERPC_MMU_2_07a:
>      case POWERPC_MMU_3_00:
>          prot &= ~ppc_hash64_iamr_prot(cpu, key);
> -     break;
> +        break;
>      default:
>          break;
>      }
> @@ -737,7 +737,7 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr 
> eaddr,
>      hwaddr pte_offset;
>      ppc_hash_pte64_t pte;
>      int exec_prot, pp_prot, amr_prot, prot;
> -    uint64_t new_pte1, dsisr;
> +    uint64_t new_pte1;
>      const int need_prot[] = {PAGE_READ, PAGE_WRITE, PAGE_EXEC};
>      hwaddr raddr;
>  
> @@ -778,11 +778,11 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr 
> eaddr,
>              } else {
>                  /* The access failed, generate the approriate interrupt */
>                  if (rwx == 2) {
> -                    ppc_hash64_set_isi(cs, env, 0x08000000);
> +                    ppc_hash64_set_isi(cs, env, SRR1_PROTFAULT);
>                  } else {
> -                    dsisr = 0x08000000;
> +                    int dsisr = DSISR_PROTFAULT;
>                      if (rwx == 1) {
> -                        dsisr |= 0x02000000;
> +                        dsisr |= DSISR_ISSTORE;
>                      }
>                      ppc_hash64_set_dsi(cs, env, eaddr, dsisr);
>                  }
> @@ -821,19 +821,19 @@ skip_slb_search:
>  
>      /* 3. Check for segment level no-execute violation */
>      if ((rwx == 2) && (slb->vsid & SLB_VSID_N)) {
> -        ppc_hash64_set_isi(cs, env, 0x10000000);
> +        ppc_hash64_set_isi(cs, env, SRR1_NOEXEC_GUARD);
>          return 1;
>      }
>  
>      /* 4. Locate the PTE in the hash table */
>      pte_offset = ppc_hash64_htab_lookup(cpu, slb, eaddr, &pte, &apshift);
>      if (pte_offset == -1) {
> -        dsisr = 0x40000000;
>          if (rwx == 2) {
> -            ppc_hash64_set_isi(cs, env, dsisr);
> +            ppc_hash64_set_isi(cs, env, SRR1_NOPTE);
>          } else {
> +            int dsisr = DSISR_NOPTE;
>              if (rwx == 1) {
> -                dsisr |= 0x02000000;
> +                dsisr |= DSISR_ISSTORE;
>              }
>              ppc_hash64_set_dsi(cs, env, eaddr, dsisr);
>          }
> @@ -856,25 +856,25 @@ skip_slb_search:
>          qemu_log_mask(CPU_LOG_MMU, "PTE access rejected\n");
>          if (rwx == 2) {
>              int srr1 = 0;
> -         if (PAGE_EXEC & ~exec_prot) {
> +            if (PAGE_EXEC & ~exec_prot) {
>                  srr1 |= SRR1_NOEXEC_GUARD; /* Access violates noexec or 
> guard */
>              } else if (PAGE_EXEC & ~pp_prot) { /* noexec takes precedence */
>                  srr1 |= SRR1_PROTFAULT; /* Access violates access authority 
> */
>              }
>              if (PAGE_EXEC & ~amr_prot) {
>                  srr1 |= SRR1_IAMR; /* Access violates virt pg class key prot 
> */
> -         }
> +            }
>              ppc_hash64_set_isi(cs, env, srr1);
>          } else {
> -            dsisr = 0;
> +            int dsisr = 0;
>              if (need_prot[rwx] & ~pp_prot) {
> -                dsisr |= 0x08000000;
> +                dsisr |= DSISR_PROTFAULT;
>              }
>              if (rwx == 1) {
> -                dsisr |= 0x02000000;
> +                dsisr |= DSISR_ISSTORE;
>              }
>              if (need_prot[rwx] & ~amr_prot) {
> -                dsisr |= 0x00200000;
> +                dsisr |= DSISR_AMR;
>              }
>              ppc_hash64_set_dsi(cs, env, eaddr, dsisr);
>          }
> diff --git a/target/ppc/mmu.h b/target/ppc/mmu.h
> index cfda7f1..f717604 100644
> --- a/target/ppc/mmu.h
> +++ b/target/ppc/mmu.h
> @@ -27,11 +27,20 @@
>  
>  /* Interrupt Fields */
>  
> +/* DSISR */
> +#define DSISR_NOPTE              0x40000000
> +/* Not permitted by access authority of encoded access authority */
> +#define DSISR_PROTFAULT          0x08000000
> +#define DSISR_ISSTORE            0x02000000
> +/* Not permitted by virtual page class key protection */
> +#define DSISR_AMR                0x00200000
> +
>  /* SRR1 */
> +#define SRR1_NOPTE               DSISR_NOPTE
>  /* Not permitted due to no-execute or guard bit set */
>  #define SRR1_NOEXEC_GUARD        0x10000000
> -#define SRR1_PROTFAULT                0x08000000
> -#define SRR1_IAMR             0x00200000
> +#define SRR1_PROTFAULT           DSISR_PROTFAULT
> +#define SRR1_IAMR                DSISR_AMR
>  
>  #ifdef TARGET_PPC64
>  

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature