Re: [RFC PATCH 26/49] ARM: new VGIC: Implement vgic_vcpu_pending_irq

[Julien Grall <julien.grall@arm.com>]

On 02/03/18 13:53, Andre Przywara wrote:
> Hi,

Hi Andre,

> On 26/02/18 16:30, Julien Grall wrote:
>>
>>
>> On 02/26/2018 04:25 PM, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 26/02/18 15:55, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 02/26/2018 03:29 PM, Andre Przywara wrote:
>>>>> On 13/02/18 16:35, Julien Grall wrote:
>>>>>>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
>>>>>>> index f4f2a04a60..9e7fb1edcb 100644
>>>>>>> --- a/xen/arch/arm/vgic/vgic.c
>>>>>>> +++ b/xen/arch/arm/vgic/vgic.c
>>>>>>> @@ -646,6 +646,38 @@ void gic_inject(void)
>>>>>>>          vgic_restore_state(current);
>>>>>>>      }
>>>>>>>      +static int vgic_vcpu_pending_irq(struct vcpu *vcpu)
>>>>>>> +{
>>>>>>> +    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>>>>>> +    struct vgic_irq *irq;
>>>>>>> +    bool pending = false;
>>>>>>> +    unsigned long flags;
>>>>>>> +
>>>>>>> +    if ( !vcpu->domain->arch.vgic.enabled )
>>>>>>> +        return false;
>>>>>>> +
>>>>>>> +    spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags);
>>>>>>> +
>>>>>>> +    list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list)
>>>>>>> +    {
>>>>>>> +        spin_lock(&irq->irq_lock);
>>>>>>> +        pending = irq_is_pending(irq) && irq->enabled;
>>>>>>> +        spin_unlock(&irq->irq_lock);
>>>>>>> +
>>>>>>> +        if ( pending )
>>>>>>> +            break;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags);
>>>>>>> +
>>>>>>> +    return pending;
>>>>>>> +}
>>>>>>> +
>>>>>>> +int gic_events_need_delivery(void)
>>>>>>
>>>>>> You probably want to rename that function or just expose
>>>>>> vgic_vcpu_pending_irq().
>>>>>
>>>>> Rename to what? I need both functions: vgic_vcpu_pending_irq() is also
>>>>> called by vgic_kick_vcpus() (later in the series).
>>>>> And gic_events_need_delivery(void) is the interface that the arch code
>>>>> expects. Shall I rename this there? To what?
>>>>
>>>> Let me start with it is a bit odd to have a function name 'gic_*' in the
>>>> virtual GIC code. So at least renaming to vgic_events_need_delivery
>>>> would be an improvement.
>>>>
>>>> Regarding the interface itself, it is ARM specific and not set in stone.
>>>> It would not be too bad to use vgic_vcpu_pending_irq(current). Is there
>>>> any reason for not doing that?
> 
> The two interfaces used for that purpose are different in the two VGICs:
> - The old VGIC only works on the current VCPU, since it peeks into the
> GICH_ register to learn the priority (regardless of whether this is
> really needed or useful).
> - The new VGIC can use this function for any VCPU, and we need this
> functionality later on (when we iterate over all VCPUs).
> So we can't use a function hardwiring "current", that would break
> vgic_kick_vcpus() in the new VGIC. And we can't pass a VCPU parameter,
> that would not work for the old VGIC.
> So I believe having this small wrapper here is the easiest solution.
> I will add a patch to rename this function to vgic_pending_irq(),
> though, so this one here looks like:
> int vgic_pending_irq(void)
> {
>      return vgic_vcpu_pending_irq(current);
> }
> 
> We can clean this up when the old VGIC gets removed.

Likely no-one in the old vGIC are going to call that function with v != 
current. This would not be the only place in Xen where a vCPU is taken 
in parameter but effectively v can only be current. That's where 
ASSERT(v == current) comes into place.

Cheers,

-- 
Julien Grall

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