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@lists.xen.org
https://lists.xen.org/xen-devel
