Re: [PATCH] svm: support VMCB cleanbits

[Christoph Egger <Christoph.Egger@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]

On Wednesday 15 December 2010 12:27:51 Tim Deegan wrote:
> Hi,
>
> Thanks for the patch!
>
> At 10:52 +0000 on 15 Dec (1292410374), Christoph Egger wrote:
> > @@ -660,16 +671,17 @@ static void svm_ctxt_switch_to(struct vc
> >
> >  static void svm_do_resume(struct vcpu *v)
> >  {
> > +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> >      bool_t debug_state = v->domain->debugger_attached;
> >
> >      if ( unlikely(v->arch.hvm_vcpu.debug_state_latch != debug_state) )
> >      {
> > -        uint32_t mask = (1U << TRAP_debug) | (1U << TRAP_int3);
> >          v->arch.hvm_vcpu.debug_state_latch = debug_state;
> >          if ( debug_state )
> > -            v->arch.hvm_svm.vmcb->exception_intercepts |= mask;
> > -        else
> > -            v->arch.hvm_svm.vmcb->exception_intercepts &= ~mask;
> > +        {
> > +            svm_set_exception_intercept(vmcb, TRAP_debug);
> > +            svm_set_exception_intercept(vmcb, TRAP_int3);
> > +        }
>
> This seems to change the logic so it doesn't clear the intercepts if
> debug_state == 0.  Is that OK?

No, that's not ok. I fixed that in the new patch.

> More generally, I'm not sure I like having all the VMCB accessor
> functions in files called "cleanbits" -- wouldn't it make sense to have
> all that in the vmcb files so people will see them and know to use them?
> You could rename the actual vmcb fields as well to catch anyone writing
> them directly, e.g. in forward-ported patches.

I renamed the 'svmcleanbits.[ch]' files to 'vmc_funcs.[ch]'

Thanks for your review.

Signed-off-by: Wei Huang <Wei.Huang2@amd.com>
Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[-- Attachment #2: xen_vmcbcleanbits.diff --]
[-- Type: text/x-diff, Size: 31856 bytes --]

# HG changeset patch
# User cegger
# Date 1292415876 -3600
vmcb cleanbits

diff -r 368321cec32b -r 2de2cdfb49b3 xen/arch/x86/hvm/svm/Makefile
--- a/xen/arch/x86/hvm/svm/Makefile
+++ b/xen/arch/x86/hvm/svm/Makefile
@@ -3,5 +3,6 @@ obj-y += emulate.o
 obj-y += entry.o
 obj-y += intr.o
 obj-y += svm.o
+obj-y += vmcb_funcs.o
 obj-y += vmcb.o
 obj-y += vpmu.o
diff -r 368321cec32b -r 2de2cdfb49b3 xen/arch/x86/hvm/svm/asid.c
--- a/xen/arch/x86/hvm/svm/asid.c
+++ b/xen/arch/x86/hvm/svm/asid.c
@@ -21,6 +21,7 @@
 #include <xen/lib.h>
 #include <xen/perfc.h>
 #include <asm/hvm/svm/asid.h>
+#include <asm/hvm/svm/vmcb_funcs.h>
 #include <asm/amd.h>
 
 void svm_asid_init(struct cpuinfo_x86 *c)
@@ -41,18 +42,19 @@ void svm_asid_init(struct cpuinfo_x86 *c
 asmlinkage void svm_asid_handle_vmrun(void)
 {
     struct vcpu *curr = current;
+    struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
     bool_t need_flush = hvm_asid_handle_vmenter();
 
     /* ASID 0 indicates that ASIDs are disabled. */
     if ( curr->arch.hvm_vcpu.asid == 0 )
     {
-        curr->arch.hvm_svm.vmcb->guest_asid  = 1;
-        curr->arch.hvm_svm.vmcb->tlb_control = 1;
+        svm_set_asid(vmcb, 1);
+        vmcb->tlb_control = 1;
         return;
     }
 
-    curr->arch.hvm_svm.vmcb->guest_asid  = curr->arch.hvm_vcpu.asid;
-    curr->arch.hvm_svm.vmcb->tlb_control = need_flush;
+    svm_set_asid(vmcb, curr->arch.hvm_vcpu.asid);
+    vmcb->tlb_control = need_flush;
 }
 
 /*
diff -r 368321cec32b -r 2de2cdfb49b3 xen/arch/x86/hvm/svm/intr.c
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -32,6 +32,7 @@
 #include <asm/hvm/support.h>
 #include <asm/hvm/vlapic.h>
 #include <asm/hvm/svm/svm.h>
+#include <asm/hvm/svm/vmcb_funcs.h>
 #include <asm/hvm/svm/intr.h>
 #include <xen/event.h>
 #include <xen/kernel.h>
@@ -56,7 +57,7 @@ static void svm_inject_nmi(struct vcpu *
      * SVM does not virtualise the NMI mask, so we emulate it by intercepting
      * the next IRET and blocking NMI injection until the intercept triggers.
      */
-    vmcb->general1_intercepts |= GENERAL1_INTERCEPT_IRET;
+    svm_set_general1_intercept(vmcb, GENERAL1_INTERCEPT_IRET);
 }
     
 static void svm_inject_extint(struct vcpu *v, int vector)
@@ -108,8 +109,8 @@ static void enable_intr_window(struct vc
     intr.fields.vector  = 0;
     intr.fields.prio    = intack.vector >> 4;
     intr.fields.ign_tpr = (intack.source != hvm_intsrc_lapic);
-    vmcb->vintr = intr;
-    vmcb->general1_intercepts |= GENERAL1_INTERCEPT_VINTR;
+    svm_set_vintr(vmcb, vintr, intr);
+    svm_set_general1_intercept(vmcb, GENERAL1_INTERCEPT_VINTR);
 }
 
 asmlinkage void svm_intr_assist(void) 
diff -r 368321cec32b -r 2de2cdfb49b3 xen/arch/x86/hvm/svm/svm.c
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -46,6 +46,7 @@
 #include <asm/hvm/emulate.h>
 #include <asm/hvm/svm/asid.h>
 #include <asm/hvm/svm/svm.h>
+#include <asm/hvm/svm/vmcb_funcs.h>
 #include <asm/hvm/svm/vmcb.h>
 #include <asm/hvm/svm/emulate.h>
 #include <asm/hvm/svm/intr.h>
@@ -115,7 +116,7 @@ static void svm_save_dr(struct vcpu *v)
 
     /* Clear the DR dirty flag and re-enable intercepts for DR accesses. */
     v->arch.hvm_vcpu.flag_dr_dirty = 0;
-    v->arch.hvm_svm.vmcb->dr_intercepts = ~0u;
+    svm_set_dr(vmcb, ~0u);
 
     v->arch.guest_context.debugreg[0] = read_debugreg(0);
     v->arch.guest_context.debugreg[1] = read_debugreg(1);
@@ -133,14 +134,14 @@ static void __restore_debug_registers(st
         return;
 
     v->arch.hvm_vcpu.flag_dr_dirty = 1;
-    vmcb->dr_intercepts = 0;
+    svm_set_dr(vmcb, 0);
 
     write_debugreg(0, v->arch.guest_context.debugreg[0]);
     write_debugreg(1, v->arch.guest_context.debugreg[1]);
     write_debugreg(2, v->arch.guest_context.debugreg[2]);
     write_debugreg(3, v->arch.guest_context.debugreg[3]);
-    vmcb->dr6 = v->arch.guest_context.debugreg[6];
-    vmcb->dr7 = v->arch.guest_context.debugreg[7];
+    svm_set_dr6(vmcb, v->arch.guest_context.debugreg[6]);
+    svm_set_dr7(vmcb, v->arch.guest_context.debugreg[7]);
 }
 
 /*
@@ -229,11 +230,8 @@ static int svm_vmcb_restore(struct vcpu 
     v->arch.hvm_svm.guest_sysenter_eip = c->sysenter_eip;
 
     if ( paging_mode_hap(v->domain) )
-    {
-        vmcb->np_enable = 1;
-        vmcb->g_pat = MSR_IA32_CR_PAT_RESET; /* guest PAT */
-        vmcb->h_cr3 = pagetable_get_paddr(p2m_get_pagetable(p2m));
-    }
+        svm_set_nested_paging(vmcb, 1, MSR_IA32_CR_PAT_RESET, 
+                              pagetable_get_paddr(p2m_get_pagetable(p2m)));
 
     if ( c->pending_valid ) 
     {
@@ -247,6 +245,7 @@ static int svm_vmcb_restore(struct vcpu 
         }
     }
 
+    vmcb->cleanbits.bytes = 0;
     paging_update_paging_modes(v);
 
     return 0;
@@ -307,7 +306,7 @@ static void svm_fpu_enter(struct vcpu *v
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
 
     setup_fpu(v);
-    vmcb->exception_intercepts &= ~(1U << TRAP_no_device);
+    svm_clear_exception_intercept(vmcb, TRAP_no_device);
 }
 
 static void svm_fpu_leave(struct vcpu *v)
@@ -325,8 +324,8 @@ static void svm_fpu_leave(struct vcpu *v
      */
     if ( !(v->arch.hvm_vcpu.guest_cr[0] & X86_CR0_TS) )
     {
-        v->arch.hvm_svm.vmcb->exception_intercepts |= 1U << TRAP_no_device;
-        vmcb->cr0 |= X86_CR0_TS;
+        svm_set_cr0(vmcb, vmcb->cr0 | X86_CR0_TS);
+        svm_set_exception_intercept(vmcb, TRAP_no_device);
     }
 }
 
@@ -351,9 +350,8 @@ static void svm_set_interrupt_shadow(str
     vmcb->interrupt_shadow =
         !!(intr_shadow & (HVM_INTR_SHADOW_MOV_SS|HVM_INTR_SHADOW_STI));
 
-    vmcb->general1_intercepts &= ~GENERAL1_INTERCEPT_IRET;
-    if ( intr_shadow & HVM_INTR_SHADOW_NMI )
-        vmcb->general1_intercepts |= GENERAL1_INTERCEPT_IRET;
+    svm_general1_intercept(vmcb, GENERAL1_INTERCEPT_IRET,
+        (intr_shadow & HVM_INTR_SHADOW_NMI));
 }
 
 static int svm_guest_x86_mode(struct vcpu *v)
@@ -377,6 +375,7 @@ static void svm_update_host_cr3(struct v
 static void svm_update_guest_cr(struct vcpu *v, unsigned int cr)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+    uint64_t value;
 
     switch ( cr )
     {
@@ -391,23 +390,25 @@ static void svm_update_guest_cr(struct v
                 svm_fpu_enter(v);
         }
 
-        vmcb->cr0 = v->arch.hvm_vcpu.guest_cr[0] | hw_cr0_mask;
+        value = v->arch.hvm_vcpu.guest_cr[0] | hw_cr0_mask;
         if ( !paging_mode_hap(v->domain) )
-            vmcb->cr0 |= X86_CR0_PG | X86_CR0_WP;
+            value |= X86_CR0_PG | X86_CR0_WP;
+        svm_set_cr0(vmcb, value);
         break;
     }
     case 2:
-        vmcb->cr2 = v->arch.hvm_vcpu.guest_cr[2];
+        svm_set_cr2(vmcb, v->arch.hvm_vcpu.guest_cr[2]);
         break;
     case 3:
-        vmcb->cr3 = v->arch.hvm_vcpu.hw_cr[3];
+        svm_set_cr3(vmcb, v->arch.hvm_vcpu.hw_cr[3]);
         hvm_asid_flush_vcpu(v);
         break;
     case 4:
-        vmcb->cr4 = HVM_CR4_HOST_MASK;
+        value = HVM_CR4_HOST_MASK;
         if ( paging_mode_hap(v->domain) )
-            vmcb->cr4 &= ~X86_CR4_PAE;
-        vmcb->cr4 |= v->arch.hvm_vcpu.guest_cr[4];
+            value &= ~X86_CR4_PAE;
+        value |= v->arch.hvm_vcpu.guest_cr[4];
+        svm_set_cr4(vmcb, value);
         break;
     default:
         BUG();
@@ -418,10 +419,12 @@ static void svm_update_guest_efer(struct
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     bool_t lma = !!(v->arch.hvm_vcpu.guest_efer & EFER_LMA);
+    uint64_t new_efer;
 
-    vmcb->efer = (v->arch.hvm_vcpu.guest_efer | EFER_SVME) & ~EFER_LME;
+    new_efer = (v->arch.hvm_vcpu.guest_efer | EFER_SVME) & ~EFER_LME;
     if ( lma )
-        vmcb->efer |= EFER_LME;
+        new_efer |= EFER_LME;
+    svm_set_efer(vmcb, new_efer);
 
     /*
      * In legacy mode (EFER.LMA=0) we natively support SYSENTER/SYSEXIT with
@@ -515,6 +518,12 @@ static void svm_set_segment_register(str
 
     switch ( seg )
     {
+    case x86_seg_cs:
+    case x86_seg_ds:
+    case x86_seg_es:
+    case x86_seg_ss: /* cpl */
+        vmcb->cleanbits.fields.seg = 0;
+        break;
     case x86_seg_fs:
     case x86_seg_gs:
     case x86_seg_tr:
@@ -553,12 +562,10 @@ static void svm_set_segment_register(str
         memcpy(&vmcb->tr, reg, sizeof(*reg));
         break;
     case x86_seg_gdtr:
-        vmcb->gdtr.base = reg->base;
-        vmcb->gdtr.limit = (uint16_t)reg->limit;
+        svm_set_gdt(vmcb, reg->base, reg->limit);
         break;
     case x86_seg_idtr:
-        vmcb->idtr.base = reg->base;
-        vmcb->idtr.limit = (uint16_t)reg->limit;
+        svm_set_idt(vmcb, reg->base, reg->limit);
         break;
     case x86_seg_ldtr:
         memcpy(&vmcb->ldtr, reg, sizeof(*reg));
@@ -571,17 +578,19 @@ static void svm_set_segment_register(str
         svm_vmload(vmcb);
 }
 
-static void svm_set_tsc_offset(struct vcpu *v, u64 offset)
+static void svm_set_tsc_offset(struct vcpu *v, uint64_t offset)
 {
-    v->arch.hvm_svm.vmcb->tsc_offset = offset;
+    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+
+    vmcb->tsc_offset = offset;
+    vmcb->cleanbits.fields.intercepts = 0;
 }
 
 static void svm_set_rdtsc_exiting(struct vcpu *v, bool_t enable)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
-    vmcb->general1_intercepts &= ~GENERAL1_INTERCEPT_RDTSC;
-    if ( enable )
-        vmcb->general1_intercepts |= GENERAL1_INTERCEPT_RDTSC;
+
+    svm_general1_intercept(vmcb, GENERAL1_INTERCEPT_RDTSC, enable);
 }
 
 static void svm_init_hypercall_page(struct domain *d, void *hypercall_page)
@@ -626,6 +635,7 @@ static void svm_ctxt_switch_from(struct 
 
 static void svm_ctxt_switch_to(struct vcpu *v)
 {
+    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     int cpu = smp_processor_id();
 
 #ifdef  __x86_64__
@@ -651,7 +661,8 @@ static void svm_ctxt_switch_to(struct vc
     svm_restore_dr(v);
 
     svm_vmsave(per_cpu(root_vmcb, cpu));
-    svm_vmload(v->arch.hvm_svm.vmcb);
+    svm_vmload(vmcb);
+    vmcb->cleanbits.bytes = 0;
     vpmu_load(v);
 
     if ( cpu_has_rdtscp )
@@ -660,16 +671,14 @@ static void svm_ctxt_switch_to(struct vc
 
 static void svm_do_resume(struct vcpu *v) 
 {
+    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     bool_t debug_state = v->domain->debugger_attached;
 
     if ( unlikely(v->arch.hvm_vcpu.debug_state_latch != debug_state) )
     {
-        uint32_t mask = (1U << TRAP_debug) | (1U << TRAP_int3);
         v->arch.hvm_vcpu.debug_state_latch = debug_state;
-        if ( debug_state )
-            v->arch.hvm_svm.vmcb->exception_intercepts |= mask;
-        else
-            v->arch.hvm_svm.vmcb->exception_intercepts &= ~mask;
+        svm_exception_intercept(vmcb, TRAP_debug, debug_state);
+        svm_exception_intercept(vmcb, TRAP_int3, debug_state);
     }
 
     if ( v->arch.hvm_svm.launch_core != smp_processor_id() )
@@ -682,8 +691,8 @@ static void svm_do_resume(struct vcpu *v
     }
 
     /* Reflect the vlapic's TPR in the hardware vtpr */
-    v->arch.hvm_svm.vmcb->vintr.fields.tpr = 
-        (vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI) & 0xFF) >> 4;
+    svm_set_vintr(vmcb, vintr.fields.tpr,
+        (vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI) & 0xFF) >> 4);
 
     hvm_do_resume(v);
     reset_stack_and_jump(svm_asm_do_resume);
@@ -740,7 +749,7 @@ static void svm_inject_exception(
         if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
         {
             __restore_debug_registers(curr);
-            vmcb->dr6 |= 0x4000;
+            svm_set_dr6(vmcb, 0x4000);
         }
     case TRAP_int3:
         if ( curr->domain->debugger_attached )
@@ -770,7 +779,8 @@ static void svm_inject_exception(
 
     if ( trapnr == TRAP_page_fault )
     {
-        vmcb->cr2 = curr->arch.hvm_vcpu.guest_cr[2] = cr2;
+        curr->arch.hvm_vcpu.guest_cr[2] = cr2;
+        svm_set_cr2(vmcb, cr2);
         HVMTRACE_LONG_2D(PF_INJECT, errcode, TRC_PAR_LONG(cr2));
     }
     else
@@ -858,6 +868,10 @@ static int svm_cpu_up(void)
     /* Initialize core's ASID handling. */
     svm_asid_init(c);
 
+    if ( cpu_has_svm_cleanbits )
+        /* The real enablement happens in svm_vmexit_handler() */
+        printk("VMCB cleanbits: Enable mask = 0x%x\n", SVMCLEANBITS_INIT);
+
 #ifdef __x86_64__
     /*
      * Check whether EFER.LMSLE can be written.
@@ -950,7 +964,7 @@ static void svm_fpu_dirty_intercept(void
     svm_fpu_enter(curr);
 
     if ( !(curr->arch.hvm_vcpu.guest_cr[0] & X86_CR0_TS) )
-        vmcb->cr0 &= ~X86_CR0_TS;
+        svm_set_cr0(vmcb, vmcb->cr0 & ~X86_CR0_TS);
 }
 
 #define bitmaskof(idx)  (1U << ((idx) & 31))
@@ -1115,28 +1129,28 @@ static int svm_msr_write_intercept(unsig
         vmcb->debugctlmsr = msr_content;
         if ( !msr_content || !cpu_has_svm_lbrv )
             break;
-        vmcb->lbr_control.fields.enable = 1;
-        svm_disable_intercept_for_msr(v, MSR_IA32_DEBUGCTLMSR);
-        svm_disable_intercept_for_msr(v, MSR_IA32_LASTBRANCHFROMIP);
-        svm_disable_intercept_for_msr(v, MSR_IA32_LASTBRANCHTOIP);
-        svm_disable_intercept_for_msr(v, MSR_IA32_LASTINTFROMIP);
-        svm_disable_intercept_for_msr(v, MSR_IA32_LASTINTTOIP);
+        svm_set_lbr(vmcb, lbr_control.fields.enable, 1);
+        svm_clear_msr_intercept(v, MSR_IA32_DEBUGCTLMSR);
+        svm_clear_msr_intercept(v, MSR_IA32_LASTBRANCHFROMIP);
+        svm_clear_msr_intercept(v, MSR_IA32_LASTBRANCHTOIP);
+        svm_clear_msr_intercept(v, MSR_IA32_LASTINTFROMIP);
+        svm_clear_msr_intercept(v, MSR_IA32_LASTINTTOIP);
         break;
 
     case MSR_IA32_LASTBRANCHFROMIP:
-        vmcb->lastbranchfromip = msr_content;
+        svm_set_lbr(vmcb, lastbranchfromip, msr_content);
         break;
 
     case MSR_IA32_LASTBRANCHTOIP:
-        vmcb->lastbranchtoip = msr_content;
+        svm_set_lbr(vmcb, lastbranchtoip, msr_content);
         break;
 
     case MSR_IA32_LASTINTFROMIP:
-        vmcb->lastintfromip = msr_content;
+        svm_set_lbr(vmcb, lastintfromip, msr_content);
         break;
 
     case MSR_IA32_LASTINTTOIP:
-        vmcb->lastinttoip = msr_content;
+        svm_set_lbr(vmcb, lastinttoip, msr_content);
         break;
 
     case MSR_K7_PERFCTR0:
@@ -1415,6 +1429,11 @@ asmlinkage void svm_vmexit_handler(struc
 
     hvm_maybe_deassert_evtchn_irq();
 
+    if ( cpu_has_svm_cleanbits )
+        vmcb->cleanbits.bytes = SVMCLEANBITS_INIT;
+    else
+        vmcb->cleanbits.bytes = 0;
+
     /* Event delivery caused this intercept? Queue for redelivery. */
     eventinj = vmcb->exitintinfo;
     if ( unlikely(eventinj.fields.v) &&
@@ -1496,8 +1515,8 @@ asmlinkage void svm_vmexit_handler(struc
         break;
 
     case VMEXIT_VINTR:
-        vmcb->vintr.fields.irq = 0;
-        vmcb->general1_intercepts &= ~GENERAL1_INTERCEPT_VINTR;
+        svm_set_vintr(vmcb, vintr.fields.irq, 0);
+        svm_clear_general1_intercept(vmcb, GENERAL1_INTERCEPT_VINTR);
         break;
 
     case VMEXIT_INVD:
@@ -1622,8 +1641,8 @@ asmlinkage void svm_vmexit_handler(struc
          * may inject an NMI before the NMI handler's IRET instruction is
          * retired.
          */
-        vmcb->general1_intercepts &= ~GENERAL1_INTERCEPT_IRET;
         vmcb->interrupt_shadow = 1;
+        svm_clear_general1_intercept(vmcb, GENERAL1_INTERCEPT_IRET);
         break;
 
     case VMEXIT_PAUSE:
@@ -1641,8 +1660,8 @@ asmlinkage void svm_vmexit_handler(struc
     }
 
     /* The exit may have updated the TPR: reflect this in the hardware vtpr */
-    vmcb->vintr.fields.tpr = 
-        (vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI) & 0xFF) >> 4;
+    svm_set_vintr(vmcb, vintr.fields.tpr,
+        (vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI) & 0xFF) >> 4);
 }
 
 asmlinkage void svm_trace_vmentry(void)
diff -r 368321cec32b -r 2de2cdfb49b3 xen/arch/x86/hvm/svm/vmcb.c
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -31,6 +31,7 @@
 #include <asm/hvm/io.h>
 #include <asm/hvm/support.h>
 #include <asm/hvm/svm/svm.h>
+#include <asm/hvm/svm/vmcb_funcs.h>
 #include <asm/hvm/svm/intr.h>
 #include <asm/hvm/svm/asid.h>
 #include <xen/event.h>
@@ -78,37 +79,6 @@ struct host_save_area *alloc_host_save_a
     return hsa;
 }
 
-void svm_intercept_msr(struct vcpu *v, uint32_t msr, int enable)
-{
-    unsigned long *msr_bitmap = v->arch.hvm_svm.msrpm;
-    unsigned long *msr_bit = NULL;
-
-    /*
-     * See AMD64 Programmers Manual, Vol 2, Section 15.10 (MSR-Bitmap Address).
-     */
-    if ( msr <= 0x1fff )
-        msr_bit = msr_bitmap + 0x0000 / BYTES_PER_LONG;
-    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
-        msr_bit = msr_bitmap + 0x0800 / BYTES_PER_LONG;
-    else if ( (msr >= 0xc0010000) && (msr <= 0xc0011fff) )
-        msr_bit = msr_bitmap + 0x1000 / BYTES_PER_LONG;
-
-    BUG_ON(msr_bit == NULL);
-
-    msr &= 0x1fff;
-
-    if ( enable )
-    {
-        __set_bit(msr * 2, msr_bit);
-        __set_bit(msr * 2 + 1, msr_bit);
-    }
-    else
-    {
-        __clear_bit(msr * 2, msr_bit);
-        __clear_bit(msr * 2 + 1, msr_bit);
-    }
-}
-
 static int construct_vmcb(struct vcpu *v)
 {
     struct arch_svm_struct *arch_svm = &v->arch.hvm_svm;
@@ -145,13 +115,13 @@ static int construct_vmcb(struct vcpu *v
         return -ENOMEM;
     memset(arch_svm->msrpm, 0xff, MSRPM_SIZE);
 
-    svm_disable_intercept_for_msr(v, MSR_FS_BASE);
-    svm_disable_intercept_for_msr(v, MSR_GS_BASE);
-    svm_disable_intercept_for_msr(v, MSR_SHADOW_GS_BASE);
-    svm_disable_intercept_for_msr(v, MSR_CSTAR);
-    svm_disable_intercept_for_msr(v, MSR_LSTAR);
-    svm_disable_intercept_for_msr(v, MSR_STAR);
-    svm_disable_intercept_for_msr(v, MSR_SYSCALL_MASK);
+    svm_clear_msr_intercept(v, MSR_FS_BASE);
+    svm_clear_msr_intercept(v, MSR_GS_BASE);
+    svm_clear_msr_intercept(v, MSR_SHADOW_GS_BASE);
+    svm_clear_msr_intercept(v, MSR_CSTAR);
+    svm_clear_msr_intercept(v, MSR_LSTAR);
+    svm_clear_msr_intercept(v, MSR_STAR);
+    svm_clear_msr_intercept(v, MSR_SYSCALL_MASK);
 
     vmcb->msrpm_base_pa = (u64)virt_to_maddr(arch_svm->msrpm);
     vmcb->iopm_base_pa  = (u64)virt_to_maddr(hvm_io_bitmap);
@@ -231,9 +201,9 @@ static int construct_vmcb(struct vcpu *v
 
     if ( paging_mode_hap(v->domain) )
     {
-        vmcb->np_enable = 1; /* enable nested paging */
-        vmcb->g_pat = MSR_IA32_CR_PAT_RESET; /* guest PAT */
-        vmcb->h_cr3 = pagetable_get_paddr(p2m_get_pagetable(p2m_get_hostp2m(v->domain)));
+        struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
+        svm_set_nested_paging(vmcb, 1, MSR_IA32_CR_PAT_RESET, 
+                              pagetable_get_paddr(p2m_get_pagetable(p2m)));
 
         /* No point in intercepting CR3 reads/writes. */
         vmcb->cr_intercepts &= ~(CR_INTERCEPT_CR3_READ|CR_INTERCEPT_CR3_WRITE);
@@ -245,7 +215,7 @@ static int construct_vmcb(struct vcpu *v
         vmcb->general1_intercepts &= ~GENERAL1_INTERCEPT_INVLPG;
 
         /* PAT is under complete control of SVM when using nested paging. */
-        svm_disable_intercept_for_msr(v, MSR_IA32_CR_PAT);
+        svm_clear_msr_intercept(v, MSR_IA32_CR_PAT);
     }
     else
     {
@@ -258,6 +228,8 @@ static int construct_vmcb(struct vcpu *v
         vmcb->general1_intercepts |= GENERAL1_INTERCEPT_PAUSE;
     }
 
+    vmcb->cleanbits.bytes = 0;
+
     return 0;
 }
 
@@ -357,7 +329,8 @@ void svm_dump_vmcb(const char *from, str
     printk("KernGSBase = 0x%016llx PAT = 0x%016llx \n", 
            (unsigned long long) vmcb->kerngsbase,
            (unsigned long long) vmcb->g_pat);
-    printk("H_CR3 = 0x%016llx\n", (unsigned long long)vmcb->h_cr3);
+    printk("H_CR3 = 0x%016llx CleanBits = 0x%08x\n", 
+           (unsigned long long)vmcb->h_cr3, vmcb->cleanbits.bytes);
 
     /* print out all the selectors */
     svm_dump_sel("CS", &vmcb->cs);
diff -r 368321cec32b -r 2de2cdfb49b3 xen/arch/x86/hvm/svm/vmcb_funcs.c
--- /dev/null
+++ b/xen/arch/x86/hvm/svm/vmcb_funcs.c
@@ -0,0 +1,57 @@
+/*
+ * svmcleanbits.c: VMCB cleanbits management
+ * Copyright (c) 2010, Advanced Micro Devices, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ *
+ */
+
+#include <xen/config.h>
+#include <xen/init.h>
+#include <asm/hvm/svm/svm.h>
+#include <asm/hvm/svm/vmcb_funcs.h>
+
+/* VMCB MSR bitmap */
+void
+svm_intercept_msr(struct vcpu *v, uint32_t msr, bool_t enabled)
+{
+    unsigned long *msr_bitmap = v->arch.hvm_svm.msrpm;
+    unsigned long *msr_bit = NULL;
+
+    /*
+     * See AMD64 Programmers Manual, Vol 2, Section 15.10 (MSR-Bitmap Address).
+     */
+    if ( msr <= 0x1fff )
+        msr_bit = msr_bitmap + 0x0000 / BYTES_PER_LONG;
+    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
+        msr_bit = msr_bitmap + 0x0800 / BYTES_PER_LONG;
+    else if ( (msr >= 0xc0010000) && (msr <= 0xc0011fff) )
+        msr_bit = msr_bitmap + 0x1000 / BYTES_PER_LONG;
+
+    BUG_ON(msr_bit == NULL);
+
+    msr &= 0x1fff;
+
+    if ( enabled )
+    {
+        __set_bit(msr * 2, msr_bit);
+        __set_bit(msr * 2 + 1, msr_bit);
+    }
+    else
+    {
+        __clear_bit(msr * 2, msr_bit);
+        __clear_bit(msr * 2 + 1, msr_bit);
+    }
+    v->arch.hvm_svm.vmcb->cleanbits.fields.iopm = 0;
+}
diff -r 368321cec32b -r 2de2cdfb49b3 xen/include/asm-x86/hvm/svm/svm.h
--- a/xen/include/asm-x86/hvm/svm/svm.h
+++ b/xen/include/asm-x86/hvm/svm/svm.h
@@ -68,12 +68,14 @@ extern u32 svm_feature_flags;
 #define SVM_FEATURE_LBRV    1
 #define SVM_FEATURE_SVML    2
 #define SVM_FEATURE_NRIPS   3
+#define SVM_FEATURE_CLEAN   5
 #define SVM_FEATURE_PAUSEF  10
 
 #define cpu_has_svm_npt     test_bit(SVM_FEATURE_NPT, &svm_feature_flags)
 #define cpu_has_svm_lbrv    test_bit(SVM_FEATURE_LBRV, &svm_feature_flags)
 #define cpu_has_svm_svml    test_bit(SVM_FEATURE_SVML, &svm_feature_flags)
 #define cpu_has_svm_nrips   test_bit(SVM_FEATURE_NRIPS, &svm_feature_flags)
+#define cpu_has_svm_cleanbits test_bit(SVM_FEATURE_CLEAN, &svm_feature_flags)
 #define cpu_has_pause_filter  test_bit(SVM_FEATURE_PAUSEF, &svm_feature_flags)
 
 #endif /* __ASM_X86_HVM_SVM_H__ */
diff -r 368321cec32b -r 2de2cdfb49b3 xen/include/asm-x86/hvm/svm/vmcb.h
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -366,6 +366,26 @@ typedef union
     } fields;
 } __attribute__ ((packed)) lbrctrl_t;
 
+typedef union
+{
+    uint32_t bytes;
+    struct
+    {
+        uint32_t intercepts: 1;
+        uint32_t iopm: 1;
+        uint32_t asid: 1;
+        uint32_t tpr: 1;
+        uint32_t np: 1;
+        uint32_t cr: 1;
+        uint32_t dr: 1;
+        uint32_t dt: 1;
+        uint32_t seg: 1;
+        uint32_t cr2: 1;
+        uint32_t lbr: 1;
+        uint32_t resv: 21;
+    } fields;
+} __attribute__ ((packed)) vmcbcleanbits_t;
+
 struct vmcb_struct {
     u32 cr_intercepts;          /* offset 0x00 */
     u32 dr_intercepts;          /* offset 0x04 */
@@ -397,7 +417,8 @@ struct vmcb_struct {
     eventinj_t  eventinj;       /* offset 0xA8 */
     u64 h_cr3;                  /* offset 0xB0 */
     lbrctrl_t lbr_control;      /* offset 0xB8 */
-    u64 res09;                  /* offset 0xC0 */
+    vmcbcleanbits_t cleanbits;  /* offset 0xC0 */
+    u32 res09;                  /* offset 0xC4 */
     u64 nextrip;                /* offset 0xC8 */
     u64 res10a[102];            /* offset 0xD0 pad to save area */
 
@@ -481,11 +502,6 @@ int  svm_create_vmcb(struct vcpu *v);
 void svm_destroy_vmcb(struct vcpu *v);
 
 void setup_vmcb_dump(void);
-
-void svm_intercept_msr(struct vcpu *v, uint32_t msr, int enable);
-#define svm_disable_intercept_for_msr(v, msr) svm_intercept_msr((v), (msr), 0)
-#define svm_enable_intercept_for_msr(v, msr) svm_intercept_msr((v), (msr), 1)
-
 #endif /* ASM_X86_HVM_SVM_VMCS_H__ */
 
 /*
diff -r 368321cec32b -r 2de2cdfb49b3 xen/include/asm-x86/hvm/svm/vmcb_funcs.h
--- /dev/null
+++ b/xen/include/asm-x86/hvm/svm/vmcb_funcs.h
@@ -0,0 +1,192 @@
+/*
+ * svmcleanbits.h: SVM VMCB CleanBits
+ * Copyright (c) 2010, Advanced Micro Devices, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ *
+ */
+
+#ifndef __ASM_X86_HVM_SVMCLEANBITS_H__
+#define __ASM_X86_HVM_SVMCLEANBITS_H__
+
+#include <asm/hvm/svm/vmcb.h>
+
+#define SVMCLEANBITS_INIT    ~0
+
+/* VMCB intercepts: covered by cleanbit 0 
+ * 
+ * The following functions cover VMCB dr_intercepts, exception_intercepts,
+ * general1_intercepts, general2_intercepts and tsc_offset. 
+ */
+#define svm_set_dr(vmcb, value)                 \
+    do {                                        \
+        vmcb->dr_intercepts = value;            \
+        vmcb->cleanbits.fields.intercepts = 0;  \
+    } while (0)
+
+static inline void
+svm_exception_intercept(struct vmcb_struct *vmcb, 
+                        unsigned int bit_pos, bool_t enabled)
+{
+    if (enabled)
+        vmcb->exception_intercepts |= (1U << bit_pos);
+    else
+        vmcb->exception_intercepts &= ~(1U << bit_pos);
+
+    vmcb->cleanbits.fields.intercepts = 0;
+}
+#define svm_set_exception_intercept(v, p)   svm_exception_intercept(v, p, 1)
+#define svm_clear_exception_intercept(v, p) svm_exception_intercept(v, p, 0)
+
+static inline void
+svm_general1_intercept(struct vmcb_struct *vmcb, 
+                       enum GenericIntercept1bits g1, bool_t enabled)
+{
+    if (enabled)
+        vmcb->general1_intercepts |= g1;
+    else
+        vmcb->general1_intercepts &= ~g1;
+
+    vmcb->cleanbits.fields.intercepts = 0;
+}
+#define svm_set_general1_intercept(v, g1)   svm_general1_intercept(v, g1, 1)
+#define svm_clear_general1_intercept(v, g1) svm_general1_intercept(v, g1, 0)
+
+static inline void
+svm_general2_intercept(struct vmcb_struct *vmcb, 
+                       enum GenericIntercept1bits g2, bool_t enabled)
+{
+    if (enabled)
+        vmcb->general2_intercepts |= g2;
+    else
+        vmcb->general2_intercepts &= ~g2;
+
+    vmcb->cleanbits.fields.intercepts = 0;
+}
+#define svm_set_general2_intercept(v, g2)   svm_general2_intercept(v, g2, 1)
+#define svm_clear_general2_intercept(v, g2) svm_general2_intercept(v, g2, 0)
+
+/* tsc_offset is capsulated hvm_funcs.set_tsc_offset(). Please use it. */
+
+/* VMCB I/O MSR permission map: covered by cleanbit 1 
+ * 
+ * The I/O permission map aren't changed. So hypervisor doesn't need to handle
+ * it. As for MSR permission map, its dirty bit is set in svm_intercept_msr().
+ * We define two macros to capsulate it.
+ */
+void svm_intercept_msr(struct vcpu *v, uint32_t msr, bool_t enable);
+#define svm_set_msr_intercept(v, msr) svm_intercept_msr((v), (msr), 1)
+#define svm_clear_msr_intercept(v, msr) svm_intercept_msr((v), (msr), 0)
+
+/* VMCB ASID: covered by cleanbit 2 */
+#define svm_set_asid(vmcb, value)               \
+    do {                                        \
+        if (vmcb->guest_asid != value)          \
+            vmcb->cleanbits.fields.asid = 0;    \
+                                                \
+        vmcb->guest_asid = value;               \
+    } while (0)
+
+/* VMCB VINTR: covered by cleanbit 3 */
+#define svm_set_vintr(vmcb, vintr_field, value) \
+    do {                                        \
+        vmcb->vintr_field = value;              \
+        vmcb->cleanbits.fields.tpr = 0;         \
+    } while (0)
+
+/* VMCB np_enable, h_cr3 and g_pat: covered by cleanbit 4 */
+#define svm_set_nested_paging(vmcb, enabled, guest_pat, host_cr3) \
+    do {                                                          \
+        vmcb->np_enable = enabled;                                \
+        vmcb->h_cr3 = host_cr3;                                   \
+        vmcb->g_pat = guest_pat;                                  \
+                                                                  \
+        vmcb->cleanbits.fields.np = 0;                            \
+    } while (0)
+
+/* VMCB CR0, CR3, CR4 and EFER: covered by cleanbit 5 */
+#define svm_set_cr0(vmcb, value)                \
+    do {                                        \
+        vmcb->cr0 = value;                      \
+        vmcb->cleanbits.fields.cr = 0;          \
+    } while (0)
+
+#define svm_set_cr3(vmcb, value)                \
+    do {                                        \
+        vmcb->cr3 = value;                      \
+        vmcb->cleanbits.fields.cr = 0;          \
+    } while (0)
+
+#define svm_set_cr4(vmcb, value)                \
+    do {                                        \
+        vmcb->cr4 = value;                      \
+        vmcb->cleanbits.fields.cr = 0;          \
+    } while (0)
+
+#define svm_set_efer(vmcb, value)               \
+    do {                                        \
+        vmcb->efer = value;                     \
+        vmcb->cleanbits.fields.cr = 0;          \
+    } while (0)
+
+/* VMCB DR6 and DR7: covered by cleanbit 6 */
+#define svm_set_dr6(vmcb, value)                \
+    do {                                        \
+        vmcb->dr6 = value;                      \
+        vmcb->cleanbits.fields.dr = 0;          \
+    } while (0)
+
+#define svm_set_dr7(vmcb, value)                \
+    do {                                        \
+        vmcb->dr7 = value;                      \
+        vmcb->cleanbits.fields.dr = 0;          \
+    } while (0)
+
+/* VMCB GDT and IDT: covered by cleanbit 7 */
+#define svm_set_gdt(vmcb, base_val, limit_val)  \
+    do {                                        \
+        vmcb->gdtr.base = base_val;             \
+        vmcb->gdtr.limit = (uint16_t)limit_val; \
+                                                \
+        vmcb->cleanbits.fields.dt = 0;          \
+    } while (0)
+
+#define svm_set_idt(vmcb, base_val, limit_val)  \
+    do {                                        \
+        vmcb->idtr.base = base_val;             \
+        vmcb->idtr.limit = (uint16_t)limit_val; \
+                                                \
+        vmcb->cleanbits.fields.dt = 0;          \
+    } while (0)
+
+/* VMCB segments: covered by cleanbit 8 
+ * 
+ * Please refer to hvm_set_segment_register(). The dirty bit was set in that
+ * function. Everyone is encouraged to use that in any case. */
+
+/* VMCB CR2: covered by cleanbit 9 */
+#define svm_set_cr2(vmcb, value)                \
+    do {                                        \
+        vmcb->cr2 = value;                      \
+        vmcb->cleanbits.fields.cr2 = 0;         \
+    } while (0)
+
+/* VMCB LBRV: covered by cleanbit 10 */
+#define svm_set_lbr(vmcb, lbr_field, value)     \
+    do {                                        \
+        vmcb->lbr_field = value;                \
+        vmcb->cleanbits.fields.lbr = 0;         \
+    } while (0)
+
+#endif /* __ASM_X86_HVM_SVMCLEANBITS_H__ */

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel