Re: [Xen-devel] [PATCH v10 11/32] ARM: GICv3: forward pending LPIs to guests

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 26/05/17 18:35, Andre Przywara wrote:
> Upon receiving an LPI on the host, we need to find the right VCPU and
> virtual IRQ number to get this IRQ injected.
> Iterate our two-level LPI table to find the domain ID and the virtual
> LPI number quickly when the host takes an LPI. We then look up the
> right VCPU in the struct pending_irq.
> We use the existing injection function to let the GIC emulation deal
> with this interrupt.
> This introduces a do_LPI() as a hardware gic_ops.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/gic-v2.c            |  7 ++++
>  xen/arch/arm/gic-v3-lpi.c        | 76 ++++++++++++++++++++++++++++++++++++++--
>  xen/arch/arm/gic-v3.c            |  1 +
>  xen/arch/arm/gic.c               |  8 ++++-
>  xen/include/asm-arm/domain.h     |  3 +-
>  xen/include/asm-arm/gic.h        |  2 ++
>  xen/include/asm-arm/gic_v3_its.h |  8 +++++
>  7 files changed, 101 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 270a136..ffbe47c 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1217,6 +1217,12 @@ static int __init gicv2_init(void)
>      return 0;
>  }
>
> +static void gicv2_do_LPI(unsigned int lpi)
> +{
> +    /* No LPIs in a GICv2 */
> +    BUG();
> +}
> +
>  const static struct gic_hw_operations gicv2_ops = {
>      .info                = &gicv2_info,
>      .init                = gicv2_init,
> @@ -1244,6 +1250,7 @@ const static struct gic_hw_operations gicv2_ops = {
>      .make_hwdom_madt     = gicv2_make_hwdom_madt,
>      .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings,
>      .iomem_deny_access   = gicv2_iomem_deny_access,
> +    .do_LPI              = gicv2_do_LPI,
>  };
>
>  /* Set up the GIC */
> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> index 292f2d0..438bbfe 100644
> --- a/xen/arch/arm/gic-v3-lpi.c
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -47,7 +47,6 @@ union host_lpi {
>      struct {
>          uint32_t virt_lpi;
>          uint16_t dom_id;
> -        uint16_t vcpu_id;
You don't explain why you remove vcpu_id from host_lpi. This likely 
require a separate patch anyway.
Also, I would prefer if you make the padding in the structure explicit 
(i.e using pad0).
>      };
>  };
>
> @@ -136,6 +135,80 @@ uint64_t gicv3_get_redist_address(unsigned int cpu, bool 
> use_pta)
>          return per_cpu(lpi_redist, cpu).redist_id << 16;
>  }
>
> +static void vgic_vcpu_inject_lpi(struct domain *d, unsigned int virq)
> +{
> +    struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
> +    struct vcpu *v = NULL;
> +
> +    if ( !p )
> +        return;
> +
> +    if ( p->lpi_vcpu_id < d->max_vcpus )
> +        v = d->vcpu[read_atomic(&p->lpi_vcpu_id)];
Hmmm, what does prevent lpi_vcpu_id to change between the check and the 
read?
> +
> +    if ( v )
v will always be valid if you read d->vcpu[....] and the way you wrote 
the code is very confusing.
It would be clearer if you do:

if ( p->lpi_vcpu_id >= d->max_vcpus )
  return;

v = ....
vgic_vcpu_inject_irq(v, irq);

Cheers,

--
Julien Grall

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