Re: [Qemu-ppc] [RFC NO-MERGE 01/12] target/ppc: Add Instruction Authority Mask Register Check

[Suraj Jitindar Singh]

On Mon, 2017-02-20 at 11:27 +1100, David Gibson wrote:
> On Fri, Feb 17, 2017 at 04:08:01PM +1100, Suraj Jitindar Singh wrote:
> > 
> > The instruction authority mask register (IAMR) can be used to
> > restrict
> > permissions for instruction fetch accesses on a per key basis for
> > each
> > of 32 different key values. Access permissions are derived based on
> > the
> > specific key value stored in the relevant page table entry.
> > 
> > The IAMR was introduced in, and is present in processors since,
> > POWER8
> > (ISA v2.07). Thus introduce a function to check access permissions
> > based
> > on the pte key value and the contents of the IAMR when handling a
> > page
> > fault to ensure sufficient access permissions for an instruction
> > fetch.
> > 
> > A hash pte contains a key value in bits 2:3|52:54 of the second
> > double word
> > of the pte, this key value gives an index into the IAMR which
> > contains 32
> > 2-bit access masks. If the least significant bit of the 2-bit
> > access mask
> > corresponding to the given key value is set (IAMR[key] & 0x1 ==
> > 0x1) then
> > the instruction fetch is not permitted and an ISI is generated
> > accordingly.
> > While we're here, add defines for the srr1 bits to be set for the
> > ISI for
> > clarity.
> > 
> > e.g.
> > 
> > pte:
> > dw0
> > [XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX]
> > dw1
> > [XX01XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX010XXXXXXXXX]
> >        ^^                                                ^^^
> > key = 01010 (0x0a)
> > 
> > IAMR:
> > [XXXXXXXXXXXXXXXXXXXX01XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX]
> >                            ^^
> > Access mask = 0b01
> > 
> > Test access mask: 0b01 & 0x1 == 0x1
> > 
> > Least significant bit of the access mask is set, thus the
> > instruction fetch
> > is not permitted. We should generate an instruction storage
> > interrupt (ISI)
> > with bit 42 of SRR1 set to indicate access precluded by virtual
> > page class
> > key protection.
> > 
> > Signed-off-by: Suraj Jitindar Singh <address@hidden>
> > ---
> >  target/ppc/mmu-hash64.c | 39
> > ++++++++++++++++++++++++++++++++++++++-
> >  target/ppc/mmu.h        |  6 ++++++
> >  2 files changed, 44 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > index ada8876..08079db 100644
> > --- a/target/ppc/mmu-hash64.c
> > +++ b/target/ppc/mmu-hash64.c
> > @@ -391,6 +391,20 @@ static int ppc_hash64_pte_prot(PowerPCCPU
> > *cpu,
> >      return prot;
> >  }
> >  
> > +/* Check the instruction access permissions specified in the IAMR
> > */
> > +static int ppc_hash64_iamr_prot(PowerPCCPU *cpu, int key)
> > +{
> > +    CPUPPCState *env = &cpu->env;
> > +    int iamr_bits = (env->spr[SPR_IAMR] >> 2*(31 - key)) & 0x3;
> > +
> > +    /*
> > +     * An instruction fetch is permitted if the IAMR bit is 0.
> > +     * If the bit is set, return PAGE_EXEC to indicate that that
> > bit must be
> > +     * cleared from the permissions, otherwise return 0.
> > +     */
> > +    return (iamr_bits & 0x1) ? PAGE_EXEC : 0;
> Returning a mask of bits to clear is a bit confusing - i.e. returning
> PAGE_EXEC when EXEC is *not* allowed.  I think a better approach
> would
> be for this function to take the "so far" permissions, apply the mask
> itself and return the resulting permission mask.

Fair Enough

> 
> > 
> > +}
> > +
> >  static int ppc_hash64_amr_prot(PowerPCCPU *cpu, ppc_hash_pte64_t
> > pte)
> >  {
> >      CPUPPCState *env = &cpu->env;
> > @@ -423,6 +437,22 @@ static int ppc_hash64_amr_prot(PowerPCCPU
> > *cpu, ppc_hash_pte64_t pte)
> >          prot &= ~PAGE_READ;
> >      }
> >  
> > +    switch (env->mmu_model) {
> > +    /*
> > +     * MMU version 2.07 and later support IAMR
> > +     * Check if the IAMR allows the instruction access - it will
> > return
> > +     * PAGE_EXEC if it doesn't (and thus that bit will be cleared)
> > or 0
> > +     * if it does (and prot will be unchanged indicating execution
> > support).
> > +     */
> > +    case POWERPC_MMU_2_07:
> > +    case POWERPC_MMU_2_07a:
> > +    case POWERPC_MMU_3_00:
> > +        prot &= ~ppc_hash64_iamr_prot(cpu, key);
> > +   break;
> > +    default:
> > +        break;
> > +    }
> > +
> >      return prot;
> >  }
> >  
> > @@ -821,7 +851,14 @@ skip_slb_search:
> >          /* Access right violation */
> >          qemu_log_mask(CPU_LOG_MMU, "PTE access rejected\n");
> >          if (rwx == 2) {
> > -            ppc_hash64_set_isi(cs, env, 0x08000000);
> > +            int srr1 = 0;
> > +            if (PAGE_EXEC & ~pp_prot) {
> > +                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;
> >              if (need_prot[rwx] & ~pp_prot) {
> > diff --git a/target/ppc/mmu.h b/target/ppc/mmu.h
> > index 9375921..1cee142 100644
> > --- a/target/ppc/mmu.h
> > +++ b/target/ppc/mmu.h
> > @@ -25,6 +25,12 @@
> >  /* Partition Table Entry Fields */
> >  #define PATBE1_GR 0x8000000000000000
> >  
> > +/* Interrupt Fields */
> > +
> > +/* SRR1 */
> > +#define SRR1_PROTFAULT              0x08000000
> > +#define SRR1_IAMR           0x00200000
> > +
> >  #ifdef TARGET_PPC64
> >  
> >  static inline bool ppc64_use_proc_tbl(PowerPCCPU *cpu)