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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

Ian.


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