Re: [PATCH] KVM: arm/arm64: don't set vtimer->cnt_ctl in kvm_arch_timer_handler

[Christoffer Dall]

On Fri, Dec 15, 2017 at 10:33:48AM +0000, Marc Zyngier wrote:
> On 15/12/17 10:10, Christoffer Dall wrote:
> > On Fri, Dec 15, 2017 at 09:09:05AM +0000, Marc Zyngier wrote:
> >> On 15/12/17 02:27, Jia He wrote:
> >>>
> >>>
> >>
> >> [...]
> >>
> >>>> @@ -367,6 +368,7 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu)
> >>>>   
> >>>>   	/* Disable the virtual timer */
> >>>>   	write_sysreg_el0(0, cntv_ctl);
> >>>> +	isb();
> >>> My only concern is whether this isb() is required here?
> >>> Sorryif this is a stupid question.I understand little about arm arch 
> >>> memory barrier. But seems isb will flush all the instruction prefetch.Do 
> >>> you think if an timer interrupt irq arrives, arm will use the previous 
> >>> instruction prefetch?
> >>
> >>
> >> This barrier has little to do with prefetch. It just guarantees that the
> >> code after the isb() is now running with a disabled virtual timer.
> >> Otherwise, a CPU can freely reorder the write_sysreg() until the next
> >> context synchronization event.
> >>
> >> An interrupt coming between the write and the barrier will also act as a
> >> context synchronization event. For more details, see the ARMv8 ARM (the
> >> glossary has a section on the concept).
> >>
> > 
> > So since an ISB doesn't guarantee that the timer will actually be
> > disabled, is it a waste of energy to have it, or should we keep it as a
> > best effort kind of thing?
> 
> nit: the ISB does offer that guarantee. It is just that the guarantee
> doesn't extend to an interrupt that has already been signalled.

right, I should have said that it doesn't guarantee that an already
signalled interrupt will have been retired prior to enabling interrupts
on the CPU again.

> 
> The main issue I have with not having an ISB is that it makes it harder
> to think of when the disabling actually happens. The disabling could be
> delayed for a very long time, and would make things harder to debug if
> they were going wrong.
> 

Yes, it definitely indicates the intention of the code and the flow, and
the fact that we have to work around delayed signals in the ISR is then
something we can describe with a comment there.

I'll add an ISB in the other version of the patch I sent before.

Thanks,
-Christoffer