Re: [RFC PATCH 24/49] ARM: new VGIC: Add IRQ sync/flush framework

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

Hi Andre,

On 09/02/18 14:39, Andre Przywara wrote:
> Implement the framework for syncing IRQs between our emulation and the
> list registers, which represent the guest's view of IRQs.
> This is done in kvm_vgic_flush_hwstate and kvm_vgic_sync_hwstate, which

You probably want to update the names here.

> gets called on guest entry and exit.
> The code talking to the actual GICv2/v3 hardware is added in the
> following patches.
> 
> This is based on Linux commit 0919e84c0fc1, written by Marc Zyngier.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>   xen/arch/arm/vgic/vgic.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/vgic/vgic.h |   2 +
>   2 files changed, 248 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index a4efd1fd03..a1f77130d4 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -380,6 +380,252 @@ int vgic_inject_irq(struct domain *d, struct vcpu *vcpu, unsigned int intid,
>       return 0;
>   }
>   
> +/**
> + * vgic_prune_ap_list - Remove non-relevant interrupts from the list
> + *
> + * @vcpu: The VCPU pointer
> + *
> + * Go over the list of "interesting" interrupts, and prune those that we
> + * won't have to consider in the near future.
> + */
> +static void vgic_prune_ap_list(struct vcpu *vcpu)
> +{
> +    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +    struct vgic_irq *irq, *tmp;
> +    unsigned long flags;
> +
> +retry:
> +    spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags);
> +
> +    list_for_each_entry_safe( irq, tmp, &vgic_cpu->ap_list_head, ap_list )

See my comment on patch #22, this is where I am worry about going 
through the list every time we enter to the hypervisor from the guest.

> +    {
> +        struct vcpu *target_vcpu, *vcpuA, *vcpuB;
> +
> +        spin_lock(&irq->irq_lock);
> +
> +        BUG_ON(vcpu != irq->vcpu);
> +
> +        target_vcpu = vgic_target_oracle(irq);
> +
> +        if ( !target_vcpu )
> +        {
> +            /*
> +             * We don't need to process this interrupt any
> +             * further, move it off the list.
> +             */
> +            list_del(&irq->ap_list);
> +            irq->vcpu = NULL;
> +            spin_unlock(&irq->irq_lock);
> +
> +            /*
> +             * This vgic_put_irq call matches the
> +             * vgic_get_irq_kref in vgic_queue_irq_unlock,
> +             * where we added the LPI to the ap_list. As
> +             * we remove the irq from the list, we drop
> +             * also drop the refcount.
> +             */
> +            vgic_put_irq(vcpu->domain, irq);
> +            continue;
> +        }
> +
> +        if ( target_vcpu == vcpu )
> +        {
> +            /* We're on the right CPU */
> +            spin_unlock(&irq->irq_lock);
> +            continue;
> +        }
> +
> +        /* This interrupt looks like it has to be migrated. */
> +
> +        spin_unlock(&irq->irq_lock);
> +        spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags);
> +
> +        /*
> +         * Ensure locking order by always locking the smallest
> +         * ID first.
> +         */
> +        if ( vcpu->vcpu_id < target_vcpu->vcpu_id )
> +        {
> +            vcpuA = vcpu;
> +            vcpuB = target_vcpu;
> +        }
> +        else
> +        {
> +            vcpuA = target_vcpu;
> +            vcpuB = vcpu;
> +        }
> +
> +        spin_lock_irqsave(&vcpuA->arch.vgic_cpu.ap_list_lock, flags);
> +        spin_lock(&vcpuB->arch.vgic_cpu.ap_list_lock);
> +        spin_lock(&irq->irq_lock);
> +
> +        /*
> +         * If the affinity has been preserved, move the
> +         * interrupt around. Otherwise, it means things have
> +         * changed while the interrupt was unlocked, and we
> +         * need to replay this.
> +         *
> +         * In all cases, we cannot trust the list not to have
> +         * changed, so we restart from the beginning.
> +         */
> +        if ( target_vcpu == vgic_target_oracle(irq) )
> +        {
> +            struct vgic_cpu *new_cpu = &target_vcpu->arch.vgic_cpu;
> +
> +            list_del(&irq->ap_list);
> +            irq->vcpu = target_vcpu;
> +            list_add_tail(&irq->ap_list, &new_cpu->ap_list_head);
> +        }
> +
> +        spin_unlock(&irq->irq_lock);
> +        spin_unlock(&vcpuB->arch.vgic_cpu.ap_list_lock);
> +        spin_unlock_irqrestore(&vcpuA->arch.vgic_cpu.ap_list_lock, flags);
> +        goto retry;
> +    }
> +
> +    spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags);
> +}
> +
> +static inline void vgic_fold_lr_state(struct vcpu *vcpu)
> +{
> +}
> +
> +/* Requires the irq_lock to be held. */
> +static inline void vgic_populate_lr(struct vcpu *vcpu,
> +                                    struct vgic_irq *irq, int lr)
> +{
> +    ASSERT(spin_is_locked(&irq->irq_lock));
> +}
> +
> +static inline void vgic_clear_lr(struct vcpu *vcpu, int lr)
> +{
> +}
> +
> +static inline void vgic_set_underflow(struct vcpu *vcpu)
> +{
> +}
> +
> +/* Requires the ap_list_lock to be held. */
> +static int compute_ap_list_depth(struct vcpu *vcpu)
> +{
> +    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +    struct vgic_irq *irq;
> +    int count = 0;
> +
> +    ASSERT(spin_is_locked(&vgic_cpu->ap_list_lock));
> +
> +    list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list)

Here another example.

> +    {
> +        spin_lock(&irq->irq_lock);
> +        /* GICv2 SGIs can count for more than one... */
> +        if ( vgic_irq_is_sgi(irq->intid) && irq->source )
> +            count += hweight8(irq->source);
> +        else
> +            count++;
> +        spin_unlock(&irq->irq_lock);
> +    }
> +    return count;
> +}
> +
> +/* Requires the VCPU's ap_list_lock to be held. */
> +static void vgic_flush_lr_state(struct vcpu *vcpu)
> +{
> +    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +    struct vgic_irq *irq;
> +    int count = 0;
> +
> +    ASSERT(spin_is_locked(&vgic_cpu->ap_list_lock));
> +
> +    if ( compute_ap_list_depth(vcpu) > gic_get_nr_lrs() )
> +        vgic_sort_ap_list(vcpu);
> +
> +    list_for_each_entry( irq, &vgic_cpu->ap_list_head, ap_list )
> +    {
> +        spin_lock(&irq->irq_lock);
> +
> +        if ( unlikely(vgic_target_oracle(irq) != vcpu) )
> +            goto next;
> +
> +        /*
> +         * If we get an SGI with multiple sources, try to get
> +         * them in all at once.
> +         */
> +        do
> +        {
> +            vgic_populate_lr(vcpu, irq, count++);
> +        } while ( irq->source && count < gic_get_nr_lrs() );
> +
> +next:
> +        spin_unlock(&irq->irq_lock);
> +
> +        if ( count == gic_get_nr_lrs() )
> +        {
> +            if ( !list_is_last(&irq->ap_list, &vgic_cpu->ap_list_head) )
> +                vgic_set_underflow(vcpu);
> +            break;
> +        }
> +    }
> +
> +    vcpu->arch.vgic_cpu.used_lrs = count;
> +
> +    /* Nuke remaining LRs */
> +    for ( ; count < gic_get_nr_lrs(); count++)
> +        vgic_clear_lr(vcpu, count);
> +}
> +
> +/*
> + * gic_clear_lrs() - Update the VGIC state from hardware after a guest's run.
> + * @vcpu: the VCPU.
> + *
> + * Sync back the hardware VGIC state after the guest has run, into our
> + * VGIC emulation structures, It reads the LRs and updates the respective
> + * struct vgic_irq, taking level/edge into account.
> + * This is the high level function which takes care of the conditions,
> + * also bails out early if there were no interrupts queued.
> + * Was: kvm_vgic_sync_hwstate()
> + */
> +void gic_clear_lrs(struct vcpu *vcpu)

I think I would prefer if we stick with the KVM name.

> +{
> +    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +
> +    /* An empty ap_list_head implies used_lrs == 0 */
> +    if ( list_empty(&vcpu->arch.vgic_cpu.ap_list_head) )
> +        return;
> +
> +    if ( vgic_cpu->used_lrs )
> +        vgic_fold_lr_state(vcpu);
> +    vgic_prune_ap_list(vcpu);
> +}
> +
> +/*
> + * gic_inject() - flush the emulation state into the hardware on guest entry
> + *
> + * Before we enter a guest, we have to translate the virtual GIC state of a
> + * VCPU into the GIC virtualization hardware registers, namely the LRs.
> + * This is the high level function which takes care about the conditions
> + * and the locking, also bails out early if there are no interrupts queued.
> + * Was: kvm_vgic_flush_hwstate()

Ditto.

> + */
> +void gic_inject(void)
> +{
> +    /*
> +     * If there are no virtual interrupts active or pending for this
> +     * VCPU, then there is no work to do and we can bail out without
> +     * taking any lock.  There is a potential race with someone injecting
> +     * interrupts to the VCPU, but it is a benign race as the VCPU will
> +     * either observe the new interrupt before or after doing this check,
> +     * and introducing additional synchronization mechanism doesn't change
> +     * this.
> +     */
> +    if ( list_empty(&current->arch.vgic_cpu.ap_list_head) )
> +        return;
> +
> +    ASSERT(!local_irq_is_enabled());
> +
> +    spin_lock(&current->arch.vgic_cpu.ap_list_lock);
> +    vgic_flush_lr_state(current);
> +    spin_unlock(&current->arch.vgic_cpu.ap_list_lock);
> +}
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
> index 5127739f0f..47fc58b81e 100644
> --- a/xen/arch/arm/vgic/vgic.h
> +++ b/xen/arch/arm/vgic/vgic.h
> @@ -17,6 +17,8 @@
>   #ifndef __XEN_ARM_VGIC_NEW_H__
>   #define __XEN_ARM_VGIC_NEW_H__
>   
> +#define vgic_irq_is_sgi(intid) ((intid) < VGIC_NR_SGIS)
> +
>   static inline bool irq_is_pending(struct vgic_irq *irq)
>   {
>       if ( irq->config == VGIC_CONFIG_EDGE )
> 

Cheers,

-- 
Julien Grall

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