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 :(
> 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
