On Tue, Aug 06, 2013 at 09:43:25AM +0100, Marc Zyngier wrote:
> 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?
>
Actually, I just ran a recent QEMU with Peter's reworked coprocessor
interface and it blew up in my face, shame on us.
> > Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> > ---
> > 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".
>
right
> > + * 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...
>
thought about this too, but discarded the idea, but seeing it written
out, it's actually not that bad - or IOW what I suggested was at least
as ugly. I'll go and steal this.
> > /* 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. */
> >
Thanks for all the spelling fixes, the time is over when I should post
to the list after 5pm I guess.
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
