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@suse.com>
---
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);
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)); \
+ })
+unsigned long __rspin_lock_irqsave(rspinlock_t *lock);
void rspin_unlock(rspinlock_t *lock);
+void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags);
#define spin_lock(l) _spin_lock(l)
#define spin_lock_cb(l, c, d) _spin_lock_cb(l, c, d)
--
2.35.3On 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@suse.com>
> ---
> 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.
>
> 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.
> +unsigned long __rspin_lock_irqsave(rspinlock_t *lock);
> void rspin_unlock(rspinlock_t *lock);
> +void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags);
>
> #define spin_lock(l) _spin_lock(l)
> #define spin_lock_cb(l, c, d) _spin_lock_cb(l, c, d)
Cheers,
Alejandro
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@suse.com>
>> ---
>> 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
© 2016 - 2026 Red Hat, Inc.