Re: [PATCH v1 11/55] powerpc/time: add API for KVM to re-arm the host timer/decrementer

[Nicholas Piggin <npiggin@gmail.com>]

Excerpts from Christophe Leroy's message of August 5, 2021 5:22 pm:
> 
> 
> Le 26/07/2021 à 05:49, Nicholas Piggin a écrit :
>> Rather than have KVM look up the host timer and fiddle with the
>> irq-work internal details, have the powerpc/time.c code provide a
>> function for KVM to re-arm the Linux timer code when exiting a
>> guest.
>> 
>> This is implementation has an improvement over existing code of
>> marking a decrementer interrupt as soft-pending if a timer has
>> expired, rather than setting DEC to a -ve value, which tended to
>> cause host timers to take two interrupts (first hdec to exit the
>> guest, then the immediate dec).
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   arch/powerpc/include/asm/time.h | 16 +++-------
>>   arch/powerpc/kernel/time.c      | 52 +++++++++++++++++++++++++++------
>>   arch/powerpc/kvm/book3s_hv.c    |  7 ++---
>>   3 files changed, 49 insertions(+), 26 deletions(-)
>> 
>> diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
>> index 69b6be617772..924b2157882f 100644
>> --- a/arch/powerpc/include/asm/time.h
>> +++ b/arch/powerpc/include/asm/time.h
>> @@ -99,18 +99,6 @@ extern void div128_by_32(u64 dividend_high, u64 dividend_low,
>>   extern void secondary_cpu_time_init(void);
>>   extern void __init time_init(void);
>>   
>> -#ifdef CONFIG_PPC64
>> -static inline unsigned long test_irq_work_pending(void)
>> -{
>> -	unsigned long x;
>> -
>> -	asm volatile("lbz %0,%1(13)"
>> -		: "=r" (x)
>> -		: "i" (offsetof(struct paca_struct, irq_work_pending)));
>> -	return x;
>> -}
>> -#endif
>> -
>>   DECLARE_PER_CPU(u64, decrementers_next_tb);
>>   
>>   static inline u64 timer_get_next_tb(void)
>> @@ -118,6 +106,10 @@ static inline u64 timer_get_next_tb(void)
>>   	return __this_cpu_read(decrementers_next_tb);
>>   }
>>   
>> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
>> +void timer_rearm_host_dec(u64 now);
>> +#endif
>> +
>>   /* Convert timebase ticks to nanoseconds */
>>   unsigned long long tb_to_ns(unsigned long long tb_ticks);
>>   
>> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
>> index 72d872b49167..016828b7401b 100644
>> --- a/arch/powerpc/kernel/time.c
>> +++ b/arch/powerpc/kernel/time.c
>> @@ -499,6 +499,16 @@ EXPORT_SYMBOL(profile_pc);
>>    * 64-bit uses a byte in the PACA, 32-bit uses a per-cpu variable...
>>    */
>>   #ifdef CONFIG_PPC64
>> +static inline unsigned long test_irq_work_pending(void)
>> +{
>> +	unsigned long x;
>> +
>> +	asm volatile("lbz %0,%1(13)"
>> +		: "=r" (x)
>> +		: "i" (offsetof(struct paca_struct, irq_work_pending)));
> 
> Can we just use READ_ONCE() instead of hard coding the read ?

Good question, probably yes. Probably calls for its own patch series, 
e.g., hw_irq.h has all similar paca accesses.

Thanks,
Nick