[PATCH v3 0/5] Fix redefinition errors for toolstack libs

[PATCH v3 0/5] Fix redefinition errors for toolstack libs

[Costin Lupu]

For replication I used gcc 10.3 on an Alpine system. In order to replicate the
redefinition error for PAGE_SIZE one should install the 'fortify-headers'
package which will change the chain of included headers by indirectly including
/usr/include/limits.h where PAGE_SIZE and PATH_MAX are defined.

Changes since v1:
- Use XC_PAGE_* macros instead of PAGE_* macros

Changes since v2:
- Define KDD_PAGE_* macros for changes in debugger/kdd/

Costin Lupu (5):
  tools/debugger: Fix PAGE_SIZE redefinition error
  tools/libfsimage: Fix PATH_MAX redefinition error
  tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error
  tools/libs/gnttab: Fix PAGE_SIZE redefinition error
  tools/ocaml: Fix redefinition errors

 tools/debugger/kdd/kdd-xen.c                  | 15 ++++------
 tools/debugger/kdd/kdd.c                      | 19 ++++++-------
 tools/debugger/kdd/kdd.h                      |  7 +++++
 tools/libfsimage/ext2fs/fsys_ext2fs.c         |  2 ++
 tools/libfsimage/reiserfs/fsys_reiserfs.c     |  2 ++
 tools/libs/foreignmemory/core.c               |  2 +-
 tools/libs/foreignmemory/freebsd.c            | 10 +++----
 tools/libs/foreignmemory/linux.c              | 23 +++++++--------
 tools/libs/foreignmemory/minios.c             |  2 +-
 tools/libs/foreignmemory/netbsd.c             | 10 +++----
 tools/libs/foreignmemory/private.h            |  9 +-----
 tools/libs/gnttab/freebsd.c                   | 28 +++++++++----------
 tools/libs/gnttab/linux.c                     | 28 +++++++++----------
 tools/libs/gnttab/netbsd.c                    | 23 +++++++--------
 tools/ocaml/libs/xc/xenctrl_stubs.c           | 10 +++----
 .../ocaml/libs/xentoollog/xentoollog_stubs.c  |  4 +++
 tools/ocaml/libs/xl/xenlight_stubs.c          |  4 +++
 17 files changed, 98 insertions(+), 100 deletions(-)

-- 
2.20.1

[PATCH v3 1/5] tools/debugger: Fix PAGE_SIZE redefinition error

[Costin Lupu]

If PAGE_SIZE is already defined in the system (e.g. in /usr/include/limits.h
header) then gcc will trigger a redefinition error because of -Werror. This
patch replaces usage of PAGE_* macros with KDD_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity (which is what we are dealing with here).

We chose to define the KDD_PAGE_* macros instead of using XC_PAGE_* macros
because (1) the code in kdd.c should not include any Xen headers and (2) to add
consistency for code in both kdd.c and kdd-xen.c.

Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
---
 tools/debugger/kdd/kdd-xen.c | 15 ++++++---------
 tools/debugger/kdd/kdd.c     | 19 ++++++++-----------
 tools/debugger/kdd/kdd.h     |  7 +++++++
 3 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/tools/debugger/kdd/kdd-xen.c b/tools/debugger/kdd/kdd-xen.c
index f3f9529f9f..e78c9311c4 100644
--- a/tools/debugger/kdd/kdd-xen.c
+++ b/tools/debugger/kdd/kdd-xen.c
@@ -48,9 +48,6 @@
 
 #define MAPSIZE 4093 /* Prime */
 
-#define PAGE_SHIFT 12
-#define PAGE_SIZE (1U << PAGE_SHIFT)
-
 struct kdd_guest {
     struct xentoollog_logger xc_log; /* Must be first for xc log callbacks */
     xc_interface *xc_handle;
@@ -72,7 +69,7 @@ static void flush_maps(kdd_guest *g)
     int i;
     for (i = 0; i < MAPSIZE; i++) {
         if (g->maps[i] != NULL)
-            munmap(g->maps[i], PAGE_SIZE);
+            munmap(g->maps[i], KDD_PAGE_SIZE);
         g->maps[i] = NULL;
     }
 }
@@ -490,13 +487,13 @@ static uint32_t kdd_access_physical_page(kdd_guest *g, uint64_t addr,
     uint32_t map_pfn, map_offset;
     uint8_t *map;
 
-    map_pfn = (addr >> PAGE_SHIFT);
-    map_offset = addr & (PAGE_SIZE - 1);
+    map_pfn = (addr >> KDD_PAGE_SHIFT);
+    map_offset = addr & (KDD_PAGE_SIZE - 1);
 
     /* Evict any mapping of the wrong frame from our slot */ 
     if (g->pfns[map_pfn % MAPSIZE] != map_pfn
         && g->maps[map_pfn % MAPSIZE] != NULL) {
-        munmap(g->maps[map_pfn % MAPSIZE], PAGE_SIZE);
+        munmap(g->maps[map_pfn % MAPSIZE], KDD_PAGE_SIZE);
         g->maps[map_pfn % MAPSIZE] = NULL;
     }
     g->pfns[map_pfn % MAPSIZE] = map_pfn;
@@ -507,7 +504,7 @@ static uint32_t kdd_access_physical_page(kdd_guest *g, uint64_t addr,
     else {
         map = xc_map_foreign_range(g->xc_handle,
                                    g->domid,
-                                   PAGE_SIZE,
+                                   KDD_PAGE_SIZE,
                                    PROT_READ|PROT_WRITE,
                                    map_pfn);
 
@@ -533,7 +530,7 @@ uint32_t kdd_access_physical(kdd_guest *g, uint64_t addr,
 {
     uint32_t chunk, rv, done = 0;
     while (len > 0) {
-        chunk = PAGE_SIZE - (addr & (PAGE_SIZE - 1));
+        chunk = KDD_PAGE_SIZE - (addr & (KDD_PAGE_SIZE - 1));
         if (chunk > len) 
             chunk = len;
         rv = kdd_access_physical_page(g, addr, chunk, buf, write);
diff --git a/tools/debugger/kdd/kdd.c b/tools/debugger/kdd/kdd.c
index 17513c2650..320c623eda 100644
--- a/tools/debugger/kdd/kdd.c
+++ b/tools/debugger/kdd/kdd.c
@@ -288,9 +288,6 @@ static void kdd_log_pkt(kdd_state *s, const char *name, kdd_pkt *p)
  *  Memory access: virtual addresses and syntactic sugar.
  */
 
-#define PAGE_SHIFT (12)
-#define PAGE_SIZE (1ULL << PAGE_SHIFT) 
-
 static uint32_t kdd_read_physical(kdd_state *s, uint64_t addr, 
                                   uint32_t len, void *buf)
 {
@@ -352,7 +349,7 @@ static uint64_t v2p(kdd_state *s, int cpuid, uint64_t va)
 
     /* Walk the appropriate number of levels */
     for (i = levels; i > 0; i--) {
-        shift = PAGE_SHIFT + bits * (i-1);
+        shift = KDD_PAGE_SHIFT + bits * (i-1);
         mask = ((1ULL << bits) - 1) << shift;
         offset = ((va & mask) >> shift) * width;
         KDD_DEBUG(s, "level %i: mask 0x%16.16"PRIx64" pa 0x%16.16"PRIx64
@@ -364,12 +361,12 @@ static uint64_t v2p(kdd_state *s, int cpuid, uint64_t va)
             return -1ULL; // Not present
         pa = entry & 0x000ffffffffff000ULL;
         if (pse && (i == 2) && (entry & 0x80)) { // Superpage
-            mask = ((1ULL << (PAGE_SHIFT + bits)) - 1);
+            mask = ((1ULL << (KDD_PAGE_SHIFT + bits)) - 1);
             return (pa & ~mask) + (va & mask);
         }
     }
 
-    return pa + (va & (PAGE_SIZE - 1));
+    return pa + (va & (KDD_PAGE_SIZE - 1));
 }
 
 static uint32_t kdd_access_virtual(kdd_state *s, int cpuid, uint64_t addr,
@@ -380,7 +377,7 @@ static uint32_t kdd_access_virtual(kdd_state *s, int cpuid, uint64_t addr,
     
     /* Process one page at a time */
     while (len > 0) {
-        chunk = PAGE_SIZE - (addr & (PAGE_SIZE - 1));
+        chunk = KDD_PAGE_SIZE - (addr & (KDD_PAGE_SIZE - 1));
         if (chunk > len) 
             chunk = len;
         pa = v2p(s, cpuid, addr);
@@ -591,7 +588,7 @@ static void get_os_info_64(kdd_state *s)
     uint64_t dbgkd_addr;
     DBGKD_GET_VERSION64 dbgkd_get_version64;
     /* Maybe 1GB is too big for the limit to search? */
-    uint32_t search_limit = (1024 * 1024 * 1024) / PAGE_SIZE; /*1GB/PageSize*/
+    uint32_t search_limit = (1024 * 1024 * 1024) / KDD_PAGE_SIZE; /*1GB/PageSize*/
     uint64_t efer;
 
     /* if we are not in 64-bit mode, fail */
@@ -620,7 +617,7 @@ static void get_os_info_64(kdd_state *s)
      * in 1GB range above the current page base address
      */
 
-    base = idt0_addr & ~(PAGE_SIZE - 1);
+    base = idt0_addr & ~(KDD_PAGE_SIZE - 1);
 
     while (search_limit) {
         uint16_t val;
@@ -633,7 +630,7 @@ static void get_os_info_64(kdd_state *s)
         if (val == MZ_HEADER) // MZ
             break;
 
-        base -= PAGE_SIZE;
+        base -= KDD_PAGE_SIZE;
         search_limit -= 1;
     }
 
@@ -720,7 +717,7 @@ static void find_os(kdd_state *s)
         /* Try each page in the potential range of kernel load addresses */
         for (limit = s->os.base + s->os.range;
              s->os.base <= limit;
-             s->os.base += PAGE_SIZE)
+             s->os.base += KDD_PAGE_SIZE)
             if (check_os(s))
                 return;
     }
diff --git a/tools/debugger/kdd/kdd.h b/tools/debugger/kdd/kdd.h
index b9a17440df..b476a76d93 100644
--- a/tools/debugger/kdd/kdd.h
+++ b/tools/debugger/kdd/kdd.h
@@ -39,6 +39,13 @@
 
 #define PACKED __attribute__((packed))
 
+/* We define our page related constants here in order to specifically
+ * avoid using the Xen page macros (this is a restriction for the code
+ * in kdd.c which should not include any Xen headers) and to add
+ * consistency for code in both kdd.c and kdd-xen.c. */
+#define KDD_PAGE_SHIFT 12
+#define KDD_PAGE_SIZE (1U << KDD_PAGE_SHIFT)
+
 /*****************************************************************************
  * Serial line protocol: Sender sends a 16-byte header with an optional
  * payload following it.  Receiver responds to each packet with an
-- 
2.20.1

Re: [PATCH v3 1/5] tools/debugger: Fix PAGE_SIZE redefinition error

[Tim Deegan]

At 11:35 +0300 on 10 May (1620646515), Costin Lupu wrote:
> If PAGE_SIZE is already defined in the system (e.g. in /usr/include/limits.h
> header) then gcc will trigger a redefinition error because of -Werror. This
> patch replaces usage of PAGE_* macros with KDD_PAGE_* macros in order to avoid
> confusion between control domain page granularity (PAGE_* definitions) and
> guest domain page granularity (which is what we are dealing with here).
> 
> We chose to define the KDD_PAGE_* macros instead of using XC_PAGE_* macros
> because (1) the code in kdd.c should not include any Xen headers and (2) to add
> consistency for code in both kdd.c and kdd-xen.c.
> 
> Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>

Reviewed-by: Tim Deegan <tim@xen.org>

Thanks!

Tim.

[PATCH v3 2/5] tools/libfsimage: Fix PATH_MAX redefinition error

[Costin Lupu]

If PATH_MAX is already defined in the system (e.g. in /usr/include/limits.h
header) then gcc will trigger a redefinition error because of -Werror.

Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
 tools/libfsimage/ext2fs/fsys_ext2fs.c     | 2 ++
 tools/libfsimage/reiserfs/fsys_reiserfs.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/tools/libfsimage/ext2fs/fsys_ext2fs.c b/tools/libfsimage/ext2fs/fsys_ext2fs.c
index a4ed10419c..5ed8fce90e 100644
--- a/tools/libfsimage/ext2fs/fsys_ext2fs.c
+++ b/tools/libfsimage/ext2fs/fsys_ext2fs.c
@@ -278,7 +278,9 @@ struct ext4_extent_header {
 
 #define EXT2_SUPER_MAGIC      0xEF53	/* include/linux/ext2_fs.h */
 #define EXT2_ROOT_INO              2	/* include/linux/ext2_fs.h */
+#ifndef PATH_MAX
 #define PATH_MAX                1024	/* include/linux/limits.h */
+#endif
 #define MAX_LINK_COUNT             5	/* number of symbolic links to follow */
 
 /* made up, these are pointers into FSYS_BUF */
diff --git a/tools/libfsimage/reiserfs/fsys_reiserfs.c b/tools/libfsimage/reiserfs/fsys_reiserfs.c
index 916eb15292..10ca657476 100644
--- a/tools/libfsimage/reiserfs/fsys_reiserfs.c
+++ b/tools/libfsimage/reiserfs/fsys_reiserfs.c
@@ -284,7 +284,9 @@ struct reiserfs_de_head
 #define S_ISDIR(mode) (((mode) & 0170000) == 0040000)
 #define S_ISLNK(mode) (((mode) & 0170000) == 0120000)
 
+#ifndef PATH_MAX
 #define PATH_MAX       1024	/* include/linux/limits.h */
+#endif
 #define MAX_LINK_COUNT    5	/* number of symbolic links to follow */
 
 /* The size of the node cache */
-- 
2.20.1

[PATCH v3 3/5] tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error

[Costin Lupu]

If PAGE_SIZE is already defined in the system (e.g. in /usr/include/limits.h
header) then gcc will trigger a redefinition error because of -Werror. This
patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity (which is what we are dealing with here).

Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
---
 tools/libs/foreignmemory/core.c    |  2 +-
 tools/libs/foreignmemory/freebsd.c | 10 +++++-----
 tools/libs/foreignmemory/linux.c   | 23 ++++++++++++-----------
 tools/libs/foreignmemory/minios.c  |  2 +-
 tools/libs/foreignmemory/netbsd.c  | 10 +++++-----
 tools/libs/foreignmemory/private.h |  9 +--------
 6 files changed, 25 insertions(+), 31 deletions(-)

diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index 28ec311af1..7edc6f0dbf 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -202,7 +202,7 @@ int xenforeignmemory_resource_size(
     if ( rc )
         return rc;
 
-    *size = fres.nr_frames << PAGE_SHIFT;
+    *size = fres.nr_frames << XC_PAGE_SHIFT;
     return 0;
 }
 
diff --git a/tools/libs/foreignmemory/freebsd.c b/tools/libs/foreignmemory/freebsd.c
index d94ea07862..2cf0fa1c38 100644
--- a/tools/libs/foreignmemory/freebsd.c
+++ b/tools/libs/foreignmemory/freebsd.c
@@ -63,7 +63,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
     privcmd_mmapbatch_t ioctlx;
     int rc;
 
-    addr = mmap(addr, num << PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
+    addr = mmap(addr, num << XC_PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
     if ( addr == MAP_FAILED )
         return NULL;
 
@@ -78,7 +78,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
     {
         int saved_errno = errno;
 
-        (void)munmap(addr, num << PAGE_SHIFT);
+        (void)munmap(addr, num << XC_PAGE_SHIFT);
         errno = saved_errno;
         return NULL;
     }
@@ -89,7 +89,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
                                  void *addr, size_t num)
 {
-    return munmap(addr, num << PAGE_SHIFT);
+    return munmap(addr, num << XC_PAGE_SHIFT);
 }
 
 int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
@@ -101,7 +101,7 @@ int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
 int osdep_xenforeignmemory_unmap_resource(xenforeignmemory_handle *fmem,
                                         xenforeignmemory_resource_handle *fres)
 {
-    return fres ? munmap(fres->addr, fres->nr_frames << PAGE_SHIFT) : 0;
+    return fres ? munmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT) : 0;
 }
 
 int osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
@@ -120,7 +120,7 @@ int osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
         /* Request for resource size.  Skip mmap(). */
         goto skip_mmap;
 
-    fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
+    fres->addr = mmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT,
                       fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
     if ( fres->addr == MAP_FAILED )
         return -1;
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index c1f35e2db7..a5f6d62567 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -134,7 +134,7 @@ static int retry_paged(int fd, uint32_t dom, void *addr,
         /* At least one gfn is still in paging state */
         ioctlx.num = 1;
         ioctlx.dom = dom;
-        ioctlx.addr = (unsigned long)addr + (i<<PAGE_SHIFT);
+        ioctlx.addr = (unsigned long)addr + (i<<XC_PAGE_SHIFT);
         ioctlx.arr = arr + i;
         ioctlx.err = err + i;
 
@@ -168,7 +168,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
     size_t i;
     int rc;
 
-    addr = mmap(addr, num << PAGE_SHIFT, prot, flags | MAP_SHARED,
+    addr = mmap(addr, num << XC_PAGE_SHIFT, prot, flags | MAP_SHARED,
                 fd, 0);
     if ( addr == MAP_FAILED )
         return NULL;
@@ -198,9 +198,10 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
          */
         privcmd_mmapbatch_t ioctlx;
         xen_pfn_t *pfn;
-        unsigned int pfn_arr_size = ROUNDUP((num * sizeof(*pfn)), PAGE_SHIFT);
+        unsigned int pfn_arr_size = ROUNDUP((num * sizeof(*pfn)), XC_PAGE_SHIFT);
+        int os_page_size = getpagesize();
 
-        if ( pfn_arr_size <= PAGE_SIZE )
+        if ( pfn_arr_size <= os_page_size )
             pfn = alloca(num * sizeof(*pfn));
         else
         {
@@ -209,7 +210,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
             if ( pfn == MAP_FAILED )
             {
                 PERROR("mmap of pfn array failed");
-                (void)munmap(addr, num << PAGE_SHIFT);
+                (void)munmap(addr, num << XC_PAGE_SHIFT);
                 return NULL;
             }
         }
@@ -242,7 +243,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
                     continue;
                 }
                 rc = map_foreign_batch_single(fd, dom, pfn + i,
-                        (unsigned long)addr + (i<<PAGE_SHIFT));
+                        (unsigned long)addr + (i<<XC_PAGE_SHIFT));
                 if ( rc < 0 )
                 {
                     rc = -errno;
@@ -254,7 +255,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
             break;
         }
 
-        if ( pfn_arr_size > PAGE_SIZE )
+        if ( pfn_arr_size > os_page_size )
             munmap(pfn, pfn_arr_size);
 
         if ( rc == -ENOENT && i == num )
@@ -270,7 +271,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
     {
         int saved_errno = errno;
 
-        (void)munmap(addr, num << PAGE_SHIFT);
+        (void)munmap(addr, num << XC_PAGE_SHIFT);
         errno = saved_errno;
         return NULL;
     }
@@ -281,7 +282,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
                                  void *addr, size_t num)
 {
-    return munmap(addr, num << PAGE_SHIFT);
+    return munmap(addr, num << XC_PAGE_SHIFT);
 }
 
 int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
@@ -293,7 +294,7 @@ int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
 int osdep_xenforeignmemory_unmap_resource(
     xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
 {
-    return fres ? munmap(fres->addr, fres->nr_frames << PAGE_SHIFT) : 0;
+    return fres ? munmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT) : 0;
 }
 
 int osdep_xenforeignmemory_map_resource(
@@ -312,7 +313,7 @@ int osdep_xenforeignmemory_map_resource(
         /* Request for resource size.  Skip mmap(). */
         goto skip_mmap;
 
-    fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
+    fres->addr = mmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT,
                       fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
     if ( fres->addr == MAP_FAILED )
         return -1;
diff --git a/tools/libs/foreignmemory/minios.c b/tools/libs/foreignmemory/minios.c
index 43341ca301..c5453736d5 100644
--- a/tools/libs/foreignmemory/minios.c
+++ b/tools/libs/foreignmemory/minios.c
@@ -55,7 +55,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
                                  void *addr, size_t num)
 {
-    return munmap(addr, num << PAGE_SHIFT);
+    return munmap(addr, num << XC_PAGE_SHIFT);
 }
 
 /*
diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
index c0b1b8f79d..597db775d7 100644
--- a/tools/libs/foreignmemory/netbsd.c
+++ b/tools/libs/foreignmemory/netbsd.c
@@ -76,7 +76,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
 {
     int fd = fmem->fd;
     privcmd_mmapbatch_v2_t ioctlx;
-    addr = mmap(addr, num * PAGE_SIZE, prot,
+    addr = mmap(addr, num * XC_PAGE_SIZE, prot,
                 flags | MAP_ANON | MAP_SHARED, -1, 0);
     if ( addr == MAP_FAILED ) {
         PERROR("osdep_xenforeignmemory_map: mmap failed");
@@ -93,7 +93,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
     {
         int saved_errno = errno;
         PERROR("osdep_xenforeignmemory_map: ioctl failed");
-        munmap(addr, num * PAGE_SIZE);
+        munmap(addr, num * XC_PAGE_SIZE);
         errno = saved_errno;
         return NULL;
     }
@@ -104,7 +104,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
                                  void *addr, size_t num)
 {
-    return munmap(addr, num * PAGE_SIZE);
+    return munmap(addr, num * XC_PAGE_SIZE);
 }
 
 int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
@@ -117,7 +117,7 @@ int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
 int osdep_xenforeignmemory_unmap_resource(
     xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
 {
-    return fres ? munmap(fres->addr, fres->nr_frames << PAGE_SHIFT) : 0;
+    return fres ? munmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT) : 0;
 }
 
 int osdep_xenforeignmemory_map_resource(
@@ -136,7 +136,7 @@ int osdep_xenforeignmemory_map_resource(
         /* Request for resource size.  Skip mmap(). */
         goto skip_mmap;
 
-    fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
+    fres->addr = mmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT,
                       fres->prot, fres->flags | MAP_ANON | MAP_SHARED, -1, 0);
     if ( fres->addr == MAP_FAILED )
         return -1;
diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
index 1ee3626dd2..65fe77aa5b 100644
--- a/tools/libs/foreignmemory/private.h
+++ b/tools/libs/foreignmemory/private.h
@@ -1,6 +1,7 @@
 #ifndef XENFOREIGNMEMORY_PRIVATE_H
 #define XENFOREIGNMEMORY_PRIVATE_H
 
+#include <xenctrl.h>
 #include <xentoollog.h>
 
 #include <xenforeignmemory.h>
@@ -10,14 +11,6 @@
 #include <xen/xen.h>
 #include <xen/sys/privcmd.h>
 
-#ifndef PAGE_SHIFT /* Mini-os, Yukk */
-#define PAGE_SHIFT           12
-#endif
-#ifndef __MINIOS__ /* Yukk */
-#define PAGE_SIZE            (1UL << PAGE_SHIFT)
-#define PAGE_MASK            (~(PAGE_SIZE-1))
-#endif
-
 struct xenforeignmemory_handle {
     xentoollog_logger *logger, *logger_tofree;
     unsigned flags;
-- 
2.20.1

Re: [PATCH v3 3/5] tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error

[Julien Grall]

Hi Costin,

On 10/05/2021 09:35, Costin Lupu wrote:
> @@ -168,7 +168,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
>       size_t i;
>       int rc;
>   
> -    addr = mmap(addr, num << PAGE_SHIFT, prot, flags | MAP_SHARED,
> +    addr = mmap(addr, num << XC_PAGE_SHIFT, prot, flags | MAP_SHARED,
>                   fd, 0);
>       if ( addr == MAP_FAILED )
>           return NULL;
> @@ -198,9 +198,10 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
>            */
>           privcmd_mmapbatch_t ioctlx;
>           xen_pfn_t *pfn;
> -        unsigned int pfn_arr_size = ROUNDUP((num * sizeof(*pfn)), PAGE_SHIFT);
> +        unsigned int pfn_arr_size = ROUNDUP((num * sizeof(*pfn)), XC_PAGE_SHIFT);
> +        int os_page_size = getpagesize();

Hmmm... Sorry I only realized now that the manpage suggests that 
getpagesize() is legacy:

     SVr4, 4.4BSD, SUSv2.  In SUSv2 the getpagesize() call is labeled 
LEGACY, and in POSIX.1-2001 it has been dropped; HP-UX does not have 
this call.

And then:

    Portable applications should employ sysconf(_SC_PAGESIZE) instead of 
getpagesize():

            #include <unistd.h>
            long sz = sysconf(_SC_PAGESIZE);

As this is only used by Linux, it is not clear to me whether this is 
important. Ian, what do you think?

>   
> -        if ( pfn_arr_size <= PAGE_SIZE )
> +        if ( pfn_arr_size <= os_page_size )

Your commit message suggests we are only s/PAGE_SHIFT/XC_PAGE_SHIFT/ but 
this is using getpagesize() instead. I agree it should be using the OS 
size. However, this should be clarified in the commit message.

The rest of the patch looks fine to me .

Cheers,

-- 
Julien Grall

Re: [PATCH v3 3/5] tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error

[Costin Lupu]

Hi Julien,

On 5/17/21 9:12 PM, Julien Grall wrote:
> Hi Costin,
> 
> On 10/05/2021 09:35, Costin Lupu wrote:
>> @@ -168,7 +168,7 @@ void
>> *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
>>       size_t i;
>>       int rc;
>>   -    addr = mmap(addr, num << PAGE_SHIFT, prot, flags | MAP_SHARED,
>> +    addr = mmap(addr, num << XC_PAGE_SHIFT, prot, flags | MAP_SHARED,
>>                   fd, 0);
>>       if ( addr == MAP_FAILED )
>>           return NULL;
>> @@ -198,9 +198,10 @@ void
>> *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
>>            */
>>           privcmd_mmapbatch_t ioctlx;
>>           xen_pfn_t *pfn;
>> -        unsigned int pfn_arr_size = ROUNDUP((num * sizeof(*pfn)),
>> PAGE_SHIFT);
>> +        unsigned int pfn_arr_size = ROUNDUP((num * sizeof(*pfn)),
>> XC_PAGE_SHIFT);
>> +        int os_page_size = getpagesize();
> 
> Hmmm... Sorry I only realized now that the manpage suggests that
> getpagesize() is legacy:
> 
>    SVr4, 4.4BSD, SUSv2.  In SUSv2 the getpagesize() call is labeled
> LEGACY, and in POSIX.1-2001 it has been dropped; HP-UX does not have
> this call.
> 
> And then:
> 
>   Portable applications should employ sysconf(_SC_PAGESIZE) instead of
> getpagesize():
> 
>           #include <unistd.h>
>           long sz = sysconf(_SC_PAGESIZE);
> 
> As this is only used by Linux, it is not clear to me whether this is
> important. Ian, what do you think?
> 

I think it would be safer to follow the man page indication. I've just
sent a v4.

>>   -        if ( pfn_arr_size <= PAGE_SIZE )
>> +        if ( pfn_arr_size <= os_page_size )
> 
> Your commit message suggests we are only s/PAGE_SHIFT/XC_PAGE_SHIFT/ but
> this is using getpagesize() instead. I agree it should be using the OS
> size. However, this should be clarified in the commit message.
> 

Done.

> The rest of the patch looks fine to me .

Thanks,
Costin

[PATCH v3 4/5] tools/libs/gnttab: Fix PAGE_SIZE redefinition error

[Costin Lupu]

If PAGE_SIZE is already defined in the system (e.g. in /usr/include/limits.h
header) then gcc will trigger a redefinition error because of -Werror. This
patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity (which is what we are dealing with here).

Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
---
 tools/libs/gnttab/freebsd.c | 28 +++++++++++++---------------
 tools/libs/gnttab/linux.c   | 28 +++++++++++++---------------
 tools/libs/gnttab/netbsd.c  | 23 ++++++++++-------------
 3 files changed, 36 insertions(+), 43 deletions(-)

diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
index 768af701c6..e42ac3fbf3 100644
--- a/tools/libs/gnttab/freebsd.c
+++ b/tools/libs/gnttab/freebsd.c
@@ -30,14 +30,11 @@
 
 #include <xen/sys/gntdev.h>
 
+#include <xenctrl.h>
 #include <xen-tools/libs.h>
 
 #include "private.h"
 
-#define PAGE_SHIFT           12
-#define PAGE_SIZE            (1UL << PAGE_SHIFT)
-#define PAGE_MASK            (~(PAGE_SIZE-1))
-
 #define DEVXEN "/dev/xen/gntdev"
 
 int osdep_gnttab_open(xengnttab_handle *xgt)
@@ -77,10 +74,11 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
     int domids_stride;
     unsigned int refs_size = ROUNDUP(count *
                                      sizeof(struct ioctl_gntdev_grant_ref),
-                                     PAGE_SHIFT);
+                                     XC_PAGE_SHIFT);
+    int os_page_size = getpagesize();
 
     domids_stride = (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN) ? 0 : 1;
-    if ( refs_size <= PAGE_SIZE )
+    if ( refs_size <= os_page_size )
         map.refs = malloc(refs_size);
     else
     {
@@ -107,7 +105,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
         goto out;
     }
 
-    addr = mmap(NULL, PAGE_SIZE * count, prot, MAP_SHARED, fd,
+    addr = mmap(NULL, XC_PAGE_SIZE * count, prot, MAP_SHARED, fd,
                 map.index);
     if ( addr != MAP_FAILED )
     {
@@ -116,7 +114,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 
         notify.index = map.index;
         notify.action = 0;
-        if ( notify_offset < PAGE_SIZE * count )
+        if ( notify_offset < XC_PAGE_SIZE * count )
         {
             notify.index += notify_offset;
             notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -131,7 +129,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
         if ( rv )
         {
             GTERROR(xgt->logger, "ioctl SET_UNMAP_NOTIFY failed");
-            munmap(addr, count * PAGE_SIZE);
+            munmap(addr, count * XC_PAGE_SIZE);
             addr = MAP_FAILED;
         }
     }
@@ -150,7 +148,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
     }
 
  out:
-    if ( refs_size > PAGE_SIZE )
+    if ( refs_size > os_page_size )
         munmap(map.refs, refs_size);
     else
         free(map.refs);
@@ -189,7 +187,7 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
     }
 
     /* Next, unmap the memory. */
-    if ( (rc = munmap(start_address, count * PAGE_SIZE)) )
+    if ( (rc = munmap(start_address, count * XC_PAGE_SIZE)) )
         return rc;
 
     /* Finally, unmap the driver slots used to store the grant information. */
@@ -256,7 +254,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
         goto out;
     }
 
-    area = mmap(NULL, count * PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
+    area = mmap(NULL, count * XC_PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
                 fd, gref_info.index);
 
     if ( area == MAP_FAILED )
@@ -268,7 +266,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 
     notify.index = gref_info.index;
     notify.action = 0;
-    if ( notify_offset < PAGE_SIZE * count )
+    if ( notify_offset < XC_PAGE_SIZE * count )
     {
         notify.index += notify_offset;
         notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -283,7 +281,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
     if ( err )
     {
         GSERROR(xgs->logger, "ioctl SET_UNMAP_NOTIFY failed");
-        munmap(area, count * PAGE_SIZE);
+        munmap(area, count * XC_PAGE_SIZE);
         area = NULL;
     }
 
@@ -306,7 +304,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 int osdep_gntshr_unshare(xengntshr_handle *xgs,
                          void *start_address, uint32_t count)
 {
-    return munmap(start_address, count * PAGE_SIZE);
+    return munmap(start_address, count * XC_PAGE_SIZE);
 }
 
 /*
diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 74331a4c7b..9ce27bee6e 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -32,14 +32,11 @@
 #include <xen/sys/gntdev.h>
 #include <xen/sys/gntalloc.h>
 
+#include <xenctrl.h>
 #include <xen-tools/libs.h>
 
 #include "private.h"
 
-#define PAGE_SHIFT           12
-#define PAGE_SIZE            (1UL << PAGE_SHIFT)
-#define PAGE_MASK            (~(PAGE_SIZE-1))
-
 #define DEVXEN "/dev/xen/"
 
 #ifndef O_CLOEXEC
@@ -92,6 +89,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
     int fd = xgt->fd;
     struct ioctl_gntdev_map_grant_ref *map;
     unsigned int map_size = sizeof(*map) + (count - 1) * sizeof(map->refs[0]);
+    int os_page_size = getpagesize();
     void *addr = NULL;
     int domids_stride = 1;
     int i;
@@ -99,11 +97,11 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
     if (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN)
         domids_stride = 0;
 
-    if ( map_size <= PAGE_SIZE )
+    if ( map_size <= os_page_size )
         map = alloca(map_size);
     else
     {
-        map_size = ROUNDUP(map_size, PAGE_SHIFT);
+        map_size = ROUNDUP(map_size, XC_PAGE_SHIFT);
         map = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
                    MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
         if ( map == MAP_FAILED )
@@ -127,7 +125,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
     }
 
  retry:
-    addr = mmap(NULL, PAGE_SIZE * count, prot, MAP_SHARED, fd,
+    addr = mmap(NULL, XC_PAGE_SIZE * count, prot, MAP_SHARED, fd,
                 map->index);
 
     if (addr == MAP_FAILED && errno == EAGAIN)
@@ -152,7 +150,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
         struct ioctl_gntdev_unmap_notify notify;
         notify.index = map->index;
         notify.action = 0;
-        if (notify_offset < PAGE_SIZE * count) {
+        if (notify_offset < XC_PAGE_SIZE * count) {
             notify.index += notify_offset;
             notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
         }
@@ -164,7 +162,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
             rv = ioctl(fd, IOCTL_GNTDEV_SET_UNMAP_NOTIFY, &notify);
         if (rv) {
             GTERROR(xgt->logger, "ioctl SET_UNMAP_NOTIFY failed");
-            munmap(addr, count * PAGE_SIZE);
+            munmap(addr, count * XC_PAGE_SIZE);
             addr = MAP_FAILED;
         }
     }
@@ -184,7 +182,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
     }
 
  out:
-    if ( map_size > PAGE_SIZE )
+    if ( map_size > os_page_size )
         munmap(map, map_size);
 
     return addr;
@@ -220,7 +218,7 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
     }
 
     /* Next, unmap the memory. */
-    if ( (rc = munmap(start_address, count * PAGE_SIZE)) )
+    if ( (rc = munmap(start_address, count * XC_PAGE_SIZE)) )
         return rc;
 
     /* Finally, unmap the driver slots used to store the grant information. */
@@ -466,7 +464,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
         goto out;
     }
 
-    area = mmap(NULL, count * PAGE_SIZE, PROT_READ | PROT_WRITE,
+    area = mmap(NULL, count * XC_PAGE_SIZE, PROT_READ | PROT_WRITE,
         MAP_SHARED, fd, gref_info->index);
 
     if (area == MAP_FAILED) {
@@ -477,7 +475,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 
     notify.index = gref_info->index;
     notify.action = 0;
-    if (notify_offset < PAGE_SIZE * count) {
+    if (notify_offset < XC_PAGE_SIZE * count) {
         notify.index += notify_offset;
         notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
     }
@@ -489,7 +487,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
         err = ioctl(fd, IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &notify);
     if (err) {
         GSERROR(xgs->logger, "ioctl SET_UNMAP_NOTIFY failed");
-        munmap(area, count * PAGE_SIZE);
+        munmap(area, count * XC_PAGE_SIZE);
         area = NULL;
     }
 
@@ -510,7 +508,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 int osdep_gntshr_unshare(xengntshr_handle *xgs,
                          void *start_address, uint32_t count)
 {
-    return munmap(start_address, count * PAGE_SIZE);
+    return munmap(start_address, count * XC_PAGE_SIZE);
 }
 
 /*
diff --git a/tools/libs/gnttab/netbsd.c b/tools/libs/gnttab/netbsd.c
index f8d7c356eb..a4ad624b54 100644
--- a/tools/libs/gnttab/netbsd.c
+++ b/tools/libs/gnttab/netbsd.c
@@ -28,15 +28,12 @@
 #include <sys/ioctl.h>
 #include <sys/mman.h>
 
+#include <xenctrl.h>
 #include <xen/xen.h>
 #include <xen/xenio.h>
 
 #include "private.h"
 
-#define PAGE_SHIFT           12
-#define PAGE_SIZE            (1UL << PAGE_SHIFT)
-#define PAGE_MASK            (~(PAGE_SIZE-1))
-
 #define DEVXEN "/kern/xen/privcmd"
 
 int osdep_gnttab_open(xengnttab_handle *xgt)
@@ -87,19 +84,19 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
     }
 
     map.count = count;
-    addr = mmap(NULL, count * PAGE_SIZE,
+    addr = mmap(NULL, count * XC_PAGE_SIZE,
                 prot, flags | MAP_ANON | MAP_SHARED, -1, 0);
     if ( map.va == MAP_FAILED )
     {
         GTERROR(xgt->logger, "osdep_gnttab_grant_map: mmap failed");
-        munmap((void *)map.va, count * PAGE_SIZE);
+        munmap((void *)map.va, count * XC_PAGE_SIZE);
         addr = MAP_FAILED;
     }
     map.va = addr;
 
     map.notify.offset = 0;
     map.notify.action = 0;
-    if ( notify_offset < PAGE_SIZE * count )
+    if ( notify_offset < XC_PAGE_SIZE * count )
     {
         map.notify.offset = notify_offset;
         map.notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -115,7 +112,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
     {
         GTERROR(xgt->logger,
             "ioctl IOCTL_GNTDEV_MMAP_GRANT_REF failed: %d", rv);
-        munmap(addr, count * PAGE_SIZE);
+        munmap(addr, count * XC_PAGE_SIZE);
         addr = MAP_FAILED;
     }
 
@@ -136,7 +133,7 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
     }
 
     /* Next, unmap the memory. */
-    rc = munmap(start_address, count * PAGE_SIZE);
+    rc = munmap(start_address, count * XC_PAGE_SIZE);
 
     return rc;
 }
@@ -187,7 +184,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
     alloc.domid = domid;
     alloc.flags = writable ? GNTDEV_ALLOC_FLAG_WRITABLE : 0;
     alloc.count = count;
-    area = mmap(NULL, count * PAGE_SIZE,
+    area = mmap(NULL, count * XC_PAGE_SIZE,
                 PROT_READ | PROT_WRITE, MAP_ANON | MAP_SHARED, -1, 0);
 
     if ( area == MAP_FAILED )
@@ -200,7 +197,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 
     alloc.notify.offset = 0;
     alloc.notify.action = 0;
-    if ( notify_offset < PAGE_SIZE * count )
+    if ( notify_offset < XC_PAGE_SIZE * count )
     {
         alloc.notify.offset = notify_offset;
         alloc.notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -215,7 +212,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
     if ( err )
     {
         GSERROR(xgs->logger, "IOCTL_GNTDEV_ALLOC_GRANT_REF failed");
-        munmap(area, count * PAGE_SIZE);
+        munmap(area, count * XC_PAGE_SIZE);
         area = MAP_FAILED;
         goto out;
     }
@@ -230,7 +227,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 int osdep_gntshr_unshare(xengntshr_handle *xgs,
                          void *start_address, uint32_t count)
 {
-    return munmap(start_address, count * PAGE_SIZE);
+    return munmap(start_address, count * XC_PAGE_SIZE);
 }
 
 /*
-- 
2.20.1

Re: [PATCH v3 4/5] tools/libs/gnttab: Fix PAGE_SIZE redefinition error

[Julien Grall]

Hi Costin,

On 10/05/2021 09:35, Costin Lupu wrote:
> If PAGE_SIZE is already defined in the system (e.g. in /usr/include/limits.h
> header) then gcc will trigger a redefinition error because of -Werror. This
> patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
> confusion between control domain page granularity (PAGE_* definitions) and
> guest domain page granularity (which is what we are dealing with here).
> 
> Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
> ---
>   tools/libs/gnttab/freebsd.c | 28 +++++++++++++---------------
>   tools/libs/gnttab/linux.c   | 28 +++++++++++++---------------
>   tools/libs/gnttab/netbsd.c  | 23 ++++++++++-------------
>   3 files changed, 36 insertions(+), 43 deletions(-)
> 
> diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
> index 768af701c6..e42ac3fbf3 100644
> --- a/tools/libs/gnttab/freebsd.c
> +++ b/tools/libs/gnttab/freebsd.c
> @@ -30,14 +30,11 @@
>   
>   #include <xen/sys/gntdev.h>
>   
> +#include <xenctrl.h>
>   #include <xen-tools/libs.h>
>   
>   #include "private.h"
>   
> -#define PAGE_SHIFT           12
> -#define PAGE_SIZE            (1UL << PAGE_SHIFT)
> -#define PAGE_MASK            (~(PAGE_SIZE-1))
> -
>   #define DEVXEN "/dev/xen/gntdev"
>   
>   int osdep_gnttab_open(xengnttab_handle *xgt)
> @@ -77,10 +74,11 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
>       int domids_stride;
>       unsigned int refs_size = ROUNDUP(count *
>                                        sizeof(struct ioctl_gntdev_grant_ref),
> -                                     PAGE_SHIFT);
> +                                     XC_PAGE_SHIFT);
> +    int os_page_size = getpagesize();

Same remark as for patch #4. This at least want to be explained in the 
commit message.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 4/5] tools/libs/gnttab: Fix PAGE_SIZE redefinition error

[Costin Lupu]

Hi Julien,

On 5/17/21 9:16 PM, Julien Grall wrote:
> Hi Costin,
> 
> On 10/05/2021 09:35, Costin Lupu wrote:
>> If PAGE_SIZE is already defined in the system (e.g. in
>> /usr/include/limits.h
>> header) then gcc will trigger a redefinition error because of -Werror.
>> This
>> patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order
>> to avoid
>> confusion between control domain page granularity (PAGE_* definitions)
>> and
>> guest domain page granularity (which is what we are dealing with here).
>>
>> Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
>> ---
>>   tools/libs/gnttab/freebsd.c | 28 +++++++++++++---------------
>>   tools/libs/gnttab/linux.c   | 28 +++++++++++++---------------
>>   tools/libs/gnttab/netbsd.c  | 23 ++++++++++-------------
>>   3 files changed, 36 insertions(+), 43 deletions(-)
>>
>> diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
>> index 768af701c6..e42ac3fbf3 100644
>> --- a/tools/libs/gnttab/freebsd.c
>> +++ b/tools/libs/gnttab/freebsd.c
>> @@ -30,14 +30,11 @@
>>     #include <xen/sys/gntdev.h>
>>   +#include <xenctrl.h>
>>   #include <xen-tools/libs.h>
>>     #include "private.h"
>>   -#define PAGE_SHIFT           12
>> -#define PAGE_SIZE            (1UL << PAGE_SHIFT)
>> -#define PAGE_MASK            (~(PAGE_SIZE-1))
>> -
>>   #define DEVXEN "/dev/xen/gntdev"
>>     int osdep_gnttab_open(xengnttab_handle *xgt)
>> @@ -77,10 +74,11 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
>>       int domids_stride;
>>       unsigned int refs_size = ROUNDUP(count *
>>                                        sizeof(struct
>> ioctl_gntdev_grant_ref),
>> -                                     PAGE_SHIFT);
>> +                                     XC_PAGE_SHIFT);
>> +    int os_page_size = getpagesize();
> 
> Same remark as for patch #4. This at least want to be explained in the
> commit message.

Done.


Thanks,
Costin

[PATCH v3 5/5] tools/ocaml: Fix redefinition errors

[Costin Lupu]

If PAGE_SIZE is already defined in the system (e.g. in /usr/include/limits.h
header) then gcc will trigger a redefinition error because of -Werror. This
patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity (which is what we are dealing with here).

Same issue applies for redefinitions of Val_none and Some_val macros which
can be already define in the OCaml system headers (e.g.
/usr/lib/ocaml/caml/mlvalues.h).

Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
---
 tools/ocaml/libs/xc/xenctrl_stubs.c            | 10 ++++------
 tools/ocaml/libs/xentoollog/xentoollog_stubs.c |  4 ++++
 tools/ocaml/libs/xl/xenlight_stubs.c           |  4 ++++
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index d05d7bb30e..f9e33e599a 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -36,14 +36,12 @@
 
 #include "mmap_stubs.h"
 
-#define PAGE_SHIFT		12
-#define PAGE_SIZE               (1UL << PAGE_SHIFT)
-#define PAGE_MASK               (~(PAGE_SIZE-1))
-
 #define _H(__h) ((xc_interface *)(__h))
 #define _D(__d) ((uint32_t)Int_val(__d))
 
+#ifndef Val_none
 #define Val_none (Val_int(0))
+#endif
 
 #define string_of_option_array(array, index) \
 	((Field(array, index) == Val_none) ? NULL : String_val(Field(Field(array, index), 0)))
@@ -818,7 +816,7 @@ CAMLprim value stub_xc_domain_memory_increase_reservation(value xch,
 	CAMLparam3(xch, domid, mem_kb);
 	int retval;
 
-	unsigned long nr_extents = ((unsigned long)(Int64_val(mem_kb))) >> (PAGE_SHIFT - 10);
+	unsigned long nr_extents = ((unsigned long)(Int64_val(mem_kb))) >> (XC_PAGE_SHIFT - 10);
 
 	uint32_t c_domid = _D(domid);
 	caml_enter_blocking_section();
@@ -924,7 +922,7 @@ CAMLprim value stub_pages_to_kib(value pages)
 {
 	CAMLparam1(pages);
 
-	CAMLreturn(caml_copy_int64(Int64_val(pages) << (PAGE_SHIFT - 10)));
+	CAMLreturn(caml_copy_int64(Int64_val(pages) << (XC_PAGE_SHIFT - 10)));
 }
 
 
diff --git a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
index bf64b211c2..e4306a0c2f 100644
--- a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
+++ b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
@@ -53,8 +53,12 @@ static char * dup_String_val(value s)
 #include "_xtl_levels.inc"
 
 /* Option type support as per http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
+#ifndef Val_none
 #define Val_none Val_int(0)
+#endif
+#ifndef Some_val
 #define Some_val(v) Field(v,0)
+#endif
 
 static value Val_some(value v)
 {
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
index 352a00134d..45b8af61c7 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -227,8 +227,12 @@ static value Val_string_list(libxl_string_list *c_val)
 }
 
 /* Option type support as per http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
+#ifndef Val_none
 #define Val_none Val_int(0)
+#endif
+#ifndef Some_val
 #define Some_val(v) Field(v,0)
+#endif
 
 static value Val_some(value v)
 {
-- 
2.20.1

Re: [PATCH v3 5/5] tools/ocaml: Fix redefinition errors

[Julien Grall]

Hi Costin,

On 10/05/2021 09:35, Costin Lupu wrote:
> If PAGE_SIZE is already defined in the system (e.g. in /usr/include/limits.h
> header) then gcc will trigger a redefinition error because of -Werror. This
> patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
> confusion between control domain page granularity (PAGE_* definitions) and
> guest domain page granularity (which is what we are dealing with here).
> 
> Same issue applies for redefinitions of Val_none and Some_val macros which
> can be already define in the OCaml system headers (e.g.
> /usr/lib/ocaml/caml/mlvalues.h).
> 
> Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

> ---
>   tools/ocaml/libs/xc/xenctrl_stubs.c            | 10 ++++------
>   tools/ocaml/libs/xentoollog/xentoollog_stubs.c |  4 ++++
>   tools/ocaml/libs/xl/xenlight_stubs.c           |  4 ++++
>   3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index d05d7bb30e..f9e33e599a 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -36,14 +36,12 @@
>   
>   #include "mmap_stubs.h"
>   
> -#define PAGE_SHIFT		12
> -#define PAGE_SIZE               (1UL << PAGE_SHIFT)
> -#define PAGE_MASK               (~(PAGE_SIZE-1))
> -
>   #define _H(__h) ((xc_interface *)(__h))
>   #define _D(__d) ((uint32_t)Int_val(__d))
>   
> +#ifndef Val_none
>   #define Val_none (Val_int(0))
> +#endif
>   
>   #define string_of_option_array(array, index) \
>   	((Field(array, index) == Val_none) ? NULL : String_val(Field(Field(array, index), 0)))
> @@ -818,7 +816,7 @@ CAMLprim value stub_xc_domain_memory_increase_reservation(value xch,
>   	CAMLparam3(xch, domid, mem_kb);
>   	int retval;
>   
> -	unsigned long nr_extents = ((unsigned long)(Int64_val(mem_kb))) >> (PAGE_SHIFT - 10);
> +	unsigned long nr_extents = ((unsigned long)(Int64_val(mem_kb))) >> (XC_PAGE_SHIFT - 10);
>   
>   	uint32_t c_domid = _D(domid);
>   	caml_enter_blocking_section();
> @@ -924,7 +922,7 @@ CAMLprim value stub_pages_to_kib(value pages)
>   {
>   	CAMLparam1(pages);
>   
> -	CAMLreturn(caml_copy_int64(Int64_val(pages) << (PAGE_SHIFT - 10)));
> +	CAMLreturn(caml_copy_int64(Int64_val(pages) << (XC_PAGE_SHIFT - 10)));
>   }
>   
>   
> diff --git a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
> index bf64b211c2..e4306a0c2f 100644
> --- a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
> +++ b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
> @@ -53,8 +53,12 @@ static char * dup_String_val(value s)
>   #include "_xtl_levels.inc"
>   
>   /* Option type support as per http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
> +#ifndef Val_none
>   #define Val_none Val_int(0)
> +#endif
> +#ifndef Some_val
>   #define Some_val(v) Field(v,0)
> +#endif
>   
>   static value Val_some(value v)
>   {
> diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
> index 352a00134d..45b8af61c7 100644
> --- a/tools/ocaml/libs/xl/xenlight_stubs.c
> +++ b/tools/ocaml/libs/xl/xenlight_stubs.c
> @@ -227,8 +227,12 @@ static value Val_string_list(libxl_string_list *c_val)
>   }
>   
>   /* Option type support as per http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
> +#ifndef Val_none
>   #define Val_none Val_int(0)
> +#endif
> +#ifndef Some_val
>   #define Some_val(v) Field(v,0)
> +#endif
>   
>   static value Val_some(value v)
>   {
> 

-- 
Julien Grall

Re: [PATCH v3 5/5] tools/ocaml: Fix redefinition errors

[Dario Faggioli]

[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]

Hey,

So...

On Mon, 2021-05-17 at 19:16 +0100, Julien Grall wrote:
> On 10/05/2021 09:35, Costin Lupu wrote:
> > If PAGE_SIZE is already defined in the system (e.g. in
> > /usr/include/limits.h
> > header) then gcc will trigger a redefinition error because of -
> > Werror. This
> > patch replaces usage of PAGE_* macros with XC_PAGE_* macros in
> > order to avoid
> > confusion between control domain page granularity (PAGE_*
> > definitions) and
> > guest domain page granularity (which is what we are dealing with
> > here).
> > 
> > Same issue applies for redefinitions of Val_none and Some_val
> > macros which
> > can be already define in the OCaml system headers (e.g.
> > /usr/lib/ocaml/caml/mlvalues.h).
> > 
... At least this part is absolutely necessary for building Xen on
openSUSE Tumbleweed, therefore:

> > Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>
> 
Tested-by: Dario Faggioli <dfaggioli@suse.com>

Thanks and Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]