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

Re: [Xen-devel] [PATCH v10 11/32] ARM: GICv3: forward pending LPIs to guests

Hi Stefano,

On 31/05/2017 18:56, Stefano Stabellini wrote:
On Wed, 31 May 2017, Julien Grall wrote:
Hi Stefano,

On 30/05/17 23:07, Stefano Stabellini wrote:
On Tue, 30 May 2017, Julien Grall wrote:

@@ -136,6 +135,80 @@ uint64_t gicv3_get_redist_address(unsigned int cpu,
bool use_pta)
         return per_cpu(lpi_redist, cpu).redist_id << 16;

+static void vgic_vcpu_inject_lpi(struct domain *d, unsigned int virq)
+    struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
+    struct vcpu *v = NULL;
+    if ( !p )
+        return;
+    if ( p->lpi_vcpu_id < d->max_vcpus )
+        v = d->vcpu[read_atomic(&p->lpi_vcpu_id)];

Hmmm, what does prevent lpi_vcpu_id to change between the check and the

Supposedly we are going to set lpi_vcpu_id only to good values? Meaning
that we are going to do the lpi_vcpu_id checks at the time of setting
lpi_vcpu_id. Thus, even if lpi_vcpu_id changes, it is not a problem. In
fact, if that is true, can we even drop the if ( p->lpi_vcpu_id <
d->max_vcpus ) test here?

I am not too confident to say lpi_vcpu_id will always be valid with the
current locking in the vGIC.

Fair enough, but I wouldn't solve a bug with unnecessary code soon to be
removed. The problem lies elsewhere and should be fixed elsewhere. We
could even add an ASSERT(p->lpi_vcpu_id < d->max_vcpus) here, would
that work for you?

The problem lies in this code because we don't have per-IRQ locking. The ASSERT is not a solution to make sure a race does not happen (it could easily happen if the LPI is pending whilst the guest is discarding it).

If you add an ASSERT you will potentially get your platform crashing time to time on discard. So I don't think the ASSERT is acceptable here.

There is a potential race between its_discard_event and this function. The
former may reset pending_irq whilst reading lpi_vcpu_id as we cannot take the
vCPU lock yet.

But all of this is racy anyway because of the locking. This will get solved by
the vGIC rework after the merge.

So For the time being I would keep the check. We can revisit it later if

This is one of those things that are unimportant because they work
either way.

I am sorry, but it matters when you can easily prevent crash and limit the racy condition.


Julien Grall

Xen-devel mailing list



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