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

[Juergen Gross <jgross@xxxxxxxx>]

On 12.12.23 14:03, Julien Grall wrote:
> 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?
I think I'll follow the common pattern and just rename the function as you
suggest.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature