On Tue, 13 Mar 2012 08:47:52 +0000, Peter Maydell <peter.maydell at linaro.org> wrote:
> On 13 March 2012 00:18, Rusty Russell <rusty at rustcorp.com.au> wrote:
> > + ? ? ? MCR(p15, 1, ANY, 9, 0, 2, ignore_write, 0),
> > + ? ? ? MRC(p15, 1, ANY, 9, 0, 2, read_l2ctlr, 0),
>
> Hmm. I'm generally a bit suspicious of anything that doesn't
> explicitly say which are crn/crm/op1/op2, because the various
> docs and tables in TRMs and manuals have no particular
> consistency about ordering and it's fantastically easy to get
> confused.
Fervently agreed! I got confused with the mappings myself, as I wrote
the code.
> However the MRC()/MCR() are quite cute.
Yes, even though two args here are redundant, I think it's a clarity
win. I can even use the same macro to generate test cases...
> Are you going to want to encode "this is how this cp15 register
> is reset" in your tables at some point? Or "r/o for user mode
> but r/w for kernel" or other complicated permissions?
I think reset values deserve a separate table, since it's not an obvious
fit here.
I thought permissions would be enforced by hardware? If we have a few
weird cases, we can open-code it in the callback. Otherwise, yeah, we
need a new field.
> One
> downside of these macros is that you've lost the ability to
> use named fields in the struct initialisers and are back to
> a plain list, which is fine when there are only 2 things other
> than the crn/crm/op1/op2 but might get ugly if we need to add
> more later.
Agreed. Perhaps a better method is to leave extra args outside the
macro (ie. move the {} out of the macro):
#define MRC(p15, _Op1, Rt1, _CRn, _CRm, _Op2) \
.is_64 = false, .is_w = false, \
.param = { .CRn = _CRn, \
.CRm = _CRm, \
.Op1 = _Op1, \
.Op2 = _Op2 }
{ MRC(p15, 1, ANY, 9, 0, 2), .f = ignore_write },
Slightly less neat, but more extensible...
Cheers,
Rusty.
--
How could I marry someone with more hair than me? http://baldalex.org
