Message ID | 20210127121337.1092-4-lushenming@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: arm64: Add VLPI migration support on GICv4.1 | expand |
On 2021/3/11 17:14, Marc Zyngier wrote: > On Wed, 27 Jan 2021 12:13:36 +0000, > Shenming Lu <lushenming@huawei.com> wrote: >> >> From: Zenghui Yu <yuzenghui@huawei.com> >> >> When setting the forwarding path of a VLPI (switch to the HW mode), >> we could also transfer the pending state from irq->pending_latch to >> VPT (especially in migration, the pending states of VLPIs are restored >> into kvm’s vgic first). And we currently send "INT+VSYNC" to trigger >> a VLPI to pending. >> >> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> >> Signed-off-by: Shenming Lu <lushenming@huawei.com> >> --- >> arch/arm64/kvm/vgic/vgic-v4.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c >> index ac029ba3d337..a3542af6f04a 100644 >> --- a/arch/arm64/kvm/vgic/vgic-v4.c >> +++ b/arch/arm64/kvm/vgic/vgic-v4.c >> @@ -449,6 +449,20 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq, >> irq->host_irq = virq; >> atomic_inc(&map.vpe->vlpi_count); >> >> + /* Transfer pending state */ >> + if (irq->pending_latch) { >> + ret = irq_set_irqchip_state(irq->host_irq, >> + IRQCHIP_STATE_PENDING, >> + irq->pending_latch); >> + WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq); >> + >> + /* >> + * Let it be pruned from ap_list later and don't bother >> + * the List Register. >> + */ >> + irq->pending_latch = false; > > NAK. If the interrupt is on the AP list, it must be pruned from it > *immediately*. The only case where it can be !pending and still on the > AP list is in interval between sync and prune. If we start messing > with this, we can't reason about the state of this list anymore. > > Consider calling vgic_queue_irq_unlock() here. Thanks for giving a hint, but it seems that vgic_queue_irq_unlock() only queues an IRQ after checking, did you mean vgic_prune_ap_list() instead? Thanks a lot for the comments! :-) Shenming > > Thanks, > > M. >
On 2021/3/12 17:05, Marc Zyngier wrote: > On Thu, 11 Mar 2021 12:32:07 +0000, > Shenming Lu <lushenming@huawei.com> wrote: >> >> On 2021/3/11 17:14, Marc Zyngier wrote: >>> On Wed, 27 Jan 2021 12:13:36 +0000, >>> Shenming Lu <lushenming@huawei.com> wrote: >>>> >>>> From: Zenghui Yu <yuzenghui@huawei.com> >>>> >>>> When setting the forwarding path of a VLPI (switch to the HW mode), >>>> we could also transfer the pending state from irq->pending_latch to >>>> VPT (especially in migration, the pending states of VLPIs are restored >>>> into kvm’s vgic first). And we currently send "INT+VSYNC" to trigger >>>> a VLPI to pending. >>>> >>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> >>>> Signed-off-by: Shenming Lu <lushenming@huawei.com> >>>> --- >>>> arch/arm64/kvm/vgic/vgic-v4.c | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> >>>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c >>>> index ac029ba3d337..a3542af6f04a 100644 >>>> --- a/arch/arm64/kvm/vgic/vgic-v4.c >>>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c >>>> @@ -449,6 +449,20 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq, >>>> irq->host_irq = virq; >>>> atomic_inc(&map.vpe->vlpi_count); >>>> >>>> + /* Transfer pending state */ >>>> + if (irq->pending_latch) { >>>> + ret = irq_set_irqchip_state(irq->host_irq, >>>> + IRQCHIP_STATE_PENDING, >>>> + irq->pending_latch); >>>> + WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq); >>>> + >>>> + /* >>>> + * Let it be pruned from ap_list later and don't bother >>>> + * the List Register. >>>> + */ >>>> + irq->pending_latch = false; >>> >>> NAK. If the interrupt is on the AP list, it must be pruned from it >>> *immediately*. The only case where it can be !pending and still on the >>> AP list is in interval between sync and prune. If we start messing >>> with this, we can't reason about the state of this list anymore. >>> >>> Consider calling vgic_queue_irq_unlock() here. >> >> Thanks for giving a hint, but it seems that vgic_queue_irq_unlock() only >> queues an IRQ after checking, did you mean vgic_prune_ap_list() instead? > > No, I really mean vgic_queue_irq_unlock(). It can be used to remove > the pending state from an interrupt, and drop it from the AP > list. This is exactly what happens when clearing the pending state of > a level interrupt, for example. Hi, I have gone through vgic_queue_irq_unlock more than once, but still can't find the place in it to drop an IRQ from the AP list... Did I miss something ?... Or could you help to point it out? Thanks very much for this! Shenming > > M. >
On 2021/3/12 19:10, Marc Zyngier wrote: > On Fri, 12 Mar 2021 10:48:29 +0000, > Shenming Lu <lushenming@huawei.com> wrote: >> >> On 2021/3/12 17:05, Marc Zyngier wrote: >>> On Thu, 11 Mar 2021 12:32:07 +0000, >>> Shenming Lu <lushenming@huawei.com> wrote: >>>> >>>> On 2021/3/11 17:14, Marc Zyngier wrote: >>>>> On Wed, 27 Jan 2021 12:13:36 +0000, >>>>> Shenming Lu <lushenming@huawei.com> wrote: >>>>>> >>>>>> From: Zenghui Yu <yuzenghui@huawei.com> >>>>>> >>>>>> When setting the forwarding path of a VLPI (switch to the HW mode), >>>>>> we could also transfer the pending state from irq->pending_latch to >>>>>> VPT (especially in migration, the pending states of VLPIs are restored >>>>>> into kvm’s vgic first). And we currently send "INT+VSYNC" to trigger >>>>>> a VLPI to pending. >>>>>> >>>>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> >>>>>> Signed-off-by: Shenming Lu <lushenming@huawei.com> >>>>>> --- >>>>>> arch/arm64/kvm/vgic/vgic-v4.c | 14 ++++++++++++++ >>>>>> 1 file changed, 14 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c >>>>>> index ac029ba3d337..a3542af6f04a 100644 >>>>>> --- a/arch/arm64/kvm/vgic/vgic-v4.c >>>>>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c >>>>>> @@ -449,6 +449,20 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq, >>>>>> irq->host_irq = virq; >>>>>> atomic_inc(&map.vpe->vlpi_count); >>>>>> >>>>>> + /* Transfer pending state */ >>>>>> + if (irq->pending_latch) { >>>>>> + ret = irq_set_irqchip_state(irq->host_irq, >>>>>> + IRQCHIP_STATE_PENDING, >>>>>> + irq->pending_latch); >>>>>> + WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq); >>>>>> + >>>>>> + /* >>>>>> + * Let it be pruned from ap_list later and don't bother >>>>>> + * the List Register. >>>>>> + */ >>>>>> + irq->pending_latch = false; >>>>> >>>>> NAK. If the interrupt is on the AP list, it must be pruned from it >>>>> *immediately*. The only case where it can be !pending and still on the >>>>> AP list is in interval between sync and prune. If we start messing >>>>> with this, we can't reason about the state of this list anymore. >>>>> >>>>> Consider calling vgic_queue_irq_unlock() here. >>>> >>>> Thanks for giving a hint, but it seems that vgic_queue_irq_unlock() only >>>> queues an IRQ after checking, did you mean vgic_prune_ap_list() instead? >>> >>> No, I really mean vgic_queue_irq_unlock(). It can be used to remove >>> the pending state from an interrupt, and drop it from the AP >>> list. This is exactly what happens when clearing the pending state of >>> a level interrupt, for example. >> >> Hi, I have gone through vgic_queue_irq_unlock more than once, but >> still can't find the place in it to drop an IRQ from the AP >> list... Did I miss something ?... Or could you help to point it >> out? Thanks very much for this! > > NO, you are right. I think this is a missing optimisation. Please call > the function anyway, as that's what is required to communicate a > change of state in general.> > I'll have a think about it. Maybe we could call vgic_prune_ap_list() if (irq->vcpu && !vgic_target_oracle(irq)) in vgic_queue_irq_unlock()... OK, I will retest this series and send a v4 soon. :-) Thanks, Shenming > > Thanks, > > M. >
On 2021/3/12 20:02, Marc Zyngier wrote: > On Fri, 12 Mar 2021 11:34:07 +0000, > Shenming Lu <lushenming@huawei.com> wrote: >> >> On 2021/3/12 19:10, Marc Zyngier wrote: >>> On Fri, 12 Mar 2021 10:48:29 +0000, >>> Shenming Lu <lushenming@huawei.com> wrote: >>>> >>>> On 2021/3/12 17:05, Marc Zyngier wrote: >>>>> On Thu, 11 Mar 2021 12:32:07 +0000, >>>>> Shenming Lu <lushenming@huawei.com> wrote: >>>>>> >>>>>> On 2021/3/11 17:14, Marc Zyngier wrote: >>>>>>> On Wed, 27 Jan 2021 12:13:36 +0000, >>>>>>> Shenming Lu <lushenming@huawei.com> wrote: >>>>>>>> >>>>>>>> From: Zenghui Yu <yuzenghui@huawei.com> >>>>>>>> >>>>>>>> When setting the forwarding path of a VLPI (switch to the HW mode), >>>>>>>> we could also transfer the pending state from irq->pending_latch to >>>>>>>> VPT (especially in migration, the pending states of VLPIs are restored >>>>>>>> into kvm’s vgic first). And we currently send "INT+VSYNC" to trigger >>>>>>>> a VLPI to pending. >>>>>>>> >>>>>>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> >>>>>>>> Signed-off-by: Shenming Lu <lushenming@huawei.com> >>>>>>>> --- >>>>>>>> arch/arm64/kvm/vgic/vgic-v4.c | 14 ++++++++++++++ >>>>>>>> 1 file changed, 14 insertions(+) >>>>>>>> >>>>>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c >>>>>>>> index ac029ba3d337..a3542af6f04a 100644 >>>>>>>> --- a/arch/arm64/kvm/vgic/vgic-v4.c >>>>>>>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c >>>>>>>> @@ -449,6 +449,20 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq, >>>>>>>> irq->host_irq = virq; >>>>>>>> atomic_inc(&map.vpe->vlpi_count); >>>>>>>> >>>>>>>> + /* Transfer pending state */ >>>>>>>> + if (irq->pending_latch) { >>>>>>>> + ret = irq_set_irqchip_state(irq->host_irq, >>>>>>>> + IRQCHIP_STATE_PENDING, >>>>>>>> + irq->pending_latch); >>>>>>>> + WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq); >>>>>>>> + >>>>>>>> + /* >>>>>>>> + * Let it be pruned from ap_list later and don't bother >>>>>>>> + * the List Register. >>>>>>>> + */ >>>>>>>> + irq->pending_latch = false; >>>>>>> >>>>>>> NAK. If the interrupt is on the AP list, it must be pruned from it >>>>>>> *immediately*. The only case where it can be !pending and still on the >>>>>>> AP list is in interval between sync and prune. If we start messing >>>>>>> with this, we can't reason about the state of this list anymore. >>>>>>> >>>>>>> Consider calling vgic_queue_irq_unlock() here. >>>>>> >>>>>> Thanks for giving a hint, but it seems that vgic_queue_irq_unlock() only >>>>>> queues an IRQ after checking, did you mean vgic_prune_ap_list() instead? >>>>> >>>>> No, I really mean vgic_queue_irq_unlock(). It can be used to remove >>>>> the pending state from an interrupt, and drop it from the AP >>>>> list. This is exactly what happens when clearing the pending state of >>>>> a level interrupt, for example. >>>> >>>> Hi, I have gone through vgic_queue_irq_unlock more than once, but >>>> still can't find the place in it to drop an IRQ from the AP >>>> list... Did I miss something ?... Or could you help to point it >>>> out? Thanks very much for this! >>> >>> NO, you are right. I think this is a missing optimisation. Please call >>> the function anyway, as that's what is required to communicate a >>> change of state in general.> >>> I'll have a think about it. >> >> Maybe we could call vgic_prune_ap_list() if (irq->vcpu && >> !vgic_target_oracle(irq)) in vgic_queue_irq_unlock()... > > The locking is pretty ugly in this case, and I don't want to reparse > the whole AP list. It is basically doing the same work as the > insertion, but with a list_del() instead of a list_add()... make sense.. Thanks, Shenming > > We can live without it for now. > >> OK, I will retest this series and send a v4 soon. :-) > > Thanks, > > M. >
diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c index ac029ba3d337..a3542af6f04a 100644 --- a/arch/arm64/kvm/vgic/vgic-v4.c +++ b/arch/arm64/kvm/vgic/vgic-v4.c @@ -449,6 +449,20 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq, irq->host_irq = virq; atomic_inc(&map.vpe->vlpi_count); + /* Transfer pending state */ + if (irq->pending_latch) { + ret = irq_set_irqchip_state(irq->host_irq, + IRQCHIP_STATE_PENDING, + irq->pending_latch); + WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq); + + /* + * Let it be pruned from ap_list later and don't bother + * the List Register. + */ + irq->pending_latch = false; + } + out: mutex_unlock(&its->its_lock); return ret;