[Xen-devel,v3,26/39] ARM: new VGIC: Add SGIPENDR register handlers

Message ID 20180321163235.12529-27-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.
As this register is v2 specific, its implementation lives entirely
in vgic-mmio-v2.c.
This register allows setting the source mask of an IPI.

This is based on Linux commit ed40213ef9b0, written by Andre Przywara.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Reviewed-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/vgic/vgic-mmio-v2.c | 81 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 79 insertions(+), 2 deletions(-)

Comments

Stefano Stabellini March 27, 2018, 10:27 p.m. | #1
On Wed, 21 Mar 2018, Andre Przywara wrote:
> As this register is v2 specific, its implementation lives entirely
> in vgic-mmio-v2.c.
> This register allows setting the source mask of an IPI.
> 
> This is based on Linux commit ed40213ef9b0, written by Andre Przywara.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Reviewed-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/vgic/vgic-mmio-v2.c | 81 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 79 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
> index 9ef80608c1..32e0f6fc33 100644
> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> @@ -181,6 +181,83 @@ static void vgic_mmio_write_target(struct vcpu *vcpu,
>      }
>  }
>  
> +static unsigned long vgic_mmio_read_sgipend(struct vcpu *vcpu,
> +                                            paddr_t addr, unsigned int len)
> +{
> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8);
> +    uint32_t val = 0;
> +    unsigned int i;
> +
> +    ASSERT(intid < VGIC_NR_SGIS);
> +
> +    for ( i = 0; i < len; i++ )
> +    {
> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
> +
> +        val |= (uint32_t)irq->source << (i * 8);

lock?
one more comment


> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +
> +    return val;
> +}
> +
> +static void vgic_mmio_write_sgipendc(struct vcpu *vcpu,
> +                                     paddr_t addr, unsigned int len,
> +                                     unsigned long val)
> +{
> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8);
> +    unsigned int i;
> +    unsigned long flags;
> +
> +    ASSERT(intid < VGIC_NR_SGIS);
> +
> +    for ( i = 0; i < len; i++ )
> +    {
> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
> +
> +        spin_lock_irqsave(&irq->irq_lock, flags);
> +
> +        irq->source &= ~((val >> (i * 8)) & 0xff);
> +        if ( !irq->source )
> +            irq->pending_latch = false;
> +
> +        spin_unlock_irqrestore(&irq->irq_lock, flags);
> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +}
> +
> +static void vgic_mmio_write_sgipends(struct vcpu *vcpu,
> +                                     paddr_t addr, unsigned int len,
> +                                     unsigned long val)
> +{
> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8);
> +    unsigned int i;
> +    unsigned long flags;
> +
> +    ASSERT(intid < VGIC_NR_SGIS);
> +
> +    for ( i = 0; i < len; i++ )
> +    {
> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
> +
> +        spin_lock_irqsave(&irq->irq_lock, flags);
> +
> +        irq->source |= (val >> (i * 8)) & 0xff;
> +
> +        if ( irq->source )
> +        {
> +            irq->pending_latch = true;
> +            vgic_queue_irq_unlock(vcpu->domain, irq, flags);
> +        }
> +        else
> +        {
> +            spin_unlock_irqrestore(&irq->irq_lock, flags);
> +        }

NIT: it should be safe to call vgic_queue_irq_unlock regardless, right?


> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +}
> +
>  static const struct vgic_register_region vgic_v2_dist_registers[] = {
>      REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
>          vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
> @@ -219,10 +296,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
>          vgic_mmio_read_raz, vgic_mmio_write_sgir, 4,
>          VGIC_ACCESS_32bit),
>      REGISTER_DESC_WITH_LENGTH(GICD_CPENDSGIR,
> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
> +        vgic_mmio_read_sgipend, vgic_mmio_write_sgipendc, 16,
>          VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
>      REGISTER_DESC_WITH_LENGTH(GICD_SPENDSGIR,
> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
> +        vgic_mmio_read_sgipend, vgic_mmio_write_sgipends, 16,
>          VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
>  };
>  
> -- 
> 2.14.1
>
Andre Przywara March 28, 2018, 10:37 a.m. | #2
Hi,

On 27/03/18 23:27, Stefano Stabellini wrote:
> On Wed, 21 Mar 2018, Andre Przywara wrote:
>> As this register is v2 specific, its implementation lives entirely
>> in vgic-mmio-v2.c.
>> This register allows setting the source mask of an IPI.
>>
>> This is based on Linux commit ed40213ef9b0, written by Andre Przywara.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
>> ---
>>  xen/arch/arm/vgic/vgic-mmio-v2.c | 81 +++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 79 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
>> index 9ef80608c1..32e0f6fc33 100644
>> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
>> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
>> @@ -181,6 +181,83 @@ static void vgic_mmio_write_target(struct vcpu *vcpu,
>>      }
>>  }
>>  
>> +static unsigned long vgic_mmio_read_sgipend(struct vcpu *vcpu,
>> +                                            paddr_t addr, unsigned int len)
>> +{
>> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8);
>> +    uint32_t val = 0;
>> +    unsigned int i;
>> +
>> +    ASSERT(intid < VGIC_NR_SGIS);
>> +
>> +    for ( i = 0; i < len; i++ )
>> +    {
>> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
>> +
>> +        val |= (uint32_t)irq->source << (i * 8);
> 
> lock?
> one more comment

(see the answer to patch 19/39)

>> +        vgic_put_irq(vcpu->domain, irq);
>> +    }
>> +
>> +    return val;
>> +}
>> +
>> +static void vgic_mmio_write_sgipendc(struct vcpu *vcpu,
>> +                                     paddr_t addr, unsigned int len,
>> +                                     unsigned long val)
>> +{
>> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8);
>> +    unsigned int i;
>> +    unsigned long flags;
>> +
>> +    ASSERT(intid < VGIC_NR_SGIS);
>> +
>> +    for ( i = 0; i < len; i++ )
>> +    {
>> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
>> +
>> +        spin_lock_irqsave(&irq->irq_lock, flags);
>> +
>> +        irq->source &= ~((val >> (i * 8)) & 0xff);
>> +        if ( !irq->source )
>> +            irq->pending_latch = false;
>> +
>> +        spin_unlock_irqrestore(&irq->irq_lock, flags);
>> +        vgic_put_irq(vcpu->domain, irq);
>> +    }
>> +}
>> +
>> +static void vgic_mmio_write_sgipends(struct vcpu *vcpu,
>> +                                     paddr_t addr, unsigned int len,
>> +                                     unsigned long val)
>> +{
>> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8);
>> +    unsigned int i;
>> +    unsigned long flags;
>> +
>> +    ASSERT(intid < VGIC_NR_SGIS);
>> +
>> +    for ( i = 0; i < len; i++ )
>> +    {
>> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
>> +
>> +        spin_lock_irqsave(&irq->irq_lock, flags);
>> +
>> +        irq->source |= (val >> (i * 8)) & 0xff;
>> +
>> +        if ( irq->source )
>> +        {
>> +            irq->pending_latch = true;
>> +            vgic_queue_irq_unlock(vcpu->domain, irq, flags);
>> +        }
>> +        else
>> +        {
>> +            spin_unlock_irqrestore(&irq->irq_lock, flags);
>> +        }
> 
> NIT: it should be safe to call vgic_queue_irq_unlock regardless, right?

I don't think vgic_queue_irq_unlock() and subsequent functions can deal
with the IRQ being pending, but not having a source bit set:
http://www.linux-arm.org/git?p=xen-ap.git;a=blob;f=xen/arch/arm/vgic/vgic-v2.c;h=6a84e741ee0a#l205

Besides, this scheme of:
	if ( needs to be injected )
	{
		make_pending();
		vgic_queue_irq_unlock();
	}
	else
	{
		spin_unlock_irqrestore();
	}

is all over the place, and I don't want to deviate from that and the KVM
implementation needlessly.

Cheers,
Andre.



>> +        vgic_put_irq(vcpu->domain, irq);
>> +    }
>> +}
>> +
>>  static const struct vgic_register_region vgic_v2_dist_registers[] = {
>>      REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
>>          vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
>> @@ -219,10 +296,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
>>          vgic_mmio_read_raz, vgic_mmio_write_sgir, 4,
>>          VGIC_ACCESS_32bit),
>>      REGISTER_DESC_WITH_LENGTH(GICD_CPENDSGIR,
>> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
>> +        vgic_mmio_read_sgipend, vgic_mmio_write_sgipendc, 16,
>>          VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
>>      REGISTER_DESC_WITH_LENGTH(GICD_SPENDSGIR,
>> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
>> +        vgic_mmio_read_sgipend, vgic_mmio_write_sgipends, 16,
>>          VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
>>  };
>>  
>> -- 
>> 2.14.1
>>

Patch

diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
index 9ef80608c1..32e0f6fc33 100644
--- a/xen/arch/arm/vgic/vgic-mmio-v2.c
+++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
@@ -181,6 +181,83 @@  static void vgic_mmio_write_target(struct vcpu *vcpu,
     }
 }
 
+static unsigned long vgic_mmio_read_sgipend(struct vcpu *vcpu,
+                                            paddr_t addr, unsigned int len)
+{
+    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8);
+    uint32_t val = 0;
+    unsigned int i;
+
+    ASSERT(intid < VGIC_NR_SGIS);
+
+    for ( i = 0; i < len; i++ )
+    {
+        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
+
+        val |= (uint32_t)irq->source << (i * 8);
+
+        vgic_put_irq(vcpu->domain, irq);
+    }
+
+    return val;
+}
+
+static void vgic_mmio_write_sgipendc(struct vcpu *vcpu,
+                                     paddr_t addr, unsigned int len,
+                                     unsigned long val)
+{
+    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8);
+    unsigned int i;
+    unsigned long flags;
+
+    ASSERT(intid < VGIC_NR_SGIS);
+
+    for ( i = 0; i < len; i++ )
+    {
+        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
+
+        spin_lock_irqsave(&irq->irq_lock, flags);
+
+        irq->source &= ~((val >> (i * 8)) & 0xff);
+        if ( !irq->source )
+            irq->pending_latch = false;
+
+        spin_unlock_irqrestore(&irq->irq_lock, flags);
+        vgic_put_irq(vcpu->domain, irq);
+    }
+}
+
+static void vgic_mmio_write_sgipends(struct vcpu *vcpu,
+                                     paddr_t addr, unsigned int len,
+                                     unsigned long val)
+{
+    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8);
+    unsigned int i;
+    unsigned long flags;
+
+    ASSERT(intid < VGIC_NR_SGIS);
+
+    for ( i = 0; i < len; i++ )
+    {
+        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
+
+        spin_lock_irqsave(&irq->irq_lock, flags);
+
+        irq->source |= (val >> (i * 8)) & 0xff;
+
+        if ( irq->source )
+        {
+            irq->pending_latch = true;
+            vgic_queue_irq_unlock(vcpu->domain, irq, flags);
+        }
+        else
+        {
+            spin_unlock_irqrestore(&irq->irq_lock, flags);
+        }
+        vgic_put_irq(vcpu->domain, irq);
+    }
+}
+
 static const struct vgic_register_region vgic_v2_dist_registers[] = {
     REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
         vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
@@ -219,10 +296,10 @@  static const struct vgic_register_region vgic_v2_dist_registers[] = {
         vgic_mmio_read_raz, vgic_mmio_write_sgir, 4,
         VGIC_ACCESS_32bit),
     REGISTER_DESC_WITH_LENGTH(GICD_CPENDSGIR,
-        vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
+        vgic_mmio_read_sgipend, vgic_mmio_write_sgipendc, 16,
         VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
     REGISTER_DESC_WITH_LENGTH(GICD_SPENDSGIR,
-        vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
+        vgic_mmio_read_sgipend, vgic_mmio_write_sgipends, 16,
         VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
 };