Re: [Xen-devel] [PATCH 43/57] ARM: new VGIC: Add SGIR register handler

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 05/03/18 16:04, Andre Przywara wrote:
> Triggering an IPI via this register is v2 specific, so the
> implementation lives entirely in vgic-mmio-v2.c.
>
> This is based on Linux commit 55cc01fb9004, written by Andre Przywara.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> ---
> Changelog RFC ... v1:
> - use symbolic names instead of magic values
> - iterates over set bits instead of every VCPU
>
>   xen/arch/arm/vgic/vgic-mmio-v2.c | 47 ++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
> index 01c6a7198c..5f1fdb9a70 100644
> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> @@ -72,6 +72,49 @@ static void vgic_mmio_write_v2_misc(struct vcpu *vcpu,
>       }
>   }
>   
> +static void vgic_mmio_write_sgir(struct vcpu *source_vcpu,
> +                                 paddr_t addr, unsigned int len,
> +                                 unsigned long val)
> +{
> +    struct domain *d = source_vcpu->domain;
> +    unsigned int nr_vcpus = d->max_vcpus;
> +    unsigned int intid = val & GICD_SGI_INTID_MASK;
> +    unsigned long targets = (val & GICD_SGI_TARGET_MASK) >>
> +                            GICD_SGI_TARGET_SHIFT;
> +    unsigned int vcpu_id;
> +
> +    switch ( val & GICD_SGI_TARGET_LIST_MASK )
> +    {
> +    case GICD_SGI_TARGET_LIST:                    /* as specified by targets */
> +        targets &= GENMASK(nr_vcpus, 0);          /* limit to existing VCPUs */
> +        break;
> +    case GICD_SGI_TARGET_OTHERS:
> +        targets = GENMASK(nr_vcpus, 0);           /* all, ...   */
> +        targets &= ~(1U << source_vcpu->vcpu_id); /*   but self */
> +        break;
> +    case GICD_SGI_TARGET_SELF:                    /* this very vCPU only */
> +        targets = (1U << source_vcpu->vcpu_id);
> +        break;
> +    case 0x3:                                     /* reserved */
> +        return;
> +    }
> +
> +    for_each_set_bit( vcpu_id, &targets, 8 )
> +    {
> +        struct vcpu *vcpu = d->vcpu[vcpu_id];
> +        struct vgic_irq *irq = vgic_get_irq(d, vcpu, intid);
> +        unsigned long flags;
> +
> +        spin_lock_irqsave(&irq->irq_lock, flags);
> +
> +        irq->pending_latch = true;
> +        irq->source |= 1U << source_vcpu->vcpu_id;
> +
> +        vgic_queue_irq_unlock(d, irq, flags);
> +        vgic_put_irq(d, irq);
> +    }
> +}
> +
>   static unsigned long vgic_mmio_read_target(struct vcpu *vcpu,
>                                              paddr_t addr, unsigned int len)
>   {
> @@ -128,7 +171,7 @@ static void vgic_mmio_write_target(struct vcpu *vcpu,
>           spin_unlock_irqrestore(&irq->irq_lock, flags);
>   
>           if ( desc )
> -            vgic_update_hardware_irq(desc, irq);
> +            vgic_sync_hardware_irq(vcpu->domain, desc, irq);
This belongs to the previous patch. But calling vgic_sync_hardware_irq 
is pretty inefficient. You will end up to call ->enable everytime you 
route an interrupt.
Also, you don't really need to take the irq->desc_lock for the ITARGET 
case as you will not modify it. So you could just call irq_set_affinity 
directly.
>   
>           vgic_put_irq(vcpu->domain, irq);
>       }
> @@ -169,7 +212,7 @@ static const struct vgic_register_region 
> vgic_v2_dist_registers[] = {
>           vgic_mmio_read_config, vgic_mmio_write_config, 2,
>           VGIC_ACCESS_32bit),
>       REGISTER_DESC_WITH_LENGTH(GICD_SGIR,
> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
> +        vgic_mmio_read_raz, vgic_mmio_write_sgir, 4,
>           VGIC_ACCESS_32bit),
>       REGISTER_DESC_WITH_LENGTH(GICD_CPENDSGIR,
>           vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
Cheers,

--
Julien Grall

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