From: David Vrabel <david.vrabel@citrix.com>
To: Ian Campbell <Ian.Campbell@citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>,
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Subject: Re: [PATCH] xen: correctly check for pending events when restoring irq flags
Date: Fri, 27 Apr 2012 12:41:17 +0100 [thread overview]
Message-ID: <4F9A85DD.9070308@citrix.com> (raw)
In-Reply-To: <1335516436.28015.169.camel@zakaz.uk.xensource.com>
On 27/04/12 09:47, Ian Campbell wrote:
> On Thu, 2012-04-26 at 19:44 +0100, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> 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 <ian.campbell@citrix.com>
>
> 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
next prev parent reply other threads:[~2012-04-27 11:41 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-26 18:44 [PATCH] xen: correctly check for pending events when restoring irq flags David Vrabel
2012-04-26 20:08 ` Konrad Rzeszutek Wilk
2012-04-27 8:47 ` Ian Campbell
2012-04-27 11:41 ` David Vrabel [this message]
2012-04-27 11:42 ` Jan Beulich
2012-04-27 11:58 ` Ian Campbell
2012-04-27 12:46 ` Jan Beulich
2012-04-27 13:09 ` Ian Campbell
2012-04-27 13:51 ` Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F9A85DD.9070308@citrix.com \
--to=david.vrabel@citrix.com \
--cc=Ian.Campbell@citrix.com \
--cc=konrad.wilk@oracle.com \
--cc=xen-devel@lists.xensource.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.