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

[Julien Grall <julien.grall@xxxxxxx>]

On 05/10/2017 06:14 PM, Andre Przywara wrote:
> Hi,
>
> On 10/05/17 12:07, Julien Grall wrote:
> > On 05/10/2017 11:47 AM, Andre Przywara wrote:
> > > Hi,
> > Hi Andre,
> >
> > > On 12/04/17 11:44, Julien Grall wrote:
> > > > On 12/04/17 01:44, Andre Przywara wrote:
> > > > > +/* Retrieve the priority of an LPI from its struct pending_irq. */
> > > > > +static int vgic_v3_lpi_get_priority(struct domain *d, uint32_t vlpi)
> > > > > +{
> > > > > +    struct pending_irq *p = vgic_v3_lpi_to_pending(d, vlpi);
> > > > > +
> > > > > +    if ( !p )
> > > > > +        return GIC_PRI_IRQ;
> > > > Why the check here? And why returning GIC_PRI_IRQ?
> > > Because you surely want to avoid dereferencing NULL?
> > > I changed the return value to 0xff, which is the lowest priority.
> > > Frankly I think we could just return anything, as we will stop handling
> > > this LPI anyway a bit later in the code if p is NULL here.
> > I agree that you want to prevent NULL. But we also want to avoid return
> > fake value because there was a caller that didn't bother to check
> > whether the interrupt is valid at first hand.
> Well, I changed the sequence in vgic_vcpu_inject_irq() back to be:
>
>         priority = vgic_get_virq_priority(v, virq);
>
>         spin_lock_irqsave(&v->arch.vgic.lock, flags);
>         n = irq_to_pending(v, virq);
>
> mostly to prevent the locking order (rank vs. VCPU lock) issue you
> mentioned. We read the latest priority value upfront, but only use it
> later if the pending_irq is valid. I don't see how this should create
> problems. Eventually this will be solved properly by the pending_irq lock.
>
> > If you ever have NULL here then there is a latent BUG in your code
> > somewhere else.
> Not in this case.
Because of the locking issue? I know there are locking issue, but it 
does not mean we should introduce bad code just for workaround them for 
the time being...
> > Ignoring the NULL and return a fake value is likely not
> > the right solution for development.
> >
> > I can see two solutions for this:
> >     - ASSERT(p)
> >     - if ( !p )
> >       {
> >          ASSERT_UNREACHABLE();
> >              return 0xff;
> >       }
> >
> > The later would still return a dumb value but at least we would catch
> > programming mistake during development.
> I think this solution asks for the ASSERT to trigger in corner cases: If
> the LPI fired on the host, but got unmapped shortly afterwards. In this
> case vgic_vcpu_inject_irq() can be reached with an invalid LPI number,
> and we handle this properly when irq_to_pending() returns NULL.
> But in this case get_priority() will be called with the same invalid
> LPI, so should be able to cope with that as well.
> Again this will eventually be solved properly with the per-IRQ lock.
See above. I still prefer to see the ASSERT firing time to time than bad 
code going in staging.
Cheers,

--
Julien Grall

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