[Qemu-devel] [PATCH]SVM CR8 undefined bug fix

[Qemu-devel] [PATCH]SVM CR8 undefined bug fix

[TeLeMan]

env->cr[8] used by SVM codes was not defined.

http://www.nabble.com/file/p14921864/svm_cr8.patch svm_cr8.patch: 

diff -p -u qemu.orig/target-i386/cpu.h qemu/target-i386/cpu.h
--- qemu.orig/target-i386/cpu.h	Mon Jan 14 11:11:08 2008
+++ qemu/target-i386/cpu.h	Thu Jan 17 23:21:22 2008
@@ -493,7 +493,7 @@ typedef struct CPUX86State {
     SegmentCache gdt; /* only base and limit are used */
     SegmentCache idt; /* only base and limit are used */
 
-    target_ulong cr[5]; /* NOTE: cr1 is unused */
+    target_ulong cr[9]; /* NOTE: cr1,cr5-cr7 are unused */
     uint32_t a20_mask;
 
     /* FPU state */
diff -p -u qemu.orig/target-i386/helper.c qemu/target-i386/helper.c
--- qemu.orig/target-i386/helper.c	Mon Jan 14 11:11:08 2008
+++ qemu/target-i386/helper.c	Thu Jan 17 23:24:04 2008
@@ -2718,6 +2718,7 @@ void helper_movl_crN_T0(int reg)
         break;
     case 8:
         cpu_set_apic_tpr(env, T0);
+        env->cr[8] = T0;
         break;
     default:
         env->cr[reg] = T0;
@@ -4065,6 +4066,7 @@ void helper_vmrun(target_ulong addr)
     int_ctl = ldl_phys(env->vm_vmcb + offsetof(struct vmcb,
control.int_ctl));
     if (int_ctl & V_INTR_MASKING_MASK) {
         env->cr[8] = int_ctl & V_TPR_MASK;
+	cpu_set_apic_tpr(env,env->cr[8]);
         if (env->eflags & IF_MASK)
             env->hflags |= HF_HIF_MASK;
     }
@@ -4376,8 +4378,10 @@ void vmexit(uint64_t exit_code, uint64_t
     cpu_x86_update_cr0(env, ldq_phys(env->vm_hsave + offsetof(struct vmcb,
save.cr0)) | CR0_PE_MASK);
     cpu_x86_update_cr4(env, ldq_phys(env->vm_hsave + offsetof(struct vmcb,
save.cr4)));
     cpu_x86_update_cr3(env, ldq_phys(env->vm_hsave + offsetof(struct vmcb,
save.cr3)));
-    if (int_ctl & V_INTR_MASKING_MASK)
+    if (int_ctl & V_INTR_MASKING_MASK) {
         env->cr[8] = ldq_phys(env->vm_hsave + offsetof(struct vmcb,
save.cr8));
+        cpu_set_apic_tpr(env,env->cr[8]);
+    }
     /* we need to set the efer after the crs so the hidden flags get set
properly */
 #ifdef TARGET_X86_64
     env->efer  = ldq_phys(env->vm_hsave + offsetof(struct vmcb,
save.efer));

-- 
View this message in context: http://www.nabble.com/-PATCH-SVM-CR8-undefined-bug-fix-tp14921864p14921864.html
Sent from the QEMU - Dev mailing list archive at Nabble.com.

Re: [Qemu-devel] [PATCH]SVM CR8 undefined bug fix

[Alexander Graf]

TeLeMan wrote:
> env->cr[8] used by SVM codes was not defined.
>
>   

As far as I remember cr8 is the very same as the TPR, so we only need to
implement one and map the other to the value we want.
My approach was to use the TPR and route the cr8 accesses to the tpr.
Even though I have to admit that this might not be consistent throughout
the code right now.

> http://www.nabble.com/file/p14921864/svm_cr8.patch svm_cr8.patch: 
>
> diff -p -u qemu.orig/target-i386/cpu.h qemu/target-i386/cpu.h
> --- qemu.orig/target-i386/cpu.h	Mon Jan 14 11:11:08 2008
> +++ qemu/target-i386/cpu.h	Thu Jan 17 23:21:22 2008
> @@ -493,7 +493,7 @@ typedef struct CPUX86State {
>      SegmentCache gdt; /* only base and limit are used */
>      SegmentCache idt; /* only base and limit are used */
>  
> -    target_ulong cr[5]; /* NOTE: cr1 is unused */
> +    target_ulong cr[9]; /* NOTE: cr1,cr5-cr7 are unused */
>      uint32_t a20_mask;
>  
>      /* FPU state */
> diff -p -u qemu.orig/target-i386/helper.c qemu/target-i386/helper.c
> --- qemu.orig/target-i386/helper.c	Mon Jan 14 11:11:08 2008
> +++ qemu/target-i386/helper.c	Thu Jan 17 23:24:04 2008
> @@ -2718,6 +2718,7 @@ void helper_movl_crN_T0(int reg)
>          break;
>      case 8:
>          cpu_set_apic_tpr(env, T0);
> +        env->cr[8] = T0;
>          break;
>      default:
>          env->cr[reg] = T0;
> @@ -4065,6 +4066,7 @@ void helper_vmrun(target_ulong addr)
>      int_ctl = ldl_phys(env->vm_vmcb + offsetof(struct vmcb,
> control.int_ctl));
>      if (int_ctl & V_INTR_MASKING_MASK) {
>          env->cr[8] = int_ctl & V_TPR_MASK;
> +	cpu_set_apic_tpr(env,env->cr[8]);
>   

This is a valid catch.

>          if (env->eflags & IF_MASK)
>              env->hflags |= HF_HIF_MASK;
>      }
> @@ -4376,8 +4378,10 @@ void vmexit(uint64_t exit_code, uint64_t
>      cpu_x86_update_cr0(env, ldq_phys(env->vm_hsave + offsetof(struct vmcb,
> save.cr0)) | CR0_PE_MASK);
>      cpu_x86_update_cr4(env, ldq_phys(env->vm_hsave + offsetof(struct vmcb,
> save.cr4)));
>      cpu_x86_update_cr3(env, ldq_phys(env->vm_hsave + offsetof(struct vmcb,
> save.cr3)));
> -    if (int_ctl & V_INTR_MASKING_MASK)
> +    if (int_ctl & V_INTR_MASKING_MASK) {
>          env->cr[8] = ldq_phys(env->vm_hsave + offsetof(struct vmcb,
>   

This too.

> save.cr8));
> +        cpu_set_apic_tpr(env,env->cr[8]);
> +    }
>      /* we need to set the efer after the crs so the hidden flags get set
> properly */
>  #ifdef TARGET_X86_64
>      env->efer  = ldq_phys(env->vm_hsave + offsetof(struct vmcb,
> save.efer));
>
>   

Re: [Qemu-devel] [PATCH]SVM CR8 undefined bug fix

[Robert William Fuller]

Alexander Graf wrote:
> TeLeMan wrote:
>> env->cr[8] used by SVM codes was not defined.
> As far as I remember cr8 is the very same as the TPR, so we only need to
> implement one and map the other to the value we want.
> My approach was to use the TPR and route the cr8 accesses to the tpr.
> Even though I have to admit that this might not be consistent throughout
> the code right now.

Am I to understand this is a TPR report?

Re: [Qemu-devel] [PATCH]SVM CR8 undefined bug fix

[Alexander Graf]

On Jan 17, 2008, at 4:57 PM, Robert William Fuller wrote:

> Alexander Graf wrote:
>> TeLeMan wrote:
>>> env->cr[8] used by SVM codes was not defined.
>> As far as I remember cr8 is the very same as the TPR, so we only  
>> need to
>> implement one and map the other to the value we want.
>> My approach was to use the TPR and route the cr8 accesses to the tpr.
>> Even though I have to admit that this might not be consistent  
>> throughout
>> the code right now.
>
> Am I to understand this is a TPR report?
>
>

The TPR is a register to control, which IRQs get routed. CR8 is the  
very same.

Their only difference is, that the TPR is implemented as an MSR,  
whereas the CR8 is a CPU register. CR8 is only supported on x86_64  
though, while the TPR works on IA32 too.

So actually the TPR is only a mirror of the CR8 values and vice versa.  
Because MSR and CR8 access are equally slow in qemu, we can simply  
always use the TPR and shadow the CR8 to this, so whenever CR8 gets  
read from or written to, the TPR gets updated.

This way we do not break IA32 compatibility (because TPR always works)  
and everything's fine.

Regards,

Alex

Re: [Qemu-devel] [PATCH]SVM CR8 undefined bug fix

[Bernhard Kauer]

On Thu, Jan 17, 2008 at 05:13:31PM +0100, Alexander Graf wrote:
> Their only difference is, that the TPR is implemented as an MSR, whereas 
> the CR8 is a CPU register.

The TPR is currently a memory mapped local Apic register. The
default address is 0xfee00080, according to Intel vol3a chapter 8...


	Bernhard Kauer

Re: [Qemu-devel] [PATCH]SVM CR8 undefined bug fix

[andrzej zaborowski]

On 17/01/2008, Alexander Graf <alex@csgraf.de> wrote:
> TeLeMan wrote:
> > env->cr[8] used by SVM codes was not defined.
>
> As far as I remember cr8 is the very same as the TPR, so we only need to
> implement one and map the other to the value we want.

I committed the original patch to avoid possible corruption of env,
for now, if you have a better patch to remove the confusion between
CR8 and TPR, please rediff on top of cvs.

Thanks