Xen devel - Aug 2013 - [PATCH 0/7] Nested VMX: APIC-v related bug fixing

From: Yang Zhang <yang.z.zhang@Intel.com>

The following patches fix the issue that fail to boot L2 guest on APIC-v
available machine. The main problem is that with APIC-v, virtual interrupt
inject
L1 is totally through APIC-v. But if virtual interrupt is arrived when L2 is
running,
L1 will detect interrupt through vmexit with reason external interrupt. If this
happens,
we should update RVI/SVI to make APIC-v working accordingly.

Yang Zhang (7):
  Nested VMX: Introduce interrupt source supporting
  Nested VMX: Allow to ack irq even virtual intr delivery is enabled
  Nested VMX: Force check ISR when L2 is running
  Nested VMX: Add interface to update vPPR
  Nested VMX: Check whether interrupt is blocked by TPR
  Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1
  Nested VMX: Clear APIC-v control bit in vmcs02

 xen/arch/x86/hvm/irq.c             |    2 +-
 xen/arch/x86/hvm/vlapic.c          |   16 +++++++++++--
 xen/arch/x86/hvm/vmx/intr.c        |    9 ++++++-
 xen/arch/x86/hvm/vmx/vmx.c         |   14 +++++++-----
 xen/arch/x86/hvm/vmx/vvmx.c        |   40 ++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/hvm/vlapic.h   |    3 +-
 xen/include/asm-x86/hvm/vmx/vmx.h  |    2 +-
 xen/include/asm-x86/hvm/vmx/vvmx.h |    1 +
 8 files changed, 73 insertions(+), 14 deletions(-)

From: Yang Zhang <yang.z.zhang@Intel.com>

Introduce interrupt source to determine the source of interrupt that inject
to L1.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/vmx/intr.c        |    4 ++--
 xen/arch/x86/hvm/vmx/vmx.c         |   14 ++++++++------
 xen/include/asm-x86/hvm/vmx/vmx.h  |    2 +-
 xen/include/asm-x86/hvm/vmx/vvmx.h |    1 +
 4 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index e376f3c..cb120f2 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -180,7 +180,7 @@ static int nvmx_intr_intercept(struct vcpu *v, struct
hvm_intack intack)
             if ( !(ctrl & PIN_BASED_EXT_INTR_MASK) )
                 return 0;
 
-            vmx_inject_extint(intack.vector);
+            vmx_inject_extint(intack.vector, intack.source);
 
             ctrl = __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx, VM_EXIT_CONTROLS);
             if ( ctrl & VM_EXIT_ACK_INTR_ON_EXIT )
@@ -309,7 +309,7 @@ void vmx_intr_assist(void)
     else
     {
         HVMTRACE_2D(INJ_VIRQ, intack.vector, /*fake=*/ 0);
-        vmx_inject_extint(intack.vector);
+        vmx_inject_extint(intack.vector, intack.source);
         pt_intr_post(v, intack);
     }
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 8ed7026..51c657f 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1208,7 +1208,7 @@ static void vmx_update_guest_efer(struct vcpu *v)
 }
 
 void nvmx_enqueue_n2_exceptions(struct vcpu *v, 
-            unsigned long intr_fields, int error_code)
+            unsigned long intr_fields, int error_code, uint8_t source)
 {
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
 
@@ -1216,6 +1216,7 @@ void nvmx_enqueue_n2_exceptions(struct vcpu *v,
         /* enqueue the exception till the VMCS switch back to L1 */
         nvmx->intr.intr_info = intr_fields;
         nvmx->intr.error_code = error_code;
+        nvmx->intr.source = source;
         vcpu_nestedhvm(v).nv_vmexit_pending = 1;
         return;
     }
@@ -1227,7 +1228,8 @@ void nvmx_enqueue_n2_exceptions(struct vcpu *v,
 
 static int nvmx_vmexit_trap(struct vcpu *v, struct hvm_trap *trap)
 {
-    nvmx_enqueue_n2_exceptions(v, trap->vector, trap->error_code);
+    nvmx_enqueue_n2_exceptions(v, trap->vector, trap->error_code,
+            hvm_intsrc_none);
     return NESTEDHVM_VMEXIT_DONE;
 }
 
@@ -1258,7 +1260,7 @@ static void __vmx_inject_exception(int trap, int type, int
error_code)
         curr->arch.hvm_vmx.vmx_emulate = 1;
 }
 
-void vmx_inject_extint(int trap)
+void vmx_inject_extint(int trap, uint8_t source)
 {
     struct vcpu *v = current;
     u32    pin_based_cntrl;
@@ -1269,7 +1271,7 @@ void vmx_inject_extint(int trap)
         if ( pin_based_cntrl & PIN_BASED_EXT_INTR_MASK ) {
             nvmx_enqueue_n2_exceptions (v, 
                INTR_INFO_VALID_MASK | (X86_EVENTTYPE_EXT_INTR<<8) | trap,
-               HVM_DELIVER_NO_ERROR_CODE);
+               HVM_DELIVER_NO_ERROR_CODE, source);
             return;
         }
     }
@@ -1288,7 +1290,7 @@ void vmx_inject_nmi(void)
         if ( pin_based_cntrl & PIN_BASED_NMI_EXITING ) {
             nvmx_enqueue_n2_exceptions (v, 
                INTR_INFO_VALID_MASK | (X86_EVENTTYPE_NMI<<8) | TRAP_nmi,
-               HVM_DELIVER_NO_ERROR_CODE);
+               HVM_DELIVER_NO_ERROR_CODE, hvm_intsrc_nmi);
             return;
         }
     }
@@ -1356,7 +1358,7 @@ static void vmx_inject_trap(struct hvm_trap *trap)
     {
         nvmx_enqueue_n2_exceptions (curr, 
             INTR_INFO_VALID_MASK | (_trap.type<<8) | _trap.vector,
-            _trap.error_code); 
+            _trap.error_code, hvm_intsrc_none);
         return;
     }
     else
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h
b/xen/include/asm-x86/hvm/vmx/vmx.h
index c33b9f9..f4d759b 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -448,7 +448,7 @@ static inline int __vmxon(u64 addr)
 
 void vmx_get_segment_register(struct vcpu *, enum x86_segment,
                               struct segment_register *);
-void vmx_inject_extint(int trap);
+void vmx_inject_extint(int trap, uint8_t source);
 void vmx_inject_nmi(void);
 
 int ept_p2m_init(struct p2m_domain *p2m);
diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h
b/xen/include/asm-x86/hvm/vmx/vvmx.h
index 3874525..be2b5c6 100644
--- a/xen/include/asm-x86/hvm/vmx/vvmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
@@ -36,6 +36,7 @@ struct nestedvmx {
     struct {
         unsigned long intr_info;
         u32           error_code;
+        uint8_t       source;
     } intr;
     struct {
         bool_t   enabled;
-- 
1.7.1

From: Yang Zhang <yang.z.zhang@Intel.com>

In some special cases, we want to ack irq regardless of virtual interrupt
delivery.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/irq.c           |    2 +-
 xen/arch/x86/hvm/vlapic.c        |    4 ++--
 xen/include/asm-x86/hvm/vlapic.h |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 9eae5de..6a6fb68 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -437,7 +437,7 @@ struct hvm_intack hvm_vcpu_ack_pending_irq(
             intack.vector = (uint8_t)vector;
         break;
     case hvm_intsrc_lapic:
-        if ( !vlapic_ack_pending_irq(v, intack.vector) )
+        if ( !vlapic_ack_pending_irq(v, intack.vector, 0) )
             intack = hvm_intack_none;
         break;
     case hvm_intsrc_vector:
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 7a154f9..20a36a0 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1048,11 +1048,11 @@ int vlapic_has_pending_irq(struct vcpu *v)
     return irr;
 }
 
-int vlapic_ack_pending_irq(struct vcpu *v, int vector)
+int vlapic_ack_pending_irq(struct vcpu *v, int vector, int force_ack)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
 
-    if ( vlapic_virtual_intr_delivery_enabled() )
+    if ( vlapic_virtual_intr_delivery_enabled() && !force_ack )
         return 1;
 
     vlapic_set_vector(vector, &vlapic->regs->data[APIC_ISR]);
diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index 021a5f2..d8c9511 100644
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -96,7 +96,7 @@ bool_t is_vlapic_lvtpc_enabled(struct vlapic *vlapic);
 void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig);
 
 int vlapic_has_pending_irq(struct vcpu *v);
-int vlapic_ack_pending_irq(struct vcpu *v, int vector);
+int vlapic_ack_pending_irq(struct vcpu *v, int vector, int force_ack);
 
 int  vlapic_init(struct vcpu *v);
 void vlapic_destroy(struct vcpu *v);
-- 
1.7.1

From: Yang Zhang <yang.z.zhang@Intel.com>

If L2 is running, external interrupt is allowed to notify CPU only
when it has higher priority than current in servicing interrupt. Since
there is no vAPIC-v for L2, so force check isr when L2 is running.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/vlapic.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 20a36a0..f2594dd 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -37,6 +37,7 @@
 #include <asm/hvm/io.h>
 #include <asm/hvm/support.h>
 #include <asm/hvm/vmx/vmx.h>
+#include <asm/hvm/nestedhvm.h>
 #include <public/hvm/ioreq.h>
 #include <public/hvm/params.h>
 
@@ -1037,7 +1038,8 @@ int vlapic_has_pending_irq(struct vcpu *v)
     if ( irr == -1 )
         return -1;
 
-    if ( vlapic_virtual_intr_delivery_enabled() )
+    if ( vlapic_virtual_intr_delivery_enabled() &&
+            !nestedhvm_vcpu_in_guestmode(v) )
         return irr;
 
     isr = vlapic_find_highest_isr(vlapic);
-- 
1.7.1

From: Yang Zhang <yang.z.zhang@Intel.com>

Add vlapic_set_ppr() to allow update vPPR which in virtual apic page.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/vlapic.c        |    8 ++++++++
 xen/include/asm-x86/hvm/vlapic.h |    1 +
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index f2594dd..caab9ea 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -168,6 +168,14 @@ static uint32_t vlapic_get_ppr(struct vlapic *vlapic)
     return ppr;
 }
 
+uint32_t vlapic_set_ppr(struct vlapic *vlapic)
+{
+   uint32_t ppr = vlapic_get_ppr(vlapic);
+   vlapic_set_reg(vlapic, APIC_PROCPRI, ppr);
+
+   return ppr;
+}
+
 static int vlapic_match_logical_addr(struct vlapic *vlapic, uint8_t mda)
 {
     int result = 0;
diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index d8c9511..6109137 100644
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -108,6 +108,7 @@ void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t
value);
 uint64_t vlapic_tdt_msr_get(struct vlapic *vlapic);
 
 int vlapic_accept_pic_intr(struct vcpu *v);
+uint32_t vlapic_set_ppr(struct vlapic *vlapic);
 
 void vlapic_adjust_i8259_target(struct domain *d);
 
-- 
1.7.1

From: Yang Zhang <yang.z.zhang@Intel.com>

If interrupt is blocked by L1''s TPR, L2 should not see it and keep
running. Adding the check before L2 to retrive interrupt.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/vmx/intr.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index cb120f2..8853939 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -165,6 +165,11 @@ static int nvmx_intr_intercept(struct vcpu *v, struct
hvm_intack intack)
 {
     u32 ctrl;
 
+    /* If blocked by L1''s tpr, then do nothing*/
+    if ( nestedhvm_vcpu_in_guestmode(v) &&
+            hvm_interrupt_blocked(v, intack) == hvm_intblk_tpr )
+        return 1;
+
     if ( nvmx_intr_blocked(v) != hvm_intblk_none )
     {
         enable_intr_window(v, intack);
-- 
1.7.1

From: Yang Zhang <yang.z.zhang@Intel.com>

If enabling APIC-v, all interrupts to L1 is delivered through APIC-v.
But when L2 is running, external interrupt will casue L1 vmexit with
reason external interrupt. Then L1 will pick up the interrupt through
vmcs. So we need to update APIC-v related structure accordingly;

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c |   37 +++++++++++++++++++++++++++++++++++++
 1 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 5dfbc54..9ba169d 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1283,6 +1283,41 @@ static void sync_exception_state(struct vcpu *v)
     }
 }
 
+static void nvmx_update_apicv(struct vcpu *v)
+{
+    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
+    struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
+    unsigned long reason = __get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_REASON);
+    uint32_t intr_info = __get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_INTR_INFO);
+
+    if ( !cpu_has_vmx_virtual_intr_delivery )
+        return;
+
+    if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
+            nvmx->intr.source == hvm_intsrc_lapic &&
+            (intr_info & INTR_INFO_VALID_MASK) )
+    {
+        unsigned long status;
+        uint32_t rvi, ppr;
+        uint32_t vector = intr_info & 0xff;
+        struct vlapic *vlapic = vcpu_vlapic(v);
+
+        WARN_ON(vector <= 0);
+
+        vlapic_ack_pending_irq(v, vector, 1);
+
+        ppr = vlapic_set_ppr(vlapic);
+        BUG_ON( (ppr & 0xf0) != (vector & 0xf0) );
+
+        status = (vector << 8) & 0xff00;
+        rvi = vlapic_has_pending_irq(v);
+        if ( rvi != -1 )
+            status |= rvi & 0xff;
+
+        __vmwrite(GUEST_INTR_STATUS, status);
+    }
+}
+
 static void virtual_vmexit(struct cpu_user_regs *regs)
 {
     struct vcpu *v = current;
@@ -1328,6 +1363,8 @@ static void virtual_vmexit(struct cpu_user_regs *regs)
     /* updating host cr0 to sync TS bit */
     __vmwrite(HOST_CR0, v->arch.hvm_vmx.host_cr0);
 
+    nvmx_update_apicv(v);
+
     vmreturn(regs, VMSUCCEED);
 }
 
-- 
1.7.1

From: Yang Zhang <yang.z.zhang@Intel.com>

There is no vAPIC-v supporting, so mask APIC-v control bit when
constructing vmcs02.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 9ba169d..eed09be 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -617,6 +617,8 @@ void nvmx_update_secondary_exec_control(struct vcpu *v,
     shadow_cntrl = __get_vvmcs(nvcpu->nv_vvmcx, SECONDARY_VM_EXEC_CONTROL);
     nvmx->ept.enabled = !!(shadow_cntrl & SECONDARY_EXEC_ENABLE_EPT);
     shadow_cntrl |= host_cntrl;
+    shadow_cntrl &= ~(SECONDARY_EXEC_APIC_REGISTER_VIRT |
+            SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
     __vmwrite(SECONDARY_VM_EXEC_CONTROL, shadow_cntrl);
 }
 
@@ -627,6 +629,7 @@ static void nvmx_update_pin_control(struct vcpu *v, unsigned
long host_cntrl)
 
     shadow_cntrl = __get_vvmcs(nvcpu->nv_vvmcx, PIN_BASED_VM_EXEC_CONTROL);
     shadow_cntrl |= host_cntrl;
+    shadow_cntrl &= ~PIN_BASED_POSTED_INTERRUPT;
     __vmwrite(PIN_BASED_VM_EXEC_CONTROL, shadow_cntrl);
 }
 
-- 
1.7.1

On 09/08/13 09:49, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
>
> Introduce interrupt source to determine the source of interrupt that inject
> to L1.
>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/hvm/vmx/intr.c        |    4 ++--
>  xen/arch/x86/hvm/vmx/vmx.c         |   14 ++++++++------
>  xen/include/asm-x86/hvm/vmx/vmx.h  |    2 +-
>  xen/include/asm-x86/hvm/vmx/vvmx.h |    1 +
>  4 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> index e376f3c..cb120f2 100644
> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c
> @@ -180,7 +180,7 @@ static int nvmx_intr_intercept(struct vcpu *v, struct
hvm_intack intack)
>              if ( !(ctrl & PIN_BASED_EXT_INTR_MASK) )
>                  return 0;
>  
> -            vmx_inject_extint(intack.vector);
> +            vmx_inject_extint(intack.vector, intack.source);
>  
>              ctrl = __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx,
VM_EXIT_CONTROLS);
>              if ( ctrl & VM_EXIT_ACK_INTR_ON_EXIT )
> @@ -309,7 +309,7 @@ void vmx_intr_assist(void)
>      else
>      {
>          HVMTRACE_2D(INJ_VIRQ, intack.vector, /*fake=*/ 0);
> -        vmx_inject_extint(intack.vector);
> +        vmx_inject_extint(intack.vector, intack.source);
>          pt_intr_post(v, intack);
>      }
>  
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 8ed7026..51c657f 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1208,7 +1208,7 @@ static void vmx_update_guest_efer(struct vcpu *v)
>  }
>  
>  void nvmx_enqueue_n2_exceptions(struct vcpu *v, 
> -            unsigned long intr_fields, int error_code)
> +            unsigned long intr_fields, int error_code, uint8_t source)
>  {
>      struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
>  
> @@ -1216,6 +1216,7 @@ void nvmx_enqueue_n2_exceptions(struct vcpu *v,
>          /* enqueue the exception till the VMCS switch back to L1 */
>          nvmx->intr.intr_info = intr_fields;
>          nvmx->intr.error_code = error_code;
> +        nvmx->intr.source = source;
>          vcpu_nestedhvm(v).nv_vmexit_pending = 1;
>          return;
>      }
> @@ -1227,7 +1228,8 @@ void nvmx_enqueue_n2_exceptions(struct vcpu *v,
>  
>  static int nvmx_vmexit_trap(struct vcpu *v, struct hvm_trap *trap)
>  {
> -    nvmx_enqueue_n2_exceptions(v, trap->vector, trap->error_code);
> +    nvmx_enqueue_n2_exceptions(v, trap->vector, trap->error_code,
> +            hvm_intsrc_none);
Alignment here...
>      return NESTEDHVM_VMEXIT_DONE;
>  }
>  
> @@ -1258,7 +1260,7 @@ static void __vmx_inject_exception(int trap, int
type, int error_code)
>          curr->arch.hvm_vmx.vmx_emulate = 1;
>  }
>  
> -void vmx_inject_extint(int trap)
> +void vmx_inject_extint(int trap, uint8_t source)
>  {
>      struct vcpu *v = current;
>      u32    pin_based_cntrl;
> @@ -1269,7 +1271,7 @@ void vmx_inject_extint(int trap)
>          if ( pin_based_cntrl & PIN_BASED_EXT_INTR_MASK ) {
>              nvmx_enqueue_n2_exceptions (v, 
>                 INTR_INFO_VALID_MASK | (X86_EVENTTYPE_EXT_INTR<<8) |
trap,
> -               HVM_DELIVER_NO_ERROR_CODE);
> +               HVM_DELIVER_NO_ERROR_CODE, source);
And these 3 hunks.

~Andrew
>              return;
>          }
>      }
> @@ -1288,7 +1290,7 @@ void vmx_inject_nmi(void)
>          if ( pin_based_cntrl & PIN_BASED_NMI_EXITING ) {
>              nvmx_enqueue_n2_exceptions (v, 
>                 INTR_INFO_VALID_MASK | (X86_EVENTTYPE_NMI<<8) |
TRAP_nmi,
> -               HVM_DELIVER_NO_ERROR_CODE);
> +               HVM_DELIVER_NO_ERROR_CODE, hvm_intsrc_nmi);
>              return;
>          }
>      }
> @@ -1356,7 +1358,7 @@ static void vmx_inject_trap(struct hvm_trap *trap)
>      {
>          nvmx_enqueue_n2_exceptions (curr, 
>              INTR_INFO_VALID_MASK | (_trap.type<<8) | _trap.vector,
> -            _trap.error_code); 
> +            _trap.error_code, hvm_intsrc_none);
>          return;
>      }
>      else
> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h
b/xen/include/asm-x86/hvm/vmx/vmx.h
> index c33b9f9..f4d759b 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -448,7 +448,7 @@ static inline int __vmxon(u64 addr)
>  
>  void vmx_get_segment_register(struct vcpu *, enum x86_segment,
>                                struct segment_register *);
> -void vmx_inject_extint(int trap);
> +void vmx_inject_extint(int trap, uint8_t source);
>  void vmx_inject_nmi(void);
>  
>  int ept_p2m_init(struct p2m_domain *p2m);
> diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h
b/xen/include/asm-x86/hvm/vmx/vvmx.h
> index 3874525..be2b5c6 100644
> --- a/xen/include/asm-x86/hvm/vmx/vvmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
> @@ -36,6 +36,7 @@ struct nestedvmx {
>      struct {
>          unsigned long intr_info;
>          u32           error_code;
> +        uint8_t       source;
>      } intr;
>      struct {
>          bool_t   enabled;

On 09/08/13 09:49, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
>
> In some special cases, we want to ack irq regardless of virtual interrupt
delivery.
Can you explain why and when we might want to force an ack?
>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/hvm/irq.c           |    2 +-
>  xen/arch/x86/hvm/vlapic.c        |    4 ++--
>  xen/include/asm-x86/hvm/vlapic.h |    2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
> index 9eae5de..6a6fb68 100644
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -437,7 +437,7 @@ struct hvm_intack hvm_vcpu_ack_pending_irq(
>              intack.vector = (uint8_t)vector;
>          break;
>      case hvm_intsrc_lapic:
> -        if ( !vlapic_ack_pending_irq(v, intack.vector) )
> +        if ( !vlapic_ack_pending_irq(v, intack.vector, 0) )
>              intack = hvm_intack_none;
>          break;
>      case hvm_intsrc_vector:
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 7a154f9..20a36a0 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1048,11 +1048,11 @@ int vlapic_has_pending_irq(struct vcpu *v)
>      return irr;
>  }
>  
> -int vlapic_ack_pending_irq(struct vcpu *v, int vector)
> +int vlapic_ack_pending_irq(struct vcpu *v, int vector, int force_ack)
>  {
>      struct vlapic *vlapic = vcpu_vlapic(v);
>  
> -    if ( vlapic_virtual_intr_delivery_enabled() )
> +    if ( vlapic_virtual_intr_delivery_enabled() && !force_ack )
>          return 1;
The logic in this entire function seems quite backwards.  It
unconditionally returns 1 in all cases, and its sole callsite is in an
if statement.

Something like:

void vlapic_ack_pending_irq(struct vcpu *v, uint8_t vector, bool_t
force_ack)
{
    struct vlapic *vlapic = vcpu_vlapic(v);

    if ( force_ack || !vlapic_virtual_intr_delivery_enabled() )
    {
        vlapic_set_vector(vector, &vlapic->regs->data[APIC_ISR]);
        vlapic_clear_irr(vector, vlapic);
    }
}

Seems substantially clearer.

~Andrew
>  
>      vlapic_set_vector(vector, &vlapic->regs->data[APIC_ISR]);
> diff --git a/xen/include/asm-x86/hvm/vlapic.h
b/xen/include/asm-x86/hvm/vlapic.h
> index 021a5f2..d8c9511 100644
> --- a/xen/include/asm-x86/hvm/vlapic.h
> +++ b/xen/include/asm-x86/hvm/vlapic.h
> @@ -96,7 +96,7 @@ bool_t is_vlapic_lvtpc_enabled(struct vlapic *vlapic);
>  void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig);
>  
>  int vlapic_has_pending_irq(struct vcpu *v);
> -int vlapic_ack_pending_irq(struct vcpu *v, int vector);
> +int vlapic_ack_pending_irq(struct vcpu *v, int vector, int force_ack);
>  
>  int  vlapic_init(struct vcpu *v);
>  void vlapic_destroy(struct vcpu *v);

Andrew Cooper wrote on 2013-08-09:
> On 09/08/13 09:49, Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> In some special cases, we want to ack irq regardless of virtual
>> interrupt
> delivery.
> 
> Can you explain why and when we might want to force an ack?
After vritual_vmexit, if the interrupt already is already recorded in vmcs12.
Then we should clear the irr and set isr regardless
APIC-v.
> 
>> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> ---
>>  xen/arch/x86/hvm/irq.c           |    2 +-
>>  xen/arch/x86/hvm/vlapic.c        |    4 ++--
>>  xen/include/asm-x86/hvm/vlapic.h |    2 +-
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>> diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c index
>> 9eae5de..6a6fb68 100644
>> --- a/xen/arch/x86/hvm/irq.c
>> +++ b/xen/arch/x86/hvm/irq.c
>> @@ -437,7 +437,7 @@ struct hvm_intack hvm_vcpu_ack_pending_irq(
>>              intack.vector = (uint8_t)vector;
>>          break;
>>      case hvm_intsrc_lapic:
>> -        if ( !vlapic_ack_pending_irq(v, intack.vector) )
>> +        if ( !vlapic_ack_pending_irq(v, intack.vector, 0) )
>>              intack = hvm_intack_none;
>>          break;
>>      case hvm_intsrc_vector:
>> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
>> index 7a154f9..20a36a0 100644
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -1048,11 +1048,11 @@ int vlapic_has_pending_irq(struct vcpu *v)
>>      return irr;
>>  }
>> -int vlapic_ack_pending_irq(struct vcpu *v, int vector)
>> +int vlapic_ack_pending_irq(struct vcpu *v, int vector, int
>> +force_ack)
>>  {
>>      struct vlapic *vlapic = vcpu_vlapic(v);
>> -    if ( vlapic_virtual_intr_delivery_enabled() )
>> +    if ( vlapic_virtual_intr_delivery_enabled() && !force_ack
)
>>          return 1;
> 
> The logic in this entire function seems quite backwards.  It
> unconditionally returns 1 in all cases, and its sole callsite is in an if
statement.
> 
> Something like:
> 
> void vlapic_ack_pending_irq(struct vcpu *v, uint8_t vector, bool_t
> force_ack) {
>     struct vlapic *vlapic = vcpu_vlapic(v);
>     
>     if ( force_ack || !vlapic_virtual_intr_delivery_enabled() )
>     {
>         vlapic_set_vector(vector, &vlapic->regs->data[APIC_ISR]);
>         vlapic_clear_irr(vector, vlapic);
>     }
> }
> 
> Seems substantially clearer.
> 
> ~Andrew
> 
>> 
>>      vlapic_set_vector(vector,
&vlapic->regs->data[APIC_ISR]); diff
>> --git a/xen/include/asm-x86/hvm/vlapic.h
>> b/xen/include/asm-x86/hvm/vlapic.h
>> index 021a5f2..d8c9511 100644
>> --- a/xen/include/asm-x86/hvm/vlapic.h
>> +++ b/xen/include/asm-x86/hvm/vlapic.h
>> @@ -96,7 +96,7 @@ bool_t is_vlapic_lvtpc_enabled(struct vlapic
>> *vlapic);  void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec,
>> uint8_t trig);
>> 
>>  int vlapic_has_pending_irq(struct vcpu *v); -int
>> vlapic_ack_pending_irq(struct vcpu *v, int vector);
>> +int vlapic_ack_pending_irq(struct vcpu *v, int vector, int
>> +force_ack);
>> 
>>  int  vlapic_init(struct vcpu *v);
>>  void vlapic_destroy(struct vcpu *v);

Best regards,
Yang

On 09/08/13 09:49, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
>
> If L2 is running, external interrupt is allowed to notify CPU only
> when it has higher priority than current in servicing interrupt. Since
> there is no vAPIC-v for L2, so force check isr when L2 is running.
>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/hvm/vlapic.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 20a36a0..f2594dd 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -37,6 +37,7 @@
>  #include <asm/hvm/io.h>
>  #include <asm/hvm/support.h>
>  #include <asm/hvm/vmx/vmx.h>
> +#include <asm/hvm/nestedhvm.h>
>  #include <public/hvm/ioreq.h>
>  #include <public/hvm/params.h>
>  
> @@ -1037,7 +1038,8 @@ int vlapic_has_pending_irq(struct vcpu *v)
>      if ( irr == -1 )
>          return -1;
>  
> -    if ( vlapic_virtual_intr_delivery_enabled() )
> +    if ( vlapic_virtual_intr_delivery_enabled() &&
> +            !nestedhvm_vcpu_in_guestmode(v) )
Alignment, but otherwise ok.

~Andrew
>          return irr;
>  
>      isr = vlapic_find_highest_isr(vlapic);

On 09/08/13 09:49, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
>
> Add vlapic_set_ppr() to allow update vPPR which in virtual apic page.
>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/hvm/vlapic.c        |    8 ++++++++
>  xen/include/asm-x86/hvm/vlapic.h |    1 +
>  2 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index f2594dd..caab9ea 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -168,6 +168,14 @@ static uint32_t vlapic_get_ppr(struct vlapic *vlapic)
>      return ppr;
>  }
>  
> +uint32_t vlapic_set_ppr(struct vlapic *vlapic)
> +{
> +   uint32_t ppr = vlapic_get_ppr(vlapic);
> +   vlapic_set_reg(vlapic, APIC_PROCPRI, ppr);
> +
> +   return ppr;
> +}
> +
>  static int vlapic_match_logical_addr(struct vlapic *vlapic, uint8_t mda)
>  {
>      int result = 0;
> diff --git a/xen/include/asm-x86/hvm/vlapic.h
b/xen/include/asm-x86/hvm/vlapic.h
> index d8c9511..6109137 100644
> --- a/xen/include/asm-x86/hvm/vlapic.h
> +++ b/xen/include/asm-x86/hvm/vlapic.h
> @@ -108,6 +108,7 @@ void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t
value);
>  uint64_t vlapic_tdt_msr_get(struct vlapic *vlapic);
>  
>  int vlapic_accept_pic_intr(struct vcpu *v);
> +uint32_t vlapic_set_ppr(struct vlapic *vlapic);
>  
>  void vlapic_adjust_i8259_target(struct domain *d);
>

On 09/08/13 09:49, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
>
> If interrupt is blocked by L1''s TPR, L2 should not see it and keep
> running. Adding the check before L2 to retrive interrupt.
>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/hvm/vmx/intr.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> index cb120f2..8853939 100644
> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c
> @@ -165,6 +165,11 @@ static int nvmx_intr_intercept(struct vcpu *v, struct
hvm_intack intack)
>  {
>      u32 ctrl;
>  
> +    /* If blocked by L1''s tpr, then do nothing*/
As you need to respin, please have a space at the end of the sentence
before */
> +    if ( nestedhvm_vcpu_in_guestmode(v) &&
> +            hvm_interrupt_blocked(v, intack) == hvm_intblk_tpr )
Alignment

~Andrew
> +        return 1;
> +
>      if ( nvmx_intr_blocked(v) != hvm_intblk_none )
>      {
>          enable_intr_window(v, intack);

On 09/08/13 09:49, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
>
> If enabling APIC-v, all interrupts to L1 is delivered through APIC-v.
> But when L2 is running, external interrupt will casue L1 vmexit with
> reason external interrupt. Then L1 will pick up the interrupt through
> vmcs. So we need to update APIC-v related structure accordingly;
>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c |   37 +++++++++++++++++++++++++++++++++++++
>  1 files changed, 37 insertions(+), 0 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 5dfbc54..9ba169d 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1283,6 +1283,41 @@ static void sync_exception_state(struct vcpu *v)
>      }
>  }
>  
> +static void nvmx_update_apicv(struct vcpu *v)
> +{
> +    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
> +    struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
> +    unsigned long reason = __get_vvmcs(nvcpu->nv_vvmcx,
VM_EXIT_REASON);
> +    uint32_t intr_info = __get_vvmcs(nvcpu->nv_vvmcx,
VM_EXIT_INTR_INFO);
> +
> +    if ( !cpu_has_vmx_virtual_intr_delivery )
> +        return;
Is it worth hoisting this check outside of the function, to avoid
needless __get_vvmcs() calls on some hardware?
> +
> +    if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
> +            nvmx->intr.source == hvm_intsrc_lapic &&
> +            (intr_info & INTR_INFO_VALID_MASK) )
> +    {
> +        unsigned long status;
> +        uint32_t rvi, ppr;
> +        uint32_t vector = intr_info & 0xff;
> +        struct vlapic *vlapic = vcpu_vlapic(v);
> +
> +        WARN_ON(vector <= 0);
> +
> +        vlapic_ack_pending_irq(v, vector, 1);
> +
> +        ppr = vlapic_set_ppr(vlapic);
> +        BUG_ON( (ppr & 0xf0) != (vector & 0xf0) );
> +
> +        status = (vector << 8) & 0xff00;
Given "vector = intr_info & 0xff;" above, this "&
0xff00" looks useless.

~Andrew
> +        rvi = vlapic_has_pending_irq(v);
> +        if ( rvi != -1 )
> +            status |= rvi & 0xff;
> +
> +        __vmwrite(GUEST_INTR_STATUS, status);
> +    }
> +}
> +
>  static void virtual_vmexit(struct cpu_user_regs *regs)
>  {
>      struct vcpu *v = current;
> @@ -1328,6 +1363,8 @@ static void virtual_vmexit(struct cpu_user_regs
*regs)
>      /* updating host cr0 to sync TS bit */
>      __vmwrite(HOST_CR0, v->arch.hvm_vmx.host_cr0);
>  
> +    nvmx_update_apicv(v);
> +
>      vmreturn(regs, VMSUCCEED);
>  }
>

On 09/08/13 09:49, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
>
> There is no vAPIC-v supporting, so mask APIC-v control bit when
> constructing vmcs02.
>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 9ba169d..eed09be 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -617,6 +617,8 @@ void nvmx_update_secondary_exec_control(struct vcpu *v,
>      shadow_cntrl = __get_vvmcs(nvcpu->nv_vvmcx,
SECONDARY_VM_EXEC_CONTROL);
>      nvmx->ept.enabled = !!(shadow_cntrl &
SECONDARY_EXEC_ENABLE_EPT);
>      shadow_cntrl |= host_cntrl;
> +    shadow_cntrl &= ~(SECONDARY_EXEC_APIC_REGISTER_VIRT |
> +            SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
Alignment again, but otherwise ok.

~Andrew
>      __vmwrite(SECONDARY_VM_EXEC_CONTROL, shadow_cntrl);
>  }
>  
> @@ -627,6 +629,7 @@ static void nvmx_update_pin_control(struct vcpu *v,
unsigned long host_cntrl)
>  
>      shadow_cntrl = __get_vvmcs(nvcpu->nv_vvmcx,
PIN_BASED_VM_EXEC_CONTROL);
>      shadow_cntrl |= host_cntrl;
> +    shadow_cntrl &= ~PIN_BASED_POSTED_INTERRUPT;
>      __vmwrite(PIN_BASED_VM_EXEC_CONTROL, shadow_cntrl);
>  }
>

>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@intel.com>
wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
I said this to you and your colleagues a number of times before:
You should copy the VMX maintainers on your patch submissions
touching VMX code. I''m not going to invest further effort in chasing
their acks for your patches.

Jan

>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@intel.com>
wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> Introduce interrupt source to determine the source of interrupt that inject
> to L1.
So where''s the consumer of that new structure field? Afaics you
only set it, but never read it.

Jan
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/hvm/vmx/intr.c        |    4 ++--
>  xen/arch/x86/hvm/vmx/vmx.c         |   14 ++++++++------
>  xen/include/asm-x86/hvm/vmx/vmx.h  |    2 +-
>  xen/include/asm-x86/hvm/vmx/vvmx.h |    1 +
>  4 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> index e376f3c..cb120f2 100644
> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c
> @@ -180,7 +180,7 @@ static int nvmx_intr_intercept(struct vcpu *v, struct 
> hvm_intack intack)
>              if ( !(ctrl & PIN_BASED_EXT_INTR_MASK) )
>                  return 0;
>  
> -            vmx_inject_extint(intack.vector);
> +            vmx_inject_extint(intack.vector, intack.source);
>  
>              ctrl = __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx, 
> VM_EXIT_CONTROLS);
>              if ( ctrl & VM_EXIT_ACK_INTR_ON_EXIT )
> @@ -309,7 +309,7 @@ void vmx_intr_assist(void)
>      else
>      {
>          HVMTRACE_2D(INJ_VIRQ, intack.vector, /*fake=*/ 0);
> -        vmx_inject_extint(intack.vector);
> +        vmx_inject_extint(intack.vector, intack.source);
>          pt_intr_post(v, intack);
>      }
>  
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 8ed7026..51c657f 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1208,7 +1208,7 @@ static void vmx_update_guest_efer(struct vcpu *v)
>  }
>  
>  void nvmx_enqueue_n2_exceptions(struct vcpu *v, 
> -            unsigned long intr_fields, int error_code)
> +            unsigned long intr_fields, int error_code, uint8_t source)
>  {
>      struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
>  
> @@ -1216,6 +1216,7 @@ void nvmx_enqueue_n2_exceptions(struct vcpu *v,
>          /* enqueue the exception till the VMCS switch back to L1 */
>          nvmx->intr.intr_info = intr_fields;
>          nvmx->intr.error_code = error_code;
> +        nvmx->intr.source = source;
>          vcpu_nestedhvm(v).nv_vmexit_pending = 1;
>          return;
>      }
> @@ -1227,7 +1228,8 @@ void nvmx_enqueue_n2_exceptions(struct vcpu *v,
>  
>  static int nvmx_vmexit_trap(struct vcpu *v, struct hvm_trap *trap)
>  {
> -    nvmx_enqueue_n2_exceptions(v, trap->vector, trap->error_code);
> +    nvmx_enqueue_n2_exceptions(v, trap->vector, trap->error_code,
> +            hvm_intsrc_none);
>      return NESTEDHVM_VMEXIT_DONE;
>  }
>  
> @@ -1258,7 +1260,7 @@ static void __vmx_inject_exception(int trap, int
type,
> int error_code)
>          curr->arch.hvm_vmx.vmx_emulate = 1;
>  }
>  
> -void vmx_inject_extint(int trap)
> +void vmx_inject_extint(int trap, uint8_t source)
>  {
>      struct vcpu *v = current;
>      u32    pin_based_cntrl;
> @@ -1269,7 +1271,7 @@ void vmx_inject_extint(int trap)
>          if ( pin_based_cntrl & PIN_BASED_EXT_INTR_MASK ) {
>              nvmx_enqueue_n2_exceptions (v, 
>                 INTR_INFO_VALID_MASK | (X86_EVENTTYPE_EXT_INTR<<8) |
trap,
> -               HVM_DELIVER_NO_ERROR_CODE);
> +               HVM_DELIVER_NO_ERROR_CODE, source);
>              return;
>          }
>      }
> @@ -1288,7 +1290,7 @@ void vmx_inject_nmi(void)
>          if ( pin_based_cntrl & PIN_BASED_NMI_EXITING ) {
>              nvmx_enqueue_n2_exceptions (v, 
>                 INTR_INFO_VALID_MASK | (X86_EVENTTYPE_NMI<<8) |
TRAP_nmi,
> -               HVM_DELIVER_NO_ERROR_CODE);
> +               HVM_DELIVER_NO_ERROR_CODE, hvm_intsrc_nmi);
>              return;
>          }
>      }
> @@ -1356,7 +1358,7 @@ static void vmx_inject_trap(struct hvm_trap *trap)
>      {
>          nvmx_enqueue_n2_exceptions (curr, 
>              INTR_INFO_VALID_MASK | (_trap.type<<8) | _trap.vector,
> -            _trap.error_code); 
> +            _trap.error_code, hvm_intsrc_none);
>          return;
>      }
>      else
> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h 
> b/xen/include/asm-x86/hvm/vmx/vmx.h
> index c33b9f9..f4d759b 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -448,7 +448,7 @@ static inline int __vmxon(u64 addr)
>  
>  void vmx_get_segment_register(struct vcpu *, enum x86_segment,
>                                struct segment_register *);
> -void vmx_inject_extint(int trap);
> +void vmx_inject_extint(int trap, uint8_t source);
>  void vmx_inject_nmi(void);
>  
>  int ept_p2m_init(struct p2m_domain *p2m);
> diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h 
> b/xen/include/asm-x86/hvm/vmx/vvmx.h
> index 3874525..be2b5c6 100644
> --- a/xen/include/asm-x86/hvm/vmx/vvmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
> @@ -36,6 +36,7 @@ struct nestedvmx {
>      struct {
>          unsigned long intr_info;
>          u32           error_code;
> +        uint8_t       source;
>      } intr;
>      struct {
>          bool_t   enabled;
> -- 
> 1.7.1

>>> On 09.08.13 at 12:32, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
> Andrew Cooper wrote on 2013-08-09:
>> On 09/08/13 09:49, Yang Zhang wrote:
>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>> 
>>> In some special cases, we want to ack irq regardless of virtual
>>> interrupt
>> delivery.
>> 
>> Can you explain why and when we might want to force an ack?
> After vritual_vmexit, if the interrupt already is already recorded in 
> vmcs12. Then we should clear the irr and set isr regardless APIC-v.
So please add this to the commit message.

Jan

>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@intel.com>
wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> In some special cases, we want to ack irq regardless of virtual interrupt 
> delivery.
Again, the whole change is meaningless. I can see reasons to break
out such preparatory changes when otherwise the resulting patch
would be huge and hard to review. That doesn''t seem to be the
case here; it rather looks like the splitting was done pretty arbitrarily
here.

Jan
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/hvm/irq.c           |    2 +-
>  xen/arch/x86/hvm/vlapic.c        |    4 ++--
>  xen/include/asm-x86/hvm/vlapic.h |    2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
> index 9eae5de..6a6fb68 100644
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -437,7 +437,7 @@ struct hvm_intack hvm_vcpu_ack_pending_irq(
>              intack.vector = (uint8_t)vector;
>          break;
>      case hvm_intsrc_lapic:
> -        if ( !vlapic_ack_pending_irq(v, intack.vector) )
> +        if ( !vlapic_ack_pending_irq(v, intack.vector, 0) )
>              intack = hvm_intack_none;
>          break;
>      case hvm_intsrc_vector:
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 7a154f9..20a36a0 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1048,11 +1048,11 @@ int vlapic_has_pending_irq(struct vcpu *v)
>      return irr;
>  }
>  
> -int vlapic_ack_pending_irq(struct vcpu *v, int vector)
> +int vlapic_ack_pending_irq(struct vcpu *v, int vector, int force_ack)
>  {
>      struct vlapic *vlapic = vcpu_vlapic(v);
>  
> -    if ( vlapic_virtual_intr_delivery_enabled() )
> +    if ( vlapic_virtual_intr_delivery_enabled() && !force_ack )
>          return 1;
>  
>      vlapic_set_vector(vector, &vlapic->regs->data[APIC_ISR]);
> diff --git a/xen/include/asm-x86/hvm/vlapic.h
b/xen/include/asm-x86/hvm/vlapic.h
> index 021a5f2..d8c9511 100644
> --- a/xen/include/asm-x86/hvm/vlapic.h
> +++ b/xen/include/asm-x86/hvm/vlapic.h
> @@ -96,7 +96,7 @@ bool_t is_vlapic_lvtpc_enabled(struct vlapic *vlapic);
>  void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig);
>  
>  int vlapic_has_pending_irq(struct vcpu *v);
> -int vlapic_ack_pending_irq(struct vcpu *v, int vector);
> +int vlapic_ack_pending_irq(struct vcpu *v, int vector, int force_ack);
>  
>  int  vlapic_init(struct vcpu *v);
>  void vlapic_destroy(struct vcpu *v);
> -- 
> 1.7.1

>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@intel.com>
wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> If L2 is running, external interrupt is allowed to notify CPU only
> when it has higher priority than current in servicing interrupt.
That''s always this way, not just for nested HVM. Hence there
must either be some piece missing from this explanation, or
the check is being inserted in the wrong place.

Jan
> Since
> there is no vAPIC-v for L2, so force check isr when L2 is running.
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/hvm/vlapic.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 20a36a0..f2594dd 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -37,6 +37,7 @@
>  #include <asm/hvm/io.h>
>  #include <asm/hvm/support.h>
>  #include <asm/hvm/vmx/vmx.h>
> +#include <asm/hvm/nestedhvm.h>
>  #include <public/hvm/ioreq.h>
>  #include <public/hvm/params.h>
>  
> @@ -1037,7 +1038,8 @@ int vlapic_has_pending_irq(struct vcpu *v)
>      if ( irr == -1 )
>          return -1;
>  
> -    if ( vlapic_virtual_intr_delivery_enabled() )
> +    if ( vlapic_virtual_intr_delivery_enabled() &&
> +            !nestedhvm_vcpu_in_guestmode(v) )
>          return irr;
>  
>      isr = vlapic_find_highest_isr(vlapic);
> -- 
> 1.7.1

>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@intel.com>
wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> Add vlapic_set_ppr() to allow update vPPR which in virtual apic page.
Again, missing the consumer of this new function.
> +uint32_t vlapic_set_ppr(struct vlapic *vlapic)
> +{
> +   uint32_t ppr = vlapic_get_ppr(vlapic);
> +   vlapic_set_reg(vlapic, APIC_PROCPRI, ppr);
Blank line needed between variable declarations and first
statement.

Jan

>>> On 09.08.13 at 12:43, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/hvm/vmx/intr.c
>> +++ b/xen/arch/x86/hvm/vmx/intr.c
>> @@ -165,6 +165,11 @@ static int nvmx_intr_intercept(struct vcpu *v,
struct hvm_intack intack)
>>  {
>>      u32 ctrl;
>>  
>> +    /* If blocked by L1''s tpr, then do nothing*/
> 
> As you need to respin, please have a space at the end of the sentence
> before */
And a period. See CODING_STYLE. Also, I think rather than "do
nothing", "nothing to do" would better describe things here.

Jan

>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@intel.com>
wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> If enabling APIC-v, all interrupts to L1 is delivered through APIC-v.
> But when L2 is running, external interrupt will casue L1 vmexit with
> reason external interrupt. Then L1 will pick up the interrupt through
> vmcs. So we need to update APIC-v related structure accordingly;
This doesn''t sound logical to me: If L1 picks up the interrupt from
VMCS, how can the be a reason/explanation for the need to update
the APIC-v related data?
> +static void nvmx_update_apicv(struct vcpu *v)
> +{
> +    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
> +    struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
> +    unsigned long reason = __get_vvmcs(nvcpu->nv_vvmcx,
VM_EXIT_REASON);
> +    uint32_t intr_info = __get_vvmcs(nvcpu->nv_vvmcx,
VM_EXIT_INTR_INFO);
> +
> +    if ( !cpu_has_vmx_virtual_intr_delivery )
> +        return;
> +
> +    if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
> +            nvmx->intr.source == hvm_intsrc_lapic &&
> +            (intr_info & INTR_INFO_VALID_MASK) )
> +    {
> +        unsigned long status;
> +        uint32_t rvi, ppr;
> +        uint32_t vector = intr_info & 0xff;
> +        struct vlapic *vlapic = vcpu_vlapic(v);
> +
> +        WARN_ON(vector <= 0);
For both this ...
> +
> +        vlapic_ack_pending_irq(v, vector, 1);
> +
> +        ppr = vlapic_set_ppr(vlapic);
> +        BUG_ON( (ppr & 0xf0) != (vector & 0xf0) );
... and this: Is it guaranteed that the guest have no influence on
the participating values? Because otherwise either or both are
security issues.

It also looks inconsistent to me that the former is a WARN_ON()
while the latter is a BUG_ON().

Besides that I agree with all of Andrew''s comments.

Jan

>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@intel.com>
wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -617,6 +617,8 @@ void nvmx_update_secondary_exec_control(struct vcpu *v,
>      shadow_cntrl = __get_vvmcs(nvcpu->nv_vvmcx,
SECONDARY_VM_EXEC_CONTROL);
>      nvmx->ept.enabled = !!(shadow_cntrl &
SECONDARY_EXEC_ENABLE_EPT);
>      shadow_cntrl |= host_cntrl;
> +    shadow_cntrl &= ~(SECONDARY_EXEC_APIC_REGISTER_VIRT |
> +            SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
>      __vmwrite(SECONDARY_VM_EXEC_CONTROL, shadow_cntrl);
>  }
>  
> @@ -627,6 +629,7 @@ static void nvmx_update_pin_control(struct vcpu *v, 
> unsigned long host_cntrl)
>  
>      shadow_cntrl = __get_vvmcs(nvcpu->nv_vvmcx,
PIN_BASED_VM_EXEC_CONTROL);
>      shadow_cntrl |= host_cntrl;
> +    shadow_cntrl &= ~PIN_BASED_POSTED_INTERRUPT;
>      __vmwrite(PIN_BASED_VM_EXEC_CONTROL, shadow_cntrl);
>  }
>  
I can see why you want to mask the bit off of host_cntrl, but is it
really correct to also mask it when set in the vVMCS? Shouldn''t
that rather result in a fault raised to it? (If that''s already the case
- I just don''t know the nested code well enough yet - then this
would still call for being adjusted logically: Mask the bit when or-ing
in host_cntrl, and assert that the bit is clear in what you read from
vVMCS. This would make much more obvious what the intentions
here are.

Jan

Jan Beulich wrote on 2013-08-09:
>>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@intel.com>
wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> Introduce interrupt source to determine the source of interrupt that
>> inject to L1.
> 
> So where''s the consumer of that new structure field? Afaics you
only
> set it, but never read it.
It is used in patch 6. I will mention it in the comments in next patch.
> 
> Jan
> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> ---
>>  xen/arch/x86/hvm/vmx/intr.c        |    4 ++--
>>  xen/arch/x86/hvm/vmx/vmx.c         |   14 ++++++++------
>>  xen/include/asm-x86/hvm/vmx/vmx.h  |    2 +-
>>  xen/include/asm-x86/hvm/vmx/vvmx.h |    1 +
>>  4 files changed, 12 insertions(+), 9 deletions(-)
>> diff --git a/xen/arch/x86/hvm/vmx/intr.c
>> b/xen/arch/x86/hvm/vmx/intr.c index e376f3c..cb120f2 100644
>> --- a/xen/arch/x86/hvm/vmx/intr.c
>> +++ b/xen/arch/x86/hvm/vmx/intr.c
>> @@ -180,7 +180,7 @@ static int nvmx_intr_intercept(struct vcpu *v,
>> struct hvm_intack intack)
>>              if ( !(ctrl & PIN_BASED_EXT_INTR_MASK) )
>>                  return 0;
>> -            vmx_inject_extint(intack.vector);
>> +            vmx_inject_extint(intack.vector, intack.source);
>> 
>>              ctrl = __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx,
>>              VM_EXIT_CONTROLS); if ( ctrl &
VM_EXIT_ACK_INTR_ON_EXIT )
>>              @@ -309,7 +309,7
>> @@ void vmx_intr_assist(void)
>>      else
>>      {
>>          HVMTRACE_2D(INJ_VIRQ, intack.vector, /*fake=*/ 0);
>> -        vmx_inject_extint(intack.vector);
>> +        vmx_inject_extint(intack.vector, intack.source);
>>          pt_intr_post(v, intack);
>>      }
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 8ed7026..51c657f 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -1208,7 +1208,7 @@ static void vmx_update_guest_efer(struct vcpu
>> *v)  }
>> 
>>  void nvmx_enqueue_n2_exceptions(struct vcpu *v,
>> -            unsigned long intr_fields, int error_code)
>> +            unsigned long intr_fields, int error_code, uint8_t
>> + source)
>>  {
>>      struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
>> @@ -1216,6 +1216,7 @@ void nvmx_enqueue_n2_exceptions(struct vcpu
> *v,
>>          /* enqueue the exception till the VMCS switch back to L1 */
>>          nvmx->intr.intr_info = intr_fields;
nvmx->intr.error_code >>          error_code; +       
nvmx->intr.source = source;
>>          vcpu_nestedhvm(v).nv_vmexit_pending = 1; return;
>>      }
>> @@ -1227,7 +1228,8 @@ void nvmx_enqueue_n2_exceptions(struct vcpu *v,
>> 
>>  static int nvmx_vmexit_trap(struct vcpu *v, struct hvm_trap *trap)  {
>> -    nvmx_enqueue_n2_exceptions(v, trap->vector,
trap->error_code);
>> +    nvmx_enqueue_n2_exceptions(v, trap->vector,
trap->error_code,
>> +            hvm_intsrc_none);
>>      return NESTEDHVM_VMEXIT_DONE;
>>  }
>> @@ -1258,7 +1260,7 @@ static void __vmx_inject_exception(int trap,
>> int type, int error_code)
>>          curr->arch.hvm_vmx.vmx_emulate = 1;  }
>> -void vmx_inject_extint(int trap)
>> +void vmx_inject_extint(int trap, uint8_t source)
>>  {
>>      struct vcpu *v = current;
>>      u32    pin_based_cntrl;
>> @@ -1269,7 +1271,7 @@ void vmx_inject_extint(int trap)
>>          if ( pin_based_cntrl & PIN_BASED_EXT_INTR_MASK ) {
>>              nvmx_enqueue_n2_exceptions (v,
>>                 INTR_INFO_VALID_MASK |
> (X86_EVENTTYPE_EXT_INTR<<8) | trap,
>> -               HVM_DELIVER_NO_ERROR_CODE);
>> +               HVM_DELIVER_NO_ERROR_CODE, source);
>>              return;
>>          }
>>      }
>> @@ -1288,7 +1290,7 @@ void vmx_inject_nmi(void)
>>          if ( pin_based_cntrl & PIN_BASED_NMI_EXITING ) {
>>              nvmx_enqueue_n2_exceptions (v,
>>                 INTR_INFO_VALID_MASK | (X86_EVENTTYPE_NMI<<8) |
> TRAP_nmi,
>> -               HVM_DELIVER_NO_ERROR_CODE);
>> +               HVM_DELIVER_NO_ERROR_CODE, hvm_intsrc_nmi);
>>              return;
>>          }
>>      }
>> @@ -1356,7 +1358,7 @@ static void vmx_inject_trap(struct hvm_trap
> *trap)
>>      {
>>          nvmx_enqueue_n2_exceptions (curr,
>>              INTR_INFO_VALID_MASK | (_trap.type<<8) |
_trap.vector,
>> -            _trap.error_code);
>> +            _trap.error_code, hvm_intsrc_none);
>>          return;
>>      }
>>      else
>> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h
>> b/xen/include/asm-x86/hvm/vmx/vmx.h
>> index c33b9f9..f4d759b 100644
>> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
>> @@ -448,7 +448,7 @@ static inline int __vmxon(u64 addr)
>> 
>>  void vmx_get_segment_register(struct vcpu *, enum x86_segment,
>>                                struct segment_register *); -void
>> vmx_inject_extint(int trap);
>> +void vmx_inject_extint(int trap, uint8_t source);
>>  void vmx_inject_nmi(void);
>>  
>>  int ept_p2m_init(struct p2m_domain *p2m); diff --git
>> a/xen/include/asm-x86/hvm/vmx/vvmx.h
>> b/xen/include/asm-x86/hvm/vmx/vvmx.h
>> index 3874525..be2b5c6 100644
>> --- a/xen/include/asm-x86/hvm/vmx/vvmx.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
>> @@ -36,6 +36,7 @@ struct nestedvmx {
>>      struct {
>>          unsigned long intr_info;
>>          u32           error_code;
>> +        uint8_t       source;
>>      } intr;
>>      struct {
>>          bool_t   enabled;
>> --
>> 1.7.1
>

Best regards,
Yang

Jan Beulich wrote on 2013-08-09:
>>>> On 09.08.13 at 12:32, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
>> Andrew Cooper wrote on 2013-08-09:
>>> On 09/08/13 09:49, Yang Zhang wrote:
>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>> 
>>>> In some special cases, we want to ack irq regardless of virtual
>>>> interrupt
>>> delivery.
>>> 
>>> Can you explain why and when we might want to force an ack?
>> After vritual_vmexit, if the interrupt already is already recorded
>> in vmcs12. Then we should clear the irr and set isr regardless APIC-v.
> 
> So please add this to the commit message.
Sure.

Best regards,
Yang

Jan Beulich wrote on 2013-08-09:
>>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@intel.com>
wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> In some special cases, we want to ack irq regardless of virtual
>> interrupt delivery.
> 
> Again, the whole change is meaningless. I can see reasons to break out
> such preparatory changes when otherwise the resulting patch would be
> huge and hard to review. That doesn''t seem to be the case here; it
> rather looks like the splitting was done pretty arbitrarily here.
No, splitting the patch is not only for easy reviewing. It is useful for
"git bisect" and debug purpose. Also, if the change is independent, we
also should to split it into a separate patch.
Here, though the change is minimal, it touches the key interrupt handle logic.
It''s better to put it into a single patch.
	
> Jan
> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> ---
>>  xen/arch/x86/hvm/irq.c           |    2 +-
>>  xen/arch/x86/hvm/vlapic.c        |    4 ++--
>>  xen/include/asm-x86/hvm/vlapic.h |    2 +-
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>> diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c index
>> 9eae5de..6a6fb68 100644
>> --- a/xen/arch/x86/hvm/irq.c
>> +++ b/xen/arch/x86/hvm/irq.c
>> @@ -437,7 +437,7 @@ struct hvm_intack hvm_vcpu_ack_pending_irq(
>>              intack.vector = (uint8_t)vector;
>>          break;
>>      case hvm_intsrc_lapic:
>> -        if ( !vlapic_ack_pending_irq(v, intack.vector) )
>> +        if ( !vlapic_ack_pending_irq(v, intack.vector, 0) )
>>              intack = hvm_intack_none;
>>          break;
>>      case hvm_intsrc_vector:
>> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
>> index 7a154f9..20a36a0 100644
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -1048,11 +1048,11 @@ int vlapic_has_pending_irq(struct vcpu *v)
>>      return irr;
>>  }
>> -int vlapic_ack_pending_irq(struct vcpu *v, int vector)
>> +int vlapic_ack_pending_irq(struct vcpu *v, int vector, int
>> +force_ack)
>>  {
>>      struct vlapic *vlapic = vcpu_vlapic(v);
>> -    if ( vlapic_virtual_intr_delivery_enabled() )
>> +    if ( vlapic_virtual_intr_delivery_enabled() && !force_ack
)
>>          return 1;
>>      vlapic_set_vector(vector,
&vlapic->regs->data[APIC_ISR]); diff
>> --git a/xen/include/asm-x86/hvm/vlapic.h
>> b/xen/include/asm-x86/hvm/vlapic.h
>> index 021a5f2..d8c9511 100644
>> --- a/xen/include/asm-x86/hvm/vlapic.h
>> +++ b/xen/include/asm-x86/hvm/vlapic.h
>> @@ -96,7 +96,7 @@ bool_t is_vlapic_lvtpc_enabled(struct vlapic
>> *vlapic);  void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec,
>> uint8_t trig);
>> 
>>  int vlapic_has_pending_irq(struct vcpu *v); -int
>> vlapic_ack_pending_irq(struct vcpu *v, int vector);
>> +int vlapic_ack_pending_irq(struct vcpu *v, int vector, int
>> +force_ack);
>> 
>>  int  vlapic_init(struct vcpu *v);
>>  void vlapic_destroy(struct vcpu *v);
>> --
>> 1.7.1
> 
>

Best regards,
Yang

Jan Beulich wrote on 2013-08-09:
>>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@intel.com>
wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> If L2 is running, external interrupt is allowed to notify CPU only
>> when it has higher priority than current in servicing interrupt.
> 
> That''s always this way, not just for nested HVM. Hence there must
> either be some piece missing from this explanation, or the check is
> being inserted in the wrong place.
How about change the comments like this:
External interrupt is allowed to notify CPU only when it has higher priority
than current in servicing interrupt. With APIC-v, the priority comparing is done
by hardware and hardware will inject the interrupt to VCPU when it recognizes an
interrupt. Currently, there is no virtual APIC-v feature available for L1, so
when L2 is running, we still need to compare interrupt priority with ISR in
hypervisor instead hardware.

> Jan
> 
>> Since
>> there is no vAPIC-v for L2, so force check isr when L2 is running.
>> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> ---
>>  xen/arch/x86/hvm/vlapic.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
>> index 20a36a0..f2594dd 100644
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -37,6 +37,7 @@
>>  #include <asm/hvm/io.h> #include <asm/hvm/support.h>
#include
>>  <asm/hvm/vmx/vmx.h> +#include <asm/hvm/nestedhvm.h>
#include
>>  <public/hvm/ioreq.h> #include <public/hvm/params.h>
>> @@ -1037,7 +1038,8 @@ int vlapic_has_pending_irq(struct vcpu *v)
>>      if ( irr == -1 )
>>          return -1;
>> -    if ( vlapic_virtual_intr_delivery_enabled() )
>> +    if ( vlapic_virtual_intr_delivery_enabled() &&
>> +            !nestedhvm_vcpu_in_guestmode(v) )
>>          return irr;
>>      isr = vlapic_find_highest_isr(vlapic);
>> --
>> 1.7.1
> 
>

Best regards,
Yang

Jan Beulich wrote on 2013-08-09:
>>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@intel.com>
wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> Add vlapic_set_ppr() to allow update vPPR which in virtual apic page.
> 
> Again, missing the consumer of this new function.
It is used in patch 6. I will mention it in comments.
> 
>> +uint32_t vlapic_set_ppr(struct vlapic *vlapic) {
>> +   uint32_t ppr = vlapic_get_ppr(vlapic);
>> +   vlapic_set_reg(vlapic, APIC_PROCPRI, ppr);
> 
> Blank line needed between variable declarations and first statement.
Sure.
> 
> Jan

Best regards,
Yang

Jan Beulich wrote on 2013-08-09:
>>>> On 09.08.13 at 12:43, Andrew Cooper
<andrew.cooper3@citrix.com>
> wrote:
>>> --- a/xen/arch/x86/hvm/vmx/intr.c
>>> +++ b/xen/arch/x86/hvm/vmx/intr.c
>>> @@ -165,6 +165,11 @@ static int nvmx_intr_intercept(struct vcpu *v,
>>> struct hvm_intack intack)  {
>>>      u32 ctrl;
>>> +    /* If blocked by L1''s tpr, then do nothing*/
>> 
>> As you need to respin, please have a space at the end of the
>> sentence before */
> 
> And a period. See CODING_STYLE. Also, I think rather than "do
> nothing", "nothing to do" would better describe things here.
Sure.

Best regards,
Yang

Jan Beulich wrote on 2013-08-09:
>>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@intel.com>
wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> If enabling APIC-v, all interrupts to L1 is delivered through APIC-v.
>> But when L2 is running, external interrupt will casue L1 vmexit with
>> reason external interrupt. Then L1 will pick up the interrupt
>> through vmcs. So we need to update APIC-v related structure
>> accordingly;
> 
> This doesn''t sound logical to me: If L1 picks up the interrupt
from
> VMCS, how can the be a reason/explanation for the need to update the
> APIC-v related data?
If L1 has the APIC-v, the interrupt is injected and acked by hardware normally.
In this case, L1 picks up the interrupt from VMCS, but it didn''t update
the APIC-v related filed. Then when L1 issue the EOI, since SVI is wrong, the
ISR will never be cleared. Also, if the PPR and RVI are wrong, the next
interrupt handle logic will totally wrong.
> 
>> +static void nvmx_update_apicv(struct vcpu *v) { +    struct nestedvmx
>> *nvmx = &vcpu_2_nvmx(v); +    struct nestedvcpu *nvcpu >>
&vcpu_nestedhvm(v); +    unsigned long reason >>
__get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_REASON); +    uint32_t intr_info
>> __get_vvmcs(nvcpu->nv_vvmcx, +VM_EXIT_INTR_INFO); + +    if (
>> !cpu_has_vmx_virtual_intr_delivery ) +        return; + +    if (
>> reason == EXIT_REASON_EXTERNAL_INTERRUPT && +           
>> nvmx->intr.source == hvm_intsrc_lapic && +           
(intr_info &
>> INTR_INFO_VALID_MASK) ) +    { +        unsigned long status; +       
>> uint32_t rvi, ppr; +        uint32_t vector = intr_info & 0xff; +
>>  struct vlapic *vlapic = vcpu_vlapic(v); + +        WARN_ON(vector
<>> 0);
> 
> For both this ...
> 
>> +
>> +        vlapic_ack_pending_irq(v, vector, 1);
>> +
>> +        ppr = vlapic_set_ppr(vlapic);
>> +        BUG_ON( (ppr & 0xf0) != (vector & 0xf0) );
> 
> ... and this: Is it guaranteed that the guest have no influence on the
> participating values? Because otherwise either or both are security issues.
> 
> It also looks inconsistent to me that the former is a WARN_ON() while
> the latter is a BUG_ON().
If the vector is wrong, it is handled in L1 not L0, so just using WARN_ON here
and L1 will handle it correctly. If PPR is wrong, then there should be somewhere
wrong in L0''s interrupt handle logic, so using BUG_ON.
> 
> Besides that I agree with all of Andrew''s comments.
> 
> Jan

Best regards,
Yang

Jan Beulich wrote on 2013-08-09:
>>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@intel.com>
wrote:
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -617,6 +617,8 @@ void nvmx_update_secondary_exec_control(struct
> vcpu *v,
>>      shadow_cntrl = __get_vvmcs(nvcpu->nv_vvmcx,
>>      SECONDARY_VM_EXEC_CONTROL); nvmx->ept.enabled = !!(shadow_cntrl
&
>>      SECONDARY_EXEC_ENABLE_EPT); shadow_cntrl |= host_cntrl;
>> +    shadow_cntrl &= ~(SECONDARY_EXEC_APIC_REGISTER_VIRT |
>> +            SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
>>      __vmwrite(SECONDARY_VM_EXEC_CONTROL, shadow_cntrl);  }
>> @@ -627,6 +629,7 @@ static void nvmx_update_pin_control(struct vcpu
>> *v, unsigned long host_cntrl)
>> 
>>      shadow_cntrl = __get_vvmcs(nvcpu->nv_vvmcx,
>>      PIN_BASED_VM_EXEC_CONTROL); shadow_cntrl |= host_cntrl; +   
>>      shadow_cntrl &= ~PIN_BASED_POSTED_INTERRUPT;
>>      __vmwrite(PIN_BASED_VM_EXEC_CONTROL, shadow_cntrl);  }
> 
> I can see why you want to mask the bit off of host_cntrl, but is it
> really correct to also mask it when set in the vVMCS? Shouldn''t
that
> rather result in a fault raised to it? (If that''s already the case
> - I just don''t know the nested code well enough yet - then this
would
> still call for being adjusted logically: Mask the bit when or-ing in
> host_cntrl, and assert that the bit is clear in what you read from
> vVMCS. This would make much more obvious what the intentions here are.
Sure, it sounds much reasonable.
> 
> Jan

Best regards,
Yang

>>> On 11.08.13 at 04:43, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2013-08-09:
>>>>> On 09.08.13 at 10:49, Yang Zhang
<yang.z.zhang@intel.com> wrote:
>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>> 
>>> In some special cases, we want to ack irq regardless of virtual
>>> interrupt delivery.
>> 
>> Again, the whole change is meaningless. I can see reasons to break out
>> such preparatory changes when otherwise the resulting patch would be
>> huge and hard to review. That doesn''t seem to be the case
here; it
>> rather looks like the splitting was done pretty arbitrarily here.
> No, splitting the patch is not only for easy reviewing. It is useful for 
> "git bisect" and debug purpose. Also, if the change is
independent, we also
> should to split it into a separate patch. 
> Here, though the change is minimal, it touches the key interrupt handle 
> logic. It''s better to put it into a single patch.
I continue to disagree.

Jan

>>> On 11.08.13 at 04:49, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2013-08-09:
>>>>> On 09.08.13 at 10:49, Yang Zhang
<yang.z.zhang@intel.com> wrote:
>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>> 
>>> If L2 is running, external interrupt is allowed to notify CPU only
>>> when it has higher priority than current in servicing interrupt.
>> 
>> That''s always this way, not just for nested HVM. Hence there
must
>> either be some piece missing from this explanation, or the check is
>> being inserted in the wrong place.
> How about change the comments like this:
> External interrupt is allowed to notify CPU only when it has higher
priority
> than current in servicing interrupt. With APIC-v, the priority comparing is
> done by hardware and hardware will inject the interrupt to VCPU when it 
> recognizes an interrupt. Currently, there is no virtual APIC-v feature 
> available for L1, so when L2 is running, we still need to compare interrupt
> priority with ISR in hypervisor instead hardware.
Yes, that would make things quite a bit more clear.

Jan

>>> On 11.08.13 at 04:59, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2013-08-09:
>>>>> On 09.08.13 at 10:49, Yang Zhang
<yang.z.zhang@intel.com> wrote:
>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>> 
>>> If enabling APIC-v, all interrupts to L1 is delivered through
APIC-v.
>>> But when L2 is running, external interrupt will casue L1 vmexit
with
>>> reason external interrupt. Then L1 will pick up the interrupt
>>> through vmcs. So we need to update APIC-v related structure
>>> accordingly;
>> 
>> This doesn''t sound logical to me: If L1 picks up the interrupt
from
>> VMCS, how can the be a reason/explanation for the need to update the
>> APIC-v related data?
> If L1 has the APIC-v, the interrupt is injected and acked by hardware 
> normally. In this case, L1 picks up the interrupt from VMCS, but it
didn''t
> update the APIC-v related filed. Then when L1 issue the EOI, since SVI is 
> wrong, the ISR will never be cleared. Also, if the PPR and RVI are wrong,
the
> next interrupt handle logic will totally wrong. 
Just saying the same with different wording doesn''t really help
me in this case...
>>> +static void nvmx_update_apicv(struct vcpu *v) { +    struct
nestedvmx
>>> *nvmx = &vcpu_2_nvmx(v); +    struct nestedvcpu *nvcpu
>>> &vcpu_nestedhvm(v); +    unsigned long reason >>>
__get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_REASON); +    uint32_t intr_info
>>> __get_vvmcs(nvcpu->nv_vvmcx, +VM_EXIT_INTR_INFO); + +    if (
>>> !cpu_has_vmx_virtual_intr_delivery ) +        return; + +    if (
>>> reason == EXIT_REASON_EXTERNAL_INTERRUPT && +           
>>> nvmx->intr.source == hvm_intsrc_lapic && +           
(intr_info &
>>> INTR_INFO_VALID_MASK) ) +    { +        unsigned long status; +
>>> uint32_t rvi, ppr; +        uint32_t vector = intr_info & 0xff;
+
>>>  struct vlapic *vlapic = vcpu_vlapic(v); + +        WARN_ON(vector
<>>> 0);
>> 
>> For both this ...
>> 
>>> +
>>> +        vlapic_ack_pending_irq(v, vector, 1);
>>> +
>>> +        ppr = vlapic_set_ppr(vlapic);
>>> +        BUG_ON( (ppr & 0xf0) != (vector & 0xf0) );
>> 
>> ... and this: Is it guaranteed that the guest have no influence on the
>> participating values? Because otherwise either or both are security
issues.
You didn''t reply to this at all.
>> It also looks inconsistent to me that the former is a WARN_ON() while
>> the latter is a BUG_ON().
> If the vector is wrong, it is handled in L1 not L0, so just using WARN_ON 
> here and L1 will handle it correctly. If PPR is wrong, then there should be
> somewhere wrong in L0''s interrupt handle logic, so using BUG_ON.
But you realize that if the warning triggers, it''s almost guaranteed
that the BUG_ON() would also trigger. So I continue to not be
convinced.

Jan

Jan Beulich wrote on 2013-08-12:
>>>> On 11.08.13 at 04:59, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2013-08-09:
>>>>>> On 09.08.13 at 10:49, Yang Zhang
<yang.z.zhang@intel.com> wrote:
>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>> 
>>>> If enabling APIC-v, all interrupts to L1 is delivered through
APIC-v.
>>>> But when L2 is running, external interrupt will casue L1 vmexit
>>>> with reason external interrupt. Then L1 will pick up the
interrupt
>>>> through vmcs. So we need to update APIC-v related structure
>>>> accordingly;
>>> 
>>> This doesn''t sound logical to me: If L1 picks up the
interrupt from
>>> VMCS, how can the be a reason/explanation for the need to update
>>> the APIC-v related data?
>> If L1 has the APIC-v, the interrupt is injected and acked by
>> hardware normally. In this case, L1 picks up the interrupt from
>> VMCS, but it didn''t update the APIC-v related filed. Then when
L1
>> issue the EOI, since SVI is wrong, the ISR will never be cleared.
>> Also, if the PPR and RVI are wrong, the next interrupt handle logic
will totally wrong.
> 
> Just saying the same with different wording doesn''t really help me
in
> this case...
Consider the following case:
virtual vmexit happen;
L1 running and the vmexit reason is external interrupt;
L1 read vector info from vmcs and go to the interrupt handler;
Interrupt handler issues EOI to ack this interrupt;
Then APIC-v hardware will trap the EOI and do vEOI update: set VISR[SVI]=0,
perform PPR update and deliver the next pending interrupt.

The problem is that, SVI is not set by hardware because the interrupt
isn''t delivered through APIC-v hardware. When software to ack the
interrupt, it will never clear the vISR successfully because the wrong SVI. So
we need to update the SVI/RVI/PPR to the right value just like the interrupt is
delivered through APIC-v hardware to make sure the following vEOI update and
vPPR update correctly.
> 
>>>> +static void nvmx_update_apicv(struct vcpu *v) { +    struct
>>>> nestedvmx *nvmx = &vcpu_2_nvmx(v); +    struct nestedvcpu
*nvcpu >>>> &vcpu_nestedhvm(v); +    unsigned long reason
>>>> __get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_REASON); +    uint32_t
intr_info
>>>> = __get_vvmcs(nvcpu->nv_vvmcx, +VM_EXIT_INTR_INFO); + +   
if (
>>>> !cpu_has_vmx_virtual_intr_delivery ) +        return; + +    if
(
>>>> reason == EXIT_REASON_EXTERNAL_INTERRUPT && +
nvmx->intr.source =>>>> hvm_intsrc_lapic && +           
(intr_info & INTR_INFO_VALID_MASK) )
>>>> +    { +        unsigned long status; + uint32_t rvi, ppr; +
>>>> uint32_t vector = intr_info & 0xff; +
>>>>  struct vlapic *vlapic = vcpu_vlapic(v); + +       
WARN_ON(vector <>>>> 0);
>>> 
>>> For both this ...
>>> 
>>>> +
>>>> +        vlapic_ack_pending_irq(v, vector, 1);
>>>> +
>>>> +        ppr = vlapic_set_ppr(vlapic);
>>>> +        BUG_ON( (ppr & 0xf0) != (vector & 0xf0) );
>>> 
>>> ... and this: Is it guaranteed that the guest have no influence on
>>> the participating values? Because otherwise either or both are
>>> security
> issues.
> 
> You didn''t reply to this at all.
The intr_info is set by hypervisor only. If guest is able to write it, then it
should be a bug in current Xen and may cause the security issue.
>>> It also looks inconsistent to me that the former is a WARN_ON()
>>> while the latter is a BUG_ON().
>> If the vector is wrong, it is handled in L1 not L0, so just using
>> WARN_ON here and L1 will handle it correctly. If PPR is wrong, then
>> there should be somewhere wrong in L0''s interrupt handle
logic, so
>> using
> BUG_ON.
> 
> But you realize that if the warning triggers, it''s almost
guaranteed
> that the
> BUG_ON() would also trigger. So I continue to not be convinced.
You are right. The two will be triggered at same time. Also, I realized that the
vector will never less than zero since I use uint32. So I will remove the
WARN_ON.
As you mentioned, to avoid the security issue, it''s better to use the
WARN_ON instead the BUG_ON.

Best regards,
Yang

Jan Beulich wrote on 2013-08-12:
>>>> On 11.08.13 at 04:43, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2013-08-09:
>>>>>> On 09.08.13 at 10:49, Yang Zhang
<yang.z.zhang@intel.com> wrote:
>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>> 
>>>> In some special cases, we want to ack irq regardless of virtual
>>>> interrupt delivery.
>>> 
>>> Again, the whole change is meaningless. I can see reasons to break
out
>>> such preparatory changes when otherwise the resulting patch would
be
>>> huge and hard to review. That doesn''t seem to be the case
here; it
>>> rather looks like the splitting was done pretty arbitrarily here.
>> No, splitting the patch is not only for easy reviewing. It is useful
for
>> "git bisect" and debug purpose. Also, if the change is
independent, we also
>> should to split it into a separate patch.
>> Here, though the change is minimal, it touches the key interrupt handle
>> logic. It''s better to put it into a single patch.
> 
> I continue to disagree.
OK. Then how about to merge it into patch 6?

Best regards,
Yang

>>> On 13.08.13 at 03:10, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2013-08-12:
>>>>> On 11.08.13 at 04:43, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
>>> Jan Beulich wrote on 2013-08-09:
>>>>>>> On 09.08.13 at 10:49, Yang Zhang
<yang.z.zhang@intel.com> wrote:
>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>> 
>>>>> In some special cases, we want to ack irq regardless of
virtual
>>>>> interrupt delivery.
>>>> 
>>>> Again, the whole change is meaningless. I can see reasons to
break out
>>>> such preparatory changes when otherwise the resulting patch
would be
>>>> huge and hard to review. That doesn''t seem to be the
case here; it
>>>> rather looks like the splitting was done pretty arbitrarily
here.
>>> No, splitting the patch is not only for easy reviewing. It is
useful for
>>> "git bisect" and debug purpose. Also, if the change is
independent, we also
>>> should to split it into a separate patch.
>>> Here, though the change is minimal, it touches the key interrupt
handle
>>> logic. It''s better to put it into a single patch.
>> 
>> I continue to disagree.
> OK. Then how about to merge it into patch 6?
Yes, that was I was asking for (patch 6 or wherever else the user
of the new code is).

Jan

Zhang, Yang Z wrote on 2013-08-13:
> Jan Beulich wrote on 2013-08-12:
>>>>> On 11.08.13 at 04:59, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
>>> Jan Beulich wrote on 2013-08-09:
>>>>>>> On 09.08.13 at 10:49, Yang Zhang
<yang.z.zhang@intel.com> wrote:
>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>> 
>>>>> If enabling APIC-v, all interrupts to L1 is delivered
through APIC-v.
>>>>> But when L2 is running, external interrupt will casue L1
vmexit
>>>>> with reason external interrupt. Then L1 will pick up the
>>>>> interrupt through vmcs. So we need to update APIC-v related
>>>>> structure accordingly;
>>>> 
>>>> This doesn''t sound logical to me: If L1 picks up the
interrupt
>>>> from VMCS, how can the be a reason/explanation for the need to
>>>> update the APIC-v related data?
>>> If L1 has the APIC-v, the interrupt is injected and acked by
>>> hardware normally. In this case, L1 picks up the interrupt from
>>> VMCS, but it didn''t update the APIC-v related filed. Then
when L1
>>> issue the EOI, since SVI is wrong, the ISR will never be cleared.
>>> Also, if the PPR and RVI are wrong, the next interrupt handle logic
>>> will totally
> wrong.
>> 
>> Just saying the same with different wording doesn''t really
help me
>> in this case...
> Consider the following case:
> virtual vmexit happen;
> L1 running and the vmexit reason is external interrupt;
> L1 read vector info from vmcs and go to the interrupt handler;
> Interrupt handler issues EOI to ack this interrupt; Then APIC-v
> hardware will trap the EOI and do vEOI update: set VISR[SVI]=0,
> perform PPR update and deliver the next pending interrupt.
> 
> The problem is that, SVI is not set by hardware because the interrupt
> isn''t delivered through APIC-v hardware. When software to ack the
> interrupt, it will never clear the vISR successfully because the wrong
> SVI. So we need to update the SVI/RVI/PPR to the right value just like
> the interrupt is delivered through APIC-v hardware to make sure the
> following vEOI update and vPPR update correctly.
> 
>> 
>>>>> +static void nvmx_update_apicv(struct vcpu *v) { +   
struct
>>>>> nestedvmx *nvmx = &vcpu_2_nvmx(v); +    struct
nestedvcpu *nvcpu >>>>> &vcpu_nestedhvm(v); +    unsigned
long reason >>>>> __get_vvmcs(nvcpu->nv_vvmcx,
VM_EXIT_REASON); +    uint32_t
>>>>> intr_info = __get_vvmcs(nvcpu->nv_vvmcx,
+VM_EXIT_INTR_INFO); + +
>>>>> if ( !cpu_has_vmx_virtual_intr_delivery ) +        return;
+ +    if
>>>>> ( reason == EXIT_REASON_EXTERNAL_INTERRUPT && +
nvmx->intr.source =>>>>> hvm_intsrc_lapic && +        
(intr_info & INTR_INFO_VALID_MASK)
>>>>> ) +    { +        unsigned long status; + uint32_t rvi,
ppr; +
>>>>> uint32_t vector = intr_info & 0xff; +
>>>>>  struct vlapic *vlapic = vcpu_vlapic(v); + +       
WARN_ON(vector <>>>>> 0);
>>>> 
>>>> For both this ...
>>>> 
>>>>> +
>>>>> +        vlapic_ack_pending_irq(v, vector, 1);
>>>>> +
>>>>> +        ppr = vlapic_set_ppr(vlapic);
>>>>> +        BUG_ON( (ppr & 0xf0) != (vector & 0xf0) );
>>>> 
>>>> ... and this: Is it guaranteed that the guest have no influence
on
>>>> the participating values? Because otherwise either or both are
>>>> security
>> issues.
>> 
>> You didn''t reply to this at all.
> The intr_info is set by hypervisor only. If guest is able to write it,
> then it should be a bug in current Xen and may cause the security issue.
> 
>>>> It also looks inconsistent to me that the former is a WARN_ON()
>>>> while the latter is a BUG_ON().
>>> If the vector is wrong, it is handled in L1 not L0, so just using
>>> WARN_ON here and L1 will handle it correctly. If PPR is wrong, then
>>> there should be somewhere wrong in L0''s interrupt handle
logic, so
>>> using
>> BUG_ON.
>> 
>> But you realize that if the warning triggers, it''s almost
guaranteed
>> that the
>> BUG_ON() would also trigger. So I continue to not be convinced.
> You are right. The two will be triggered at same time. Also, I
> realized that the vector will never less than zero since I use uint32.
> So I will remove the WARN_ON.
> As you mentioned, to avoid the security issue, it''s better to use
the
> WARN_ON instead the BUG_ON.
> 
> Best regards,
> Yang
Hi Jan,

Any concern about this patch? If no, I will send out the second version based on
current comments.

Best regards,
Yang

>>> On 15.08.13 at 03:41, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
> Any concern about this patch? If no, I will send out the second version 
> based on current comments.
I raised all concerns already, if you dealt with those that you didn''t
eliminate by explanation, then just go ahead with the next rev.

Jan