* [Qemu-devel] [PATCH 3/3] target-arm: Implement cp15 VA->PA translation
@ 2011-02-15 10:49 Adam Lackorzynski
2011-02-16 15:57 ` Peter Maydell
0 siblings, 1 reply; 3+ messages in thread
From: Adam Lackorzynski @ 2011-02-15 10:49 UTC (permalink / raw)
To: qemu-devel
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(-)
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;
--
1.7.2.3
Adam
--
Adam adam@os.inf.tu-dresden.de
Lackorzynski http://os.inf.tu-dresden.de/~adam/
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] target-arm: Implement cp15 VA->PA translation
2011-02-15 10:49 [Qemu-devel] [PATCH 3/3] target-arm: Implement cp15 VA->PA translation Adam Lackorzynski
@ 2011-02-16 15:57 ` Peter Maydell
2011-02-17 10:52 ` Adam Lackorzynski
0 siblings, 1 reply; 3+ messages in thread
From: Peter Maydell @ 2011-02-16 15:57 UTC (permalink / raw)
To: Adam Lackorzynski; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] target-arm: Implement cp15 VA->PA translation
2011-02-16 15:57 ` Peter Maydell
@ 2011-02-17 10:52 ` Adam Lackorzynski
0 siblings, 0 replies; 3+ messages in thread
From: Adam Lackorzynski @ 2011-02-17 10:52 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel
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 <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().
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 <adam@os.inf.tu-dresden.de>
---
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 adam@os.inf.tu-dresden.de
Lackorzynski http://os.inf.tu-dresden.de/~adam/
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-02-17 10:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-02-15 10:49 [Qemu-devel] [PATCH 3/3] target-arm: Implement cp15 VA->PA translation Adam Lackorzynski
2011-02-16 15:57 ` Peter Maydell
2011-02-17 10:52 ` Adam Lackorzynski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).