Re: [PATCH v7 05/28] xen/arm: ITS: Port ITS driver to Xen

[Julien Grall <julien.grall@citrix.com>]

Hi Vijay,

On 22/09/2015 10:55, Vijay Kilari wrote:
> On Mon, Sep 21, 2015 at 4:53 PM, Julien Grall <julien.grall@citrix.com> wrote:
>> Hi Vijay,
>>
>> The only things I haven't check on this patch was the ITS command structure.
> [...]
>> Why a padding of only 56 bits? Shouldn't it be 248 bits (i.e 31 * 8
>> bits) to fit all the command?
>>
>>> +    } hdr;
>>> +    struct __packed {
>>> +        u8 cmd;
>>> +        u8 res1[3];
>>> +        u32 devid;
>>> +        u64 size:5;
>>> +        u64 res2:59;
>>> +        /* XXX: Though itt is 40 bit. Keep it 48 to avoid shift */
>>> +        u64 res3:8;
>>
>> It's very confusing for the reviewer to see a mix of usage (u8 res1[n],
>> u64 res3:8) within the same structure.
>>
>> The later (i.e u64 field:n) is the best to use because we can match
>> quickly the size with the spec.
>>
>
> Do you mean to convert all field type as 64 as below?

Yes.

>
>      struct __packed {
>          u64 cmd:8;
>          u64 res:24;
>          u64 devid:32;
>          u64 size:5;
>          u64 res2:59;
>          /* XXX: Though itt is 40 bit. Keep it 48 to avoid shift */

BTW, this comment is not clear. Please explain in the comment why you 
want to use 48 bit. I.e

"XXX: The ITT holds the upper bits of the address [47-8]. Include res3 
to avoid shifting?"

>          u64 res3:8;
>          u64 itt:40;
>          u64 res4:15;
>          u64 valid:1;
>          u64 res5;
>      } mapd_cmd;
>

Regards,

-- 
Julien Grall