Re: [PATCH v4 10/27] ARM: GICv3: forward pending LPIs to guests

[Julien Grall <julien.grall@arm.com>]

Hi Andre,

On 03/04/17 21:28, 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@arm.com>
> ---
>  xen/arch/arm/gic-v3-lpi.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/gic.c         |  8 +++++++-
>  xen/arch/arm/vgic-v3.c     | 11 +++++++++++
>  xen/arch/arm/vgic.c        |  9 ++++++++-
>  xen/include/asm-arm/irq.h  |  2 ++
>  xen/include/asm-arm/vgic.h |  2 ++
>  6 files changed, 72 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> index d3ee141..ad89863 100644
> --- a/xen/arch/arm/gic-v3-lpi.c
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -125,6 +125,48 @@ 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);
> +
> +    /* Unmapped events are marked with an invalid LPI ID. */
> +    if ( hlpi.virt_lpi == INVALID_LPI )
> +        return;
> +
> +    d = rcu_lock_domain_by_id(hlpi.dom_id);
> +    if ( !d )
> +        return;
> +
> +    /* Make sure we don't step beyond the vcpu array. */
> +    if ( hlpi.vcpu_id >= d->max_vcpus )
> +    {
> +        rcu_unlock_domain(d);
> +        return;
> +    }
> +
> +    vcpu = d->vcpu[hlpi.vcpu_id];
> +
> +    vgic_vcpu_inject_irq(vcpu, hlpi.virt_lpi);
> +
> +    rcu_unlock_domain(d);
> +}
> +
>  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 3ed6f81..a6037d4 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -709,7 +709,13 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>              do_IRQ(regs, irq, is_fiq);
>              local_irq_disable();
>          }
> -        else if (unlikely(irq < 16))
> +#ifdef CONFIG_HAS_ITS
> +        else if ( is_lpi(irq) )
> +        {
> +            do_LPI(irq);
> +        }
> +#endif

I really don't want to see GICv3 specific code called in common code. 
Please introduce a specific callback in gic_hw_operations.

We tried to make the common code as generic as possible and if you 
notice we have a clear separation today. I am not sure why we should 
break this separation for your purpose...

[...]

> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 95fa0ba..797fd86 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -347,6 +347,17 @@ struct pending_irq *lpi_to_pending(struct domain *d, unsigned int lpi)
>      return pirq;
>  }
>
> +/* Retrieve the priority of an LPI from its struct pending_irq. */
> +int vgic_lpi_get_priority(struct domain *d, uint32_t vlpi)
> +{
> +    struct pending_irq *p = lpi_to_pending(d, vlpi);
> +
> +    if ( !p )
> +        return GIC_PRI_IRQ;
> +
> +    return p->lpi_priority;
> +}
> +
>  static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
>                                            uint32_t gicr_reg,
>                                            register_t r)
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 28f6f66..4720f46 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -226,10 +226,17 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq)
>
>  static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq)
>  {
> -    struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
> +    struct vgic_irq_rank *rank;
>      unsigned long flags;
>      int priority;
>
> +#ifdef CONFIG_HAS_ITS
> +    /* LPIs don't have a rank, also store their priority separately. */
> +    if ( is_lpi(virq) )
> +        return vgic_lpi_get_priority(v->domain, virq);
> +#endif

Same remark here.

> +
> +    rank = vgic_rank_irq(v, virq);
>      vgic_lock_rank(v, rank, flags);
>      priority = rank->priority[virq & INTERRUPT_RANK_MASK];
>      vgic_unlock_rank(v, rank, flags);
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index 2f7ee8a..0cd0117 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -46,6 +46,8 @@ static inline bool is_lpi(unsigned int irq)
>      return irq >= LPI_OFFSET;
>  }
>
> +void do_LPI(unsigned int irq);
> +
>  #define domain_pirq_to_irq(d, pirq) (pirq)
>
>  bool_t is_assignable_irq(unsigned int irq);
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 69ef160..a24a971 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -66,12 +66,14 @@ struct pending_irq
>  #define GIC_IRQ_GUEST_VISIBLE  2
>  #define GIC_IRQ_GUEST_ENABLED  3
>  #define GIC_IRQ_GUEST_MIGRATING   4
> +#define GIC_IRQ_GUEST_LPI_PENDING 5

This new bit is not used in this code. Please document this purpose of 
it....

>      unsigned long status;
>      struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
>      unsigned int irq;
>  #define GIC_INVALID_LR         (uint8_t)~0
>      uint8_t lr;
>      uint8_t priority;
> +    uint8_t lpi_priority;       /* Caches the priority if this is an LPI. */

Please explain in the commit message why you introduce lpi_priority? 
This could technically be retrieved from the priority table...

>      /* inflight is used to append instances of pending_irq to
>       * vgic.inflight_irqs */
>      struct list_head inflight;
>

Cheers,

-- 
Julien Grall

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