Re: [XEN v2 02/12] xen/Arm: GICv3: Move the macros to compute the affnity level to arm64/arm32

[Julien Grall <julien@xxxxxxx>]

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,

--
Julien Grall