[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.