Re: [kvm-unit-tests PATCH v2 2/2] s390x: firq: floating interrupt test

[Thomas Huth <thuth@redhat.com>]

On 03/12/2021 12.18, Claudio Imbrenda wrote:
> On Fri, 3 Dec 2021 11:55:31 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 02/12/2021 13.35, David Hildenbrand wrote:
>>> We had a KVM BUG fixed by kernel commit a3e03bc1368c ("KVM: s390: index
>>> kvm->arch.idle_mask by vcpu_idx"), whereby a floating interrupt might get
>>> stuck forever because a CPU in the wait state would not get woken up.
>>>
>>> The issue can be triggered when CPUs are created in a nonlinear fashion,
>>> such that the CPU address ("core-id") and the KVM cpu id don't match.
>>>
>>> So let's start with a floating interrupt test that will trigger a
>>> floating interrupt (via SCLP) to be delivered to a CPU in the wait state.
>>
>> Thank you very much for tackling this! Some remarks below...
>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>    lib/s390x/sclp.c    |  11 ++--
>>>    lib/s390x/sclp.h    |   1 +
>>>    s390x/Makefile      |   1 +
>>>    s390x/firq.c        | 122 ++++++++++++++++++++++++++++++++++++++++++++
>>>    s390x/unittests.cfg |  10 ++++
>>>    5 files changed, 142 insertions(+), 3 deletions(-)
>>>    create mode 100644 s390x/firq.c
>>>
>>> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
>>> index 0272249..33985eb 100644
>>> --- a/lib/s390x/sclp.c
>>> +++ b/lib/s390x/sclp.c
>>> @@ -60,9 +60,7 @@ void sclp_setup_int(void)
>>>    void sclp_handle_ext(void)
>>>    {
>>>    	ctl_clear_bit(0, CTL0_SERVICE_SIGNAL);
>>> -	spin_lock(&sclp_lock);
>>> -	sclp_busy = false;
>>> -	spin_unlock(&sclp_lock);
>>> +	sclp_clear_busy();
>>>    }
>>>    
>>>    void sclp_wait_busy(void)
>>> @@ -89,6 +87,13 @@ void sclp_mark_busy(void)
>>>    	}
>>>    }
>>>    
>>> +void sclp_clear_busy(void)
>>> +{
>>> +	spin_lock(&sclp_lock);
>>> +	sclp_busy = false;
>>> +	spin_unlock(&sclp_lock);
>>> +}
>>> +
>>>    static void sclp_read_scp_info(ReadInfo *ri, int length)
>>>    {
>>>    	unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED,
>>> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
>>> index 61e9cf5..fead007 100644
>>> --- a/lib/s390x/sclp.h
>>> +++ b/lib/s390x/sclp.h
>>> @@ -318,6 +318,7 @@ void sclp_setup_int(void);
>>>    void sclp_handle_ext(void);
>>>    void sclp_wait_busy(void);
>>>    void sclp_mark_busy(void);
>>> +void sclp_clear_busy(void);
>>>    void sclp_console_setup(void);
>>>    void sclp_print(const char *str);
>>>    void sclp_read_info(void);
>>> diff --git a/s390x/Makefile b/s390x/Makefile
>>> index f95f2e6..1e567c1 100644
>>> --- a/s390x/Makefile
>>> +++ b/s390x/Makefile
>>> @@ -25,6 +25,7 @@ tests += $(TEST_DIR)/uv-host.elf
>>>    tests += $(TEST_DIR)/edat.elf
>>>    tests += $(TEST_DIR)/mvpg-sie.elf
>>>    tests += $(TEST_DIR)/spec_ex-sie.elf
>>> +tests += $(TEST_DIR)/firq.elf
>>>    
>>>    tests_binary = $(patsubst %.elf,%.bin,$(tests))
>>>    ifneq ($(HOST_KEY_DOCUMENT),)
>>> diff --git a/s390x/firq.c b/s390x/firq.c
>>> new file mode 100644
>>> index 0000000..1f87718
>>> --- /dev/null
>>> +++ b/s390x/firq.c
>>> @@ -0,0 +1,122 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Floating interrupt tests.
>>> + *
>>> + * Copyright 2021 Red Hat Inc
>>> + *
>>> + * Authors:
>>> + *    David Hildenbrand <david@redhat.com>
>>> + */
>>> +#include <libcflat.h>
>>> +#include <asm/asm-offsets.h>
>>> +#include <asm/interrupt.h>
>>> +#include <asm/page.h>
>>> +#include <asm-generic/barrier.h>
>>> +
>>> +#include <sclp.h>
>>> +#include <smp.h>
>>> +#include <alloc_page.h>
>>> +
>>> +static void wait_for_sclp_int(void)
>>> +{
>>> +	/* Enable SCLP interrupts on this CPU only. */
>>> +	ctl_set_bit(0, CTL0_SERVICE_SIGNAL);
>>> +
>>> +	/* Enable external interrupts and go to the wait state. */
>>> +	wait_for_interrupt(PSW_MASK_EXT);
>>> +}
>>
>> What happens if the CPU got an interrupt? Should there be a "while (true)"
> 
> it should not get any interrupts, but if it does anyway...
> 
>> at the end of the function to avoid that the CPU ends up crashing at the end
>> of the function?
> 
> ... we have this in smp_cpu_setup_state, after the call to the actual
> function body:
> 
> /* If the function returns, just loop here */
> 0:      j       0
> 
> so if the function returns, it will hang in there anyway

Ah, great, so we're fine indeed!

  Thomas