Hi all,
On 13/08/2019 17:24, Marc Zyngier wrote:
> On Tue, 13 Aug 2019 16:44:21 +0100,
> Julien Grall <julien.grall@xxxxxxx> wrote:
>>
>> Hi Sebastian,
>>
>> On 8/13/19 1:58 PM, bigeasy@xxxxxxxxxxxxx wrote:
>>> On 2019-07-27 14:37:11 [+0100], Julien Grall wrote:
>>>>>> 8<------------
>>>>>> --- a/virt/kvm/arm/arch_timer.c
>>>>>> +++ b/virt/kvm/arm/arch_timer.c
>>>>>> @@ -80,7 +80,7 @@ static inline bool userspace_irqchip(str
>>>>>> static void soft_timer_start(struct hrtimer *hrt, u64 ns)
>>>>>> {
>>>>>> hrtimer_start(hrt, ktime_add_ns(ktime_get(), ns),
>>>>>> - HRTIMER_MODE_ABS);
>>>>>> + HRTIMER_MODE_ABS_HARD);
>>>>>> }
>>>>>
>>>>> That's pretty neat, and matches the patch you already have for
>>>>> x86. Feel free to add my
>>>>>
>>>>> Acked-by: Marc Zyngier <maz@xxxxxxxxxx>
>>>>
>>>> I can confirm the warning now disappeared. Feel free to added my tested-by:
>>>>
>>>> Tested-by: Julien Grall <julien.grall@xxxxxxx>
>>>>
>>>
>>> |kvm_hrtimer_expire()
>>> | kvm_timer_update_irq()
>>> | kvm_vgic_inject_irq()
>>> | vgic_lazy_init()
>>> | if (unlikely(!vgic_initialized(kvm))) {
>>> | if (kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V2)
>>> | return -EBUSY;
>>> |
>>> | mutex_lock(&kvm->lock);
>>>
>>> Is this possible path of any concern? This should throw a warning also
>>> for !RT so probably not…
>>
>> Hmmm, theoretically yes. In practice, it looks like the hrtimer will
>> not be started before kvm_vcpu_first_run_init() is called on the first
>> run.
>
> Exactly. Even if you restore the timer in a "firing" configuration,
> you'll have to execute the vgic init before any background timer gets
> programmed, let alone expired.
>
> Yes, the interface is terrible.
>
>> The function will call kvm_vgic_map_resources() which will initialize
>> the vgic if not already done.
>>
>> Looking around, I think this is here to cater the case where
>> KVM_IRQ_LINE is called before running.
>>
>> I am not yet familiar with the vgic, so I may have missed something.
>>
>>>
>>> I prepared the patch below. This one could go straight to tglx's timer tree
>>> since he has the _HARD bits there. I *think* it requires to set the bits
>>> _HARD during _init() and _start() otherwise there is (or was) a warning…
>>>
>>> Sebastian
>>> 8<------------
>>>
>>> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>>> Date: Tue, 13 Aug 2019 14:29:41 +0200
>>> Subject: [PATCH] KVM: arm/arm64: Let the timer expire in hardirq context on RT
>>>
>>> The timers are canceled from an preempt-notifier which is invoked with
>>> disabled preemption which is not allowed on PREEMPT_RT.
>>> The timer callback is short so in could be invoked in hard-IRQ context
>>> on -RT.
>>>
>>> Let the timer expire on hard-IRQ context even on -RT.
>>>
>>> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>>> Acked-by: Marc Zyngier <maz@xxxxxxxxxx>
>>> Tested-by: Julien Grall <julien.grall@xxxxxxx>
>>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
>>> ---
>>> virt/kvm/arm/arch_timer.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>>> index 1be486d5d7cb4..0bfa7c5b5c890 100644
>>> --- a/virt/kvm/arm/arch_timer.c
>>> +++ b/virt/kvm/arm/arch_timer.c
>>> @@ -80,7 +80,7 @@ static inline bool userspace_irqchip(struct kvm *kvm)
>>> static void soft_timer_start(struct hrtimer *hrt, u64 ns)
>>> {
>>> hrtimer_start(hrt, ktime_add_ns(ktime_get(), ns),
>>> - HRTIMER_MODE_ABS);
>>> + HRTIMER_MODE_ABS_HARD);
>>> }
>>> static void soft_timer_cancel(struct hrtimer *hrt)
>>> @@ -697,11 +697,11 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
>>> update_vtimer_cntvoff(vcpu, kvm_phys_timer_read());
>>> ptimer->cntvoff = 0;
>>> - hrtimer_init(&timer->bg_timer, CLOCK_MONOTONIC,
>>> HRTIMER_MODE_ABS);
>>> + hrtimer_init(&timer->bg_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_HARD);
>>> timer->bg_timer.function = kvm_bg_timer_expire;
>>> - hrtimer_init(&vtimer->hrtimer, CLOCK_MONOTONIC,
>>> HRTIMER_MODE_ABS);
>>> - hrtimer_init(&ptimer->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
>>> + hrtimer_init(&vtimer->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_HARD);
>>> + hrtimer_init(&ptimer->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_HARD);
>>> vtimer->hrtimer.function = kvm_hrtimer_expire;
>>> ptimer->hrtimer.function = kvm_hrtimer_expire;
>>>
>
> Patch looks fine, please add it to the pile of RT stuff! ;-)
Sadly, I managed to hit the same BUG_ON() today with this patch
applied on top v5.2-rt1-rebase. :/ Although, it is more difficult
to hit than previously.
[ 157.449545] 000: BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:968
[ 157.449569] 000: in_atomic(): 1, irqs_disabled(): 0, pid: 990, name: kvm-vcpu-1
[ 157.449579] 000: 2 locks held by kvm-vcpu-1/990:
[ 157.449592] 000: #0: 00000000c2fc8217 (&vcpu->mutex){+.+.}, at: kvm_vcpu_ioctl+0x70/0xae0
[ 157.449638] 000: #1: 0000000096863801 (&cpu_base->softirq_expiry_lock){+.+.}, at: hrtimer_grab_expiry_lock+0x24/0x40
[ 157.449677] 000: Preemption disabled at:
[ 157.449679] 000: [<ffff0000111a4538>] schedule+0x30/0xd8
[ 157.449702] 000: CPU: 0 PID: 990 Comm: kvm-vcpu-1 Tainted: G W 5.2.0-rt1-00001-gd368139e892f #104
[ 157.449712] 000: Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Jan 23 2017
[ 157.449718] 000: Call trace:
[ 157.449722] 000: dump_backtrace+0x0/0x130
[ 157.449730] 000: show_stack+0x14/0x20
[ 157.449738] 000: dump_stack+0xbc/0x104
[ 157.449747] 000: ___might_sleep+0x198/0x238
[ 157.449756] 000: rt_spin_lock+0x5c/0x70
[ 157.449765] 000: hrtimer_grab_expiry_lock+0x24/0x40
[ 157.449773] 000: hrtimer_cancel+0x1c/0x38
[ 157.449780] 000: kvm_timer_vcpu_load+0x78/0x3e0
[ 157.449791] 000: kvm_arch_vcpu_load+0x130/0x298
[ 157.449800] 000: kvm_sched_in+0x38/0x68
[ 157.449808] 000: finish_task_switch+0x14c/0x300
[ 157.449816] 000: __schedule+0x2b8/0x8d0
[ 157.449826] 000: schedule+0x38/0xd8
[ 157.449833] 000: kvm_vcpu_block+0xac/0x790
[ 157.449841] 000: kvm_handle_wfx+0x210/0x520
[ 157.449852] 000: handle_exit+0x134/0x1d0
[ 157.449861] 000: kvm_arch_vcpu_ioctl_run+0x650/0xbb8
[ 157.449869] 000: kvm_vcpu_ioctl+0x3a0/0xae0
[ 157.449877] 000: do_vfs_ioctl+0xbc/0x910
[ 157.449887] 000: ksys_ioctl+0x78/0xa8
[ 157.449896] 000: __arm64_sys_ioctl+0x1c/0x28
[ 157.449904] 000: el0_svc_common.constprop.0+0x90/0x188
[ 157.449915] 000: el0_svc_handler+0x28/0x78
[ 157.449925] 000: el0_svc+0x8/0xc
[ 173.521497] 002: hrtimer: interrupt took 19281 ns
I will do some debug and see what I can find.
Cheers,
--
Julien Grall
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
