Re: [PATCH v7 05/12] xen/arm: nr_lrs should be uint8_t

[Julien Grall <julien.grall@linaro.org>]

On 04/23/2014 02:07 PM, Ian Campbell wrote:
> On Wed, 2014-04-23 at 13:53 +0100, Julien Grall wrote:
>> On 04/23/2014 01:47 PM, Ian Campbell wrote:
>>> On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
>>>> A later patch is going to use uint8_t to keep track of LRs.
>>>> Both GICv3 and GICv2 don't need any more than an uint8_t to keep track
>>>> of the number of LRs.
>>>
>>> Did you confirm that this doesn't lead to inefficient loading and
>>> masking stuff on access?
>>>
>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>> Acked-by: Julien Grall <julien.grall@linaro.org>
>>>
>>> If yes then:
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>> Although TBH I'm not sure why unsigned int was so harmful here.
>>
>> In struct irq_pending (see next patch: #6) the lr is stored using an uint8.
>>
>> IHMO, the both variable should have the same type to avoid mistake.
> 
> That could easily be avoided by a range check before setting nr_lrs,
> which we must have to do anyway.

We might want to check against the hardcoded value used to create gic_lr
(i.e 64).

For your sentence "Did you confirm that this doesn't lead to inefficient
loading and masking stuff on access?", there is an instruction to only
load/store 8 bits (strb, ldrb).
I don't see how the compiler will do inefficient loading.

-- 
Julien Grall