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

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

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