Re: [Xen-devel] [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 7 Oct 2015, Julien Grall wrote:
> +/*
> + * Store a ITARGETSR register in a convenient way and migrate the vIRQ
> + * if necessary. This function only deals with ITARGETSR8 and onwards.
> + *
> + * Note the offset will be aligned to the appropriate boundary.
> + */
> +static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank 
> *rank,
> +                                 unsigned int offset, uint32_t itargetsr)
> +{
> +    unsigned int i;
> +
> +    ASSERT(spin_is_locked(&rank->lock));
> +
> +    /*
> +     * The ITARGETSR0-7, used for SGIs/PPIs, are implemented RO in the
> +     * emulation and should never call this function.
> +     *
> +     * They all live in the first rank.
> +     */
> +    BUILD_BUG_ON(NR_INTERRUPT_PER_RANK != 32);
> +    ASSERT(rank->index >= 1);
> +
> +    offset &= INTERRUPT_RANK_MASK;
> +    offset &= ~(NR_TARGET_PER_REG - 1);
> +
> +    for ( i = 0; i < NR_TARGET_PER_REG; i++, offset++ )
> +    {
> +        unsigned int new_target, old_target;
> +
> +        /*
> +         * SPIs are using the 1-N model (see 1.4.3 in ARM IHI 0048B).
> +         * While the interrupt could be set pending to all the vCPUs in
> +         * target list, it's not guarantee by the spec.
> +         * For simplicity, always route the vIRQ to the first interrupt
> +         * in the target list
> +         */
> +        new_target = ffs(itargetsr & ((1 << NR_BIT_PER_TARGET) - 1));
> +        old_target = rank->vcpu[offset];
> +
> +        itargetsr >>= NR_BIT_PER_TARGET;
> +
> +        /*
> +         * Ignore the write request for this interrupt if the new target
> +         * is invalid.
> +         */
> +        if ( !new_target || (new_target > d->max_vcpus) )
> +            continue;
> +
> +        /* The vCPU ID always start from 0 */
> +        new_target--;
> +
> +        /* Only migrate the vIRQ if the target vCPU has changed */
> +        if ( new_target != old_target )
> +        {
> +            unsigned int virq = rank->index * NR_INTERRUPT_PER_RANK + offset;
> +
> +            vgic_migrate_irq(d->vcpu[old_target],
> +                             d->vcpu[new_target],
> +                             virq);
> +        }
> +
> +        rank->vcpu[offset] = new_target;
> +    }
> +}

This introduces a behavioural change: previously 32-bit writes which
contained one 0 byte would be ignored in their entirety. See:

http://marc.info/?l=xen-devel&m=140431564123221

Even though I am not entirely sure about which one is the correct
behaviour according to the spec, I prefer the old one because I think it
is less surprising to users.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel