xen/arm: gicv3: Computation of GICD_TYPER.IDbits

xen/arm: gicv3: Computation of GICD_TYPER.IDbits

[Vijay Kilari]

Hi Julien,

 static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 {
     ...
           case GICD_TYPER:
           {
               /*
                * Number of interrupt identifier bits supported by the GIC
                * Stream Protocol Interface
                */
               unsigned int irq_bits =
get_count_order(vgic_num_irqs(v->domain));
}

I think we should use ilog2() instead of get_count_order()?

Regards
Vijay

Re: xen/arm: gicv3: Computation of GICD_TYPER.IDbits

[Ian Campbell]

On Fri, 2015-09-18 at 12:56 +0530, Vijay Kilari wrote:
> Hi Julien,
> 
>  static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
>  {
>      ...
>            case GICD_TYPER:
>            {
>                /*
>                 * Number of interrupt identifier bits supported by the
> GIC
>                 * Stream Protocol Interface
>                 */
>                unsigned int irq_bits =
> get_count_order(vgic_num_irqs(v->domain));
> }
> 
> I think we should use ilog2() instead of get_count_order()?

Please explain why.

Ian.

Re: xen/arm: gicv3: Computation of GICD_TYPER.IDbits

[Vijay Kilari]

On Fri, Sep 18, 2015 at 2:03 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Fri, 2015-09-18 at 12:56 +0530, Vijay Kilari wrote:
>> Hi Julien,
>>
>>  static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
>>  {
>>      ...
>>            case GICD_TYPER:
>>            {
>>                /*
>>                 * Number of interrupt identifier bits supported by the
>> GIC
>>                 * Stream Protocol Interface
>>                 */
>>                unsigned int irq_bits =
>> get_count_order(vgic_num_irqs(v->domain));
>> }
>>
>> I think we should use ilog2() instead of get_count_order()?
>
> Please explain why.

For a given value, get_count_order returns next highest 2^n bit that can contain
this number, where as ilog2 returns number max highest bit set to
contain the value.
(Here is the output of get_count_order() and ilog2() for various sizes )

128K get_count_order 17 log2 17
64K get_count_order   16 log2 16
48K get_count_order   16 log2 15
32K get_count_order   15 log2 15
24K get_count_order   15 log2 14
16K get_count_order   14 log2 14
8K get_count_order     13 log2 13

Here if irq_bits should specify number of bits required to specify the number.
So log2 is more appropriate. Even Linux ITS driver uses ilog2 to count number
of bits required.

Regards
Vijay

Re: xen/arm: gicv3: Computation of GICD_TYPER.IDbits

[Julien Grall]

On 18/09/15 10:48, Vijay Kilari wrote:
> On Fri, Sep 18, 2015 at 2:03 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
>> On Fri, 2015-09-18 at 12:56 +0530, Vijay Kilari wrote:
>>> Hi Julien,
>>>
>>>  static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
>>>  {
>>>      ...
>>>            case GICD_TYPER:
>>>            {
>>>                /*
>>>                 * Number of interrupt identifier bits supported by the
>>> GIC
>>>                 * Stream Protocol Interface
>>>                 */
>>>                unsigned int irq_bits =
>>> get_count_order(vgic_num_irqs(v->domain));
>>> }
>>>
>>> I think we should use ilog2() instead of get_count_order()?
>>
>> Please explain why.
> 
> For a given value, get_count_order returns next highest 2^n bit that can contain
> this number, where as ilog2 returns number max highest bit set to
> contain the value.
> (Here is the output of get_count_order() and ilog2() for various sizes )
> 
> 128K get_count_order 17 log2 17
> 64K get_count_order   16 log2 16
> 48K get_count_order   16 log2 15

2^16 = 64K
2^15 = 32K

So here the number of bits returned wouldn't be enough to fit the value.
We would expose the wrong number of IRQ to the guest.

> 32K get_count_order   15 log2 15
> 24K get_count_order   15 log2 14
> 16K get_count_order   14 log2 14
> 8K get_count_order     13 log2 13
> 
> Here if irq_bits should specify number of bits required to specify the number.
> So log2 is more appropriate. Even Linux ITS driver uses ilog2 to count number
> of bits required.

ilog2 is used in the ITS code because they round up the number of ITEs
to a power of 2.

ilog2 will always return the highest bit set in the number. So if it's
not a power of two it will return one bit less than it's required to
contain the value.

get_count_order will take care of it by adding 1 if the number is not a
power of 2.

Regards,

-- 
Julien Grall