From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=59809 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pq1T4-0005DA-EM for qemu-devel@nongnu.org; Thu, 17 Feb 2011 05:52:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pq1T2-0004At-GO for qemu-devel@nongnu.org; Thu, 17 Feb 2011 05:52:14 -0500 Received: from os.inf.tu-dresden.de ([141.76.48.99]:59452) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pq1T2-0004AY-4n for qemu-devel@nongnu.org; Thu, 17 Feb 2011 05:52:12 -0500 Date: Thu, 17 Feb 2011 11:52:05 +0100 From: Adam Lackorzynski Subject: Re: [Qemu-devel] [PATCH 3/3] target-arm: Implement cp15 VA->PA translation Message-ID: <20110217105205.GB7187@os.inf.tu-dresden.de> References: <20110215104942.GD19666@os.inf.tu-dresden.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org Hi, thanks for the review! On Wed Feb 16, 2011 at 15:57:59 +0000, Peter Maydell wrote: > On 15 February 2011 10:49, Adam Lackorzynski = wrote: > > Implement VA->PA translations by cp15-c7 that went through unchanged > > previously. >=20 > > + =A0 =A0 =A0 =A0uint32_t c7_par; =A0/* Translation result. */ >=20 > I think this new env field needs extra code so it is saved > and loaded by machine.c:cpu_save() and cpu_load(). Yes, noticed already myself. =20 > > =A0 =A0 case 7: /* Cache control. =A0*/ >=20 > We should be insisting that op1 =3D=3D 0 (otherwise bad_reg). Ok. > > =A0 =A0 =A0 =A0 env->cp15.c15_i_max =3D 0x000; > > =A0 =A0 =A0 =A0 env->cp15.c15_i_min =3D 0xff0; > > - =A0 =A0 =A0 =A0/* No cache, so nothing to do. =A0*/ > > - =A0 =A0 =A0 =A0/* ??? MPCore has VA to PA translation functions. =A0*/ > > + =A0 =A0 =A0 =A0/* No cache, so nothing to do except VA->PA translatio= ns. */ > > + =A0 =A0 =A0 =A0if (arm_feature(env, ARM_FEATURE_V6)) { >=20 > 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). >=20 > 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). I tried to add this too, should just be two more if expressions. > > + =A0 =A0 =A0 =A0 =A0 =A0switch (crm) { > > + =A0 =A0 =A0 =A0 =A0 =A0case 4: > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0env->cp15.c7_par =3D val; >=20 > We shouldn't be allowing the reserved and impdef bits > to be set here. Ok. > I think it would be cleaner to write: > access_type =3D op2 & 1; > is_user =3D op2 & 2; > other_sec_state =3D op2 & 4; > if (other_sec_state) { > /* Only supported with TrustZone */ > goto bad_reg; > } >=20 > rather than have this big switch statement. Good idea. > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D get_phys_addr_v6(env, val, acc= ess_type, is_user, > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 &phys_addr, &prot, &page_size); >=20 > 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. >=20 > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (ret =3D=3D 0) { > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0env->cp15.c7_par =3D phys_addr; >=20 > 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. >=20 > 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. So for the time being a comment is ok? > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (page_size > TARGET_PAGE_SI= ZE) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0env->cp15.c7_par |=3D = 1 << 1; >=20 > 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.) >=20 > Also, coding style mandates braces. Ok. > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else { > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0env->cp15.c7_par =3D ret | 1; >=20 > 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. >=20 > > @@ -1789,6 +1833,9 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t= insn) > > =A0 =A0 =A0 =A0 =A0 =A0} > > =A0 =A0 =A0 =A0 } > > =A0 =A0 case 7: /* Cache control. =A0*/ > > + =A0 =A0 =A0 =A0if (crm =3D=3D 4 && op2 =3D=3D 0) { > > + =A0 =A0 =A0 =A0 =A0 =A0return env->cp15.c7_par; > > + =A0 =A0 =A0 =A0} >=20 > Again, we want op1 =3D=3D 0 as well. Ok. I'm not really sure about the cp15.c15_i_max and cp15.c15_i_min, they only seem to be used with ARM_FEATURE_OMAPCP so an if could be put around them. New version: Subject: [PATCH 2/3] target-arm: Implement cp15 VA->PA translation Implement VA->PA translations by cp15-c7 that went through unchanged previously. Signed-off-by: Adam Lackorzynski --- target-arm/cpu.h | 1 + target-arm/helper.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++= -- target-arm/machine.c | 2 ++ 3 files changed, 49 insertions(+), 2 deletions(-) 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..23c719b 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -1456,8 +1456,49 @@ void HELPER(set_cp15)(CPUState *env, uint32_t insn, = uint32_t val) case 7: /* Cache control. */ env->cp15.c15_i_max =3D 0x000; env->cp15.c15_i_min =3D 0xff0; - /* No cache, so nothing to do. */ - /* ??? MPCore has VA to PA translation functions. */ + if (op1 !=3D 0) { + goto bad_reg; + } + /* No cache, so nothing to do except VA->PA translations. */ + if (arm_feature(env, ARM_FEATURE_V6K)) { + switch (crm) { + case 4: + if (arm_feature(env, ARM_FEATURE_V7)) { + env->cp15.c7_par =3D val & 0xfffff6ff; + } else { + env->cp15.c7_par =3D val & 0xfffff1ff; + } + break; + case 8: { + uint32_t phys_addr; + target_ulong page_size; + int prot; + int ret, is_user =3D op2 & 2; + int access_type =3D op2 & 1; + + if (op2 & 4) { + /* Other states are only available with TrustZone */ + goto bad_reg; + } + ret =3D get_phys_addr(env, val, access_type, is_user, + &phys_addr, &prot, &page_size); + if (ret =3D=3D 0) { + /* We do not set any attribute bits in the PAR */ + if (page_size =3D=3D (1 << 24) + && arm_feature(env, ARM_FEATURE_V7)) { + env->cp15.c7_par =3D (phys_addr & 0xff000000) | 1 = << 1; + } else { + env->cp15.c7_par =3D phys_addr & 0xfffff000; + } + } else { + env->cp15.c7_par =3D ((ret & (10 << 1)) >> 5) | + ((ret & (12 << 1)) >> 6) | + ((ret & 0xf) << 1) | 1; + } + break; + } + } + } break; case 8: /* MMU TLB control. */ switch (op2) { @@ -1789,6 +1830,9 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t ins= n) } } case 7: /* Cache control. */ + if (crm =3D=3D 4 && op1 =3D=3D 0 && op2 =3D=3D 0) { + return env->cp15.c7_par; + } /* FIXME: Should only clear Z flag if destination is r15. */ env->ZF =3D 0; return 0; diff --git a/target-arm/machine.c b/target-arm/machine.c index 3925d3a..a18b7dc 100644 --- a/target-arm/machine.c +++ b/target-arm/machine.c @@ -41,6 +41,7 @@ void cpu_save(QEMUFile *f, void *opaque) } qemu_put_be32(f, env->cp15.c6_insn); qemu_put_be32(f, env->cp15.c6_data); + qemu_put_be32(f, env->cp15.c7_par); qemu_put_be32(f, env->cp15.c9_insn); qemu_put_be32(f, env->cp15.c9_data); qemu_put_be32(f, env->cp15.c13_fcse); @@ -148,6 +149,7 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id) } env->cp15.c6_insn =3D qemu_get_be32(f); env->cp15.c6_data =3D qemu_get_be32(f); + env->cp15.c7_par =3D qemu_get_be32(f); env->cp15.c9_insn =3D qemu_get_be32(f); env->cp15.c9_data =3D qemu_get_be32(f); env->cp15.c13_fcse =3D qemu_get_be32(f); --=20 1.7.2.3 Adam --=20 Adam adam@os.inf.tu-dresden.de Lackorzynski http://os.inf.tu-dresden.de/~adam/