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

Thomas Huth
2021-12-03T11:22:57Z

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