[PATCH for-4.9 v2] hvm/vpt: fix inconsistent views of vIOAPIC in vmx_intr_assist()

[PATCH for-4.9 v2] hvm/vpt: fix inconsistent views of vIOAPIC in vmx_intr_assist()

[Chao Gao]

When injecting periodic timer interrupt in vmx_intr_assist(),
multi-read operations are done during one event delivery. For
example, if a periodic timer interrupt is from PIT, when set the
corresponding bit in vIRR, the corresponding RTE is accessed in
pt_update_irq(). When this function returns, it accesses the RTE
again to get the vector it sets in vIRR.  Between the two
accesses, the content of RTE may have been changed by another CPU
for no protection method in use. This case can incur the
assertion failure in vmx_intr_assist().  Also after a periodic
timer interrupt is injected successfully, pt_irq_posted() will
decrease the count (pending_intr_nr). But if the corresponding
RTE has been changed, pt_irq_posted() will not decrease the
count, resulting one more timer interrupt.

More discussion and history can be found in
1.https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg00906.html
2.https://lists.xenproject.org/archives/html/xen-devel/2017-01/msg01019.html

This patch introduces a new field 'latched_vector' to structure
periodic_timer. The field is only valid between calling pt_update_irq()
and calling pt_irq_posted() in vmx_intr_assist(). The new field is set
to the vector we set in vIRR at the first access we describe above, is
used in the following two accesses through calling pt_irq_vector() and
finally cleared in pt_irq_posted() or updated in next calling
vmx_intr_assist(). The latching operation should be protected by
irq_lock to prevent other vCPUs changing the value we are interest in.
Thus, provide a -locked variant of hvm_isa_irq_assert() to avoid
deadlock.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
This patch is to fix a user triggerable assertion failure. I think it should
be fixed in 4.9.

---
v2:
- only use hold irq_lock in pt_update_irq(). Avoid holding irq_lock
in vmx_intr_assist() to avoid putting too much functions in the
locked-region.

---
 xen/arch/x86/hvm/irq.c        | 11 ++++++---
 xen/arch/x86/hvm/vpt.c        | 56 +++++++++++++++++++++++++++----------------
 xen/include/asm-x86/hvm/vpt.h |  6 +++++
 xen/include/xen/hvm/irq.h     |  2 ++
 4 files changed, 52 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 8625584..60deb6e 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -126,20 +126,25 @@ void hvm_pci_intx_deassert(
     spin_unlock(&d->arch.hvm_domain.irq_lock);
 }
 
-void hvm_isa_irq_assert(
+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/vpt.c b/xen/arch/x86/hvm/vpt.c
index e3f2039..0edfc4a 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -75,33 +75,43 @@ void hvm_set_guest_time(struct vcpu *v, u64 guest_time)
     }
 }
 
-static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
+static void pt_set_latched_vector(struct periodic_time *pt, enum hvm_intsrc src)
 {
     struct vcpu *v = pt->vcpu;
     struct hvm_vioapic *vioapic;
-    unsigned int gsi, isa_irq, pin;
+    unsigned int gsi, pin;
+
+    ASSERT(pt->source == PTSRC_isa);
+    ASSERT(src == hvm_intsrc_lapic);
+    gsi = hvm_isa_irq_to_gsi(pt->irq);
+    vioapic = gsi_vioapic(v->domain, gsi, &pin);
+    if ( !vioapic )
+    {
+        dprintk(XENLOG_WARNING, "d%u: invalid GSI (%u) for platform timer\n",
+                v->domain->domain_id, gsi);
+        domain_crash(v->domain);
+        return;
+    }
+
+    pt->latched_vector = vioapic->redirtbl[pin].fields.vector;
+}
+
+static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
+{
+    struct vcpu *v = pt->vcpu;
+    unsigned int isa_irq;
 
     if ( pt->source == PTSRC_lapic )
         return pt->irq;
 
     isa_irq = pt->irq;
-    gsi = hvm_isa_irq_to_gsi(isa_irq);
 
     if ( src == hvm_intsrc_pic )
         return (v->domain->arch.hvm_domain.vpic[isa_irq >> 3].irq_base
                 + (isa_irq & 7));
 
     ASSERT(src == hvm_intsrc_lapic);
-    vioapic = gsi_vioapic(v->domain, gsi, &pin);
-    if ( !vioapic )
-    {
-        dprintk(XENLOG_WARNING, "d%u: invalid GSI (%u) for platform timer\n",
-                v->domain->domain_id, gsi);
-        domain_crash(v->domain);
-        return -1;
-    }
-
-    return vioapic->redirtbl[pin].fields.vector;
+    return pt->latched_vector;
 }
 
 static int pt_irq_masked(struct periodic_time *pt)
@@ -253,6 +263,7 @@ int pt_update_irq(struct vcpu *v)
     struct periodic_time *pt, *temp, *earliest_pt;
     uint64_t max_lag;
     int irq, is_lapic;
+    bool handle_by_pic = false;
 
     spin_lock(&v->arch.hvm_vcpu.tm_lock);
 
@@ -297,7 +308,15 @@ int pt_update_irq(struct vcpu *v)
     else
     {
         hvm_isa_irq_deassert(v->domain, irq);
-        hvm_isa_irq_assert(v->domain, 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 )
+        {
+            handle_by_pic = true;
+            pt_set_latched_vector(earliest_pt, hvm_intsrc_lapic);
+        }
+        spin_unlock(&v->domain->arch.hvm_domain.irq_lock);
     }
 
     /*
@@ -305,12 +324,7 @@ int pt_update_irq(struct vcpu *v)
      * 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 handle_by_pic ? -1 : pt_irq_vector(earliest_pt, hvm_intsrc_lapic);
 }
 
 static struct periodic_time *is_pt_irq(
@@ -347,6 +361,7 @@ void pt_intr_post(struct vcpu *v, struct hvm_intack intack)
         return;
     }
 
+    pt->latched_vector = -1;
     pt->irq_issued = 0;
 
     if ( pt->one_shot )
@@ -414,6 +429,7 @@ void create_periodic_time(
     pt->pending_intr_nr = 0;
     pt->do_not_freeze = 0;
     pt->irq_issued = 0;
+    pt->latched_vector = -1;
 
     /* Periodic timer must be at least 0.1ms. */
     if ( (period < 100000) && period )
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index 21166ed..2daf356 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -46,6 +46,12 @@ struct periodic_time {
 #define PTSRC_lapic  2 /* LAPIC time source */
     u8 source;                  /* PTSRC_ */
     u8 irq;
+    /*
+     * Vector set in vIRR in one interrupt delivery. Using this value can
+     * avoid reading the IOAPIC RTE again. Valid only when its source is
+     * 'PTSRC_isa' and handled by vlapic.
+     */
+    int16_t latched_vector;
     struct vcpu *vcpu;          /* vcpu timer interrupt delivers to */
     u32 pending_intr_nr;        /* pending timer interrupts */
     u64 period;                 /* frequency in ns */
diff --git a/xen/include/xen/hvm/irq.h b/xen/include/xen/hvm/irq.h
index 671a6f2..2451e8d 100644
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -120,6 +120,8 @@ void hvm_pci_intx_deassert(
 /* 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);
 
-- 
1.8.3.1


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

Re: [PATCH for-4.9 v2] hvm/vpt: fix inconsistent views of vIOAPIC in vmx_intr_assist()

[Chao Gao]

On Fri, Apr 28, 2017 at 01:59:47PM +0800, Tian, Kevin wrote:
>> From: Gao, Chao
>> Sent: Thursday, April 27, 2017 10:47 AM
>> 
>> When injecting periodic timer interrupt in vmx_intr_assist(),
>> multi-read operations are done during one event delivery. For
>> example, if a periodic timer interrupt is from PIT, when set the
>> corresponding bit in vIRR, the corresponding RTE is accessed in
>> pt_update_irq(). When this function returns, it accesses the RTE
>> again to get the vector it sets in vIRR.  Between the two
>> accesses, the content of RTE may have been changed by another CPU
>> for no protection method in use. This case can incur the
>> assertion failure in vmx_intr_assist().  Also after a periodic
>> timer interrupt is injected successfully, pt_irq_posted() will
>> decrease the count (pending_intr_nr). But if the corresponding
>> RTE has been changed, pt_irq_posted() will not decrease the
>> count, resulting one more timer interrupt.
>> 
>> More discussion and history can be found in
>> 1.https://lists.xenproject.org/archives/html/xen-devel/2017-
>> 03/msg00906.html
>> 2.https://lists.xenproject.org/archives/html/xen-devel/2017-
>> 01/msg01019.html
>> 
>> This patch introduces a new field 'latched_vector' to structure
>> periodic_timer. The field is only valid between calling pt_update_irq()
>> and calling pt_irq_posted() in vmx_intr_assist(). The new field is set
>> to the vector we set in vIRR at the first access we describe above, is
>> used in the following two accesses through calling pt_irq_vector() and
>> finally cleared in pt_irq_posted() or updated in next calling
>> vmx_intr_assist(). The latching operation should be protected by
>> irq_lock to prevent other vCPUs changing the value we are interest in.
>> Thus, provide a -locked variant of hvm_isa_irq_assert() to avoid
>> deadlock.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> +++ b/xen/arch/x86/hvm/vpt.c
>> @@ -75,33 +75,43 @@ void hvm_set_guest_time(struct vcpu *v, u64
>> guest_time)
>>      }
>>  }
>> 
>> -static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
>> +static void pt_set_latched_vector(struct periodic_time *pt, enum
>> hvm_intsrc src)
>>  {
>>      struct vcpu *v = pt->vcpu;
>>      struct hvm_vioapic *vioapic;
>> -    unsigned int gsi, isa_irq, pin;
>> +    unsigned int gsi, pin;
>> +
>> +    ASSERT(pt->source == PTSRC_isa);
>> +    ASSERT(src == hvm_intsrc_lapic);
>
>Do we really need add such limitation here? Is it simpler to rename
>original pt_irq_vector as pt_set_latched_vector by adding one
>line to record returned value for all conditions and then define
>pt_irq_vector simply as returning pt_latched_vector?

Almost agree. We don't latch vector for case 'handle_by_pic == true'.
Need be more careful for this case. For that case, the vector is
decided in hvm_vcpu_ack_pending_irq(). But at that place, we don't
know which periodic timer's latched_vector filed should be set.
Is traversal a clean way? or let pt_irq_vector() take 'hvm_intsrc_pic'
as a special case.

>
>> +    gsi = hvm_isa_irq_to_gsi(pt->irq);
>> +    vioapic = gsi_vioapic(v->domain, gsi, &pin);
>> +    if ( !vioapic )
>> +    {
>> +        dprintk(XENLOG_WARNING, "d%u: invalid GSI (%u) for platform
>> timer\n",
>> +                v->domain->domain_id, gsi);
>> +        domain_crash(v->domain);
>> +        return;
>> +    }
>> +
>> +    pt->latched_vector = vioapic->redirtbl[pin].fields.vector;
>> +}
>> +
>>      else
>>      {
>>          hvm_isa_irq_deassert(v->domain, irq);
>> -        hvm_isa_irq_assert(v->domain, 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 )
>> +        {
>> +            handle_by_pic = true;
>> +            pt_set_latched_vector(earliest_pt, hvm_intsrc_lapic);
>> +        }
>
>I think you changed the original semantics here. Originally -1
>is returned when above condition is true otherwise pt_irq_vector
>is returned. The otherwise condition is what you would like to
>fix by latching vector. However your patch actually reverts
>the policy.

Indeed. will fix this. Thanks to figure this out.

Thanks
Chao

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

Re: [PATCH for-4.9 v2] hvm/vpt: fix inconsistent views of vIOAPIC in vmx_intr_assist()

[Tian, Kevin]

> From: Gao, Chao
> Sent: Thursday, April 27, 2017 10:47 AM
> 
> When injecting periodic timer interrupt in vmx_intr_assist(),
> multi-read operations are done during one event delivery. For
> example, if a periodic timer interrupt is from PIT, when set the
> corresponding bit in vIRR, the corresponding RTE is accessed in
> pt_update_irq(). When this function returns, it accesses the RTE
> again to get the vector it sets in vIRR.  Between the two
> accesses, the content of RTE may have been changed by another CPU
> for no protection method in use. This case can incur the
> assertion failure in vmx_intr_assist().  Also after a periodic
> timer interrupt is injected successfully, pt_irq_posted() will
> decrease the count (pending_intr_nr). But if the corresponding
> RTE has been changed, pt_irq_posted() will not decrease the
> count, resulting one more timer interrupt.
> 
> More discussion and history can be found in
> 1.https://lists.xenproject.org/archives/html/xen-devel/2017-
> 03/msg00906.html
> 2.https://lists.xenproject.org/archives/html/xen-devel/2017-
> 01/msg01019.html
> 
> This patch introduces a new field 'latched_vector' to structure
> periodic_timer. The field is only valid between calling pt_update_irq()
> and calling pt_irq_posted() in vmx_intr_assist(). The new field is set
> to the vector we set in vIRR at the first access we describe above, is
> used in the following two accesses through calling pt_irq_vector() and
> finally cleared in pt_irq_posted() or updated in next calling
> vmx_intr_assist(). The latching operation should be protected by
> irq_lock to prevent other vCPUs changing the value we are interest in.
> Thus, provide a -locked variant of hvm_isa_irq_assert() to avoid
> deadlock.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
> This patch is to fix a user triggerable assertion failure. I think it should
> be fixed in 4.9.
> 
> ---
> v2:
> - only use hold irq_lock in pt_update_irq(). Avoid holding irq_lock
> in vmx_intr_assist() to avoid putting too much functions in the
> locked-region.
> 
> ---
>  xen/arch/x86/hvm/irq.c        | 11 ++++++---
>  xen/arch/x86/hvm/vpt.c        | 56 +++++++++++++++++++++++++++-------------
> ---
>  xen/include/asm-x86/hvm/vpt.h |  6 +++++
>  xen/include/xen/hvm/irq.h     |  2 ++
>  4 files changed, 52 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
> index 8625584..60deb6e 100644
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -126,20 +126,25 @@ void hvm_pci_intx_deassert(
>      spin_unlock(&d->arch.hvm_domain.irq_lock);
>  }
> 
> -void hvm_isa_irq_assert(
> +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/vpt.c b/xen/arch/x86/hvm/vpt.c
> index e3f2039..0edfc4a 100644
> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -75,33 +75,43 @@ void hvm_set_guest_time(struct vcpu *v, u64
> guest_time)
>      }
>  }
> 
> -static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
> +static void pt_set_latched_vector(struct periodic_time *pt, enum
> hvm_intsrc src)
>  {
>      struct vcpu *v = pt->vcpu;
>      struct hvm_vioapic *vioapic;
> -    unsigned int gsi, isa_irq, pin;
> +    unsigned int gsi, pin;
> +
> +    ASSERT(pt->source == PTSRC_isa);
> +    ASSERT(src == hvm_intsrc_lapic);

Do we really need add such limitation here? Is it simpler to rename
original pt_irq_vector as pt_set_latched_vector by adding one
line to record returned value for all conditions and then define
pt_irq_vector simply as returning pt_latched_vector?

> +    gsi = hvm_isa_irq_to_gsi(pt->irq);
> +    vioapic = gsi_vioapic(v->domain, gsi, &pin);
> +    if ( !vioapic )
> +    {
> +        dprintk(XENLOG_WARNING, "d%u: invalid GSI (%u) for platform
> timer\n",
> +                v->domain->domain_id, gsi);
> +        domain_crash(v->domain);
> +        return;
> +    }
> +
> +    pt->latched_vector = vioapic->redirtbl[pin].fields.vector;
> +}
> +
> +static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
> +{
> +    struct vcpu *v = pt->vcpu;
> +    unsigned int isa_irq;
> 
>      if ( pt->source == PTSRC_lapic )
>          return pt->irq;
> 
>      isa_irq = pt->irq;
> -    gsi = hvm_isa_irq_to_gsi(isa_irq);
> 
>      if ( src == hvm_intsrc_pic )
>          return (v->domain->arch.hvm_domain.vpic[isa_irq >> 3].irq_base
>                  + (isa_irq & 7));
> 
>      ASSERT(src == hvm_intsrc_lapic);
> -    vioapic = gsi_vioapic(v->domain, gsi, &pin);
> -    if ( !vioapic )
> -    {
> -        dprintk(XENLOG_WARNING, "d%u: invalid GSI (%u) for platform
> timer\n",
> -                v->domain->domain_id, gsi);
> -        domain_crash(v->domain);
> -        return -1;
> -    }
> -
> -    return vioapic->redirtbl[pin].fields.vector;
> +    return pt->latched_vector;
>  }
> 
>  static int pt_irq_masked(struct periodic_time *pt)
> @@ -253,6 +263,7 @@ int pt_update_irq(struct vcpu *v)
>      struct periodic_time *pt, *temp, *earliest_pt;
>      uint64_t max_lag;
>      int irq, is_lapic;
> +    bool handle_by_pic = false;
> 
>      spin_lock(&v->arch.hvm_vcpu.tm_lock);
> 
> @@ -297,7 +308,15 @@ int pt_update_irq(struct vcpu *v)
>      else
>      {
>          hvm_isa_irq_deassert(v->domain, irq);
> -        hvm_isa_irq_assert(v->domain, 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 )
> +        {
> +            handle_by_pic = true;
> +            pt_set_latched_vector(earliest_pt, hvm_intsrc_lapic);
> +        }

I think you changed the original semantics here. Originally -1
is returned when above condition is true otherwise pt_irq_vector
is returned. The otherwise condition is what you would like to
fix by latching vector. However your patch actually reverts
the policy.

> +        spin_unlock(&v->domain->arch.hvm_domain.irq_lock);
>      }
> 
>      /*
> @@ -305,12 +324,7 @@ int pt_update_irq(struct vcpu *v)
>       * 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 handle_by_pic ? -1 : pt_irq_vector(earliest_pt, hvm_intsrc_lapic);
>  }
> 
>  static struct periodic_time *is_pt_irq(
> @@ -347,6 +361,7 @@ void pt_intr_post(struct vcpu *v, struct hvm_intack
> intack)
>          return;
>      }
> 
> +    pt->latched_vector = -1;
>      pt->irq_issued = 0;
> 
>      if ( pt->one_shot )
> @@ -414,6 +429,7 @@ void create_periodic_time(
>      pt->pending_intr_nr = 0;
>      pt->do_not_freeze = 0;
>      pt->irq_issued = 0;
> +    pt->latched_vector = -1;
> 
>      /* Periodic timer must be at least 0.1ms. */
>      if ( (period < 100000) && period )
> diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-
> x86/hvm/vpt.h
> index 21166ed..2daf356 100644
> --- a/xen/include/asm-x86/hvm/vpt.h
> +++ b/xen/include/asm-x86/hvm/vpt.h
> @@ -46,6 +46,12 @@ struct periodic_time {
>  #define PTSRC_lapic  2 /* LAPIC time source */
>      u8 source;                  /* PTSRC_ */
>      u8 irq;
> +    /*
> +     * Vector set in vIRR in one interrupt delivery. Using this value can
> +     * avoid reading the IOAPIC RTE again. Valid only when its source is
> +     * 'PTSRC_isa' and handled by vlapic.
> +     */
> +    int16_t latched_vector;
>      struct vcpu *vcpu;          /* vcpu timer interrupt delivers to */
>      u32 pending_intr_nr;        /* pending timer interrupts */
>      u64 period;                 /* frequency in ns */
> diff --git a/xen/include/xen/hvm/irq.h b/xen/include/xen/hvm/irq.h
> index 671a6f2..2451e8d 100644
> --- a/xen/include/xen/hvm/irq.h
> +++ b/xen/include/xen/hvm/irq.h
> @@ -120,6 +120,8 @@ void hvm_pci_intx_deassert(
>  /* 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);
> 
> --
> 1.8.3.1


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

Re: [PATCH for-4.9 v2] hvm/vpt: fix inconsistent views of vIOAPIC in vmx_intr_assist()

[Jan Beulich]

>>> On 27.04.17 at 04:47, <chao.gao@intel.com> wrote:
> When injecting periodic timer interrupt in vmx_intr_assist(),
> multi-read operations are done during one event delivery. For
> example, if a periodic timer interrupt is from PIT, when set the
> corresponding bit in vIRR, the corresponding RTE is accessed in
> pt_update_irq(). When this function returns, it accesses the RTE
> again to get the vector it sets in vIRR.  Between the two
> accesses, the content of RTE may have been changed by another CPU
> for no protection method in use. This case can incur the
> assertion failure in vmx_intr_assist().

Btw., irrespective of this I'm not convinced this patch really is
4.9 material: While analyzing possible causes for the ASSERT()
to trigger, you've found a way no real OS would ever use. And
we'll need to do something about that ASSERT() anyway before
4.9 goes out.

> This patch introduces a new field 'latched_vector' to structure
> periodic_timer. The field is only valid between calling pt_update_irq()
> and calling pt_irq_posted() in vmx_intr_assist(). The new field is set
> to the vector we set in vIRR at the first access we describe above, is
> used in the following two accesses through calling pt_irq_vector() and
> finally cleared in pt_irq_posted() or updated in next calling
> vmx_intr_assist().

Please refer to the correct functions - there's no pt_irq_posted(), and
vmx_intr_assist() doesn't itself update the field.

> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -75,33 +75,43 @@ void hvm_set_guest_time(struct vcpu *v, u64 guest_time)
>      }
>  }
>  
> -static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
> +static void pt_set_latched_vector(struct periodic_time *pt, enum hvm_intsrc src)
>  {
>      struct vcpu *v = pt->vcpu;
>      struct hvm_vioapic *vioapic;

Both should be const. Also you don't really need v anywhere below,
all uses are v->domain.

> -    unsigned int gsi, isa_irq, pin;
> +    unsigned int gsi, pin;
> +
> +    ASSERT(pt->source == PTSRC_isa);
> +    ASSERT(src == hvm_intsrc_lapic);
> +    gsi = hvm_isa_irq_to_gsi(pt->irq);
> +    vioapic = gsi_vioapic(v->domain, gsi, &pin);
> +    if ( !vioapic )
> +    {
> +        dprintk(XENLOG_WARNING, "d%u: invalid GSI (%u) for platform timer\n",
> +                v->domain->domain_id, gsi);
> +        domain_crash(v->domain);
> +        return;
> +    }
> +
> +    pt->latched_vector = vioapic->redirtbl[pin].fields.vector;
> +}
> +
> +static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
> +{
> +    struct vcpu *v = pt->vcpu;

There is not need for this local variable - it's used exactly once (and
then again only to get to the domain).

> +    unsigned int isa_irq;
>  
>      if ( pt->source == PTSRC_lapic )
>          return pt->irq;
>  
>      isa_irq = pt->irq;
> -    gsi = hvm_isa_irq_to_gsi(isa_irq);
>  
>      if ( src == hvm_intsrc_pic )
>          return (v->domain->arch.hvm_domain.vpic[isa_irq >> 3].irq_base
>                  + (isa_irq & 7));
>  
>      ASSERT(src == hvm_intsrc_lapic);
> -    vioapic = gsi_vioapic(v->domain, gsi, &pin);
> -    if ( !vioapic )
> -    {
> -        dprintk(XENLOG_WARNING, "d%u: invalid GSI (%u) for platform timer\n",
> -                v->domain->domain_id, gsi);
> -        domain_crash(v->domain);
> -        return -1;
> -    }
> -
> -    return vioapic->redirtbl[pin].fields.vector;
> +    return pt->latched_vector;
>  }



> @@ -253,6 +263,7 @@ int pt_update_irq(struct vcpu *v)
>      struct periodic_time *pt, *temp, *earliest_pt;
>      uint64_t max_lag;
>      int irq, is_lapic;
> +    bool handle_by_pic = false;

What do you need this variable for? You can simply clear is_lapic
instead. And if you needed it, please follow the naming of the
other one (i.e. would want to be is_pic).

> @@ -297,7 +308,15 @@ int pt_update_irq(struct vcpu *v)
>      else
>      {
>          hvm_isa_irq_deassert(v->domain, irq);
> -        hvm_isa_irq_assert(v->domain, 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 )

Please don't pointlessly complicate expressions: There's no need
to take the address of v->domain->arch.hvm_domain here. (I
notice that it has been this way originally, but as you're touching
it, you should also clean this up.)

> +        {
> +            handle_by_pic = true;
> +            pt_set_latched_vector(earliest_pt, hvm_intsrc_lapic);

I don't follow: If the interrupt is to be handled by the PIC, why
do you need to latch the vector in that case at all, and even
more so _only_ in that case? When delivered via PIC, the IO-APIC
RTE won't have a valid vector field anyway (it's supposed to be
in ExtINT mode; see __vlapic_accept_pic_intr()). Am I overlooking
anything here?

> @@ -305,12 +324,7 @@ int pt_update_irq(struct vcpu *v)
>       * 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 handle_by_pic ? -1 : pt_irq_vector(earliest_pt, hvm_intsrc_lapic);

Please either use unlikely() or invert the condition and swap the
other two operands.

> --- a/xen/include/asm-x86/hvm/vpt.h
> +++ b/xen/include/asm-x86/hvm/vpt.h
> @@ -46,6 +46,12 @@ struct periodic_time {
>  #define PTSRC_lapic  2 /* LAPIC time source */
>      u8 source;                  /* PTSRC_ */
>      u8 irq;
> +    /*
> +     * Vector set in vIRR in one interrupt delivery. Using this value can
> +     * avoid reading the IOAPIC RTE again. Valid only when its source is
> +     * 'PTSRC_isa' and handled by vlapic.
> +     */
> +    int16_t latched_vector;

Why is this PTSRC_isa specific? In the description you say
"for example, if a periodic timer interrupt is from PIT".

And then, if there is a requirement for this to be handled by the
LAPIC, you don't need a 16-bit field: You can easily use 0 to
indicate the field is not valid, as vectors below 0x10 are all invalid
in that case.

Jan

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

Re: [PATCH for-4.9 v2] hvm/vpt: fix inconsistent views of vIOAPIC in vmx_intr_assist()

[Chao Gao]

On Fri, Apr 28, 2017 at 02:36:11AM -0600, Jan Beulich wrote:
>>>> On 27.04.17 at 04:47, <chao.gao@intel.com> wrote:
>> When injecting periodic timer interrupt in vmx_intr_assist(),
>> multi-read operations are done during one event delivery. For
>> example, if a periodic timer interrupt is from PIT, when set the
>> corresponding bit in vIRR, the corresponding RTE is accessed in
>> pt_update_irq(). When this function returns, it accesses the RTE
>> again to get the vector it sets in vIRR.  Between the two
>> accesses, the content of RTE may have been changed by another CPU
>> for no protection method in use. This case can incur the
>> assertion failure in vmx_intr_assist().
>
>Btw., irrespective of this I'm not convinced this patch really is
>4.9 material: While analyzing possible causes for the ASSERT()
>to trigger, you've found a way no real OS would ever use. And
>we'll need to do something about that ASSERT() anyway before
>4.9 goes out.

Fine to me.

>> @@ -253,6 +263,7 @@ int pt_update_irq(struct vcpu *v)
>>      struct periodic_time *pt, *temp, *earliest_pt;
>>      uint64_t max_lag;
>>      int irq, is_lapic;
>> +    bool handle_by_pic = false;
>
>What do you need this variable for? You can simply clear is_lapic
>instead. And if you needed it, please follow the naming of the
>other one (i.e. would want to be is_pic).

The interrupt source and interrupt controller is different here.
for interrupt source we have RTSRC_isa and RTSRC_lapic.
I think is_lapic is related to interrupt source.
and 'handle_by_pic' means that vpic relay this interrupt, which
is relevant to struct hvm_intsrc. Anyhow, it is not a big problem.

>
>> @@ -297,7 +308,15 @@ int pt_update_irq(struct vcpu *v)
>>      else
>>      {
>>          hvm_isa_irq_deassert(v->domain, irq);
>> -        hvm_isa_irq_assert(v->domain, 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 )
>
>Please don't pointlessly complicate expressions: There's no need
>to take the address of v->domain->arch.hvm_domain here. (I
>notice that it has been this way originally, but as you're touching
>it, you should also clean this up.)

will do.

>
>> +        {
>> +            handle_by_pic = true;
>> +            pt_set_latched_vector(earliest_pt, hvm_intsrc_lapic);
>
>I don't follow: If the interrupt is to be handled by the PIC, why
>do you need to latch the vector in that case at all, and even
>more so _only_ in that case? When delivered via PIC, the IO-APIC
>RTE won't have a valid vector field anyway (it's supposed to be
>in ExtINT mode; see __vlapic_accept_pic_intr()). Am I overlooking
>anything here?

Ok. I made a mistake here. Only need latching vector for the inverse case.

>
>> @@ -305,12 +324,7 @@ int pt_update_irq(struct vcpu *v)
>>       * 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 handle_by_pic ? -1 : pt_irq_vector(earliest_pt, hvm_intsrc_lapic);
>
>Please either use unlikely() or invert the condition and swap the
>other two operands.

Agree.

>
>> --- a/xen/include/asm-x86/hvm/vpt.h
>> +++ b/xen/include/asm-x86/hvm/vpt.h
>> @@ -46,6 +46,12 @@ struct periodic_time {
>>  #define PTSRC_lapic  2 /* LAPIC time source */
>>      u8 source;                  /* PTSRC_ */
>>      u8 irq;
>> +    /*
>> +     * Vector set in vIRR in one interrupt delivery. Using this value can
>> +     * avoid reading the IOAPIC RTE again. Valid only when its source is
>> +     * 'PTSRC_isa' and handled by vlapic.
>> +     */
>> +    int16_t latched_vector;
>
>Why is this PTSRC_isa specific? In the description you say
>"for example, if a periodic timer interrupt is from PIT".

PTSRC_lapic means the pt is lapic interrupt timer. We can read 'irq'
field directly from structure periodic_timer. For other cases, namely
PTSRC_isa, we should get vector from vioapic or vpic. For cases handle
by vpic, the vector is generated in hvm_vcpu_ack_pending_irq(). I can
latched here. So we have two restrictions. The first one is easy to
remove. The latter I think needs some changes to
hvm_vcpu_ack_pending_irq().

>
>And then, if there is a requirement for this to be handled by the
>LAPIC, you don't need a 16-bit field: You can easily use 0 to
>indicate the field is not valid, as vectors below 0x10 are all invalid
>in that case.

will do.

Thanks
Chao

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

Re: [PATCH for-4.9 v2] hvm/vpt: fix inconsistent views of vIOAPIC in vmx_intr_assist()

[Jan Beulich]

>>> On 28.04.17 at 11:09, <chao.gao@intel.com> wrote:
> On Fri, Apr 28, 2017 at 02:36:11AM -0600, Jan Beulich wrote:
>>>>> On 27.04.17 at 04:47, <chao.gao@intel.com> wrote:
>>> --- a/xen/include/asm-x86/hvm/vpt.h
>>> +++ b/xen/include/asm-x86/hvm/vpt.h
>>> @@ -46,6 +46,12 @@ struct periodic_time {
>>>  #define PTSRC_lapic  2 /* LAPIC time source */
>>>      u8 source;                  /* PTSRC_ */
>>>      u8 irq;
>>> +    /*
>>> +     * Vector set in vIRR in one interrupt delivery. Using this value can
>>> +     * avoid reading the IOAPIC RTE again. Valid only when its source is
>>> +     * 'PTSRC_isa' and handled by vlapic.
>>> +     */
>>> +    int16_t latched_vector;
>>
>>Why is this PTSRC_isa specific? In the description you say
>>"for example, if a periodic timer interrupt is from PIT".
> 
> PTSRC_lapic means the pt is lapic interrupt timer. We can read 'irq'
> field directly from structure periodic_timer. For other cases, namely
> PTSRC_isa, we should get vector from vioapic or vpic.

Oh, I'm sorry - PTSRC_isa covers all of HPET, PIT, and RTC.
This aspect is fine then as is.

Jan


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