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 19:43, Konrad Rzeszutek Wilk wrote:
>
>>> --- 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
>> Perhaps better to have all these in tmem.h, to not clutter this
>> header?
> Yes and no. The other sysctl had their #defines for commands here so I figured I
> would follow that rule. But I am OK keeping it in tmem.h and just do
>
> /* For the 'cmd' values consule tmem.h. Look for XEN_SYSCTL_TMEM_OP_* */

Better to stay consistent with sysctl.h.   Separating the structure and
the subops is unhelpful to people reading the code.

>>> +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. */
>>> +    uint8_t  pad[4];    /* Padding so structure is the same under 32 and 64. */
>> uint32_t please. And despite this being an (easily changeable) sysctl,
>> verifying that it's zero on input would be nice.
> Yes! That was what I forgotten!
>>> --- 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
>> Perhaps say where it moved in a brief comment?
> Yes, good idea.
>
> /* Deprecated. These operations are done now via XEN_SYSCTL_tmem_op
>  * using the sysctl hypercall. */

I hope "XEN_SYSCTL_tmem_op" is sufficient to imply "using the sysctl
hypercall" ;)

>
>>> --- a/xen/xsm/flask/hooks.c
>>> +++ b/xen/xsm/flask/hooks.c
>>> @@ -761,6 +761,9 @@ static int flask_sysctl(int cmd)
>>>      case XEN_SYSCTL_tbuf_op:
>>>          return domain_has_xen(current->domain, XEN__TBUFCONTROL);
>>>  
>>> +    case XEN_SYSCTL_tmem_op:
>>> +        return domain_has_xen(current->domain, XEN__TMEM_CONTROL);
>>> +
>>>      case XEN_SYSCTL_sched_id:
>>>          return domain_has_xen(current->domain, XEN__GETSCHEDULER);
>> Hmm, these cases appear to be roughly sorted numerically, i.e.
>> yours would normally go at the end.
> I recall a comment from Andrew asking the newly introduced commands to
> be in alphabetical order. But perhaps that was for domctl which is more
> .. ah.. random?

Really? that doesn't sound like me.  Apologies if it was.  (Alphabetic
for obj-y in a Makefile perhaps?)

Numeric would be far better here.

~Andrew