| Message ID | 1480513841-7565-9-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 30.11.16 at 14:50, <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -2656,6 +2656,8 @@ x86_emulate( > &dst.val, op_bytes, ctxt, ops)) != 0 || > (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 ) > goto done; > + if ( src.val == x86_seg_ss ) > + ctxt->retire.mov_ss = 1; > break; While I don't mind it being done here (i.e. it can have my R-b as is), wouldn't it be even better to put this into load_seg() itself? Jan
On 01/12/16 10:18, Jan Beulich wrote: >>>> On 30.11.16 at 14:50, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -2656,6 +2656,8 @@ x86_emulate( >> &dst.val, op_bytes, ctxt, ops)) != 0 || >> (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 ) >> goto done; >> + if ( src.val == x86_seg_ss ) >> + ctxt->retire.mov_ss = 1; >> break; > While I don't mind it being done here (i.e. it can have my R-b as is), > wouldn't it be even better to put this into load_seg() itself? That would cause the mov_ss flag to be incorrectly set for `lss`. ~Andrew
>>> On 01.12.16 at 11:51, <andrew.cooper3@citrix.com> wrote: > On 01/12/16 10:18, Jan Beulich wrote: >>>>> On 30.11.16 at 14:50, <andrew.cooper3@citrix.com> wrote: >>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >>> @@ -2656,6 +2656,8 @@ x86_emulate( >>> &dst.val, op_bytes, ctxt, ops)) != 0 || >>> (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 ) >>> goto done; >>> + if ( src.val == x86_seg_ss ) >>> + ctxt->retire.mov_ss = 1; >>> break; >> While I don't mind it being done here (i.e. it can have my R-b as is), >> wouldn't it be even better to put this into load_seg() itself? > > That would cause the mov_ss flag to be incorrectly set for `lss`. Oh, good point. So as said, Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index 416812e..bacdee6 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -2656,6 +2656,8 @@ x86_emulate( &dst.val, op_bytes, ctxt, ops)) != 0 || (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 ) goto done; + if ( src.val == x86_seg_ss ) + ctxt->retire.mov_ss = 1; break; case 0x0e: /* push %%cs */ @@ -2668,7 +2670,6 @@ x86_emulate( case 0x17: /* pop %%ss */ src.val = x86_seg_ss; - ctxt->retire.mov_ss = 1; goto pop_seg; case 0x1e: /* push %%ds */
The mov_ss retire flag should only be set once load_seg() has returned success. In particular, it should not be set if an exception occured when trying to load %ss. _hvm_emulate_one(), currently the sole user of mov_ss, only consideres it in the case that x86_emulate() returns X86EMUL_OKAY, so this bug isn't actually exposed to guests. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> v3: * New --- xen/arch/x86/x86_emulate/x86_emulate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)