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

[Jean Guyader <jean.guyader@xxxxxxxxx>]

On 24 August 2012 13:06, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> 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.
>

Ok I have removed this file from the patch series,  we already have a
copy of this file in the Linux driver so those helper functions can be found
there.

Thanks,
Jean

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