diff mbox series

[kvm-unit-tests,v6,08/10] s390x: define wfi: wait for interrupt

Message ID 1587725152-25569-9-git-send-email-pmorel@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390x: Testing the Channel Subsystem I/O | expand

Commit Message

Pierre Morel April 24, 2020, 10:45 a.m. UTC
wfi(irq_mask) allows the programm to wait for an interrupt.
The interrupt handler is in charge to remove the WAIT bit
when it finished handling interrupt.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Janosch Frank April 27, 2020, 12:59 p.m. UTC | #1
On 4/24/20 12:45 PM, Pierre Morel wrote:
> wfi(irq_mask) allows the programm to wait for an interrupt.

s/programm/program/

> The interrupt handler is in charge to remove the WAIT bit
> when it finished handling interrupt.

...finished handling the interrupt.

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index a0d2362..e04866c 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -13,6 +13,7 @@
>  #define PSW_MASK_EXT			0x0100000000000000UL
>  #define PSW_MASK_DAT			0x0400000000000000UL
>  #define PSW_MASK_SHORT_PSW		0x0008000000000000UL
> +#define PSW_MASK_WAIT			0x0002000000000000UL
>  #define PSW_MASK_PSTATE			0x0001000000000000UL
>  #define PSW_MASK_BA			0x0000000080000000UL
>  #define PSW_MASK_EA			0x0000000100000000UL
> @@ -254,6 +255,16 @@ static inline void load_psw_mask(uint64_t mask)
>  		: "+r" (tmp) :  "a" (&psw) : "memory", "cc" );
>  }
>  
> +static inline void wfi(uint64_t irq_mask)

enabled_wait()

> +{
> +	uint64_t psw_mask;

You can directly initialize this variable.

> +
> +	psw_mask = extract_psw_mask();
> +	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);

Maybe add a comment here:

/*
 * After being woken and having processed the interrupt, let's restore
the PSW mask.
*/

> +	load_psw_mask(psw_mask);
> +}
> +
> +
>  static inline void enter_pstate(void)
>  {
>  	uint64_t mask;
>
Pierre Morel April 28, 2020, 8:44 a.m. UTC | #2
On 2020-04-27 14:59, Janosch Frank wrote:
> On 4/24/20 12:45 PM, Pierre Morel wrote:
>> wfi(irq_mask) allows the programm to wait for an interrupt.
> 
> s/programm/program/

Thx,

> 
>> The interrupt handler is in charge to remove the WAIT bit
>> when it finished handling interrupt.
> 
> ...finished handling the interrupt.

OK, thx

> 

>>   }
>>   
>> +static inline void wfi(uint64_t irq_mask)
> 
> enabled_wait()


I do not like enabled_wait(), we do not know what is enabled and we do 
not know what we are waiting for.

What about wait_for_interrupt()

> 
>> +{
>> +	uint64_t psw_mask;
> 
> You can directly initialize this variable.
> 
>> +
>> +	psw_mask = extract_psw_mask();
>> +	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);
> 
> Maybe add a comment here:
> 
> /*
>   * After being woken and having processed the interrupt, let's restore
> the PSW mask.
> */
> 
>> +	load_psw_mask(psw_mask);
>> +}
>> +

I can do this, but wasn't it obvious?


Regards,
Pierre
Janosch Frank April 28, 2020, 9:20 a.m. UTC | #3
On 4/28/20 10:44 AM, Pierre Morel wrote:
> 
> 
> On 2020-04-27 14:59, Janosch Frank wrote:
>> On 4/24/20 12:45 PM, Pierre Morel wrote:
>>> wfi(irq_mask) allows the programm to wait for an interrupt.
>>
>> s/programm/program/
> 
> Thx,
> 
>>
>>> The interrupt handler is in charge to remove the WAIT bit
>>> when it finished handling interrupt.
>>
>> ...finished handling the interrupt.
> 
> OK, thx
> 
>>
> 
>>>   }
>>>   
>>> +static inline void wfi(uint64_t irq_mask)
>>
>> enabled_wait()
> 
> 
> I do not like enabled_wait(), we do not know what is enabled and we do 
> not know what we are waiting for.
> 
> What about wait_for_interrupt()

As long as it's not called wfi...

> 
>>
>>> +{
>>> +	uint64_t psw_mask;
>>
>> You can directly initialize this variable.
>>
>>> +
>>> +	psw_mask = extract_psw_mask();
>>> +	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);
>>
>> Maybe add a comment here:
>>
>> /*
>>   * After being woken and having processed the interrupt, let's restore
>> the PSW mask.
>> */
>>
>>> +	load_psw_mask(psw_mask);
>>> +}
>>> +
> 
> I can do this, but wasn't it obvious?

It took me a minute, so it will take even longer for developers that are
not yet familiar with s390 kernel development.

> 
> 
> Regards,
> Pierre
>
Pierre Morel April 28, 2020, 9:27 a.m. UTC | #4
On 2020-04-28 11:20, Janosch Frank wrote:
> On 4/28/20 10:44 AM, Pierre Morel wrote:
>>
>>
>> On 2020-04-27 14:59, Janosch Frank wrote:
>>> On 4/24/20 12:45 PM, Pierre Morel wrote:
>>>> wfi(irq_mask) allows the programm to wait for an interrupt.
>>>
>>> s/programm/program/
>>
>> Thx,
>>
>>>
>>>> The interrupt handler is in charge to remove the WAIT bit
>>>> when it finished handling interrupt.
>>>
>>> ...finished handling the interrupt.
>>
>> OK, thx
>>
>>>
>>
>>>>    }
>>>>    
>>>> +static inline void wfi(uint64_t irq_mask)
>>>
>>> enabled_wait()
>>
>>
>> I do not like enabled_wait(), we do not know what is enabled and we do
>> not know what we are waiting for.
>>
>> What about wait_for_interrupt()
> 
> As long as it's not called wfi...
> 
>>
>>>
>>>> +{
>>>> +	uint64_t psw_mask;
>>>
>>> You can directly initialize this variable.
>>>
>>>> +
>>>> +	psw_mask = extract_psw_mask();
>>>> +	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);
>>>
>>> Maybe add a comment here:
>>>
>>> /*
>>>    * After being woken and having processed the interrupt, let's restore
>>> the PSW mask.
>>> */
>>>
>>>> +	load_psw_mask(psw_mask);
>>>> +}
>>>> +
>>
>> I can do this, but wasn't it obvious?
> 
> It took me a minute, so it will take even longer for developers that are
> not yet familiar with s390 kernel development.
> 
>>
>>
>> Regards,
>> Pierre
>>
> 
> 

OK, will do
Thx

Pierre
diff mbox series

Patch

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index a0d2362..e04866c 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -13,6 +13,7 @@ 
 #define PSW_MASK_EXT			0x0100000000000000UL
 #define PSW_MASK_DAT			0x0400000000000000UL
 #define PSW_MASK_SHORT_PSW		0x0008000000000000UL
+#define PSW_MASK_WAIT			0x0002000000000000UL
 #define PSW_MASK_PSTATE			0x0001000000000000UL
 #define PSW_MASK_BA			0x0000000080000000UL
 #define PSW_MASK_EA			0x0000000100000000UL
@@ -254,6 +255,16 @@  static inline void load_psw_mask(uint64_t mask)
 		: "+r" (tmp) :  "a" (&psw) : "memory", "cc" );
 }
 
+static inline void wfi(uint64_t irq_mask)
+{
+	uint64_t psw_mask;
+
+	psw_mask = extract_psw_mask();
+	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);
+	load_psw_mask(psw_mask);
+}
+
+
 static inline void enter_pstate(void)
 {
 	uint64_t mask;