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

[Janis Schoetterl-Glausch <scgl@linux.ibm.com>]

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?