[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,
Andre Przywara Feb. 23, 2018, 3:18 p.m. | #4
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?
> 
>> +    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?

Well, this is because KVM does it this way, for abstraction reasons,
mostly. Looking over the users I see that mostly we are indeed after the
struct irq_desc. But it would also increase struct vgic_irq by 4 bytes ;-)

I could try to make to make the change, but am not fully convinced.

What are your arguments for that change?

Cheers,
Andre.

>> +            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,
>
Julien Grall Feb. 26, 2018, 11:20 a.m. | #5
Hi Andre,

On 23/02/18 15:18, Andre Przywara wrote:
>>> +    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?
> 
> Well, this is because KVM does it this way, for abstraction reasons,
> mostly. Looking over the users I see that mostly we are indeed after the
> struct irq_desc. But it would also increase struct vgic_irq by 4 bytes ;-)
> 
> I could try to make to make the change, but am not fully convinced.
> 
> What are your arguments for that change?

To be honest, I don't have much arguments :). My main concern is using 
irq_to_desc wrongly when we will add support for routing PPI to domains.
This would useful to support the virtual timer without the hack we 
currently have.

In the PPI context, irq_to_desc would always return the PPI irq_desc of 
the current CPU. I am not entirely if this will always be ok for us. But 
I might be over cautious :).

So I guess, we can keep it like that for now.

Cheers,
Andre Przywara Feb. 27, 2018, 1:54 p.m. | #6
Hi,

On 19/02/18 14:13, Julien Grall wrote:
> 
> 
> 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.

But why would you mistrust a DomU in this respect?
The only point I see is that a guest has *some* influence on a hardware
access, but I fail to see how a single MMIO read-modify-write sequence
would actually impact the host. Especially since we do it only on
enabling an IRQ.
Looking more closely at the existing VGIC code we might want to check if
the hardware IRQ was already enabled before entering the
"if ( p->desc != NULL )" branch, btw.

So is this the concern? Commit b0003bdd690 wasn't really enlightening in
this respect.

Cheers,
Andre.
Julien Grall Feb. 27, 2018, 2:34 p.m. | #7
On 27/02/18 13:54, Andre Przywara wrote:
> Hi,
> 
> On 19/02/18 14:13, Julien Grall wrote:
>>
>>
>> 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.
> 
> But why would you mistrust a DomU in this respect?
> The only point I see is that a guest has *some* influence on a hardware
> access, but I fail to see how a single MMIO read-modify-write sequence
> would actually impact the host. Especially since we do it only on
> enabling an IRQ.
> Looking more closely at the existing VGIC code we might want to check if
> the hardware IRQ was already enabled before entering the
> "if ( p->desc != NULL )" branch, btw.

That's not an issue here. You can only enter in vgic_enable_irqs if the 
virtual interrupt was previously disabled. Because the physical 
interrupt is routed to the guest, it will also be disabled at that time.

> 
> So is this the concern? Commit b0003bdd690 wasn't really enlightening in
> this respect.

It was not really clear if it would be an issue when I wrote the patch. 
We trust the hardware domain so it is fine to let him configure the 
interrupt. For the guests, this will be taken from the DT (see 
gic_route_irq_to_guest). So this is likely to get configured correctly 
for the guest.

What I was worry about is whether we need to sanitize the ICFGR when the 
interrupt is routed to another domain. But if you can clear that, then I 
guess it should be ok.

However, I would prefer to do this in a separate patch and keep 
irq_type_set_by_domain around. This is to match the current vGIC and not 
changing too much Xen behavior.

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 */