Re: [Xen-devel] [PATCH RFC 1/7] xen: Relocate mem_access and mem_event into common.

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Mon, Aug 25, 2014 at 7:19 PM, Andres Lagar Cavilla <andres@xxxxxxxxxxxxxxxx> wrote:

On Fri, Aug 22, 2014 at 2:30 AM, Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx> wrote:

In preparation to add support for ARM LPAE mem_event, relocate mem_access
and mem_event into common Xen code. This patch makes no functional changes
to the X86 side, for ARM mem_event and mem_access functions are just
placeholder stubs.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>

Big patch and assuming code motion LGTM. Couple of observations though.

Snip...

 

-        req->flags |= MEM_EVENT_FLAG_FOREIGN;
-        ASSERT( !(req->flags & MEM_EVENT_FLAG_VCPU_PAUSED) );

Take the opportunity to downgrade the aggressiveness of this at some point in this series. (I'd prefer to keep code motion patches as purely code motion).

A faulty tool stack can brick a debug hypervisor. Unpleasant while dev/test.

I'm a little bit hazy in what situations this could arise and what it is trying to protect against. I could wrap the condition into an unlikely() and have a debug message printed rather than bricking the VMM with ASSERT() and/or enable the ASSERT only when we are building with debug=y.

 

Snip ...

+++ b/xen/common/mem_access.c
@@ -0,0 +1,137 @@
+/******************************************************************************
+ * mem_access.c
+ *
+ * Memory access support.
+ *
+ * Copyright (c) 2011 Virtuata, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+
+#include <xen/sched.h>
+#include <xen/guest_access.h>
+#include <xen/hypercall.h>
+#include <asm/p2m.h>
+#include <public/memory.h>
+#include <xen/mem_event.h>
+#include <xsm/xsm.h>
+
+#ifdef CONFIG_X86

Presumably this [will later be|should be] changed to consider CONFIG_ARM?

Many more like that one below with the same comment.

Thanks

Andres

Right, this code motion was already big enough that I figured it would make for an easier review if I split the series here.

Tamas

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