Re: [PATCH] xen: correctly check for pending events when restoring irq flags

["Jan Beulich" <JBeulich@suse.com>]

>>> On 27.04.12 at 15:09, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2012-04-27 at 13:46 +0100, Jan Beulich wrote:
>> >>> On 27.04.12 at 13:58, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Fri, 2012-04-27 at 12:42 +0100, Jan Beulich wrote:
>> >> >>> On 27.04.12 at 10:47, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > 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.
> 
> Could they be doing this? Whatever they put there is going to get
> clobbered by Xen whenever it injects an event, isn't it? Or do you mean
> that a guest can force an upcall by writing non-zero itself? Do guests
> actually do that? (why?)

Yes, there is such a case even in Linux - see unmask_evtchn(). And
tightening an existing spec in ways that affect things we don't
control seems undesirable (and unnecessary 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.
> 
> Does Xen ever clear an upcall, isn't that always the guest? Xen only
> ever writes 1, at least as far as I can see...

Oh, yes, you're right of course.

> What do you think of the following?

Reads okay.

Jan

> --- a/xen/include/public/xen.h	Thu Apr 26 10:03:08 2012 +0100
> +++ b/xen/include/public/xen.h	Fri Apr 27 14:09:00 2012 +0100
> @@ -559,16 +559,18 @@ typedef struct vcpu_time_info vcpu_time_
>  
>  struct vcpu_info {
>      /*
> -     * '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
> -     * a set-and-check race. Note that the mask is only accessed by Xen
> -     * on the CPU that is currently hosting the VCPU. This means that the
> -     * pending and mask flags can be updated by the guest without special
> -     * synchronisation (i.e., no need for the x86 LOCK prefix).
> -     * This may seem suboptimal because if the pending flag is set by
> -     * a different CPU then an IPI may be scheduled even when the mask
> -     * is set. However, note:
> +     * 'evtchn_upcall_pending' is written to '1' by Xen to indicate a
> +     * pending notification for a particular VCPU. Xen will never
> +     * write any other value but it is legitimate for a guest to store
> +     * any other non-zero value here to trigger an upcall. It is then
> +     * cleared by the guest OS /before/ checking for pending work,
> +     * thus avoiding a set-and-check race. Note that the mask is only
> +     * accessed by Xen on the CPU that is currently hosting the
> +     * VCPU. This means that the pending and mask flags can be updated
> +     * by the guest without special synchronisation (i.e., no need for
> +     * the x86 LOCK prefix).  This may seem suboptimal because if the
> +     * pending flag is set by a different CPU then an IPI may be
> +     * scheduled even when the mask is set. However, note:
>       *  1. The task of 'interrupt holdoff' is covered by the per-event-
>       *     channel mask bits. A 'noisy' event that is continually being
>       *     triggered can be masked at source at this very precise