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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Vijay,

On 22/09/2015 10:55, Vijay Kilari wrote:
> On Mon, Sep 21, 2015 at 4:53 PM, Julien Grall <julien.grall@xxxxxxxxxx> 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

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