All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR
@ 2017-10-20  0:35 Chao Gao
  2017-10-20 11:42 ` Jan Beulich
  2017-10-23  8:08 ` Tian, Kevin
  0 siblings, 2 replies; 6+ messages in thread
From: Chao Gao @ 2017-10-20  0:35 UTC (permalink / raw)
  To: xen-devel
  Cc: Kevin Tian, Jun Nakajima, Andrew Cooper, Julien Grall,
	Jan Beulich, Chao Gao

pt_update_irq() is expected to return the vector number of periodic
timer interrupt, which should be set in vIRR of vlapic or in PIR.
Otherwise it would trigger the assertion in vmx_intr_assist(), please
seeing https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg00915.html.

But it fails to achieve that in the following two case:
1. hvm_isa_irq_assert() may not set the corresponding bit in vIRR for
mask field of IOAPIC RTE is set. Please refer to the call tree
vmx_intr_assist() -> pt_update_irq() -> hvm_isa_irq_assert() ->
assert_irq() -> assert_gsi() -> vioapic_irq_positive_edge(). The patch
checks whether the vector is set or not in vIRR of vlapic or PIR before
returning.

2. someone changes the vector field of IOAPIC RTE between asserting
the irq and getting the vector of the irq, leading to setting the
old vector number but returning a different vector number. This patch
allows hvm_isa_irq_assert() to accept a callback which can get the
interrupt vector with irq_lock held. Thus, no one can change the vector
between the two operations.

BTW, the first argument of pi_test_and_set_pir() should be uint8_t
and I take this chance to fix it.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
To Julien:
This patch is to fix a possible cause of an assertion failure related to
periodic timer interrupt. OSSTEST reports regression occasionally when the bug
happens. I intend to merge this patch in 4.10 and then observe whether
the bug disappears or not.

---
passed the two simple xtf tests in 
https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg00915.html.
, which are designed to produce the above two cases.

v3:
- change the first argument of pi_test_pir() to uint8_t
- change the first argument of pi_test_and_set_pir() to uint8_t
- return -1 when no callback is passed to hvm_isa_irq_assert()
- check hvm_isa_irq_assert(.., vioapic_get_vector) in case the callback failed

v2:
- add a callback to hvm_isa_irq_assert() to avoid code duplication
- Constify vlapic argument of vlapic_test_irq()

---
 xen/arch/x86/hvm/dm.c             |  2 +-
 xen/arch/x86/hvm/irq.c            | 11 +++++++++--
 xen/arch/x86/hvm/pmtimer.c        |  2 +-
 xen/arch/x86/hvm/rtc.c            |  2 +-
 xen/arch/x86/hvm/vlapic.c         | 12 ++++++++++++
 xen/arch/x86/hvm/vmx/vmx.c        |  7 +++++++
 xen/arch/x86/hvm/vpt.c            | 39 ++++++++++++++++++++++++++-------------
 xen/include/asm-x86/hvm/hvm.h     |  1 +
 xen/include/asm-x86/hvm/irq.h     | 12 ++++++++++--
 xen/include/asm-x86/hvm/vlapic.h  |  1 +
 xen/include/asm-x86/hvm/vmx/vmx.h |  7 ++++++-
 11 files changed, 75 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 32ade95..a787f43 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -143,7 +143,7 @@ static int set_isa_irq_level(struct domain *d, uint8_t isa_irq,
         hvm_isa_irq_deassert(d, isa_irq);
         break;
     case 1:
-        hvm_isa_irq_assert(d, isa_irq);
+        hvm_isa_irq_assert(d, isa_irq, NULL);
         break;
     default:
         return -EINVAL;
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index e425df9..0077f68 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -168,11 +168,13 @@ void hvm_gsi_deassert(struct domain *d, unsigned int gsi)
     spin_unlock(&d->arch.hvm_domain.irq_lock);
 }
 
-void hvm_isa_irq_assert(
-    struct domain *d, unsigned int isa_irq)
+int hvm_isa_irq_assert(struct domain *d, unsigned int isa_irq,
+                       int (*get_vector)(const struct domain *d,
+                                         unsigned int gsi))
 {
     struct hvm_irq *hvm_irq = hvm_domain_irq(d);
     unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
+    int vector = -1;
 
     ASSERT(isa_irq <= 15);
 
@@ -182,7 +184,12 @@ void hvm_isa_irq_assert(
          (hvm_irq->gsi_assert_count[gsi]++ == 0) )
         assert_irq(d, gsi, isa_irq);
 
+    if ( get_vector )
+        vector = get_vector(d, gsi);
+
     spin_unlock(&d->arch.hvm_domain.irq_lock);
+
+    return vector;
 }
 
 void hvm_isa_irq_deassert(
diff --git a/xen/arch/x86/hvm/pmtimer.c b/xen/arch/x86/hvm/pmtimer.c
index b70c299..435647f 100644
--- a/xen/arch/x86/hvm/pmtimer.c
+++ b/xen/arch/x86/hvm/pmtimer.c
@@ -61,7 +61,7 @@ static void pmt_update_sci(PMTState *s)
     ASSERT(spin_is_locked(&s->lock));
 
     if ( acpi->pm1a_en & acpi->pm1a_sts & SCI_MASK )
-        hvm_isa_irq_assert(s->vcpu->domain, SCI_IRQ);
+        hvm_isa_irq_assert(s->vcpu->domain, SCI_IRQ, NULL);
     else
         hvm_isa_irq_deassert(s->vcpu->domain, SCI_IRQ);
 }
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index bcfa169..cb75b99 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -75,7 +75,7 @@ static void rtc_update_irq(RTCState *s)
     s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
     if ( rtc_mode_is(s, no_ack) )
         hvm_isa_irq_deassert(vrtc_domain(s), RTC_IRQ);
-    hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
+    hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ, NULL);
 }
 
 /* Called by the VPT code after it's injected a PF interrupt for us.
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 4bfc53e..50f53bd 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -137,6 +137,18 @@ static void vlapic_error(struct vlapic *vlapic, unsigned int errmask)
     spin_unlock_irqrestore(&vlapic->esr_lock, flags);
 }
 
+bool vlapic_test_irq(const struct vlapic *vlapic, uint8_t vec)
+{
+    if ( unlikely(vec < 16) )
+        return false;
+
+    if ( hvm_funcs.test_pir &&
+         hvm_funcs.test_pir(const_vlapic_vcpu(vlapic), vec) )
+        return true;
+
+    return vlapic_test_vector(vec, &vlapic->regs->data[APIC_IRR]);
+}
+
 void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
 {
     struct vcpu *target = vlapic_vcpu(vlapic);
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c214870..b18ccea 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2125,6 +2125,11 @@ static void vmx_sync_pir_to_irr(struct vcpu *v)
         vlapic_set_vector(i, &vlapic->regs->data[APIC_IRR]);
 }
 
+static bool vmx_test_pir(const struct vcpu *v, uint8_t vec)
+{
+    return pi_test_pir(vec, &v->arch.hvm_vmx.pi_desc);
+}
+
 static void vmx_handle_eoi(u8 vector)
 {
     unsigned long status;
@@ -2352,6 +2357,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
     .process_isr          = vmx_process_isr,
     .deliver_posted_intr  = vmx_deliver_posted_intr,
     .sync_pir_to_irr      = vmx_sync_pir_to_irr,
+    .test_pir             = vmx_test_pir,
     .handle_eoi           = vmx_handle_eoi,
     .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
     .enable_msr_interception = vmx_enable_msr_interception,
@@ -2499,6 +2505,7 @@ const struct hvm_function_table * __init start_vmx(void)
     {
         vmx_function_table.deliver_posted_intr = NULL;
         vmx_function_table.sync_pir_to_irr = NULL;
+        vmx_function_table.test_pir = NULL;
     }
 
     if ( cpu_has_vmx_tsc_scaling )
diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 3841140..181f4cb 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -252,7 +252,7 @@ int pt_update_irq(struct vcpu *v)
     struct list_head *head = &v->arch.hvm_vcpu.tm_list;
     struct periodic_time *pt, *temp, *earliest_pt;
     uint64_t max_lag;
-    int irq, is_lapic;
+    int irq, is_lapic, pt_vector;
 
     spin_lock(&v->arch.hvm_vcpu.tm_lock);
 
@@ -292,25 +292,38 @@ int pt_update_irq(struct vcpu *v)
 
     spin_unlock(&v->arch.hvm_vcpu.tm_lock);
 
+    /*
+     * If periodic timer interrut is handled by lapic, its vector in
+     * IRR is returned and used to set eoi_exit_bitmap for virtual
+     * interrupt delivery case. Otherwise return -1 to do nothing.
+     */
     if ( is_lapic )
+    {
         vlapic_set_irq(vcpu_vlapic(v), irq, 0);
+        pt_vector = irq;
+    }
     else
     {
         hvm_isa_irq_deassert(v->domain, irq);
-        hvm_isa_irq_assert(v->domain, irq);
+        if ( platform_legacy_irq(irq) && vlapic_accept_pic_intr(v) &&
+             v->domain->arch.hvm_domain.vpic[irq >> 3].int_output )
+        {
+            hvm_isa_irq_assert(v->domain, irq, NULL);
+            pt_vector = -1;
+        }
+        else
+        {
+            pt_vector = hvm_isa_irq_assert(v->domain, irq, vioapic_get_vector);
+            /*
+             * hvm_isa_irq_assert may not set the corresponding bit in vIRR
+             * when mask field of IOAPIC RTE is set. Check it again.
+             */
+            if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
+                pt_vector = -1;
+        }
     }
 
-    /*
-     * If periodic timer interrut is handled by lapic, its vector in
-     * IRR is returned and used to set eoi_exit_bitmap for virtual
-     * interrupt delivery case. Otherwise return -1 to do nothing.  
-     */ 
-    if ( !is_lapic &&
-         platform_legacy_irq(irq) && vlapic_accept_pic_intr(v) &&
-         (&v->domain->arch.hvm_domain)->vpic[irq >> 3].int_output )
-        return -1;
-    else 
-        return pt_irq_vector(earliest_pt, hvm_intsrc_lapic);
+    return pt_vector;
 }
 
 static struct periodic_time *is_pt_irq(
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index b687e03..6ecad33 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -195,6 +195,7 @@ struct hvm_function_table {
     void (*process_isr)(int isr, struct vcpu *v);
     void (*deliver_posted_intr)(struct vcpu *v, u8 vector);
     void (*sync_pir_to_irr)(struct vcpu *v);
+    bool (*test_pir)(const struct vcpu *v, uint8_t vector);
     void (*handle_eoi)(u8 vector);
 
     /*Walk nested p2m  */
diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h
index 3b6b4bd..f756cb5 100644
--- a/xen/include/asm-x86/hvm/irq.h
+++ b/xen/include/asm-x86/hvm/irq.h
@@ -187,8 +187,16 @@ void hvm_pci_intx_assert(struct domain *d, unsigned int device,
 void hvm_pci_intx_deassert(struct domain *d, unsigned int device,
                            unsigned int intx);
 
-/* Modify state of an ISA device's IRQ wire. */
-void hvm_isa_irq_assert(struct domain *d, unsigned int isa_irq);
+/*
+ * Modify state of an ISA device's IRQ wire. For some cases, we are
+ * interested in the interrupt vector of the irq, but once the irq_lock
+ * is released, the vector may be changed by others. get_vector() callback
+ * allows us to get the interrupt vector in the protection of irq_lock.
+ * For most cases, just set get_vector to NULL.
+ */
+int hvm_isa_irq_assert(struct domain *d, unsigned int isa_irq,
+                       int (*get_vector)(const struct domain *d,
+                                         unsigned int gsi));
 void hvm_isa_irq_deassert(struct domain *d, unsigned int isa_irq);
 
 /* Modify state of GSIs. */
diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index a63fcd5..212c36b 100644
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -110,6 +110,7 @@ static inline void vlapic_set_reg(
 
 bool_t is_vlapic_lvtpc_enabled(struct vlapic *vlapic);
 
+bool vlapic_test_irq(const struct vlapic *vlapic, uint8_t vec);
 void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig);
 
 int vlapic_has_pending_irq(struct vcpu *v);
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 4889a64..7341cb1 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -104,11 +104,16 @@ void vmx_update_secondary_exec_control(struct vcpu *v);
 
 #define POSTED_INTR_ON  0
 #define POSTED_INTR_SN  1
-static inline int pi_test_and_set_pir(int vector, struct pi_desc *pi_desc)
+static inline int pi_test_and_set_pir(uint8_t vector, struct pi_desc *pi_desc)
 {
     return test_and_set_bit(vector, pi_desc->pir);
 }
 
+static inline int pi_test_pir(uint8_t vector, const struct pi_desc *pi_desc)
+{
+    return test_bit(vector, pi_desc->pir);
+}
+
 static inline int pi_test_and_set_on(struct pi_desc *pi_desc)
 {
     return test_and_set_bit(POSTED_INTR_ON, &pi_desc->control);
-- 
1.8.3.1


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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR
  2017-10-20  0:35 [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR Chao Gao
@ 2017-10-20 11:42 ` Jan Beulich
  2017-10-20 13:23   ` Julien Grall
  2017-10-23  8:08 ` Tian, Kevin
  1 sibling, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-10-20 11:42 UTC (permalink / raw)
  To: Chao Gao; +Cc: Andrew Cooper, Kevin Tian, Julien Grall, Jun Nakajima, xen-devel

>>> On 20.10.17 at 02:35, <chao.gao@intel.com> wrote:
> pt_update_irq() is expected to return the vector number of periodic
> timer interrupt, which should be set in vIRR of vlapic or in PIR.
> Otherwise it would trigger the assertion in vmx_intr_assist(), please
> seeing 
> https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg00915.html.
> 
> But it fails to achieve that in the following two case:
> 1. hvm_isa_irq_assert() may not set the corresponding bit in vIRR for
> mask field of IOAPIC RTE is set. Please refer to the call tree
> vmx_intr_assist() -> pt_update_irq() -> hvm_isa_irq_assert() ->
> assert_irq() -> assert_gsi() -> vioapic_irq_positive_edge(). The patch
> checks whether the vector is set or not in vIRR of vlapic or PIR before
> returning.
> 
> 2. someone changes the vector field of IOAPIC RTE between asserting
> the irq and getting the vector of the irq, leading to setting the
> old vector number but returning a different vector number. This patch
> allows hvm_isa_irq_assert() to accept a callback which can get the
> interrupt vector with irq_lock held. Thus, no one can change the vector
> between the two operations.
> 
> BTW, the first argument of pi_test_and_set_pir() should be uint8_t
> and I take this chance to fix it.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR
  2017-10-20 11:42 ` Jan Beulich
@ 2017-10-20 13:23   ` Julien Grall
  2017-10-20 14:16     ` Jan Beulich
  0 siblings, 1 reply; 6+ messages in thread
From: Julien Grall @ 2017-10-20 13:23 UTC (permalink / raw)
  To: Jan Beulich, Chao Gao; +Cc: Andrew Cooper, Kevin Tian, Jun Nakajima, xen-devel

Hi Jan,

On 20/10/17 12:42, Jan Beulich wrote:
>>>> On 20.10.17 at 02:35, <chao.gao@intel.com> wrote:
>> pt_update_irq() is expected to return the vector number of periodic
>> timer interrupt, which should be set in vIRR of vlapic or in PIR.
>> Otherwise it would trigger the assertion in vmx_intr_assist(), please
>> seeing
>> https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg00915.html.
>>
>> But it fails to achieve that in the following two case:
>> 1. hvm_isa_irq_assert() may not set the corresponding bit in vIRR for
>> mask field of IOAPIC RTE is set. Please refer to the call tree
>> vmx_intr_assist() -> pt_update_irq() -> hvm_isa_irq_assert() ->
>> assert_irq() -> assert_gsi() -> vioapic_irq_positive_edge(). The patch
>> checks whether the vector is set or not in vIRR of vlapic or PIR before
>> returning.
>>
>> 2. someone changes the vector field of IOAPIC RTE between asserting
>> the irq and getting the vector of the irq, leading to setting the
>> old vector number but returning a different vector number. This patch
>> allows hvm_isa_irq_assert() to accept a callback which can get the
>> interrupt vector with irq_lock held. Thus, no one can change the vector
>> between the two operations.
>>
>> BTW, the first argument of pi_test_and_set_pir() should be uint8_t
>> and I take this chance to fix it.
>>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Do you have any opinion on this patch going to Xen 4.10?

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR
  2017-10-20 13:23   ` Julien Grall
@ 2017-10-20 14:16     ` Jan Beulich
  2017-10-23 13:59       ` Julien Grall
  0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-10-20 14:16 UTC (permalink / raw)
  To: Julien Grall; +Cc: Andrew Cooper, Kevin Tian, xen-devel, Jun Nakajima, Chao Gao

>>> On 20.10.17 at 15:23, <julien.grall@linaro.org> wrote:
> On 20/10/17 12:42, Jan Beulich wrote:
>>>>> On 20.10.17 at 02:35, <chao.gao@intel.com> wrote:
>>> pt_update_irq() is expected to return the vector number of periodic
>>> timer interrupt, which should be set in vIRR of vlapic or in PIR.
>>> Otherwise it would trigger the assertion in vmx_intr_assist(), please
>>> seeing
>>> https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg00915.html.
>>>
>>> But it fails to achieve that in the following two case:
>>> 1. hvm_isa_irq_assert() may not set the corresponding bit in vIRR for
>>> mask field of IOAPIC RTE is set. Please refer to the call tree
>>> vmx_intr_assist() -> pt_update_irq() -> hvm_isa_irq_assert() ->
>>> assert_irq() -> assert_gsi() -> vioapic_irq_positive_edge(). The patch
>>> checks whether the vector is set or not in vIRR of vlapic or PIR before
>>> returning.
>>>
>>> 2. someone changes the vector field of IOAPIC RTE between asserting
>>> the irq and getting the vector of the irq, leading to setting the
>>> old vector number but returning a different vector number. This patch
>>> allows hvm_isa_irq_assert() to accept a callback which can get the
>>> interrupt vector with irq_lock held. Thus, no one can change the vector
>>> between the two operations.
>>>
>>> BTW, the first argument of pi_test_and_set_pir() should be uint8_t
>>> and I take this chance to fix it.
>>>
>>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> 
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Do you have any opinion on this patch going to Xen 4.10?

Well, the author having hopes that this addresses the assertion
failure we keep seeing in osstest every once in a while, I think
we certainly want to have it despite me not being fully convinced
that it'll actually help. I'm sufficiently convinced though it won't do
any bad.

Jan


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR
  2017-10-20  0:35 [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR Chao Gao
  2017-10-20 11:42 ` Jan Beulich
@ 2017-10-23  8:08 ` Tian, Kevin
  1 sibling, 0 replies; 6+ messages in thread
From: Tian, Kevin @ 2017-10-23  8:08 UTC (permalink / raw)
  To: Gao, Chao, xen-devel
  Cc: Andrew Cooper, Julien Grall, Jan Beulich, Nakajima, Jun

> From: Gao, Chao
> Sent: Friday, October 20, 2017 8:35 AM
> 
> pt_update_irq() is expected to return the vector number of periodic
> timer interrupt, which should be set in vIRR of vlapic or in PIR.
> Otherwise it would trigger the assertion in vmx_intr_assist(), please
> seeing https://lists.xenproject.org/archives/html/xen-devel/2017-
> 10/msg00915.html.
> 
> But it fails to achieve that in the following two case:
> 1. hvm_isa_irq_assert() may not set the corresponding bit in vIRR for
> mask field of IOAPIC RTE is set. Please refer to the call tree
> vmx_intr_assist() -> pt_update_irq() -> hvm_isa_irq_assert() ->
> assert_irq() -> assert_gsi() -> vioapic_irq_positive_edge(). The patch
> checks whether the vector is set or not in vIRR of vlapic or PIR before
> returning.
> 
> 2. someone changes the vector field of IOAPIC RTE between asserting
> the irq and getting the vector of the irq, leading to setting the
> old vector number but returning a different vector number. This patch
> allows hvm_isa_irq_assert() to accept a callback which can get the
> interrupt vector with irq_lock held. Thus, no one can change the vector
> between the two operations.
> 
> BTW, the first argument of pi_test_and_set_pir() should be uint8_t
> and I take this chance to fix it.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR
  2017-10-20 14:16     ` Jan Beulich
@ 2017-10-23 13:59       ` Julien Grall
  0 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2017-10-23 13:59 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Andrew Cooper, Kevin Tian, xen-devel, Jun Nakajima, Chao Gao

Hi,

On 20/10/17 15:16, Jan Beulich wrote:
>>>> On 20.10.17 at 15:23, <julien.grall@linaro.org> wrote:
>> On 20/10/17 12:42, Jan Beulich wrote:
>>>>>> On 20.10.17 at 02:35, <chao.gao@intel.com> wrote:
>>>> pt_update_irq() is expected to return the vector number of periodic
>>>> timer interrupt, which should be set in vIRR of vlapic or in PIR.
>>>> Otherwise it would trigger the assertion in vmx_intr_assist(), please
>>>> seeing
>>>> https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg00915.html.
>>>>
>>>> But it fails to achieve that in the following two case:
>>>> 1. hvm_isa_irq_assert() may not set the corresponding bit in vIRR for
>>>> mask field of IOAPIC RTE is set. Please refer to the call tree
>>>> vmx_intr_assist() -> pt_update_irq() -> hvm_isa_irq_assert() ->
>>>> assert_irq() -> assert_gsi() -> vioapic_irq_positive_edge(). The patch
>>>> checks whether the vector is set or not in vIRR of vlapic or PIR before
>>>> returning.
>>>>
>>>> 2. someone changes the vector field of IOAPIC RTE between asserting
>>>> the irq and getting the vector of the irq, leading to setting the
>>>> old vector number but returning a different vector number. This patch
>>>> allows hvm_isa_irq_assert() to accept a callback which can get the
>>>> interrupt vector with irq_lock held. Thus, no one can change the vector
>>>> between the two operations.
>>>>
>>>> BTW, the first argument of pi_test_and_set_pir() should be uint8_t
>>>> and I take this chance to fix it.
>>>>
>>>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>>>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> Do you have any opinion on this patch going to Xen 4.10?
> 
> Well, the author having hopes that this addresses the assertion
> failure we keep seeing in osstest every once in a while, I think
> we certainly want to have it despite me not being fully convinced
> that it'll actually help. I'm sufficiently convinced though it won't do
> any bad.

I guess it is worth having a try then:

Release-acked-by: Julien Grall <julien.grall@linaro.org>

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-10-23 13:59 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-20  0:35 [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR Chao Gao
2017-10-20 11:42 ` Jan Beulich
2017-10-20 13:23   ` Julien Grall
2017-10-20 14:16     ` Jan Beulich
2017-10-23 13:59       ` Julien Grall
2017-10-23  8:08 ` Tian, Kevin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.