[PATCH] x86/vpt: fix a bug in pt_update_irq()

[PATCH] x86/vpt: fix a bug in pt_update_irq()

[Chao Gao]

pt_update_irq() is expected to return the vector number of periodic
timer interrupt, which should be set in vIRR of vlapic. 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 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
holds the irq_lock when doing the two operations to prevent the case.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 xen/arch/x86/hvm/irq.c           | 11 ++++++----
 xen/arch/x86/hvm/vlapic.c        | 11 ++++++++++
 xen/arch/x86/hvm/vpt.c           | 43 ++++++++++++++++++++++++++++------------
 xen/include/asm-x86/hvm/irq.h    |  1 +
 xen/include/asm-x86/hvm/vlapic.h |  1 +
 5 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index e425df9..7b0c0b1 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -168,20 +168,23 @@ 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)
+void hvm_isa_irq_assert_locked(struct domain *d, unsigned int isa_irq)
 {
     struct hvm_irq *hvm_irq = hvm_domain_irq(d);
     unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
 
     ASSERT(isa_irq <= 15);
-
-    spin_lock(&d->arch.hvm_domain.irq_lock);
+    ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock));
 
     if ( !__test_and_set_bit(isa_irq, &hvm_irq->isa_irq.i) &&
          (hvm_irq->gsi_assert_count[gsi]++ == 0) )
         assert_irq(d, gsi, isa_irq);
+}
 
+void hvm_isa_irq_assert(struct domain *d, unsigned int isa_irq)
+{
+    spin_lock(&d->arch.hvm_domain.irq_lock);
+    hvm_isa_irq_assert_locked(d, isa_irq);
     spin_unlock(&d->arch.hvm_domain.irq_lock);
 }
 
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 4bfc53e..b27b15b 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -137,6 +137,17 @@ static void vlapic_error(struct vlapic *vlapic, unsigned int errmask)
     spin_unlock_irqrestore(&vlapic->esr_lock, flags);
 }
 
+bool vlapic_test_irq(struct vlapic *vlapic, uint8_t vec)
+{
+    if ( unlikely(vec < 16) )
+        return false;
+
+    if ( hvm_funcs.sync_pir_to_irr )
+        hvm_funcs.sync_pir_to_irr(vlapic_vcpu(vlapic));
+
+    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/vpt.c b/xen/arch/x86/hvm/vpt.c
index 3841140..f4451fd 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,42 @@ 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);
+        /*
+         * Hold 'irq_lock' to prevent changing the interrupt vector between
+         * asserting the irq and getting the interrupt vector of the irq.
+         */
+        spin_lock(&v->domain->arch.hvm_domain.irq_lock);
+        hvm_isa_irq_assert_locked(v->domain, irq);
+        if ( platform_legacy_irq(irq) && vlapic_accept_pic_intr(v) &&
+             v->domain->arch.hvm_domain.vpic[irq >> 3].int_output )
+            pt_vector = -1;
+        else
+        {
+            pt_vector = pt_irq_vector(earliest_pt, hvm_intsrc_lapic);
+            /*
+             * hvm_isa_irq_assert_locked() may not set the corresponding bit
+             * in vIRR when mask field of IOAPIC RTE is set. Check it again.
+             */
+            if ( !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
+                pt_vector = -1;
+        }
+        spin_unlock(&v->domain->arch.hvm_domain.irq_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 &&
-         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/irq.h b/xen/include/asm-x86/hvm/irq.h
index 3b6b4bd..d20131a 100644
--- a/xen/include/asm-x86/hvm/irq.h
+++ b/xen/include/asm-x86/hvm/irq.h
@@ -189,6 +189,7 @@ void hvm_pci_intx_deassert(struct domain *d, unsigned int device,
 
 /* Modify state of an ISA device's IRQ wire. */
 void hvm_isa_irq_assert(struct domain *d, unsigned int isa_irq);
+void hvm_isa_irq_assert_locked(struct domain *d, unsigned int isa_irq);
 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..1f849c4 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(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);
-- 
1.8.3.1


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

Re: [PATCH] x86/vpt: fix a bug in pt_update_irq()

[Jan Beulich]

>>> On 09.10.17 at 23:32, <chao.gao@intel.com> wrote:

First of all - please use a better subject. If someone finds another
bug in this function in, say, half a year's time, how will we tell apart
the two patches from looking at just the list of titles several years
later?

> pt_update_irq() is expected to return the vector number of periodic
> timer interrupt, which should be set in vIRR of vlapic. 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 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
> holds the irq_lock when doing the two operations to prevent the case.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>

Point 2 is very unlikely to be the cause of the failed assertion that
osstest keeps hitting once in a while. Did your analysis yield
indication that point 1 is what is happening there?

> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -168,20 +168,23 @@ 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)
> +void hvm_isa_irq_assert_locked(struct domain *d, unsigned int isa_irq)

Please don't introduce a non-static function like this. Instead I
would suggest you introduce a new helper function doing what
you introduce as replacement to the call to
hvm_isa_irq_assert(). That'll presumably involve passing a
get_vector() callback to a wrapper of pt_irq_vector() (or to an
abbreviated form of it, as "src" is hvm_intsrc_lapic), since I
understand you need this called with the lock held.

And once you do this I don't think it'll be worthwhile breaking
out hvm_isa_irq_assert_locked() at all - you'll just have a
sibling to hvm_isa_irq_assert(). Or, considering the few callers
the function has, simply giving that function itself an optional
callback parameter might be even better (eliminating any code
duplication).

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -137,6 +137,17 @@ static void vlapic_error(struct vlapic *vlapic, unsigned int errmask)
>      spin_unlock_irqrestore(&vlapic->esr_lock, flags);
>  }
>  
> +bool vlapic_test_irq(struct vlapic *vlapic, uint8_t vec)

The way the function is named, the pointer should be const
qualified. However, the function does more than just testing
current state:

> +{
> +    if ( unlikely(vec < 16) )
> +        return false;
> +
> +    if ( hvm_funcs.sync_pir_to_irr )
> +        hvm_funcs.sync_pir_to_irr(vlapic_vcpu(vlapic));

Question is whether this is really necessary, of whether instead
you could just return the state of the respective PIR bit here. I'd
prefer that over giving the function a name no longer suggesting
it leaves all state alone.

Jan


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

Re: [PATCH] x86/vpt: fix a bug in pt_update_irq()

[Chao Gao]

On Fri, Oct 13, 2017 at 02:25:38AM -0600, Jan Beulich wrote:
>>>> On 09.10.17 at 23:32, <chao.gao@intel.com> wrote:
>
>First of all - please use a better subject. If someone finds another
>bug in this function in, say, half a year's time, how will we tell apart
>the two patches from looking at just the list of titles several years
>later?

Will update.

>
>> pt_update_irq() is expected to return the vector number of periodic
>> timer interrupt, which should be set in vIRR of vlapic. 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 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
>> holds the irq_lock when doing the two operations to prevent the case.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>
>Point 2 is very unlikely to be the cause of the failed assertion that
>osstest keeps hitting once in a while. Did your analysis yield
>indication that point 1 is what is happening there?

I believe it is likely to be the case. On the other hand, the
assertion can be triggered in above two cases; it needs to be
fixed.

>
>> --- a/xen/arch/x86/hvm/irq.c
>> +++ b/xen/arch/x86/hvm/irq.c
>> @@ -168,20 +168,23 @@ 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)
>> +void hvm_isa_irq_assert_locked(struct domain *d, unsigned int isa_irq)
>
>Please don't introduce a non-static function like this. Instead I
>would suggest you introduce a new helper function doing what
>you introduce as replacement to the call to
>hvm_isa_irq_assert(). That'll presumably involve passing a
>get_vector() callback to a wrapper of pt_irq_vector() (or to an
>abbreviated form of it, as "src" is hvm_intsrc_lapic), since I
>understand you need this called with the lock held.
>
>And once you do this I don't think it'll be worthwhile breaking
>out hvm_isa_irq_assert_locked() at all - you'll just have a
>sibling to hvm_isa_irq_assert(). Or, considering the few callers
>the function has, simply giving that function itself an optional
>callback parameter might be even better (eliminating any code
>duplication).

Ok. I understand what you suggestion. Will give it a shot.

>
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -137,6 +137,17 @@ static void vlapic_error(struct vlapic *vlapic, unsigned int errmask)
>>      spin_unlock_irqrestore(&vlapic->esr_lock, flags);
>>  }
>>  
>> +bool vlapic_test_irq(struct vlapic *vlapic, uint8_t vec)
>
>The way the function is named, the pointer should be const
>qualified. However, the function does more than just testing
>current state:
>
>> +{
>> +    if ( unlikely(vec < 16) )
>> +        return false;
>> +
>> +    if ( hvm_funcs.sync_pir_to_irr )
>> +        hvm_funcs.sync_pir_to_irr(vlapic_vcpu(vlapic));
>
>Question is whether this is really necessary, of whether instead
>you could just return the state of the respective PIR bit here. I'd
>prefer that over giving the function a name no longer suggesting
>it leaves all state alone.

It is a good suggestion. But I incline to check the PIR bit and if the
bit is set, return true and otherwise return the state of the vIRR bit
in case PIR bits are already synced to vIRR.

Thanks
Chao

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

Re: [PATCH] x86/vpt: fix a bug in pt_update_irq()

[Jan Beulich]

>>> On 13.10.17 at 10:14, <chao.gao@intel.com> wrote:
> On Fri, Oct 13, 2017 at 02:25:38AM -0600, Jan Beulich wrote:
>>>>> On 09.10.17 at 23:32, <chao.gao@intel.com> wrote:
>>> +{
>>> +    if ( unlikely(vec < 16) )
>>> +        return false;
>>> +
>>> +    if ( hvm_funcs.sync_pir_to_irr )
>>> +        hvm_funcs.sync_pir_to_irr(vlapic_vcpu(vlapic));
>>
>>Question is whether this is really necessary, of whether instead
>>you could just return the state of the respective PIR bit here. I'd
>>prefer that over giving the function a name no longer suggesting
>>it leaves all state alone.
> 
> It is a good suggestion. But I incline to check the PIR bit and if the
> bit is set, return true and otherwise return the state of the vIRR bit
> in case PIR bits are already synced to vIRR.

Oh, right, of course you should return the effective "or" of the
two bits.

Jan


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