[dpdk-dev] [PATCH v3 0/4] bidirect guest channel

[dpdk-dev] [PATCH v3 0/4] bidirect guest channel

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Extend guest channel API to allow bidirectional
communication. Modify power manager host and guest
side to communicate in both directions.

v3:
* fix global_fds[lcore_id] comparison to invalid value
* check 0 to verify if read function actually read some data
* define _NACK cmd instead of _NAK
* simplify rte_power_guest_channel_receive_msg func logic

v2:
* send ack only if power operation return positive value
* log diffent error for unexpected incoming command and
  error during ack/nak cmd sending


Marcin Hajkowski (4):
  power: fix invalid socket indicator value
  power: extend guest channel api for reading
  power: process incoming confirmation cmds
  power: send confirmation cmd to vm guest

 examples/vm_power_manager/channel_monitor.c   | 67 ++++++++++++++--
 examples/vm_power_manager/guest_cli/Makefile  |  1 +
 .../guest_cli/vm_power_cli_guest.c            | 65 +++++++++++++---
 lib/librte_power/channel_commands.h           |  5 ++
 lib/librte_power/guest_channel.c              | 77 +++++++++++++++++--
 lib/librte_power/guest_channel.h              | 35 +++++++++
 lib/librte_power/rte_power_version.map        |  1 +
 7 files changed, 227 insertions(+), 24 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH v3 0/4] bidirect guest channel

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Extend guest channel API to allow bidirectional
communication. Modify power manager host and guest
side to communicate in both directions.

v3:
* fix global_fds[lcore_id] comparison to invalid value
* check 0 to verify if read function actually read some data
* define _NACK cmd instead of _NAK
* simplify rte_power_guest_channel_receive_msg func logic

v2:
* send ack only if power operation return positive value
* log diffent error for unexpected incoming command and
  error during ack/nak cmd sending


Marcin Hajkowski (4):
  power: fix invalid socket indicator value
  power: extend guest channel api for reading
  power: process incoming confirmation cmds
  power: send confirmation cmd to vm guest

 examples/vm_power_manager/channel_monitor.c   | 67 ++++++++++++++--
 examples/vm_power_manager/guest_cli/Makefile  |  1 +
 .../guest_cli/vm_power_cli_guest.c            | 65 +++++++++++++---
 lib/librte_power/channel_commands.h           |  5 ++
 lib/librte_power/guest_channel.c              | 77 +++++++++++++++++--
 lib/librte_power/guest_channel.h              | 35 +++++++++
 lib/librte_power/rte_power_version.map        |  1 +
 7 files changed, 227 insertions(+), 24 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH v3 1/4] power: fix invalid socket indicator value

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Currently 0 is being used for not connected slot indication.
This is not consistent with linux doc which identifies 0 as valid
(connected) slot, thus modification was done to change it.

Fixes: cd0d5547 ("power: vm communication channels in guest")
Cc: stable@dpdk.org

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 lib/librte_power/guest_channel.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
index c17ea46b4..9cf7d2cb2 100644
--- a/lib/librte_power/guest_channel.c
+++ b/lib/librte_power/guest_channel.c
@@ -19,7 +19,7 @@
 
 #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
 
-static int global_fds[RTE_MAX_LCORE];
+static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
 
 int
 guest_channel_host_connect(const char *path, unsigned int lcore_id)
@@ -35,7 +35,7 @@ guest_channel_host_connect(const char *path, unsigned int lcore_id)
 		return -1;
 	}
 	/* check if path is already open */
-	if (global_fds[lcore_id] != 0) {
+	if (global_fds[lcore_id] != -1) {
 		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is already open with fd %d\n",
 				lcore_id, global_fds[lcore_id]);
 		return -1;
@@ -84,7 +84,7 @@ guest_channel_host_connect(const char *path, unsigned int lcore_id)
 	return 0;
 error:
 	close(fd);
-	global_fds[lcore_id] = 0;
+	global_fds[lcore_id] = -1;
 	return -1;
 }
 
@@ -100,7 +100,7 @@ guest_channel_send_msg(struct channel_packet *pkt, unsigned int lcore_id)
 		return -1;
 	}
 
-	if (global_fds[lcore_id] == 0) {
+	if (global_fds[lcore_id] < 0) {
 		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
 		return -1;
 	}
@@ -134,8 +134,8 @@ guest_channel_host_disconnect(unsigned int lcore_id)
 				lcore_id, RTE_MAX_LCORE-1);
 		return;
 	}
-	if (global_fds[lcore_id] == 0)
+	if (global_fds[lcore_id] < 0)
 		return;
 	close(global_fds[lcore_id]);
-	global_fds[lcore_id] = 0;
+	global_fds[lcore_id] = -1;
 }
-- 
2.17.2

[dpdk-dev] [PATCH v3 1/4] power: fix invalid socket indicator value

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Currently 0 is being used for not connected slot indication.
This is not consistent with linux doc which identifies 0 as valid
(connected) slot, thus modification was done to change it.

Fixes: cd0d5547 ("power: vm communication channels in guest")
Cc: stable@dpdk.org

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 lib/librte_power/guest_channel.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
index c17ea46b4..9cf7d2cb2 100644
--- a/lib/librte_power/guest_channel.c
+++ b/lib/librte_power/guest_channel.c
@@ -19,7 +19,7 @@
 
 #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
 
-static int global_fds[RTE_MAX_LCORE];
+static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
 
 int
 guest_channel_host_connect(const char *path, unsigned int lcore_id)
@@ -35,7 +35,7 @@ guest_channel_host_connect(const char *path, unsigned int lcore_id)
 		return -1;
 	}
 	/* check if path is already open */
-	if (global_fds[lcore_id] != 0) {
+	if (global_fds[lcore_id] != -1) {
 		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is already open with fd %d\n",
 				lcore_id, global_fds[lcore_id]);
 		return -1;
@@ -84,7 +84,7 @@ guest_channel_host_connect(const char *path, unsigned int lcore_id)
 	return 0;
 error:
 	close(fd);
-	global_fds[lcore_id] = 0;
+	global_fds[lcore_id] = -1;
 	return -1;
 }
 
@@ -100,7 +100,7 @@ guest_channel_send_msg(struct channel_packet *pkt, unsigned int lcore_id)
 		return -1;
 	}
 
-	if (global_fds[lcore_id] == 0) {
+	if (global_fds[lcore_id] < 0) {
 		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
 		return -1;
 	}
@@ -134,8 +134,8 @@ guest_channel_host_disconnect(unsigned int lcore_id)
 				lcore_id, RTE_MAX_LCORE-1);
 		return;
 	}
-	if (global_fds[lcore_id] == 0)
+	if (global_fds[lcore_id] < 0)
 		return;
 	close(global_fds[lcore_id]);
-	global_fds[lcore_id] = 0;
+	global_fds[lcore_id] = -1;
 }
-- 
2.17.2

[dpdk-dev] [PATCH v3 2/4] power: extend guest channel api for reading

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Added new experimental API rte_power_guest_channel_receive_msg
which gives possibility to receive messages send to guest.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 lib/librte_power/channel_commands.h    |  5 ++
 lib/librte_power/guest_channel.c       | 65 ++++++++++++++++++++++++++
 lib/librte_power/guest_channel.h       | 35 ++++++++++++++
 lib/librte_power/rte_power_version.map |  1 +
 4 files changed, 106 insertions(+)

diff --git a/lib/librte_power/channel_commands.h b/lib/librte_power/channel_commands.h
index e7b93a797..33fd53a6d 100644
--- a/lib/librte_power/channel_commands.h
+++ b/lib/librte_power/channel_commands.h
@@ -28,6 +28,11 @@ extern "C" {
 #define CPU_POWER_SCALE_MIN     4
 #define CPU_POWER_ENABLE_TURBO  5
 #define CPU_POWER_DISABLE_TURBO 6
+
+/* Generic Power Command Response */
+#define CPU_POWER_CMD_ACK       1
+#define CPU_POWER_CMD_NACK      2
+
 #define HOURS 24
 
 #define MAX_VFS 10
diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
index 9cf7d2cb2..3213fa769 100644
--- a/lib/librte_power/guest_channel.c
+++ b/lib/librte_power/guest_channel.c
@@ -10,6 +10,7 @@
 #include <fcntl.h>
 #include <string.h>
 #include <errno.h>
+#include <poll.h>
 
 
 #include <rte_log.h>
@@ -19,6 +20,9 @@
 
 #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
 
+/* Timeout for incoming message in milliseconds. */
+#define TIMEOUT 10
+
 static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
 
 int
@@ -125,6 +129,67 @@ int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
 	return guest_channel_send_msg(pkt, lcore_id);
 }
 
+int power_guest_channel_read_msg(struct channel_packet *pkt,
+			unsigned int lcore_id)
+{
+	int ret;
+
+	struct pollfd fds;
+	fds.fd = global_fds[lcore_id];
+	fds.events = POLLIN;
+
+	ret = poll(&fds, 1, TIMEOUT);
+	if (ret == 0) {
+		RTE_LOG(DEBUG, GUEST_CHANNEL, "Timeout occurs during poll function.\n");
+		return -1;
+	} else if (ret < 0) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Error occurred during poll function: %s\n",
+				strerror(ret));
+		return -1;
+	}
+
+	void *buffer = pkt;
+
+	if (lcore_id >= RTE_MAX_LCORE) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
+				lcore_id, RTE_MAX_LCORE-1);
+		return -1;
+	}
+
+	if (global_fds[lcore_id] < 0) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
+		return -1;
+	}
+
+	int buffer_len = sizeof(*pkt);
+
+	while (buffer_len > 0) {
+		ret = read(global_fds[lcore_id],
+				buffer, buffer_len);
+		if (ret < 0) {
+			if (errno == EINTR)
+				continue;
+			return -1;
+		}
+		/* No data has been read, this can indicate not
+		 * consistent buffer_len with actual data size.
+		 */
+		if (ret == 0) {
+			RTE_LOG(ERR, GUEST_CHANNEL, "No more data has been read.\n");
+			return -1;
+		}
+		buffer = (char *)buffer + ret;
+		buffer_len -= ret;
+	}
+
+	return 0;
+}
+
+int rte_power_guest_channel_receive_msg(struct channel_packet *pkt,
+			unsigned int lcore_id)
+{
+	return power_guest_channel_read_msg(pkt, lcore_id);
+}
 
 void
 guest_channel_host_disconnect(unsigned int lcore_id)
diff --git a/lib/librte_power/guest_channel.h b/lib/librte_power/guest_channel.h
index 373d39898..7c385df39 100644
--- a/lib/librte_power/guest_channel.h
+++ b/lib/librte_power/guest_channel.h
@@ -68,6 +68,41 @@ int guest_channel_send_msg(struct channel_packet *pkt, unsigned int lcore_id);
 int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
 			unsigned int lcore_id);
 
+/**
+ * Read a message contained in pkt over the Virtio-Serial
+ * from the host endpoint.
+ *
+ * @param pkt
+ *  Pointer to a populated struct channel_packet
+ *
+ * @param lcore_id
+ *  lcore_id.
+ *
+ * @return
+ *  - 0 on success.
+ *  - Negative on error.
+ */
+int power_guest_channel_read_msg(struct channel_packet *pkt,
+		unsigned int lcore_id);
+
+/**
+ * Receive a message contained in pkt over the Virtio-Serial
+ * from the host endpoint.
+ *
+ * @param pkt
+ *  Pointer to a populated struct channel_packet
+ *
+ * @param lcore_id
+ *  lcore_id.
+ *
+ * @return
+ *  - 0 on success.
+ *  - Negative on error.
+ */
+int __rte_experimental
+rte_power_guest_channel_receive_msg(struct channel_packet *pkt,
+			unsigned int lcore_id);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_power/rte_power_version.map b/lib/librte_power/rte_power_version.map
index 042917360..69f5ea3f4 100644
--- a/lib/librte_power/rte_power_version.map
+++ b/lib/librte_power/rte_power_version.map
@@ -44,4 +44,5 @@ EXPERIMENTAL {
 	rte_power_empty_poll_stat_update;
 	rte_power_poll_stat_fetch;
 	rte_power_poll_stat_update;
+	rte_power_guest_channel_receive_msg;
 };
-- 
2.17.2

[dpdk-dev] [PATCH v3 2/4] power: extend guest channel api for reading

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Added new experimental API rte_power_guest_channel_receive_msg
which gives possibility to receive messages send to guest.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 lib/librte_power/channel_commands.h    |  5 ++
 lib/librte_power/guest_channel.c       | 65 ++++++++++++++++++++++++++
 lib/librte_power/guest_channel.h       | 35 ++++++++++++++
 lib/librte_power/rte_power_version.map |  1 +
 4 files changed, 106 insertions(+)

diff --git a/lib/librte_power/channel_commands.h b/lib/librte_power/channel_commands.h
index e7b93a797..33fd53a6d 100644
--- a/lib/librte_power/channel_commands.h
+++ b/lib/librte_power/channel_commands.h
@@ -28,6 +28,11 @@ extern "C" {
 #define CPU_POWER_SCALE_MIN     4
 #define CPU_POWER_ENABLE_TURBO  5
 #define CPU_POWER_DISABLE_TURBO 6
+
+/* Generic Power Command Response */
+#define CPU_POWER_CMD_ACK       1
+#define CPU_POWER_CMD_NACK      2
+
 #define HOURS 24
 
 #define MAX_VFS 10
diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
index 9cf7d2cb2..3213fa769 100644
--- a/lib/librte_power/guest_channel.c
+++ b/lib/librte_power/guest_channel.c
@@ -10,6 +10,7 @@
 #include <fcntl.h>
 #include <string.h>
 #include <errno.h>
+#include <poll.h>
 
 
 #include <rte_log.h>
@@ -19,6 +20,9 @@
 
 #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
 
+/* Timeout for incoming message in milliseconds. */
+#define TIMEOUT 10
+
 static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
 
 int
@@ -125,6 +129,67 @@ int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
 	return guest_channel_send_msg(pkt, lcore_id);
 }
 
+int power_guest_channel_read_msg(struct channel_packet *pkt,
+			unsigned int lcore_id)
+{
+	int ret;
+
+	struct pollfd fds;
+	fds.fd = global_fds[lcore_id];
+	fds.events = POLLIN;
+
+	ret = poll(&fds, 1, TIMEOUT);
+	if (ret == 0) {
+		RTE_LOG(DEBUG, GUEST_CHANNEL, "Timeout occurs during poll function.\n");
+		return -1;
+	} else if (ret < 0) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Error occurred during poll function: %s\n",
+				strerror(ret));
+		return -1;
+	}
+
+	void *buffer = pkt;
+
+	if (lcore_id >= RTE_MAX_LCORE) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
+				lcore_id, RTE_MAX_LCORE-1);
+		return -1;
+	}
+
+	if (global_fds[lcore_id] < 0) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
+		return -1;
+	}
+
+	int buffer_len = sizeof(*pkt);
+
+	while (buffer_len > 0) {
+		ret = read(global_fds[lcore_id],
+				buffer, buffer_len);
+		if (ret < 0) {
+			if (errno == EINTR)
+				continue;
+			return -1;
+		}
+		/* No data has been read, this can indicate not
+		 * consistent buffer_len with actual data size.
+		 */
+		if (ret == 0) {
+			RTE_LOG(ERR, GUEST_CHANNEL, "No more data has been read.\n");
+			return -1;
+		}
+		buffer = (char *)buffer + ret;
+		buffer_len -= ret;
+	}
+
+	return 0;
+}
+
+int rte_power_guest_channel_receive_msg(struct channel_packet *pkt,
+			unsigned int lcore_id)
+{
+	return power_guest_channel_read_msg(pkt, lcore_id);
+}
 
 void
 guest_channel_host_disconnect(unsigned int lcore_id)
diff --git a/lib/librte_power/guest_channel.h b/lib/librte_power/guest_channel.h
index 373d39898..7c385df39 100644
--- a/lib/librte_power/guest_channel.h
+++ b/lib/librte_power/guest_channel.h
@@ -68,6 +68,41 @@ int guest_channel_send_msg(struct channel_packet *pkt, unsigned int lcore_id);
 int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
 			unsigned int lcore_id);
 
+/**
+ * Read a message contained in pkt over the Virtio-Serial
+ * from the host endpoint.
+ *
+ * @param pkt
+ *  Pointer to a populated struct channel_packet
+ *
+ * @param lcore_id
+ *  lcore_id.
+ *
+ * @return
+ *  - 0 on success.
+ *  - Negative on error.
+ */
+int power_guest_channel_read_msg(struct channel_packet *pkt,
+		unsigned int lcore_id);
+
+/**
+ * Receive a message contained in pkt over the Virtio-Serial
+ * from the host endpoint.
+ *
+ * @param pkt
+ *  Pointer to a populated struct channel_packet
+ *
+ * @param lcore_id
+ *  lcore_id.
+ *
+ * @return
+ *  - 0 on success.
+ *  - Negative on error.
+ */
+int __rte_experimental
+rte_power_guest_channel_receive_msg(struct channel_packet *pkt,
+			unsigned int lcore_id);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_power/rte_power_version.map b/lib/librte_power/rte_power_version.map
index 042917360..69f5ea3f4 100644
--- a/lib/librte_power/rte_power_version.map
+++ b/lib/librte_power/rte_power_version.map
@@ -44,4 +44,5 @@ EXPERIMENTAL {
 	rte_power_empty_poll_stat_update;
 	rte_power_poll_stat_fetch;
 	rte_power_poll_stat_update;
+	rte_power_guest_channel_receive_msg;
 };
-- 
2.17.2

[dpdk-dev] [PATCH v3 3/4] power: process incoming confirmation cmds

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Extend vm_power_guest to check incoming confirmations
of messages previously sent to host.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 examples/vm_power_manager/guest_cli/Makefile  |  1 +
 .../guest_cli/vm_power_cli_guest.c            | 65 +++++++++++++++----
 2 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/examples/vm_power_manager/guest_cli/Makefile b/examples/vm_power_manager/guest_cli/Makefile
index a5634eacf..51a5010ab 100644
--- a/examples/vm_power_manager/guest_cli/Makefile
+++ b/examples/vm_power_manager/guest_cli/Makefile
@@ -18,6 +18,7 @@ SRCS-y := main.c vm_power_cli_guest.c parse.c
 
 CFLAGS += -O3 -I$(RTE_SDK)/lib/librte_power/
 CFLAGS += $(WERROR_FLAGS)
+CFLAGS += -DALLOW_EXPERIMENTAL_API
 
 # workaround for a gcc bug with noreturn attribute
 # http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12603
diff --git a/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c b/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
index 2d9e7689a..698dd5062 100644
--- a/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
+++ b/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
@@ -132,6 +132,26 @@ struct cmd_set_cpu_freq_result {
 	cmdline_fixed_string_t cmd;
 };
 
+static int
+check_response_cmd(unsigned int lcore_id, int *result)
+{
+	struct channel_packet pkt;
+	int ret;
+
+	ret = rte_power_guest_channel_receive_msg(&pkt, lcore_id);
+	if (ret < 0)
+		return -1;
+
+	if (pkt.command != CPU_POWER_CMD_ACK &&
+		pkt.command != CPU_POWER_CMD_NACK) {
+		RTE_LOG(DEBUG, POWER, "Not expected command has been received.\n");
+		return -1;
+	}
+
+	*result = (pkt.command == CPU_POWER_CMD_ACK);
+	return 0;
+}
+
 static void
 cmd_set_cpu_freq_parsed(void *parsed_result, struct cmdline *cl,
 		       __attribute__((unused)) void *data)
@@ -139,20 +159,31 @@ cmd_set_cpu_freq_parsed(void *parsed_result, struct cmdline *cl,
 	int ret = -1;
 	struct cmd_set_cpu_freq_result *res = parsed_result;
 
-	if (!strcmp(res->cmd , "up"))
+	if (!strcmp(res->cmd, "up"))
 		ret = rte_power_freq_up(res->lcore_id);
-	else if (!strcmp(res->cmd , "down"))
+	else if (!strcmp(res->cmd, "down"))
 		ret = rte_power_freq_down(res->lcore_id);
-	else if (!strcmp(res->cmd , "min"))
+	else if (!strcmp(res->cmd, "min"))
 		ret = rte_power_freq_min(res->lcore_id);
-	else if (!strcmp(res->cmd , "max"))
+	else if (!strcmp(res->cmd, "max"))
 		ret = rte_power_freq_max(res->lcore_id);
 	else if (!strcmp(res->cmd, "enable_turbo"))
 		ret = rte_power_freq_enable_turbo(res->lcore_id);
 	else if (!strcmp(res->cmd, "disable_turbo"))
 		ret = rte_power_freq_disable_turbo(res->lcore_id);
-	if (ret != 1)
+
+	if (ret != 1) {
 		cmdline_printf(cl, "Error sending message: %s\n", strerror(ret));
+		return;
+	}
+	int result;
+	ret = check_response_cmd(res->lcore_id, &result);
+	if (ret < 0) {
+		RTE_LOG(DEBUG, POWER, "No confirmation for sent msg received\n");
+	} else {
+		cmdline_printf(cl, "Ack for sent msg received with result: %s.\n",
+				result == 1 ? "SUCCESS" : "ERROR");
+	}
 }
 
 cmdline_parse_token_string_t cmd_set_cpu_freq =
@@ -185,16 +216,26 @@ struct cmd_send_policy_result {
 };
 
 static inline int
-send_policy(struct channel_packet *pkt)
+send_policy(struct channel_packet *pkt, struct cmdline *cl)
 {
 	int ret;
 
 	ret = rte_power_guest_channel_send_msg(pkt, 1);
-	if (ret == 0)
-		return 1;
-	RTE_LOG(DEBUG, POWER, "Error sending message: %s\n",
-			ret > 0 ? strerror(ret) : "channel not connected");
-	return -1;
+	if (ret < 0) {
+		RTE_LOG(DEBUG, POWER, "Error sending message: %s\n",
+				ret > 0 ? strerror(ret) : "channel not connected");
+		return -1;
+	}
+
+	int result;
+	ret = check_response_cmd(1, &result);
+	if (ret < 0) {
+		RTE_LOG(DEBUG, POWER, "No confirmation for sent policy received\n");
+	} else {
+		cmdline_printf(cl, "Ack for sent policy received with result: %s.\n",
+				result == 1 ? "SUCCESS" : "ERROR");
+	}
+	return 1;
 }
 
 static void
@@ -206,7 +247,7 @@ cmd_send_policy_parsed(void *parsed_result, struct cmdline *cl,
 
 	if (!strcmp(res->cmd, "now")) {
 		printf("Sending Policy down now!\n");
-		ret = send_policy(&policy);
+		ret = send_policy(&policy, cl);
 	}
 	if (ret != 1)
 		cmdline_printf(cl, "Error sending message: %s\n",
-- 
2.17.2

[dpdk-dev] [PATCH v3 3/4] power: process incoming confirmation cmds

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Extend vm_power_guest to check incoming confirmations
of messages previously sent to host.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 examples/vm_power_manager/guest_cli/Makefile  |  1 +
 .../guest_cli/vm_power_cli_guest.c            | 65 +++++++++++++++----
 2 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/examples/vm_power_manager/guest_cli/Makefile b/examples/vm_power_manager/guest_cli/Makefile
index a5634eacf..51a5010ab 100644
--- a/examples/vm_power_manager/guest_cli/Makefile
+++ b/examples/vm_power_manager/guest_cli/Makefile
@@ -18,6 +18,7 @@ SRCS-y := main.c vm_power_cli_guest.c parse.c
 
 CFLAGS += -O3 -I$(RTE_SDK)/lib/librte_power/
 CFLAGS += $(WERROR_FLAGS)
+CFLAGS += -DALLOW_EXPERIMENTAL_API
 
 # workaround for a gcc bug with noreturn attribute
 # http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12603
diff --git a/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c b/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
index 2d9e7689a..698dd5062 100644
--- a/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
+++ b/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
@@ -132,6 +132,26 @@ struct cmd_set_cpu_freq_result {
 	cmdline_fixed_string_t cmd;
 };
 
+static int
+check_response_cmd(unsigned int lcore_id, int *result)
+{
+	struct channel_packet pkt;
+	int ret;
+
+	ret = rte_power_guest_channel_receive_msg(&pkt, lcore_id);
+	if (ret < 0)
+		return -1;
+
+	if (pkt.command != CPU_POWER_CMD_ACK &&
+		pkt.command != CPU_POWER_CMD_NACK) {
+		RTE_LOG(DEBUG, POWER, "Not expected command has been received.\n");
+		return -1;
+	}
+
+	*result = (pkt.command == CPU_POWER_CMD_ACK);
+	return 0;
+}
+
 static void
 cmd_set_cpu_freq_parsed(void *parsed_result, struct cmdline *cl,
 		       __attribute__((unused)) void *data)
@@ -139,20 +159,31 @@ cmd_set_cpu_freq_parsed(void *parsed_result, struct cmdline *cl,
 	int ret = -1;
 	struct cmd_set_cpu_freq_result *res = parsed_result;
 
-	if (!strcmp(res->cmd , "up"))
+	if (!strcmp(res->cmd, "up"))
 		ret = rte_power_freq_up(res->lcore_id);
-	else if (!strcmp(res->cmd , "down"))
+	else if (!strcmp(res->cmd, "down"))
 		ret = rte_power_freq_down(res->lcore_id);
-	else if (!strcmp(res->cmd , "min"))
+	else if (!strcmp(res->cmd, "min"))
 		ret = rte_power_freq_min(res->lcore_id);
-	else if (!strcmp(res->cmd , "max"))
+	else if (!strcmp(res->cmd, "max"))
 		ret = rte_power_freq_max(res->lcore_id);
 	else if (!strcmp(res->cmd, "enable_turbo"))
 		ret = rte_power_freq_enable_turbo(res->lcore_id);
 	else if (!strcmp(res->cmd, "disable_turbo"))
 		ret = rte_power_freq_disable_turbo(res->lcore_id);
-	if (ret != 1)
+
+	if (ret != 1) {
 		cmdline_printf(cl, "Error sending message: %s\n", strerror(ret));
+		return;
+	}
+	int result;
+	ret = check_response_cmd(res->lcore_id, &result);
+	if (ret < 0) {
+		RTE_LOG(DEBUG, POWER, "No confirmation for sent msg received\n");
+	} else {
+		cmdline_printf(cl, "Ack for sent msg received with result: %s.\n",
+				result == 1 ? "SUCCESS" : "ERROR");
+	}
 }
 
 cmdline_parse_token_string_t cmd_set_cpu_freq =
@@ -185,16 +216,26 @@ struct cmd_send_policy_result {
 };
 
 static inline int
-send_policy(struct channel_packet *pkt)
+send_policy(struct channel_packet *pkt, struct cmdline *cl)
 {
 	int ret;
 
 	ret = rte_power_guest_channel_send_msg(pkt, 1);
-	if (ret == 0)
-		return 1;
-	RTE_LOG(DEBUG, POWER, "Error sending message: %s\n",
-			ret > 0 ? strerror(ret) : "channel not connected");
-	return -1;
+	if (ret < 0) {
+		RTE_LOG(DEBUG, POWER, "Error sending message: %s\n",
+				ret > 0 ? strerror(ret) : "channel not connected");
+		return -1;
+	}
+
+	int result;
+	ret = check_response_cmd(1, &result);
+	if (ret < 0) {
+		RTE_LOG(DEBUG, POWER, "No confirmation for sent policy received\n");
+	} else {
+		cmdline_printf(cl, "Ack for sent policy received with result: %s.\n",
+				result == 1 ? "SUCCESS" : "ERROR");
+	}
+	return 1;
 }
 
 static void
@@ -206,7 +247,7 @@ cmd_send_policy_parsed(void *parsed_result, struct cmdline *cl,
 
 	if (!strcmp(res->cmd, "now")) {
 		printf("Sending Policy down now!\n");
-		ret = send_policy(&policy);
+		ret = send_policy(&policy, cl);
 	}
 	if (ret != 1)
 		cmdline_printf(cl, "Error sending message: %s\n",
-- 
2.17.2

[dpdk-dev] [PATCH v3 4/4] power: send confirmation cmd to vm guest

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Use new guest channel API to send confirmation
message for received power command.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 examples/vm_power_manager/channel_monitor.c | 67 +++++++++++++++++++--
 1 file changed, 61 insertions(+), 6 deletions(-)

diff --git a/examples/vm_power_manager/channel_monitor.c b/examples/vm_power_manager/channel_monitor.c
index 1a3a0fa76..df1dc1205 100644
--- a/examples/vm_power_manager/channel_monitor.c
+++ b/examples/vm_power_manager/channel_monitor.c
@@ -627,6 +627,39 @@ apply_policy(struct policy *pol)
 		apply_workload_profile(pol);
 }
 
+static int
+write_binary_packet(struct channel_packet *pkt, struct channel_info *chan_info)
+{
+	int ret, buffer_len = sizeof(*pkt);
+	void *buffer = pkt;
+
+	if (chan_info->fd == 0) {
+		RTE_LOG(ERR, CHANNEL_MONITOR, "Channel is not connected\n");
+		return -1;
+	}
+
+	while (buffer_len > 0) {
+		ret = write(chan_info->fd, buffer, buffer_len);
+		if (ret == -1) {
+			if (errno == EINTR)
+				continue;
+			return -1;
+		}
+		buffer = (char *)buffer + ret;
+		buffer_len -= ret;
+	}
+	return 0;
+}
+
+static int
+send_ack_for_received_cmd(struct channel_packet *pkt,
+						struct channel_info *chan_info,
+						uint32_t command)
+{
+	pkt->command = command;
+	return write_binary_packet(pkt, chan_info);
+}
+
 static int
 process_request(struct channel_packet *pkt, struct channel_info *chan_info)
 {
@@ -645,33 +678,55 @@ process_request(struct channel_packet *pkt, struct channel_info *chan_info)
 		else
 			core_num = pkt->resource_id;
 
+		bool valid_unit = true;
+		int scale_res;
+
 		switch (pkt->unit) {
 		case(CPU_POWER_SCALE_MIN):
-			power_manager_scale_core_min(core_num);
+			scale_res = power_manager_scale_core_min(core_num);
 			break;
 		case(CPU_POWER_SCALE_MAX):
-			power_manager_scale_core_max(core_num);
+			scale_res = power_manager_scale_core_max(core_num);
 			break;
 		case(CPU_POWER_SCALE_DOWN):
-			power_manager_scale_core_down(core_num);
+			scale_res = power_manager_scale_core_down(core_num);
 			break;
 		case(CPU_POWER_SCALE_UP):
-			power_manager_scale_core_up(core_num);
+			scale_res = power_manager_scale_core_up(core_num);
 			break;
 		case(CPU_POWER_ENABLE_TURBO):
-			power_manager_enable_turbo_core(core_num);
+			scale_res = power_manager_enable_turbo_core(core_num);
 			break;
 		case(CPU_POWER_DISABLE_TURBO):
-			power_manager_disable_turbo_core(core_num);
+			scale_res = power_manager_disable_turbo_core(core_num);
 			break;
 		default:
+			valid_unit = false;
 			break;
 		}
+
+		int ret = -1;
+		if (valid_unit) {
+			ret = send_ack_for_received_cmd(pkt,
+						chan_info,
+						scale_res > 0 ?
+						CPU_POWER_CMD_ACK
+						: CPU_POWER_CMD_NACK);
+			if (ret < 0)
+				RTE_LOG(DEBUG, CHANNEL_MONITOR, "Error during sending ack command.\n");
+		} else
+			RTE_LOG(DEBUG, CHANNEL_MONITOR, "Unexpected unit type.\n");
+
 	}
 
 	if (pkt->command == PKT_POLICY) {
 		RTE_LOG(INFO, CHANNEL_MONITOR, "Processing policy request %s\n",
 				pkt->vm_name);
+		int ret = send_ack_for_received_cmd(pkt,
+						chan_info,
+						CPU_POWER_CMD_ACK);
+		if (ret < 0)
+			RTE_LOG(DEBUG, CHANNEL_MONITOR, "Error during sending ack command.\n");
 		update_policy(pkt);
 		policy_is_set = 1;
 	}
-- 
2.17.2

[dpdk-dev] [PATCH v3 4/4] power: send confirmation cmd to vm guest

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Use new guest channel API to send confirmation
message for received power command.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 examples/vm_power_manager/channel_monitor.c | 67 +++++++++++++++++++--
 1 file changed, 61 insertions(+), 6 deletions(-)

diff --git a/examples/vm_power_manager/channel_monitor.c b/examples/vm_power_manager/channel_monitor.c
index 1a3a0fa76..df1dc1205 100644
--- a/examples/vm_power_manager/channel_monitor.c
+++ b/examples/vm_power_manager/channel_monitor.c
@@ -627,6 +627,39 @@ apply_policy(struct policy *pol)
 		apply_workload_profile(pol);
 }
 
+static int
+write_binary_packet(struct channel_packet *pkt, struct channel_info *chan_info)
+{
+	int ret, buffer_len = sizeof(*pkt);
+	void *buffer = pkt;
+
+	if (chan_info->fd == 0) {
+		RTE_LOG(ERR, CHANNEL_MONITOR, "Channel is not connected\n");
+		return -1;
+	}
+
+	while (buffer_len > 0) {
+		ret = write(chan_info->fd, buffer, buffer_len);
+		if (ret == -1) {
+			if (errno == EINTR)
+				continue;
+			return -1;
+		}
+		buffer = (char *)buffer + ret;
+		buffer_len -= ret;
+	}
+	return 0;
+}
+
+static int
+send_ack_for_received_cmd(struct channel_packet *pkt,
+						struct channel_info *chan_info,
+						uint32_t command)
+{
+	pkt->command = command;
+	return write_binary_packet(pkt, chan_info);
+}
+
 static int
 process_request(struct channel_packet *pkt, struct channel_info *chan_info)
 {
@@ -645,33 +678,55 @@ process_request(struct channel_packet *pkt, struct channel_info *chan_info)
 		else
 			core_num = pkt->resource_id;
 
+		bool valid_unit = true;
+		int scale_res;
+
 		switch (pkt->unit) {
 		case(CPU_POWER_SCALE_MIN):
-			power_manager_scale_core_min(core_num);
+			scale_res = power_manager_scale_core_min(core_num);
 			break;
 		case(CPU_POWER_SCALE_MAX):
-			power_manager_scale_core_max(core_num);
+			scale_res = power_manager_scale_core_max(core_num);
 			break;
 		case(CPU_POWER_SCALE_DOWN):
-			power_manager_scale_core_down(core_num);
+			scale_res = power_manager_scale_core_down(core_num);
 			break;
 		case(CPU_POWER_SCALE_UP):
-			power_manager_scale_core_up(core_num);
+			scale_res = power_manager_scale_core_up(core_num);
 			break;
 		case(CPU_POWER_ENABLE_TURBO):
-			power_manager_enable_turbo_core(core_num);
+			scale_res = power_manager_enable_turbo_core(core_num);
 			break;
 		case(CPU_POWER_DISABLE_TURBO):
-			power_manager_disable_turbo_core(core_num);
+			scale_res = power_manager_disable_turbo_core(core_num);
 			break;
 		default:
+			valid_unit = false;
 			break;
 		}
+
+		int ret = -1;
+		if (valid_unit) {
+			ret = send_ack_for_received_cmd(pkt,
+						chan_info,
+						scale_res > 0 ?
+						CPU_POWER_CMD_ACK
+						: CPU_POWER_CMD_NACK);
+			if (ret < 0)
+				RTE_LOG(DEBUG, CHANNEL_MONITOR, "Error during sending ack command.\n");
+		} else
+			RTE_LOG(DEBUG, CHANNEL_MONITOR, "Unexpected unit type.\n");
+
 	}
 
 	if (pkt->command == PKT_POLICY) {
 		RTE_LOG(INFO, CHANNEL_MONITOR, "Processing policy request %s\n",
 				pkt->vm_name);
+		int ret = send_ack_for_received_cmd(pkt,
+						chan_info,
+						CPU_POWER_CMD_ACK);
+		if (ret < 0)
+			RTE_LOG(DEBUG, CHANNEL_MONITOR, "Error during sending ack command.\n");
 		update_policy(pkt);
 		policy_is_set = 1;
 	}
-- 
2.17.2

Re: [dpdk-dev] [PATCH v3 1/4] power: fix invalid socket indicator value

[Maxime Coquelin]

On 3/21/19 11:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Currently 0 is being used for not connected slot indication.
> This is not consistent with linux doc which identifies 0 as valid
> (connected) slot, thus modification was done to change it.
> 
> Fixes: cd0d5547 ("power: vm communication channels in guest")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   lib/librte_power/guest_channel.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
> index c17ea46b4..9cf7d2cb2 100644
> --- a/lib/librte_power/guest_channel.c
> +++ b/lib/librte_power/guest_channel.c
> @@ -19,7 +19,7 @@
>   
>   #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
>   
> -static int global_fds[RTE_MAX_LCORE];
> +static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
>   
>   int
>   guest_channel_host_connect(const char *path, unsigned int lcore_id)
> @@ -35,7 +35,7 @@ guest_channel_host_connect(const char *path, unsigned int lcore_id)
>   		return -1;
>   	}
>   	/* check if path is already open */
> -	if (global_fds[lcore_id] != 0) {
> +	if (global_fds[lcore_id] != -1) {
>   		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is already open with fd %d\n",
>   				lcore_id, global_fds[lcore_id]);
>   		return -1;
> @@ -84,7 +84,7 @@ guest_channel_host_connect(const char *path, unsigned int lcore_id)
>   	return 0;
>   error:
>   	close(fd);
> -	global_fds[lcore_id] = 0;
> +	global_fds[lcore_id] = -1;
>   	return -1;
>   }
>   
> @@ -100,7 +100,7 @@ guest_channel_send_msg(struct channel_packet *pkt, unsigned int lcore_id)
>   		return -1;
>   	}
>   
> -	if (global_fds[lcore_id] == 0) {
> +	if (global_fds[lcore_id] < 0) {
>   		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
>   		return -1;
>   	}
> @@ -134,8 +134,8 @@ guest_channel_host_disconnect(unsigned int lcore_id)
>   				lcore_id, RTE_MAX_LCORE-1);
>   		return;
>   	}
> -	if (global_fds[lcore_id] == 0)
> +	if (global_fds[lcore_id] < 0)
>   		return;
>   	close(global_fds[lcore_id]);
> -	global_fds[lcore_id] = 0;
> +	global_fds[lcore_id] = -1;
>   }
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Re: [dpdk-dev] [PATCH v3 1/4] power: fix invalid socket indicator value

[Maxime Coquelin]

On 3/21/19 11:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Currently 0 is being used for not connected slot indication.
> This is not consistent with linux doc which identifies 0 as valid
> (connected) slot, thus modification was done to change it.
> 
> Fixes: cd0d5547 ("power: vm communication channels in guest")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   lib/librte_power/guest_channel.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
> index c17ea46b4..9cf7d2cb2 100644
> --- a/lib/librte_power/guest_channel.c
> +++ b/lib/librte_power/guest_channel.c
> @@ -19,7 +19,7 @@
>   
>   #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
>   
> -static int global_fds[RTE_MAX_LCORE];
> +static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
>   
>   int
>   guest_channel_host_connect(const char *path, unsigned int lcore_id)
> @@ -35,7 +35,7 @@ guest_channel_host_connect(const char *path, unsigned int lcore_id)
>   		return -1;
>   	}
>   	/* check if path is already open */
> -	if (global_fds[lcore_id] != 0) {
> +	if (global_fds[lcore_id] != -1) {
>   		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is already open with fd %d\n",
>   				lcore_id, global_fds[lcore_id]);
>   		return -1;
> @@ -84,7 +84,7 @@ guest_channel_host_connect(const char *path, unsigned int lcore_id)
>   	return 0;
>   error:
>   	close(fd);
> -	global_fds[lcore_id] = 0;
> +	global_fds[lcore_id] = -1;
>   	return -1;
>   }
>   
> @@ -100,7 +100,7 @@ guest_channel_send_msg(struct channel_packet *pkt, unsigned int lcore_id)
>   		return -1;
>   	}
>   
> -	if (global_fds[lcore_id] == 0) {
> +	if (global_fds[lcore_id] < 0) {
>   		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
>   		return -1;
>   	}
> @@ -134,8 +134,8 @@ guest_channel_host_disconnect(unsigned int lcore_id)
>   				lcore_id, RTE_MAX_LCORE-1);
>   		return;
>   	}
> -	if (global_fds[lcore_id] == 0)
> +	if (global_fds[lcore_id] < 0)
>   		return;
>   	close(global_fds[lcore_id]);
> -	global_fds[lcore_id] = 0;
> +	global_fds[lcore_id] = -1;
>   }
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Re: [dpdk-dev] [PATCH v3 2/4] power: extend guest channel api for reading

[Maxime Coquelin]

On 3/21/19 11:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Added new experimental API rte_power_guest_channel_receive_msg
> which gives possibility to receive messages send to guest.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   lib/librte_power/channel_commands.h    |  5 ++
>   lib/librte_power/guest_channel.c       | 65 ++++++++++++++++++++++++++
>   lib/librte_power/guest_channel.h       | 35 ++++++++++++++
>   lib/librte_power/rte_power_version.map |  1 +
>   4 files changed, 106 insertions(+)
> 
> diff --git a/lib/librte_power/channel_commands.h b/lib/librte_power/channel_commands.h
> index e7b93a797..33fd53a6d 100644
> --- a/lib/librte_power/channel_commands.h
> +++ b/lib/librte_power/channel_commands.h
> @@ -28,6 +28,11 @@ extern "C" {
>   #define CPU_POWER_SCALE_MIN     4
>   #define CPU_POWER_ENABLE_TURBO  5
>   #define CPU_POWER_DISABLE_TURBO 6
> +
> +/* Generic Power Command Response */
> +#define CPU_POWER_CMD_ACK       1
> +#define CPU_POWER_CMD_NACK      2
> +
>   #define HOURS 24
>   
>   #define MAX_VFS 10
> diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
> index 9cf7d2cb2..3213fa769 100644
> --- a/lib/librte_power/guest_channel.c
> +++ b/lib/librte_power/guest_channel.c
> @@ -10,6 +10,7 @@
>   #include <fcntl.h>
>   #include <string.h>
>   #include <errno.h>
> +#include <poll.h>
>   
>   
>   #include <rte_log.h>
> @@ -19,6 +20,9 @@
>   
>   #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
>   
> +/* Timeout for incoming message in milliseconds. */
> +#define TIMEOUT 10
> +
>   static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
>   
>   int
> @@ -125,6 +129,67 @@ int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
>   	return guest_channel_send_msg(pkt, lcore_id);
>   }
>   
> +int power_guest_channel_read_msg(struct channel_packet *pkt,
> +			unsigned int lcore_id)
> +{
> +	int ret;
> +
> +	struct pollfd fds;
> +	fds.fd = global_fds[lcore_id];
> +	fds.events = POLLIN;
> +
> +	ret = poll(&fds, 1, TIMEOUT);
> +	if (ret == 0) {
> +		RTE_LOG(DEBUG, GUEST_CHANNEL, "Timeout occurs during poll function.\n");
> +		return -1;
> +	} else if (ret < 0) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Error occurred during poll function: %s\n",
> +				strerror(ret));
> +		return -1;
> +	}
> +
> +	void *buffer = pkt;

I would prefer it to be declared at the beginning of the function,
but at least, declare it just before the while loop.

> +
> +	if (lcore_id >= RTE_MAX_LCORE) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
> +				lcore_id, RTE_MAX_LCORE-1);
> +		return -1;
> +	}
> +
> +	if (global_fds[lcore_id] < 0) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
> +		return -1;
> +	}
> +
> +	int buffer_len = sizeof(*pkt);
> +
> +	while (buffer_len > 0) {
> +		ret = read(global_fds[lcore_id],
> +				buffer, buffer_len);
> +		if (ret < 0) {
> +			if (errno == EINTR)
> +				continue;
> +			return -1;
> +		}
> +		/* No data has been read, this can indicate not
> +		 * consistent buffer_len with actual data size.
> +		 */
> +		if (ret == 0) {
> +			RTE_LOG(ERR, GUEST_CHANNEL, "No more data has been read.\n");
> +			return -1;
> +		}
> +		buffer = (char *)buffer + ret;
> +		buffer_len -= ret;
> +	}
> +
> +	return 0;
> +}
> +

Re: [dpdk-dev] [PATCH v3 2/4] power: extend guest channel api for reading

[Maxime Coquelin]

On 3/21/19 11:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Added new experimental API rte_power_guest_channel_receive_msg
> which gives possibility to receive messages send to guest.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   lib/librte_power/channel_commands.h    |  5 ++
>   lib/librte_power/guest_channel.c       | 65 ++++++++++++++++++++++++++
>   lib/librte_power/guest_channel.h       | 35 ++++++++++++++
>   lib/librte_power/rte_power_version.map |  1 +
>   4 files changed, 106 insertions(+)
> 
> diff --git a/lib/librte_power/channel_commands.h b/lib/librte_power/channel_commands.h
> index e7b93a797..33fd53a6d 100644
> --- a/lib/librte_power/channel_commands.h
> +++ b/lib/librte_power/channel_commands.h
> @@ -28,6 +28,11 @@ extern "C" {
>   #define CPU_POWER_SCALE_MIN     4
>   #define CPU_POWER_ENABLE_TURBO  5
>   #define CPU_POWER_DISABLE_TURBO 6
> +
> +/* Generic Power Command Response */
> +#define CPU_POWER_CMD_ACK       1
> +#define CPU_POWER_CMD_NACK      2
> +
>   #define HOURS 24
>   
>   #define MAX_VFS 10
> diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
> index 9cf7d2cb2..3213fa769 100644
> --- a/lib/librte_power/guest_channel.c
> +++ b/lib/librte_power/guest_channel.c
> @@ -10,6 +10,7 @@
>   #include <fcntl.h>
>   #include <string.h>
>   #include <errno.h>
> +#include <poll.h>
>   
>   
>   #include <rte_log.h>
> @@ -19,6 +20,9 @@
>   
>   #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
>   
> +/* Timeout for incoming message in milliseconds. */
> +#define TIMEOUT 10
> +
>   static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
>   
>   int
> @@ -125,6 +129,67 @@ int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
>   	return guest_channel_send_msg(pkt, lcore_id);
>   }
>   
> +int power_guest_channel_read_msg(struct channel_packet *pkt,
> +			unsigned int lcore_id)
> +{
> +	int ret;
> +
> +	struct pollfd fds;
> +	fds.fd = global_fds[lcore_id];
> +	fds.events = POLLIN;
> +
> +	ret = poll(&fds, 1, TIMEOUT);
> +	if (ret == 0) {
> +		RTE_LOG(DEBUG, GUEST_CHANNEL, "Timeout occurs during poll function.\n");
> +		return -1;
> +	} else if (ret < 0) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Error occurred during poll function: %s\n",
> +				strerror(ret));
> +		return -1;
> +	}
> +
> +	void *buffer = pkt;

I would prefer it to be declared at the beginning of the function,
but at least, declare it just before the while loop.

> +
> +	if (lcore_id >= RTE_MAX_LCORE) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
> +				lcore_id, RTE_MAX_LCORE-1);
> +		return -1;
> +	}
> +
> +	if (global_fds[lcore_id] < 0) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
> +		return -1;
> +	}
> +
> +	int buffer_len = sizeof(*pkt);
> +
> +	while (buffer_len > 0) {
> +		ret = read(global_fds[lcore_id],
> +				buffer, buffer_len);
> +		if (ret < 0) {
> +			if (errno == EINTR)
> +				continue;
> +			return -1;
> +		}
> +		/* No data has been read, this can indicate not
> +		 * consistent buffer_len with actual data size.
> +		 */
> +		if (ret == 0) {
> +			RTE_LOG(ERR, GUEST_CHANNEL, "No more data has been read.\n");
> +			return -1;
> +		}
> +		buffer = (char *)buffer + ret;
> +		buffer_len -= ret;
> +	}
> +
> +	return 0;
> +}
> +

Re: [dpdk-dev] [PATCH v3 2/4] power: extend guest channel api for reading

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
> Sent: Friday, March 22, 2019 11:00 AM
> To: Hajkowski, MarcinX <marcinx.hajkowski@intel.com>; Hunt, David <david.hunt@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/4] power: extend guest channel api for reading
> 
> 
> 
> On 3/21/19 11:55 AM, Hajkowski wrote:
> > From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> >
> > Added new experimental API rte_power_guest_channel_receive_msg
> > which gives possibility to receive messages send to guest.
> >
> > Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> > ---
> >   lib/librte_power/channel_commands.h    |  5 ++
> >   lib/librte_power/guest_channel.c       | 65 ++++++++++++++++++++++++++
> >   lib/librte_power/guest_channel.h       | 35 ++++++++++++++
> >   lib/librte_power/rte_power_version.map |  1 +
> >   4 files changed, 106 insertions(+)
> >
> > diff --git a/lib/librte_power/channel_commands.h b/lib/librte_power/channel_commands.h
> > index e7b93a797..33fd53a6d 100644
> > --- a/lib/librte_power/channel_commands.h
> > +++ b/lib/librte_power/channel_commands.h
> > @@ -28,6 +28,11 @@ extern "C" {
> >   #define CPU_POWER_SCALE_MIN     4
> >   #define CPU_POWER_ENABLE_TURBO  5
> >   #define CPU_POWER_DISABLE_TURBO 6
> > +
> > +/* Generic Power Command Response */
> > +#define CPU_POWER_CMD_ACK       1
> > +#define CPU_POWER_CMD_NACK      2
> > +
> >   #define HOURS 24
> >
> >   #define MAX_VFS 10
> > diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
> > index 9cf7d2cb2..3213fa769 100644
> > --- a/lib/librte_power/guest_channel.c
> > +++ b/lib/librte_power/guest_channel.c
> > @@ -10,6 +10,7 @@
> >   #include <fcntl.h>
> >   #include <string.h>
> >   #include <errno.h>
> > +#include <poll.h>
> >
> >
> >   #include <rte_log.h>
> > @@ -19,6 +20,9 @@
> >
> >   #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
> >
> > +/* Timeout for incoming message in milliseconds. */
> > +#define TIMEOUT 10
> > +
> >   static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
> >
> >   int
> > @@ -125,6 +129,67 @@ int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
> >   	return guest_channel_send_msg(pkt, lcore_id);
> >   }
> >
> > +int power_guest_channel_read_msg(struct channel_packet *pkt,
> > +			unsigned int lcore_id)
> > +{
> > +	int ret;
> > +
> > +	struct pollfd fds;
> > +	fds.fd = global_fds[lcore_id];
> > +	fds.events = POLLIN;
> > +
> > +	ret = poll(&fds, 1, TIMEOUT);
> > +	if (ret == 0) {
> > +		RTE_LOG(DEBUG, GUEST_CHANNEL, "Timeout occurs during poll function.\n");
> > +		return -1;
> > +	} else if (ret < 0) {
> > +		RTE_LOG(ERR, GUEST_CHANNEL, "Error occurred during poll function: %s\n",
> > +				strerror(ret));
> > +		return -1;
> > +	}
> > +
> > +	void *buffer = pkt;
> 
> I would prefer it to be declared at the beginning of the function,

+1 here and in other places.
please follow dpdk coding style convention.

> but at least, declare it just before the while loop.
> 
> > +
> > +	if (lcore_id >= RTE_MAX_LCORE) {
> > +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
> > +				lcore_id, RTE_MAX_LCORE-1);
> > +		return -1;
> > +	}
> > +
> > +	if (global_fds[lcore_id] < 0) {
> > +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
> > +		return -1;
> > +	}
> > +
> > +	int buffer_len = sizeof(*pkt);
> > +
> > +	while (buffer_len > 0) {
> > +		ret = read(global_fds[lcore_id],
> > +				buffer, buffer_len);
> > +		if (ret < 0) {
> > +			if (errno == EINTR)
> > +				continue;
> > +			return -1;
> > +		}
> > +		/* No data has been read, this can indicate not
> > +		 * consistent buffer_len with actual data size.
> > +		 */
> > +		if (ret == 0) {
> > +			RTE_LOG(ERR, GUEST_CHANNEL, "No more data has been read.\n");
> > +			return -1;
> > +		}
> > +		buffer = (char *)buffer + ret;
> > +		buffer_len -= ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +

Re: [dpdk-dev] [PATCH v3 2/4] power: extend guest channel api for reading

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
> Sent: Friday, March 22, 2019 11:00 AM
> To: Hajkowski, MarcinX <marcinx.hajkowski@intel.com>; Hunt, David <david.hunt@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/4] power: extend guest channel api for reading
> 
> 
> 
> On 3/21/19 11:55 AM, Hajkowski wrote:
> > From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> >
> > Added new experimental API rte_power_guest_channel_receive_msg
> > which gives possibility to receive messages send to guest.
> >
> > Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> > ---
> >   lib/librte_power/channel_commands.h    |  5 ++
> >   lib/librte_power/guest_channel.c       | 65 ++++++++++++++++++++++++++
> >   lib/librte_power/guest_channel.h       | 35 ++++++++++++++
> >   lib/librte_power/rte_power_version.map |  1 +
> >   4 files changed, 106 insertions(+)
> >
> > diff --git a/lib/librte_power/channel_commands.h b/lib/librte_power/channel_commands.h
> > index e7b93a797..33fd53a6d 100644
> > --- a/lib/librte_power/channel_commands.h
> > +++ b/lib/librte_power/channel_commands.h
> > @@ -28,6 +28,11 @@ extern "C" {
> >   #define CPU_POWER_SCALE_MIN     4
> >   #define CPU_POWER_ENABLE_TURBO  5
> >   #define CPU_POWER_DISABLE_TURBO 6
> > +
> > +/* Generic Power Command Response */
> > +#define CPU_POWER_CMD_ACK       1
> > +#define CPU_POWER_CMD_NACK      2
> > +
> >   #define HOURS 24
> >
> >   #define MAX_VFS 10
> > diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
> > index 9cf7d2cb2..3213fa769 100644
> > --- a/lib/librte_power/guest_channel.c
> > +++ b/lib/librte_power/guest_channel.c
> > @@ -10,6 +10,7 @@
> >   #include <fcntl.h>
> >   #include <string.h>
> >   #include <errno.h>
> > +#include <poll.h>
> >
> >
> >   #include <rte_log.h>
> > @@ -19,6 +20,9 @@
> >
> >   #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
> >
> > +/* Timeout for incoming message in milliseconds. */
> > +#define TIMEOUT 10
> > +
> >   static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
> >
> >   int
> > @@ -125,6 +129,67 @@ int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
> >   	return guest_channel_send_msg(pkt, lcore_id);
> >   }
> >
> > +int power_guest_channel_read_msg(struct channel_packet *pkt,
> > +			unsigned int lcore_id)
> > +{
> > +	int ret;
> > +
> > +	struct pollfd fds;
> > +	fds.fd = global_fds[lcore_id];
> > +	fds.events = POLLIN;
> > +
> > +	ret = poll(&fds, 1, TIMEOUT);
> > +	if (ret == 0) {
> > +		RTE_LOG(DEBUG, GUEST_CHANNEL, "Timeout occurs during poll function.\n");
> > +		return -1;
> > +	} else if (ret < 0) {
> > +		RTE_LOG(ERR, GUEST_CHANNEL, "Error occurred during poll function: %s\n",
> > +				strerror(ret));
> > +		return -1;
> > +	}
> > +
> > +	void *buffer = pkt;
> 
> I would prefer it to be declared at the beginning of the function,

+1 here and in other places.
please follow dpdk coding style convention.

> but at least, declare it just before the while loop.
> 
> > +
> > +	if (lcore_id >= RTE_MAX_LCORE) {
> > +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
> > +				lcore_id, RTE_MAX_LCORE-1);
> > +		return -1;
> > +	}
> > +
> > +	if (global_fds[lcore_id] < 0) {
> > +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
> > +		return -1;
> > +	}
> > +
> > +	int buffer_len = sizeof(*pkt);
> > +
> > +	while (buffer_len > 0) {
> > +		ret = read(global_fds[lcore_id],
> > +				buffer, buffer_len);
> > +		if (ret < 0) {
> > +			if (errno == EINTR)
> > +				continue;
> > +			return -1;
> > +		}
> > +		/* No data has been read, this can indicate not
> > +		 * consistent buffer_len with actual data size.
> > +		 */
> > +		if (ret == 0) {
> > +			RTE_LOG(ERR, GUEST_CHANNEL, "No more data has been read.\n");
> > +			return -1;
> > +		}
> > +		buffer = (char *)buffer + ret;
> > +		buffer_len -= ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +

Re: [dpdk-dev] [PATCH v3 1/4] power: fix invalid socket indicator value

[Burakov, Anatoly]

On 21-Mar-19 10:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Currently 0 is being used for not connected slot indication.
> This is not consistent with linux doc which identifies 0 as valid
> (connected) slot, thus modification was done to change it.
> 
> Fixes: cd0d5547 ("power: vm communication channels in guest")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   lib/librte_power/guest_channel.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
> index c17ea46b4..9cf7d2cb2 100644
> --- a/lib/librte_power/guest_channel.c
> +++ b/lib/librte_power/guest_channel.c

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3 1/4] power: fix invalid socket indicator value

[Burakov, Anatoly]

On 21-Mar-19 10:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Currently 0 is being used for not connected slot indication.
> This is not consistent with linux doc which identifies 0 as valid
> (connected) slot, thus modification was done to change it.
> 
> Fixes: cd0d5547 ("power: vm communication channels in guest")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   lib/librte_power/guest_channel.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
> index c17ea46b4..9cf7d2cb2 100644
> --- a/lib/librte_power/guest_channel.c
> +++ b/lib/librte_power/guest_channel.c

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3 2/4] power: extend guest channel api for reading

[Burakov, Anatoly]

On 21-Mar-19 10:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Added new experimental API rte_power_guest_channel_receive_msg
> which gives possibility to receive messages send to guest.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   lib/librte_power/channel_commands.h    |  5 ++
>   lib/librte_power/guest_channel.c       | 65 ++++++++++++++++++++++++++
>   lib/librte_power/guest_channel.h       | 35 ++++++++++++++
>   lib/librte_power/rte_power_version.map |  1 +
>   4 files changed, 106 insertions(+)
> 
> diff --git a/lib/librte_power/channel_commands.h b/lib/librte_power/channel_commands.h
> index e7b93a797..33fd53a6d 100644
> --- a/lib/librte_power/channel_commands.h
> +++ b/lib/librte_power/channel_commands.h
> @@ -28,6 +28,11 @@ extern "C" {
>   #define CPU_POWER_SCALE_MIN     4
>   #define CPU_POWER_ENABLE_TURBO  5
>   #define CPU_POWER_DISABLE_TURBO 6
> +
> +/* Generic Power Command Response */
> +#define CPU_POWER_CMD_ACK       1
> +#define CPU_POWER_CMD_NACK      2
> +
>   #define HOURS 24
>   
>   #define MAX_VFS 10
> diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
> index 9cf7d2cb2..3213fa769 100644
> --- a/lib/librte_power/guest_channel.c
> +++ b/lib/librte_power/guest_channel.c
> @@ -10,6 +10,7 @@
>   #include <fcntl.h>
>   #include <string.h>
>   #include <errno.h>
> +#include <poll.h>
>   
>   
>   #include <rte_log.h>
> @@ -19,6 +20,9 @@
>   
>   #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
>   
> +/* Timeout for incoming message in milliseconds. */
> +#define TIMEOUT 10
> +
>   static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
>   
>   int
> @@ -125,6 +129,67 @@ int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
>   	return guest_channel_send_msg(pkt, lcore_id);
>   }
>   
> +int power_guest_channel_read_msg(struct channel_packet *pkt,
> +			unsigned int lcore_id)
> +{
> +	int ret;
> +
> +	struct pollfd fds;

Nitpick, but we generally prefer having a newline after definitions.

> +	fds.fd = global_fds[lcore_id];
> +	fds.events = POLLIN;
> +
> +	ret = poll(&fds, 1, TIMEOUT);
> +	if (ret == 0) {
> +		RTE_LOG(DEBUG, GUEST_CHANNEL, "Timeout occurs during poll function.\n");

Occurred?

> +		return -1;
> +	} else if (ret < 0) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Error occurred during poll function: %s\n",
> +				strerror(ret));
> +		return -1;
> +	}
> +
> +	void *buffer = pkt;

Declarations have to be in the beginning of the function.

> +
> +	if (lcore_id >= RTE_MAX_LCORE) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
> +				lcore_id, RTE_MAX_LCORE-1);
> +		return -1;
> +	}
> +
> +	if (global_fds[lcore_id] < 0) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
> +		return -1;
> +	}
> +
> +	int buffer_len = sizeof(*pkt);

Same, declarations on top.

> +
> +	while (buffer_len > 0) {
> +		ret = read(global_fds[lcore_id],
> +				buffer, buffer_len);
> +		if (ret < 0) {
> +			if (errno == EINTR)
> +				continue;
> +			return -1;
> +		}
> +		/* No data has been read, this can indicate not
> +		 * consistent buffer_len with actual data size.
> +		 */
> +		if (ret == 0) {
> +			RTE_LOG(ERR, GUEST_CHANNEL, "No more data has been read.\n");
> +			return -1;
> +		}

If i remember correctly, this cannot happen with a blocking fifo read 
unless the fifo was closed either by us or by the other end. Perhaps the 
error message should be reworded to reflect that? E.g. something like 
"Expected more data, but fifo is closed"?

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3 2/4] power: extend guest channel api for reading

[Burakov, Anatoly]

On 21-Mar-19 10:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Added new experimental API rte_power_guest_channel_receive_msg
> which gives possibility to receive messages send to guest.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   lib/librte_power/channel_commands.h    |  5 ++
>   lib/librte_power/guest_channel.c       | 65 ++++++++++++++++++++++++++
>   lib/librte_power/guest_channel.h       | 35 ++++++++++++++
>   lib/librte_power/rte_power_version.map |  1 +
>   4 files changed, 106 insertions(+)
> 
> diff --git a/lib/librte_power/channel_commands.h b/lib/librte_power/channel_commands.h
> index e7b93a797..33fd53a6d 100644
> --- a/lib/librte_power/channel_commands.h
> +++ b/lib/librte_power/channel_commands.h
> @@ -28,6 +28,11 @@ extern "C" {
>   #define CPU_POWER_SCALE_MIN     4
>   #define CPU_POWER_ENABLE_TURBO  5
>   #define CPU_POWER_DISABLE_TURBO 6
> +
> +/* Generic Power Command Response */
> +#define CPU_POWER_CMD_ACK       1
> +#define CPU_POWER_CMD_NACK      2
> +
>   #define HOURS 24
>   
>   #define MAX_VFS 10
> diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
> index 9cf7d2cb2..3213fa769 100644
> --- a/lib/librte_power/guest_channel.c
> +++ b/lib/librte_power/guest_channel.c
> @@ -10,6 +10,7 @@
>   #include <fcntl.h>
>   #include <string.h>
>   #include <errno.h>
> +#include <poll.h>
>   
>   
>   #include <rte_log.h>
> @@ -19,6 +20,9 @@
>   
>   #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
>   
> +/* Timeout for incoming message in milliseconds. */
> +#define TIMEOUT 10
> +
>   static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
>   
>   int
> @@ -125,6 +129,67 @@ int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
>   	return guest_channel_send_msg(pkt, lcore_id);
>   }
>   
> +int power_guest_channel_read_msg(struct channel_packet *pkt,
> +			unsigned int lcore_id)
> +{
> +	int ret;
> +
> +	struct pollfd fds;

Nitpick, but we generally prefer having a newline after definitions.

> +	fds.fd = global_fds[lcore_id];
> +	fds.events = POLLIN;
> +
> +	ret = poll(&fds, 1, TIMEOUT);
> +	if (ret == 0) {
> +		RTE_LOG(DEBUG, GUEST_CHANNEL, "Timeout occurs during poll function.\n");

Occurred?

> +		return -1;
> +	} else if (ret < 0) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Error occurred during poll function: %s\n",
> +				strerror(ret));
> +		return -1;
> +	}
> +
> +	void *buffer = pkt;

Declarations have to be in the beginning of the function.

> +
> +	if (lcore_id >= RTE_MAX_LCORE) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
> +				lcore_id, RTE_MAX_LCORE-1);
> +		return -1;
> +	}
> +
> +	if (global_fds[lcore_id] < 0) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
> +		return -1;
> +	}
> +
> +	int buffer_len = sizeof(*pkt);

Same, declarations on top.

> +
> +	while (buffer_len > 0) {
> +		ret = read(global_fds[lcore_id],
> +				buffer, buffer_len);
> +		if (ret < 0) {
> +			if (errno == EINTR)
> +				continue;
> +			return -1;
> +		}
> +		/* No data has been read, this can indicate not
> +		 * consistent buffer_len with actual data size.
> +		 */
> +		if (ret == 0) {
> +			RTE_LOG(ERR, GUEST_CHANNEL, "No more data has been read.\n");
> +			return -1;
> +		}

If i remember correctly, this cannot happen with a blocking fifo read 
unless the fifo was closed either by us or by the other end. Perhaps the 
error message should be reworded to reflect that? E.g. something like 
"Expected more data, but fifo is closed"?

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3 3/4] power: process incoming confirmation cmds

[Burakov, Anatoly]

On 21-Mar-19 10:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Extend vm_power_guest to check incoming confirmations
> of messages previously sent to host.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   examples/vm_power_manager/guest_cli/Makefile  |  1 +
>   .../guest_cli/vm_power_cli_guest.c            | 65 +++++++++++++++----
>   2 files changed, 54 insertions(+), 12 deletions(-)
> 
> diff --git a/examples/vm_power_manager/guest_cli/Makefile b/examples/vm_power_manager/guest_cli/Makefile
> index a5634eacf..51a5010ab 100644
> --- a/examples/vm_power_manager/guest_cli/Makefile
> +++ b/examples/vm_power_manager/guest_cli/Makefile
> @@ -18,6 +18,7 @@ SRCS-y := main.c vm_power_cli_guest.c parse.c
>   
>   CFLAGS += -O3 -I$(RTE_SDK)/lib/librte_power/
>   CFLAGS += $(WERROR_FLAGS)
> +CFLAGS += -DALLOW_EXPERIMENTAL_API
>   
>   # workaround for a gcc bug with noreturn attribute
>   # http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12603
> diff --git a/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c b/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
> index 2d9e7689a..698dd5062 100644
> --- a/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
> +++ b/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
> @@ -132,6 +132,26 @@ struct cmd_set_cpu_freq_result {
>   	cmdline_fixed_string_t cmd;
>   };
>   
> +static int
> +check_response_cmd(unsigned int lcore_id, int *result)
> +{
> +	struct channel_packet pkt;
> +	int ret;
> +
> +	ret = rte_power_guest_channel_receive_msg(&pkt, lcore_id);
> +	if (ret < 0)
> +		return -1;
> +
> +	if (pkt.command != CPU_POWER_CMD_ACK &&
> +		pkt.command != CPU_POWER_CMD_NACK) {
> +		RTE_LOG(DEBUG, POWER, "Not expected command has been received.\n");
> +		return -1;
> +	}

Maybe use switch?

E.g.

swithc (pkt.command) {
case ACK:
case NACK:
     *result = blah;
     break;
default:
     log failure;
     fail;
}

> +
> +	*result = (pkt.command == CPU_POWER_CMD_ACK);
> +	return 0;
> +}
> +
>   static void
>   cmd_set_cpu_freq_parsed(void *parsed_result, struct cmdline *cl,
>   		       __attribute__((unused)) void *data)
> @@ -139,20 +159,31 @@ cmd_set_cpu_freq_parsed(void *parsed_result, struct cmdline *cl,
>   	int ret = -1;
>   	struct cmd_set_cpu_freq_result *res = parsed_result;
>   
> -	if (!strcmp(res->cmd , "up"))
> +	if (!strcmp(res->cmd, "up"))
>   		ret = rte_power_freq_up(res->lcore_id);
> -	else if (!strcmp(res->cmd , "down"))
> +	else if (!strcmp(res->cmd, "down"))
>   		ret = rte_power_freq_down(res->lcore_id);
> -	else if (!strcmp(res->cmd , "min"))
> +	else if (!strcmp(res->cmd, "min"))
>   		ret = rte_power_freq_min(res->lcore_id);
> -	else if (!strcmp(res->cmd , "max"))
> +	else if (!strcmp(res->cmd, "max"))
>   		ret = rte_power_freq_max(res->lcore_id);
>   	else if (!strcmp(res->cmd, "enable_turbo"))
>   		ret = rte_power_freq_enable_turbo(res->lcore_id);
>   	else if (!strcmp(res->cmd, "disable_turbo"))
>   		ret = rte_power_freq_disable_turbo(res->lcore_id);
> -	if (ret != 1)
> +
> +	if (ret != 1) {
>   		cmdline_printf(cl, "Error sending message: %s\n", strerror(ret));
> +		return;
> +	}
> +	int result;
> +	ret = check_response_cmd(res->lcore_id, &result);
> +	if (ret < 0) {
> +		RTE_LOG(DEBUG, POWER, "No confirmation for sent msg received\n");
> +	} else {
> +		cmdline_printf(cl, "Ack for sent msg received with result: %s.\n",
> +				result == 1 ? "SUCCESS" : "ERROR");

Here and in other places: others can correct me if i'm wrong, but i'm 
pretty sure RTE_LOG(..., POWER) is reserved for logs related to power 
library, and are not supposed to be used in user applications. If 
logging in an application is required, there are USER log levels.

I can see that the app in question already does that in a few places, so 
it's kind of consistent with how things are already, but i would 
question the value of consistency in cases like these. Not sure how to 
proceed here.

> +	}
>   }
>   
>   cmdline_parse_token_string_t cmd_set_cpu_freq =
> @@ -185,16 +216,26 @@ struct cmd_send_policy_result {
>   };
>   
>   static inline int
> -send_policy(struct channel_packet *pkt)
> +send_policy(struct channel_packet *pkt, struct cmdline *cl)
>   {
>   	int ret;
>   
>   	ret = rte_power_guest_channel_send_msg(pkt, 1);
> -	if (ret == 0)
> -		return 1;
> -	RTE_LOG(DEBUG, POWER, "Error sending message: %s\n",
> -			ret > 0 ? strerror(ret) : "channel not connected");
> -	return -1;
> +	if (ret < 0) {
> +		RTE_LOG(DEBUG, POWER, "Error sending message: %s\n",
> +				ret > 0 ? strerror(ret) : "channel not connected");
> +		return -1;
> +	}
> +
> +	int result;
> +	ret = check_response_cmd(1, &result);
> +	if (ret < 0) {
> +		RTE_LOG(DEBUG, POWER, "No confirmation for sent policy received\n");
> +	} else {
> +		cmdline_printf(cl, "Ack for sent policy received with result: %s.\n",
> +				result == 1 ? "SUCCESS" : "ERROR");
> +	}
> +	return 1;
>   }
>   
>   static void
> @@ -206,7 +247,7 @@ cmd_send_policy_parsed(void *parsed_result, struct cmdline *cl,
>   
>   	if (!strcmp(res->cmd, "now")) {
>   		printf("Sending Policy down now!\n");
> -		ret = send_policy(&policy);
> +		ret = send_policy(&policy, cl);
>   	}
>   	if (ret != 1)
>   		cmdline_printf(cl, "Error sending message: %s\n",
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3 3/4] power: process incoming confirmation cmds

[Burakov, Anatoly]

On 21-Mar-19 10:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Extend vm_power_guest to check incoming confirmations
> of messages previously sent to host.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   examples/vm_power_manager/guest_cli/Makefile  |  1 +
>   .../guest_cli/vm_power_cli_guest.c            | 65 +++++++++++++++----
>   2 files changed, 54 insertions(+), 12 deletions(-)
> 
> diff --git a/examples/vm_power_manager/guest_cli/Makefile b/examples/vm_power_manager/guest_cli/Makefile
> index a5634eacf..51a5010ab 100644
> --- a/examples/vm_power_manager/guest_cli/Makefile
> +++ b/examples/vm_power_manager/guest_cli/Makefile
> @@ -18,6 +18,7 @@ SRCS-y := main.c vm_power_cli_guest.c parse.c
>   
>   CFLAGS += -O3 -I$(RTE_SDK)/lib/librte_power/
>   CFLAGS += $(WERROR_FLAGS)
> +CFLAGS += -DALLOW_EXPERIMENTAL_API
>   
>   # workaround for a gcc bug with noreturn attribute
>   # http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12603
> diff --git a/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c b/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
> index 2d9e7689a..698dd5062 100644
> --- a/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
> +++ b/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
> @@ -132,6 +132,26 @@ struct cmd_set_cpu_freq_result {
>   	cmdline_fixed_string_t cmd;
>   };
>   
> +static int
> +check_response_cmd(unsigned int lcore_id, int *result)
> +{
> +	struct channel_packet pkt;
> +	int ret;
> +
> +	ret = rte_power_guest_channel_receive_msg(&pkt, lcore_id);
> +	if (ret < 0)
> +		return -1;
> +
> +	if (pkt.command != CPU_POWER_CMD_ACK &&
> +		pkt.command != CPU_POWER_CMD_NACK) {
> +		RTE_LOG(DEBUG, POWER, "Not expected command has been received.\n");
> +		return -1;
> +	}

Maybe use switch?

E.g.

swithc (pkt.command) {
case ACK:
case NACK:
     *result = blah;
     break;
default:
     log failure;
     fail;
}

> +
> +	*result = (pkt.command == CPU_POWER_CMD_ACK);
> +	return 0;
> +}
> +
>   static void
>   cmd_set_cpu_freq_parsed(void *parsed_result, struct cmdline *cl,
>   		       __attribute__((unused)) void *data)
> @@ -139,20 +159,31 @@ cmd_set_cpu_freq_parsed(void *parsed_result, struct cmdline *cl,
>   	int ret = -1;
>   	struct cmd_set_cpu_freq_result *res = parsed_result;
>   
> -	if (!strcmp(res->cmd , "up"))
> +	if (!strcmp(res->cmd, "up"))
>   		ret = rte_power_freq_up(res->lcore_id);
> -	else if (!strcmp(res->cmd , "down"))
> +	else if (!strcmp(res->cmd, "down"))
>   		ret = rte_power_freq_down(res->lcore_id);
> -	else if (!strcmp(res->cmd , "min"))
> +	else if (!strcmp(res->cmd, "min"))
>   		ret = rte_power_freq_min(res->lcore_id);
> -	else if (!strcmp(res->cmd , "max"))
> +	else if (!strcmp(res->cmd, "max"))
>   		ret = rte_power_freq_max(res->lcore_id);
>   	else if (!strcmp(res->cmd, "enable_turbo"))
>   		ret = rte_power_freq_enable_turbo(res->lcore_id);
>   	else if (!strcmp(res->cmd, "disable_turbo"))
>   		ret = rte_power_freq_disable_turbo(res->lcore_id);
> -	if (ret != 1)
> +
> +	if (ret != 1) {
>   		cmdline_printf(cl, "Error sending message: %s\n", strerror(ret));
> +		return;
> +	}
> +	int result;
> +	ret = check_response_cmd(res->lcore_id, &result);
> +	if (ret < 0) {
> +		RTE_LOG(DEBUG, POWER, "No confirmation for sent msg received\n");
> +	} else {
> +		cmdline_printf(cl, "Ack for sent msg received with result: %s.\n",
> +				result == 1 ? "SUCCESS" : "ERROR");

Here and in other places: others can correct me if i'm wrong, but i'm 
pretty sure RTE_LOG(..., POWER) is reserved for logs related to power 
library, and are not supposed to be used in user applications. If 
logging in an application is required, there are USER log levels.

I can see that the app in question already does that in a few places, so 
it's kind of consistent with how things are already, but i would 
question the value of consistency in cases like these. Not sure how to 
proceed here.

> +	}
>   }
>   
>   cmdline_parse_token_string_t cmd_set_cpu_freq =
> @@ -185,16 +216,26 @@ struct cmd_send_policy_result {
>   };
>   
>   static inline int
> -send_policy(struct channel_packet *pkt)
> +send_policy(struct channel_packet *pkt, struct cmdline *cl)
>   {
>   	int ret;
>   
>   	ret = rte_power_guest_channel_send_msg(pkt, 1);
> -	if (ret == 0)
> -		return 1;
> -	RTE_LOG(DEBUG, POWER, "Error sending message: %s\n",
> -			ret > 0 ? strerror(ret) : "channel not connected");
> -	return -1;
> +	if (ret < 0) {
> +		RTE_LOG(DEBUG, POWER, "Error sending message: %s\n",
> +				ret > 0 ? strerror(ret) : "channel not connected");
> +		return -1;
> +	}
> +
> +	int result;
> +	ret = check_response_cmd(1, &result);
> +	if (ret < 0) {
> +		RTE_LOG(DEBUG, POWER, "No confirmation for sent policy received\n");
> +	} else {
> +		cmdline_printf(cl, "Ack for sent policy received with result: %s.\n",
> +				result == 1 ? "SUCCESS" : "ERROR");
> +	}
> +	return 1;
>   }
>   
>   static void
> @@ -206,7 +247,7 @@ cmd_send_policy_parsed(void *parsed_result, struct cmdline *cl,
>   
>   	if (!strcmp(res->cmd, "now")) {
>   		printf("Sending Policy down now!\n");
> -		ret = send_policy(&policy);
> +		ret = send_policy(&policy, cl);
>   	}
>   	if (ret != 1)
>   		cmdline_printf(cl, "Error sending message: %s\n",
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3 4/4] power: send confirmation cmd to vm guest

[Burakov, Anatoly]

On 21-Mar-19 10:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Use new guest channel API to send confirmation
> message for received power command.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   examples/vm_power_manager/channel_monitor.c | 67 +++++++++++++++++++--
>   1 file changed, 61 insertions(+), 6 deletions(-)
> 
> diff --git a/examples/vm_power_manager/channel_monitor.c b/examples/vm_power_manager/channel_monitor.c
> index 1a3a0fa76..df1dc1205 100644
> --- a/examples/vm_power_manager/channel_monitor.c
> +++ b/examples/vm_power_manager/channel_monitor.c
> @@ -627,6 +627,39 @@ apply_policy(struct policy *pol)
>   		apply_workload_profile(pol);
>   }
>   
> +static int
> +write_binary_packet(struct channel_packet *pkt, struct channel_info *chan_info)
> +{
> +	int ret, buffer_len = sizeof(*pkt);
> +	void *buffer = pkt;
> +
> +	if (chan_info->fd == 0) {

Shouldn't this be -1?

> +		RTE_LOG(ERR, CHANNEL_MONITOR, "Channel is not connected\n");
> +		return -1;
> +	}
> +
> +	while (buffer_len > 0) {
> +		ret = write(chan_info->fd, buffer, buffer_len);
> +		if (ret == -1) {
> +			if (errno == EINTR)
> +				continue;

Perhaps writing out a debug message with strerror(errno) here?

> +			return -1;
> +		}
> +		buffer = (char *)buffer + ret;
> +		buffer_len -= ret;
> +	}
> +	return 0;
> +}
> +
> +static int
> +send_ack_for_received_cmd(struct channel_packet *pkt,
> +						struct channel_info *chan_info,
> +						uint32_t command)

Too much tabs IMO :)

> +{
> +	pkt->command = command;
> +	return write_binary_packet(pkt, chan_info);
> +}
> +
>   static int
>   process_request(struct channel_packet *pkt, struct channel_info *chan_info)
>   {
> @@ -645,33 +678,55 @@ process_request(struct channel_packet *pkt, struct channel_info *chan_info)
>   		else
>   			core_num = pkt->resource_id;
>   
> +		bool valid_unit = true;
> +		int scale_res;
> +
>   		switch (pkt->unit) {
>   		case(CPU_POWER_SCALE_MIN):
> -			power_manager_scale_core_min(core_num);
> +			scale_res = power_manager_scale_core_min(core_num);
>   			break;
>   		case(CPU_POWER_SCALE_MAX):
> -			power_manager_scale_core_max(core_num);
> +			scale_res = power_manager_scale_core_max(core_num);
>   			break;
>   		case(CPU_POWER_SCALE_DOWN):
> -			power_manager_scale_core_down(core_num);
> +			scale_res = power_manager_scale_core_down(core_num);
>   			break;
>   		case(CPU_POWER_SCALE_UP):
> -			power_manager_scale_core_up(core_num);
> +			scale_res = power_manager_scale_core_up(core_num);
>   			break;
>   		case(CPU_POWER_ENABLE_TURBO):
> -			power_manager_enable_turbo_core(core_num);
> +			scale_res = power_manager_enable_turbo_core(core_num);
>   			break;
>   		case(CPU_POWER_DISABLE_TURBO):
> -			power_manager_disable_turbo_core(core_num);
> +			scale_res = power_manager_disable_turbo_core(core_num);
>   			break;
>   		default:
> +			valid_unit = false;
>   			break;
>   		}
> +
> +		int ret = -1;
> +		if (valid_unit) {
> +			ret = send_ack_for_received_cmd(pkt,
> +						chan_info,
> +						scale_res > 0 ?
> +						CPU_POWER_CMD_ACK
> +						: CPU_POWER_CMD_NACK);

I think layout like this looks more readable:

ret = send_ack_for_received_cmd(pkt,
		chan_info,
		scale_res > 0 ?
			CPU_POWER_CMD_ACK :
			CPU_POWER_CMD_NACK);

Note the two tabs (not three), extra tab for ternary, and colon on the 
first line rather than the second one.

> +			if (ret < 0)
> +				RTE_LOG(DEBUG, CHANNEL_MONITOR, "Error during sending ack command.\n");
> +		} else
> +			RTE_LOG(DEBUG, CHANNEL_MONITOR, "Unexpected unit type.\n");
> +
>   	}
>   
>   	if (pkt->command == PKT_POLICY) {
>   		RTE_LOG(INFO, CHANNEL_MONITOR, "Processing policy request %s\n",
>   				pkt->vm_name);
> +		int ret = send_ack_for_received_cmd(pkt,
> +						chan_info,
> +						CPU_POWER_CMD_ACK);

Again, four tabs seems way too much. Two maybe?

> +		if (ret < 0)
> +			RTE_LOG(DEBUG, CHANNEL_MONITOR, "Error during sending ack command.\n");
>   		update_policy(pkt);
>   		policy_is_set = 1;
>   	}
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3 4/4] power: send confirmation cmd to vm guest

[Burakov, Anatoly]

On 21-Mar-19 10:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Use new guest channel API to send confirmation
> message for received power command.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   examples/vm_power_manager/channel_monitor.c | 67 +++++++++++++++++++--
>   1 file changed, 61 insertions(+), 6 deletions(-)
> 
> diff --git a/examples/vm_power_manager/channel_monitor.c b/examples/vm_power_manager/channel_monitor.c
> index 1a3a0fa76..df1dc1205 100644
> --- a/examples/vm_power_manager/channel_monitor.c
> +++ b/examples/vm_power_manager/channel_monitor.c
> @@ -627,6 +627,39 @@ apply_policy(struct policy *pol)
>   		apply_workload_profile(pol);
>   }
>   
> +static int
> +write_binary_packet(struct channel_packet *pkt, struct channel_info *chan_info)
> +{
> +	int ret, buffer_len = sizeof(*pkt);
> +	void *buffer = pkt;
> +
> +	if (chan_info->fd == 0) {

Shouldn't this be -1?

> +		RTE_LOG(ERR, CHANNEL_MONITOR, "Channel is not connected\n");
> +		return -1;
> +	}
> +
> +	while (buffer_len > 0) {
> +		ret = write(chan_info->fd, buffer, buffer_len);
> +		if (ret == -1) {
> +			if (errno == EINTR)
> +				continue;

Perhaps writing out a debug message with strerror(errno) here?

> +			return -1;
> +		}
> +		buffer = (char *)buffer + ret;
> +		buffer_len -= ret;
> +	}
> +	return 0;
> +}
> +
> +static int
> +send_ack_for_received_cmd(struct channel_packet *pkt,
> +						struct channel_info *chan_info,
> +						uint32_t command)

Too much tabs IMO :)

> +{
> +	pkt->command = command;
> +	return write_binary_packet(pkt, chan_info);
> +}
> +
>   static int
>   process_request(struct channel_packet *pkt, struct channel_info *chan_info)
>   {
> @@ -645,33 +678,55 @@ process_request(struct channel_packet *pkt, struct channel_info *chan_info)
>   		else
>   			core_num = pkt->resource_id;
>   
> +		bool valid_unit = true;
> +		int scale_res;
> +
>   		switch (pkt->unit) {
>   		case(CPU_POWER_SCALE_MIN):
> -			power_manager_scale_core_min(core_num);
> +			scale_res = power_manager_scale_core_min(core_num);
>   			break;
>   		case(CPU_POWER_SCALE_MAX):
> -			power_manager_scale_core_max(core_num);
> +			scale_res = power_manager_scale_core_max(core_num);
>   			break;
>   		case(CPU_POWER_SCALE_DOWN):
> -			power_manager_scale_core_down(core_num);
> +			scale_res = power_manager_scale_core_down(core_num);
>   			break;
>   		case(CPU_POWER_SCALE_UP):
> -			power_manager_scale_core_up(core_num);
> +			scale_res = power_manager_scale_core_up(core_num);
>   			break;
>   		case(CPU_POWER_ENABLE_TURBO):
> -			power_manager_enable_turbo_core(core_num);
> +			scale_res = power_manager_enable_turbo_core(core_num);
>   			break;
>   		case(CPU_POWER_DISABLE_TURBO):
> -			power_manager_disable_turbo_core(core_num);
> +			scale_res = power_manager_disable_turbo_core(core_num);
>   			break;
>   		default:
> +			valid_unit = false;
>   			break;
>   		}
> +
> +		int ret = -1;
> +		if (valid_unit) {
> +			ret = send_ack_for_received_cmd(pkt,
> +						chan_info,
> +						scale_res > 0 ?
> +						CPU_POWER_CMD_ACK
> +						: CPU_POWER_CMD_NACK);

I think layout like this looks more readable:

ret = send_ack_for_received_cmd(pkt,
		chan_info,
		scale_res > 0 ?
			CPU_POWER_CMD_ACK :
			CPU_POWER_CMD_NACK);

Note the two tabs (not three), extra tab for ternary, and colon on the 
first line rather than the second one.

> +			if (ret < 0)
> +				RTE_LOG(DEBUG, CHANNEL_MONITOR, "Error during sending ack command.\n");
> +		} else
> +			RTE_LOG(DEBUG, CHANNEL_MONITOR, "Unexpected unit type.\n");
> +
>   	}
>   
>   	if (pkt->command == PKT_POLICY) {
>   		RTE_LOG(INFO, CHANNEL_MONITOR, "Processing policy request %s\n",
>   				pkt->vm_name);
> +		int ret = send_ack_for_received_cmd(pkt,
> +						chan_info,
> +						CPU_POWER_CMD_ACK);

Again, four tabs seems way too much. Two maybe?

> +		if (ret < 0)
> +			RTE_LOG(DEBUG, CHANNEL_MONITOR, "Error during sending ack command.\n");
>   		update_policy(pkt);
>   		policy_is_set = 1;
>   	}
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3 0/4] bidirect guest channel

[Thomas Monjalon]

21/03/2019 11:55, Hajkowski:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Extend guest channel API to allow bidirectional
> communication. Modify power manager host and guest
> side to communicate in both directions.
> 
> v3:
> * fix global_fds[lcore_id] comparison to invalid value
> * check 0 to verify if read function actually read some data
> * define _NACK cmd instead of _NAK
> * simplify rte_power_guest_channel_receive_msg func logic
> 
> v2:
> * send ack only if power operation return positive value
> * log diffent error for unexpected incoming command and
>   error during ack/nak cmd sending

Please use --in-reply-to to keep all versions in the same thread, thanks.

Re: [dpdk-dev] [PATCH v3 0/4] bidirect guest channel

[Thomas Monjalon]

21/03/2019 11:55, Hajkowski:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Extend guest channel API to allow bidirectional
> communication. Modify power manager host and guest
> side to communicate in both directions.
> 
> v3:
> * fix global_fds[lcore_id] comparison to invalid value
> * check 0 to verify if read function actually read some data
> * define _NACK cmd instead of _NAK
> * simplify rte_power_guest_channel_receive_msg func logic
> 
> v2:
> * send ack only if power operation return positive value
> * log diffent error for unexpected incoming command and
>   error during ack/nak cmd sending

Please use --in-reply-to to keep all versions in the same thread, thanks.

Re: [dpdk-dev] [PATCH v3 3/4] power: process incoming confirmation cmds

[Pattan, Reshma]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Hajkowski
> Sent: Thursday, March 21, 2019 10:55 AM
> +		RTE_LOG(DEBUG, POWER, "No confirmation for sent msg
> received\n");
> +	} 

As Anatoly mentioned, we should not use POWER log here, instead #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1 
is defined for GUEST_CHANNEL logs, you should use that for the logs in this file.

Thanks,
Reshma

Re: [dpdk-dev] [PATCH v3 3/4] power: process incoming confirmation cmds

[Pattan, Reshma]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Hajkowski
> Sent: Thursday, March 21, 2019 10:55 AM
> +		RTE_LOG(DEBUG, POWER, "No confirmation for sent msg
> received\n");
> +	} 

As Anatoly mentioned, we should not use POWER log here, instead #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1 
is defined for GUEST_CHANNEL logs, you should use that for the logs in this file.

Thanks,
Reshma

[dpdk-dev] [PATCH v4 0/4] bidirect guest channel

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Extend guest channel API to allow bidirectional
communication. Modify power manager host and guest
side to communicate in both directions.

---

v4:
* [vm_power_manager] treat 0 as valid socket id
* [guest_manager] use user level logs
* correct code formatting

v3:
* fix global_fds[lcore_id] comparison to invalid value
* check 0 to verify if read function actually read some data
* define _NACK cmd instead of _NAK
* simplify rte_power_guest_channel_receive_msg func logic

v2:
* send ack only if power operation return positive value
* log diffent error for unexpected incoming command and
  error during ack/nak cmd sending


Marcin Hajkowski (4):
  power: fix invalid socket indicator value
  power: extend guest channel api for reading
  power: process incoming confirmation cmds
  power: send confirmation cmd to vm guest

 examples/vm_power_manager/channel_monitor.c   | 68 ++++++++++++++++--
 examples/vm_power_manager/guest_cli/Makefile  |  1 +
 .../guest_cli/vm_power_cli_guest.c            | 72 +++++++++++++++----
 lib/librte_power/channel_commands.h           |  5 ++
 lib/librte_power/guest_channel.c              | 72 +++++++++++++++++--
 lib/librte_power/guest_channel.h              | 35 +++++++++
 lib/librte_power/rte_power_version.map        |  1 +
 7 files changed, 229 insertions(+), 25 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH v4 0/4] bidirect guest channel

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Extend guest channel API to allow bidirectional
communication. Modify power manager host and guest
side to communicate in both directions.

---

v4:
* [vm_power_manager] treat 0 as valid socket id
* [guest_manager] use user level logs
* correct code formatting

v3:
* fix global_fds[lcore_id] comparison to invalid value
* check 0 to verify if read function actually read some data
* define _NACK cmd instead of _NAK
* simplify rte_power_guest_channel_receive_msg func logic

v2:
* send ack only if power operation return positive value
* log diffent error for unexpected incoming command and
  error during ack/nak cmd sending


Marcin Hajkowski (4):
  power: fix invalid socket indicator value
  power: extend guest channel api for reading
  power: process incoming confirmation cmds
  power: send confirmation cmd to vm guest

 examples/vm_power_manager/channel_monitor.c   | 68 ++++++++++++++++--
 examples/vm_power_manager/guest_cli/Makefile  |  1 +
 .../guest_cli/vm_power_cli_guest.c            | 72 +++++++++++++++----
 lib/librte_power/channel_commands.h           |  5 ++
 lib/librte_power/guest_channel.c              | 72 +++++++++++++++++--
 lib/librte_power/guest_channel.h              | 35 +++++++++
 lib/librte_power/rte_power_version.map        |  1 +
 7 files changed, 229 insertions(+), 25 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH v4 1/4] power: fix invalid socket indicator value

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Currently 0 is being used for not connected slot indication.
This is not consistent with linux doc which identifies 0 as valid
(connected) slot, thus modification was done to change it.

Fixes: cd0d5547 ("power: vm communication channels in guest")
Cc: stable@dpdk.org

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 lib/librte_power/guest_channel.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
index c17ea46b4..9cf7d2cb2 100644
--- a/lib/librte_power/guest_channel.c
+++ b/lib/librte_power/guest_channel.c
@@ -19,7 +19,7 @@
 
 #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
 
-static int global_fds[RTE_MAX_LCORE];
+static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
 
 int
 guest_channel_host_connect(const char *path, unsigned int lcore_id)
@@ -35,7 +35,7 @@ guest_channel_host_connect(const char *path, unsigned int lcore_id)
 		return -1;
 	}
 	/* check if path is already open */
-	if (global_fds[lcore_id] != 0) {
+	if (global_fds[lcore_id] != -1) {
 		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is already open with fd %d\n",
 				lcore_id, global_fds[lcore_id]);
 		return -1;
@@ -84,7 +84,7 @@ guest_channel_host_connect(const char *path, unsigned int lcore_id)
 	return 0;
 error:
 	close(fd);
-	global_fds[lcore_id] = 0;
+	global_fds[lcore_id] = -1;
 	return -1;
 }
 
@@ -100,7 +100,7 @@ guest_channel_send_msg(struct channel_packet *pkt, unsigned int lcore_id)
 		return -1;
 	}
 
-	if (global_fds[lcore_id] == 0) {
+	if (global_fds[lcore_id] < 0) {
 		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
 		return -1;
 	}
@@ -134,8 +134,8 @@ guest_channel_host_disconnect(unsigned int lcore_id)
 				lcore_id, RTE_MAX_LCORE-1);
 		return;
 	}
-	if (global_fds[lcore_id] == 0)
+	if (global_fds[lcore_id] < 0)
 		return;
 	close(global_fds[lcore_id]);
-	global_fds[lcore_id] = 0;
+	global_fds[lcore_id] = -1;
 }
-- 
2.17.2

[dpdk-dev] [PATCH v4 1/4] power: fix invalid socket indicator value

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Currently 0 is being used for not connected slot indication.
This is not consistent with linux doc which identifies 0 as valid
(connected) slot, thus modification was done to change it.

Fixes: cd0d5547 ("power: vm communication channels in guest")
Cc: stable@dpdk.org

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 lib/librte_power/guest_channel.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
index c17ea46b4..9cf7d2cb2 100644
--- a/lib/librte_power/guest_channel.c
+++ b/lib/librte_power/guest_channel.c
@@ -19,7 +19,7 @@
 
 #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
 
-static int global_fds[RTE_MAX_LCORE];
+static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
 
 int
 guest_channel_host_connect(const char *path, unsigned int lcore_id)
@@ -35,7 +35,7 @@ guest_channel_host_connect(const char *path, unsigned int lcore_id)
 		return -1;
 	}
 	/* check if path is already open */
-	if (global_fds[lcore_id] != 0) {
+	if (global_fds[lcore_id] != -1) {
 		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is already open with fd %d\n",
 				lcore_id, global_fds[lcore_id]);
 		return -1;
@@ -84,7 +84,7 @@ guest_channel_host_connect(const char *path, unsigned int lcore_id)
 	return 0;
 error:
 	close(fd);
-	global_fds[lcore_id] = 0;
+	global_fds[lcore_id] = -1;
 	return -1;
 }
 
@@ -100,7 +100,7 @@ guest_channel_send_msg(struct channel_packet *pkt, unsigned int lcore_id)
 		return -1;
 	}
 
-	if (global_fds[lcore_id] == 0) {
+	if (global_fds[lcore_id] < 0) {
 		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
 		return -1;
 	}
@@ -134,8 +134,8 @@ guest_channel_host_disconnect(unsigned int lcore_id)
 				lcore_id, RTE_MAX_LCORE-1);
 		return;
 	}
-	if (global_fds[lcore_id] == 0)
+	if (global_fds[lcore_id] < 0)
 		return;
 	close(global_fds[lcore_id]);
-	global_fds[lcore_id] = 0;
+	global_fds[lcore_id] = -1;
 }
-- 
2.17.2

[dpdk-dev] [PATCH v4 2/4] power: extend guest channel api for reading

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Added new experimental API rte_power_guest_channel_receive_msg
which gives possibility to receive messages send to guest.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 lib/librte_power/channel_commands.h    |  5 +++
 lib/librte_power/guest_channel.c       | 60 ++++++++++++++++++++++++++
 lib/librte_power/guest_channel.h       | 35 +++++++++++++++
 lib/librte_power/rte_power_version.map |  1 +
 4 files changed, 101 insertions(+)

diff --git a/lib/librte_power/channel_commands.h b/lib/librte_power/channel_commands.h
index e7b93a797..33fd53a6d 100644
--- a/lib/librte_power/channel_commands.h
+++ b/lib/librte_power/channel_commands.h
@@ -28,6 +28,11 @@ extern "C" {
 #define CPU_POWER_SCALE_MIN     4
 #define CPU_POWER_ENABLE_TURBO  5
 #define CPU_POWER_DISABLE_TURBO 6
+
+/* Generic Power Command Response */
+#define CPU_POWER_CMD_ACK       1
+#define CPU_POWER_CMD_NACK      2
+
 #define HOURS 24
 
 #define MAX_VFS 10
diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
index 9cf7d2cb2..888423891 100644
--- a/lib/librte_power/guest_channel.c
+++ b/lib/librte_power/guest_channel.c
@@ -10,6 +10,7 @@
 #include <fcntl.h>
 #include <string.h>
 #include <errno.h>
+#include <poll.h>
 
 
 #include <rte_log.h>
@@ -19,6 +20,9 @@
 
 #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
 
+/* Timeout for incoming message in milliseconds. */
+#define TIMEOUT 10
+
 static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
 
 int
@@ -125,6 +129,62 @@ int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
 	return guest_channel_send_msg(pkt, lcore_id);
 }
 
+int power_guest_channel_read_msg(struct channel_packet *pkt,
+			unsigned int lcore_id)
+{
+	int ret;
+	struct pollfd fds;
+	void *buffer = pkt;
+	int buffer_len = sizeof(*pkt);
+
+	fds.fd = global_fds[lcore_id];
+	fds.events = POLLIN;
+
+	ret = poll(&fds, 1, TIMEOUT);
+	if (ret == 0) {
+		RTE_LOG(DEBUG, GUEST_CHANNEL, "Timeout occurred during poll function.\n");
+		return -1;
+	} else if (ret < 0) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Error occurred during poll function: %s\n",
+				strerror(ret));
+		return -1;
+	}
+
+	if (lcore_id >= RTE_MAX_LCORE) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
+				lcore_id, RTE_MAX_LCORE-1);
+		return -1;
+	}
+
+	if (global_fds[lcore_id] < 0) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
+		return -1;
+	}
+
+	while (buffer_len > 0) {
+		ret = read(global_fds[lcore_id],
+				buffer, buffer_len);
+		if (ret < 0) {
+			if (errno == EINTR)
+				continue;
+			return -1;
+		}
+		if (ret == 0) {
+			RTE_LOG(ERR, GUEST_CHANNEL, "Expected more data, but connection has been closed.\n");
+			return -1;
+		}
+		buffer = (char *)buffer + ret;
+		buffer_len -= ret;
+	}
+
+	return 0;
+}
+
+int rte_power_guest_channel_receive_msg(struct channel_packet *pkt,
+			unsigned int lcore_id)
+{
+	return power_guest_channel_read_msg(pkt, lcore_id);
+}
 
 void
 guest_channel_host_disconnect(unsigned int lcore_id)
diff --git a/lib/librte_power/guest_channel.h b/lib/librte_power/guest_channel.h
index 373d39898..7c385df39 100644
--- a/lib/librte_power/guest_channel.h
+++ b/lib/librte_power/guest_channel.h
@@ -68,6 +68,41 @@ int guest_channel_send_msg(struct channel_packet *pkt, unsigned int lcore_id);
 int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
 			unsigned int lcore_id);
 
+/**
+ * Read a message contained in pkt over the Virtio-Serial
+ * from the host endpoint.
+ *
+ * @param pkt
+ *  Pointer to a populated struct channel_packet
+ *
+ * @param lcore_id
+ *  lcore_id.
+ *
+ * @return
+ *  - 0 on success.
+ *  - Negative on error.
+ */
+int power_guest_channel_read_msg(struct channel_packet *pkt,
+		unsigned int lcore_id);
+
+/**
+ * Receive a message contained in pkt over the Virtio-Serial
+ * from the host endpoint.
+ *
+ * @param pkt
+ *  Pointer to a populated struct channel_packet
+ *
+ * @param lcore_id
+ *  lcore_id.
+ *
+ * @return
+ *  - 0 on success.
+ *  - Negative on error.
+ */
+int __rte_experimental
+rte_power_guest_channel_receive_msg(struct channel_packet *pkt,
+			unsigned int lcore_id);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_power/rte_power_version.map b/lib/librte_power/rte_power_version.map
index 042917360..69f5ea3f4 100644
--- a/lib/librte_power/rte_power_version.map
+++ b/lib/librte_power/rte_power_version.map
@@ -44,4 +44,5 @@ EXPERIMENTAL {
 	rte_power_empty_poll_stat_update;
 	rte_power_poll_stat_fetch;
 	rte_power_poll_stat_update;
+	rte_power_guest_channel_receive_msg;
 };
-- 
2.17.2

[dpdk-dev] [PATCH v4 2/4] power: extend guest channel api for reading

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Added new experimental API rte_power_guest_channel_receive_msg
which gives possibility to receive messages send to guest.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 lib/librte_power/channel_commands.h    |  5 +++
 lib/librte_power/guest_channel.c       | 60 ++++++++++++++++++++++++++
 lib/librte_power/guest_channel.h       | 35 +++++++++++++++
 lib/librte_power/rte_power_version.map |  1 +
 4 files changed, 101 insertions(+)

diff --git a/lib/librte_power/channel_commands.h b/lib/librte_power/channel_commands.h
index e7b93a797..33fd53a6d 100644
--- a/lib/librte_power/channel_commands.h
+++ b/lib/librte_power/channel_commands.h
@@ -28,6 +28,11 @@ extern "C" {
 #define CPU_POWER_SCALE_MIN     4
 #define CPU_POWER_ENABLE_TURBO  5
 #define CPU_POWER_DISABLE_TURBO 6
+
+/* Generic Power Command Response */
+#define CPU_POWER_CMD_ACK       1
+#define CPU_POWER_CMD_NACK      2
+
 #define HOURS 24
 
 #define MAX_VFS 10
diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
index 9cf7d2cb2..888423891 100644
--- a/lib/librte_power/guest_channel.c
+++ b/lib/librte_power/guest_channel.c
@@ -10,6 +10,7 @@
 #include <fcntl.h>
 #include <string.h>
 #include <errno.h>
+#include <poll.h>
 
 
 #include <rte_log.h>
@@ -19,6 +20,9 @@
 
 #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
 
+/* Timeout for incoming message in milliseconds. */
+#define TIMEOUT 10
+
 static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
 
 int
@@ -125,6 +129,62 @@ int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
 	return guest_channel_send_msg(pkt, lcore_id);
 }
 
+int power_guest_channel_read_msg(struct channel_packet *pkt,
+			unsigned int lcore_id)
+{
+	int ret;
+	struct pollfd fds;
+	void *buffer = pkt;
+	int buffer_len = sizeof(*pkt);
+
+	fds.fd = global_fds[lcore_id];
+	fds.events = POLLIN;
+
+	ret = poll(&fds, 1, TIMEOUT);
+	if (ret == 0) {
+		RTE_LOG(DEBUG, GUEST_CHANNEL, "Timeout occurred during poll function.\n");
+		return -1;
+	} else if (ret < 0) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Error occurred during poll function: %s\n",
+				strerror(ret));
+		return -1;
+	}
+
+	if (lcore_id >= RTE_MAX_LCORE) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
+				lcore_id, RTE_MAX_LCORE-1);
+		return -1;
+	}
+
+	if (global_fds[lcore_id] < 0) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
+		return -1;
+	}
+
+	while (buffer_len > 0) {
+		ret = read(global_fds[lcore_id],
+				buffer, buffer_len);
+		if (ret < 0) {
+			if (errno == EINTR)
+				continue;
+			return -1;
+		}
+		if (ret == 0) {
+			RTE_LOG(ERR, GUEST_CHANNEL, "Expected more data, but connection has been closed.\n");
+			return -1;
+		}
+		buffer = (char *)buffer + ret;
+		buffer_len -= ret;
+	}
+
+	return 0;
+}
+
+int rte_power_guest_channel_receive_msg(struct channel_packet *pkt,
+			unsigned int lcore_id)
+{
+	return power_guest_channel_read_msg(pkt, lcore_id);
+}
 
 void
 guest_channel_host_disconnect(unsigned int lcore_id)
diff --git a/lib/librte_power/guest_channel.h b/lib/librte_power/guest_channel.h
index 373d39898..7c385df39 100644
--- a/lib/librte_power/guest_channel.h
+++ b/lib/librte_power/guest_channel.h
@@ -68,6 +68,41 @@ int guest_channel_send_msg(struct channel_packet *pkt, unsigned int lcore_id);
 int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
 			unsigned int lcore_id);
 
+/**
+ * Read a message contained in pkt over the Virtio-Serial
+ * from the host endpoint.
+ *
+ * @param pkt
+ *  Pointer to a populated struct channel_packet
+ *
+ * @param lcore_id
+ *  lcore_id.
+ *
+ * @return
+ *  - 0 on success.
+ *  - Negative on error.
+ */
+int power_guest_channel_read_msg(struct channel_packet *pkt,
+		unsigned int lcore_id);
+
+/**
+ * Receive a message contained in pkt over the Virtio-Serial
+ * from the host endpoint.
+ *
+ * @param pkt
+ *  Pointer to a populated struct channel_packet
+ *
+ * @param lcore_id
+ *  lcore_id.
+ *
+ * @return
+ *  - 0 on success.
+ *  - Negative on error.
+ */
+int __rte_experimental
+rte_power_guest_channel_receive_msg(struct channel_packet *pkt,
+			unsigned int lcore_id);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_power/rte_power_version.map b/lib/librte_power/rte_power_version.map
index 042917360..69f5ea3f4 100644
--- a/lib/librte_power/rte_power_version.map
+++ b/lib/librte_power/rte_power_version.map
@@ -44,4 +44,5 @@ EXPERIMENTAL {
 	rte_power_empty_poll_stat_update;
 	rte_power_poll_stat_fetch;
 	rte_power_poll_stat_update;
+	rte_power_guest_channel_receive_msg;
 };
-- 
2.17.2

[dpdk-dev] [PATCH v4 3/4] power: process incoming confirmation cmds

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Extend vm_power_guest to check incoming confirmations
of messages previously sent to host.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 examples/vm_power_manager/guest_cli/Makefile  |  1 +
 .../guest_cli/vm_power_cli_guest.c            | 72 +++++++++++++++----
 2 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/examples/vm_power_manager/guest_cli/Makefile b/examples/vm_power_manager/guest_cli/Makefile
index e35a68d0f..67cf08193 100644
--- a/examples/vm_power_manager/guest_cli/Makefile
+++ b/examples/vm_power_manager/guest_cli/Makefile
@@ -18,6 +18,7 @@ SRCS-y := main.c vm_power_cli_guest.c parse.c
 
 CFLAGS += -O3 -I$(RTE_SDK)/lib/librte_power/
 CFLAGS += $(WERROR_FLAGS)
+CFLAGS += -DALLOW_EXPERIMENTAL_API
 
 # workaround for a gcc bug with noreturn attribute
 # http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12603
diff --git a/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c b/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
index 2d9e7689a..af49dfef8 100644
--- a/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
+++ b/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
@@ -27,7 +27,7 @@
 #define CHANNEL_PATH "/dev/virtio-ports/virtio.serial.port.poweragent"
 
 
-#define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
+#define RTE_LOGTYPE_GUEST_CLI RTE_LOGTYPE_USER1
 
 struct cmd_quit_result {
 	cmdline_fixed_string_t quit;
@@ -132,6 +132,31 @@ struct cmd_set_cpu_freq_result {
 	cmdline_fixed_string_t cmd;
 };
 
+static int
+check_response_cmd(unsigned int lcore_id, int *result)
+{
+	struct channel_packet pkt;
+	int ret;
+
+	ret = rte_power_guest_channel_receive_msg(&pkt, lcore_id);
+	if (ret < 0)
+		return -1;
+
+	switch (pkt.command) {
+	case(CPU_POWER_CMD_ACK):
+		*result = 1;
+		break;
+	case(CPU_POWER_CMD_NACK):
+		*result = 0;
+		break;
+	default:
+		RTE_LOG(DEBUG, GUEST_CLI, "Not expected command has been received.\n");
+		return -1;
+	}
+
+	return 0;
+}
+
 static void
 cmd_set_cpu_freq_parsed(void *parsed_result, struct cmdline *cl,
 		       __attribute__((unused)) void *data)
@@ -139,20 +164,31 @@ cmd_set_cpu_freq_parsed(void *parsed_result, struct cmdline *cl,
 	int ret = -1;
 	struct cmd_set_cpu_freq_result *res = parsed_result;
 
-	if (!strcmp(res->cmd , "up"))
+	if (!strcmp(res->cmd, "up"))
 		ret = rte_power_freq_up(res->lcore_id);
-	else if (!strcmp(res->cmd , "down"))
+	else if (!strcmp(res->cmd, "down"))
 		ret = rte_power_freq_down(res->lcore_id);
-	else if (!strcmp(res->cmd , "min"))
+	else if (!strcmp(res->cmd, "min"))
 		ret = rte_power_freq_min(res->lcore_id);
-	else if (!strcmp(res->cmd , "max"))
+	else if (!strcmp(res->cmd, "max"))
 		ret = rte_power_freq_max(res->lcore_id);
 	else if (!strcmp(res->cmd, "enable_turbo"))
 		ret = rte_power_freq_enable_turbo(res->lcore_id);
 	else if (!strcmp(res->cmd, "disable_turbo"))
 		ret = rte_power_freq_disable_turbo(res->lcore_id);
-	if (ret != 1)
+
+	if (ret != 1) {
 		cmdline_printf(cl, "Error sending message: %s\n", strerror(ret));
+		return;
+	}
+	int result;
+	ret = check_response_cmd(res->lcore_id, &result);
+	if (ret < 0) {
+		RTE_LOG(DEBUG, GUEST_CLI, "No confirmation for sent msg received\n");
+	} else {
+		cmdline_printf(cl, "Ack for sent msg received with result: %s.\n",
+				result == 1 ? "SUCCESS" : "ERROR");
+	}
 }
 
 cmdline_parse_token_string_t cmd_set_cpu_freq =
@@ -185,16 +221,26 @@ struct cmd_send_policy_result {
 };
 
 static inline int
-send_policy(struct channel_packet *pkt)
+send_policy(struct channel_packet *pkt, struct cmdline *cl)
 {
 	int ret;
 
 	ret = rte_power_guest_channel_send_msg(pkt, 1);
-	if (ret == 0)
-		return 1;
-	RTE_LOG(DEBUG, POWER, "Error sending message: %s\n",
-			ret > 0 ? strerror(ret) : "channel not connected");
-	return -1;
+	if (ret < 0) {
+		RTE_LOG(DEBUG, GUEST_CLI, "Error sending message: %s\n",
+				ret > 0 ? strerror(ret) : "channel not connected");
+		return -1;
+	}
+
+	int result;
+	ret = check_response_cmd(1, &result);
+	if (ret < 0) {
+		RTE_LOG(DEBUG, GUEST_CLI, "No confirmation for sent policy received\n");
+	} else {
+		cmdline_printf(cl, "Ack for sent policy received with result: %s.\n",
+				result == 1 ? "SUCCESS" : "ERROR");
+	}
+	return 1;
 }
 
 static void
@@ -206,7 +252,7 @@ cmd_send_policy_parsed(void *parsed_result, struct cmdline *cl,
 
 	if (!strcmp(res->cmd, "now")) {
 		printf("Sending Policy down now!\n");
-		ret = send_policy(&policy);
+		ret = send_policy(&policy, cl);
 	}
 	if (ret != 1)
 		cmdline_printf(cl, "Error sending message: %s\n",
-- 
2.17.2

[dpdk-dev] [PATCH v4 3/4] power: process incoming confirmation cmds

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Extend vm_power_guest to check incoming confirmations
of messages previously sent to host.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 examples/vm_power_manager/guest_cli/Makefile  |  1 +
 .../guest_cli/vm_power_cli_guest.c            | 72 +++++++++++++++----
 2 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/examples/vm_power_manager/guest_cli/Makefile b/examples/vm_power_manager/guest_cli/Makefile
index e35a68d0f..67cf08193 100644
--- a/examples/vm_power_manager/guest_cli/Makefile
+++ b/examples/vm_power_manager/guest_cli/Makefile
@@ -18,6 +18,7 @@ SRCS-y := main.c vm_power_cli_guest.c parse.c
 
 CFLAGS += -O3 -I$(RTE_SDK)/lib/librte_power/
 CFLAGS += $(WERROR_FLAGS)
+CFLAGS += -DALLOW_EXPERIMENTAL_API
 
 # workaround for a gcc bug with noreturn attribute
 # http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12603
diff --git a/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c b/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
index 2d9e7689a..af49dfef8 100644
--- a/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
+++ b/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
@@ -27,7 +27,7 @@
 #define CHANNEL_PATH "/dev/virtio-ports/virtio.serial.port.poweragent"
 
 
-#define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
+#define RTE_LOGTYPE_GUEST_CLI RTE_LOGTYPE_USER1
 
 struct cmd_quit_result {
 	cmdline_fixed_string_t quit;
@@ -132,6 +132,31 @@ struct cmd_set_cpu_freq_result {
 	cmdline_fixed_string_t cmd;
 };
 
+static int
+check_response_cmd(unsigned int lcore_id, int *result)
+{
+	struct channel_packet pkt;
+	int ret;
+
+	ret = rte_power_guest_channel_receive_msg(&pkt, lcore_id);
+	if (ret < 0)
+		return -1;
+
+	switch (pkt.command) {
+	case(CPU_POWER_CMD_ACK):
+		*result = 1;
+		break;
+	case(CPU_POWER_CMD_NACK):
+		*result = 0;
+		break;
+	default:
+		RTE_LOG(DEBUG, GUEST_CLI, "Not expected command has been received.\n");
+		return -1;
+	}
+
+	return 0;
+}
+
 static void
 cmd_set_cpu_freq_parsed(void *parsed_result, struct cmdline *cl,
 		       __attribute__((unused)) void *data)
@@ -139,20 +164,31 @@ cmd_set_cpu_freq_parsed(void *parsed_result, struct cmdline *cl,
 	int ret = -1;
 	struct cmd_set_cpu_freq_result *res = parsed_result;
 
-	if (!strcmp(res->cmd , "up"))
+	if (!strcmp(res->cmd, "up"))
 		ret = rte_power_freq_up(res->lcore_id);
-	else if (!strcmp(res->cmd , "down"))
+	else if (!strcmp(res->cmd, "down"))
 		ret = rte_power_freq_down(res->lcore_id);
-	else if (!strcmp(res->cmd , "min"))
+	else if (!strcmp(res->cmd, "min"))
 		ret = rte_power_freq_min(res->lcore_id);
-	else if (!strcmp(res->cmd , "max"))
+	else if (!strcmp(res->cmd, "max"))
 		ret = rte_power_freq_max(res->lcore_id);
 	else if (!strcmp(res->cmd, "enable_turbo"))
 		ret = rte_power_freq_enable_turbo(res->lcore_id);
 	else if (!strcmp(res->cmd, "disable_turbo"))
 		ret = rte_power_freq_disable_turbo(res->lcore_id);
-	if (ret != 1)
+
+	if (ret != 1) {
 		cmdline_printf(cl, "Error sending message: %s\n", strerror(ret));
+		return;
+	}
+	int result;
+	ret = check_response_cmd(res->lcore_id, &result);
+	if (ret < 0) {
+		RTE_LOG(DEBUG, GUEST_CLI, "No confirmation for sent msg received\n");
+	} else {
+		cmdline_printf(cl, "Ack for sent msg received with result: %s.\n",
+				result == 1 ? "SUCCESS" : "ERROR");
+	}
 }
 
 cmdline_parse_token_string_t cmd_set_cpu_freq =
@@ -185,16 +221,26 @@ struct cmd_send_policy_result {
 };
 
 static inline int
-send_policy(struct channel_packet *pkt)
+send_policy(struct channel_packet *pkt, struct cmdline *cl)
 {
 	int ret;
 
 	ret = rte_power_guest_channel_send_msg(pkt, 1);
-	if (ret == 0)
-		return 1;
-	RTE_LOG(DEBUG, POWER, "Error sending message: %s\n",
-			ret > 0 ? strerror(ret) : "channel not connected");
-	return -1;
+	if (ret < 0) {
+		RTE_LOG(DEBUG, GUEST_CLI, "Error sending message: %s\n",
+				ret > 0 ? strerror(ret) : "channel not connected");
+		return -1;
+	}
+
+	int result;
+	ret = check_response_cmd(1, &result);
+	if (ret < 0) {
+		RTE_LOG(DEBUG, GUEST_CLI, "No confirmation for sent policy received\n");
+	} else {
+		cmdline_printf(cl, "Ack for sent policy received with result: %s.\n",
+				result == 1 ? "SUCCESS" : "ERROR");
+	}
+	return 1;
 }
 
 static void
@@ -206,7 +252,7 @@ cmd_send_policy_parsed(void *parsed_result, struct cmdline *cl,
 
 	if (!strcmp(res->cmd, "now")) {
 		printf("Sending Policy down now!\n");
-		ret = send_policy(&policy);
+		ret = send_policy(&policy, cl);
 	}
 	if (ret != 1)
 		cmdline_printf(cl, "Error sending message: %s\n",
-- 
2.17.2

[dpdk-dev] [PATCH v4 4/4] power: send confirmation cmd to vm guest

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Use new guest channel API to send confirmation
message for received power command.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 examples/vm_power_manager/channel_monitor.c | 68 +++++++++++++++++++--
 1 file changed, 62 insertions(+), 6 deletions(-)

diff --git a/examples/vm_power_manager/channel_monitor.c b/examples/vm_power_manager/channel_monitor.c
index 7892d75de..ed580b36a 100644
--- a/examples/vm_power_manager/channel_monitor.c
+++ b/examples/vm_power_manager/channel_monitor.c
@@ -627,6 +627,41 @@ apply_policy(struct policy *pol)
 		apply_workload_profile(pol);
 }
 
+static int
+write_binary_packet(struct channel_packet *pkt, struct channel_info *chan_info)
+{
+	int ret, buffer_len = sizeof(*pkt);
+	void *buffer = pkt;
+
+	if (chan_info->fd < 0) {
+		RTE_LOG(ERR, CHANNEL_MONITOR, "Channel is not connected\n");
+		return -1;
+	}
+
+	while (buffer_len > 0) {
+		ret = write(chan_info->fd, buffer, buffer_len);
+		if (ret == -1) {
+			if (errno == EINTR)
+				continue;
+			RTE_LOG(ERR, CHANNEL_MONITOR, "Write function failed due to %s.\n",
+					strerror(errno));
+			return -1;
+		}
+		buffer = (char *)buffer + ret;
+		buffer_len -= ret;
+	}
+	return 0;
+}
+
+static int
+send_ack_for_received_cmd(struct channel_packet *pkt,
+		struct channel_info *chan_info,
+		uint32_t command)
+{
+	pkt->command = command;
+	return write_binary_packet(pkt, chan_info);
+}
+
 static int
 process_request(struct channel_packet *pkt, struct channel_info *chan_info)
 {
@@ -650,33 +685,54 @@ process_request(struct channel_packet *pkt, struct channel_info *chan_info)
 		RTE_LOG(DEBUG, CHANNEL_MONITOR, "Processing requested cmd for cpu:%d\n",
 			core_num);
 
+		bool valid_unit = true;
+		int scale_res;
+
 		switch (pkt->unit) {
 		case(CPU_POWER_SCALE_MIN):
-			power_manager_scale_core_min(core_num);
+			scale_res = power_manager_scale_core_min(core_num);
 			break;
 		case(CPU_POWER_SCALE_MAX):
-			power_manager_scale_core_max(core_num);
+			scale_res = power_manager_scale_core_max(core_num);
 			break;
 		case(CPU_POWER_SCALE_DOWN):
-			power_manager_scale_core_down(core_num);
+			scale_res = power_manager_scale_core_down(core_num);
 			break;
 		case(CPU_POWER_SCALE_UP):
-			power_manager_scale_core_up(core_num);
+			scale_res = power_manager_scale_core_up(core_num);
 			break;
 		case(CPU_POWER_ENABLE_TURBO):
-			power_manager_enable_turbo_core(core_num);
+			scale_res = power_manager_enable_turbo_core(core_num);
 			break;
 		case(CPU_POWER_DISABLE_TURBO):
-			power_manager_disable_turbo_core(core_num);
+			scale_res = power_manager_disable_turbo_core(core_num);
 			break;
 		default:
+			valid_unit = false;
 			break;
 		}
+
+		if (valid_unit) {
+			ret = send_ack_for_received_cmd(pkt,
+					chan_info,
+					scale_res > 0 ?
+						CPU_POWER_CMD_ACK :
+						CPU_POWER_CMD_NACK);
+			if (ret < 0)
+				RTE_LOG(DEBUG, CHANNEL_MONITOR, "Error during sending ack command.\n");
+		} else
+			RTE_LOG(DEBUG, CHANNEL_MONITOR, "Unexpected unit type.\n");
+
 	}
 
 	if (pkt->command == PKT_POLICY) {
 		RTE_LOG(INFO, CHANNEL_MONITOR, "Processing policy request %s\n",
 				pkt->vm_name);
+		int ret = send_ack_for_received_cmd(pkt,
+				chan_info,
+				CPU_POWER_CMD_ACK);
+		if (ret < 0)
+			RTE_LOG(DEBUG, CHANNEL_MONITOR, "Error during sending ack command.\n");
 		update_policy(pkt);
 		policy_is_set = 1;
 	}
-- 
2.17.2

[dpdk-dev] [PATCH v4 4/4] power: send confirmation cmd to vm guest

[Hajkowski]

From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Use new guest channel API to send confirmation
message for received power command.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 examples/vm_power_manager/channel_monitor.c | 68 +++++++++++++++++++--
 1 file changed, 62 insertions(+), 6 deletions(-)

diff --git a/examples/vm_power_manager/channel_monitor.c b/examples/vm_power_manager/channel_monitor.c
index 7892d75de..ed580b36a 100644
--- a/examples/vm_power_manager/channel_monitor.c
+++ b/examples/vm_power_manager/channel_monitor.c
@@ -627,6 +627,41 @@ apply_policy(struct policy *pol)
 		apply_workload_profile(pol);
 }
 
+static int
+write_binary_packet(struct channel_packet *pkt, struct channel_info *chan_info)
+{
+	int ret, buffer_len = sizeof(*pkt);
+	void *buffer = pkt;
+
+	if (chan_info->fd < 0) {
+		RTE_LOG(ERR, CHANNEL_MONITOR, "Channel is not connected\n");
+		return -1;
+	}
+
+	while (buffer_len > 0) {
+		ret = write(chan_info->fd, buffer, buffer_len);
+		if (ret == -1) {
+			if (errno == EINTR)
+				continue;
+			RTE_LOG(ERR, CHANNEL_MONITOR, "Write function failed due to %s.\n",
+					strerror(errno));
+			return -1;
+		}
+		buffer = (char *)buffer + ret;
+		buffer_len -= ret;
+	}
+	return 0;
+}
+
+static int
+send_ack_for_received_cmd(struct channel_packet *pkt,
+		struct channel_info *chan_info,
+		uint32_t command)
+{
+	pkt->command = command;
+	return write_binary_packet(pkt, chan_info);
+}
+
 static int
 process_request(struct channel_packet *pkt, struct channel_info *chan_info)
 {
@@ -650,33 +685,54 @@ process_request(struct channel_packet *pkt, struct channel_info *chan_info)
 		RTE_LOG(DEBUG, CHANNEL_MONITOR, "Processing requested cmd for cpu:%d\n",
 			core_num);
 
+		bool valid_unit = true;
+		int scale_res;
+
 		switch (pkt->unit) {
 		case(CPU_POWER_SCALE_MIN):
-			power_manager_scale_core_min(core_num);
+			scale_res = power_manager_scale_core_min(core_num);
 			break;
 		case(CPU_POWER_SCALE_MAX):
-			power_manager_scale_core_max(core_num);
+			scale_res = power_manager_scale_core_max(core_num);
 			break;
 		case(CPU_POWER_SCALE_DOWN):
-			power_manager_scale_core_down(core_num);
+			scale_res = power_manager_scale_core_down(core_num);
 			break;
 		case(CPU_POWER_SCALE_UP):
-			power_manager_scale_core_up(core_num);
+			scale_res = power_manager_scale_core_up(core_num);
 			break;
 		case(CPU_POWER_ENABLE_TURBO):
-			power_manager_enable_turbo_core(core_num);
+			scale_res = power_manager_enable_turbo_core(core_num);
 			break;
 		case(CPU_POWER_DISABLE_TURBO):
-			power_manager_disable_turbo_core(core_num);
+			scale_res = power_manager_disable_turbo_core(core_num);
 			break;
 		default:
+			valid_unit = false;
 			break;
 		}
+
+		if (valid_unit) {
+			ret = send_ack_for_received_cmd(pkt,
+					chan_info,
+					scale_res > 0 ?
+						CPU_POWER_CMD_ACK :
+						CPU_POWER_CMD_NACK);
+			if (ret < 0)
+				RTE_LOG(DEBUG, CHANNEL_MONITOR, "Error during sending ack command.\n");
+		} else
+			RTE_LOG(DEBUG, CHANNEL_MONITOR, "Unexpected unit type.\n");
+
 	}
 
 	if (pkt->command == PKT_POLICY) {
 		RTE_LOG(INFO, CHANNEL_MONITOR, "Processing policy request %s\n",
 				pkt->vm_name);
+		int ret = send_ack_for_received_cmd(pkt,
+				chan_info,
+				CPU_POWER_CMD_ACK);
+		if (ret < 0)
+			RTE_LOG(DEBUG, CHANNEL_MONITOR, "Error during sending ack command.\n");
 		update_policy(pkt);
 		policy_is_set = 1;
 	}
-- 
2.17.2

Re: [dpdk-dev] [PATCH v4 3/4] power: process incoming confirmation cmds

[Pattan, Reshma]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Hajkowski
> +		RTE_LOG(DEBUG, GUEST_CLI, "Not expected command has
> been received.\n");

Instead of : "Not expected command has been received ".  How about, Received invalid response from host, expecting NACK/ACK?
But let's hear from others.

> +	int result;
> +	ret = check_response_cmd(res->lcore_id, &result);
> +	if (ret < 0) {
> +		RTE_LOG(DEBUG, GUEST_CLI, "No confirmation for sent msg
> received\n");
> +	} else {
> +		cmdline_printf(cl, "Ack for sent msg received with result:
> %s.\n",
> +				result == 1 ? "SUCCESS" : "ERROR");
> +	}
>  }

In else statement how about just checking for result ACK/NACK and have log saying
received ACK /received NACK for msg

Thanks,
Reshma
 

Re: [dpdk-dev] [PATCH v4 3/4] power: process incoming confirmation cmds

[Pattan, Reshma]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Hajkowski
> +		RTE_LOG(DEBUG, GUEST_CLI, "Not expected command has
> been received.\n");

Instead of : "Not expected command has been received ".  How about, Received invalid response from host, expecting NACK/ACK?
But let's hear from others.

> +	int result;
> +	ret = check_response_cmd(res->lcore_id, &result);
> +	if (ret < 0) {
> +		RTE_LOG(DEBUG, GUEST_CLI, "No confirmation for sent msg
> received\n");
> +	} else {
> +		cmdline_printf(cl, "Ack for sent msg received with result:
> %s.\n",
> +				result == 1 ? "SUCCESS" : "ERROR");
> +	}
>  }

In else statement how about just checking for result ACK/NACK and have log saying
received ACK /received NACK for msg

Thanks,
Reshma