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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Thomas Huth (2023-07-13 09:17:28)
> 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 \u20ac.

The point of having these defines was to convey the meaning of the parameter to my reader.

When I read

    irq_set_dat_mode(true, AS_HOME);

it's less clear to me that the first parameter toggles the DAT mode compared to this:

    irq_set_dat_mode(IRQ_DAT_ON, AS_HOME);

That being said, here it's pretty clear from the function name what the first parameter is, so what's the preferred opinion?

[...]
> > diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> > index 3f993a363ae2..9b1bc6ce819d 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,41 @@ 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/

Argh, thanks.

*reprioritizes task to look for a spell checker*

> 
> > + * to initalize CRs, the restart new PSW is never touched to avoid the chicken
> 
> dito
> 
> > + * 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?
> 
> (alternatively, you could turn the AS_* defines into a properly named enum 
> and use that type here instead)

Yes, let's just do that.




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux