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

[Julien Grall <julien.grall@arm.com>]

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,

-- 
Julien Grall

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