On Fri, May 16, 2014 at 10:01 AM, Fabian Aggeler <aggel...@ethz.ch> wrote: > From: Sergey Fedorov <s.fedo...@samsung.com> > > CPACR register allows to control access rights to coprocessor 0-13 > interfaces. Bits corresponding to unimplemented coprocessors should be > RAZ/WI. QEMU implements only VFP coprocessor on ARMv6+ targets. So only > cp10 & cp11 bits are writable. > > Signed-off-by: Sergey Fedorov <s.fedo...@samsung.com> > Signed-off-by: Fabian Aggeler <aggel...@ethz.ch> > --- > > This patch was previously part of the Security Extensions patchset and > got separated because it is not Sec-Ext specific. > In addition to a style fix of the comment I added qemu_log_mask() as > suggested. > > target-arm/helper.c | 6 ++++++ > target-arm/translate.c | 29 ++++++++++++++++++++++++++--- > 2 files changed, 32 insertions(+), 3 deletions(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 417161e..3cb1ac0 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -477,6 +477,12 @@ static const ARMCPRegInfo not_v7_cp_reginfo[] = { > static void cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri, > uint64_t value) > { > + uint32_t mask = 0; > + > + if (arm_feature(env, ARM_FEATURE_VFP)) { > + mask |= 0x00f00000; /* VFP coprocessor: cp10 & cp11 */ > + } > + value &= mask; > if (env->cp15.c1_coproc != value) { > env->cp15.c1_coproc = value; > /* ??? Is this safe when called from within a TB? */ > diff --git a/target-arm/translate.c b/target-arm/translate.c > index a4d920b..9b298d0 100644 > --- a/target-arm/translate.c > +++ b/target-arm/translate.c > @@ -6864,9 +6864,32 @@ static int disas_coproc_insn(CPUARMState * env, > DisasContext *s, uint32_t insn) > const ARMCPRegInfo *ri; > > cpnum = (insn >> 8) & 0xf; > - if (arm_feature(env, ARM_FEATURE_XSCALE) > - && ((env->cp15.c15_cpar ^ 0x3fff) & (1 << cpnum))) > - return 1; > + if (cpnum < 14) { > + if (arm_feature(env, ARM_FEATURE_XSCALE)) { > + if (~env->cp15.c15_cpar & (1 << cpnum)) { > + return 1; > + } > + } else { > + /* Bits [20:21] of CPACR control access to cp10 > + * Bits [23:22] of CPACR control access to cp11 > + */
Thinking about this a little more, there's nothing CP10/CP11 specific about this logic. The comment should be more general than this (or dropped). It would go stale if someone patched in a new CP. Perhaps add a "..." to indicate that you are illustrating a repeating pattern + /* Bits [20:21] of CPACR control access to cp10 + * Bits [23:22] of CPACR control access to cp11 + * ... + */ With droppage or comment generalisation: Reviewed-by: Peter Crosthwaite <peter.crosthwa...@xilinx.com> > + switch ((env->cp15.c1_coproc >> (cpnum * 2)) & 3) { > + case 0: /* access denied */ > + return 1; > + case 1: /* privileged mode access only */ > + if (IS_USER(s)) { > + return 1; > + } > + break; > + case 2: /* reserved */ > + qemu_log_mask(LOG_GUEST_ERROR, "CPACR bits for cp%d set to " > + "0b10 (unpredictable)\n", > cpnum); > + return 1; > + case 3: /* privileged and user mode access */ > + break; > + } > + } > + } > > /* First check for coprocessor space used for actual instructions */ > switch (cpnum) { > -- > 1.8.3.2 > >