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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 31/05/2017 18:56, Stefano Stabellini wrote:
> On Wed, 31 May 2017, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 30/05/17 23:07, Stefano Stabellini wrote:
> > > On Tue, 30 May 2017, Julien Grall wrote:
> > > > >      };
> > > > >  };
> > > > >
> > > > > @@ -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?
> > I am not too confident to say lpi_vcpu_id will always be valid with the
> > current locking in the vGIC.
> Fair enough, but I wouldn't solve a bug with unnecessary code soon to be
> removed. The problem lies elsewhere and should be fixed elsewhere. We
> could even add an ASSERT(p->lpi_vcpu_id < d->max_vcpus) here, would
> that work for you?
The problem lies in this code because we don't have per-IRQ locking. The 
ASSERT is not a solution to make sure a race does not happen (it could 
easily happen if the LPI is pending whilst the guest is discarding it).
If you add an ASSERT you will potentially get your platform crashing 
time to time on discard. So I don't think the ASSERT is acceptable here.
> > There is a potential race between its_discard_event and this function. The
> > former may reset pending_irq whilst reading lpi_vcpu_id as we cannot take the
> > vCPU lock yet.
> >
> > But all of this is racy anyway because of the locking. This will get solved by
> > the vGIC rework after the merge.
> >
> > So For the time being I would keep the check. We can revisit it later if
> > necessary.
> This is one of those things that are unimportant because they work
> either way.
I am sorry, but it matters when you can easily prevent crash and limit 
the racy condition.
Cheers,

--
Julien Grall

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