On 12 August 2012 23:24, Rusty Russell <rusty.russell@xxxxxxxxxx> wrote:
> On Wed, 8 Aug 2012 15:58:11 +0100, Peter Maydell <peter.maydell@xxxxxxxxxx> wrote:
>> On 12 July 2012 06:54, Rusty Russell <rusty.russell@xxxxxxxxxx> wrote:
>> > +/* If you need to interpret the index values, here's the key. */
>> > +#define KVM_ARM_MSR_COPROC_MASK 0xFFFF0000
>> > +#define KVM_ARM_MSR_64_BIT_MASK 0x00008000
>> > +#define KVM_ARM_MSR_64_OPC1_MASK 0x000000F0
>> > +#define KVM_ARM_MSR_64_CRM_MASK 0x0000000F
>> > +#define KVM_ARM_MSR_32_CRM_MASK 0x0000000F
>> > +#define KVM_ARM_MSR_32_OPC2_MASK 0x00000070
>> > +#define KVM_ARM_MSR_32_CRN_MASK 0x00000780
>> > +#define KVM_ARM_MSR_32_OPC1_MASK 0x00003800
>>
>> >From userspace's point of view, these defines are not very helpful,
>> since they don't make it easy to construct an index from a
>> (copro,crn,opc1,crm,opc2) tuple. Your QEMU patchset includes:
>>
>> #define MSR32_INDEX_OF(coproc, crn, opc1, crm, opc2) \
>> (((coproc)<<16) | ((opc1)<<11) | ((crn)<<7) | ((opc2)<<4) | (crm))
>>
>> ...but it would be nice if those magic numbers were actually
>> defined constants from the kernel headers I think.
>
> Hmm, you should have ffs(), which makes this easy:
>
> /* Given a simple mask, get those bits. */
> static inline u32 get_bits(u32 index, u32 mask)
> {
> return (index & mask) >> (ffs(mask) - 1);
> }
>
> Since your ffs probably ends up as __builtin_ffs(), it should evaporate
> nicely with this constant.
I don't want to rely on a 'probably', thanks. Also QEMU's standard
function for extracting bits from a bitfield is
extract32(value, start, len), so just having the mask is entirely
the wrong thing to feed into that. So these #defines are awkward
both for constructing an index from a tuple and for extracting a
tuple given an index.
Basically QEMU is the primary userspace consumer of these constants;
if you don't want to be helpful you might as well not define them
in a public header file at all :-)
On a related note, now I come to start writing this code, it turns
out that it's awkward having the kernel and QEMU not using the same
arbitrary mapping from (cp,is64,crn,crm,opc1,opc2) to index, because
it means having to do conversion code to get from the kernel's MSR
index number to fish a QEMU ARMCPRegInfo* out of our hashtable.
So one of us should change. I don't particularly object to changing
QEMU, but your chosen layout is kind of painful, because the
differing placement of opc1 for 32 and 64 bits means you can't do
the equivalent of qemu's current:
#define ENCODE_CP_REG(cp, is64, crn, crm, opc1, opc2) \
(((cp) << 16) | ((is64) << 15) | ((crn) << 11) | \
((crm) << 7) | ((opc1) << 3) | (opc2))
without adding an annoying ?: to it. I'm not sure it's annoying
enough to be worth changing though.
-- PMM
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
