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

Re: [Xen-devel] [PATCH v8 23/27] ARM: vITS: handle INV command

On 10/05/17 16:11, Andre Przywara wrote:

Hi Andre,

On 12/04/17 18:20, Julien Grall wrote:
On 12/04/17 01:44, Andre Przywara wrote:

+    ASSERT(spin_is_locked(&v->arch.vgic.lock));

The locking is likely to wrong here too (see patch #2). For instance
with a MOVI then INV on interrupt enabled.

+    if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+    {
+        if ( !list_empty(&p->inflight) &&
+             !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
+            gic_raise_guest_irq(v, vlpi, p->lpi_priority);
+    }
+    else
+    {
+        clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
+        list_del_init(&p->lr_queue);
+    }
+static int its_handle_inv(struct virt_its *its, uint64_t *cmdptr)
+    struct domain *d = its->d;
+    uint32_t devid = its_cmd_get_deviceid(cmdptr);
+    uint32_t eventid = its_cmd_get_id(cmdptr);
+    struct pending_irq *p;
+    unsigned long flags;
+    struct vcpu *vcpu;
+    uint32_t vlpi;
+    int ret = -1;
+    /* Translate the event into a vCPU/vLPI pair. */
+    if ( !read_itte(its, devid, eventid, &vcpu, &vlpi) )
+        return -1;
+    if ( vlpi == INVALID_LPI )
+        return -1;
+    spin_lock_irqsave(&vcpu->arch.vgic.lock, flags);
+    p = d->arch.vgic.handler->lpi_to_pending(d, vlpi);
+    if ( !p )
+        goto out_unlock;

As said on v5, this could be simpler and use the pending_irqs in the
device. That would be an improvement though. So a would be good.

Originally I found it more straight-forward to use the one existing
interface (the rbtree) we also use in the VGIC part, which would allow
us to handle locking or ref-counting in one central place.
But indeed the ITS command handling has all the data we need to find the
pending_irq directly from the virtual device.
So I replaced all lpi_to_pending() calls in those handlers with a new
function gicv3_its_get_event_pending_irq(), which looks up the struct
from an ITS/device/event triple.
I take and keep the its->lock for the runtime of these functions, so
those events and their memory will not vanish meanwhile.

Does that make sense?

It makes sense to keep the ref-counting in one central place. But it is better to avoid reading guest memory and therefore avoid most of checking and overhead to translate the IPA to PA.

That's why I suggested to use pending_irqs :).


Julien Grall

Xen-devel mailing list



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