thr3ads.net - Xen devel - [PATCH v2 0/5] VMX: fix interaction of Viridian emulation with advanced features [Jun 2013]

If this information is useful, please help other people find it:
Share via:

Jan Beulich

2013-Jun-24 06:54 UTC

[PATCH v2 0/5] 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: 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.

Jan Beulich

2013-Jun-24 07:03 UTC

head link

[PATCH v2 1/5] 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,





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

Jan Beulich

2013-Jun-24 07:04 UTC

head link

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

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

Jan Beulich

2013-Jun-24 07:06 UTC

head link

[PATCH v2 3/5] 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);





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

Jan Beulich

2013-Jun-24 07:06 UTC

head link

[PATCH v2 4/5] 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;





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

Jan Beulich

2013-Jun-24 07:08 UTC

head link

[PATCH v2 5/5] 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;





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

George Dunlap

2013-Jun-24 10:10 UTC

head link

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

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

Jan Beulich

2013-Jun-24 12:52 UTC

head link

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

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

George Dunlap

2013-Jun-24 13:09 UTC

head link

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

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

Jan Beulich

2013-Jun-24 13:26 UTC

head link

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

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

George Dunlap

2013-Jun-24 13:29 UTC

head link

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

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

Jan Beulich

2013-Jun-24 13:48 UTC

head link

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

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

Paul Durrant

2013-Jun-25 10:29 UTC

head link

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

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

George Dunlap

2013-Jun-25 13:43 UTC

head link

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

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

Jan Beulich

2013-Jun-25 13:59 UTC

head link

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

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

Jan Beulich

2013-Jul-04 08:38 UTC

head link

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

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

Andrew Cooper

2013-Jul-04 09:03 UTC

head link

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

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


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

Andrew Cooper

2013-Jul-04 09:10 UTC

head link

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

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


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

Zhang, Yang Z

2013-Jul-04 09:24 UTC

head link

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

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

Andrew Cooper

2013-Jul-04 09:38 UTC

head link

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

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


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

Andrew Cooper

2013-Jul-04 09:38 UTC

head link

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

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


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

Jan Beulich

2013-Jul-04 10:05 UTC

head link

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

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

Andrew Cooper

2013-Jul-04 10:18 UTC

head link

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

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>

Xen devel - Jun 2013 - [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

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

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

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

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

[PATCH v2 5/5] Viridian: cleanup

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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