[Xen-devel,v3,15/39] ARM: new VGIC: Implement vgic_vcpu_pending_irq

Message ID 20180321163235.12529-16-andre.przywara@linaro.org
State New
Headers show
Series
  • New VGIC(-v2) implementation
Related show

Commit Message

Andre Przywara March 21, 2018, 4:32 p.m.
Tell Xen whether a particular VCPU has an IRQ that needs handling
in the guest. This is used to decide whether a VCPU is runnable or
if a hypercall should be preempted to let the guest handle the IRQ.

This is based on Linux commit 90eee56c5f90, written by Eric Auger.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
Changelog v2 ... v3:
- adjust vgic_vcpu_pending_irq() to return integers, not false/true

Changelog v1 ... v2:
- adjust to new vgic_vcpu_pending_irq() prototype, drop wrapper

 xen/arch/arm/vgic/vgic.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Julien Grall March 22, 2018, 3:52 a.m. | #1
Hi Andre,

On 03/21/2018 04:32 PM, Andre Przywara wrote:
> Tell Xen whether a particular VCPU has an IRQ that needs handling
> in the guest. This is used to decide whether a VCPU is runnable or
> if a hypercall should be preempted to let the guest handle the IRQ.
> 
> This is based on Linux commit 90eee56c5f90, written by Eric Auger.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
> Changelog v2 ... v3:
> - adjust vgic_vcpu_pending_irq() to return integers, not false/true

I would have preferred to have the return switch to bool instead. I 
guess this can be done on a follow-up. With one comment below:

Reviewed-by: Julien Grall <julien.grall@arm.com>

> 
> Changelog v1 ... v2:
> - adjust to new vgic_vcpu_pending_irq() prototype, drop wrapper
> 
>   xen/arch/arm/vgic/vgic.c | 37 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index 2fa595f4f7..925cda4580 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -647,6 +647,43 @@ void vgic_sync_to_lrs(void)
>       gic_hw_ops->update_hcr_status(GICH_HCR_EN, 1);
>   }
>   
> +/**
> + * vgic_vcpu_pending_irq() - determine if interrupts need to be injected
> + * @vcpu: The vCPU on which to check for interrupts.
> + *
> + * Checks whether there is an interrupt on the given VCPU which needs
> + * handling in the guest. This requires at least one IRQ to be pending
> + * and enabled.
> + *
> + * Returns: 1 if the guest should run to handle interrupts, 0 otherwise.

NIT: Because of "ret = irq_is_pending(irq) && irq->enabled", you will 
return a non-zero value if the guest should run to handle interrupts.

> + */
> +int vgic_vcpu_pending_irq(struct vcpu *vcpu)
> +{
> +    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
> +    struct vgic_irq *irq;
> +    unsigned long flags;
> +    int ret = 0;
> +
> +    if ( !vcpu->domain->arch.vgic.enabled )
> +        return 0;
> +
> +    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);
> +        ret = irq_is_pending(irq) && irq->enabled;
> +        spin_unlock(&irq->irq_lock);
> +
> +        if ( ret )
> +            break;
> +    }
> +
> +    spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags);
> +
> +    return ret;
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> 

Cheers,
Andre Przywara March 22, 2018, 11:15 a.m. | #2
Hi,

On 22/03/18 03:52, Julien Grall wrote:
> Hi Andre,
> 
> On 03/21/2018 04:32 PM, Andre Przywara wrote:
>> Tell Xen whether a particular VCPU has an IRQ that needs handling
>> in the guest. This is used to decide whether a VCPU is runnable or
>> if a hypercall should be preempted to let the guest handle the IRQ.
>>
>> This is based on Linux commit 90eee56c5f90, written by Eric Auger.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>> Changelog v2 ... v3:
>> - adjust vgic_vcpu_pending_irq() to return integers, not false/true
> 
> I would have preferred to have the return switch to bool instead. I
> guess this can be done on a follow-up. With one comment below:

I did that originally, but then you meanwhile merged that first patch
already. So I didn't want to add another patch to this series.
I am fine with changing this afterwards, probably as part of a fixup series.

> Reviewed-by: Julien Grall <julien.grall@arm.com>

Thanks!

Cheers,
Andre.

>>
>> Changelog v1 ... v2:
>> - adjust to new vgic_vcpu_pending_irq() prototype, drop wrapper
>>
>>   xen/arch/arm/vgic/vgic.c | 37 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
>> index 2fa595f4f7..925cda4580 100644
>> --- a/xen/arch/arm/vgic/vgic.c
>> +++ b/xen/arch/arm/vgic/vgic.c
>> @@ -647,6 +647,43 @@ void vgic_sync_to_lrs(void)
>>       gic_hw_ops->update_hcr_status(GICH_HCR_EN, 1);
>>   }
>>   +/**
>> + * vgic_vcpu_pending_irq() - determine if interrupts need to be injected
>> + * @vcpu: The vCPU on which to check for interrupts.
>> + *
>> + * Checks whether there is an interrupt on the given VCPU which needs
>> + * handling in the guest. This requires at least one IRQ to be pending
>> + * and enabled.
>> + *
>> + * Returns: 1 if the guest should run to handle interrupts, 0 otherwise.
> 
> NIT: Because of "ret = irq_is_pending(irq) && irq->enabled", you will
> return a non-zero value if the guest should run to handle interrupts.
> 
>> + */
>> +int vgic_vcpu_pending_irq(struct vcpu *vcpu)
>> +{
>> +    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
>> +    struct vgic_irq *irq;
>> +    unsigned long flags;
>> +    int ret = 0;
>> +
>> +    if ( !vcpu->domain->arch.vgic.enabled )
>> +        return 0;
>> +
>> +    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);
>> +        ret = irq_is_pending(irq) && irq->enabled;
>> +        spin_unlock(&irq->irq_lock);
>> +
>> +        if ( ret )
>> +            break;
>> +    }
>> +
>> +    spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags);
>> +
>> +    return ret;
>> +}
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
>>
> 
> Cheers,
>
Stefano Stabellini March 26, 2018, 11:34 p.m. | #3
On Thu, 22 Mar 2018, Andre Przywara wrote:
> Hi,

> 

> On 22/03/18 03:52, Julien Grall wrote:

> > Hi Andre,

> > 

> > On 03/21/2018 04:32 PM, Andre Przywara wrote:

> >> Tell Xen whether a particular VCPU has an IRQ that needs handling

> >> in the guest. This is used to decide whether a VCPU is runnable or

> >> if a hypercall should be preempted to let the guest handle the IRQ.

> >>

> >> This is based on Linux commit 90eee56c5f90, written by Eric Auger.

> >>

> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

> >> ---

> >> Changelog v2 ... v3:

> >> - adjust vgic_vcpu_pending_irq() to return integers, not false/true

> > 

> > I would have preferred to have the return switch to bool instead. I

> > guess this can be done on a follow-up. With one comment below:

> 

> I did that originally, but then you meanwhile merged that first patch

> already. So I didn't want to add another patch to this series.

> I am fine with changing this afterwards, probably as part of a fixup series.

> 

> > Reviewed-by: Julien Grall <julien.grall@arm.com>

> 

> Thanks!


Acked-by: Stefano Stabellini <sstabellini@kernel.org>



> Cheers,

> Andre.

> 

> >>

> >> Changelog v1 ... v2:

> >> - adjust to new vgic_vcpu_pending_irq() prototype, drop wrapper

> >>

> >>   xen/arch/arm/vgic/vgic.c | 37 +++++++++++++++++++++++++++++++++++++

> >>   1 file changed, 37 insertions(+)

> >>

> >> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c

> >> index 2fa595f4f7..925cda4580 100644

> >> --- a/xen/arch/arm/vgic/vgic.c

> >> +++ b/xen/arch/arm/vgic/vgic.c

> >> @@ -647,6 +647,43 @@ void vgic_sync_to_lrs(void)

> >>       gic_hw_ops->update_hcr_status(GICH_HCR_EN, 1);

> >>   }

> >>   +/**

> >> + * vgic_vcpu_pending_irq() - determine if interrupts need to be injected

> >> + * @vcpu: The vCPU on which to check for interrupts.

> >> + *

> >> + * Checks whether there is an interrupt on the given VCPU which needs

> >> + * handling in the guest. This requires at least one IRQ to be pending

> >> + * and enabled.

> >> + *

> >> + * Returns: 1 if the guest should run to handle interrupts, 0 otherwise.

> > 

> > NIT: Because of "ret = irq_is_pending(irq) && irq->enabled", you will

> > return a non-zero value if the guest should run to handle interrupts.

> > 

> >> + */

> >> +int vgic_vcpu_pending_irq(struct vcpu *vcpu)

> >> +{

> >> +    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;

> >> +    struct vgic_irq *irq;

> >> +    unsigned long flags;

> >> +    int ret = 0;

> >> +

> >> +    if ( !vcpu->domain->arch.vgic.enabled )

> >> +        return 0;

> >> +

> >> +    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);

> >> +        ret = irq_is_pending(irq) && irq->enabled;

> >> +        spin_unlock(&irq->irq_lock);

> >> +

> >> +        if ( ret )

> >> +            break;

> >> +    }

> >> +

> >> +    spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags);

> >> +

> >> +    return ret;

> >> +}

> >> +

> >>   /*

> >>    * Local variables:

> >>    * mode: C

> >>

> > 

> > Cheers,

> > 

>

Patch

diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index 2fa595f4f7..925cda4580 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -647,6 +647,43 @@  void vgic_sync_to_lrs(void)
     gic_hw_ops->update_hcr_status(GICH_HCR_EN, 1);
 }
 
+/**
+ * vgic_vcpu_pending_irq() - determine if interrupts need to be injected
+ * @vcpu: The vCPU on which to check for interrupts.
+ *
+ * Checks whether there is an interrupt on the given VCPU which needs
+ * handling in the guest. This requires at least one IRQ to be pending
+ * and enabled.
+ *
+ * Returns: 1 if the guest should run to handle interrupts, 0 otherwise.
+ */
+int vgic_vcpu_pending_irq(struct vcpu *vcpu)
+{
+    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
+    struct vgic_irq *irq;
+    unsigned long flags;
+    int ret = 0;
+
+    if ( !vcpu->domain->arch.vgic.enabled )
+        return 0;
+
+    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);
+        ret = irq_is_pending(irq) && irq->enabled;
+        spin_unlock(&irq->irq_lock);
+
+        if ( ret )
+            break;
+    }
+
+    spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags);
+
+    return ret;
+}
+
 /*
  * Local variables:
  * mode: C