All of lore.kernel.org
 help / color / mirror / Atom feed
* [kvm-unit-tests PATCH v1 0/2] better smp interrupt checks
@ 2022-06-03 15:40 Claudio Imbrenda
  2022-06-03 15:40 ` [kvm-unit-tests PATCH v1 1/2] s390x: skey.c: rework the interrupt handler Claudio Imbrenda
  2022-06-03 15:40 ` [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks Claudio Imbrenda
  0 siblings, 2 replies; 11+ messages in thread
From: Claudio Imbrenda @ 2022-06-03 15:40 UTC (permalink / raw)
  To: kvm; +Cc: linux-s390, frankja, scgl, pmorel, nrb, thuth

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.

Also fix skey.c to be compatible with the new interrupt handling.

Claudio Imbrenda (2):
  s390x: skey.c: rework the interrupt handler
  lib: s390x: better smp interrupt checks

 lib/s390x/asm/arch_def.h |  7 ++++++-
 lib/s390x/interrupt.c    | 38 ++++++++++++++++----------------------
 s390x/skey.c             | 24 +++++++++---------------
 3 files changed, 31 insertions(+), 38 deletions(-)

-- 
2.36.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [kvm-unit-tests PATCH v1 1/2] s390x: skey.c: rework the interrupt handler
  2022-06-03 15:40 [kvm-unit-tests PATCH v1 0/2] better smp interrupt checks Claudio Imbrenda
@ 2022-06-03 15:40 ` Claudio Imbrenda
  2022-06-07 13:29   ` Nico Boehr
  2022-06-03 15:40 ` [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks Claudio Imbrenda
  1 sibling, 1 reply; 11+ messages in thread
From: Claudio Imbrenda @ 2022-06-03 15:40 UTC (permalink / raw)
  To: kvm; +Cc: linux-s390, frankja, scgl, pmorel, nrb, thuth

The skey test currently uses a cleanup function to work around the
issues that arise when the lowcore is not mapped, since the interrupt
handler needs to access it.

Instead of a cleanup function, simply disable DAT for the interrupt
handler for the tests that remap page 0. This is needed in preparation
of and upcoming patch that will cause the interrupt handler to read
from lowcore before calling the cleanup function.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/skey.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/s390x/skey.c b/s390x/skey.c
index 32bf1070..d01c6f79 100644
--- a/s390x/skey.c
+++ b/s390x/skey.c
@@ -15,6 +15,7 @@
 #include <asm/facility.h>
 #include <asm/mem.h>
 
+static struct lowcore *lc;
 
 static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
 
@@ -250,19 +251,6 @@ static void set_prefix_key_1(uint32_t *prefix_ptr)
 	);
 }
 
-/*
- * We remapped page 0, making the lowcore inaccessible, which breaks the normal
- * handler and breaks skipping the faulting instruction.
- * Just disable dynamic address translation to make things work.
- */
-static void dat_fixup_pgm_int(void)
-{
-	uint64_t psw_mask = extract_psw_mask();
-
-	psw_mask &= ~PSW_MASK_DAT;
-	load_psw_mask(psw_mask);
-}
-
 #define PREFIX_AREA_SIZE (PAGE_SIZE * 2)
 static char lowcore_tmp[PREFIX_AREA_SIZE] __attribute__((aligned(PREFIX_AREA_SIZE)));
 
@@ -318,7 +306,13 @@ static void test_set_prefix(void)
 	report(get_prefix() == old_prefix, "did not set prefix");
 	report_prefix_pop();
 
-	register_pgm_cleanup_func(dat_fixup_pgm_int);
+	/*
+	 * Page 0 will be remapped, making the lowcore inaccessible, which
+	 * breaks the normal handler and breaks skipping the faulting
+	 * instruction. Disable dynamic address translation for the
+	 * interrupt handler to make things work.
+	 */
+	lc->pgm_new_psw.mask &= ~PSW_MASK_DAT;
 
 	report_prefix_push("remapped page, fetch protection");
 	set_prefix(old_prefix);
@@ -356,7 +350,7 @@ static void test_set_prefix(void)
 	report_prefix_pop();
 
 	ctl_clear_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
-	register_pgm_cleanup_func(NULL);
+	lc->pgm_new_psw.mask |= PSW_MASK_DAT;
 	report_prefix_pop();
 	set_storage_key(pagebuf, 0x00, 0);
 	report_prefix_pop();
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks
  2022-06-03 15:40 [kvm-unit-tests PATCH v1 0/2] better smp interrupt checks Claudio Imbrenda
  2022-06-03 15:40 ` [kvm-unit-tests PATCH v1 1/2] s390x: skey.c: rework the interrupt handler Claudio Imbrenda
@ 2022-06-03 15:40 ` Claudio Imbrenda
  2022-06-07 10:01   ` Janis Schoetterl-Glausch
                     ` (2 more replies)
  1 sibling, 3 replies; 11+ messages in thread
From: Claudio Imbrenda @ 2022-06-03 15:40 UTC (permalink / raw)
  To: kvm; +Cc: linux-s390, frankja, scgl, pmorel, nrb, thuth

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 */
+	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();
@@ -30,7 +27,7 @@ void expect_pgm_int(void)
 
 void expect_ext_int(void)
 {
-	ext_int_expected = true;
+	lc->ext_int_expected = 1;
 	lc->ext_int_code = 0;
 	mb();
 }
@@ -43,7 +40,7 @@ uint16_t clear_pgm_int(void)
 	code = lc->pgm_int_code;
 	lc->pgm_int_code = 0;
 	lc->trans_exc_id = 0;
-	pgm_int_expected = false;
+	lc->pgm_int_expected = 0;
 	return code;
 }
 
@@ -57,7 +54,7 @@ void check_pgm_int_code(uint16_t code)
 
 void register_pgm_cleanup_func(void (*f)(void))
 {
-	pgm_cleanup_func = f;
+	lc->pgm_cleanup_func = f;
 }
 
 static void fixup_pgm_int(struct stack_frame_int *stack)
@@ -184,24 +181,23 @@ static void print_pgm_info(struct stack_frame_int *stack)
 
 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)();
 	else
 		fixup_pgm_int(stack);
 }
 
 void handle_ext_int(struct stack_frame_int *stack)
 {
-	if (!ext_int_expected &&
-	    lc->ext_int_code != EXT_IRQ_SERVICE_SIG) {
+	if (!lc->ext_int_expected && lc->ext_int_code != EXT_IRQ_SERVICE_SIG) {
 		report_abort("Unexpected external call interrupt (code %#x): on cpu %d at %#lx",
 			     lc->ext_int_code, stap(), lc->ext_old_psw.addr);
 		return;
@@ -211,7 +207,7 @@ void handle_ext_int(struct stack_frame_int *stack)
 		stack->crs[0] &= ~(1UL << 9);
 		sclp_handle_ext();
 	} else {
-		ext_int_expected = false;
+		lc->ext_int_expected = 0;
 	}
 
 	if (!(stack->crs[0] & CR0_EXTM_MASK))
@@ -224,12 +220,10 @@ void handle_mcck_int(void)
 		     stap(), lc->mcck_old_psw.addr);
 }
 
-static void (*io_int_func)(void);
-
 void handle_io_int(void)
 {
-	if (io_int_func)
-		return io_int_func();
+	if (lc->io_int_func)
+		return lc->io_int_func();
 
 	report_abort("Unexpected io interrupt: on cpu %d at %#lx",
 		     stap(), lc->io_old_psw.addr);
@@ -237,17 +231,17 @@ void handle_io_int(void)
 
 int register_io_int_func(void (*f)(void))
 {
-	if (io_int_func)
+	if (lc->io_int_func)
 		return -1;
-	io_int_func = f;
+	lc->io_int_func = f;
 	return 0;
 }
 
 int unregister_io_int_func(void (*f)(void))
 {
-	if (io_int_func != f)
+	if (lc->io_int_func != f)
 		return -1;
-	io_int_func = NULL;
+	lc->io_int_func = NULL;
 	return 0;
 }
 
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks
  2022-06-03 15:40 ` [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks Claudio Imbrenda
@ 2022-06-07 10:01   ` Janis Schoetterl-Glausch
  2022-06-07 11:08     ` Claudio Imbrenda
  2022-06-07 14:23   ` Nico Boehr
  2022-06-10  9:43   ` Janosch Frank
  2 siblings, 1 reply; 11+ messages in thread
From: Janis Schoetterl-Glausch @ 2022-06-07 10:01 UTC (permalink / raw)
  To: Claudio Imbrenda, kvm; +Cc: linux-s390, frankja, pmorel, nrb, thuth

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?


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks
  2022-06-07 10:01   ` Janis Schoetterl-Glausch
@ 2022-06-07 11:08     ` Claudio Imbrenda
  0 siblings, 0 replies; 11+ messages in thread
From: Claudio Imbrenda @ 2022-06-07 11:08 UTC (permalink / raw)
  To: Janis Schoetterl-Glausch; +Cc: kvm, linux-s390, frankja, pmorel, nrb, thuth

On Tue, 7 Jun 2022 12:01:11 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> 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.

that's exactly what I wanted to avoid. uint32_t can easily be accessed
atomically and/or compare-and-swapped if needed.

I don't like using true/false for things that are not bools

> 
> > +	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?
> 

because that is how it was before; both have the same semantics anyway


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [kvm-unit-tests PATCH v1 1/2] s390x: skey.c: rework the interrupt handler
  2022-06-03 15:40 ` [kvm-unit-tests PATCH v1 1/2] s390x: skey.c: rework the interrupt handler Claudio Imbrenda
@ 2022-06-07 13:29   ` Nico Boehr
  0 siblings, 0 replies; 11+ messages in thread
From: Nico Boehr @ 2022-06-07 13:29 UTC (permalink / raw)
  To: Claudio Imbrenda; +Cc: kvm, linux-s390, frankja, scgl, pmorel, thuth

On Fri,  3 Jun 2022 17:40:36 +0200
Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:

> The skey test currently uses a cleanup function to work around the
> issues that arise when the lowcore is not mapped, since the interrupt
> handler needs to access it.
> 
> Instead of a cleanup function, simply disable DAT for the interrupt
> handler for the tests that remap page 0. This is needed in preparation
> of and upcoming patch that will cause the interrupt handler to read
> from lowcore before calling the cleanup function.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks
  2022-06-03 15:40 ` [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks Claudio Imbrenda
  2022-06-07 10:01   ` Janis Schoetterl-Glausch
@ 2022-06-07 14:23   ` Nico Boehr
  2022-06-07 14:41     ` Claudio Imbrenda
  2022-06-10  9:43   ` Janosch Frank
  2 siblings, 1 reply; 11+ messages in thread
From: Nico Boehr @ 2022-06-07 14:23 UTC (permalink / raw)
  To: Claudio Imbrenda; +Cc: kvm, linux-s390, frankja, scgl, pmorel, thuth

On Fri,  3 Jun 2022 17:40:37 +0200
Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:

> 0x1200 */ diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 27d3b767..e57946f0 100644
[...]
> @@ -30,7 +27,7 @@ void expect_pgm_int(void)
>  
>  void expect_ext_int(void)
>  {
> -	ext_int_expected = true;
> +	lc->ext_int_expected = 1;

External Interrupts can be floating; so if I am not mistaken it could be delivered to a different CPU which didn't expect it.

[...]
> @@ -237,17 +231,17 @@ void handle_io_int(void)
>  
>  int register_io_int_func(void (*f)(void))
>  {
> -	if (io_int_func)
> +	if (lc->io_int_func)
>  		return -1;

IO interrupts also are floating. Same concern as for the external interrupts.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks
  2022-06-07 14:23   ` Nico Boehr
@ 2022-06-07 14:41     ` Claudio Imbrenda
  2022-06-07 14:48       ` Nico Boehr
  0 siblings, 1 reply; 11+ messages in thread
From: Claudio Imbrenda @ 2022-06-07 14:41 UTC (permalink / raw)
  To: Nico Boehr; +Cc: kvm, linux-s390, frankja, scgl, pmorel, thuth

On Tue, 7 Jun 2022 16:23:09 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> On Fri,  3 Jun 2022 17:40:37 +0200
> Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:
> 
> > 0x1200 */ diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> > index 27d3b767..e57946f0 100644  
> [...]
> > @@ -30,7 +27,7 @@ void expect_pgm_int(void)
> >  
> >  void expect_ext_int(void)
> >  {
> > -	ext_int_expected = true;
> > +	lc->ext_int_expected = 1;  
> 
> External Interrupts can be floating; so if I am not mistaken it could be delivered to a different CPU which didn't expect it.

yes I have considered that (maybe I should add this in the patch
description)

for floating interrupts, the testcase should take care to mask the
interrupt on the CPUs where it should not be received.

by default all interrupts are masked anyway and CPUs should only enable
them on a case by case basis

> 
> [...]
> > @@ -237,17 +231,17 @@ void handle_io_int(void)
> >  
> >  int register_io_int_func(void (*f)(void))
> >  {
> > -	if (io_int_func)
> > +	if (lc->io_int_func)
> >  		return -1;  
> 
> IO interrupts also are floating. Same concern as for the external interrupts.

same here

the alternative is to have a separate handling of floating vs
non-floating interrupts, which maybe would get a little out of hand

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks
  2022-06-07 14:41     ` Claudio Imbrenda
@ 2022-06-07 14:48       ` Nico Boehr
  2022-06-07 16:43         ` Claudio Imbrenda
  0 siblings, 1 reply; 11+ messages in thread
From: Nico Boehr @ 2022-06-07 14:48 UTC (permalink / raw)
  To: Claudio Imbrenda; +Cc: kvm, linux-s390, frankja, scgl, pmorel, thuth

On Tue, 7 Jun 2022 16:41:13 +0200
Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:

> yes I have considered that (maybe I should add this in the patch
> description)

Yes, and not just that; maybe rename expect_ext_int to expect_ext_int_on_this_cpu, same for register_io_int_func.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks
  2022-06-07 14:48       ` Nico Boehr
@ 2022-06-07 16:43         ` Claudio Imbrenda
  0 siblings, 0 replies; 11+ messages in thread
From: Claudio Imbrenda @ 2022-06-07 16:43 UTC (permalink / raw)
  To: Nico Boehr; +Cc: kvm, linux-s390, frankja, scgl, pmorel, thuth

On Tue, 7 Jun 2022 16:48:57 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> On Tue, 7 Jun 2022 16:41:13 +0200
> Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:
> 
> > yes I have considered that (maybe I should add this in the patch
> > description)  
> 
> Yes, and not just that; maybe rename expect_ext_int to expect_ext_int_on_this_cpu, same for register_io_int_func.

fair enough

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks
  2022-06-03 15:40 ` [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks Claudio Imbrenda
  2022-06-07 10:01   ` Janis Schoetterl-Glausch
  2022-06-07 14:23   ` Nico Boehr
@ 2022-06-10  9:43   ` Janosch Frank
  2 siblings, 0 replies; 11+ messages in thread
From: Janosch Frank @ 2022-06-10  9:43 UTC (permalink / raw)
  To: Claudio Imbrenda, kvm; +Cc: linux-s390, scgl, pmorel, nrb, thuth

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 */
> +	uint8_t		pad_0x03b8[0x11b0 - 0x03b8];	/* 0x03b8 */

Before we directly pollute the lowcore I'd much rather have a pointer to 
a struct. We could then either use any area of the lowcore by adding a 
union or we extend the SMP lib per-cpu structs.

I don't want to have to review offset calculations for every change of 
per-cpu data. They are just way too easy to get wrong.



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-06-10  9:43 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-03 15:40 [kvm-unit-tests PATCH v1 0/2] better smp interrupt checks Claudio Imbrenda
2022-06-03 15:40 ` [kvm-unit-tests PATCH v1 1/2] s390x: skey.c: rework the interrupt handler Claudio Imbrenda
2022-06-07 13:29   ` Nico Boehr
2022-06-03 15:40 ` [kvm-unit-tests PATCH v1 2/2] lib: s390x: better smp interrupt checks Claudio Imbrenda
2022-06-07 10:01   ` Janis Schoetterl-Glausch
2022-06-07 11:08     ` Claudio Imbrenda
2022-06-07 14:23   ` Nico Boehr
2022-06-07 14:41     ` Claudio Imbrenda
2022-06-07 14:48       ` Nico Boehr
2022-06-07 16:43         ` Claudio Imbrenda
2022-06-10  9:43   ` Janosch Frank

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.