Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1721867 > unrolled thread

Re: [PATCH v3 51/59] KVM: arm/arm64: GICv4: Add doorbell interrupt handling

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 51/59] KVM: arm/arm64: GICv4: Add doorbell interrupt  handling Christoffer Dall <cdall@linaro.org> - 2017-08-28 20:20 +0200
    Re: [PATCH v3 51/59] KVM: arm/arm64: GICv4: Add doorbell interrupt  handling Marc Zyngier <marc.zyngier@arm.com> - 2017-08-30 17:40 +0200
      Re: [PATCH v3 51/59] KVM: arm/arm64: GICv4: Add doorbell interrupt  handling Christoffer Dall <cdall@linaro.org> - 2017-08-30 23:00 +0200
        Re: [PATCH v3 51/59] KVM: arm/arm64: GICv4: Add doorbell interrupt  handling Marc Zyngier <marc.zyngier@arm.com> - 2017-08-31 10:20 +0200

#1721867 — Re: [PATCH v3 51/59] KVM: arm/arm64: GICv4: Add doorbell interrupt handling

On Mon, Jul 31, 2017 at 06:26:29PM +0100, Marc Zyngier wrote:
> When a vPE is not running, a VLPI being made pending results in a
> doorbell interrupt being delivered. Let's handle this interrupt
> and update the pending_last flag that indicates that VLPIs are
> pending. The corresponding vcpu is also kicked into action.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-v4.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
> index 534d3051a078..6af3cde6d7d4 100644
> --- a/virt/kvm/arm/vgic/vgic-v4.c
> +++ b/virt/kvm/arm/vgic/vgic-v4.c
> @@ -21,6 +21,19 @@
>  
>  #include "vgic.h"
>  
> +static irqreturn_t vgic_v4_doorbell_handler(int irq, void *info)
> +{
> +	struct kvm_vcpu *vcpu = info;
> +
> +	if (!kvm_vgic_vcpu_pending_irq(vcpu)) {
> +		vcpu->arch.vgic_cpu.vgic_v3.its_vpe.pending_last = true;
> +		kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
> +		kvm_vcpu_kick(vcpu);
> +	}

Can this ever fire while vgic_v4_init() is running and before te rest of
the system has been properly initialized with some entertaining results
to follow?  (I'm not sure if spurious doorbell non-resident vPE
interrupts is a thing or not).

> +
> +	return IRQ_HANDLED;
> +}
> +
>  int vgic_v4_init(struct kvm *kvm)
>  {
>  	struct vgic_dist *dist = &kvm->arch.vgic;
> @@ -57,16 +70,37 @@ int vgic_v4_init(struct kvm *kvm)
>  		return ret;
>  	}
>  
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		int irq = dist->its_vm.vpes[i]->irq;
> +
> +		ret = request_irq(irq, vgic_v4_doorbell_handler,
> +				  0, "vcpu", vcpu);
> +		if (ret) {
> +			kvm_err("failed to allocate vcpu IRQ%d\n", irq);
> +			dist->its_vm.nr_vpes = i;

That's a neat trick for the error handling.  Might deserve a tiny
comment.

> +			break;
> +		}
> +	}
> +
> +	if (ret)
> +		vgic_v4_teardown(kvm);
> +
>  	return ret;
>  }
>  
>  void vgic_v4_teardown(struct kvm *kvm)
>  {
>  	struct its_vm *its_vm = &kvm->arch.vgic.its_vm;
> +	int i;
>  
>  	if (!its_vm->vpes)
>  		return;
>  
> +	for (i = 0; i < its_vm->nr_vpes; i++) {
> +		struct kvm_vcpu *vcpu = kvm_get_vcpu(kvm, i);
> +		free_irq(its_vm->vpes[i]->irq, vcpu);
> +	}
> +
>  	its_free_vcpu_irqs(its_vm);
>  	kfree(its_vm->vpes);
>  	its_vm->nr_vpes = 0;
> -- 
> 2.11.0
> 

Thanks,
-Christoffer

[toc] | [next] | [standalone]

#1723469

On 28/08/17 19:18, Christoffer Dall wrote:
> On Mon, Jul 31, 2017 at 06:26:29PM +0100, Marc Zyngier wrote:
>> When a vPE is not running, a VLPI being made pending results in a
>> doorbell interrupt being delivered. Let's handle this interrupt
>> and update the pending_last flag that indicates that VLPIs are
>> pending. The corresponding vcpu is also kicked into action.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-v4.c | 34 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
>> index 534d3051a078..6af3cde6d7d4 100644
>> --- a/virt/kvm/arm/vgic/vgic-v4.c
>> +++ b/virt/kvm/arm/vgic/vgic-v4.c
>> @@ -21,6 +21,19 @@
>>  
>>  #include "vgic.h"
>>  
>> +static irqreturn_t vgic_v4_doorbell_handler(int irq, void *info)
>> +{
>> +	struct kvm_vcpu *vcpu = info;
>> +
>> +	if (!kvm_vgic_vcpu_pending_irq(vcpu)) {
>> +		vcpu->arch.vgic_cpu.vgic_v3.its_vpe.pending_last = true;
>> +		kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
>> +		kvm_vcpu_kick(vcpu);
>> +	}
> 
> Can this ever fire while vgic_v4_init() is running and before te rest of
> the system has been properly initialized with some entertaining results
> to follow?  (I'm not sure if spurious doorbell non-resident vPE
> interrupts is a thing or not).

It could if you only had this patch. The following patch makes sure that
the interrupt does not get enabled at request time, meaning it will only
get enabled when the vcpu will eventually block.

And yes, spurious doorbells are a real thing. And they suck.

> 
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  int vgic_v4_init(struct kvm *kvm)
>>  {
>>  	struct vgic_dist *dist = &kvm->arch.vgic;
>> @@ -57,16 +70,37 @@ int vgic_v4_init(struct kvm *kvm)
>>  		return ret;
>>  	}
>>  
>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>> +		int irq = dist->its_vm.vpes[i]->irq;
>> +
>> +		ret = request_irq(irq, vgic_v4_doorbell_handler,
>> +				  0, "vcpu", vcpu);
>> +		if (ret) {
>> +			kvm_err("failed to allocate vcpu IRQ%d\n", irq);
>> +			dist->its_vm.nr_vpes = i;
> 
> That's a neat trick for the error handling.  Might deserve a tiny
> comment.

Ah, yes, will do.

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1723661

On Wed, Aug 30, 2017 at 04:36:06PM +0100, Marc Zyngier wrote:
> On 28/08/17 19:18, Christoffer Dall wrote:
> > On Mon, Jul 31, 2017 at 06:26:29PM +0100, Marc Zyngier wrote:
> >> When a vPE is not running, a VLPI being made pending results in a
> >> doorbell interrupt being delivered. Let's handle this interrupt
> >> and update the pending_last flag that indicates that VLPIs are
> >> pending. The corresponding vcpu is also kicked into action.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  virt/kvm/arm/vgic/vgic-v4.c | 34 ++++++++++++++++++++++++++++++++++
> >>  1 file changed, 34 insertions(+)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
> >> index 534d3051a078..6af3cde6d7d4 100644
> >> --- a/virt/kvm/arm/vgic/vgic-v4.c
> >> +++ b/virt/kvm/arm/vgic/vgic-v4.c
> >> @@ -21,6 +21,19 @@
> >>  
> >>  #include "vgic.h"
> >>  
> >> +static irqreturn_t vgic_v4_doorbell_handler(int irq, void *info)
> >> +{
> >> +	struct kvm_vcpu *vcpu = info;
> >> +
> >> +	if (!kvm_vgic_vcpu_pending_irq(vcpu)) {
> >> +		vcpu->arch.vgic_cpu.vgic_v3.its_vpe.pending_last = true;
> >> +		kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
> >> +		kvm_vcpu_kick(vcpu);
> >> +	}
> > 
> > Can this ever fire while vgic_v4_init() is running and before te rest of
> > the system has been properly initialized with some entertaining results
> > to follow?  (I'm not sure if spurious doorbell non-resident vPE
> > interrupts is a thing or not).
> 
> It could if you only had this patch. The following patch makes sure that
> the interrupt does not get enabled at request time, meaning it will only
> get enabled when the vcpu will eventually block.
> 
> And yes, spurious doorbells are a real thing. And they suck.
> 

Ah, my abilities to forward read on a patch series are quite poor.

Thanks,
-Christoffer

[toc] | [prev] | [next] | [standalone]

#1723977

On 30/08/17 21:58, Christoffer Dall wrote:
> On Wed, Aug 30, 2017 at 04:36:06PM +0100, Marc Zyngier wrote:
>> On 28/08/17 19:18, Christoffer Dall wrote:
>>> On Mon, Jul 31, 2017 at 06:26:29PM +0100, Marc Zyngier wrote:
>>>> When a vPE is not running, a VLPI being made pending results in a
>>>> doorbell interrupt being delivered. Let's handle this interrupt
>>>> and update the pending_last flag that indicates that VLPIs are
>>>> pending. The corresponding vcpu is also kicked into action.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  virt/kvm/arm/vgic/vgic-v4.c | 34 ++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 34 insertions(+)
>>>>
>>>> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
>>>> index 534d3051a078..6af3cde6d7d4 100644
>>>> --- a/virt/kvm/arm/vgic/vgic-v4.c
>>>> +++ b/virt/kvm/arm/vgic/vgic-v4.c
>>>> @@ -21,6 +21,19 @@
>>>>  
>>>>  #include "vgic.h"
>>>>  
>>>> +static irqreturn_t vgic_v4_doorbell_handler(int irq, void *info)
>>>> +{
>>>> +	struct kvm_vcpu *vcpu = info;
>>>> +
>>>> +	if (!kvm_vgic_vcpu_pending_irq(vcpu)) {
>>>> +		vcpu->arch.vgic_cpu.vgic_v3.its_vpe.pending_last = true;
>>>> +		kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
>>>> +		kvm_vcpu_kick(vcpu);
>>>> +	}
>>>
>>> Can this ever fire while vgic_v4_init() is running and before te rest of
>>> the system has been properly initialized with some entertaining results
>>> to follow?  (I'm not sure if spurious doorbell non-resident vPE
>>> interrupts is a thing or not).
>>
>> It could if you only had this patch. The following patch makes sure that
>> the interrupt does not get enabled at request time, meaning it will only
>> get enabled when the vcpu will eventually block.
>>
>> And yes, spurious doorbells are a real thing. And they suck.
>>
> 
> Ah, my abilities to forward read on a patch series are quite poor.
Not quite. It indicates that the patch split is a bit wrong, and that

	irq_set_status_flags(irq, IRQ_NOAUTOEN | IRQ_DISABLE_UNLAZY);

should really be in this patch and not the following one.

I'll fix that as I rebase the whole thing.

Thanks,

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web