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

[Julien Grall <julien.grall@xxxxxxx>]

On 10/05/17 16:11, Andre Przywara wrote:
> Hi,
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 :).

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel