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

[Dushyant Behl <myselfdushyantbehl@xxxxxxxxx>]

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@xxxxxxxxx> 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

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