From 597b8a146781e1c7ab4a76a463c64cd8fbaced76 Mon Sep 17 00:00:00 2001 From: Shengwen Cheng Date: Thu, 27 Jul 2023 18:09:40 +0800 Subject: [PATCH] Refactor VirtIO MMIO register map handling This commit addresses the following: 1. Re-organizes the source structures to improve code organization and maintainability. 2. Introduces the use of opaque pointer for information hiding, enhancing encapsulation and abstraction. 3. Enforces mnemonic enums instead of numbers, enhancing code readability and maintainability. By incorporating these changes, the codebase becomes more structured and easier to understand and maintain. --- common.h | 9 ++++ device.h | 6 ++- virtio-blk.c | 146 +++++++++++++++++++++++++++++++++++---------------- virtio-net.c | 107 ++++++++++++++++++++++++++----------- virtio.h | 33 ++++++++++++ 5 files changed, 223 insertions(+), 78 deletions(-) diff --git a/common.h b/common.h index fe3887e..2a45682 100644 --- a/common.h +++ b/common.h @@ -16,3 +16,12 @@ static inline int ilog2(int x) { return 31 - __builtin_clz(x | 1); } + +/* Range check + * For any variable range checking: + * if (x >= minx && x <= maxx) ... + * it is faster to use bit operation: + * if ((signed)((x - minx) | (maxx - x)) >= 0) ... + */ +#define RANGE_CHECK(x, minx, size) \ + ((int32_t) ((x - minx) | (minx + size - 1 - x)) >= 0) diff --git a/device.h b/device.h index 262fd95..55d05b3 100644 --- a/device.h +++ b/device.h @@ -1,6 +1,7 @@ #pragma once #include "riscv.h" +#include "virtio.h" /* RAM */ @@ -100,6 +101,8 @@ typedef struct { /* supplied by environment */ int tap_fd; uint32_t *ram; + /* implementation-specific */ + void *priv; } virtio_net_state_t; void virtio_net_read(vm_t *core, @@ -147,7 +150,8 @@ typedef struct { /* supplied by environment */ uint32_t *ram; uint32_t *disk; - uint64_t capacity; + /* implementation-specific */ + void *priv; } virtio_blk_state_t; void virtio_blk_read(vm_t *vm, diff --git a/virtio-blk.c b/virtio-blk.c index 2003aff..4da1d56 100644 --- a/virtio-blk.c +++ b/virtio-blk.c @@ -9,6 +9,7 @@ #include #include +#include "common.h" #include "device.h" #include "riscv.h" #include "riscv_private.h" @@ -16,11 +17,46 @@ #define DISK_BLK_SIZE 512 +#define VBLK_DEV_CNT_MAX 1 + #define VBLK_FEATURES_0 0 #define VBLK_FEATURES_1 1 /* VIRTIO_F_VERSION_1 */ #define VBLK_QUEUE_NUM_MAX 1024 #define VBLK_QUEUE (vblk->queues[vblk->QueueSel]) +#define PRIV(x) ((struct virtio_blk_config *) x->priv) + +struct virtio_blk_config { + uint64_t capacity; + uint32_t size_max; + uint32_t seg_max; + + struct virtio_blk_geometry { + uint16_t cylinders; + uint8_t heads; + uint8_t sectors; + } geometry; + + uint32_t blk_size; + + struct virtio_blk_topology { + uint8_t physical_block_exp; + uint8_t alignment_offset; + uint16_t min_io_size; + uint32_t opt_io_size; + } topology; + + uint8_t writeback; + uint8_t unused0[3]; + uint32_t max_discard_sectors; + uint32_t max_discard_seg; + uint32_t discard_sector_alignment; + uint32_t max_write_zeroes_sectors; + uint32_t max_write_zeroes_seg; + uint8_t write_zeroes_may_unmap; + uint8_t unused1[3]; +} __attribute__((packed)); + struct vblk_req_header { uint32_t type; uint32_t reserved; @@ -28,6 +64,9 @@ struct vblk_req_header { uint8_t status; } __attribute__((packed)); +struct virtio_blk_config vblk_configs[VBLK_DEV_CNT_MAX]; +int vblk_dev_cnt = 0; + static void virtio_blk_set_fail(virtio_blk_state_t *vblk) { vblk->Status |= VIRTIO_STATUS__DEVICE_NEEDS_RESET; @@ -52,11 +91,13 @@ static void virtio_blk_update_status(virtio_blk_state_t *vblk, uint32_t status) /* Reset */ uint32_t *ram = vblk->ram; uint32_t *disk = vblk->disk; - uint32_t capacity = vblk->capacity; + void *priv = vblk->priv; + uint32_t capacity = PRIV(vblk)->capacity; memset(vblk, 0, sizeof(*vblk)); vblk->ram = ram; vblk->disk = disk; - vblk->capacity = capacity; + vblk->priv = priv; + PRIV(vblk)->capacity = capacity; } static void virtio_blk_write_handler(virtio_blk_state_t *vblk, @@ -128,7 +169,7 @@ static int virtio_blk_desc_handler(virtio_blk_state_t *vblk, uint8_t *status = (uint8_t *) ((uintptr_t) vblk->ram + vq_desc[2].addr); /* Check sector index is valid */ - if (sector > (vblk->capacity - 1)) { + if (sector > (PRIV(vblk)->capacity - 1)) { *status = VIRTIO_BLK_S_IOERR; return -1; } @@ -219,125 +260,129 @@ static bool virtio_blk_reg_read(virtio_blk_state_t *vblk, uint32_t addr, uint32_t *value) { - /* TODO: replace the register address with enum. - * For the address after the configuration space, it should be - * handled by structure pointer depend on the device. - */ +#define _(reg) VIRTIO_##reg switch (addr) { - case 0: /* MagicValue (R) */ + case _(MagicValue): *value = 0x74726976; return true; - case 1: /* Version (R) */ + case _(Version): *value = 2; return true; - case 2: /* DeviceID (R) */ + case _(DeviceID): *value = 2; return true; - case 3: /* VendorID (R) */ + case _(VendorID): *value = VIRTIO_VENDOR_ID; return true; - case 4: /* DeviceFeatures (R) */ + case _(DeviceFeatures): *value = vblk->DeviceFeaturesSel == 0 ? VBLK_FEATURES_0 : (vblk->DeviceFeaturesSel == 1 ? VBLK_FEATURES_1 : 0); return true; - case 13: /* QueueNumMax (R) */ + case _(QueueNumMax): *value = VBLK_QUEUE_NUM_MAX; return true; - case 17: /* QueueReady (RW) */ + case _(QueueReady): *value = VBLK_QUEUE.ready ? 1 : 0; return true; - case 24: /* InterruptStatus (R) */ + case _(InterruptStatus): *value = vblk->InterruptStatus; return true; - case 28: /* Status (RW) */ + case _(Status): *value = vblk->Status; return true; - case 63: /* ConfigGeneration (R) */ + case _(ConfigGeneration): *value = 0; return true; - case 64: /* CapacityLow (R) */ - *value = 0x00000000ffffffff & vblk->capacity; - return true; - case 65: /* CapacityHigh (R) */ - *value = vblk->capacity >> 32; - return true; default: - return false; + /* Invalid address which exceeded the range */ + if (!RANGE_CHECK(addr, _(Config), sizeof(struct virtio_blk_config))) + return false; + + /* Read configuration from the corresponding register */ + *value = ((uint32_t *) PRIV(vblk))[addr - _(Config)]; + + return true; } +#undef _ } static bool virtio_blk_reg_write(virtio_blk_state_t *vblk, uint32_t addr, uint32_t value) { - /* TODO: replace the register address with enum. - * For the address after the configuration space, it should be - * handled by structure pointer depend on the device. - */ +#define _(reg) VIRTIO_##reg switch (addr) { - case 5: /* DeviceFeaturesSel (W) */ + case _(DeviceFeaturesSel): vblk->DeviceFeaturesSel = value; return true; - case 8: /* DriverFeatures (W) */ + case _(DriverFeatures): vblk->DriverFeaturesSel == 0 ? (vblk->DriverFeatures = value) : 0; return true; - case 9: /* DriverFeaturesSel (W) */ + case _(DriverFeaturesSel): vblk->DriverFeaturesSel = value; return true; - case 12: /* QueueSel (W) */ + case _(QueueSel): if (value < ARRAY_SIZE(vblk->queues)) vblk->QueueSel = value; else virtio_blk_set_fail(vblk); return true; - case 14: /* QueueNum (W) */ + case _(QueueNum): if (value > 0 && value <= VBLK_QUEUE_NUM_MAX) VBLK_QUEUE.QueueNum = value; else virtio_blk_set_fail(vblk); return true; - case 17: /* QueueReady (RW) */ + case _(QueueReady): VBLK_QUEUE.ready = value & 1; if (value & 1) VBLK_QUEUE.last_avail = vblk->ram[VBLK_QUEUE.QueueAvail] >> 16; return true; - case 32: /* QueueDescLow (W) */ + case _(QueueDescLow): VBLK_QUEUE.QueueDesc = vblk_preprocess(vblk, value); return true; - case 33: /* QueueDescHigh (W) */ + case _(QueueDescHigh): if (value) virtio_blk_set_fail(vblk); return true; - case 36: /* QueueAvailLow (W) */ + case _(QueueDriverLow): VBLK_QUEUE.QueueAvail = vblk_preprocess(vblk, value); return true; - case 37: /* QueueAvailHigh (W) */ + case _(QueueDriverHigh): if (value) virtio_blk_set_fail(vblk); return true; - case 40: /* QueueUsedLow (W) */ + case _(QueueDeviceLow): VBLK_QUEUE.QueueUsed = vblk_preprocess(vblk, value); return true; - case 41: /* QueueUsedHigh (W) */ + case _(QueueDeviceHigh): if (value) virtio_blk_set_fail(vblk); return true; - case 20: /* QueueNotify (W) */ + case _(QueueNotify): if (value < ARRAY_SIZE(vblk->queues)) virtio_queue_notify_handler(vblk, value); else virtio_blk_set_fail(vblk); return true; - case 25: /* InterruptACK (W) */ + case _(InterruptACK): vblk->InterruptStatus &= ~value; return true; - case 28: /* Status (RW) */ + case _(Status): virtio_blk_update_status(vblk, value); return true; default: - return false; + /* Invalid address which exceeded the range */ + if (!RANGE_CHECK(addr, _(Config), sizeof(struct virtio_blk_config))) + return false; + + /* Write configuration to the corresponding register */ + ((uint32_t *) PRIV(vblk))[addr - _(Config)] = value; + + return true; } +#undef _ } void virtio_blk_read(vm_t *vm, @@ -386,11 +431,20 @@ void virtio_blk_write(vm_t *vm, uint32_t *virtio_blk_init(virtio_blk_state_t *vblk, char *disk_file) { + if (vblk_dev_cnt >= VBLK_DEV_CNT_MAX) { + fprintf(stderr, + "Excedded the number of virtio-blk device can be allocated.\n"); + exit(2); + } + + /* Allocate memory for the private member */ + vblk->priv = &vblk_configs[vblk_dev_cnt++]; + /* No disk image is provided */ if (!disk_file) { /* By setting the block capacity to zero, the kernel will * then not to touch the device after booting */ - vblk->capacity = 0; + PRIV(vblk)->capacity = 0; return NULL; } @@ -417,7 +471,7 @@ uint32_t *virtio_blk_init(virtio_blk_state_t *vblk, char *disk_file) close(disk_fd); vblk->disk = disk_mem; - vblk->capacity = (disk_size - 1) / DISK_BLK_SIZE + 1; + PRIV(vblk)->capacity = (disk_size - 1) / DISK_BLK_SIZE + 1; return disk_mem; } diff --git a/virtio-net.c b/virtio-net.c index 6de2479..f720851 100644 --- a/virtio-net.c +++ b/virtio-net.c @@ -11,6 +11,7 @@ #include #include +#include "common.h" #include "device.h" #include "riscv.h" #include "riscv_private.h" @@ -18,6 +19,8 @@ #define TAP_INTERFACE "tap%d" +#define VNET_DEV_CNT_MAX 1 + #define VNET_FEATURES_0 0 #define VNET_FEATURES_1 1 /* VIRTIO_F_VERSION_1 */ #define VNET_QUEUE_NUM_MAX 1024 @@ -27,8 +30,20 @@ ((addr) < RAM_SIZE && !((addr) &0b11) ? ((addr) >> 2) \ : (virtio_net_set_fail(vnet), 0)) +#define PRIV(x) ((struct virtio_net_config *) x->priv) + enum { VNET_QUEUE_RX = 0, VNET_QUEUE_TX = 1 }; +struct virtio_net_config { + uint8_t mac[6]; + uint16_t status; + uint16_t max_virtqueue_pairs; + uint16_t mtu; +} __attribute__((packed)); + +struct virtio_net_config vnet_configs[VNET_DEV_CNT_MAX]; +int vnet_dev_cnt = 0; + static void virtio_net_set_fail(virtio_net_state_t *vnet) { vnet->Status |= VIRTIO_STATUS__DEVICE_NEEDS_RESET; @@ -45,8 +60,10 @@ static void virtio_net_update_status(virtio_net_state_t *vnet, uint32_t status) /* Reset */ int tap_fd = vnet->tap_fd; uint32_t *ram = vnet->ram; + void *priv = vnet->priv; memset(vnet, 0, sizeof(*vnet)); vnet->tap_fd = tap_fd, vnet->ram = ram; + vnet->priv = priv; } static bool vnet_iovec_write(struct iovec **vecs, @@ -218,79 +235,90 @@ static bool virtio_net_reg_read(virtio_net_state_t *vnet, uint32_t addr, uint32_t *value) { +#define _(reg) VIRTIO_##reg switch (addr) { - case 0: /* MagicValue (R) */ + case _(MagicValue): *value = 0x74726976; return true; - case 1: /* Version (R) */ + case _(Version): *value = 2; return true; - case 2: /* DeviceID (R) */ + case _(DeviceID): *value = 1; return true; - case 3: /* VendorID (R) */ + case _(VendorID): *value = VIRTIO_VENDOR_ID; return true; - case 4: /* DeviceFeatures (R) */ + case _(DeviceFeatures): *value = vnet->DeviceFeaturesSel == 0 ? VNET_FEATURES_0 : (vnet->DeviceFeaturesSel == 1 ? VNET_FEATURES_1 : 0); return true; - case 13: /* QueueNumMax (R) */ + case _(QueueNumMax): *value = VNET_QUEUE_NUM_MAX; return true; - case 17: /* QueueReady (RW) */ + case _(QueueReady): *value = VNET_QUEUE.ready ? 1 : 0; return true; - case 24: /* InterruptStatus (R) */ + case _(InterruptStatus): *value = vnet->InterruptStatus; return true; - case 28: /* Status (RW) */ + case _(Status): *value = vnet->Status; return true; - case 63: /* ConfigGeneration (R) */ + case _(ConfigGeneration): *value = 0; return true; - /* TODO: We should also expose MAC address (even if with invalid value) - * under 8-bit accesses. + + /* TODO: May want to check the occasion that the Linux kernel + * touches the MAC address of the virtio-net under 8-bit accesses */ default: - return false; + /* Invalid address which exceeded the range */ + if (!RANGE_CHECK(addr, _(Config), sizeof(struct virtio_net_config))) + return false; + + /* Read configuration from the corresponding register */ + *value = ((uint32_t *) PRIV(vnet))[addr - _(Config)]; + + return true; } +#undef _ } static bool virtio_net_reg_write(virtio_net_state_t *vnet, uint32_t addr, uint32_t value) { +#define _(reg) VIRTIO_##reg switch (addr) { - case 5: /* DeviceFeaturesSel (W) */ + case _(DeviceFeaturesSel): vnet->DeviceFeaturesSel = value; return true; - case 8: /* DriverFeatures (W) */ + case _(DriverFeatures): vnet->DriverFeaturesSel == 0 ? (vnet->DriverFeatures = value) : 0; return true; - case 9: /* DriverFeaturesSel (W) */ + case _(DriverFeaturesSel): vnet->DriverFeaturesSel = value; return true; - case 12: /* QueueSel (W) */ + case _(QueueSel): if (value < ARRAY_SIZE(vnet->queues)) vnet->QueueSel = value; else virtio_net_set_fail(vnet); return true; - case 14: /* QueueNum (W) */ + case _(QueueNum): if (value > 0 && value <= VNET_QUEUE_NUM_MAX) VNET_QUEUE.QueueNum = value; else virtio_net_set_fail(vnet); return true; - case 17: /* QueueReady (RW) */ + case _(QueueReady): VNET_QUEUE.ready = value & 1; if (value & 1) VNET_QUEUE.last_avail = vnet->ram[VNET_QUEUE.QueueAvail] >> 16; @@ -298,29 +326,29 @@ static bool virtio_net_reg_write(virtio_net_state_t *vnet, vnet->ram[VNET_QUEUE.QueueAvail] |= 1; /* set VIRTQ_AVAIL_F_NO_INTERRUPT */ return true; - case 32: /* QueueDescLow (W) */ + case _(QueueDescLow): VNET_QUEUE.QueueDesc = VNET_PREPROCESS_ADDR(value); return true; - case 33: /* QueueDescHigh (W) */ + case _(QueueDescHigh): if (value) virtio_net_set_fail(vnet); return true; - case 36: /* QueueAvailLow (W) */ + case _(QueueDriverLow): VNET_QUEUE.QueueAvail = VNET_PREPROCESS_ADDR(value); return true; - case 37: /* QueueAvailHigh (W) */ + case _(QueueDriverHigh): if (value) virtio_net_set_fail(vnet); return true; - case 40: /* QueueUsedLow (W) */ + case _(QueueDeviceLow): VNET_QUEUE.QueueUsed = VNET_PREPROCESS_ADDR(value); return true; - case 41: /* QueueUsedHigh (W) */ + case _(QueueDeviceHigh): if (value) virtio_net_set_fail(vnet); return true; - case 20: /* QueueNotify (W) */ + case _(QueueNotify): if (value < ARRAY_SIZE(vnet->queues)) { switch (value) { case VNET_QUEUE_RX: @@ -334,19 +362,27 @@ static bool virtio_net_reg_write(virtio_net_state_t *vnet, virtio_net_set_fail(vnet); } return true; - case 25: /* InterruptACK (W) */ + case _(InterruptACK): vnet->InterruptStatus &= ~value; return true; - case 28: /* Status (RW) */ + case _(Status): virtio_net_update_status(vnet, value); return true; - /* TODO: We should also expose MAC address (even if with invalid value) - * under 8-bit accesses. + /* TODO: May want to check the occasion that the Linux kernel + * touches the MAC address of the virtio-net under 8-bit accesses */ default: - return false; + /* Invalid address which exceeded the range */ + if (!RANGE_CHECK(addr, _(Config), sizeof(struct virtio_net_config))) + return false; + + /* Write configuration to the corresponding register */ + ((uint32_t *) PRIV(vnet))[addr - _(Config)] = value; + + return true; } +#undef _ } void virtio_net_read(vm_t *vm, @@ -395,6 +431,15 @@ void virtio_net_write(vm_t *vm, bool virtio_net_init(virtio_net_state_t *vnet) { + if (vnet_dev_cnt >= VNET_DEV_CNT_MAX) { + fprintf(stderr, + "Excedded the number of virtio-net device can be allocated.\n"); + exit(2); + } + + /* Allocate memory for the private member */ + vnet->priv = &vnet_configs[vnet_dev_cnt++]; + vnet->tap_fd = open("/dev/net/tun", O_RDWR); if (vnet->tap_fd < 0) { fprintf(stderr, "failed to open TAP device: %s\n", strerror(errno)); diff --git a/virtio.h b/virtio.h index f7151fa..b893ef5 100644 --- a/virtio.h +++ b/virtio.h @@ -24,6 +24,39 @@ #define VIRTIO_BLK_S_IOERR 1 #define VIRTIO_BLK_S_UNSUPP 2 +/* VirtIO MMIO registers */ +#define VIRTIO_REG_LIST \ + _(MagicValue, 0x000) /* R */ \ + _(Version, 0x004) /* R */ \ + _(DeviceID, 0x008) /* R */ \ + _(VendorID, 0x00c) /* R */ \ + _(DeviceFeatures, 0x010) /* R */ \ + _(DeviceFeaturesSel, 0x014) /* W */ \ + _(DriverFeatures, 0x020) /* W */ \ + _(DriverFeaturesSel, 0x024) /* W */ \ + _(QueueSel, 0x030) /* W */ \ + _(QueueNumMax, 0x034) /* R */ \ + _(QueueNum, 0x038) /* W */ \ + _(QueueReady, 0x044) /* RW */ \ + _(QueueNotify, 0x050) /* W */ \ + _(InterruptStatus, 0x60) /* R */ \ + _(InterruptACK, 0x064) /* W */ \ + _(Status, 0x070) /* RW */ \ + _(QueueDescLow, 0x080) /* W */ \ + _(QueueDescHigh, 0x084) /* W */ \ + _(QueueDriverLow, 0x090) /* W */ \ + _(QueueDriverHigh, 0x094) /* W */ \ + _(QueueDeviceLow, 0x0a0) /* W */ \ + _(QueueDeviceHigh, 0x0a4) /* W */ \ + _(ConfigGeneration, 0x0fc) /* R */ \ + _(Config, 0x100) /* RW */ + +enum { +#define _(reg, addr) VIRTIO_##reg = addr >> 2, + VIRTIO_REG_LIST +#undef _ +}; + struct virtq_desc { uint32_t addr; uint32_t len;