* [PATCH] x86/emul: only emulate possibly operand sizes for POPA
@ 2012-11-07 16:08 Jan Beulich
2012-11-07 17:10 ` Keir Fraser
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2012-11-07 16:08 UTC (permalink / raw)
To: xen-devel
[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]
This opcode neither support 1-byte operands, nor does it support 8-byte
ones (since the opcode is undefined in 64-bit mode). Simplify the code
accordingly.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1996,13 +1996,10 @@ x86_emulate(
if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
&dst.val, op_bytes, ctxt, ops)) != 0 )
goto done;
- switch ( op_bytes )
- {
- case 1: *(uint8_t *)regs[i] = (uint8_t)dst.val; break;
- case 2: *(uint16_t *)regs[i] = (uint16_t)dst.val; break;
- case 4: *regs[i] = (uint32_t)dst.val; break; /* 64b: zero-ext */
- case 8: *regs[i] = dst.val; break;
- }
+ if ( op_bytes != 2 )
+ *regs[i] = (uint32_t)dst.val; /* 64b: zero-ext */
+ else
+ *(uint16_t *)regs[i] = (uint16_t)dst.val;
}
break;
}
[-- Attachment #2: x86-emul-popa-sizes.patch --]
[-- Type: text/plain, Size: 1128 bytes --]
x86/emul: only emulate possibly operand sizes for POPA
This opcode neither support 1-byte operands, nor does it support 8-byte
ones (since the opcode is undefined in 64-bit mode). Simplify the code
accordingly.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1996,13 +1996,10 @@ x86_emulate(
if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
&dst.val, op_bytes, ctxt, ops)) != 0 )
goto done;
- switch ( op_bytes )
- {
- case 1: *(uint8_t *)regs[i] = (uint8_t)dst.val; break;
- case 2: *(uint16_t *)regs[i] = (uint16_t)dst.val; break;
- case 4: *regs[i] = (uint32_t)dst.val; break; /* 64b: zero-ext */
- case 8: *regs[i] = dst.val; break;
- }
+ if ( op_bytes != 2 )
+ *regs[i] = (uint32_t)dst.val; /* 64b: zero-ext */
+ else
+ *(uint16_t *)regs[i] = (uint16_t)dst.val;
}
break;
}
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/emul: only emulate possibly operand sizes for POPA
2012-11-07 16:08 [PATCH] x86/emul: only emulate possibly operand sizes for POPA Jan Beulich
@ 2012-11-07 17:10 ` Keir Fraser
2012-11-08 7:34 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Keir Fraser @ 2012-11-07 17:10 UTC (permalink / raw)
To: Jan Beulich, xen-devel
On 07/11/2012 16:08, "Jan Beulich" <JBeulich@suse.com> wrote:
> This opcode neither support 1-byte operands, nor does it support 8-byte
> ones (since the opcode is undefined in 64-bit mode). Simplify the code
> accordingly.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1996,13 +1996,10 @@ x86_emulate(
> if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
> &dst.val, op_bytes, ctxt, ops)) != 0 )
> goto done;
> - switch ( op_bytes )
> - {
> - case 1: *(uint8_t *)regs[i] = (uint8_t)dst.val; break;
> - case 2: *(uint16_t *)regs[i] = (uint16_t)dst.val; break;
> - case 4: *regs[i] = (uint32_t)dst.val; break; /* 64b: zero-ext */
> - case 8: *regs[i] = dst.val; break;
> - }
> + if ( op_bytes != 2 )
> + *regs[i] = (uint32_t)dst.val; /* 64b: zero-ext */
> + else
> + *(uint16_t *)regs[i] = (uint16_t)dst.val;
Would prefer:
if ( op_bytes == 2 )
*(uint16_t *)regs[i] = (uint16_t)dst.val;
else
*regs[i] = dst.val;
Handles the exceptional case immediately after its predicate. And the cast
from uint32_t, and 64b-related comment, are pointless and in fact misleading
in the default case, since as you say the instruction is invalid in 64-bit
mode.
Apart from that:
Acked-by: Keir Fraser <keir@xen.org>
-- Keir
> }
> break;
> }
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/emul: only emulate possibly operand sizes for POPA
2012-11-07 17:10 ` Keir Fraser
@ 2012-11-08 7:34 ` Jan Beulich
2012-11-08 7:48 ` Keir Fraser
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2012-11-08 7:34 UTC (permalink / raw)
To: Keir Fraser; +Cc: xen-devel
>>> On 07.11.12 at 18:10, Keir Fraser <keir@xen.org> wrote:
> On 07/11/2012 16:08, "Jan Beulich" <JBeulich@suse.com> wrote:
>
>> This opcode neither support 1-byte operands, nor does it support 8-byte
>> ones (since the opcode is undefined in 64-bit mode). Simplify the code
>> accordingly.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -1996,13 +1996,10 @@ x86_emulate(
>> if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
>> &dst.val, op_bytes, ctxt, ops)) != 0 )
>> goto done;
>> - switch ( op_bytes )
>> - {
>> - case 1: *(uint8_t *)regs[i] = (uint8_t)dst.val; break;
>> - case 2: *(uint16_t *)regs[i] = (uint16_t)dst.val; break;
>> - case 4: *regs[i] = (uint32_t)dst.val; break; /* 64b: zero-ext */
>> - case 8: *regs[i] = dst.val; break;
>> - }
>> + if ( op_bytes != 2 )
>> + *regs[i] = (uint32_t)dst.val; /* 64b: zero-ext */
>> + else
>> + *(uint16_t *)regs[i] = (uint16_t)dst.val;
>
> Would prefer:
> if ( op_bytes == 2 )
> *(uint16_t *)regs[i] = (uint16_t)dst.val;
> else
> *regs[i] = dst.val;
>
> Handles the exceptional case immediately after its predicate.
I had it that way first, but compilers tend to prefer (in terms of
static branch prediction) the if() body over the else one. Doesn't
matter that much here of course, but I'm generally trying to
follow such guidelines even in non performance critical paths so
that in case code gets cloned elsewhere it doesn't require extra
reviewing or adjustment.
> And the cast
> from uint32_t, and 64b-related comment, are pointless and in fact misleading
> in the default case, since as you say the instruction is invalid in 64-bit
> mode.
And I considered that aspect too: Even if invalid in 64-bit mode, it
is valid in compatibility mode, and in that case the zero-extension
makes sense (as does the comment).
Jan
> Apart from that:
> Acked-by: Keir Fraser <keir@xen.org>
>
> -- Keir
>
>> }
>> break;
>> }
>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/emul: only emulate possibly operand sizes for POPA
2012-11-08 7:34 ` Jan Beulich
@ 2012-11-08 7:48 ` Keir Fraser
2012-11-08 8:34 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Keir Fraser @ 2012-11-08 7:48 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On 08/11/2012 07:34, "Jan Beulich" <JBeulich@suse.com> wrote:
>> Would prefer:
>> if ( op_bytes == 2 )
>> *(uint16_t *)regs[i] = (uint16_t)dst.val;
>> else
>> *regs[i] = dst.val;
>>
>> Handles the exceptional case immediately after its predicate.
>
> I had it that way first, but compilers tend to prefer (in terms of
> static branch prediction) the if() body over the else one. Doesn't
> matter that much here of course, but I'm generally trying to
> follow such guidelines even in non performance critical paths so
> that in case code gets cloned elsewhere it doesn't require extra
> reviewing or adjustment.
Should follow such guidelines where the optimisation matters. I think
shaping code to follow such guidelines all the time, is misguided. I'd
rather have the fractionally more readable version than the
possibly-fractionally faster version.
>> And the cast
>> from uint32_t, and 64b-related comment, are pointless and in fact misleading
>> in the default case, since as you say the instruction is invalid in 64-bit
>> mode.
>
> And I considered that aspect too: Even if invalid in 64-bit mode, it
> is valid in compatibility mode, and in that case the zero-extension
> makes sense (as does the comment).
I did wonder. The top halves of 64b registers are not used in compatibility
mode. Are their contents at all guaranteed to be
maintained/updated/preserved in any meaningful way across transitions into
and out of compatibility mode? I wasn't aware they were, and in that case
the cast and comment are indeed pointless.
-- Keir
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/emul: only emulate possibly operand sizes for POPA
2012-11-08 7:48 ` Keir Fraser
@ 2012-11-08 8:34 ` Jan Beulich
2012-11-08 9:08 ` Keir Fraser
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2012-11-08 8:34 UTC (permalink / raw)
To: Keir Fraser; +Cc: xen-devel
>>> On 08.11.12 at 08:48, Keir Fraser <keir.xen@gmail.com> wrote:
> On 08/11/2012 07:34, "Jan Beulich" <JBeulich@suse.com> wrote:
>
>>> Would prefer:
>>> if ( op_bytes == 2 )
>>> *(uint16_t *)regs[i] = (uint16_t)dst.val;
>>> else
>>> *regs[i] = dst.val;
>>>
>>> Handles the exceptional case immediately after its predicate.
>>
>> I had it that way first, but compilers tend to prefer (in terms of
>> static branch prediction) the if() body over the else one. Doesn't
>> matter that much here of course, but I'm generally trying to
>> follow such guidelines even in non performance critical paths so
>> that in case code gets cloned elsewhere it doesn't require extra
>> reviewing or adjustment.
>
> Should follow such guidelines where the optimisation matters. I think
> shaping code to follow such guidelines all the time, is misguided. I'd
> rather have the fractionally more readable version than the
> possibly-fractionally faster version.
Okay, will adjust accordingly then.
>>> And the cast
>>> from uint32_t, and 64b-related comment, are pointless and in fact misleading
>>> in the default case, since as you say the instruction is invalid in 64-bit
>>> mode.
>>
>> And I considered that aspect too: Even if invalid in 64-bit mode, it
>> is valid in compatibility mode, and in that case the zero-extension
>> makes sense (as does the comment).
>
> I did wonder. The top halves of 64b registers are not used in compatibility
> mode. Are their contents at all guaranteed to be
> maintained/updated/preserved in any meaningful way across transitions into
> and out of compatibility mode? I wasn't aware they were, and in that case
> the cast and comment are indeed pointless.
There's no architectural guarantee, but that's how CPUs work.
The important aspect (from an information leak perspective) is
that upper halves don't get zeroed explicitly when switching
between compatibility and 64-bit modes.
Now we can of course utilize that read_ulong() already does the
zero extension (but if it didn't, we would leak stack contents here,
so it may still be worth a comment), to the net effect of
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1996,13 +1996,11 @@ x86_emulate(
if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
&dst.val, op_bytes, ctxt, ops)) != 0 )
goto done;
- switch ( op_bytes )
- {
- case 1: *(uint8_t *)regs[i] = (uint8_t)dst.val; break;
- case 2: *(uint16_t *)regs[i] = (uint16_t)dst.val; break;
- case 4: *regs[i] = (uint32_t)dst.val; break; /* 64b: zero-ext */
- case 8: *regs[i] = dst.val; break;
- }
+ if ( op_bytes == 2 )
+ *(uint16_t *)regs[i] = (uint16_t)dst.val;
+ else
+ /* No need for zero-extension - read_ulong() already did so. */
+ *regs[i] = dst.val;
}
break;
}
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/emul: only emulate possibly operand sizes for POPA
2012-11-08 8:34 ` Jan Beulich
@ 2012-11-08 9:08 ` Keir Fraser
0 siblings, 0 replies; 6+ messages in thread
From: Keir Fraser @ 2012-11-08 9:08 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On 08/11/2012 08:34, "Jan Beulich" <JBeulich@suse.com> wrote:
>> I did wonder. The top halves of 64b registers are not used in compatibility
>> mode. Are their contents at all guaranteed to be
>> maintained/updated/preserved in any meaningful way across transitions into
>> and out of compatibility mode? I wasn't aware they were, and in that case
>> the cast and comment are indeed pointless.
>
> There's no architectural guarantee, but that's how CPUs work.
> The important aspect (from an information leak perspective) is
> that upper halves don't get zeroed explicitly when switching
> between compatibility and 64-bit modes.
>
> Now we can of course utilize that read_ulong() already does the
> zero extension (but if it didn't, we would leak stack contents here,
> so it may still be worth a comment), to the net effect of
The concern over information leak is fair. Just leave the code line as-is
then with the explicit cast and end-of-line comment.
K.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-11-08 9:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-07 16:08 [PATCH] x86/emul: only emulate possibly operand sizes for POPA Jan Beulich
2012-11-07 17:10 ` Keir Fraser
2012-11-08 7:34 ` Jan Beulich
2012-11-08 7:48 ` Keir Fraser
2012-11-08 8:34 ` Jan Beulich
2012-11-08 9:08 ` Keir Fraser
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.