On 15 February 2011 10:49, Adam Lackorzynski <a...@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
