Re: [Xen-devel] [PATCH v5 2/6] xen/arm: track the state of guest IRQs

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 16 Dec 2013, Ian Campbell wrote:
> On Thu, 2013-12-12 at 18:59 +0000, Stefano Stabellini wrote:
> > Introduce a status field in struct pending_irq. Valid states are
> > GUEST_PENDING, GUEST_VISIBLE and GUEST_ENABLED and they are not mutually
> > exclusive.  See the in-code comment for an explanation of the states and
> > how they are used.
> > Use atomic operations to set and clear the status bits. Note that
> > setting GIC_IRQ_GUEST_VISIBLE and clearing GIC_IRQ_GUEST_PENDING can be
> > done in two separate operations as the underlying pending status is
> > actually only cleared on the LR after the guest ACKs the interrupts.
> > Until that happens it's not possible to receive another interrupt.
> > 
> > The main effect of this patch is that an IRQ can be set to GUEST_PENDING
> > while it is being serviced by the guest. In maintenance_interrupt we
> > check whether GUEST_PENDING is set and if it is we add the irq back into
> > the lr_pending queue so that it's going to be reinjected one more time,
> > if the interrupt is still enabled at the vgicd level.
> > If it is not, it is going to be injected as soon as the guest renables
> > the interrupt.
> > 
> > One exception is evtchn_irq: in that case we don't want to
> > set the GIC_IRQ_GUEST_PENDING bit if it is already GUEST_VISIBLE,
> > because as part of the event handling loop, the guest would realize that
> > new events are present even without a new notification.
> > Also we already have a way to figure out exactly when we do need to
> > inject a second notification if vgic_vcpu_inject_irq is called after the
> > end of the guest event handling loop and before the guest EOIs the
> > interrupt (see db453468d92369e7182663fb13e14d83ec4ce456 "arm: vgic: fix
> > race between evtchn upcall and evtchnop_send").
> > 
> > Don't call gic_inject_irq_stop from maintenance_interrupt because
> > gic_inject (called by leave_hypervisor_tail) is going to call
> > gic_inject_irq_start/stop appropriately later anyway.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 
> Acked and applied, along with the rest of the series, which was already
> acked.
> 
> One comment:
> 
> > +static inline void gic_add_to_lr_pending(struct vcpu *v, unsigned int irq,
> > +        unsigned int priority)
> 
> Can't this take struct pending_irq *n as a parameter instead of int and
> use n->priority and n->irq?
> 
> All of the callers seem to have an appropriate n in hand except for
> gic_set_guest_irq which, if you buy into the above, should also take a
> pending_irq *. Likewise gic_set_lr could too.
> 
> Doing this saves some irq_to_pending lookups (not that they are really
> expensive) but also avoid the risk of the irq and priority becoming out
> of sync and seems generally logical to use the pending_irq as the
> "handle" for these ops.

I'll send a patch

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel