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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Ian,

On 23/10/15 10:30, Ian Campbell wrote:
> On Thu, 2015-10-22 at 17:51 +0100, Julien Grall wrote:
>> 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.
> 
> It's the use of the generic "REG" when there is only one relevant register
> (which could be named) which I found confusing, since the current name
> implies it has wider relevance than it actually does.

Ok. I guess you'd like to rename NR_BITS_PER_TARGET? If so, do you have
any suggestion?

>>> 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.
> 
> FWIW given a constant NR_BITS which is a power of two I think i*NR_BITS
> would end up a shift with any reasonable compiler.

I will give a look.

> 
>>>> +         * 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.
> 
> Assuming I remember correctly that XENLOG_G_WARNING is ratelimited in
> default configurations, then OK.

Correct, any XENLOG_G_* is rate-limited by default.

Regards,

-- 
Julien Grall

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