From ad087c8c1573848b17683fe02776df0b73b82e13 Mon Sep 17 00:00:00 2001 From: William Henderson Date: Wed, 30 Aug 2023 13:34:11 +0000 Subject: more fixes from Thanos's review Signed-off-by: William Henderson --- lib/dma.c | 97 +++++++++++++++++++++++++++---------- lib/dma.h | 6 +-- lib/libvfio-user.c | 139 +++++++++++++++++++++++++++++++++-------------------- lib/migration.c | 6 +++ 4 files changed, 166 insertions(+), 82 deletions(-) diff --git a/lib/dma.c b/lib/dma.c index 67ee6c1..1a73c7c 100644 --- a/lib/dma.c +++ b/lib/dma.c @@ -611,11 +611,11 @@ dma_controller_dirty_page_get(dma_controller_t *dma, vfu_dma_addr_t addr, } if (pgsize == dma->dirty_pgsize) { - dirty_page_get_same_pgsize(region, bitmap, server_bitmap_size); + dirty_page_get_same_pgsize(region, bitmap, client_bitmap_size); } else { dirty_page_get_different_pgsize(region, bitmap, server_bitmap_size, - client_bitmap_size, pgsize, - dma->dirty_pgsize); + dma->dirty_pgsize, client_bitmap_size, + pgsize); } #ifdef DEBUG @@ -657,47 +657,92 @@ dirty_page_get_same_pgsize(dma_memory_region_t *region, char *bitmap, void dirty_page_get_different_pgsize(dma_memory_region_t *region, char *bitmap, - size_t server_bitmap_size, - size_t client_bitmap_size, size_t server_pgsize, - size_t client_pgsize) + size_t server_bitmap_size, size_t server_pgsize, + size_t client_bitmap_size, size_t client_pgsize) { - uint8_t bit = 0; - size_t i; - int j; + /* + * The index of the bit in the client bitmap that we are currently + * considering. By keeping track of this separately to the for loops, we + * allow for one server bit to be repeated for multiple client bytes, or + * multiple bytes' worth of server bits to be OR'd together to calculate one + * client bit. + */ + uint8_t client_bit_idx = 0; + /* + * The index of the byte in the server bitmap that we are currently + * considering. + */ + size_t server_byte_idx; + /* + * The index of the bit in the currently considered byte of the server + * bitmap that we are currently considering. + */ + int server_bit_idx_into_byte; - bool extend = server_pgsize <= client_pgsize; + /* + * Whether we are extending the server bitmap (repeating bits to + * generate a larger client bitmap) or not (combining bits with OR to + * generate a smaller client bitmap). + * + * If the server page size is greater than the client's requested page size, + * we will be extending. + */ + bool extend = server_pgsize >= client_pgsize; + /* + * If extending, the number of times to repeat each bit of the server + * bitmap. If not, the number of bits of the server bitmap to OR together to + * calculate one bit of the client bitmap. + */ size_t factor = extend ? - client_pgsize / server_pgsize : server_pgsize / client_pgsize; + server_pgsize / client_pgsize : client_pgsize / server_pgsize; - for (i = 0; i < server_bitmap_size && - bit / CHAR_BIT < (size_t)client_bitmap_size; i++) { + /* + * Iterate through the bytes of the server bitmap. + */ + for (server_byte_idx = 0; server_byte_idx < server_bitmap_size && + client_bit_idx / CHAR_BIT < client_bitmap_size; server_byte_idx++) { uint8_t out = 0; - dirty_page_exchange(&out, ®ion->dirty_bitmap[i]); + dirty_page_exchange(&out, ®ion->dirty_bitmap[server_byte_idx]); /* - * Iterate through the bits of the byte, repeating or combining bits - * to reach the desired page size. + * Iterate through the bits of the server byte, repeating or combining + * bits to reach the desired page size. */ - for (j = 0; j < CHAR_BIT; j++) { - uint8_t b = (out >> j) & 1; + for (server_bit_idx_into_byte = 0; + server_bit_idx_into_byte < CHAR_BIT; + server_bit_idx_into_byte++) { + uint8_t server_bit = (out >> server_bit_idx_into_byte) & 1; if (extend) { /* * Repeat `factor` times the bit at index `j` of `out`. + * + * OR the same bit from the server bitmap (`server_bit`) with + * `factor` bits in the client bitmap, from `client_bit_idx` to + * `end_client_bit_idx`. */ - size_t new_bit = bit + factor; - while (bit < new_bit) { - bitmap[bit / 8] |= b << (bit % 8); - bit++; + size_t end_client_bit_idx = client_bit_idx + factor; + while (client_bit_idx < end_client_bit_idx) { + bitmap[client_bit_idx / CHAR_BIT] |= + server_bit << (client_bit_idx % CHAR_BIT); + client_bit_idx++; } } else { /* - * OR the same bit with `factor` bits of the raw bitmap. + * OR `factor` bits of the server bitmap with the same bit at + * index `client_bit_idx` in the client bitmap. + */ + bitmap[client_bit_idx / CHAR_BIT] |= + server_bit << (client_bit_idx % CHAR_BIT); + + /* + * Only move onto the next bit in the client bitmap once we've + * OR'd `factor` bits. */ - bitmap[bit / 8] |= b << (bit % 8); - if (((i * 8) + j) % factor == factor - 1) { - bit++; + if (((server_byte_idx * CHAR_BIT) + server_bit_idx_into_byte) + % factor == factor - 1) { + client_bit_idx++; } } } diff --git a/lib/dma.h b/lib/dma.h index d3c4e18..eaa4052 100644 --- a/lib/dma.h +++ b/lib/dma.h @@ -392,9 +392,9 @@ dirty_page_get_same_pgsize(dma_memory_region_t *region, char *bitmap, size_t bitmap_size); void dirty_page_get_different_pgsize(dma_memory_region_t *region, char *bitmap, - size_t bitmap_size, - size_t converted_bitmap_size, size_t pgsize, - size_t converted_pgsize); + size_t server_bitmap_size, size_t server_pgsize, + size_t client_bitmap_size, + size_t client_pgsize); bool dma_sg_is_mappable(const dma_controller_t *dma, const dma_sg_t *sg); diff --git a/lib/libvfio-user.c b/lib/libvfio-user.c index e54e95b..c0ca55d 100644 --- a/lib/libvfio-user.c +++ b/lib/libvfio-user.c @@ -962,8 +962,10 @@ static int handle_migration_device_feature_get(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg, struct vfio_user_device_feature *req) { - // all supported outgoing data is currently the same size as - // vfio_user_device_feature_migration + /* + * All supported outgoing data is currently the same size as + * struct vfio_user_device_feature_migration. + */ msg->out.iov.iov_len = sizeof(struct vfio_user_device_feature) + sizeof(struct vfio_user_device_feature_migration); @@ -1005,7 +1007,10 @@ handle_migration_device_feature_get(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg, return 0; } - default: return ERROR_INT(EINVAL); + default: + vfu_log(vfu_ctx, LOG_ERR, "invalid flags for migration GET (%d)", + req->flags); + return ERROR_INT(EINVAL); } } @@ -1030,6 +1035,12 @@ handle_dma_device_feature_get(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg, struct vfio_user_device_feature_dma_logging_report *rep = (void *)req->data; + dma_controller_t *dma = vfu_ctx->dma; + + if (dma == NULL) { + vfu_log(vfu_ctx, LOG_ERR, "DMA not enabled for DMA device feature"); + } + ssize_t bitmap_size = get_bitmap_size(rep->length, rep->page_size); if (bitmap_size < 0) { return bitmap_size; @@ -1054,10 +1065,6 @@ handle_dma_device_feature_get(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg, res->argsz = msg->out.iov.iov_len; - dma_controller_t *dma = vfu_ctx->dma; - - assert(dma != NULL); - char *bitmap = (char *)msg->out.iov.iov_base + header_size; int ret = dma_controller_dirty_page_get(dma, @@ -1087,12 +1094,12 @@ handle_dma_device_feature_set(vfu_ctx_t *vfu_ctx, uint32_t feature, (void *)res->data; return dma_controller_dirty_page_logging_start(dma, ctl->page_size); - } else { - assert(feature == VFIO_DEVICE_FEATURE_DMA_LOGGING_STOP); - - dma_controller_dirty_page_logging_stop(dma); - return 0; } + + assert(feature == VFIO_DEVICE_FEATURE_DMA_LOGGING_STOP); + + dma_controller_dirty_page_logging_stop(dma); + return 0; } static int @@ -1102,6 +1109,7 @@ handle_device_feature(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg) assert(msg != NULL); if (msg->in.iov.iov_len < sizeof(struct vfio_user_device_feature)) { + vfu_log(vfu_ctx, LOG_ERR, "message too short"); return ERROR_INT(EINVAL); } @@ -1113,65 +1121,90 @@ handle_device_feature(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg) uint32_t supported_ops = device_feature_flags_supported(vfu_ctx, feature); if ((req->flags & supported_ops) != operations || supported_ops == 0) { + vfu_log(vfu_ctx, LOG_ERR, "unsupported operation(s)"); return ERROR_INT(EINVAL); } ssize_t ret; - if (req->flags & VFIO_DEVICE_FEATURE_PROBE) { - msg->out.iov.iov_len = msg->in.iov.iov_len; - - if (req->argsz < msg->out.iov.iov_len) { - msg->out.iov.iov_len = 0; - return ERROR_INT(EINVAL); + switch (operations) { + case VFIO_DEVICE_FEATURE_GET: { + if (is_migration_feature(feature)) { + ret = handle_migration_device_feature_get(vfu_ctx, msg, req); + } else if (is_dma_feature(feature)) { + ret = handle_dma_device_feature_get(vfu_ctx, msg, req); + } else { + vfu_log(vfu_ctx, LOG_ERR, "unsupported feature %d for GET", + feature); + return ERROR_INT(EINVAL); + } + break; } - msg->out.iov.iov_base = malloc(msg->out.iov.iov_len); + case VFIO_DEVICE_FEATURE_SET: { + msg->out.iov.iov_len = msg->in.iov.iov_len; - if (msg->out.iov.iov_base == NULL) { - return ERROR_INT(ENOMEM); - } + if (req->argsz < msg->out.iov.iov_len) { + vfu_log(vfu_ctx, LOG_ERR, "bad argsz (%d<%ld)", req->argsz, + msg->out.iov.iov_len); + msg->out.iov.iov_len = 0; + return ERROR_INT(EINVAL); + } - memcpy(msg->out.iov.iov_base, msg->in.iov.iov_base, - msg->out.iov.iov_len); + msg->out.iov.iov_base = malloc(msg->out.iov.iov_len); - ret = 0; - } else if (req->flags & VFIO_DEVICE_FEATURE_GET) { - if (is_migration_feature(feature)) { - ret = handle_migration_device_feature_get(vfu_ctx, msg, req); - } else if (is_dma_feature(feature)) { - ret = handle_dma_device_feature_get(vfu_ctx, msg, req); - } else { - return ERROR_INT(EINVAL); - } - } else if (req->flags & VFIO_DEVICE_FEATURE_SET) { - msg->out.iov.iov_len = msg->in.iov.iov_len; + if (msg->out.iov.iov_base == NULL) { + return ERROR_INT(ENOMEM); + } - if (req->argsz < msg->out.iov.iov_len) { - msg->out.iov.iov_len = 0; - return ERROR_INT(EINVAL); - } + memcpy(msg->out.iov.iov_base, msg->in.iov.iov_base, + msg->out.iov.iov_len); - msg->out.iov.iov_base = malloc(msg->out.iov.iov_len); + struct vfio_user_device_feature *res = msg->out.iov.iov_base; - if (msg->out.iov.iov_base == NULL) { - return ERROR_INT(ENOMEM); + if (is_migration_feature(feature)) { + ret = handle_migration_device_feature_set(vfu_ctx, feature, res); + } else if (is_dma_feature(feature)) { + ret = handle_dma_device_feature_set(vfu_ctx, feature, res); + } else { + vfu_log(vfu_ctx, LOG_ERR, "unsupported feature %d for SET", + feature); + return ERROR_INT(EINVAL); + } + break; } - memcpy(msg->out.iov.iov_base, msg->in.iov.iov_base, - msg->out.iov.iov_len); + default: { + /* + * PROBE allows GET/SET to also be set (to specify which operations + * we want to probe the feature for), so we only check that PROBE + * is set, not that it is the only operation flag set. + */ + if (!(operations & VFIO_DEVICE_FEATURE_PROBE)) { + vfu_log(vfu_ctx, LOG_ERR, "no operation specified"); + return ERROR_INT(EINVAL); + } - struct vfio_user_device_feature *res = msg->out.iov.iov_base; + msg->out.iov.iov_len = msg->in.iov.iov_len; - if (is_migration_feature(feature)) { - ret = handle_migration_device_feature_set(vfu_ctx, feature, res); - } else if (is_dma_feature(feature)) { - ret = handle_dma_device_feature_set(vfu_ctx, feature, res); - } else { - return ERROR_INT(EINVAL); + if (req->argsz < msg->out.iov.iov_len) { + vfu_log(vfu_ctx, LOG_ERR, "bad argsz (%d<%ld)", req->argsz, + msg->out.iov.iov_len); + msg->out.iov.iov_len = 0; + return ERROR_INT(EINVAL); + } + + msg->out.iov.iov_base = malloc(msg->out.iov.iov_len); + + if (msg->out.iov.iov_base == NULL) { + return ERROR_INT(ENOMEM); + } + + memcpy(msg->out.iov.iov_base, msg->in.iov.iov_base, + msg->out.iov.iov_len); + + ret = 0; } - } else { - return ERROR_INT(EINVAL); } return ret; diff --git a/lib/migration.c b/lib/migration.c index 20d542c..aefd103 100644 --- a/lib/migration.c +++ b/lib/migration.c @@ -292,6 +292,7 @@ handle_mig_data_read(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg) assert(msg != NULL); if (msg->in.iov.iov_len < sizeof(struct vfio_user_mig_data)) { + vfu_log(vfu_ctx, LOG_ERR, "message too short"); return ERROR_INT(EINVAL); } @@ -299,6 +300,7 @@ handle_mig_data_read(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg) struct vfio_user_mig_data *req = msg->in.iov.iov_base; if (vfu_ctx->migration == NULL) { + vfu_log(vfu_ctx, LOG_ERR, "migration not enabled"); return ERROR_INT(EINVAL); } @@ -333,6 +335,7 @@ handle_mig_data_read(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg) ssize_t ret = migr->callbacks.read_data(vfu_ctx, &res->data, req->size); if (ret < 0) { + vfu_log(vfu_ctx, LOG_ERR, "read_data callback failed"); msg->out.iov.iov_len = 0; return ret; } @@ -350,6 +353,7 @@ handle_mig_data_write(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg) assert(msg != NULL); if (msg->in.iov.iov_len < sizeof(struct vfio_user_mig_data)) { + vfu_log(vfu_ctx, LOG_ERR, "message too short"); return ERROR_INT(EINVAL); } @@ -357,6 +361,7 @@ handle_mig_data_write(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg) struct vfio_user_mig_data *req = msg->in.iov.iov_base; if (vfu_ctx->migration == NULL) { + vfu_log(vfu_ctx, LOG_ERR, "migration not enabled"); return ERROR_INT(EINVAL); } @@ -381,6 +386,7 @@ handle_mig_data_write(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg) ssize_t ret = migr->callbacks.write_data(vfu_ctx, &req->data, req->size); if (ret < 0) { + vfu_log(vfu_ctx, LOG_ERR, "write_data callback failed"); return ret; } else if (ret != req->size) { vfu_log(vfu_ctx, LOG_ERR, "migration data partial write"); -- cgit v1.1