[Xen-devel,v3,19/39] ARM: new VGIC: Add ENABLE registers handlers

Message ID 20180321163235.12529-20-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 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.
This introduces a vgic_sync_hardware_irq() function, which updates the
physical side of a hardware mapped virtual IRQ.
Because the existing locking order between vgic_irq->irq_lock and
irq_desc->lock dictates so, we drop the irq_lock and retake them in the
proper order.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Reviewed-by: Julien Grall <julien.grall@arm.com>
---
Changelog v2 ... v3:
- fix indentation
- fix wording in comment
- add Reviewed-by:

Changelog v1 ... v2:
- ASSERT on h/w IRQ and vIRQ staying in sync

 xen/arch/arm/vgic/vgic-mmio-v2.c |   4 +-
 xen/arch/arm/vgic/vgic-mmio.c    | 117 +++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/vgic/vgic-mmio.h    |  11 ++++
 xen/arch/arm/vgic/vgic.c         |  40 +++++++++++++
 xen/arch/arm/vgic/vgic.h         |   3 +
 5 files changed, 173 insertions(+), 2 deletions(-)

Comments

Stefano Stabellini March 27, 2018, 9:06 p.m. | #1
On Wed, 21 Mar 2018, 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.
> This introduces a vgic_sync_hardware_irq() function, which updates the
> physical side of a hardware mapped virtual IRQ.
> Because the existing locking order between vgic_irq->irq_lock and
> irq_desc->lock dictates so, we drop the irq_lock and retake them in the
> proper order.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Reviewed-by: Julien Grall <julien.grall@arm.com>
> ---
> Changelog v2 ... v3:
> - fix indentation
> - fix wording in comment
> - add Reviewed-by:
> 
> Changelog v1 ... v2:
> - ASSERT on h/w IRQ and vIRQ staying in sync
> 
>  xen/arch/arm/vgic/vgic-mmio-v2.c |   4 +-
>  xen/arch/arm/vgic/vgic-mmio.c    | 117 +++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/vgic/vgic-mmio.h    |  11 ++++
>  xen/arch/arm/vgic/vgic.c         |  40 +++++++++++++
>  xen/arch/arm/vgic/vgic.h         |   3 +
>  5 files changed, 173 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
> index 43c1ab5906..7efd1c4eb4 100644
> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> @@ -89,10 +89,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
>          vgic_mmio_read_rao, vgic_mmio_write_wi, 1,
>          VGIC_ACCESS_32bit),
>      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER,
> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> +        vgic_mmio_read_enable, vgic_mmio_write_senable, 1,
>          VGIC_ACCESS_32bit),
>      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER,
> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> +        vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
>          VGIC_ACCESS_32bit),
>      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR,
>          vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c
> index a03e8d88b9..f219b7c509 100644
> --- a/xen/arch/arm/vgic/vgic-mmio.c
> +++ b/xen/arch/arm/vgic/vgic-mmio.c
> @@ -39,6 +39,123 @@ 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)
> +{
> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
> +    uint32_t value = 0;
> +    unsigned 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);

Don't we need to take the irq->irq_lock before reading irq->enabled?


> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +
> +    return value;
> +}
> +
> +void vgic_mmio_write_senable(struct vcpu *vcpu,
> +                             paddr_t addr, unsigned int len,
> +                             unsigned long val)
> +{
> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
> +    unsigned int i;
> +
> +    for_each_set_bit( i, &val, len * 8 )
> +    {
> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
> +        unsigned long flags;
> +        irq_desc_t *desc;
> +
> +        spin_lock_irqsave(&irq->irq_lock, flags);
> +
> +        if ( irq->enabled )            /* skip already enabled IRQs */
> +        {
> +            spin_unlock_irqrestore(&irq->irq_lock, flags);
> +            vgic_put_irq(vcpu->domain, irq);
> +            continue;
> +        }
> +
> +        irq->enabled = true;
> +        if ( irq->hw )
> +        {
> +            /*
> +             * The irq cannot be a PPI, we only support delivery
> +             * of SPIs to guests.
> +             */
> +            ASSERT(irq->hwintid >= VGIC_NR_PRIVATE_IRQS);
> +
> +            desc = irq_to_desc(irq->hwintid);
> +        }
> +        else
> +            desc = NULL;
> +
> +        vgic_queue_irq_unlock(vcpu->domain, irq, flags);
> +
> +        if ( desc )
> +            vgic_sync_hardware_irq(vcpu->domain, desc, irq);
> +
> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +}
> +
> +void vgic_mmio_write_cenable(struct vcpu *vcpu,
> +                             paddr_t addr, unsigned int len,
> +                             unsigned long val)
> +{
> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
> +    unsigned 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);
> +
> +        if ( !irq->enabled )            /* skip already disabled IRQs */
> +        {
> +            spin_unlock_irqrestore(&irq->irq_lock, flags);
> +            vgic_put_irq(vcpu->domain, irq);
> +            continue;
> +        }
> +
> +        irq->enabled = false;
> +
> +        if ( irq->hw )
> +        {
> +            /*
> +             * The irq cannot be a PPI, we only support delivery
> +             * of SPIs to guests.
> +             */
> +            ASSERT(irq->hwintid >= VGIC_NR_PRIVATE_IRQS);
> +
> +            desc = irq_to_desc(irq->hwintid);
> +        }
> +        else
> +            desc = NULL;
> +
> +        spin_unlock_irqrestore(&irq->irq_lock, flags);
> +
> +        if ( desc )
> +            vgic_sync_hardware_irq(vcpu->domain, desc, irq);
> +
> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +}
> +
>  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 c280668694..a2cebd77f4 100644
> --- a/xen/arch/arm/vgic/vgic-mmio.h
> +++ b/xen/arch/arm/vgic/vgic-mmio.h
> @@ -86,6 +86,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);
>  
>  #endif
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index 37b425a16c..90041eb071 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -699,6 +699,46 @@ void vgic_kick_vcpus(struct domain *d)
>      }
>  }
>  
> +static unsigned int translate_irq_type(bool is_level)
> +{
> +    return is_level ? IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING;
> +}
> +
> +void vgic_sync_hardware_irq(struct domain *d,
> +                            irq_desc_t *desc, struct vgic_irq *irq)
> +{
> +    unsigned long flags;
> +
> +    spin_lock_irqsave(&desc->lock, flags);
> +    spin_lock(&irq->irq_lock);
> +
> +    /*
> +     * We forbid tinkering with the hardware IRQ association during
> +     * a domain's lifetime.
> +     */
> +    ASSERT(irq->hw && desc->irq == irq->hwintid);
> +
> +    if ( irq->enabled )
> +    {
> +        /*
> +         * We might end up from various callers, so check that the
> +         * interrrupt is disabled before trying to change the config.
> +         */
> +        if ( irq_type_set_by_domain(d) &&
> +             test_bit(_IRQ_DISABLED, &desc->status) )
> +            gic_set_irq_type(desc, translate_irq_type(irq->config));
> +
> +        if ( irq->target_vcpu )
> +            irq_set_affinity(desc, cpumask_of(irq->target_vcpu->processor));
> +        desc->handler->enable(desc);
> +    }
> +    else
> +        desc->handler->disable(desc);
> +
> +    spin_unlock(&irq->irq_lock);
> +    spin_unlock_irqrestore(&desc->lock, flags);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
> index aed7e4179a..071e061066 100644
> --- a/xen/arch/arm/vgic/vgic.h
> +++ b/xen/arch/arm/vgic/vgic.h
> @@ -55,6 +55,9 @@ static inline void vgic_get_irq_kref(struct vgic_irq *irq)
>      atomic_inc(&irq->refcount);
>  }
>  
> +void vgic_sync_hardware_irq(struct domain *d,
> +                            irq_desc_t *desc, struct vgic_irq *irq);
> +
>  void vgic_v2_fold_lr_state(struct vcpu *vcpu);
>  void vgic_v2_populate_lr(struct vcpu *vcpu, struct vgic_irq *irq, int lr);
>  void vgic_v2_set_underflow(struct vcpu *vcpu);
> -- 
> 2.14.1
>
Andre Przywara March 28, 2018, 9:09 a.m. | #2
Hi,

On 27/03/18 22:06, Stefano Stabellini wrote:
> On Wed, 21 Mar 2018, 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.
>> This introduces a vgic_sync_hardware_irq() function, which updates the
>> physical side of a hardware mapped virtual IRQ.
>> Because the existing locking order between vgic_irq->irq_lock and
>> irq_desc->lock dictates so, we drop the irq_lock and retake them in the
>> proper order.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
>> ---
>> Changelog v2 ... v3:
>> - fix indentation
>> - fix wording in comment
>> - add Reviewed-by:
>>
>> Changelog v1 ... v2:
>> - ASSERT on h/w IRQ and vIRQ staying in sync
>>
>>  xen/arch/arm/vgic/vgic-mmio-v2.c |   4 +-
>>  xen/arch/arm/vgic/vgic-mmio.c    | 117 +++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/vgic/vgic-mmio.h    |  11 ++++
>>  xen/arch/arm/vgic/vgic.c         |  40 +++++++++++++
>>  xen/arch/arm/vgic/vgic.h         |   3 +
>>  5 files changed, 173 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
>> index 43c1ab5906..7efd1c4eb4 100644
>> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
>> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
>> @@ -89,10 +89,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
>>          vgic_mmio_read_rao, vgic_mmio_write_wi, 1,
>>          VGIC_ACCESS_32bit),
>>      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER,
>> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
>> +        vgic_mmio_read_enable, vgic_mmio_write_senable, 1,
>>          VGIC_ACCESS_32bit),
>>      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER,
>> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
>> +        vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
>>          VGIC_ACCESS_32bit),
>>      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR,
>>          vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
>> diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c
>> index a03e8d88b9..f219b7c509 100644
>> --- a/xen/arch/arm/vgic/vgic-mmio.c
>> +++ b/xen/arch/arm/vgic/vgic-mmio.c
>> @@ -39,6 +39,123 @@ 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)
>> +{
>> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
>> +    uint32_t value = 0;
>> +    unsigned 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);
> 
> Don't we need to take the irq->irq_lock before reading irq->enabled?

Not really. A boolean has no illegal state, so we can't read any
intermediate values.

If you think about concurrent writes: That is even racy on real
hardware, and normally you expect a sane driver to take a lock around
every distributor access (cf. spin_lock(&gicv2.lock)).
Keep in mind that only a guest can change the enabled state.

So the rationale behind those unlocked reads is:
As long as it doesn't harm the hypervisor, we don't care too much about
being 100% correct in a situation that is out of spec anyway.
We discussed this issue also with Julien before:
https://lists.xen.org/archives/html/xen-devel/2018-02/msg02148.html

Cheers,
Andre.
Stefano Stabellini March 28, 2018, 5:19 p.m. | #3
On Wed, 28 Mar 2018, Andre Przywara wrote:
> Hi,
> 
> On 27/03/18 22:06, Stefano Stabellini wrote:
> > On Wed, 21 Mar 2018, 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.
> >> This introduces a vgic_sync_hardware_irq() function, which updates the
> >> physical side of a hardware mapped virtual IRQ.
> >> Because the existing locking order between vgic_irq->irq_lock and
> >> irq_desc->lock dictates so, we drop the irq_lock and retake them in the
> >> proper order.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> >> Reviewed-by: Julien Grall <julien.grall@arm.com>
> >> ---
> >> Changelog v2 ... v3:
> >> - fix indentation
> >> - fix wording in comment
> >> - add Reviewed-by:
> >>
> >> Changelog v1 ... v2:
> >> - ASSERT on h/w IRQ and vIRQ staying in sync
> >>
> >>  xen/arch/arm/vgic/vgic-mmio-v2.c |   4 +-
> >>  xen/arch/arm/vgic/vgic-mmio.c    | 117 +++++++++++++++++++++++++++++++++++++++
> >>  xen/arch/arm/vgic/vgic-mmio.h    |  11 ++++
> >>  xen/arch/arm/vgic/vgic.c         |  40 +++++++++++++
> >>  xen/arch/arm/vgic/vgic.h         |   3 +
> >>  5 files changed, 173 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
> >> index 43c1ab5906..7efd1c4eb4 100644
> >> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> >> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> >> @@ -89,10 +89,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> >>          vgic_mmio_read_rao, vgic_mmio_write_wi, 1,
> >>          VGIC_ACCESS_32bit),
> >>      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER,
> >> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> >> +        vgic_mmio_read_enable, vgic_mmio_write_senable, 1,
> >>          VGIC_ACCESS_32bit),
> >>      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER,
> >> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> >> +        vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
> >>          VGIC_ACCESS_32bit),
> >>      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR,
> >>          vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> >> diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c
> >> index a03e8d88b9..f219b7c509 100644
> >> --- a/xen/arch/arm/vgic/vgic-mmio.c
> >> +++ b/xen/arch/arm/vgic/vgic-mmio.c
> >> @@ -39,6 +39,123 @@ 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)
> >> +{
> >> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
> >> +    uint32_t value = 0;
> >> +    unsigned 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);
> > 
> > Don't we need to take the irq->irq_lock before reading irq->enabled?
> 
> Not really. A boolean has no illegal state, so we can't read any
> intermediate values.
> 
> If you think about concurrent writes: That is even racy on real
> hardware, and normally you expect a sane driver to take a lock around
> every distributor access (cf. spin_lock(&gicv2.lock)).
> Keep in mind that only a guest can change the enabled state.
> 
> So the rationale behind those unlocked reads is:
> As long as it doesn't harm the hypervisor, we don't care too much about
> being 100% correct in a situation that is out of spec anyway.
> We discussed this issue also with Julien before:
> https://lists.xen.org/archives/html/xen-devel/2018-02/msg02148.html

OK, I buy the argument.

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

Patch

diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
index 43c1ab5906..7efd1c4eb4 100644
--- a/xen/arch/arm/vgic/vgic-mmio-v2.c
+++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
@@ -89,10 +89,10 @@  static const struct vgic_register_region vgic_v2_dist_registers[] = {
         vgic_mmio_read_rao, vgic_mmio_write_wi, 1,
         VGIC_ACCESS_32bit),
     REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER,
-        vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
+        vgic_mmio_read_enable, vgic_mmio_write_senable, 1,
         VGIC_ACCESS_32bit),
     REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER,
-        vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
+        vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
         VGIC_ACCESS_32bit),
     REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR,
         vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c
index a03e8d88b9..f219b7c509 100644
--- a/xen/arch/arm/vgic/vgic-mmio.c
+++ b/xen/arch/arm/vgic/vgic-mmio.c
@@ -39,6 +39,123 @@  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)
+{
+    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
+    uint32_t value = 0;
+    unsigned 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;
+}
+
+void vgic_mmio_write_senable(struct vcpu *vcpu,
+                             paddr_t addr, unsigned int len,
+                             unsigned long val)
+{
+    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
+    unsigned int i;
+
+    for_each_set_bit( i, &val, len * 8 )
+    {
+        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
+        unsigned long flags;
+        irq_desc_t *desc;
+
+        spin_lock_irqsave(&irq->irq_lock, flags);
+
+        if ( irq->enabled )            /* skip already enabled IRQs */
+        {
+            spin_unlock_irqrestore(&irq->irq_lock, flags);
+            vgic_put_irq(vcpu->domain, irq);
+            continue;
+        }
+
+        irq->enabled = true;
+        if ( irq->hw )
+        {
+            /*
+             * The irq cannot be a PPI, we only support delivery
+             * of SPIs to guests.
+             */
+            ASSERT(irq->hwintid >= VGIC_NR_PRIVATE_IRQS);
+
+            desc = irq_to_desc(irq->hwintid);
+        }
+        else
+            desc = NULL;
+
+        vgic_queue_irq_unlock(vcpu->domain, irq, flags);
+
+        if ( desc )
+            vgic_sync_hardware_irq(vcpu->domain, desc, irq);
+
+        vgic_put_irq(vcpu->domain, irq);
+    }
+}
+
+void vgic_mmio_write_cenable(struct vcpu *vcpu,
+                             paddr_t addr, unsigned int len,
+                             unsigned long val)
+{
+    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
+    unsigned 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);
+
+        if ( !irq->enabled )            /* skip already disabled IRQs */
+        {
+            spin_unlock_irqrestore(&irq->irq_lock, flags);
+            vgic_put_irq(vcpu->domain, irq);
+            continue;
+        }
+
+        irq->enabled = false;
+
+        if ( irq->hw )
+        {
+            /*
+             * The irq cannot be a PPI, we only support delivery
+             * of SPIs to guests.
+             */
+            ASSERT(irq->hwintid >= VGIC_NR_PRIVATE_IRQS);
+
+            desc = irq_to_desc(irq->hwintid);
+        }
+        else
+            desc = NULL;
+
+        spin_unlock_irqrestore(&irq->irq_lock, flags);
+
+        if ( desc )
+            vgic_sync_hardware_irq(vcpu->domain, desc, irq);
+
+        vgic_put_irq(vcpu->domain, irq);
+    }
+}
+
 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 c280668694..a2cebd77f4 100644
--- a/xen/arch/arm/vgic/vgic-mmio.h
+++ b/xen/arch/arm/vgic/vgic-mmio.h
@@ -86,6 +86,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);
 
 #endif
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index 37b425a16c..90041eb071 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -699,6 +699,46 @@  void vgic_kick_vcpus(struct domain *d)
     }
 }
 
+static unsigned int translate_irq_type(bool is_level)
+{
+    return is_level ? IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING;
+}
+
+void vgic_sync_hardware_irq(struct domain *d,
+                            irq_desc_t *desc, struct vgic_irq *irq)
+{
+    unsigned long flags;
+
+    spin_lock_irqsave(&desc->lock, flags);
+    spin_lock(&irq->irq_lock);
+
+    /*
+     * We forbid tinkering with the hardware IRQ association during
+     * a domain's lifetime.
+     */
+    ASSERT(irq->hw && desc->irq == irq->hwintid);
+
+    if ( irq->enabled )
+    {
+        /*
+         * We might end up from various callers, so check that the
+         * interrrupt is disabled before trying to change the config.
+         */
+        if ( irq_type_set_by_domain(d) &&
+             test_bit(_IRQ_DISABLED, &desc->status) )
+            gic_set_irq_type(desc, translate_irq_type(irq->config));
+
+        if ( irq->target_vcpu )
+            irq_set_affinity(desc, cpumask_of(irq->target_vcpu->processor));
+        desc->handler->enable(desc);
+    }
+    else
+        desc->handler->disable(desc);
+
+    spin_unlock(&irq->irq_lock);
+    spin_unlock_irqrestore(&desc->lock, flags);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
index aed7e4179a..071e061066 100644
--- a/xen/arch/arm/vgic/vgic.h
+++ b/xen/arch/arm/vgic/vgic.h
@@ -55,6 +55,9 @@  static inline void vgic_get_irq_kref(struct vgic_irq *irq)
     atomic_inc(&irq->refcount);
 }
 
+void vgic_sync_hardware_irq(struct domain *d,
+                            irq_desc_t *desc, struct vgic_irq *irq);
+
 void vgic_v2_fold_lr_state(struct vcpu *vcpu);
 void vgic_v2_populate_lr(struct vcpu *vcpu, struct vgic_irq *irq, int lr);
 void vgic_v2_set_underflow(struct vcpu *vcpu);