[PATCH RFC] VMX: fix vmx_handle_eoi()

[PATCH RFC] VMX: fix vmx_handle_eoi()

[Jan Beulich]

In commit 303066fdb1e ("VMX: fix interaction of APIC-V and Viridian
emulation") I screwed up quite heavily: Instead of clearing SVI, RVI was
cleared. Furthermore, unconditional clearing of SVI is wrong too - other
ISR bits should be taken into account.

Introduce a new helper set_svi(), split out of vmx_process_isr(), and
use it also from vmx_handle_eoi().

Following the problems in vmx_intr_assist() (see the still present big
block of debugging code there) also warn (once) if EOI'd vector and
original SVI don't match.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: Untested, as I didn't see any of the possibly resulting problems
     in any of my environments. But perhaps this is (part of) the
     reason of lost interrupts XenServer folks have been observing.

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -448,7 +448,7 @@ 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);
+        hvm_funcs.handle_eoi(vector, vlapic_find_highest_isr(vlapic));
 
     vlapic_handle_EOI(vlapic, vector);
 
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1941,17 +1941,14 @@ static void vmx_update_eoi_exit_bitmap(s
         vmx_clear_eoi_exit_bitmap(v, vector);
 }
 
-static void vmx_process_isr(int isr, struct vcpu *v)
+static u8 set_svi(int isr)
 {
     unsigned long status;
     u8 old;
-    unsigned int i;
-    const struct vlapic *vlapic = vcpu_vlapic(v);
 
     if ( isr < 0 )
         isr = 0;
 
-    vmx_vmcs_enter(v);
     __vmread(GUEST_INTR_STATUS, &status);
     old = status >> VMX_GUEST_INTR_STATUS_SVI_OFFSET;
     if ( isr != old )
@@ -1961,6 +1958,18 @@ static void vmx_process_isr(int isr, str
         __vmwrite(GUEST_INTR_STATUS, status);
     }
 
+    return old;
+}
+
+static void vmx_process_isr(int isr, struct vcpu *v)
+{
+    unsigned int i;
+    const struct vlapic *vlapic = vcpu_vlapic(v);
+
+    vmx_vmcs_enter(v);
+
+    set_svi(isr);
+
     /*
      * Theoretically, only level triggered interrupts can have their
      * corresponding bits set in the eoi exit bitmap. That is, the bits
@@ -2111,14 +2120,13 @@ static bool vmx_test_pir(const struct vc
     return pi_test_pir(vec, &v->arch.hvm.vmx.pi_desc);
 }
 
-static void vmx_handle_eoi(u8 vector)
+static void vmx_handle_eoi(uint8_t vector, int isr)
 {
-    unsigned long status;
+    u8 old_svi = set_svi(isr);
+    static bool warned;
 
-    /* We need to clear the SVI field. */
-    __vmread(GUEST_INTR_STATUS, &status);
-    status &= VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
-    __vmwrite(GUEST_INTR_STATUS, status);
+    if ( vector != old_svi && !test_and_set_bool(warned) )
+        printk(XENLOG_WARNING "EOI for %02x but SVI=%02x\n", vector, old_svi);
 }
 
 static void vmx_enable_msr_interception(struct domain *d, uint32_t msr)
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -201,7 +201,7 @@ struct hvm_function_table {
     void (*deliver_posted_intr)(struct vcpu *v, u8 vector);
     void (*sync_pir_to_irr)(struct vcpu *v);
     bool (*test_pir)(const struct vcpu *v, uint8_t vector);
-    void (*handle_eoi)(u8 vector);
+    void (*handle_eoi)(uint8_t vector, int isr);
 
     /*Walk nested p2m  */
     int (*nhvm_hap_walk_L1_p2m)(struct vcpu *v, paddr_t L2_gpa,





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC] VMX: fix vmx_handle_eoi()

[Jan Beulich]

>>> On 12.10.18 at 11:32, <JBeulich@suse.com> wrote:
> In commit 303066fdb1e ("VMX: fix interaction of APIC-V and Viridian
> emulation") I screwed up quite heavily: Instead of clearing SVI, RVI was
> cleared.

I was wrong here:

> @@ -2111,14 +2120,13 @@ static bool vmx_test_pir(const struct vc
>      return pi_test_pir(vec, &v->arch.hvm.vmx.pi_desc);
>  }
>  
> -static void vmx_handle_eoi(u8 vector)
> +static void vmx_handle_eoi(uint8_t vector, int isr)
>  {
> -    unsigned long status;
> +    u8 old_svi = set_svi(isr);
> +    static bool warned;
>  
> -    /* We need to clear the SVI field. */
> -    __vmread(GUEST_INTR_STATUS, &status);
> -    status &= VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;

This indeed kept RVI, but wrongly masked off bits 16 and up. But
that's benign as long as those bits don#t have a meaning anyway.

The second aspect - ignoring ISR bits - still holds, so the change
itself is still needed afaict. Just the first paragraph of the
description would need to change.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC] VMX: fix vmx_handle_eoi()

[Tian, Kevin]

+Chao to help take a look.

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, October 12, 2018 5:33 PM
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Nakajima, Jun <jun.nakajima@intel.com>; Tian,
> Kevin <kevin.tian@intel.com>
> Subject: [PATCH RFC] VMX: fix vmx_handle_eoi()
> 
> In commit 303066fdb1e ("VMX: fix interaction of APIC-V and Viridian
> emulation") I screwed up quite heavily: Instead of clearing SVI, RVI was
> cleared. Furthermore, unconditional clearing of SVI is wrong too - other
> ISR bits should be taken into account.
> 
> Introduce a new helper set_svi(), split out of vmx_process_isr(), and
> use it also from vmx_handle_eoi().
> 
> Following the problems in vmx_intr_assist() (see the still present big
> block of debugging code there) also warn (once) if EOI'd vector and
> original SVI don't match.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> RFC: Untested, as I didn't see any of the possibly resulting problems
>      in any of my environments. But perhaps this is (part of) the
>      reason of lost interrupts XenServer folks have been observing.
> 
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -448,7 +448,7 @@ 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);
> +        hvm_funcs.handle_eoi(vector, vlapic_find_highest_isr(vlapic));
> 
>      vlapic_handle_EOI(vlapic, vector);
> 
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1941,17 +1941,14 @@ static void vmx_update_eoi_exit_bitmap(s
>          vmx_clear_eoi_exit_bitmap(v, vector);
>  }
> 
> -static void vmx_process_isr(int isr, struct vcpu *v)
> +static u8 set_svi(int isr)
>  {
>      unsigned long status;
>      u8 old;
> -    unsigned int i;
> -    const struct vlapic *vlapic = vcpu_vlapic(v);
> 
>      if ( isr < 0 )
>          isr = 0;
> 
> -    vmx_vmcs_enter(v);
>      __vmread(GUEST_INTR_STATUS, &status);
>      old = status >> VMX_GUEST_INTR_STATUS_SVI_OFFSET;
>      if ( isr != old )
> @@ -1961,6 +1958,18 @@ static void vmx_process_isr(int isr, str
>          __vmwrite(GUEST_INTR_STATUS, status);
>      }
> 
> +    return old;
> +}
> +
> +static void vmx_process_isr(int isr, struct vcpu *v)
> +{
> +    unsigned int i;
> +    const struct vlapic *vlapic = vcpu_vlapic(v);
> +
> +    vmx_vmcs_enter(v);
> +
> +    set_svi(isr);
> +
>      /*
>       * Theoretically, only level triggered interrupts can have their
>       * corresponding bits set in the eoi exit bitmap. That is, the bits
> @@ -2111,14 +2120,13 @@ static bool vmx_test_pir(const struct vc
>      return pi_test_pir(vec, &v->arch.hvm.vmx.pi_desc);
>  }
> 
> -static void vmx_handle_eoi(u8 vector)
> +static void vmx_handle_eoi(uint8_t vector, int isr)
>  {
> -    unsigned long status;
> +    u8 old_svi = set_svi(isr);
> +    static bool warned;
> 
> -    /* We need to clear the SVI field. */
> -    __vmread(GUEST_INTR_STATUS, &status);
> -    status &= VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
> -    __vmwrite(GUEST_INTR_STATUS, status);
> +    if ( vector != old_svi && !test_and_set_bool(warned) )
> +        printk(XENLOG_WARNING "EOI for %02x but SVI=%02x\n", vector,
> old_svi);
>  }
> 
>  static void vmx_enable_msr_interception(struct domain *d, uint32_t msr)
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -201,7 +201,7 @@ struct hvm_function_table {
>      void (*deliver_posted_intr)(struct vcpu *v, u8 vector);
>      void (*sync_pir_to_irr)(struct vcpu *v);
>      bool (*test_pir)(const struct vcpu *v, uint8_t vector);
> -    void (*handle_eoi)(u8 vector);
> +    void (*handle_eoi)(uint8_t vector, int isr);
> 
>      /*Walk nested p2m  */
>      int (*nhvm_hap_walk_L1_p2m)(struct vcpu *v, paddr_t L2_gpa,
> 
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC] VMX: fix vmx_handle_eoi()

[Chao Gao]

On Fri, Oct 12, 2018 at 03:32:59AM -0600, Jan Beulich wrote:
>In commit 303066fdb1e ("VMX: fix interaction of APIC-V and Viridian
>emulation") I screwed up quite heavily: Instead of clearing SVI, RVI was
>cleared. Furthermore, unconditional clearing of SVI is wrong too - other
>ISR bits should be taken into account.
>
>Introduce a new helper set_svi(), split out of vmx_process_isr(), and
>use it also from vmx_handle_eoi().
>
>Following the problems in vmx_intr_assist() (see the still present big
>block of debugging code there) also warn (once) if EOI'd vector and
>original SVI don't match.
>
>Signed-off-by: Jan Beulich <jbeulich@suse.com>

After correcting the description in the first paragraph

Reviewed-by: Chao Gao <chao.gao@intel.com>

I am curious about in which case, the EOI'd vector differs from original SVI.
The sole caller of vmx_handle_eoi() is vlapic_EOI_set(), where the highest bit
of APIC_ISR is assigned to EOI'd vector. So it means the SVI isn't equal to
the highest bit in APIC_ISR. But SDM says "The processor treats this value
(SVI) as the highest priority virtual interrupt that is in service". Maybe in
some places, software or hardware (for instance, when performing virtual
interrupt delivery, SVI got updated without comparing with the current SVI)
fails to make SVI always equal to the highest bit of APIC_ISR.

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC] VMX: fix vmx_handle_eoi()

[Jan Beulich]

>>> On 19.10.18 at 16:30, <chao.gao@intel.com> wrote:
> On Fri, Oct 12, 2018 at 03:32:59AM -0600, Jan Beulich wrote:
>>In commit 303066fdb1e ("VMX: fix interaction of APIC-V and Viridian
>>emulation") I screwed up quite heavily: Instead of clearing SVI, RVI was
>>cleared. Furthermore, unconditional clearing of SVI is wrong too - other
>>ISR bits should be taken into account.
>>
>>Introduce a new helper set_svi(), split out of vmx_process_isr(), and
>>use it also from vmx_handle_eoi().
>>
>>Following the problems in vmx_intr_assist() (see the still present big
>>block of debugging code there) also warn (once) if EOI'd vector and
>>original SVI don't match.
>>
>>Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> After correcting the description in the first paragraph
> 
> Reviewed-by: Chao Gao <chao.gao@intel.com>

With this - any chance of getting the necessary acks? Or
am I expected to re-submit with the (already) adjusted
description?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC] VMX: fix vmx_handle_eoi()

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, October 25, 2018 5:46 PM
> 
> >>> On 19.10.18 at 16:30, <chao.gao@intel.com> wrote:
> > On Fri, Oct 12, 2018 at 03:32:59AM -0600, Jan Beulich wrote:
> >>In commit 303066fdb1e ("VMX: fix interaction of APIC-V and Viridian
> >>emulation") I screwed up quite heavily: Instead of clearing SVI, RVI was
> >>cleared. Furthermore, unconditional clearing of SVI is wrong too - other
> >>ISR bits should be taken into account.
> >>
> >>Introduce a new helper set_svi(), split out of vmx_process_isr(), and
> >>use it also from vmx_handle_eoi().
> >>
> >>Following the problems in vmx_intr_assist() (see the still present big
> >>block of debugging code there) also warn (once) if EOI'd vector and
> >>original SVI don't match.
> >>
> >>Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > After correcting the description in the first paragraph
> >
> > Reviewed-by: Chao Gao <chao.gao@intel.com>
> 
> With this - any chance of getting the necessary acks? Or
> am I expected to re-submit with the (already) adjusted
> description?
> 

Acked-by: Kevin Tian <kevin.tian@intel.com>. Please fix the
description when committing. 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel