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-14 08:44:10)
> On 13/07/2023 17.30, Nico Boehr wrote:
> > 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?
> 
> I see your point, but if it is clear from the function name like it is in 
> this case, I'd go with "true" and "false" directly, without the indirection 
> via #define.

OK, will do.

> ...
> >>> + * 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*
> 
> codespell (https://github.com/codespell-project/codespell) is your friend!

Thanks!




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux