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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

>...
>+typedef uint64_t v4v_pfn_t;

We already have xen_pfn_t, so why do you need yet another
flavor?

>...
>+struct v4v_info
>+{
>+    uint64_t ring_magic;
>+    uint64_t data_magic;
>+    evtchn_port_t evtchn;

Missing padding at the end?

>+};
>+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).

>+/*
>+ * 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.

Jan


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