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

Nico Boehr
2023-06-15T12:46:52Z

Quoting Janosch Frank (2023-06-05 10:42:40)
> On 6/1/23 09:01, Nico Boehr wrote:
> > When toggling DAT or switch address space modes, it is likely that
>
> s/switch/switching/

Done

[...]
> > diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> > index 3f993a363ae2..d97b5a3a7e97 100644
> > --- a/lib/s390x/interrupt.c
> > +++ b/lib/s390x/interrupt.c
> > @@ -9,6 +9,7 @@
> > */
> > #include <libcflat.h>
> > #include <asm/barrier.h>
> > +#include <asm/mem.h>
> > #include <asm/asm-offsets.h>
> > #include <sclp.h>
> > #include <interrupt.h>
> > @@ -104,6 +105,40 @@ void register_ext_cleanup_func(void (*f)(struct stack_frame_int *))
> > THIS_CPU->ext_cleanup_func = f;
> > }
> >
> > +/**
> > + * irq_set_dat_mode - Set the DAT mode of all interrupt handlers, except for
> > + * restart.
> > + * This will update the DAT mode and address space mode of all interrupt new
> > + * PSWs.
> > + *
> > + * Since enabling DAT needs initalized CRs and the restart new PSW is often used
>
> s/initalized/initialized/

fixed

> > +void irq_set_dat_mode(bool dat, uint64_t as)
> > +{
> > + 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),
> > + NULL /* sentinel */
> > + };
> > +
> > + assert(as == AS_PRIM || as == AS_ACCR || as == AS_SECN || as == AS_HOME);
>
> /* There are only 4 spaces */
> assert(as < 4); ?

Well you won't find that when when grepping for any of the AS_*, so no I don't
think that's better.

> > +
> > + for (struct psw *psw = irq_psws[0]; psw != NULL; psw++) {
>
> While this is ok in gnu99/c99 I generally prefer declaring the variable
> outside of the loop since it's more readable when using structs and unions.

OK.

>
> > + psw->dat = dat;
> > + if (dat)
> > + psw->as = as;
>
> Does that check even matter?

Well, when DAT is off, AS bacially becomes a don't care. I thought it's a good
idea to preserve the AS in this case, but this can certainly be argued about.

I updated the comment and clarified that AS is only touched if DAT is on. Let me
know if you prefer removal of the if.