| Message ID | 1249952431-7764-1-git-send-email-aliguori@us.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/11/2009 04:00 AM, Anthony Liguori wrote: > According to 16.2.5 in the SDM, eflags.vm in the tss is consulted before loading > and new segments. If eflags.vm == 1, then the segments are treated as 16-bit > segments. > > This fixes an invalid vmentry failure in a custom OS that was happening after > a task switch into vm8086 mode. Since the segments were being mistakenly > treated as 32-bit, we loaded garbage state. > > Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> > --- > arch/x86/kvm/x86.c | 23 +++++++++++++++++------ > 1 files changed, 17 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 6263991..48b1be9 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -4050,6 +4050,16 @@ static void save_state_to_tss32(struct kvm_vcpu *vcpu, > tss->ldt_selector = get_segment_selector(vcpu, VCPU_SREG_LDTR); > } > > +static int load_seg_from_tss32(struct kvm_vcpu *vcpu, > + struct tss_segment_32 *tss, > + u16 selector, int type_bits, int seg) > +{ > + if ((tss->eflags& X86_EFLAGS_VM)) > + return kvm_load_realmode_segment(vcpu, selector, seg); > + > + return kvm_load_segment_descriptor(vcpu, selector, type_bits, seg); > +} > + > kvm_load_segment_descriptor() can already load plain segments: if (!(vcpu->arch.cr0 & X86_CR0_PE)) return kvm_load_realmode_segment(vcpu, selector, seg); so we can simplify the patch a bit by extending the check.
Avi Kivity wrote: > On 08/11/2009 04:00 AM, Anthony Liguori wrote: > > kvm_load_segment_descriptor() can already load plain segments: > > if (!(vcpu->arch.cr0 & X86_CR0_PE)) > return kvm_load_realmode_segment(vcpu, selector, seg); > > so we can simplify the patch a bit by extending the check. I went down that road but you need to check the to-be-switched-into tss, which isn't available directly from vcpu so we would have to either pass the tss as a parameter to the function or at least a flag indicating that we're going into vm8086 mode. There are other callers of kvm_load_segment_descriptor that would need updating and it already takes a few extra parameters. That's why I went down this road. Happy to change though if you'd prefer a different approach.
On 08/11/2009 07:45 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> On 08/11/2009 04:00 AM, Anthony Liguori wrote: >> >> kvm_load_segment_descriptor() can already load plain segments: >> >> if (!(vcpu->arch.cr0 & X86_CR0_PE)) >> return kvm_load_realmode_segment(vcpu, selector, seg); >> >> so we can simplify the patch a bit by extending the check. > > I went down that road but you need to check the to-be-switched-into > tss, which isn't available directly from vcpu so we would have to > either pass the tss as a parameter to the function or at least a flag > indicating that we're going into vm8086 mode. > eflags is already updated at this point, so no need for an additional parameter. > There are other callers of kvm_load_segment_descriptor that would need > updating and it already takes a few extra parameters. That's why I > went down this road. The other callers are a good reason to do it in kvm_load_segment_descriptor(), so they don't have to perform the check themselves. In fact with this change your patch will fix a few more bugs.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 6263991..48b1be9 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4050,6 +4050,16 @@ static void save_state_to_tss32(struct kvm_vcpu *vcpu, tss->ldt_selector = get_segment_selector(vcpu, VCPU_SREG_LDTR); } +static int load_seg_from_tss32(struct kvm_vcpu *vcpu, + struct tss_segment_32 *tss, + u16 selector, int type_bits, int seg) +{ + if ((tss->eflags & X86_EFLAGS_VM)) + return kvm_load_realmode_segment(vcpu, selector, seg); + + return kvm_load_segment_descriptor(vcpu, selector, type_bits, seg); +} + static int load_state_from_tss32(struct kvm_vcpu *vcpu, struct tss_segment_32 *tss) { @@ -4070,23 +4080,24 @@ static int load_state_from_tss32(struct kvm_vcpu *vcpu, if (kvm_load_segment_descriptor(vcpu, tss->ldt_selector, 0, VCPU_SREG_LDTR)) return 1; - if (kvm_load_segment_descriptor(vcpu, tss->es, 1, VCPU_SREG_ES)) + if (load_seg_from_tss32(vcpu, tss, tss->es, 1, VCPU_SREG_ES)) return 1; - if (kvm_load_segment_descriptor(vcpu, tss->cs, 9, VCPU_SREG_CS)) + if (load_seg_from_tss32(vcpu, tss, tss->cs, 9, VCPU_SREG_CS)) return 1; - if (kvm_load_segment_descriptor(vcpu, tss->ss, 1, VCPU_SREG_SS)) + if (load_seg_from_tss32(vcpu, tss, tss->ss, 1, VCPU_SREG_SS)) return 1; - if (kvm_load_segment_descriptor(vcpu, tss->ds, 1, VCPU_SREG_DS)) + if (load_seg_from_tss32(vcpu, tss, tss->ds, 1, VCPU_SREG_DS)) return 1; - if (kvm_load_segment_descriptor(vcpu, tss->fs, 1, VCPU_SREG_FS)) + if (load_seg_from_tss32(vcpu, tss, tss->fs, 1, VCPU_SREG_FS)) return 1; - if (kvm_load_segment_descriptor(vcpu, tss->gs, 1, VCPU_SREG_GS)) + if (load_seg_from_tss32(vcpu, tss, tss->gs, 1, VCPU_SREG_GS)) return 1; + return 0; }
According to 16.2.5 in the SDM, eflags.vm in the tss is consulted before loading and new segments. If eflags.vm == 1, then the segments are treated as 16-bit segments. This fixes an invalid vmentry failure in a custom OS that was happening after a task switch into vm8086 mode. Since the segments were being mistakenly treated as 32-bit, we loaded garbage state. Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- arch/x86/kvm/x86.c | 23 +++++++++++++++++------ 1 files changed, 17 insertions(+), 6 deletions(-)