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

[Jürgen Groß]

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 <jgr...@suse.com>
---
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