| Message ID | 20221021153128.44226-3-ayankuma@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Arm: Enable GICv3 for AArch32 | expand |
On 10/21/22 18:31, Ayan Kumar Halder wrote: Hi Ayan > Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm64/ \ > include/asm/cputype.h#L14 , these macros are specific for arm64. > > When one computes MPIDR_LEVEL_SHIFT(3), it crosses the width of a 32 > bit register. > > Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm/include/ \ > asm/cputype.h#L54 , these macros are specific for arm32. > > Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> > --- > xen/arch/arm/include/asm/arm32/processor.h | 10 ++++++++++ > xen/arch/arm/include/asm/arm64/processor.h | 13 +++++++++++++ > xen/arch/arm/include/asm/processor.h | 14 -------------- > 3 files changed, 23 insertions(+), 14 deletions(-) > > diff --git a/xen/arch/arm/include/asm/arm32/processor.h b/xen/arch/arm/include/asm/arm32/processor.h > index 4e679f3273..3e03ce78dc 100644 > --- a/xen/arch/arm/include/asm/arm32/processor.h > +++ b/xen/arch/arm/include/asm/arm32/processor.h > @@ -56,6 +56,16 @@ struct cpu_user_regs > uint32_t pad1; /* Doubleword-align the user half of the frame */ > }; > > +/* > + * Macros to extract affinity level. Picked from kernel > + */ > + > +#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) > + > #endif > > #endif /* __ASM_ARM_ARM32_PROCESSOR_H */ > diff --git a/xen/arch/arm/include/asm/arm64/processor.h b/xen/arch/arm/include/asm/arm64/processor.h > index c749f80ad9..c026334eec 100644 > --- a/xen/arch/arm/include/asm/arm64/processor.h > +++ b/xen/arch/arm/include/asm/arm64/processor.h > @@ -84,6 +84,19 @@ struct cpu_user_regs > uint64_t sp_el1, elr_el1; > }; > > +/* > + * Macros to extract affinity level. picked from kernel > + */ > + > +#define MPIDR_LEVEL_BITS_SHIFT 3 > +#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) > + > +#define MPIDR_LEVEL_SHIFT(level) \ > + (((1 << level) >> 1) << MPIDR_LEVEL_BITS_SHIFT) > + > +#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ > + ((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) > + > #undef __DECL_REG > > #endif /* __ASSEMBLY__ */ > diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h > index 1dd81d7d52..7d90c3b5f2 100644 > --- a/xen/arch/arm/include/asm/processor.h > +++ b/xen/arch/arm/include/asm/processor.h > @@ -118,20 +118,6 @@ > #define MPIDR_INVALID (~MPIDR_HWID_MASK) > #define MPIDR_LEVEL_BITS (8) > > - > -/* > - * Macros to extract affinity level. picked from kernel > - */ > - > -#define MPIDR_LEVEL_BITS_SHIFT 3 > -#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) > - > -#define MPIDR_LEVEL_SHIFT(level) \ > - (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) > - > -#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ > - (((mpidr) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) > - > #define AFFINITY_MASK(level) ~((_AC(0x1,UL) << MPIDR_LEVEL_SHIFT(level)) - 1) > > /* TTBCR Translation Table Base Control Register */ Since only the definition of the MPIDR_AFFINITY_LEVEL() differs, maybe you could add only this one to the arch specific headers e.g for arm64: #define MPIDR_LEVEL_SHIFT(level) \ (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) for arm32: #define MPIDR_LEVEL_SHIFT(level) \ ((level) << MPIDR_LEVEL_BITS_SHIFT) But in any case don't forget to add parentheses around the macro parameters when an operator acts on them to avoid trouble with operator precedence (MISRA-C Rule 20.7 :))
On 21/10/2022 22:18, Xenia Ragiadakou wrote: > On 10/21/22 18:31, Ayan Kumar Halder wrote: > Hi Ayan Hi Xenia, > >> Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm64/ \ >> include/asm/cputype.h#L14 , these macros are specific for arm64. >> >> When one computes MPIDR_LEVEL_SHIFT(3), it crosses the width of a 32 >> bit register. >> >> Refer >> https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm/include/ \ >> asm/cputype.h#L54 , these macros are specific for arm32. >> >> Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> >> --- >> xen/arch/arm/include/asm/arm32/processor.h | 10 ++++++++++ >> xen/arch/arm/include/asm/arm64/processor.h | 13 +++++++++++++ >> xen/arch/arm/include/asm/processor.h | 14 -------------- >> 3 files changed, 23 insertions(+), 14 deletions(-) >> >> diff --git a/xen/arch/arm/include/asm/arm32/processor.h >> b/xen/arch/arm/include/asm/arm32/processor.h >> index 4e679f3273..3e03ce78dc 100644 >> --- a/xen/arch/arm/include/asm/arm32/processor.h >> +++ b/xen/arch/arm/include/asm/arm32/processor.h >> @@ -56,6 +56,16 @@ struct cpu_user_regs >> uint32_t pad1; /* Doubleword-align the user half of the frame */ >> }; >> +/* >> + * Macros to extract affinity level. Picked from kernel >> + */ >> + >> +#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) >> + >> #endif >> #endif /* __ASM_ARM_ARM32_PROCESSOR_H */ >> diff --git a/xen/arch/arm/include/asm/arm64/processor.h >> b/xen/arch/arm/include/asm/arm64/processor.h >> index c749f80ad9..c026334eec 100644 >> --- a/xen/arch/arm/include/asm/arm64/processor.h >> +++ b/xen/arch/arm/include/asm/arm64/processor.h >> @@ -84,6 +84,19 @@ struct cpu_user_regs >> uint64_t sp_el1, elr_el1; >> }; >> +/* >> + * Macros to extract affinity level. picked from kernel >> + */ >> + >> +#define MPIDR_LEVEL_BITS_SHIFT 3 >> +#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) >> + >> +#define MPIDR_LEVEL_SHIFT(level) \ >> + (((1 << level) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >> + >> +#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ >> + ((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) >> + >> #undef __DECL_REG >> #endif /* __ASSEMBLY__ */ >> diff --git a/xen/arch/arm/include/asm/processor.h >> b/xen/arch/arm/include/asm/processor.h >> index 1dd81d7d52..7d90c3b5f2 100644 >> --- a/xen/arch/arm/include/asm/processor.h >> +++ b/xen/arch/arm/include/asm/processor.h >> @@ -118,20 +118,6 @@ >> #define MPIDR_INVALID (~MPIDR_HWID_MASK) >> #define MPIDR_LEVEL_BITS (8) >> - >> -/* >> - * Macros to extract affinity level. picked from kernel >> - */ >> - >> -#define MPIDR_LEVEL_BITS_SHIFT 3 >> -#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) >> - >> -#define MPIDR_LEVEL_SHIFT(level) \ >> - (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >> - >> -#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ >> - (((mpidr) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) >> - >> #define AFFINITY_MASK(level) ~((_AC(0x1,UL) << >> MPIDR_LEVEL_SHIFT(level)) - 1) >> /* TTBCR Translation Table Base Control Register */ > > Since only the definition of the MPIDR_AFFINITY_LEVEL() differs, maybe > you could add only this one to the arch specific headers e.g > for arm64: > #define MPIDR_LEVEL_SHIFT(level) \ > (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) > for arm32: > #define MPIDR_LEVEL_SHIFT(level) \ > ((level) << MPIDR_LEVEL_BITS_SHIFT) Also, MPIDR_AFFINITY_LEVEL needs to be defined in arch specific headers as it differs between arm32 and arm64. However, MPIDR_LEVEL_MASK can be defined in the common header (as it is same for arm32 and arm64). Please let me know if it makes sense. > > But in any case don't forget to add parentheses around the macro > parameters when an operator acts on them to avoid trouble with > operator precedence (MISRA-C Rule 20.7 :)) Thanks for pointing it out. Yes, this is a mistake in my patches. - Ayan
Hi Ayan, On 10/24/22 14:00, Ayan Kumar Halder wrote: > > On 21/10/2022 22:18, Xenia Ragiadakou wrote: >> On 10/21/22 18:31, Ayan Kumar Halder wrote: >> Hi Ayan > Hi Xenia, >> >>> Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm64/ \ >>> include/asm/cputype.h#L14 , these macros are specific for arm64. >>> >>> When one computes MPIDR_LEVEL_SHIFT(3), it crosses the width of a 32 >>> bit register. >>> >>> Refer >>> https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm/include/ \ >>> asm/cputype.h#L54 , these macros are specific for arm32. >>> >>> Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> >>> --- >>> xen/arch/arm/include/asm/arm32/processor.h | 10 ++++++++++ >>> xen/arch/arm/include/asm/arm64/processor.h | 13 +++++++++++++ >>> xen/arch/arm/include/asm/processor.h | 14 -------------- >>> 3 files changed, 23 insertions(+), 14 deletions(-) >>> >>> diff --git a/xen/arch/arm/include/asm/arm32/processor.h >>> b/xen/arch/arm/include/asm/arm32/processor.h >>> index 4e679f3273..3e03ce78dc 100644 >>> --- a/xen/arch/arm/include/asm/arm32/processor.h >>> +++ b/xen/arch/arm/include/asm/arm32/processor.h >>> @@ -56,6 +56,16 @@ struct cpu_user_regs >>> uint32_t pad1; /* Doubleword-align the user half of the frame */ >>> }; >>> +/* >>> + * Macros to extract affinity level. Picked from kernel >>> + */ >>> + >>> +#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) >>> + Above, since #define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level) you can replace (MPIDR_LEVEL_BITS * level) with MPIDR_LEVEL_SHIFT(level) in the definition of MPIDR_AFFINITY_LEVEL. You will see that it is identical to the arm64 definition #define MPIDR_AFFINITY_LEVEL(mpidr, level) \ ((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) >>> #endif >>> #endif /* __ASM_ARM_ARM32_PROCESSOR_H */ >>> diff --git a/xen/arch/arm/include/asm/arm64/processor.h >>> b/xen/arch/arm/include/asm/arm64/processor.h >>> index c749f80ad9..c026334eec 100644 >>> --- a/xen/arch/arm/include/asm/arm64/processor.h >>> +++ b/xen/arch/arm/include/asm/arm64/processor.h >>> @@ -84,6 +84,19 @@ struct cpu_user_regs >>> uint64_t sp_el1, elr_el1; >>> }; >>> +/* >>> + * Macros to extract affinity level. picked from kernel >>> + */ >>> + >>> +#define MPIDR_LEVEL_BITS_SHIFT 3 >>> +#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) >>> + >>> +#define MPIDR_LEVEL_SHIFT(level) \ >>> + (((1 << level) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >>> + >>> +#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ >>> + ((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) >>> + >>> #undef __DECL_REG >>> #endif /* __ASSEMBLY__ */ >>> diff --git a/xen/arch/arm/include/asm/processor.h >>> b/xen/arch/arm/include/asm/processor.h >>> index 1dd81d7d52..7d90c3b5f2 100644 >>> --- a/xen/arch/arm/include/asm/processor.h >>> +++ b/xen/arch/arm/include/asm/processor.h >>> @@ -118,20 +118,6 @@ >>> #define MPIDR_INVALID (~MPIDR_HWID_MASK) >>> #define MPIDR_LEVEL_BITS (8) >>> - >>> -/* >>> - * Macros to extract affinity level. picked from kernel >>> - */ >>> - >>> -#define MPIDR_LEVEL_BITS_SHIFT 3 >>> -#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) >>> - >>> -#define MPIDR_LEVEL_SHIFT(level) \ >>> - (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >>> - >>> -#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ >>> - (((mpidr) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) >>> - >>> #define AFFINITY_MASK(level) ~((_AC(0x1,UL) << >>> MPIDR_LEVEL_SHIFT(level)) - 1) >>> /* TTBCR Translation Table Base Control Register */ >> >> Since only the definition of the MPIDR_AFFINITY_LEVEL() differs, maybe >> you could add only this one to the arch specific headers e.g >> for arm64: >> #define MPIDR_LEVEL_SHIFT(level) \ >> (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >> for arm32: >> #define MPIDR_LEVEL_SHIFT(level) \ >> ((level) << MPIDR_LEVEL_BITS_SHIFT) > > Also, MPIDR_AFFINITY_LEVEL needs to be defined in arch specific headers > as it differs between arm32 and arm64. As I point out above, there is no difference between arm32 and arm64 regarding the definition of MPIDR_AFFINITY_LEVEL(level). > > However, MPIDR_LEVEL_MASK can be defined in the common header (as it is > same for arm32 and arm64). > > Please let me know if it makes sense. > >> >> But in any case don't forget to add parentheses around the macro >> parameters when an operator acts on them to avoid trouble with >> operator precedence (MISRA-C Rule 20.7 :)) > > Thanks for pointing it out. Yes, this is a mistake in my patches. > > - Ayan >
On 24/10/2022 12:35, Xenia Ragiadakou wrote: > Hi Ayan, Hi Xenia, > > On 10/24/22 14:00, Ayan Kumar Halder wrote: >> >> On 21/10/2022 22:18, Xenia Ragiadakou wrote: >>> On 10/21/22 18:31, Ayan Kumar Halder wrote: >>> Hi Ayan >> Hi Xenia, >>> >>>> Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm64/ \ >>>> include/asm/cputype.h#L14 , these macros are specific for arm64. >>>> >>>> When one computes MPIDR_LEVEL_SHIFT(3), it crosses the width of a 32 >>>> bit register. >>>> >>>> Refer >>>> https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm/include/ \ >>>> asm/cputype.h#L54 , these macros are specific for arm32. >>>> >>>> Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> >>>> --- >>>> xen/arch/arm/include/asm/arm32/processor.h | 10 ++++++++++ >>>> xen/arch/arm/include/asm/arm64/processor.h | 13 +++++++++++++ >>>> xen/arch/arm/include/asm/processor.h | 14 -------------- >>>> 3 files changed, 23 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/include/asm/arm32/processor.h >>>> b/xen/arch/arm/include/asm/arm32/processor.h >>>> index 4e679f3273..3e03ce78dc 100644 >>>> --- a/xen/arch/arm/include/asm/arm32/processor.h >>>> +++ b/xen/arch/arm/include/asm/arm32/processor.h >>>> @@ -56,6 +56,16 @@ struct cpu_user_regs >>>> uint32_t pad1; /* Doubleword-align the user half of the frame */ >>>> }; >>>> +/* >>>> + * Macros to extract affinity level. Picked from kernel >>>> + */ >>>> + >>>> +#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) >>>> + > > Above, since > #define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level) > you can replace (MPIDR_LEVEL_BITS * level) with > MPIDR_LEVEL_SHIFT(level) in the definition of MPIDR_AFFINITY_LEVEL. > You will see that it is identical to the arm64 definition > #define MPIDR_AFFINITY_LEVEL(mpidr, level) \ > ((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) Currently, MPIDR_AFFINITY_LEVEL(mpidr, 3) differs between arm32 and arm64:- In arm32 :- (mpidr >> 24) & 0xff In arm64 :- (mpidr >> 32) & 0xff I think this is what is expected. See xen/arch/arm/gic-v3.c , static inline uint64_t gicv3_mpidr_to_affinity(int cpu) { uint64_t mpidr = cpu_logical_map(cpu); return (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 | MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 | MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 | MPIDR_AFFINITY_LEVEL(mpidr, 0)); } > >>>> #endif >>>> #endif /* __ASM_ARM_ARM32_PROCESSOR_H */ >>>> diff --git a/xen/arch/arm/include/asm/arm64/processor.h >>>> b/xen/arch/arm/include/asm/arm64/processor.h >>>> index c749f80ad9..c026334eec 100644 >>>> --- a/xen/arch/arm/include/asm/arm64/processor.h >>>> +++ b/xen/arch/arm/include/asm/arm64/processor.h >>>> @@ -84,6 +84,19 @@ struct cpu_user_regs >>>> uint64_t sp_el1, elr_el1; >>>> }; >>>> +/* >>>> + * Macros to extract affinity level. picked from kernel >>>> + */ >>>> + >>>> +#define MPIDR_LEVEL_BITS_SHIFT 3 >>>> +#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) >>>> + >>>> +#define MPIDR_LEVEL_SHIFT(level) \ >>>> + (((1 << level) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >>>> + >>>> +#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ >>>> + ((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) >>>> + >>>> #undef __DECL_REG >>>> #endif /* __ASSEMBLY__ */ >>>> diff --git a/xen/arch/arm/include/asm/processor.h >>>> b/xen/arch/arm/include/asm/processor.h >>>> index 1dd81d7d52..7d90c3b5f2 100644 >>>> --- a/xen/arch/arm/include/asm/processor.h >>>> +++ b/xen/arch/arm/include/asm/processor.h >>>> @@ -118,20 +118,6 @@ >>>> #define MPIDR_INVALID (~MPIDR_HWID_MASK) >>>> #define MPIDR_LEVEL_BITS (8) >>>> - >>>> -/* >>>> - * Macros to extract affinity level. picked from kernel >>>> - */ >>>> - >>>> -#define MPIDR_LEVEL_BITS_SHIFT 3 >>>> -#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) >>>> - >>>> -#define MPIDR_LEVEL_SHIFT(level) \ >>>> - (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >>>> - >>>> -#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ >>>> - (((mpidr) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) >>>> - >>>> #define AFFINITY_MASK(level) ~((_AC(0x1,UL) << >>>> MPIDR_LEVEL_SHIFT(level)) - 1) >>>> /* TTBCR Translation Table Base Control Register */ >>> >>> Since only the definition of the MPIDR_AFFINITY_LEVEL() differs, >>> maybe you could add only this one to the arch specific headers e.g >>> for arm64: >>> #define MPIDR_LEVEL_SHIFT(level) \ >>> (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >>> for arm32: >>> #define MPIDR_LEVEL_SHIFT(level) \ >>> ((level) << MPIDR_LEVEL_BITS_SHIFT) >> >> Also, MPIDR_AFFINITY_LEVEL needs to be defined in arch specific >> headers as it differs between arm32 and arm64. > > As I point out above, there is no difference between arm32 and arm64 > regarding the definition of MPIDR_AFFINITY_LEVEL(level). Please see above and let me know if it makes sense. - Ayan > >> >> However, MPIDR_LEVEL_MASK can be defined in the common header (as it >> is same for arm32 and arm64). >> >> Please let me know if it makes sense. >> >>> >>> But in any case don't forget to add parentheses around the macro >>> parameters when an operator acts on them to avoid trouble with >>> operator precedence (MISRA-C Rule 20.7 :)) >> >> Thanks for pointing it out. Yes, this is a mistake in my patches. >> >> - Ayan >> >
On 10/24/22 16:01, Ayan Kumar Halder wrote: > > On 24/10/2022 12:35, Xenia Ragiadakou wrote: >> Hi Ayan, > Hi Xenia, >> >> On 10/24/22 14:00, Ayan Kumar Halder wrote: >>> >>> On 21/10/2022 22:18, Xenia Ragiadakou wrote: >>>> On 10/21/22 18:31, Ayan Kumar Halder wrote: >>>> Hi Ayan >>> Hi Xenia, >>>> >>>>> Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm64/ \ >>>>> include/asm/cputype.h#L14 , these macros are specific for arm64. >>>>> >>>>> When one computes MPIDR_LEVEL_SHIFT(3), it crosses the width of a 32 >>>>> bit register. >>>>> >>>>> Refer >>>>> https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm/include/ \ >>>>> asm/cputype.h#L54 , these macros are specific for arm32. >>>>> >>>>> Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> >>>>> --- >>>>> xen/arch/arm/include/asm/arm32/processor.h | 10 ++++++++++ >>>>> xen/arch/arm/include/asm/arm64/processor.h | 13 +++++++++++++ >>>>> xen/arch/arm/include/asm/processor.h | 14 -------------- >>>>> 3 files changed, 23 insertions(+), 14 deletions(-) >>>>> >>>>> diff --git a/xen/arch/arm/include/asm/arm32/processor.h >>>>> b/xen/arch/arm/include/asm/arm32/processor.h >>>>> index 4e679f3273..3e03ce78dc 100644 >>>>> --- a/xen/arch/arm/include/asm/arm32/processor.h >>>>> +++ b/xen/arch/arm/include/asm/arm32/processor.h >>>>> @@ -56,6 +56,16 @@ struct cpu_user_regs >>>>> uint32_t pad1; /* Doubleword-align the user half of the frame */ >>>>> }; >>>>> +/* >>>>> + * Macros to extract affinity level. Picked from kernel >>>>> + */ >>>>> + >>>>> +#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) >>>>> + >> >> Above, since >> #define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level) >> you can replace (MPIDR_LEVEL_BITS * level) with >> MPIDR_LEVEL_SHIFT(level) in the definition of MPIDR_AFFINITY_LEVEL. >> You will see that it is identical to the arm64 definition >> #define MPIDR_AFFINITY_LEVEL(mpidr, level) \ >> ((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) > > Currently, MPIDR_AFFINITY_LEVEL(mpidr, 3) differs between arm32 and arm64:- > > In arm32 :- (mpidr >> 24) & 0xff > > In arm64 :- (mpidr >> 32) & 0xff Correct. This is the case because the MPIDR_LEVEL_SHIFT(level) differs between arm32 and arm64. The definition of MPIDR_AFFINITY_LEVEL is common in both. More specifically, for level 3, #define MPIDR_LEVEL_SHIFT(level) \ ((level) << MPIDR_LEVEL_BITS_SHIFT) #define MPIDR_AFFINITY_LEVEL(mpidr, level) \ (((mpidr) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) gives (mpidr >> 24) & 0xff While #define MPIDR_LEVEL_SHIFT(level) \ (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) #define MPIDR_AFFINITY_LEVEL(mpidr, level) \ (((mpidr) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) gives (mpidr >> 32) & 0xff > > I think this is what is expected. See xen/arch/arm/gic-v3.c , > > static inline uint64_t gicv3_mpidr_to_affinity(int cpu) > { > uint64_t mpidr = cpu_logical_map(cpu); > return (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 | > MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 | > MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 | > MPIDR_AFFINITY_LEVEL(mpidr, 0)); > } > >> >>>>> #endif >>>>> #endif /* __ASM_ARM_ARM32_PROCESSOR_H */ >>>>> diff --git a/xen/arch/arm/include/asm/arm64/processor.h >>>>> b/xen/arch/arm/include/asm/arm64/processor.h >>>>> index c749f80ad9..c026334eec 100644 >>>>> --- a/xen/arch/arm/include/asm/arm64/processor.h >>>>> +++ b/xen/arch/arm/include/asm/arm64/processor.h >>>>> @@ -84,6 +84,19 @@ struct cpu_user_regs >>>>> uint64_t sp_el1, elr_el1; >>>>> }; >>>>> +/* >>>>> + * Macros to extract affinity level. picked from kernel >>>>> + */ >>>>> + >>>>> +#define MPIDR_LEVEL_BITS_SHIFT 3 >>>>> +#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) >>>>> + >>>>> +#define MPIDR_LEVEL_SHIFT(level) \ >>>>> + (((1 << level) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >>>>> + >>>>> +#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ >>>>> + ((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) >>>>> + >>>>> #undef __DECL_REG >>>>> #endif /* __ASSEMBLY__ */ >>>>> diff --git a/xen/arch/arm/include/asm/processor.h >>>>> b/xen/arch/arm/include/asm/processor.h >>>>> index 1dd81d7d52..7d90c3b5f2 100644 >>>>> --- a/xen/arch/arm/include/asm/processor.h >>>>> +++ b/xen/arch/arm/include/asm/processor.h >>>>> @@ -118,20 +118,6 @@ >>>>> #define MPIDR_INVALID (~MPIDR_HWID_MASK) >>>>> #define MPIDR_LEVEL_BITS (8) >>>>> - >>>>> -/* >>>>> - * Macros to extract affinity level. picked from kernel >>>>> - */ >>>>> - >>>>> -#define MPIDR_LEVEL_BITS_SHIFT 3 >>>>> -#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) >>>>> - >>>>> -#define MPIDR_LEVEL_SHIFT(level) \ >>>>> - (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >>>>> - >>>>> -#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ >>>>> - (((mpidr) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) >>>>> - >>>>> #define AFFINITY_MASK(level) ~((_AC(0x1,UL) << >>>>> MPIDR_LEVEL_SHIFT(level)) - 1) >>>>> /* TTBCR Translation Table Base Control Register */ >>>> >>>> Since only the definition of the MPIDR_AFFINITY_LEVEL() differs, >>>> maybe you could add only this one to the arch specific headers e.g >>>> for arm64: >>>> #define MPIDR_LEVEL_SHIFT(level) \ >>>> (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >>>> for arm32: >>>> #define MPIDR_LEVEL_SHIFT(level) \ >>>> ((level) << MPIDR_LEVEL_BITS_SHIFT) >>> >>> Also, MPIDR_AFFINITY_LEVEL needs to be defined in arch specific >>> headers as it differs between arm32 and arm64. >> >> As I point out above, there is no difference between arm32 and arm64 >> regarding the definition of MPIDR_AFFINITY_LEVEL(level). > > Please see above and let me know if it makes sense. > > - Ayan > >> >>> >>> However, MPIDR_LEVEL_MASK can be defined in the common header (as it >>> is same for arm32 and arm64). >>> >>> Please let me know if it makes sense. >>> >>>> >>>> But in any case don't forget to add parentheses around the macro >>>> parameters when an operator acts on them to avoid trouble with >>>> operator precedence (MISRA-C Rule 20.7 :)) >>> >>> Thanks for pointing it out. Yes, this is a mistake in my patches. >>> >>> - Ayan >>> >>
diff --git a/xen/arch/arm/include/asm/arm32/processor.h b/xen/arch/arm/include/asm/arm32/processor.h index 4e679f3273..3e03ce78dc 100644 --- a/xen/arch/arm/include/asm/arm32/processor.h +++ b/xen/arch/arm/include/asm/arm32/processor.h @@ -56,6 +56,16 @@ struct cpu_user_regs uint32_t pad1; /* Doubleword-align the user half of the frame */ }; +/* + * Macros to extract affinity level. Picked from kernel + */ + +#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) + #endif #endif /* __ASM_ARM_ARM32_PROCESSOR_H */ diff --git a/xen/arch/arm/include/asm/arm64/processor.h b/xen/arch/arm/include/asm/arm64/processor.h index c749f80ad9..c026334eec 100644 --- a/xen/arch/arm/include/asm/arm64/processor.h +++ b/xen/arch/arm/include/asm/arm64/processor.h @@ -84,6 +84,19 @@ struct cpu_user_regs uint64_t sp_el1, elr_el1; }; +/* + * Macros to extract affinity level. picked from kernel + */ + +#define MPIDR_LEVEL_BITS_SHIFT 3 +#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) + +#define MPIDR_LEVEL_SHIFT(level) \ + (((1 << level) >> 1) << MPIDR_LEVEL_BITS_SHIFT) + +#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ + ((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) + #undef __DECL_REG #endif /* __ASSEMBLY__ */ diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h index 1dd81d7d52..7d90c3b5f2 100644 --- a/xen/arch/arm/include/asm/processor.h +++ b/xen/arch/arm/include/asm/processor.h @@ -118,20 +118,6 @@ #define MPIDR_INVALID (~MPIDR_HWID_MASK) #define MPIDR_LEVEL_BITS (8) - -/* - * Macros to extract affinity level. picked from kernel - */ - -#define MPIDR_LEVEL_BITS_SHIFT 3 -#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) - -#define MPIDR_LEVEL_SHIFT(level) \ - (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) - -#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ - (((mpidr) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) - #define AFFINITY_MASK(level) ~((_AC(0x1,UL) << MPIDR_LEVEL_SHIFT(level)) - 1) /* TTBCR Translation Table Base Control Register */
Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm64/ \ include/asm/cputype.h#L14 , these macros are specific for arm64. When one computes MPIDR_LEVEL_SHIFT(3), it crosses the width of a 32 bit register. Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm/include/ \ asm/cputype.h#L54 , these macros are specific for arm32. Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> --- xen/arch/arm/include/asm/arm32/processor.h | 10 ++++++++++ xen/arch/arm/include/asm/arm64/processor.h | 13 +++++++++++++ xen/arch/arm/include/asm/processor.h | 14 -------------- 3 files changed, 23 insertions(+), 14 deletions(-)