[PATCH v2 0/5] VMX: fix interaction of Viridian emulation with advanced features

[PATCH v2 0/5] VMX: fix interaction of Viridian emulation with advanced features

[Jan Beulich]

1: VMX: fix interaction of APIC-V and Viridian emulation
2: VMX/Viridian: suppress MSR-based APIC suggestion when having APIC-V
3: VMX: suppress pointless indirect calls
4: Viridian: populate CPUID leaf 6
5: Viridian: cleanup

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Split off cleanup parts from patch 1 (into new patch 3). Add new
    patch 5.

[PATCH v2 1/5] VMX: fix interaction of APIC-V and Viridian emulation

[Jan Beulich]

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

Viridian using a synthetic MSR for issuing EOI notifications bypasses
the normal in-processor handling, which would clear
GUEST_INTR_STATUS.SVI. Hence we need to do this in software in order
for future interrupts to get delivered.

Based on analysis by Yang Z Zhang <yang.z.zhang@intel.com>.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Split off cleanup parts to new patch 3.

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -386,6 +386,9 @@ void vlapic_EOI_set(struct vlapic *vlapi
 
     vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]);
 
+    if ( hvm_funcs.handle_eoi )
+        hvm_funcs.handle_eoi(vector);
+
     if ( vlapic_test_and_clear_vector(vector, &vlapic->regs->data[APIC_TMR]) )
         vioapic_update_EOI(vlapic_domain(vlapic), vector);
 
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1502,6 +1502,15 @@ static void vmx_sync_pir_to_irr(struct v
         vlapic_set_vector(i, &vlapic->regs->data[APIC_IRR]);
 }
 
+static void vmx_handle_eoi(u8 vector)
+{
+    unsigned long status = __vmread(GUEST_INTR_STATUS);
+
+    /* We need to clear the SVI field. */
+    status &= VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
+    __vmwrite(GUEST_INTR_STATUS, status);
+}
+
 static struct hvm_function_table __initdata vmx_function_table = {
     .name                 = "VMX",
     .cpu_up_prepare       = vmx_cpu_up_prepare,
@@ -1554,6 +1563,7 @@ static struct hvm_function_table __initd
     .process_isr          = vmx_process_isr,
     .deliver_posted_intr  = vmx_deliver_posted_intr,
     .sync_pir_to_irr      = vmx_sync_pir_to_irr,
+    .handle_eoi           = vmx_handle_eoi,
     .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
 };
 
@@ -1580,7 +1590,10 @@ const struct hvm_function_table * __init
 
         setup_ept_dump();
     }
- 
+
+    if ( !cpu_has_vmx_virtual_intr_delivery )
+        vmx_function_table.handle_eoi = NULL;
+
     if ( cpu_has_vmx_posted_intr_processing )
         alloc_direct_apic_vector(&posted_intr_vector, event_check_interrupt);
     else
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -186,6 +186,7 @@ struct hvm_function_table {
     void (*process_isr)(int isr, struct vcpu *v);
     void (*deliver_posted_intr)(struct vcpu *v, u8 vector);
     void (*sync_pir_to_irr)(struct vcpu *v);
+    void (*handle_eoi)(u8 vector);
 
     /*Walk nested p2m  */
     int (*nhvm_hap_walk_L1_p2m)(struct vcpu *v, paddr_t L2_gpa,




[-- Attachment #2: x86-VMX-Viridian-vINTR.patch --]
[-- Type: text/plain, Size: 2584 bytes --]

VMX: fix interaction of APIC-V and Viridian emulation

Viridian using a synthetic MSR for issuing EOI notifications bypasses
the normal in-processor handling, which would clear
GUEST_INTR_STATUS.SVI. Hence we need to do this in software in order
for future interrupts to get delivered.

Based on analysis by Yang Z Zhang <yang.z.zhang@intel.com>.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Split off cleanup parts to new patch 3.

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -386,6 +386,9 @@ void vlapic_EOI_set(struct vlapic *vlapi
 
     vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]);
 
+    if ( hvm_funcs.handle_eoi )
+        hvm_funcs.handle_eoi(vector);
+
     if ( vlapic_test_and_clear_vector(vector, &vlapic->regs->data[APIC_TMR]) )
         vioapic_update_EOI(vlapic_domain(vlapic), vector);
 
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1502,6 +1502,15 @@ static void vmx_sync_pir_to_irr(struct v
         vlapic_set_vector(i, &vlapic->regs->data[APIC_IRR]);
 }
 
+static void vmx_handle_eoi(u8 vector)
+{
+    unsigned long status = __vmread(GUEST_INTR_STATUS);
+
+    /* We need to clear the SVI field. */
+    status &= VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
+    __vmwrite(GUEST_INTR_STATUS, status);
+}
+
 static struct hvm_function_table __initdata vmx_function_table = {
     .name                 = "VMX",
     .cpu_up_prepare       = vmx_cpu_up_prepare,
@@ -1554,6 +1563,7 @@ static struct hvm_function_table __initd
     .process_isr          = vmx_process_isr,
     .deliver_posted_intr  = vmx_deliver_posted_intr,
     .sync_pir_to_irr      = vmx_sync_pir_to_irr,
+    .handle_eoi           = vmx_handle_eoi,
     .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
 };
 
@@ -1580,7 +1590,10 @@ const struct hvm_function_table * __init
 
         setup_ept_dump();
     }
- 
+
+    if ( !cpu_has_vmx_virtual_intr_delivery )
+        vmx_function_table.handle_eoi = NULL;
+
     if ( cpu_has_vmx_posted_intr_processing )
         alloc_direct_apic_vector(&posted_intr_vector, event_check_interrupt);
     else
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -186,6 +186,7 @@ struct hvm_function_table {
     void (*process_isr)(int isr, struct vcpu *v);
     void (*deliver_posted_intr)(struct vcpu *v, u8 vector);
     void (*sync_pir_to_irr)(struct vcpu *v);
+    void (*handle_eoi)(u8 vector);
 
     /*Walk nested p2m  */
     int (*nhvm_hap_walk_L1_p2m)(struct vcpu *v, paddr_t L2_gpa,

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

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

[PATCH v2 2/5] VMX/Viridian: suppress MSR-based APIC suggestion when having APIC-V

[Jan Beulich]

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

When the CPU has the necessary capabilities, having Windows use
synthetic MSR reads/writes is bogus, as this still requires emulation
(which is pretty much guaranteed to be slower than having the hardware
carry out the operation).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -87,8 +87,9 @@ int cpuid_viridian_leaves(unsigned int l
         if ( (d->arch.hvm_domain.viridian.guest_os_id.raw == 0) ||
              (d->arch.hvm_domain.viridian.guest_os_id.fields.os < 4) )
             break;
-        *eax = (CPUID4A_MSR_BASED_APIC |
-                CPUID4A_RELAX_TIMER_INT);
+        *eax = CPUID4A_RELAX_TIMER_INT;
+        if ( !cpu_has_vmx_apic_reg_virt )
+            *eax |= CPUID4A_MSR_BASED_APIC;
         *ebx = 2047; /* long spin count */
         break;
     }




[-- Attachment #2: x86-Viridian-CPUID-APICV.patch --]
[-- Type: text/plain, Size: 940 bytes --]

VMX/Viridian: suppress MSR-based APIC suggestion when having APIC-V

When the CPU has the necessary capabilities, having Windows use
synthetic MSR reads/writes is bogus, as this still requires emulation
(which is pretty much guaranteed to be slower than having the hardware
carry out the operation).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -87,8 +87,9 @@ int cpuid_viridian_leaves(unsigned int l
         if ( (d->arch.hvm_domain.viridian.guest_os_id.raw == 0) ||
              (d->arch.hvm_domain.viridian.guest_os_id.fields.os < 4) )
             break;
-        *eax = (CPUID4A_MSR_BASED_APIC |
-                CPUID4A_RELAX_TIMER_INT);
+        *eax = CPUID4A_RELAX_TIMER_INT;
+        if ( !cpu_has_vmx_apic_reg_virt )
+            *eax |= CPUID4A_MSR_BASED_APIC;
         *ebx = 2047; /* long spin count */
         break;
     }

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

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

[PATCH v2 3/5] VMX: suppress pointless indirect calls

[Jan Beulich]

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

Get the other virtual interrupt delivery related actors in sync
with the newly added handle_eoi() one: Clear the respective pointers
(thus avoiding the call from generic code) when the feature is
unavailable instead of checking feature availability in the actors.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1411,13 +1411,10 @@ static void vmx_set_info_guest(struct vc
 
 static void vmx_update_eoi_exit_bitmap(struct vcpu *v, u8 vector, u8 trig)
 {
-    if ( cpu_has_vmx_virtual_intr_delivery )
-    {
-        if (trig)
-            vmx_set_eoi_exit_bitmap(v, vector);
-        else
-            vmx_clear_eoi_exit_bitmap(v, vector);
-    }
+    if ( trig )
+        vmx_set_eoi_exit_bitmap(v, vector);
+    else
+        vmx_clear_eoi_exit_bitmap(v, vector);
 }
 
 static int vmx_virtual_intr_delivery_enabled(void)
@@ -1430,9 +1427,6 @@ static void vmx_process_isr(int isr, str
     unsigned long status;
     u8 old;
 
-    if ( !cpu_has_vmx_virtual_intr_delivery )
-        return;
-
     if ( isr < 0 )
         isr = 0;
 
@@ -1592,7 +1586,11 @@ const struct hvm_function_table * __init
     }
 
     if ( !cpu_has_vmx_virtual_intr_delivery )
+    {
+        vmx_function_table.update_eoi_exit_bitmap = NULL;
+        vmx_function_table.process_isr = NULL;
         vmx_function_table.handle_eoi = NULL;
+    }
 
     if ( cpu_has_vmx_posted_intr_processing )
         alloc_direct_apic_vector(&posted_intr_vector, event_check_interrupt);




[-- Attachment #2: x86-VMX-vINTR-cleanup.patch --]
[-- Type: text/plain, Size: 1612 bytes --]

VMX: suppress pointless indirect calls

Get the other virtual interrupt delivery related actors in sync
with the newly added handle_eoi() one: Clear the respective pointers
(thus avoiding the call from generic code) when the feature is
unavailable instead of checking feature availability in the actors.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1411,13 +1411,10 @@ static void vmx_set_info_guest(struct vc
 
 static void vmx_update_eoi_exit_bitmap(struct vcpu *v, u8 vector, u8 trig)
 {
-    if ( cpu_has_vmx_virtual_intr_delivery )
-    {
-        if (trig)
-            vmx_set_eoi_exit_bitmap(v, vector);
-        else
-            vmx_clear_eoi_exit_bitmap(v, vector);
-    }
+    if ( trig )
+        vmx_set_eoi_exit_bitmap(v, vector);
+    else
+        vmx_clear_eoi_exit_bitmap(v, vector);
 }
 
 static int vmx_virtual_intr_delivery_enabled(void)
@@ -1430,9 +1427,6 @@ static void vmx_process_isr(int isr, str
     unsigned long status;
     u8 old;
 
-    if ( !cpu_has_vmx_virtual_intr_delivery )
-        return;
-
     if ( isr < 0 )
         isr = 0;
 
@@ -1592,7 +1586,11 @@ const struct hvm_function_table * __init
     }
 
     if ( !cpu_has_vmx_virtual_intr_delivery )
+    {
+        vmx_function_table.update_eoi_exit_bitmap = NULL;
+        vmx_function_table.process_isr = NULL;
         vmx_function_table.handle_eoi = NULL;
+    }
 
     if ( cpu_has_vmx_posted_intr_processing )
         alloc_direct_apic_vector(&posted_intr_vector, event_check_interrupt);

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

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

[PATCH v2 4/5] Viridian: populate CPUID leaf 6

[Jan Beulich]

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

Properly reporting hardware features we use can only help Windows in
making decisions towards its own performance tuning.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -41,6 +41,11 @@
 #define CPUID4A_MSR_BASED_APIC  (1 << 3)
 #define CPUID4A_RELAX_TIMER_INT (1 << 5)
 
+/* Viridian CPUID 4000006, Implementation HW features detected and in use. */
+#define CPUID6A_APIC_OVERLAY    (1 << 0)
+#define CPUID6A_MSR_BITMAPS     (1 << 1)
+#define CPUID6A_NESTED_PAGING   (1 << 3)
+
 int cpuid_viridian_leaves(unsigned int leaf, unsigned int *eax,
                           unsigned int *ebx, unsigned int *ecx,
                           unsigned int *edx)
@@ -92,6 +97,15 @@ int cpuid_viridian_leaves(unsigned int l
             *eax |= CPUID4A_MSR_BASED_APIC;
         *ebx = 2047; /* long spin count */
         break;
+    case 6:
+        /* Detected and in use hardware features. */
+        if ( cpu_has_vmx_virtualize_apic_accesses )
+            *eax |= CPUID6A_APIC_OVERLAY;
+        if ( cpu_has_vmx_msr_bitmap || (read_efer() & EFER_SVME) )
+            *eax |= CPUID6A_MSR_BITMAPS;
+        if ( hap_enabled(d) )
+            *eax |= CPUID6A_NESTED_PAGING;
+        break;
     }
 
     return 1;




[-- Attachment #2: x86-Viridian-CPUID-leaf6.patch --]
[-- Type: text/plain, Size: 1350 bytes --]

Viridian: populate CPUID leaf 6

Properly reporting hardware features we use can only help Windows in
making decisions towards its own performance tuning.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -41,6 +41,11 @@
 #define CPUID4A_MSR_BASED_APIC  (1 << 3)
 #define CPUID4A_RELAX_TIMER_INT (1 << 5)
 
+/* Viridian CPUID 4000006, Implementation HW features detected and in use. */
+#define CPUID6A_APIC_OVERLAY    (1 << 0)
+#define CPUID6A_MSR_BITMAPS     (1 << 1)
+#define CPUID6A_NESTED_PAGING   (1 << 3)
+
 int cpuid_viridian_leaves(unsigned int leaf, unsigned int *eax,
                           unsigned int *ebx, unsigned int *ecx,
                           unsigned int *edx)
@@ -92,6 +97,15 @@ int cpuid_viridian_leaves(unsigned int l
             *eax |= CPUID4A_MSR_BASED_APIC;
         *ebx = 2047; /* long spin count */
         break;
+    case 6:
+        /* Detected and in use hardware features. */
+        if ( cpu_has_vmx_virtualize_apic_accesses )
+            *eax |= CPUID6A_APIC_OVERLAY;
+        if ( cpu_has_vmx_msr_bitmap || (read_efer() & EFER_SVME) )
+            *eax |= CPUID6A_MSR_BITMAPS;
+        if ( hap_enabled(d) )
+            *eax |= CPUID6A_NESTED_PAGING;
+        break;
     }
 
     return 1;

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

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

[PATCH v2 5/5] Viridian: cleanup

[Jan Beulich]

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

- functions used only locally should be static
- constify parameters of dump functions

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -111,7 +111,7 @@ int cpuid_viridian_leaves(unsigned int l
     return 1;
 }
 
-void dump_guest_os_id(struct domain *d)
+static void dump_guest_os_id(const struct domain *d)
 {
     gdprintk(XENLOG_INFO, "GUEST_OS_ID:\n");
     gdprintk(XENLOG_INFO, "\tvendor: %x\n",
@@ -128,7 +128,7 @@ void dump_guest_os_id(struct domain *d)
             d->arch.hvm_domain.viridian.guest_os_id.fields.build_number);
 }
 
-void dump_hypercall(struct domain *d)
+static void dump_hypercall(const struct domain *d)
 {
     gdprintk(XENLOG_INFO, "HYPERCALL:\n");
     gdprintk(XENLOG_INFO, "\tenabled: %x\n",
@@ -137,7 +137,7 @@ void dump_hypercall(struct domain *d)
             (unsigned long)d->arch.hvm_domain.viridian.hypercall_gpa.fields.pfn);
 }
 
-void dump_apic_assist(struct vcpu *v)
+static void dump_apic_assist(const struct vcpu *v)
 {
     gdprintk(XENLOG_INFO, "APIC_ASSIST[%d]:\n", v->vcpu_id);
     gdprintk(XENLOG_INFO, "\tenabled: %x\n",
@@ -180,7 +180,7 @@ static void enable_hypercall_page(struct
     put_page_and_type(page);
 }
 
-void initialize_apic_assist(struct vcpu *v)
+static void initialize_apic_assist(struct vcpu *v)
 {
     struct domain *d = v->domain;
     unsigned long gmfn = v->arch.hvm_vcpu.viridian.apic_assist.fields.pfn;




[-- Attachment #2: x86-Viridian-cleanup.patch --]
[-- Type: text/plain, Size: 1517 bytes --]

Viridian: cleanup

- functions used only locally should be static
- constify parameters of dump functions

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -111,7 +111,7 @@ int cpuid_viridian_leaves(unsigned int l
     return 1;
 }
 
-void dump_guest_os_id(struct domain *d)
+static void dump_guest_os_id(const struct domain *d)
 {
     gdprintk(XENLOG_INFO, "GUEST_OS_ID:\n");
     gdprintk(XENLOG_INFO, "\tvendor: %x\n",
@@ -128,7 +128,7 @@ void dump_guest_os_id(struct domain *d)
             d->arch.hvm_domain.viridian.guest_os_id.fields.build_number);
 }
 
-void dump_hypercall(struct domain *d)
+static void dump_hypercall(const struct domain *d)
 {
     gdprintk(XENLOG_INFO, "HYPERCALL:\n");
     gdprintk(XENLOG_INFO, "\tenabled: %x\n",
@@ -137,7 +137,7 @@ void dump_hypercall(struct domain *d)
             (unsigned long)d->arch.hvm_domain.viridian.hypercall_gpa.fields.pfn);
 }
 
-void dump_apic_assist(struct vcpu *v)
+static void dump_apic_assist(const struct vcpu *v)
 {
     gdprintk(XENLOG_INFO, "APIC_ASSIST[%d]:\n", v->vcpu_id);
     gdprintk(XENLOG_INFO, "\tenabled: %x\n",
@@ -180,7 +180,7 @@ static void enable_hypercall_page(struct
     put_page_and_type(page);
 }
 
-void initialize_apic_assist(struct vcpu *v)
+static void initialize_apic_assist(struct vcpu *v)
 {
     struct domain *d = v->domain;
     unsigned long gmfn = v->arch.hvm_vcpu.viridian.apic_assist.fields.pfn;

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

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

Re: [PATCH v2 1/5] VMX: fix interaction of APIC-V and Viridian emulation

[George Dunlap]

On 24/06/13 08:03, Jan Beulich wrote:
> Viridian using a synthetic MSR for issuing EOI notifications bypasses
> the normal in-processor handling, which would clear
> GUEST_INTR_STATUS.SVI. Hence we need to do this in software in order
> for future interrupts to get delivered.
>
> Based on analysis by Yang Z Zhang <yang.z.zhang@intel.com>.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Hmm... so there are three paths which may end up calling this vmx EOI 
code -- from viridian.c:wrmsr_vidiridan_regs(), from 
vlapic.c:vlapic_reg_write(), and vmx_handle_eoi_write().

Obviously the viridian code is what we want.  But which of the other two 
paths will also end up taking it, and is it correct?  In other words, 
for which of those will cpu_has_vmx_virtual_intr_delivery be set?

  -George

> ---
> v2: Split off cleanup parts to new patch 3.
>
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -386,6 +386,9 @@ void vlapic_EOI_set(struct vlapic *vlapi
>   
>       vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]);
>   
> +    if ( hvm_funcs.handle_eoi )
> +        hvm_funcs.handle_eoi(vector);
> +
>       if ( vlapic_test_and_clear_vector(vector, &vlapic->regs->data[APIC_TMR]) )
>           vioapic_update_EOI(vlapic_domain(vlapic), vector);
>   
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1502,6 +1502,15 @@ static void vmx_sync_pir_to_irr(struct v
>           vlapic_set_vector(i, &vlapic->regs->data[APIC_IRR]);
>   }
>   
> +static void vmx_handle_eoi(u8 vector)
> +{
> +    unsigned long status = __vmread(GUEST_INTR_STATUS);
> +
> +    /* We need to clear the SVI field. */
> +    status &= VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
> +    __vmwrite(GUEST_INTR_STATUS, status);
> +}
> +
>   static struct hvm_function_table __initdata vmx_function_table = {
>       .name                 = "VMX",
>       .cpu_up_prepare       = vmx_cpu_up_prepare,
> @@ -1554,6 +1563,7 @@ static struct hvm_function_table __initd
>       .process_isr          = vmx_process_isr,
>       .deliver_posted_intr  = vmx_deliver_posted_intr,
>       .sync_pir_to_irr      = vmx_sync_pir_to_irr,
> +    .handle_eoi           = vmx_handle_eoi,
>       .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
>   };
>   
> @@ -1580,7 +1590,10 @@ const struct hvm_function_table * __init
>   
>           setup_ept_dump();
>       }
> -
> +
> +    if ( !cpu_has_vmx_virtual_intr_delivery )
> +        vmx_function_table.handle_eoi = NULL;
> +
>       if ( cpu_has_vmx_posted_intr_processing )
>           alloc_direct_apic_vector(&posted_intr_vector, event_check_interrupt);
>       else
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -186,6 +186,7 @@ struct hvm_function_table {
>       void (*process_isr)(int isr, struct vcpu *v);
>       void (*deliver_posted_intr)(struct vcpu *v, u8 vector);
>       void (*sync_pir_to_irr)(struct vcpu *v);
> +    void (*handle_eoi)(u8 vector);
>   
>       /*Walk nested p2m  */
>       int (*nhvm_hap_walk_L1_p2m)(struct vcpu *v, paddr_t L2_gpa,
>
>
>

Re: [PATCH v2 1/5] VMX: fix interaction of APIC-V and Viridian emulation

[Jan Beulich]

>>> On 24.06.13 at 12:10, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 24/06/13 08:03, Jan Beulich wrote:
>> Viridian using a synthetic MSR for issuing EOI notifications bypasses
>> the normal in-processor handling, which would clear
>> GUEST_INTR_STATUS.SVI. Hence we need to do this in software in order
>> for future interrupts to get delivered.
>>
>> Based on analysis by Yang Z Zhang <yang.z.zhang@intel.com>.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Hmm... so there are three paths which may end up calling this vmx EOI 
> code -- from viridian.c:wrmsr_vidiridan_regs(), from 
> vlapic.c:vlapic_reg_write(), and vmx_handle_eoi_write().

This is the very reason why I favored patch 2 over this one for
4.3 ...

> Obviously the viridian code is what we want.  But which of the other two 
> paths will also end up taking it, and is it correct?  In other words, 
> for which of those will cpu_has_vmx_virtual_intr_delivery be set?

The two other paths would in the worst case end up re-doing what
the hardware already did.

However, both of them can't occur with APIC register virtualization
enabled, and during early investigation of this issue I was told that
even if they're formally independent features, virtual interrupt
delivery always implies APIC register virtualization.

Jan

Re: [PATCH v2 1/5] VMX: fix interaction of APIC-V and Viridian emulation

[George Dunlap]

On 24/06/13 13:52, Jan Beulich wrote:
>>>> On 24.06.13 at 12:10, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> On 24/06/13 08:03, Jan Beulich wrote:
>>> Viridian using a synthetic MSR for issuing EOI notifications bypasses
>>> the normal in-processor handling, which would clear
>>> GUEST_INTR_STATUS.SVI. Hence we need to do this in software in order
>>> for future interrupts to get delivered.
>>>
>>> Based on analysis by Yang Z Zhang <yang.z.zhang@intel.com>.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Hmm... so there are three paths which may end up calling this vmx EOI
>> code -- from viridian.c:wrmsr_vidiridan_regs(), from
>> vlapic.c:vlapic_reg_write(), and vmx_handle_eoi_write().
> This is the very reason why I favored patch 2 over this one for
> 4.3 ...

Yes, I think I didn't realize that when I looked at the patch on 
Friday.  (It was the end of a very tiring week.)

What other operating systems have you tested patch #2 with?  IIRC Vista 
and Win7 both also have extensions, IIRC.  Also, has either #1 or #2 
been tested on AMD boxen?

Choosing #1 involves the risk that we've missed something an will make 
one of those three cases *not* like real hardware, which seems fairly 
small.  Choosing #2 involves the risk that MS may not have implemented 
the feature flag checking properly -- they almost surely test it *with* 
the feature flag much more than *without* it.  Even if they do test 
without it, they may not test with the particular combination of flags 
that we are proposing.

So overall, I still tend to think #1 is probably less risky.  But as I 
said, I'm willing to go with either one.

  -George

Re: [PATCH v2 1/5] VMX: fix interaction of APIC-V and Viridian emulation

[Jan Beulich]

>>> On 24.06.13 at 15:09, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 24/06/13 13:52, Jan Beulich wrote:
>>>>> On 24.06.13 at 12:10, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>>> On 24/06/13 08:03, Jan Beulich wrote:
>>>> Viridian using a synthetic MSR for issuing EOI notifications bypasses
>>>> the normal in-processor handling, which would clear
>>>> GUEST_INTR_STATUS.SVI. Hence we need to do this in software in order
>>>> for future interrupts to get delivered.
>>>>
>>>> Based on analysis by Yang Z Zhang <yang.z.zhang@intel.com>.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Hmm... so there are three paths which may end up calling this vmx EOI
>>> code -- from viridian.c:wrmsr_vidiridan_regs(), from
>>> vlapic.c:vlapic_reg_write(), and vmx_handle_eoi_write().
>> This is the very reason why I favored patch 2 over this one for
>> 4.3 ...
> 
> Yes, I think I didn't realize that when I looked at the patch on 
> Friday.  (It was the end of a very tiring week.)
> 
> What other operating systems have you tested patch #2 with?  IIRC Vista 
> and Win7 both also have extensions, IIRC.

Win7 surely has been tested, but I doubt Vista has (we don't
routinely do that).

> Also, has either #1 or #2 been tested on AMD boxen?

No, and I don't see the point. The actor #1 adds is simply NULL for
SVM (and hence behavior doesn't change), and the flag tested in
#2 is VMX specific too (so behavior doesn't change either).

> Choosing #1 involves the risk that we've missed something an will make 
> one of those three cases *not* like real hardware, which seems fairly 
> small.  Choosing #2 involves the risk that MS may not have implemented 
> the feature flag checking properly -- they almost surely test it *with* 
> the feature flag much more than *without* it.  Even if they do test 
> without it, they may not test with the particular combination of flags 
> that we are proposing.
> 
> So overall, I still tend to think #1 is probably less risky.  But as I 
> said, I'm willing to go with either one.

Which might as well mean to go with both, provided we get acks
soon.

Jan

Re: [PATCH v2 1/5] VMX: fix interaction of APIC-V and Viridian emulation

[George Dunlap]

On 24/06/13 14:26, Jan Beulich wrote:
>>>> On 24.06.13 at 15:09, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> On 24/06/13 13:52, Jan Beulich wrote:
>>>>>> On 24.06.13 at 12:10, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>>>> On 24/06/13 08:03, Jan Beulich wrote:
>>>>> Viridian using a synthetic MSR for issuing EOI notifications bypasses
>>>>> the normal in-processor handling, which would clear
>>>>> GUEST_INTR_STATUS.SVI. Hence we need to do this in software in order
>>>>> for future interrupts to get delivered.
>>>>>
>>>>> Based on analysis by Yang Z Zhang <yang.z.zhang@intel.com>.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> Hmm... so there are three paths which may end up calling this vmx EOI
>>>> code -- from viridian.c:wrmsr_vidiridan_regs(), from
>>>> vlapic.c:vlapic_reg_write(), and vmx_handle_eoi_write().
>>> This is the very reason why I favored patch 2 over this one for
>>> 4.3 ...
>> Yes, I think I didn't realize that when I looked at the patch on
>> Friday.  (It was the end of a very tiring week.)
>>
>> What other operating systems have you tested patch #2 with?  IIRC Vista
>> and Win7 both also have extensions, IIRC.
> Win7 surely has been tested, but I doubt Vista has (we don't
> routinely do that).
>
>> Also, has either #1 or #2 been tested on AMD boxen?
> No, and I don't see the point. The actor #1 adds is simply NULL for
> SVM (and hence behavior doesn't change), and the flag tested in
> #2 is VMX specific too (so behavior doesn't change either).
>
>> Choosing #1 involves the risk that we've missed something an will make
>> one of those three cases *not* like real hardware, which seems fairly
>> small.  Choosing #2 involves the risk that MS may not have implemented
>> the feature flag checking properly -- they almost surely test it *with*
>> the feature flag much more than *without* it.  Even if they do test
>> without it, they may not test with the particular combination of flags
>> that we are proposing.
>>
>> So overall, I still tend to think #1 is probably less risky.  But as I
>> said, I'm willing to go with either one.
> Which might as well mean to go with both, provided we get acks
> soon.

Um, I think exactly the opposite.  The question we want to ask now is, 
"What option will fix the problem with the lowest risk of having another 
problem crop up?"  If we choose *both*, then we get a failure if there 
is a secondary problem in *either* patch.

That's like saying, "I'm not sure which of these revolvers is loaded/has 
more bullets, so if I'm going to play Russian roulette, I might as well 
do it with both."  :-)

   -George

Re: [PATCH v2 1/5] VMX: fix interaction of APIC-V and Viridian emulation

[Jan Beulich]

>>> On 24.06.13 at 15:29, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 24/06/13 14:26, Jan Beulich wrote:
>>>>> On 24.06.13 at 15:09, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>>> So overall, I still tend to think #1 is probably less risky.  But as I
>>> said, I'm willing to go with either one.
>> Which might as well mean to go with both, provided we get acks
>> soon.
> 
> Um, I think exactly the opposite.  The question we want to ask now is, 
> "What option will fix the problem with the lowest risk of having another 
> problem crop up?"  If we choose *both*, then we get a failure if there 
> is a secondary problem in *either* patch.
> 
> That's like saying, "I'm not sure which of these revolvers is loaded/has 
> more bullets, so if I'm going to play Russian roulette, I might as well 
> do it with both."  :-)

Yeah, if you're taking this from the pessimistic side...

Jan

Re: [PATCH v2 2/5] VMX/Viridian: suppress MSR-based APIC suggestion when having APIC-V

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 24 June 2013 08:05
> To: xen-devel
> Cc: Paul Durrant; George Dunlap; Eddie Dong; Jun Nakajima; Yang Z Zhang;
> Keir (Xen.org)
> Subject: [PATCH v2 2/5] VMX/Viridian: suppress MSR-based APIC suggestion
> when having APIC-V
> 
> When the CPU has the necessary capabilities, having Windows use
> synthetic MSR reads/writes is bogus, as this still requires emulation
> (which is pretty much guaranteed to be slower than having the hardware
> carry out the operation).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 

Seems better just to not use the MSR in this case so I favour this patch over #1, hence

Ack-ed by: Paul Durrant <paul.durrant@citrix.com>

> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -87,8 +87,9 @@ int cpuid_viridian_leaves(unsigned int l
>          if ( (d->arch.hvm_domain.viridian.guest_os_id.raw == 0) ||
>               (d->arch.hvm_domain.viridian.guest_os_id.fields.os < 4) )
>              break;
> -        *eax = (CPUID4A_MSR_BASED_APIC |
> -                CPUID4A_RELAX_TIMER_INT);
> +        *eax = CPUID4A_RELAX_TIMER_INT;
> +        if ( !cpu_has_vmx_apic_reg_virt )
> +            *eax |= CPUID4A_MSR_BASED_APIC;
>          *ebx = 2047; /* long spin count */
>          break;
>      }
> 
> 

Re: [PATCH v2 2/5] VMX/Viridian: suppress MSR-based APIC suggestion when having APIC-V

[George Dunlap]

On 06/25/2013 11:29 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 24 June 2013 08:05
>> To: xen-devel
>> Cc: Paul Durrant; George Dunlap; Eddie Dong; Jun Nakajima; Yang Z Zhang;
>> Keir (Xen.org)
>> Subject: [PATCH v2 2/5] VMX/Viridian: suppress MSR-based APIC suggestion
>> when having APIC-V
>>
>> When the CPU has the necessary capabilities, having Windows use
>> synthetic MSR reads/writes is bogus, as this still requires emulation
>> (which is pretty much guaranteed to be slower than having the hardware
>> carry out the operation).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>
> Seems better just to not use the MSR in this case so I favour this patch over #1, hence
>
> Ack-ed by: Paul Durrant <paul.durrant@citrix.com>

This may push this over into the "#2 is probably a better idea" 
category; Paul, Yang, and Jan all think it's less intrusive.

Jan, I think it's your call -- You have an ack from me to put either #1 
or #2 in for 4.3 (but please not both).

  -George

Re: [PATCH v2 2/5] VMX/Viridian: suppress MSR-based APIC suggestion when having APIC-V

[Jan Beulich]

>>> On 25.06.13 at 15:43, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 06/25/2013 11:29 AM, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>> Sent: 24 June 2013 08:05
>>> To: xen-devel
>>> Cc: Paul Durrant; George Dunlap; Eddie Dong; Jun Nakajima; Yang Z Zhang;
>>> Keir (Xen.org)
>>> Subject: [PATCH v2 2/5] VMX/Viridian: suppress MSR-based APIC suggestion
>>> when having APIC-V
>>>
>>> When the CPU has the necessary capabilities, having Windows use
>>> synthetic MSR reads/writes is bogus, as this still requires emulation
>>> (which is pretty much guaranteed to be slower than having the hardware
>>> carry out the operation).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>
>> Seems better just to not use the MSR in this case so I favour this patch 
> over #1, hence
>>
>> Ack-ed by: Paul Durrant <paul.durrant@citrix.com>
> 
> This may push this over into the "#2 is probably a better idea" 
> category; Paul, Yang, and Jan all think it's less intrusive.
> 
> Jan, I think it's your call -- You have an ack from me to put either #1 
> or #2 in for 4.3 (but please not both).

So I pushed this one, considering the above and the lack of an
ack for #1.

Jan

Re: [PATCH v2 0/5] VMX: fix interaction of Viridian emulation with advanced features

[Jan Beulich]

>>> On 24.06.13 at 08:54, "Jan Beulich" <JBeulich@suse.com> wrote:
> 1: VMX: fix interaction of APIC-V and Viridian emulation
> 2: VMX/Viridian: suppress MSR-based APIC suggestion when having APIC-V
> 3: VMX: suppress pointless indirect calls
> 4: Viridian: populate CPUID leaf 6
> 5: Viridian: cleanup
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Split off cleanup parts from patch 1 (into new patch 3). Add new
>     patch 5.

While patch 2 went in for 4.3, all of the others here are still
awaiting reviews/acks. Anyone?

Thanks, Jan

Re: [PATCH v2 1/5] VMX: fix interaction of APIC-V and Viridian emulation

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2835 bytes --]

On 24/06/13 08:03, Jan Beulich wrote:
> Viridian using a synthetic MSR for issuing EOI notifications bypasses
> the normal in-processor handling, which would clear
> GUEST_INTR_STATUS.SVI. Hence we need to do this in software in order
> for future interrupts to get delivered.
>
> Based on analysis by Yang Z Zhang <yang.z.zhang@intel.com>.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> v2: Split off cleanup parts to new patch 3.
>
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -386,6 +386,9 @@ void vlapic_EOI_set(struct vlapic *vlapi
>  
>      vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]);
>  
> +    if ( hvm_funcs.handle_eoi )
> +        hvm_funcs.handle_eoi(vector);
> +
>      if ( vlapic_test_and_clear_vector(vector, &vlapic->regs->data[APIC_TMR]) )
>          vioapic_update_EOI(vlapic_domain(vlapic), vector);
>  
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1502,6 +1502,15 @@ static void vmx_sync_pir_to_irr(struct v
>          vlapic_set_vector(i, &vlapic->regs->data[APIC_IRR]);
>  }
>  
> +static void vmx_handle_eoi(u8 vector)
> +{
> +    unsigned long status = __vmread(GUEST_INTR_STATUS);
> +
> +    /* We need to clear the SVI field. */
> +    status &= VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
> +    __vmwrite(GUEST_INTR_STATUS, status);
> +}
> +
>  static struct hvm_function_table __initdata vmx_function_table = {
>      .name                 = "VMX",
>      .cpu_up_prepare       = vmx_cpu_up_prepare,
> @@ -1554,6 +1563,7 @@ static struct hvm_function_table __initd
>      .process_isr          = vmx_process_isr,
>      .deliver_posted_intr  = vmx_deliver_posted_intr,
>      .sync_pir_to_irr      = vmx_sync_pir_to_irr,
> +    .handle_eoi           = vmx_handle_eoi,
>      .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
>  };
>  
> @@ -1580,7 +1590,10 @@ const struct hvm_function_table * __init
>  
>          setup_ept_dump();
>      }
> - 
> +
> +    if ( !cpu_has_vmx_virtual_intr_delivery )
> +        vmx_function_table.handle_eoi = NULL;
> +
>      if ( cpu_has_vmx_posted_intr_processing )
>          alloc_direct_apic_vector(&posted_intr_vector, event_check_interrupt);
>      else
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -186,6 +186,7 @@ struct hvm_function_table {
>      void (*process_isr)(int isr, struct vcpu *v);
>      void (*deliver_posted_intr)(struct vcpu *v, u8 vector);
>      void (*sync_pir_to_irr)(struct vcpu *v);
> +    void (*handle_eoi)(u8 vector);
>  
>      /*Walk nested p2m  */
>      int (*nhvm_hap_walk_L1_p2m)(struct vcpu *v, paddr_t L2_gpa,
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 3736 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 3/5] VMX: suppress pointless indirect calls

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1857 bytes --]

On 24/06/13 08:06, Jan Beulich wrote:
> Get the other virtual interrupt delivery related actors in sync
> with the newly added handle_eoi() one: Clear the respective pointers
> (thus avoiding the call from generic code) when the feature is
> unavailable instead of checking feature availability in the actors.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1411,13 +1411,10 @@ static void vmx_set_info_guest(struct vc
>  
>  static void vmx_update_eoi_exit_bitmap(struct vcpu *v, u8 vector, u8 trig)
>  {
> -    if ( cpu_has_vmx_virtual_intr_delivery )
> -    {
> -        if (trig)
> -            vmx_set_eoi_exit_bitmap(v, vector);
> -        else
> -            vmx_clear_eoi_exit_bitmap(v, vector);
> -    }
> +    if ( trig )
> +        vmx_set_eoi_exit_bitmap(v, vector);
> +    else
> +        vmx_clear_eoi_exit_bitmap(v, vector);
>  }
>  
>  static int vmx_virtual_intr_delivery_enabled(void)
> @@ -1430,9 +1427,6 @@ static void vmx_process_isr(int isr, str
>      unsigned long status;
>      u8 old;
>  
> -    if ( !cpu_has_vmx_virtual_intr_delivery )
> -        return;
> -
>      if ( isr < 0 )
>          isr = 0;
>  
> @@ -1592,7 +1586,11 @@ const struct hvm_function_table * __init
>      }
>  
>      if ( !cpu_has_vmx_virtual_intr_delivery )
> +    {
> +        vmx_function_table.update_eoi_exit_bitmap = NULL;
> +        vmx_function_table.process_isr = NULL;
>          vmx_function_table.handle_eoi = NULL;
> +    }
>  
>      if ( cpu_has_vmx_posted_intr_processing )
>          alloc_direct_apic_vector(&posted_intr_vector, event_check_interrupt);
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 2690 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 0/5] VMX: fix interaction of Viridian emulation with advanced features

[Zhang, Yang Z]

Jan Beulich wrote on 2013-07-04:
>>>> On 24.06.13 at 08:54, "Jan Beulich" <JBeulich@suse.com> wrote:
>> 1: VMX: fix interaction of APIC-V and Viridian emulation
>> 2: VMX/Viridian: suppress MSR-based APIC suggestion when having
>> APIC-V
>> 3: VMX: suppress pointless indirect calls
>> 4: Viridian: populate CPUID leaf 6
>> 5: Viridian: cleanup
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: Split off cleanup parts from patch 1 (into new patch 3). Add new
>>     patch 5.
> 
> While patch 2 went in for 4.3, all of the others here are still
> awaiting reviews/acks. Anyone?
> 
> Thanks, Jan

Reviewed-by: Yang Zhang <yang.z.zhang@intel.com>

Best regards,
Yang

Re: [PATCH v2 4/5] Viridian: populate CPUID leaf 6

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1723 bytes --]

On 24/06/13 08:06, Jan Beulich wrote:
> Properly reporting hardware features we use can only help Windows in
> making decisions towards its own performance tuning.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Does viridian allow for the possibility of these flags to change during
runtime, e.g. migrating an HVM domain from hardware supporting HAP to
hardware which can only manage shadow?

~Andrew

>
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -41,6 +41,11 @@
>  #define CPUID4A_MSR_BASED_APIC  (1 << 3)
>  #define CPUID4A_RELAX_TIMER_INT (1 << 5)
>  
> +/* Viridian CPUID 4000006, Implementation HW features detected and in use. */
> +#define CPUID6A_APIC_OVERLAY    (1 << 0)
> +#define CPUID6A_MSR_BITMAPS     (1 << 1)
> +#define CPUID6A_NESTED_PAGING   (1 << 3)
> +
>  int cpuid_viridian_leaves(unsigned int leaf, unsigned int *eax,
>                            unsigned int *ebx, unsigned int *ecx,
>                            unsigned int *edx)
> @@ -92,6 +97,15 @@ int cpuid_viridian_leaves(unsigned int l
>              *eax |= CPUID4A_MSR_BASED_APIC;
>          *ebx = 2047; /* long spin count */
>          break;
> +    case 6:
> +        /* Detected and in use hardware features. */
> +        if ( cpu_has_vmx_virtualize_apic_accesses )
> +            *eax |= CPUID6A_APIC_OVERLAY;
> +        if ( cpu_has_vmx_msr_bitmap || (read_efer() & EFER_SVME) )
> +            *eax |= CPUID6A_MSR_BITMAPS;
> +        if ( hap_enabled(d) )
> +            *eax |= CPUID6A_NESTED_PAGING;
> +        break;
>      }
>  
>      return 1;
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 2553 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 5/5] Viridian: cleanup

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1777 bytes --]

On 24/06/13 08:08, Jan Beulich wrote:
> - functions used only locally should be static
> - constify parameters of dump functions
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -111,7 +111,7 @@ int cpuid_viridian_leaves(unsigned int l
>      return 1;
>  }
>  
> -void dump_guest_os_id(struct domain *d)
> +static void dump_guest_os_id(const struct domain *d)
>  {
>      gdprintk(XENLOG_INFO, "GUEST_OS_ID:\n");
>      gdprintk(XENLOG_INFO, "\tvendor: %x\n",
> @@ -128,7 +128,7 @@ void dump_guest_os_id(struct domain *d)
>              d->arch.hvm_domain.viridian.guest_os_id.fields.build_number);
>  }
>  
> -void dump_hypercall(struct domain *d)
> +static void dump_hypercall(const struct domain *d)
>  {
>      gdprintk(XENLOG_INFO, "HYPERCALL:\n");
>      gdprintk(XENLOG_INFO, "\tenabled: %x\n",
> @@ -137,7 +137,7 @@ void dump_hypercall(struct domain *d)
>              (unsigned long)d->arch.hvm_domain.viridian.hypercall_gpa.fields.pfn);
>  }
>  
> -void dump_apic_assist(struct vcpu *v)
> +static void dump_apic_assist(const struct vcpu *v)
>  {
>      gdprintk(XENLOG_INFO, "APIC_ASSIST[%d]:\n", v->vcpu_id);
>      gdprintk(XENLOG_INFO, "\tenabled: %x\n",
> @@ -180,7 +180,7 @@ static void enable_hypercall_page(struct
>      put_page_and_type(page);
>  }
>  
> -void initialize_apic_assist(struct vcpu *v)
> +static void initialize_apic_assist(struct vcpu *v)
>  {
>      struct domain *d = v->domain;
>      unsigned long gmfn = v->arch.hvm_vcpu.viridian.apic_assist.fields.pfn;
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 2630 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 4/5] Viridian: populate CPUID leaf 6

[Jan Beulich]

>>> On 04.07.13 at 11:38, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 24/06/13 08:06, Jan Beulich wrote:
>> Properly reporting hardware features we use can only help Windows in
>> making decisions towards its own performance tuning.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Does viridian allow for the possibility of these flags to change during
> runtime, e.g. migrating an HVM domain from hardware supporting HAP to
> hardware which can only manage shadow?

Don't know, but this is only reporting capabilities of the hypervisor,
so if they store those anywhere for any purpose they wouldn't
have migration in mind even on their own hypervisor.

Jan

Re: [PATCH v2 4/5] Viridian: populate CPUID leaf 6

[Andrew Cooper]

On 04/07/13 11:05, Jan Beulich wrote:
>>>> On 04.07.13 at 11:38, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 24/06/13 08:06, Jan Beulich wrote:
>>> Properly reporting hardware features we use can only help Windows in
>>> making decisions towards its own performance tuning.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Does viridian allow for the possibility of these flags to change during
>> runtime, e.g. migrating an HVM domain from hardware supporting HAP to
>> hardware which can only manage shadow?
> Don't know, but this is only reporting capabilities of the hypervisor,
> so if they store those anywhere for any purpose they wouldn't
> have migration in mind even on their own hypervisor.
>
> Jan
>

Very true.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>