| Message ID | 20221031151326.22634-3-ayankuma@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Arm: Enable GICv3 for AArch32 (v8R) | expand |
Hi Ayan, On 31/10/2022 16:13, Ayan Kumar Halder wrote: > > > Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm64/ \ You should not split the link as it is becoming unusable in that form. > include/asm/cputype.h#L14 , for the macros specific for arm64. > > Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm/include/ \ Same here. > asm/cputype.h#L54 , for the macros specific for arm32. > > MPIDR_LEVEL_SHIFT() differs between 64 and 32 bit. > For 64 bit :- > > aff_lev3 aff_lev2 aff_lev1 aff_lev0 > |________|________|________|________|________| > 40 32 24 16 8 0 > > For 32 bit :- > > aff_lev3 aff_lev2 aff_lev1 aff_lev0 > |________|________|________|________| > 32 24 16 8 0 > Where did you get this info from? FWICS by looking at ARM ARM DDI 0487I.a D17-6118, "Aff3 is not supported in AArch32 state." > Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> > --- > > Changes from :- > v1 - 1. Rearranged the macro defines so that the common code (between arm32 > and arm64) is placed in "arm/include/asm/processor.h". > > xen/arch/arm/include/asm/arm32/processor.h | 5 +++++ > xen/arch/arm/include/asm/arm64/processor.h | 8 ++++++++ > xen/arch/arm/include/asm/processor.h | 6 ------ > 3 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/include/asm/arm32/processor.h b/xen/arch/arm/include/asm/arm32/processor.h > index 4e679f3273..82aa7f8d9d 100644 > --- a/xen/arch/arm/include/asm/arm32/processor.h > +++ b/xen/arch/arm/include/asm/arm32/processor.h > @@ -56,6 +56,11 @@ struct cpu_user_regs > uint32_t pad1; /* Doubleword-align the user half of the frame */ > }; > > +/* > + * Macros to extract affinity level. Picked from kernel > + */ No need for a multiline comment here and everywhere else. > +#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * (level)) > + > #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..295483a9dd 100644 > --- a/xen/arch/arm/include/asm/arm64/processor.h > +++ b/xen/arch/arm/include/asm/arm64/processor.h > @@ -84,6 +84,14 @@ 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_SHIFT(level) \ > + (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) > + You should move these macros below __DECL_REG as they do not require having it defined. > #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..ecfb62bbbe 100644 > --- a/xen/arch/arm/include/asm/processor.h > +++ b/xen/arch/arm/include/asm/processor.h > @@ -122,13 +122,7 @@ > /* > * 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) > > -- > 2.17.1 > > ~Michal
On 31/10/2022 18:53, Michal Orzel wrote: > > > Hi Ayan, > > On 31/10/2022 16:13, Ayan Kumar Halder wrote: >> >> >> Refer https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv6.1-rc1%2Fsource%2Farch%2Farm64%2F&data=05%7C01%7Cmichal.orzel%40amd.com%7C0b2a0d1537104c2391d008dabb68eabb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638028356554609284%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=RhhL0XWxLJsO7vsP0DoP1QMvUMwGV%2F4FPJwAyvStj4k%3D&reserved=0 \ > You should not split the link as it is becoming unusable in that form. > >> include/asm/cputype.h#L14 , for the macros specific for arm64. >> >> Refer https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv6.1-rc1%2Fsource%2Farch%2Farm%2Finclude%2F&data=05%7C01%7Cmichal.orzel%40amd.com%7C0b2a0d1537104c2391d008dabb68eabb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638028356554609284%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=gLsNWm5%2BSyy51rn%2BA6H8PrWg8Yv%2BERicyyDjshOd3hc%3D&reserved=0 \ > Same here. > >> asm/cputype.h#L54 , for the macros specific for arm32. >> >> MPIDR_LEVEL_SHIFT() differs between 64 and 32 bit. >> For 64 bit :- >> >> aff_lev3 aff_lev2 aff_lev1 aff_lev0 >> |________|________|________|________|________| >> 40 32 24 16 8 0 >> >> For 32 bit :- >> >> aff_lev3 aff_lev2 aff_lev1 aff_lev0 >> |________|________|________|________| >> 32 24 16 8 0 >> > > Where did you get this info from? > FWICS by looking at ARM ARM DDI 0487I.a D17-6118, > "Aff3 is not supported in AArch32 state." We're talking about arm32 and not AArch32. My bad. Nevertheless, looking at ARM ARM DDI 0406C.d B4-1644, MPIDR for Armv7A/R also does not have aff3. > > >> Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> >> --- >> >> Changes from :- >> v1 - 1. Rearranged the macro defines so that the common code (between arm32 >> and arm64) is placed in "arm/include/asm/processor.h". >> >> xen/arch/arm/include/asm/arm32/processor.h | 5 +++++ >> xen/arch/arm/include/asm/arm64/processor.h | 8 ++++++++ >> xen/arch/arm/include/asm/processor.h | 6 ------ >> 3 files changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/arm/include/asm/arm32/processor.h b/xen/arch/arm/include/asm/arm32/processor.h >> index 4e679f3273..82aa7f8d9d 100644 >> --- a/xen/arch/arm/include/asm/arm32/processor.h >> +++ b/xen/arch/arm/include/asm/arm32/processor.h >> @@ -56,6 +56,11 @@ struct cpu_user_regs >> uint32_t pad1; /* Doubleword-align the user half of the frame */ >> }; >> >> +/* >> + * Macros to extract affinity level. Picked from kernel >> + */ > No need for a multiline comment here and everywhere else. > >> +#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * (level)) >> + >> #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..295483a9dd 100644 >> --- a/xen/arch/arm/include/asm/arm64/processor.h >> +++ b/xen/arch/arm/include/asm/arm64/processor.h >> @@ -84,6 +84,14 @@ 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_SHIFT(level) \ >> + (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >> + > You should move these macros below __DECL_REG as they do not require having it defined. > >> #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..ecfb62bbbe 100644 >> --- a/xen/arch/arm/include/asm/processor.h >> +++ b/xen/arch/arm/include/asm/processor.h >> @@ -122,13 +122,7 @@ >> /* >> * 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) >> >> -- >> 2.17.1 >> >> > > ~Michal >
On 01/11/2022 08:58, Michal Orzel wrote: > > On 31/10/2022 18:53, Michal Orzel wrote: >> >> Hi Ayan, Hi Michal, >> >> On 31/10/2022 16:13, Ayan Kumar Halder wrote: >>> >>> Refer https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv6.1-rc1%2Fsource%2Farch%2Farm64%2F&data=05%7C01%7Cmichal.orzel%40amd.com%7C0b2a0d1537104c2391d008dabb68eabb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638028356554609284%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=RhhL0XWxLJsO7vsP0DoP1QMvUMwGV%2F4FPJwAyvStj4k%3D&reserved=0 \ >> You should not split the link as it is becoming unusable in that form. >> >>> include/asm/cputype.h#L14 , for the macros specific for arm64. >>> >>> Refer https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv6.1-rc1%2Fsource%2Farch%2Farm%2Finclude%2F&data=05%7C01%7Cmichal.orzel%40amd.com%7C0b2a0d1537104c2391d008dabb68eabb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638028356554609284%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=gLsNWm5%2BSyy51rn%2BA6H8PrWg8Yv%2BERicyyDjshOd3hc%3D&reserved=0 \ >> Same here. >> >>> asm/cputype.h#L54 , for the macros specific for arm32. >>> >>> MPIDR_LEVEL_SHIFT() differs between 64 and 32 bit. >>> For 64 bit :- >>> >>> aff_lev3 aff_lev2 aff_lev1 aff_lev0 >>> |________|________|________|________|________| >>> 40 32 24 16 8 0 >>> >>> For 32 bit :- >>> >>> aff_lev3 aff_lev2 aff_lev1 aff_lev0 >>> |________|________|________|________| >>> 32 24 16 8 0 >>> >> Where did you get this info from? >> FWICS by looking at ARM ARM DDI 0487I.a D17-6118, >> "Aff3 is not supported in AArch32 state." > We're talking about arm32 and not AArch32. My bad. > Nevertheless, looking at ARM ARM DDI 0406C.d B4-1644, > MPIDR for Armv7A/R also does not have aff3. I was illustrating how the bits are represented in software Refer https://elixir.bootlin.com/linux/latest/source/drivers/irqchip/irq-gic-v3.c#L625 I was trying to depict how "u64 <https://elixir.bootlin.com/linux/latest/C/ident/u64> aff <https://elixir.bootlin.com/linux/latest/C/ident/aff>" bit representation differs between arm32 and arm64. - Ayan > >> >>> Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> >>> --- >>> >>> Changes from :- >>> v1 - 1. Rearranged the macro defines so that the common code (between arm32 >>> and arm64) is placed in "arm/include/asm/processor.h". >>> >>> xen/arch/arm/include/asm/arm32/processor.h | 5 +++++ >>> xen/arch/arm/include/asm/arm64/processor.h | 8 ++++++++ >>> xen/arch/arm/include/asm/processor.h | 6 ------ >>> 3 files changed, 13 insertions(+), 6 deletions(-) >>> >>> diff --git a/xen/arch/arm/include/asm/arm32/processor.h b/xen/arch/arm/include/asm/arm32/processor.h >>> index 4e679f3273..82aa7f8d9d 100644 >>> --- a/xen/arch/arm/include/asm/arm32/processor.h >>> +++ b/xen/arch/arm/include/asm/arm32/processor.h >>> @@ -56,6 +56,11 @@ struct cpu_user_regs >>> uint32_t pad1; /* Doubleword-align the user half of the frame */ >>> }; >>> >>> +/* >>> + * Macros to extract affinity level. Picked from kernel >>> + */ >> No need for a multiline comment here and everywhere else. >> >>> +#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * (level)) >>> + >>> #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..295483a9dd 100644 >>> --- a/xen/arch/arm/include/asm/arm64/processor.h >>> +++ b/xen/arch/arm/include/asm/arm64/processor.h >>> @@ -84,6 +84,14 @@ 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_SHIFT(level) \ >>> + (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >>> + >> You should move these macros below __DECL_REG as they do not require having it defined. >> >>> #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..ecfb62bbbe 100644 >>> --- a/xen/arch/arm/include/asm/processor.h >>> +++ b/xen/arch/arm/include/asm/processor.h >>> @@ -122,13 +122,7 @@ >>> /* >>> * 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) >>> >>> -- >>> 2.17.1 >>> >>> >> ~Michal >>
Hi, Title: The macros you are moving are not GICv3 specific. On 31/10/2022 15:13, Ayan Kumar Halder wrote: > Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm64/ \ > include/asm/cputype.h#L14 , for the macros specific for arm64. > > Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm/include/ \ > asm/cputype.h#L54 , for the macros specific for arm32. > > MPIDR_LEVEL_SHIFT() differs between 64 and 32 bit. > For 64 bit :- > > aff_lev3 aff_lev2 aff_lev1 aff_lev0 > |________|________|________|________|________| > 40 32 24 16 8 0 > > For 32 bit :- > > aff_lev3 aff_lev2 aff_lev1 aff_lev0 > |________|________|________|________| > 32 24 16 8 0 As discussed with Michal, AFF3 doesn't exist for 32-bit. So it is not clear to me what we are gaining by moving the macros. Cheers,
On 11/2/22 10:46, Julien Grall wrote: > Hi, > > Title: The macros you are moving are not GICv3 specific. > > On 31/10/2022 15:13, Ayan Kumar Halder wrote: >> Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm64/ \ >> include/asm/cputype.h#L14 , for the macros specific for arm64. >> >> Refer >> https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm/include/ \ >> asm/cputype.h#L54 , for the macros specific for arm32. >> >> MPIDR_LEVEL_SHIFT() differs between 64 and 32 bit. > For 64 bit :- >> >> aff_lev3 aff_lev2 aff_lev1 aff_lev0 >> |________|________|________|________|________| >> 40 32 24 16 8 0 >> >> For 32 bit :- >> >> aff_lev3 aff_lev2 aff_lev1 aff_lev0 >> |________|________|________|________| >> 32 24 16 8 0 > > As discussed with Michal, AFF3 doesn't exist for 32-bit. So it is not > clear to me what we are gaining by moving the macros. > I cannot understand what do you mean by "what we are gaining by moving the macros". IIUC, when identifying the cpu topology, a mask is applied to the value of MPIDR_EL1 #ifdef CONFIG_ARM_64 #define MPIDR_HWID_MASK _AC(0xff00ffffff,UL) #else #define MPIDR_HWID_MASK _AC(0xffffff,U) #endif So, for arm32, the affinity at level 3 is considered to be 0. Do you mean, what we are gaining by defining the MPIDR_LEVEL_SHIFT in a different way for arm32 and for arm64? IMO, we need to do so, because the shift, used to retrieve the affinity at each level, cannot be calculated using the same logic i.e (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) For arm32 the affinity at each level is calculated as follows ((level) << MPIDR_LEVEL_BITS_SHIFT) Also, IMO, since MPIDR_HWID_MASK is defined in the common header, maybe the same needs to be done for MPIDR_LEVEL_SHIFT, to make easier for someone reading the code to understand the difference. I hope I'm not out of context. > Cheers, >
Hi Xenia, On 02/11/2022 09:57, Xenia Ragiadakou wrote: > > On 11/2/22 10:46, Julien Grall wrote: >> Hi, >> >> Title: The macros you are moving are not GICv3 specific. >> >> On 31/10/2022 15:13, Ayan Kumar Halder wrote: >>> Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm64/ \ >>> include/asm/cputype.h#L14 , for the macros specific for arm64. >>> >>> Refer >>> https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm/include/ \ >>> asm/cputype.h#L54 , for the macros specific for arm32. >>> >>> MPIDR_LEVEL_SHIFT() differs between 64 and 32 bit. > For 64 bit :- >>> >>> aff_lev3 aff_lev2 aff_lev1 aff_lev0 >>> |________|________|________|________|________| >>> 40 32 24 16 8 0 >>> >>> For 32 bit :- >>> >>> aff_lev3 aff_lev2 aff_lev1 aff_lev0 >>> |________|________|________|________| >>> 32 24 16 8 0 >> >> As discussed with Michal, AFF3 doesn't exist for 32-bit. So it is not >> clear to me what we are gaining by moving the macros. >> > > I cannot understand what do you mean by "what we are gaining by moving > the macros". > > IIUC, when identifying the cpu topology, a mask is applied to the value > of MPIDR_EL1 > #ifdef CONFIG_ARM_64 > #define MPIDR_HWID_MASK _AC(0xff00ffffff,UL) > #else > #define MPIDR_HWID_MASK _AC(0xffffff,U) > #endif > So, for arm32, the affinity at level 3 is considered to be 0. > > Do you mean, what we are gaining by defining the MPIDR_LEVEL_SHIFT in a > different way for arm32 and for arm64? Yes. There are nothing justifying the move so far. > > IMO, we need to do so, because the shift, used to retrieve the affinity > at each level, cannot be calculated using the same logic i.e > (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) > > For arm32 the affinity at each level is calculated as follows > ((level) << MPIDR_LEVEL_BITS_SHIFT) I understand they are written differently. But if you look at the layout, AFF0, AFF1, AFF2 are in the same position. AFF3 doesn't exist for arm32 and, AFAICT, the shift will not matter because the bits 40:32 will be zeroed in any case. So I don't see the problem of using the arm64 version. Cheers,
Hi Julien, On 11/2/22 12:10, Julien Grall wrote: > Hi Xenia, > > On 02/11/2022 09:57, Xenia Ragiadakou wrote: >> >> On 11/2/22 10:46, Julien Grall wrote: >>> Hi, >>> >>> Title: The macros you are moving are not GICv3 specific. >>> >>> On 31/10/2022 15:13, Ayan Kumar Halder wrote: >>>> Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm64/ \ >>>> include/asm/cputype.h#L14 , for the macros specific for arm64. >>>> >>>> Refer >>>> https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm/include/ \ >>>> asm/cputype.h#L54 , for the macros specific for arm32. >>>> >>>> MPIDR_LEVEL_SHIFT() differs between 64 and 32 bit. > For 64 bit :- >>>> >>>> aff_lev3 aff_lev2 aff_lev1 aff_lev0 >>>> |________|________|________|________|________| >>>> 40 32 24 16 8 0 >>>> >>>> For 32 bit :- >>>> >>>> aff_lev3 aff_lev2 aff_lev1 aff_lev0 >>>> |________|________|________|________| >>>> 32 24 16 8 0 >>> >>> As discussed with Michal, AFF3 doesn't exist for 32-bit. So it is not >>> clear to me what we are gaining by moving the macros. >>> >> >> I cannot understand what do you mean by "what we are gaining by moving >> the macros". > > >> IIUC, when identifying the cpu topology, a mask is applied to the >> value of MPIDR_EL1 >> #ifdef CONFIG_ARM_64 >> #define MPIDR_HWID_MASK _AC(0xff00ffffff,UL) >> #else >> #define MPIDR_HWID_MASK _AC(0xffffff,U) >> #endif >> So, for arm32, the affinity at level 3 is considered to be 0. >> >> Do you mean, what we are gaining by defining the MPIDR_LEVEL_SHIFT in >> a different way for arm32 and for arm64? > > Yes. There are nothing justifying the move so far. > >> >> IMO, we need to do so, because the shift, used to retrieve the >> affinity at each level, cannot be calculated using the same logic i.e >> (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >> >> For arm32 the affinity at each level is calculated as follows >> ((level) << MPIDR_LEVEL_BITS_SHIFT) > > I understand they are written differently. But if you look at the > layout, AFF0, AFF1, AFF2 are in the same position. AFF3 doesn't exist > for arm32 and, AFAICT, the shift will not matter because the bits 40:32 > will be zeroed in any case. > > So I don't see the problem of using the arm64 version. Now I see :) ... IIUC you are proposing to just cast the mpidr in MPIDR_AFFINITY_LEVEL(mpidr, level) to uint64_t? In this case, I think, UL, below, needs to also to become ULL #define AFFINITY_MASK(level) ~((_AC(0x1,UL) << MPIDR_LEVEL_SHIFT(level)) - 1) > > Cheers, >
Hi Xenia, On 02/11/2022 10:36, Xenia Ragiadakou wrote: > Hi Julien, > > On 11/2/22 12:10, Julien Grall wrote: >> Hi Xenia, >> >> On 02/11/2022 09:57, Xenia Ragiadakou wrote: >>> >>> On 11/2/22 10:46, Julien Grall wrote: >>>> Hi, >>>> >>>> Title: The macros you are moving are not GICv3 specific. >>>> >>>> On 31/10/2022 15:13, Ayan Kumar Halder wrote: >>>>> Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm64/ \ >>>>> include/asm/cputype.h#L14 , for the macros specific for arm64. >>>>> >>>>> Refer >>>>> https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm/include/ \ >>>>> asm/cputype.h#L54 , for the macros specific for arm32. >>>>> >>>>> MPIDR_LEVEL_SHIFT() differs between 64 and 32 bit. > For 64 bit :- >>>>> >>>>> aff_lev3 aff_lev2 aff_lev1 aff_lev0 >>>>> |________|________|________|________|________| >>>>> 40 32 24 16 8 0 >>>>> >>>>> For 32 bit :- >>>>> >>>>> aff_lev3 aff_lev2 aff_lev1 aff_lev0 >>>>> |________|________|________|________| >>>>> 32 24 16 8 0 >>>> >>>> As discussed with Michal, AFF3 doesn't exist for 32-bit. So it is >>>> not clear to me what we are gaining by moving the macros. >>>> >>> >>> I cannot understand what do you mean by "what we are gaining by >>> moving the macros". >> > >>> IIUC, when identifying the cpu topology, a mask is applied to the >>> value of MPIDR_EL1 >>> #ifdef CONFIG_ARM_64 >>> #define MPIDR_HWID_MASK _AC(0xff00ffffff,UL) >>> #else >>> #define MPIDR_HWID_MASK _AC(0xffffff,U) >>> #endif >>> So, for arm32, the affinity at level 3 is considered to be 0. >>> >>> Do you mean, what we are gaining by defining the MPIDR_LEVEL_SHIFT in >>> a different way for arm32 and for arm64? >> >> Yes. There are nothing justifying the move so far. >> >>> >>> IMO, we need to do so, because the shift, used to retrieve the >>> affinity at each level, cannot be calculated using the same logic i.e >>> (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) >>> >>> For arm32 the affinity at each level is calculated as follows >>> ((level) << MPIDR_LEVEL_BITS_SHIFT) >> >> I understand they are written differently. But if you look at the >> layout, AFF0, AFF1, AFF2 are in the same position. AFF3 doesn't exist >> for arm32 and, AFAICT, the shift will not matter because the bits >> 40:32 will be zeroed in any case. >> >> So I don't see the problem of using the arm64 version. > > Now I see :) ... IIUC you are proposing to just cast the mpidr in > MPIDR_AFFINITY_LEVEL(mpidr, level) to uint64_t? Not necessarilly. The other approach is to make sure that no arm32 code is calling AFFINITY_MASK() with a level >= 3 (see vpsci.c for instance). Cheers,
diff --git a/xen/arch/arm/include/asm/arm32/processor.h b/xen/arch/arm/include/asm/arm32/processor.h index 4e679f3273..82aa7f8d9d 100644 --- a/xen/arch/arm/include/asm/arm32/processor.h +++ b/xen/arch/arm/include/asm/arm32/processor.h @@ -56,6 +56,11 @@ 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_SHIFT(level) (MPIDR_LEVEL_BITS * (level)) + #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..295483a9dd 100644 --- a/xen/arch/arm/include/asm/arm64/processor.h +++ b/xen/arch/arm/include/asm/arm64/processor.h @@ -84,6 +84,14 @@ 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_SHIFT(level) \ + (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT) + #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..ecfb62bbbe 100644 --- a/xen/arch/arm/include/asm/processor.h +++ b/xen/arch/arm/include/asm/processor.h @@ -122,13 +122,7 @@ /* * 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)
Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm64/ \ include/asm/cputype.h#L14 , for the macros specific for arm64. Refer https://elixir.bootlin.com/linux/v6.1-rc1/source/arch/arm/include/ \ asm/cputype.h#L54 , for the macros specific for arm32. MPIDR_LEVEL_SHIFT() differs between 64 and 32 bit. For 64 bit :- aff_lev3 aff_lev2 aff_lev1 aff_lev0 |________|________|________|________|________| 40 32 24 16 8 0 For 32 bit :- aff_lev3 aff_lev2 aff_lev1 aff_lev0 |________|________|________|________| 32 24 16 8 0 Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> --- Changes from :- v1 - 1. Rearranged the macro defines so that the common code (between arm32 and arm64) is placed in "arm/include/asm/processor.h". xen/arch/arm/include/asm/arm32/processor.h | 5 +++++ xen/arch/arm/include/asm/arm64/processor.h | 8 ++++++++ xen/arch/arm/include/asm/processor.h | 6 ------ 3 files changed, 13 insertions(+), 6 deletions(-)