Re: [PATCH 4/5] xen: Add V4V implementation

[Tim Deegan <tim@xen.org>]

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.