diff mbox series

[Xen-devel,v3,6/8] ARM: VGIC: factor out vgic_get_hw_irq_desc()

Message ID 20180124181058.6157-7-andre.przywara@linaro.org
State Superseded
Headers show
Series ARM: VGIC/GIC separation cleanups | expand

Commit Message

Andre Przywara Jan. 24, 2018, 6:10 p.m. UTC 
At the moment we happily access the VGIC internal struct pending_irq
(which describes a virtual IRQ) in irq.c.
Factor out the actually needed functionality to learn the associated
hardware IRQ and move that into gic-vgic.c to improve abstraction.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/gic-vgic.c    | 15 +++++++++++++++
 xen/arch/arm/irq.c         |  7 ++-----
 xen/include/asm-arm/vgic.h |  2 ++
 3 files changed, 19 insertions(+), 5 deletions(-)

Comments

Julien Grall Jan. 31, 2018, 4:16 p.m. UTC | #1 
Hi,

On 24/01/18 18:10, Andre Przywara wrote:
> At the moment we happily access the VGIC internal struct pending_irq
> (which describes a virtual IRQ) in irq.c.
> Factor out the actually needed functionality to learn the associated
> hardware IRQ and move that into gic-vgic.c to improve abstraction.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>   xen/arch/arm/gic-vgic.c    | 15 +++++++++++++++
>   xen/arch/arm/irq.c         |  7 ++-----
>   xen/include/asm-arm/vgic.h |  2 ++
>   3 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index d44e4dacd3..3ad98dcd3a 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -411,6 +411,21 @@ void gic_dump_vgic_info(struct vcpu *v)
>           printk("Pending irq=%d\n", p->irq);
>   }
>   
> +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v,
> +                                      unsigned int virq)
> +{
> +    struct pending_irq *p;
> +
> +    if ( !v )
> +        v = d->vcpu[0];

I dislike this new function in the current state. Let's imagine someone 
decide to pass a PPI but no vCPU. The vCPU would now be default to vCPU0 
and potentially returning the wrong irq_desc (imagine PPI passthrough 
such as for the vtimer).

So the code needs at least checking the vCPU is passed in the case of a 
PPI. I would be happy with an ASSERT().

Cheers,
Andre Przywara Jan. 31, 2018, 4:24 p.m. UTC | #2 
Hi,

On 31/01/18 16:16, Julien Grall wrote:
> Hi,
> 
> On 24/01/18 18:10, Andre Przywara wrote:
>> At the moment we happily access the VGIC internal struct pending_irq
>> (which describes a virtual IRQ) in irq.c.
>> Factor out the actually needed functionality to learn the associated
>> hardware IRQ and move that into gic-vgic.c to improve abstraction.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>> ---
>>   xen/arch/arm/gic-vgic.c    | 15 +++++++++++++++
>>   xen/arch/arm/irq.c         |  7 ++-----
>>   xen/include/asm-arm/vgic.h |  2 ++
>>   3 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>> index d44e4dacd3..3ad98dcd3a 100644
>> --- a/xen/arch/arm/gic-vgic.c
>> +++ b/xen/arch/arm/gic-vgic.c
>> @@ -411,6 +411,21 @@ void gic_dump_vgic_info(struct vcpu *v)
>>           printk("Pending irq=%d\n", p->irq);
>>   }
>>   +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu
>> *v,
>> +                                      unsigned int virq)
>> +{
>> +    struct pending_irq *p;
>> +
>> +    if ( !v )
>> +        v = d->vcpu[0];
> 
> I dislike this new function in the current state. Let's imagine someone
> decide to pass a PPI but no vCPU. The vCPU would now be default to vCPU0
> and potentially returning the wrong irq_desc (imagine PPI passthrough
> such as for the vtimer).
> 
> So the code needs at least checking the vCPU is passed in the case of a
> PPI. I would be happy with an ASSERT().

Yeah, good point. Something like ASSERT(!v && virq >= 32), I guess?

Cheers,
Andre.
Julien Grall Jan. 31, 2018, 4:25 p.m. UTC | #3 
On 31/01/18 16:24, Andre Przywara wrote:
> Hi,
> 
> On 31/01/18 16:16, Julien Grall wrote:
>> Hi,
>>
>> On 24/01/18 18:10, Andre Przywara wrote:
>>> At the moment we happily access the VGIC internal struct pending_irq
>>> (which describes a virtual IRQ) in irq.c.
>>> Factor out the actually needed functionality to learn the associated
>>> hardware IRQ and move that into gic-vgic.c to improve abstraction.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>>> ---
>>>    xen/arch/arm/gic-vgic.c    | 15 +++++++++++++++
>>>    xen/arch/arm/irq.c         |  7 ++-----
>>>    xen/include/asm-arm/vgic.h |  2 ++
>>>    3 files changed, 19 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>>> index d44e4dacd3..3ad98dcd3a 100644
>>> --- a/xen/arch/arm/gic-vgic.c
>>> +++ b/xen/arch/arm/gic-vgic.c
>>> @@ -411,6 +411,21 @@ void gic_dump_vgic_info(struct vcpu *v)
>>>            printk("Pending irq=%d\n", p->irq);
>>>    }
>>>    +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu
>>> *v,
>>> +                                      unsigned int virq)
>>> +{
>>> +    struct pending_irq *p;
>>> +
>>> +    if ( !v )
>>> +        v = d->vcpu[0];
>>
>> I dislike this new function in the current state. Let's imagine someone
>> decide to pass a PPI but no vCPU. The vCPU would now be default to vCPU0
>> and potentially returning the wrong irq_desc (imagine PPI passthrough
>> such as for the vtimer).
>>
>> So the code needs at least checking the vCPU is passed in the case of a
>> PPI. I would be happy with an ASSERT().
> 
> Yeah, good point. Something like ASSERT(!v && virq >= 32), I guess?

Yes. It should be enough.

Cheers,


> Cheers,
> Andre.
>
diff mbox series

Patch

diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index d44e4dacd3..3ad98dcd3a 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -411,6 +411,21 @@  void gic_dump_vgic_info(struct vcpu *v)
         printk("Pending irq=%d\n", p->irq);
 }
 
+struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v,
+                                      unsigned int virq)
+{
+    struct pending_irq *p;
+
+    if ( !v )
+        v = d->vcpu[0];
+
+    p = irq_to_pending(v, virq);
+    if ( !p )
+        return NULL;
+
+    return p->desc;
+}
+
 int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
                         struct irq_desc *desc)
 {
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 7f133de549..62103a20e3 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -534,19 +534,16 @@  int release_guest_irq(struct domain *d, unsigned int virq)
     struct irq_desc *desc;
     struct irq_guest *info;
     unsigned long flags;
-    struct pending_irq *p;
     int ret;
 
     /* Only SPIs are supported */
     if ( virq < NR_LOCAL_IRQS || virq >= vgic_num_irqs(d) )
         return -EINVAL;
 
-    p = spi_to_pending(d, virq);
-    if ( !p->desc )
+    desc = vgic_get_hw_irq_desc(d, NULL, virq);
+    if ( !desc )
         return -EINVAL;
 
-    desc = p->desc;
-
     spin_lock_irqsave(&desc->lock, flags);
 
     ret = -EINVAL;
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index f4240df371..ebc0cfaee8 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -219,6 +219,8 @@  int vgic_v2_init(struct domain *d, int *mmio_count);
 int vgic_v3_init(struct domain *d, int *mmio_count);
 
 bool vgic_evtchn_irq_pending(struct vcpu *v);
+struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v,
+                                      unsigned int virq);
 int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
                         struct irq_desc *desc);