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