[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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




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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.