Re: [Xen-devel] [PATCH] ARM: new VGIC: evtchn: fix potential race in vcpu_mark_events_pending()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 4 Apr 2018, Andre Przywara wrote:
> Hi,
> 
> On 04/04/18 01:04, Stefano Stabellini wrote:
> > On Tue, 3 Apr 2018, Julien Grall wrote:
> >> On 29/03/18 18:35, Stefano Stabellini wrote:
> >>> On Thu, 29 Mar 2018, Andre Przywara wrote:
> >>>> Stefano pointed out the following situation:
> >>>> ----------------------
> >>>> 1) vcpuA/cpuA is running, it has already handled the event, cleared
> >>>> evtchn_upcall_pending and EOIed the event_irq but hasn't trapped into
> >>>> Xen yet. It is still in guest mode.
> >>>>
> >>>> 2) Xen on cpuB calls vcpu_mark_events_pending(vcpuA), then calls
> >>>> vgic_inject_irq. However, because irq->line_level is high, it is not
> >>>> injected.
> >>>>
> >>>> 3) vcpuA has to wait until trapping into Xen, calling
> >>>> vcpu_update_evtchn_irq, and going back to guest mode before receiving
> >>>> the event. This is theoretically a very long time.
> >>>> ----------------------
> >>>>
> >>>> Fix this by updating the state of our emulated IRQ line level inside
> >>>> vcpu_mark_events_pending(), before trying to inject the new interrupt.
> >>>>
> >>>> Despite having two calls to vgic_inject_irq(), only one will actually do
> >>>> something:
> >>>> - If the emulated line level was already in sync with the actual flag,
> >>>>    the VGIC ignores the first call, due to vgic_validate_injection().
> >>>> - If the emulated line level was high, but the flag says it should have
> >>>>    been low, vgic_inject_irq() will just update the line_level state.
> >>>> - If the emulated line level was low, but the flags says it should have
> >>>>    been high, we will inject the interrupt. The second call is then a
> >>>>    NOP.
> >>>>
> >>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> >>>> ---
> >>>> Hi,
> >>>>
> >>>> this would ideally have been part of a former patch:
> >>>> "[PATCH v3 06/39] ARM: evtchn: Handle level triggered IRQs correctly",
> >>>> but this has been merged already, so this has to be a follow-up.
> >>>> Ideally this would be merged before the final patch that introduces the
> >>>> CONFIG_NEW_VGIC Kconfig symbol, so that the old code gets never compiled.
> >>>>
> >>>> Thanks,
> >>>> Andre
> >>>>
> >>>>   xen/arch/arm/domain.c | 11 +++++++++--
> >>>>   1 file changed, 9 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> >>>> index 9688e62f78..11fa9002dc 100644
> >>>> --- a/xen/arch/arm/domain.c
> >>>> +++ b/xen/arch/arm/domain.c
> >>>> @@ -947,10 +947,17 @@ void vcpu_mark_events_pending(struct vcpu *v)
> >>>>       int already_pending = test_and_set_bit(
> >>>>           0, (unsigned long *)&vcpu_info(v, evtchn_upcall_pending));
> >>>>   -    if ( already_pending )
> >>>> -        return;
> >>>> +#ifdef CONFIG_NEW_VGIC
> >>>> +    /* Update the state of the current interrupt line. */
> >>>> +    vgic_inject_irq(v->domain, v, v->domain->arch.evtchn_irq,
> >>>> already_pending);
> >>>>   +    /* Make the level IRQ pending. That's a NOP if it was already. */
> >>>>       vgic_inject_irq(v->domain, v, v->domain->arch.evtchn_irq, true);
> >>>> +#else
> >>>> +    /* Only signal the VGIC if it wasn't already pending. */
> >>>> +    if ( !already_pending )
> >>>> +        vgic_inject_irq(v->domain, v, v->domain->arch.evtchn_irq, true);
> >>>> +#endif
> >>>>   }
> >>>
> >>> The issue with this is that it is potentially racy, against vcpuA
> >>> trapping into Xen and executing vcpu_update_evtchn_irq, that also reads
> >>> evtchn_upcall_pending, then calls vgic_inject_irq. The last
> >>> vgic_inject_irq executed could be the one passing level = false, losing
> >>> the notification.
> >>>
> >>> I might have a better idea: what if we just vcpu_kick(v) and do nothing
> >>> else? There is no need to call vgic_inject_irq from here because the
> >>> other vcpu will take care of doing it for us after trapping into Xen
> >>> (vcpu_update_evtchn_irq). It also needs to trap into Xen anyway to be
> >>> able to receive the new event as soon as possible.
> >>>
> >>> The code would become:
> >>>
> >>>    if ( already_pending )
> >>>        return;
> >>>
> >>>    vgic_inject_irq(v->domain, v, v->domain->arch.evtchn_irq, true);
> >>>
> >>> #ifdef CONFIG_NEW_VGIC
> >>>    vcpu_kick(v);
> >>> #endif
> >>>
> >>>
> >>> Note that vgic_inject_irq already does a vcpu_kick but only after
> >>> passing the vgic_validate_injection check, which would fail in this case
> >>> because irq->line_level is not up-to-date.
> >>>
> >>> What do you think?
> >>
> >> At the moment, the issue you describe is a non-issue because we set the EOI
> >> bit in the LR (see vgic_v2_populate_lr). So there are no need to kick the
> >> other vCPU as it would enter to Xen as soon as the event channel interrupt 
> >> is
> >> been eoied by the guest.
> >>
> >> Therefore, I believe we don't need to have a different version of
> >> vcpu_mark_events_pending for the new vGIC.
> > 
> > Ah! It makes sense.
> > 
> > Andre, why did you choose to set the EOI bit in the LR for non-hardware
> > interrupts? If it came up in previous email exchanges, my apologies.
> 
> So for hardware mapped level IRQs it goes like this:
> 1) The host ACKs the IRQ, making it active & pending on the GIC side.
> 2) We inject it as pending into the guest, with the HW bit set.
> 3) The guest ACKs it, it becomes active (only) in the LR.
> 4) The guest handles the request, probably lowering the hardware IRQ
> line on the way. This changes the state to just active on the GIC side.
> 5) The guest EOIs it, which changes the state to inactive in both the LR
> on on the GIC. Newly pending IRQs would be correctly handled as new
> interrupts.
> 
> For virtual level IRQs we want to do something similar, but we would be
> missing step 5. By using the EOI interrupt, we can update the state of
> the virtual IRQ there as well. So a bit like for this use case here.

Maybe I misunderstood your explanation.

Why is missing step 5) an issue? Also, I don't think that step 5) is
actually missing: the guest EOIs the virtual interrupt, and as a result
the interrupt changes state to inactive in the LR, which is what we want
right?


> Another point is that - in contrast for instance to the VPL011 - for the
> evtchn we don't know when the virtual IRQ line goes low, because this is
> done by the guest, without any trapping or Xen code getting involved.

Right, this is the issue we have been discussing earlier in this thread,
and it looks like we have a good solution for it. I don't think it
should discourage us from considering the EOI bit for virtual
interrupts.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel