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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.04.12 at 10:47, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Thu, 2012-04-26 at 19:44 +0100, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@xxxxxxxxxx>
>> 
>> 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? Which makes me wonder how it used to
> work at all!
> 
>> 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).

It is _too much_ trickery, as it implies that the pending field, when set,
will always be 1. This is not sanctioned by the specification (quoting
the hypervisor's xen/include/public/xen.h):

     * 'evtchn_upcall_pending' is written non-zero by Xen to indicate
     * a pending notification for a particular VCPU. It is then cleared 
     * by the guest OS /before/ checking for pending work, thus avoiding

Note that it says "non-zero", not "1".

But yes, this isn't the fault of the patch here, so this is also not an
objection to this patch.

And yes, it can still be done with a single compare afaict, just not
directly on the memory operand.

Jan

> Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> 
> 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.
> 
> Ian.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx 
> http://lists.xen.org/xen-devel 




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