Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adapt to new (v2) VFIO migration interface #741

Closed
wants to merge 9 commits into from
Closed

Adapt to new (v2) VFIO migration interface #741

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e31b36a
update to latest version of the spec
w-henderson Jul 6, 2023
f753886
start updating spec for migration v2
w-henderson Jul 6, 2023
2d38f2f
start implementing migration v2 (WIP)
w-henderson Jun 26, 2023
4bf29bc
first draft libvfio-user migration v2 implementation
w-henderson Jun 26, 2023
ddf1f0a
implement migration v2 read/write
w-henderson Jun 27, 2023
3228e38
move samples to migration v2
w-henderson Jun 27, 2023
45e4246
fix: make client/server sample work with migration v2
w-henderson Jul 5, 2023
dcc535f
add DMA dirty page logging to the spec
w-henderson Jul 6, 2023
7c68798
fix: correct command enum for updated spec
w-henderson Jul 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions include/vfio-user.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ enum vfio_user_command {
VFIO_USER_DMA_WRITE = 12,
VFIO_USER_DEVICE_RESET = 13,
VFIO_USER_DIRTY_PAGES = 14,
VFIO_USER_DEVICE_FEATURE = 15,
w-henderson marked this conversation as resolved.
Show resolved Hide resolved
VFIO_USER_MAX,
};

Expand Down Expand Up @@ -228,6 +229,44 @@ struct vfio_user_bitmap_range {

#endif /* VFIO_REGION_TYPE_MIGRATION */

/* Analogous to vfio_device_feature */
struct vfio_user_device_feature {
uint32_t argsz;
uint32_t flags;
#define VFIO_DEVICE_FEATURE_MASK (0xffff) /* 16-bit feature index */
w-henderson marked this conversation as resolved.
Show resolved Hide resolved
#define VFIO_DEVICE_FEATURE_GET (1 << 16) /* Get feature into data[] */
#define VFIO_DEVICE_FEATURE_SET (1 << 17) /* Set feature from data[] */
#define VFIO_DEVICE_FEATURE_PROBE (1 << 18) /* Probe feature support */
uint8_t data[];
} __attribute__((packed));

/* Analogous to vfio_device_feature_migration */
struct vfio_user_device_feature_migration {
uint64_t flags;
#define VFIO_MIGRATION_STOP_COPY (1 << 0)
#define VFIO_MIGRATION_P2P (1 << 1)
#define VFIO_MIGRATION_PRE_COPY (1 << 2)
} __attribute__((packed));
w-henderson marked this conversation as resolved.
Show resolved Hide resolved
#define VFIO_DEVICE_FEATURE_MIGRATION 1

/* Analogous to vfio_device_feature_mig_state */
struct vfio_user_device_feature_mig_state {
uint32_t device_state;
uint32_t data_fd;
} __attribute__((packed));
w-henderson marked this conversation as resolved.
Show resolved Hide resolved
#define VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE 2

enum vfio_device_mig_state {
w-henderson marked this conversation as resolved.
Show resolved Hide resolved
VFIO_DEVICE_STATE_ERROR = 0,
VFIO_DEVICE_STATE_STOP = 1,
VFIO_DEVICE_STATE_RUNNING = 2,
VFIO_DEVICE_STATE_STOP_COPY = 3,
VFIO_DEVICE_STATE_RESUMING = 4,
VFIO_DEVICE_STATE_RUNNING_P2P = 5,
VFIO_DEVICE_STATE_PRE_COPY = 6,
VFIO_DEVICE_STATE_PRE_COPY_P2P = 7,
};

#ifdef __cplusplus
}
#endif
Expand Down
43 changes: 43 additions & 0 deletions lib/libvfio-user.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1052,6 +1052,45 @@ handle_dirty_pages(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
return ret;
}

static int
handle_device_feature(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
{
struct vfio_user_device_feature *req = msg->in.iov.iov_base;
w-henderson marked this conversation as resolved.
Show resolved Hide resolved

if (vfu_ctx->migration == NULL) {
w-henderson marked this conversation as resolved.
Show resolved Hide resolved
return -EINVAL;
}

if (!migration_feature_supported(req->flags)) {
w-henderson marked this conversation as resolved.
Show resolved Hide resolved
// FIXME what error code to return? we really want "not supported"
// instead of "not permitted"?
return -EINVAL;
}

ssize_t ret;

if (req->flags & VFIO_DEVICE_FEATURE_PROBE) {
jlevon marked this conversation as resolved.
Show resolved Hide resolved
msg->out.iov.iov_base = msg->in.iov.iov_base;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation doesn't make much sense to me: it has no effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We set the reply to equal the request, as this is what VFIO and hence vfio-user dictates for the success response - this has since been replaced by the following, as the aliasing caused a double free: 😳

msg->out.iov.iov_base = malloc(msg->in.iov.iov_len);
msg->out.iov.iov_len = msg->in.iov.iov_len;
memcpy(msg->out.iov.iov_base, msg->in.iov.iov_base,
       msg->out.iov.iov_len);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such an API would not be useful. I think you're supposed to respond with the set of flags known to the server. (And, if get, or set, is set, whether the server supports that verb for the given flag(s)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure we want to diverge from VFIO here?

From documentation of VFIO_DEVICE_FEATURE:

Get, set, or probe feature data of the device. The feature is selected using the FEATURE_MASK portion of the flags field. Support for a feature can be probed by setting both the FEATURE_MASK and PROBE bits. A probe may optionally include the GET and/or SET bits to determine read vs write access of the feature respectively. Probing a feature will return success if the feature is supported and all of the optionally indicated GET/SET methods are supported. The format of the data portion of the structure is specific to the given feature. The data portion is not required for probing. GET and SET are mutually exclusive, except for use with PROBE.

msg->out.iov.iov_len = msg->in.iov.iov_len;

ret = 0;
} else if (req->flags & VFIO_DEVICE_FEATURE_GET) {
msg->out.iov.iov_base = calloc(8, 1);
w-henderson marked this conversation as resolved.
Show resolved Hide resolved
msg->out.iov.iov_len = 8;

ret = migration_feature_get(vfu_ctx, req->flags,
msg->out.iov.iov_base);
} else if (req->flags & VFIO_DEVICE_FEATURE_SET) {
msg->out.iov.iov_base = msg->in.iov.iov_base;
jlevon marked this conversation as resolved.
Show resolved Hide resolved
msg->out.iov.iov_len = msg->in.iov.iov_len;

ret = migration_feature_set(vfu_ctx, req->flags,
msg->out.iov.iov_base);
}

return ret;
}

static vfu_msg_t *
alloc_msg(struct vfio_user_header *hdr, int *fds, size_t nr_fds)
{
Expand Down Expand Up @@ -1221,6 +1260,10 @@ handle_request(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
}
break;

case VFIO_USER_DEVICE_FEATURE:
ret = handle_device_feature(vfu_ctx, msg);
break;

default:
msg->processed_cmd = false;
vfu_log(vfu_ctx, LOG_ERR, "bad command %d", msg->hdr.cmd);
Expand Down
54 changes: 54 additions & 0 deletions lib/migration.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,60 @@ migration_region_access(vfu_ctx_t *vfu_ctx, char *buf, size_t count,
return count;
}

bool
migration_feature_supported(uint32_t flags) {
w-henderson marked this conversation as resolved.
Show resolved Hide resolved
switch (flags & VFIO_DEVICE_FEATURE_MASK) {
case VFIO_DEVICE_FEATURE_MIGRATION:
return !(flags & VFIO_DEVICE_FEATURE_SET); // not supported for set
w-henderson marked this conversation as resolved.
Show resolved Hide resolved
case VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE:
jlevon marked this conversation as resolved.
Show resolved Hide resolved
return true;
default:
return false;
};
}

ssize_t
migration_feature_get(vfu_ctx_t *vfu_ctx, uint32_t flags, void *buf)
{
struct vfio_user_device_feature_migration *res;
struct vfio_user_device_feature_mig_state *state;

switch (flags & VFIO_DEVICE_FEATURE_MASK) {
w-henderson marked this conversation as resolved.
Show resolved Hide resolved
case VFIO_DEVICE_FEATURE_MIGRATION:
res = buf;
// FIXME are these always supported? Can we consider to be
// "supported" if said support is just an empty callback?
res->flags = VFIO_MIGRATION_STOP_COPY | VFIO_MIGRATION_PRE_COPY;
w-henderson marked this conversation as resolved.
Show resolved Hide resolved

w-henderson marked this conversation as resolved.
Show resolved Hide resolved
return 0;

case VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE:
state = buf;
state->device_state = vfu_ctx->migration->info.device_state;

return 0;

default:
return -EINVAL;

};
}

ssize_t
migration_feature_set(UNUSED vfu_ctx_t *vfu_ctx, uint32_t flags,
UNUSED void *buf)
{
if (flags & VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE) {
// struct vfio_user_device_feature_mig_state *res = buf;

// TODO migrate to res->device_state state if allowed to

return 0;
w-henderson marked this conversation as resolved.
Show resolved Hide resolved
}

return -EINVAL;
}

bool
MOCK_DEFINE(device_is_stopped_and_copying)(struct migration *migr)
{
Expand Down
9 changes: 9 additions & 0 deletions lib/migration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,15 @@ ssize_t
migration_region_access(vfu_ctx_t *vfu_ctx, char *buf, size_t count,
loff_t pos, bool is_write);

bool
migration_feature_supported(uint32_t flags);

ssize_t
migration_feature_get(vfu_ctx_t *vfu_ctx, uint32_t flags, void *buf);

ssize_t
migration_feature_set(vfu_ctx_t *vfu_ctx, uint32_t flags, void *buf);

bool
migration_available(vfu_ctx_t *vfu_ctx);

Expand Down
20 changes: 19 additions & 1 deletion test/py/libvfio_user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,16 @@
VFIO_DEVICE_STATE_V1_RESUMING = (1 << 2)
VFIO_DEVICE_STATE_MASK = ((1 << 3) - 1)

VFIO_DEVICE_FEATURE_MIGRATION = (1)
w-henderson marked this conversation as resolved.
Show resolved Hide resolved
VFIO_DEVICE_FEATURE_MASK = (0xffff)
VFIO_DEVICE_FEATURE_GET = (1 << 16)
VFIO_DEVICE_FEATURE_SET = (1 << 17)
VFIO_DEVICE_FEATURE_PROBE = (1 << 18)

VFIO_MIGRATION_STOP_COPY = (1 << 0)
VFIO_MIGRATION_P2P = (1 << 1)
VFIO_MIGRATION_PRE_COPY = (1 << 2)


# libvfio-user defines

Expand Down Expand Up @@ -171,7 +181,8 @@ def is_32bit():
VFIO_USER_DMA_WRITE = 12
VFIO_USER_DEVICE_RESET = 13
VFIO_USER_DIRTY_PAGES = 14
VFIO_USER_MAX = 15
VFIO_USER_DEVICE_FEATURE = 15
VFIO_USER_MAX = 16

VFIO_USER_F_TYPE_COMMAND = 0
VFIO_USER_F_TYPE_REPLY = 1
Expand Down Expand Up @@ -569,6 +580,13 @@ def __str__(self):
(hex(self.dma_addr), self.region, hex(self.length),
hex(self.offset), self.writeable)

class vfio_user_device_feature(Structure):
_pack_ = 1
_fields_ = [
("argsz", c.c_uint32),
("flags", c.c_uint32)
]


class vfio_user_migration_info(Structure):
_pack_ = 1
Expand Down