Patchwork [3/3] target-arm: Implement cp15 VA->PA translation

login
register
mail settings
Submitter Adam Lackorzynski
Date Feb. 15, 2011, 10:49 a.m.
Message ID <20110215104942.GD19666@os.inf.tu-dresden.de>
Download mbox | patch
Permalink /patch/83233/
State New
Headers show

Comments

Adam Lackorzynski - Feb. 15, 2011, 10:49 a.m.
Implement VA->PA translations by cp15-c7 that went through unchanged
previously.

Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
---
 target-arm/cpu.h    |    1 +
 target-arm/helper.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 50 insertions(+), 2 deletions(-)
Peter Maydell - Feb. 16, 2011, 3:57 p.m.
On 15 February 2011 10:49, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
> Implement VA->PA translations by cp15-c7 that went through unchanged
> previously.

> +        uint32_t c7_par;  /* Translation result. */

I think this new env field needs extra code so it is saved
and loaded by machine.c:cpu_save() and cpu_load().

>     case 7: /* Cache control.  */

We should be insisting that op1 == 0 (otherwise bad_reg).

>         env->cp15.c15_i_max = 0x000;
>         env->cp15.c15_i_min = 0xff0;
> -        /* No cache, so nothing to do.  */
> -        /* ??? MPCore has VA to PA translation functions.  */
> +        /* No cache, so nothing to do except VA->PA translations. */
> +        if (arm_feature(env, ARM_FEATURE_V6)) {

This is the wrong feature switch. The VA-PA translation registers
are only architectural in v7. Before that, they exist in 11MPcore
and 1176 but not 1136. So we should be testing for v7 or
11MPcore (since we don't model 1176).

Also, the format of the PAR is different in 1176/11MPcore!
(in the comments below I'm generally talking about the v7
format, not 1176/mpcore).

> +            switch (crm) {
> +            case 4:
> +                env->cp15.c7_par = val;

We shouldn't be allowing the reserved and impdef bits
to be set here.

> +                break;
> +            case 8: {
> +                uint32_t phys_addr;
> +                target_ulong page_size;
> +                int prot;
> +                int ret, is_user;
> +                int access_type;
> +
> +                switch (op2) {
> +                case 0: /* priv read */
> +                    is_user = 0;
> +                    access_type = 0;
> +                    break;
> +                case 1: /* priv write */
> +                    is_user = 0;
> +                    access_type = 1;
> +                    break;
> +                case 2: /* user read */
> +                    is_user = 1;
> +                    access_type = 0;
> +                    break;
> +                case 3: /* user write */
> +                    is_user = 1;
> +                    access_type = 1;
> +                    break;
> +                default: /* 4-7 are only available with TZ */
> +                    goto bad_reg;
> +                }

I think it would be cleaner to write:
 access_type = op2 & 1;
 is_user = op2 & 2;
 other_sec_state = op2 & 4;
 if (other_sec_state) {
    /* Only supported with TrustZone */
    goto bad_reg;
 }

rather than have this big switch statement.

> +                ret = get_phys_addr_v6(env, val, access_type, is_user,
> +                                       &phys_addr, &prot, &page_size);

This will do the wrong thing when the MMU is disabled,
and it doesn't account for the FSCE either. I think that
just using get_phys_addr() will fix both of these.

> +                if (ret == 0) {
> +                    env->cp15.c7_par = phys_addr;

You need to mask out bits [11..0] of phys_addr here:
if the caller passed in a VA with them set then
get_phys_addr* will pass them back out to you again.

Also we ought ideally to be setting the various
attributes bits based on the TLB entry, although
I appreciate that since qemu doesn't currently do
any of that decoding it would be a bit tedious to
have to add it just for the benefit of VA-PA translation.

> +                    if (page_size > TARGET_PAGE_SIZE)
> +                        env->cp15.c7_par |= 1 << 1;

This isn't correct: the SS bit should only be set if this
was a SuperSection, not for anything larger than a page.
(And if it is a SuperSection then the meaning of the
PAR PA field changes, which for us means that we
need to zero bits [23:12] since we don't support
>32bit physaddrs.)

Also, coding style mandates braces.

> +                } else {
> +                    env->cp15.c7_par = ret | 1;

This isn't quite right -- the return value from
get_phys_addr*() is in the same format as the
DFSR (eg with the domain in bits [7:4]), and
the PAR bits [6:1] should be the equivalent
of DFSR bits [12,10,3:0]. So you need a bit
of shifting and masking here.

> @@ -1789,6 +1833,9 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t insn)
>            }
>         }
>     case 7: /* Cache control.  */
> +        if (crm == 4 && op2 == 0) {
> +            return env->cp15.c7_par;
> +        }

Again, we want op1 == 0 as well.

-- PMM

Patch

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index c9febfa..603574b 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -126,6 +126,7 @@  typedef struct CPUARMState {
         uint32_t c6_region[8]; /* MPU base/size registers.  */
         uint32_t c6_insn; /* Fault address registers.  */
         uint32_t c6_data;
+        uint32_t c7_par;  /* Translation result. */
         uint32_t c9_insn; /* Cache lockdown registers.  */
         uint32_t c9_data;
         uint32_t c13_fcse; /* FCSE PID.  */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 7f63a28..32cc795 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1456,8 +1456,52 @@  void HELPER(set_cp15)(CPUState *env, uint32_t insn, uint32_t val)
     case 7: /* Cache control.  */
         env->cp15.c15_i_max = 0x000;
         env->cp15.c15_i_min = 0xff0;
-        /* No cache, so nothing to do.  */
-        /* ??? MPCore has VA to PA translation functions.  */
+        /* No cache, so nothing to do except VA->PA translations. */
+        if (arm_feature(env, ARM_FEATURE_V6)) {
+            switch (crm) {
+            case 4:
+                env->cp15.c7_par = val;
+                break;
+            case 8: {
+                uint32_t phys_addr;
+                target_ulong page_size;
+                int prot;
+                int ret, is_user;
+                int access_type;
+
+                switch (op2) {
+                case 0: /* priv read */
+                    is_user = 0;
+                    access_type = 0;
+                    break;
+                case 1: /* priv write */
+                    is_user = 0;
+                    access_type = 1;
+                    break;
+                case 2: /* user read */
+                    is_user = 1;
+                    access_type = 0;
+                    break;
+                case 3: /* user write */
+                    is_user = 1;
+                    access_type = 1;
+                    break;
+                default: /* 4-7 are only available with TZ */
+                    goto bad_reg;
+                }
+                ret = get_phys_addr_v6(env, val, access_type, is_user,
+                                       &phys_addr, &prot, &page_size);
+                if (ret == 0) {
+                    env->cp15.c7_par = phys_addr;
+                    if (page_size > TARGET_PAGE_SIZE)
+                        env->cp15.c7_par |= 1 << 1;
+                } else {
+                    env->cp15.c7_par = ret | 1;
+                }
+                break;
+            }
+            }
+        }
         break;
     case 8: /* MMU TLB control.  */
         switch (op2) {
@@ -1789,6 +1833,9 @@  uint32_t HELPER(get_cp15)(CPUState *env, uint32_t insn)
 	    }
         }
     case 7: /* Cache control.  */
+        if (crm == 4 && op2 == 0) {
+            return env->cp15.c7_par;
+        }
         /* FIXME: Should only clear Z flag if destination is r15.  */
         env->ZF = 0;
         return 0;