[ PATCH 2/2] xen: enable Virtual-interrupt delivery

[ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Li, Jiongxi]

Virtual interrupt delivery avoids Xen to inject vAPIC interrupts manually, which is fully taken care of by the hardware. This needs some special awareness into existing interrupr injection path:
For pending interrupt from vLAPIC, instead of direct injection, we may need update architecture specific indicators before resuming to guest.
Before returning to guest, RVI should be updated if any pending IRRs
EOI exit bitmap controls whether an EOI write should cause VM-Exit. If set, a trap-like induced EOI VM-Exit is triggered. The approach here is to manipulate EOI exit bitmap based on value of TMR. Level triggered irq requires a hook in vLAPIC EOI write, so that vIOAPIC EOI is triggered and emulated

Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
Signed-off-by: Jiongxi Li <jiongxi.li@intel.com>

diff -r cb821c24ca74 xen/arch/x86/hvm/irq.c
--- a/xen/arch/x86/hvm/irq.c  Fri Aug 31 09:30:38 2012 +0800
+++ b/xen/arch/x86/hvm/irq.c         Fri Aug 31 09:49:39 2012 +0800
@@ -452,7 +452,11 @@ struct hvm_intack hvm_vcpu_ack_pending_i

 int hvm_local_events_need_delivery(struct vcpu *v)
{
-    struct hvm_intack intack = hvm_vcpu_has_pending_irq(v);
+    struct hvm_intack intack;
+
+    pt_update_irq(v);
+
+    intack = hvm_vcpu_has_pending_irq(v);

     if ( likely(intack.source == hvm_intsrc_none) )
         return 0;
diff -r cb821c24ca74 xen/arch/x86/hvm/vlapic.c
--- a/xen/arch/x86/hvm/vlapic.c      Fri Aug 31 09:30:38 2012 +0800
+++ b/xen/arch/x86/hvm/vlapic.c   Fri Aug 31 09:49:39 2012 +0800
@@ -143,7 +143,16 @@ static int vlapic_find_highest_irr(struc
int vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
{
     if ( trig )
+    {
         vlapic_set_vector(vec, &vlapic->regs->data[APIC_TMR]);
+        if ( cpu_has_vmx_virtual_intr_delivery )
+            vmx_set_eoi_exit_bitmap(vlapic_vcpu(vlapic), vec);
+    }
+    else
+    {
+        if ( cpu_has_vmx_virtual_intr_delivery )
+            vmx_clear_eoi_exit_bitmap(vlapic_vcpu(vlapic), vec);
+    }

     /* We may need to wake up target vcpu, besides set pending bit here */
     return !vlapic_test_and_set_irr(vec, vlapic);
@@ -410,6 +419,22 @@ void vlapic_EOI_set(struct vlapic *vlapi
     hvm_dpci_msi_eoi(current->domain, vector);
}

+/*
+ * When "Virtual Interrupt Delivery" is enabled, this function is used
+ * to handle EOI-induced VM exit
+ */
+void vlapic_handle_EOI_induced_exit(struct vlapic *vlapic, int vector)
+{
+    ASSERT(cpu_has_vmx_virtual_intr_delivery);
+
+    if ( vlapic_test_and_clear_vector(vector, &vlapic->regs->data[APIC_TMR]) )
+    {
+        vioapic_update_EOI(vlapic_domain(vlapic), vector);
+    }
+
+    hvm_dpci_msi_eoi(current->domain, vector);
+}
+
int vlapic_ipi(
     struct vlapic *vlapic, uint32_t icr_low, uint32_t icr_high)
{
@@ -1014,6 +1039,9 @@ int vlapic_has_pending_irq(struct vcpu *
     if ( irr == -1 )
         return -1;

+    if ( cpu_has_vmx_virtual_intr_delivery )
+        return irr;
+
     isr = vlapic_find_highest_isr(vlapic);
     isr = (isr != -1) ? isr : 0;
     if ( (isr & 0xf0) >= (irr & 0xf0) )
@@ -1026,6 +1054,9 @@ int vlapic_ack_pending_irq(struct vcpu *
{
     struct vlapic *vlapic = vcpu_vlapic(v);

+    if ( cpu_has_vmx_virtual_intr_delivery )
+        return 1;
+
    vlapic_set_vector(vector, &vlapic->regs->data[APIC_ISR]);
     vlapic_clear_irr(vector, vlapic);

diff -r cb821c24ca74 xen/arch/x86/hvm/vmx/intr.c
--- a/xen/arch/x86/hvm/vmx/intr.c Fri Aug 31 09:30:38 2012 +0800
+++ b/xen/arch/x86/hvm/vmx/intr.c       Fri Aug 31 09:49:39 2012 +0800
@@ -227,19 +227,43 @@ void vmx_intr_assist(void)
             goto out;

         intblk = hvm_interrupt_blocked(v, intack);
-        if ( intblk == hvm_intblk_tpr )
+        if ( cpu_has_vmx_virtual_intr_delivery )
         {
-            ASSERT(vlapic_enabled(vcpu_vlapic(v)));
-            ASSERT(intack.source == hvm_intsrc_lapic);
-            tpr_threshold = intack.vector >> 4;
-            goto out;
+            /* Set "Interrupt-window exiting" for ExtINT */
+            if ( (intblk != hvm_intblk_none) &&
+                 ( (intack.source == hvm_intsrc_pic) ||
+                 ( intack.source == hvm_intsrc_vector) ) )
+            {
+                enable_intr_window(v, intack);
+                goto out;
+            }
+
+            if ( __vmread(VM_ENTRY_INTR_INFO) & INTR_INFO_VALID_MASK )
+            {
+                if ( (intack.source == hvm_intsrc_pic) ||
+                     (intack.source == hvm_intsrc_nmi) ||
+                     (intack.source == hvm_intsrc_mce) )
+                    enable_intr_window(v, intack);
+
+                goto out;
+            }
         }
+        else
+        {
+            if ( intblk == hvm_intblk_tpr )
+            {
+                ASSERT(vlapic_enabled(vcpu_vlapic(v)));
+                ASSERT(intack.source == hvm_intsrc_lapic);
+                tpr_threshold = intack.vector >> 4;
+                goto out;
+            }

-        if ( (intblk != hvm_intblk_none) ||
-             (__vmread(VM_ENTRY_INTR_INFO) & INTR_INFO_VALID_MASK) )
-        {
-            enable_intr_window(v, intack);
-            goto out;
+            if ( (intblk != hvm_intblk_none) ||
+                 (__vmread(VM_ENTRY_INTR_INFO) & INTR_INFO_VALID_MASK) )
+            {
+                enable_intr_window(v, intack);
+                goto out;
+            }
         }

         intack = hvm_vcpu_ack_pending_irq(v, intack);
@@ -253,6 +277,29 @@ void vmx_intr_assist(void)
     {
         hvm_inject_hw_exception(TRAP_machine_check, HVM_DELIVER_NO_ERROR_CODE);
     }
+    else if ( cpu_has_vmx_virtual_intr_delivery &&
+              intack.source != hvm_intsrc_pic &&
+              intack.source != hvm_intsrc_vector )
+    {
+        /* we need update the RVI field */
+        unsigned long status = __vmread(GUEST_INTR_STATUS);
+        status &= ~(unsigned long)0x0FF;
+        status |= (unsigned long)0x0FF & 
+                    intack.vector;
+        __vmwrite(GUEST_INTR_STATUS, status);
+        if (v->arch.hvm_vmx.eoi_exitmap_changed) {
+#define UPDATE_EOI_EXITMAP(v, e) {                             \
+        if (test_and_clear_bit(e, &(v).eoi_exitmap_changed)) {      \
+                __vmwrite(EOI_EXIT_BITMAP##e, (v).eoi_exit_bitmap[e]);}}
+
+                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 0);
+                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 1);
+                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 2);
+                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 3);
+        }
+
+        pt_intr_post(v, intack);
+    }
     else
     {
         HVMTRACE_2D(INJ_VIRQ, intack.vector, /*fake=*/ 0);
@@ -262,11 +309,16 @@ void vmx_intr_assist(void)

     /* Is there another IRQ to queue up behind this one? */
     intack = hvm_vcpu_has_pending_irq(v);
-    if ( unlikely(intack.source != hvm_intsrc_none) )
-        enable_intr_window(v, intack);
+    if ( !cpu_has_vmx_virtual_intr_delivery ||
+         intack.source == hvm_intsrc_pic ||
+         intack.source == hvm_intsrc_vector )
+    {
+        if ( unlikely(intack.source != hvm_intsrc_none) )
+            enable_intr_window(v, intack);
+    }

  out:
-    if ( cpu_has_vmx_tpr_shadow )
+    if ( !cpu_has_vmx_virtual_intr_delivery && cpu_has_vmx_tpr_shadow )
         __vmwrite(TPR_THRESHOLD, tpr_threshold);
}

diff -r cb821c24ca74 xen/arch/x86/hvm/vmx/vmcs.c
--- a/xen/arch/x86/hvm/vmx/vmcs.c       Fri Aug 31 09:30:38 2012 +0800
+++ b/xen/arch/x86/hvm/vmx/vmcs.c    Fri Aug 31 09:49:39 2012 +0800
@@ -90,6 +90,7 @@ static void __init vmx_display_features(
     P(cpu_has_vmx_msr_bitmap, "MSR direct-access bitmap");
     P(cpu_has_vmx_unrestricted_guest, "Unrestricted Guest");
     P(cpu_has_vmx_apic_reg_virt, "APIC Register Virtualization");
+    P(cpu_has_vmx_virtual_intr_delivery, "Virtual Interrupt Delivery");
#undef P

     if ( !printed )
@@ -188,11 +189,12 @@ static int vmx_init_vmcs_config(void)
             opt |= SECONDARY_EXEC_UNRESTRICTED_GUEST;

         /*
-         * "APIC Register Virtualization"
+         * "APIC Register Virtualization" and "Virtual Interrupt Delivery"
          * can be set only when "use TPR shadow" is set
          */
         if ( _vmx_cpu_based_exec_control & CPU_BASED_TPR_SHADOW )
-            opt |= SECONDARY_EXEC_APIC_REGISTER_VIRT;
+            opt |= SECONDARY_EXEC_APIC_REGISTER_VIRT |
+                   SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY;

 
         _vmx_secondary_exec_control = adjust_vmx_controls(
@@ -787,6 +789,22 @@ static int construct_vmcs(struct vcpu *v
     __vmwrite(IO_BITMAP_A, virt_to_maddr((char *)hvm_io_bitmap + 0));
     __vmwrite(IO_BITMAP_B, virt_to_maddr((char *)hvm_io_bitmap + PAGE_SIZE));

+    if ( cpu_has_vmx_virtual_intr_delivery )
+    {
+        /* EOI-exit bitmap */
+        v->arch.hvm_vmx.eoi_exit_bitmap[0] = (uint64_t)0;
+        __vmwrite(EOI_EXIT_BITMAP0, v->arch.hvm_vmx.eoi_exit_bitmap[0]);
+        v->arch.hvm_vmx.eoi_exit_bitmap[1] = (uint64_t)0;
+        __vmwrite(EOI_EXIT_BITMAP1, v->arch.hvm_vmx.eoi_exit_bitmap[1]);
+        v->arch.hvm_vmx.eoi_exit_bitmap[2] = (uint64_t)0;
+        __vmwrite(EOI_EXIT_BITMAP2, v->arch.hvm_vmx.eoi_exit_bitmap[2]);
+        v->arch.hvm_vmx.eoi_exit_bitmap[3] = (uint64_t)0;
+        __vmwrite(EOI_EXIT_BITMAP3, v->arch.hvm_vmx.eoi_exit_bitmap[3]);
+
+        /* Initialise Guest Interrupt Status (RVI and SVI) to 0 */
+        __vmwrite(GUEST_INTR_STATUS, 0);
+    }
+
     /* Host data selectors. */
     __vmwrite(HOST_SS_SELECTOR, __HYPERVISOR_DS);
     __vmwrite(HOST_DS_SELECTOR, __HYPERVISOR_DS);
@@ -1028,6 +1046,30 @@ int vmx_add_host_load_msr(u32 msr)
     return 0;
}

+void vmx_set_eoi_exit_bitmap(struct vcpu *v, u8 vector)
+{
+    int index, offset, changed;
+
+    index = vector >> 6; 
+    offset = vector & 63;
+    changed = !test_and_set_bit(offset,
+                  (uint64_t *)&v->arch.hvm_vmx.eoi_exit_bitmap[index]);
+    if (changed)
+        set_bit(index, &v->arch.hvm_vmx.eoi_exitmap_changed);
+}
+
+void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector)
+{
+    int index, offset, changed;
+
+    index = vector >> 6; 
+    offset = vector & 63;
+    changed = test_and_clear_bit(offset,
+                  (uint64_t *)&v->arch.hvm_vmx.eoi_exit_bitmap[index]);
+    if (changed)
+        set_bit(index, &v->arch.hvm_vmx.eoi_exitmap_changed);
+}
+
int vmx_create_vmcs(struct vcpu *v)
{
     struct arch_vmx_struct *arch_vmx = &v->arch.hvm_vmx;
diff -r cb821c24ca74 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c         Fri Aug 31 09:30:38 2012 +0800
+++ b/xen/arch/x86/hvm/vmx/vmx.c      Fri Aug 31 09:49:39 2012 +0800
@@ -2674,6 +2674,16 @@ void vmx_vmexit_handler(struct cpu_user_
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
         break;

+    case EXIT_REASON_EOI_INDUCED:
+    {
+        int vector;
+        exit_qualification = __vmread(EXIT_QUALIFICATION);
+        vector = exit_qualification & 0xff;
+
+        vlapic_handle_EOI_induced_exit(vcpu_vlapic(current), vector);
+        break;
+    }
+
     case EXIT_REASON_IO_INSTRUCTION:
         exit_qualification = __vmread(EXIT_QUALIFICATION);
         if ( exit_qualification & 0x10 )
diff -r cb821c24ca74 xen/include/asm-x86/hvm/vlapic.h
--- a/xen/include/asm-x86/hvm/vlapic.h Fri Aug 31 09:30:38 2012 +0800
+++ b/xen/include/asm-x86/hvm/vlapic.h       Fri Aug 31 09:49:39 2012 +0800
@@ -100,6 +100,7 @@ int vlapic_accept_pic_intr(struct vcpu *
void vlapic_adjust_i8259_target(struct domain *d);

 void vlapic_EOI_set(struct vlapic *vlapic);
+void vlapic_handle_EOI_induced_exit(struct vlapic *vlapic, int vector);

 int vlapic_ipi(struct vlapic *vlapic, uint32_t icr_low, uint32_t icr_high);

diff -r cb821c24ca74 xen/include/asm-x86/hvm/vmx/vmcs.h
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h  Fri Aug 31 09:30:38 2012 +0800
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h        Fri Aug 31 09:49:39 2012 +0800
@@ -110,6 +110,9 @@ struct arch_vmx_struct {
     unsigned int         host_msr_count;
     struct vmx_msr_entry *host_msr_area;

+    uint32_t             eoi_exitmap_changed;
+    uint64_t             eoi_exit_bitmap[4];
+
     unsigned long        host_cr0;

     /* Is the guest in real mode? */
@@ -183,6 +186,7 @@ extern u32 vmx_vmentry_control;
#define SECONDARY_EXEC_WBINVD_EXITING           0x00000040
#define SECONDARY_EXEC_UNRESTRICTED_GUEST       0x00000080
#define SECONDARY_EXEC_APIC_REGISTER_VIRT       0x00000100
+#define SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY    0x00000200
#define SECONDARY_EXEC_PAUSE_LOOP_EXITING       0x00000400
#define SECONDARY_EXEC_ENABLE_INVPCID           0x00001000
extern u32 vmx_secondary_exec_control;
@@ -233,6 +237,8 @@ extern bool_t cpu_has_vmx_ins_outs_instr
     (vmx_secondary_exec_control & SECONDARY_EXEC_PAUSE_LOOP_EXITING)
#define cpu_has_vmx_apic_reg_virt \
     (vmx_secondary_exec_control & SECONDARY_EXEC_APIC_REGISTER_VIRT)
+#define cpu_has_vmx_virtual_intr_delivery \
+    (vmx_secondary_exec_control & SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY)

 /* GUEST_INTERRUPTIBILITY_INFO flags. */
#define VMX_INTR_SHADOW_STI             0x00000001
@@ -251,6 +257,7 @@ enum vmcs_field {
     GUEST_GS_SELECTOR               = 0x0000080a,
     GUEST_LDTR_SELECTOR             = 0x0000080c,
     GUEST_TR_SELECTOR               = 0x0000080e,
+    GUEST_INTR_STATUS               = 0x00000810,
     HOST_ES_SELECTOR                = 0x00000c00,
     HOST_CS_SELECTOR                = 0x00000c02,
     HOST_SS_SELECTOR                = 0x00000c04,
@@ -278,6 +285,14 @@ enum vmcs_field {
     APIC_ACCESS_ADDR_HIGH           = 0x00002015,
     EPT_POINTER                     = 0x0000201a,
     EPT_POINTER_HIGH                = 0x0000201b,
+    EOI_EXIT_BITMAP0                = 0x0000201c,
+    EOI_EXIT_BITMAP0_HIGH           = 0x0000201d,
+    EOI_EXIT_BITMAP1                = 0x0000201e,
+    EOI_EXIT_BITMAP1_HIGH           = 0x0000201f,
+    EOI_EXIT_BITMAP2                = 0x00002020,
+    EOI_EXIT_BITMAP2_HIGH           = 0x00002021,
+    EOI_EXIT_BITMAP3                = 0x00002022,
+    EOI_EXIT_BITMAP3_HIGH           = 0x00002023,
    GUEST_PHYSICAL_ADDRESS          = 0x00002400,
     GUEST_PHYSICAL_ADDRESS_HIGH     = 0x00002401,
     VMCS_LINK_POINTER               = 0x00002800,
@@ -398,6 +413,8 @@ int vmx_write_guest_msr(u32 msr, u64 val
int vmx_add_guest_msr(u32 msr);
int vmx_add_host_load_msr(u32 msr);
void vmx_vmcs_switch(struct vmcs_struct *from, struct vmcs_struct *to);
+void vmx_set_eoi_exit_bitmap(struct vcpu *v, u8 vector);
+void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector);

 #endif /* ASM_X86_HVM_VMX_VMCS_H__ */

diff -r cb821c24ca74 xen/include/asm-x86/hvm/vmx/vmx.h
--- a/xen/include/asm-x86/hvm/vmx/vmx.h    Fri Aug 31 09:30:38 2012 +0800
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h Fri Aug 31 09:49:39 2012 +0800
@@ -119,6 +119,7 @@ void vmx_update_cpu_exec_control(struct 
 #define EXIT_REASON_MCE_DURING_VMENTRY  41
#define EXIT_REASON_TPR_BELOW_THRESHOLD 43
#define EXIT_REASON_APIC_ACCESS         44
+#define EXIT_REASON_EOI_INDUCED         45
#define EXIT_REASON_ACCESS_GDTR_OR_IDTR 46
#define EXIT_REASON_ACCESS_LDTR_OR_TR   47
#define EXIT_REASON_EPT_VIOLATION       48

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Keir Fraser]

On 31/08/2012 10:30, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:

> Virtual interrupt delivery avoids Xen to inject vAPIC interrupts manually,
> which is fully taken care of by the hardware. This needs some special
> awareness into existing interrupr injection path:
> For pending interrupt from vLAPIC, instead of direct injection, we may need
> update architecture specific indicators before resuming to guest.
> Before returning to guest, RVI should be updated if any pending IRRs
> EOI exit bitmap controls whether an EOI write should cause VM-Exit. If set, a
> trap-like induced EOI VM-Exit is triggered. The approach here is to manipulate
> EOI exit bitmap based on value of TMR. Level triggered irq requires a hook in
> vLAPIC EOI write, so that vIOAPIC EOI is triggered and emulated

Thanks. A couple of quick comments below. This will need some careful review
from a couple of us, I expect.

 -- Keir

> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> Signed-off-by: Jiongxi Li <jiongxi.li@intel.com>
> 
> diff -r cb821c24ca74 xen/arch/x86/hvm/irq.c
> --- a/xen/arch/x86/hvm/irq.c  Fri Aug 31 09:30:38 2012 +0800
> +++ b/xen/arch/x86/hvm/irq.c         Fri Aug 31 09:49:39 2012 +0800
> @@ -452,7 +452,11 @@ struct hvm_intack hvm_vcpu_ack_pending_i
> 
>  int hvm_local_events_need_delivery(struct vcpu *v)
> {
> -    struct hvm_intack intack = hvm_vcpu_has_pending_irq(v);
> +    struct hvm_intack intack;
> +
> +    pt_update_irq(v);

Why would this change be needed for vAPIC?

> +    intack = hvm_vcpu_has_pending_irq(v);
> 
>      if ( likely(intack.source == hvm_intsrc_none) )
>          return 0;
> diff -r cb821c24ca74 xen/arch/x86/hvm/vlapic.c
> --- a/xen/arch/x86/hvm/vlapic.c      Fri Aug 31 09:30:38 2012 +0800
> +++ b/xen/arch/x86/hvm/vlapic.c   Fri Aug 31 09:49:39 2012 +0800
> @@ -143,7 +143,16 @@ static int vlapic_find_highest_irr(struc
> int vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
> {
>      if ( trig )
> +    {
>          vlapic_set_vector(vec, &vlapic->regs->data[APIC_TMR]);
> +        if ( cpu_has_vmx_virtual_intr_delivery )
> +            vmx_set_eoi_exit_bitmap(vlapic_vcpu(vlapic), vec);
> +    }
> +    else
> +    {
> +        if ( cpu_has_vmx_virtual_intr_delivery )
> +            vmx_clear_eoi_exit_bitmap(vlapic_vcpu(vlapic), vec);
> +    }
> 
>      /* We may need to wake up target vcpu, besides set pending bit here */
>      return !vlapic_test_and_set_irr(vec, vlapic);
> @@ -410,6 +419,22 @@ void vlapic_EOI_set(struct vlapic *vlapi
>      hvm_dpci_msi_eoi(current->domain, vector);
> }
> 
> +/*
> + * When "Virtual Interrupt Delivery" is enabled, this function is used
> + * to handle EOI-induced VM exit
> + */
> +void vlapic_handle_EOI_induced_exit(struct vlapic *vlapic, int vector)
> +{
> +    ASSERT(cpu_has_vmx_virtual_intr_delivery);
> +
> +    if ( vlapic_test_and_clear_vector(vector, &vlapic->regs->data[APIC_TMR])
> )
> +    {
> +        vioapic_update_EOI(vlapic_domain(vlapic), vector);
> +    }

No need for braces for single-line statement.

> +    hvm_dpci_msi_eoi(current->domain, vector);
> +}
> +
> int vlapic_ipi(
>      struct vlapic *vlapic, uint32_t icr_low, uint32_t icr_high)
> {
> @@ -1014,6 +1039,9 @@ int vlapic_has_pending_irq(struct vcpu *
>      if ( irr == -1 )
>          return -1;
> 
> +    if ( cpu_has_vmx_virtual_intr_delivery )
> +        return irr;

Why is it correct to ignore ISR here? I guess vAPIC deals with interrupt
window automatically, maybe?

>      isr = vlapic_find_highest_isr(vlapic);
>      isr = (isr != -1) ? isr : 0;
>      if ( (isr & 0xf0) >= (irr & 0xf0) )
> @@ -1026,6 +1054,9 @@ int vlapic_ack_pending_irq(struct vcpu *
> {
>      struct vlapic *vlapic = vcpu_vlapic(v);
> 
> +    if ( cpu_has_vmx_virtual_intr_delivery )
> +        return 1;
> +
>     vlapic_set_vector(vector, &vlapic->regs->data[APIC_ISR]);
>      vlapic_clear_irr(vector, vlapic);
> 

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Jan Beulich]

>>> On 31.08.12 at 11:30, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
> +/*
> + * When "Virtual Interrupt Delivery" is enabled, this function is used
> + * to handle EOI-induced VM exit
> + */
> +void vlapic_handle_EOI_induced_exit(struct vlapic *vlapic, int vector)
> +{
> +    ASSERT(cpu_has_vmx_virtual_intr_delivery);
> +
> +    if ( vlapic_test_and_clear_vector(vector, &vlapic->regs->data[APIC_TMR]) )

Why test_and_clear rather than just test?

> +    {
> +        vioapic_update_EOI(vlapic_domain(vlapic), vector);
> +    }
> +
> +    hvm_dpci_msi_eoi(current->domain, vector);
> +}
>...
> --- a/xen/arch/x86/hvm/vmx/intr.c Fri Aug 31 09:30:38 2012 +0800
> +++ b/xen/arch/x86/hvm/vmx/intr.c       Fri Aug 31 09:49:39 2012 +0800
> @@ -227,19 +227,43 @@ void vmx_intr_assist(void)
>              goto out;
> 
>          intblk = hvm_interrupt_blocked(v, intack);
> -        if ( intblk == hvm_intblk_tpr )
> +        if ( cpu_has_vmx_virtual_intr_delivery )
>          {
> -            ASSERT(vlapic_enabled(vcpu_vlapic(v)));
> -            ASSERT(intack.source == hvm_intsrc_lapic);
> -            tpr_threshold = intack.vector >> 4;
> -            goto out;
> +            /* Set "Interrupt-window exiting" for ExtINT */
> +            if ( (intblk != hvm_intblk_none) &&
> +                 ( (intack.source == hvm_intsrc_pic) ||
> +                 ( intack.source == hvm_intsrc_vector) ) )
> +            {
> +                enable_intr_window(v, intack);
> +                goto out;
> +            }
> +
> +            if ( __vmread(VM_ENTRY_INTR_INFO) & INTR_INFO_VALID_MASK )
> +            {
> +                if ( (intack.source == hvm_intsrc_pic) ||
> +                     (intack.source == hvm_intsrc_nmi) ||
> +                     (intack.source == hvm_intsrc_mce) )
> +                    enable_intr_window(v, intack);
> +
> +                goto out;
> +            }
>          }
> +        else
> +        {
> +            if ( intblk == hvm_intblk_tpr )
> +            {
> +                ASSERT(vlapic_enabled(vcpu_vlapic(v)));
> +                ASSERT(intack.source == hvm_intsrc_lapic);
> +                tpr_threshold = intack.vector >> 4;
> +                goto out;
> +            }
> 
> -        if ( (intblk != hvm_intblk_none) ||
> -             (__vmread(VM_ENTRY_INTR_INFO) & INTR_INFO_VALID_MASK) )
> -        {
> -            enable_intr_window(v, intack);
> -            goto out;
> +            if ( (intblk != hvm_intblk_none) ||
> +                 (__vmread(VM_ENTRY_INTR_INFO) & INTR_INFO_VALID_MASK) )
> +            {
> +                enable_intr_window(v, intack);
> +                goto out;
> +            }
>          }

If you made the above and if()/else if() series, some of the
differences would go away, making the changes easier to
review.

> 
>          intack = hvm_vcpu_ack_pending_irq(v, intack);
> @@ -253,6 +277,29 @@ void vmx_intr_assist(void)
>      {
>          hvm_inject_hw_exception(TRAP_machine_check, HVM_DELIVER_NO_ERROR_CODE);
>      }
> +    else if ( cpu_has_vmx_virtual_intr_delivery &&
> +              intack.source != hvm_intsrc_pic &&
> +              intack.source != hvm_intsrc_vector )
> +    {
> +        /* we need update the RVI field */
> +        unsigned long status = __vmread(GUEST_INTR_STATUS);
> +        status &= ~(unsigned long)0x0FF;
> +        status |= (unsigned long)0x0FF & 
> +                    intack.vector;
> +        __vmwrite(GUEST_INTR_STATUS, status);
> +        if (v->arch.hvm_vmx.eoi_exitmap_changed) {
> +#define UPDATE_EOI_EXITMAP(v, e) {                             \
> +        if (test_and_clear_bit(e, &(v).eoi_exitmap_changed)) {      \

Here and elsewhere - do you really need locked accesses?

> +                __vmwrite(EOI_EXIT_BITMAP##e, (v).eoi_exit_bitmap[e]);}}
> +
> +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 0);

This is not very logical: Passing just 'v' to the macro, and accessing
the full field there would be more consistent.

Furthermore, here and in other places you fail to write the upper
halves for 32-bit, yet you also don't disable the code for 32-bit
afaics.

> +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 1);
> +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 2);
> +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 3);
> +        }
> +
> +        pt_intr_post(v, intack);
> +    }
>      else
>      {
>          HVMTRACE_2D(INJ_VIRQ, intack.vector, /*fake=*/ 0);

Jan

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Li, Jiongxi]

Sorry for the late response.

> -----Original Message-----
> From: Keir Fraser [mailto:keir.xen@gmail.com]
> Sent: Friday, August 31, 2012 5:58 PM
> To: Li, Jiongxi; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [ PATCH 2/2] xen: enable Virtual-interrupt delivery
> 
> On 31/08/2012 10:30, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
> 
> > Virtual interrupt delivery avoids Xen to inject vAPIC interrupts
> > manually, which is fully taken care of by the hardware. This needs
> > some special awareness into existing interrupr injection path:
> > For pending interrupt from vLAPIC, instead of direct injection, we may
> > need update architecture specific indicators before resuming to guest.
> > Before returning to guest, RVI should be updated if any pending IRRs
> > EOI exit bitmap controls whether an EOI write should cause VM-Exit. If
> > set, a trap-like induced EOI VM-Exit is triggered. The approach here
> > is to manipulate EOI exit bitmap based on value of TMR. Level
> > triggered irq requires a hook in vLAPIC EOI write, so that vIOAPIC EOI
> > is triggered and emulated
> 
> Thanks. A couple of quick comments below. This will need some careful review
> from a couple of us, I expect.
> 
>  -- Keir
> 
> > Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> > Signed-off-by: Jiongxi Li <jiongxi.li@intel.com>
> >
> > diff -r cb821c24ca74 xen/arch/x86/hvm/irq.c
> > --- a/xen/arch/x86/hvm/irq.c  Fri Aug 31 09:30:38 2012 +0800
> > +++ b/xen/arch/x86/hvm/irq.c         Fri Aug 31 09:49:39 2012 +0800
> > @@ -452,7 +452,11 @@ struct hvm_intack hvm_vcpu_ack_pending_i
> >
> >  int hvm_local_events_need_delivery(struct vcpu *v) {
> > -    struct hvm_intack intack = hvm_vcpu_has_pending_irq(v);
> > +    struct hvm_intack intack;
> > +
> > +    pt_update_irq(v);
> 
> Why would this change be needed for vAPIC?
When vcpu is scheduled out, there will be periodic timer interrupt pending for injection. Every VMExit is a chance to inject the pending periodic timer interrupt on vmx_intr_assist. In no virtual interrupt delivery case, although injected timer interrupt is edge trigger, EOI always induces VMExit, pending periodic timer can be kept injected till there is no pending left. But in virtual interrupt delivery case, only level trigger interrupt can induce VMExit, there is much less chance for injecting pending timer interrupt through VMExit. 
Adding pt_update_irq here is another code path to inject pending periodic timer, but it can't guarantee every pending timer interrupt can be injected. Another way is to make every EOI of pending timer interrupt induce VMExit, but it may obey the spirit of virtual interrupt delivery - reducing VMExit.
We have been evaluating that.
> 
> > +    intack = hvm_vcpu_has_pending_irq(v);
> >
> >      if ( likely(intack.source == hvm_intsrc_none) )
> >          return 0;
> > diff -r cb821c24ca74 xen/arch/x86/hvm/vlapic.c
> > --- a/xen/arch/x86/hvm/vlapic.c      Fri Aug 31 09:30:38 2012 +0800
> > +++ b/xen/arch/x86/hvm/vlapic.c   Fri Aug 31 09:49:39 2012 +0800
> > @@ -143,7 +143,16 @@ static int vlapic_find_highest_irr(struc int
> > vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig) {
> >      if ( trig )
> > +    {
> >          vlapic_set_vector(vec, &vlapic->regs->data[APIC_TMR]);
> > +        if ( cpu_has_vmx_virtual_intr_delivery )
> > +            vmx_set_eoi_exit_bitmap(vlapic_vcpu(vlapic), vec);
> > +    }
> > +    else
> > +    {
> > +        if ( cpu_has_vmx_virtual_intr_delivery )
> > +            vmx_clear_eoi_exit_bitmap(vlapic_vcpu(vlapic), vec);
> > +    }
> >
> >      /* We may need to wake up target vcpu, besides set pending bit here
> */
> >      return !vlapic_test_and_set_irr(vec, vlapic); @@ -410,6 +419,22
> > @@ void vlapic_EOI_set(struct vlapic *vlapi
> >      hvm_dpci_msi_eoi(current->domain, vector); }
> >
> > +/*
> > + * When "Virtual Interrupt Delivery" is enabled, this function is
> > +used
> > + * to handle EOI-induced VM exit
> > + */
> > +void vlapic_handle_EOI_induced_exit(struct vlapic *vlapic, int
> > +vector) {
> > +    ASSERT(cpu_has_vmx_virtual_intr_delivery);
> > +
> > +    if ( vlapic_test_and_clear_vector(vector,
> > + &vlapic->regs->data[APIC_TMR])
> > )
> > +    {
> > +        vioapic_update_EOI(vlapic_domain(vlapic), vector);
> > +    }
> 
> No need for braces for single-line statement.
OK.
> 
> > +    hvm_dpci_msi_eoi(current->domain, vector); }
> > +
> > int vlapic_ipi(
> >      struct vlapic *vlapic, uint32_t icr_low, uint32_t icr_high) { @@
> > -1014,6 +1039,9 @@ int vlapic_has_pending_irq(struct vcpu *
> >      if ( irr == -1 )
> >          return -1;
> >
> > +    if ( cpu_has_vmx_virtual_intr_delivery )
> > +        return irr;
> 
> Why is it correct to ignore ISR here? I guess vAPIC deals with interrupt window
> automatically, maybe?
In delivery of virtual interrupt and VEOI updates, VISR[vector] is updated automatically by hardware, software doesn't need to care much about it
> 
> >      isr = vlapic_find_highest_isr(vlapic);
> >      isr = (isr != -1) ? isr : 0;
> >      if ( (isr & 0xf0) >= (irr & 0xf0) ) @@ -1026,6 +1054,9 @@ int
> > vlapic_ack_pending_irq(struct vcpu * {
> >      struct vlapic *vlapic = vcpu_vlapic(v);
> >
> > +    if ( cpu_has_vmx_virtual_intr_delivery )
> > +        return 1;
> > +
> >     vlapic_set_vector(vector, &vlapic->regs->data[APIC_ISR]);
> >      vlapic_clear_irr(vector, vlapic);
> >
> 

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Li, Jiongxi]

Sorry for the late response.

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org
> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Jan Beulich
> Sent: Friday, August 31, 2012 8:31 PM
> To: Li, Jiongxi
> Cc: xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [ PATCH 2/2] xen: enable Virtual-interrupt delivery
> 
> >>> On 31.08.12 at 11:30, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
> > +/*
> > + * When "Virtual Interrupt Delivery" is enabled, this function is
> > +used
> > + * to handle EOI-induced VM exit
> > + */
> > +void vlapic_handle_EOI_induced_exit(struct vlapic *vlapic, int
> > +vector) {
> > +    ASSERT(cpu_has_vmx_virtual_intr_delivery);
> > +
> > +    if ( vlapic_test_and_clear_vector(vector,
> > + &vlapic->regs->data[APIC_TMR]) )
> 
> Why test_and_clear rather than just test?
While virtual interrupt delivery is enabled, 'vlapic_EOI_set' function won't be called( 'vlapic_EOI_set' also has the test and clear call). ' vlapic->regs->data[APIC_TMR]' set by 'vlapic_set_irq' should be clear here, or the interrupt of the same vector will be always treated as level trigger even it becomes edge trigger later.
> 
> > +    {
> > +        vioapic_update_EOI(vlapic_domain(vlapic), vector);
> > +    }
> > +
> > +    hvm_dpci_msi_eoi(current->domain, vector); }
> >...
> > --- a/xen/arch/x86/hvm/vmx/intr.c Fri Aug 31 09:30:38 2012 +0800
> > +++ b/xen/arch/x86/hvm/vmx/intr.c       Fri Aug 31 09:49:39 2012 +0800
> > @@ -227,19 +227,43 @@ void vmx_intr_assist(void)
> >              goto out;
> >
> >          intblk = hvm_interrupt_blocked(v, intack);
> > -        if ( intblk == hvm_intblk_tpr )
> > +        if ( cpu_has_vmx_virtual_intr_delivery )
> >          {
> > -            ASSERT(vlapic_enabled(vcpu_vlapic(v)));
> > -            ASSERT(intack.source == hvm_intsrc_lapic);
> > -            tpr_threshold = intack.vector >> 4;
> > -            goto out;
> > +            /* Set "Interrupt-window exiting" for ExtINT */
> > +            if ( (intblk != hvm_intblk_none) &&
> > +                 ( (intack.source == hvm_intsrc_pic) ||
> > +                 ( intack.source == hvm_intsrc_vector) ) )
> > +            {
> > +                enable_intr_window(v, intack);
> > +                goto out;
> > +            }
> > +
> > +            if ( __vmread(VM_ENTRY_INTR_INFO) &
> INTR_INFO_VALID_MASK )
> > +            {
> > +                if ( (intack.source == hvm_intsrc_pic) ||
> > +                     (intack.source == hvm_intsrc_nmi) ||
> > +                     (intack.source == hvm_intsrc_mce) )
> > +                    enable_intr_window(v, intack);
> > +
> > +                goto out;
> > +            }
> >          }
> > +        else
> > +        {
> > +            if ( intblk == hvm_intblk_tpr )
> > +            {
> > +                ASSERT(vlapic_enabled(vcpu_vlapic(v)));
> > +                ASSERT(intack.source == hvm_intsrc_lapic);
> > +                tpr_threshold = intack.vector >> 4;
> > +                goto out;
> > +            }
> >
> > -        if ( (intblk != hvm_intblk_none) ||
> > -             (__vmread(VM_ENTRY_INTR_INFO) &
> INTR_INFO_VALID_MASK) )
> > -        {
> > -            enable_intr_window(v, intack);
> > -            goto out;
> > +            if ( (intblk != hvm_intblk_none) ||
> > +                 (__vmread(VM_ENTRY_INTR_INFO) &
> INTR_INFO_VALID_MASK) )
> > +            {
> > +                enable_intr_window(v, intack);
> > +                goto out;
> > +            }
> >          }
> 
> If you made the above and if()/else if() series, some of the differences would go
> away, making the changes easier to review.
I can't quite understand you here.
The original code looked like:
if (a)
{}
if (b)
{}
And my change as follow:
if ( cpu_has_vmx_virtual_intr_delivery )
{
}
else
{
  if (a)
  {}
  if (b)
  {}
}
How should I adjust the code?

> 
> >
> >          intack = hvm_vcpu_ack_pending_irq(v, intack); @@ -253,6
> > +277,29 @@ void vmx_intr_assist(void)
> >      {
> >          hvm_inject_hw_exception(TRAP_machine_check,
> HVM_DELIVER_NO_ERROR_CODE);
> >      }
> > +    else if ( cpu_has_vmx_virtual_intr_delivery &&
> > +              intack.source != hvm_intsrc_pic &&
> > +              intack.source != hvm_intsrc_vector )
> > +    {
> > +        /* we need update the RVI field */
> > +        unsigned long status = __vmread(GUEST_INTR_STATUS);
> > +        status &= ~(unsigned long)0x0FF;
> > +        status |= (unsigned long)0x0FF &
> > +                    intack.vector;
> > +        __vmwrite(GUEST_INTR_STATUS, status);
> > +        if (v->arch.hvm_vmx.eoi_exitmap_changed) {
> > +#define UPDATE_EOI_EXITMAP(v, e) {                             \
> > +        if (test_and_clear_bit(e, &(v).eoi_exitmap_changed)) {      \
> 
> Here and elsewhere - do you really need locked accesses?
Do you mean using the '__test_and_set_bit' ?
> 
> > +                __vmwrite(EOI_EXIT_BITMAP##e,
> > + (v).eoi_exit_bitmap[e]);}}
> > +
> > +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 0);
> 
> This is not very logical: Passing just 'v' to the macro, and accessing the full field
> there would be more consistent.
OK, I will modify the passing just 'v' to the macro and modify the macro
> 
> Furthermore, here and in other places you fail to write the upper halves for
> 32-bit, yet you also don't disable the code for 32-bit afaics.
OK, __vmwrite(EOI_EXIT_BITMAP##e, (v).eoi_exit_bitmap[e]) ;would be
  __vmwrite(EOI_EXIT_BITMAP##e, (v).eoi_exit_bitmap[e])
#ifdef __i386__
  __vmwrite(EOI_EXIT_BITMAP##e##_HIGH, (v).eoi_exit_bitmap[e] >> 32)
#endif
> 
> > +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 1);
> > +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 2);
> > +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 3);
> > +        }
> > +
> > +        pt_intr_post(v, intack);
> > +    }
> >      else
> >      {
> >          HVMTRACE_2D(INJ_VIRQ, intack.vector, /*fake=*/ 0);
> 
> Jan
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Jan Beulich]

>>> On 06.09.12 at 12:00, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
>> > +/*
>> > + * When "Virtual Interrupt Delivery" is enabled, this function is
>> > +used
>> > + * to handle EOI-induced VM exit
>> > + */
>> > +void vlapic_handle_EOI_induced_exit(struct vlapic *vlapic, int
>> > +vector) {
>> > +    ASSERT(cpu_has_vmx_virtual_intr_delivery);
>> > +
>> > +    if ( vlapic_test_and_clear_vector(vector,
>> > + &vlapic->regs->data[APIC_TMR]) )
>> 
>> Why test_and_clear rather than just test?
> While virtual interrupt delivery is enabled, 'vlapic_EOI_set' function won't 
> be called( 'vlapic_EOI_set' also has the test and clear call). ' 

Ah, okay.

>> > --- a/xen/arch/x86/hvm/vmx/intr.c Fri Aug 31 09:30:38 2012 +0800
>> > +++ b/xen/arch/x86/hvm/vmx/intr.c       Fri Aug 31 09:49:39 2012 +0800
>> > @@ -227,19 +227,43 @@ void vmx_intr_assist(void)
>> >              goto out;
>> >
>> >          intblk = hvm_interrupt_blocked(v, intack);
>> > -        if ( intblk == hvm_intblk_tpr )
>> > +        if ( cpu_has_vmx_virtual_intr_delivery )
>> >          {
>> > -            ASSERT(vlapic_enabled(vcpu_vlapic(v)));
>> > -            ASSERT(intack.source == hvm_intsrc_lapic);
>> > -            tpr_threshold = intack.vector >> 4;
>> > -            goto out;
>> > +            /* Set "Interrupt-window exiting" for ExtINT */
>> > +            if ( (intblk != hvm_intblk_none) &&
>> > +                 ( (intack.source == hvm_intsrc_pic) ||
>> > +                 ( intack.source == hvm_intsrc_vector) ) )
>> > +            {
>> > +                enable_intr_window(v, intack);
>> > +                goto out;
>> > +            }
>> > +
>> > +            if ( __vmread(VM_ENTRY_INTR_INFO) &
>> INTR_INFO_VALID_MASK )
>> > +            {
>> > +                if ( (intack.source == hvm_intsrc_pic) ||
>> > +                     (intack.source == hvm_intsrc_nmi) ||
>> > +                     (intack.source == hvm_intsrc_mce) )
>> > +                    enable_intr_window(v, intack);
>> > +
>> > +                goto out;
>> > +            }
>> >          }
>> > +        else
>> > +        {
>> > +            if ( intblk == hvm_intblk_tpr )
>> > +            {
>> > +                ASSERT(vlapic_enabled(vcpu_vlapic(v)));
>> > +                ASSERT(intack.source == hvm_intsrc_lapic);
>> > +                tpr_threshold = intack.vector >> 4;
>> > +                goto out;
>> > +            }
>> >
>> > -        if ( (intblk != hvm_intblk_none) ||
>> > -             (__vmread(VM_ENTRY_INTR_INFO) &
>> INTR_INFO_VALID_MASK) )
>> > -        {
>> > -            enable_intr_window(v, intack);
>> > -            goto out;
>> > +            if ( (intblk != hvm_intblk_none) ||
>> > +                 (__vmread(VM_ENTRY_INTR_INFO) &
>> INTR_INFO_VALID_MASK) )
>> > +            {
>> > +                enable_intr_window(v, intack);
>> > +                goto out;
>> > +            }
>> >          }
>> 
>> If you made the above and if()/else if() series, some of the differences 
> would go
>> away, making the changes easier to review.
> I can't quite understand you here.
> The original code looked like:
> if (a)
> {}
> if (b)
> {}
> And my change as follow:
> if ( cpu_has_vmx_virtual_intr_delivery )
> {
> }
> else
> {
>   if (a)
>   {}
>   if (b)
>   {}
> }
> How should I adjust the code?

Considering that the original could already have been written
with if/else-if, I was suggesting to expand this to your addition:

if ( cpu_has_vmx_virtual_intr_delivery )
{
}
else if (a)
  {}
else if (b)
  {}

which will avoid any (indentation only) changes past the first of
the two else-if-s. Plus it would make the logic of the code more
clear, at once likely making apparent that there'll now be quite
a few "goto out"-s that ought to be check for being replaceable
by fewer instances of them placed slightly differently.

>> > +#define UPDATE_EOI_EXITMAP(v, e) {                             \
>> > +        if (test_and_clear_bit(e, &(v).eoi_exitmap_changed)) {      \
>> 
>> Here and elsewhere - do you really need locked accesses?
> Do you mean using the '__test_and_set_bit' ?

Yes, if suitable.

>> Furthermore, here and in other places you fail to write the upper halves for
>> 32-bit, yet you also don't disable the code for 32-bit afaics.
> OK, __vmwrite(EOI_EXIT_BITMAP##e, (v).eoi_exit_bitmap[e]) ;would be
>   __vmwrite(EOI_EXIT_BITMAP##e, (v).eoi_exit_bitmap[e])
> #ifdef __i386__
>   __vmwrite(EOI_EXIT_BITMAP##e##_HIGH, (v).eoi_exit_bitmap[e] >> 32)
> #endif

Something along those lines, yes.

Jan

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Jan Beulich]

>>> On 06.09.12 at 12:00, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
>> From: Keir Fraser [mailto:keir.xen@gmail.com]
>> On 31/08/2012 10:30, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
>> > --- a/xen/arch/x86/hvm/irq.c  Fri Aug 31 09:30:38 2012 +0800
>> > +++ b/xen/arch/x86/hvm/irq.c         Fri Aug 31 09:49:39 2012 +0800
>> > @@ -452,7 +452,11 @@ struct hvm_intack hvm_vcpu_ack_pending_i
>> >
>> >  int hvm_local_events_need_delivery(struct vcpu *v) {
>> > -    struct hvm_intack intack = hvm_vcpu_has_pending_irq(v);
>> > +    struct hvm_intack intack;
>> > +
>> > +    pt_update_irq(v);
>> 
>> Why would this change be needed for vAPIC?
> When vcpu is scheduled out, there will be periodic timer interrupt pending 

Probably rather "may"?

> for injection. Every VMExit is a chance to inject the pending periodic timer 
> interrupt on vmx_intr_assist. In no virtual interrupt delivery case, although 
> injected timer interrupt is edge trigger, EOI always induces VMExit, pending 
> periodic timer can be kept injected till there is no pending left. But in 
> virtual interrupt delivery case, only level trigger interrupt can induce 
> VMExit, there is much less chance for injecting pending timer interrupt 
> through VMExit. 

And it's not possible to set the respective VIRR[] bit, and let the
hardware take care of injecting at the right time?

> Adding pt_update_irq here is another code path to inject pending periodic 
> timer, but it can't guarantee every pending timer interrupt can be injected. 
> Another way is to make every EOI of pending timer interrupt induce VMExit, 
> but it may obey the spirit of virtual interrupt delivery - reducing VMExit.
> We have been evaluating that.

With which result?

Jan

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Keir Fraser]

On 06/09/2012 11:00, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:

>>>  int hvm_local_events_need_delivery(struct vcpu *v) {
>>> -    struct hvm_intack intack = hvm_vcpu_has_pending_irq(v);
>>> +    struct hvm_intack intack;
>>> +
>>> +    pt_update_irq(v);
>> 
>> Why would this change be needed for vAPIC?
> When vcpu is scheduled out, there will be periodic timer interrupt pending for
> injection. Every VMExit is a chance to inject the pending periodic timer
> interrupt on vmx_intr_assist. In no virtual interrupt delivery case, although
> injected timer interrupt is edge trigger, EOI always induces VMExit, pending
> periodic timer can be kept injected till there is no pending left. But in
> virtual interrupt delivery case, only level trigger interrupt can induce
> VMExit, there is much less chance for injecting pending timer interrupt
> through VMExit. 
> Adding pt_update_irq here is another code path to inject pending periodic
> timer, but it can't guarantee every pending timer interrupt can be injected.
> Another way is to make every EOI of pending timer interrupt induce VMExit, but
> it may obey the spirit of virtual interrupt delivery - reducing VMExit.
> We have been evaluating that.

Should cause EOI to induce VMExit only when there is more periodic timer
work to be done? That would be better than this hack.

 -- Keir

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Christoph Egger]

On 09/06/12 12:35, Jan Beulich wrote:

>>>> On 06.09.12 at 12:00, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
>>>> +/*
>>>> + * When "Virtual Interrupt Delivery" is enabled, this function is
>>>> +used
>>>> + * to handle EOI-induced VM exit
>>>> + */
>>>> +void vlapic_handle_EOI_induced_exit(struct vlapic *vlapic, int
>>>> +vector) {
>>>> +    ASSERT(cpu_has_vmx_virtual_intr_delivery);
>>>> +
>>>> +    if ( vlapic_test_and_clear_vector(vector,
>>>> + &vlapic->regs->data[APIC_TMR]) )
>>>
>>> Why test_and_clear rather than just test?
>> While virtual interrupt delivery is enabled, 'vlapic_EOI_set' function won't 
>> be called( 'vlapic_EOI_set' also has the test and clear call). ' 
> 
> Ah, okay.
> 
>>>> --- a/xen/arch/x86/hvm/vmx/intr.c Fri Aug 31 09:30:38 2012 +0800
>>>> +++ b/xen/arch/x86/hvm/vmx/intr.c       Fri Aug 31 09:49:39 2012 +0800
>>>> @@ -227,19 +227,43 @@ void vmx_intr_assist(void)
>>>>              goto out;
>>>>
>>>>          intblk = hvm_interrupt_blocked(v, intack);
>>>> -        if ( intblk == hvm_intblk_tpr )
>>>> +        if ( cpu_has_vmx_virtual_intr_delivery )
>>>>          {
>>>> -            ASSERT(vlapic_enabled(vcpu_vlapic(v)));
>>>> -            ASSERT(intack.source == hvm_intsrc_lapic);
>>>> -            tpr_threshold = intack.vector >> 4;
>>>> -            goto out;
>>>> +            /* Set "Interrupt-window exiting" for ExtINT */
>>>> +            if ( (intblk != hvm_intblk_none) &&
>>>> +                 ( (intack.source == hvm_intsrc_pic) ||
>>>> +                 ( intack.source == hvm_intsrc_vector) ) )
>>>> +            {
>>>> +                enable_intr_window(v, intack);
>>>> +                goto out;
>>>> +            }
>>>> +
>>>> +            if ( __vmread(VM_ENTRY_INTR_INFO) &
>>> INTR_INFO_VALID_MASK )
>>>> +            {
>>>> +                if ( (intack.source == hvm_intsrc_pic) ||
>>>> +                     (intack.source == hvm_intsrc_nmi) ||
>>>> +                     (intack.source == hvm_intsrc_mce) )
>>>> +                    enable_intr_window(v, intack);
>>>> +
>>>> +                goto out;
>>>> +            }
>>>>          }
>>>> +        else
>>>> +        {
>>>> +            if ( intblk == hvm_intblk_tpr )
>>>> +            {
>>>> +                ASSERT(vlapic_enabled(vcpu_vlapic(v)));
>>>> +                ASSERT(intack.source == hvm_intsrc_lapic);
>>>> +                tpr_threshold = intack.vector >> 4;
>>>> +                goto out;
>>>> +            }
>>>>
>>>> -        if ( (intblk != hvm_intblk_none) ||
>>>> -             (__vmread(VM_ENTRY_INTR_INFO) &
>>> INTR_INFO_VALID_MASK) )
>>>> -        {
>>>> -            enable_intr_window(v, intack);
>>>> -            goto out;
>>>> +            if ( (intblk != hvm_intblk_none) ||
>>>> +                 (__vmread(VM_ENTRY_INTR_INFO) &
>>> INTR_INFO_VALID_MASK) )
>>>> +            {
>>>> +                enable_intr_window(v, intack);
>>>> +                goto out;
>>>> +            }
>>>>          }
>>>
>>> If you made the above and if()/else if() series, some of the differences 
>> would go
>>> away, making the changes easier to review.
>> I can't quite understand you here.
>> The original code looked like:
>> if (a)
>> {}
>> if (b)
>> {}
>> And my change as follow:
>> if ( cpu_has_vmx_virtual_intr_delivery )
>> {
>> }
>> else
>> {
>>   if (a)
>>   {}
>>   if (b)
>>   {}
>> }
>> How should I adjust the code?
> 
> Considering that the original could already have been written
> with if/else-if, I was suggesting to expand this to your addition:
> 
> if ( cpu_has_vmx_virtual_intr_delivery )
> {
> }
> else if (a)
>   {}
> else if (b)
>   {}


I think, move the VMX part into hvm/vmx/* and call a function pointer
from common code. Having VMX or SVM specific code in common code
is broken by design.

Christoph

-- 

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Jan Beulich]

>>> On 06.09.12 at 13:03, Christoph Egger <Christoph.Egger@amd.com> wrote:
> I think, move the VMX part into hvm/vmx/* and call a function pointer
> from common code. Having VMX or SVM specific code in common code
> is broken by design.

Yes - I had commented on that aspect already.

Jan

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Zhang, Yang Z]

Jan Beulich wrote on 2012-08-31:
>>>> On 31.08.12 at 11:30, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
>> +/*
>> + * When "Virtual Interrupt Delivery" is enabled, this function is used
>> + * to handle EOI-induced VM exit
>> + */
>> +void vlapic_handle_EOI_induced_exit(struct vlapic *vlapic, int vector)
>> +{
>> +    ASSERT(cpu_has_vmx_virtual_intr_delivery);
>> +
>> +    if ( vlapic_test_and_clear_vector(vector,
> &vlapic->regs->data[APIC_TMR]) )
> 
> Why test_and_clear rather than just test?

The current logic requires to clear it when guest writes EOI. When VCPU received an interrupt, it set the TMR if it is a level trigger interrupt and do nothing if it is edge triggered(it assumes the corresponding bit in TMR is clear).

>> +    {
>> +        vioapic_update_EOI(vlapic_domain(vlapic), vector);
>> +    }
>> +
>> +    hvm_dpci_msi_eoi(current->domain, vector);
>> +}
>> ...
>> --- a/xen/arch/x86/hvm/vmx/intr.c Fri Aug 31 09:30:38 2012 +0800
>> +++ b/xen/arch/x86/hvm/vmx/intr.c       Fri Aug 31 09:49:39 2012 +0800
>> @@ -227,19 +227,43 @@ void vmx_intr_assist(void)
>>              goto out;
>>          intblk = hvm_interrupt_blocked(v, intack);
>> -        if ( intblk == hvm_intblk_tpr )
>> +        if ( cpu_has_vmx_virtual_intr_delivery )
>>          {
>> -            ASSERT(vlapic_enabled(vcpu_vlapic(v))); -           
>> ASSERT(intack.source == hvm_intsrc_lapic); -            tpr_threshold =
>> intack.vector >> 4; -            goto out; +            /* Set
>> "Interrupt-window exiting" for ExtINT */ +            if ( (intblk !=
>> hvm_intblk_none) && +                 ( (intack.source ==
>> hvm_intsrc_pic) || +                 ( intack.source ==
>> hvm_intsrc_vector) ) ) +            { +               
>> enable_intr_window(v, intack); +                goto out; +           
>> } + +            if ( __vmread(VM_ENTRY_INTR_INFO) &
>> INTR_INFO_VALID_MASK ) +            { +                if (
>> (intack.source == hvm_intsrc_pic) || +                    
>> (intack.source == hvm_intsrc_nmi) || +                    
>> (intack.source == hvm_intsrc_mce) ) +                   
>> enable_intr_window(v, intack); + +                goto out; +          
>>  }
>>          }
>> +        else
>> +        {
>> +            if ( intblk == hvm_intblk_tpr )
>> +            {
>> +                ASSERT(vlapic_enabled(vcpu_vlapic(v)));
>> +                ASSERT(intack.source == hvm_intsrc_lapic);
>> +                tpr_threshold = intack.vector >> 4;
>> +                goto out;
>> +            }
>> 
>> -        if ( (intblk != hvm_intblk_none) || -            
>> (__vmread(VM_ENTRY_INTR_INFO) & INTR_INFO_VALID_MASK) ) -        { -   
>>         enable_intr_window(v, intack); -            goto out; +        
>>    if ( (intblk != hvm_intblk_none) || +                
>> (__vmread(VM_ENTRY_INTR_INFO) & INTR_INFO_VALID_MASK) ) +            {
>> +                enable_intr_window(v, intack); +                goto
>> out; +            }
>>          }
> 
> If you made the above and if()/else if() series, some of the
> differences would go away, making the changes easier to
> review.
> 
>> 
>>          intack = hvm_vcpu_ack_pending_irq(v, intack);
>> @@ -253,6 +277,29 @@ void vmx_intr_assist(void)
>>      {
>>          hvm_inject_hw_exception(TRAP_machine_check,
> HVM_DELIVER_NO_ERROR_CODE);
>>      }
>> +    else if ( cpu_has_vmx_virtual_intr_delivery &&
>> +              intack.source != hvm_intsrc_pic &&
>> +              intack.source != hvm_intsrc_vector )
>> +    {
>> +        /* we need update the RVI field */
>> +        unsigned long status = __vmread(GUEST_INTR_STATUS);
>> +        status &= ~(unsigned long)0x0FF;
>> +        status |= (unsigned long)0x0FF &
>> +                    intack.vector;
>> +        __vmwrite(GUEST_INTR_STATUS, status);
>> +        if (v->arch.hvm_vmx.eoi_exitmap_changed) {
>> +#define UPDATE_EOI_EXITMAP(v, e) {                             \
>> +        if (test_and_clear_bit(e, &(v).eoi_exitmap_changed)) {      \
> 
> Here and elsewhere - do you really need locked accesses?
>
>> +                __vmwrite(EOI_EXIT_BITMAP##e,
>> (v).eoi_exit_bitmap[e]);}} + +               
>> UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 0);
> 
> This is not very logical: Passing just 'v' to the macro, and accessing
> the full field there would be more consistent.
> 
> Furthermore, here and in other places you fail to write the upper halves
> for 32-bit, yet you also don't disable the code for 32-bit afaics.
> 
>> +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 1);
>> +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 2);
>> +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 3);
>> +        }
>> +
>> +        pt_intr_post(v, intack);
>> +    }
>>      else
>>      {
>>          HVMTRACE_2D(INJ_VIRQ, intack.vector, /*fake=*/ 0);
> 
> Jan
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


Best regards,
Yang

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Zhang, Yang Z]

Jan Beulich wrote on 2012-09-06:
>>>> On 06.09.12 at 12:00, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
>>> From: Keir Fraser [mailto:keir.xen@gmail.com]
>>> On 31/08/2012 10:30, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
>>>> --- a/xen/arch/x86/hvm/irq.c  Fri Aug 31 09:30:38 2012 +0800
>>>> +++ b/xen/arch/x86/hvm/irq.c         Fri Aug 31 09:49:39 2012 +0800
>>>> @@ -452,7 +452,11 @@ struct hvm_intack hvm_vcpu_ack_pending_i
>>>> 
>>>>  int hvm_local_events_need_delivery(struct vcpu *v) {
>>>> -    struct hvm_intack intack = hvm_vcpu_has_pending_irq(v);
>>>> +    struct hvm_intack intack;
>>>> +
>>>> +    pt_update_irq(v);
>>> 
>>> Why would this change be needed for vAPIC?
>> When vcpu is scheduled out, there will be periodic timer interrupt pending
> 
> Probably rather "may"?

yes, there may have pending timer interrupt.

>> for injection. Every VMExit is a chance to inject the pending periodic timer
>> interrupt on vmx_intr_assist. In no virtual interrupt delivery case, although
>> injected timer interrupt is edge trigger, EOI always induces VMExit, pending
>> periodic timer can be kept injected till there is no pending left. But in
>> virtual interrupt delivery case, only level trigger interrupt can induce
>> VMExit, there is much less chance for injecting pending timer interrupt
>> through VMExit.
> 
> And it's not possible to set the respective VIRR[] bit, and let the
> hardware take care of injecting at the right time?

Right, we can use this way to inject the interrupt. 
But it still need a watchdog to aware when the bit in VIRR is cleared by guest, and the cost is same with forcing VM exit when writing EOI for timer interrupt.

>> Adding pt_update_irq here is another code path to inject pending periodic
>> timer, but it can't guarantee every pending timer interrupt can be injected.
>> Another way is to make every EOI of pending timer interrupt induce VMExit,
>> but it may obey the spirit of virtual interrupt delivery - reducing VMExit.
>> We have been evaluating that.
> 
> With which result?

We are doing it now. Will let you know the result when get it.

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


Best regards,
Yang

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Li, Jiongxi]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org
> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Keir Fraser
> Sent: Thursday, September 06, 2012 6:47 PM
> To: Li, Jiongxi; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [ PATCH 2/2] xen: enable Virtual-interrupt delivery
> 
> On 06/09/2012 11:00, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
> 
> >>>  int hvm_local_events_need_delivery(struct vcpu *v) {
> >>> -    struct hvm_intack intack = hvm_vcpu_has_pending_irq(v);
> >>> +    struct hvm_intack intack;
> >>> +
> >>> +    pt_update_irq(v);
> >>
> >> Why would this change be needed for vAPIC?
> > When vcpu is scheduled out, there will be periodic timer interrupt
> > pending for injection. Every VMExit is a chance to inject the pending
> > periodic timer interrupt on vmx_intr_assist. In no virtual interrupt
> > delivery case, although injected timer interrupt is edge trigger, EOI
> > always induces VMExit, pending periodic timer can be kept injected
> > till there is no pending left. But in virtual interrupt delivery case,
> > only level trigger interrupt can induce VMExit, there is much less
> > chance for injecting pending timer interrupt through VMExit.
> > Adding pt_update_irq here is another code path to inject pending
> > periodic timer, but it can't guarantee every pending timer interrupt can be
> injected.
> > Another way is to make every EOI of pending timer interrupt induce
> > VMExit, but it may obey the spirit of virtual interrupt delivery - reducing
> VMExit.
> > We have been evaluating that.
> 
> Should cause EOI to induce VMExit only when there is more periodic timer work
> to be done? That would be better than this hack.
> 
Yes, in our new patch, we set eoi_exit_bitmap for periodic timer interrup to cause EOI-induced VM exit.
Please refer to patch v2
>  -- Keir
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Li, Jiongxi]

> -----Original Message-----
> From: Zhang, Yang Z
> Sent: Friday, September 07, 2012 10:08 AM
> To: Jan Beulich; Li, Jiongxi
> Cc: Keir Fraser; xen-devel@lists.xen.org
> Subject: RE: [Xen-devel] [ PATCH 2/2] xen: enable Virtual-interrupt delivery
> 
> Jan Beulich wrote on 2012-09-06:
> >>>> On 06.09.12 at 12:00, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
> >>> From: Keir Fraser [mailto:keir.xen@gmail.com] On 31/08/2012 10:30,
> >>> "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
> >>>> --- a/xen/arch/x86/hvm/irq.c  Fri Aug 31 09:30:38 2012 +0800
> >>>> +++ b/xen/arch/x86/hvm/irq.c         Fri Aug 31 09:49:39 2012 +0800
> >>>> @@ -452,7 +452,11 @@ struct hvm_intack hvm_vcpu_ack_pending_i
> >>>>
> >>>>  int hvm_local_events_need_delivery(struct vcpu *v) {
> >>>> -    struct hvm_intack intack = hvm_vcpu_has_pending_irq(v);
> >>>> +    struct hvm_intack intack;
> >>>> +
> >>>> +    pt_update_irq(v);
> >>>
> >>> Why would this change be needed for vAPIC?
> >> When vcpu is scheduled out, there will be periodic timer interrupt
> >> pending
> >
> > Probably rather "may"?
> 
> yes, there may have pending timer interrupt.
> 
> >> for injection. Every VMExit is a chance to inject the pending
> >> periodic timer interrupt on vmx_intr_assist. In no virtual interrupt
> >> delivery case, although injected timer interrupt is edge trigger, EOI
> >> always induces VMExit, pending periodic timer can be kept injected
> >> till there is no pending left. But in virtual interrupt delivery
> >> case, only level trigger interrupt can induce VMExit, there is much
> >> less chance for injecting pending timer interrupt through VMExit.
> >
> > And it's not possible to set the respective VIRR[] bit, and let the
> > hardware take care of injecting at the right time?
> 
> Right, we can use this way to inject the interrupt.
> But it still need a watchdog to aware when the bit in VIRR is cleared by guest,
> and the cost is same with forcing VM exit when writing EOI for timer interrupt.
> 
> >> Adding pt_update_irq here is another code path to inject pending
> >> periodic timer, but it can't guarantee every pending timer interrupt can be
> injected.
> >> Another way is to make every EOI of pending timer interrupt induce
> >> VMExit, but it may obey the spirit of virtual interrupt delivery - reducing
> VMExit.
> >> We have been evaluating that.
> >
> > With which result?
> 
> We are doing it now. Will let you know the result when get it.
> 
In our new patch, we set eoi_exit_bitmap for periodic timer interrup to cause EOI-induced VM exit, then pending periodic time interrups have the chance to be injected for compensation
Please refer to patch v2

> > Jan
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> 
> Best regards,
> Yang
> 

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Li, Jiongxi]

A new patch is sent out according to review comment.
Also please see my comment interlaced

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, September 06, 2012 6:35 PM
> To: Li, Jiongxi
> Cc: xen-devel@lists.xen.org
> Subject: RE: [Xen-devel] [ PATCH 2/2] xen: enable Virtual-interrupt delivery
> 
> >>> On 06.09.12 at 12:00, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
> >> > +/*
> >> > + * When "Virtual Interrupt Delivery" is enabled, this function is
> >> > +used
> >> > + * to handle EOI-induced VM exit
> >> > + */
> >> > +void vlapic_handle_EOI_induced_exit(struct vlapic *vlapic, int
> >> > +vector) {
> >> > +    ASSERT(cpu_has_vmx_virtual_intr_delivery);
> >> > +
> >> > +    if ( vlapic_test_and_clear_vector(vector,
> >> > + &vlapic->regs->data[APIC_TMR]) )
> >>
> >> Why test_and_clear rather than just test?
> > While virtual interrupt delivery is enabled, 'vlapic_EOI_set' function
> > won't be called( 'vlapic_EOI_set' also has the test and clear call). '
> 
> Ah, okay.
> 
> >> > --- a/xen/arch/x86/hvm/vmx/intr.c Fri Aug 31 09:30:38 2012 +0800
> >> > +++ b/xen/arch/x86/hvm/vmx/intr.c       Fri Aug 31 09:49:39 2012
> +0800
> >> > @@ -227,19 +227,43 @@ void vmx_intr_assist(void)
> >> >              goto out;
> >> >
> >> >          intblk = hvm_interrupt_blocked(v, intack);
> >> > -        if ( intblk == hvm_intblk_tpr )
> >> > +        if ( cpu_has_vmx_virtual_intr_delivery )
> >> >          {
> >> > -            ASSERT(vlapic_enabled(vcpu_vlapic(v)));
> >> > -            ASSERT(intack.source == hvm_intsrc_lapic);
> >> > -            tpr_threshold = intack.vector >> 4;
> >> > -            goto out;
> >> > +            /* Set "Interrupt-window exiting" for ExtINT */
> >> > +            if ( (intblk != hvm_intblk_none) &&
> >> > +                 ( (intack.source == hvm_intsrc_pic) ||
> >> > +                 ( intack.source == hvm_intsrc_vector) ) )
> >> > +            {
> >> > +                enable_intr_window(v, intack);
> >> > +                goto out;
> >> > +            }
> >> > +
> >> > +            if ( __vmread(VM_ENTRY_INTR_INFO) &
> >> INTR_INFO_VALID_MASK )
> >> > +            {
> >> > +                if ( (intack.source == hvm_intsrc_pic) ||
> >> > +                     (intack.source == hvm_intsrc_nmi) ||
> >> > +                     (intack.source == hvm_intsrc_mce) )
> >> > +                    enable_intr_window(v, intack);
> >> > +
> >> > +                goto out;
> >> > +            }
> >> >          }
> >> > +        else
> >> > +        {
> >> > +            if ( intblk == hvm_intblk_tpr )
> >> > +            {
> >> > +                ASSERT(vlapic_enabled(vcpu_vlapic(v)));
> >> > +                ASSERT(intack.source == hvm_intsrc_lapic);
> >> > +                tpr_threshold = intack.vector >> 4;
> >> > +                goto out;
> >> > +            }
> >> >
> >> > -        if ( (intblk != hvm_intblk_none) ||
> >> > -             (__vmread(VM_ENTRY_INTR_INFO) &
> >> INTR_INFO_VALID_MASK) )
> >> > -        {
> >> > -            enable_intr_window(v, intack);
> >> > -            goto out;
> >> > +            if ( (intblk != hvm_intblk_none) ||
> >> > +                 (__vmread(VM_ENTRY_INTR_INFO) &
> >> INTR_INFO_VALID_MASK) )
> >> > +            {
> >> > +                enable_intr_window(v, intack);
> >> > +                goto out;
> >> > +            }
> >> >          }
> >>
> >> If you made the above and if()/else if() series, some of the
> >> differences
> > would go
> >> away, making the changes easier to review.
> > I can't quite understand you here.
> > The original code looked like:
> > if (a)
> > {}
> > if (b)
> > {}
> > And my change as follow:
> > if ( cpu_has_vmx_virtual_intr_delivery ) { } else {
> >   if (a)
> >   {}
> >   if (b)
> >   {}
> > }
> > How should I adjust the code?
> 
> Considering that the original could already have been written with if/else-if, I
> was suggesting to expand this to your addition:
> 
> if ( cpu_has_vmx_virtual_intr_delivery ) { } else if (a)
>   {}
> else if (b)
>   {}
> 
> which will avoid any (indentation only) changes past the first of the two else-if-s.
> Plus it would make the logic of the code more clear, at once likely making
> apparent that there'll now be quite a few "goto out"-s that ought to be check
> for being replaceable by fewer instances of them placed slightly differently.
It is a good suggestion. But the original code is two parallel if() case, not the if/else-if case, and can't be changed to if/else-if case, so I just keep the original code here. :)

> 
> >> > +#define UPDATE_EOI_EXITMAP(v, e)
> {                             \
> >> > +        if (test_and_clear_bit(e, &(v).eoi_exitmap_changed)) {      \
> >>
> >> Here and elsewhere - do you really need locked accesses?
> > Do you mean using the '__test_and_set_bit' ?
> 
> Yes, if suitable.
Since the parameter may also be changed in 'vlapic_set_irq' function, and that function may be called asynchronously, so a lock is needed here.

> >> Furthermore, here and in other places you fail to write the upper
> >> halves for 32-bit, yet you also don't disable the code for 32-bit afaics.
> > OK, __vmwrite(EOI_EXIT_BITMAP##e, (v).eoi_exit_bitmap[e]) ;would be
> >   __vmwrite(EOI_EXIT_BITMAP##e, (v).eoi_exit_bitmap[e]) #ifdef
> > __i386__
> >   __vmwrite(EOI_EXIT_BITMAP##e##_HIGH, (v).eoi_exit_bitmap[e] >> 32)
> > #endif
> 
> Something along those lines, yes.
> 
> Jan

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Jan Beulich]

>>> On 13.09.12 at 12:13, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
>> Considering that the original could already have been written with 
> if/else-if, I
>> was suggesting to expand this to your addition:
>> 
>> if ( cpu_has_vmx_virtual_intr_delivery ) { } else if (a)
>>   {}
>> else if (b)
>>   {}
>> 
>> which will avoid any (indentation only) changes past the first of the two 
> else-if-s.
>> Plus it would make the logic of the code more clear, at once likely making
>> apparent that there'll now be quite a few "goto out"-s that ought to be check
>> for being replaceable by fewer instances of them placed slightly 
> differently.
> It is a good suggestion. But the original code is two parallel if() case, 
> not the if/else-if case, and can't be changed to if/else-if case, so I just 
> keep the original code here. :)

That's simply not true. The code before your patch is

        if ( intblk == hvm_intblk_tpr )
        {
            ...
            goto out;
        }

        if ( (intblk != hvm_intblk_none) || ... )
        {
            ...
            goto out;
        }

which can easily be re-written into and if()/else if() (due to the
goto at the first if() body's end). All you want in your patch is
then to prepend another if() and convert the initial if() into an
else if() too.

Jan

Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery

[Li, Jiongxi]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, September 13, 2012 8:01 PM
> To: Li, Jiongxi
> Cc: xen-devel@lists.xen.org
> Subject: RE: [Xen-devel] [ PATCH 2/2] xen: enable Virtual-interrupt delivery
> 
> >>> On 13.09.12 at 12:13, "Li, Jiongxi" <jiongxi.li@intel.com> wrote:
> >> Considering that the original could already have been written with
> > if/else-if, I
> >> was suggesting to expand this to your addition:
> >>
> >> if ( cpu_has_vmx_virtual_intr_delivery ) { } else if (a)
> >>   {}
> >> else if (b)
> >>   {}
> >>
> >> which will avoid any (indentation only) changes past the first of the
> >> two
> > else-if-s.
> >> Plus it would make the logic of the code more clear, at once likely
> >> making apparent that there'll now be quite a few "goto out"-s that
> >> ought to be check for being replaceable by fewer instances of them
> >> placed slightly
> > differently.
> > It is a good suggestion. But the original code is two parallel if()
> > case, not the if/else-if case, and can't be changed to if/else-if
> > case, so I just keep the original code here. :)
> 
> That's simply not true. The code before your patch is
> 
>         if ( intblk == hvm_intblk_tpr )
>         {
>             ...
>             goto out;
>         }
> 
>         if ( (intblk != hvm_intblk_none) || ... )
>         {
>             ...
>             goto out;
>         }
> 
> which can easily be re-written into and if()/else if() (due to the goto at the first
> if() body's end). All you want in your patch is then to prepend another if() and
> convert the initial if() into an else if() too.
> 
I get your idea now, sorry for the misunderstanding before. A new patch for this will be sent out
> Jan