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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

> >>> +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.
> > I will gladly fix this interface in further patches. By all means!
> 
> What I wish to avoid is 4.6 releasing with the API in this state, as
> adjusting valgrind and strace to compensate will be miserable.

Right.
> 
> The best solution would be to have this patch and the fixups adjacent in
> the series, at which point the valgrind/strace adjustments can start
> with the clean API for 4.6

Yes. I shall post this patchset plus extra patches next week.

> 
> >>> +    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.
> > Ah, will do what Jan asked and just put a big comment. And keep the old
> > name (or maybe add 'DEPRECATED'?)
> 
> If you are going to the effort of renaming, then just delete.  Either
> way will break the build of unsuspecting code, and the latter leaves us
> with less junk.

It will break the tmem hypervisor code as it has a check for
TMEM_CONTROL (which per Jan suggestion should return -E.. something).

> 
> A comment however will be useful in all cases.

Aye.