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 13:46:25 +0100 Message-ID: <4F9AB141020000780008075D@nat28.tlf.novell.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> <1335527893.28015.191.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: <1335527893.28015.191.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: Ian Campbell Cc: "xen-devel@lists.xensource.com" , David Vrabel , Konrad Rzeszutek Wilk List-Id: xen-devel@lists.xenproject.org >>> On 27.04.12 at 13:58, Ian Campbell wrote: > 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 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