Am 20.08.2012 um 06:57 schrieb Rusty Russell <rusty.russell@xxxxxxxxxx>:
> On Thu, 16 Aug 2012 17:31:53 +0100, Peter Maydell <peter.maydell@xxxxxxxxxx> wrote:
>> 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 :-)
>
> To be precise, you'd prefer:
>
> #define KVM_ARM_MSR_COPROC_MASK_START 16
> #define KVM_ARM_MSR_COPROC_MASK_LEN 16
>
> vs:
> #define KVM_ARM_MSR_COPROC_MASK 0xFFFF0000
>
> What a huge readability lose. I can't describe how distasteful it is to
> do this because QEMU would have to write a macro :(
The usual way this is done in the kernel is
#define xxx_SHIFT 16
#define xxx_MASK (0xffff << xxx_SHIFT)
Right? Would that work for you guys? It'd certainly be easier to read.
Alex
>
>> 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.
>
> Yeah, that was dumb. I'll copy qemu, since Christoffer didn't hate the
> idea.
>
> Does this work for you?
>
> Thanks,
> Rusty.
>
> diff --git a/arch/arm/include/asm/kvm.h b/arch/arm/include/asm/kvm.h
> index d040a2a..a2c343a 100644
> --- a/arch/arm/include/asm/kvm.h
> +++ b/arch/arm/include/asm/kvm.h
> @@ -106,14 +106,16 @@ struct kvm_msr_list {
> __u32 indices[0];
> };
>
> -/* 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
> +/* If you need to interpret the index values, here are the bit offsets. */
> +#define KVM_ARM_MSR_COPROC_START 16 /* Mask: 0xFFFF0000 */
> +#define KVM_ARM_MSR_64_BIT_START 15 /* Mask: 0x00008000 */
> +#define KVM_ARM_MSR_32_OPC2_START 0 /* Mask: 0x00000007 */
> +#define KVM_ARM_MSR_32_OPC2_LEN 3
> +#define KVM_ARM_MSR_OPC1_START 3 /* Mask: 0x00000078 */
> +#define KVM_ARM_MSR_OPC1_LEN 4
> +#define KVM_ARM_MSR_CRM_START 7 /* Mask: 0x00000780 */
> +#define KVM_ARM_MSR_CRM_LEN 4
> +#define KVM_ARM_MSR_32_CRN_START 11 /* Mask: 0x00007800 */
> +#define KVM_ARM_MSR_32_CRN_LEN 4
>
> #endif /* __ARM_KVM_H__ */
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index ede9310..fe7d867 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -584,25 +584,31 @@ int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
> *****************************************************************************/
>
> /* Given a simple mask, get those bits. */
> -static inline u32 get_bits(u32 index, u32 mask)
> +static inline u32 get_bits(u32 index, u32 start, u32 len)
> {
> - return (index & mask) >> (ffs(mask) - 1);
> + return (index >> start) & ((1 << len)-1);
> }
>
> static void index_to_params(u32 index, struct coproc_params *params)
> {
> - if (get_bits(index, KVM_ARM_MSR_64_BIT_MASK)) {
> + if (get_bits(index, KVM_ARM_MSR_64_BIT_START, 1)) {
> params->is_64bit = true;
> - params->CRm = get_bits(index, KVM_ARM_MSR_64_CRM_MASK);
> - params->Op1 = get_bits(index, KVM_ARM_MSR_64_OPC1_MASK);
> + params->CRm = get_bits(index, KVM_ARM_MSR_CRM_START,
> + KVM_ARM_MSR_CRM_LEN);
> + params->Op1 = get_bits(index, KVM_ARM_MSR_OPC1_START,
> + KVM_ARM_MSR_OPC1_LEN);
> params->Op2 = 0;
> params->CRn = 0;
> } else {
> params->is_64bit = false;
> - params->CRn = get_bits(index, KVM_ARM_MSR_32_CRN_MASK);
> - params->CRm = get_bits(index, KVM_ARM_MSR_32_CRM_MASK);
> - params->Op1 = get_bits(index, KVM_ARM_MSR_32_OPC1_MASK);
> - params->Op2 = get_bits(index, KVM_ARM_MSR_32_OPC2_MASK);
> + params->CRn = get_bits(index, KVM_ARM_MSR_32_CRN_START,
> + KVM_ARM_MSR_32_CRN_LEN);
> + params->CRm = get_bits(index, KVM_ARM_MSR_CRM_START,
> + KVM_ARM_MSR_CRM_LEN);
> + params->Op1 = get_bits(index, KVM_ARM_MSR_OPC1_START,
> + KVM_ARM_MSR_OPC1_LEN);
> + params->Op2 = get_bits(index, KVM_ARM_MSR_32_OPC2_START,
> + KVM_ARM_MSR_32_OPC2_LEN);
> }
> }
>
> @@ -615,7 +621,7 @@ static const struct coproc_reg *index_to_coproc_reg(struct kvm_vcpu *vcpu,
> struct coproc_params params;
>
> /* We only do cp15 for now. */
> - if (get_bits(index, KVM_ARM_MSR_COPROC_MASK != 15))
> + if (get_bits(index, KVM_ARM_MSR_COPROC_START, 16) != 15)
> return NULL;
>
> index_to_params(index, ¶ms);
> @@ -841,24 +847,18 @@ static int cmp_reg(const struct coproc_reg *i1, const struct coproc_reg *i2)
> return i1->Op2 - i2->Op2;
> }
>
> -/* Puts in the position indicated by mask (assumes val fits in mask) */
> -static inline u32 set_bits(u32 val, u32 mask)
> -{
> - return val << (ffs(mask)-1);
> -}
> -
> static u32 cp15_to_index(const struct coproc_reg *reg)
> {
> - u32 val = set_bits(15, KVM_ARM_MSR_COPROC_MASK);
> + u32 val = (15 << KVM_ARM_MSR_COPROC_START);
> if (reg->is_64) {
> - val |= set_bits(1, KVM_ARM_MSR_64_BIT_MASK);
> - val |= set_bits(reg->Op1, KVM_ARM_MSR_64_OPC1_MASK);
> - val |= set_bits(reg->CRm, KVM_ARM_MSR_64_CRM_MASK);
> + val |= (1 << KVM_ARM_MSR_64_BIT_START);
> + val |= (reg->Op1 << KVM_ARM_MSR_OPC1_START);
> + val |= (reg->CRm << KVM_ARM_MSR_CRM_START);
> } else {
> - val |= set_bits(reg->Op1, KVM_ARM_MSR_32_OPC1_MASK);
> - val |= set_bits(reg->Op2, KVM_ARM_MSR_32_OPC2_MASK);
> - val |= set_bits(reg->CRm, KVM_ARM_MSR_32_CRM_MASK);
> - val |= set_bits(reg->CRn, KVM_ARM_MSR_32_CRN_MASK);
> + val |= (reg->Op1 << KVM_ARM_MSR_OPC1_START);
> + val |= (reg->Op2 << KVM_ARM_MSR_32_OPC2_START);
> + val |= (reg->CRm << KVM_ARM_MSR_CRM_START);
> + val |= (reg->CRn << KVM_ARM_MSR_32_CRN_START);
> }
> return val;
> }
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
