Re: [Xen-devel] [PATCH v2 1/3] mem_access: modifications to mem_event enable API.

[Dushyant Behl <myselfdushyantbehl@xxxxxxxxx>]

On Mon, Aug 25, 2014 at 9:15 PM, Andres Lagar Cavilla <andres.lagarcavilla@xxxxxxxxx> wrote:

On Sun, Aug 24, 2014 at 5:26 PM, Dushyant Behl <myselfdushyantbehl@xxxxxxxxx> wrote:

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@xxxxxxxxx>
---
Âtools/libxc/xc_mem_access.c    Â| 8 ++++--

Âtools/libxc/xc_mem_event.c     | 55 +++++++++++++++++++++++++++----------
Â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, 69 insertions(+), 36 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..cdbeca8 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 -1 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,

If the idea is to assign the mapped ring_page here (so it can be munmap-ed later), then this should be void **, shouldn't it?

Yeah i also think the idea is correct to have this void **.Â

Â

+Â Â Â Â Â Â Â Â Â Â Â Â 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 -1;

  Â}

  Â/* 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 -1;

  Â}

  Â/* 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;

Is ring_pfn (or mmap_pfn) needed anymore? The two were needed because foreign_batch would modify in place the passed pfn to signal error conditions. foreign_bulk treats the pfn as read-only, so I think we can get rid of either.

Â

  Â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;

If the convention will be to set errno (Ians?), then set errno here

errno = (err) ? : EINVAL;

or similar

Yes, actually I missed it, errno should be set here.

I'll try to do an arm build with these changes and then send the next version accordingly.

Thanks and Regards,

Dushyant Behl

Â

Â

      Â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;

@@ -165,11 +187,16 @@ void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
    Â}

    Âif ( ring_page )
-Â Â Â Â Â Â munmap(ring_page, XC_PAGE_SIZE);
+Â Â Â Â {
+Â Â Â Â Â Â rc2 = munmap(ring_page, XC_PAGE_SIZE);
+Â Â Â Â Â Â if ( rc2 < 0 )
+Â Â Â Â Â Â Â Â PERROR("Error while munmap of ring_page");
+Â Â Â Â }

    Âring_page = NULL;

    Âerrno = saved_errno;

+Â Â Â Â rc1 = -1;

  Â}

-Â Â return ring_page;
+Â Â return rc1;
Â}

diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index c50a7c9..3d455e7 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
+ * and sets ring_page equal to mapped page.

yeah, ring_page should be void **

Â

+ * Returns 0 if success and if failure returns -1 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..9d043d7 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 -1 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

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