Xen devel - Jun 2013 - [PATCH RFC 0/3] VMX: fix interaction of Viridian emulation with advanced features

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

The concepts patch 1 bases upon have been tested to fix the Win2008
boot hang previously reported, but the specific patch here so far
hasn''t
got validated. Additionally I will also want to know whether patch 2
alone would also suffice to address the problem (it should in theory, but
so far it was only tested when at the same time also not setting
CPUID3A_MSR_APIC_ACCESS) - if it does, that might be a candidate
to still go into 4.3.

The above is why the series is being posted as RFC.

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

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

Also get the other virtual interrupt delivery related actors in sync
with the newly added one in clearing 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/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
@@ -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;
 
@@ -1502,6 +1496,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 +1557,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,6 +1584,13 @@ const struct hvm_function_table * __init
 
         setup_ept_dump();
     }
+
+    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);
--- 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

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





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

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;





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

>>> On 21.06.13 at 12:31, "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: Viridian: populate CPUID leaf 6
> 
> The concepts patch 1 bases upon have been tested to fix the Win2008
> boot hang previously reported, but the specific patch here so far
hasn''t
> got validated. Additionally I will also want to know whether patch 2
> alone would also suffice to address the problem (it should in theory, but
> so far it was only tested when at the same time also not setting
> CPUID3A_MSR_APIC_ACCESS) - if it does, that might be a candidate
> to still go into 4.3.
George,

the tests meanwhile completed successfully, i.e. both patch 1
and patch 2 alone address the issue. I''d therefore like to propose
patch 2 as an immediate non-intrusive fix for 4.3, whereas I''d be
fine deferring the first one until after 4.3; deferring the third one
seems obvious as it is not fixing any known bug.

Paul, would you btw feel like reviewing/acking patches 2 and 3?

Jan

On 21/06/13 14:57, Jan Beulich wrote:
>>>> On 21.06.13 at 12:31, "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: Viridian: populate CPUID leaf 6
>>
>> The concepts patch 1 bases upon have been tested to fix the Win2008
>> boot hang previously reported, but the specific patch here so far
hasn''t
>> got validated. Additionally I will also want to know whether patch 2
>> alone would also suffice to address the problem (it should in theory,
but
>> so far it was only tested when at the same time also not setting
>> CPUID3A_MSR_APIC_ACCESS) - if it does, that might be a candidate
>> to still go into 4.3.
> George,
>
> the tests meanwhile completed successfully, i.e. both patch 1
> and patch 2 alone address the issue. I''d therefore like to propose
> patch 2 as an immediate non-intrusive fix for 4.3, whereas I''d be
> fine deferring the first one until after 4.3; deferring the third one
> seems obvious as it is not fixing any known bug.
>
> Paul, would you btw feel like reviewing/acking patches 2 and 3?
Just to clarify -- #1 makes it so that the software EOI does what the 
hardware EOI would do if it were used instead?

It''s a bit hard to tell which one of 1 or 2 is the lowest risk from a 
release perspective -- my instinct is to go with #1 (assuming I''ve 
understood it correctly), as it relies on expected behavior of the 
hardware, not on expected behavior of Windows.  But I think whichever 
you think is best is fine with me, as long as you get an Ack from Paul 
for #2.

  -George

George Dunlap wrote on 2013-06-22:
> On 21/06/13 14:57, Jan Beulich wrote:
>>>>> On 21.06.13 at 12:31, "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: Viridian: populate CPUID leaf 6
>>> 
>>> The concepts patch 1 bases upon have been tested to fix the Win2008
>>> boot hang previously reported, but the specific patch here so far
hasn''t
>>> got validated. Additionally I will also want to know whether patch
2
>>> alone would also suffice to address the problem (it should in
theory, but
>>> so far it was only tested when at the same time also not setting
>>> CPUID3A_MSR_APIC_ACCESS) - if it does, that might be a candidate
>>> to still go into 4.3.
>> George,
>> 
>> the tests meanwhile completed successfully, i.e. both patch 1
>> and patch 2 alone address the issue. I''d therefore like to
propose
>> patch 2 as an immediate non-intrusive fix for 4.3, whereas I''d
be
>> fine deferring the first one until after 4.3; deferring the third one
>> seems obvious as it is not fixing any known bug.
>> 
>> Paul, would you btw feel like reviewing/acking patches 2 and 3?
> 
> Just to clarify -- #1 makes it so that the software EOI does what the
> hardware EOI would do if it were used instead?
Correct!
> It''s a bit hard to tell which one of 1 or 2 is the lowest risk
from a
> release perspective -- my instinct is to go with #1 (assuming I''ve
> understood it correctly), as it relies on expected behavior of the
> hardware, not on expected behavior of Windows.  But I think whichever
> you think is best is fine with me, as long as you get an Ack from Paul
> for #2.
I think both #1 and #2 are necessary. We cannot expect the OS always follows the
rule.

Best regards,
Yang

>>> On 24.06.13 at 03:00, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
> George Dunlap wrote on 2013-06-22:
>> It''s a bit hard to tell which one of 1 or 2 is the lowest risk
from a
>> release perspective -- my instinct is to go with #1 (assuming
I''ve
>> understood it correctly), as it relies on expected behavior of the
>> hardware, not on expected behavior of Windows.  But I think whichever
>> you think is best is fine with me, as long as you get an Ack from Paul
>> for #2.
> I think both #1 and #2 are necessary. We cannot expect the OS always
follows
> the rule.
Sure, ultimately we want both. The question whether to limit
ourselves to one of them is merely for the immediate 4.3
release.

Jan

Jan Beulich wrote on 2013-06-24:
>>>> On 24.06.13 at 03:00, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
>> George Dunlap wrote on 2013-06-22:
>>> It''s a bit hard to tell which one of 1 or 2 is the lowest
risk from a
>>> release perspective -- my instinct is to go with #1 (assuming
I''ve
>>> understood it correctly), as it relies on expected behavior of the
>>> hardware, not on expected behavior of Windows.  But I think
whichever
>>> you think is best is fine with me, as long as you get an Ack from
Paul
>>> for #2.
>> I think both #1 and #2 are necessary. We cannot expect the OS always
follows
>> the rule.
> 
> Sure, ultimately we want both. The question whether to limit ourselves
> to one of them is merely for the immediate 4.3 release.
If talking about 4.3 release, I agree with you. #2 is non-intrusive.

Best regards,
Yang

>>> On 21.06.13 at 18:25, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> It''s a bit hard to tell which one of 1 or 2 is the lowest risk
from a
> release perspective -- my instinct is to go with #1 (assuming I''ve
> understood it correctly), as it relies on expected behavior of the 
> hardware, not on expected behavior of Windows.  But I think whichever 
> you think is best is fine with me, as long as you get an Ack from Paul 
> for #2.
If you prefer #1, I''m fine with that. I proposed #2 to be it just
because it''s less of a change. For us to go with #1, let me split
off the cleanup aspects of it into a separate patch; we''d want
an ack from a VMX maintainer for it perhaps just as much as that
from Paul for #2.

Jan