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... -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
