Re: [PATCH 03/11] xen/arm: vpl011: Refactor evtchn_send in Xen to allow sending events from a xen bound channel

[Bhupinder Thakur <bhupinder.thakur@linaro.org>]

Hi Jan,

On 6 March 2017 at 13:45, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 05.03.17 at 13:39, <julien.grall@arm.com> wrote:
>> My knowledge is limited for this code. So I've just CCed "The REST"
>> maintainers. Please do CC them in the future.
>
> Indeed.
>
I will take care of this in the future.

>> On 02/21/2017 11:26 AM, Bhupinder Thakur wrote:
>>> Breakup evtchn_send() to allow sending events for a Xen bound channel. Currently,
>>> there is a check in evtchn_send() i.e. is_consumer_xen() that if the event channel
>>> is bound to a xen consumer then event generation is not allowed for that channel.
>>> This check is to disallow a guest from raising an event via this channel. However,
>>> it should allow Xen to send a event via this channel as it is required for sending
>>> vpl011 event to the dom0.
>>>
>>> This change introduces a new function raw_evtchn_send() which sends the event
>>> without this check. The current evtchn_send() calls this function after doing the
>>> xen consumer check. Xen uses the raw_evtchm_send() version to send the event thus
>>> bypassing the check.
>
> Why would Xen want to send an event it is itself the consumer of?
> Surely there are better ways to communicate state internally? The
> more that you say you want the event sent to Dom0...
>
As a consumer, Xen receives event from dom0. It also needs to send
events to dom0 to indicate that there is data in the ring buffer for
dom0 to read. I am using a xen bound event channel for
sending/receiving events to/from dom0. I added a new function
raw_evtchn_send() to allow Xen to send events to dom0 without doing
the is_xen_consumer check. Note that this check is still there in
evtchn_send() to disallow guests to raise events on the xen bound
channel.

>>> @@ -650,25 +651,21 @@ static long evtchn_close(struct domain *d1, int port1, bool_t guest)
>>>      return rc;
>>>  }
>>>
>>> -int evtchn_send(struct domain *ld, unsigned int lport)
>>> +int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data)
>>>  {
>>>      struct evtchn *lchn, *rchn;
>>>      struct domain *rd;
>>> -    int            rport, ret = 0;
>>> +    int rport, ret=0;
>
> There's only whitespace change here, and just the break existing
> formatting. Please avoid stray changes like this.
>
I will fix this in the next version.

>>> @@ -696,6 +693,32 @@ int evtchn_send(struct domain *ld, unsigned int lport)
>>>      }
>>>
>>>  out:
>>> +    if ( !data )
>>> +        spin_unlock(&lchn->lock);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +int evtchn_send(struct domain *ld, unsigned int lport)
>>> +{
>>> +    struct evtchn *lchn;
>>> +    int ret;
>>> +
>>> +    if ( !port_is_valid(ld, lport) )
>>> +        return -EINVAL;
>>> +
>>> +    lchn = evtchn_from_port(ld, lport);
>>> +
>>> +    spin_lock(&lchn->lock);
>>> +
>>> +    if ( unlikely(consumer_is_xen(lchn)) )
>>> +    {
>>> +        printk("evtchn_send failed to send via xen event channel\n");
>>> +        return -EINVAL;
>
> As a general remark: Splitting out functionality into a new function
> should _never_ be accompanied by silent behavioral changes for
> pre-existing code paths. In the case here there was no printk()
> before, and I see no justification for one to be added.
>
Ok. I will remove the printk.

> Jan
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel