RE: [PATCH v3 2/3] KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr()

[Pavel Fedin]

 Hello!

> > --- cut ---
> > Additionally, remove unnecessary vgic_set_lr() and LR_STATE_PENDING check
> > in vgic_unqueue_irqs(), because all these things are now done by the
> > following vgic_retire_lr().
> > --- cut ---
> 
> This does not explain the question I'm raising.
> 
> After applying this patch, and before applying your next patch,
> unqueueing an IRQ will not restore the pending state on the
> distributor, but just throw that piece of state away

 It will restore the state and not throw it away.
 I guess i'm just not clear enough and you misunderstand me. This check in 
vgic_unqueue_irqs() is redundant from the beginning.
Let's look at current vgic_retire_lr():
https://git.kernel.org/cgit/linux/kernel/git/kvmarm/kvmarm.git/tree/virt/kvm/arm/vgic.c?h=next#n1099
 It already does LR_STATE_PENDING check and pushback by itself, since 
cff9211eb1a1f58ce7f5a2d596b617928fd4be0e (it's your commit,
BTW), so that this check:
https://git.kernel.org/cgit/linux/kernel/git/kvmarm/kvmarm.git/tree/virt/kvm/arm/vgic.c?h=next#n728
 is already redundant. So actually this is a separate change, and perhaps it's 
my fault to squash it in.

> which breaks bisectability and makes it impossible to understand the logic by 
> looking
> at this commit in isolation.

 Will this be understood better if i make this particular refactor a separate 
commit, with better explanations?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr()

[Christoffer Dall]

On Mon, Oct 26, 2015 at 06:49:51PM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > >   /*
> > >* Reestablish the pending state on the distributor and the
> > > -  * CPU interface.  It may have already been pending, but that
> > > -  * is fine, then we are only setting a few bits that were
> > > -  * already set.
> > > -  */
> > > - if (lr.state & LR_STATE_PENDING) {
> > > - vgic_dist_irq_set_pending(vcpu, lr.irq);
> > 
> > this looks wrong: You should still be setting the pending state on the
> > distributor, perhaps this is an ordering issue with the last patch?
> 
>  No, explained in the commit msg:
> --- cut ---
> Additionally, remove unnecessary vgic_set_lr() and LR_STATE_PENDING check
> in vgic_unqueue_irqs(), because all these things are now done by the
> following vgic_retire_lr().
> --- cut ---

This does not explain the question I'm raising.

After applying this patch, and before applying your next patch,
unqueueing an IRQ will not restore the pending state on the
distributor, but just throw that piece of state away, which breaks
bisectability and makes it impossible to understand the logic by looking
at this commit in isolation.

Please maintain at least current working semantics on a patch by patch
basis unless it's absolutely impossible to do so.

That's what I meant with an ordering issue with your last patch to which
you so elegantly reply 'No'.

Please rework this.

-Christoffer

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 2/3] KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr()

[Pavel Fedin]

 Hello!

> > /*
> >  * Reestablish the pending state on the distributor and the
> > -* CPU interface.  It may have already been pending, but that
> > -* is fine, then we are only setting a few bits that were
> > -* already set.
> > -*/
> > -   if (lr.state & LR_STATE_PENDING) {
> > -   vgic_dist_irq_set_pending(vcpu, lr.irq);
> 
> this looks wrong: You should still be setting the pending state on the
> distributor, perhaps this is an ordering issue with the last patch?

 No, explained in the commit msg:
--- cut ---
Additionally, remove unnecessary vgic_set_lr() and LR_STATE_PENDING check
in vgic_unqueue_irqs(), because all these things are now done by the
following vgic_retire_lr().
--- cut ---
 The last patch touches vgic_irq_clear_queued(). LR_STATE_PENDING check was 
included in vgic_retire_lr() by
cff9211eb1a1f58ce7f5a2d596b617928fd4be0e ("arm/arm64: KVM: Fix arch timer 
behavior for disabled interrupts "), so i simply removed
duplicated code.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr()

[Christoffer Dall]

On Mon, Oct 26, 2015 at 12:00:29PM +0300, Pavel Fedin wrote:
> Now we see that vgic_set_lr() and vgic_sync_lr_elrsr() are always used
> together. Merge them into one function, saving from second vgic_ops
> dereferencing every time.
> 
> Additionally, remove unnecessary vgic_set_lr() and LR_STATE_PENDING check
> in vgic_unqueue_irqs(), because all these things are now done by the
> following vgic_retire_lr().
> 
> Signed-off-by: Pavel Fedin 
> ---
>  include/kvm/arm_vgic.h |  1 -
>  virt/kvm/arm/vgic-v2.c |  5 -
>  virt/kvm/arm/vgic-v3.c |  5 -
>  virt/kvm/arm/vgic.c| 33 -
>  4 files changed, 4 insertions(+), 40 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 3936bf8..f62addc 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -112,7 +112,6 @@ struct vgic_vmcr {
>  struct vgic_ops {
>   struct vgic_lr  (*get_lr)(const struct kvm_vcpu *, int);
>   void(*set_lr)(struct kvm_vcpu *, int, struct vgic_lr);
> - void(*sync_lr_elrsr)(struct kvm_vcpu *, int, struct vgic_lr);
>   u64 (*get_elrsr)(const struct kvm_vcpu *vcpu);
>   u64 (*get_eisr)(const struct kvm_vcpu *vcpu);
>   void(*clear_eisr)(struct kvm_vcpu *vcpu);
> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
> index c0f5d7f..ff02f08 100644
> --- a/virt/kvm/arm/vgic-v2.c
> +++ b/virt/kvm/arm/vgic-v2.c
> @@ -79,11 +79,7 @@ static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
>   lr_val |= (lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT);
>  
>   vcpu->arch.vgic_cpu.vgic_v2.vgic_lr[lr] = lr_val;
> -}
>  
> -static void vgic_v2_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
> -   struct vgic_lr lr_desc)
> -{
>   if (!(lr_desc.state & LR_STATE_MASK))
>   vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr |= (1ULL << lr);
>   else
> @@ -167,7 +163,6 @@ static void vgic_v2_enable(struct kvm_vcpu *vcpu)
>  static const struct vgic_ops vgic_v2_ops = {
>   .get_lr = vgic_v2_get_lr,
>   .set_lr = vgic_v2_set_lr,
> - .sync_lr_elrsr  = vgic_v2_sync_lr_elrsr,
>   .get_elrsr  = vgic_v2_get_elrsr,
>   .get_eisr   = vgic_v2_get_eisr,
>   .clear_eisr = vgic_v2_clear_eisr,
> diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
> index 92003cb..487d635 100644
> --- a/virt/kvm/arm/vgic-v3.c
> +++ b/virt/kvm/arm/vgic-v3.c
> @@ -112,11 +112,7 @@ static void vgic_v3_set_lr(struct kvm_vcpu *vcpu, int lr,
>   }
>  
>   vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[LR_INDEX(lr)] = lr_val;
> -}
>  
> -static void vgic_v3_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
> -   struct vgic_lr lr_desc)
> -{
>   if (!(lr_desc.state & LR_STATE_MASK))
>   vcpu->arch.vgic_cpu.vgic_v3.vgic_elrsr |= (1U << lr);
>   else
> @@ -212,7 +208,6 @@ static void vgic_v3_enable(struct kvm_vcpu *vcpu)
>  static const struct vgic_ops vgic_v3_ops = {
>   .get_lr = vgic_v3_get_lr,
>   .set_lr = vgic_v3_set_lr,
> - .sync_lr_elrsr  = vgic_v3_sync_lr_elrsr,
>   .get_elrsr  = vgic_v3_get_elrsr,
>   .get_eisr   = vgic_v3_get_eisr,
>   .clear_eisr = vgic_v3_clear_eisr,
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 265a410..43f2564 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -717,28 +717,13 @@ void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
>* interrupt then move the active state to the
>* distributor tracking bit.
>*/
> - if (lr.state & LR_STATE_ACTIVE) {
> + if (lr.state & LR_STATE_ACTIVE)
>   vgic_irq_set_active(vcpu, lr.irq);
> - lr.state &= ~LR_STATE_ACTIVE;
> - }
>  
>   /*
>* Reestablish the pending state on the distributor and the
> -  * CPU interface.  It may have already been pending, but that
> -  * is fine, then we are only setting a few bits that were
> -  * already set.
> -  */
> - if (lr.state & LR_STATE_PENDING) {
> - vgic_dist_irq_set_pending(vcpu, lr.irq);

this looks wrong: You should still be setting the pending state on the
distributor, perhaps this is an ordering issue with the last patch?

-Christoffer

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html