Re: [Xen-devel] [PATCH v4 5/8] ARM: VGIC: factor out vgic_connect_hw_irq()

[Julien Grall <julien.grall@xxxxxxx>]

On 02/06/2018 03:44 PM, Andre Przywara wrote:
> Hi,
>
> On 06/02/18 14:21, Julien Grall wrote:
> > Hi Andre,
> >
> > On 02/05/2018 04:19 PM, Andre Przywara wrote:
> > > At the moment we happily access VGIC internal data structures like
> > > the rank and struct pending_irq in gic.c, which should be VGIC agnostic.
> > >
> > > Factor out a new function vgic_connect_hw_irq(), which allows a virtual
> > > IRQ to be connected to a hardware IRQ (using the hw bit in the LR).
> > >
> > > This removes said accesses to VGIC data structures and improves
> > > abstraction.
> > I was expecting some explanation regarding the new locking order in the
> > commit message.
> Well, there is no real change in the new locking order. We enter both
> gic_route_irq_to_guest() and gic_remove_irq_from_guest() with the desc
> lock held, then take the rank lock at some point in time and drop it
> again. The only change is how long we hold the rank lock, but this
> should have no effect, since we don't manipulate any virtual IRQ
> properties (rank or not) in the code lines that are now no longer under
> the lock.
> I can try to summarise this in the commit message.
Sorry, somehow I wrote locking order when I was meant to say about how 
long you take the lock.
Something along what you say above looks good to me. What I want to 
avoid is running into a problem and wondering why the locking was modified.
Cheers,

> > > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > > ---
> > >    xen/arch/arm/gic-vgic.c    | 36 ++++++++++++++++++++++++++++++++++++
> > >    xen/arch/arm/gic.c         | 44
> > > ++++++++++----------------------------------
> > >    xen/include/asm-arm/vgic.h |  2 ++
> > >    3 files changed, 48 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> > > index 1d5744ecc8..fff7c01ee8 100644
> > > --- a/xen/arch/arm/gic-vgic.c
> > > +++ b/xen/arch/arm/gic-vgic.c
> > > @@ -397,6 +397,42 @@ void gic_dump_vgic_info(struct vcpu *v)
> > >            printk("Pending irq=%d\n", p->irq);
> > >    }
> > >    +int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned
> > > int virq,
> > > +                        struct irq_desc *desc, bool connect)
> > > +{
> > > +    unsigned long flags;
> > > +    /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
> > > +     * route SPIs to guests, it doesn't make any difference. */
> > Please fix the coding style as requested in v3.
> Ah, sorry, I forgot that over the rewrite.
>
> > > +    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
> > > +    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
> > > +    struct pending_irq *p = irq_to_pending(v_target, virq);
> > > +    int ret = 0;
> > > +
> > > +    ASSERT(connect && desc);
> > I am not sure why you allow desc to be non-NULL when disconnecting it
> > (see more below).
> I consider passing the desc pointer in addition to the vIRQ number
> redundant in case we want to drop the association. The only reason we do
> this is to work around the somewhat opposite locking order. So this is a
> property of the existing code, which I consider at least somewhat dodgy.
>
> In order to allow changing this in the future (for instance with the new
> VGIC), I introduced the "connect" parameter and decided to make "desc"
> optional in case "connect" is false.
> If we give it anyway, we check for a match, which is what we do with the
> current code. So we keep this molly guard.
>
> > > +
> > > +    /* We are taking to rank lock to prevent parallel connections. */
> > > +    vgic_lock_rank(v_target, rank, flags);
> > > +
> > > +    if ( connect )
> > > +    {
> > > +        /* The VIRQ should not be already enabled by the guest */
> > > +        if ( !p->desc &&
> > > +             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> > > +            p->desc = desc;
> > > +        else
> > > +            ret = -EBUSY;
> > > +    }
> > > +    else
> > > +    {
> > > +        if ( !desc || p->desc == desc )
> >  From a quick glance, no caller will have desc is NULL.
> Yes, for now.
>
> > Even if it was,
> > it will not harm because p->desc will be set to NULL.
> >
> > > +            p->desc = NULL;
> > > +    }
> > But likely you want to return an error if p->desc != desc as this is a
> > user input error. Ignoring it is a pretty bad.
> Right, good point. Actually this is what I had in mind, but somehow
> managed to derail it. Will fix it.
>
> Thanks for the review!
> Andre.
>
> > > +
> > > +    vgic_unlock_rank(v_target, rank, flags);
> > > +
> > > +    return ret;
> > > +}
> > > +
> > >    /*
> > >     * Local variables:
> > >     * mode: C
> > Cheers,
--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel