From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH] xen: correctly check for pending events when restoring irq flags Date: Fri, 27 Apr 2012 12:41:17 +0100 Message-ID: <4F9A85DD.9070308@citrix.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> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "xen-devel@lists.xensource.com" , Konrad Rzeszutek Wilk List-Id: xen-devel@lists.xenproject.org On 27/04/12 09: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? 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 > > 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