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

[Stefano Stabellini <sstabellini@kernel.org>]

On Tue, 30 May 2017, Julien Grall wrote:
> 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@arm.com>
> > ---
> >  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?

Supposedly we are going to set lpi_vcpu_id only to good values? Meaning
that we are going to do the lpi_vcpu_id checks at the time of setting
lpi_vcpu_id. Thus, even if lpi_vcpu_id changes, it is not a problem. In
fact, if that is true, can we even drop the if ( p->lpi_vcpu_id <
d->max_vcpus ) test here?


> > +
> > +    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);

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