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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.12.2023 10: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

In how far is this a necessary part of the series?

> --- 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);
>      }
>  

I view these as layering violations; ...

> --- 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);

... this should remain static. The question therefore just is whether
to omit this patch or ...

> @@ -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);
> -}

... whether to retain these two functions as thin wrappers around the
new, more generic construct.

Jan