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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.03.17 at 13:39, <julien.grall@xxxxxxx> wrote:
> My knowledge is limited for this code. So I've just CCed "The REST" 
> maintainers. Please do CC them in the future.

Indeed.

> 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...

>> @@ -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.

>> @@ -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.

Jan


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