[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 05/29/2013 04:34 AM, Jan Beulich wrote:
On 28.05.13 at 21:43, Ross Philipson<ross.philipson@xxxxxxxxxx>  wrote:
@@ -320,6 +328,8 @@ struct domain *domain_create(
      if ( init_status&  INIT_arch )
+    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.


+static void v4v_ring_remove_mfns(struct domain *d, struct v4v_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 @@

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.



Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.