[PATCH] support protected mode mmio with non-zero CS base

[PATCH] support protected mode mmio with non-zero CS base

[Jan Beulich]

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);

RE: [PATCH] support protected mode mmio with non-zero CS base

[Petersson, Mats]

 

> -----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
> 
> 
> 

RE: [PATCH] support protected mode mmio with non-zero CS base

[Jan Beulich]

>>      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

Re: [PATCH] support protected mode mmio with non-zero CS base

[Keir Fraser]

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

RE: [PATCH] support protected mode mmio with non-zero CS base

[Petersson, Mats]

 

> -----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
> 
> 
> 

RE: [PATCH] support protected mode mmio with non-zero CS base

[Jan Beulich]

>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

Re: [PATCH] support protected mode mmio with non-zero CS base

[Keir Fraser]

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

Re: [PATCH] support protected mode mmio with non-zero CS base

[Jan Beulich]

>>> 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 I

Maybe 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