Hi Christoffer, On 06/08/13 05:41, Christoffer Dall wrote: > The PAR was exported as CRn == 7 and CRm == 0, but in fact the primary > coprocessor register number was determined by CRm for 64-bit coprocessor > registers as the user space API was modelled after the coprocessor > access instructions (see the ARM ARM rev. C - B3-1445). > > However, just changing the CRn to CRm breaks the sorting check when > booting the kernel, because the internal kernel logic always treats CRn > as the primary register number, and it makes the table sorting > impossible to understand for humans. > > Alternatively we could change the logic to always have CRn == CRm, but > that becomes unclear in the number of ways we do lookup of a coprocessor > register. We could also have a separate 64-bit table but that feels > somewhat over-engineerd. Instead, keep CRn the primary representation
over-engineered > of the primary corproc. register number in-kernel and always export the coproc. > primary number as CRm as per the existing user space ABI. > > Note: The TTBR registers just magically worked because they happened to > follow the CRn(0) regs and were considered CRn(0) in the in-kernel > representation. Nice catch. This unfortunately shows how little of the userspace interface we've been actually using so far. I suppose you found this by playing with save/restore? > Signed-off-by: Christoffer Dall <[email protected]> > --- > arch/arm/kvm/coproc.c | 23 +++++++++++++++++------ > arch/arm/kvm/coproc.h | 2 ++ > arch/arm/kvm/coproc_a15.c | 5 ++++- > 3 files changed, 23 insertions(+), 7 deletions(-) > > diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c > index 4a51990..fc5fec2 100644 > --- a/arch/arm/kvm/coproc.c > +++ b/arch/arm/kvm/coproc.c > @@ -146,7 +146,10 @@ static bool pm_fake(struct kvm_vcpu *vcpu, > #define access_pmintenclr pm_fake > > /* Architected CP15 registers. > - * Important: Must be sorted ascending by CRn, CRM, Op1, Op2 > + * CRn denotes the primary register number, but is copied to the CRm in the > + * user space API in line with the terminology used in the ARM ARM. Please consider adding something like "in the 64bit case only". > + * Important: Must be sorted ascending by CRn, CRM, Op1, Op2 and with 64-bit > + * registers preceeding 32-bit ones. preceding > */ > static const struct coproc_reg cp15_regs[] = { > /* CSSELR: swapped by interrupt.S. */ > @@ -154,8 +157,8 @@ static const struct coproc_reg cp15_regs[] = { > NULL, reset_unknown, c0_CSSELR }, > > /* TTBR0/TTBR1: swapped by interrupt.S. */ > - { CRm( 2), Op1( 0), is64, NULL, reset_unknown64, c2_TTBR0 }, > - { CRm( 2), Op1( 1), is64, NULL, reset_unknown64, c2_TTBR1 }, > + { CRn( 2), Op1( 0), is64, NULL, reset_unknown64, c2_TTBR0 }, > + { CRn( 2), Op1( 1), is64, NULL, reset_unknown64, c2_TTBR1 }, How about this (untested) alternative possibility: #define CRm64(_x) .CRn = _x, .is64 = 1, { CRm64( 2), Op1( 0), NULL, reset_unknown64, c2_TTBR0 }, { CRm64( 2), Op1( 1), NULL, reset_unknown64, c2_TTBR1 }, It has the benefit of still showing CRm as being the official field, and hide the ugliness into the macro. Still ugly though... > /* TTBCR: swapped by interrupt.S. */ > { CRn( 2), CRm( 0), Op1( 0), Op2( 2), is32, > @@ -399,12 +402,13 @@ static bool index_to_params(u64 id, struct > coproc_params *params) > | KVM_REG_ARM_OPC1_MASK)) > return false; > params->is_64bit = true; > - params->CRm = ((id & KVM_REG_ARM_CRM_MASK) > + /* CRm to CRn: see cp15_to_index for details */ > + params->CRn = ((id & KVM_REG_ARM_CRM_MASK) > >> KVM_REG_ARM_CRM_SHIFT); > params->Op1 = ((id & KVM_REG_ARM_OPC1_MASK) > >> KVM_REG_ARM_OPC1_SHIFT); > params->Op2 = 0; > - params->CRn = 0; > + params->CRm = 0; > return true; > default: > return false; > @@ -898,7 +902,14 @@ static u64 cp15_to_index(const struct coproc_reg *reg) > if (reg->is_64) { > val |= KVM_REG_SIZE_U64; > val |= (reg->Op1 << KVM_REG_ARM_OPC1_SHIFT); > - val |= (reg->CRm << KVM_REG_ARM_CRM_SHIFT); > + /* > + * CRn always denotes the primary coproc. reg. nr. for the > + * in-kernel representation, but the user space API uses the > + * CRm for the encoding, because it is modelled after the > + * MRRC/MCRR instructions: see the ARM ARM rev. c page > + * B3-1445 > + */ > + val |= (reg->CRn << KVM_REG_ARM_CRM_SHIFT); > } else { > val |= KVM_REG_SIZE_U32; > val |= (reg->Op1 << KVM_REG_ARM_OPC1_SHIFT); > diff --git a/arch/arm/kvm/coproc.h b/arch/arm/kvm/coproc.h > index b7301d3..dccb904 100644 > --- a/arch/arm/kvm/coproc.h > +++ b/arch/arm/kvm/coproc.h > @@ -135,6 +135,8 @@ static inline int cmp_reg(const struct coproc_reg *i1, > return -1; > if (i1->CRn != i2->CRn) > return i1->CRn - i2->CRn; > + if (i1->is_64 != i2->is_64) > + return i2->is_64 - i1->is_64; > if (i1->CRm != i2->CRm) > return i1->CRm - i2->CRm; > if (i1->Op1 != i2->Op1) > diff --git a/arch/arm/kvm/coproc_a15.c b/arch/arm/kvm/coproc_a15.c > index 685063a..3d8f61f 100644 > --- a/arch/arm/kvm/coproc_a15.c > +++ b/arch/arm/kvm/coproc_a15.c > @@ -114,7 +114,10 @@ static bool access_l2ectlr(struct kvm_vcpu *vcpu, > > /* > * A15-specific CP15 registers. > - * Important: Must be sorted ascending by CRn, CRM, Op1, Op2 > + * CRn denotes the primary register number, but is copied to the CRm in the > + * user space API in line with the terminology used in the ARM ARM. > + * Important: Must be sorted ascending by CRn, CRM, Op1, Op2 and with 64-bit > + * registers preceeding 32-bit ones. > */ > static const struct coproc_reg a15_regs[] = { > /* MPIDR: we use VMPIDR for guest access. */ > Cheers, M. -- Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
