Re: [Xen-devel] [PATCH v4 08/10] xen/arm: second irq injection while the first irq is still inflight

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> Set GICH_LR_PENDING in the corresponding GICH_LR to inject a second irq
> while the first one is still active.
> If the first irq is already pending (not active), just clear
> GIC_IRQ_GUEST_PENDING because the irq has already been injected and is
> already visible by the guest.
> If the irq has already been EOI'ed then just clear the GICH_LR right
> away and move the interrupt to lr_pending so that it is going to be
> reinjected by gic_restore_pending_irqs on return to guest.
> 
> If the target cpu is not the current cpu, then set GIC_IRQ_GUEST_PENDING
> and send an SGI. The target cpu is going to be interrupted and call
> gic_clear_lrs, that is going to take the same actions.
> 
> Unify the inflight and non-inflight code paths in vgic_vcpu_inject_irq.
> 
> Do not call vgic_vcpu_inject_irq from gic_inject if
> evtchn_upcall_pending is set. If we remove that call, we don't need to
> special case evtchn_irq in vgic_vcpu_inject_irq anymore.

That's the consequence of removing it, but what is the rationale for why
it is OK?

> We also need to force the first injection of evtchn_irq (call
> gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending
> is already set by common code on vcpu creation.

This is because the common code expects that the guest is moving from
sharedinfo based vcpu info using VCPUOP_register_vcpu_info on x86, but
on ARM we start off that way anyway.

I suppose it's a minor wrinkle, but I wonder if we can get rid of it...

> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 
> ---
> Changes in v3:
> - do not use the PENDING and ACTIVE state for HW interrupts;
> - unify the inflight and non-inflight code paths in
> vgic_vcpu_inject_irq.
> ---
>  xen/arch/arm/gic.c  |   89 
> +++++++++++++++++++++++++++++----------------------
>  xen/arch/arm/vgic.c |   33 ++++++++++---------
>  2 files changed, 68 insertions(+), 54 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 3f061cf..128d071 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -67,6 +67,8 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
>  /* Maximum cpu interface per GIC */
>  #define NR_GIC_CPU_IF 8
>  
> +static void _gic_clear_lr(struct vcpu *v, int i);

Single underbar prefixes are reserved for the compiler.

gic_clear_one_lr would be an adequate name I think.

You could also have done this at the time you introduced gic_clear_lrs,
which would save me now asking: is the split into _gic_clear_lr pure
code motion or are there actual substantive changes in it?

Ian.


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