Dan Magenheimer
2009-Jul-28 18:00 UTC
[Xen-devel] compat tool problem with new tmem save/restore tmem_op struct
Hi Jan --
I''m trying to implement save/restore code for tmem
and to do so I need to pass other variants of the
tmem_op struct between tools (dom0) and the hypervisor
so I made the changes below to struct tmem_op.
Unfortunately the nested union seems to break your compat
translation tool. Is this an easy fix for your
tool? If not, I suppose I can recode to only
use a single level of union.
Thanks,
Dan
P.S. I tried renaming the nested "u" to "u2" but
it still causes the same problem.
struct tmem_op {
uint32_t cmd;
int32_t pool_id; /* private > 0; shared < 0; 0 is invalid */
union {
struct { /* for cmd == TMEM_NEW_POOL */
uint64_t uuid[2];
uint32_t flags;
} new;
struct {
uint64_t object;
uint32_t index;
uint32_t tmem_offset;
uint32_t pfn_offset;
uint32_t len;
tmem_cli_mfn_t cmfn; /* client machine page frame */
} gen;
struct { /* for cmd == TMEM_CONTROL */
uint32_t subop;
uint32_t cli_id;
tmem_cli_va_t buf;
union {
struct {
uint32_t arg1;
uint32_t arg2;
} gen;
struct {
uint32_t flags;
uint64_t uuid[2];
} new; /* also used for auth */
struct {
uint64_t oid;
uint32_t index;
uint32_t bufsize;
} restore;
struct {
tmem_cli_va_t p_pool_id;
tmem_cli_va_t p_oid;
tmem_cli_va_t p_index;
uint32_t bufsize;
} save;
} u;
} ctrl;
} u;
};
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Dan Magenheimer
2009-Jul-28 21:11 UTC
[Xen-devel] RE: compat tool problem with new tmem save/restore tmem_op struct
> Hi Jan -- > > I''m trying to implement save/restore code for tmem > and to do so I need to pass other variants of the > tmem_op struct between tools (dom0) and the hypervisor > so I made the changes below to struct tmem_op. > Unfortunately the nested union seems to break your compat > translation tool. Is this an easy fix for your > tool? If not, I suppose I can recode to only > use a single level of union.FYI, I''ve converted the code to a single level union, but the result is questionable as the subop that selects which structure in the union is the first element of each structure. This also required some messy work in the CONFIG_COMPAT in tmh_get_tmemop_from_client, but it compiles now and I can proceed with debugging. Thanks, Dan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Jul-29 06:54 UTC
[Xen-devel] Re: compat tool problem with new tmem save/restore tmem_op struct
>>> Dan Magenheimer <dan.magenheimer@oracle.com> 28.07.09 20:00 >>> >I''m trying to implement save/restore code for tmem >and to do so I need to pass other variants of the >tmem_op struct between tools (dom0) and the hypervisor >so I made the changes below to struct tmem_op. >Unfortunately the nested union seems to break your compat >translation tool. Is this an easy fix for your >tool? If not, I suppose I can recode to only >use a single level of union.Would be good if you pointed out *how* it fails - it doesn''t look as if it should have a problem with such a layout (and hence if it does I''d think it should be fixed). I''ll try it out myself once I can get to it, but it''s unlikely that this would be earlier than next week. And (after having read you follow up mail) I''d think flattening the structure isn''t the right thing to do. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Dan Magenheimer
2009-Jul-29 16:00 UTC
[Xen-devel] RE: compat tool problem with new tmem save/restore tmem_op struct
> Would be good if you pointed out *how* it failsSorry, I should have thought to send the make output with the first message. See attached. (Warning, this gcc output has some strange escape characters for quotes so may appear to be a binary file.)> And (after having read you follow up mail) I''d think > flattening the structure > isn''t the right thing to do.Agree. I''ll continue debugging with the flattened structure and then switch back to the nested union if/when you are able to fix the tool. Switching back will hopefully be syntactic only with search/replace. Thanks, Dan> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@novell.com] > Sent: Wednesday, July 29, 2009 12:54 AM > To: Dan Magenheimer > Cc: Xen-Devel (E-mail) > Subject: Re: compat tool problem with new tmem save/restore tmem_op > struct > > >>> Dan Magenheimer <dan.magenheimer@oracle.com> 28.07.09 20:00 >>> > >I''m trying to implement save/restore code for tmem > >and to do so I need to pass other variants of the > >tmem_op struct between tools (dom0) and the hypervisor > >so I made the changes below to struct tmem_op. > >Unfortunately the nested union seems to break your compat > >translation tool. Is this an easy fix for your > >tool? If not, I suppose I can recode to only > >use a single level of union. > > Would be good if you pointed out *how* it fails - it doesn''t > look as if it > should have a problem with such a layout (and hence if it > does I''d think > it should be fixed). I''ll try it out myself once I can get to > it, but it''s unlikely > that this would be earlier than next week. > > And (after having read you follow up mail) I''d think > flattening the structure > isn''t the right thing to do. > > Jan_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Dan Magenheimer
2009-Jul-30 22:39 UTC
[Xen-devel] RE: compat tool problem with new tmem save/restore tmem_op struct
> Agree. I''ll continue debugging with the flattened > structure and then switch back to the nested union > if/when you are able to fix the tool. Switching > back will hopefully be syntactic only with search/replace.Urk... now with the flattened (single-level union) structure, the accessors generated for my ctrl_save structure seem to be getting garbage. Looking in hex at the pointers that are getting to the hypervisor, it appears that padding and/or sizing (and thus the offsets?) of the consecutive tmem_cli_va_t struct components are wrong (with 32-bit dom0 and 64-bit hyp). So I am basically dead in the water with the tmem save/restore. Relevant patch chunks below. This stuff sure isn''t very easy or intuitive! I know I could rewrite it to work, avoiding compat/handle stuff entirely, but would prefer to use the compat/handle mechanism if possible. If you see anything wrong, please let me know! Thanks, Dan ======================== diff -r 5333e6497af6 xen/include/public/tmem.h --- a/xen/include/public/tmem.h Mon Jul 20 15:45:50 2009 +0100 +++ b/xen/include/public/tmem.h Thu Jul 30 16:33:37 2009 -0600 @@ -70,19 +92,12 @@ typedef XEN_GUEST_HANDLE(char) tmem_cli_ typedef XEN_GUEST_HANDLE(char) tmem_cli_va_t; struct tmem_op { uint32_t cmd; - int32_t pool_id; /* private > 0; shared < 0; 0 is invalid */ + int32_t pool_id; union { struct { /* for cmd == TMEM_NEW_POOL */ uint64_t uuid[2]; uint32_t flags; } new; - struct { /* for cmd == TMEM_CONTROL */ - uint32_t subop; - uint32_t cli_id; - uint32_t arg1; - uint32_t arg2; - tmem_cli_va_t buf; - } ctrl; struct { uint64_t object; uint32_t index; @@ -91,6 +106,36 @@ struct tmem_op { uint32_t len; tmem_cli_mfn_t cmfn; /* client machine page frame */ } gen; + struct { /* for cmd == TMEM_CONTROL */ + uint32_t subop; /* must be first */ + uint32_t cli_id; + uint32_t arg1; + uint32_t arg2; + tmem_cli_va_t buf; + } ctrl_gen; + struct { + uint32_t subop; /* must be first */ + uint32_t cli_id; + uint64_t uuid[2]; + uint32_t flags; + } ctrl_auth; /* also used for restore new */ + struct { + uint32_t subop; /* must be first */ + uint32_t cli_id; + tmem_cli_va_t p_pool_id; + tmem_cli_va_t p_oid; + tmem_cli_va_t p_index; + tmem_cli_va_t buf; + uint32_t bufsize; + } ctrl_save; + struct { + uint32_t subop; + uint32_t cli_id; + uint64_t oid; + uint32_t index; + tmem_cli_va_t buf; + uint32_t bufsize; + } ctrl_restore; } u; }; typedef struct tmem_op tmem_op_t; diff -r 5333e6497af6 xen/include/xen/tmem_xen.h --- a/xen/include/xen/tmem_xen.h Mon Jul 20 15:45:50 2009 +0100 +++ b/xen/include/xen/tmem_xen.h Thu Jul 30 16:33:37 2009 -0600 @@ -302,13 +309,44 @@ static inline int tmh_get_tmemop_from_cl switch ( cop.cmd ) { case TMEM_NEW_POOL: u = XLAT_tmem_op_u_new; break; - case TMEM_CONTROL: u = XLAT_tmem_op_u_ctrl; break; + case TMEM_CONTROL: + switch ( cop.u.ctrl_gen.subop ) + { + case TMEMC_SHARED_POOL_DEAUTH: + case TMEMC_SHARED_POOL_AUTH: + case TMEMC_RESTORE_NEW_POOL: + u = XLAT_tmem_op_u_ctrl_auth; + break; + case TMEMC_SAVE_GET_NEXT_PAGE: + case TMEMC_SAVE_GET_NEXT_INV: + u = XLAT_tmem_op_u_ctrl_save; + case TMEMC_RESTORE_PUT_PAGE: + case TMEMC_RESTORE_FLUSH_PAGE: + u = XLAT_tmem_op_u_ctrl_restore; + default: + u = XLAT_tmem_op_u_ctrl_gen; + } break; default: u = XLAT_tmem_op_u_gen; break; } -#define XLAT_tmem_op_HNDL_u_ctrl_buf(_d_, _s_) \ - guest_from_compat_handle((_d_)->u.ctrl.buf, (_s_)->u.ctrl.buf) +#define XLAT_tmem_op_HNDL_u_ctrl_gen_buf(_d_, _s_) \ + guest_from_compat_handle((_d_)->u.ctrl_gen.buf, (_s_)->u.ctrl_gen.buf) +#define XLAT_tmem_op_HNDL_u_ctrl_save_p_pool_id(_d_, _s_) \ + guest_from_compat_handle((_d_)->u.ctrl_save.p_pool_id, (_s_)->u.ctrl_save.p_pool_id) +#define XLAT_tmem_op_HNDL_u_ctrl_save_p_oid(_d_, _s_) \ + guest_from_compat_handle((_d_)->u.ctrl_save.p_oid, (_s_)->u.ctrl_save.p_oid) +#define XLAT_tmem_op_HNDL_u_ctrl_save_p_index(_d_, _s_) \ + guest_from_compat_handle((_d_)->u.ctrl_save.p_index, (_s_)->u.ctrl_save.p_index) +#define XLAT_tmem_op_HNDL_u_ctrl_save_buf(_d_, _s_) \ + guest_from_compat_handle((_d_)->u.ctrl_save.buf, (_s_)->u.ctrl_save.buf) +#define XLAT_tmem_op_HNDL_u_ctrl_restore_buf(_d_, _s_) \ + guest_from_compat_handle((_d_)->u.ctrl_restore.buf, (_s_)->u.ctrl_restore.buf) XLAT_tmem_op(op, &cop); -#undef XLAT_tmem_op_HNDL_u_ctrl_buf +#undef XLAT_tmem_op_HNDL_u_ctrl_gen_buf +#undef XLAT_tmem_op_HNDL_u_ctrl_save_p_pool_id +#undef XLAT_tmem_op_HNDL_u_ctrl_save_p_oid +#undef XLAT_tmem_op_HNDL_u_ctrl_save_p_index +#undef XLAT_tmem_op_HNDL_u_ctrl_save_buf +#undef XLAT_tmem_op_HNDL_u_ctrl_restore_buf return 0; } #endif _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Jul-31 06:29 UTC
[Xen-devel] RE: compat tool problem with new tmem save/restore tmem_op struct
>>> Dan Magenheimer <dan.magenheimer@oracle.com> 31.07.09 00:39 >>> >Urk... now with the flattened (single-level union) >structure, the accessors generated for my >ctrl_save structure seem to be getting garbage. >Looking in hex at the pointers that are getting >to the hypervisor, it appears that padding and/or >sizing (and thus the offsets?) of the consecutive >tmem_cli_va_t struct components are wrong >(with 32-bit dom0 and 64-bit hyp).Hmm, pretty odd: Your code looks right afaict without actually trying to compile it (apart from a few missing break statements). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Dan Magenheimer
2009-Jul-31 17:17 UTC
[Xen-devel] RE: compat tool problem with new tmem save/restore tmem_op struct
> >>> Dan Magenheimer <dan.magenheimer@oracle.com> 31.07.09 00:39 >>> > >Urk... now with the flattened (single-level union) > >structure, the accessors generated for my > >ctrl_save structure seem to be getting garbage. > >Looking in hex at the pointers that are getting > >to the hypervisor, it appears that padding and/or > >sizing (and thus the offsets?) of the consecutive > >tmem_cli_va_t struct components are wrong > >(with 32-bit dom0 and 64-bit hyp). > > Hmm, pretty odd: Your code looks right afaict without > actually trying to > compile it (apart from a few missing break statements).Well I''ve fallen back to a simpler single-level union by generalizing the names and somewhat overloading some of the elements of the structures. As a result, you can ignore my report of the problems with the two-level union and the above problem (which I think was a problem with having a number of consecutive handles in a struct in the union... but didn''t isolate it any further). Sorry if you''ve wasted any more time on this. Thanks, Dan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel