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

[Julien Grall <julien.grall@xxxxxxx>]

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.
If you ever have NULL here then there is a latent BUG in your code 
somewhere else. 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.
> > AFAICT, vgic_v3_lpi_to_pending should never return NULL when reading the
> > priority. Or else, you are in big trouble.
> That depends on where and when you call it, which I don't want to make
> any assumptions about.
> In my latest version the vgic_get_virq_priority() call now stays at the
> very beginning of vgic_vcpu_inject_irq(), so at this point the LPI could
> have been unmapped meanwhile.
> Surely you will bail out handling this LPI later in the code if it
> returns NULL here, but for the sake of this function I think we need the
> check.
> To me it looks a bit like having this abstraction here is a bit
> pointless and complicates things, but well ....
Well, I am not against very defensive programming. I am more against 
returning a fake value that may impact the rest of the vGIC (not even 
mentioning the lack of comment explain why). After all, the priority is 
controlled by the guest and not the hypervisor.
I suggested few way above to catch those errors.

Cheers,

--
Julien Grall

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