Re: [RFC PATCH v1 02/12] Arm: GICv3: Move the macros to compute the affnity level to arm64/arm32

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

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@xxxxxxx>
> > > > > ---
> > > > >   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
--
Xenia