[PATCH v0 0/3] Modifications to mem_event_enable API and addition of teardown routine.

[PATCH v0 0/3] Modifications to mem_event_enable API and addition of teardown routine.

[Dushyant Behl]

This patch series is the corrected version of the following patch series,
http://lists.xenproject.org/archives/html/xen-devel/2014-08/msg00847.html
http://lists.xenproject.org/archives/html/xen-devel/2014-08/msg00848.html
http://lists.xenproject.org/archives/html/xen-devel/2014-08/msg00846.html
http://lists.xenproject.org/archives/html/xen-devel/2014-08/msg00849.html

The previous patch series had some code which was actually present in the tree
after addition of XSA-99. Please ignore the older patch series and have a look
at this patch series.

[PATCH v0 1/3] mem_access: modifications to mem_event enable API.
This patch modifies the xc_mem_event_enable API by -
 1. adding shared ring init.
 2. clearing ring page.
 3. replacing calls to deprecated API's.
 4. updating the xc_mem_event_enable API to have simmilar signature with other
    mem_event API's (This was my personal choice and is certainly debatable).

[PATCH v0 2/3] mem_event: Added new helper API to teardown mem event and unmap ring_page.
This patch adds a new xc_mem_event_teardown API to teardown the mem event setup
for the PAGING, ACCESS and SHARING HVM params.

[PATCH v0 3/3] xenpaging: updated code to use safer mem_event API's for setup and teardown.
This patch simply updates the code to use the newly created API's.

Signed-off-by: Dushyant Behl <myselfdushyantbehl@gmail.com>

[PATCH v0 1/3] mem_access: modifications to mem_event enable API.

[Dushyant Behl]

tools/libxc/:
1. Modified the API xc_mem_event_enable to initialize
   shared ring to communicate with the hypervisor along with enabling mem_event.
2. Added memset to clear the ring_page of any bogus input before enabling any events.
3. Replaced calls to deprecated function xc_map_foreign_batch with calls
   to xc_map_foreign_bulk. The function xc_map_foreign_bulk has a cleaner
   error reporting interface than xc_map_foreign_batch.
4. The API xc_mem_event_enable is now modified to return int rather than void *,
   this was done to synchronize this API's behaviour with other mem_event API's.

tools/tests/xen-access/: Updated code to use the new helper API.

tools/ocaml/libs/xc/xenctrl_stubs.c: Changed the name of a macro from RING_SIZE
to BUF_RING_SIZE because the earlier name collided with xen shared ring
deinitions in io/ring.h

Signed-off-by: Dushyant Behl <myselfdushyantbehl@gmail.com>
---
 tools/libxc/xc_mem_access.c         |  8 ++++--
 tools/libxc/xc_mem_event.c          | 49 +++++++++++++++++++++++++++----------
 tools/libxc/xc_private.h            | 10 +++++---
 tools/libxc/xenctrl.h               |  9 ++++---
 tools/ocaml/libs/xc/xenctrl_stubs.c |  6 ++---
 tools/tests/xen-access/xen-access.c | 17 +++++--------
 6 files changed, 64 insertions(+), 35 deletions(-)

diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
index 461f0e9..89050be 100644
--- a/tools/libxc/xc_mem_access.c
+++ b/tools/libxc/xc_mem_access.c
@@ -24,9 +24,13 @@
 #include "xc_private.h"
 #include <xen/memory.h>
 
-void *xc_mem_access_enable(xc_interface *xch, domid_t domain_id, uint32_t *port)
+int xc_mem_access_enable(xc_interface *xch, domid_t domain_id,
+                         uint32_t *port, void *ring_page,
+                         mem_event_back_ring_t *back_ring)
 {
-    return xc_mem_event_enable(xch, domain_id, HVM_PARAM_ACCESS_RING_PFN, port);
+    return xc_mem_event_enable(xch, domain_id,
+                               HVM_PARAM_ACCESS_RING_PFN,
+                               port, ring_page, back_ring);
 }
 
 int xc_mem_access_disable(xc_interface *xch, domid_t domain_id)
diff --git a/tools/libxc/xc_mem_event.c b/tools/libxc/xc_mem_event.c
index faf1cc6..3525a83 100644
--- a/tools/libxc/xc_mem_event.c
+++ b/tools/libxc/xc_mem_event.c
@@ -22,6 +22,7 @@
  */
 
 #include "xc_private.h"
+#include <xen/mem_event.h>
 
 int xc_mem_event_control(xc_interface *xch, domid_t domain_id, unsigned int op,
                          unsigned int mode, uint32_t *port)
@@ -56,19 +57,27 @@ int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
     return do_memory_op(xch, mode, &meo, sizeof(meo));
 }
 
-void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
-                          uint32_t *port)
+/*
+ * Enables mem_event and initializes shared ring to communicate with hypervisor
+ * returns 0 if success and if failure returns -errno with
+ * errno properly set to indicate possible error.
+ * param can be HVM_PARAM_PAGING/ACCESS/SHARING_RING_PFN
+ */
+int xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
+                        uint32_t *port, void *ring_page,
+                        mem_event_back_ring_t *back_ring)
 {
-    void *ring_page = NULL;
     uint64_t pfn;
     xen_pfn_t ring_pfn, mmap_pfn;
     unsigned int op, mode;
-    int rc1, rc2, saved_errno;
+    int rc1, rc2, saved_errno, err;
+
+    ring_page = NULL;
 
     if ( !port )
     {
         errno = EINVAL;
-        return NULL;
+        return -errno;
     }
 
     /* Pause the domain for ring page setup */
@@ -76,7 +85,7 @@ void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
     if ( rc1 != 0 )
     {
         PERROR("Unable to pause domain\n");
-        return NULL;
+        return -errno;
     }
 
     /* Get the pfn of the ring page */
@@ -89,9 +98,9 @@ void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
 
     ring_pfn = pfn;
     mmap_pfn = pfn;
-    ring_page = xc_map_foreign_batch(xch, domain_id, PROT_READ | PROT_WRITE,
-                                     &mmap_pfn, 1);
-    if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
+    ring_page = xc_map_foreign_bulk(xch, domain_id,
+                                    PROT_READ | PROT_WRITE, &mmap_pfn, &err, 1);
+    if ( (err != 0) || (ring_page == NULL) )
     {
         /* Map failed, populate ring page */
         rc1 = xc_domain_populate_physmap_exact(xch, domain_id, 1, 0, 0,
@@ -103,15 +112,23 @@ void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
         }
 
         mmap_pfn = ring_pfn;
-        ring_page = xc_map_foreign_batch(xch, domain_id, PROT_READ | PROT_WRITE,
-                                         &mmap_pfn, 1);
-        if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
+        ring_page = xc_map_foreign_bulk(xch, domain_id, PROT_READ | PROT_WRITE,
+                                        &mmap_pfn, &err, 1);
+        if ( (err != 0) || (ring_page == NULL) )
         {
             PERROR("Could not map the ring page\n");
+            rc1 = -1;
             goto out;
         }
     }
 
+    /* Clear the ring page */
+    memset(ring_page, 0, PAGE_SIZE);
+
+    /* Initialise ring */
+    SHARED_RING_INIT((mem_event_sring_t *)ring_page);
+    BACK_RING_INIT(back_ring, (mem_event_sring_t *)ring_page, PAGE_SIZE);
+
     switch ( param )
     {
     case HVM_PARAM_PAGING_RING_PFN:
@@ -149,7 +166,12 @@ void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
     /* Remove the ring_pfn from the guest's physmap */
     rc1 = xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, &ring_pfn);
     if ( rc1 != 0 )
+    {
         PERROR("Failed to remove ring page from guest physmap");
+        goto out;
+    }
+
+    rc1 = 0;
 
  out:
     saved_errno = errno;
@@ -169,7 +191,8 @@ void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
         ring_page = NULL;
 
         errno = saved_errno;
+        rc1 = -errno;
     }
 
-    return ring_page;
+    return rc1;
 }
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index c50a7c9..cf9b223 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -32,6 +32,7 @@
 #include "xenctrl.h"
 #include "xenctrlosdep.h"
 
+#include <xen/mem_event.h>
 #include <xen/sys/privcmd.h>
 
 #if defined(HAVE_VALGRIND_MEMCHECK_H) && !defined(NDEBUG) && !defined(__MINIOS__)
@@ -377,10 +378,13 @@ int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
                         unsigned int op, unsigned int mode,
                         uint64_t gfn, void *buffer);
 /*
- * Enables mem_event and returns the mapped ring page indicated by param.
+ * Enables mem_event and initializes shared ring to communicate with hypervisor
+ * sets ring_page equal to mapped page.
+ * Returns 0 if success and if failure returns -errno with errno properly set.
  * param can be HVM_PARAM_PAGING/ACCESS/SHARING_RING_PFN
  */
-void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
-                          uint32_t *port);
+int xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
+                        uint32_t *port, void *ring_page,
+                        mem_event_back_ring_t *back_ring);
 
 #endif /* __XC_PRIVATE_H__ */
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 1c5d0db..d21f026 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -47,6 +47,7 @@
 #include <xen/xsm/flask_op.h>
 #include <xen/tmem.h>
 #include <xen/kexec.h>
+#include <xen/mem_event.h>
 
 #include "xentoollog.h"
 
@@ -2258,11 +2259,13 @@ int xc_mem_paging_load(xc_interface *xch, domid_t domain_id,
  */
 
 /*
- * Enables mem_access and returns the mapped ring page.
- * Will return NULL on error.
+ * Enables mem_access and sets arg ring page equal to mapped page.
+ * Will return 0 on success and -errno on error.
  * Caller has to unmap this page when done.
  */
-void *xc_mem_access_enable(xc_interface *xch, domid_t domain_id, uint32_t *port);
+int xc_mem_access_enable(xc_interface *xch, domid_t domain_id,
+                         uint32_t *port, void *ring_page,
+                         mem_event_back_ring_t *back_ring);
 int xc_mem_access_disable(xc_interface *xch, domid_t domain_id);
 int xc_mem_access_resume(xc_interface *xch, domid_t domain_id);
 
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index f0810eb..beccb38 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -526,12 +526,12 @@ CAMLprim value stub_xc_evtchn_reset(value xch, value domid)
 }
 
 
-#define RING_SIZE 32768
-static char ring[RING_SIZE];
+#define BUF_RING_SIZE 32768
+static char ring[BUF_RING_SIZE];
 
 CAMLprim value stub_xc_readconsolering(value xch)
 {
-	unsigned int size = RING_SIZE - 1;
+	unsigned int size = BUF_RING_SIZE - 1;
 	char *ring_ptr = ring;
 	int retval;
 
diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index 090df5f..9242f86 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -245,11 +245,12 @@ xenaccess_t *xenaccess_init(xc_interface **xch_r, domid_t domain_id)
     mem_event_ring_lock_init(&xenaccess->mem_event);
 
     /* Enable mem_access */
-    xenaccess->mem_event.ring_page =
-            xc_mem_access_enable(xenaccess->xc_handle,
-                                 xenaccess->mem_event.domain_id,
-                                 &xenaccess->mem_event.evtchn_port);
-    if ( xenaccess->mem_event.ring_page == NULL )
+    rc = xc_mem_access_enable(xenaccess->xc_handle,
+                              xenaccess->mem_event.domain_id,
+                              &xenaccess->mem_event.evtchn_port,
+                              xenaccess->mem_event.ring_page,
+                              &xenaccess->mem_event.back_ring);
+    if ( rc < 0 )
     {
         switch ( errno ) {
             case EBUSY:
@@ -287,12 +288,6 @@ xenaccess_t *xenaccess_init(xc_interface **xch_r, domid_t domain_id)
     evtchn_bind = 1;
     xenaccess->mem_event.port = rc;
 
-    /* Initialise ring */
-    SHARED_RING_INIT((mem_event_sring_t *)xenaccess->mem_event.ring_page);
-    BACK_RING_INIT(&xenaccess->mem_event.back_ring,
-                   (mem_event_sring_t *)xenaccess->mem_event.ring_page,
-                   XC_PAGE_SIZE);
-
     /* Get domaininfo */
     xenaccess->domain_info = malloc(sizeof(xc_domaininfo_t));
     if ( xenaccess->domain_info == NULL )
-- 
1.9.1

Re: [PATCH v0 1/3] mem_access: modifications to mem_event enable API.

[Aravindh Puthiyaparambil (aravindp)]

>4. The API xc_mem_event_enable is now modified to return int rather than
>void *,
>   this was done to synchronize this API's behaviour with other mem_event
>API's.

FWIW, since I am the one that introduced this... I am fine with the change. Though I did think that the norm was to return -1 on error and set errno to the appropriate value.

>diff --git a/tools/libxc/xc_mem_event.c b/tools/libxc/xc_mem_event.c
> index faf1cc6..3525a83 100644
>--- a/tools/libxc/xc_mem_event.c
>+++ b/tools/libxc/xc_mem_event.c

<snip>

>@@ -89,9 +98,9 @@ void *xc_mem_event_enable(xc_interface *xch,
>domid_t domain_id, int param,
>
>     ring_pfn = pfn;
>     mmap_pfn = pfn;
>-    ring_page = xc_map_foreign_batch(xch, domain_id, PROT_READ |
>PROT_WRITE,
>-                                     &mmap_pfn, 1);
>-    if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
>+    ring_page = xc_map_foreign_bulk(xch, domain_id,
>+                                    PROT_READ | PROT_WRITE,
>&mmap_pfn, &err, 1);
>+    if ( (err != 0) || (ring_page == NULL) )

Please remove the redundant brackets.

>     {
>         /* Map failed, populate ring page */
>         rc1 = xc_domain_populate_physmap_exact(xch, domain_id, 1, 0, 0, @@ -
>103,15 +112,23 @@ void *xc_mem_event_enable(xc_interface *xch,
>domid_t domain_id, int param,
>         }
>
>         mmap_pfn = ring_pfn;
>-        ring_page = xc_map_foreign_batch(xch, domain_id, PROT_READ |
>PROT_WRITE,
>-                                         &mmap_pfn, 1);
>-        if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
>+        ring_page = xc_map_foreign_bulk(xch, domain_id, PROT_READ |
>PROT_WRITE,
>+                                        &mmap_pfn, &err, 1);
>+        if ( (err != 0) || (ring_page == NULL) )

Please remove the redundant brackets.

>diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h index
>c50a7c9..cf9b223 100644
>--- a/tools/libxc/xc_private.h
>+++ b/tools/libxc/xc_private.h
>@@ -32,6 +32,7 @@
> #include "xenctrl.h"
> #include "xenctrlosdep.h"
>
>+#include <xen/mem_event.h>
> #include <xen/sys/privcmd.h>
>
> #if defined(HAVE_VALGRIND_MEMCHECK_H) && !defined(NDEBUG) &&
>!defined(__MINIOS__)
>@@ -377,10 +378,13 @@ int xc_mem_event_memop(xc_interface *xch,
>domid_t domain_id,
>                         unsigned int op, unsigned int mode,
>                         uint64_t gfn, void *buffer);
> /*
>- * Enables mem_event and returns the mapped ring page indicated by
>param.
>+ * Enables mem_event and initializes shared ring to communicate with
>+ hypervisor

Full stop or and.

>+ * sets ring_page equal to mapped page.
>+ * Returns 0 if success and if failure returns -errno with errno properly set.
>  * param can be HVM_PARAM_PAGING/ACCESS/SHARING_RING_PFN
>  */
>-void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int
>param,
>-                          uint32_t *port);
>+int xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int
>param,
>+                        uint32_t *port, void *ring_page,
>+                        mem_event_back_ring_t *back_ring);
>
> #endif /* __XC_PRIVATE_H__ */
>diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h index 1c5d0db..d21f026
>100644
>--- a/tools/libxc/xenctrl.h
>+++ b/tools/libxc/xenctrl.h
>@@ -47,6 +47,7 @@
> #include <xen/xsm/flask_op.h>
> #include <xen/tmem.h>
> #include <xen/kexec.h>
>+#include <xen/mem_event.h>
>
> #include "xentoollog.h"
>
>@@ -2258,11 +2259,13 @@ int xc_mem_paging_load(xc_interface *xch,
>domid_t domain_id,
>  */
>
> /*
>- * Enables mem_access and returns the mapped ring page.
>- * Will return NULL on error.
>+ * Enables mem_access and sets arg ring page equal to mapped page.

ring_page in the above line. I would leave arg out like you did in the previous comment.

>+ * Will return 0 on success and -errno on error.
>  * Caller has to unmap this page when done.
>  */
>-void *xc_mem_access_enable(xc_interface *xch, domid_t domain_id,
>uint32_t *port);
>+int xc_mem_access_enable(xc_interface *xch, domid_t domain_id,
>+                         uint32_t *port, void *ring_page,
>+                         mem_event_back_ring_t *back_ring);
> int xc_mem_access_disable(xc_interface *xch, domid_t domain_id);  int
>xc_mem_access_resume(xc_interface *xch, domid_t domain_id);

Thanks,
Aravindh

Re: [PATCH v0 1/3] mem_access: modifications to mem_event enable API.

[Dushyant Behl]

[-- Attachment #1.1: Type: text/plain, Size: 5009 bytes --]

Hi Aravindh,

Thank you for taking time to review the patches, I have updated the patches
according to your comments.
I actually looked in the code after your comment and thought that since
returning -1 was present mostly so I have changed the functions to return
-1 with errno properly set, Hope this is fine.
Please have a look at the v2 of patch series.

Thanks and Regards,
Dushyant Behl


On Sat, Aug 23, 2014 at 3:35 AM, Aravindh Puthiyaparambil (aravindp) <
aravindp@cisco.com> wrote:

> >4. The API xc_mem_event_enable is now modified to return int rather than
> >void *,
> >   this was done to synchronize this API's behaviour with other mem_event
> >API's.
>
> FWIW, since I am the one that introduced this... I am fine with the
> change. Though I did think that the norm was to return -1 on error and set
> errno to the appropriate value.
>
> >diff --git a/tools/libxc/xc_mem_event.c b/tools/libxc/xc_mem_event.c
> > index faf1cc6..3525a83 100644
> >--- a/tools/libxc/xc_mem_event.c
> >+++ b/tools/libxc/xc_mem_event.c
>
> <snip>
>
> >@@ -89,9 +98,9 @@ void *xc_mem_event_enable(xc_interface *xch,
> >domid_t domain_id, int param,
> >
> >     ring_pfn = pfn;
> >     mmap_pfn = pfn;
> >-    ring_page = xc_map_foreign_batch(xch, domain_id, PROT_READ |
> >PROT_WRITE,
> >-                                     &mmap_pfn, 1);
> >-    if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
> >+    ring_page = xc_map_foreign_bulk(xch, domain_id,
> >+                                    PROT_READ | PROT_WRITE,
> >&mmap_pfn, &err, 1);
> >+    if ( (err != 0) || (ring_page == NULL) )
>
> Please remove the redundant brackets.
>
> >     {
> >         /* Map failed, populate ring page */
> >         rc1 = xc_domain_populate_physmap_exact(xch, domain_id, 1, 0, 0,
> @@ -
> >103,15 +112,23 @@ void *xc_mem_event_enable(xc_interface *xch,
> >domid_t domain_id, int param,
> >         }
> >
> >         mmap_pfn = ring_pfn;
> >-        ring_page = xc_map_foreign_batch(xch, domain_id, PROT_READ |
> >PROT_WRITE,
> >-                                         &mmap_pfn, 1);
> >-        if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
> >+        ring_page = xc_map_foreign_bulk(xch, domain_id, PROT_READ |
> >PROT_WRITE,
> >+                                        &mmap_pfn, &err, 1);
> >+        if ( (err != 0) || (ring_page == NULL) )
>
> Please remove the redundant brackets.
>
> >diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h index
> >c50a7c9..cf9b223 100644
> >--- a/tools/libxc/xc_private.h
> >+++ b/tools/libxc/xc_private.h
> >@@ -32,6 +32,7 @@
> > #include "xenctrl.h"
> > #include "xenctrlosdep.h"
> >
> >+#include <xen/mem_event.h>
> > #include <xen/sys/privcmd.h>
> >
> > #if defined(HAVE_VALGRIND_MEMCHECK_H) && !defined(NDEBUG) &&
> >!defined(__MINIOS__)
> >@@ -377,10 +378,13 @@ int xc_mem_event_memop(xc_interface *xch,
> >domid_t domain_id,
> >                         unsigned int op, unsigned int mode,
> >                         uint64_t gfn, void *buffer);
> > /*
> >- * Enables mem_event and returns the mapped ring page indicated by
> >param.
> >+ * Enables mem_event and initializes shared ring to communicate with
> >+ hypervisor
>
> Full stop or and.
>
> >+ * sets ring_page equal to mapped page.
> >+ * Returns 0 if success and if failure returns -errno with errno
> properly set.
> >  * param can be HVM_PARAM_PAGING/ACCESS/SHARING_RING_PFN
> >  */
> >-void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int
> >param,
> >-                          uint32_t *port);
> >+int xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int
> >param,
> >+                        uint32_t *port, void *ring_page,
> >+                        mem_event_back_ring_t *back_ring);
> >
> > #endif /* __XC_PRIVATE_H__ */
> >diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h index
> 1c5d0db..d21f026
> >100644
> >--- a/tools/libxc/xenctrl.h
> >+++ b/tools/libxc/xenctrl.h
> >@@ -47,6 +47,7 @@
> > #include <xen/xsm/flask_op.h>
> > #include <xen/tmem.h>
> > #include <xen/kexec.h>
> >+#include <xen/mem_event.h>
> >
> > #include "xentoollog.h"
> >
> >@@ -2258,11 +2259,13 @@ int xc_mem_paging_load(xc_interface *xch,
> >domid_t domain_id,
> >  */
> >
> > /*
> >- * Enables mem_access and returns the mapped ring page.
> >- * Will return NULL on error.
> >+ * Enables mem_access and sets arg ring page equal to mapped page.
>
> ring_page in the above line. I would leave arg out like you did in the
> previous comment.
>
> >+ * Will return 0 on success and -errno on error.
> >  * Caller has to unmap this page when done.
> >  */
> >-void *xc_mem_access_enable(xc_interface *xch, domid_t domain_id,
> >uint32_t *port);
> >+int xc_mem_access_enable(xc_interface *xch, domid_t domain_id,
> >+                         uint32_t *port, void *ring_page,
> >+                         mem_event_back_ring_t *back_ring);
> > int xc_mem_access_disable(xc_interface *xch, domid_t domain_id);  int
> >xc_mem_access_resume(xc_interface *xch, domid_t domain_id);
>
> Thanks,
> Aravindh
>
>

[-- Attachment #1.2: Type: text/html, Size: 6577 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v0 1/3] mem_access: modifications to mem_event enable API.

[Aravindh Puthiyaparambil (aravindp)]

[-- Attachment #1.1: Type: text/plain, Size: 585 bytes --]

Thank you for taking time to review the patches, I have updated the patches according to your comments.

Not a problem. Glad to be of some help.

I actually looked in the code after your comment and thought that since returning -1 was present mostly so I have changed the functions to return -1 with errno properly set, Hope this is fine.
Please have a look at the v2 of patch series.

In future while sending out newer patches in the series, please don’t skip patch version numbers. This is just to reduce confusion among the reviewers and maintainers.

Thanks,
Aravindh

[-- Attachment #1.2: Type: text/html, Size: 3413 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v0 1/3] mem_access: modifications to mem_event enable API.

[Ian Campbell]

On Fri, 2014-08-22 at 22:05 +0000, Aravindh Puthiyaparambil (aravindp)
wrote:
> >4. The API xc_mem_event_enable is now modified to return int rather than
> >void *,
> >   this was done to synchronize this API's behaviour with other mem_event
> >API's.
> 
> FWIW, since I am the one that introduced this... I am fine with the
> change. Though I did think that the norm was to return -1 on error and
> set errno to the appropriate value.

Sadly there isn't really a norm in libxc now, we use basically every
possible way of returning errors.

In so far as we are moving in any particular direction to try and
rationalise this I think we are trying to get to set errno and return -1
(like a normal POSIXy library)

Ian.

[PATCH v0 2/3] mem_event: Added new helper API to teardown mem event setup and unmap ring_page.

[Dushyant Behl]

tools/libxc/xc_mem_event.c: Added new generic API to teardown mem event setup,
the API supports hvm params PAGING, ACCESS and SHARING and also completes the
obvious job of unmapping ring_page.

tools/libxc/xc_mem_access.c: Modified mem_event_disable to use the new teardown API.

tools/tests/xen-access/: Updated code to use the new API's.

Signed-off-by: Dushyant Behl <myselfdushyantbehl@gmail.com>
---
 tools/libxc/xc_mem_access.c         |  9 +++---
 tools/libxc/xc_mem_event.c          | 59 +++++++++++++++++++++++++++++++++++++
 tools/libxc/xc_private.h            |  8 +++++
 tools/libxc/xenctrl.h               |  3 +-
 tools/tests/xen-access/xen-access.c |  6 ++--
 5 files changed, 74 insertions(+), 11 deletions(-)

diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
index 89050be..ea24689 100644
--- a/tools/libxc/xc_mem_access.c
+++ b/tools/libxc/xc_mem_access.c
@@ -33,12 +33,11 @@ int xc_mem_access_enable(xc_interface *xch, domid_t domain_id,
                                port, ring_page, back_ring);
 }
 
-int xc_mem_access_disable(xc_interface *xch, domid_t domain_id)
+int xc_mem_access_disable(xc_interface *xch, domid_t domain_id, void *ring_page)
 {
-    return xc_mem_event_control(xch, domain_id,
-                                XEN_DOMCTL_MEM_EVENT_OP_ACCESS_DISABLE,
-                                XEN_DOMCTL_MEM_EVENT_OP_ACCESS,
-                                NULL);
+    return xc_mem_event_teardown(xch, domain_id,
+                                HVM_PARAM_ACCESS_RING_PFN,
+                                ring_page);
 }
 
 int xc_mem_access_resume(xc_interface *xch, domid_t domain_id)
diff --git a/tools/libxc/xc_mem_event.c b/tools/libxc/xc_mem_event.c
index 3525a83..6cd1894 100644
--- a/tools/libxc/xc_mem_event.c
+++ b/tools/libxc/xc_mem_event.c
@@ -196,3 +196,62 @@ int xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
 
     return rc1;
 }
+
+/*
+ * Teardown mem_event
+ * returns 0 on success, if failure returns -errno with errno properly set.
+ * param can be HVM_PARAM_PAGING/ACCESS/SHARING_RING_PFN
+ */
+int xc_mem_event_teardown(xc_interface *xch, domid_t domain_id,
+                          int param, void *ring_page)
+{
+    int rc;
+    unsigned int op, mode;
+
+    switch ( param )
+    {
+        case HVM_PARAM_PAGING_RING_PFN:
+            op = XEN_DOMCTL_MEM_EVENT_OP_PAGING_DISABLE;
+            mode = XEN_DOMCTL_MEM_EVENT_OP_PAGING;
+            break;
+
+        case HVM_PARAM_ACCESS_RING_PFN:
+            op = XEN_DOMCTL_MEM_EVENT_OP_ACCESS_DISABLE;
+            mode = XEN_DOMCTL_MEM_EVENT_OP_ACCESS;
+            break;
+
+        case HVM_PARAM_SHARING_RING_PFN:
+            op = XEN_DOMCTL_MEM_EVENT_OP_SHARING_DISABLE;
+            mode = XEN_DOMCTL_MEM_EVENT_OP_SHARING;
+            break;
+
+        /*
+         * This is for the outside chance that the HVM_PARAM is valid but is invalid
+         * as far as mem_event goes.
+         */
+        default:
+            errno = EINVAL;
+            rc = -1;
+            goto out;
+    }
+
+    /* Remove the ring page. */
+    rc = munmap(ring_page, PAGE_SIZE);
+    if ( rc < 0 )
+        PERROR("Error while disabling paging in xen");
+
+    ring_page = NULL;
+
+    rc = xc_mem_event_control(xch, domain_id, op, mode, NULL);
+    if ( rc != 0 )
+    {
+        PERROR("Failed to disable mem_event\n");
+        goto out;
+    }
+
+  out:
+    if (rc != 0)
+        rc = -errno;
+
+    return rc;
+}
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index cf9b223..7120a08 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -387,4 +387,12 @@ int xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
                         uint32_t *port, void *ring_page,
                         mem_event_back_ring_t *back_ring);
 
+/*
+ * Teardown mem_event
+ * returns 0 on success, if failure returns -errno with errno properly set.
+ * param can be HVM_PARAM_PAGING/ACCESS/SHARING_RING_PFN
+ */
+int xc_mem_event_teardown(xc_interface *xch, domid_t domain_id,
+                          int param, void *ring_page);
+
 #endif /* __XC_PRIVATE_H__ */
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index d21f026..cfd6019 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -2261,12 +2261,11 @@ int xc_mem_paging_load(xc_interface *xch, domid_t domain_id,
 /*
  * Enables mem_access and sets arg ring page equal to mapped page.
  * Will return 0 on success and -errno on error.
- * Caller has to unmap this page when done.
  */
 int xc_mem_access_enable(xc_interface *xch, domid_t domain_id,
                          uint32_t *port, void *ring_page,
                          mem_event_back_ring_t *back_ring);
-int xc_mem_access_disable(xc_interface *xch, domid_t domain_id);
+int xc_mem_access_disable(xc_interface *xch, domid_t domain_id, void *ring_page);
 int xc_mem_access_resume(xc_interface *xch, domid_t domain_id);
 
 /*
diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index 9242f86..a4ef578 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -170,13 +170,11 @@ int xenaccess_teardown(xc_interface *xch, xenaccess_t *xenaccess)
         return 0;
 
     /* Tear down domain xenaccess in Xen */
-    if ( xenaccess->mem_event.ring_page )
-        munmap(xenaccess->mem_event.ring_page, XC_PAGE_SIZE);
-
     if ( mem_access_enable )
     {
         rc = xc_mem_access_disable(xenaccess->xc_handle,
-                                   xenaccess->mem_event.domain_id);
+                                   xenaccess->mem_event.domain_id,
+                                   xenaccess->mem_event.ring_page);
         if ( rc != 0 )
         {
             ERROR("Error tearing down domain xenaccess in xen");
-- 
1.9.1

Re: [PATCH v0 2/3] mem_event: Added new helper API to teardown mem event setup and unmap ring_page.

[Aravindh Puthiyaparambil (aravindp)]

>>diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
>>index 89050be..ea24689 100644
>>--- a/tools/libxc/xc_mem_access.c
>>+++ b/tools/libxc/xc_mem_access.c
>>@@ -33,12 +33,11 @@ int xc_mem_access_enable(xc_interface *xch,
>domid_t
>>domain_id,
>>                                port, ring_page, back_ring);  }
>>
>>-int xc_mem_access_disable(xc_interface *xch, domid_t domain_id)
>>+int xc_mem_access_disable(xc_interface *xch, domid_t domain_id, void
>>*ring_page)
>> {
>>-    return xc_mem_event_control(xch, domain_id,
>>-                                XEN_DOMCTL_MEM_EVENT_OP_ACCESS_DISABLE,
>>-                                XEN_DOMCTL_MEM_EVENT_OP_ACCESS,
>>-                                NULL);
>>+    return xc_mem_event_teardown(xch, domain_id,
>>+                                HVM_PARAM_ACCESS_RING_PFN,
>>+                                ring_page);
>> }
>
>Coding style. Please line up the parameters after the "(".
>
>> int xc_mem_access_resume(xc_interface *xch, domid_t domain_id) diff
>>--git a/tools/libxc/xc_mem_event.c b/tools/libxc/xc_mem_event.c index
>>3525a83..6cd1894 100644
>>--- a/tools/libxc/xc_mem_event.c
>>+++ b/tools/libxc/xc_mem_event.c
>>@@ -196,3 +196,62 @@ int xc_mem_event_enable(xc_interface *xch,
>domid_t
>>domain_id, int param,
>>
>>     return rc1;
>> }
>>+
>>+/*
>>+ * Teardown mem_event
>>+ * returns 0 on success, if failure returns -errno with errno properly set.
>>+ * param can be HVM_PARAM_PAGING/ACCESS/SHARING_RING_PFN
>>+ */
>
>Same comment as in the previous review. I did think that the norm was to
>return -1 on error and set errno to the appropriate value. I guess a maintainer
>could confirm this.
>
>>+int xc_mem_event_teardown(xc_interface *xch, domid_t domain_id,
>>+                          int param, void *ring_page) {
>>+    int rc;
>>+    unsigned int op, mode;
>>+    switch ( param )
>>+    {
>>+        case HVM_PARAM_PAGING_RING_PFN:
>>+            op = XEN_DOMCTL_MEM_EVENT_OP_PAGING_DISABLE;
>>+            mode = XEN_DOMCTL_MEM_EVENT_OP_PAGING;
>>+            break;
>>+
>>+        case HVM_PARAM_ACCESS_RING_PFN:
>>+            op = XEN_DOMCTL_MEM_EVENT_OP_ACCESS_DISABLE;
>>+            mode = XEN_DOMCTL_MEM_EVENT_OP_ACCESS;
>>+            break;
>>+
>>+        case HVM_PARAM_SHARING_RING_PFN:
>>+            op = XEN_DOMCTL_MEM_EVENT_OP_SHARING_DISABLE;
>>+            mode = XEN_DOMCTL_MEM_EVENT_OP_SHARING;
>>+            break;
>>+
>>+        /*
>>+         * This is for the outside chance that the HVM_PARAM is valid
>>but is invalid
>>+         * as far as mem_event goes.
>>+         */
>>+        default:
>>+            errno = EINVAL;
>>+            rc = -1;
>>+            goto out;
>>+    }
>>+
>>+    /* Remove the ring page. */
>>+    rc = munmap(ring_page, PAGE_SIZE);
>>+    if ( rc < 0 )
>>+        PERROR("Error while disabling paging in xen");
>
>You could say error in munmap instead of a generic statement. And if you are
>going to display a error message here you should add one in the error path of
>xc_mem_event_enable() too.
>
>>+    ring_page = NULL;
>>+
>>+    rc = xc_mem_event_control(xch, domain_id, op, mode, NULL);
>>+    if ( rc != 0 )
>>+    {
>>+        PERROR("Failed to disable mem_event\n");
>>+        goto out;
>
>Why do you need a goto here? You could set rc to -errno in default case of the
>"switch" statement and in the above "if" and just have a "return rc" at the out
>label.
>
>>+    }
>>+
>>+  out:
>>+    if (rc != 0)
>
>Coding style.
>
>>+        rc = -errno;
>>+
>>+    return rc;
>>+}
>>diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h index
>>cf9b223..7120a08 100644
>>--- a/tools/libxc/xc_private.h
>>+++ b/tools/libxc/xc_private.h
>>@@ -387,4 +387,12 @@ int xc_mem_event_enable(xc_interface *xch,
>domid_t
>>domain_id, int param,
>>                         uint32_t *port, void *ring_page,
>>                         mem_event_back_ring_t *back_ring);
>>
>>+/*
>>+ * Teardown mem_event
>
>Fullstop.
>
>>+ * returns 0 on success, if failure returns -errno with errno properly set.
>
>Please capitalize the "r".
>
>>+ * param can be HVM_PARAM_PAGING/ACCESS/SHARING_RING_PFN
>>+ */
>>+int xc_mem_event_teardown(xc_interface *xch, domid_t domain_id,
>>+                          int param, void *ring_page);
>>+
>
>Why is this called mem_event_teardown()? Won't mem_event_disable() be
>more appropriate?
>
>Thanks,
>Aravindh

Adding xen-devel back as I dropped it accidently.

[PATCH v0 3/3] xenpaging: updated code to use safer mem_event API's for setup and teardown.

[Dushyant Behl]

tools/libxc/xc_mem_paging.c: updated mem_paging enable and disable API's to use
the mem event enable and disable routines. The mem event API's take care of
security issues mentioned in XSA-99 and also provide more coarse grained behaviour.

tools/xenpaging/xenpaging.c: added calls to the new API's and removed the code
which duplicated the new API behaviour.

Signed-off-by: Dushyant Behl <myselfdushyantbehl@gmail.com>
---
 tools/libxc/xc_mem_paging.c | 34 ++++++++++++++---------------
 tools/libxc/xenctrl.h       | 14 ++++++++++--
 tools/xenpaging/xenpaging.c | 52 ++++++---------------------------------------
 3 files changed, 36 insertions(+), 64 deletions(-)

diff --git a/tools/libxc/xc_mem_paging.c b/tools/libxc/xc_mem_paging.c
index 8aa7d4d..826bdb7 100644
--- a/tools/libxc/xc_mem_paging.c
+++ b/tools/libxc/xc_mem_paging.c
@@ -23,28 +23,28 @@
 
 #include "xc_private.h"
 
-
+/*
+ * Enables mem_paging and sets arg ring page equal to mapped page.
+ * Will return 0 on success and -errno on error.
+ */
 int xc_mem_paging_enable(xc_interface *xch, domid_t domain_id,
-                         uint32_t *port)
+                         uint32_t *port,  void *ring_page,
+                         mem_event_back_ring_t *back_ring)
 {
-    if ( !port )
-    {
-        errno = EINVAL;
-        return -1;
-    }
-        
-    return xc_mem_event_control(xch, domain_id,
-                                XEN_DOMCTL_MEM_EVENT_OP_PAGING_ENABLE,
-                                XEN_DOMCTL_MEM_EVENT_OP_PAGING,
-                                port);
+    return xc_mem_event_enable(xch, domain_id,
+                               HVM_PARAM_PAGING_RING_PFN,
+                               port, ring_page, back_ring);
 }
 
-int xc_mem_paging_disable(xc_interface *xch, domid_t domain_id)
+/*
+ * Disable mem_paging and unmap ring page.
+ * Will return 0 on success and -errno on error.
+ */
+int xc_mem_paging_disable(xc_interface *xch, domid_t domain_id, void *ring_page)
 {
-    return xc_mem_event_control(xch, domain_id,
-                                XEN_DOMCTL_MEM_EVENT_OP_PAGING_DISABLE,
-                                XEN_DOMCTL_MEM_EVENT_OP_PAGING,
-                                NULL);
+    return xc_mem_event_teardown(xch, domain_id,
+                                 HVM_PARAM_ACCESS_RING_PFN,
+                                 ring_page);
 }
 
 int xc_mem_paging_nominate(xc_interface *xch, domid_t domain_id, unsigned long gfn)
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index cfd6019..6acbfa9 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -2244,8 +2244,18 @@ int xc_tmem_restore_extra(xc_interface *xch, int dom, int fd);
  * Hardware-Assisted Paging (i.e. Intel EPT, AMD NPT). Moreover, AMD NPT
  * support is considered experimental.
  */
-int xc_mem_paging_enable(xc_interface *xch, domid_t domain_id, uint32_t *port);
-int xc_mem_paging_disable(xc_interface *xch, domid_t domain_id);
+/*
+ * Enables mem_paging and sets arg ring page equal to mapped page.
+ * returns 0 on success and -errno on error.
+ */
+int xc_mem_paging_enable(xc_interface *xch, domid_t domain_id,
+                         uint32_t *port,  void *ring_page,
+                         mem_event_back_ring_t *back_ring);
+/*
+ * Disables mem_paging and unmaps ring page.
+ * returns 0 on success and -errno on error.
+ */
+int xc_mem_paging_disable(xc_interface *xch, domid_t domain_id, void *ring_page);
 int xc_mem_paging_nominate(xc_interface *xch, domid_t domain_id,
                            unsigned long gfn);
 int xc_mem_paging_evict(xc_interface *xch, domid_t domain_id, unsigned long gfn);
diff --git a/tools/xenpaging/xenpaging.c b/tools/xenpaging/xenpaging.c
index 82c1ee4..4a841bf 100644
--- a/tools/xenpaging/xenpaging.c
+++ b/tools/xenpaging/xenpaging.c
@@ -337,40 +337,12 @@ static struct xenpaging *xenpaging_init(int argc, char *argv[])
         PERROR("Could not bind to xenpaging watch\n");
         goto err;
     }
-
-    /* Map the ring page */
-    xc_get_hvm_param(xch, paging->mem_event.domain_id, 
-                        HVM_PARAM_PAGING_RING_PFN, &ring_pfn);
-    mmap_pfn = ring_pfn;
-    paging->mem_event.ring_page = 
-        xc_map_foreign_batch(xch, paging->mem_event.domain_id, 
-                                PROT_READ | PROT_WRITE, &mmap_pfn, 1);
-    if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
-    {
-        /* Map failed, populate ring page */
-        rc = xc_domain_populate_physmap_exact(paging->xc_handle, 
-                                              paging->mem_event.domain_id,
-                                              1, 0, 0, &ring_pfn);
-        if ( rc != 0 )
-        {
-            PERROR("Failed to populate ring gfn\n");
-            goto err;
-        }
-
-        mmap_pfn = ring_pfn;
-        paging->mem_event.ring_page = 
-            xc_map_foreign_batch(xch, paging->mem_event.domain_id, 
-                                    PROT_READ | PROT_WRITE, &mmap_pfn, 1);
-        if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
-        {
-            PERROR("Could not map the ring page\n");
-            goto err;
-        }
-    }
     
-    /* Initialise Xen */
+    /* Enable mem paging and initialize shared ring to communicate with xen. */
     rc = xc_mem_paging_enable(xch, paging->mem_event.domain_id,
-                             &paging->mem_event.evtchn_port);
+                              &paging->mem_event.evtchn_port,
+                              paging->mem_event.ring_page,
+                              &paging->mem_event.back_ring);
     if ( rc != 0 )
     {
         switch ( errno ) {
@@ -413,17 +385,6 @@ static struct xenpaging *xenpaging_init(int argc, char *argv[])
 
     paging->mem_event.port = rc;
 
-    /* Initialise ring */
-    SHARED_RING_INIT((mem_event_sring_t *)paging->mem_event.ring_page);
-    BACK_RING_INIT(&paging->mem_event.back_ring,
-                   (mem_event_sring_t *)paging->mem_event.ring_page,
-                   PAGE_SIZE);
-
-    /* Now that the ring is set, remove it from the guest's physmap */
-    if ( xc_domain_decrease_reservation_exact(xch, 
-                    paging->mem_event.domain_id, 1, 0, &ring_pfn) )
-        PERROR("Failed to remove ring from guest physmap");
-
     /* Get max_pages from guest if not provided via cmdline */
     if ( !paging->max_pages )
     {
@@ -523,9 +484,10 @@ static void xenpaging_teardown(struct xenpaging *paging)
     xs_unwatch(paging->xs_handle, "@releaseDomain", watch_token);
 
     paging->xc_handle = NULL;
+
     /* Tear down domain paging in Xen */
-    munmap(paging->mem_event.ring_page, PAGE_SIZE);
-    rc = xc_mem_paging_disable(xch, paging->mem_event.domain_id);
+    rc = xc_mem_paging_disable(xch, paging->mem_event.domain_id,
+                               paging->mem_event.ring_page);
     if ( rc != 0 )
     {
         PERROR("Error tearing down domain paging in xen");
-- 
1.9.1