Re: [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks

Janis Schoetterl-Glausch
2022-06-07T10:01:22Z

On 6/3/22 17:40, Claudio Imbrenda wrote:
> Use per-CPU flags and callbacks for Program, Extern, and I/O interrupts
> instead of global variables.
>
> This allows for more accurate error handling; a CPU waiting for an
> interrupt will not have it "stolen" by a different CPU that was not
> supposed to wait for one, and now two CPUs can wait for interrupts at
> the same time.
>
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
> lib/s390x/asm/arch_def.h | 7 ++++++-
> lib/s390x/interrupt.c | 38 ++++++++++++++++----------------------
> 2 files changed, 22 insertions(+), 23 deletions(-)
>
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 72553819..3a0d9c43 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -124,7 +124,12 @@ struct lowcore {
> uint8_t pad_0x0280[0x0308 - 0x0280]; /* 0x0280 */
> uint64_t sw_int_crs[16]; /* 0x0308 */
> struct psw sw_int_psw; /* 0x0388 */
> - uint8_t pad_0x0310[0x11b0 - 0x0398]; /* 0x0398 */
> + uint32_t pgm_int_expected; /* 0x0398 */
> + uint32_t ext_int_expected; /* 0x039c */
> + void (*pgm_cleanup_func)(void); /* 0x03a0 */
> + void (*ext_cleanup_func)(void); /* 0x03a8 */
> + void (*io_int_func)(void); /* 0x03b0 */

If you switch the function pointers and the *_expected around,
you can use bools for the latter, right?
I think, since they're names suggest that they're bools, they should
be. Additionally I prefer true/false over 1/0, since the latter raises
the questions if other values are also used.

> + uint8_t pad_0x03b8[0x11b0 - 0x03b8]; /* 0x03b8 */
> uint64_t mcck_ext_sa_addr; /* 0x11b0 */
> uint8_t pad_0x11b8[0x1200 - 0x11b8]; /* 0x11b8 */
> uint64_t fprs_sa[16]; /* 0x1200 */
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 27d3b767..e57946f0 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -15,14 +15,11 @@
> #include <fault.h>
> #include <asm/page.h>
>
> -static bool pgm_int_expected;
> -static bool ext_int_expected;
> -static void (*pgm_cleanup_func)(void);
> static struct lowcore *lc;
>
> void expect_pgm_int(void)
> {
> - pgm_int_expected = true;
> + lc->pgm_int_expected = 1;
> lc->pgm_int_code = 0;
> lc->trans_exc_id = 0;
> mb();

[...]

> void handle_pgm_int(struct stack_frame_int *stack)
> {
> - if (!pgm_int_expected) {
> + if (!lc->pgm_int_expected) {
> /* Force sclp_busy to false, otherwise we will loop forever */
> sclp_handle_ext();
> print_pgm_info(stack);
> }
>
> - pgm_int_expected = false;
> + lc->pgm_int_expected = 0;
>
> - if (pgm_cleanup_func)
> - (*pgm_cleanup_func)();
> + if (lc->pgm_cleanup_func)
> + (*lc->pgm_cleanup_func)();

[...]

> + if (lc->io_int_func)
> + return lc->io_int_func();
Why is a difference between the function pointer usages here?