On Wed, Feb 12, 2014 at 10:14:03AM -0800, K. Y. Srinivasan wrote: > 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 Windows platform. > Here is a link that provides additional details on this functionality: > > http://technet.microsoft.com/en-us/library/dn464282.aspx > > In V1 version of the patch I have addressed comments from > Olaf Hering <olaf@xxxxxxxxx> and Dan Carpenter <dan.carpenter@xxxxxxxxxx> > > In V2 version of this patch I did some minor cleanup (making some globals > static). In V4 version of the patch I have addressed all of Olaf's > most recent set of comments/concerns. > > In V5 version of the patch I had addressed Greg's most recent comments. > I would like to thank Greg for suggesting that I use misc device; it has > significantly simplified the code. > > In this version of the patch I have cleaned up error message based on Olaf's > comments. I have also rebased the patch based on the current tip. > > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx> > --- > drivers/hv/Makefile | 2 +- > drivers/hv/hv_fcopy.c | 388 +++++++++++++++++++++++++++++++++++++++++++ > drivers/hv/hv_util.c | 10 + > include/linux/hyperv.h | 16 ++- > include/uapi/linux/hyperv.h | 46 +++++ > tools/hv/hv_fcopy_daemon.c | 195 ++++++++++++++++++++++ > 6 files changed, 655 insertions(+), 2 deletions(-) > create mode 100644 drivers/hv/hv_fcopy.c > create mode 100644 tools/hv/hv_fcopy_daemon.c > > diff --git a/drivers/hv/Makefile b/drivers/hv/Makefile > index 0a74b56..5e4dfa4 100644 > --- a/drivers/hv/Makefile > +++ b/drivers/hv/Makefile > @@ -5,4 +5,4 @@ obj-$(CONFIG_HYPERV_BALLOON) += hv_balloon.o > hv_vmbus-y := vmbus_drv.o \ > hv.o connection.o channel.o \ > channel_mgmt.o ring_buffer.o > -hv_utils-y := hv_util.o hv_kvp.o hv_snapshot.o > +hv_utils-y := hv_util.o hv_kvp.o hv_snapshot.o hv_fcopy.o > diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c > new file mode 100644 > index 0000000..15cf35c > --- /dev/null > +++ b/drivers/hv/hv_fcopy.c > @@ -0,0 +1,388 @@ > +/* > + * An implementation of file copy service. > + * > + * Copyright (C) 2014, Microsoft, Inc. > + * > + * Author : K. Y. Srinivasan <ksrinivasan@xxxxxxxxxx> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License version 2 as published > + * by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or > + * NON INFRINGEMENT. See the GNU General Public License for more > + * details. > + * > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/semaphore.h> > +#include <linux/fs.h> > +#include <linux/nls.h> > +#include <linux/workqueue.h> > +#include <linux/cdev.h> > +#include <linux/hyperv.h> > +#include <linux/sched.h> > +#include <linux/uaccess.h> > +#include <linux/miscdevice.h> > + > +#define WIN8_SRV_MAJOR 1 > +#define WIN8_SRV_MINOR 1 > +#define WIN8_SRV_VERSION (WIN8_SRV_MAJOR << 16 | WIN8_SRV_MINOR) > + > +/* > + * Global state maintained for transaction that is being processed. > + * For a class of integration services, including the "file copy service", > + * the specified protocol is a "request/response" protocol which means that > + * there can only be single outstanding transaction from the host at any > + * given point in time. We use this to simplify memory management in this > + * driver - we cache and process only one message at a time. > + * > + * While the request/response protocol is guaranteed by the host, we further > + * ensure this by serializing packet processing in this driver - we do not > + * read additional packets from the VMBUs until the current packet is fully > + * handled. > + * > + * The transaction "active" state is set when we receive a request from the > + * host and we cleanup this state when the transaction is completed - when we > + * respond to the host with our response. When the transaction active state is > + * set, we defer handling incoming packets. > + */ > + > +static struct { > + bool active; /* transaction status - active or not */ > + int recv_len; /* number of bytes received. */ > + struct hv_fcopy_hdr *fcopy_msg; /* current message */ > + struct hv_start_fcopy message; /* sent to daemon */ > + struct vmbus_channel *recv_channel; /* chn we got the request */ > + u64 recv_req_id; /* request ID. */ > + void *fcopy_context; /* for the channel callback */ > + struct semaphore read_sema; > +} fcopy_transaction; > + > +static bool opened; /* currently device opened */ > + > +/* > + * Before we can accept copy messages from the host, we need > + * to handshake with the user level daemon. This state tracks > + * if we are in the handshake phase. > + */ > +static bool in_hand_shake = true; > +static void fcopy_send_data(void); > +static void fcopy_respond_to_host(int error); > +static void fcopy_work_func(struct work_struct *dummy); > +static DECLARE_DELAYED_WORK(fcopy_work, fcopy_work_func); > +static u8 *recv_buffer; > + > +static void fcopy_work_func(struct work_struct *dummy) > +{ > + /* > + * If the timer fires, the user-mode component has not responded; > + * process the pending transaction. > + */ > + fcopy_respond_to_host(HV_E_FAIL); > +} > + > +static int fcopy_handle_handshake(u32 version) > +{ > + switch (version) { > + case FCOPY_CURRENT_VERSION: > + break; > + default: > + /* > + * For now we will fail the registration. > + * If and when we have multiple versions to > + * deal with, we will be backward compatible. > + * We will add this code when needed. > + */ > + return -EINVAL; > + } > + pr_info("FCP: user-mode registering done. Daemon version: %d\n", > + version); Why polute the syslog with this information? > + fcopy_transaction.active = false; > + if (fcopy_transaction.fcopy_context) > + hv_fcopy_onchannelcallback(fcopy_transaction.fcopy_context); > + in_hand_shake = false; > + return sizeof(int); sizeof(int)? Since when is that a valid return value, why are you returning that? Ok, I see the caller of this function, and why you are doing this, but it's really not obvious at all, you should just return -ERROR or 0 and then deal with the return value at the caller, as it's not obvious what is going on at all here. > +static void fcopy_send_data(void) > +{ > + struct hv_start_fcopy *smsg_out = &fcopy_transaction.message; > + int operation = fcopy_transaction.fcopy_msg->operation; > + struct hv_start_fcopy *smsg_in; > + > + /* > + * The strings sent from the host are encoded in > + * 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: > + memset(smsg_out, 0, sizeof(struct hv_start_fcopy)); > + smsg_out->hdr.operation = operation; > + smsg_in = (struct hv_start_fcopy *)fcopy_transaction.fcopy_msg; > + > + utf16s_to_utf8s((wchar_t *)smsg_in->file_name, W_MAX_PATH, > + UTF16_LITTLE_ENDIAN, > + (__u8 *)smsg_out->file_name, W_MAX_PATH - 1); > + > + utf16s_to_utf8s((wchar_t *)smsg_in->path_name, W_MAX_PATH, > + UTF16_LITTLE_ENDIAN, > + (__u8 *)smsg_out->path_name, W_MAX_PATH - 1); > + > + smsg_out->copy_flags = smsg_in->copy_flags; > + smsg_out->file_size = smsg_in->file_size; > + break; > + > + default: > + break; > + } > + up(&fcopy_transaction.read_sema); > + return; > +} > + > +/* > + * Send a response back to the host. > + */ > + > +static void > +fcopy_respond_to_host(int error) > +{ > + struct icmsg_hdr *icmsghdrp; What's with the "p"? :) > + u32 buf_len; > + struct vmbus_channel *channel; > + u64 req_id; > + > + /* > + * Copy the global state for completing the transaction. Note that > + * only one transaction can be active at a time. > + */ Who enforces this "one at a time" restriction? > + > + buf_len = fcopy_transaction.recv_len; > + channel = fcopy_transaction.recv_channel; > + req_id = fcopy_transaction.recv_req_id; > + > + fcopy_transaction.active = false; > + > + icmsghdrp = (struct icmsg_hdr *) > + &recv_buffer[sizeof(struct vmbuspipe_hdr)]; > + > + if (channel->onchannel_callback == NULL) > + /* > + * We have raced with util driver being unloaded; > + * silently return. > + */ > + return; > + > + icmsghdrp->status = error; > + icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION | ICMSGHDRFLAG_RESPONSE; > + vmbus_sendpacket(channel, recv_buffer, buf_len, req_id, > + VM_PKT_DATA_INBAND, 0); > +} > + > +void hv_fcopy_onchannelcallback(void *context) > +{ > + struct vmbus_channel *channel = context; > + u32 recvlen; > + u64 requestid; > + struct hv_fcopy_hdr *fcopy_msg; > + struct icmsg_hdr *icmsghdrp; > + struct icmsg_negotiate *negop = NULL; > + int util_fw_version; > + int fcopy_srv_version; > + > + if (fcopy_transaction.active) { > + /* > + * We will defer processing this callback once > + * the current transaction is complete. > + */ > + fcopy_transaction.fcopy_context = context; > + return; > + } > + > + vmbus_recvpacket(channel, recv_buffer, PAGE_SIZE * 2, &recvlen, > + &requestid); > + if (recvlen <= 0) > + return; > + > + icmsghdrp = (struct icmsg_hdr *)&recv_buffer[ > + sizeof(struct vmbuspipe_hdr)]; > + if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) { > + util_fw_version = UTIL_FW_VERSION; > + fcopy_srv_version = WIN8_SRV_VERSION; > + vmbus_prep_negotiate_resp(icmsghdrp, negop, recv_buffer, > + util_fw_version, fcopy_srv_version); > + } else { > + fcopy_msg = (struct hv_fcopy_hdr *)&recv_buffer[ > + sizeof(struct vmbuspipe_hdr) + > + sizeof(struct icmsg_hdr)]; > + > + /* > + * Stash away this global state for completing the > + * transaction; note transactions are serialized. > + */ > + > + fcopy_transaction.active = true; > + fcopy_transaction.recv_len = recvlen; > + fcopy_transaction.recv_channel = channel; > + fcopy_transaction.recv_req_id = requestid; > + fcopy_transaction.fcopy_msg = fcopy_msg; > + > + /* > + * Send the information to the user-level daemon. > + */ > + fcopy_send_data(); > + schedule_delayed_work(&fcopy_work, 5*HZ); > + return; > + } > + icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION | ICMSGHDRFLAG_RESPONSE; > + vmbus_sendpacket(channel, recv_buffer, recvlen, requestid, > + VM_PKT_DATA_INBAND, 0); > +} > + > +/* > + * Create a char device that can support read/write for passing > + * the payload. > + */ > + > +static ssize_t fcopy_read(struct file *file, char __user *buf, > + size_t count, loff_t *ppos) > +{ > + void *src; > + size_t copy_size; > + int operation; > + > + /* > + * Wait until there is something to be read. > + */ > + if (down_interruptible(&fcopy_transaction.read_sema)) > + return -EINTR; > + if (!opened) > + return -ENODEV; How could this ever happen? > + > + operation = fcopy_transaction.fcopy_msg->operation; > + > + if (operation == START_FILE_COPY) { > + src = &fcopy_transaction.message; > + copy_size = sizeof(struct hv_start_fcopy); > + if (count < copy_size) > + return 0; > + } else { > + src = fcopy_transaction.fcopy_msg; > + copy_size = sizeof(struct hv_do_fcopy); > + if (count < copy_size) > + return 0; > + } > + if (copy_to_user(buf, src, copy_size)) > + return -EFAULT; > + > + return copy_size; > +} > + > +static ssize_t fcopy_write(struct file *file, const char __user *buf, > + size_t count, loff_t *ppos) > +{ > + int response = 0; > + > + if (count != sizeof(int)) > + return -EINVAL; > + > + if (copy_from_user(&response, buf, sizeof(int))) > + return -EFAULT; > + > + if (in_hand_shake) > + return fcopy_handle_handshake(response); > + > + /* > + * Complete the transaction by forwarding the result > + * to the host. But first, cancel the timeout. > + */ > + if (cancel_delayed_work_sync(&fcopy_work)) > + fcopy_respond_to_host(response); > + > + return sizeof(int); > +} > + > +int fcopy_open(struct inode *inode, struct file *f) > +{ > + if (opened) > + return -EBUSY; Why? What does it matter if this is ever opened more than once (and can it even?) > + > + /* > + * The daemon is alive; setup the state. > + */ > + opened = true; > + return 0; > +} > + > +int fcopy_release(struct inode *inode, struct file *f) > +{ > + /* > + * The daemon has exited; reset the state. > + */ > + in_hand_shake = true; > + opened = false; > + return 0; > +} > + > + > +static const struct file_operations fcopy_fops = { > + .read = fcopy_read, > + .write = fcopy_write, > + .release = fcopy_release, > + .open = fcopy_open, > +}; > + > +static struct miscdevice fcopy_misc = { > + .minor = MISC_DYNAMIC_MINOR, > + .name = "vmbus/hv_fcopy", Why the subdir? Does that work properly? > + .fops = &fcopy_fops, > +}; > + > +static int fcopy_dev_init(void) > +{ > + return misc_register(&fcopy_misc); > +} > + > +static void fcopy_dev_deinit(void) > +{ > + opened = false; Is this really true here? You are using "opened" to mean different things it seems :( > + /* > + * Signal the semaphore as the device is > + * going away. > + */ > + up(&fcopy_transaction.read_sema); > + misc_deregister(&fcopy_misc); > +} > + > +int hv_fcopy_init(struct hv_util_service *srv) > +{ > + recv_buffer = srv->recv_buffer; > + > + /* > + * When this driver loads, the user level daemon that > + * processes the host requests may not yet be running. > + * Defer processing channel callbacks until the daemon > + * has registered. > + */ > + fcopy_transaction.active = true; > + sema_init(&fcopy_transaction.read_sema, 0); > + > + return fcopy_dev_init(); > +} > + > +void hv_fcopy_deinit(void) > +{ > + cancel_delayed_work_sync(&fcopy_work); > + fcopy_dev_deinit(); > +} > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c > index 62dfd246..efc5e83 100644 > --- a/drivers/hv/hv_util.c > +++ b/drivers/hv/hv_util.c > @@ -82,6 +82,12 @@ static struct hv_util_service util_vss = { > .util_deinit = hv_vss_deinit, > }; > > +static struct hv_util_service util_fcopy = { > + .util_cb = hv_fcopy_onchannelcallback, > + .util_init = hv_fcopy_init, > + .util_deinit = hv_fcopy_deinit, > +}; > + > static void perform_shutdown(struct work_struct *dummy) > { > orderly_poweroff(true); > @@ -401,6 +407,10 @@ static const struct hv_vmbus_device_id id_table[] = { > { HV_VSS_GUID, > .driver_data = (unsigned long)&util_vss > }, > + /* File copy GUID */ > + { HV_FCOPY_GUID, > + .driver_data = (unsigned long)&util_fcopy > + }, > { }, > }; > > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > index fb66fba..d1ae9e4 100644 > --- a/include/linux/hyperv.h > +++ b/include/linux/hyperv.h > @@ -30,7 +30,6 @@ > #include <linux/types.h> > #include <linux/scatterlist.h> > #include <linux/list.h> > -#include <linux/uuid.h> > #include <linux/timer.h> > #include <linux/workqueue.h> > #include <linux/completion.h> > @@ -1050,6 +1049,17 @@ void vmbus_driver_unregister(struct hv_driver *hv_driver); > } > > /* > + * Guest File Copy Service > + * {34D14BE3-DEE4-41c8-9AE7-6B174977C192} > + */ > + > +#define HV_FCOPY_GUID \ > + .guid = { \ > + 0xE3, 0x4B, 0xD1, 0x34, 0xE4, 0xDE, 0xC8, 0x41, \ > + 0x9A, 0xE7, 0x6B, 0x17, 0x49, 0x77, 0xC1, 0x92 \ > + } > + > +/* > * Common header for Hyper-V ICs > */ > > @@ -1160,6 +1170,10 @@ void hv_vss_onchannelcallback(void *); > extern u64 hyperv_mmio_start; > extern u64 hyperv_mmio_size; > > +int hv_fcopy_init(struct hv_util_service *); > +void hv_fcopy_deinit(void); > +void hv_fcopy_onchannelcallback(void *); Why are these needed here and not in a local .h file in drivers/hv/? thanks, greg k-h _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
