Re: [kvm-unit-tests PATCH v2 5/6] s390x: lib: sie: don't reenter SIE on pgm int

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Tue, 16 May 2023 15:04:55 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> At the moment, when a PGM int occurs while in SIE, we will just reenter
> SIE after the interrupt handler was called.
> 
> This is because sie() has a loop which checks icptcode and re-enters SIE
> if it is zero.
> 
> However, this behaviour is quite undesirable for SIE tests, since it
> doesn't give the host the chance to assert on the PGM int. Instead, we
> will just re-enter SIE, on nullifing conditions even causing the
> exception again.
> 
> In sie(), check whether a pgm int code is set in lowcore. If it has,
> exit the loop so the test can react to the interrupt. Add a new function
> read_pgm_int_code() to obtain the interrupt code.
> 
> Note that this introduces a slight oddity with sie and pgm int in
> certain cases: If a PGM int occurs between a expect_pgm_int() and sie(),
> we will now never enter SIE until the pgm_int_code is cleared by e.g.
> clear_pgm_int().
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  lib/s390x/asm/interrupt.h |  1 +
>  lib/s390x/interrupt.c     | 15 +++++++++++++++
>  lib/s390x/sie.c           |  4 +++-
>  lib/s390x/sie.h           |  1 -
>  4 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index 55759002dce2..2d7eb1907458 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -81,6 +81,7 @@ void handle_svc_int(void);
>  void expect_pgm_int(void);
>  void expect_ext_int(void);
>  uint16_t clear_pgm_int(void);
> +uint16_t read_pgm_int_code(void);
>  void check_pgm_int_code(uint16_t code);
>  
>  #define IRQ_DAT_ON	true
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 2e5309cee40f..82b4259d433c 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -60,6 +60,21 @@ uint16_t clear_pgm_int(void)
>  	return code;
>  }
>  
> +/**
> + * read_pgm_int_code - Get the program interruption code of the last pgm int
> + * on the current CPU.
> + *
> + * This is similar to clear_pgm_int(), except that it doesn't clear the
> + * interruption information from lowcore.
> + *
> + * Returns 0 when none occured.
> + */
> +uint16_t read_pgm_int_code(void)

could this whole function go in the header as static inline?

> +{
> +	mb();

is the mb really needed?

> +	return lowcore.pgm_int_code;
> +}
> +
>  /**
>   * check_pgm_int_code - Check the program interrupt code on the current CPU.
>   * @code the expected program interrupt code on the current CPU
> diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
> index ffa8ec91a423..632740edd431 100644
> --- a/lib/s390x/sie.c
> +++ b/lib/s390x/sie.c
> @@ -13,6 +13,7 @@
>  #include <libcflat.h>
>  #include <sie.h>
>  #include <asm/page.h>
> +#include <asm/interrupt.h>
>  #include <libcflat.h>
>  #include <alloc_page.h>
>  
> @@ -65,7 +66,8 @@ void sie(struct vm *vm)
>  	/* also handle all interruptions in home space while in SIE */
>  	irq_set_dat_mode(IRQ_DAT_ON, AS_HOME);
>  
> -	while (vm->sblk->icptcode == 0) {
> +	/* leave SIE when we have an intercept or an interrupt so the test can react to it */
> +	while (vm->sblk->icptcode == 0 && !read_pgm_int_code()) {
>  		sie64a(vm->sblk, &vm->save_area);
>  		sie_handle_validity(vm);
>  	}
> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> index 0b00fb709776..147cb0f2a556 100644
> --- a/lib/s390x/sie.h
> +++ b/lib/s390x/sie.h
> @@ -284,6 +284,5 @@ void sie_handle_validity(struct vm *vm);
>  void sie_guest_sca_create(struct vm *vm);
>  void sie_guest_create(struct vm *vm, uint64_t guest_mem, uint64_t guest_mem_len);
>  void sie_guest_destroy(struct vm *vm);
> -bool sie_had_pgm_int(struct vm *vm);

... ?

>  
>  #endif /* _S390X_SIE_H_ */