Advertising

Let's make sure this one addresses the need of the gic one, and then
I'll merge this one into arm/next first and drop the other one.
>
> > However changing it to 64-bit with this patch would result
> > in a get_mpidr() call that returns uint64_t on arm64 and uint32_t on
> > arm32, which won't be nice for common code. Andre brought up during the
> > review of [1] that we should be using the architectural types for register
> > accessors.
>
> At least for registers that differ in size between A32 and A64. Many
> system registers are actually 32-bit wide and are explicitly stated as
> such in the ARM(v8) ARM (for instance MIDR_EL1).
> Yes, the A64 msr/mrs instructions only know a 64-bit register encoding,
> but the actual content is often confined to 32 bits (in MIDR or PMCR,
> for instance).
> So I wonder if we should take care of those with an explicit uint32_t
> return type?
>
> > That means, that while internally all the above functions can
> > know what's 32-bit and what's 64-bit, using uint32/64_t appropriately,
> > the external interfaces should be 'unsigned long', 'unsigned int',
> > 'unsigned long long'.
>
> Not so sure about that.
> I think we may need _three_ types of system register accessors?
> 1) always 32-bit (e.g. MIDR_EL1): use uint32_t
> 2) always 64-bit (e.g. CNTVCT_EL0),: use uint64_t
> 3) natural register size (e.g. MPIDR_EL1): use unsigned long
>
> Does that make sense or is that overkill?
OK, now that'd I've rethought about this, the natural sizes weren't
a good idea. They don't really give us better common code anyway,
because 64-bit code may want to look at the upper 32 bits. I think
we need to forget (3) above, and special case MPIDR for arm32 accesses.
MPIDR is special because it gets used by core smp and gic code, and we
don't want #ifdefs everywhere.
So, for this patch, I think we only need to add
DEFINE_GET_SYSREG32(mpidr32, 0, c0, c0, 5)
static inline uint64_t get_mpidr(void)
{
return get_mpidr32();
}
to the 32-bit processor.h
and in the 64-bit file we need to switch DEFINE_GET_SYSREG32(mpidr, el1)
to DEFINE_GET_SYSREG64(mpidr, el1). Also, the second reply I made still
holds; always using 64-bit (unsigned long) internally for AArch64
registers, but choosing to just return the lower 32 bits for the "32-bit"
ones (Peter's arguments weren't lost on me :-)
How's that sound?
Thanks,
drew
(Changing get_mpidr() to return a uint64_t means I'll need to fixup [2],
but no biggy...)
[2]
https://github.com/rhdrjones/kvm-unit-tests/commit/9f3f7f8141a98d0cd5175ad6cb491a4e1c5f7cd9
>
> Cheers,
> Andre.
>
> > [1]
> > https://github.com/rhdrjones/kvm-unit-tests/commit/57e48b8e6dc2ddf4b2e4eb1ceb5a5f87f2dd074b
> >
> > Thanks,
> > drew
> >
> >>
> >> /* Only support Aff0 for now, gicv2 only */
> >> #define mpidr_to_cpu(mpidr) ((int)((mpidr) & 0xff))
> >> --
> >> 1.8.3.1
> >>
> >>
>