Re: [Qemu-ppc] [RFC NO-MERGE 03/12] target/ppc: Move no-execute and guarded page checking into new function

[Suraj Jitindar Singh]

On Mon, 2017-02-20 at 11:38 +1100, David Gibson wrote:
> On Fri, Feb 17, 2017 at 04:08:03PM +1100, Suraj Jitindar Singh wrote:
> > 
> > A pte entry has bit fields which can be used to make a page no-
> > execute or
> > guarded, if either of these bits are set then an instruction access
> > to this
> > page will fail. Currently these bits are checked with the pp_prot
> > function
> > however the ISA specifies that the access authority controlled by
> > the
> > key-pp value pair should only be checked on an instruction access
> > after
> > the no-execute and guard bits have already been verified to permit
> > the
> > access.
> > 
> > Move the no-execute and guard bit checking into a new separate
> > function.
> > Note that we can remove the check for the no-execute bit in the slb
> > entry
> > since this check was already performed above when we obtained the
> > slb
> > entry.
> > 
> > In the event that the no-execute or guard bits are set, an ISI
> > should be
> > generated with the SRR1_NOEXEC_GUARD (0x10000000) bit set in srr1.
> > Add a
> > define for this for clarity.
> > 
> > Signed-off-by: Suraj Jitindar Singh <address@hidden>
> > ---
> >  target/ppc/mmu-hash64.c | 25 ++++++++++++++++---------
> >  target/ppc/mmu.h        |  2 ++
> >  2 files changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > index de507b9..a7ffedf 100644
> > --- a/target/ppc/mmu-hash64.c
> > +++ b/target/ppc/mmu-hash64.c
> > @@ -338,6 +338,14 @@ void ppc_hash64_set_external_hpt(PowerPCCPU
> > *cpu, void *hpt, int shift,
> >      }
> >  }
> >  
> > +/* Check No-Execute or Guarded Storage */
> > +static inline bool ppc_hash64_pte_noexec_guard(PowerPCCPU *cpu,
> > +                                               ppc_hash_pte64_t
> > pte)
> > +{
> > +    return (pte.pte1 & HPTE64_R_N) || (pte.pte1 & HPTE64_R_G);
> > +}
> > +
> > +/* Check Basic Storage Protection */
> >  static int ppc_hash64_pte_prot(PowerPCCPU *cpu,
> >                                 ppc_slb_t *slb, ppc_hash_pte64_t
> > pte)
> >  {
> > @@ -381,12 +389,6 @@ static int ppc_hash64_pte_prot(PowerPCCPU
> > *cpu,
> >          }
> >      }
> >  
> > -    /* No execute if either noexec or guarded bits set */
> > -    if (!(pte.pte1 & HPTE64_R_N) || (pte.pte1 & HPTE64_R_G)
> > -        || (slb->vsid & SLB_VSID_N)) {
> > -        prot |= PAGE_EXEC;
> > -    }
> > -
> >      return prot;
> >  }
> >  
> > @@ -734,7 +736,7 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU
> > *cpu, vaddr eaddr,
> >      unsigned apshift;
> >      hwaddr pte_offset;
> >      ppc_hash_pte64_t pte;
> > -    int pp_prot, amr_prot, prot;
> > +    int exec_prot, pp_prot, amr_prot, prot;
> >      uint64_t new_pte1, dsisr;
> >      const int need_prot[] = {PAGE_READ, PAGE_WRITE, PAGE_EXEC};
> >      hwaddr raddr;
> > @@ -842,16 +844,21 @@ skip_slb_search:
> >  
> >      /* 5. Check access permissions */
> >  
> > +    exec_prot = ppc_hash64_pte_noexec_guard(cpu, pte) ? 0 :
> > PAGE_EXEC;
> >      pp_prot = ppc_hash64_pte_prot(cpu, slb, pte);
> >      amr_prot = ppc_hash64_amr_prot(cpu, pte);
> > -    prot = pp_prot & amr_prot;
> > +    /* Exec permissions CANNOT take away read or write permissions
> > */
> > +    prot = exec_prot | PAGE_READ | PAGE_WRITE;
> > +    prot &= pp_prot & amr_prot;
> Ok.. so, I take back what I said on the last patch.  Looking at this
> whole lot again, it becomes clearer that this is basically applying a
> sequence of masks to an initial permit-everything mask.
> 
> I don't particularly mind how that masking is done - even returning
> the not-permitted bits is ok - as long as each of the masks is done
> consistently.  So, they *all* return a permitted mask, or the *all*
> return a not-permitted mask.  At the moment these aren't consistent,
> some return a boolean equivalent, some return a mask and so forth.

I'll clean this up to make it consistent

> >  
> >      if ((need_prot[rwx] & ~prot) != 0) {
> >          /* Access right violation */
> >          qemu_log_mask(CPU_LOG_MMU, "PTE access rejected\n");
> >          if (rwx == 2) {
> >              int srr1 = 0;
> > -            if (PAGE_EXEC & ~pp_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) {
> > diff --git a/target/ppc/mmu.h b/target/ppc/mmu.h
> > index 1cee142..cfda7f1 100644
> > --- a/target/ppc/mmu.h
> > +++ b/target/ppc/mmu.h
> > @@ -28,6 +28,8 @@
> >  /* Interrupt Fields */
> >  
> >  /* SRR1 */
> > +/* Not permitted due to no-execute or guard bit set */
> > +#define SRR1_NOEXEC_GUARD        0x10000000
> >  #define SRR1_PROTFAULT              0x08000000
> >  #define SRR1_IAMR           0x00200000
> >