[Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?

[Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?

[Bill Paul]

[-- Attachment #1: Type: text/plain, Size: 2750 bytes --]

Nobody has commented on this yet. According to my reading of the Intel 
documentation, the SYSRET instruction is supposed to force the RPL bits of the 
%ss register to 3 when returning to user mode. The actual sequence is:

SS.Selector <-- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *)

However, the code in helper_sysret() leaves them at 0 (in other words, the "OR 
3" part of the above sequence is missing). It does set the privilege level 
bits of %cs correctly though.

This has caused me trouble with some of my VxWorks development: code that runs 
okay on real hardware will crash on QEMU, unless I apply the patch below.

Can someone confirm that this is in fact a real bug? The Intel architecture 
manual seems quite clear about the SYSRET behavior. The bug seems to have been 
around as far back as QEMU 0.10.5.

I am using QEMU 2.2.0 on FreeBSD/amd64 9.1-RELEASE.

-Bill

Signed-off-by: Bill Paul <wpaul@windriver.com>

---
 target-i386/seg_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
index fa374d0..2bc757a 100644
--- a/target-i386/seg_helper.c
+++ b/target-i386/seg_helper.c
@@ -1043,7 +1043,7 @@ void helper_sysret(CPUX86State *env, int dflag)
                                    DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);
             env->eip = (uint32_t)env->regs[R_ECX];
         }
-        cpu_x86_load_seg_cache(env, R_SS, selector + 8,
+        cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,
                                0, 0xffffffff,
                                DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
                                DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
@@ -1056,7 +1056,7 @@ void helper_sysret(CPUX86State *env, int dflag)
                                DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
                                DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);
         env->eip = (uint32_t)env->regs[R_ECX];
-        cpu_x86_load_seg_cache(env, R_SS, selector + 8,
+        cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,
                                0, 0xffffffff,
                                DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
                                DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
-- 
1.8.0

-- 
=============================================================================
-Bill Paul            (510) 749-2329 | Senior Member of Technical Staff,
                 wpaul@windriver.com | Master of Unix-Fu - Wind River Systems
=============================================================================
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================

[-- Attachment #2: Type: text/html, Size: 10873 bytes --]

Re: [Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?

[Stefan Weil]

[-- Attachment #1: Type: text/plain, Size: 3229 bytes --]

Hi Bill,

sending text only e-mails might help. Usually git send-email is better 
than using KMail or other mail software.
Use tools like git gui to get a correct signature. Any optional personal 
comments should come directly after
the line with ---. Could you please re-send your patch? Check it before 
sending with scripts/checkpatch.pl.

Maybe this looks like much regulations to fix a bug, but some rules are 
necessary to keep a project
like QEMU alive.

Cc'ing Paolo and Richard because this might be an important bug fix for 
the next release.
(scripts/get_maintainer.pl tells which maintainers should be cc'ed).

Regards
Stefan


Am 09.03.2015 um 23:02 schrieb Bill Paul:
>
> Nobody has commented on this yet. According to my reading of the Intel 
> documentation, the SYSRET instruction is supposed to force the RPL 
> bits of the %ss register to 3 when returning to user mode. The actual 
> sequence is:
>
> SS.Selector <-- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *)
>
> However, the code in helper_sysret() leaves them at 0 (in other words, 
> the "OR 3" part of the above sequence is missing). It does set the 
> privilege level bits of %cs correctly though.
>
> This has caused me trouble with some of my VxWorks development: code 
> that runs okay on real hardware will crash on QEMU, unless I apply the 
> patch below.
>
> Can someone confirm that this is in fact a real bug? The Intel 
> architecture manual seems quite clear about the SYSRET behavior. The 
> bug seems to have been around as far back as QEMU 0.10.5.
>
> I am using QEMU 2.2.0 on FreeBSD/amd64 9.1-RELEASE.
>
> -Bill
>
> Signed-off-by: Bill Paul <wpaul@windriver.com>
>
> ---
>
> target-i386/seg_helper.c | 4 ++--
>
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
>
> index fa374d0..2bc757a 100644
>
> --- a/target-i386/seg_helper.c
>
> +++ b/target-i386/seg_helper.c
>
> @@ -1043,7 +1043,7 @@ void helper_sysret(CPUX86State *env, int dflag)
>
> DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);
>
> env->eip = (uint32_t)env->regs[R_ECX];
>
> }
>
> - cpu_x86_load_seg_cache(env, R_SS, selector + 8,
>
> + cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,
>
> 0, 0xffffffff,
>
> DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
>
> DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
>
> @@ -1056,7 +1056,7 @@ void helper_sysret(CPUX86State *env, int dflag)
>
> DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
>
> DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);
>
> env->eip = (uint32_t)env->regs[R_ECX];
>
> - cpu_x86_load_seg_cache(env, R_SS, selector + 8,
>
> + cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,
>
> 0, 0xffffffff,
>
> DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
>
> DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
>
> -- 
>
> 1.8.0
>
> -- 
>
> =============================================================================
>
> -Bill Paul (510) 749-2329 | Senior Member of Technical Staff,
>
> wpaul@windriver.com | Master of Unix-Fu - Wind River Systems
>
> =============================================================================
>
> "I put a dollar in a change machine. Nothing changed." - George Carlin
>
> =============================================================================
>


[-- Attachment #2: Type: text/html, Size: 10935 bytes --]

Re: [Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?

[Bill Paul]

Of all the gin joints in all the towns in all the world, Stefan Weil had to 
walk into mine at 15:23:36 on Monday 09 March 2015 and say:

> Hi Bill,
> 
> sending text only e-mails might help. Usually git send-email is better
> than using KMail or other mail software.

You know, I thought I was sending in plain text. Somewhere along the line 
KMail lied to me.

> Use tools like git gui to get a correct signature. Any optional personal
> comments should come directly after
> the line with ---.

After? Or before?

> Could you please re-send your patch? Check it before
> sending with scripts/checkpatch.pl.
> 
> Maybe this looks like much regulations to fix a bug, but some rules are
> necessary to keep a project
> like QEMU alive.

I'm going to send it once more because it was entirely my fault that my stupid 
mailer kept sending in HTML mode, but that's it.

-Bill

--
=============================================================================
-Bill Paul            (510) 749-2329 | Senior Member of Technical Staff,
                 wpaul@windriver.com | Master of Unix-Fu - Wind River Systems
=============================================================================
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================

Re: [Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?

[Stefan Weil]

Please see my comment below.

Am 09.03.2015 um 23:46 schrieb Bill Paul:
> Of all the gin joints in all the towns in all the world, Stefan Weil had to
> walk into mine at 15:23:36 on Monday 09 March 2015 and say:
>
>> Hi Bill,
>>
>> sending text only e-mails might help. Usually git send-email is better
>> than using KMail or other mail software.
> You know, I thought I was sending in plain text. Somewhere along the line
> KMail lied to me.

Even when using plain text, mailing programs will often wrap lines and 
or do other nasty things.
Create your patch with "git format-patch", add optional personal 
comments after the ---,
use scripts/get_maintainer.pl and scripts/checkpatch.pl on your patch, 
and then send it
with "git send-email".

>
>> Use tools like git gui to get a correct signature. Any optional personal
>> comments should come directly after
>> the line with ---.
> After? Or before?


See http://patchwork.ozlabs.org/patch/447644/for an example.


>> Could you please re-send your patch? Check it before
>> sending with scripts/checkpatch.pl.
>>
>> Maybe this looks like much regulations to fix a bug, but some rules are
>> necessary to keep a project
>> like QEMU alive.
> I'm going to send it once more because it was entirely my fault that my stupid
> mailer kept sending in HTML mode, but that's it.
>
> -Bill

Cheers
Stefan

[Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?

[Bill Paul]

I'm certain I'm sending this in plain text mode this time. According to my 
reading of the Intel documentation, the SYSRET instruction is supposed to 
force the RPL bits of the %ss register to 3 when returning to user mode. The 
actual sequence is:

SS.Selector <-- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *)

However, the code in helper_sysret() leaves them at 0 (in other words, the "OR 
3" part of the above sequence is missing). It does set the privilege level 
bits of %cs correctly though.

This has caused me trouble with some of my VxWorks development: code that runs 
okay on real hardware will crash on QEMU, unless I apply the patch below.

Can someone confirm that this is in fact a real bug? The Intel architecture 
manual seems quite clear about the SYSRET behavior. The bug seems to have been 
around as far back as QEMU 0.10.5.

I am using QEMU 2.2.0 on FreeBSD/amd64 9.1-RELEASE.

-Bill

Signed-off-by: Bill Paul <wpaul@windriver.com>

---
 target-i386/seg_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
index fa374d0..2bc757a 100644
--- a/target-i386/seg_helper.c
+++ b/target-i386/seg_helper.c
@@ -1043,7 +1043,7 @@ void helper_sysret(CPUX86State *env, int dflag)
                                    DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);
             env->eip = (uint32_t)env->regs[R_ECX];
         }
-        cpu_x86_load_seg_cache(env, R_SS, selector + 8,
+        cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,
                                0, 0xffffffff,
                                DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
                                DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
@@ -1056,7 +1056,7 @@ void helper_sysret(CPUX86State *env, int dflag)
                                DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
                                DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);
         env->eip = (uint32_t)env->regs[R_ECX];
-        cpu_x86_load_seg_cache(env, R_SS, selector + 8,
+        cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,
                                0, 0xffffffff,
                                DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
                                DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
-- 
1.8.0

Re: [Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?

[Fam Zheng]

On Mon, 03/09 15:48, Bill Paul wrote:
> I'm certain I'm sending this in plain text mode this time. According to my 
> reading of the Intel documentation, the SYSRET instruction is supposed to 
> force the RPL bits of the %ss register to 3 when returning to user mode. The 
> actual sequence is:
> 
> SS.Selector <-- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *)
> 
> However, the code in helper_sysret() leaves them at 0 (in other words, the "OR 
> 3" part of the above sequence is missing). It does set the privilege level 
> bits of %cs correctly though.
> 
> This has caused me trouble with some of my VxWorks development: code that runs 
> okay on real hardware will crash on QEMU, unless I apply the patch below.
> 
> Can someone confirm that this is in fact a real bug? The Intel architecture 
> manual seems quite clear about the SYSRET behavior. The bug seems to have been 
> around as far back as QEMU 0.10.5.
> 
> I am using QEMU 2.2.0 on FreeBSD/amd64 9.1-RELEASE.
> 
> -Bill

Reviewed-by: Fam Zheng <famz@redhat.com>

It matches the manual now. Cc'ing maintainers and qemu-stable.

Some trimming may be needed when applying, please follow the guideline [1] when
submitting patches - better to add "[PATCH]" prefix to subject, Cc relevant
people, and leave any unrelated words to the patch itself below the "---" line
so that they don't get committed to the git log, including those in the subject
line.

[1]: http://wiki.qemu-project.org/Contribute/SubmitAPatch

> 
> Signed-off-by: Bill Paul <wpaul@windriver.com>
> 
> ---
>  target-i386/seg_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
> index fa374d0..2bc757a 100644
> --- a/target-i386/seg_helper.c
> +++ b/target-i386/seg_helper.c
> @@ -1043,7 +1043,7 @@ void helper_sysret(CPUX86State *env, int dflag)
>                                     DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);
>              env->eip = (uint32_t)env->regs[R_ECX];
>          }
> -        cpu_x86_load_seg_cache(env, R_SS, selector + 8,
> +        cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,
>                                 0, 0xffffffff,
>                                 DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
>                                 DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
> @@ -1056,7 +1056,7 @@ void helper_sysret(CPUX86State *env, int dflag)
>                                 DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
>                                 DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);
>          env->eip = (uint32_t)env->regs[R_ECX];
> -        cpu_x86_load_seg_cache(env, R_SS, selector + 8,
> +        cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,
>                                 0, 0xffffffff,
>                                 DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
>                                 DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
> -- 
> 1.8.0
> 

Re: [Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?

[Paolo Bonzini]

On 09/03/2015 23:48, Bill Paul wrote:
> I'm certain I'm sending this in plain text mode this time. According to my 
> reading of the Intel documentation, the SYSRET instruction is supposed to 
> force the RPL bits of the %ss register to 3 when returning to user mode. The 
> actual sequence is:
> 
> SS.Selector <-- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *)
> 
> However, the code in helper_sysret() leaves them at 0 (in other words, the "OR 
> 3" part of the above sequence is missing). It does set the privilege level 
> bits of %cs correctly though.
> 
> This has caused me trouble with some of my VxWorks development: code that runs 
> okay on real hardware will crash on QEMU, unless I apply the patch below.
> 
> Can someone confirm that this is in fact a real bug? The Intel architecture 
> manual seems quite clear about the SYSRET behavior. The bug seems to have been 
> around as far back as QEMU 0.10.5.
> 
> I am using QEMU 2.2.0 on FreeBSD/amd64 9.1-RELEASE.
> 
> -Bill
> 
> Signed-off-by: Bill Paul <wpaul@windriver.com>
> 
> ---
>  target-i386/seg_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
> index fa374d0..2bc757a 100644
> --- a/target-i386/seg_helper.c
> +++ b/target-i386/seg_helper.c
> @@ -1043,7 +1043,7 @@ void helper_sysret(CPUX86State *env, int dflag)
>                                     DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);
>              env->eip = (uint32_t)env->regs[R_ECX];
>          }
> -        cpu_x86_load_seg_cache(env, R_SS, selector + 8,
> +        cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,
>                                 0, 0xffffffff,
>                                 DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
>                                 DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
> @@ -1056,7 +1056,7 @@ void helper_sysret(CPUX86State *env, int dflag)
>                                 DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
>                                 DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);
>          env->eip = (uint32_t)env->regs[R_ECX];
> -        cpu_x86_load_seg_cache(env, R_SS, selector + 8,
> +        cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,
>                                 0, 0xffffffff,
>                                 DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
>                                 DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
> 

Applied, thanks.  I will send a pull request today.

Paolo