On Mon, Mar 12, 2012 at 8:18 PM, Rusty Russell <rusty at rustcorp.com.au> wrote:
> 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.
>
looks definitely better. Take a look at my attempt to mangle it in the
v7 series as well and let me know which you prefer. I am undecided at
this point.
> (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 :)
>
awesome
>> > +
>> > + ? ? ? ? ? ? ? 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).
>
ok, well, let's do it this way and see if it's a bottle-neck when we
have hardware.
>> > +
>> > + ? ? ? ? ? ? ? /* If function fails, it should complain. */
>>
>> questionable comment
>
> s/complain/print its own diagnostic/?
>
yeah, now I see why you had the comment in the first place
>> 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 :)
>
hehe, ok, I will mangle:)
-Christoffer
