Re: [PATCH 4/4] xen: Add V4V implementation

[Jean Guyader <jean.guyader@gmail.com>]

On 20 September 2012 13:20, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 20.09.12 at 13:42, Jean Guyader <jean.guyader@citrix.com> 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.
>

I'll probably switch to uint64_t for the v4v mfn list instead of using
xen_pfn_t which
are unsigned long. That way I can save the need for a compat wrapper.

>>+                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).
>

There are plenty of other place in Xen where we copy data from
a guest with some sort of lock held.

I understand that the new code should do it's best to make sure
it works correctly with the Xen paging code.

The best way to solve this might not be try to to avoid copying from
guest memory under a lock. I've discussed this with Tim and maybe
we could introduce a new copy_from_guest which is aware of the paging
and returns a specific error, and then we could cleanly unlock everything
and issue a continuation that will fixup the memory.

>>+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.
>


Thanks for the review,
Jean