Re: [PATCH v3 06/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]()

[Juergen Gross <jgross@xxxxxxxx>]

On 24.11.23 20:23, Alejandro Vallejo wrote:
> On 20/11/2023 11:38, 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 e1356f696a..f72769e79b 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);
> This feels a bit weird because rspin_lock_irqsave() being lowercase
> hints that it's a either a function or behaves like one. For that it
> should take &flags instead.
I don't think so. This is common behavior with the Linux kernel, and I
think we should keep it. Especially as I believe rspin_lock_irqsave()
and spin_lock_irqsave() should have a similar interface. Or would you want
us to change more than 250 call sites of spin_lock_irqsave()?

> >       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 26c667d3cc..17716fc4eb 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)
> > +{
> > +    unsigned long flags;
> > +
> > +    local_irq_save(flags);
> > +    rspin_lock(lock);
> > +
> > +    return flags;
> > +}
> > +
> >   void rspin_unlock(rspinlock_t *lock)
> >   {
> >       if ( likely(--lock->recurse_cnt == 0) )
> > @@ -484,6 +494,12 @@ void rspin_unlock(rspinlock_t *lock)
> >       }
> >   }
> > +void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags)
> > +{
> > +    rspin_unlock(lock);
> > +    local_irq_restore(flags);
> > +}
> > +
> >   #ifdef CONFIG_DEBUG_LOCK_PROFILE
> >   struct lock_profile_anc {
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 369b2f9077..cc743b67ec 100644
> > --- 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);
> >   /*
> >    * To control the amount of printing, thresholds are added.
> > @@ -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);
> > -}
> > -
> >   void console_force_unlock(void)
> >   {
> >       watchdog_disable();
> > diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
> > index ab5c30c0da..dff0096b27 100644
> > --- a/xen/include/xen/console.h
> > +++ b/xen/include/xen/console.h
> > @@ -8,8 +8,11 @@
> >   #define __CONSOLE_H__
> >   #include <xen/inttypes.h>
> > +#include <xen/spinlock.h>
> >   #include <public/xen.h>
> > +extern rspinlock_t console_lock;
> > +
> >   struct xen_sysctl_readconsole;
> >   long read_console_ring(struct xen_sysctl_readconsole *op);
> > @@ -20,8 +23,6 @@ void console_init_postirq(void);
> >   void console_endboot(void);
> >   int console_has(const char *device);
> > -unsigned long console_lock_recursive_irqsave(void);
> > -void console_unlock_recursive_irqrestore(unsigned long flags);
> >   void console_force_unlock(void);
> >   void console_start_sync(void);
> > diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
> > index c99ee52458..53f0f72ac4 100644
> > --- a/xen/include/xen/spinlock.h
> > +++ b/xen/include/xen/spinlock.h
> > @@ -218,7 +218,14 @@ void _spin_barrier(spinlock_t *lock);
> >    */
> >   int rspin_trylock(rspinlock_t *lock);
> >   void rspin_lock(rspinlock_t *lock);
> > +#define rspin_lock_irqsave(l, f)                                \
> > +    ({                                                          \
> > +        BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long));       \
> > +        ((f) = __rspin_lock_irqsave(l));                        \
> > +    })
> If f is &flags, then s/f/*(f)/ would be needed in these 2 cases.
>
> On other matters if we had -Wconversion turned on by default that
> BUILD_BUG_ON() wouldn't be needed. Not that you can do it (I'm sure the codebase 
> would explode everywhere if we switched it on), but might be
> something to consider in the future.
Again, this is like Linux kernel. And I don't think we want explicit casts
everywhere when e.g. moving values between fixed sized fields and standard
integer typed fields.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature