|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 5/8] tmem: Move TMEM_CONTROL subop of tmem hypercall to sysctl.
On Thu, Aug 27, 2015 at 02:12:35PM +0100, Andrew Cooper wrote:
> 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.
Lets do that in another patch. This patch is just to move the operation
from one hypercall to another - with the minimum amount of changes.
I will gladly modify the case statements in subsequent patches.
>
> > @@ -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.
Again, I tried to make this move as minimal as possible and not
introduce other sensible changes - and defer them to later patches.
>
> > --- 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.
I will gladly fix this interface in further patches. By all means!
>
> 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.
Yes.
>
> > + 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'?)
>
> > #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.
I tried that and it blew up with tons of other dependencies. This makes
it much simpler without adding extra #include.
>
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |