From 92e9b4998d78bad7857b8fd769e89f3835661643 Mon Sep 17 00:00:00 2001 From: William Henderson Date: Wed, 30 Aug 2023 15:31:17 +0000 Subject: respond to more of Thanos's review Signed-off-by: William Henderson --- lib/dma.c | 33 ++++++++++++++++++--------------- lib/libvfio-user.c | 7 +++++-- lib/migration.c | 14 +++++++++----- 3 files changed, 32 insertions(+), 22 deletions(-) diff --git a/lib/dma.c b/lib/dma.c index bde1226..b9588b7 100644 --- a/lib/dma.c +++ b/lib/dma.c @@ -579,7 +579,7 @@ dirty_page_get_extend(dma_memory_region_t *region, char *bitmap, */ uint8_t client_bit_idx = 0; size_t server_byte_idx; - int server_bit_idx_into_byte; + int server_bit_idx; size_t factor = server_pgsize / client_pgsize; /* @@ -587,6 +587,7 @@ dirty_page_get_extend(dma_memory_region_t *region, char *bitmap, */ for (server_byte_idx = 0; server_byte_idx < server_bitmap_size; server_byte_idx++) { + if (client_bit_idx / CHAR_BIT >= client_bitmap_size) { break; } @@ -599,10 +600,8 @@ dirty_page_get_extend(dma_memory_region_t *region, char *bitmap, * Iterate through the bits of the server byte, repeating bits to reach * the desired page size. */ - 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; + for (server_bit_idx = 0; server_bit_idx < CHAR_BIT; server_bit_idx++) { + uint8_t server_bit = (out >> server_bit_idx) & 1; /* * Repeat `factor` times the bit at index `j` of `out`. @@ -611,11 +610,12 @@ dirty_page_get_extend(dma_memory_region_t *region, char *bitmap, * `factor` bits in the client bitmap, from `client_bit_idx` to * `end_client_bit_idx`. */ - size_t end_client_bit_idx = client_bit_idx + factor; - while (client_bit_idx < end_client_bit_idx) { + for (size_t end_client_bit_idx = client_bit_idx + factor; + client_bit_idx < end_client_bit_idx; + client_bit_idx++) { + bitmap[client_bit_idx / CHAR_BIT] |= server_bit << (client_bit_idx % CHAR_BIT); - client_bit_idx++; } } } @@ -634,7 +634,7 @@ dirty_page_get_combine(dma_memory_region_t *region, char *bitmap, */ uint8_t client_bit_idx = 0; size_t server_byte_idx; - int server_bit_idx_into_byte; + int server_bit_idx; size_t factor = client_pgsize / server_pgsize; /* @@ -642,6 +642,7 @@ dirty_page_get_combine(dma_memory_region_t *region, char *bitmap, */ for (server_byte_idx = 0; server_byte_idx < server_bitmap_size; server_byte_idx++) { + if (client_bit_idx / CHAR_BIT >= client_bitmap_size) { break; } @@ -654,10 +655,8 @@ dirty_page_get_combine(dma_memory_region_t *region, char *bitmap, * Iterate through the bits of the server byte, combining bits to reach * the desired page size. */ - 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; + for (server_bit_idx = 0; server_bit_idx < CHAR_BIT; server_bit_idx++) { + uint8_t server_bit = (out >> server_bit_idx) & 1; /* * OR `factor` bits of the server bitmap with the same bit at @@ -670,9 +669,13 @@ dirty_page_get_combine(dma_memory_region_t *region, char *bitmap, * Only move onto the next bit in the client bitmap once we've * OR'd `factor` bits. */ - if (((server_byte_idx * CHAR_BIT) + server_bit_idx_into_byte) - % factor == factor - 1) { + if (((server_byte_idx * CHAR_BIT) + server_bit_idx) % factor + == factor - 1) { client_bit_idx++; + + if (client_bit_idx / CHAR_BIT >= client_bitmap_size) { + return; + } } } } diff --git a/lib/libvfio-user.c b/lib/libvfio-user.c index c0ca55d..03b1f9d 100644 --- a/lib/libvfio-user.c +++ b/lib/libvfio-user.c @@ -1039,6 +1039,7 @@ handle_dma_device_feature_get(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg, if (dma == NULL) { vfu_log(vfu_ctx, LOG_ERR, "DMA not enabled for DMA device feature"); + return ERROR_INT(EINVAL); } ssize_t bitmap_size = get_bitmap_size(rep->length, rep->page_size); @@ -1109,7 +1110,8 @@ 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"); + vfu_log(vfu_ctx, LOG_ERR, "message too short (%ld)", + msg->in.iov.iov_len); return ERROR_INT(EINVAL); } @@ -1121,7 +1123,8 @@ 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)"); + vfu_log(vfu_ctx, LOG_ERR, "unsupported operation(s), flags=%d", + req->flags); return ERROR_INT(EINVAL); } diff --git a/lib/migration.c b/lib/migration.c index aefd103..8901a00 100644 --- a/lib/migration.c +++ b/lib/migration.c @@ -292,7 +292,8 @@ 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"); + vfu_log(vfu_ctx, LOG_ERR, "message too short (%ld)", + msg->in.iov.iov_len); return ERROR_INT(EINVAL); } @@ -335,7 +336,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"); + vfu_log(vfu_ctx, LOG_ERR, "read_data callback failed, errno=%d", errno); msg->out.iov.iov_len = 0; return ret; } @@ -353,7 +354,8 @@ 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"); + vfu_log(vfu_ctx, LOG_ERR, "message too short (%ld)", + msg->in.iov.iov_len); return ERROR_INT(EINVAL); } @@ -386,10 +388,12 @@ 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"); + vfu_log(vfu_ctx, LOG_ERR, "write_data callback failed, errno=%d", + errno); return ret; } else if (ret != req->size) { - vfu_log(vfu_ctx, LOG_ERR, "migration data partial write"); + vfu_log(vfu_ctx, LOG_ERR, "migration data partial write of size=%ld", + ret); return ERROR_INT(EINVAL); } -- cgit v1.1