[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH V2 01/10] target/ppc: Add Instruction Authority Ma
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH V2 01/10] target/ppc: Add Instruction Authority Mask Register Check |
Date: |
Fri, 3 Mar 2017 15:20:21 +1100 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
On Fri, Mar 03, 2017 at 02:34:29PM +1100, Sam Bobroff wrote:
> On Wed, Mar 01, 2017 at 06:12:52PM +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>
>
> Nice explanation. A few comments, below.
>
> Reviewed-by: Sam Bobroff <address@hidden>
>
> > ---
> >
> > V1 -> V2:
> > - ppc_hash64_iamr_prot() returns a mask of permitted access types to match
> > current permissions functions where the return value is anded to get the
> > least permissive access permissions.
> > ---
> > target/ppc/mmu-book3s-v3.h | 6 ++++++
> > target/ppc/mmu-hash64.c | 41 ++++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 46 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/ppc/mmu-book3s-v3.h b/target/ppc/mmu-book3s-v3.h
> > index 636f6ab..1cc0b8f 100644
> > --- a/target/ppc/mmu-book3s-v3.h
> > +++ b/target/ppc/mmu-book3s-v3.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)
> > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > index 6da334c..b02d4ad 100644
> > --- a/target/ppc/mmu-hash64.c
> > +++ b/target/ppc/mmu-hash64.c
> > @@ -343,6 +343,23 @@ 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;
>
> My brain would appreciate an extra set of parenthesis to make it clear
> that the key is part of the shift.
>
> > +
> > + /*
> > + * An instruction fetch is permitted if the IAMR bit is 0.
> > + * If the bit is set, return PAGE_READ | PAGE_WRITE because this bit
> > + * can only take away EXEC permissions not READ or WRITE permissions.
> > + * If bit is cleared return PAGE_READ | PAGE_WRITE | PAGE_EXEC since
> > + * EXEC permissions are allowed.
> > + */
> > + return (iamr_bits & 0x1) ? PAGE_READ | PAGE_WRITE :
> > + PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> > +}
> > +
> > static int ppc_hash64_amr_prot(PowerPCCPU *cpu, ppc_hash_pte64_t pte)
> > {
> > CPUPPCState *env = &cpu->env;
> > @@ -375,6 +392,21 @@ 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_3_00:
> > + prot &= ppc_hash64_iamr_prot(cpu, key);
>
> What about (instead of adding ppc_hash64_iamr_prot()) adding a function
> that selects the AMR or IAMR key bits (that you could also use earlier
> to set the amrbits variable in the existing code), and then doing:
> if (amr_key_bits(env->spr[SPR_IAMR], key) & 0x1) {
> prot &= ~PAGE_EXEC;
> }
> If that would work it seems easier to understand to me.
Perhaps, but too late - I merged this already.
>
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > return prot;
> > }
> >
> > @@ -774,7 +806,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) {
>
--
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
- [Qemu-ppc] [PATCH V2 00/10] target/ppc: Implement POWER9 pseries TCG RADIX Support, Suraj Jitindar Singh, 2017/03/01
- [Qemu-ppc] [PATCH V2 01/10] target/ppc: Add Instruction Authority Mask Register Check, Suraj Jitindar Singh, 2017/03/01
- [Qemu-ppc] [PATCH V2 02/10] target/ppc: Add execute permission checking to access authority check, Suraj Jitindar Singh, 2017/03/01
- [Qemu-ppc] [PATCH V2 03/10] target/ppc: Move no-execute and guarded page checking into new function, Suraj Jitindar Singh, 2017/03/01
- [Qemu-ppc] [PATCH V2 04/10] target/ppc: Rework hash mmu page fault code and add defines for clarity, Suraj Jitindar Singh, 2017/03/01
- [Qemu-ppc] [PATCH V2 05/10] target/ppc: Add ibm, processor-radix-AP-encodings for TCG, Suraj Jitindar Singh, 2017/03/01
- [Qemu-ppc] [PATCH V2 06/10] target/ppc: Add POWER9/ISAv3.00 to compat_table, Suraj Jitindar Singh, 2017/03/01
- [Qemu-ppc] [PATCH V2 07/10] target/ppc: Flush TLB on write to PIDR, Suraj Jitindar Singh, 2017/03/01