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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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

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