|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 4/5] xen: Add V4V implementation
OK, some detailed comments below. A lot of it is just nits, but one or
two serious concerns. We still have some ongoing discussion of the
overall design in other threads, too...
At 17:26 +0100 on 28 Jun (1340904385), Jean Guyader wrote:
> +#ifdef V4V_DEBUG
> +#define MY_FILE "v4v.c"
Something wrong with __FILE__ ?
> +#define v4v_dprintk(format, args...) \
> + do { \
> + printk("%s:%d " format, \
> + MY_FILE, __LINE__, ## args ); \
> + } while ( 1 == 0 )
> +#else
> +#define v4v_dprintk(format, ... ) (void)0
> +#endif
> +
>
> +#ifdef V4V_DEBUG
> +static void
> +v4v_hexdump (void *_p, int len)
> +{
> + uint8_t *buf = (uint8_t *) _p;
> + int i, j;
> +
> + for (i = 0; i < len; i += 16)
Coding style is 'for ( i = 0; i < len; i += 16 )' (and similarly throughout).
> + {
> + printk (KERN_ERR "%p:", &buf[i]);
> + for (j = 0; j < 16; ++j)
> + {
> + int k = i + j;
> + if (k < len)
Likewise 'if ( k < len )'
> + printk (" %02x", buf[k]);
but 'printk(...)' with no space before the args.
> +/*
> + * ring buffer
> + */
> +
> +/* called must have L3 */
Maybe make these comments into ASSERT()s?
> +static void
> +v4v_ring_unmap (struct v4v_ring_info *ring_info)
> +{
> + int i;
> + for (i = 0; i < ring_info->npage; ++i)
> + {
> + if (!ring_info->mfn_mapping[i])
> + continue;
> + v4v_dprintk("");
I'm OK with having a lot of compiled-out debug printks, but that's
taking it a bit far. :)
> +/* called must have L3 */
> +static int
> +v4v_memcpy_from_guest_ring (void *_dst, struct v4v_ring_info *ring_info,
> + uint32_t offset, uint32_t len)
This function is only ever called to copy the ring_info out of a ring so
it probably doesn't need to be so general (handling multiple pages &c).
I guess the compiler can figure out that offset is always == 0 and trim
the dead code but we might as well cut it from the source too. :)
> +{
> + int page = offset >> PAGE_SHIFT;
> + uint8_t *src;
> + uint8_t *dst = _dst;
> +
> + offset &= PAGE_SIZE - 1;
> +
> + while ((offset + len) > PAGE_SIZE)
> + {
> + src = v4v_ring_map_page (ring_info, page);
> +
> + if (!src)
> + {
> + return -EFAULT;
> + }
While I'm kvetching about style, maybe lose the braces around
single-line clauses like this.
> +
> + v4v_dprintk("memcpy(%p,%p+%d,%d)\n",
> + dst, src, offset,
> + (int) (PAGE_SIZE - offset));
> + memcpy (dst, src + offset, PAGE_SIZE - offset);
> +
> + page++;
> + len -= PAGE_SIZE - offset;
> + dst += PAGE_SIZE - offset;
> + offset = 0;
> + }
> +
> + src = v4v_ring_map_page (ring_info, page);
> + if (!src)
> + {
> + return -EFAULT;
> + }
> +
> + v4v_dprintk("memcpy(%p,%p+%d,%d)\n", dst, src, offset, len);
> + memcpy (dst, src + offset, len);
> +
> + return 0;
> +}
> +
> +
> +/* called must have L3 */
> +static int
> +v4v_update_tx_ptr (struct v4v_ring_info *ring_info, uint32_t tx_ptr)
> +{
> + uint8_t *dst = v4v_ring_map_page (ring_info, 0);
> + volatile uint32_t *p = (uint32_t *)(dst + offsetof (v4v_ring_t, tx_ptr));
What's the intention of using 'volatile' here?
If it's to make sure you get a single atomic write you should probably
use the write_atomic() macro, which compiles to an explicit asm op of
the right size -- GCC explicitly does _not_ guarantee that it will use
atomic updates (though in practice it surely will).
If you want to make sure the receiver doesn't see the tx update before
the data, I think you need to use explicit memory barriers. GCC doesn't
guarantee not to reorder other non-volatile accesses past this volatile
one, and even if it did this is common code and you can't rely on x86's
program-order semantics.
> +
> + if (!dst)
> + return -EFAULT;
> + *p = tx_ptr;
> + return 0;
> +}
> +
> +/* called must have L3 */
> +static int
> +v4v_memcpy_to_guest_ring (struct v4v_ring_info *ring_info, uint32_t offset,
> + void *_src, uint32_t len)
This function and the _from_guest one are nearly identical, except for
the actual copy and updating the source pointer. Is there any sensible
way to combine them? Or would the result be too ugly?
> +static int
> +v4v_ringbuf_get_rx_ptr (struct domain *d, struct v4v_ring_info *ring_info,
> + uint32_t * rx_ptr)
> +{
> + v4v_ring_t *ringp;
> +
> + if ( ring_info->npage == 0 )
> + return -1;
> +
> + ringp = map_domain_page (mfn_x (ring_info->mfns[0]));
> +
> + v4v_dprintk("v4v_ringbuf_payload_space: mapped %p to %p\n",
> + (void *) mfn_x (ring_info->mfns[0]), ringp);
> + if ( !ringp )
> + return -1;
> +
> + *rx_ptr = *(volatile uint32_t *) &ringp->rx_ptr;
I have the same comments about 'volatile' as I did above.
> +/*caller must have L3*/
> +static size_t
> +v4v_ringbuf_insert (struct domain *d,
> + struct v4v_ring_info *ring_info,
> + struct v4v_ring_id *src_id, uint32_t proto,
> + XEN_GUEST_HANDLE (void) buf_hnd_void, uint32_t len)
>
> +static ssize_t
> +v4v_ringbuf_insertv (struct domain *d,
> + struct v4v_ring_info *ring_info,
> + struct v4v_ring_id *src_id, uint32_t proto,
> + XEN_GUEST_HANDLE (v4v_iov_t) iovs, uint32_t niov,
> + uint32_t len)
These two functions have a lot of repeated code as well.
Could insert() be coded as a wrapper around insertv()? If the
guest-handle-munging is a problem, maybe we could push the same decision
up the stack and only provide the vector version at the hypercall
interface?
> +/*caller must hold W(L2) */
> +static void v4v_ring_remove_mfns (struct v4v_ring_info *ring_info)
> +{
> + int i;
> +
> + if ( ring_info->mfns )
> + {
> + for ( i=0; i < ring_info->npage; ++i )
> + if (mfn_x(ring_info->mfns[i]) != 0)
> + put_page_and_type(mfn_to_page(mfn_x(ring_info->mfns[i])));
> + xfree (ring_info->mfns);
> + }
> + ring_info->mfns = NULL;
I think this should be freeing mfn_mapping too.
> +#ifdef __i386__
> +# define V4V_RING_MAGIC 0xdf6977f231abd910ULL
> +# define V4V_PFN_LIST_MAGIC 0x91dd6159045b302dULL
> +#else
> +# define V4V_RING_MAGIC 0xdf6977f231abd910
> +# define V4V_PFN_LIST_MAGIC 0x91dd6159045b302d
> +#endif
Why the ifdef (and likewise for other magic numbers in this header)?
> +#define V4V_DOMID_INVALID (0x7FFFU)
> +#define V4V_DOMID_NONE V4V_DOMID_INVALID
> +#define V4V_DOMID_ANY V4V_DOMID_INVALID
The only one of these actually used in the rest of the patch is
V4V_DOMID_NONE (in a context where surely _ANY would be better).
Can you get rid of the others?
> +#define V4V_PORT_NONE 0
> +
> +/*
> + * struct v4v_iov
> + * {
> + * 64 bits: iov_base
> + * 64 bits: iov_len
> + * }
> + */
I agree with Jan - it would be better to provide the actual definitions
of these structures, even if non-GCC users might need to post-process or
rewrite the header.
> +#define V4V_RING_DATA_F_EMPTY 1U << 0 /* Ring is empty */
> +#define V4V_RING_DATA_F_EXISTS 1U << 1 /* Ring exists */
> +#define V4V_RING_DATA_F_PENDING 1U << 2 /* Pending interrupt exists - do
> not
> + rely on this field - for
> + profiling only */
> +#define V4V_RING_DATA_F_SUFFICIENT 1U << 3 /* Sufficient space to queue
> + space_required bytes exists */
> +
Please put parentheses around these flags.
> +static inline uint16_t
> +v4v_hash_fn (struct v4v_ring_id *id)
> +{
> + uint16_t ret;
Your indentation has got confused here (and for the rest of this file).
Cheers,
Tim.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |