Xen devel - Feb 2009 - [PATCH 8/9] tmem: invocations of tmem code from existing xen code

[8] invocations of tmem code from existing xen code

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Dan Magenheimer <dan.magenheimer@oracle.com> 06.02.09 01:47
>>>
>[8] invocations of tmem code from existing xen code
You use do_tmem_op() directly for 32-on-64 mode, which I think is wrong
given the definition of the interface structure (you''d minimally have
to use
XEN_GUEST_HANDLE_64 in order for this to be correct). But even if it was
right (or if you make it so), it would in my opinion be a requirement to
assure it is by adding a respective entry to xen/include/xlat.lst.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> You use do_tmem_op() directly for 32-on-64 mode, which I 
> think is wrong
> given the definition of the interface structure (you''d 
> minimally have to use
> XEN_GUEST_HANDLE_64 in order for this to be correct). But 
> even if it was
> right (or if you make it so), it would in my opinion be a 
> requirement to
> assure it is by adding a respective entry to xen/include/xlat.lst.
I have to admit I am baffled by all aspects of XEN_GUEST_HANDLE
and just hacked on it until the code worked.  I would be grateful
for either an explanation of xen/include/xlat.lst or a pointer
to an explanation... or, even better, hints on how to fix
my code to properly use XEN_GUEST_HANDLE_64.

I would like the code to definitely support 32-on-64 and
64-on-64.  32-on-32 is optional; because of xenheap limitations
it is largely a toy, but still useful for testing.

All of this works today, but possibly only due to hackery,
which I am eager to fix properly if told how.

Thanks,
Dan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Dan Magenheimer <dan.magenheimer@oracle.com> 06.02.09 15:25
>>>
>I have to admit I am baffled by all aspects of XEN_GUEST_HANDLE
>and just hacked on it until the code worked.  I would be grateful
>for either an explanation of xen/include/xlat.lst or a pointer
>to an explanation... or, even better, hints on how to fix
>my code to properly use XEN_GUEST_HANDLE_64.
The easiest thing is certainly to simply replace XEN_GUEST_HANDLE by
XEN_GUEST_HANDLE_64.

As to xlat.lst, just add ? entry there - the ? says the type needs to be
checked, and it is followed by the type''s name and the header it is
found in. This will generate a CHECK_<typename> macro in
xen/include/compat/xlat.h, which you then ought to use somewhere in
the source code. Just pick one of the existing entries to see how this
works.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >>> Dan Magenheimer <dan.magenheimer@oracle.com> 06.02.09
15:25 >>>
> >I have to admit I am baffled by all aspects of XEN_GUEST_HANDLE
> >and just hacked on it until the code worked.  I would be grateful
> >for either an explanation of xen/include/xlat.lst or a pointer
> >to an explanation... or, even better, hints on how to fix
> >my code to properly use XEN_GUEST_HANDLE_64.
> 
> The easiest thing is certainly to simply replace XEN_GUEST_HANDLE by
> XEN_GUEST_HANDLE_64.
OK, I did that and it compiles fine.  What''s the difference
between the two?  Is the _64 version used when the structure
contains a 64-bit value that might not be aligned on a 64-bit
boundary?
> As to xlat.lst, just add ? entry there - the ? says the type 
> needs to be
> checked, and it is followed by the type''s name and the header it
is
> found in. This will generate a CHECK_<typename> macro in
> xen/include/compat/xlat.h, which you then ought to use somewhere in
> the source code. Just pick one of the existing entries to see how this
> works.
I''m afraid that after an hour or two of trying to understand
this xlat code (and why it exists*) and trying to duplicate it for
tmem (which works without it), I''m still not able to get anything
to compile, probably because I don''t really understand what''s
going
on.  Could you be a bit more precise as to what is needed?
Perhaps a README or wiki page would be useful.

Just adding tmem_op...tmem.h to xlat.lst and adding a new
common/compat/tmem_xen.c with:

#define xen_tmem_op tmem_op
CHECK_tmem_op;
#undef xen_tmem_op

doesn''t seem to work (generates "sed: can''t read
compat/tmem.h"),
so please define "use somewhere" more precisely.  The
existing entries don''t provide much guidance as they all seem
to be doing complex things, that I don''t think I need.

Also, ignoring the structure alignment/packing rules, is this
supposed to get rid of the masking I do in cli_mfn_to_va()
when the hypercall is made by a 32-bit guest?

Thanks,
Dan

* I guess I come from an era where it is the system programmer''s
  responsibility to ensure that data structures are defined to
  ensure they meet all the packing and alignment restrictions of
  all the architectures they are intended to run on.  While I''m
  all for compile-time checks to catch poor programming or old
  16- and 32-bit code that now needs to run on a 64-bit machine,
  this seems, well, a bit overboard imho.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Dan Magenheimer <dan.magenheimer@oracle.com> 06.02.09 20:00
>>>
>OK, I did that and it compiles fine.  What''s the difference
>between the two?  Is the _64 version used when the structure
>contains a 64-bit value that might not be aligned on a 64-bit
>boundary?
Not only, it''s also widening the field to 64-bits to make sure the
(native)
64-bit accessors can be used on it.
>Just adding tmem_op...tmem.h to xlat.lst and adding a new
>common/compat/tmem_xen.c with:
>
>#define xen_tmem_op tmem_op
>CHECK_tmem_op;
>#undef xen_tmem_op
>
>doesn''t seem to work (generates "sed: can''t read
compat/tmem.h"),
Yeah, I forgot that with a new header you''d have to also add that new
header''s name to header-y in xen/include/Makefile, so that a compat
header would be generated.
>so please define "use somewhere" more precisely.  The
>existing entries don''t provide much guidance as they all seem
>to be doing complex things, that I don''t think I need.
If it seems to cumbersome, just leave it off for the moment, and I''ll
submit
a fixup patch after it got merged.
>Also, ignoring the structure alignment/packing rules, is this
>supposed to get rid of the masking I do in cli_mfn_to_va()
>when the hypercall is made by a 32-bit guest?
I thought it was (I didn''t invent it), but it appears it
isn''t. Though there also
shouldn''t be a need to - all the guest can clobber by specifying a
handle
that doesn''t have the upper 32 bits clear is its argument translation
area,
which means it can only shoot itself in the foot. But you ought to be using
guest_handle_cast there (and perhaps elsewhere) anyway (rather than
referencing handle.p directly), which would allow hiding any masking in
case it would turn out necessary.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> If it seems to cumbersome, just leave it off for the moment, 
> and I''ll submit
> a fixup patch after it got merged.
Thanks very much!  I''ll leave my partially complete compat
changes in the next round as there are probably just one
or two things I am missing.
 
> But you ought to be using
> guest_handle_cast there (and perhaps elsewhere) anyway (rather than
> referencing handle.p directly), which would allow hiding any 
> masking in
> case it would turn out necessary.
I think cli_mfn_to_va() is the only place.  Since the current
code works and my attempts to fix it to use guest_handle_cast
have failed, I''d appreciate post-merge fixup help here too!

Thanks,
Dan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel