On Thu, Aug 16, 2012 at 12:31 PM, Peter Maydell
<peter.maydell@xxxxxxxxxx> wrote:
> 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.
>
I have no objects for moving the opc1 for the 64 bit version in the
kernel if that makes life easier...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
