Re: [PATCH v4 05/12] xen/spinlock: add rspin_[un]lock_irq[save|restore]()

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 12/12/2023 09:47, 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 7724306116..21227877b3 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);
>   
>       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 422a7fb1db..c1a9ba1304 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)
This is going to be a problem with MISRA (see Rule 21.1). Can you move 
the double underscore to the end? Alternatively, I am not sure I see the 
benefits of the function here. So maybe we can fold the code in the 
macro below?
Other than that, the rest of the patch LGTM.

Cheers,

--
Julien Grall