On Thu, 13 Jul 2023 09:17:28 +0200
Thomas Huth <thuth@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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);
> > }
> >
> > /*
>
