[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.