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

Thomas Huth
2023-07-14T06:45:19Z

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@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 \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.

...
>>> + * 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!

Thomas