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

["Aravindh Puthiyaparambil (aravindp)" <aravindp@xxxxxxxxx>]

>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

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