Re: [PATCH v3 06/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]()

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 20/11/2023 11:38, Juergen Gross wrote:
> Instead of special casing rspin_lock_irqsave() and
> rspin_unlock_irqrestore() for the console lock, add those functions
> to spinlock handling and use them where needed.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> V2:
> - new patch
> ---
>   xen/arch/x86/traps.c       | 14 ++++++++------
>   xen/common/spinlock.c      | 16 ++++++++++++++++
>   xen/drivers/char/console.c | 18 +-----------------
>   xen/include/xen/console.h  |  5 +++--
>   xen/include/xen/spinlock.h |  7 +++++++
>   5 files changed, 35 insertions(+), 25 deletions(-)
>
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index e1356f696a..f72769e79b 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -647,13 +647,15 @@ void show_stack_overflow(unsigned int cpu, const struct 
> cpu_user_regs *regs)
>   void show_execution_state(const struct cpu_user_regs *regs)
>   {
>       /* Prevent interleaving of output. */
> -    unsigned long flags = console_lock_recursive_irqsave();
> +    unsigned long flags;
> +
> +    rspin_lock_irqsave(&console_lock, flags);
This feels a bit weird because rspin_lock_irqsave() being lowercase
hints that it's a either a function or behaves like one. For that it
should take &flags instead.

>   
>       show_registers(regs);
>       show_code(regs);
>       show_stack(regs);
>   
> -    console_unlock_recursive_irqrestore(flags);
> +    rspin_unlock_irqrestore(&console_lock, flags);
>   }
>   
>   void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs)
> @@ -663,7 +665,7 @@ void cf_check show_execution_state_nonconst(struct 
> cpu_user_regs *regs)
>   
>   void vcpu_show_execution_state(struct vcpu *v)
>   {
> -    unsigned long flags = 0;
> +    unsigned long flags;
>   
>       if ( test_bit(_VPF_down, &v->pause_flags) )
>       {
> @@ -698,7 +700,7 @@ void vcpu_show_execution_state(struct vcpu *v)
>   #endif
>   
>       /* Prevent interleaving of output. */
> -    flags = console_lock_recursive_irqsave();
> +    rspin_lock_irqsave(&console_lock, flags);
>   
>       vcpu_show_registers(v);
>   
> @@ -708,7 +710,7 @@ void vcpu_show_execution_state(struct vcpu *v)
>            * Stop interleaving prevention: The necessary P2M lookups involve
>            * locking, which has to occur with IRQs enabled.
>            */
> -        console_unlock_recursive_irqrestore(flags);
> +        rspin_unlock_irqrestore(&console_lock, flags);
>   
>           show_hvm_stack(v, &v->arch.user_regs);
>       }
> @@ -717,7 +719,7 @@ void vcpu_show_execution_state(struct vcpu *v)
>           if ( guest_kernel_mode(v, &v->arch.user_regs) )
>               show_guest_stack(v, &v->arch.user_regs);
>   
> -        console_unlock_recursive_irqrestore(flags);
> +        rspin_unlock_irqrestore(&console_lock, flags);
>       }
>   
>   #ifdef CONFIG_HVM
> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
> index 26c667d3cc..17716fc4eb 100644
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -475,6 +475,16 @@ void rspin_lock(rspinlock_t *lock)
>       lock->recurse_cnt++;
>   }
>   
> +unsigned long __rspin_lock_irqsave(rspinlock_t *lock)
> +{
> +    unsigned long flags;
> +
> +    local_irq_save(flags);
> +    rspin_lock(lock);
> +
> +    return flags;
> +}
> +
>   void rspin_unlock(rspinlock_t *lock)
>   {
>       if ( likely(--lock->recurse_cnt == 0) )
> @@ -484,6 +494,12 @@ void rspin_unlock(rspinlock_t *lock)
>       }
>   }
>   
> +void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags)
> +{
> +    rspin_unlock(lock);
> +    local_irq_restore(flags);
> +}
> +
>   #ifdef CONFIG_DEBUG_LOCK_PROFILE
>   
>   struct lock_profile_anc {
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 369b2f9077..cc743b67ec 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -120,7 +120,7 @@ static int __read_mostly sercon_handle = -1;
>   int8_t __read_mostly opt_console_xen; /* console=xen */
>   #endif
>   
> -static DEFINE_RSPINLOCK(console_lock);
> +DEFINE_RSPINLOCK(console_lock);
>   
>   /*
>    * To control the amount of printing, thresholds are added.
> @@ -1158,22 +1158,6 @@ void console_end_log_everything(void)
>       atomic_dec(&print_everything);
>   }
>   
> -unsigned long console_lock_recursive_irqsave(void)
> -{
> -    unsigned long flags;
> -
> -    local_irq_save(flags);
> -    rspin_lock(&console_lock);
> -
> -    return flags;
> -}
> -
> -void console_unlock_recursive_irqrestore(unsigned long flags)
> -{
> -    rspin_unlock(&console_lock);
> -    local_irq_restore(flags);
> -}
> -
>   void console_force_unlock(void)
>   {
>       watchdog_disable();
> diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
> index ab5c30c0da..dff0096b27 100644
> --- a/xen/include/xen/console.h
> +++ b/xen/include/xen/console.h
> @@ -8,8 +8,11 @@
>   #define __CONSOLE_H__
>   
>   #include <xen/inttypes.h>
> +#include <xen/spinlock.h>
>   #include <public/xen.h>
>   
> +extern rspinlock_t console_lock;
> +
>   struct xen_sysctl_readconsole;
>   long read_console_ring(struct xen_sysctl_readconsole *op);
>   
> @@ -20,8 +23,6 @@ void console_init_postirq(void);
>   void console_endboot(void);
>   int console_has(const char *device);
>   
> -unsigned long console_lock_recursive_irqsave(void);
> -void console_unlock_recursive_irqrestore(unsigned long flags);
>   void console_force_unlock(void);
>   
>   void console_start_sync(void);
> diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
> index c99ee52458..53f0f72ac4 100644
> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -218,7 +218,14 @@ void _spin_barrier(spinlock_t *lock);
>    */
>   int rspin_trylock(rspinlock_t *lock);
>   void rspin_lock(rspinlock_t *lock);
> +#define rspin_lock_irqsave(l, f)                                \
> +    ({                                                          \
> +        BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long));       \
> +        ((f) = __rspin_lock_irqsave(l));                        \
> +    })
If f is &flags, then s/f/*(f)/ would be needed in these 2 cases.

On other matters if we had -Wconversion turned on by default that
BUILD_BUG_ON() wouldn't be needed. Not that you can do it (I'm sure the 
codebase would explode everywhere if we switched it on), but might be
something to consider in the future.

> +unsigned long __rspin_lock_irqsave(rspinlock_t *lock);
>   void rspin_unlock(rspinlock_t *lock);
> +void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags);
>   
>   #define spin_lock(l)                  _spin_lock(l)
>   #define spin_lock_cb(l, c, d)         _spin_lock_cb(l, c, d)
Cheers,
Alejandro