Re: [Xen-devel] [PATCH 1 of 7] Tools: Sanitize mem_event/access/paging interfaces

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-02-23 at 06:05 +0000, Andres Lagar-Cavilla wrote:
> tools/libxc/xc_mem_access.c         |  10 ++++++++--
>  tools/libxc/xc_mem_event.c          |  13 ++++++++-----
>  tools/libxc/xc_mem_paging.c         |  10 ++++++++--
>  tools/libxc/xenctrl.h               |   6 +++---
>  tools/tests/xen-access/xen-access.c |  22 ++++------------------
>  tools/xenpaging/xenpaging.c         |  18 +++---------------
>  tools/xenpaging/xenpaging.h         |   2 +-
>  xen/arch/x86/mm/mem_event.c         |  33 +++------------------------------
>  xen/include/public/domctl.h         |   4 ++--
>  xen/include/public/mem_event.h      |   4 ----
>  xen/include/xen/sched.h             |   2 --
>  11 files changed, 40 insertions(+), 84 deletions(-)
> 
> 
> Don't use the superfluous shared page, return the event channel directly as
> part of the domctl struct, instead.
> 
> In-tree consumers (xenpaging, xen-access) updated. This is an ABI/API change,
> so please voice any concerns.
> 
> Known pending issues:
> - pager could die and its ring page could be used by some other process, yet
>   Xen retains the mapping to it.
> - use a saner interface for the paging_load buffer.
> 
> This change also affects the x86/mm bits in the hypervisor that process the
> mem_event setup domctl.
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
> 
> diff -r 8ddd13cc783e -r 3b24188d8815 tools/libxc/xc_mem_access.c
> --- a/tools/libxc/xc_mem_access.c
> +++ b/tools/libxc/xc_mem_access.c
> @@ -25,12 +25,18 @@
> 
> 
>  int xc_mem_access_enable(xc_interface *xch, domid_t domain_id,
> -                        void *shared_page, void *ring_page)
> +                         uint32_t *port, void *ring_page)
>  {
> +    if ( !port )
> +    {
> +        errno = -EINVAL;

Aren't errno vals normally +ve?

Is there any situation where port==NULL would be valid, i.e. any chance
that this might happen, or are such callers just buggy?

> +        return -1;
> +    }

> +
>      return xc_mem_event_control(xch, domain_id,
>                                  XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE,
>                                  XEN_DOMCTL_MEM_EVENT_OP_ACCESS,
> -                                shared_page, ring_page);
> +                                port, ring_page);
>  }
> 
>  int xc_mem_access_disable(xc_interface *xch, domid_t domain_id)
> diff -r 8ddd13cc783e -r 3b24188d8815 tools/libxc/xc_mem_event.c
> --- a/tools/libxc/xc_mem_event.c
> +++ b/tools/libxc/xc_mem_event.c
> @@ -24,19 +24,22 @@
>  #include "xc_private.h"
> 
>  int xc_mem_event_control(xc_interface *xch, domid_t domain_id, unsigned int 
> op,
> -                         unsigned int mode, void *page, void *ring_page)
> +                         unsigned int mode, uint32_t *port, void *ring_page)
>  {
>      DECLARE_DOMCTL;
> +    int rc;
> 
>      domctl.cmd = XEN_DOMCTL_mem_event_op;
>      domctl.domain = domain_id;
>      domctl.u.mem_event_op.op = op;
>      domctl.u.mem_event_op.mode = mode;
> -
> -    domctl.u.mem_event_op.shared_addr = (unsigned long)page;
> -    domctl.u.mem_event_op.ring_addr = (unsigned long)ring_page;
> +    domctl.u.mem_event_op.ring_addr = (unsigned long) ring_page;
> 
> -    return do_domctl(xch, &domctl);
> +    errno = 0;
> +    rc = do_domctl(xch, &domctl);
> +    if ( !rc && port )
> +        *port = domctl.u.mem_event_op.port;
> +    return rc;

Clearing errno is an unusual pattern, normally callers only expect errno
to contain a valid value on failure. Are you trying to provide some
backwards compatibility with something or is there another reason for
doing it this way?

>  }
> 
>  int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
> diff -r 8ddd13cc783e -r 3b24188d8815 tools/libxc/xc_mem_paging.c
> --- a/tools/libxc/xc_mem_paging.c
> +++ b/tools/libxc/xc_mem_paging.c
> @@ -25,12 +25,18 @@
> 
> 
>  int xc_mem_paging_enable(xc_interface *xch, domid_t domain_id,
> -                        void *shared_page, void *ring_page)
> +                         uint32_t *port, void *ring_page)
>  {
> +    if ( !port )
> +    {
> +        errno = -EINVAL;

Same comments as before.

> +        return -1;
> +    }
> +
>      return xc_mem_event_control(xch, domain_id,
>                                  XEN_DOMCTL_MEM_EVENT_OP_PAGING_ENABLE,
>                                  XEN_DOMCTL_MEM_EVENT_OP_PAGING,
> -                                shared_page, ring_page);
> +                                port, ring_page);
>  }
> 

Ian.



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