i#751: Introduce shared_code_x86_to_x64.

Reviewer: rnk@google.com
i#751: Introduce shared_code_x86_to_x64.

i#751: Introduce shared_code_x86_to_x64.

For the purpose of this CL, the shared_code_x86_to_x64 is the same as
shared_code. Later, it will contain IBL routines that use r8-r10 as
scratch space.

We switch IBL in a mode-switching far jump.

Now, emit_far_ibl() looks like before r1519.

Also wrap the option x86_to_x64 with #ifdef X64. (not sure if I should
submit it as a separate CL.)

[Reid Kleckner (google), 2012-08-08 15:03:10]

Only two comments.  Derek is the one that wrote the mixed mode ibl support, so I
think he should also take a look.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.h
File trunk/core/x86/arch.h (right):

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.h#newcode516
trunk/core/x86/arch.h:516: bool x86_to_x64_mode; /* Does this code use r8-r10 as
scratch (for x86_to_x64)? */
OK, so these extra modes and far_ref's are only necessary to support 32-bit
fragments with ibls in the future.  My inclination is to sidestep this
complexity by not supporting 32-bit fragments with indirect branches.  But I
don't think that will work, because the whole reason for moving the mode
switching into the ibl code was to move it out of the fragment, so the fragment
can be decoded all in one mode.

I reviewed this part of the mixed mode support, but Derek wrote it, so maybe he
should take a look.  :)

If we go down this road, it might be cleaner to have a mode enum { x64, x86,
x86_to_x64 } and an array of far_ref_t's indexed by mode.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/emit_utils.c
File trunk/core/x86/emit_utils.c (right):

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/emit_utils.c#newc...
trunk/core/x86/emit_utils.c:5990: IF_X64(bool mode;)
You can move this into mixed_mode_enabled() to limit scope and avoid the IF_X64.
 I'd also give it a more descriptive name, thinking "source_is_x86".

[bruening, 2012-08-09 15:56:16]

LGTM if you remove the far_jmp_opnd_for_x86_to_x64, per our chat discussion.

Note that putting back the detection of mode changes in far ibl is actually
required for correctness (i#865) so that's a plus.

Re: general discussion about whether assuming all fragments are translated is a
big simplification: not sure I agree, b/c  in order to co-opt x86_mode and
FRAG_32_BIT and other flags you'd have to add more checks in places where those
assume underlying 32-bit.  Yes you could avoid the extra generated_code_t, but
it seems that that's the only real simplification, and you force yourself down
the emulation road which IMHO would probably end up w/ a parallel code cache
construct which I want to avoid.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c
File trunk/core/x86/arch.c (right):

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode301
trunk/core/x86/arch.c:301: *   when x86_mode is false and x86_to_x64_mode is
true => inits shared_code_x86_to_x64.
hmm, maybe an enum is better.  though that would require more code changes. 
just leave it, seems fine.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode303
trunk/core/x86/arch.c:303: *     fragments are all 64-bit for -x86_to_x64 (xref
i#862).
the "since..." doesn't make sense: it's that they are inherently incompatible. 
it has nothing to do w/ i#862.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode514
trunk/core/x86/arch.c:514: far_ibl_set_targets(ibl_code_t src_ibl[], ibl_code_t
tgt_ibl[], bool x86_to_x64_mode)
you can remove this param

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode527
trunk/core/x86/arch.c:527: if (x86_to_x64_mode) {
per our discussion, you can revert this change completely: no reason to have
separate behavior or separate far_jmp_opnd vars

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode606
trunk/core/x86/arch.c:606: shared_gencode_init(IF_X64(false/*x64 mode*/)
_IF_X64(false/*not x86_to_x64 mode*/));
line too long (90-char max)

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode614
trunk/core/x86/arch.c:614: * shared_code_x86 (xref i#862).
it seems odd to force this here: why not generate both and make it easier to add
32-bit fragment support later?

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode622
trunk/core/x86/arch.c:622: far_ibl_set_targets(shared_code->trace_ibl,
shared_code_x86_to_x64->trace_ibl, true);
if it weren't for i#865 you could use the same-mode far ibl but it's not quite
correct so never mind

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode622
trunk/core/x86/arch.c:622: far_ibl_set_targets(shared_code->trace_ibl,
shared_code_x86_to_x64->trace_ibl, true);
line too long, here and below (90-char max).  though you need to remove the
param so some will fit.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode803
trunk/core/x86/arch.c:803: &ibl_code->far_jmp_opnd_for_x86_to_x64 :
indentation looks off

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode809
trunk/core/x86/arch.c:809: &ibl_code->far_jmp_unlinked_opnd_for_x86_to_x64 :
indentation looks off

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.h
File trunk/core/x86/arch.h (right):

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.h#newcode516
trunk/core/x86/arch.h:516: bool x86_to_x64_mode; /* Does this code use r8-r10 as
scratch (for x86_to_x64)? */
this needs to be in generated_code_t for optimizing context switch code.  I
guess I have x86_mode cached here as well as in the parent (b/c no backpointer).
 could keep duplicate, or add backpointer.

-x86_to_x64 doesn't need to support 32-bit ibl: it only needs to support 32-bit
context switches, b/c it can limit 32-bit fragments to one-instruction bbs for
un-translatable instrs.  thus it can avoid any 32-bit indirect branches, far
branches, and syscalls.  maybe there's some other reason to support 32-bit
general fragments but I can't think of it so it seems reasonable to me to assume
there are no 32-bit ibls.  however, I'm not sure that really simplifies much
here.  it does simplify the far ibl a little, and that we can get by just not
supporting 32-bit fragments w/ far ctis.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.h#newcode522
trunk/core/x86/arch.h:522: far_ref_t far_jmp_unlinked_opnd_for_x86_to_x64;
why do you need these?  each ibl still has just one corresponding other-mode
ibl.  why do you need a separate copy?

per our offline discussion, we're in agreement: these will be removed

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.h#newcode709
trunk/core/x86/arch.h:709: #define SHARED_CODE_FOR_32BIT_APP \
style: () (makes it clearer it's a complex expression, not just a numeric const)

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.h#newcode725
trunk/core/x86/arch.h:725: SHARED_CODE_FOR_32BIT_APP : shared_code;
why not use X64_CACHE_MODE_DC() here and then you don't need the FIXME above and
can add 32-bit support w/ fewer changes?

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/emit_utils.c
File trunk/core/x86/emit_utils.c (right):

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/emit_utils.c#newc...
trunk/core/x86/emit_utils.c:5983: * FIXME i#862: this won't work if we have
32-bit fragments.
even if we use 32-bit fragments for BCD, etc.,
we can disallow 32-bit fragments from having far branches,
so assuming no 32-bit un-translated code entering here seems reasonable.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/emit_utils.c#newc...
trunk/core/x86/emit_utils.c:5995: mode = DYNAMO_OPTION(x86_to_x64) ?
ibl_code->x86_to_x64_mode : ibl_code->x86_mode;
ok, now that a separate far_ibl is used for translated code, you can now detect
whether it's a mode change => prior version of this code.

note that detecting the mode change is actually required for correct execution:
see i#865.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/emit_utils.c#newc...
trunk/core/x86/emit_utils.c:5998: short selector = mode ? CS64_SELECTOR :
CS32_SELECTOR;
"mode" is vague: better name?

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/emit_utils.c#newc...
trunk/core/x86/emit_utils.c:6053: /* currently, ibl is always 64-bit for
-x86_to_x64 (xref i#862) */
we can disallow 32-bit fragments from having indirect branches or far branches
or system calls, and thus don't need any 32-bit ibl.

even if we allow 32-bit indirection, here we have to pick one lookup method, and
we'd go w/ the most common, which would assume a 32-bit target has been
translated: so even for a same-mode far cti in a 32-bit (untranslated) fragment,
we'd want to do a mode change here.  worth a comment about that.  seems fine to
make the assumptions above, and then we don't have to do that mode change.

[yangt, 2012-08-09 21:46:02]

Modified a lot of places. Would you please take another look?

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c
File trunk/core/x86/arch.c (right):

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode301
trunk/core/x86/arch.c:301: *   when x86_mode is false and x86_to_x64_mode is
true => inits shared_code_x86_to_x64.
On 2012/08/09 15:56:16, bruening wrote:
> hmm, maybe an enum is better.  though that would require more code changes. 
> just leave it, seems fine.

Changed to use gencode_mode_t.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode303
trunk/core/x86/arch.c:303: *     fragments are all 64-bit for -x86_to_x64 (xref
i#862).
On 2012/08/09 15:56:16, bruening wrote:
> the "since..." doesn't make sense: it's that they are inherently incompatible.

> it has nothing to do w/ i#862.

Done.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode514
trunk/core/x86/arch.c:514: far_ibl_set_targets(ibl_code_t src_ibl[], ibl_code_t
tgt_ibl[], bool x86_to_x64_mode)
On 2012/08/09 15:56:16, bruening wrote:
> you can remove this param

Done.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode527
trunk/core/x86/arch.c:527: if (x86_to_x64_mode) {
On 2012/08/09 15:56:16, bruening wrote:
> per our discussion, you can revert this change completely: no reason to have
> separate behavior or separate far_jmp_opnd vars

Done.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode606
trunk/core/x86/arch.c:606: shared_gencode_init(IF_X64(false/*x64 mode*/)
_IF_X64(false/*not x86_to_x64 mode*/));
On 2012/08/09 15:56:16, bruening wrote:
> line too long (90-char max)

Done.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode614
trunk/core/x86/arch.c:614: * shared_code_x86 (xref i#862).
On 2012/08/09 15:56:16, bruening wrote:
> it seems odd to force this here: why not generate both and make it easier to
add
> 32-bit fragment support later?

Done.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode622
trunk/core/x86/arch.c:622: far_ibl_set_targets(shared_code->trace_ibl,
shared_code_x86_to_x64->trace_ibl, true);
On 2012/08/09 15:56:16, bruening wrote:
> line too long, here and below (90-char max).  though you need to remove the
> param so some will fit.

Done.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode803
trunk/core/x86/arch.c:803: &ibl_code->far_jmp_opnd_for_x86_to_x64 :
On 2012/08/09 15:56:16, bruening wrote:
> indentation looks off

Done.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.c#newcode809
trunk/core/x86/arch.c:809: &ibl_code->far_jmp_unlinked_opnd_for_x86_to_x64 :
On 2012/08/09 15:56:16, bruening wrote:
> indentation looks off

Done.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.h
File trunk/core/x86/arch.h (right):

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.h#newcode516
trunk/core/x86/arch.h:516: bool x86_to_x64_mode; /* Does this code use r8-r10 as
scratch (for x86_to_x64)? */
On 2012/08/09 15:56:16, bruening wrote:
> this needs to be in generated_code_t for optimizing context switch code.  I
> guess I have x86_mode cached here as well as in the parent (b/c no
backpointer).
>  could keep duplicate, or add backpointer.
> 
> -x86_to_x64 doesn't need to support 32-bit ibl: it only needs to support
32-bit
> context switches, b/c it can limit 32-bit fragments to one-instruction bbs for
> un-translatable instrs.  thus it can avoid any 32-bit indirect branches, far
> branches, and syscalls.  maybe there's some other reason to support 32-bit
> general fragments but I can't think of it so it seems reasonable to me to
assume
> there are no 32-bit ibls.  however, I'm not sure that really simplifies much
> here.  it does simplify the far ibl a little, and that we can get by just not
> supporting 32-bit fragments w/ far ctis.

ok.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.h#newcode522
trunk/core/x86/arch.h:522: far_ref_t far_jmp_unlinked_opnd_for_x86_to_x64;
On 2012/08/09 15:56:16, bruening wrote:
> why do you need these?  each ibl still has just one corresponding other-mode
> ibl.  why do you need a separate copy?
> 
> per our offline discussion, we're in agreement: these will be removed

Done.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.h#newcode709
trunk/core/x86/arch.h:709: #define SHARED_CODE_FOR_32BIT_APP \
On 2012/08/09 15:56:16, bruening wrote:
> style: () (makes it clearer it's a complex expression, not just a numeric
const)

Done.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/arch.h#newcode725
trunk/core/x86/arch.h:725: SHARED_CODE_FOR_32BIT_APP : shared_code;
On 2012/08/09 15:56:16, bruening wrote:
> why not use X64_CACHE_MODE_DC() here and then you don't need the FIXME above
and
> can add 32-bit support w/ fewer changes?

Done. As a side effect this file now includes decode.h.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/emit_utils.c
File trunk/core/x86/emit_utils.c (right):

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/emit_utils.c#newc...
trunk/core/x86/emit_utils.c:5983: * FIXME i#862: this won't work if we have
32-bit fragments.
On 2012/08/09 15:56:16, bruening wrote:
> even if we use 32-bit fragments for BCD, etc.,
> we can disallow 32-bit fragments from having far branches,
> so assuming no 32-bit un-translated code entering here seems reasonable.

Done.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/emit_utils.c#newc...
trunk/core/x86/emit_utils.c:5990: IF_X64(bool mode;)
On 2012/08/08 15:03:10, Reid Kleckner (google) wrote:
> You can move this into mixed_mode_enabled() to limit scope and avoid the
IF_X64.
>  I'd also give it a more descriptive name, thinking "source_is_x86".

Done.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/emit_utils.c#newc...
trunk/core/x86/emit_utils.c:5995: mode = DYNAMO_OPTION(x86_to_x64) ?
ibl_code->x86_to_x64_mode : ibl_code->x86_mode;
On 2012/08/09 15:56:16, bruening wrote:
> ok, now that a separate far_ibl is used for translated code, you can now
detect
> whether it's a mode change => prior version of this code.
> 
> note that detecting the mode change is actually required for correct
execution:
> see i#865.

ok.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/emit_utils.c#newc...
trunk/core/x86/emit_utils.c:5998: short selector = mode ? CS64_SELECTOR :
CS32_SELECTOR;
On 2012/08/09 15:56:16, bruening wrote:
> "mode" is vague: better name?

Changed to source_is_x86 as Reid suggested.

http://codereview.appspot.com/6453095/diff/1/trunk/core/x86/emit_utils.c#newc...
trunk/core/x86/emit_utils.c:6053: /* currently, ibl is always 64-bit for
-x86_to_x64 (xref i#862) */
On 2012/08/09 15:56:16, bruening wrote:
> we can disallow 32-bit fragments from having indirect branches or far branches
> or system calls, and thus don't need any 32-bit ibl.
> 
> even if we allow 32-bit indirection, here we have to pick one lookup method,
and
> we'd go w/ the most common, which would assume a 32-bit target has been
> translated: so even for a same-mode far cti in a 32-bit (untranslated)
fragment,
> we'd want to do a mode change here.  worth a comment about that.  seems fine
to
> make the assumptions above, and then we don't have to do that mode change.

I'd disallow 32-bit indirection. I've copied & pasted your comment into the
code.

[bruening, 2012-08-10 21:11:53]

LGTM

http://codereview.appspot.com/6453095/diff/1009/trunk/core/x86/emit_utils.c
File trunk/core/x86/emit_utils.c (right):

http://codereview.appspot.com/6453095/diff/1009/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:5982: */
can you add a FIXME i#865 comment about preserving the app's r8-r15 during
32-bit execution