Re: [PATCH] xen: x86_32: do not enable iterrupts when returning from exception in interrupt context

Re: [PATCH] xen: x86_32: do not enable iterrupts when returning from exception in interrupt context

[Jeremy Fitzhardinge]

On 08/31/2011 04:47 PM, Igor Mammedov wrote:
> If vmalloc page_fault happens inside of interrupt handler with interrupts
> disabled then on exit path from exception handler when there is no pending
> interrupts, the following code (arch/x86/xen/xen-asm_32.S:112):
>
> 	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
> 	sete XEN_vcpu_info_mask(%eax)
>
> will enable interrupts even if they has been previously disabled according to
> eflags from the bounce frame (arch/x86/xen/xen-asm_32.S:99)
>
> 	testb $X86_EFLAGS_IF>>8, 8+1+ESP_OFFSET(%esp)
> 	setz XEN_vcpu_info_mask(%eax)
>
> Solution is in setting XEN_vcpu_info_mask only when it should be set
> according to
> 	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
> but not clearing it if there isn't any pending events.

Wow, that's a great find.  I guess it shows how rarely we end up doing
an exception return with interrupts disabled, since that's been there
since, erm, 2.6.23?

But this could definitely explain some bugs where interrupts became
unexpectedly re-enabled.  Were you tracking one down when you found this?

> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  arch/x86/xen/xen-asm_32.S |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/xen/xen-asm_32.S b/arch/x86/xen/xen-asm_32.S
> index 22a2093..313dca7 100644
> --- a/arch/x86/xen/xen-asm_32.S
> +++ b/arch/x86/xen/xen-asm_32.S
> @@ -113,10 +113,14 @@ xen_iret_start_crit:
>  
>  	/*
>  	 * If there's something pending, mask events again so we can
> -	 * jump back into xen_hypervisor_callback
> +	 * jump back into xen_hypervisor_callback. Otherwise do not
> +	 * touch XEN_vcpu_info_mask.
>  	 */
> +	jne ignore_vcpu_info_mask
>  	sete XEN_vcpu_info_mask(%eax)
>  
> +ignore_vcpu_info_mask:
> +

This should be:

	jne 1f
	movb $1, XEN_vcpu_info_mask(%eax)

1:	popl %eax 


There's no point in using sete if we're already using a conditional jump
to avoid the write, and it's better to use local labels for little
control flow changes like this.

Thanks,
    J

[PATCH] xen: x86_32: do not enable iterrupts when returning from exception in interrupt context

[Igor Mammedov]

If vmalloc page_fault happens inside of interrupt handler with interrupts
disabled then on exit path from exception handler when there is no pending
interrupts, the following code (arch/x86/xen/xen-asm_32.S:112):

	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
	sete XEN_vcpu_info_mask(%eax)

will enable interrupts even if they has been previously disabled according to
eflags from the bounce frame (arch/x86/xen/xen-asm_32.S:99)

	testb $X86_EFLAGS_IF>>8, 8+1+ESP_OFFSET(%esp)
	setz XEN_vcpu_info_mask(%eax)

Solution is in setting XEN_vcpu_info_mask only when it should be set
according to
	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
but not clearing it if there isn't any pending events.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 arch/x86/xen/xen-asm_32.S |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/x86/xen/xen-asm_32.S b/arch/x86/xen/xen-asm_32.S
index 22a2093..313dca7 100644
--- a/arch/x86/xen/xen-asm_32.S
+++ b/arch/x86/xen/xen-asm_32.S
@@ -113,10 +113,14 @@ xen_iret_start_crit:
 
 	/*
 	 * If there's something pending, mask events again so we can
-	 * jump back into xen_hypervisor_callback
+	 * jump back into xen_hypervisor_callback. Otherwise do not
+	 * touch XEN_vcpu_info_mask.
 	 */
+	jne ignore_vcpu_info_mask
 	sete XEN_vcpu_info_mask(%eax)
 
+ignore_vcpu_info_mask:
+
 	popl %eax
 
 	/*
-- 
1.7.1

Re: [PATCH] xen: x86_32: do not enable iterrupts when returning from exception in interrupt context

[Igor Mammedov]

On 09/01/2011 12:37 AM, Jeremy Fitzhardinge wrote:
> On 08/31/2011 04:47 PM, Igor Mammedov wrote:
>> If vmalloc page_fault happens inside of interrupt handler with interrupts
>> disabled then on exit path from exception handler when there is no pending
>> interrupts, the following code (arch/x86/xen/xen-asm_32.S:112):
>>
>> 	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
>> 	sete XEN_vcpu_info_mask(%eax)
>>
>> will enable interrupts even if they has been previously disabled according to
>> eflags from the bounce frame (arch/x86/xen/xen-asm_32.S:99)
>>
>> 	testb $X86_EFLAGS_IF>>8, 8+1+ESP_OFFSET(%esp)
>> 	setz XEN_vcpu_info_mask(%eax)
>>
>> Solution is in setting XEN_vcpu_info_mask only when it should be set
>> according to
>> 	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
>> but not clearing it if there isn't any pending events.
>
> Wow, that's a great find.  I guess it shows how rarely we end up doing
> an exception return with interrupts disabled, since that's been there
> since, erm, 2.6.23?
>
> But this could definitely explain some bugs where interrupts became
> unexpectedly re-enabled.  Were you tracking one down when you found this?
>
>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>> ---
>>   arch/x86/xen/xen-asm_32.S |    6 +++++-
>>   1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/xen/xen-asm_32.S b/arch/x86/xen/xen-asm_32.S
>> index 22a2093..313dca7 100644
>> --- a/arch/x86/xen/xen-asm_32.S
>> +++ b/arch/x86/xen/xen-asm_32.S
>> @@ -113,10 +113,14 @@ xen_iret_start_crit:
>>
>>   	/*
>>   	 * If there's something pending, mask events again so we can
>> -	 * jump back into xen_hypervisor_callback
>> +	 * jump back into xen_hypervisor_callback. Otherwise do not
>> +	 * touch XEN_vcpu_info_mask.
>>   	 */
>> +	jne ignore_vcpu_info_mask
>>   	sete XEN_vcpu_info_mask(%eax)
>>
>> +ignore_vcpu_info_mask:
>> +
>
> This should be:
>
> 	jne 1f
> 	movb $1, XEN_vcpu_info_mask(%eax)
>
> 1:	popl %eax
>
>
> There's no point in using sete if we're already using a conditional jump
> to avoid the write, and it's better to use local labels for little
> control flow changes like this.
>
> Thanks,
>
       J
Jeremy,

Thanks for review, I'll re-post it soon.

[PATCH v2] xen: x86_32: do not enable iterrupts when returning from exception in interrupt context

[Igor Mammedov]

If vmalloc page_fault happens inside of interrupt handler with interrupts
disabled then on exit path from exception handler when there is no pending
interrupts, the following code (arch/x86/xen/xen-asm_32.S:112):

	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
	sete XEN_vcpu_info_mask(%eax)

will enable interrupts even if they has been previously disabled according to
eflags from the bounce frame (arch/x86/xen/xen-asm_32.S:99)

	testb $X86_EFLAGS_IF>>8, 8+1+ESP_OFFSET(%esp)
	setz XEN_vcpu_info_mask(%eax)

Solution is in setting XEN_vcpu_info_mask only when it should be set
according to
	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
but not clearing it if there isn't any pending events.

Reproducer for bug is attached to RHBZ 707552

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Jeremy Fitzhardinge <jeremy@goop.org>
---
 arch/x86/xen/xen-asm_32.S |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/xen/xen-asm_32.S b/arch/x86/xen/xen-asm_32.S
index 22a2093..b040b0e 100644
--- a/arch/x86/xen/xen-asm_32.S
+++ b/arch/x86/xen/xen-asm_32.S
@@ -113,11 +113,13 @@ xen_iret_start_crit:
 
 	/*
 	 * If there's something pending, mask events again so we can
-	 * jump back into xen_hypervisor_callback
+	 * jump back into xen_hypervisor_callback. Otherwise do not
+	 * touch XEN_vcpu_info_mask.
 	 */
-	sete XEN_vcpu_info_mask(%eax)
+	jne 1f
+	movb $1, XEN_vcpu_info_mask(%eax)
 
-	popl %eax
+1:	popl %eax
 
 	/*
 	 * From this point on the registers are restored and the stack
-- 
1.7.1

Re: [PATCH v2] xen: x86_32: do not enable iterrupts when returning from exception in interrupt context

[Konrad Rzeszutek Wilk]

On Thu, Sep 01, 2011 at 01:46:55PM +0200, Igor Mammedov wrote:
> If vmalloc page_fault happens inside of interrupt handler with interrupts
> disabled then on exit path from exception handler when there is no pending
> interrupts, the following code (arch/x86/xen/xen-asm_32.S:112):
> 
> 	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
> 	sete XEN_vcpu_info_mask(%eax)
> 
> will enable interrupts even if they has been previously disabled according to
> eflags from the bounce frame (arch/x86/xen/xen-asm_32.S:99)
> 
> 	testb $X86_EFLAGS_IF>>8, 8+1+ESP_OFFSET(%esp)
> 	setz XEN_vcpu_info_mask(%eax)
> 
> Solution is in setting XEN_vcpu_info_mask only when it should be set
> according to
> 	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
> but not clearing it if there isn't any pending events.
> 
> Reproducer for bug is attached to RHBZ 707552
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Jeremy Fitzhardinge <jeremy@goop.org>

Stuck it in the queue for 3.1 and stable. Thanks for finding this one!

Re: [PATCH v2] xen: x86_32: do not enable iterrupts when returning from exception in interrupt context

[Jeremy Fitzhardinge]

On 09/01/2011 04:46 AM, Igor Mammedov wrote:
> If vmalloc page_fault happens inside of interrupt handler with interrupts
> disabled then on exit path from exception handler when there is no pending
> interrupts, the following code (arch/x86/xen/xen-asm_32.S:112):
>
> 	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
> 	sete XEN_vcpu_info_mask(%eax)
>
> will enable interrupts even if they has been previously disabled according to
> eflags from the bounce frame (arch/x86/xen/xen-asm_32.S:99)
>
> 	testb $X86_EFLAGS_IF>>8, 8+1+ESP_OFFSET(%esp)
> 	setz XEN_vcpu_info_mask(%eax)
>
> Solution is in setting XEN_vcpu_info_mask only when it should be set
> according to
> 	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
> but not clearing it if there isn't any pending events.
>
> Reproducer for bug is attached to RHBZ 707552
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Jeremy Fitzhardinge <jeremy@goop.org>

One nit, this should be acked-by or reviewed-by, not signed-off-by,
since the patch isn't passing through my hands.

    J

Re: [PATCH v2] xen: x86_32: do not enable iterrupts when returning from exception in interrupt context

[Igor Mammedov]

On 09/01/2011 06:46 PM, Jeremy Fitzhardinge wrote:
> On 09/01/2011 04:46 AM, Igor Mammedov wrote:
>> If vmalloc page_fault happens inside of interrupt handler with interrupts
>> disabled then on exit path from exception handler when there is no pending
>> interrupts, the following code (arch/x86/xen/xen-asm_32.S:112):
>>
>> 	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
>> 	sete XEN_vcpu_info_mask(%eax)
>>
>> will enable interrupts even if they has been previously disabled according to
>> eflags from the bounce frame (arch/x86/xen/xen-asm_32.S:99)
>>
>> 	testb $X86_EFLAGS_IF>>8, 8+1+ESP_OFFSET(%esp)
>> 	setz XEN_vcpu_info_mask(%eax)
>>
>> Solution is in setting XEN_vcpu_info_mask only when it should be set
>> according to
>> 	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
>> but not clearing it if there isn't any pending events.
>>
>> Reproducer for bug is attached to RHBZ 707552
>>
>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>> Signed-off-by: Jeremy Fitzhardinge<jeremy@goop.org>
>
> One nit, this should be acked-by or reviewed-by, not signed-off-by,
> since the patch isn't passing through my hands.
>
>      J

I'm new to this stuff, would you like me to re-post it?
Next time will do it right.

-------
Thanks,
    Igor

Re: [Xen-devel] Re: [PATCH v2] xen: x86_32: do not enable iterrupts when returning from exception in interrupt context

[Konrad Rzeszutek Wilk]

On Fri, Sep 02, 2011 at 10:18:23AM +0200, Igor Mammedov wrote:
> On 09/01/2011 06:46 PM, Jeremy Fitzhardinge wrote:
> >On 09/01/2011 04:46 AM, Igor Mammedov wrote:
> >>If vmalloc page_fault happens inside of interrupt handler with interrupts
> >>disabled then on exit path from exception handler when there is no pending
> >>interrupts, the following code (arch/x86/xen/xen-asm_32.S:112):
> >>
> >>	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
> >>	sete XEN_vcpu_info_mask(%eax)
> >>
> >>will enable interrupts even if they has been previously disabled according to
> >>eflags from the bounce frame (arch/x86/xen/xen-asm_32.S:99)
> >>
> >>	testb $X86_EFLAGS_IF>>8, 8+1+ESP_OFFSET(%esp)
> >>	setz XEN_vcpu_info_mask(%eax)
> >>
> >>Solution is in setting XEN_vcpu_info_mask only when it should be set
> >>according to
> >>	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
> >>but not clearing it if there isn't any pending events.
> >>
> >>Reproducer for bug is attached to RHBZ 707552
> >>
> >>Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> >>Signed-off-by: Jeremy Fitzhardinge<jeremy@goop.org>
> >
> >One nit, this should be acked-by or reviewed-by, not signed-off-by,
> >since the patch isn't passing through my hands.
> >
> >     J
> 
> I'm new to this stuff, would you like me to re-post it?

That is OK.  I fixed it up in the git commit. Thanks for finding this one!

Re: [Xen-devel] Re: [PATCH v2] xen: x86_32: do not enable iterrupts when returning from exception in interrupt context

[Igor Mammedov]

On 09/02/2011 03:40 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Sep 02, 2011 at 10:18:23AM +0200, Igor Mammedov wrote:
>> On 09/01/2011 06:46 PM, Jeremy Fitzhardinge wrote:
>>> On 09/01/2011 04:46 AM, Igor Mammedov wrote:
>>>> If vmalloc page_fault happens inside of interrupt handler with interrupts
>>>> disabled then on exit path from exception handler when there is no pending
>>>> interrupts, the following code (arch/x86/xen/xen-asm_32.S:112):
>>>>
>>>> 	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
>>>> 	sete XEN_vcpu_info_mask(%eax)
>>>>
>>>> will enable interrupts even if they has been previously disabled according to
>>>> eflags from the bounce frame (arch/x86/xen/xen-asm_32.S:99)
>>>>
>>>> 	testb $X86_EFLAGS_IF>>8, 8+1+ESP_OFFSET(%esp)
>>>> 	setz XEN_vcpu_info_mask(%eax)
>>>>
>>>> Solution is in setting XEN_vcpu_info_mask only when it should be set
>>>> according to
>>>> 	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
>>>> but not clearing it if there isn't any pending events.
>>>>
>>>> Reproducer for bug is attached to RHBZ 707552
>>>>
>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>>> Signed-off-by: Jeremy Fitzhardinge<jeremy@goop.org>
>>>
>>> One nit, this should be acked-by or reviewed-by, not signed-off-by,
>>> since the patch isn't passing through my hands.
>>>
>>>      J
>>
>> I'm new to this stuff, would you like me to re-post it?
>
> That is OK.  I fixed it up in the git commit. Thanks for finding this one!

You're welcome!
I've learned a lot while debugging it. In particular, how to use kvm and qemu's
gdbstub to debug xen and guest using gdb.

-- 
Thanks,
  Igor

Re: [Xen-devel] Re: [PATCH v2] xen: x86_32: do not enable iterrupts when returning from exception in interrupt context

[Konrad Rzeszutek Wilk]

On Fri, Sep 02, 2011 at 04:01:35PM +0200, Igor Mammedov wrote:
> On 09/02/2011 03:40 PM, Konrad Rzeszutek Wilk wrote:
> >On Fri, Sep 02, 2011 at 10:18:23AM +0200, Igor Mammedov wrote:
> >>On 09/01/2011 06:46 PM, Jeremy Fitzhardinge wrote:
> >>>On 09/01/2011 04:46 AM, Igor Mammedov wrote:
> >>>>If vmalloc page_fault happens inside of interrupt handler with interrupts
> >>>>disabled then on exit path from exception handler when there is no pending
> >>>>interrupts, the following code (arch/x86/xen/xen-asm_32.S:112):
> >>>>
> >>>>	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
> >>>>	sete XEN_vcpu_info_mask(%eax)
> >>>>
> >>>>will enable interrupts even if they has been previously disabled according to
> >>>>eflags from the bounce frame (arch/x86/xen/xen-asm_32.S:99)
> >>>>
> >>>>	testb $X86_EFLAGS_IF>>8, 8+1+ESP_OFFSET(%esp)
> >>>>	setz XEN_vcpu_info_mask(%eax)
> >>>>
> >>>>Solution is in setting XEN_vcpu_info_mask only when it should be set
> >>>>according to
> >>>>	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
> >>>>but not clearing it if there isn't any pending events.
> >>>>
> >>>>Reproducer for bug is attached to RHBZ 707552
> >>>>
> >>>>Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> >>>>Signed-off-by: Jeremy Fitzhardinge<jeremy@goop.org>
> >>>
> >>>One nit, this should be acked-by or reviewed-by, not signed-off-by,
> >>>since the patch isn't passing through my hands.
> >>>
> >>>     J
> >>
> >>I'm new to this stuff, would you like me to re-post it?
> >
> >That is OK.  I fixed it up in the git commit. Thanks for finding this one!
> 
> You're welcome!
> I've learned a lot while debugging it. In particular, how to use kvm and qemu's
> gdbstub to debug xen and guest using gdb.

Oh, would you be interested in writting a blog article on xen.org by any chance?
That sounds pretty nifty!

Re: [Xen-devel] Re: [PATCH v2] xen: x86_32: do not enable iterrupts when returning from exception in interrupt context

[Igor Mammedov]

On 09/02/2011 04:47 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Sep 02, 2011 at 04:01:35PM +0200, Igor Mammedov wrote:
>> On 09/02/2011 03:40 PM, Konrad Rzeszutek Wilk wrote:
>>> On Fri, Sep 02, 2011 at 10:18:23AM +0200, Igor Mammedov wrote:
>>>> On 09/01/2011 06:46 PM, Jeremy Fitzhardinge wrote:
>>>>> On 09/01/2011 04:46 AM, Igor Mammedov wrote:
>>>>>> If vmalloc page_fault happens inside of interrupt handler with interrupts
>>>>>> disabled then on exit path from exception handler when there is no pending
>>>>>> interrupts, the following code (arch/x86/xen/xen-asm_32.S:112):
>>>>>>
>>>>>> 	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
>>>>>> 	sete XEN_vcpu_info_mask(%eax)
>>>>>>
>>>>>> will enable interrupts even if they has been previously disabled according to
>>>>>> eflags from the bounce frame (arch/x86/xen/xen-asm_32.S:99)
>>>>>>
>>>>>> 	testb $X86_EFLAGS_IF>>8, 8+1+ESP_OFFSET(%esp)
>>>>>> 	setz XEN_vcpu_info_mask(%eax)
>>>>>>
>>>>>> Solution is in setting XEN_vcpu_info_mask only when it should be set
>>>>>> according to
>>>>>> 	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
>>>>>> but not clearing it if there isn't any pending events.
>>>>>>
>>>>>> Reproducer for bug is attached to RHBZ 707552
>>>>>>
>>>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>>>>> Signed-off-by: Jeremy Fitzhardinge<jeremy@goop.org>
>>>>>
>>>>> One nit, this should be acked-by or reviewed-by, not signed-off-by,
>>>>> since the patch isn't passing through my hands.
>>>>>
>>>>>      J
>>>>
>>>> I'm new to this stuff, would you like me to re-post it?
>>>
>>> That is OK.  I fixed it up in the git commit. Thanks for finding this one!
>>
>> You're welcome!
>> I've learned a lot while debugging it. In particular, how to use kvm and qemu's
>> gdbstub to debug xen and guest using gdb.
>
> Oh, would you be interested in writting a blog article on xen.org by any chance?
> That sounds pretty nifty!

Sure, that is on my todo list. I'll just gather separated bits of information
together, on how to prepare host for debugging. And description of problems and
limitations of this approach, that I've stumbled on.


-- 
Thanks,
  Igor