2023-07-14 11:07:06

by Saurabh Singh Sengar

[permalink] [raw]
Subject: [PATCH v3 0/3] UIO driver for low speed Hyper-V devices

Hyper-V is adding multiple low speed "speciality" synthetic devices.
Instead of writing a new kernel-level VMBus driver for each device,
make the devices accessible to user space through a UIO-based
hv_vmbus_client driver. Each device can then be supported by a user
space driver. This approach optimizes the development process and
provides flexibility to user space applications to control the key
interactions with the VMBus ring buffer.

The new synthetic devices are low speed devices that don't support
VMBus monitor bits, and so they must use vmbus_setevent() to notify
the host of ring buffer updates. The new driver provides this
functionality along with a configurable ring buffer size.

Moreover, this series of patches incorporates an update to the fcopy
application, enabling it to seamlessly utilize the new interface. The
older fcopy driver and application will be phased out gradually.
Development of other similar userspace drivers is still underway.

Moreover, this patch series adds a new implementation of the fcopy
application that uses the new UIO driver. The older fcopy driver and
application will be phased out gradually. Development of other similar
userspace drivers is still underway.

[V3]
- Removed ringbuffer sysfs entry and used uio framework for mmap
- Remove ".id_table = NULL"
- kasprintf -> devm_kasprintf
- Change global variable ring_size to per device
- More checks on value which can be set for ring_size
- Remove driverctl, and used echo command instead for driver documentation
- Remove unnecessary one time use macros
- Change kernel version and date for sysfs documentation
- Update documentation.
- Made ring buffer data offset depend on page size
- remove rte_smp_rwmb macro and reused rte_compiler_barrier instead
- Added legal counsel sign-off
- simplify mmap
- Removed "Link:" tag
- Improve cover letter and commit messages
- Improve debug prints
- Instead of hardcoded instance id, query from class id sysfs
- Set the ring_size value from application
- new application compilation dependent on x86
- Not removing the old driver and application for backward compatibility

[V2]
- Update driver info in Documentation/driver-api/uio-howto.rst
- Update ring_size sysfs info in Documentation/ABI/stable/sysfs-bus-vmbus
- Remove DRIVER_VERSION
- Remove refcnt
- scnprintf -> sysfs_emit
- sysfs_create_file -> ATTRIBUTE_GROUPS + ".driver.groups";
- sysfs_create_bin_file -> device_create_bin_file
- dev_notice -> dev_err
- remove MODULE_VERSION
- simpler sysfs path, less parsing

Saurabh Sengar (3):
uio: Add hv_vmbus_client driver
tools: hv: Add vmbus_bufring
tools: hv: Add new fcopy application based on uio driver

Documentation/ABI/stable/sysfs-bus-vmbus | 10 +
Documentation/driver-api/uio-howto.rst | 54 +++
drivers/uio/Kconfig | 12 +
drivers/uio/Makefile | 1 +
drivers/uio/uio_hv_vmbus_client.c | 218 +++++++++
tools/hv/Build | 2 +
tools/hv/Makefile | 21 +-
tools/hv/hv_fcopy_uio_daemon.c | 578 +++++++++++++++++++++++
tools/hv/vmbus_bufring.c | 297 ++++++++++++
tools/hv/vmbus_bufring.h | 154 ++++++
10 files changed, 1346 insertions(+), 1 deletion(-)
create mode 100644 drivers/uio/uio_hv_vmbus_client.c
create mode 100644 tools/hv/hv_fcopy_uio_daemon.c
create mode 100644 tools/hv/vmbus_bufring.c
create mode 100644 tools/hv/vmbus_bufring.h

--
2.34.1



2023-07-14 11:15:25

by Saurabh Singh Sengar

[permalink] [raw]
Subject: [PATCH v3 3/3] tools: hv: Add new fcopy application based on uio driver

Implement the file copy service for Linux guests on Hyper-V. This
permits the host to copy a file (over VMBus) into the guest. This
facility is part of "guest integration services" supported on the
Hyper-V platform.

Here is a link that provides additional details on this functionality:

http://technet.microsoft.com/en-us/library/dn464282.aspx

This new fcopy application uses uio_hv_vmbus_client driver which
makes the earlier hv_util based driver and application obsolete.

Signed-off-by: Saurabh Sengar <[email protected]>
---
[V3]
- Improve cover letter and commit messages
- Improve debug prints
- Instead of hardcoded instance id, query from class id sysfs
- Set the ring_size value from application
- Update the application to mmap /dev/uio instead of sysfs
- new application compilation dependent on x86

[V2]
- simpler sysfs path

tools/hv/Build | 1 +
tools/hv/Makefile | 10 +-
tools/hv/hv_fcopy_uio_daemon.c | 578 +++++++++++++++++++++++++++++++++
3 files changed, 588 insertions(+), 1 deletion(-)
create mode 100644 tools/hv/hv_fcopy_uio_daemon.c

diff --git a/tools/hv/Build b/tools/hv/Build
index 2a667d3d94cb..efcbb74a0d23 100644
--- a/tools/hv/Build
+++ b/tools/hv/Build
@@ -2,3 +2,4 @@ hv_kvp_daemon-y += hv_kvp_daemon.o
hv_vss_daemon-y += hv_vss_daemon.o
hv_fcopy_daemon-y += hv_fcopy_daemon.o
vmbus_bufring-y += vmbus_bufring.o
+hv_fcopy_uio_daemon-y += hv_fcopy_uio_daemon.o
diff --git a/tools/hv/Makefile b/tools/hv/Makefile
index 33cf488fd20f..678c6c450a53 100644
--- a/tools/hv/Makefile
+++ b/tools/hv/Makefile
@@ -21,8 +21,10 @@ override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include

ifeq ($(SRCARCH),x86)
ALL_LIBS := libvmbus_bufring.a
-endif
+ALL_TARGETS := hv_kvp_daemon hv_vss_daemon hv_fcopy_daemon hv_fcopy_uio_daemon
+else
ALL_TARGETS := hv_kvp_daemon hv_vss_daemon hv_fcopy_daemon
+endif
ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS)) $(patsubst %,$(OUTPUT)%,$(ALL_LIBS))

ALL_SCRIPTS := hv_get_dhcp_info.sh hv_get_dns_info.sh hv_set_ifconfig.sh
@@ -56,6 +58,12 @@ $(HV_FCOPY_DAEMON_IN): FORCE
$(OUTPUT)hv_fcopy_daemon: $(HV_FCOPY_DAEMON_IN)
$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@

+HV_FCOPY_UIO_DAEMON_IN := $(OUTPUT)hv_fcopy_uio_daemon-in.o
+$(HV_FCOPY_UIO_DAEMON_IN): FORCE
+ $(Q)$(MAKE) $(build)=hv_fcopy_uio_daemon
+$(OUTPUT)hv_fcopy_uio_daemon: $(HV_FCOPY_UIO_DAEMON_IN) libvmbus_bufring.a
+ $(QUIET_LINK)$(CC) -lm $< -L. -lvmbus_bufring -o $@
+
clean:
rm -f $(ALL_PROGRAMS)
find $(or $(OUTPUT),.) -name '*.o' -delete -o -name '\.*.d' -delete
diff --git a/tools/hv/hv_fcopy_uio_daemon.c b/tools/hv/hv_fcopy_uio_daemon.c
new file mode 100644
index 000000000000..e8618a30dc7e
--- /dev/null
+++ b/tools/hv/hv_fcopy_uio_daemon.c
@@ -0,0 +1,578 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * An implementation of host to guest copy functionality for Linux.
+ *
+ * Copyright (C) 2023, Microsoft, Inc.
+ *
+ * Author : K. Y. Srinivasan <[email protected]>
+ * Author : Saurabh Sengar <[email protected]>
+ *
+ */
+
+#include <dirent.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <getopt.h>
+#include <locale.h>
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syslog.h>
+#include <unistd.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <linux/hyperv.h>
+#include "vmbus_bufring.h"
+
+#define ICMSGTYPE_NEGOTIATE 0
+#define ICMSGTYPE_FCOPY 7
+
+#define WIN8_SRV_MAJOR 1
+#define WIN8_SRV_MINOR 1
+#define WIN8_SRV_VERSION (WIN8_SRV_MAJOR << 16 | WIN8_SRV_MINOR)
+
+#define MAX_PATH_LEN 300
+#define MAX_LINE_LEN 40
+#define DEVICES_SYSFS "/sys/bus/vmbus/devices"
+#define FCOPY_CLASS_ID "34d14be3-dee4-41c8-9ae7-6b174977c192"
+
+#define FCOPY_VER_COUNT 1
+static const int fcopy_versions[] = {
+ WIN8_SRV_VERSION
+};
+
+#define FW_VER_COUNT 1
+static const int fw_versions[] = {
+ UTIL_FW_VERSION
+};
+
+#define HV_RING_SIZE (4 * 4096)
+
+unsigned char desc[HV_RING_SIZE];
+
+static int target_fd;
+static char target_fname[PATH_MAX];
+static unsigned long long filesize;
+
+static int hv_fcopy_create_file(char *file_name, char *path_name, __u32 flags)
+{
+ int error = HV_E_FAIL;
+ char *q, *p;
+
+ filesize = 0;
+ p = (char *)path_name;
+ snprintf(target_fname, sizeof(target_fname), "%s/%s",
+ (char *)path_name, (char *)file_name);
+
+ /*
+ * Check to see if the path is already in place; if not,
+ * create if required.
+ */
+ while ((q = strchr(p, '/')) != NULL) {
+ if (q == p) {
+ p++;
+ continue;
+ }
+ *q = '\0';
+ if (access(path_name, F_OK)) {
+ if (flags & CREATE_PATH) {
+ if (mkdir(path_name, 0755)) {
+ syslog(LOG_ERR, "Failed to create %s",
+ path_name);
+ goto done;
+ }
+ } else {
+ syslog(LOG_ERR, "Invalid path: %s", path_name);
+ goto done;
+ }
+ }
+ p = q + 1;
+ *q = '/';
+ }
+
+ if (!access(target_fname, F_OK)) {
+ syslog(LOG_INFO, "File: %s exists", target_fname);
+ if (!(flags & OVER_WRITE)) {
+ error = HV_ERROR_ALREADY_EXISTS;
+ goto done;
+ }
+ }
+
+ target_fd = open(target_fname,
+ O_RDWR | O_CREAT | O_TRUNC | O_CLOEXEC, 0744);
+ if (target_fd == -1) {
+ syslog(LOG_INFO, "Open Failed: %s", strerror(errno));
+ goto done;
+ }
+
+ error = 0;
+done:
+ if (error)
+ target_fname[0] = '\0';
+ return error;
+}
+
+static int hv_copy_data(struct hv_do_fcopy *cpmsg)
+{
+ ssize_t bytes_written;
+ int ret = 0;
+
+ bytes_written = pwrite(target_fd, cpmsg->data, cpmsg->size,
+ cpmsg->offset);
+
+ filesize += cpmsg->size;
+ if (bytes_written != cpmsg->size) {
+ switch (errno) {
+ case ENOSPC:
+ ret = HV_ERROR_DISK_FULL;
+ break;
+ default:
+ ret = HV_E_FAIL;
+ break;
+ }
+ syslog(LOG_ERR, "pwrite failed to write %llu bytes: %ld (%s)",
+ filesize, (long)bytes_written, strerror(errno));
+ }
+
+ return ret;
+}
+
+/*
+ * Reset target_fname to "" in the two below functions for hibernation: if
+ * the fcopy operation is aborted by hibernation, the daemon should remove the
+ * partially-copied file; to achieve this, the hv_utils driver always fakes a
+ * CANCEL_FCOPY message upon suspend, and later when the VM resumes back,
+ * the daemon calls hv_copy_cancel() to remove the file; if a file is copied
+ * successfully before suspend, hv_copy_finished() must reset target_fname to
+ * avoid that the file can be incorrectly removed upon resume, since the faked
+ * CANCEL_FCOPY message is spurious in this case.
+ */
+static int hv_copy_finished(void)
+{
+ close(target_fd);
+ target_fname[0] = '\0';
+ return 0;
+}
+
+static void print_usage(char *argv[])
+{
+ fprintf(stderr, "Usage: %s [options]\n"
+ "Options are:\n"
+ " -n, --no-daemon stay in foreground, don't daemonize\n"
+ " -h, --help print this help\n", argv[0]);
+}
+
+static bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp, unsigned char *buf,
+ unsigned int buflen, const int *fw_version, int fw_vercnt,
+ const int *srv_version, int srv_vercnt,
+ int *nego_fw_version, int *nego_srv_version)
+{
+ int icframe_major, icframe_minor;
+ int icmsg_major, icmsg_minor;
+ int fw_major, fw_minor;
+ int srv_major, srv_minor;
+ int i, j;
+ bool found_match = false;
+ struct icmsg_negotiate *negop;
+
+ /* Check that there's enough space for icframe_vercnt, icmsg_vercnt */
+ if (buflen < ICMSG_HDR + offsetof(struct icmsg_negotiate, reserved)) {
+ syslog(LOG_ERR, "Invalid icmsg negotiate");
+ return false;
+ }
+
+ icmsghdrp->icmsgsize = 0x10;
+ negop = (struct icmsg_negotiate *)&buf[ICMSG_HDR];
+
+ icframe_major = negop->icframe_vercnt;
+ icframe_minor = 0;
+
+ icmsg_major = negop->icmsg_vercnt;
+ icmsg_minor = 0;
+
+ /* Validate negop packet */
+ if (icframe_major > IC_VERSION_NEGOTIATION_MAX_VER_COUNT ||
+ icmsg_major > IC_VERSION_NEGOTIATION_MAX_VER_COUNT ||
+ ICMSG_NEGOTIATE_PKT_SIZE(icframe_major, icmsg_major) > buflen) {
+ syslog(LOG_ERR, "Invalid icmsg negotiate - icframe_major: %u, icmsg_major: %u\n",
+ icframe_major, icmsg_major);
+ goto fw_error;
+ }
+
+ /*
+ * Select the framework version number we will
+ * support.
+ */
+
+ for (i = 0; i < fw_vercnt; i++) {
+ fw_major = (fw_version[i] >> 16);
+ fw_minor = (fw_version[i] & 0xFFFF);
+
+ for (j = 0; j < negop->icframe_vercnt; j++) {
+ if (negop->icversion_data[j].major == fw_major &&
+ negop->icversion_data[j].minor == fw_minor) {
+ icframe_major = negop->icversion_data[j].major;
+ icframe_minor = negop->icversion_data[j].minor;
+ found_match = true;
+ break;
+ }
+ }
+
+ if (found_match)
+ break;
+ }
+
+ if (!found_match)
+ goto fw_error;
+
+ found_match = false;
+
+ for (i = 0; i < srv_vercnt; i++) {
+ srv_major = (srv_version[i] >> 16);
+ srv_minor = (srv_version[i] & 0xFFFF);
+
+ for (j = negop->icframe_vercnt;
+ (j < negop->icframe_vercnt + negop->icmsg_vercnt);
+ j++) {
+ if (negop->icversion_data[j].major == srv_major &&
+ negop->icversion_data[j].minor == srv_minor) {
+ icmsg_major = negop->icversion_data[j].major;
+ icmsg_minor = negop->icversion_data[j].minor;
+ found_match = true;
+ break;
+ }
+ }
+
+ if (found_match)
+ break;
+ }
+
+ /*
+ * Respond with the framework and service
+ * version numbers we can support.
+ */
+fw_error:
+ if (!found_match) {
+ negop->icframe_vercnt = 0;
+ negop->icmsg_vercnt = 0;
+ } else {
+ negop->icframe_vercnt = 1;
+ negop->icmsg_vercnt = 1;
+ }
+
+ if (nego_fw_version)
+ *nego_fw_version = (icframe_major << 16) | icframe_minor;
+
+ if (nego_srv_version)
+ *nego_srv_version = (icmsg_major << 16) | icmsg_minor;
+
+ negop->icversion_data[0].major = icframe_major;
+ negop->icversion_data[0].minor = icframe_minor;
+ negop->icversion_data[1].major = icmsg_major;
+ negop->icversion_data[1].minor = icmsg_minor;
+
+ return found_match;
+}
+
+static void wcstoutf8(char *dest, const __u16 *src, size_t dest_size)
+{
+ size_t len = 0;
+
+ while (len < dest_size) {
+ if (src[len] < 0x80)
+ dest[len++] = (char)(*src++);
+ else
+ dest[len++] = 'X';
+ }
+
+ dest[len] = '\0';
+}
+
+static int hv_fcopy_start(struct hv_start_fcopy *smsg_in)
+{
+ setlocale(LC_ALL, "en_US.utf8");
+ size_t file_size, path_size;
+ char *file_name, *path_name;
+ char *in_file_name = (char *)smsg_in->file_name;
+ char *in_path_name = (char *)smsg_in->path_name;
+
+ file_size = wcstombs(NULL, (const wchar_t *restrict)in_file_name, 0) + 1;
+ path_size = wcstombs(NULL, (const wchar_t *restrict)in_path_name, 0) + 1;
+
+ file_name = (char *)malloc(file_size * sizeof(char));
+ path_name = (char *)malloc(path_size * sizeof(char));
+
+ wcstoutf8(file_name, (__u16 *)in_file_name, file_size);
+ wcstoutf8(path_name, (__u16 *)in_path_name, path_size);
+
+ return hv_fcopy_create_file(file_name, path_name, smsg_in->copy_flags);
+}
+
+static int hv_fcopy_send_data(struct hv_fcopy_hdr *fcopy_msg, int recvlen)
+{
+ int operation = fcopy_msg->operation;
+
+ /*
+ * The strings sent from the host are encoded in
+ * utf16; convert it to utf8 strings.
+ * The host assures us that the utf16 strings will not exceed
+ * the max lengths specified. We will however, reserve room
+ * for the string terminating character - in the utf16s_utf8s()
+ * function we limit the size of the buffer where the converted
+ * string is placed to W_MAX_PATH -1 to guarantee
+ * that the strings can be properly terminated!
+ */
+
+ switch (operation) {
+ case START_FILE_COPY:
+ return hv_fcopy_start((struct hv_start_fcopy *)fcopy_msg);
+ case WRITE_TO_FILE:
+ return hv_copy_data((struct hv_do_fcopy *)fcopy_msg);
+ case COMPLETE_FCOPY:
+ return hv_copy_finished();
+ }
+
+ return HV_E_FAIL;
+}
+
+/* process the packet recv from host */
+static int fcopy_pkt_process(struct vmbus_br *txbr)
+{
+ int ret, offset, pktlen;
+ int fcopy_srv_version;
+ const struct vmbus_chanpkt_hdr *pkt;
+ struct hv_fcopy_hdr *fcopy_msg;
+ struct icmsg_hdr *icmsghdr;
+
+ pkt = (const struct vmbus_chanpkt_hdr *)desc;
+ offset = pkt->hlen << 3;
+ pktlen = (pkt->tlen << 3) - offset;
+ icmsghdr = (struct icmsg_hdr *)&desc[offset + sizeof(struct vmbuspipe_hdr)];
+ icmsghdr->status = HV_E_FAIL;
+
+ if (icmsghdr->icmsgtype == ICMSGTYPE_NEGOTIATE) {
+ if (vmbus_prep_negotiate_resp(icmsghdr, desc + offset, pktlen, fw_versions,
+ FW_VER_COUNT, fcopy_versions, FCOPY_VER_COUNT,
+ NULL, &fcopy_srv_version)) {
+ syslog(LOG_INFO, "FCopy IC version %d.%d",
+ fcopy_srv_version >> 16, fcopy_srv_version & 0xFFFF);
+ icmsghdr->status = 0;
+ }
+ } else if (icmsghdr->icmsgtype == ICMSGTYPE_FCOPY) {
+ /* Ensure recvlen is big enough to contain hv_fcopy_hdr */
+ if (pktlen < ICMSG_HDR + sizeof(struct hv_fcopy_hdr)) {
+ syslog(LOG_ERR, "Invalid Fcopy hdr. Packet length too small: %u",
+ pktlen);
+ return -ENOBUFS;
+ }
+
+ fcopy_msg = (struct hv_fcopy_hdr *)&desc[offset + ICMSG_HDR];
+ icmsghdr->status = hv_fcopy_send_data(fcopy_msg, pktlen);
+ }
+
+ icmsghdr->icflags = ICMSGHDRFLAG_TRANSACTION | ICMSGHDRFLAG_RESPONSE;
+ ret = rte_vmbus_chan_send(txbr, 0x6, desc + offset, pktlen, 0);
+ if (ret) {
+ syslog(LOG_ERR, "Write to ringbuffer failed err: %d", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void fcopy_get_first_folder(char *path, char *chan_no)
+{
+ DIR *dir = opendir(path);
+ struct dirent *entry;
+
+ if (!dir) {
+ syslog(LOG_ERR, "Failed to open directory (errno=%s).\n", strerror(errno));
+ return;
+ }
+
+ while ((entry = readdir(dir)) != NULL) {
+ if (entry->d_type == DT_DIR && strcmp(entry->d_name, ".") != 0 &&
+ strcmp(entry->d_name, "..") != 0) {
+ strcpy(chan_no, entry->d_name);
+ break;
+ }
+ }
+
+ closedir(dir);
+}
+
+static void fcopy_set_ring_size(char *path, char *inst, int size)
+{
+ char ring_size_path[MAX_PATH_LEN] = {0};
+ FILE *fd;
+
+ snprintf(ring_size_path, sizeof(ring_size_path), "%s/%s/%s", path, inst, "ring_size");
+ fd = fopen(ring_size_path, "w");
+ if (!fd) {
+ syslog(LOG_WARNING, "Failed to open ring_size file (errno=%s).\n", strerror(errno));
+ return;
+ }
+ fprintf(fd, "%d", size);
+ fclose(fd);
+}
+
+static char *fcopy_read_sysfs(char *path, char *buf, int len)
+{
+ FILE *fd;
+ char *ret;
+
+ fd = fopen(path, "r");
+ if (!fd)
+ return NULL;
+
+ ret = fgets(buf, len, fd);
+ fclose(fd);
+
+ return ret;
+}
+
+static int fcopy_get_instance_id(char *path, char *class_id, char *inst)
+{
+ DIR *dir = opendir(path);
+ struct dirent *entry;
+ char tmp_path[MAX_PATH_LEN] = {0};
+ char line[MAX_LINE_LEN];
+
+ if (!dir) {
+ syslog(LOG_ERR, "Failed to open directory (errno=%s).\n", strerror(errno));
+ return -EINVAL;
+ }
+
+ while ((entry = readdir(dir)) != NULL) {
+ if (entry->d_type == DT_LNK && strcmp(entry->d_name, ".") != 0 &&
+ strcmp(entry->d_name, "..") != 0) {
+ /* search for the sysfs path with matching class_id */
+ snprintf(tmp_path, sizeof(tmp_path), "%s/%s/%s",
+ path, entry->d_name, "class_id");
+ if (!fcopy_read_sysfs(tmp_path, line, MAX_LINE_LEN))
+ continue;
+
+ /* class id matches, now fetch the instance id from device_id */
+ if (strstr(line, class_id)) {
+ snprintf(tmp_path, sizeof(tmp_path), "%s/%s/%s",
+ path, entry->d_name, "device_id");
+ if (!fcopy_read_sysfs(tmp_path, line, MAX_LINE_LEN))
+ continue;
+ /* remove braces */
+ strncpy(inst, line + 1, strlen(line) - 3);
+ break;
+ }
+ }
+ }
+
+ closedir(dir);
+ return 0;
+}
+
+int main(int argc, char *argv[])
+{
+ int fcopy_fd = -1, tmp = 1;
+ int daemonize = 1, long_index = 0, opt, ret = -EINVAL;
+ struct vmbus_br txbr, rxbr;
+ void *ring;
+ uint32_t len = HV_RING_SIZE;
+ char uio_name[10] = {0};
+ char uio_dev_path[15] = {0};
+ char uio_path[MAX_PATH_LEN] = {0};
+ char inst[MAX_LINE_LEN] = {0};
+
+ static struct option long_options[] = {
+ {"help", no_argument, 0, 'h' },
+ {"no-daemon", no_argument, 0, 'n' },
+ {0, 0, 0, 0 }
+ };
+
+ while ((opt = getopt_long(argc, argv, "hn", long_options,
+ &long_index)) != -1) {
+ switch (opt) {
+ case 'n':
+ daemonize = 0;
+ break;
+ case 'h':
+ default:
+ print_usage(argv);
+ exit(EXIT_FAILURE);
+ }
+ }
+
+ if (daemonize && daemon(1, 0)) {
+ syslog(LOG_ERR, "daemon() failed; error: %s", strerror(errno));
+ exit(EXIT_FAILURE);
+ }
+
+ openlog("HV_UIO_FCOPY", 0, LOG_USER);
+ syslog(LOG_INFO, "starting; pid is:%d", getpid());
+
+ /* get instance id */
+ if (fcopy_get_instance_id(DEVICES_SYSFS, FCOPY_CLASS_ID, inst))
+ exit(EXIT_FAILURE);
+
+ /* set ring_size value */
+ fcopy_set_ring_size(DEVICES_SYSFS, inst, HV_RING_SIZE);
+
+ /* get /dev/uioX dev path and open it */
+ snprintf(uio_path, sizeof(uio_path), "%s/%s/%s", DEVICES_SYSFS, inst, "uio");
+ fcopy_get_first_folder(uio_path, uio_name);
+ snprintf(uio_dev_path, sizeof(uio_dev_path), "/dev/%s", uio_name);
+ fcopy_fd = open(uio_dev_path, O_RDWR);
+
+ if (fcopy_fd < 0) {
+ syslog(LOG_ERR, "open %s failed; error: %d %s",
+ uio_dev_path, errno, strerror(errno));
+ syslog(LOG_ERR, "Please make sure module uio_hv_vmbus_client is loaded and" \
+ " device is not used by any other application\n");
+ ret = fcopy_fd;
+ exit(EXIT_FAILURE);
+ }
+
+ ring = mmap(NULL, 2 * HV_RING_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fcopy_fd, 0);
+ if (ring == MAP_FAILED) {
+ ret = errno;
+ syslog(LOG_ERR, "mmap ringbuffer failed; error: %d %s", ret, strerror(ret));
+ goto close;
+ }
+ vmbus_br_setup(&txbr, ring, HV_RING_SIZE);
+ vmbus_br_setup(&rxbr, (char *)ring + HV_RING_SIZE, HV_RING_SIZE);
+
+ while (1) {
+ /*
+ * In this loop we process fcopy messages after the
+ * handshake is complete.
+ */
+ ret = pread(fcopy_fd, &tmp, sizeof(int), 0);
+ if (ret < 0) {
+ syslog(LOG_ERR, "pread failed: %s", strerror(errno));
+ continue;
+ }
+
+ len = HV_RING_SIZE;
+ ret = rte_vmbus_chan_recv_raw(&rxbr, desc, &len);
+ if (unlikely(ret <= 0)) {
+ /* This indicates a failure to communicate (or worse) */
+ syslog(LOG_ERR, "VMBus channel recv error: %d", ret);
+ } else {
+ ret = fcopy_pkt_process(&txbr);
+ if (ret < 0)
+ goto close;
+
+ /* Signal host */
+ tmp = 1;
+ if ((write(fcopy_fd, &tmp, sizeof(int))) != sizeof(int)) {
+ ret = errno;
+ syslog(LOG_ERR, "Registration failed: %s\n", strerror(ret));
+ goto close;
+ }
+ }
+ }
+close:
+ close(fcopy_fd);
+ return ret;
+}
--
2.34.1


2023-08-02 22:21:04

by Michael Kelley (LINUX)

[permalink] [raw]
Subject: RE: [PATCH v3 3/3] tools: hv: Add new fcopy application based on uio driver

From: Saurabh Sengar <[email protected]> Sent: Friday, July 14, 2023 3:26 AM
>
> Implement the file copy service for Linux guests on Hyper-V. This
> permits the host to copy a file (over VMBus) into the guest. This
> facility is part of "guest integration services" supported on the
> Hyper-V platform.
>
> Here is a link that provides additional details on this functionality:
>
> https://learn.microsoft.com/en-us/powershell/module/hyper-v/copy-vmfile?view=windowsserver2022-ps
>
> This new fcopy application uses uio_hv_vmbus_client driver which
> makes the earlier hv_util based driver and application obsolete.
>
> Signed-off-by: Saurabh Sengar <[email protected]>
> ---
> [V3]
> - Improve cover letter and commit messages
> - Improve debug prints
> - Instead of hardcoded instance id, query from class id sysfs
> - Set the ring_size value from application
> - Update the application to mmap /dev/uio instead of sysfs
> - new application compilation dependent on x86
>
> [V2]
> - simpler sysfs path
>
> tools/hv/Build | 1 +
> tools/hv/Makefile | 10 +-
> tools/hv/hv_fcopy_uio_daemon.c | 578 +++++++++++++++++++++++++++++++++
> 3 files changed, 588 insertions(+), 1 deletion(-)
> create mode 100644 tools/hv/hv_fcopy_uio_daemon.c
>
> diff --git a/tools/hv/Build b/tools/hv/Build
> index 2a667d3d94cb..efcbb74a0d23 100644
> --- a/tools/hv/Build
> +++ b/tools/hv/Build
> @@ -2,3 +2,4 @@ hv_kvp_daemon-y += hv_kvp_daemon.o
> hv_vss_daemon-y += hv_vss_daemon.o
> hv_fcopy_daemon-y += hv_fcopy_daemon.o
> vmbus_bufring-y += vmbus_bufring.o
> +hv_fcopy_uio_daemon-y += hv_fcopy_uio_daemon.o
> diff --git a/tools/hv/Makefile b/tools/hv/Makefile
> index 33cf488fd20f..678c6c450a53 100644
> --- a/tools/hv/Makefile
> +++ b/tools/hv/Makefile
> @@ -21,8 +21,10 @@ override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -
> I$(OUTPUT)include
>
> ifeq ($(SRCARCH),x86)
> ALL_LIBS := libvmbus_bufring.a
> -endif
> +ALL_TARGETS := hv_kvp_daemon hv_vss_daemon hv_fcopy_daemon
> hv_fcopy_uio_daemon
> +else
> ALL_TARGETS := hv_kvp_daemon hv_vss_daemon hv_fcopy_daemon
> +endif
> ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS)) $(patsubst
> %,$(OUTPUT)%,$(ALL_LIBS))
>
> ALL_SCRIPTS := hv_get_dhcp_info.sh hv_get_dns_info.sh hv_set_ifconfig.sh
> @@ -56,6 +58,12 @@ $(HV_FCOPY_DAEMON_IN): FORCE
> $(OUTPUT)hv_fcopy_daemon: $(HV_FCOPY_DAEMON_IN)
> $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
>
> +HV_FCOPY_UIO_DAEMON_IN := $(OUTPUT)hv_fcopy_uio_daemon-in.o
> +$(HV_FCOPY_UIO_DAEMON_IN): FORCE
> + $(Q)$(MAKE) $(build)=hv_fcopy_uio_daemon
> +$(OUTPUT)hv_fcopy_uio_daemon: $(HV_FCOPY_UIO_DAEMON_IN)
> libvmbus_bufring.a
> + $(QUIET_LINK)$(CC) -lm $< -L. -lvmbus_bufring -o $@
> +
> clean:
> rm -f $(ALL_PROGRAMS)
> find $(or $(OUTPUT),.) -name '*.o' -delete -o -name '\.*.d' -delete
> diff --git a/tools/hv/hv_fcopy_uio_daemon.c b/tools/hv/hv_fcopy_uio_daemon.c
> new file mode 100644
> index 000000000000..e8618a30dc7e
> --- /dev/null
> +++ b/tools/hv/hv_fcopy_uio_daemon.c
> @@ -0,0 +1,578 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * An implementation of host to guest copy functionality for Linux.
> + *
> + * Copyright (C) 2023, Microsoft, Inc.
> + *
> + * Author : K. Y. Srinivasan <[email protected]>
> + * Author : Saurabh Sengar <[email protected]>
> + *
> + */
> +
> +#include <dirent.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <getopt.h>
> +#include <locale.h>
> +#include <stdbool.h>
> +#include <stddef.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <syslog.h>
> +#include <unistd.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <linux/hyperv.h>
> +#include "vmbus_bufring.h"
> +
> +#define ICMSGTYPE_NEGOTIATE 0
> +#define ICMSGTYPE_FCOPY 7
> +
> +#define WIN8_SRV_MAJOR 1
> +#define WIN8_SRV_MINOR 1
> +#define WIN8_SRV_VERSION (WIN8_SRV_MAJOR << 16 | WIN8_SRV_MINOR)
> +
> +#define MAX_PATH_LEN 300
> +#define MAX_LINE_LEN 40
> +#define DEVICES_SYSFS "/sys/bus/vmbus/devices"
> +#define FCOPY_CLASS_ID "34d14be3-dee4-41c8-9ae7-6b174977c192"
> +
> +#define FCOPY_VER_COUNT 1
> +static const int fcopy_versions[] = {
> + WIN8_SRV_VERSION
> +};
> +
> +#define FW_VER_COUNT 1
> +static const int fw_versions[] = {
> + UTIL_FW_VERSION
> +};
> +
> +#define HV_RING_SIZE (4 * 4096)
> +
> +unsigned char desc[HV_RING_SIZE];
> +
> +static int target_fd;
> +static char target_fname[PATH_MAX];
> +static unsigned long long filesize;
> +
> +static int hv_fcopy_create_file(char *file_name, char *path_name, __u32 flags)
> +{
> + int error = HV_E_FAIL;
> + char *q, *p;
> +
> + filesize = 0;
> + p = (char *)path_name;
> + snprintf(target_fname, sizeof(target_fname), "%s/%s",
> + (char *)path_name, (char *)file_name);
> +
> + /*
> + * Check to see if the path is already in place; if not,
> + * create if required.
> + */
> + while ((q = strchr(p, '/')) != NULL) {
> + if (q == p) {
> + p++;
> + continue;
> + }
> + *q = '\0';
> + if (access(path_name, F_OK)) {
> + if (flags & CREATE_PATH) {
> + if (mkdir(path_name, 0755)) {
> + syslog(LOG_ERR, "Failed to create %s",
> + path_name);
> + goto done;
> + }
> + } else {
> + syslog(LOG_ERR, "Invalid path: %s", path_name);
> + goto done;
> + }
> + }
> + p = q + 1;
> + *q = '/';
> + }
> +
> + if (!access(target_fname, F_OK)) {
> + syslog(LOG_INFO, "File: %s exists", target_fname);
> + if (!(flags & OVER_WRITE)) {
> + error = HV_ERROR_ALREADY_EXISTS;
> + goto done;
> + }
> + }
> +
> + target_fd = open(target_fname,
> + O_RDWR | O_CREAT | O_TRUNC | O_CLOEXEC, 0744);
> + if (target_fd == -1) {
> + syslog(LOG_INFO, "Open Failed: %s", strerror(errno));
> + goto done;
> + }
> +
> + error = 0;
> +done:
> + if (error)
> + target_fname[0] = '\0';
> + return error;
> +}
> +
> +static int hv_copy_data(struct hv_do_fcopy *cpmsg)
> +{
> + ssize_t bytes_written;
> + int ret = 0;
> +
> + bytes_written = pwrite(target_fd, cpmsg->data, cpmsg->size,
> + cpmsg->offset);
> +
> + filesize += cpmsg->size;
> + if (bytes_written != cpmsg->size) {
> + switch (errno) {
> + case ENOSPC:
> + ret = HV_ERROR_DISK_FULL;
> + break;
> + default:
> + ret = HV_E_FAIL;
> + break;
> + }
> + syslog(LOG_ERR, "pwrite failed to write %llu bytes: %ld (%s)",
> + filesize, (long)bytes_written, strerror(errno));
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * Reset target_fname to "" in the two below functions for hibernation: if
> + * the fcopy operation is aborted by hibernation, the daemon should remove the
> + * partially-copied file; to achieve this, the hv_utils driver always fakes a
> + * CANCEL_FCOPY message upon suspend, and later when the VM resumes back,
> + * the daemon calls hv_copy_cancel() to remove the file; if a file is copied
> + * successfully before suspend, hv_copy_finished() must reset target_fname to
> + * avoid that the file can be incorrectly removed upon resume, since the faked
> + * CANCEL_FCOPY message is spurious in this case.
> + */
> +static int hv_copy_finished(void)
> +{
> + close(target_fd);
> + target_fname[0] = '\0';
> + return 0;
> +}
> +
> +static void print_usage(char *argv[])
> +{
> + fprintf(stderr, "Usage: %s [options]\n"
> + "Options are:\n"
> + " -n, --no-daemon stay in foreground, don't daemonize\n"
> + " -h, --help print this help\n", argv[0]);
> +}
> +
> +static bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp, unsigned char *buf,
> + unsigned int buflen, const int *fw_version, int fw_vercnt,
> + const int *srv_version, int srv_vercnt,
> + int *nego_fw_version, int *nego_srv_version)
> +{
> + int icframe_major, icframe_minor;
> + int icmsg_major, icmsg_minor;
> + int fw_major, fw_minor;
> + int srv_major, srv_minor;
> + int i, j;
> + bool found_match = false;
> + struct icmsg_negotiate *negop;
> +
> + /* Check that there's enough space for icframe_vercnt, icmsg_vercnt */
> + if (buflen < ICMSG_HDR + offsetof(struct icmsg_negotiate, reserved)) {
> + syslog(LOG_ERR, "Invalid icmsg negotiate");
> + return false;
> + }
> +
> + icmsghdrp->icmsgsize = 0x10;
> + negop = (struct icmsg_negotiate *)&buf[ICMSG_HDR];
> +
> + icframe_major = negop->icframe_vercnt;
> + icframe_minor = 0;
> +
> + icmsg_major = negop->icmsg_vercnt;
> + icmsg_minor = 0;
> +
> + /* Validate negop packet */
> + if (icframe_major > IC_VERSION_NEGOTIATION_MAX_VER_COUNT ||
> + icmsg_major > IC_VERSION_NEGOTIATION_MAX_VER_COUNT ||
> + ICMSG_NEGOTIATE_PKT_SIZE(icframe_major, icmsg_major) > buflen) {
> + syslog(LOG_ERR, "Invalid icmsg negotiate - icframe_major: %u, icmsg_major: %u\n",
> + icframe_major, icmsg_major);
> + goto fw_error;
> + }
> +
> + /*
> + * Select the framework version number we will
> + * support.
> + */
> +
> + for (i = 0; i < fw_vercnt; i++) {
> + fw_major = (fw_version[i] >> 16);
> + fw_minor = (fw_version[i] & 0xFFFF);
> +
> + for (j = 0; j < negop->icframe_vercnt; j++) {
> + if (negop->icversion_data[j].major == fw_major &&
> + negop->icversion_data[j].minor == fw_minor) {
> + icframe_major = negop->icversion_data[j].major;
> + icframe_minor = negop->icversion_data[j].minor;
> + found_match = true;
> + break;
> + }
> + }
> +
> + if (found_match)
> + break;
> + }
> +
> + if (!found_match)
> + goto fw_error;
> +
> + found_match = false;
> +
> + for (i = 0; i < srv_vercnt; i++) {
> + srv_major = (srv_version[i] >> 16);
> + srv_minor = (srv_version[i] & 0xFFFF);
> +
> + for (j = negop->icframe_vercnt;
> + (j < negop->icframe_vercnt + negop->icmsg_vercnt);
> + j++) {
> + if (negop->icversion_data[j].major == srv_major &&
> + negop->icversion_data[j].minor == srv_minor) {
> + icmsg_major = negop->icversion_data[j].major;
> + icmsg_minor = negop->icversion_data[j].minor;
> + found_match = true;
> + break;
> + }
> + }
> +
> + if (found_match)
> + break;
> + }
> +
> + /*
> + * Respond with the framework and service
> + * version numbers we can support.
> + */
> +fw_error:
> + if (!found_match) {
> + negop->icframe_vercnt = 0;
> + negop->icmsg_vercnt = 0;
> + } else {
> + negop->icframe_vercnt = 1;
> + negop->icmsg_vercnt = 1;
> + }
> +
> + if (nego_fw_version)
> + *nego_fw_version = (icframe_major << 16) | icframe_minor;
> +
> + if (nego_srv_version)
> + *nego_srv_version = (icmsg_major << 16) | icmsg_minor;
> +
> + negop->icversion_data[0].major = icframe_major;
> + negop->icversion_data[0].minor = icframe_minor;
> + negop->icversion_data[1].major = icmsg_major;
> + negop->icversion_data[1].minor = icmsg_minor;
> +
> + return found_match;
> +}
> +
> +static void wcstoutf8(char *dest, const __u16 *src, size_t dest_size)
> +{
> + size_t len = 0;
> +
> + while (len < dest_size) {
> + if (src[len] < 0x80)
> + dest[len++] = (char)(*src++);
> + else
> + dest[len++] = 'X';
> + }
> +
> + dest[len] = '\0';
> +}
> +
> +static int hv_fcopy_start(struct hv_start_fcopy *smsg_in)
> +{
> + setlocale(LC_ALL, "en_US.utf8");
> + size_t file_size, path_size;
> + char *file_name, *path_name;
> + char *in_file_name = (char *)smsg_in->file_name;
> + char *in_path_name = (char *)smsg_in->path_name;
> +
> + file_size = wcstombs(NULL, (const wchar_t *restrict)in_file_name, 0) + 1;
> + path_size = wcstombs(NULL, (const wchar_t *restrict)in_path_name, 0) + 1;
> +
> + file_name = (char *)malloc(file_size * sizeof(char));
> + path_name = (char *)malloc(path_size * sizeof(char));
> +
> + wcstoutf8(file_name, (__u16 *)in_file_name, file_size);
> + wcstoutf8(path_name, (__u16 *)in_path_name, path_size);
> +
> + return hv_fcopy_create_file(file_name, path_name, smsg_in->copy_flags);
> +}
> +
> +static int hv_fcopy_send_data(struct hv_fcopy_hdr *fcopy_msg, int recvlen)
> +{
> + int operation = fcopy_msg->operation;
> +
> + /*
> + * The strings sent from the host are encoded in
> + * utf16; convert it to utf8 strings.
> + * The host assures us that the utf16 strings will not exceed
> + * the max lengths specified. We will however, reserve room
> + * for the string terminating character - in the utf16s_utf8s()
> + * function we limit the size of the buffer where the converted
> + * string is placed to W_MAX_PATH -1 to guarantee
> + * that the strings can be properly terminated!
> + */
> +
> + switch (operation) {
> + case START_FILE_COPY:
> + return hv_fcopy_start((struct hv_start_fcopy *)fcopy_msg);
> + case WRITE_TO_FILE:
> + return hv_copy_data((struct hv_do_fcopy *)fcopy_msg);
> + case COMPLETE_FCOPY:
> + return hv_copy_finished();
> + }
> +
> + return HV_E_FAIL;
> +}
> +
> +/* process the packet recv from host */
> +static int fcopy_pkt_process(struct vmbus_br *txbr)
> +{
> + int ret, offset, pktlen;
> + int fcopy_srv_version;
> + const struct vmbus_chanpkt_hdr *pkt;
> + struct hv_fcopy_hdr *fcopy_msg;
> + struct icmsg_hdr *icmsghdr;
> +
> + pkt = (const struct vmbus_chanpkt_hdr *)desc;
> + offset = pkt->hlen << 3;
> + pktlen = (pkt->tlen << 3) - offset;
> + icmsghdr = (struct icmsg_hdr *)&desc[offset + sizeof(struct vmbuspipe_hdr)];
> + icmsghdr->status = HV_E_FAIL;
> +
> + if (icmsghdr->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> + if (vmbus_prep_negotiate_resp(icmsghdr, desc + offset, pktlen, fw_versions,
> + FW_VER_COUNT, fcopy_versions, FCOPY_VER_COUNT,
> + NULL, &fcopy_srv_version)) {
> + syslog(LOG_INFO, "FCopy IC version %d.%d",
> + fcopy_srv_version >> 16, fcopy_srv_version & 0xFFFF);
> + icmsghdr->status = 0;
> + }
> + } else if (icmsghdr->icmsgtype == ICMSGTYPE_FCOPY) {
> + /* Ensure recvlen is big enough to contain hv_fcopy_hdr */
> + if (pktlen < ICMSG_HDR + sizeof(struct hv_fcopy_hdr)) {
> + syslog(LOG_ERR, "Invalid Fcopy hdr. Packet length too small: %u",
> + pktlen);
> + return -ENOBUFS;
> + }
> +
> + fcopy_msg = (struct hv_fcopy_hdr *)&desc[offset + ICMSG_HDR];
> + icmsghdr->status = hv_fcopy_send_data(fcopy_msg, pktlen);
> + }
> +
> + icmsghdr->icflags = ICMSGHDRFLAG_TRANSACTION | ICMSGHDRFLAG_RESPONSE;
> + ret = rte_vmbus_chan_send(txbr, 0x6, desc + offset, pktlen, 0);
> + if (ret) {
> + syslog(LOG_ERR, "Write to ringbuffer failed err: %d", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void fcopy_get_first_folder(char *path, char *chan_no)
> +{
> + DIR *dir = opendir(path);
> + struct dirent *entry;
> +
> + if (!dir) {
> + syslog(LOG_ERR, "Failed to open directory (errno=%s).\n", strerror(errno));
> + return;
> + }
> +
> + while ((entry = readdir(dir)) != NULL) {
> + if (entry->d_type == DT_DIR && strcmp(entry->d_name, ".") != 0 &&
> + strcmp(entry->d_name, "..") != 0) {
> + strcpy(chan_no, entry->d_name);
> + break;
> + }
> + }
> +
> + closedir(dir);
> +}
> +
> +static void fcopy_set_ring_size(char *path, char *inst, int size)
> +{
> + char ring_size_path[MAX_PATH_LEN] = {0};
> + FILE *fd;
> +
> + snprintf(ring_size_path, sizeof(ring_size_path), "%s/%s/%s", path, inst, "ring_size");
> + fd = fopen(ring_size_path, "w");
> + if (!fd) {
> + syslog(LOG_WARNING, "Failed to open ring_size file (errno=%s).\n", strerror(errno));
> + return;
> + }
> + fprintf(fd, "%d", size);

Check for and log an error if the new value isn't accepted by the kernel driver?
The code is using a ring size value that should be accepted by the kernel driver,
but weird stuff happens and it's probably better to know about it.

> + fclose(fd);
> +}
> +
> +static char *fcopy_read_sysfs(char *path, char *buf, int len)
> +{
> + FILE *fd;
> + char *ret;
> +
> + fd = fopen(path, "r");
> + if (!fd)
> + return NULL;
> +
> + ret = fgets(buf, len, fd);
> + fclose(fd);
> +
> + return ret;
> +}
> +
> +static int fcopy_get_instance_id(char *path, char *class_id, char *inst)
> +{
> + DIR *dir = opendir(path);
> + struct dirent *entry;
> + char tmp_path[MAX_PATH_LEN] = {0};
> + char line[MAX_LINE_LEN];
> +
> + if (!dir) {
> + syslog(LOG_ERR, "Failed to open directory (errno=%s).\n", strerror(errno));
> + return -EINVAL;
> + }
> +
> + while ((entry = readdir(dir)) != NULL) {
> + if (entry->d_type == DT_LNK && strcmp(entry->d_name, ".") != 0 &&
> + strcmp(entry->d_name, "..") != 0) {
> + /* search for the sysfs path with matching class_id */
> + snprintf(tmp_path, sizeof(tmp_path), "%s/%s/%s",
> + path, entry->d_name, "class_id");
> + if (!fcopy_read_sysfs(tmp_path, line, MAX_LINE_LEN))
> + continue;
> +
> + /* class id matches, now fetch the instance id from device_id */
> + if (strstr(line, class_id)) {
> + snprintf(tmp_path, sizeof(tmp_path), "%s/%s/%s",
> + path, entry->d_name, "device_id");
> + if (!fcopy_read_sysfs(tmp_path, line, MAX_LINE_LEN))
> + continue;
> + /* remove braces */
> + strncpy(inst, line + 1, strlen(line) - 3);
> + break;
> + }
> + }
> + }
> +
> + closedir(dir);
> + return 0;

If this function doesn't find a matching class_id, it appears that it
returns 0, but with the "inst" parameter unset. The caller will then
proceed as if "inst" was set when it is actually an uninitialized stack
variable. Probably need some better error detection and handling.

> +}
> +
> +int main(int argc, char *argv[])
> +{
> + int fcopy_fd = -1, tmp = 1;
> + int daemonize = 1, long_index = 0, opt, ret = -EINVAL;
> + struct vmbus_br txbr, rxbr;
> + void *ring;
> + uint32_t len = HV_RING_SIZE;
> + char uio_name[10] = {0};
> + char uio_dev_path[15] = {0};
> + char uio_path[MAX_PATH_LEN] = {0};
> + char inst[MAX_LINE_LEN] = {0};
> +
> + static struct option long_options[] = {
> + {"help", no_argument, 0, 'h' },
> + {"no-daemon", no_argument, 0, 'n' },
> + {0, 0, 0, 0 }
> + };
> +
> + while ((opt = getopt_long(argc, argv, "hn", long_options,
> + &long_index)) != -1) {
> + switch (opt) {
> + case 'n':
> + daemonize = 0;
> + break;
> + case 'h':
> + default:
> + print_usage(argv);
> + exit(EXIT_FAILURE);
> + }
> + }
> +
> + if (daemonize && daemon(1, 0)) {
> + syslog(LOG_ERR, "daemon() failed; error: %s", strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> +
> + openlog("HV_UIO_FCOPY", 0, LOG_USER);
> + syslog(LOG_INFO, "starting; pid is:%d", getpid());
> +
> + /* get instance id */
> + if (fcopy_get_instance_id(DEVICES_SYSFS, FCOPY_CLASS_ID, inst))
> + exit(EXIT_FAILURE);

Per above, need better error handling. And since the syslog is now open,
any errors should be logged rather than having the process just
mysteriously exit.

> +
> + /* set ring_size value */
> + fcopy_set_ring_size(DEVICES_SYSFS, inst, HV_RING_SIZE);
> +
> + /* get /dev/uioX dev path and open it */
> + snprintf(uio_path, sizeof(uio_path), "%s/%s/%s", DEVICES_SYSFS, inst, "uio");
> + fcopy_get_first_folder(uio_path, uio_name);
> + snprintf(uio_dev_path, sizeof(uio_dev_path), "/dev/%s", uio_name);
> + fcopy_fd = open(uio_dev_path, O_RDWR);
> +
> + if (fcopy_fd < 0) {
> + syslog(LOG_ERR, "open %s failed; error: %d %s",
> + uio_dev_path, errno, strerror(errno));
> + syslog(LOG_ERR, "Please make sure module uio_hv_vmbus_client is loaded and" \
> + " device is not used by any other application\n");
> + ret = fcopy_fd;
> + exit(EXIT_FAILURE);
> + }
> +
> + ring = mmap(NULL, 2 * HV_RING_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fcopy_fd, 0);
> + if (ring == MAP_FAILED) {
> + ret = errno;
> + syslog(LOG_ERR, "mmap ringbuffer failed; error: %d %s", ret, strerror(ret));
> + goto close;
> + }
> + vmbus_br_setup(&txbr, ring, HV_RING_SIZE);
> + vmbus_br_setup(&rxbr, (char *)ring + HV_RING_SIZE, HV_RING_SIZE);
> +
> + while (1) {
> + /*
> + * In this loop we process fcopy messages after the
> + * handshake is complete.
> + */
> + ret = pread(fcopy_fd, &tmp, sizeof(int), 0);
> + if (ret < 0) {
> + syslog(LOG_ERR, "pread failed: %s", strerror(errno));
> + continue;
> + }
> +
> + len = HV_RING_SIZE;
> + ret = rte_vmbus_chan_recv_raw(&rxbr, desc, &len);
> + if (unlikely(ret <= 0)) {
> + /* This indicates a failure to communicate (or worse) */
> + syslog(LOG_ERR, "VMBus channel recv error: %d", ret);
> + } else {
> + ret = fcopy_pkt_process(&txbr);
> + if (ret < 0)
> + goto close;
> +
> + /* Signal host */
> + tmp = 1;
> + if ((write(fcopy_fd, &tmp, sizeof(int))) != sizeof(int)) {
> + ret = errno;
> + syslog(LOG_ERR, "Registration failed: %s\n", strerror(ret));
> + goto close;
> + }
> + }
> + }
> +close:
> + close(fcopy_fd);
> + return ret;
> +}
> --
> 2.34.1


2023-08-03 14:08:23

by Saurabh Singh Sengar

[permalink] [raw]
Subject: RE: [PATCH v3 3/3] tools: hv: Add new fcopy application based on uio driver



> -----Original Message-----
> From: Michael Kelley (LINUX) <[email protected]>
> Sent: Thursday, August 3, 2023 3:15 AM
> To: Saurabh Sengar <[email protected]>; KY Srinivasan
> <[email protected]>; Haiyang Zhang <[email protected]>;
> [email protected]; Dexuan Cui <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: [EXTERNAL] RE: [PATCH v3 3/3] tools: hv: Add new fcopy application
> based on uio driver
>
> From: Saurabh Sengar <[email protected]> Sent: Friday, July 14,
> 2023 3:26 AM
> >
> > Implement the file copy service for Linux guests on Hyper-V. This
> > permits the host to copy a file (over VMBus) into the guest. This
> > facility is part of "guest integration services" supported on the
> > Hyper-V platform.
> >
> > Here is a link that provides additional details on this functionality:
> >
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flear
> > n.microsoft.com%2Fen-us%2Fpowershell%2Fmodule%2Fhyper-v%2Fcopy-
> vmfile%
> > 3Fview%3Dwindowsserver2022-
> ps&data=05%7C01%7Cssengar%40microsoft.com%7
> >
> Ca5edc1b9d6574e2e6e3108db93a1c558%7C72f988bf86f141af91ab2d7cd011
> db47%7
> >
> C1%7C0%7C638266095311741847%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAwMD
> >
> AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C
> &sdata
> > =VudSwIKYJJJgPKxNbbfCnOjia1lfKCdijnSn94OWm8Q%3D&reserved=0
> >
> > This new fcopy application uses uio_hv_vmbus_client driver which makes
> > the earlier hv_util based driver and application obsolete.
> >
> > Signed-off-by: Saurabh Sengar <[email protected]>
> > ---
> > [V3]
> > - Improve cover letter and commit messages
> > - Improve debug prints
> > - Instead of hardcoded instance id, query from class id sysfs
> > - Set the ring_size value from application
> > - Update the application to mmap /dev/uio instead of sysfs
> > - new application compilation dependent on x86
> >
> > [V2]
> > - simpler sysfs path
> >
> > tools/hv/Build | 1 +
> > tools/hv/Makefile | 10 +-
> > tools/hv/hv_fcopy_uio_daemon.c | 578
> > +++++++++++++++++++++++++++++++++
> > 3 files changed, 588 insertions(+), 1 deletion(-) create mode 100644
> > tools/hv/hv_fcopy_uio_daemon.c
> >
> > diff --git a/tools/hv/Build b/tools/hv/Build index
> > 2a667d3d94cb..efcbb74a0d23 100644
> > --- a/tools/hv/Build
> > +++ b/tools/hv/Build
> > @@ -2,3 +2,4 @@ hv_kvp_daemon-y += hv_kvp_daemon.o
> hv_vss_daemon-y +=
> > hv_vss_daemon.o hv_fcopy_daemon-y += hv_fcopy_daemon.o
> > vmbus_bufring-y += vmbus_bufring.o
> > +hv_fcopy_uio_daemon-y += hv_fcopy_uio_daemon.o
> > diff --git a/tools/hv/Makefile b/tools/hv/Makefile index
> > 33cf488fd20f..678c6c450a53 100644
> > --- a/tools/hv/Makefile
> > +++ b/tools/hv/Makefile
> > @@ -21,8 +21,10 @@ override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -
> > I$(OUTPUT)include
> >
> > ifeq ($(SRCARCH),x86)
> > ALL_LIBS := libvmbus_bufring.a
> > -endif
> > +ALL_TARGETS := hv_kvp_daemon hv_vss_daemon hv_fcopy_daemon
> > hv_fcopy_uio_daemon
> > +else
> > ALL_TARGETS := hv_kvp_daemon hv_vss_daemon hv_fcopy_daemon
> > +endif
> > ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS)) $(patsubst
> > %,$(OUTPUT)%,$(ALL_LIBS))
> >
> > ALL_SCRIPTS := hv_get_dhcp_info.sh hv_get_dns_info.sh
> > hv_set_ifconfig.sh @@ -56,6 +58,12 @@ $(HV_FCOPY_DAEMON_IN):
> FORCE
> > $(OUTPUT)hv_fcopy_daemon: $(HV_FCOPY_DAEMON_IN)
> > $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
> >
> > +HV_FCOPY_UIO_DAEMON_IN := $(OUTPUT)hv_fcopy_uio_daemon-in.o
> > +$(HV_FCOPY_UIO_DAEMON_IN): FORCE
> > + $(Q)$(MAKE) $(build)=hv_fcopy_uio_daemon
> > +$(OUTPUT)hv_fcopy_uio_daemon: $(HV_FCOPY_UIO_DAEMON_IN)
> > libvmbus_bufring.a
> > + $(QUIET_LINK)$(CC) -lm $< -L. -lvmbus_bufring -o $@
> > +
> > clean:
> > rm -f $(ALL_PROGRAMS)
> > find $(or $(OUTPUT),.) -name '*.o' -delete -o -name '\.*.d' -delete
> > diff --git a/tools/hv/hv_fcopy_uio_daemon.c
> > b/tools/hv/hv_fcopy_uio_daemon.c new file mode 100644 index
> > 000000000000..e8618a30dc7e
> > --- /dev/null
> > +++ b/tools/hv/hv_fcopy_uio_daemon.c
> > @@ -0,0 +1,578 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * An implementation of host to guest copy functionality for Linux.
> > + *
> > + * Copyright (C) 2023, Microsoft, Inc.
> > + *
> > + * Author : K. Y. Srinivasan <[email protected]>
> > + * Author : Saurabh Sengar <[email protected]>
> > + *
> > + */
> > +
> > +#include <dirent.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <getopt.h>
> > +#include <locale.h>
> > +#include <stdbool.h>
> > +#include <stddef.h>
> > +#include <stdint.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <syslog.h>
> > +#include <unistd.h>
> > +#include <sys/mman.h>
> > +#include <sys/stat.h>
> > +#include <linux/hyperv.h>
> > +#include "vmbus_bufring.h"
> > +
> > +#define ICMSGTYPE_NEGOTIATE 0
> > +#define ICMSGTYPE_FCOPY 7
> > +
> > +#define WIN8_SRV_MAJOR 1
> > +#define WIN8_SRV_MINOR 1
> > +#define WIN8_SRV_VERSION (WIN8_SRV_MAJOR << 16 |
> WIN8_SRV_MINOR)
> > +
> > +#define MAX_PATH_LEN 300
> > +#define MAX_LINE_LEN 40
> > +#define DEVICES_SYSFS "/sys/bus/vmbus/devices"
> > +#define FCOPY_CLASS_ID "34d14be3-dee4-41c8-9ae7-
> 6b174977c192"
> > +
> > +#define FCOPY_VER_COUNT 1
> > +static const int fcopy_versions[] = {
> > + WIN8_SRV_VERSION
> > +};
> > +
> > +#define FW_VER_COUNT 1
> > +static const int fw_versions[] = {
> > + UTIL_FW_VERSION
> > +};
> > +
> > +#define HV_RING_SIZE (4 * 4096)
> > +
> > +unsigned char desc[HV_RING_SIZE];
> > +
> > +static int target_fd;
> > +static char target_fname[PATH_MAX];
> > +static unsigned long long filesize;
> > +
> > +static int hv_fcopy_create_file(char *file_name, char *path_name,
> > +__u32 flags) {
> > + int error = HV_E_FAIL;
> > + char *q, *p;
> > +
> > + filesize = 0;
> > + p = (char *)path_name;
> > + snprintf(target_fname, sizeof(target_fname), "%s/%s",
> > + (char *)path_name, (char *)file_name);
> > +
> > + /*
> > + * Check to see if the path is already in place; if not,
> > + * create if required.
> > + */
> > + while ((q = strchr(p, '/')) != NULL) {
> > + if (q == p) {
> > + p++;
> > + continue;
> > + }
> > + *q = '\0';
> > + if (access(path_name, F_OK)) {
> > + if (flags & CREATE_PATH) {
> > + if (mkdir(path_name, 0755)) {
> > + syslog(LOG_ERR, "Failed to create
> %s",
> > + path_name);
> > + goto done;
> > + }
> > + } else {
> > + syslog(LOG_ERR, "Invalid path: %s",
> path_name);
> > + goto done;
> > + }
> > + }
> > + p = q + 1;
> > + *q = '/';
> > + }
> > +
> > + if (!access(target_fname, F_OK)) {
> > + syslog(LOG_INFO, "File: %s exists", target_fname);
> > + if (!(flags & OVER_WRITE)) {
> > + error = HV_ERROR_ALREADY_EXISTS;
> > + goto done;
> > + }
> > + }
> > +
> > + target_fd = open(target_fname,
> > + O_RDWR | O_CREAT | O_TRUNC | O_CLOEXEC,
> 0744);
> > + if (target_fd == -1) {
> > + syslog(LOG_INFO, "Open Failed: %s", strerror(errno));
> > + goto done;
> > + }
> > +
> > + error = 0;
> > +done:
> > + if (error)
> > + target_fname[0] = '\0';
> > + return error;
> > +}
> > +
> > +static int hv_copy_data(struct hv_do_fcopy *cpmsg) {
> > + ssize_t bytes_written;
> > + int ret = 0;
> > +
> > + bytes_written = pwrite(target_fd, cpmsg->data, cpmsg->size,
> > + cpmsg->offset);
> > +
> > + filesize += cpmsg->size;
> > + if (bytes_written != cpmsg->size) {
> > + switch (errno) {
> > + case ENOSPC:
> > + ret = HV_ERROR_DISK_FULL;
> > + break;
> > + default:
> > + ret = HV_E_FAIL;
> > + break;
> > + }
> > + syslog(LOG_ERR, "pwrite failed to write %llu bytes: %ld (%s)",
> > + filesize, (long)bytes_written, strerror(errno));
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +/*
> > + * Reset target_fname to "" in the two below functions for
> > +hibernation: if
> > + * the fcopy operation is aborted by hibernation, the daemon should
> > +remove the
> > + * partially-copied file; to achieve this, the hv_utils driver always
> > +fakes a
> > + * CANCEL_FCOPY message upon suspend, and later when the VM
> resumes
> > +back,
> > + * the daemon calls hv_copy_cancel() to remove the file; if a file is
> > +copied
> > + * successfully before suspend, hv_copy_finished() must reset
> > +target_fname to
> > + * avoid that the file can be incorrectly removed upon resume, since
> > +the faked
> > + * CANCEL_FCOPY message is spurious in this case.
> > + */
> > +static int hv_copy_finished(void)
> > +{
> > + close(target_fd);
> > + target_fname[0] = '\0';
> > + return 0;
> > +}
> > +
> > +static void print_usage(char *argv[]) {
> > + fprintf(stderr, "Usage: %s [options]\n"
> > + "Options are:\n"
> > + " -n, --no-daemon stay in foreground, don't
> daemonize\n"
> > + " -h, --help print this help\n", argv[0]);
> > +}
> > +
> > +static bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
> unsigned char *buf,
> > + unsigned int buflen, const int *fw_version,
> int fw_vercnt,
> > + const int *srv_version, int srv_vercnt,
> > + int *nego_fw_version, int *nego_srv_version)
> {
> > + int icframe_major, icframe_minor;
> > + int icmsg_major, icmsg_minor;
> > + int fw_major, fw_minor;
> > + int srv_major, srv_minor;
> > + int i, j;
> > + bool found_match = false;
> > + struct icmsg_negotiate *negop;
> > +
> > + /* Check that there's enough space for icframe_vercnt, icmsg_vercnt
> */
> > + if (buflen < ICMSG_HDR + offsetof(struct icmsg_negotiate, reserved)) {
> > + syslog(LOG_ERR, "Invalid icmsg negotiate");
> > + return false;
> > + }
> > +
> > + icmsghdrp->icmsgsize = 0x10;
> > + negop = (struct icmsg_negotiate *)&buf[ICMSG_HDR];
> > +
> > + icframe_major = negop->icframe_vercnt;
> > + icframe_minor = 0;
> > +
> > + icmsg_major = negop->icmsg_vercnt;
> > + icmsg_minor = 0;
> > +
> > + /* Validate negop packet */
> > + if (icframe_major > IC_VERSION_NEGOTIATION_MAX_VER_COUNT ||
> > + icmsg_major > IC_VERSION_NEGOTIATION_MAX_VER_COUNT ||
> > + ICMSG_NEGOTIATE_PKT_SIZE(icframe_major, icmsg_major) >
> buflen) {
> > + syslog(LOG_ERR, "Invalid icmsg negotiate - icframe_major:
> %u, icmsg_major: %u\n",
> > + icframe_major, icmsg_major);
> > + goto fw_error;
> > + }
> > +
> > + /*
> > + * Select the framework version number we will
> > + * support.
> > + */
> > +
> > + for (i = 0; i < fw_vercnt; i++) {
> > + fw_major = (fw_version[i] >> 16);
> > + fw_minor = (fw_version[i] & 0xFFFF);
> > +
> > + for (j = 0; j < negop->icframe_vercnt; j++) {
> > + if (negop->icversion_data[j].major == fw_major &&
> > + negop->icversion_data[j].minor == fw_minor) {
> > + icframe_major = negop-
> >icversion_data[j].major;
> > + icframe_minor = negop-
> >icversion_data[j].minor;
> > + found_match = true;
> > + break;
> > + }
> > + }
> > +
> > + if (found_match)
> > + break;
> > + }
> > +
> > + if (!found_match)
> > + goto fw_error;
> > +
> > + found_match = false;
> > +
> > + for (i = 0; i < srv_vercnt; i++) {
> > + srv_major = (srv_version[i] >> 16);
> > + srv_minor = (srv_version[i] & 0xFFFF);
> > +
> > + for (j = negop->icframe_vercnt;
> > + (j < negop->icframe_vercnt + negop->icmsg_vercnt);
> > + j++) {
> > + if (negop->icversion_data[j].major == srv_major &&
> > + negop->icversion_data[j].minor == srv_minor) {
> > + icmsg_major = negop-
> >icversion_data[j].major;
> > + icmsg_minor = negop-
> >icversion_data[j].minor;
> > + found_match = true;
> > + break;
> > + }
> > + }
> > +
> > + if (found_match)
> > + break;
> > + }
> > +
> > + /*
> > + * Respond with the framework and service
> > + * version numbers we can support.
> > + */
> > +fw_error:
> > + if (!found_match) {
> > + negop->icframe_vercnt = 0;
> > + negop->icmsg_vercnt = 0;
> > + } else {
> > + negop->icframe_vercnt = 1;
> > + negop->icmsg_vercnt = 1;
> > + }
> > +
> > + if (nego_fw_version)
> > + *nego_fw_version = (icframe_major << 16) | icframe_minor;
> > +
> > + if (nego_srv_version)
> > + *nego_srv_version = (icmsg_major << 16) | icmsg_minor;
> > +
> > + negop->icversion_data[0].major = icframe_major;
> > + negop->icversion_data[0].minor = icframe_minor;
> > + negop->icversion_data[1].major = icmsg_major;
> > + negop->icversion_data[1].minor = icmsg_minor;
> > +
> > + return found_match;
> > +}
> > +
> > +static void wcstoutf8(char *dest, const __u16 *src, size_t dest_size)
> > +{
> > + size_t len = 0;
> > +
> > + while (len < dest_size) {
> > + if (src[len] < 0x80)
> > + dest[len++] = (char)(*src++);
> > + else
> > + dest[len++] = 'X';
> > + }
> > +
> > + dest[len] = '\0';
> > +}
> > +
> > +static int hv_fcopy_start(struct hv_start_fcopy *smsg_in) {
> > + setlocale(LC_ALL, "en_US.utf8");
> > + size_t file_size, path_size;
> > + char *file_name, *path_name;
> > + char *in_file_name = (char *)smsg_in->file_name;
> > + char *in_path_name = (char *)smsg_in->path_name;
> > +
> > + file_size = wcstombs(NULL, (const wchar_t *restrict)in_file_name, 0) +
> 1;
> > + path_size = wcstombs(NULL, (const wchar_t *restrict)in_path_name,
> 0)
> > ++ 1;
> > +
> > + file_name = (char *)malloc(file_size * sizeof(char));
> > + path_name = (char *)malloc(path_size * sizeof(char));
> > +
> > + wcstoutf8(file_name, (__u16 *)in_file_name, file_size);
> > + wcstoutf8(path_name, (__u16 *)in_path_name, path_size);
> > +
> > + return hv_fcopy_create_file(file_name, path_name,
> > +smsg_in->copy_flags); }
> > +
> > +static int hv_fcopy_send_data(struct hv_fcopy_hdr *fcopy_msg, int
> > +recvlen) {
> > + int operation = fcopy_msg->operation;
> > +
> > + /*
> > + * The strings sent from the host are encoded in
> > + * utf16; convert it to utf8 strings.
> > + * The host assures us that the utf16 strings will not exceed
> > + * the max lengths specified. We will however, reserve room
> > + * for the string terminating character - in the utf16s_utf8s()
> > + * function we limit the size of the buffer where the converted
> > + * string is placed to W_MAX_PATH -1 to guarantee
> > + * that the strings can be properly terminated!
> > + */
> > +
> > + switch (operation) {
> > + case START_FILE_COPY:
> > + return hv_fcopy_start((struct hv_start_fcopy *)fcopy_msg);
> > + case WRITE_TO_FILE:
> > + return hv_copy_data((struct hv_do_fcopy *)fcopy_msg);
> > + case COMPLETE_FCOPY:
> > + return hv_copy_finished();
> > + }
> > +
> > + return HV_E_FAIL;
> > +}
> > +
> > +/* process the packet recv from host */ static int
> > +fcopy_pkt_process(struct vmbus_br *txbr) {
> > + int ret, offset, pktlen;
> > + int fcopy_srv_version;
> > + const struct vmbus_chanpkt_hdr *pkt;
> > + struct hv_fcopy_hdr *fcopy_msg;
> > + struct icmsg_hdr *icmsghdr;
> > +
> > + pkt = (const struct vmbus_chanpkt_hdr *)desc;
> > + offset = pkt->hlen << 3;
> > + pktlen = (pkt->tlen << 3) - offset;
> > + icmsghdr = (struct icmsg_hdr *)&desc[offset + sizeof(struct
> vmbuspipe_hdr)];
> > + icmsghdr->status = HV_E_FAIL;
> > +
> > + if (icmsghdr->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> > + if (vmbus_prep_negotiate_resp(icmsghdr, desc + offset,
> pktlen, fw_versions,
> > + FW_VER_COUNT, fcopy_versions,
> FCOPY_VER_COUNT,
> > + NULL, &fcopy_srv_version)) {
> > + syslog(LOG_INFO, "FCopy IC version %d.%d",
> > + fcopy_srv_version >> 16, fcopy_srv_version &
> 0xFFFF);
> > + icmsghdr->status = 0;
> > + }
> > + } else if (icmsghdr->icmsgtype == ICMSGTYPE_FCOPY) {
> > + /* Ensure recvlen is big enough to contain hv_fcopy_hdr */
> > + if (pktlen < ICMSG_HDR + sizeof(struct hv_fcopy_hdr)) {
> > + syslog(LOG_ERR, "Invalid Fcopy hdr. Packet length too
> small: %u",
> > + pktlen);
> > + return -ENOBUFS;
> > + }
> > +
> > + fcopy_msg = (struct hv_fcopy_hdr *)&desc[offset +
> ICMSG_HDR];
> > + icmsghdr->status = hv_fcopy_send_data(fcopy_msg, pktlen);
> > + }
> > +
> > + icmsghdr->icflags = ICMSGHDRFLAG_TRANSACTION |
> ICMSGHDRFLAG_RESPONSE;
> > + ret = rte_vmbus_chan_send(txbr, 0x6, desc + offset, pktlen, 0);
> > + if (ret) {
> > + syslog(LOG_ERR, "Write to ringbuffer failed err: %d", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void fcopy_get_first_folder(char *path, char *chan_no) {
> > + DIR *dir = opendir(path);
> > + struct dirent *entry;
> > +
> > + if (!dir) {
> > + syslog(LOG_ERR, "Failed to open directory (errno=%s).\n",
> strerror(errno));
> > + return;
> > + }
> > +
> > + while ((entry = readdir(dir)) != NULL) {
> > + if (entry->d_type == DT_DIR && strcmp(entry->d_name, ".")
> != 0 &&
> > + strcmp(entry->d_name, "..") != 0) {
> > + strcpy(chan_no, entry->d_name);
> > + break;
> > + }
> > + }
> > +
> > + closedir(dir);
> > +}
> > +
> > +static void fcopy_set_ring_size(char *path, char *inst, int size) {
> > + char ring_size_path[MAX_PATH_LEN] = {0};
> > + FILE *fd;
> > +
> > + snprintf(ring_size_path, sizeof(ring_size_path), "%s/%s/%s", path, inst,
> "ring_size");
> > + fd = fopen(ring_size_path, "w");
> > + if (!fd) {
> > + syslog(LOG_WARNING, "Failed to open ring_size file
> (errno=%s).\n", strerror(errno));
> > + return;
> > + }
> > + fprintf(fd, "%d", size);
>
> Check for and log an error if the new value isn't accepted by the kernel
> driver?
> The code is using a ring size value that should be accepted by the kernel
> driver, but weird stuff happens and it's probably better to know about it.

Ok, will add error check.

>
> > + fclose(fd);
> > +}
> > +
> > +static char *fcopy_read_sysfs(char *path, char *buf, int len) {
> > + FILE *fd;
> > + char *ret;
> > +
> > + fd = fopen(path, "r");
> > + if (!fd)
> > + return NULL;
> > +
> > + ret = fgets(buf, len, fd);
> > + fclose(fd);
> > +
> > + return ret;
> > +}
> > +
> > +static int fcopy_get_instance_id(char *path, char *class_id, char
> > +*inst) {
> > + DIR *dir = opendir(path);
> > + struct dirent *entry;
> > + char tmp_path[MAX_PATH_LEN] = {0};
> > + char line[MAX_LINE_LEN];
> > +
> > + if (!dir) {
> > + syslog(LOG_ERR, "Failed to open directory (errno=%s).\n",
> strerror(errno));
> > + return -EINVAL;
> > + }
> > +
> > + while ((entry = readdir(dir)) != NULL) {
> > + if (entry->d_type == DT_LNK && strcmp(entry->d_name, ".")
> != 0 &&
> > + strcmp(entry->d_name, "..") != 0) {
> > + /* search for the sysfs path with matching class_id */
> > + snprintf(tmp_path, sizeof(tmp_path), "%s/%s/%s",
> > + path, entry->d_name, "class_id");
> > + if (!fcopy_read_sysfs(tmp_path, line, MAX_LINE_LEN))
> > + continue;
> > +
> > + /* class id matches, now fetch the instance id from
> device_id */
> > + if (strstr(line, class_id)) {
> > + snprintf(tmp_path, sizeof(tmp_path),
> "%s/%s/%s",
> > + path, entry->d_name, "device_id");
> > + if (!fcopy_read_sysfs(tmp_path, line,
> MAX_LINE_LEN))
> > + continue;
> > + /* remove braces */
> > + strncpy(inst, line + 1, strlen(line) - 3);
> > + break;
> > + }
> > + }
> > + }
> > +
> > + closedir(dir);
> > + return 0;
>
> If this function doesn't find a matching class_id, it appears that it returns 0,
> but with the "inst" parameter unset. The caller will then proceed as if "inst"
> was set when it is actually an uninitialized stack variable. Probably need
> some better error detection and handling.

Good point !
Let me fix it in next version.

>
> > +}
> > +
> > +int main(int argc, char *argv[])
> > +{
> > + int fcopy_fd = -1, tmp = 1;
> > + int daemonize = 1, long_index = 0, opt, ret = -EINVAL;
> > + struct vmbus_br txbr, rxbr;
> > + void *ring;
> > + uint32_t len = HV_RING_SIZE;
> > + char uio_name[10] = {0};
> > + char uio_dev_path[15] = {0};
> > + char uio_path[MAX_PATH_LEN] = {0};
> > + char inst[MAX_LINE_LEN] = {0};
> > +
> > + static struct option long_options[] = {
> > + {"help", no_argument, 0, 'h' },
> > + {"no-daemon", no_argument, 0, 'n' },
> > + {0, 0, 0, 0 }
> > + };
> > +
> > + while ((opt = getopt_long(argc, argv, "hn", long_options,
> > + &long_index)) != -1) {
> > + switch (opt) {
> > + case 'n':
> > + daemonize = 0;
> > + break;
> > + case 'h':
> > + default:
> > + print_usage(argv);
> > + exit(EXIT_FAILURE);
> > + }
> > + }
> > +
> > + if (daemonize && daemon(1, 0)) {
> > + syslog(LOG_ERR, "daemon() failed; error: %s",
> strerror(errno));
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > + openlog("HV_UIO_FCOPY", 0, LOG_USER);
> > + syslog(LOG_INFO, "starting; pid is:%d", getpid());
> > +
> > + /* get instance id */
> > + if (fcopy_get_instance_id(DEVICES_SYSFS, FCOPY_CLASS_ID, inst))
> > + exit(EXIT_FAILURE);
>
> Per above, need better error handling. And since the syslog is now open, any
> errors should be logged rather than having the process just mysteriously exit.

OK.

- Saurabh