Re: [PATCH v2 5/8] tmem: Move TMEM_CONTROL subop of tmem hypercall to sysctl.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 27/08/15 12:02, Konrad Rzeszutek Wilk wrote:
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -1808,25 +1808,25 @@ static PyObject *pyxc_tmem_control(XcObject *self,
>                          &pool_id, &subop, &cli_id, &arg1, &arg2, &buf) )
>          return NULL;
>  
> -    if ( (subop == TMEMC_LIST) && (arg1 > 32768) )
> +    if ( (subop == XEN_SYSCTL_TMEM_OP_LIST) && (arg1 > 32768) )
>          arg1 = 32768;
>  
>      if ( (rc = xc_tmem_control(self->xc_handle, pool_id, subop, cli_id, arg1, arg2, buffer)) < 0 )
>          return Py_BuildValue("i", rc);
>  
>      switch (subop) {
> -        case TMEMC_LIST:
> +        case XEN_SYSCTL_TMEM_OP_LIST:
>              return Py_BuildValue("s", buffer);
> -        case TMEMC_FLUSH:
> +        case XEN_SYSCTL_TMEM_OP_FLUSH:
>              return Py_BuildValue("i", rc);
> -        case TMEMC_QUERY_FREEABLE_MB:
> +        case XEN_SYSCTL_TMEM_OP_QUERY_FREEABLE_MB:
>              return Py_BuildValue("i", rc);
> -        case TMEMC_THAW:
> -        case TMEMC_FREEZE:
> -        case TMEMC_DESTROY:
> -        case TMEMC_SET_WEIGHT:
> -        case TMEMC_SET_CAP:
> -        case TMEMC_SET_COMPRESS:
> +        case XEN_SYSCTL_TMEM_OP_THAW:
> +        case XEN_SYSCTL_TMEM_OP_FREEZE:
> +        case XEN_SYSCTL_TMEM_OP_DESTROY:
> +        case XEN_SYSCTL_TMEM_OP_SET_WEIGHT:
> +        case XEN_SYSCTL_TMEM_OP_SET_CAP:
> +        case XEN_SYSCTL_TMEM_OP_SET_COMPRESS:

Any case statements falling through into default like this can simply be
dropped.

> @@ -2555,77 +2556,72 @@ static int tmemc_restore_flush_page(int cli_id, uint32_t pool_id, struct oid *oi
>      return do_tmem_flush_page(pool,oidp,index);
>  }
>  
> -static int do_tmem_control(struct tmem_op *op)
> +int tmem_control(struct xen_sysctl_tmem_op *op)
>  {
>      int ret;
>      uint32_t pool_id = op->pool_id;
> -    uint32_t subop = op->u.ctrl.subop;
> -    struct oid *oidp = (struct oid *)(&op->u.ctrl.oid[0]);
> +    uint32_t cmd = op->cmd;
> +    struct oid *oidp = (struct oid *)(&op->oid[0]);

Please put a struct xen_sysctl_tmem_oid definition in sysctl.h and avoid
this typesystem abuse.

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -710,6 +710,48 @@ struct xen_sysctl_psr_cat_op {
>  typedef struct xen_sysctl_psr_cat_op xen_sysctl_psr_cat_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_psr_cat_op_t);
>  
> +#define XEN_SYSCTL_TMEM_OP_ALL_CLIENTS 0xFFFFU
> +
> +#define XEN_SYSCTL_TMEM_OP_THAW                   0
> +#define XEN_SYSCTL_TMEM_OP_FREEZE                 1
> +#define XEN_SYSCTL_TMEM_OP_FLUSH                  2
> +#define XEN_SYSCTL_TMEM_OP_DESTROY                3
> +#define XEN_SYSCTL_TMEM_OP_LIST                   4
> +#define XEN_SYSCTL_TMEM_OP_SET_WEIGHT             5
> +#define XEN_SYSCTL_TMEM_OP_SET_CAP                6
> +#define XEN_SYSCTL_TMEM_OP_SET_COMPRESS           7
> +#define XEN_SYSCTL_TMEM_OP_QUERY_FREEABLE_MB      8
> +#define XEN_SYSCTL_TMEM_OP_SAVE_BEGIN             10
> +#define XEN_SYSCTL_TMEM_OP_SAVE_GET_VERSION       11
> +#define XEN_SYSCTL_TMEM_OP_SAVE_GET_MAXPOOLS      12
> +#define XEN_SYSCTL_TMEM_OP_SAVE_GET_CLIENT_WEIGHT 13
> +#define XEN_SYSCTL_TMEM_OP_SAVE_GET_CLIENT_CAP    14
> +#define XEN_SYSCTL_TMEM_OP_SAVE_GET_CLIENT_FLAGS  15
> +#define XEN_SYSCTL_TMEM_OP_SAVE_GET_POOL_FLAGS    16
> +#define XEN_SYSCTL_TMEM_OP_SAVE_GET_POOL_NPAGES   17
> +#define XEN_SYSCTL_TMEM_OP_SAVE_GET_POOL_UUID     18
> +#define XEN_SYSCTL_TMEM_OP_SAVE_GET_NEXT_PAGE     19
> +#define XEN_SYSCTL_TMEM_OP_SAVE_GET_NEXT_INV      20
> +#define XEN_SYSCTL_TMEM_OP_SAVE_END               21
> +#define XEN_SYSCTL_TMEM_OP_RESTORE_BEGIN          30
> +#define XEN_SYSCTL_TMEM_OP_RESTORE_PUT_PAGE       32
> +#define XEN_SYSCTL_TMEM_OP_RESTORE_FLUSH_PAGE     33
> +
> +struct xen_sysctl_tmem_op {
> +    uint32_t cmd;       /* IN: XEN_SYSCTL_TMEM_OP_* . */
> +    int32_t pool_id;    /* IN: 0 by default unless _SAVE_*, RESTORE_* .*/
> +    uint32_t cli_id;    /* IN: client id, 0 for XEN_SYSCTL_TMEM_QUERY_FREEABLE_MB
> +                           for all others can be the domain id or
> +                           XEN_SYSCTL_TMEM_OP_ALL_CLIENTS for all. */
> +    uint32_t arg1;      /* IN: If not applicable to command use 0. */
> +    uint32_t arg2;      /* IN: If not applicable to command use 0. */

Please can this interface be fixed as part of the move, even if it is in
subsequent patches for clarity.

Part of the issue with the XSA-17 security audit was that these args are
completely opaque.

> +    uint8_t  pad[4];    /* Padding so structure is the same under 32 and 64. */

probably better to use a uint32_t here.

> +    uint64_t oid[3];    /* IN: If not applicable to command use 0. */
> +    XEN_GUEST_HANDLE_64(char) buf; /* IN/OUT: Buffer to save and restore ops. */
> +};
> +typedef struct xen_sysctl_tmem_op xen_sysctl_tmem_op_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_tmem_op_t);
> +
>  struct xen_sysctl {
>      uint32_t cmd;
>  #define XEN_SYSCTL_readconsole                    1
> @@ -734,6 +776,7 @@ struct xen_sysctl {
>  #define XEN_SYSCTL_psr_cmt_op                    21
>  #define XEN_SYSCTL_pcitopoinfo                   22
>  #define XEN_SYSCTL_psr_cat_op                    23
> +#define XEN_SYSCTL_tmem_op                       24
>      uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
>      union {
>          struct xen_sysctl_readconsole       readconsole;
> @@ -758,6 +801,7 @@ struct xen_sysctl {
>          struct xen_sysctl_coverage_op       coverage_op;
>          struct xen_sysctl_psr_cmt_op        psr_cmt_op;
>          struct xen_sysctl_psr_cat_op        psr_cat_op;
> +        struct xen_sysctl_tmem_op           tmem_op;
>          uint8_t                             pad[128];
>      } u;
>  };
> diff --git a/xen/include/public/tmem.h b/xen/include/public/tmem.h
> index 4fd2fc6..208f5a6 100644
> --- a/xen/include/public/tmem.h
> +++ b/xen/include/public/tmem.h
> @@ -33,7 +33,7 @@
>  #define TMEM_SPEC_VERSION          1
>  
>  /* Commands to HYPERVISOR_tmem_op() */
> -#define TMEM_CONTROL               0
> +#define TMEM_CONTROL_MOVED         0

If anything, this should be deleted as opposed to being renamed.  The
_MOVED suffix is confusing, as TMEM_CONTROL_MOVED sounds like it could
be a plausible TMEM operation.

>  #define TMEM_NEW_POOL              1
>  #define TMEM_DESTROY_POOL          2
>  #define TMEM_PUT_PAGE              4
> @@ -48,35 +48,9 @@
>  #endif
>  
>  /* Privileged commands to HYPERVISOR_tmem_op() */
> -#define TMEM_AUTH                 101 
> +#define TMEM_AUTH                 101
>  #define TMEM_RESTORE_NEW          102
>  
> -/* Subops for HYPERVISOR_tmem_op(TMEM_CONTROL) */
> -#define TMEMC_THAW                   0
> -#define TMEMC_FREEZE                 1
> -#define TMEMC_FLUSH                  2
> -#define TMEMC_DESTROY                3
> -#define TMEMC_LIST                   4
> -#define TMEMC_SET_WEIGHT             5
> -#define TMEMC_SET_CAP                6
> -#define TMEMC_SET_COMPRESS           7
> -#define TMEMC_QUERY_FREEABLE_MB      8
> -#define TMEMC_SAVE_BEGIN             10
> -#define TMEMC_SAVE_GET_VERSION       11
> -#define TMEMC_SAVE_GET_MAXPOOLS      12
> -#define TMEMC_SAVE_GET_CLIENT_WEIGHT 13
> -#define TMEMC_SAVE_GET_CLIENT_CAP    14
> -#define TMEMC_SAVE_GET_CLIENT_FLAGS  15
> -#define TMEMC_SAVE_GET_POOL_FLAGS    16
> -#define TMEMC_SAVE_GET_POOL_NPAGES   17
> -#define TMEMC_SAVE_GET_POOL_UUID     18
> -#define TMEMC_SAVE_GET_NEXT_PAGE     19
> -#define TMEMC_SAVE_GET_NEXT_INV      20
> -#define TMEMC_SAVE_END               21
> -#define TMEMC_RESTORE_BEGIN          30
> -#define TMEMC_RESTORE_PUT_PAGE       32
> -#define TMEMC_RESTORE_FLUSH_PAGE     33
> -
>  /* Bits for HYPERVISOR_tmem_op(TMEM_NEW_POOL) */
>  #define TMEM_POOL_PERSIST          1
>  #define TMEM_POOL_SHARED           2
> @@ -110,16 +84,7 @@ struct tmem_op {
>              uint32_t flags;
>              uint32_t arg1;
>          } creat; /* for cmd == TMEM_NEW_POOL, TMEM_AUTH, TMEM_RESTORE_NEW */
> -        struct { 
> -            uint32_t subop;
> -            uint32_t cli_id;
> -            uint32_t arg1;
> -            uint32_t arg2;
> -            uint64_t oid[3];
> -            tmem_cli_va_t buf;
> -        } ctrl; /* for cmd == TMEM_CONTROL */
>          struct {
> -            
>              uint64_t oid[3];
>              uint32_t index;
>              uint32_t tmem_offset;
> diff --git a/xen/include/xen/tmem.h b/xen/include/xen/tmem.h
> index 5dbf9d5..32a542a 100644
> --- a/xen/include/xen/tmem.h
> +++ b/xen/include/xen/tmem.h
> @@ -9,6 +9,9 @@
>  #ifndef __XEN_TMEM_H__
>  #define __XEN_TMEM_H__
>  
> +struct xen_sysctl_tmem_op;

#include<public/sysctl.h> please.

~Andrew