Re: [Xen-devel] [PATCH v2 10/27] ARM: GICv3: forward pending LPIs to guests

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 03/16/2017 11:20 AM, Andre Przywara wrote:
> Upon receiving an LPI, we need to find the right VCPU and virtual IRQ
> number to get this IRQ injected.
> Iterate our two-level LPI table to find this information quickly when
> the host takes an LPI. Call the existing injection function to let the
> GIC emulation deal with this interrupt.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/gic-v3-lpi.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/gic.c        |  6 ++++--
>  xen/include/asm-arm/irq.h |  8 ++++++++
>  3 files changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> index 59d3ba4..0579976 100644
> --- a/xen/arch/arm/gic-v3-lpi.c
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -104,6 +104,47 @@ uint64_t gicv3_get_redist_address(unsigned int cpu, bool 
> use_pta)
>          return per_cpu(lpi_redist, cpu).redist_id << 16;
>  }
>
> +/*
> + * Handle incoming LPIs, which are a bit special, because they are potentially
> + * numerous and also only get injected into guests. Treat them specially here,
> + * by just looking up their target vCPU and virtual LPI number and hand it
> + * over to the injection function.
> + */
> +void do_LPI(unsigned int lpi)
> +{
> +    struct domain *d;
> +    union host_lpi *hlpip, hlpi;
> +    struct vcpu *vcpu;
> +
> +    WRITE_SYSREG32(lpi, ICC_EOIR1_EL1);
> +
> +    hlpip = gic_get_host_lpi(lpi);
> +    if ( !hlpip )
> +        return;
> +
> +    hlpi.data = read_u64_atomic(&hlpip->data);
> +
> +    /* We may have mapped more host LPIs than the guest actually asked for. */
Another way, is the interrupt has been received at the same time the 
guest is configuring it. What will happen if the interrupt is lost?
> +    if ( !hlpi.virt_lpi )
> +        return;
> +
> +    d = get_domain_by_id(hlpi.dom_id);
It would be enough to use rcu_lock_domain_by_id here.

> +    if ( !d )
> +        return;
> +
> +    if ( hlpi.vcpu_id >= d->max_vcpus )
A comment would be certainly useful here to explain why this check.

> +    {
> +        put_domain(d);
> +        return;
> +    }
> +
> +    vcpu = d->vcpu[hlpi.vcpu_id];
> +
> +    put_domain(d);
> +
> +    vgic_vcpu_inject_irq(vcpu, hlpi.virt_lpi);
> +}
> +
>  static int gicv3_lpi_allocate_pendtable(uint64_t *reg)
>  {
>      uint64_t val;
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index bd3c032..7286e5d 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -700,8 +700,10 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>              local_irq_enable();
>              do_IRQ(regs, irq, is_fiq);
>              local_irq_disable();
> -        }
> -        else if (unlikely(irq < 16))
> +        } else if ( is_lpi(irq) )
> +        {
> +            do_LPI(irq);
I really don't want to see GICv3 specific code called in common code. 
Please introduce a specific callback in gic_hw_operations.
> +        } else if ( unlikely(irq < 16) )
Coding style:

}
else if (...)
{
}
else if (...)

>          {
>              do_sgi(regs, irq);
>          }
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index 8f7a167..ee47de8 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -34,6 +34,14 @@ struct irq_desc *__irq_to_desc(int irq);
>
>  void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq);
>
> +#ifdef CONFIG_HAS_ITS
> +void do_LPI(unsigned int irq);
> +#else
> +static inline void do_LPI(unsigned int irq)
> +{
> +}
> +#endif
> +
This would avoid such ugly hack where do_LPI is define in gic-v3-its.c 
but declared in irq.h.
Cheers,

--
Julien Grall

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