RE: tmem 32-on-64 needs (Re: [Xen-devel] [PATCH] tmem cleanup)

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> Dan Magenheimer <dan.magenheimer@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel