[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/3] target-arm: Implement cp15 VA->PA translati
From: |
Adam Lackorzynski |
Subject: |
Re: [Qemu-devel] [PATCH 3/3] target-arm: Implement cp15 VA->PA translation |
Date: |
Thu, 17 Feb 2011 11:52:05 +0100 |
User-agent: |
Mutt/1.5.20 (2009-06-14) |
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 <address@hidden> 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().
Yes, noticed already myself.
> > case 7: /* Cache control. */
>
> We should be insisting that op1 == 0 (otherwise bad_reg).
Ok.
> > 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).
I tried to add this too, should just be two more if expressions.
> > + switch (crm) {
> > + case 4:
> > + env->cp15.c7_par = val;
>
> 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 = 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.
Good idea.
> > + 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.
So for the time being a comment is ok?
> > + 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.
Ok.
> > + } 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.
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 <address@hidden>
---
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 = 0x000;
env->cp15.c15_i_min = 0xff0;
- /* No cache, so nothing to do. */
- /* ??? MPCore has VA to PA translation functions. */
+ if (op1 != 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 = val & 0xfffff6ff;
+ } else {
+ env->cp15.c7_par = val & 0xfffff1ff;
+ }
+ break;
+ case 8: {
+ uint32_t phys_addr;
+ target_ulong page_size;
+ int prot;
+ int ret, is_user = op2 & 2;
+ int access_type = op2 & 1;
+
+ if (op2 & 4) {
+ /* Other states are only available with TrustZone */
+ goto bad_reg;
+ }
+ ret = get_phys_addr(env, val, access_type, is_user,
+ &phys_addr, &prot, &page_size);
+ if (ret == 0) {
+ /* We do not set any attribute bits in the PAR */
+ if (page_size == (1 << 24)
+ && arm_feature(env, ARM_FEATURE_V7)) {
+ env->cp15.c7_par = (phys_addr & 0xff000000) | 1 << 1;
+ } else {
+ env->cp15.c7_par = phys_addr & 0xfffff000;
+ }
+ } else {
+ env->cp15.c7_par = ((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 insn)
}
}
case 7: /* Cache control. */
+ if (crm == 4 && op1 == 0 && op2 == 0) {
+ return env->cp15.c7_par;
+ }
/* FIXME: Should only clear Z flag if destination is r15. */
env->ZF = 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 = qemu_get_be32(f);
env->cp15.c6_data = qemu_get_be32(f);
+ env->cp15.c7_par = qemu_get_be32(f);
env->cp15.c9_insn = qemu_get_be32(f);
env->cp15.c9_data = qemu_get_be32(f);
env->cp15.c13_fcse = qemu_get_be32(f);
--
1.7.2.3
Adam
--
Adam address@hidden
Lackorzynski http://os.inf.tu-dresden.de/~adam/