On Wed, Nov 19 2014 at 02:28:10 AM, Shannon Zhao <zhaoshenglong@xxxxxxxxxx> wrote:
> On 2014/11/18 18:21, Marc Zyngier wrote:
>> On 18/11/14 10:12, Shannon Zhao wrote:
>>> When call kvm_vgic_inject_irq to inject irq, we can known which vcpu
>>> the IRQ for by the irq_num and the cpuid. So we should just kick
>>> this vcpu to avoid iterating through all.
>>>
>>> Signed-off-by: Shannon Zhao <zhaoshenglong@xxxxxxxxxx>
>>> ---
>>> virt/kvm/arm/vgic.c | 32 ++++++++++++++++++++++++--------
>>> 1 files changed, 24 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>> index 82db9d6..9dad67a 100644
>>> --- a/virt/kvm/arm/vgic.c
>>> +++ b/virt/kvm/arm/vgic.c
>>> @@ -95,6 +95,7 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
>>> static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
>>> static void vgic_update_state(struct kvm *kvm);
>>> static void vgic_kick_vcpus(struct kvm *kvm);
>>> +static void vgic_kick_vcpu(struct kvm *kvm, int cpuid);
>>> static u8 *vgic_get_sgi_sources(struct vgic_dist *dist, int vcpu_id, int sgi);
>>> static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg);
>>> static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
>>> @@ -1589,6 +1590,21 @@ static void vgic_kick_vcpus(struct kvm *kvm)
>>> }
>>> }
>>>
>>> +static void vgic_kick_vcpu(struct kvm *kvm, int cpuid)
>>> +{
>>> + struct kvm_vcpu *vcpu;
>>> +
>>> + /*
>>> + * We've injected an interrupt, kick the specified vcpu
>>> + */
>>> + if (cpuid < atomic_read(&kvm->online_vcpus)) {
>>> + vcpu = kvm_get_vcpu(kvm, cpuid);
>>> + if (vcpu != NULL) {
>>> + kvm_vcpu_kick(vcpu);
>>> + }
>>> + }
>>
>> Why do you need all these checks? Why would cpuid be wrong at any time
>> here, given that we've extracted it from supposedly valid information?
>> Can you give an example of such a situation?
>>
>
> Sorry, thanks for your indication.
>
> I considered the situation that guest offline one vcpu which a SPI bind to.
> I didn't think clearly before and I have tested at this situation.
> It's an overprotection.
>
> So remove the "vgic_kick_vcpu" and just call "kvm_vcpu_kick" in "kvm_vgic_inject_irq" ?
>
> int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
> bool level)
> {
> if (likely(vgic_initialized(kvm)) &&
> vgic_update_irq_pending(kvm, &cpuid, irq_num, level))
> /*
> * We've injected an interrupt, kick the specified vcpu
> */
> kvm_vcpu_kick(kvm_get_vcpu(kvm, cpuid));
>
> return 0;
> }
That would be a lot better. You may also want to rethink the way you use
cpuid in vgic_update_irq_pending() so that you only update the returned
value on the positive path (that should make the patch a lot smaller),
Thanks,
M.
--
Jazz is not dead. It just smells funny.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
