Re: [Xen-devel] [PATCH (V9) 2/2] xen: Add V4V implementation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.05.13 at 21:43, Ross Philipson <ross.philipson@xxxxxxxxxx> wrote:
> @@ -320,6 +328,8 @@ struct domain *domain_create(
>      xfree(d->mem_event);
>      if ( init_status & INIT_arch )
>          arch_domain_destroy(d);
> +    if ( init_status & INIT_v4v )
> +     v4v_destroy(d);

Hard tab.

> +static long
> +v4v_ringbuf_insertv(struct domain *d,
> +                    struct v4v_ring_info *ring_info,
> +                    v4v_ring_id_t *src_id, uint32_t message_type,
> +                    XEN_GUEST_HANDLE_PARAM(v4v_iov_t) iovs,
> +                    uint32_t niov, size_t len)
> +{
>...
> +    do {
>...
> +    } while ( 0 );
> +
> +    v4v_ring_unmap(ring_info);

With the unmapping of all pages getting deferred till here - is there
a sensible upper limit on the number of accumulated mappings? Or
are you blindly running the host out of mapping space?

> +static int
> +v4v_find_ring_mfns(struct domain *d, struct v4v_ring_info *ring_info,
> +                   uint32_t npage, XEN_GUEST_HANDLE_PARAM(v4v_pfn_t) pfn_hnd)
> +{
>...
> +    for ( i = 0; i < npage; ++i )
> +    {
> +        unsigned long pfn;

If you use scope restricted local variables (which I appreciate),
please do so consistently at least within functions. I'm particularly
thinking of "t" here...

> +
> +        if ( copy_from_guest_offset(&pfn, pfn_hnd, i, 1) )
> +        {
> +            ret = -EFAULT;
> +            v4v_dprintk("EFAULT\n");
> +            break;
> +        }
> +
> +        mfn = mfn_x(get_gfn(d, pfn, &t));
> +        if ( !mfn_valid(mfn) )
> +        {
> +            put_gfn(d, pfn);
> +            printk(KERN_ERR "v4v domain %d passed invalid mfn %"PRI_mfn" 
> ring %p seq %d\n",
> +                    d->domain_id, mfn, ring_info, i);
> +            ret = -EINVAL;
> +            break;

No handling of paged out or shared pages here, and not even a
comment saying this needs further adjustment?

Also in code that isn't a Linux clone, please use XENLOG_ instead
of KERN_ for message levels. And you absolutely have to use
XENLOG_G_ for messages concerning guest activities.

And then you properly use PRI_mfn here, yet elsewhere I saw MFNs
getting printed using bogus (void *) casts. This also calls for being
done consistently.

Finally, also printing the PFN here might aid guest side debugging.

> +static void v4v_ring_remove_mfns(struct domain *d, struct v4v_ring_info 
> *ring_info)
> +{
>...
> +    if ( ring_info->mfn_mapping )
> +        xfree(ring_info->mfn_mapping);

Pointless if().

> +#define V4V_RING_MAGIC          0xa822f72bb0b9d8ccUL
> +#define V4V_RING_DATA_MAGIC  0x45fe852220b801d4UL

Hard tab.

> +#define V4V_MESSAGE_DGRAM       0x3c2c1db8
> +#define V4V_MESSAGE_STREAM   0x70f6a8e5

Again. Was this really cleaned up, as the overview patch said?

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -24,6 +24,7 @@
>  #include <public/sysctl.h>
>  #include <public/vcpu.h>
>  #include <public/mem_event.h>
> +#include <xen/v4v.h>

Please don't, or if you absolutely have to, not after the public
headers, but along with the other xen/ ones.

> --- /dev/null
> +++ b/xen/include/xen/v4v.h
>...
> +struct v4v_domain;

What is this good for? There's no use in any of the following
function declarations.

Jan


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