[PATCH RFC] xen/evtchn: Implement EVTCHNOP_send_imm as a companian to EVTCHNOP_send

[PATCH RFC] xen/evtchn: Implement EVTCHNOP_send_imm as a companian to EVTCHNOP_send

[Andrew Cooper]

The event channel ABI passes a pointer to a subop-specific structure.  Some of
these structures however are smaller than the pointers passed in the ABI;
EVTCHNOP_send specifically passes a 4 byte evtchn port, using a 4 or 8 byte
pointer.

For translated guests (x86 HVM and all ARM), passing the port number directly
avoids a guest pagetable walk, which can be very expensive for Xen to perform.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Juergen Gross <jgross@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

RFC: is it worth converting the close, unmask, reset (and possibly
expand_array) as well?
---
 xen/common/event_channel.c         |  4 ++++
 xen/include/public/event_channel.h | 13 ++++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index c69f9db..79d8614 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -1098,6 +1098,10 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     }
 
+    case EVTCHNOP_send_imm:
+        rc = evtchn_send(current->domain, (unsigned long)arg.p);
+        break;
+
     case EVTCHNOP_status: {
         struct evtchn_status status;
         if ( copy_from_guest(&status, arg, 1) != 0 )
diff --git a/xen/include/public/event_channel.h b/xen/include/public/event_channel.h
index 44c549d..834787a 100644
--- a/xen/include/public/event_channel.h
+++ b/xen/include/public/event_channel.h
@@ -57,6 +57,9 @@
  * `
  * @cmd  == EVTCHNOP_* (event-channel operation).
  * @args == struct evtchn_* Operation-specific extra arguments (NULL if none).
+ *
+ * For @cmd with an _imm suffix, data are passed as an integer, rather than a
+ * pointer to a structure.
  */
 
 /* ` enum event_channel_op { // EVTCHNOP_* => struct evtchn_* */
@@ -64,7 +67,7 @@
 #define EVTCHNOP_bind_virq        1
 #define EVTCHNOP_bind_pirq        2
 #define EVTCHNOP_close            3
-#define EVTCHNOP_send             4
+#define EVTCHNOP_send             4 /* Deprecated.  Use EVTCHNOP_send_imm in preference. */
 #define EVTCHNOP_status           5
 #define EVTCHNOP_alloc_unbound    6
 #define EVTCHNOP_bind_ipi         7
@@ -74,6 +77,7 @@
 #define EVTCHNOP_init_control    11
 #define EVTCHNOP_expand_array    12
 #define EVTCHNOP_set_priority    13
+#define EVTCHNOP_send_imm        14
 /* ` } */
 
 typedef uint32_t evtchn_port_t;
@@ -186,8 +190,11 @@ struct evtchn_close {
 typedef struct evtchn_close evtchn_close_t;
 
 /*
- * EVTCHNOP_send: Send an event to the remote end of the channel whose local
- * endpoint is <port>.
+ * EVTCHNOP_send{,_imm}: Send an event to the remote end of the channel whose
+ * local endpoint is <port>.
+ *
+ * For EVTCHNOP_send, arg is a pointer to an evtchn_send_t.  For
+ * EVTCHNOP_send_imm, arg is the port directly.
  */
 struct evtchn_send {
     /* IN parameters. */
-- 
2.1.4


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

Re: [PATCH RFC] xen/evtchn: Implement EVTCHNOP_send_imm as a companian to EVTCHNOP_send

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Andrew Cooper
> Sent: 13 July 2017 09:51
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Juergen Gross <jgross@suse.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Julien Grall <julien.grall@arm.com>; Jan
> Beulich <JBeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: [Xen-devel] [PATCH RFC] xen/evtchn: Implement
> EVTCHNOP_send_imm as a companian to EVTCHNOP_send
> 
> The event channel ABI passes a pointer to a subop-specific structure.  Some
> of
> these structures however are smaller than the pointers passed in the ABI;
> EVTCHNOP_send specifically passes a 4 byte evtchn port, using a 4 or 8 byte
> pointer.
> 
> For translated guests (x86 HVM and all ARM), passing the port number
> directly
> avoids a guest pagetable walk, which can be very expensive for Xen to
> perform.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

LGTM. I suspect this will help performance of PV drivers quite a bit.

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> RFC: is it worth converting the close, unmask, reset (and possibly
> expand_array) as well?
> ---
>  xen/common/event_channel.c         |  4 ++++
>  xen/include/public/event_channel.h | 13 ++++++++++---
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index c69f9db..79d8614 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1098,6 +1098,10 @@ long do_event_channel_op(int cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
> 
> +    case EVTCHNOP_send_imm:
> +        rc = evtchn_send(current->domain, (unsigned long)arg.p);
> +        break;
> +
>      case EVTCHNOP_status: {
>          struct evtchn_status status;
>          if ( copy_from_guest(&status, arg, 1) != 0 )
> diff --git a/xen/include/public/event_channel.h
> b/xen/include/public/event_channel.h
> index 44c549d..834787a 100644
> --- a/xen/include/public/event_channel.h
> +++ b/xen/include/public/event_channel.h
> @@ -57,6 +57,9 @@
>   * `
>   * @cmd  == EVTCHNOP_* (event-channel operation).
>   * @args == struct evtchn_* Operation-specific extra arguments (NULL if
> none).
> + *
> + * For @cmd with an _imm suffix, data are passed as an integer, rather than
> a
> + * pointer to a structure.
>   */
> 
>  /* ` enum event_channel_op { // EVTCHNOP_* => struct evtchn_* */
> @@ -64,7 +67,7 @@
>  #define EVTCHNOP_bind_virq        1
>  #define EVTCHNOP_bind_pirq        2
>  #define EVTCHNOP_close            3
> -#define EVTCHNOP_send             4
> +#define EVTCHNOP_send             4 /* Deprecated.  Use
> EVTCHNOP_send_imm in preference. */
>  #define EVTCHNOP_status           5
>  #define EVTCHNOP_alloc_unbound    6
>  #define EVTCHNOP_bind_ipi         7
> @@ -74,6 +77,7 @@
>  #define EVTCHNOP_init_control    11
>  #define EVTCHNOP_expand_array    12
>  #define EVTCHNOP_set_priority    13
> +#define EVTCHNOP_send_imm        14
>  /* ` } */
> 
>  typedef uint32_t evtchn_port_t;
> @@ -186,8 +190,11 @@ struct evtchn_close {
>  typedef struct evtchn_close evtchn_close_t;
> 
>  /*
> - * EVTCHNOP_send: Send an event to the remote end of the channel
> whose local
> - * endpoint is <port>.
> + * EVTCHNOP_send{,_imm}: Send an event to the remote end of the
> channel whose
> + * local endpoint is <port>.
> + *
> + * For EVTCHNOP_send, arg is a pointer to an evtchn_send_t.  For
> + * EVTCHNOP_send_imm, arg is the port directly.
>   */
>  struct evtchn_send {
>      /* IN parameters. */
> --
> 2.1.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

Re: [PATCH RFC] xen/evtchn: Implement EVTCHNOP_send_imm as a companian to EVTCHNOP_send

[Juergen Groß]

On 07/13/2017 09:50 AM, Andrew Cooper wrote:
> The event channel ABI passes a pointer to a subop-specific structure.  Some of
> these structures however are smaller than the pointers passed in the ABI;
> EVTCHNOP_send specifically passes a 4 byte evtchn port, using a 4 or 8 byte
> pointer.
> 
> For translated guests (x86 HVM and all ARM), passing the port number directly
> avoids a guest pagetable walk, which can be very expensive for Xen to perform.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'm absolutely in favor of the approach.

Could we please discuss which hypercalls should offer an immediate
variant before implementing one after the other? I believe those should
all be introduced at the same time in order to make deciding which
variant to use in domU easier: I don't want to have a boolean for each
possible hypercall, but just one.

Juergen

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

Re: [PATCH RFC] xen/evtchn: Implement EVTCHNOP_send_imm as a companian to EVTCHNOP_send

[Wei Liu]

On Thu, Jul 13, 2017 at 12:14:35PM +0200, Juergen Groß wrote:
> On 07/13/2017 09:50 AM, Andrew Cooper wrote:
> > The event channel ABI passes a pointer to a subop-specific structure.  Some of
> > these structures however are smaller than the pointers passed in the ABI;
> > EVTCHNOP_send specifically passes a 4 byte evtchn port, using a 4 or 8 byte
> > pointer.
> > 
> > For translated guests (x86 HVM and all ARM), passing the port number directly
> > avoids a guest pagetable walk, which can be very expensive for Xen to perform.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> I'm absolutely in favor of the approach.
> 

+1

> Could we please discuss which hypercalls should offer an immediate
> variant before implementing one after the other? I believe those should
> all be introduced at the same time in order to make deciding which
> variant to use in domU easier: I don't want to have a boolean for each
> possible hypercall, but just one.

+1

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

Re: [PATCH RFC] xen/evtchn: Implement EVTCHNOP_send_imm as a companian to EVTCHNOP_send

[Jan Beulich]

>>> Andrew Cooper <andrew.cooper3@citrix.com> 07/13/17 9:50 AM >>>
>RFC: is it worth converting the close, unmask, reset (and possibly
>expand_array) as well?

I can't see close and even more so reset to be performance critical.
Unmask otoh may be (depending on use); no sure about expand_array.

Jan


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

Re: [PATCH RFC] xen/evtchn: Implement EVTCHNOP_send_imm as a companian to EVTCHNOP_send

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Jan
> Beulich
> Sent: 13 July 2017 21:36
> To: Andrew Cooper <Andrew.Cooper3@citrix.com>
> Cc: Juergen Gross <jgross@suse.com>; sstabellini@kernel.org; Wei Liu
> <wei.liu2@citrix.com>; xen-devel@lists.xen.org; julien.grall@arm.com;
> Roger Pau Monne <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH RFC] xen/evtchn: Implement
> EVTCHNOP_send_imm as a companian to EVTCHNOP_send
> 
> >>> Andrew Cooper <andrew.cooper3@citrix.com> 07/13/17 9:50 AM >>>
> >RFC: is it worth converting the close, unmask, reset (and possibly
> >expand_array) as well?
> 
> I can't see close and even more so reset to be performance critical.
> Unmask otoh may be (depending on use); no sure about expand_array.
>

IIRC the unmask hypercall only needs to be made in certain circumstances when direct unmasking of the fifo structure might race (can't remember the detail offhand), but it still seems like a reasonable target for an immediate argument.

  Paul
 
> Jan
> 
> 
> _______________________________________________
> 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

Re: [PATCH RFC] xen/evtchn: Implement EVTCHNOP_send_imm as a companian to EVTCHNOP_send

[Jan Beulich]

>>> On 13.07.17 at 09:50, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1098,6 +1098,10 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case EVTCHNOP_send_imm:
> +        rc = evtchn_send(current->domain, (unsigned long)arg.p);

Two more things: For one this discards the upper half of the 64-bit
handle. I'd suggest you instead check it to be zero. And then x86's
do_event_channel_op_compat() should refuse to handle "immediate"
commands.

Jan


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

Re: [PATCH RFC] xen/evtchn: Implement EVTCHNOP_send_imm as a companian to EVTCHNOP_send

[Stefano Stabellini]

On Fri, 14 Jul 2017, Jan Beulich wrote:
> >>> On 13.07.17 at 09:50, <andrew.cooper3@citrix.com> wrote:
> > --- a/xen/common/event_channel.c
> > +++ b/xen/common/event_channel.c
> > @@ -1098,6 +1098,10 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >          break;
> >      }
> >  
> > +    case EVTCHNOP_send_imm:
> > +        rc = evtchn_send(current->domain, (unsigned long)arg.p);
> 
> Two more things: For one this discards the upper half of the 64-bit
> handle. I'd suggest you instead check it to be zero.

+1, keeping in mind that arg will be 32-bit on ARM32 platforms and
64-bit on ARM64 platforms.

Moreover, evtchn_send takes an unsigned int as argument, why are you
casting arg.p to (unsigned long)?


> And then x86's do_event_channel_op_compat() should refuse to handle
> "immediate" commands.

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

Re: [PATCH RFC] xen/evtchn: Implement EVTCHNOP_send_imm as a companian to EVTCHNOP_send

[Andrew Cooper]

On 17/07/2017 19:30, Stefano Stabellini wrote:
> On Fri, 14 Jul 2017, Jan Beulich wrote:
>>>>> On 13.07.17 at 09:50, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -1098,6 +1098,10 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>          break;
>>>      }
>>>  
>>> +    case EVTCHNOP_send_imm:
>>> +        rc = evtchn_send(current->domain, (unsigned long)arg.p);
>> Two more things: For one this discards the upper half of the 64-bit
>> handle. I'd suggest you instead check it to be zero.
> +1, keeping in mind that arg will be 32-bit on ARM32 platforms and
> 64-bit on ARM64 platforms.
>
> Moreover, evtchn_send takes an unsigned int as argument, why are you
> casting arg.p to (unsigned long)?

Because arg.p is a pointer, and casting that to an unsigned int will
break on 64bit builds.  (IIRC, GCC tolerates casting a pointer to an
integer of suitable width, but not if a truncation would happen).

It probably wants to be:

uintptr_t port = (uintptr_t)arg.p;

if ( port > (evtchn_port_t)-1 )
    fail;

to be properly within the C spec.

~Andrew

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