Re: [PATCH v4 18/21] KVM: ARM64: Add PMU overflow interrupt routing

[Marc Zyngier <marc.zyngier@arm.com>]

On 02/12/15 09:49, Shannon Zhao wrote:
> 
> 
> On 2015/12/2 16:45, Marc Zyngier wrote:
>> On 02/12/15 02:40, Shannon Zhao wrote:
>>>>
>>>>
>>>> On 2015/12/2 0:57, Marc Zyngier wrote:
>>>>>> On 01/12/15 16:26, Shannon Zhao wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2015/12/1 23:41, Marc Zyngier wrote:
>>>>>>>>>>>> The reason is that when guest clear the overflow register, it will trap
>>>>>>>>>>>>>> to kvm and call kvm_pmu_sync_hwstate() as you see above. At this moment,
>>>>>>>>>>>>>> the overflow register is still overflowed(that is some bit is still 1).
>>>>>>>>>>>>>> So We need to use some flag to mark we already inject this interrupt.
>>>>>>>>>>>>>> And if during guest handling the overflow, there is a new overflow
>>>>>>>>>>>>>> happening, the pmu->irq_pending will be set ture by
>>>>>>>>>>>>>> kvm_pmu_perf_overflow(), then it needs to inject this new interrupt, right?
>>>>>>>>>> I don't think so. This is a level interrupt, so the level should stay
>>>>>>>>>> high as long as the guest hasn't cleared all possible sources for that
>>>>>>>>>> interrupt.
>>>>>>>>>>
>>>>>>>>>> For your example, the guest writes to PMOVSCLR to clear the overflow
>>>>>>>>>> caused by a given counter. If the status is now 0, the interrupt line
>>>>>>>>>> drops. If the status is still non zero, the line stays high. And I
>>>>>>>>>> believe that writing a 1 to PMOVSSET would actually trigger an
>>>>>>>>>> interrupt, or keep it high if it has already high.
>>>>>>>>>>
>>>>>>>> Right, writing 1 to PMOVSSET will trigger an interrupt.
>>>>>>>>
>>>>>>>>>> In essence, do not try to maintain side state. I've been bitten.
>>>>>>>>
>>>>>>>> So on VM entry, it check if PMOVSSET is zero. If not, call 
>>>>>>>> kvm_vgic_inject_irq to set the level high. If so, set the level low.
>>>>>>>> On VM exit, it seems there is nothing to do.
>>>>>>
>>>>>> It is even simpler than that:
>>>>>>
>>>>>> - When you get an overflow, you inject an interrupt with the level set to 1.
>>>>>> - When the overflow register gets cleared, you inject the same interrupt
>>>>>> with the level set to 0.
>>>>>>
>>>>>> I don't think you need to do anything else, and the world switch should
>>>>>> be left untouched.
>>>>>>
>>>>
>>>> On 2015/7/17 23:28, Christoffer Dall wrote:>> > +		
>>>> kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
>>>>>>>>>> +					    pmu->irq_num, 1);
>>>>>> what context is this overflow handler function?  kvm_vgic_inject_irq
>>>>>> grabs a mutex, so it can sleep...
>>>>>>
>>>>>> from a quick glance at the perf core code, it looks like this is in
>>>>>> interrupt context, so that call to kvm_vgic_inject_irq looks bad.
>>>>>>
>>>>
>>>> But as Christoffer said before, it's not good to call
>>>> kvm_vgic_inject_irq directly in interrupt context. So if we just kick
>>>> the vcpu here and call kvm_vgic_inject_irq on VM entry, is this fine?
>> Possibly. I'm slightly worried that inject_irq itself is going to kick
>> the vcpu again for no good reason. 
> Yes, this will introduce a extra kick. What's the impact of kicking a
> kicked vcpu?

As long as you only kick yourself, it shouldn't be much (trying to
decipher vcpu_kick).

>> I guess we'll find out (and maybe
>> we'll add a kvm_vgic_inject_irq_no_kick_please() helper...).
> And add a parameter "bool kick" for vgic_update_irq_pending ?

Given that we're completely rewriting the thing, I'd rather not add more
hacks to it if we can avoid it.

Give it a go, and we'll find out!

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...