From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ross Philipson Subject: Re: [PATCH (V9) 2/2] xen: Add V4V implementation Date: Wed, 29 May 2013 15:26:44 -0400 Message-ID: <51A65674.7090607@citrix.com> References: <1369770211-4509-1-git-send-email-ross.philipson@citrix.com> <1369770211-4509-3-git-send-email-ross.philipson@citrix.com> <51A5D9AD02000078000D97DE@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51A5D9AD02000078000D97DE@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 05/29/2013 04:34 AM, Jan Beulich wrote: >>>> 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? It is limited by the maximum size of the ring that was created. In theory that could be large. I could look at freeing them in the loop as it progresses. > >> +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... I will go through and make this consistent. > >> + >> + 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? Right, need to adddress this. I guess we should be looking for p2m_is_shared/p2m_is_paging and avoid those types. There may be other types to avoid too like p2m_is_grant etc. > > 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. Wilco > >> +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? It was cleaned up. I inherited the patch set and went through it. In the diff from version 8 and the inherited set I saw quite a bit of cleanup. But that is not an excuse - I will clean the rest up, sorry. > >> --- 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. It is a forward declaration of the main internal v4v struct. It is used in sched.h in the domain struct - thus the #include you asked about earlier. I will of course move that include. Thanks Ross > > Jan >