[PATCH v2] tools/xl: correctly shows split eventchannel for netfront

[PATCH v2] tools/xl: correctly shows split eventchannel for netfront

[Annie Li]

From: Annie Li <annie.li@oracle.com>

After split eventchannel feature was supported by netback/netfront,
"xl network-list" does not show eventchannel correctly. Add tx-/rx-evt-ch
to show tx/rx eventchannel correctly.

V2: keep evtch field without breaking the API

Signed-off-by: Annie Li <annie.li@oracle.com>
---
 tools/libxl/libxl.c         |    8 ++++++++
 tools/libxl/libxl.h         |   12 ++++++++++++
 tools/libxl/libxl_types.idl |    3 +++
 tools/libxl/xl_cmdimpl.c    |   12 ++++++------
 4 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2845ca4..4ce9efc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3142,6 +3142,14 @@ int libxl_device_nic_getinfo(libxl_ctx *ctx, uint32_t domid,
     nicinfo->state = val ? strtoul(val, NULL, 10) : -1;
     val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/event-channel", nicpath));
     nicinfo->evtch = val ? strtoul(val, NULL, 10) : -1;
+    if(nicinfo->evtch > 0)
+        nicinfo->evtch_rx = nicinfo->evtch;
+    else {
+        val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/event-channel-tx", nicpath));
+        nicinfo->evtch = val ? strtoul(val, NULL, 10) : -1;
+        val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/event-channel-rx", nicpath));
+        nicinfo->evtch_rx = val ? strtoul(val, NULL, 10) : -1;
+    }
     val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/tx-ring-ref", nicpath));
     nicinfo->rref_tx = val ? strtoul(val, NULL, 10) : -1;
     val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/rx-ring-ref", nicpath));
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 12d6c31..27ef565 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -409,6 +409,18 @@
  */
 #define LIBXL_HAVE_DRIVER_DOMAIN_CREATION 1
 
+/*
+ * LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL
+ * If this is defined, libxl_nicinfo will contain an integer type field: evtch_rx,
+ * containing the value of eventchannel for rx path of netback&netfront which support
+ * split event channel. The original evtch field contains the value of eventchannel
+ * for tx path in such case.
+ *
+ */
+#if LIBXL_API_VERSION > 0x040400
+#define LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL 1
+#endif
+
 /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
  * called from within libxl itself. Callers outside libxl, who
  * do not #include libxl_internal.h, are fine. */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 649ce50..b1b4946 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -489,6 +489,9 @@ libxl_nicinfo = Struct("nicinfo", [
     ("devid", libxl_devid),
     ("state", integer),
     ("evtch", integer),
+#ifdef LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL
+    ("evtch_rx", integer),
+#endif
     ("rref_tx", integer),
     ("rref_rx", integer),
     ], dir=DIR_OUT)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c30f495..be1162e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5842,9 +5842,9 @@ int main_networklist(int argc, char **argv)
         /* No options */
     }
 
-    /*      Idx  BE   MAC   Hdl  Sta  evch txr/rxr  BE-path */
-    printf("%-3s %-2s %-17s %-6s %-5s %-6s %5s/%-5s %-30s\n",
-           "Idx", "BE", "Mac Addr.", "handle", "state", "evt-ch", "tx-", "rx-ring-ref", "BE-path");
+    /*      Idx  BE   MAC   Hdl  Sta  txev/rxev txr/rxr  BE-path */
+    printf("%-3s %-2s %-17s %-6s %-5s %6s/%-6s %5s/%-5s %-30s\n",
+           "Idx", "BE", "Mac Addr.", "handle", "state", "tx-", "rx-evt-ch", "tx-", "rx-ring-ref", "BE-path");
     for (argv += optind, argc -= optind; argc > 0; --argc, ++argv) {
         uint32_t domid = find_domain(*argv);
         nics = libxl_device_nic_list(ctx, domid, &nb);
@@ -5857,9 +5857,9 @@ int main_networklist(int argc, char **argv)
                 printf("%-3d %-2d ", nicinfo.devid, nicinfo.backend_id);
                 /* MAC */
                 printf(LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nics[i].mac));
-                /* Hdl  Sta  evch txr/rxr  BE-path */
-                printf("%6d %5d %6d %5d/%-11d %-30s\n",
-                       nicinfo.devid, nicinfo.state, nicinfo.evtch,
+                /* Hdl  Sta  txev/rxev txr/rxr  BE-path */
+                printf(" %6d %5d %6d/%-9d %5d/%-11d %-30s\n",
+                       nicinfo.devid, nicinfo.state, nicinfo.evtch, nicinfo.evtch_rx,
                        nicinfo.rref_tx, nicinfo.rref_rx, nicinfo.backend);
                 libxl_nicinfo_dispose(&nicinfo);
             }
-- 
1.7.3.4

Re: [PATCH v2] tools/xl: correctly shows split eventchannel for netfront

[Ian Campbell]

On Fri, 2014-01-24 at 18:51 +0800, Annie Li wrote:

> +/*
> + * LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL
> + * If this is defined, libxl_nicinfo will contain an integer type field: evtch_rx,
> + * containing the value of eventchannel for rx path of netback&netfront which support
> + * split event channel. The original evtch field contains the value of eventchannel
> + * for tx path in such case.

I think it can be either "event channel" (two words) or
"evtchn" (abbreviation) but not "eventchannel".

> + *
> + */
> +#if LIBXL_API_VERSION > 0x040400
> +#define LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL 1

This should be unconditional. There are several existing examples in
libxl.h which you could have copied.

> +#endif
> +
>  /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
>   * called from within libxl itself. Callers outside libxl, who
>   * do not #include libxl_internal.h, are fine. */
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 649ce50..b1b4946 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -489,6 +489,9 @@ libxl_nicinfo = Struct("nicinfo", [
>      ("devid", libxl_devid),
>      ("state", integer),
>      ("evtch", integer),
> +#ifdef LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL
> +    ("evtch_rx", integer),
> +#endif

Did you even build this? Because it can't have worked (Libxl_types.idl
is a Python source file which is not preprocessed).

In any case, this ifdef is unneccessary and wrong. the LIBXL_HAVE
indicates to the consumer that the field is present, but it should not
actually gate the presence of the field.

Think about it -- if an application is building against libxl version
4.4 (which was therefore built with evtchn_rx present) but requests
LIBXL_API_VERSION == 0x040300 then your patch has now created an ABI
mismatch.

Ian.

Re: [PATCH v2] tools/xl: correctly shows split eventchannel for netfront

[annie li]

On 2014/1/24 18:58, Ian Campbell wrote:
> On Fri, 2014-01-24 at 18:51 +0800, Annie Li wrote:
>
>> +/*
>> + * LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL
>> + * If this is defined, libxl_nicinfo will contain an integer type field: evtch_rx,
>> + * containing the value of eventchannel for rx path of netback&netfront which support
>> + * split event channel. The original evtch field contains the value of eventchannel
>> + * for tx path in such case.
> I think it can be either "event channel" (two words) or
> "evtchn" (abbreviation) but not "eventchannel".

Ok, thanks.

>
>> + *
>> + */
>> +#if LIBXL_API_VERSION > 0x040400
>> +#define LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL 1
> This should be unconditional. There are several existing examples in
> libxl.h which you could have copied.

OK

>
>> +#endif
>> +
>>   /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
>>    * called from within libxl itself. Callers outside libxl, who
>>    * do not #include libxl_internal.h, are fine. */
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 649ce50..b1b4946 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -489,6 +489,9 @@ libxl_nicinfo = Struct("nicinfo", [
>>       ("devid", libxl_devid),
>>       ("state", integer),
>>       ("evtch", integer),
>> +#ifdef LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL
>> +    ("evtch_rx", integer),
>> +#endif
> Did you even build this? Because it can't have worked (Libxl_types.idl
> is a Python source file which is not preprocessed).

My fault, I only built out xl from libxl and replace current one for 
test, not familiar with Python.:-(

>
> In any case, this ifdef is unneccessary and wrong. the LIBXL_HAVE
> indicates to the consumer that the field is present, but it should not
> actually gate the presence of the field.
>
> Think about it -- if an application is building against libxl version
> 4.4 (which was therefore built with evtchn_rx present) but requests
> LIBXL_API_VERSION == 0x040300 then your patch has now created an ABI
> mismatch.

I misunderstood LIBXL_HAVE_* in libxl.h, and thought it is used by libxl 
itself. Much clear now, thanks.

Thanks
Annie