Re: [PATCH] evtchn: simplify sending of notifications

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/01/15 11:42, Jan Beulich wrote:
>>>> On 12.01.15 at 12:33, <andrew.cooper3@citrix.com> wrote:
>> On 12/01/15 08:57, Jan Beulich wrote:
>>> --- a/xen/include/xen/event.h
>>> +++ b/xen/include/xen/event.h
>>> @@ -152,10 +152,11 @@ static inline void evtchn_port_init(stru
>>>          d->evtchn_port_ops->init(d, evtchn);
>>>  }
>>>  
>>> -static inline void evtchn_port_set_pending(struct vcpu *v,
>>> +static inline void evtchn_port_set_pending(struct domain *d,
>>> +                                           unsigned int vcpu_id,
>>>                                             struct evtchn *evtchn)
>> I would rename this to the, now vacant, evtchn_set_pending().  It takes
>> an evtchn* not a port.  (Its sole caller was evtchn_set_pending(), so
>> the patch won't grow)
> No (and I had actually considered it) - that would get its name out of
> sync with all its sibling wrappers.

Ah yes - consistency is more important than correctness here.

>
>> Furthermore, all callers except send_guest_vcpu_virq() currently use
>> evtchn->notify_vcpu_id to get a struct vcpu* to pass.  I think you can
>> drop the vcpu_id parameter and use evtchn->notify_vcpu_id directly,
>> which reduces the likelyhood of a bug where the evtchn is bound to one
>> vcpu but a caller gets the wrong id and raises the event channel on the
>> wrong vcpu.
> Generally a nice idea, but it doesn't immediately/obviously fit with
> the use in send_guest_vcpu_virq().

It is awkward that some of the virqs will get delivered on an arbitrary
vcpu, especially as the virq API requires the binding domain to choose a
destination vcpu.  Either way, this is not something to be addressed in
a cleanup patch.

Therefore the original patch is Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

~Andrew