From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH (V9) 2/2] xen: Add V4V implementation Date: Wed, 29 May 2013 09:34:21 +0100 Message-ID: <51A5D9AD02000078000D97DE@nat28.tlf.novell.com> References: <1369770211-4509-1-git-send-email-ross.philipson@citrix.com> <1369770211-4509-3-git-send-email-ross.philipson@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1369770211-4509-3-git-send-email-ross.philipson@citrix.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ross Philipson Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org >>> On 28.05.13 at 21:43, Ross Philipson 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 > #include > #include > +#include 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