[PATCH 0/7] Nested VMX: APIC-v related bug fixing

[PATCH 0/7] Nested VMX: APIC-v related bug fixing

[Yang Zhang]

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

[PATCH 1/7] Nested VMX: Introduce interrupt source supporting

[Yang Zhang]

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

[PATCH 2/7] Nested VMX: Allow to ack irq even virtual intr delivery is enabled

[Yang Zhang]

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

[PATCH 3/7] Nested VMX: Force check ISR when L2 is running

[Yang Zhang]

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

[PATCH 4/7] Nested VMX: Add interface to update vPPR

[Yang Zhang]

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

[PATCH 5/7] Nested VMX: Check whether interrupt is blocked by TPR

[Yang Zhang]

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

[PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1

[Yang Zhang]

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

[PATCH 7/7] Nested VMX: Clear APIC-v control bit in vmcs02

[Yang Zhang]

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

Re: [PATCH 1/7] Nested VMX: Introduce interrupt source supporting

[Andrew Cooper]

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;

Re: [PATCH 2/7] Nested VMX: Allow to ack irq even virtual intr delivery is enabled

[Andrew Cooper]

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

Re: [PATCH 2/7] Nested VMX: Allow to ack irq even virtual intr delivery is enabled

[Zhang, Yang Z]

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

Re: [PATCH 3/7] Nested VMX: Force check ISR when L2 is running

[Andrew Cooper]

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

Re: [PATCH 4/7] Nested VMX: Add interface to update vPPR

[Andrew Cooper]

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

Re: [PATCH 5/7] Nested VMX: Check whether interrupt is blocked by TPR

[Andrew Cooper]

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

Re: [PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1

[Andrew Cooper]

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

Re: [PATCH 7/7] Nested VMX: Clear APIC-v control bit in vmcs02

[Andrew Cooper]

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

Re: [PATCH 0/7] Nested VMX: APIC-v related bug fixing

[Jan Beulich]

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

Re: [PATCH 1/7] Nested VMX: Introduce interrupt source supporting

[Jan Beulich]

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

Re: [PATCH 2/7] Nested VMX: Allow to ack irq even virtual intr delivery is enabled

[Jan Beulich]

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

Re: [PATCH 2/7] Nested VMX: Allow to ack irq even virtual intr delivery is enabled

[Jan Beulich]

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

Re: [PATCH 3/7] Nested VMX: Force check ISR when L2 is running

[Jan Beulich]

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

Re: [PATCH 4/7] Nested VMX: Add interface to update vPPR

[Jan Beulich]

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

Re: [PATCH 5/7] Nested VMX: Check whether interrupt is blocked by TPR

[Jan Beulich]

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

Re: [PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1

[Jan Beulich]

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

Re: [PATCH 7/7] Nested VMX: Clear APIC-v control bit in vmcs02

[Jan Beulich]

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

Re: [PATCH 1/7] Nested VMX: Introduce interrupt source supporting

[Zhang, Yang Z]

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

Re: [PATCH 2/7] Nested VMX: Allow to ack irq even virtual intr delivery is enabled

[Zhang, Yang Z]

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

Re: [PATCH 2/7] Nested VMX: Allow to ack irq even virtual intr delivery is enabled

[Zhang, Yang Z]

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

Re: [PATCH 3/7] Nested VMX: Force check ISR when L2 is running

[Zhang, Yang Z]

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

Re: [PATCH 4/7] Nested VMX: Add interface to update vPPR

[Zhang, Yang Z]

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

Re: [PATCH 5/7] Nested VMX: Check whether interrupt is blocked by TPR

[Zhang, Yang Z]

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

Re: [PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1

[Zhang, Yang Z]

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

Re: [PATCH 7/7] Nested VMX: Clear APIC-v control bit in vmcs02

[Zhang, Yang Z]

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

Re: [PATCH 2/7] Nested VMX: Allow to ack irq even virtual intr delivery is enabled

[Jan Beulich]

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

Re: [PATCH 3/7] Nested VMX: Force check ISR when L2 is running

[Jan Beulich]

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

Re: [PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1

[Jan Beulich]

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

Re: [PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1

[Zhang, Yang Z]

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

Re: [PATCH 2/7] Nested VMX: Allow to ack irq even virtual intr delivery is enabled

[Zhang, Yang Z]

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

Re: [PATCH 2/7] Nested VMX: Allow to ack irq even virtual intr delivery is enabled

[Jan Beulich]

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

Re: [PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1

[Zhang, Yang Z]

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

Re: [PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1

[Jan Beulich]

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