Xen devel - Jun 2009 - [PATCH] tmem cleanup

- don''t mis-use guest handle for passing an MFN value
- eliminate unnecessary (and misplaced) use of XEN_GUEST_HANDLE_64
- use copy_from_guest() instead of __copy_from_guest() for loading the
  argument structure

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- 2009-06-10.orig/xen/common/tmem_xen.c	2009-05-27 13:54:07.000000000 +0200
+++ 2009-06-10/xen/common/tmem_xen.c	2009-06-15 15:00:48.000000000 +0200
@@ -87,10 +87,7 @@ static inline void *cli_mfn_to_va(tmem_c
     unsigned long cli_mfn;
     p2m_type_t t;
 
-
-    if (is_pv_32on64_vcpu(current))
-        cmfn.p = (void *)((unsigned long)cmfn.p & 0xffffffffUL);
-    cli_mfn = mfn_x(gfn_to_mfn(current->domain,(unsigned
long)cmfn.p,&t));
+    cli_mfn = mfn_x(gfn_to_mfn(current->domain, cmfn, &t));
     if (t != p2m_ram_rw)
         return NULL;
     if (pcli_mfn != NULL)
--- 2009-06-10.orig/xen/include/public/tmem.h	2009-05-27 13:54:07.000000000
+0200
+++ 2009-06-10/xen/include/public/tmem.h	2009-06-15 14:58:25.000000000 +0200
@@ -66,7 +66,7 @@
 
 
 #ifndef __ASSEMBLY__
-typedef XEN_GUEST_HANDLE(void) tmem_cli_mfn_t;
+typedef xen_pfn_t tmem_cli_mfn_t;
 typedef XEN_GUEST_HANDLE(char) tmem_cli_va_t;
 struct tmem_op {
     uint32_t cmd;
@@ -95,7 +95,6 @@ struct tmem_op {
 };
 typedef struct tmem_op tmem_op_t;
 DEFINE_XEN_GUEST_HANDLE(tmem_op_t);
-typedef XEN_GUEST_HANDLE_64(tmem_op_t) tmem_cli_op_t;
 
 #endif
 
--- 2009-06-10.orig/xen/include/xen/tmem_xen.h	2009-05-27 13:54:07.000000000
+0200
+++ 2009-06-10/xen/include/xen/tmem_xen.h	2009-06-15 15:11:54.000000000 +0200
@@ -281,12 +281,12 @@ static inline bool_t tmh_current_is_priv
 /* these typedefs are in the public/tmem.h interface
 typedef XEN_GUEST_HANDLE(void) cli_mfn_t;
 typedef XEN_GUEST_HANDLE(char) cli_va_t;
-typedef XEN_GUEST_HANDLE(tmem_op_t) cli_tmemop_t;
 */
+typedef XEN_GUEST_HANDLE(tmem_op_t) tmem_cli_op_t;
 
 static inline int tmh_get_tmemop_from_client(tmem_op_t *op, tmem_cli_op_t uops)
 {
-    return __copy_from_guest(op, uops, 1);
+    return copy_from_guest(op, uops, 1);
 }
 
 static inline void tmh_copy_to_client_buf_offset(tmem_cli_va_t clibuf, int off,




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

>>> "Jan Beulich" <JBeulich@novell.com> 15.06.09 15:47
>>>
>- don''t mis-use guest handle for passing an MFN value
>- eliminate unnecessary (and misplaced) use of XEN_GUEST_HANDLE_64
>- use copy_from_guest() instead of __copy_from_guest() for loading the
>  argument structure
I ran across these when looking at what changes 32-on-64 support for tmem
would require. However, there''s another issue that I didn''t
feel like immediately
sending a patch out for: xen/include/public/tmem.h uses anonymous unions
and structures - all other public headers (with the exception of the declaration
of xenpf_set_processor_pminfo and xen_sysctl_pm_op, which probably are
mistakes/oversights) avoid this (or use it only when it is known to be only
compiled with gcc)) as being a non-standard feature, and the compatibility
header generation script depends on all compound types'' fields to be
named.

I realize that this will require changes on the client side too, which is why I
first wanted to see if there''s anything that I''m overlooking.

Jan


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

Hi Jan --

Thanks much for looking at this.

32-on-64 support does work already, but I think as we discussed
before, it is due to careful structure organizaton which is
fragile (e.g. if someone adds another field in the future).

I wasn''t aware of the restriction on public unions.  The
changes are strictly syntactic, correct?
> and the compatibility header generation script depends on
> all compound types'' fields to be named.
Ah, so that''s what was causing those problems for me!
> first wanted to see if there''s anything that I''m
overlooking.
I can''t think of anything you might be overlooking.
Do you want me to submit the patch or do you already
have one underway?
>--- 2009-06-10.orig/xen/common/tmem_xen.c	2009-05-27 13:54:07.000000000
+0200
>+++ 2009-06-10/xen/common/tmem_xen.c	2009-06-15 15:00:48.000000000 +0200
>@@ -87,10 +87,7 @@ static inline void *cli_mfn_to_va(tmem_c
>     unsigned long cli_mfn;
>     p2m_type_t t;
> 
>-
>-    if (is_pv_32on64_vcpu(current))
>-        cmfn.p = (void *)((unsigned long)cmfn.p & 0xffffffffUL);
>-    cli_mfn = mfn_x(gfn_to_mfn(current->domain,(unsigned
long)cmfn.p,&t));
>+    cli_mfn = mfn_x(gfn_to_mfn(current->domain, cmfn, &t));
Are you sure this works?  I seem to recall I tried that first
and it failed on 32-on-64.  However it was a long time ago
so may have been due to a different problem.

Thanks again,
Dan
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@novell.com]
> Sent: Monday, June 15, 2009 8:09 AM
> To: Dan Magenheimer
> Cc: xen-devel@lists.xensource.com
> Subject: tmem 32-on-64 needs (Re: [Xen-devel] [PATCH] tmem cleanup)
> 
> 
> >>> "Jan Beulich" <JBeulich@novell.com> 15.06.09
15:47 >>>
> >- don''t mis-use guest handle for passing an MFN value
> >- eliminate unnecessary (and misplaced) use of XEN_GUEST_HANDLE_64
> >- use copy_from_guest() instead of __copy_from_guest() for 
> loading the
> >  argument structure
> 
> I ran across these when looking at what changes 32-on-64 
> support for tmem
> would require. However, there''s another issue that I
didn''t
> feel like immediately
> sending a patch out for: xen/include/public/tmem.h uses 
> anonymous unions
> and structures - all other public headers (with the exception 
> of the declaration
> of xenpf_set_processor_pminfo and xen_sysctl_pm_op, which probably are
> mistakes/oversights) avoid this (or use it only when it is 
> known to be only
> compiled with gcc)) as being a non-standard feature, and the 
> compatibility
> header generation script depends on all compound types'' 
> fields to be named.
> 
> I realize that this will require changes on the client side 
> too, which is why I
> first wanted to see if there''s anything that I''m
overlooking.
> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, Jun 15, 2009 at 03:09:26PM +0100, Jan Beulich wrote:
> sending a patch out for: xen/include/public/tmem.h uses anonymous unions
> and structures - all other public headers (with the exception of the
declaration
> of xenpf_set_processor_pminfo and xen_sysctl_pm_op, which probably are
> mistakes/oversights)
FYI both the pminfo code and anything else that uses anon struct/unions in
public headers is a constant pain for us. Sun Studio compilers can sort
of do it, but it''s too buggy to use right now.

regards
john

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

>>> Dan Magenheimer <dan.magenheimer@oracle.com> 15.06.09 18:17
>>>
>32-on-64 support does work already, but I think as we discussed
>before, it is due to careful structure organizaton which is
>fragile (e.g. if someone adds another field in the future).
It works only most of the time: The structure sizes differ between 32- and
64-bits, and if a 32-bit guest placed such a structure right at the end of a
page, followed by a not-present page, unexpected -EFAULT returns would
result. With (non-64-bit) guest handles among the structure members,
there''s no way to circumvent argument translation.
>I wasn''t aware of the restriction on public unions.  The
>changes are strictly syntactic, correct?
Yes.
>> and the compatibility header generation script depends on
>> all compound types'' fields to be named.
>
>Ah, so that''s what was causing those problems for me!
No, I don''t think so. According to the commented out code you added to
xen/include/xlat.lst, your intention was to just verify that the 32- and 64-
bit layouts match - as per the above explanation this was expected to
not be the case.

Instead, requesting translation here, things do *unexpectedly* build -
because the script ignores the unnamed union.
>> first wanted to see if there''s anything that I''m
overlooking.
>
>I can''t think of anything you might be overlooking.
>Do you want me to submit the patch or do you already
>have one underway?
I have one halfway ready - what would be nice is if the linux side patch
could be delayed until the adjustment is in place (and preferably until
Keir afterwards sync-ed over the public headers), so there would not
be temporary build breakage.
>>--- 2009-06-10.orig/xen/common/tmem_xen.c	2009-05-27 13:54:07.000000000
+0200
>>+++ 2009-06-10/xen/common/tmem_xen.c	2009-06-15 15:00:48.000000000 +0200
>>@@ -87,10 +87,7 @@ static inline void *cli_mfn_to_va(tmem_c
>>     unsigned long cli_mfn;
>>     p2m_type_t t;
>> 
>>-
>>-    if (is_pv_32on64_vcpu(current))
>>-        cmfn.p = (void *)((unsigned long)cmfn.p & 0xffffffffUL);
>>-    cli_mfn = mfn_x(gfn_to_mfn(current->domain,(unsigned
long)cmfn.p,&t));
>>+    cli_mfn = mfn_x(gfn_to_mfn(current->domain, cmfn, &t));
>
>Are you sure this works?  I seem to recall I tried that first
>and it failed on 32-on-64.  However it was a long time ago
>so may have been due to a different problem.
By itself it will (temporarily) break 32-on-64, but (a) misusing a handle for
passing an MFN is broken anyway (MFNs get passed elsewhere without
causing issues for 32-on-64) and (b) doing open-coded, randomly placed
handle manipulation is fragile and a latent source of future issues.

Jan


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

>>> John Levon <levon@movementarian.org> 15.06.09 20:54
>>>
>On Mon, Jun 15, 2009 at 03:09:26PM +0100, Jan Beulich wrote:
>
>> sending a patch out for: xen/include/public/tmem.h uses anonymous
unions
>> and structures - all other public headers (with the exception of the
declaration
>> of xenpf_set_processor_pminfo and xen_sysctl_pm_op, which probably are
>> mistakes/oversights)
>
>FYI both the pminfo code and anything else that uses anon struct/unions in
>public headers is a constant pain for us. Sun Studio compilers can sort
>of do it, but it''s too buggy to use right now.
I''ll have to see whether gcc has a mode to make it reject anything non-
standard - if so, I''d like to add to the build a header validation step
that
ensures no extensions are used outside of __GNUC__, __XEN__, or __XEN_TOOLS__
conditionals.

In any case, I added fixing the public headers to my to-do list.

Jan


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

On 16/06/2009 07:53, "Jan Beulich" <JBeulich@novell.com> wrote:
>>> first wanted to see if there''s anything that I''m
overlooking.
>> 
>> I can''t think of anything you might be overlooking.
>> Do you want me to submit the patch or do you already
>> have one underway?
> 
> I have one halfway ready - what would be nice is if the linux side patch
> could be delayed until the adjustment is in place (and preferably until
> Keir afterwards sync-ed over the public headers), so there would not
> be temporary build breakage.
If unions are going to be de-anonymised etc I think it makes sense to delay
until the interface changes are made.

 -- Keir



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