Jan Beulich
2006-Nov-21 16:55 UTC
[Xen-devel] [PATCH] support protected mode mmio with non-zero CS base
This helps newer isolinux'' graphical boot code (which crashes without
this).
Signed-off-by: Jan Beulich <jbeulich@novell.com>
Index: 2006-11-17/xen/arch/x86/hvm/platform.c
==================================================================---
2006-11-17.orig/xen/arch/x86/hvm/platform.c 2006-10-16 20:56:36.000000000 +0200
+++ 2006-11-17/xen/arch/x86/hvm/platform.c 2006-11-21 13:29:03.000000000 +0100
@@ -895,9 +895,10 @@ void handle_mmio(unsigned long va, unsig
realmode = hvm_realmode(v);
if ( realmode )
- inst_addr = (regs->cs << 4) + regs->eip;
+ inst_addr = regs->cs << 4;
else
- inst_addr = regs->eip;
+ inst_addr = hvm_get_segment_base(current, seg_cs);
+ inst_addr += regs->eip;
memset(inst, 0, MAX_INST_LEN);
if ( inst_copy_from_guest(inst, inst_addr, inst_len) != inst_len ) {
Index: 2006-11-17/xen/arch/x86/hvm/svm/svm.c
==================================================================---
2006-11-17.orig/xen/arch/x86/hvm/svm/svm.c 2006-11-21 11:29:10.000000000 +0100
+++ 2006-11-17/xen/arch/x86/hvm/svm/svm.c 2006-11-21 13:22:35.000000000 +0100
@@ -510,6 +510,24 @@ unsigned long svm_get_ctrl_reg(struct vc
return 0; /* dummy */
}
+static unsigned long svm_get_segment_base(struct vcpu *v, enum segment seg)
+{
+ switch ( seg )
+ {
+ case seg_cs: return v->arch.hvm_svm.vmcb->cs.base;
+ case seg_ds: return v->arch.hvm_svm.vmcb->ds.base;
+ case seg_es: return v->arch.hvm_svm.vmcb->es.base;
+ case seg_fs: return v->arch.hvm_svm.vmcb->fs.base;
+ case seg_gs: return v->arch.hvm_svm.vmcb->gs.base;
+ case seg_ss: return v->arch.hvm_svm.vmcb->ss.base;
+ case seg_tr: return v->arch.hvm_svm.vmcb->tr.base;
+ case seg_gdtr: return v->arch.hvm_svm.vmcb->gdtr.base;
+ case seg_idtr: return v->arch.hvm_svm.vmcb->idtr.base;
+ case seg_ldtr: return v->arch.hvm_svm.vmcb->ldtr.base;
+ }
+ BUG();
+ return 0;
+}
/* Make sure that xen intercepts any FP accesses from current */
static void svm_stts(struct vcpu *v)
@@ -821,6 +839,7 @@ int start_svm(void)
hvm_funcs.pae_enabled = svm_pae_enabled;
hvm_funcs.guest_x86_mode = svm_guest_x86_mode;
hvm_funcs.get_guest_ctrl_reg = svm_get_ctrl_reg;
+ hvm_funcs.get_segment_base = svm_get_segment_base;
hvm_funcs.update_host_cr3 = svm_update_host_cr3;
Index: 2006-11-17/xen/arch/x86/hvm/vmx/vmx.c
==================================================================---
2006-11-17.orig/xen/arch/x86/hvm/vmx/vmx.c 2006-11-21 11:29:10.000000000 +0100
+++ 2006-11-17/xen/arch/x86/hvm/vmx/vmx.c 2006-11-21 13:27:45.000000000 +0100
@@ -501,6 +501,28 @@ static unsigned long vmx_get_ctrl_reg(st
return 0; /* dummy */
}
+static unsigned long vmx_get_segment_base(struct vcpu *v, enum segment seg)
+{
+ unsigned long base;
+
+ BUG_ON(v != current);
+ switch ( seg )
+ {
+ case seg_cs: __vmread(GUEST_CS_BASE, &base); break;
+ case seg_ds: __vmread(GUEST_DS_BASE, &base); break;
+ case seg_es: __vmread(GUEST_ES_BASE, &base); break;
+ case seg_fs: __vmread(GUEST_FS_BASE, &base); break;
+ case seg_gs: __vmread(GUEST_GS_BASE, &base); break;
+ case seg_ss: __vmread(GUEST_SS_BASE, &base); break;
+ case seg_tr: __vmread(GUEST_TR_BASE, &base); break;
+ case seg_gdtr: __vmread(GUEST_GDTR_BASE, &base); break;
+ case seg_idtr: __vmread(GUEST_IDTR_BASE, &base); break;
+ case seg_ldtr: __vmread(GUEST_LDTR_BASE, &base); break;
+ default: BUG(); base = 0; break;
+ }
+ return base;
+}
+
/* Make sure that xen intercepts any FP accesses from current */
static void vmx_stts(struct vcpu *v)
{
@@ -619,6 +640,7 @@ static void vmx_setup_hvm_funcs(void)
hvm_funcs.pae_enabled = vmx_pae_enabled;
hvm_funcs.guest_x86_mode = vmx_guest_x86_mode;
hvm_funcs.get_guest_ctrl_reg = vmx_get_ctrl_reg;
+ hvm_funcs.get_segment_base = vmx_get_segment_base;
hvm_funcs.update_host_cr3 = vmx_update_host_cr3;
Index: 2006-11-17/xen/include/asm-x86/hvm/hvm.h
==================================================================---
2006-11-17.orig/xen/include/asm-x86/hvm/hvm.h 2006-09-27 21:51:56.000000000
+0200
+++ 2006-11-17/xen/include/asm-x86/hvm/hvm.h 2006-11-21 13:09:55.000000000 +0100
@@ -20,6 +20,19 @@
#ifndef __ASM_X86_HVM_HVM_H__
#define __ASM_X86_HVM_HVM_H__
+enum segment {
+ seg_cs,
+ seg_ss,
+ seg_ds,
+ seg_es,
+ seg_fs,
+ seg_gs,
+ seg_tr,
+ seg_ldtr,
+ seg_gdtr,
+ seg_idtr
+};
+
/*
* The hardware virtual machine (HVM) interface abstracts away from the
* x86/x86_64 CPU virtualization assist specifics. Currently this interface
@@ -52,6 +65,7 @@ struct hvm_function_table {
* 1) determine whether the guest is in real or vm8086 mode,
* 2) determine whether paging is enabled,
* 3) return the current guest control-register value
+ * 4) return the current guest segment descriptor base
*/
int (*realmode)(struct vcpu *v);
int (*paging_enabled)(struct vcpu *v);
@@ -59,6 +73,7 @@ struct hvm_function_table {
int (*pae_enabled)(struct vcpu *v);
int (*guest_x86_mode)(struct vcpu *v);
unsigned long (*get_guest_ctrl_reg)(struct vcpu *v, unsigned int num);
+ unsigned long (*get_segment_base)(struct vcpu *v, enum segment seg);
/*
* Re-set the value of CR3 that Xen runs on when handling VM exits
@@ -161,6 +186,12 @@ hvm_get_guest_ctrl_reg(struct vcpu *v, u
return 0; /* force to fail */
}
+static inline unsigned long
+hvm_get_segment_base(struct vcpu *v, enum segment seg)
+{
+ return hvm_funcs.get_segment_base(v, seg);
+}
+
void hvm_stts(struct vcpu *v);
void hvm_set_guest_time(struct vcpu *v, u64 gtime);
void hvm_freeze_time(struct vcpu *v);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Petersson, Mats
2006-Nov-21 17:03 UTC
RE: [Xen-devel] [PATCH] support protected mode mmio with non-zero CS base
> -----Original Message----- > From: xen-devel-bounces@lists.xensource.com > [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of > Jan Beulich > Sent: 21 November 2006 16:55 > To: xen-devel@lists.xensource.com > Subject: [Xen-devel] [PATCH] support protected mode mmio with > non-zero CS base > > This helps newer isolinux'' graphical boot code (which crashes > without this). > > Signed-off-by: Jan Beulich <jbeulich@novell.com> > > Index: 2006-11-17/xen/arch/x86/hvm/platform.c > ==================================================================> --- 2006-11-17.orig/xen/arch/x86/hvm/platform.c > 2006-10-16 20:56:36.000000000 +0200 > +++ 2006-11-17/xen/arch/x86/hvm/platform.c 2006-11-21 > 13:29:03.000000000 +0100 > @@ -895,9 +895,10 @@ void handle_mmio(unsigned long va, unsig > > realmode = hvm_realmode(v); > if ( realmode ) > - inst_addr = (regs->cs << 4) + regs->eip; > + inst_addr = regs->cs << 4; > else > - inst_addr = regs->eip; > + inst_addr = hvm_get_segment_base(current, seg_cs);Remove the "if ( realmode ) " and just use the segment base address. The base-address in the register should be correct even in realmod, or the processor is broken. [I don''t think this code is being executed from vmxassist - if it is, then that''s a different special case!]. Theoretically, you should also check that (eip <= segment.limit), and issue GP-fault if not true. -- Mats> + inst_addr += regs->eip; > > memset(inst, 0, MAX_INST_LEN); > if ( inst_copy_from_guest(inst, inst_addr, inst_len) != > inst_len ) { > Index: 2006-11-17/xen/arch/x86/hvm/svm/svm.c > ==================================================================> --- 2006-11-17.orig/xen/arch/x86/hvm/svm/svm.c > 2006-11-21 11:29:10.000000000 +0100 > +++ 2006-11-17/xen/arch/x86/hvm/svm/svm.c 2006-11-21 > 13:22:35.000000000 +0100 > @@ -510,6 +510,24 @@ unsigned long svm_get_ctrl_reg(struct vc > return 0; /* dummy */ > } > > +static unsigned long svm_get_segment_base(struct vcpu *v, > enum segment seg) > +{ > + switch ( seg ) > + { > + case seg_cs: return v->arch.hvm_svm.vmcb->cs.base; > + case seg_ds: return v->arch.hvm_svm.vmcb->ds.base; > + case seg_es: return v->arch.hvm_svm.vmcb->es.base; > + case seg_fs: return v->arch.hvm_svm.vmcb->fs.base; > + case seg_gs: return v->arch.hvm_svm.vmcb->gs.base; > + case seg_ss: return v->arch.hvm_svm.vmcb->ss.base; > + case seg_tr: return v->arch.hvm_svm.vmcb->tr.base; > + case seg_gdtr: return v->arch.hvm_svm.vmcb->gdtr.base; > + case seg_idtr: return v->arch.hvm_svm.vmcb->idtr.base; > + case seg_ldtr: return v->arch.hvm_svm.vmcb->ldtr.base; > + } > + BUG(); > + return 0; > +} > > /* Make sure that xen intercepts any FP accesses from current */ > static void svm_stts(struct vcpu *v) > @@ -821,6 +839,7 @@ int start_svm(void) > hvm_funcs.pae_enabled = svm_pae_enabled; > hvm_funcs.guest_x86_mode = svm_guest_x86_mode; > hvm_funcs.get_guest_ctrl_reg = svm_get_ctrl_reg; > + hvm_funcs.get_segment_base = svm_get_segment_base; > > hvm_funcs.update_host_cr3 = svm_update_host_cr3; > > Index: 2006-11-17/xen/arch/x86/hvm/vmx/vmx.c > ==================================================================> --- 2006-11-17.orig/xen/arch/x86/hvm/vmx/vmx.c > 2006-11-21 11:29:10.000000000 +0100 > +++ 2006-11-17/xen/arch/x86/hvm/vmx/vmx.c 2006-11-21 > 13:27:45.000000000 +0100 > @@ -501,6 +501,28 @@ static unsigned long vmx_get_ctrl_reg(st > return 0; /* dummy */ > } > > +static unsigned long vmx_get_segment_base(struct vcpu *v, > enum segment seg) > +{ > + unsigned long base; > + > + BUG_ON(v != current); > + switch ( seg ) > + { > + case seg_cs: __vmread(GUEST_CS_BASE, &base); break; > + case seg_ds: __vmread(GUEST_DS_BASE, &base); break; > + case seg_es: __vmread(GUEST_ES_BASE, &base); break; > + case seg_fs: __vmread(GUEST_FS_BASE, &base); break; > + case seg_gs: __vmread(GUEST_GS_BASE, &base); break; > + case seg_ss: __vmread(GUEST_SS_BASE, &base); break; > + case seg_tr: __vmread(GUEST_TR_BASE, &base); break; > + case seg_gdtr: __vmread(GUEST_GDTR_BASE, &base); break; > + case seg_idtr: __vmread(GUEST_IDTR_BASE, &base); break; > + case seg_ldtr: __vmread(GUEST_LDTR_BASE, &base); break; > + default: BUG(); base = 0; break; > + } > + return base; > +} > + > /* Make sure that xen intercepts any FP accesses from current */ > static void vmx_stts(struct vcpu *v) > { > @@ -619,6 +640,7 @@ static void vmx_setup_hvm_funcs(void) > hvm_funcs.pae_enabled = vmx_pae_enabled; > hvm_funcs.guest_x86_mode = vmx_guest_x86_mode; > hvm_funcs.get_guest_ctrl_reg = vmx_get_ctrl_reg; > + hvm_funcs.get_segment_base = vmx_get_segment_base; > > hvm_funcs.update_host_cr3 = vmx_update_host_cr3; > > Index: 2006-11-17/xen/include/asm-x86/hvm/hvm.h > ==================================================================> --- 2006-11-17.orig/xen/include/asm-x86/hvm/hvm.h > 2006-09-27 21:51:56.000000000 +0200 > +++ 2006-11-17/xen/include/asm-x86/hvm/hvm.h 2006-11-21 > 13:09:55.000000000 +0100 > @@ -20,6 +20,19 @@ > #ifndef __ASM_X86_HVM_HVM_H__ > #define __ASM_X86_HVM_HVM_H__ > > +enum segment { > + seg_cs, > + seg_ss, > + seg_ds, > + seg_es, > + seg_fs, > + seg_gs, > + seg_tr, > + seg_ldtr, > + seg_gdtr, > + seg_idtr > +}; > + > /* > * The hardware virtual machine (HVM) interface abstracts > away from the > * x86/x86_64 CPU virtualization assist specifics. Currently > this interface > @@ -52,6 +65,7 @@ struct hvm_function_table { > * 1) determine whether the guest is in real or vm8086 mode, > * 2) determine whether paging is enabled, > * 3) return the current guest control-register value > + * 4) return the current guest segment descriptor base > */ > int (*realmode)(struct vcpu *v); > int (*paging_enabled)(struct vcpu *v); > @@ -59,6 +73,7 @@ struct hvm_function_table { > int (*pae_enabled)(struct vcpu *v); > int (*guest_x86_mode)(struct vcpu *v); > unsigned long (*get_guest_ctrl_reg)(struct vcpu *v, > unsigned int num); > + unsigned long (*get_segment_base)(struct vcpu *v, enum > segment seg); > > /* > * Re-set the value of CR3 that Xen runs on when > handling VM exits > @@ -161,6 +186,12 @@ hvm_get_guest_ctrl_reg(struct vcpu *v, u > return 0; /* force to fail */ > } > > +static inline unsigned long > +hvm_get_segment_base(struct vcpu *v, enum segment seg) > +{ > + return hvm_funcs.get_segment_base(v, seg); > +} > + > void hvm_stts(struct vcpu *v); > void hvm_set_guest_time(struct vcpu *v, u64 gtime); > void hvm_freeze_time(struct vcpu *v); > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2006-Nov-22 07:36 UTC
RE: [Xen-devel] [PATCH] support protected mode mmio with non-zero CS base
>> realmode = hvm_realmode(v); >> if ( realmode ) >> - inst_addr = (regs->cs << 4) + regs->eip; >> + inst_addr = regs->cs << 4; >> else >> - inst_addr = regs->eip; >> + inst_addr = hvm_get_segment_base(current, seg_cs); > >Remove the "if ( realmode ) " and just use the segment base address. The >base-address in the register should be correct even in realmod, or the >processor is broken. [I don''t think this code is being executed from >vmxassist - if it is, then that''s a different special case!].I intentionally didn''t, as at least on VMX the read operation could be significantly slower than a shift (and due to the indirect call it will be slower even on SVM).>Theoretically, you should also check that (eip <= segment.limit), and >issue GP-fault if not true.Again intentionally no: If the original instruction managed to generate a page fault, than it must have been entirely within limits - otherwise hardware would have generated a GP fault. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2006-Nov-22 07:51 UTC
Re: [Xen-devel] [PATCH] support protected mode mmio with non-zero CS base
On 22/11/06 7:36 am, "Jan Beulich" <jbeulich@novell.com> wrote:>> Remove the "if ( realmode ) " and just use the segment base address. The >> base-address in the register should be correct even in realmod, or the >> processor is broken. [I don''t think this code is being executed from >> vmxassist - if it is, then that''s a different special case!]. > > I intentionally didn''t, as at least on VMX the read operation could > be significantly slower than a shift (and due to the indirect call it will > be slower even on SVM).We''ll be using the segment base info everywhere in future, otherwise big real mode does not work. Also, realmode is going to be the uncommon case by far here (and we don''t care that much for its performance as long as it doesn''t noticeably suck too badly). -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Petersson, Mats
2006-Nov-22 11:15 UTC
RE: [Xen-devel] [PATCH] support protected mode mmio with non-zero CS base
> -----Original Message----- > From: xen-devel-bounces@lists.xensource.com > [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of > Jan Beulich > Sent: 22 November 2006 07:36 > To: Petersson, Mats > Cc: xen-devel@lists.xensource.com > Subject: RE: [Xen-devel] [PATCH] support protected mode mmio > with non-zero CS base > > >> realmode = hvm_realmode(v); > >> if ( realmode ) > >> - inst_addr = (regs->cs << 4) + regs->eip; > >> + inst_addr = regs->cs << 4; > >> else > >> - inst_addr = regs->eip; > >> + inst_addr = hvm_get_segment_base(current, seg_cs); > > > >Remove the "if ( realmode ) " and just use the segment base > address. The > >base-address in the register should be correct even in > realmod, or the > >processor is broken. [I don''t think this code is being executed from > >vmxassist - if it is, then that''s a different special case!]. > > I intentionally didn''t, as at least on VMX the read operation could > be significantly slower than a shift (and due to the indirect > call it will > be slower even on SVM).Yes, but it''s also possibly incorrect if the machine is in "big realmode", which is an entirely valid way to run code in x86 processors, and I think it''s better to fix it "properly" than to have to fix it again when someone finds another fault in the code, because someone wrote some code differently. The next problem will of course be that data-fetches where the segment base is non-zero. I think the only case where that is likely to happen in mmio is for MOVS instructions, as everything else is presumably using the faulting address to know where the MMIO address is. But I''m OK with not fixing this right now.> > >Theoretically, you should also check that (eip <= segment.limit), and > >issue GP-fault if not true. > > Again intentionally no: If the original instruction managed > to generate > a page fault, than it must have been entirely within limits - > otherwise > hardware would have generated a GP fault.Yes, I agree. -- Mats> > Jan > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2006-Nov-22 11:24 UTC
RE: [Xen-devel] [PATCH] support protected mode mmio with non-zero CS base
>Yes, but it''s also possibly incorrect if the machine is in "big >realmode", which is an entirely valid way to run code in x86 processors, >and I think it''s better to fix it "properly" than to have to fix it >again when someone finds another fault in the code, because someone >wrote some code differently.I meanwhile got convinced by Keir that this is indeed the better way.>The next problem will of course be that data-fetches where the segment >base is non-zero. I think the only case where that is likely to happen >in mmio is for MOVS instructions, as everything else is presumably using >the faulting address to know where the MMIO address is. But I''m OK with >not fixing this right now.I intend to, and I suppose I''ll find more places. The base address use is only one part for string instructions - the more involved (or perhaps better the one causing more overhead) is that for repeated ones, we''ll now have to do limit checks, as the CPU will have checked only the first item. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2006-Nov-22 11:44 UTC
Re: [Xen-devel] [PATCH] support protected mode mmio with non-zero CS base
On 22/11/06 11:24, "Jan Beulich" <jbeulich@novell.com> wrote:>> Yes, but it''s also possibly incorrect if the machine is in "big >> realmode", which is an entirely valid way to run code in x86 processors, >> and I think it''s better to fix it "properly" than to have to fix it >> again when someone finds another fault in the code, because someone >> wrote some code differently. > > I meanwhile got convinced by Keir that this is indeed the better way.Actually I disagree with you, but the patch was a strict improvement so I checked it in as-is. But the realmode-specific path will go away in a future patch, I''m quite sure. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2006-Nov-22 12:46 UTC
Re: [Xen-devel] [PATCH] support protected mode mmio with non-zero CS base
>>> Keir Fraser <keir@xensource.com> 22.11.06 12:44 >>> >On 22/11/06 11:24, "Jan Beulich" <jbeulich@novell.com> wrote: >>> Yes, but it''s also possibly incorrect if the machine is in "big >>> realmode", which is an entirely valid way to run code in x86 processors, >>> and I think it''s better to fix it "properly" than to have to fix it >>> again when someone finds another fault in the code, because someone >>> wrote some code differently. >> >> I meanwhile got convinced by Keir that this is indeed the better way. > >Actually I disagree with you, but the patch was a strict improvement so IMaybe a mere misunderstanding - I meant to say that I got convinced to have no real mode specific code there (and I changed my internal patch, which got meanwhile extended quite a bit [will post later once I tested VMX], accordingly).>checked it in as-is. But the realmode-specific path will go away in a future >patch, I''m quite sure.Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel