Re: [PATCH] tools/misc/xencov.c: Use MAP_WIRED on NetBSD

[Frediano Ziglio <frediano.ziglio@citrite.net>]

Subject: [PATCH] gcov: Do not use mmap directly but use xc_hypercall_buffer_alloc

xencov.c did not compile on NetBSD so use xc_hypercall_buffer which is
more portable.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
---
 tools/misc/xencov.c |   19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

Here you are. It get tested on a x64 machine, if somebody could test it
with a NetBSD would be great.

diff --git a/tools/misc/xencov.c b/tools/misc/xencov.c
index 6645a30..e38d473 100644
--- a/tools/misc/xencov.c
+++ b/tools/misc/xencov.c
@@ -24,7 +24,6 @@
 #include <unistd.h>
 #include <errno.h>
 #include <err.h>
-#include <sys/mman.h>
 
 static xc_interface *gcov_xch = NULL;
 
@@ -35,7 +34,7 @@ static void gcov_init(void)
         err(1, "opening interface");
 }
 
-int gcov_get_info(int op, struct xen_sysctl *sys, void *ptr)
+int gcov_get_info(int op, struct xen_sysctl *sys, uint8_t *ptr)
 {
     struct xen_sysctl_coverage_op *cov;
 
@@ -44,7 +43,7 @@ int gcov_get_info(int op, struct xen_sysctl *sys, void *ptr)
 
     cov = &sys->u.coverage_op;
     cov->cmd = op;
-    cov->u.raw_info.p = ptr;
+    set_xen_guest_handle_raw(cov->u.raw_info, ptr);
 
     return xc_sysctl(gcov_xch, sys);
 }
@@ -52,10 +51,8 @@ int gcov_get_info(int op, struct xen_sysctl *sys, void *ptr)
 static void gcov_read(const char *fn, int reset)
 {
     struct xen_sysctl sys;
-    unsigned page_size = sysconf(_SC_PAGESIZE);
     uint32_t total_len;
-    uint8_t *p;
-    size_t size;
+    DECLARE_HYPERCALL_BUFFER(uint8_t, p);
     FILE *f;
     int op = reset ? XEN_SYSCTL_COVERAGE_read_and_reset :
                      XEN_SYSCTL_COVERAGE_read;
@@ -71,12 +68,9 @@ static void gcov_read(const char *fn, int reset)
         errx(1, "coverage size too big %u bytes\n", total_len);
 
     /* allocate */
-    size = total_len + page_size;
-    size -= (size % page_size);
-    p = mmap(0, size, PROT_WRITE|PROT_READ,
-             MAP_PRIVATE|MAP_ANON|MAP_LOCKED, -1, 0);
-    if ( p == (uint8_t *) -1 )
-        err(1, "mapping memory for coverage");
+    p = xc_hypercall_buffer_alloc(gcov_xch, p, total_len);
+    if ( p == NULL )
+        err(1, "allocating memory for coverage");
 
     /* get data */
     memset(p, 0, total_len);
@@ -94,6 +88,7 @@ static void gcov_read(const char *fn, int reset)
         err(1, "writing coverage to file");
     if (f != stdout)
         fclose(f);
+    xc_hypercall_buffer_free(gcov_xch, p);
 }
 
 static void gcov_reset(void)
-- 
1.7.10.4


On Fri, 2013-05-31 at 12:25 +0100, Ian Campbell wrote:
> On Thu, 2013-05-30 at 14:46 +0200, Christoph Egger wrote:
> > tools/xencov: MAP_LOCKED is Linux specific. NetBSD's equivalent is MAP_WIRED.
> 
> It looks to me as if xencov.c's shouldn't be using mmap in the first
> place. It should be using the libxc hypercall buffer code.
> 
> I think xencov.c should be using xc_hypercall_buffer_alloc(_pages) and
> gcov_get_info should be xc_gcov_get_info & inside libxc not open coded
> in this tool. Not least because of its abuse of the GUEST_HANDLE
> interface (going directly at .p instead of using xen_set_guest_handle(),
> which is forbidden).
> 
> Frediano, do you have time to sort this out?
> 
> NB On Linux the hypercall buffers currently turn into MAP_PRIVATE|
> MAP_ANONYMOUS|MAP_LOCKED but on netbsd it is just xc_memalign.
> Presumably netbsd doesn't have to worry about the hypercall buffer being
> unmapped/swapped/paged/etc under its feet? Or if it does it needs to be
> fixed in libxc...
> 
> Thanks,
> Ian.
> 
> > From: Miguel Clara <miguelmclara@gmail.com>
> > Signed-off-by: Christoph Egger <chegger@amazon.de>
> > 
> > diff -r f1fa13daf4b7 -r 97a4f7e36a8f tools/misc/xencov.c
> > --- a/tools/misc/xencov.c	Fri May 10 13:45:02 2013 +0000
> > +++ b/tools/misc/xencov.c	Thu May 30 12:40:13 2013 +0000
> > @@ -26,6 +26,11 @@
> >  #include <err.h>
> >  #include <sys/mman.h>
> >  
> > +#ifdef __NetBSD__
> > +/* MAP_LOCKED is Linux specific. MAP_WIRED is NetBSD's equivalent. */
> > +#define MAP_LOCKED MAP_WIRED
> > +#endif
> > +
> >  static xc_interface *gcov_xch = NULL;
> >  
> >  static void gcov_init(void)
> 
>