| Message ID | 1473350810-10857-7-git-send-email-vladimir.murzin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/09/16 17:06, Vladimir Murzin wrote: > vgic-v3 driver queries CPU affinity level up to Aff3, which is valid > for arm64. However, for arm up to Aff2 levels are supported, so > querying for third level ends with upper bits of MPIDR are treated as > valid affinity level which is not true. Make sure we report zero for > any affinity level above two. > > Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> > --- > arch/arm/include/asm/cputype.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h > index 1ee94c7..f08fac4 100644 > --- a/arch/arm/include/asm/cputype.h > +++ b/arch/arm/include/asm/cputype.h > @@ -55,9 +55,10 @@ > > #define MPIDR_LEVEL_BITS 8 > #define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) > +#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level) > > #define MPIDR_AFFINITY_LEVEL(mpidr, level) \ > - ((mpidr >> (MPIDR_LEVEL_BITS * level)) & MPIDR_LEVEL_MASK) > + (((mpidr & MPIDR_HWID_BITMASK) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) > > #define ARM_CPU_IMP_ARM 0x41 > #define ARM_CPU_IMP_INTEL 0x69 > There is something I don't quite get. Is this patch really necessary? Are there cases where we construct a MPIDR that can have Aff3 set on 32bit? Thanks, M.
On 09/09/16 17:59, Marc Zyngier wrote: > On 08/09/16 17:06, Vladimir Murzin wrote: >> vgic-v3 driver queries CPU affinity level up to Aff3, which is valid >> for arm64. However, for arm up to Aff2 levels are supported, so >> querying for third level ends with upper bits of MPIDR are treated as >> valid affinity level which is not true. Make sure we report zero for >> any affinity level above two. >> >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> >> --- >> arch/arm/include/asm/cputype.h | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h >> index 1ee94c7..f08fac4 100644 >> --- a/arch/arm/include/asm/cputype.h >> +++ b/arch/arm/include/asm/cputype.h >> @@ -55,9 +55,10 @@ >> >> #define MPIDR_LEVEL_BITS 8 >> #define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) >> +#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level) >> >> #define MPIDR_AFFINITY_LEVEL(mpidr, level) \ >> - ((mpidr >> (MPIDR_LEVEL_BITS * level)) & MPIDR_LEVEL_MASK) >> + (((mpidr & MPIDR_HWID_BITMASK) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) >> >> #define ARM_CPU_IMP_ARM 0x41 >> #define ARM_CPU_IMP_INTEL 0x69 >> > > There is something I don't quite get. Is this patch really necessary? > Are there cases where we construct a MPIDR that can have Aff3 set on 32bit? I've just checked and it seems that I've been keeping the change for MPIDR_AFFINITY_LEVEL() since vgic-old era where it was used with static u32 compress_mpidr(unsigned long mpidr) { u32 ret; ret = MPIDR_AFFINITY_LEVEL(mpidr, 0); ret |= MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8; ret |= MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16; ret |= MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24; return ret; } and now that code gone and nobody passes level 3 into this macro, so I can drop this change. However, I do need MPIDR_LEVEL_SHIFT() macro since it is used in vgic-v3. Should I leave patch as is or you have something in mind? Cheers Vladimir > > Thanks, > > M. >
On 12/09/16 10:39, Vladimir Murzin wrote: > On 09/09/16 17:59, Marc Zyngier wrote: >> On 08/09/16 17:06, Vladimir Murzin wrote: >>> vgic-v3 driver queries CPU affinity level up to Aff3, which is valid >>> for arm64. However, for arm up to Aff2 levels are supported, so >>> querying for third level ends with upper bits of MPIDR are treated as >>> valid affinity level which is not true. Make sure we report zero for >>> any affinity level above two. >>> >>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> >>> --- >>> arch/arm/include/asm/cputype.h | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h >>> index 1ee94c7..f08fac4 100644 >>> --- a/arch/arm/include/asm/cputype.h >>> +++ b/arch/arm/include/asm/cputype.h >>> @@ -55,9 +55,10 @@ >>> >>> #define MPIDR_LEVEL_BITS 8 >>> #define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) >>> +#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level) >>> >>> #define MPIDR_AFFINITY_LEVEL(mpidr, level) \ >>> - ((mpidr >> (MPIDR_LEVEL_BITS * level)) & MPIDR_LEVEL_MASK) >>> + (((mpidr & MPIDR_HWID_BITMASK) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) >>> >>> #define ARM_CPU_IMP_ARM 0x41 >>> #define ARM_CPU_IMP_INTEL 0x69 >>> >> >> There is something I don't quite get. Is this patch really necessary? >> Are there cases where we construct a MPIDR that can have Aff3 set on 32bit? > > I've just checked and it seems that I've been keeping the change for > MPIDR_AFFINITY_LEVEL() since vgic-old era where it was used with > > static u32 compress_mpidr(unsigned long mpidr) > { > u32 ret; > > ret = MPIDR_AFFINITY_LEVEL(mpidr, 0); > ret |= MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8; > ret |= MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16; > ret |= MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24; > > return ret; > } > > and now that code gone and nobody passes level 3 into this macro, so I > can drop this change. > > However, I do need MPIDR_LEVEL_SHIFT() macro since it is used in vgic-v3. > > Should I leave patch as is or you have something in mind? I think that for the sake of keeping the change minimal, it'd be better to just add the MPIDR_LEVEL_SHIFT macro, and drop the other changes. Thanks, M.
On 12/09/16 10:48, Marc Zyngier wrote: > On 12/09/16 10:39, Vladimir Murzin wrote: >> On 09/09/16 17:59, Marc Zyngier wrote: >>> On 08/09/16 17:06, Vladimir Murzin wrote: >>>> vgic-v3 driver queries CPU affinity level up to Aff3, which is valid >>>> for arm64. However, for arm up to Aff2 levels are supported, so >>>> querying for third level ends with upper bits of MPIDR are treated as >>>> valid affinity level which is not true. Make sure we report zero for >>>> any affinity level above two. >>>> >>>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> >>>> --- >>>> arch/arm/include/asm/cputype.h | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h >>>> index 1ee94c7..f08fac4 100644 >>>> --- a/arch/arm/include/asm/cputype.h >>>> +++ b/arch/arm/include/asm/cputype.h >>>> @@ -55,9 +55,10 @@ >>>> >>>> #define MPIDR_LEVEL_BITS 8 >>>> #define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) >>>> +#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level) >>>> >>>> #define MPIDR_AFFINITY_LEVEL(mpidr, level) \ >>>> - ((mpidr >> (MPIDR_LEVEL_BITS * level)) & MPIDR_LEVEL_MASK) >>>> + (((mpidr & MPIDR_HWID_BITMASK) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) >>>> >>>> #define ARM_CPU_IMP_ARM 0x41 >>>> #define ARM_CPU_IMP_INTEL 0x69 >>>> >>> >>> There is something I don't quite get. Is this patch really necessary? >>> Are there cases where we construct a MPIDR that can have Aff3 set on 32bit? >> >> I've just checked and it seems that I've been keeping the change for >> MPIDR_AFFINITY_LEVEL() since vgic-old era where it was used with >> >> static u32 compress_mpidr(unsigned long mpidr) >> { >> u32 ret; >> >> ret = MPIDR_AFFINITY_LEVEL(mpidr, 0); >> ret |= MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8; >> ret |= MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16; >> ret |= MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24; >> >> return ret; >> } >> >> and now that code gone and nobody passes level 3 into this macro, so I >> can drop this change. >> >> However, I do need MPIDR_LEVEL_SHIFT() macro since it is used in vgic-v3. >> >> Should I leave patch as is or you have something in mind? > > I think that for the sake of keeping the change minimal, it'd be better > to just add the MPIDR_LEVEL_SHIFT macro, and drop the other changes. Noted! Thanks Vladimir > > Thanks, > > M. >
diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h index 1ee94c7..f08fac4 100644 --- a/arch/arm/include/asm/cputype.h +++ b/arch/arm/include/asm/cputype.h @@ -55,9 +55,10 @@ #define MPIDR_LEVEL_BITS 8 #define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) +#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level) #define MPIDR_AFFINITY_LEVEL(mpidr, level) \ - ((mpidr >> (MPIDR_LEVEL_BITS * level)) & MPIDR_LEVEL_MASK) + (((mpidr & MPIDR_HWID_BITMASK) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) #define ARM_CPU_IMP_ARM 0x41 #define ARM_CPU_IMP_INTEL 0x69
vgic-v3 driver queries CPU affinity level up to Aff3, which is valid for arm64. However, for arm up to Aff2 levels are supported, so querying for third level ends with upper bits of MPIDR are treated as valid affinity level which is not true. Make sure we report zero for any affinity level above two. Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> --- arch/arm/include/asm/cputype.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)