[Xen-devel,RFC,30/49] ARM: new VGIC: Add ENABLE registers handlers

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

Commit Message

Andre Przywara Feb. 9, 2018, 2:39 p.m.
As the enable register handlers are shared between the v2 and v3
emulation, their implementation goes into vgic-mmio.c, to be easily
referenced from the v3 emulation as well later.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/vgic/vgic-mmio-v2.c |   4 +-
 xen/arch/arm/vgic/vgic-mmio.c    | 114 +++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/vgic/vgic-mmio.h    |  11 ++++
 3 files changed, 127 insertions(+), 2 deletions(-)

Comments

Julien Grall Feb. 16, 2018, 4:57 p.m. | #1
Hi Andre,

On 09/02/18 14:39, Andre Przywara wrote:
> As the enable register handlers are shared between the v2 and v3
> emulation, their implementation goes into vgic-mmio.c, to be easily
> referenced from the v3 emulation as well later.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>   xen/arch/arm/vgic/vgic-mmio-v2.c |   4 +-
>   xen/arch/arm/vgic/vgic-mmio.c    | 114 +++++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/vgic/vgic-mmio.h    |  11 ++++
>   3 files changed, 127 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
> index 0926b3243e..eca6840ff9 100644
> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> @@ -74,10 +74,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
>           vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
>           VGIC_ACCESS_32bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER,
> -        vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
> +        vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
>           VGIC_ACCESS_32bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER,
> -        vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
> +        vgic_mmio_read_enable, vgic_mmio_write_cenable, NULL, NULL, 1,
>           VGIC_ACCESS_32bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR,
>           vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
> diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c
> index 59703a6909..3d9fa02a10 100644
> --- a/xen/arch/arm/vgic/vgic-mmio.c
> +++ b/xen/arch/arm/vgic/vgic-mmio.c
> @@ -39,6 +39,120 @@ void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr,
>       /* Ignore */
>   }
>   
> +/*
> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value
> + * of the enabled bit, so there is only one function for both here.
> + */
> +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
> +                    paddr_t addr, unsigned int len)

Indentation.

> +{
> +    u32 intid = VGIC_ADDR_TO_INTID(addr, 1);

uint32_t here please.

> +    u32 value = 0;

Same here.

> +    int i;
> +
> +    /* Loop over all IRQs affected by this read */
> +    for ( i = 0; i < len * 8; i++ )
> +    {
> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
> +
> +        if ( irq->enabled )
> +            value |= (1U << i);
> +
> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +
> +    return value;
> +}
> +
> +static void vgic_handle_hardware_irq(irq_desc_t *desc, int irq_type,

Looking below irq_type should a enum vgic_irq_config and not an int.

> +                     bool enable)

Indentation.

> +{
> +    unsigned long flags;
> +
> +//  irq_set_affinity(desc, cpumask_of(v_target->processor));

Why is that commented?

> +    spin_lock_irqsave(&desc->lock, flags);
> +    if ( enable )
> +    {
> +        gic_set_irq_type(desc, irq_type == VGIC_CONFIG_LEVEL ?
> +                 IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING);

Indentation and I would prefer a helper to convert between the vgic 
value and the IRQ_TYPE. This would make the code easier to read.

Also, this code does not replicate correctly the current vGIC. 
gic_set_irq_type is only allowed to be used when 
irq_set_type_by_domain(d) returns true. If you consider this change 
valid, then I would like to know why.

> +        desc->handler->enable(desc);
> +    }
> +    else
> +        desc->handler->disable(desc);
> +    spin_unlock_irqrestore(&desc->lock, flags);
> +}
> +
> +void vgic_mmio_write_senable(struct vcpu *vcpu,
> +                 paddr_t addr, unsigned int len,
> +                 unsigned long val)

Indentation.

> +{
> +    u32 intid = VGIC_ADDR_TO_INTID(addr, 1);

uint32_t.

> +    irq_desc_t *desc;
> +    int i;
> +    unsigned long flags;
> +    enum vgic_irq_config config;
> +
> +    for_each_set_bit( i, &val, len * 8 )
> +    {
> +        struct vgic_irq *irq;
> +
> +        irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
> +
> +        spin_lock_irqsave(&irq->irq_lock, flags);
> +        irq->enabled = true;
> +        if ( irq->hw )
> +        {
> +            /*
> +             * The irq cannot be a PPI, we only support delivery
> +             * of SPIs to guests.
> +             */
> +            ASSERT(irq->hwintid >= 32);
> +
> +            desc = irq_to_desc(irq->hwintid);

What is the rationale behind storing hwintid rather than the irq_desc 
directly?

> +            config = irq->config;
> +        }
> +        else
> +            desc = NULL;
> +        vgic_queue_irq_unlock(vcpu->domain, irq, flags);
> +
> +        vgic_put_irq(vcpu->domain, irq);
> +
> +        if ( desc )
> +            vgic_handle_hardware_irq(desc, config, true);

This is slightly strange. You handle the hardware IRQ outside the 
virtual IRQ lock. It means that the hardware IRQ may end up enabled but 
the virtual IRQ disabled.

> +    }
> +}
> +
> +void vgic_mmio_write_cenable(struct vcpu *vcpu,
> +                 paddr_t addr, unsigned int len,
> +                 unsigned long val)
> +{
> +    u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> +    int i;
> +
> +    for_each_set_bit( i, &val, len * 8 )
> +    {
> +        struct vgic_irq *irq;
> +        unsigned long flags;
> +        irq_desc_t *desc;
> +
> +        irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
> +        spin_lock_irqsave(&irq->irq_lock, flags);
> +
> +        irq->enabled = false;
> +
> +        if ( irq->hw )
> +            desc = irq_to_desc(irq->hwintid);
> +        else
> +            desc = NULL;
> +
> +        spin_unlock_irqrestore(&irq->irq_lock, flags);
> +        vgic_put_irq(vcpu->domain, irq);
> +
> +        if ( desc )
> +            vgic_handle_hardware_irq(desc, 0, false);

Same remark here.

> +    }
> +}
> +
>   static int match_region(const void *key, const void *elt)
>   {
>       const unsigned int offset = (unsigned long)key;
> diff --git a/xen/arch/arm/vgic/vgic-mmio.h b/xen/arch/arm/vgic/vgic-mmio.h
> index 10ac682296..9f34bd1aec 100644
> --- a/xen/arch/arm/vgic/vgic-mmio.h
> +++ b/xen/arch/arm/vgic/vgic-mmio.h
> @@ -137,6 +137,17 @@ unsigned long vgic_mmio_read_rao(struct vcpu *vcpu,
>   void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr,
>                           unsigned int len, unsigned long val);
>   
> +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
> +                    paddr_t addr, unsigned int len);

Indentation.

> +
> +void vgic_mmio_write_senable(struct vcpu *vcpu,
> +                 paddr_t addr, unsigned int len,
> +                 unsigned long val);

Ditto.

> +
> +void vgic_mmio_write_cenable(struct vcpu *vcpu,
> +                 paddr_t addr, unsigned int len,
> +                 unsigned long val);

Ditto.

> +
>   unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
>   
>   /* Find the proper register handler entry given a certain address offset */
> 

Cheers,
Andre Przywara Feb. 19, 2018, 12:41 p.m. | #2
Hi,

On 16/02/18 16:57, Julien Grall wrote:
> Hi Andre,
> 
> On 09/02/18 14:39, Andre Przywara wrote:
>> As the enable register handlers are shared between the v2 and v3
>> emulation, their implementation goes into vgic-mmio.c, to be easily
>> referenced from the v3 emulation as well later.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   xen/arch/arm/vgic/vgic-mmio-v2.c |   4 +-
>>   xen/arch/arm/vgic/vgic-mmio.c    | 114
>> +++++++++++++++++++++++++++++++++++++++
>>   xen/arch/arm/vgic/vgic-mmio.h    |  11 ++++
>>   3 files changed, 127 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c
>> b/xen/arch/arm/vgic/vgic-mmio-v2.c
>> index 0926b3243e..eca6840ff9 100644
>> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
>> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
>> @@ -74,10 +74,10 @@ static const struct vgic_register_region
>> vgic_v2_dist_registers[] = {
>>           vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
>>           VGIC_ACCESS_32bit),
>>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER,
>> -        vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
>> +        vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
>>           VGIC_ACCESS_32bit),
>>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER,
>> -        vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
>> +        vgic_mmio_read_enable, vgic_mmio_write_cenable, NULL, NULL, 1,
>>           VGIC_ACCESS_32bit),
>>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR,
>>           vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
>> diff --git a/xen/arch/arm/vgic/vgic-mmio.c
>> b/xen/arch/arm/vgic/vgic-mmio.c
>> index 59703a6909..3d9fa02a10 100644
>> --- a/xen/arch/arm/vgic/vgic-mmio.c
>> +++ b/xen/arch/arm/vgic/vgic-mmio.c
>> @@ -39,6 +39,120 @@ void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t
>> addr,
>>       /* Ignore */
>>   }
>>   +/*
>> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the
>> value
>> + * of the enabled bit, so there is only one function for both here.
>> + */
>> +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
>> +                    paddr_t addr, unsigned int len)
> 
> Indentation.
> 
>> +{
>> +    u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> 
> uint32_t here please.
> 
>> +    u32 value = 0;
> 
> Same here.
> 
>> +    int i;
>> +
>> +    /* Loop over all IRQs affected by this read */
>> +    for ( i = 0; i < len * 8; i++ )
>> +    {
>> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid
>> + i);
>> +
>> +        if ( irq->enabled )
>> +            value |= (1U << i);
>> +
>> +        vgic_put_irq(vcpu->domain, irq);
>> +    }
>> +
>> +    return value;
>> +}
>> +
>> +static void vgic_handle_hardware_irq(irq_desc_t *desc, int irq_type,
> 
> Looking below irq_type should a enum vgic_irq_config and not an int.
> 
>> +                     bool enable)
> 
> Indentation.
> 
>> +{
>> +    unsigned long flags;
>> +
>> +//  irq_set_affinity(desc, cpumask_of(v_target->processor));
> 
> Why is that commented?

That should indeed be a TODO:, actually.
As we already discussed, KVM does not implement this
hardware-follows-virtual affinity. This line is just copied from the old
VGIC, to remind me to address this. But I need to check the locking
order here first and if there are any other side effects of changing the
hardware affinity in this particular context.

>> +    spin_lock_irqsave(&desc->lock, flags);
>> +    if ( enable )
>> +    {
>> +        gic_set_irq_type(desc, irq_type == VGIC_CONFIG_LEVEL ?
>> +                 IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING);
> 
> Indentation and I would prefer a helper to convert between the vgic
> value and the IRQ_TYPE. This would make the code easier to read.
> 
> Also, this code does not replicate correctly the current vGIC.
> gic_set_irq_type is only allowed to be used when
> irq_set_type_by_domain(d) returns true. If you consider this change
> valid, then I would like to know why.

So what is/was the rationale for not allowing IRQ type changes for
non-privileged guests? If you allow to pass through an hardware IRQ to a
guest (which is the case this function handles), then I don't see why a
guest would not be allowed to change the configuration? It seems rather
odd, I guess it's up to the guest to know which type of IRQ this is?

>> +        desc->handler->enable(desc);
>> +    }
>> +    else
>> +        desc->handler->disable(desc);
>> +    spin_unlock_irqrestore(&desc->lock, flags);
>> +}
>> +
>> +void vgic_mmio_write_senable(struct vcpu *vcpu,
>> +                 paddr_t addr, unsigned int len,
>> +                 unsigned long val)
> 
> Indentation.
> 
>> +{
>> +    u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> 
> uint32_t.
> 
>> +    irq_desc_t *desc;
>> +    int i;
>> +    unsigned long flags;
>> +    enum vgic_irq_config config;
>> +
>> +    for_each_set_bit( i, &val, len * 8 )
>> +    {
>> +        struct vgic_irq *irq;
>> +
>> +        irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
>> +
>> +        spin_lock_irqsave(&irq->irq_lock, flags);
>> +        irq->enabled = true;
>> +        if ( irq->hw )
>> +        {
>> +            /*
>> +             * The irq cannot be a PPI, we only support delivery
>> +             * of SPIs to guests.
>> +             */
>> +            ASSERT(irq->hwintid >= 32);
>> +
>> +            desc = irq_to_desc(irq->hwintid);
> 
> What is the rationale behind storing hwintid rather than the irq_desc
> directly?
> 
>> +            config = irq->config;
>> +        }
>> +        else
>> +            desc = NULL;
>> +        vgic_queue_irq_unlock(vcpu->domain, irq, flags);
>> +
>> +        vgic_put_irq(vcpu->domain, irq);
>> +
>> +        if ( desc )
>> +            vgic_handle_hardware_irq(desc, config, true);
> 
> This is slightly strange. You handle the hardware IRQ outside the
> virtual IRQ lock. It means that the hardware IRQ may end up enabled but
> the virtual IRQ disabled.

Yeah, good catch. This can't be easily called before dropping the IRQ
lock, as this would violate the locking order.
But I can try to re-take the IRQ lock after having acquired the desc
lock, then use the enabled (and config) value from struct vgic_irq directly.

Cheers,
Andre.

> 
>> +    }
>> +}
>> +
>> +void vgic_mmio_write_cenable(struct vcpu *vcpu,
>> +                 paddr_t addr, unsigned int len,
>> +                 unsigned long val)
>> +{
>> +    u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>> +    int i;
>> +
>> +    for_each_set_bit( i, &val, len * 8 )
>> +    {
>> +        struct vgic_irq *irq;
>> +        unsigned long flags;
>> +        irq_desc_t *desc;
>> +
>> +        irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
>> +        spin_lock_irqsave(&irq->irq_lock, flags);
>> +
>> +        irq->enabled = false;
>> +
>> +        if ( irq->hw )
>> +            desc = irq_to_desc(irq->hwintid);
>> +        else
>> +            desc = NULL;
>> +
>> +        spin_unlock_irqrestore(&irq->irq_lock, flags);
>> +        vgic_put_irq(vcpu->domain, irq);
>> +
>> +        if ( desc )
>> +            vgic_handle_hardware_irq(desc, 0, false);
> 
> Same remark here.
> 
>> +    }
>> +}
>> +
>>   static int match_region(const void *key, const void *elt)
>>   {
>>       const unsigned int offset = (unsigned long)key;
>> diff --git a/xen/arch/arm/vgic/vgic-mmio.h
>> b/xen/arch/arm/vgic/vgic-mmio.h
>> index 10ac682296..9f34bd1aec 100644
>> --- a/xen/arch/arm/vgic/vgic-mmio.h
>> +++ b/xen/arch/arm/vgic/vgic-mmio.h
>> @@ -137,6 +137,17 @@ unsigned long vgic_mmio_read_rao(struct vcpu *vcpu,
>>   void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr,
>>                           unsigned int len, unsigned long val);
>>   +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
>> +                    paddr_t addr, unsigned int len);
> 
> Indentation.
> 
>> +
>> +void vgic_mmio_write_senable(struct vcpu *vcpu,
>> +                 paddr_t addr, unsigned int len,
>> +                 unsigned long val);
> 
> Ditto.
> 
>> +
>> +void vgic_mmio_write_cenable(struct vcpu *vcpu,
>> +                 paddr_t addr, unsigned int len,
>> +                 unsigned long val);
> 
> Ditto.
> 
>> +
>>   unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
>>     /* Find the proper register handler entry given a certain address
>> offset */
>>
> 
> Cheers,
>
Julien Grall Feb. 19, 2018, 2:13 p.m. | #3
On 19/02/18 12:41, Andre Przywara wrote:
> Hi,

Hi,

> On 16/02/18 16:57, Julien Grall wrote:
>> On 09/02/18 14:39, Andre Przywara wrote:
>>> +    spin_lock_irqsave(&desc->lock, flags);
>>> +    if ( enable )
>>> +    {
>>> +        gic_set_irq_type(desc, irq_type == VGIC_CONFIG_LEVEL ?
>>> +                 IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING);
>>
>> Indentation and I would prefer a helper to convert between the vgic
>> value and the IRQ_TYPE. This would make the code easier to read.
>>
>> Also, this code does not replicate correctly the current vGIC.
>> gic_set_irq_type is only allowed to be used when
>> irq_set_type_by_domain(d) returns true. If you consider this change
>> valid, then I would like to know why.
> 
> So what is/was the rationale for not allowing IRQ type changes for
> non-privileged guests? If you allow to pass through an hardware IRQ to a
> guest (which is the case this function handles), then I don't see why a
> guest would not be allowed to change the configuration? It seems rather
> odd, I guess it's up to the guest to know which type of IRQ this is?

If you can answer the question on top of irq_type_set_by_domain (i.e 
"See whether it is possible to let any domain configure the type) then 
we can remove it. We decided to only allow for the hardware domain 
because we trust it.

Cheers,

Patch

diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
index 0926b3243e..eca6840ff9 100644
--- a/xen/arch/arm/vgic/vgic-mmio-v2.c
+++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
@@ -74,10 +74,10 @@  static const struct vgic_register_region vgic_v2_dist_registers[] = {
         vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
         VGIC_ACCESS_32bit),
     REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER,
-        vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
+        vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
         VGIC_ACCESS_32bit),
     REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER,
-        vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
+        vgic_mmio_read_enable, vgic_mmio_write_cenable, NULL, NULL, 1,
         VGIC_ACCESS_32bit),
     REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR,
         vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c
index 59703a6909..3d9fa02a10 100644
--- a/xen/arch/arm/vgic/vgic-mmio.c
+++ b/xen/arch/arm/vgic/vgic-mmio.c
@@ -39,6 +39,120 @@  void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr,
     /* Ignore */
 }
 
+/*
+ * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value
+ * of the enabled bit, so there is only one function for both here.
+ */
+unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
+                    paddr_t addr, unsigned int len)
+{
+    u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+    u32 value = 0;
+    int i;
+
+    /* Loop over all IRQs affected by this read */
+    for ( i = 0; i < len * 8; i++ )
+    {
+        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
+
+        if ( irq->enabled )
+            value |= (1U << i);
+
+        vgic_put_irq(vcpu->domain, irq);
+    }
+
+    return value;
+}
+
+static void vgic_handle_hardware_irq(irq_desc_t *desc, int irq_type,
+                     bool enable)
+{
+    unsigned long flags;
+
+//  irq_set_affinity(desc, cpumask_of(v_target->processor));
+    spin_lock_irqsave(&desc->lock, flags);
+    if ( enable )
+    {
+        gic_set_irq_type(desc, irq_type == VGIC_CONFIG_LEVEL ?
+                 IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING);
+        desc->handler->enable(desc);
+    }
+    else
+        desc->handler->disable(desc);
+    spin_unlock_irqrestore(&desc->lock, flags);
+}
+
+void vgic_mmio_write_senable(struct vcpu *vcpu,
+                 paddr_t addr, unsigned int len,
+                 unsigned long val)
+{
+    u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+    irq_desc_t *desc;
+    int i;
+    unsigned long flags;
+    enum vgic_irq_config config;
+
+    for_each_set_bit( i, &val, len * 8 )
+    {
+        struct vgic_irq *irq;
+
+        irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
+
+        spin_lock_irqsave(&irq->irq_lock, flags);
+        irq->enabled = true;
+        if ( irq->hw )
+        {
+            /*
+             * The irq cannot be a PPI, we only support delivery
+             * of SPIs to guests.
+             */
+            ASSERT(irq->hwintid >= 32);
+
+            desc = irq_to_desc(irq->hwintid);
+            config = irq->config;
+        }
+        else
+            desc = NULL;
+        vgic_queue_irq_unlock(vcpu->domain, irq, flags);
+
+        vgic_put_irq(vcpu->domain, irq);
+
+        if ( desc )
+            vgic_handle_hardware_irq(desc, config, true);
+    }
+}
+
+void vgic_mmio_write_cenable(struct vcpu *vcpu,
+                 paddr_t addr, unsigned int len,
+                 unsigned long val)
+{
+    u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+    int i;
+
+    for_each_set_bit( i, &val, len * 8 )
+    {
+        struct vgic_irq *irq;
+        unsigned long flags;
+        irq_desc_t *desc;
+
+        irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
+        spin_lock_irqsave(&irq->irq_lock, flags);
+
+        irq->enabled = false;
+
+        if ( irq->hw )
+            desc = irq_to_desc(irq->hwintid);
+        else
+            desc = NULL;
+
+        spin_unlock_irqrestore(&irq->irq_lock, flags);
+        vgic_put_irq(vcpu->domain, irq);
+
+        if ( desc )
+            vgic_handle_hardware_irq(desc, 0, false);
+    }
+}
+
 static int match_region(const void *key, const void *elt)
 {
     const unsigned int offset = (unsigned long)key;
diff --git a/xen/arch/arm/vgic/vgic-mmio.h b/xen/arch/arm/vgic/vgic-mmio.h
index 10ac682296..9f34bd1aec 100644
--- a/xen/arch/arm/vgic/vgic-mmio.h
+++ b/xen/arch/arm/vgic/vgic-mmio.h
@@ -137,6 +137,17 @@  unsigned long vgic_mmio_read_rao(struct vcpu *vcpu,
 void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr,
                         unsigned int len, unsigned long val);
 
+unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
+                    paddr_t addr, unsigned int len);
+
+void vgic_mmio_write_senable(struct vcpu *vcpu,
+                 paddr_t addr, unsigned int len,
+                 unsigned long val);
+
+void vgic_mmio_write_cenable(struct vcpu *vcpu,
+                 paddr_t addr, unsigned int len,
+                 unsigned long val);
+
 unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
 
 /* Find the proper register handler entry given a certain address offset */