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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi Konrad,

On 4 March 2017 at 02:43, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxx
>> https://lists.xen.org/xen-devel

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