Re: [PATCH v3 1/3] libxl/cpumap: Add xc_cpumap_[setcpu, clearcpu, testcpu] to complement xc_cpumap_alloc.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Mar 25, 2015 at 08:53:01AM +0000, Dario Faggioli wrote:
> On Tue, 2015-03-24 at 16:29 -0400, Konrad Rzeszutek Wilk wrote:
> > On Tue, Mar 24, 2015 at 05:46:04PM +0000, Ian Campbell wrote:
> > > Is it necessary to worry about alignment here, since xc_cpumap_t is
> > > actually a uint8_t*.
> >
> > We can also do and not worry about it:
> >
> > diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
> > index 7514b84..19a1b18 100644
> > --- a/tools/libxc/xc_misc.c
> > +++ b/tools/libxc/xc_misc.c
> > @@ -94,19 +94,22 @@ xc_cpumap_t xc_cpumap_alloc(xc_interface *xch)
> >      return calloc(1, sz);
> >  }
> >  
> > +#define BITS_PER_CPUMAP(map) (sizeof(*map) * 8)
> > +#define CPUMAP_ENTRY(cpu, map) ((map))[(cpu) / BITS_PER_CPUMAP(map)]
> > +#define CPUMAP_SHIFT(cpu, map) ((cpu) % BITS_PER_CPUMAP(map))
> >
> > [...]
> >
> Maybe it's only me, but I really find it a bit hard to figure out what
> the differences between this and what's in xc_bitops.h are.
> 
> If going for this, I'd say that the reasons why we need these, and such
> differences between these and BITMAP_* should be made evident somehow
> (changelog, comments, etc.).

Here is an updated patch:

>From dbb1bf2d12b24a6a91ad95abef0d4310e7141b79 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Mon, 23 Mar 2015 11:52:54 -0400
Subject: [PATCH] libxl/cpumap: Add xc_cpumap_[setcpu, clearcpu, testcpu] to
 complement xc_cpumap_alloc.

We export the xc_cpumap_alloc but not the bit operations.
One could include 'xc_bitops.h' but that is naughty - so instead
we just export the proper functions to do it on the xc_cpumap_t
typedef.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
----
v2: Use our own macro to make sure ARM is not affected negatively
---
 tools/libxc/include/xenctrl.h |  9 +++++++++
 tools/libxc/xc_misc.c         | 30 ++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 4e9537e..565f098 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -394,6 +394,15 @@ int xc_get_cpumap_size(xc_interface *xch);
 /* allocate a cpumap */
 xc_cpumap_t xc_cpumap_alloc(xc_interface *xch);
 
+/* clear an CPU from the cpumap. */
+void xc_cpumap_clearcpu(int cpu, xc_cpumap_t map);
+
+/* set an CPU in the cpumap. */
+void xc_cpumap_setcpu(int cpu, xc_cpumap_t map);
+
+/* Test whether the CPU in cpumap is set. */
+int xc_cpumap_testcpu(int cpu, xc_cpumap_t map);
+
 /*
  * NODEMAP handling
  */
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index be68291..e113385 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -18,6 +18,7 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
+#include "xc_bitops.h"
 #include "xc_private.h"
 #include <xen/hvm/hvm_op.h>
 
@@ -93,6 +94,35 @@ xc_cpumap_t xc_cpumap_alloc(xc_interface *xch)
     return calloc(1, sz);
 }
 
+/*
+ * xc_bitops.h has macros that do this as well - however xc_cpumap_t
+ * is an array of uint8 (1byte) while said macros expect an array
+ * of unsigned long (8 bytes). This is not a problem when setting
+ * an bit (as the computation will come with the same offset) - the
+ * issue is when clearing an bit. The aligment on ARM could affect
+ * other structures that are close in memory causing memory corruption.
+ * Hence our own macros that differ only in that we compute the
+ * size of the array elements (sizeof(*map)) as opposed to assuming
+ * it is unsigned long.
+ */
+#define BITS_PER_CPUMAP(map) (sizeof(*map) * 8)
+#define CPUMAP_ENTRY(cpu, map) ((map))[(cpu) / BITS_PER_CPUMAP(map)]
+#define CPUMAP_SHIFT(cpu, map) ((cpu) % BITS_PER_CPUMAP(map))
+void xc_cpumap_clearcpu(int cpu, xc_cpumap_t map)
+{
+    CPUMAP_ENTRY(cpu, map) &= ~(1U << CPUMAP_SHIFT(cpu, map));
+}
+
+void xc_cpumap_setcpu(int cpu, xc_cpumap_t map)
+{
+    CPUMAP_ENTRY(cpu, map) |= (1U << CPUMAP_SHIFT(cpu, map));
+}
+
+int xc_cpumap_testcpu(int cpu, xc_cpumap_t map)
+{
+    return (CPUMAP_ENTRY(cpu, map) >> CPUMAP_SHIFT(cpu, map)) & 1;
+}
+
 xc_nodemap_t xc_nodemap_alloc(xc_interface *xch)
 {
     int sz;
-- 
2.1.0