Re: [PATCH] xen: correctly check for pending events when restoring irq flags

[David Vrabel <david.vrabel@citrix.com>]

On 27/04/12 09:47, Ian Campbell wrote:
> On Thu, 2012-04-26 at 19:44 +0100, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> In xen_restore_fl_direct(), xen_force_evtchn_callback() was being
>> called even if no events were pending.
> 
> In actual fact it seems that the callback was actually being called if
> and only if no events were pending?

Or if events are masked, but this wasn't as bad as it sounds as Xen
would not actually do the upcall.

> Which makes me wonder how it used to work at all!

Xen would have to raise an event during a local_save_flags() /
local_restore_flags() /and/ after missing the call to
xen_force_evtchn_callback(), the guest would have to do no more
hypercalls at all.  This is possible I guess but seems really unlikely
to me.

>> diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
>> index 79d7362..3e45aa0 100644
>> --- a/arch/x86/xen/xen-asm.S
>> +++ b/arch/x86/xen/xen-asm.S
>> @@ -96,7 +96,7 @@ ENTRY(xen_restore_fl_direct)
>>  
>>  	/* check for unmasked and pending */
>>  	cmpw $0x0001, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
>> -	jz 1f
>> +	jnz 1f
>>  2:	call check_events
>>  1:
> 
> Took me a while, this is a bit tricksy (and it may well be too early for
> me to be decoding it) since the check here is trying to check both
> pending and masked in a single cmpw, but I think this is correct. It
> will call check_events now only when the combined mask+pending word is
> 0x0001 (aka unmasked, pending).
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Does xen_irq_enable_direct have the same sort of issue? No, in that case
> we are doing a straight forward test of pending without involving masked
> (since it has just unmasked) and so jz is correct.

Thanks for the review.  This was a tricky one to pin down.

David