| Message ID | 1475055746-22401-9-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 28, 2016 at 05:42:22AM -0400, Konrad Rzeszutek Wilk wrote: > the struct tmem_op. > > No functional change. But it makes the code so much easier > to read. > > Note: We still have to do this awkward 'guest_handle_cast' > otherwise it will not compile on ARM - which defines _two_ > of these macros (__guest_handle_64_xen_sysctl_tmem_client_t > and __guest_handle_xen_sysctl_tmem_client_t). And if cast is > not used then a compile error comes up as we use the wrong one. > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > > v1: First submission. > --- > tools/libxc/xc_tmem.c | 4 +-- Acked-by: Wei Liu <wei.liu2@citrix.com>
>>> On 28.09.16 at 11:42, <konrad.wilk@oracle.com> wrote: > Note: We still have to do this awkward 'guest_handle_cast' > otherwise it will not compile on ARM - which defines _two_ > of these macros (__guest_handle_64_xen_sysctl_tmem_client_t > and __guest_handle_xen_sysctl_tmem_client_t). And if cast is > not used then a compile error comes up as we use the wrong one. This seems suspicious, but it's hard to judge without knowing what exactly the errors were. > --- a/xen/common/tmem_control.c > +++ b/xen/common/tmem_control.c > @@ -258,7 +258,7 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len, > return 0; > } > > -static int __tmemc_set_var(struct client *client, uint32_t subop, > +static int __tmemc_set_var(struct client *client, > XEN_GUEST_HANDLE_PARAM(xen_sysctl_tmem_client_t) buf) > { > domid_t cli_id = client->cli_id; > @@ -267,11 +267,6 @@ static int __tmemc_set_var(struct client *client, uint32_t subop, > > ASSERT(client); > > - if ( subop != XEN_SYSCTL_TMEM_OP_SET_CLIENT_INFO ) > - { > - tmem_client_warn("tmem: unknown subop %d for tmemc_set_var\n", subop); > - return -1; > - } > if ( copy_from_guest(&info, buf, 1) ) > return -EFAULT; The adjustments above look pretty unrelated to the purpose of the patch, but well - you're the maintainer of this code. Jan
On Wed, Sep 28, 2016 at 06:56:40AM -0600, Jan Beulich wrote: > >>> On 28.09.16 at 11:42, <konrad.wilk@oracle.com> wrote: > > Note: We still have to do this awkward 'guest_handle_cast' > > otherwise it will not compile on ARM - which defines _two_ > > of these macros (__guest_handle_64_xen_sysctl_tmem_client_t > > and __guest_handle_xen_sysctl_tmem_client_t). And if cast is > > not used then a compile error comes up as we use the wrong one. > > This seems suspicious, but it's hard to judge without knowing what > exactly the errors were. tmem_control.c: In function ‘tmem_control’: tmem_control.c:426:9: error: incompatible type for argument 2 of ‘tmemc_set_client_info’ ret = tmemc_set_client_info(op->cli_id, op->u.client); ^ tmem_control.c:302:12: note: expected ‘__guest_handle_xen_sysctl_tmem_client_t’ but argument is of type ‘__guest_handle_64_xen_sysctl_tmem_client_t’ static int tmemc_set_client_info(domid_t cli_id, ^ tmem_control.c:432:9: error: incompatible type for argument 2 of ‘tmemc_get_client_info’ ret = tmemc_get_client_info(op->cli_id, op->u.client); ^ > > > --- a/xen/common/tmem_control.c > > +++ b/xen/common/tmem_control.c > > @@ -258,7 +258,7 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len, > > return 0; > > } > > > > -static int __tmemc_set_var(struct client *client, uint32_t subop, > > +static int __tmemc_set_var(struct client *client, > > XEN_GUEST_HANDLE_PARAM(xen_sysctl_tmem_client_t) buf) > > { > > domid_t cli_id = client->cli_id; > > @@ -267,11 +267,6 @@ static int __tmemc_set_var(struct client *client, uint32_t subop, > > > > ASSERT(client); > > > > - if ( subop != XEN_SYSCTL_TMEM_OP_SET_CLIENT_INFO ) > > - { > > - tmem_client_warn("tmem: unknown subop %d for tmemc_set_var\n", subop); > > - return -1; > > - } > > if ( copy_from_guest(&info, buf, 1) ) > > return -EFAULT; > > The adjustments above look pretty unrelated to the purpose of the > patch, but well - you're the maintainer of this code. <smacks himself in the head> That is indeed wrong. This should have been done in the previous patch: "tmem/libxc: Squash XEN_SYSCTL_TMEM_OP_[SET|SAVE].." > > Jan >
>>> On 30.09.16 at 16:36, <konrad@kernel.org> wrote: > On Wed, Sep 28, 2016 at 06:56:40AM -0600, Jan Beulich wrote: >> >>> On 28.09.16 at 11:42, <konrad.wilk@oracle.com> wrote: >> > Note: We still have to do this awkward 'guest_handle_cast' >> > otherwise it will not compile on ARM - which defines _two_ >> > of these macros (__guest_handle_64_xen_sysctl_tmem_client_t >> > and __guest_handle_xen_sysctl_tmem_client_t). And if cast is >> > not used then a compile error comes up as we use the wrong one. >> >> This seems suspicious, but it's hard to judge without knowing what >> exactly the errors were. > > tmem_control.c: In function ‘tmem_control’: > tmem_control.c:426:9: error: incompatible type for argument 2 of > ‘tmemc_set_client_info’ > ret = tmemc_set_client_info(op->cli_id, op->u.client); > ^ > tmem_control.c:302:12: note: expected > ‘__guest_handle_xen_sysctl_tmem_client_t’ but argument is of type > ‘__guest_handle_64_xen_sysctl_tmem_client_t’ > static int tmemc_set_client_info(domid_t cli_id, > ^ Looks like you want to pass around a 64-bit handle then? >> > --- a/xen/common/tmem_control.c >> > +++ b/xen/common/tmem_control.c >> > @@ -258,7 +258,7 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len, >> > return 0; >> > } >> > >> > -static int __tmemc_set_var(struct client *client, uint32_t subop, >> > +static int __tmemc_set_var(struct client *client, >> > XEN_GUEST_HANDLE_PARAM(xen_sysctl_tmem_client_t) buf) >> > { >> > domid_t cli_id = client->cli_id; >> > @@ -267,11 +267,6 @@ static int __tmemc_set_var(struct client *client, uint32_t subop, >> > >> > ASSERT(client); >> > >> > - if ( subop != XEN_SYSCTL_TMEM_OP_SET_CLIENT_INFO ) >> > - { >> > - tmem_client_warn("tmem: unknown subop %d for tmemc_set_var\n", subop); >> > - return -1; >> > - } >> > if ( copy_from_guest(&info, buf, 1) ) >> > return -EFAULT; >> >> The adjustments above look pretty unrelated to the purpose of the >> patch, but well - you're the maintainer of this code. > > <smacks himself in the head> Don't be that harsh to yourself. Jan
On Fri, Sep 30, 2016 at 08:56:03AM -0600, Jan Beulich wrote: > >>> On 30.09.16 at 16:36, <konrad@kernel.org> wrote: > > On Wed, Sep 28, 2016 at 06:56:40AM -0600, Jan Beulich wrote: > >> >>> On 28.09.16 at 11:42, <konrad.wilk@oracle.com> wrote: > >> > Note: We still have to do this awkward 'guest_handle_cast' > >> > otherwise it will not compile on ARM - which defines _two_ > >> > of these macros (__guest_handle_64_xen_sysctl_tmem_client_t > >> > and __guest_handle_xen_sysctl_tmem_client_t). And if cast is > >> > not used then a compile error comes up as we use the wrong one. > >> > >> This seems suspicious, but it's hard to judge without knowing what > >> exactly the errors were. > > > > tmem_control.c: In function ‘tmem_control’: > > tmem_control.c:426:9: error: incompatible type for argument 2 of > > ‘tmemc_set_client_info’ > > ret = tmemc_set_client_info(op->cli_id, op->u.client); > > ^ > > tmem_control.c:302:12: note: expected > > ‘__guest_handle_xen_sysctl_tmem_client_t’ but argument is of type > > ‘__guest_handle_64_xen_sysctl_tmem_client_t’ > > static int tmemc_set_client_info(domid_t cli_id, > > ^ > > Looks like you want to pass around a 64-bit handle then? <nods> Which I found is XEN_GUEST_HANDLE, not XEN_GUEST_HANDLE_PARAM. .. snip.. > >> > if ( copy_from_guest(&info, buf, 1) ) > >> > return -EFAULT; > >> > >> The adjustments above look pretty unrelated to the purpose of the > >> patch, but well - you're the maintainer of this code. > > > > <smacks himself in the head> > > Don't be that harsh to yourself. :-) > > Jan
diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c index 74d2d85..cb0dfa1 100644 --- a/tools/libxc/xc_tmem.c +++ b/tools/libxc/xc_tmem.c @@ -87,7 +87,7 @@ int xc_tmem_control(xc_interface *xch, } } - set_xen_guest_handle(sysctl.u.tmem_op.buf, buf); + set_xen_guest_handle(sysctl.u.tmem_op.u.buf, buf); rc = do_sysctl(xch, &sysctl); @@ -133,7 +133,7 @@ int xc_tmem_control_oid(xc_interface *xch, } } - set_xen_guest_handle(sysctl.u.tmem_op.buf, buf); + set_xen_guest_handle(sysctl.u.tmem_op.u.buf, buf); rc = do_sysctl(xch, &sysctl); diff --git a/xen/common/tmem.c b/xen/common/tmem.c index 27164ce..ab354b6 100644 --- a/xen/common/tmem.c +++ b/xen/common/tmem.c @@ -1843,19 +1843,19 @@ int do_tmem_control(struct xen_sysctl_tmem_op *op) case XEN_SYSCTL_TMEM_OP_RESTORE_BEGIN: case XEN_SYSCTL_TMEM_OP_SAVE_END: ret = tmemc_save_subop(op->cli_id, pool_id, cmd, - guest_handle_cast(op->buf, char), op->arg1); + guest_handle_cast(op->u.buf, char), op->arg1); break; case XEN_SYSCTL_TMEM_OP_SAVE_GET_NEXT_PAGE: ret = tmemc_save_get_next_page(op->cli_id, pool_id, - guest_handle_cast(op->buf, char), op->arg1); + guest_handle_cast(op->u.buf, char), op->arg1); break; case XEN_SYSCTL_TMEM_OP_SAVE_GET_NEXT_INV: ret = tmemc_save_get_next_inv(op->cli_id, - guest_handle_cast(op->buf, char), op->arg1); + guest_handle_cast(op->u.buf, char), op->arg1); break; case XEN_SYSCTL_TMEM_OP_RESTORE_PUT_PAGE: ret = tmemc_restore_put_page(op->cli_id, pool_id, oidp, op->arg2, - guest_handle_cast(op->buf, char), op->arg1); + guest_handle_cast(op->u.buf, char), op->arg1); break; case XEN_SYSCTL_TMEM_OP_RESTORE_FLUSH_PAGE: ret = tmemc_restore_flush_page(op->cli_id, pool_id, oidp, op->arg2); diff --git a/xen/common/tmem_control.c b/xen/common/tmem_control.c index 8bb9d48..942d4c9 100644 --- a/xen/common/tmem_control.c +++ b/xen/common/tmem_control.c @@ -258,7 +258,7 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len, return 0; } -static int __tmemc_set_var(struct client *client, uint32_t subop, +static int __tmemc_set_var(struct client *client, XEN_GUEST_HANDLE_PARAM(xen_sysctl_tmem_client_t) buf) { domid_t cli_id = client->cli_id; @@ -267,11 +267,6 @@ static int __tmemc_set_var(struct client *client, uint32_t subop, ASSERT(client); - if ( subop != XEN_SYSCTL_TMEM_OP_SET_CLIENT_INFO ) - { - tmem_client_warn("tmem: unknown subop %d for tmemc_set_var\n", subop); - return -1; - } if ( copy_from_guest(&info, buf, 1) ) return -EFAULT; @@ -296,20 +291,17 @@ static int __tmemc_set_var(struct client *client, uint32_t subop, return 0; } -static int tmemc_set_var(domid_t cli_id, uint32_t subop, - XEN_GUEST_HANDLE_PARAM(void) buf) +static int tmemc_set_client_info(domid_t cli_id, + XEN_GUEST_HANDLE_PARAM(xen_sysctl_tmem_client_t) info) { struct client *client; int ret = -ENOENT; - XEN_GUEST_HANDLE_PARAM(xen_sysctl_tmem_client_t) info; - - info = guest_handle_cast(buf, xen_sysctl_tmem_client_t); if ( cli_id == TMEM_CLI_ID_NULL ) { list_for_each_entry(client,&tmem_global.client_list,client_list) { - ret = __tmemc_set_var(client, subop, info); + ret = __tmemc_set_var(client, info); if (ret) break; } @@ -318,11 +310,32 @@ static int tmemc_set_var(domid_t cli_id, uint32_t subop, { client = tmem_client_from_cli_id(cli_id); if ( client ) - ret = __tmemc_set_var(client, subop, info); + ret = __tmemc_set_var(client, info); } return ret; } +static int tmemc_get_client_info(int cli_id, + XEN_GUEST_HANDLE_PARAM(xen_sysctl_tmem_client_t) info) +{ + struct client *client = tmem_client_from_cli_id(cli_id); + + if ( client ) + { + if ( copy_to_guest(info, &client->info, 1) ) + return -EFAULT; + } + else + { + xen_sysctl_tmem_client_t generic = { .version = TMEM_SPEC_VERSION, + .maxpools = MAX_POOLS_PER_DOMAIN }; + if ( copy_to_guest(info, &generic, 1) ) + return -EFAULT; + } + + return 0; +} + static int tmemc_save_subop(int cli_id, uint32_t pool_id, uint32_t subop, XEN_GUEST_HANDLE_PARAM(void) buf, uint32_t arg1) { @@ -330,27 +343,9 @@ static int tmemc_save_subop(int cli_id, uint32_t pool_id, uint32_t subop, struct tmem_pool *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN) ? NULL : client->pools[pool_id]; int rc = -1; - XEN_GUEST_HANDLE_PARAM(xen_sysctl_tmem_client_t) info; switch(subop) { - case XEN_SYSCTL_TMEM_OP_GET_CLIENT_INFO: - rc = 0; - info = guest_handle_cast(buf, xen_sysctl_tmem_client_t); - if ( client ) - { - if ( copy_to_guest(info, &client->info, 1) ) - rc = -EFAULT; - } - else - { - xen_sysctl_tmem_client_t generic = { .version = TMEM_SPEC_VERSION, - .maxpools = MAX_POOLS_PER_DOMAIN }; - - if ( copy_to_guest(info, &generic, 1) ) - rc = -EFAULT; - } - break; case XEN_SYSCTL_TMEM_OP_SAVE_GET_POOL_FLAGS: if ( pool == NULL ) break; @@ -400,21 +395,24 @@ int tmem_control(struct xen_sysctl_tmem_op *op) break; case XEN_SYSCTL_TMEM_OP_LIST: ret = tmemc_list(op->cli_id, - guest_handle_cast(op->buf, char), op->arg1, op->arg2); + guest_handle_cast(op->u.buf, char), op->arg1, op->arg2); break; case XEN_SYSCTL_TMEM_OP_SET_CLIENT_INFO: - ret = tmemc_set_var(op->cli_id, cmd, - guest_handle_cast(op->buf, void)); + ret = tmemc_set_client_info(op->cli_id, + guest_handle_cast(op->u.client, xen_sysctl_tmem_client_t)); break; case XEN_SYSCTL_TMEM_OP_QUERY_FREEABLE_MB: ret = tmem_freeable_pages() >> (20 - PAGE_SHIFT); break; case XEN_SYSCTL_TMEM_OP_GET_CLIENT_INFO: + ret = tmemc_get_client_info(op->cli_id, + guest_handle_cast(op->u.client, xen_sysctl_tmem_client_t)); + break; case XEN_SYSCTL_TMEM_OP_SAVE_GET_POOL_FLAGS: case XEN_SYSCTL_TMEM_OP_SAVE_GET_POOL_NPAGES: case XEN_SYSCTL_TMEM_OP_SAVE_GET_POOL_UUID: ret = tmemc_save_subop(op->cli_id, pool_id, cmd, - guest_handle_cast(op->buf, void), op->arg1); + guest_handle_cast(op->u.buf, void), op->arg1); break; default: ret = do_tmem_control(op); diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index 27e051b..52898a7 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -782,8 +782,8 @@ struct tmem_handle { }; /* - * XEN_SYSCTL_TMEM_OP_[GET,SAVE]_CLIENT overrides the 'buf' in - * xen_sysctl_tmem_op with this structure - and contain data used + * XEN_SYSCTL_TMEM_OP_[GET,SAVE]_CLIENT uses the 'client' in + * xen_sysctl_tmem_op with this structure - which is mostly used * during migration. */ struct tmem_client { @@ -812,8 +812,11 @@ struct xen_sysctl_tmem_op { uint32_t arg2; /* IN: If not applicable to command use 0. */ uint32_t pad; /* Padding so structure is the same under 32 and 64. */ xen_tmem_oid_t oid; /* IN: If not applicable to command use 0s. */ - XEN_GUEST_HANDLE_64(char) buf; /* IN/OUT: Buffer to save/restore, - and set/get ops. */ + union { + XEN_GUEST_HANDLE_64(char) buf; /* IN/OUT: Buffer to save/restore */ + XEN_GUEST_HANDLE_64(xen_sysctl_tmem_client_t) client; /* IN/OUT for */ + /* XEN_SYSCTL_TMEM_OP_[GET,SAVE]_CLIENT. */ + } u; }; typedef struct xen_sysctl_tmem_op xen_sysctl_tmem_op_t; DEFINE_XEN_GUEST_HANDLE(xen_sysctl_tmem_op_t);
the struct tmem_op. No functional change. But it makes the code so much easier to read. Note: We still have to do this awkward 'guest_handle_cast' otherwise it will not compile on ARM - which defines _two_ of these macros (__guest_handle_64_xen_sysctl_tmem_client_t and __guest_handle_xen_sysctl_tmem_client_t). And if cast is not used then a compile error comes up as we use the wrong one. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> v1: First submission. --- tools/libxc/xc_tmem.c | 4 +-- xen/common/tmem.c | 8 +++--- xen/common/tmem_control.c | 68 ++++++++++++++++++++++----------------------- xen/include/public/sysctl.h | 11 +++++--- 4 files changed, 46 insertions(+), 45 deletions(-)