From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH] xen: correctly check for pending events when restoring irq flags Date: Fri, 27 Apr 2012 12:58:13 +0100 Message-ID: <1335527893.28015.191.camel@zakaz.uk.xensource.com> References: <1335465846-22229-1-git-send-email-david.vrabel@citrix.com> <1335516436.28015.169.camel@zakaz.uk.xensource.com> <4F9AA23102000078000806DA@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4F9AA23102000078000806DA@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: "xen-devel@lists.xensource.com" , David Vrabel , Konrad Rzeszutek Wilk List-Id: xen-devel@lists.xenproject.org On Fri, 2012-04-27 at 12:42 +0100, Jan Beulich wrote: > >>> On 27.04.12 at 10:47, Ian Campbell wrote: > > On Thu, 2012-04-26 at 19:44 +0100, David Vrabel wrote: > >> From: David Vrabel > >> > >> 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.e. could we legitimately tighten the spec? > 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). Ian.