Re: [Xen-devel] [PATCH 4/4] xen: Add V4V implementation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.09.12 at 13:42, Jean Guyader <jean.guyader@xxxxxxxxxx> wrote:
>+        case V4VOP_register_ring:
>+            {
>+                XEN_GUEST_HANDLE(v4v_ring_t) ring_hnd =
>+                    guest_handle_cast(arg1, v4v_ring_t);
>+                XEN_GUEST_HANDLE(xen_pfn_t) pfn_hnd =
>+                    guest_handle_cast(arg2, xen_pfn_t);
>+                uint32_t npage = arg3;
>+                if ( unlikely(!guest_handle_okay(ring_hnd, 1)) )
>+                    goto out;
>+                if ( unlikely(!guest_handle_okay(pfn_hnd, npage)) )
>+                    goto out;

Here and below - this isn't sufficient for compat guests, or else
you give them a way to point into the compat m2p table.

>+                if ( unlikely(!guest_handle_okay(addr_hnd, 1)) )
>+                    goto out;
>+                if ( copy_from_guest(&addr, addr_hnd, 1) )
>+                    goto out;

The former is redundant with the latter. Also, if you already
used guest_handle_okay() on an array, you then can (and
should) use the cheaper __copy_{to,from}_guest() functions.
While looking at this, I also spotted at least on guest access
without error checking. Plus many guest accesses happen
with some sort of lock held - that'll cause problems when the
guest memory you're trying to access was paged out (quite
likely you would be able to point out other such cases, but
those likely predate paging, and hence are an oversight of
whoever introduced the paging code).

>+struct v4v_ring_message_header
>+{
>+    uint32_t len;
>+    uint32_t pad0;

Why? v4v_addr_t is 4-byte aligned.

>+    v4v_addr_t source;
>+    uint32_t message_type;
>+    uint32_t pad1;

And again, why?

>+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
>+    uint8_t data[];
>+#elif defined(__GNUC__)
>+    uint8_t data[0];
>+#endif
>+};
...
>+/*
>+ * V4VOP_register_ring
>+ *
>+ * Registers a ring with Xen. If a ring with the same v4v_ring_id exists,
>+ * the hypercall will return -EEXIST.
>+ *
>+ * do_v4v_op(V4VOP_unregister_ring,
>+ *           XEN_GUEST_HANDLE(v4v_ring_t), XEN_GUEST_HANDLE(xen_pfn_t),
>+ *           npage, 0)
>+ */
>+#define V4VOP_register_ring   1

"register" or "unregister"?

Also, note the use of v4v_ring_t here, but ...

>+/*
>+ * V4VOP_unregister_ring
>+ *
>+ * Unregister a ring.
>+ *
>+ * do_v4v_op(V4VOP_unregister_ring,
>+ *           XEN_GUEST_HANDLE(v4v_ring),
>+ *           NULL, 0, 0)
>+ */
>+#define V4VOP_unregister_ring         2

... not here. Please be consistent, even if this is just comments.
(Again at least for V4VOP_sendv.)

>+/*
>+ * V4VOP_viptables_list
>+ *
>+ * Delete a filtering rules at a position or the rule

Delete? Also, here and above, make clear whether you talk
about a single or multiple rules.

>+ * that matches "rule".
>+ *
>+ * do_v4v_op(V4VOP_viptables_list,
>+ *           XEN_GUEST_HANDLE(v4v_vitpables_list_t) list,
>+ *           NULL, 0, 0)
>+ */
>+#define V4VOP_viptables_list    8

Also, with all of the padding fields and unused arguments (for
certain sub-ops) - if you see the slightest chance of them ever
getting used for some extension, you will want to make sure
they got zero-filled by the guest.

Jan


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