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

Janosch Frank
2023-06-05T09:30:48Z

On 6/1/23 09:02, Nico Boehr 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.

That's the reason why we set an invalid PGM PSW new for the assembly
snippets. Seems like I didn't add it for C snippets for some reason -_-

This code is fine but it doesn't fully fix the usability aspect and
leaves a few questions open:
- Do we want to stick to the code 8 handling?
- Do we want to assert like with validities and PGMs outside of SIE?
- Should sie() have a int return code like in KVM?

>
> 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().
>
> Also add missing include of facility.h to mem.h.

?

>
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
> lib/s390x/asm/interrupt.h | 14 ++++++++++++++
> lib/s390x/asm/mem.h | 1 +
> lib/s390x/sie.c | 4 +++-
> 3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index 55759002dce2..fb4283a40a1b 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -99,4 +99,18 @@ static inline void low_prot_disable(void)
> ctl_clear_bit(0, CTL0_LOW_ADDR_PROT);
> }
>
> +/**
> + * read_pgm_int_code - Get the program interruption code of the last pgm int
> + * on the current CPU.

All of the other functions are in the c file.

> + *
> + * This is similar to clear_pgm_int(), except that it doesn't clear the
> + * interruption information from lowcore.
> + *
> + * Returns 0 when none occured.

s/r/rr/

> + */
> +static inline uint16_t read_pgm_int_code(void)
> +{

No mb()?

> + return lowcore.pgm_int_code;
> +}
> +
> #endif
> diff --git a/lib/s390x/asm/mem.h b/lib/s390x/asm/mem.h
> index 64ef59b546a4..94d58c34f53f 100644
> --- a/lib/s390x/asm/mem.h
> +++ b/lib/s390x/asm/mem.h
> @@ -8,6 +8,7 @@
> #ifndef _ASMS390X_MEM_H_
> #define _ASMS390X_MEM_H_
> #include <asm/arch_def.h>
> +#include <asm/facility.h>
>
> /* create pointer while avoiding compiler warnings */
> #define OPAQUE_PTR(x) ((void *)(((uint64_t)&lowcore) + (x)))
> 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);
> }