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

Message ID 1487676368-22356-4-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series
  • pl011 emulation support in Xen
Related show

Commit Message

Bhupinder Thakur Feb. 21, 2017, 11:26 a.m.
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.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
 xen/common/event_channel.c | 49 ++++++++++++++++++++++++++++++++++------------
 xen/include/xen/event.h    |  6 ++++++
 2 files changed, 42 insertions(+), 13 deletions(-)

Comments

Konrad Rzeszutek Wilk March 3, 2017, 9:13 p.m. | #1
On Tue, Feb 21, 2017 at 04:56:00PM +0530, 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,

Did any code archeology help in idenfitying why this was done this way?

You should explain why it was done - what was the use case , and why
your change will not change this semantic.
> 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

.. and without taking a lock? Why?


> xen consumer check. Xen uses the raw_evtchm_send() version to send the event thus
> bypassing the check.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>  xen/common/event_channel.c | 49 ++++++++++++++++++++++++++++++++++------------
>  xen/include/xen/event.h    |  6 ++++++
>  2 files changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index 638dc5e..4b039f3 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -27,6 +27,7 @@
>  #include <xen/keyhandler.h>
>  #include <xen/event_fifo.h>
>  #include <asm/current.h>
> +#include <xen/domain_page.h>
>  
>  #include <public/xen.h>
>  #include <public/event_channel.h>
> @@ -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;

Please that code as is.

>  
> -    if ( !port_is_valid(ld, lport) )
> -        return -EINVAL;
> -
> -    lchn = evtchn_from_port(ld, lport);
> -
> -    spin_lock(&lchn->lock);
> -
> -    /* Guest cannot send via a Xen-attached event channel. */
> -    if ( unlikely(consumer_is_xen(lchn)) )
> +    if ( !data )
>      {
> -        ret = -EINVAL;
> -        goto out;
> +        if ( !port_is_valid(ld, lport) )
> +            return -EINVAL;
> +        lchn = evtchn_from_port(ld, lport);
> +        spin_lock(&lchn->lock);

That won't do. Please keep the format of the old code as much
as possible (hint: Those newlines).

>      }
> +    else
> +        lchn = (struct evtchn *)data; 
>  
>      ret = xsm_evtchn_send(XSM_HOOK, ld, lchn);
>      if ( ret )
> @@ -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");

No. Please do not.
> +        return -EINVAL;
> +    }
> +
> +    ret = raw_evtchn_send(ld, lport, lchn);
> +
>      spin_unlock(&lchn->lock);
>  
>      return ret;
> diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
> index 5008c80..9bd17db 100644
> --- a/xen/include/xen/event.h
> +++ b/xen/include/xen/event.h
> @@ -45,6 +45,12 @@ void send_guest_pirq(struct domain *, const struct pirq *);
>  /* Send a notification from a given domain's event-channel port. */
>  int evtchn_send(struct domain *d, unsigned int lport);
>  
> +/* 
> + * This function is same as evntchn_send() except it does not do xen consumer check
> + * to allow the events to be sent from xen bound channels.

And it also looks to ignore the locking? Could you explain why?

> + */
> +int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data);
> +
>  /* Bind a local event-channel port to the specified VCPU. */
>  long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id);
>  
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
Julien Grall March 5, 2017, 12:39 p.m. | #2
Hi Bhupinder,

My knowledge is limited for this code. So I've just CCed "The REST" 
maintainers. Please do CC them 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.
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>  xen/common/event_channel.c | 49 ++++++++++++++++++++++++++++++++++------------
>  xen/include/xen/event.h    |  6 ++++++
>  2 files changed, 42 insertions(+), 13 deletions(-)
>
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index 638dc5e..4b039f3 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -27,6 +27,7 @@
>  #include <xen/keyhandler.h>
>  #include <xen/event_fifo.h>
>  #include <asm/current.h>
> +#include <xen/domain_page.h>

Why did you include xen/domain_page.h?

>
>  #include <public/xen.h>
>  #include <public/event_channel.h>
> @@ -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;
>
> -    if ( !port_is_valid(ld, lport) )
> -        return -EINVAL;
> -
> -    lchn = evtchn_from_port(ld, lport);
> -
> -    spin_lock(&lchn->lock);
> -
> -    /* Guest cannot send via a Xen-attached event channel. */
> -    if ( unlikely(consumer_is_xen(lchn)) )
> +    if ( !data )
>      {
> -        ret = -EINVAL;
> -        goto out;
> +        if ( !port_is_valid(ld, lport) )
> +            return -EINVAL;
> +        lchn = evtchn_from_port(ld, lport);
> +        spin_lock(&lchn->lock);
>      }
> +    else
> +        lchn = (struct evtchn *)data;
>
>      ret = xsm_evtchn_send(XSM_HOOK, ld, lchn);
>      if ( ret )
> @@ -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;
> +    }
> +
> +    ret = raw_evtchn_send(ld, lport, lchn);
> +
>      spin_unlock(&lchn->lock);
>
>      return ret;
> diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
> index 5008c80..9bd17db 100644
> --- a/xen/include/xen/event.h
> +++ b/xen/include/xen/event.h
> @@ -45,6 +45,12 @@ void send_guest_pirq(struct domain *, const struct pirq *);
>  /* Send a notification from a given domain's event-channel port. */
>  int evtchn_send(struct domain *d, unsigned int lport);
>
> +/*
> + * This function is same as evntchn_send() except it does not do xen consumer check
> + * to allow the events to be sent from xen bound channels.
> + */
> +int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data);
> +
>  /* Bind a local event-channel port to the specified VCPU. */
>  long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id);
>
>

Cheers,
Jan Beulich March 6, 2017, 8:15 a.m. | #3
>>> 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.

> 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
Bhupinder Thakur March 6, 2017, 10:16 a.m. | #4
Hi Konrad,

On 4 March 2017 at 02:43, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Tue, Feb 21, 2017 at 04:56:00PM +0530, 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,
>
> Did any code archeology help in idenfitying why this was done this way?
>
> You should explain why it was done - what was the use case , and why
> your change will not change this semantic.

I think there was never a use case earlier to generate events from the
Xen itself to the guest. The event generation always happened on
behalf of a guest via evtchn_send(), which was invoked through a
hypercall by the guest. To disallow a guest from sending an event via
an event channel which is bound to Xen, there is a check in
evtchn_send() to check if it is a xen bound channel and if it is then
disallow sending an event via that channel because that channel is
meant only for Xen.

For pl011 emulation, there is a requirement for Xen to generate the
event to dom0 when there is data for dom0 to read in the ring buffer.
Since I am using a Xen bound channel so that Xen can receive events
from dom0 when there is data for Xen to read, the above mentioned
check would not allow the event to be sent to dom0.

Since this event is required to be generated from Xen only, it is safe
to allow to send the event from a xen bound channel. That is why I
introduced a new function raw_evtchn_send() to allow Xen to do that
while still keeping that restriction for the guests who use
evtchn_send() as it is today.

>> 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
>
> .. and without taking a lock? Why?
>
The third parameter to raw_evtchn_send() is the event channel
structure pointer. When raw_evtchn_send() is called from inside
evtchn_send(), it would be passed a pointer to the local event channel
structure and lock already taken (which would be case most of the
times). But when it is called directly by Xen to send an event to dom0
(as in case of pl011), the third parameter will be passed as NULL. In
this case, the lock and a reference to local channel structure would
be taken inside raw_evtchn_send().

>
>> xen consumer check. Xen uses the raw_evtchm_send() version to send the event thus
>> bypassing the check.
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>> ---
>>  xen/common/event_channel.c | 49 ++++++++++++++++++++++++++++++++++------------
>>  xen/include/xen/event.h    |  6 ++++++
>>  2 files changed, 42 insertions(+), 13 deletions(-)
>>
>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>> index 638dc5e..4b039f3 100644
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -27,6 +27,7 @@
>>  #include <xen/keyhandler.h>
>>  #include <xen/event_fifo.h>
>>  #include <asm/current.h>
>> +#include <xen/domain_page.h>
>>
>>  #include <public/xen.h>
>>  #include <public/event_channel.h>
>> @@ -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;
>
> Please that code as is.
>
I will remove these cosmetic changes in the next patch.

>>
>> -    if ( !port_is_valid(ld, lport) )
>> -        return -EINVAL;
>> -
>> -    lchn = evtchn_from_port(ld, lport);
>> -
>> -    spin_lock(&lchn->lock);
>> -
>> -    /* Guest cannot send via a Xen-attached event channel. */
>> -    if ( unlikely(consumer_is_xen(lchn)) )
>> +    if ( !data )
>>      {
>> -        ret = -EINVAL;
>> -        goto out;
>> +        if ( !port_is_valid(ld, lport) )
>> +            return -EINVAL;
>> +        lchn = evtchn_from_port(ld, lport);
>> +        spin_lock(&lchn->lock);
>
> That won't do. Please keep the format of the old code as much
> as possible (hint: Those newlines).
>
I will remove these cosmetic changes in the next patch.

>>      }
>> +    else
>> +        lchn = (struct evtchn *)data;
>>
>>      ret = xsm_evtchn_send(XSM_HOOK, ld, lchn);
>>      if ( ret )
>> @@ -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");
>
> No. Please do not.
I will remove the failure print.

>> +        return -EINVAL;
>> +    }
>> +
>> +    ret = raw_evtchn_send(ld, lport, lchn);
>> +
>>      spin_unlock(&lchn->lock);
>>
>>      return ret;
>> diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
>> index 5008c80..9bd17db 100644
>> --- a/xen/include/xen/event.h
>> +++ b/xen/include/xen/event.h
>> @@ -45,6 +45,12 @@ void send_guest_pirq(struct domain *, const struct pirq *);
>>  /* Send a notification from a given domain's event-channel port. */
>>  int evtchn_send(struct domain *d, unsigned int lport);
>>
>> +/*
>> + * This function is same as evntchn_send() except it does not do xen consumer check
>> + * to allow the events to be sent from xen bound channels.
>
> And it also looks to ignore the locking? Could you explain why?
>
It will take the lock when invoked directly (explained above).

>> + */
>> +int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data);
>> +
>>  /* Bind a local event-channel port to the specified VCPU. */
>>  long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id);
>>
>> --
>> 2.7.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> https://lists.xen.org/xen-devel
Jan Beulich March 6, 2017, 10:35 a.m. | #5
>>> On 06.03.17 at 11:16, <bhupinder.thakur@linaro.org> wrote:
> On 4 March 2017 at 02:43, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> On Tue, Feb 21, 2017 at 04:56:00PM +0530, 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,
>>
>> Did any code archeology help in idenfitying why this was done this way?
>>
>> You should explain why it was done - what was the use case , and why
>> your change will not change this semantic.
> 
> I think there was never a use case earlier to generate events from the
> Xen itself to the guest. The event generation always happened on
> behalf of a guest via evtchn_send(), which was invoked through a
> hypercall by the guest.

This is not true: All interrupts being converted to events go this
route, without any hypercall involved. Same for signaling qemu
to do emulation of an emulated device (memory of port) access.

> To disallow a guest from sending an event via
> an event channel which is bound to Xen, there is a check in
> evtchn_send() to check if it is a xen bound channel and if it is then
> disallow sending an event via that channel because that channel is
> meant only for Xen.

As said in reply to your patch - I think you got directions mixed up
here: You talk about Xen sending an event to a guest, but at the
same time you fiddle with code preventing guests from sending
events to Xen.

> For pl011 emulation, there is a requirement for Xen to generate the
> event to dom0 when there is data for dom0 to read in the ring buffer.

This is not different from hardware signaling the need for service
via an interrupt, which - always for PV guests, sometimes for HVM
ones - is being converted to an event.

> Since I am using a Xen bound channel so that Xen can receive events
> from dom0 when there is data for Xen to read, the above mentioned
> check would not allow the event to be sent to dom0.

Now here you come to talk about the direction which indeed is not
allowed. But breaking out part of the function won't help you, as
the new (raw) function then mustn't be called as a result of some
guest's request (hypercall or whatever), which includes Dom0.

> Since this event is required to be generated from Xen only, it is safe
> to allow to send the event from a xen bound channel. That is why I
> introduced a new function raw_evtchn_send() to allow Xen to do that
> while still keeping that restriction for the guests who use
> evtchn_send() as it is today.

And now it becomes confusing again: Yet another time you talk about
Xen sending the event. Please can you cleanly separate both directions
of event flow, and talk only about the relevant one in the respective
contexts?

Jan
Bhupinder Thakur March 6, 2017, 10:44 a.m. | #6
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
>
Jan Beulich March 6, 2017, 10:54 a.m. | #7
>>> On 06.03.17 at 11:44, <bhupinder.thakur@linaro.org> wrote:
> 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:
>>> 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.

I can see why Xen needs to send events; I can't see why Dom0 couldn't
simply make a hypercall instead of sending an event if it needs to signal
something.

Jan
Bhupinder Thakur March 6, 2017, 11:12 a.m. | #8
Hi,

On 6 March 2017 at 16:24, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 06.03.17 at 11:44, <bhupinder.thakur@linaro.org> wrote:
>> 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:
>>>> 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.
>
> I can see why Xen needs to send events; I can't see why Dom0 couldn't
> simply make a hypercall instead of sending an event if it needs to signal
> something.
>
We decided to reuse the same PV console interface for pl011 as used
for PV console in xenconsole running on dom0, which is events/ring
buffer based. From xenconsole point of view, there is no difference in
terms of handling a pl011 console and a PV console.

> Jan
>
Bhupinder Thakur March 28, 2017, 9:43 a.m. | #9
Hi,

Now I am using notify_via_xen_event_channel() to send the vpl011
events to dom0. So no changes are required in evtchn_send() and this
patch can be dropped.

Regards,
Bhupinder

On 6 March 2017 at 16:42, Bhupinder Thakur <bhupinder.thakur@linaro.org> wrote:
> Hi,
>
> On 6 March 2017 at 16:24, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 06.03.17 at 11:44, <bhupinder.thakur@linaro.org> wrote:
>>> 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:
>>>>> 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.
>>
>> I can see why Xen needs to send events; I can't see why Dom0 couldn't
>> simply make a hypercall instead of sending an event if it needs to signal
>> something.
>>
> We decided to reuse the same PV console interface for pl011 as used
> for PV console in xenconsole running on dom0, which is events/ring
> buffer based. From xenconsole point of view, there is no difference in
> terms of handling a pl011 console and a PV console.
>
>> Jan
>>

Patch

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 638dc5e..4b039f3 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -27,6 +27,7 @@ 
 #include <xen/keyhandler.h>
 #include <xen/event_fifo.h>
 #include <asm/current.h>
+#include <xen/domain_page.h>
 
 #include <public/xen.h>
 #include <public/event_channel.h>
@@ -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;
 
-    if ( !port_is_valid(ld, lport) )
-        return -EINVAL;
-
-    lchn = evtchn_from_port(ld, lport);
-
-    spin_lock(&lchn->lock);
-
-    /* Guest cannot send via a Xen-attached event channel. */
-    if ( unlikely(consumer_is_xen(lchn)) )
+    if ( !data )
     {
-        ret = -EINVAL;
-        goto out;
+        if ( !port_is_valid(ld, lport) )
+            return -EINVAL;
+        lchn = evtchn_from_port(ld, lport);
+        spin_lock(&lchn->lock);
     }
+    else
+        lchn = (struct evtchn *)data; 
 
     ret = xsm_evtchn_send(XSM_HOOK, ld, lchn);
     if ( ret )
@@ -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;
+    }
+
+    ret = raw_evtchn_send(ld, lport, lchn);
+
     spin_unlock(&lchn->lock);
 
     return ret;
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 5008c80..9bd17db 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -45,6 +45,12 @@  void send_guest_pirq(struct domain *, const struct pirq *);
 /* Send a notification from a given domain's event-channel port. */
 int evtchn_send(struct domain *d, unsigned int lport);
 
+/* 
+ * This function is same as evntchn_send() except it does not do xen consumer check
+ * to allow the events to be sent from xen bound channels.
+ */
+int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data);
+
 /* Bind a local event-channel port to the specified VCPU. */
 long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id);