Re: [Xen-devel] [PATCH v4 11/16] xen/arm: calculate vgic irq rank based on register size

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Fri, May 30, 2014 at 4:06 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
>
>
> On 30/05/14 11:24, Vijay Kilari wrote:
>>
>> On Fri, May 30, 2014 at 3:28 PM, Julien Grall <julien.grall@xxxxxxxxxx>
>> wrote:
>>>
>>> Hi Vijay,
>>>
>>>>>>
>>>>>>        case GICD_ISENABLER ... GICD_ISENABLERN:
>>>>>> -        if ( dabt.size != 2 ) goto bad_width;
>>>>>> -        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ISENABLER);
>>>>>> +        if ( dabt.size != DABT_WORD ) goto bad_width;
>>>>>> +        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ISENABLER,
>>>>>> DABT_WORD);
>>>>>
>>>>>
>>>>>
>>>>> In your commit message you explicitly say that use DABT_* will help you
>>>>> to get the register offset but... you still hardcode the size.
>>>>>
>>>>> Why can't you use dabt.size here? And all the other places.
>>>>
>>>>
>>>>
>>>>      dabt.size gives the current register access size but not the actual
>>>> register size.
>>>
>>>
>>>
>>> In this specific case, the register access size and the actual register
>>> size
>>> is the same...
>>
>>
>> Yes, in most of the cases it is same. But there are some register
>> access that supports
>> both byte and word size access. In that case we have to choose always
>> the register size DABT_*
>>
>> To be consistent I have not used dabt.size.  In case if byte access to
>> particular register
>> is added then one can go wrong.
>
>
> With your explanation, I don't see any reason to replace all the dabt.size
> != number by dat.size != DABT_*.

  Two things are done here
    (1)  In the code 'dabt.size != number' this number is always
BYTE/HALF_WORD/WORD/DOUBLE
      defined by hsr registers.  Instead of checking for hard coded
values I used hsr defined values.
    (2) The vgic_irq_rank also depends on the same hsr defined values
to calculate irq rank.
         Also, this make vgic_irq_rank generic as it takes register
size as parameter to calculate irq rank instead
          of hard coding to value 2 in previous patches

  IMO, still we can retain (1) changes and may be for point(2) we can
define similar enum to avoid
  using hsr definitions

>
> Regards,
>
> --
> Julien Grall

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