| Message ID | 581389CB020000780011A9C1@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28/10/16 16:24, Jan Beulich wrote: > Commit 0888d36bb2 ("x86/emul: Correct the decoding of SReg3 operands") > overlooked three places where x86_seg_cs was assumed to be zero. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Sorry for breaking this (especially as I had mentally noted to do something with these loops). > > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -1499,18 +1499,18 @@ static void vmx_update_guest_cr(struct v > /* Entering or leaving real mode: adjust the segment registers. > * Need to read them all either way, as realmode reads can update > * the saved values we'll use when returning to prot mode. */ > - for ( s = x86_seg_cs ; s <= x86_seg_tr ; s++ ) > + for ( s = 0; s <= x86_seg_tr ; s++ ) As you are changing these lines, mind dropping the space between tr and ; ? Alternatively, swapping x86_seg_tr for ARRAY_SIZE(reg) so the indices never get out of sync? Finally, perhaps an extra BUILD_BUG_ON(x86_seg_tr != x86_seg_gs + 1), to cover the expectation of this bit of code? > vmx_get_segment_register(v, s, ®[s]); > v->arch.hvm_vmx.vmx_realmode = realmode; > > if ( realmode ) > { > - for ( s = x86_seg_cs ; s <= x86_seg_tr ; s++ ) > + for ( s = 0; s <= x86_seg_tr ; s++ ) > vmx_set_segment_register(v, s, ®[s]); > } > else > { > - for ( s = x86_seg_cs ; s <= x86_seg_tr ; s++ ) > + for ( s = 0; s <= x86_seg_tr ; s++ ) > if ( !(v->arch.hvm_vmx.vm86_segment_mask & (1<<s)) ) > vmx_set_segment_register( > v, s, &v->arch.hvm_vmx.vm86_saved_seg[s]); > > >
On Fri, Oct 28, 2016 at 04:29:24PM +0100, Andrew Cooper wrote: > On 28/10/16 16:24, Jan Beulich wrote: > > Commit 0888d36bb2 ("x86/emul: Correct the decoding of SReg3 operands") > > overlooked three places where x86_seg_cs was assumed to be zero. > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Release-acked-by: Wei Liu <wei.liu2@citrix.com>
>>> On 28.10.16 at 17:29, <andrew.cooper3@citrix.com> wrote: > On 28/10/16 16:24, Jan Beulich wrote: >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -1499,18 +1499,18 @@ static void vmx_update_guest_cr(struct v >> /* Entering or leaving real mode: adjust the segment registers. >> * Need to read them all either way, as realmode reads can update >> * the saved values we'll use when returning to prot mode. */ >> - for ( s = x86_seg_cs ; s <= x86_seg_tr ; s++ ) >> + for ( s = 0; s <= x86_seg_tr ; s++ ) > > As you are changing these lines, mind dropping the space between tr and ; ? How did I not notice them? > Alternatively, swapping x86_seg_tr for ARRAY_SIZE(reg) so the indices > never get out of sync? > > Finally, perhaps an extra BUILD_BUG_ON(x86_seg_tr != x86_seg_gs + 1), to > cover the expectation of this bit of code? Done both. v2 coming after another smoke test. Jan
--- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1499,18 +1499,18 @@ static void vmx_update_guest_cr(struct v /* Entering or leaving real mode: adjust the segment registers. * Need to read them all either way, as realmode reads can update * the saved values we'll use when returning to prot mode. */ - for ( s = x86_seg_cs ; s <= x86_seg_tr ; s++ ) + for ( s = 0; s <= x86_seg_tr ; s++ ) vmx_get_segment_register(v, s, ®[s]); v->arch.hvm_vmx.vmx_realmode = realmode; if ( realmode ) { - for ( s = x86_seg_cs ; s <= x86_seg_tr ; s++ ) + for ( s = 0; s <= x86_seg_tr ; s++ ) vmx_set_segment_register(v, s, ®[s]); } else { - for ( s = x86_seg_cs ; s <= x86_seg_tr ; s++ ) + for ( s = 0; s <= x86_seg_tr ; s++ ) if ( !(v->arch.hvm_vmx.vm86_segment_mask & (1<<s)) ) vmx_set_segment_register( v, s, &v->arch.hvm_vmx.vm86_saved_seg[s]);
Commit 0888d36bb2 ("x86/emul: Correct the decoding of SReg3 operands") overlooked three places where x86_seg_cs was assumed to be zero. Signed-off-by: Jan Beulich <jbeulich@suse.com> VMX: fix realmode emulation SReg handling Commit 0888d36bb2 ("x86/emul: Correct the decoding of SReg3 operands") overlooked three places where x86_seg_cs was assumed to be zero. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1499,18 +1499,18 @@ static void vmx_update_guest_cr(struct v /* Entering or leaving real mode: adjust the segment registers. * Need to read them all either way, as realmode reads can update * the saved values we'll use when returning to prot mode. */ - for ( s = x86_seg_cs ; s <= x86_seg_tr ; s++ ) + for ( s = 0; s <= x86_seg_tr ; s++ ) vmx_get_segment_register(v, s, ®[s]); v->arch.hvm_vmx.vmx_realmode = realmode; if ( realmode ) { - for ( s = x86_seg_cs ; s <= x86_seg_tr ; s++ ) + for ( s = 0; s <= x86_seg_tr ; s++ ) vmx_set_segment_register(v, s, ®[s]); } else { - for ( s = x86_seg_cs ; s <= x86_seg_tr ; s++ ) + for ( s = 0; s <= x86_seg_tr ; s++ ) if ( !(v->arch.hvm_vmx.vm86_segment_mask & (1<<s)) ) vmx_set_segment_register( v, s, &v->arch.hvm_vmx.vm86_saved_seg[s]);