Re: [Xen-devel] [RFC PATCH 35/49] ARM: new VGIC: Add TARGET registers handlers

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 09/02/18 14:39, Andre Przywara wrote:
> The target register handlers are v2 emulation specific, so their
> implementation lives entirely in vgic-mmio-v2.c.
> We copy the old VGIC behaviour of assigning an IRQ to the first VCPU
> set in the target mask instead of making it possibly pending on
> multiple VCPUs.
>
> This is based on Linux commit 2c234d6f1826, written by Andre Przywara.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> ---
>   xen/arch/arm/vgic/vgic-mmio-v2.c | 52 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
> index c0b88b347e..c59f2c1ba7 100644
> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> @@ -66,6 +66,56 @@ static void vgic_mmio_write_v2_misc(struct vcpu *vcpu,
>       }
>   }
>   
> +static unsigned long vgic_mmio_read_target(struct vcpu *vcpu,
> +                       paddr_t addr, unsigned int len)
Indentation

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

> +    int i;
unsigned int

> +    u64 val = 0;

Why 64-bit? IIRC, the target register is 32-bit.


> +
> +    for ( i = 0; i < len; i++ )
> +    {
> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain,
> +                            vcpu, intid + i);
Indenation.

> +
> +        val |= (u64)irq->targets << (i * 8);
See above regarding u64.

> +
> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +
> +    return val;
> +}
> +
> +static void vgic_mmio_write_target(struct vcpu *vcpu,
> +                   paddr_t addr, unsigned int len,
> +                   unsigned long val)
Indentation

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

> +    u8 cpu_mask = GENMASK(vcpu->domain->max_vcpus - 1, 0);
uint32_t

> +    int i;
unsigned int

> +    unsigned long flags;
> +
> +    /* GICD_ITARGETSR[0-7] are read-only */
> +    if ( intid < VGIC_NR_PRIVATE_IRQS )
> +        return;
> +
> +    for ( i = 0; i < len; i++ )
> +    {
> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain,
> +                            NULL, intid + i);
> +        int target;
unsigned int

> +
> +        spin_lock_irqsave(&irq->irq_lock, flags);
> +
> +        irq->targets = (val >> (i * 8)) & cpu_mask;
> +        target = irq->targets ? (ffs(irq->targets) - 1) : 0;
Here you will route the IRQ to vCPU 0 if the mask is invalid. Is it 
intended? Do you have a pointer in the spec?
> +        irq->target_vcpu = vcpu->domain->vcpu[target];
> +
> +        spin_unlock_irqrestore(&irq->irq_lock, flags);
> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +}
> +
>   static const struct vgic_register_region vgic_v2_dist_registers[] = {
>       REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
>           vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
> @@ -95,7 +145,7 @@ static const struct vgic_register_region 
> vgic_v2_dist_registers[] = {
>           vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
>           8, VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ITARGETSR,
> -        vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 8,
> +        vgic_mmio_read_target, vgic_mmio_write_target, NULL, NULL, 8,
>           VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICFGR,
>           vgic_mmio_read_config, vgic_mmio_write_config, NULL, NULL, 2,
Cheers,

--
Julien Grall

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