[PATCH] arch: drop get_processor_id()

Jan Beulich posted 1 patch 6 months ago
Failed in applying to current master (apply log)
[PATCH] arch: drop get_processor_id()
Posted by Jan Beulich 6 months ago
smp_processor_id() is what we commonly use; there's no need for it to
have an alias.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/include/asm/current.h
+++ b/xen/arch/arm/include/asm/current.h
@@ -53,7 +53,7 @@ static inline struct cpu_info *get_cpu_i
 
 DECLARE_PER_CPU(unsigned int, cpu_id);
 
-#define get_processor_id()     this_cpu(cpu_id)
+#define smp_processor_id()     this_cpu(cpu_id)
 #define set_processor_id(id)                            \
 do {                                                    \
     WRITE_SYSREG(__per_cpu_offset[(id)], TPIDR_EL2);    \
--- a/xen/arch/arm/include/asm/smp.h
+++ b/xen/arch/arm/include/asm/smp.h
@@ -11,8 +11,6 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_
 
 #define cpu_is_offline(cpu) unlikely(!cpu_online(cpu))
 
-#define smp_processor_id() get_processor_id()
-
 /*
  * Do we, for platform reasons, need to actually keep CPUs online when we
  * would otherwise prefer them to be off?
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -401,7 +401,7 @@ void start_secondary(void)
 /* Shut down the current CPU */
 void __cpu_disable(void)
 {
-    unsigned int cpu = get_processor_id();
+    unsigned int cpu = smp_processor_id();
 
     local_irq_disable();
     gic_disable_cpu();
--- a/xen/arch/x86/include/asm/current.h
+++ b/xen/arch/x86/include/asm/current.h
@@ -99,7 +99,7 @@ static inline struct cpu_info *get_cpu_i
 #define set_current(vcpu)     (get_cpu_info()->current_vcpu = (vcpu))
 #define current               (get_current())
 
-#define get_processor_id()    (get_cpu_info()->processor_id)
+#define smp_processor_id()    (get_cpu_info()->processor_id)
 #define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs)
 
 /*
--- a/xen/arch/x86/include/asm/smp.h
+++ b/xen/arch/x86/include/asm/smp.h
@@ -48,13 +48,6 @@ extern void cpu_exit_clear(unsigned int
 extern void cpu_uninit(unsigned int cpu);
 int cpu_add(uint32_t apic_id, uint32_t acpi_id, uint32_t pxm);
 
-/*
- * This function is needed by all SMP systems. It must _always_ be valid
- * from the initial startup. We map APIC_BASE very early in page_setup(),
- * so this is correct in the x86 case.
- */
-#define smp_processor_id() get_processor_id()
-
 void __stop_this_cpu(void);
 
 long cf_check cpu_up_helper(void *data);
--- a/xen/arch/x86/mm/mm-locks.h
+++ b/xen/arch/x86/mm/mm-locks.h
@@ -122,7 +122,7 @@ static inline void mm_rwlock_init(mm_rwl
 
 static inline int mm_write_locked_by_me(mm_rwlock_t *l)
 {
-    return (l->locker == get_processor_id());
+    return (l->locker == smp_processor_id());
 }
 
 static inline void _mm_write_lock(const struct domain *d, mm_rwlock_t *l,
@@ -132,7 +132,7 @@ static inline void _mm_write_lock(const
     {
         _check_lock_level(d, level);
         percpu_write_lock(p2m_percpu_rwlock, &l->lock);
-        l->locker = get_processor_id();
+        l->locker = smp_processor_id();
         l->locker_function = func;
         l->unlock_level = _get_lock_level();
         _set_lock_level(_lock_level(d, level));
Re: [PATCH] arch: drop get_processor_id()
Posted by Roger Pau Monné 6 months ago
On Mon, Oct 30, 2023 at 02:29:09PM +0100, Jan Beulich wrote:
> smp_processor_id() is what we commonly use; there's no need for it to
> have an alias.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH] arch: drop get_processor_id()
Posted by Bertrand Marquis 6 months ago
Hi Jan,

> On 30 Oct 2023, at 14:29, Jan Beulich <jbeulich@suse.com> wrote:
> 
> smp_processor_id() is what we commonly use; there's no need for it to
> have an alias.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

For the arm part:

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> 
> --- a/xen/arch/arm/include/asm/current.h
> +++ b/xen/arch/arm/include/asm/current.h
> @@ -53,7 +53,7 @@ static inline struct cpu_info *get_cpu_i
> 
> DECLARE_PER_CPU(unsigned int, cpu_id);
> 
> -#define get_processor_id()     this_cpu(cpu_id)
> +#define smp_processor_id()     this_cpu(cpu_id)
> #define set_processor_id(id)                            \
> do {                                                    \
>     WRITE_SYSREG(__per_cpu_offset[(id)], TPIDR_EL2);    \
> --- a/xen/arch/arm/include/asm/smp.h
> +++ b/xen/arch/arm/include/asm/smp.h
> @@ -11,8 +11,6 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_
> 
> #define cpu_is_offline(cpu) unlikely(!cpu_online(cpu))
> 
> -#define smp_processor_id() get_processor_id()
> -
> /*
>  * Do we, for platform reasons, need to actually keep CPUs online when we
>  * would otherwise prefer them to be off?
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -401,7 +401,7 @@ void start_secondary(void)
> /* Shut down the current CPU */
> void __cpu_disable(void)
> {
> -    unsigned int cpu = get_processor_id();
> +    unsigned int cpu = smp_processor_id();
> 
>     local_irq_disable();
>     gic_disable_cpu();
> --- a/xen/arch/x86/include/asm/current.h
> +++ b/xen/arch/x86/include/asm/current.h
> @@ -99,7 +99,7 @@ static inline struct cpu_info *get_cpu_i
> #define set_current(vcpu)     (get_cpu_info()->current_vcpu = (vcpu))
> #define current               (get_current())
> 
> -#define get_processor_id()    (get_cpu_info()->processor_id)
> +#define smp_processor_id()    (get_cpu_info()->processor_id)
> #define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs)
> 
> /*
> --- a/xen/arch/x86/include/asm/smp.h
> +++ b/xen/arch/x86/include/asm/smp.h
> @@ -48,13 +48,6 @@ extern void cpu_exit_clear(unsigned int
> extern void cpu_uninit(unsigned int cpu);
> int cpu_add(uint32_t apic_id, uint32_t acpi_id, uint32_t pxm);
> 
> -/*
> - * This function is needed by all SMP systems. It must _always_ be valid
> - * from the initial startup. We map APIC_BASE very early in page_setup(),
> - * so this is correct in the x86 case.
> - */
> -#define smp_processor_id() get_processor_id()
> -
> void __stop_this_cpu(void);
> 
> long cf_check cpu_up_helper(void *data);
> --- a/xen/arch/x86/mm/mm-locks.h
> +++ b/xen/arch/x86/mm/mm-locks.h
> @@ -122,7 +122,7 @@ static inline void mm_rwlock_init(mm_rwl
> 
> static inline int mm_write_locked_by_me(mm_rwlock_t *l)
> {
> -    return (l->locker == get_processor_id());
> +    return (l->locker == smp_processor_id());
> }
> 
> static inline void _mm_write_lock(const struct domain *d, mm_rwlock_t *l,
> @@ -132,7 +132,7 @@ static inline void _mm_write_lock(const
>     {
>         _check_lock_level(d, level);
>         percpu_write_lock(p2m_percpu_rwlock, &l->lock);
> -        l->locker = get_processor_id();
> +        l->locker = smp_processor_id();
>         l->locker_function = func;
>         l->unlock_level = _get_lock_level();
>         _set_lock_level(_lock_level(d, level));
Re: [PATCH] arch: drop get_processor_id()
Posted by Oleksii 6 months ago
Hi Jan,

On Mon, 2023-10-30 at 14:29 +0100, Jan Beulich wrote:
> smp_processor_id() is what we commonly use; there's no need for it to
> have an alias.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/arm/include/asm/current.h
> +++ b/xen/arch/arm/include/asm/current.h
> @@ -53,7 +53,7 @@ static inline struct cpu_info *get_cpu_i
>  
>  DECLARE_PER_CPU(unsigned int, cpu_id);
>  
> -#define get_processor_id()     this_cpu(cpu_id)
> +#define smp_processor_id()     this_cpu(cpu_id)
>  #define set_processor_id(id)                            \
>  do {                                                    \
>      WRITE_SYSREG(__per_cpu_offset[(id)], TPIDR_EL2);    \
> --- a/xen/arch/arm/include/asm/smp.h
> +++ b/xen/arch/arm/include/asm/smp.h
> @@ -11,8 +11,6 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_
>  
>  #define cpu_is_offline(cpu) unlikely(!cpu_online(cpu))
>  
> -#define smp_processor_id() get_processor_id()
> -
>  /*
>   * Do we, for platform reasons, need to actually keep CPUs online
> when we
>   * would otherwise prefer them to be off?
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -401,7 +401,7 @@ void start_secondary(void)
>  /* Shut down the current CPU */
>  void __cpu_disable(void)
>  {
> -    unsigned int cpu = get_processor_id();
> +    unsigned int cpu = smp_processor_id();
>  
>      local_irq_disable();
>      gic_disable_cpu();
> --- a/xen/arch/x86/include/asm/current.h
> +++ b/xen/arch/x86/include/asm/current.h
> @@ -99,7 +99,7 @@ static inline struct cpu_info *get_cpu_i
>  #define set_current(vcpu)     (get_cpu_info()->current_vcpu =
> (vcpu))
>  #define current               (get_current())
>  
> -#define get_processor_id()    (get_cpu_info()->processor_id)
> +#define smp_processor_id()    (get_cpu_info()->processor_id)
>  #define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs)
>  
>  /*
> --- a/xen/arch/x86/include/asm/smp.h
> +++ b/xen/arch/x86/include/asm/smp.h
> @@ -48,13 +48,6 @@ extern void cpu_exit_clear(unsigned int
>  extern void cpu_uninit(unsigned int cpu);
>  int cpu_add(uint32_t apic_id, uint32_t acpi_id, uint32_t pxm);
>  
> -/*
> - * This function is needed by all SMP systems. It must _always_ be
> valid
> - * from the initial startup. We map APIC_BASE very early in
> page_setup(),
> - * so this is correct in the x86 case.
> - */
> -#define smp_processor_id() get_processor_id()
> -
>  void __stop_this_cpu(void);
>  
>  long cf_check cpu_up_helper(void *data);
> --- a/xen/arch/x86/mm/mm-locks.h
> +++ b/xen/arch/x86/mm/mm-locks.h
> @@ -122,7 +122,7 @@ static inline void mm_rwlock_init(mm_rwl
>  
>  static inline int mm_write_locked_by_me(mm_rwlock_t *l)
>  {
> -    return (l->locker == get_processor_id());
> +    return (l->locker == smp_processor_id());
>  }
>  
>  static inline void _mm_write_lock(const struct domain *d,
> mm_rwlock_t *l,
> @@ -132,7 +132,7 @@ static inline void _mm_write_lock(const
>      {
>          _check_lock_level(d, level);
>          percpu_write_lock(p2m_percpu_rwlock, &l->lock);
> -        l->locker = get_processor_id();
> +        l->locker = smp_processor_id();
>          l->locker_function = func;
>          l->unlock_level = _get_lock_level();
>          _set_lock_level(_lock_level(d, level));

Looks good to me.

Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~ Oleksii