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 <[email protected]>
> ---
> 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
