[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 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

 


Rackspace

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