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

[Jan Beulich]

>>> On 16.10.17 at 15:46,  wrote:
> On Mon, Oct 16, 2017 at 08:26:09AM -0600, Jan Beulich wrote:
> On 16.10.17 at 15:13,  wrote:
>>> On Mon, Oct 16, 2017 at 07:15:16AM -0600, Jan Beulich wrote:
>>> On 13.10.17 at 07:10,  wrote:
> --- 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 = 0;

Why zero (which is valid aiui) instead of e.g. -1?
>>> 
>>> vector also serves as the return value. I want to return 0 if no
>>> callback is set.  And the callback, get_vector, can override the return
>>> value. Do you think it is reasonable?
>>
>>Why "also" - being the return value is the only purpose of "vector".
>>And as said - zero is a valid vector, and I wouldn't like to see the
>>function return a valid but meaningless vector number.
> 
> But if no callback is set, would it be a little weird to return -1 which
> always means failure?

To me, -1 doesn't mean "failure" here, but "no valid vector".

Jan


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

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

[Chao Gao]

On Mon, Oct 16, 2017 at 08:26:09AM -0600, Jan Beulich wrote:
 On 16.10.17 at 15:13,  wrote:
>> On Mon, Oct 16, 2017 at 07:15:16AM -0600, Jan Beulich wrote:
>> On 13.10.17 at 07:10,  wrote:
 --- 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 = 0;
>>>
>>>Why zero (which is valid aiui) instead of e.g. -1?
>> 
>> vector also serves as the return value. I want to return 0 if no
>> callback is set.  And the callback, get_vector, can override the return
>> value. Do you think it is reasonable?
>
>Why "also" - being the return value is the only purpose of "vector".
>And as said - zero is a valid vector, and I wouldn't like to see the
>function return a valid but meaningless vector number.

But if no callback is set, would it be a little weird to return -1 which
always means failure? Considering no caller would be confused by the
return value (since except the caller introduced by this patch, no one
would check the return value), I don't insist on this.

>
 --- a/xen/include/asm-x86/hvm/vmx/vmx.h
 +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
 @@ -109,6 +109,11 @@ static inline int pi_test_and_set_pir(int vector, 
 struct pi_desc *pi_desc)
  return test_and_set_bit(vector, pi_desc->pir);
  }
  
 +static inline int pi_test_pir(int vector, const struct pi_desc *pi_desc)
>>>
>>>This should not be a signed quantity - uint8_t or unsigned int
>>>please.
>> 
>> Yes.
>
>I.e. meaning you're fine with either variant, leaving it up to me
>which one to use?

Yes, both of them are ok to me.

Thanks
Chao

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

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

[Jan Beulich]

>>> On 16.10.17 at 15:13,  wrote:
> On Mon, Oct 16, 2017 at 07:15:16AM -0600, Jan Beulich wrote:
> On 13.10.17 at 07:10,  wrote:
>>> --- 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 = 0;
>>
>>Why zero (which is valid aiui) instead of e.g. -1?
> 
> vector also serves as the return value. I want to return 0 if no
> callback is set.  And the callback, get_vector, can override the return
> value. Do you think it is reasonable?

Why "also" - being the return value is the only purpose of "vector".
And as said - zero is a valid vector, and I wouldn't like to see the
function return a valid but meaningless vector number.

>>> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
>>> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
>>> @@ -109,6 +109,11 @@ static inline int pi_test_and_set_pir(int vector, 
>>> struct pi_desc *pi_desc)
>>>  return test_and_set_bit(vector, pi_desc->pir);
>>>  }
>>>  
>>> +static inline int pi_test_pir(int vector, const struct pi_desc *pi_desc)
>>
>>This should not be a signed quantity - uint8_t or unsigned int
>>please.
> 
> Yes.

I.e. meaning you're fine with either variant, leaving it up to me
which one to use?

Jan


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

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

[Chao Gao]

On Mon, Oct 16, 2017 at 07:15:16AM -0600, Jan Beulich wrote:
 On 13.10.17 at 07:10,  wrote:
>> --- 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 = 0;
>
>Why zero (which is valid aiui) instead of e.g. -1?

vector also serves as the return value. I want to return 0 if no
callback is set.  And the callback, get_vector, can override the return
value. Do you think it is reasonable?

>
>> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
>> @@ -109,6 +109,11 @@ static inline int pi_test_and_set_pir(int vector, 
>> struct pi_desc *pi_desc)
>>  return test_and_set_bit(vector, pi_desc->pir);
>>  }
>>  
>> +static inline int pi_test_pir(int vector, const struct pi_desc *pi_desc)
>
>This should not be a signed quantity - uint8_t or unsigned int
>please.

Yes.

>
>I wouldn't mind making suitable adjustments while committing (and
>then adding my R-b), but that requires your feedback which way
>things should be.

Sure. I will appreciate it.
>
>Also please don't forget to Cc the release manager, unless you
>intend this fix only for after 4.10.

Hi, 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 at first and then observe whether
the bug disappears or not. Since Jan said he could do some adjustments to the
patch when committing, Could you give acked-by on this patch?

Thanks
Chao.

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

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

[Jan Beulich]

>>> On 13.10.17 at 07:10,  wrote:
> --- 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 = 0;

Why zero (which is valid aiui) instead of e.g. -1?

> @@ -292,25 +292,38 @@ int pt_update_irq(struct vcpu *v)
>  
>  spin_unlock(>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);

I like that you got away without introducing a new wrapper function
at all.

> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -109,6 +109,11 @@ static inline int pi_test_and_set_pir(int vector, struct 
> pi_desc *pi_desc)
>  return test_and_set_bit(vector, pi_desc->pir);
>  }
>  
> +static inline int pi_test_pir(int vector, const struct pi_desc *pi_desc)

This should not be a signed quantity - uint8_t or unsigned int
please.

I wouldn't mind making suitable adjustments while committing (and
then adding my R-b), but that requires your feedback which way
things should be.

Also please don't forget to Cc the release manager, unless you
intend this fix only for after 4.10.

Jan


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

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

[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.

Signed-off-by: Chao Gao 
---
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.

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 |  5 +
 11 files changed, 74 insertions(+), 20 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..d79a367 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 = 0;
 
 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);
+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(>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)