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

[Andre Przywara <andre.przywara@xxxxxxx>]

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.

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

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