Re: [Xen-devel] [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR

2017-10-23 Thread Julien Grall 

Hi,

On 20/10/17 15:16, Jan Beulich wrote:

On 20.10.17 at 15:23,  wrote:

On 20/10/17 12:42, Jan Beulich wrote:

On 20.10.17 at 02:35,  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 


Reviewed-by: Jan Beulich 


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 

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR

2017-10-23 Thread Tian, Kevin 
> 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 

Reviewed-by: Kevin Tian 

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

Re: [Xen-devel] [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR

2017-10-20 Thread Jan Beulich 
>>> On 20.10.17 at 15:23,  wrote:
> On 20/10/17 12:42, Jan Beulich wrote:
> On 20.10.17 at 02:35,  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 
>> 
>> Reviewed-by: Jan Beulich 
> 
> 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

Re: [Xen-devel] [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR

2017-10-20 Thread Julien Grall 

Hi Jan,

On 20/10/17 12:42, Jan Beulich wrote:

On 20.10.17 at 02:35,  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 


Reviewed-by: Jan Beulich 


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

Re: [Xen-devel] [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR

2017-10-20 Thread Jan Beulich 
>>> On 20.10.17 at 02:35,  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 

Reviewed-by: Jan Beulich 



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

[Xen-devel] [PATCH v3 for 4.10] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR

2017-10-20 Thread 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 
---
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(>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(>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(>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);
+