Re: [RFC v3 3/5] KVM: implement wire protocol

Jason Wang
2021-03-09T06:20:42Z

On 2021/2/21 8:04 下午, Elena Afanasova wrote:
> Add ioregionfd blocking read/write operations.
>
> Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
> ---
> v3:
> - change wire protocol license
> - remove ioregionfd_cmd info and drop appropriate macros
> - fix ioregionfd state machine
> - add sizeless ioregions support
> - drop redundant check in ioregion_read/write()
>
> include/uapi/linux/ioregion.h | 30 +++++++
> virt/kvm/ioregion.c | 162 +++++++++++++++++++++++++++++++++-
> 2 files changed, 190 insertions(+), 2 deletions(-)
> create mode 100644 include/uapi/linux/ioregion.h
>
> diff --git a/include/uapi/linux/ioregion.h b/include/uapi/linux/ioregion.h
> new file mode 100644
> index 000000000000..58f9b5ba6186
> --- /dev/null
> +++ b/include/uapi/linux/ioregion.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: ((GPL-2.0-only WITH Linux-syscall-note) OR BSD-3-Clause) */
> +#ifndef _UAPI_LINUX_IOREGION_H
> +#define _UAPI_LINUX_IOREGION_H
> +
> +/* Wire protocol */
> +
> +struct ioregionfd_cmd {
> + __u8 cmd;
> + __u8 size_exponent : 4;
> + __u8 resp : 1;
> + __u8 padding[6];
> + __u64 user_data;
> + __u64 offset;
> + __u64 data;
> +};

Sorry if I've asked this before. Do we need a id for each
request/response? E.g an ioregion fd could be used by multiple vCPUS.
VCPU needs to have a way to find which request belongs to itself or not?

> +
> +struct ioregionfd_resp {
> + __u64 data;
> + __u8 pad[24];
> +};
> +
> +#define IOREGIONFD_CMD_READ 0
> +#define IOREGIONFD_CMD_WRITE 1
> +
> +#define IOREGIONFD_SIZE_8BIT 0
> +#define IOREGIONFD_SIZE_16BIT 1
> +#define IOREGIONFD_SIZE_32BIT 2
> +#define IOREGIONFD_SIZE_64BIT 3
> +
> +#endif
> diff --git a/virt/kvm/ioregion.c b/virt/kvm/ioregion.c
> index e09ef3e2c9d7..1e1c7772d274 100644
> --- a/virt/kvm/ioregion.c
> +++ b/virt/kvm/ioregion.c
> @@ -3,6 +3,7 @@
> #include <linux/fs.h>
> #include <kvm/iodev.h>
> #include "eventfd.h"
> +#include <uapi/linux/ioregion.h>
>
> void
> kvm_ioregionfd_init(struct kvm *kvm)
> @@ -40,18 +41,175 @@ ioregion_release(struct ioregion *p)
> kfree(p);
> }
>
> +static bool
> +pack_cmd(struct ioregionfd_cmd *cmd, u64 offset, u64 len, u8 opt, u8 resp,
> + u64 user_data, const void *val)
> +{
> + switch (len) {
> + case 0:
> + break;
> + case 1:
> + cmd->size_exponent = IOREGIONFD_SIZE_8BIT;
> + break;
> + case 2:
> + cmd->size_exponent = IOREGIONFD_SIZE_16BIT;
> + break;
> + case 4:
> + cmd->size_exponent = IOREGIONFD_SIZE_32BIT;
> + break;
> + case 8:
> + cmd->size_exponent = IOREGIONFD_SIZE_64BIT;
> + break;
> + default:
> + return false;
> + }
> +
> + if (val)
> + memcpy(&cmd->data, val, len);
> + cmd->user_data = user_data;
> + cmd->offset = offset;
> + cmd->cmd = opt;
> + cmd->resp = resp;
> +
> + return true;
> +}
> +
> +enum {
> + SEND_CMD,
> + GET_REPLY,
> + COMPLETE
> +};
> +
> +static void
> +ioregion_save_ctx(struct kvm_vcpu *vcpu, bool in, gpa_t addr, u8 state, void *val)
> +{
> + vcpu->ioregion_ctx.is_interrupted = true;
> + vcpu->ioregion_ctx.val = val;
> + vcpu->ioregion_ctx.state = state;
> + vcpu->ioregion_ctx.addr = addr;
> + vcpu->ioregion_ctx.in = in;
> +}
> +
> static int
> ioregion_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this, gpa_t addr,
> int len, void *val)
> {
> - return -EOPNOTSUPP;
> + struct ioregion *p = to_ioregion(this);
> + union {
> + struct ioregionfd_cmd cmd;
> + struct ioregionfd_resp resp;
> + } buf;
> + int ret = 0;
> + int state = SEND_CMD;
> +
> + if (unlikely(vcpu->ioregion_ctx.is_interrupted)) {
> + vcpu->ioregion_ctx.is_interrupted = false;
> +
> + switch (vcpu->ioregion_ctx.state) {
> + case SEND_CMD:
> + goto send_cmd;
> + case GET_REPLY:
> + goto get_repl;
> + default:
> + return -EINVAL;
> + }
> + }
> +
> +send_cmd:
> + memset(&buf, 0, sizeof(buf));
> + if (!pack_cmd(&buf.cmd, addr - p->paddr, len, IOREGIONFD_CMD_READ,
> + 1, p->user_data, NULL))
> + return -EOPNOTSUPP;
> +
> + ret = kernel_write(p->wf, &buf.cmd, sizeof(buf.cmd), 0);
> + state = (ret == sizeof(buf.cmd)) ? GET_REPLY : SEND_CMD;
> + if (signal_pending(current) && state == SEND_CMD) {

Can the signal be delivered after a success of kernel_write()? If yes,
is there any side effect if we want to redo the write here?

> + ioregion_save_ctx(vcpu, 1, addr, state, val);
> + return -EINTR;
> + }
> + if (ret != sizeof(buf.cmd)) {
> + ret = (ret < 0) ? ret : -EIO;
> + return (ret == -EAGAIN || ret == -EWOULDBLOCK) ? -EINVAL : ret;
> + }
> + if (!p->rf)
> + return 0;
> +
> +get_repl:
> + memset(&buf, 0, sizeof(buf));
> + ret = kernel_read(p->rf, &buf.resp, sizeof(buf.resp), 0);
> + state = (ret == sizeof(buf.resp)) ? COMPLETE : GET_REPLY;
> + if (signal_pending(current) && state == GET_REPLY) {
> + ioregion_save_ctx(vcpu, 1, addr, state, val);
> + return -EINTR;
> + }
> + if (ret != sizeof(buf.resp)) {
> + ret = (ret < 0) ? ret : -EIO;
> + return (ret == -EAGAIN || ret == -EWOULDBLOCK) ? -EINVAL : ret;
> + }

We may need to unify the duplicated codes here with send_cmd.

> +
> + memcpy(val, &buf.resp.data, len);
> +
> + return 0;
> }
>
> static int
> ioregion_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this, gpa_t addr,
> int len, const void *val)
> {
> - return -EOPNOTSUPP;
> + struct ioregion *p = to_ioregion(this);
> + union {
> + struct ioregionfd_cmd cmd;
> + struct ioregionfd_resp resp;
> + } buf;
> + int ret = 0;
> + int state = SEND_CMD;
> +
> + if (unlikely(vcpu->ioregion_ctx.is_interrupted)) {
> + vcpu->ioregion_ctx.is_interrupted = false;
> +
> + switch (vcpu->ioregion_ctx.state) {
> + case SEND_CMD:
> + goto send_cmd;
> + case GET_REPLY:
> + goto get_repl;
> + default:
> + return -EINVAL;
> + }
> + }
> +
> +send_cmd:
> + memset(&buf, 0, sizeof(buf));
> + if (!pack_cmd(&buf.cmd, addr - p->paddr, len, IOREGIONFD_CMD_WRITE,
> + p->posted_writes ? 0 : 1, p->user_data, val))
> + return -EOPNOTSUPP;
> +
> + ret = kernel_write(p->wf, &buf.cmd, sizeof(buf.cmd), 0);
> + state = (ret == sizeof(buf.cmd)) ? GET_REPLY : SEND_CMD;
> + if (signal_pending(current) && state == SEND_CMD) {
> + ioregion_save_ctx(vcpu, 0, addr, state, (void *)val);
> + return -EINTR;
> + }
> + if (ret != sizeof(buf.cmd)) {
> + ret = (ret < 0) ? ret : -EIO;
> + return (ret == -EAGAIN || ret == -EWOULDBLOCK) ? -EINVAL : ret;
> + }
> +
> +get_repl:
> + if (!p->posted_writes) {
> + memset(&buf, 0, sizeof(buf));
> + ret = kernel_read(p->rf, &buf.resp, sizeof(buf.resp), 0);
> + state = (ret == sizeof(buf.resp)) ? COMPLETE : GET_REPLY;
> + if (signal_pending(current) && state == GET_REPLY) {
> + ioregion_save_ctx(vcpu, 0, addr, state, (void *)val);
> + return -EINTR;
> + }
> + if (ret != sizeof(buf.resp)) {
> + ret = (ret < 0) ? ret : -EIO;
> + return (ret == -EAGAIN || ret == -EWOULDBLOCK) ? -EINVAL : ret;
> + }
> + }
> +
> + return 0;

It looks to me we had more chance to unify the code with ioregion_read().

Thanks

> }
>
> /*