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

Groups > linux.kernel > #1721874 > unrolled thread

Re: [PATCH v3 53/59] KVM: arm/arm64: GICv4: Hook vPE scheduling into vgic flush/sync

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 53/59] KVM: arm/arm64: GICv4: Hook vPE scheduling into  vgic flush/sync Christoffer Dall <cdall@linaro.org> - 2017-08-28 20:20 +0200
    Re: [PATCH v3 53/59] KVM: arm/arm64: GICv4: Hook vPE scheduling into  vgic flush/sync Marc Zyngier <marc.zyngier@arm.com> - 2017-08-30 12:00 +0200
      Re: [PATCH v3 53/59] KVM: arm/arm64: GICv4: Hook vPE scheduling into  vgic flush/sync Christoffer Dall <cdall@linaro.org> - 2017-08-30 14:00 +0200

#1721874 — Re: [PATCH v3 53/59] KVM: arm/arm64: GICv4: Hook vPE scheduling into vgic flush/sync

On Mon, Jul 31, 2017 at 06:26:31PM +0100, Marc Zyngier wrote:
> The redistributor needs to be told which vPE is about to be run,
> and tells us whether there is any pending VLPI on exit.
> 
> Let's add the scheduling calls to the vgic flush/sync functions,
> allowing the VLPIs to be delivered to the guest.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-v4.c | 24 ++++++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic.c    |  4 ++++
>  virt/kvm/arm/vgic/vgic.h    |  1 +
>  3 files changed, 29 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
> index 50721c4e3da5..0a8deefbcf1c 100644
> --- a/virt/kvm/arm/vgic/vgic-v4.c
> +++ b/virt/kvm/arm/vgic/vgic-v4.c
> @@ -119,6 +119,30 @@ void vgic_v4_teardown(struct kvm *kvm)
>  	its_vm->vpes = NULL;
>  }
>  
> +int vgic_v4_schedule(struct kvm_vcpu *vcpu, bool on)
> +{
> +	int irq = vcpu->arch.vgic_cpu.vgic_v3.its_vpe.irq;
> +
> +	if (!vgic_is_v4_capable(vcpu->kvm) || !irq)
> +		return 0;

why do we need to check the its_vpe.irq here?  This check is certainly
not untuitive, as I don't understand what happened on a v4 capable
system that somehow failed.  Is it because a specific VM is configured
to not use VLPIs, or?

> +
> +	/*
> +	 * Before making the VPE resident, make sure the redistributor
> +	 * expects us here.
> +	 */
> +	if (on) {
> +		int err;
> +
> +		err = irq_set_affinity(irq, cpumask_of(smp_processor_id()));

This is pretty unintuitive, and coming here without having read your
documentation may make people completely puzzled.  Could we provide a
pointer to the documentation that explains how the vpe irq hooks this
all together?

> +		if (err) {
> +			kvm_err("failed irq_set_affinity IRQ%d (%d)\n", irq, err);
> +			return err;
> +		}
> +	}
> +
> +	return its_schedule_vpe(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe, on);
> +}
> +

I'd prefer this function be split into two and follow the vgic notation
of having a flush and a sync function.

>  static struct vgic_its *vgic_get_its(struct kvm *kvm,
>  				     struct kvm_kernel_irq_routing_entry *irq_entry)
>  {
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index dfac894f6f03..9ab52108989d 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -721,6 +721,8 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
>  {
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  
> +	WARN_ON(vgic_v4_schedule(vcpu, false));
> +

This is in the critical path, so would it be worth considering a static
key to cater for non-GICv4 systems here?

>  	/* An empty ap_list_head implies used_lrs == 0 */
>  	if (list_empty(&vcpu->arch.vgic_cpu.ap_list_head))
>  		return;
> @@ -733,6 +735,8 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
>  /* Flush our emulation state into the GIC hardware before entering the guest. */
>  void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
>  {
> +	WARN_ON(vgic_v4_schedule(vcpu, true));
> +
>  	/*
>  	 * If there are no virtual interrupts active or pending for this
>  	 * VCPU, then there is no work to do and we can bail out without
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 1210bf4681dc..693b654acf4d 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -234,5 +234,6 @@ int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
>  bool vgic_is_v4_capable(struct kvm *kvm);
>  int vgic_v4_init(struct kvm *kvm);
>  void vgic_v4_teardown(struct kvm *kvm);
> +int vgic_v4_schedule(struct kvm_vcpu *vcpu, bool on);
>  
>  #endif
> -- 
> 2.11.0
> 
Functionally, this looks correct.

Thanks,
-Christoffer

[toc] | [next] | [standalone]

#1723175

On 28/08/17 19:17, Christoffer Dall wrote:
> On Mon, Jul 31, 2017 at 06:26:31PM +0100, Marc Zyngier wrote:
>> The redistributor needs to be told which vPE is about to be run,
>> and tells us whether there is any pending VLPI on exit.
>>
>> Let's add the scheduling calls to the vgic flush/sync functions,
>> allowing the VLPIs to be delivered to the guest.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-v4.c | 24 ++++++++++++++++++++++++
>>  virt/kvm/arm/vgic/vgic.c    |  4 ++++
>>  virt/kvm/arm/vgic/vgic.h    |  1 +
>>  3 files changed, 29 insertions(+)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
>> index 50721c4e3da5..0a8deefbcf1c 100644
>> --- a/virt/kvm/arm/vgic/vgic-v4.c
>> +++ b/virt/kvm/arm/vgic/vgic-v4.c
>> @@ -119,6 +119,30 @@ void vgic_v4_teardown(struct kvm *kvm)
>>  	its_vm->vpes = NULL;
>>  }
>>  
>> +int vgic_v4_schedule(struct kvm_vcpu *vcpu, bool on)
>> +{
>> +	int irq = vcpu->arch.vgic_cpu.vgic_v3.its_vpe.irq;
>> +
>> +	if (!vgic_is_v4_capable(vcpu->kvm) || !irq)
>> +		return 0;
> 
> why do we need to check the its_vpe.irq here?  This check is certainly
> not untuitive, as I don't understand what happened on a v4 capable
> system that somehow failed.  Is it because a specific VM is configured
> to not use VLPIs, or?

Hmm. I think that's a debug leftover from my early attempt at making
things work with QEMU, which initializes things in the opposite order
as kvmtool. It should be removed (or replaced by a fat WARN_ON).

>> +
>> +	/*
>> +	 * Before making the VPE resident, make sure the redistributor
>> +	 * expects us here.
>> +	 */
>> +	if (on) {
>> +		int err;
>> +
>> +		err = irq_set_affinity(irq, cpumask_of(smp_processor_id()));
> 
> This is pretty unintuitive, and coming here without having read your
> documentation may make people completely puzzled.  Could we provide a
> pointer to the documentation that explains how the vpe irq hooks this
> all together?

Sure, will do.

> 
>> +		if (err) {
>> +			kvm_err("failed irq_set_affinity IRQ%d (%d)\n", irq, err);
>> +			return err;
>> +		}
>> +	}
>> +
>> +	return its_schedule_vpe(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe, on);
>> +}
>> +
> 
> I'd prefer this function be split into two and follow the vgic notation
> of having a flush and a sync function.

Yes, makes sense.

>>  static struct vgic_its *vgic_get_its(struct kvm *kvm,
>>  				     struct kvm_kernel_irq_routing_entry *irq_entry)
>>  {
>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
>> index dfac894f6f03..9ab52108989d 100644
>> --- a/virt/kvm/arm/vgic/vgic.c
>> +++ b/virt/kvm/arm/vgic/vgic.c
>> @@ -721,6 +721,8 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
>>  {
>>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>  
>> +	WARN_ON(vgic_v4_schedule(vcpu, false));
>> +
> 
> This is in the critical path, so would it be worth considering a static
> key to cater for non-GICv4 systems here?

Hey, for once I wasn't trying to optimize early! ;-) This would be
useful indeed, as I expect GICv4 systems to be the absolute minority for
the foreseeable future.

> 
>>  	/* An empty ap_list_head implies used_lrs == 0 */
>>  	if (list_empty(&vcpu->arch.vgic_cpu.ap_list_head))
>>  		return;
>> @@ -733,6 +735,8 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
>>  /* Flush our emulation state into the GIC hardware before entering the guest. */
>>  void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
>>  {
>> +	WARN_ON(vgic_v4_schedule(vcpu, true));
>> +
>>  	/*
>>  	 * If there are no virtual interrupts active or pending for this
>>  	 * VCPU, then there is no work to do and we can bail out without
>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>> index 1210bf4681dc..693b654acf4d 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -234,5 +234,6 @@ int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
>>  bool vgic_is_v4_capable(struct kvm *kvm);
>>  int vgic_v4_init(struct kvm *kvm);
>>  void vgic_v4_teardown(struct kvm *kvm);
>> +int vgic_v4_schedule(struct kvm_vcpu *vcpu, bool on);
>>  
>>  #endif
>> -- 
>> 2.11.0
>>
> Functionally, this looks correct.
> 
> Thanks,
> -Christoffer
> 

Thanks,

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

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

#1723236

On Wed, Aug 30, 2017 at 10:59:46AM +0100, Marc Zyngier wrote:
> On 28/08/17 19:17, Christoffer Dall wrote:
> > On Mon, Jul 31, 2017 at 06:26:31PM +0100, Marc Zyngier wrote:
> >> The redistributor needs to be told which vPE is about to be run,
> >> and tells us whether there is any pending VLPI on exit.
> >>
> >> Let's add the scheduling calls to the vgic flush/sync functions,
> >> allowing the VLPIs to be delivered to the guest.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  virt/kvm/arm/vgic/vgic-v4.c | 24 ++++++++++++++++++++++++
> >>  virt/kvm/arm/vgic/vgic.c    |  4 ++++
> >>  virt/kvm/arm/vgic/vgic.h    |  1 +
> >>  3 files changed, 29 insertions(+)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
> >> index 50721c4e3da5..0a8deefbcf1c 100644
> >> --- a/virt/kvm/arm/vgic/vgic-v4.c
> >> +++ b/virt/kvm/arm/vgic/vgic-v4.c
> >> @@ -119,6 +119,30 @@ void vgic_v4_teardown(struct kvm *kvm)
> >>  	its_vm->vpes = NULL;
> >>  }
> >>  
> >> +int vgic_v4_schedule(struct kvm_vcpu *vcpu, bool on)
> >> +{
> >> +	int irq = vcpu->arch.vgic_cpu.vgic_v3.its_vpe.irq;
> >> +
> >> +	if (!vgic_is_v4_capable(vcpu->kvm) || !irq)
> >> +		return 0;
> > 
> > why do we need to check the its_vpe.irq here?  This check is certainly
> > not untuitive, as I don't understand what happened on a v4 capable
> > system that somehow failed.  Is it because a specific VM is configured
> > to not use VLPIs, or?
> 
> Hmm. I think that's a debug leftover from my early attempt at making
> things work with QEMU, which initializes things in the opposite order
> as kvmtool. It should be removed (or replaced by a fat WARN_ON).
> 
> >> +
> >> +	/*
> >> +	 * Before making the VPE resident, make sure the redistributor
> >> +	 * expects us here.
> >> +	 */
> >> +	if (on) {
> >> +		int err;
> >> +
> >> +		err = irq_set_affinity(irq, cpumask_of(smp_processor_id()));
> > 
> > This is pretty unintuitive, and coming here without having read your
> > documentation may make people completely puzzled.  Could we provide a
> > pointer to the documentation that explains how the vpe irq hooks this
> > all together?
> 
> Sure, will do.
> 
> > 
> >> +		if (err) {
> >> +			kvm_err("failed irq_set_affinity IRQ%d (%d)\n", irq, err);
> >> +			return err;
> >> +		}
> >> +	}
> >> +
> >> +	return its_schedule_vpe(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe, on);
> >> +}
> >> +
> > 
> > I'd prefer this function be split into two and follow the vgic notation
> > of having a flush and a sync function.
> 
> Yes, makes sense.
> 
> >>  static struct vgic_its *vgic_get_its(struct kvm *kvm,
> >>  				     struct kvm_kernel_irq_routing_entry *irq_entry)
> >>  {
> >> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> >> index dfac894f6f03..9ab52108989d 100644
> >> --- a/virt/kvm/arm/vgic/vgic.c
> >> +++ b/virt/kvm/arm/vgic/vgic.c
> >> @@ -721,6 +721,8 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
> >>  {
> >>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> >>  
> >> +	WARN_ON(vgic_v4_schedule(vcpu, false));
> >> +
> > 
> > This is in the critical path, so would it be worth considering a static
> > key to cater for non-GICv4 systems here?
> 
> Hey, for once I wasn't trying to optimize early! ;-) This would be
> useful indeed, as I expect GICv4 systems to be the absolute minority for
> the foreseeable future.
> 

Right.  For the record, I don't mind getting this functional first, and
adding the static key later, as you prefer.

Thanks,
-Christoffer

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web