Re: [PATCH v5 3/4] KVM: s390: Change the fi->lock to a raw_spinlock for RT case

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 5/7/26 5:56 AM, Heiko Carstens wrote:
> On Wed, May 06, 2026 at 10:50:52AM -0400, Douglas Freimuth wrote:
>> On 5/6/26 12:57 AM, Heiko Carstens wrote:
>>> On Tue, May 05, 2026 at 07:37:27PM +0200, Douglas Freimuth wrote:
>>>> s390 needs to maintain support for an RT kernel. This requires the
>>>> floating interrupt lock, fi->lock to be changed to a raw spin lock
>>>> since the fi->lock maybe called with interrupts disabled in __inject_io.
>>>>
>>>> Signed-off-by: Douglas Freimuth <freimuth@linux.ibm.com>
>>>> ---
>>>>   arch/s390/include/asm/kvm_host.h |  2 +-
>>>>   arch/s390/kvm/intercept.c        |  4 +-
>>>>   arch/s390/kvm/interrupt.c        | 68 ++++++++++++++++----------------
>>>>   arch/s390/kvm/kvm-s390.c         |  2 +-
>>>>   4 files changed, 38 insertions(+), 38 deletions(-)
>>>
>>> s390 does not support RT, but I guess you are referring to a lockdep splat
>>> which you would see without doing this change, similar like we have seen at
>>> other places.
>>>
>>> Can you include the relevant parts of the splat for reference, please?
>>
>> Heiko, thank you for you response. I dont recall trapping it with lockdep
>> (while it was on) but discussion on the mailing list in an earlier version
>> made us look closer (and we saw it across the AI models that reviewed the
>> patch.) It appears that while RT isn't supported it can still be compiled in
>> to the kernel so we wanted to mitigate the issues we would add to if someone
>> does that while not impacting non-RT environments, the main use case.
> 
> RT support cannot be compiled in for s390, because of the missing
> "select ARCH_SUPPORTS_RT", however you can still enable lockdep checks
> for raw_spinlock vs spinlock nesting, which this seems to appear about?
> 
> See PROVE_RAW_LOCK_NESTING config option for a more detailed description.
> 
> Therefore my question about a lockdep splat. However I don't see why
> using spin_lock() instead of raw_spin_lock() alone in irq disabled
> context could be problematic. On the other hand this patch does

Hi Heiko,

AFAIU it is only problematic if we (s390) should ever want to support RT
in the future.

As the name implies, the point of kvm_arch_set_irq_inatomic() is to
inject the interrupt without the possibility of sleeping, or
alternatively recognizing the need to sleep and fall back to a queued
"slow path" that can safely sleep while delivering it.

My original thinking was 'well, it won't hurt to use the raw spinlocks
in the new code' so I set Doug down this road with my review comments --
I did not consider that there would be a need for additional fallout
like this patch, which means increased chance of regressions (see below)
to accomodate a feature that we don't support today.

If you are saying it's OK to simply not care about RT for s390 now, then
AFAICT it should be fine to just use s/raw_spin_)lock/spin_lock/ for
this whole series, drop this patch and then ignore the subsequent
Sashiko complaints about RT.

What do you think?

> introduce a
> 
>       raw_spin_lock();
>       spin_lock();
>       spin_unlock();
>       raw_spin_unlock();
> 
> sequence in __deliver_machine_check() which seems to be incorrect and
> indeed should generate a lockdep splat iff PROVE_RAW_LOCK_NESTING is
> enabled.

+1
Doug, I know you've run with lockdep enabled before on this series --
please make sure to test with lockdep for next version

Thanks,
Matt