MPIDR register usage in ARMv8

MPIDR register usage in ARMv8

[Vijay Kilari]

Hi Ian,

I understand that arm64/head.S is using MPIDR definitions
from xen/include/asm-arm/processor.h which is valid for arm32 but
not for ARMv8 as below

/* MPIDR Multiprocessor Affinity Register */
#define _MPIDR_UP           (30)
#define MPIDR_UP            (_AC(1,U) << _MPIDR_UP)
#define _MPIDR_SMP          (31)
#define MPIDR_SMP           (_AC(1,U) << _MPIDR_SMP)
#define MPIDR_AFF0_SHIFT    (0)
#define MPIDR_AFF0_MASK     (_AC(0xff,U) << MPIDR_AFF0_SHIFT)
#define MPIDR_HWID_MASK     _AC(0xffffff,U)
#define MPIDR_INVALID       (~MPIDR_HWID_MASK)

The same is used in arm64/head.S checking for bit 31 (_MPIDR_SMP) which is not
valid in MPIDR_EL1 register definition also MPIDR_HWID_MASK should
be updated for ARMv8

arm64/head.s:

        mrs   x0, mpidr_el1
        tbz   x0, _MPIDR_SMP, 1f     /* Multiprocessor extension not
supported? */
        tbnz  x0, _MPIDR_UP, 1f      /* Uniprocessor system? */

        mov   x13, #(~MPIDR_HWID_MASK)
        bic   x24, x0, x13           /* Mask out flags to get CPU ID */
1:

Do you agree that this requires change?

Regards
Vijay

Re: MPIDR register usage in ARMv8

[Julien Grall]

On 16/04/14 06:53, Vijay Kilari wrote:
> Hi Ian,

Hi Vijaya,
>
> I understand that arm64/head.S is using MPIDR definitions
> from xen/include/asm-arm/processor.h which is valid for arm32 but
> not for ARMv8 as below
>
> /* MPIDR Multiprocessor Affinity Register */
> #define _MPIDR_UP           (30)
> #define MPIDR_UP            (_AC(1,U) << _MPIDR_UP)
> #define _MPIDR_SMP          (31)
> #define MPIDR_SMP           (_AC(1,U) << _MPIDR_SMP)
> #define MPIDR_AFF0_SHIFT    (0)
> #define MPIDR_AFF0_MASK     (_AC(0xff,U) << MPIDR_AFF0_SHIFT)
> #define MPIDR_HWID_MASK     _AC(0xffffff,U)
> #define MPIDR_INVALID       (~MPIDR_HWID_MASK)
>
> The same is used in arm64/head.S checking for bit 31 (_MPIDR_SMP) which is not
> valid in MPIDR_EL1 register definition also MPIDR_HWID_MASK should
> be updated for ARMv8

Bit 31 is RAO on ARM64. So the check is useless below.

Except the MPIDR_HWID_MASK I don't see any problem as the only 
difference between ARMv8 and ARMv7 is adding a new affinity field (AFF3).

>
> arm64/head.s:
>
>          mrs   x0, mpidr_el1
>          tbz   x0, _MPIDR_SMP, 1f     /* Multiprocessor extension not
> supported? */
>          tbnz  x0, _MPIDR_UP, 1f      /* Uniprocessor system? */
>
>          mov   x13, #(~MPIDR_HWID_MASK)
>          bic   x24, x0, x13           /* Mask out flags to get CPU ID */
> 1:
>
> Do you agree that this requires change?

What are the changes? You only copied the existing code?

Regards,

-- 
Julien Grall

Re: MPIDR register usage in ARMv8

[Vijay Kilari]

On Wed, Apr 16, 2014 at 2:36 PM, Julien Grall <julien.grall@linaro.org> wrote:
>
>
> On 16/04/14 06:53, Vijay Kilari wrote:
>>
>> Hi Ian,
>
>
> Hi Vijaya,
>
>>
>> I understand that arm64/head.S is using MPIDR definitions
>> from xen/include/asm-arm/processor.h which is valid for arm32 but
>> not for ARMv8 as below
>>
>> /* MPIDR Multiprocessor Affinity Register */
>> #define _MPIDR_UP           (30)
>> #define MPIDR_UP            (_AC(1,U) << _MPIDR_UP)
>> #define _MPIDR_SMP          (31)
>> #define MPIDR_SMP           (_AC(1,U) << _MPIDR_SMP)
>> #define MPIDR_AFF0_SHIFT    (0)
>> #define MPIDR_AFF0_MASK     (_AC(0xff,U) << MPIDR_AFF0_SHIFT)
>> #define MPIDR_HWID_MASK     _AC(0xffffff,U)
>> #define MPIDR_INVALID       (~MPIDR_HWID_MASK)
>>
>> The same is used in arm64/head.S checking for bit 31 (_MPIDR_SMP) which is
>> not
>> valid in MPIDR_EL1 register definition also MPIDR_HWID_MASK should
>> be updated for ARMv8
>
>
> Bit 31 is RAO on ARM64. So the check is useless below.
>
> Except the MPIDR_HWID_MASK I don't see any problem as the only difference
> between ARMv8 and ARMv7 is adding a new affinity field (AFF3).

Thanks. Yes, I will send a patch. I will move these MPIDR definitions
from include/asm-arm/processor.h
to include/asm-arm/arm32/processor.h & include/asm-arm/arm64/processor.h
>
>
>>
>> arm64/head.s:
>>
>>          mrs   x0, mpidr_el1
>>          tbz   x0, _MPIDR_SMP, 1f     /* Multiprocessor extension not
>> supported? */
>>          tbnz  x0, _MPIDR_UP, 1f      /* Uniprocessor system? */
>>
>>          mov   x13, #(~MPIDR_HWID_MASK)
>>          bic   x24, x0, x13           /* Mask out flags to get CPU ID */
>> 1:
>>
>> Do you agree that this requires change?
>
>
> What are the changes? You only copied the existing code?
It is just copy of existing code.
>
> Regards,
>
> --
> Julien Grall

Re: MPIDR register usage in ARMv8

[Ian Campbell]

On Wed, 2014-04-16 at 14:58 +0530, Vijay Kilari wrote:
> > Except the MPIDR_HWID_MASK I don't see any problem as the only difference
> > between ARMv8 and ARMv7 is adding a new affinity field (AFF3).
> 
> Thanks. Yes, I will send a patch. I will move these MPIDR definitions
> from include/asm-arm/processor.h
> to include/asm-arm/arm32/processor.h & include/asm-arm/arm64/processor.h

I don't think that is strictly necessary, something like:

#define MPIDR_AFF0_SHIFT    (0)
#define MPIDR_AFF0_MASK     (_AC(0xff,U) << MPIDR_AFF0_SHIFT)
#define MPIDR_AFF1_SHIFT    (8)
#define MPIDR_AFF1_MASK     (_AC(0xff,U) << MPIDR_AFF1_SHIFT)
#define MPIDR_AFF2_SHIFT    (16)
#define MPIDR_AFF2_MASK     (_AC(0xff,U) << MPIDR_AFF2_SHIFT)
#define MPIDR_AFF3_SHIFT    (32)
#define MPIDR_AFF3_MASK     (_AC(0xff,U) << MPIDR_AFF3_SHIFT)
#ifdef CONFIG_ARM_32
#define MPIDR_HWID_MASK     MPIDR_AFF0_MASK|MPIDR_AFF1_MASK|MPIDR_AFF2_MASK
else
#define MPIDR_HWID_MASK     MPIDR_AFF0_MASK|MPIDR_AFF1_MASK|MPIDR_AFF2_MASK|MPIDR_AFF3_MASK
#endif
#define MPIDR_INVALID       (~MPIDR_HWID_MASK)

would be fine I think.

Ian.

Re: MPIDR register usage in ARMv8

[Ian Campbell]

On Wed, 2014-04-16 at 11:23 +0530, Vijay Kilari wrote:
> Hi Ian,
> 
> I understand that arm64/head.S is using MPIDR definitions
> from xen/include/asm-arm/processor.h which is valid for arm32 but
> not for ARMv8 as below
> 
> /* MPIDR Multiprocessor Affinity Register */
> #define _MPIDR_UP           (30)
> #define MPIDR_UP            (_AC(1,U) << _MPIDR_UP)
> #define _MPIDR_SMP          (31)
> #define MPIDR_SMP           (_AC(1,U) << _MPIDR_SMP)
> #define MPIDR_AFF0_SHIFT    (0)
> #define MPIDR_AFF0_MASK     (_AC(0xff,U) << MPIDR_AFF0_SHIFT)
> #define MPIDR_HWID_MASK     _AC(0xffffff,U)
> #define MPIDR_INVALID       (~MPIDR_HWID_MASK)
> 
> The same is used in arm64/head.S checking for bit 31 (_MPIDR_SMP) which is not
> valid in MPIDR_EL1 register definition

It's mostly there for symmetry with the arm32 code, and since arm64
mandates read-as-one (not strictly speaking invalid) to match the
meaning of the bit patterns on arm32 to the requirements of arm64 I
think it is mostly harmless and I suppose it is future proof against the
possibility that future v8 revisions might reintroduce the meanings of
those bits.

>  also MPIDR_HWID_MASK should
> be updated for ARMv8

No harm in adding AFF3 into the mix I suppose.

> 
> arm64/head.s:
> 
>         mrs   x0, mpidr_el1
>         tbz   x0, _MPIDR_SMP, 1f     /* Multiprocessor extension not
> supported? */
>         tbnz  x0, _MPIDR_UP, 1f      /* Uniprocessor system? */
> 
>         mov   x13, #(~MPIDR_HWID_MASK)
>         bic   x24, x0, x13           /* Mask out flags to get CPU ID */
> 1:
> 
> Do you agree that this requires change?