Re: [RFC PATCH 30/49] ARM: new VGIC: Add ENABLE registers handlers

[Andre Przywara <andre.przywara@linaro.org>]

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,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel