Re: [Xen-devel] [RFC 01/29] xen/arm: lr must be included in range [0-nr_lr[

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 04/29/2013 03:55 PM, Ian Campbell wrote:

> On Mon, 2013-04-29 at 00:01 +0100, Julien Grall wrote:
> 
> Typo in subject, [ at the end, did you mean ] or ) ? ( I can never
> remember which one is inclusive)

I mean ). But [ is also accepted even if it's non-standard.
http://mathworld.wolfram.com/OpenInterval.html
I will fix it.

>> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
>> ---
>>  xen/arch/arm/gic.c |    4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index bc8faf2..bac2af2 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -512,7 +512,9 @@ static inline void gic_set_lr(int lr, unsigned int 
>> virtual_irq,
>>  {
>>      int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
>>  
>> -    BUG_ON(lr > nr_lrs);
>> +    BUG_ON(lr >= nr_lrs);
>> +    BUG_ON(lr < 0);
> 
>> +    BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
> 
> This suggests that STATE_MASK is inconveniently defined, it'd be more
> normal to include the SHIFT in the mask.
> 
> The same is true of all the other GICH_LR_*_MASK/SHIFT defines. The only
> one which is used is GICH_LR_VIRTUAL_MASK+SHIFT, and I guess I can see
> why in that case. 
>


I think all GICH_LR_*_MASK must be consistent. If we modify
GICH_LR_STATE_* we need to modify all the others.

This is basically the same for GICH_LR_PRIORITY_MASK/SHIFT but
it's seems we don't use the mask.

-- 
Julien

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