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

[Jürgen Groß <jgross@xxxxxxxx>]

On 28.02.24 16:09, Jan Beulich wrote:
> 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?
Not really necessary. It just seemed wrong to have an open coded
variant of rspin_lock_irqsave() and rspin_unlock_irqrestore().

> > --- 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.
I'd vote for the latter.


Juergen