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

From: Juergen Gross <jgross@suse.com>
To: Julien Grall <julien@xen.org>, xen-devel@lists.xenproject.org
Cc: "Jan Beulich" <jbeulich@suse.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>
Subject: Re: [PATCH v4 05/12] xen/spinlock: add rspin_[un]lock_irq[save|restore]()
Date: Tue, 12 Dec 2023 15:16:48 +0100	[thread overview]
Message-ID: <bc236e16-cbc5-4303-a8e8-cdd8e2efd0d6@suse.com> (raw)
In-Reply-To: <319c928c-500c-4f41-8a65-3ffdeb888d63@xen.org>

[-- Attachment #1.1.1: Type: text/plain, Size: 3708 bytes --]

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@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 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 #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]