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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.08.12 at 13:57, Jean Guyader <jean.guyader@xxxxxxxxx> wrote:
> On 6 August 2012 09: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.
>>
>>>...
>>>+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.
>>
> 
> This file (v4v_utils.h) has utility that could be used by drivers, we don't 
> use
> them in Xen but we through it will be convenient to have such function
> accessible for one to write a v4v driver a v4v driver.
> 
> What would be the right place for those?

I think public/io/ would be the best matching place, but it's
certainly also not ideal. Maybe we ought to introduce public/lib/
or some such for it?

But then again I don't see how this comment of yours relates to
the earlier comments I made.

Jan


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