diff mbox series

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

Message ID 2040286fc39b7e1d46376a8e75ac59d8d3be5aff.1620633386.git.costin.lupu@cs.pub.ro (mailing list archive)
State Superseded
Headers show
Series Fix redefinition errors for toolstack libs | expand

Commit Message

Costin Lupu May 10, 2021, 8:35 a.m. UTC
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(-)

Comments

Julien Grall May 17, 2021, 6:12 p.m. UTC | #1
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,
Costin Lupu June 8, 2021, 12:37 p.m. UTC | #2
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
diff mbox series

Patch

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;