Instead of having to include xenctrl.h for getting definitions of cpu
barriers, add a dedicated header for that purpose.

Switch the xen-9pfsd daemon to use the new header instead of xenctrl.h.

This is in preparation of making Xenstore independent from libxenctrl.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V1:
- new patch
---
 tools/9pfsd/io.c            |  5 +++-
 tools/include/xen-barrier.h | 51 +++++++++++++++++++++++++++++++++++++
 tools/include/xenctrl.h     | 28 +-------------------
 tools/libs/ctrl/Makefile    |  2 +-
 4 files changed, 57 insertions(+), 29 deletions(-)
 create mode 100644 tools/include/xen-barrier.h

diff --git a/tools/9pfsd/io.c b/tools/9pfsd/io.c
index 468e0241f5..14cfcaf568 100644
--- a/tools/9pfsd/io.c
+++ b/tools/9pfsd/io.c
@@ -13,15 +13,18 @@
 
 #include <assert.h>
 #include <errno.h>
+#include <stdarg.h>
 #include <stdbool.h>
+#include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <syslog.h>
+#include <unistd.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <dirent.h>
 #include <fcntl.h>
-#include <xenctrl.h>           /* For cpu barriers. */
+#include <xen-barrier.h>
 #include <xen-tools/common-macros.h>
 
 #include "xen-9pfsd.h"
diff --git a/tools/include/xen-barrier.h b/tools/include/xen-barrier.h
new file mode 100644
index 0000000000..62036f528b
--- /dev/null
+++ b/tools/include/xen-barrier.h
@@ -0,0 +1,51 @@
+/******************************************************************************
+ * xen-barrier.h
+ *
+ * Definition of CPU barriers, part of libxenctrl.
+ *
+ * Copyright (c) 2003-2004, K A Fraser.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef XENBARRIER_H
+#define XENBARRIER_H
+
+/*
+ *  DEFINITIONS FOR CPU BARRIERS
+ */
+
+#define xen_barrier() asm volatile ( "" : : : "memory")
+
+#if defined(__i386__)
+#define xen_mb()  asm volatile ( "lock addl $0, -4(%%esp)" ::: "memory" )
+#define xen_rmb() xen_barrier()
+#define xen_wmb() xen_barrier()
+#elif defined(__x86_64__)
+#define xen_mb()  asm volatile ( "lock addl $0, -32(%%rsp)" ::: "memory" )
+#define xen_rmb() xen_barrier()
+#define xen_wmb() xen_barrier()
+#elif defined(__arm__)
+#define xen_mb()   asm volatile ("dmb" : : : "memory")
+#define xen_rmb()  asm volatile ("dmb" : : : "memory")
+#define xen_wmb()  asm volatile ("dmb" : : : "memory")
+#elif defined(__aarch64__)
+#define xen_mb()   asm volatile ("dmb sy" : : : "memory")
+#define xen_rmb()  asm volatile ("dmb sy" : : : "memory")
+#define xen_wmb()  asm volatile ("dmb sy" : : : "memory")
+#else
+#error "Define barriers"
+#endif
+
+#endif /* XENBARRIER_H */
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 29617585c5..ea57e9dbb9 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -48,6 +48,7 @@
 #include <xen/platform.h>
 
 #include "xentoollog.h"
+#include "xen-barrier.h"
 
 #if defined(__i386__) || defined(__x86_64__)
 #include <xen/foreign/x86_32.h>
@@ -61,33 +62,6 @@
 
 #define INVALID_MFN  (~0UL)
 
-/*
- *  DEFINITIONS FOR CPU BARRIERS
- */
-
-#define xen_barrier() asm volatile ( "" : : : "memory")
-
-#if defined(__i386__)
-#define xen_mb()  asm volatile ( "lock addl $0, -4(%%esp)" ::: "memory" )
-#define xen_rmb() xen_barrier()
-#define xen_wmb() xen_barrier()
-#elif defined(__x86_64__)
-#define xen_mb()  asm volatile ( "lock addl $0, -32(%%rsp)" ::: "memory" )
-#define xen_rmb() xen_barrier()
-#define xen_wmb() xen_barrier()
-#elif defined(__arm__)
-#define xen_mb()   asm volatile ("dmb" : : : "memory")
-#define xen_rmb()  asm volatile ("dmb" : : : "memory")
-#define xen_wmb()  asm volatile ("dmb" : : : "memory")
-#elif defined(__aarch64__)
-#define xen_mb()   asm volatile ("dmb sy" : : : "memory")
-#define xen_rmb()  asm volatile ("dmb sy" : : : "memory")
-#define xen_wmb()  asm volatile ("dmb sy" : : : "memory")
-#else
-#error "Define barriers"
-#endif
-
-
 #define XENCTRL_HAS_XC_INTERFACE 1
 /* In Xen 4.0 and earlier, xc_interface_open and xc_evtchn_open would
  * both return ints being the file descriptor.  In 4.1 and later, they
diff --git a/tools/libs/ctrl/Makefile b/tools/libs/ctrl/Makefile
index 5fe0bfad0c..acce8639d3 100644
--- a/tools/libs/ctrl/Makefile
+++ b/tools/libs/ctrl/Makefile
@@ -3,7 +3,7 @@ include $(XEN_ROOT)/tools/Rules.mk
 
 include Makefile.common
 
-LIBHEADER := xenctrl.h xenctrl_compat.h
+LIBHEADER := xenctrl.h xenctrl_compat.h xen-barrier.h
 PKG_CONFIG_FILE := xencontrol.pc
 PKG_CONFIG_NAME := Xencontrol
 
-- 
2.43.0

On Wed, Oct 23, 2024 at 03:10:04PM +0200, Juergen Gross wrote:
> diff --git a/tools/include/xen-barrier.h b/tools/include/xen-barrier.h
> new file mode 100644
> index 0000000000..62036f528b
> --- /dev/null
> +++ b/tools/include/xen-barrier.h
> @@ -0,0 +1,51 @@
> +/******************************************************************************
> + * xen-barrier.h
> + *
> + * Definition of CPU barriers, part of libxenctrl.

Does it needs to be part of "libxenctrl" ? :-) Since the goal is to be
able to use the header without xenctrl.

> + *
> + * Copyright (c) 2003-2004, K A Fraser.

I'm not sure this copyright line is enough, looking at `git blame`.

Keir introduce xen_barrier macro in 2012, in
8d3f757328e1 ("libxc: Update rmb/wmb for x86.")
Stefano introduced the Arm macro in 2012, in daa314fe1938 ("arm: compile
libxc"), and Ian in 2013 in ae4b6f29a983 ("tools: libxc: arm64
support").
There's been a modification by Andrew in 2020, so Citrix copyright,
in de16a8fa0db7 ("x86: Use LOCK ADD instead of MFENCE for smp_mb()").

So overall, we probably want:
Copyright (C) 2003-2012, K A Fraser.
Copyright (C) 2012-2020 Citrix Systems, Inc.


> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation;
> + * version 2.1 of the License.
> + *
> + * This library 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
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef XENBARRIER_H
> +#define XENBARRIER_H

With an extra '_' for the '-' in the header filename?
    XEN_BARRIER_H


Otherwise, the rest of the patch looks fine to me, even without the rest
of the series.

Cheers,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

On 26.11.24 18:28, Anthony PERARD wrote:
> On Wed, Oct 23, 2024 at 03:10:04PM +0200, Juergen Gross wrote:
>> diff --git a/tools/include/xen-barrier.h b/tools/include/xen-barrier.h
>> new file mode 100644
>> index 0000000000..62036f528b
>> --- /dev/null
>> +++ b/tools/include/xen-barrier.h
>> @@ -0,0 +1,51 @@
>> +/******************************************************************************
>> + * xen-barrier.h
>> + *
>> + * Definition of CPU barriers, part of libxenctrl.
> 
> Does it needs to be part of "libxenctrl" ? :-) Since the goal is to be
> able to use the header without xenctrl.

Yeah, but it was part of libxenctrl until now.

So it needs to be distributed together with xenctrl.h, otherwise we'd
need another distribution unit libxenctrl has to depend on.

>> + *
>> + * Copyright (c) 2003-2004, K A Fraser.
> 
> I'm not sure this copyright line is enough, looking at `git blame`.
> 
> Keir introduce xen_barrier macro in 2012, in
> 8d3f757328e1 ("libxc: Update rmb/wmb for x86.")
> Stefano introduced the Arm macro in 2012, in daa314fe1938 ("arm: compile
> libxc"), and Ian in 2013 in ae4b6f29a983 ("tools: libxc: arm64
> support").
> There's been a modification by Andrew in 2020, so Citrix copyright,
> in de16a8fa0db7 ("x86: Use LOCK ADD instead of MFENCE for smp_mb()").
> 
> So overall, we probably want:
> Copyright (C) 2003-2012, K A Fraser.
> Copyright (C) 2012-2020 Citrix Systems, Inc.

Hmm, I just copied the Copyright from xenctrl.h, where I took the contents
of the new header from.

I can see a reason for adding the Copyright for Citrix, but IMHO this
_should_ have happened when the changes were applied in 2020. And I'm feeling
a little bit uneasy to add a Copyright for a company I'm not working for. How
do I really know Citrix is wanting that Copyright to be added?

>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation;
>> + * version 2.1 of the License.
>> + *
>> + * This library 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
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef XENBARRIER_H
>> +#define XENBARRIER_H
> 
> With an extra '_' for the '-' in the header filename?
>      XEN_BARRIER_H

Okay.

> Otherwise, the rest of the patch looks fine to me, even without the rest
> of the series.

Thanks,


Juergen