Re: [Xen-devel] [PATCH 5/5] xen: Add V4V implementation

[Jean Guyader <jean.guyader@xxxxxxxxx>]

On 6 August 2012 01:45, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@xxxxxxxxxx> wrote:
>>--- /dev/null
>>+++ b/xen/include/public/v4v.h
>>...
>>+#define V4V_DOMID_ANY           0x7fffU
>
> I think I asked this before - why not use one of the pre-existing
> DOMID values? And if there is a good reason, it should be said
> here in a comment, to avoid the same question being asked
> later again.
>

I replaced 0x7fffU with DOMID_INVALID.

>>...
>>+typedef uint64_t v4v_pfn_t;
>
> We already have xen_pfn_t, so why do you need yet another
> flavor?
>

No, replaced with xen_pfn_t.

>>...
>>+struct v4v_info
>>+{
>>+    uint64_t ring_magic;
>>+    uint64_t data_magic;
>>+    evtchn_port_t evtchn;
>
> Missing padding at the end?
>

ack.

>>+};
>>+typedef struct v4v_info v4v_info_t;
>>+
>>+#define V4V_ROUNDUP(a) (((a) +0xf ) & ~0xf)
>
> Doesn't seem to belong here. Or is the subsequent comment
> actually related to this (in which case it should be moved ahead
> of the definition and made match it).
>

V4V_ROUNDUP is now in v4v.c and I move the description above
the define.

Thanks for the review,
Jean

>>+/*
>>+ * Messages on the ring are padded to 128 bits
>>+ * Len here refers to the exact length of the data not including the
>>+ * 128 bit header. The message uses
>>+ * ((len +0xf) & ~0xf) + sizeof(v4v_ring_message_header) bytes
>>+ */
>>...
>>+/*
>>+ * HYPERCALLS
>>+ */
>>...
>
> In the block below here, please get the naming (do_v4v_op()
> vs v4v_hypercall()) and the use of newlines (either always one
> or always two between individual hypercall descriptions)
> consistent. Hmm, even the descriptions don't seem to always
> match the definitions (not really obvious because apparently
> again the descriptions follow the definitions, whereas the
> opposite is the usual way to arrange things).
>
>>--- /dev/null
>>+++ b/xen/include/xen/v4v_utils.h
>>...
>>+/* Compiler specific hacks */
>>+#if defined(__GNUC__)
>>+# define V4V_UNUSED __attribute__ ((unused))
>>+# ifndef __STRICT_ANSI__
>>+#  define V4V_INLINE inline
>>+# else
>>+#  define V4V_INLINE
>>+# endif
>>+#else /* !__GNUC__ */
>>+# define V4V_UNUSED
>>+# define V4V_INLINE
>>+#endif
>
> This suggests the header is really intended to be public?
>
>>...
>>+static V4V_INLINE uint32_t
>>+v4v_ring_bytes_to_read (volatile struct v4v_ring *r)
>
> No space between function name and opening parenthesis
> (throughout this file).
>
>>...
>>+V4V_UNUSED static V4V_INLINE ssize_t
>
> V4V_UNUSED? Doesn't make sense in conjunction with
> V4V_INLINE, at least as long as you're using GNU extensions
> anyway (see above as to the disposition of the header).
>
>>+v4v_copy_out (struct v4v_ring *r, struct v4v_addr *from, uint32_t * protocol,
>>+              void *_buf, size_t t, int consume)
>
> Dead functions shouldn't be placed here.
>
>>...
>>+static ssize_t
>>+v4v_copy_out_offset (struct v4v_ring *r, struct v4v_addr *from,
>>+                     uint32_t * protocol, void *_buf, size_t t, int consume,
>>+                     size_t skip) V4V_UNUSED;
>>+
>>+V4V_INLINE static ssize_t
>>+v4v_copy_out_offset (struct v4v_ring *r, struct v4v_addr *from,
>>+                     uint32_t * protocol, void *_buf, size_t t, int consume,
>>+                     size_t skip)
>>+{
>
> What's the point of having a declaration followed immediately by
> a definition? Also, the function is dead too.
>

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