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

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

On 07/10/15 18:26, Stefano Stabellini wrote:
> 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

It's true that this may change the behavior of a guest writing the
ITARGETSR with a byte to 0. But what's the point to have a guest relying
on its write to be ignored?

Aside that, I don't think we are bound to emulate exactly the same
behavior in each version of Xen. We may decide to slightly change it
because we have to rework the code to get a common emulation or fix a bug.

> 
> 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.

How ignoring the whole write would be less surprising? ;) There is no
warning at all, so it's very difficult for the user to know why the
write has been ignored.

Furthermore, based on the spec (4.3.12 in IHI 0048B.b): "A register
field corresponding to an unimplemented interrupt is RAZ/WI."

If the user knows that an interrupt is not implemented, he may decide to
write 0 in the corresponding byte. With the current solution, the whole
write access is ignored.

The solution suggested in this patch is less restrictive and will just
ignore the corresponding byte if it's 0.

On another side, nothing in the spec specified what happen if the target
field is 0 for a valid interrupt. But I think this is OK to just ignore
it and carry on. It's simpler to implement.

I didn't mention this such things in the commit message. I will update
it if we decide to carry on with this patch.

Regards,

-- 
Julien Grall