On Sun, 11 Mar 2012 19:33:10 -0400, Christoffer Dall <c.dall at virtualopensystems.com> wrote:
> On Fri, Mar 9, 2012 at 10:44 PM, Rusty Russell <rusty at rustcorp.com.au> wrote:
> > + ? ? ? /* The CP15 c15 register is architecturally implementation
> > + ? ? ? ?* defined, but some guest kernels attempt to read/write a
> > + ? ? ? ?* diagnostics register here. We always return 0 and ignore
> > + ? ? ? ?* writes and hope for the best. */
>
> this is technically not kernel style comments is it...
Indeed, a bad habit of mine.
> > + ? ? ? { .is_64 = false, .is_w = true, .f = ignore_write, .arg = 1,
> > + ? ? ? ? .param = { .CRn = 15, .CRm = ANY, .Op1 = ANY, .Op2 = ANY } },
> > + ? ? ? { .is_64 = false, .is_w = false, .f = read_zero, .arg = 1,
> > + ? ? ? ? .param = { .CRn = 15, .CRm = ANY, .Op1 = ANY, .Op2 = ANY } },
> > +};
>
> The overall idea is good, but I personally find this table quite messy
> looking. It's really hard to peak into it and see exactly what's going
> on. Perhaps it helps with the right syntax coloring, but not everyone
> uses that.
>
> I tried a few things, but nothing really works great. Perhaps using a
> macro that makes the 64,w,f,a arguments shorter and indenting the
> setting of those so .param values like CRn does not have interfering
> lines between them would help. I'll give this is a try...
Yes, it's pretty messy... how's this, instead, which makes it read like
the asm does. I can add extra param checks, too. I reverted the
print_cp_instr() change too.
(compile tested only).
diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
index bd2c3dc..cb838d0 100644
--- a/arch/arm/kvm/emulate.c
+++ b/arch/arm/kvm/emulate.c
@@ -167,17 +167,14 @@ struct coproc_params {
static void print_cp_instr(const struct coproc_params *p)
{
- /* Look, we even formatted it for you to paste into the table! */
if (p->is_64bit) {
- kvm_debug("{ .is_64bit = true, .is_write = %s, .param ="
- " { .CRm = %lu, .Op1 = %lu } }",
- p->is_write ? "true" : "false", p->CRm, p->Op1);
+ kvm_debug("%s\tp15, %lu, r%lu, r%lu, c%lu",
+ (p->is_write) ? "mcrr" : "mrrc",
+ p->Op1, p->Rt1, p->Rt2, p->CRm);
} else {
- kvm_debug("{ .is_64bit = false, .is_write = %s, .param ="
- " { .CRn = %lu, .CRm = %lu,"
- " .Op1 = %lu, .Op2 = %lu } }",
- p->is_write ? "true" : "false",
- p->CRn, p->CRm, p->Op1, p->Op2);
+ kvm_debug("%s\tp15, %lu, r%lu, c%lu, c%lu, %lu",
+ (p->is_write) ? "mcr" : "mrc",
+ p->Op1, p->Rt1, p->CRn, p->CRm, p->Op2);
}
}
@@ -237,17 +234,21 @@ static bool read_l2ctlr(struct kvm_vcpu *vcpu,
return true;
}
-static bool access_cp15_reg(struct kvm_vcpu *vcpu,
- const struct coproc_params *p,
- unsigned long cp15_reg)
+static bool read_cp15_reg(struct kvm_vcpu *vcpu,
+ const struct coproc_params *p,
+ unsigned long cp15_reg)
{
- if (p->is_write)
- vcpu->arch.cp15[cp15_reg] = *vcpu_reg(vcpu, p->Rt1);
- else
- *vcpu_reg(vcpu, p->Rt1) = vcpu->arch.cp15[cp15_reg];
+ *vcpu_reg(vcpu, p->Rt1) = vcpu->arch.cp15[cp15_reg];
return true;
}
+static bool write_cp15_reg(struct kvm_vcpu *vcpu,
+ const struct coproc_params *p,
+ unsigned long cp15_reg)
+{
+ vcpu->arch.cp15[cp15_reg] = *vcpu_reg(vcpu, p->Rt1);
+ return true;
+}
/* Any field which is 0xFFFFFFFF == any. */
#define ANY (-1UL)
@@ -267,42 +268,54 @@ struct coproc_emulate {
unsigned long arg;
};
+#define MRC(p15, _Op1, Rt1, _CRn, _CRm, _Op2, fn, _arg) \
+ { .is_64 = false, .is_w = false, .f = (fn), .arg = (_arg), \
+ .param = { .CRn = _CRn, \
+ .CRm = _CRm, \
+ .Op1 = _Op1, \
+ .Op2 = _Op2 } }
+#define MCR(p15, _Op1, Rt1, _CRn, _CRm, _Op2, fn, _arg) \
+ { .is_64 = false, .is_w = true, .f = (fn), .arg = (_arg), \
+ .param = { .CRn = _CRn, \
+ .CRm = _CRm, \
+ .Op1 = _Op1, \
+ .Op2 = _Op2 } }
static const struct coproc_emulate coproc_emulate[] = {
/* Ignore writes to L2CTLR access */
- { .is_64 = false, .is_w = true, .f = ignore_write,
- .param = { .CRn = 9, .CRm = 0, .Op1 = 1, .Op2 = 2 } },
- { .is_64 = false, .is_w = false, .f = read_l2ctlr,
- .param = { .CRn = 9, .CRm = 0, .Op1 = 1, .Op2 = 2 } },
+ MCR(p15, 1, ANY, 9, 0, 2, ignore_write, 0),
+ MRC(p15, 1, ANY, 9, 0, 2, read_l2ctlr, 0),
+
/* Hack alert!!! */
- { .is_64 = false, .is_w = true, .f = ignore_write,
- .param = { .CRn = 9, .CRm = ANY, .Op1 = ANY, .Op2 = ANY } },
- { .is_64 = false, .is_w = false, .f = read_zero,
- .param = { .CRn = 9, .CRm = ANY, .Op1 = ANY, .Op2 = ANY } },
+ MCR(p15, ANY, ANY, 9, ANY, ANY, ignore_write, 0),
+ MRC(p15, ANY, ANY, 9, ANY, ANY, read_zero, 0),
- /* These CRn == 10 entries may not need to exist - if we can
+ /*
+ * These CRn == 10 entries may not need to exist - if we can
* ignore guest attempts to tamper with TLB lockdowns then it
* should be enough to store/restore the host/guest PRRR and
* NMRR memory remap registers and allow guest direct access
- * to these registers. */
+ * to these registers.
+ */
/* TLB Lockdown operations - ignored */
- { .is_64 = false, .is_w = true, .f = ignore_write,
- .param = { .CRn = 10, .CRm = 0, .Op1 = ANY, .Op2 = ANY } },
- { .is_64 = false, .is_w = ANY, .f = access_cp15_reg,
- .arg = c10_PRRR,
- .param = { .CRn = 10, .CRm = 2, .Op1 = 0, .Op2 = 0 } },
- { .is_64 = false, .is_w = ANY, .f = access_cp15_reg,
- .arg = c10_NMRR,
- .param = { .CRn = 10, .CRm = 2, .Op1 = 0, .Op2 = 1 } },
-
- /* The CP15 c15 register is architecturally implementation
+ MRC(p15, 0, ANY, 10, 0, ANY, ignore_write, 0),
+ MRC(p15, 0, ANY, 10, 1, ANY, ignore_write, 0),
+ MRC(p15, 0, ANY, 10, 4, ANY, ignore_write, 0),
+ MRC(p15, 0, ANY, 10, 8, ANY, ignore_write, 0),
+
+ MCR(p15, 0, ANY, 10, 2, 0, read_cp15_reg, c10_PRRR),
+ MRC(p15, 0, ANY, 10, 2, 0, write_cp15_reg, c10_PRRR),
+ MCR(p15, 0, ANY, 10, 2, 1, read_cp15_reg, c10_NMRR),
+ MRC(p15, 0, ANY, 10, 2, 1, write_cp15_reg, c10_NMRR),
+
+ /*
+ * The CP15 c15 register is architecturally implementation
* defined, but some guest kernels attempt to read/write a
* diagnostics register here. We always return 0 and ignore
- * writes and hope for the best. */
- { .is_64 = false, .is_w = true, .f = ignore_write, .arg = 1,
- .param = { .CRn = 15, .CRm = ANY, .Op1 = ANY, .Op2 = ANY } },
- { .is_64 = false, .is_w = false, .f = read_zero, .arg = 1,
- .param = { .CRn = 15, .CRm = ANY, .Op1 = ANY, .Op2 = ANY } },
+ * writes and hope for the best.
+ */
+ MCR(p15, ANY, ANY, 15, ANY, ANY, ignore_write, 1),
+ MRC(p15, ANY, ANY, 15, ANY, ANY, read_zero, 1),
};
static inline bool match(unsigned long val, unsigned long param)
> > + ? ? ? for (i = 0; i < ARRAY_SIZE(coproc_emulate); i++) {
> > + ? ? ? ? ? ? ? const struct coproc_emulate *e = &coproc_emulate[i];
>
> e: entry, emulate? hmmm.
Yes :)
> > +
> > + ? ? ? ? ? ? ? if (!match(params->is_64bit, e->is_64))
> > + ? ? ? ? ? ? ? ? ? ? ? continue;
> > + ? ? ? ? ? ? ? if (!match(params->is_write, e->is_w))
> > + ? ? ? ? ? ? ? ? ? ? ? continue;
> > + ? ? ? ? ? ? ? if (!match(params->CRn, e->param.CRn))
> > + ? ? ? ? ? ? ? ? ? ? ? continue;
> > + ? ? ? ? ? ? ? if (!match(params->CRm, e->param.CRm))
> > + ? ? ? ? ? ? ? ? ? ? ? continue;
> > + ? ? ? ? ? ? ? if (!match(params->Op1, e->param.Op1))
> > + ? ? ? ? ? ? ? ? ? ? ? continue;
> > + ? ? ? ? ? ? ? if (!match(params->Op2, e->param.Op2))
> > + ? ? ? ? ? ? ? ? ? ? ? continue;
>
> are you at all worried about a performance hit here?
Maybe, but there are many options. One is to cache the hsr value in
kvm_handle_cp15_32, for example. Another is binary search. Another is
to reorder the table staticly, and a final one is to do so dynamically
(with something like RCU).
> > +
> > + ? ? ? ? ? ? ? /* If function fails, it should complain. */
>
> questionable comment
s/complain/print its own diagnostic/?
> I'll merge this for the next patch series and we can think about
> refining it from there.
Please never hesitate to mangle my patches beyond recognition. It's
more efficient to assume we agree, and if I really hate something, I can
counter-patch :)
Thanks!
Rusty.
--
How could I marry someone with more hair than me? http://baldalex.org
