Re: [kvm-unit-tests PATCH v5 2/6] s390x: add function to set DAT mode for all interrupts

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

On Thu, 13 Jul 2023 09:17:28 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 12/07/2023 13.41, Nico Boehr wrote:
> > When toggling DAT or switch address space modes, it is likely that
> > interrupts should be handled in the same DAT or address space mode.
> > 
> > Add a function which toggles DAT and address space mode for all
> > interruptions, except restart interrupts.
> > 
> > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> > ---
> >   lib/s390x/asm/interrupt.h |  4 ++++
> >   lib/s390x/interrupt.c     | 36 ++++++++++++++++++++++++++++++++++++
> >   lib/s390x/mmu.c           |  5 +++--
> >   3 files changed, 43 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> > index 35c1145f0349..55759002dce2 100644
> > --- a/lib/s390x/asm/interrupt.h
> > +++ b/lib/s390x/asm/interrupt.h
> > @@ -83,6 +83,10 @@ void expect_ext_int(void);
> >   uint16_t clear_pgm_int(void);
> >   void check_pgm_int_code(uint16_t code);
> >   
> > +#define IRQ_DAT_ON	true
> > +#define IRQ_DAT_OFF	false  
> 
> Just a matter of taste, but IMHO having defines like this for just using 
> them as boolean parameter to one function is a little bit overkill already. 
> I'd rather rename the "bool dat" below into "bool use_dat" and then use 
> "true" and "false" directly as a parameter for that function instead. 
> Anyway, just my 0.02 €.

+1

(or an enum, but I think a bool would be better, since it's a boolean
value)

[...]

> > + * and egg situation.
> > + *
> > + * @dat specifies whether to use DAT or not
> > + * @as specifies the address space mode to use - one of AS_PRIM, AS_ACCR,
> > + * AS_SECN or AS_HOME.
> > + */
> > +void irq_set_dat_mode(bool dat, uint64_t as)  
> 
> why uint64_t for "as" ? "int" should be enough?

or even a char ;)

> 
> (alternatively, you could turn the AS_* defines into a properly named enum 
> and use that type here instead)

I like this more ^

> 
>   Thomas
> 
> > +{
> > +	struct psw* irq_psws[] = {
> > +		OPAQUE_PTR(GEN_LC_EXT_NEW_PSW),
> > +		OPAQUE_PTR(GEN_LC_SVC_NEW_PSW),
> > +		OPAQUE_PTR(GEN_LC_PGM_NEW_PSW),
> > +		OPAQUE_PTR(GEN_LC_MCCK_NEW_PSW),
> > +		OPAQUE_PTR(GEN_LC_IO_NEW_PSW),
> > +	};
> > +	struct psw *psw;
> > +
> > +	assert(as == AS_PRIM || as == AS_ACCR || as == AS_SECN || as == AS_HOME);
> > +
> > +	for (size_t i = 0; i < ARRAY_SIZE(irq_psws); i++) {
> > +		psw = irq_psws[i];
> > +		psw->dat = dat;
> > +		if (dat)
> > +			psw->as = as;
> > +	}
> > +}
> > +
> >   static void fixup_pgm_int(struct stack_frame_int *stack)
> >   {
> >   	/* If we have an error on SIE we directly move to sie_exit */
> > diff --git a/lib/s390x/mmu.c b/lib/s390x/mmu.c
> > index b474d7021d3f..199bd3fbc9c8 100644
> > --- a/lib/s390x/mmu.c
> > +++ b/lib/s390x/mmu.c
> > @@ -12,6 +12,7 @@
> >   #include <asm/pgtable.h>
> >   #include <asm/arch_def.h>
> >   #include <asm/barrier.h>
> > +#include <asm/interrupt.h>
> >   #include <vmalloc.h>
> >   #include "mmu.h"
> >   
> > @@ -41,8 +42,8 @@ static void mmu_enable(pgd_t *pgtable)
> >   	/* enable dat (primary == 0 set as default) */
> >   	enable_dat();
> >   
> > -	/* we can now also use DAT unconditionally in our PGM handler */
> > -	lowcore.pgm_new_psw.mask |= PSW_MASK_DAT;
> > +	/* we can now also use DAT in all interrupt handlers */
> > +	irq_set_dat_mode(IRQ_DAT_ON, AS_PRIM);
> >   }
> >   
> >   /*  
>