Re: [PATCH v4 2/5] xen/arm: vgic-v2: Don't ignore a write in ITARGETSR if one field is 0

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

On 22/10/15 17:07, Ian Campbell wrote:
>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
>> index 665afeb..6b7eab3 100644
>> --- a/xen/arch/arm/vgic-v2.c
>> +++ b/xen/arch/arm/vgic-v2.c
>> @@ -50,6 +50,94 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase,
>> paddr_t vbase)
>>      vgic_v2_hw.vbase = vbase;
>>  }
>>  
>> +#define NR_TARGET_PER_REG 4U
>> +#define NR_BIT_PER_TARGET 8U
> 
> NR_TARGETS_ and NR_BITS_...
> 
> "REG" there is a bit generic, given this only works for registers with 8
> -bit fields, _ITARGETSR instead?

Well this is within the vgic-v2.c and only one register contains target.
So I found pointless to add ITARGETSR to the name.

> 
>> +/*
>> + * Store a ITARGETSR register. 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;
>> +    unsigned int regidx = REG_RANK_INDEX(8, offset, DABT_WORD);
>> +    unsigned int virq;
>> +
>> +    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);
>> +
>> +    virq = rank->index * NR_INTERRUPT_PER_RANK + offset;
>> +
>> +    for ( i = 0; i < NR_TARGET_PER_REG; i++, offset++, virq++ )
>> +    {
>> +        unsigned int new_target, old_target;
>> +        uint8_t new_mask, old_mask;
>> +
>> +        new_mask = itargetsr & ((1 << NR_BIT_PER_TARGET) - 1);
>> +        old_mask = vgic_byte_read(rank->v2.itargets[regidx], i);
>> +
>> +        itargetsr >>= NR_BIT_PER_TARGET;
> 
> This is really a part of the for() iteration expression, but oddly place
> here.
> If you turn the "((1 << NR_BIT_PER_TARGET) - 1)" thing into a #define or
> constant, then you may find that extracting the relevant byte from the
> unshifted itargetsr using (itargetsr >> (i*NR_BITS_PER_TARGET) and then
> applying the mask is clean enough to use here instead.

I placed this shift here because I didn't want to use ... >> (i *
NR_BIT_..) which require a multiplication rather than a shift in the
resulting code.

Furthermore, I found the for loop more complicate to read with itargetsr
>>= inside.

Anyway, I prefer the latter solution so I will use it in the next version.

> 
> I don't think you rely on the shifting of itargetsr apart from when
> calculating new_mask.

Correct.

>> +        /*
>> +         * 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.
> 
> "guaranteed"
> 
>> +         * For simplicity, always route the vIRQ to the first interrupt
>> +         * in the target list
>> +         */
>> +        new_target = ffs(new_mask);
>> +        old_target = ffs(old_mask);
>> +
>> +        /* The current target should always be valid */
>> +        ASSERT(old_target && (old_target <= d->max_vcpus));
>> +
>> +        /*
>> +         * Ignore the write request for this interrupt if the new target
>> +         * is invalid.
>> +         * XXX: From the spec, if the target list is not valid, the
>> +         * interrupt should be ignored (i.e not forwarding to the
> 
> "forwarded".
> 
>> +         * guest).
>> +         */
>> +        if ( !new_target || (new_target > d->max_vcpus) )
>> +        {
>> +            printk(XENLOG_G_DEBUG
> 
> gdprintk?

I would prefer to keep this printk in non-debug to help us catching any
OS potentially using this trick.

Based on that I would even use XENLOG_G_WARNING because this is not
really compliant to the spec and we are meant to fix it.

Regards,

-- 
Julien Grall