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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.04.12 at 13:58, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Fri, 2012-04-27 at 12:42 +0100, Jan Beulich wrote:
>> >>> 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".
> 
> Hrm, has it ever not been 1 in practice?

I don't think so.

> i.e. could we legitimately tighten the spec?

I wouldn't want to recommend this. In particular, we can't all of the
sudden keep guests from storing other non-zero values in here.

While I'm not in favor of this either, what we could do is specify that
Xen will only ever write 0 or 1 in here, while other non-zero values
are okay to be used by guests.

>> 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.
> 
> Code size is also a concern here since this sequence of instructions is
> used for inline patching (not sure what the size limit actually is
> though).

Oh yes, didn't think of that aspect.

Jan


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