This is an automated email from Gerrit.
"Antonio Borneo <borneo.anto...@gmail.com>" just uploaded a new patch set to
Gerrit, which you can find at https://review.openocd.org/c/openocd/+/8122
-- gerrit
commit 6686f696a7b504b0673979a54a69a6e5bfae608d
Author: Antonio Borneo <borneo.anto...@gmail.com>
Date: Sun Jan 28 14:58:46 2024 +0100
drivers: jtag_vpi: separate host data from socket buffer
The driver's code reuse the same struct jtag_vpi for host data and
for the buffer that will be used during socket read/write.
This creates constraints on endianness swap, order of fields in
the struct, plus potential issues with alignment and padding that
should be addressed through pack compiler's attribute.
Drop this ugly code by splitting the host struct from the socket
buffer:
- populate the socket buffer before socket write;
- populate the host data after socket read.
While there, highlight a possible data usage improvement by using
the same array in struct jtag_vpi for both socket read and write.
Change-Id: Iaa6cd4d3dab45cee9061ec3f9ddeed0180d22840
Signed-off-by: Antonio Borneo <borneo.anto...@gmail.com>
diff --git a/src/jtag/drivers/jtag_vpi.c b/src/jtag/drivers/jtag_vpi.c
index c2b3b08083..6a31df1254 100644
--- a/src/jtag/drivers/jtag_vpi.c
+++ b/src/jtag/drivers/jtag_vpi.c
@@ -31,6 +31,7 @@
#define DEFAULT_SERVER_PORT 5555
#define XFERT_MAX_SIZE 512
+#define VPI_CMD_BUF_SIZE (4 + XFERT_MAX_SIZE + XFERT_MAX_SIZE + 4 + 4)
#define CMD_RESET 0
#define CMD_TMS_SEQ 1
@@ -49,21 +50,16 @@ static int sockfd;
static struct sockaddr_in serv_addr;
/* One jtag_vpi "packet" as sent over a TCP channel. */
+/*
+ * TODO: Buffer_out and buffer_in are used exclusively, so far.
+ * We can collapse them in a single array buffer[XFERT_MAX_SIZE].
+ */
struct vpi_cmd {
- union {
- uint32_t cmd;
- unsigned char cmd_buf[4];
- };
+ uint32_t cmd;
unsigned char buffer_out[XFERT_MAX_SIZE];
unsigned char buffer_in[XFERT_MAX_SIZE];
- union {
- uint32_t length;
- unsigned char length_buf[4];
- };
- union {
- uint32_t nb_bits;
- unsigned char nb_bits_buf[4];
- };
+ uint32_t length;
+ uint32_t nb_bits;
};
static char *jtag_vpi_cmd_to_str(int cmd_num)
@@ -86,6 +82,7 @@ static char *jtag_vpi_cmd_to_str(int cmd_num)
static int jtag_vpi_send_cmd(struct vpi_cmd *vpi)
{
+ uint8_t cmd_buf[VPI_CMD_BUF_SIZE];
int retval;
/* Optional low-level JTAG debug */
@@ -121,12 +118,21 @@ static int jtag_vpi_send_cmd(struct vpi_cmd *vpi)
The choice of little endian goes against usual networking conventions
but is intentional to remain compatible with most older OpenOCD
builds
(i.e. builds on little-endian platforms). */
- h_u32_to_le(vpi->cmd_buf, vpi->cmd);
- h_u32_to_le(vpi->length_buf, vpi->length);
- h_u32_to_le(vpi->nb_bits_buf, vpi->nb_bits);
+ unsigned int idx = 0;
+ h_u32_to_le(cmd_buf + idx, vpi->cmd);
+ idx += 4;
+ memcpy(cmd_buf + idx, vpi->buffer_out, XFERT_MAX_SIZE);
+ idx += XFERT_MAX_SIZE;
+ /* erase buffer_in */
+ memset(cmd_buf + idx, 0, XFERT_MAX_SIZE);
+ idx += XFERT_MAX_SIZE;
+ h_u32_to_le(cmd_buf + idx, vpi->length);
+ idx += 4;
+ h_u32_to_le(cmd_buf + idx, vpi->nb_bits);
+ idx += 4;
retry_write:
- retval = write_socket(sockfd, vpi, sizeof(struct vpi_cmd));
+ retval = write_socket(sockfd, cmd_buf, sizeof(cmd_buf));
if (retval < 0) {
/* Account for the case when socket write is interrupted. */
@@ -144,7 +150,7 @@ retry_write:
/* TODO: Clean way how adapter drivers can report fatal errors
to upper layers of OpenOCD and let it perform an orderly
shutdown? */
exit(-1);
- } else if (retval < (int)sizeof(struct vpi_cmd)) {
+ } else if (retval < (int)sizeof(cmd_buf)) {
/* This means we could not send all data, which is most likely
fatal
for the jtag_vpi connection (the underlying TCP connection
likely not
usable anymore) */
@@ -158,10 +164,12 @@ retry_write:
static int jtag_vpi_receive_cmd(struct vpi_cmd *vpi)
{
+ uint8_t cmd_buf[VPI_CMD_BUF_SIZE];
unsigned bytes_buffered = 0;
- while (bytes_buffered < sizeof(struct vpi_cmd)) {
- int bytes_to_receive = sizeof(struct vpi_cmd) - bytes_buffered;
- int retval = read_socket(sockfd, ((char *)vpi) +
bytes_buffered, bytes_to_receive);
+
+ while (bytes_buffered < sizeof(cmd_buf)) {
+ int bytes_to_receive = sizeof(cmd_buf) - bytes_buffered;
+ int retval = read_socket(sockfd, cmd_buf + bytes_buffered,
bytes_to_receive);
if (retval < 0) {
#ifdef _WIN32
int wsa_err = WSAGetLastError();
@@ -188,9 +196,17 @@ static int jtag_vpi_receive_cmd(struct vpi_cmd *vpi)
}
/* Use little endian when transmitting/receiving jtag_vpi cmds. */
- vpi->cmd = le_to_h_u32(vpi->cmd_buf);
- vpi->length = le_to_h_u32(vpi->length_buf);
- vpi->nb_bits = le_to_h_u32(vpi->nb_bits_buf);
+ unsigned int idx = 0;
+ vpi->cmd = le_to_h_u32(cmd_buf + idx);
+ idx += 4;
+ /* skip buffer_out, we are only receiving */
+ idx += XFERT_MAX_SIZE;
+ memcpy(vpi->buffer_in, cmd_buf + idx, XFERT_MAX_SIZE);
+ idx += XFERT_MAX_SIZE;
+ vpi->length = le_to_h_u32(cmd_buf + idx);
+ idx += 4;
+ vpi->nb_bits = le_to_h_u32(cmd_buf + idx);
+ idx += 4;
return ERROR_OK;
}
--
