Re: [Qemu-devel] [PATCH v2 10/23] target-arm: implement CPACR register logic

[Fabian Aggeler]

On 14/05/14 20:39, Fedorov Sergey wrote:
> 14.05.2014 10:06, Sergey Fedorov пишет:
> > On 13.05.2014 20:15, Fabian Aggeler wrote:
> > > From: Sergey Fedorov <address@hidden>
> > >
> > > 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 <address@hidden>
> > > Signed-off-by: Fabian Aggeler <address@hidden>
> > > ---
> > >   target-arm/helper.c    |  6 ++++++
> > >   target-arm/translate.c | 26 +++++++++++++++++++++++---
> > >   2 files changed, 29 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > > index cf1f88c..4e82259 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 87d0918..c815fb3 100644
> > > --- a/target-arm/translate.c
> > > +++ b/target-arm/translate.c
> > > @@ -6866,9 +6866,29 @@ 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 */
> > > +            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 */
> > > +                return 1;
> > > +            case 3: /* privileged and user mode access */
> > > +                break;
> > > +            }
> > > +        }
> > > +    }
> > >
> > >       /* First check for coprocessor space used for actual instructions */
> > >       switch (cpnum) {
> > Please, look at disas_vfp_insn() and disas_neon_*_insn() functions.
> > Looks like them should be updated. In that case do not forget to adjust
> > arm_cpu_reset() so user emulation would be able to execute VFP/NEON
> > instructions.
>
> See ARM ARM v7-AR B1.11.1

I don't quite get what you mean. Bits 20-24 of c1_coproc already get set 
to 1 for user emulation in arm_cpu_reset(). And disas_cfp_insn and 
disas_neon_*_insn() all check s->cpacr_fpen in the beginning (which gets 
set in cpu_get_tb_cpu_state() if bits 20-22 of c1_coproc are set to 3 or 
(1 && cpu is in user mode)).

So I guess we should add some checks for NSACR, to only set that flag if 
the corresponding NSACR bit is set.

> > Thanks,
> > Sergey.