| Message ID | 20230601070202.152094-6-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Add support for running guests without MSO/MSL | expand |
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); > }
On Mon, 5 Jun 2023 11:30:36 +0200 Janosch Frank <frankja@linux.ibm.com> wrote: [,,,] > > + */ > > +static inline uint16_t read_pgm_int_code(void) > > +{ > > No mb()? is one needed there? > > > + 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); > > } >
Quoting Janosch Frank (2023-06-05 11:30:36) > 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 -_- True, C snippets should have a invalid PGM new PSW too. Let me have a try after my holiday... *writes TODO* > 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? Well, I think we need to distinguish between two kinds of PGMs: - PGMs in the guest, - PGMs caused by SIE on the host (e.g. because the gpa-hpa mapping is not present) The first case is out of scope for this patch, but certainly something which can be improved. This patch focuses on the latter case, where code 8 handling is irrelevant since the PGM is always delivered to the host. > - Do we want to assert like with validities and PGMs outside of SIE? Well, we would assert() except if we have an expect_pgm_int(), which we do have :) > - Should sie() have a int return code like in KVM? That's a lovely idea, maybe we should have this at some point in the future, but I suppose it would require reshuffling large parts of the tests, so please excuse me if I don't want to do this in the context of this series :) *writes another TODO* [...] > > Also add missing include of facility.h to mem.h. > > ? That explains why we're including facility.h in mem.h below. [...] > > 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. Claudio requested this to be in the C file, I really don't mind much. Claudio, maybe you can elaborate why you wanted it in the header. > > + * > > + * 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/ Fixed. > > + */ > > +static inline uint16_t read_pgm_int_code(void) > > +{ > > No mb()? This is a function call, so none should be needed, no?
On 6/30/23 16:59, Nico Boehr wrote: > Quoting Janosch Frank (2023-06-05 11:30:36) >> 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 -_- > > True, C snippets should have a invalid PGM new PSW too. Let me have a try after > my holiday... *writes TODO* > >> 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? > > Well, I think we need to distinguish between two kinds of PGMs: > - PGMs in the guest, > - PGMs caused by SIE on the host (e.g. because the gpa-hpa mapping is not > present) > > The first case is out of scope for this patch, but certainly something which can > be improved. > > This patch focuses on the latter case, where code 8 handling is irrelevant since > the PGM is always delivered to the host. Seems like I wasn't fully awake and/or distracted by hunger when I was reading this patch -_-
On Fri, 30 Jun 2023 16:59:06 +0200 Nico Boehr <nrb@linux.ibm.com> wrote: > [...] > > > 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. > > Claudio requested this to be in the C file, I really don't mind much. Claudio, you meant header > maybe you can elaborate why you wanted it in the header. so it can be inlined it's literally just a read... you can put it in the C file if you want, but it seems a waste to me tbh > > > > + * > > > + * 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/ > > Fixed. > > > > + */ > > > +static inline uint16_t read_pgm_int_code(void) > > > +{ > > > > No mb()? > > This is a function call, so none should be needed, no?
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. + * + * This is similar to clear_pgm_int(), except that it doesn't clear the + * interruption information from lowcore. + * + * Returns 0 when none occured. + */ +static inline uint16_t read_pgm_int_code(void) +{ + 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); }
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(). 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(-)