From 4034159c1a4f5c433ae4987ff35e037a071272b7 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Mon, 8 Apr 2024 05:47:29 -0400 Subject: [PATCH 1/7] Revert "hw/virtio: Add support for VDPA network simulation devices" This reverts commit cd341fd1ffded978b2aa0b5309b00be7c42e347c. The patch adds non-upstream code in include/standard-headers/linux/virtio_pci.h which would make maintainance harder. Revert for now. Suggested-by: Jason Wang Message-Id: Acked-by: Jason Wang Signed-off-by: Michael S. Tsirkin --- MAINTAINERS | 5 - docs/system/device-emulation.rst | 1 - docs/system/devices/vdpa-net.rst | 121 ------------- hw/net/virtio-net.c | 16 -- hw/virtio/virtio-pci.c | 189 +------------------- hw/virtio/virtio.c | 39 ---- include/hw/virtio/virtio-pci.h | 5 - include/hw/virtio/virtio.h | 19 -- include/standard-headers/linux/virtio_pci.h | 7 - 9 files changed, 3 insertions(+), 399 deletions(-) delete mode 100644 docs/system/devices/vdpa-net.rst diff --git a/MAINTAINERS b/MAINTAINERS index e71183eef9..249b678fc6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2370,11 +2370,6 @@ F: hw/virtio/vhost-user-scmi* F: include/hw/virtio/vhost-user-scmi.h F: tests/qtest/libqos/virtio-scmi.* -vdpa-net -M: Hao Chen -S: Maintained -F: docs/system/devices/vdpa-net.rst - virtio-crypto M: Gonglei S: Supported diff --git a/docs/system/device-emulation.rst b/docs/system/device-emulation.rst index e4a27f53c8..f19777411c 100644 --- a/docs/system/device-emulation.rst +++ b/docs/system/device-emulation.rst @@ -99,4 +99,3 @@ Emulated Devices devices/canokey.rst devices/usb-u2f.rst devices/igb.rst - devices/vdpa-net.rst diff --git a/docs/system/devices/vdpa-net.rst b/docs/system/devices/vdpa-net.rst deleted file mode 100644 index 323d8c926a..0000000000 --- a/docs/system/devices/vdpa-net.rst +++ /dev/null @@ -1,121 +0,0 @@ -vdpa net -============ - -This document explains the setup and usage of the vdpa network device. -The vdpa network device is a paravirtualized vdpa emulate device. - -Description ------------ - -VDPA net devices support dirty page bitmap mark and vring state saving and recovery. - -Users can use this VDPA device for live migration simulation testing in a nested virtualization environment. - -Registers layout ----------------- - -The vdpa device add live migrate registers layout as follow:: - - Offset Register Name Bitwidth Associated vq - 0x0 LM_LOGGING_CTRL 4bits - 0x10 LM_BASE_ADDR_LOW 32bits - 0x14 LM_BASE_ADDR_HIGH 32bits - 0x18 LM_END_ADDR_LOW 32bits - 0x1c LM_END_ADDR_HIGH 32bits - 0x20 LM_RING_STATE_OFFSET 32bits vq0 - 0x24 LM_RING_STATE_OFFSET 32bits vq1 - 0x28 LM_RING_STATE_OFFSET 32bits vq2 - ...... - 0x20+1023*4 LM_RING_STATE_OFFSET 32bits vq1023 - -These registers are extended at the end of the notify bar space. - -Architecture diagram --------------------- -:: - - |------------------------------------------------------------------------| - | guest-L1-user-space | - | | - | |----------------------------------------| - | | [virtio-net driver] | - | | ^ guest-L2-src(iommu=on) | - | |--------------|-------------------------| - | | | qemu-L2-src(viommu) | - | [dpdk-vdpa]<->[vhost socket]<-+->[vhost-user backend(iommu=on)] | - -------------------------------------------------------------------------- - -------------------------------------------------------------------------- - | ^ guest-L1-kernel-space | - | | | - | [VFIO] | - | ^ | - | | guest-L1-src(iommu=on) | - --------|----------------------------------------------------------------- - --------|----------------------------------------------------------------- - | [vdpa net device(iommu=on)] [manager nic device] | - | | | | - | | | | - | [tap device] qemu-L1-src(viommu) | | - ------------------------------------------------+------------------------- - | - | - --------------------- | - | kernel net bridge |<----- - | virbr0 |<---------------------------------- - --------------------- | - | - | - -------------------------------------------------------------------------- | - | guest-L1-user-space | | - | | | - | |----------------------------------------| | - | | [virtio-net driver] | | - | | ^ guest-L2-dst(iommu=on) | | - | |--------------|-------------------------| | - | | | qemu-L2-dst(viommu) | | - | [dpdk-vdpa]<->[vhost socket]<-+->[vhost-user backend(iommu=on)] | | - -------------------------------------------------------------------------- | - -------------------------------------------------------------------------- | - | ^ guest-L1-kernel-space | | - | | | | - | [VFIO] | | - | ^ | | - | | guest-L1-dst(iommu=on) | | - --------|----------------------------------------------------------------- | - --------|----------------------------------------------------------------- | - | [vdpa net device(iommu=on)] [manager nic device]----------------+---- - | | | - | | | - | [tap device] qemu-L1-dst(viommu) | - -------------------------------------------------------------------------- - - -Device properties ------------------ - -The Virtio vdpa device can be configured with the following properties: - - * ``vdpa=on`` open vdpa device emulated. - -Usages --------- -This patch add virtio sriov support and vdpa live migrate support. -You can open vdpa by set xml file as follow:: - - - - - - - - - - -Limitations ------------ -1. Dependent on tap device with param ``vhost=off``. -2. Nested virtualization environment only supports ``q35`` machines. -3. Current only support split vring live migrate. - - - diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 58014a92ad..24e5e7d347 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -2039,22 +2039,6 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, goto err; } - /* Mark dirty page's bitmap of guest memory */ - if (vdev->lm_logging_ctrl == LM_ENABLE) { - uint64_t chunk = elem->in_addr[i] / VHOST_LOG_CHUNK; - /* Get chunk index */ - BitmapMemoryRegionCaches *caches = qatomic_rcu_read(&vdev->caches); - uint64_t index = chunk / 8; - uint64_t shift = chunk % 8; - uint8_t val = 0; - address_space_read_cached(&caches->bitmap, index, &val, - sizeof(val)); - val |= 1 << shift; - address_space_write_cached(&caches->bitmap, index, &val, - sizeof(val)); - address_space_cache_invalidate(&caches->bitmap, index, sizeof(val)); - } - elems[i] = elem; lens[i] = total; i++; diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index eaaf86402c..cb6940fc0e 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1442,155 +1442,6 @@ int virtio_pci_add_shm_cap(VirtIOPCIProxy *proxy, return virtio_pci_add_mem_cap(proxy, &cap.cap); } -/* Called within call_rcu(). */ -static void bitmap_free_region_cache(BitmapMemoryRegionCaches *caches) -{ - assert(caches != NULL); - address_space_cache_destroy(&caches->bitmap); - g_free(caches); -} - -static void lm_disable(VirtIODevice *vdev) -{ - BitmapMemoryRegionCaches *caches; - caches = qatomic_read(&vdev->caches); - qatomic_rcu_set(&vdev->caches, NULL); - if (caches) { - call_rcu(caches, bitmap_free_region_cache, rcu); - } -} - -static void lm_enable(VirtIODevice *vdev) -{ - BitmapMemoryRegionCaches *old = vdev->caches; - BitmapMemoryRegionCaches *new = NULL; - hwaddr addr, end, size; - int64_t len; - - addr = vdev->lm_base_addr_low | ((hwaddr)(vdev->lm_base_addr_high) << 32); - end = vdev->lm_end_addr_low | ((hwaddr)(vdev->lm_end_addr_high) << 32); - size = end - addr; - if (size <= 0) { - error_report("Invalid lm size."); - return; - } - - new = g_new0(BitmapMemoryRegionCaches, 1); - len = address_space_cache_init(&new->bitmap, vdev->dma_as, addr, size, - true); - if (len < size) { - virtio_error(vdev, "Cannot map bitmap"); - goto err_bitmap; - } - qatomic_rcu_set(&vdev->caches, new); - - if (old) { - call_rcu(old, bitmap_free_region_cache, rcu); - } - - return; - -err_bitmap: - address_space_cache_destroy(&new->bitmap); - g_free(new); -} - -static uint64_t virtio_pci_lm_read(void *opaque, hwaddr addr, - unsigned size) -{ - VirtIOPCIProxy *proxy = opaque; - VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); - hwaddr offset_end = LM_VRING_STATE_OFFSET + - virtio_pci_queue_mem_mult(proxy) * VIRTIO_QUEUE_MAX; - uint32_t val; - int qid; - - if (vdev == NULL) { - return UINT64_MAX; - } - switch (addr) { - case LM_LOGGING_CTRL: - val = vdev->lm_logging_ctrl; - break; - case LM_BASE_ADDR_LOW: - val = vdev->lm_base_addr_low; - break; - case LM_BASE_ADDR_HIGH: - val = vdev->lm_base_addr_high; - break; - case LM_END_ADDR_LOW: - val = vdev->lm_end_addr_low; - break; - case LM_END_ADDR_HIGH: - val = vdev->lm_end_addr_high; - break; - default: - if (addr >= LM_VRING_STATE_OFFSET && addr <= offset_end) { - qid = (addr - LM_VRING_STATE_OFFSET) / - virtio_pci_queue_mem_mult(proxy); - val = virtio_queue_get_vring_states(vdev, qid); - } else - val = 0; - - break; - } - - return val; -} - -static void virtio_pci_lm_write(void *opaque, hwaddr addr, - uint64_t val, unsigned size) -{ - VirtIOPCIProxy *proxy = opaque; - VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); - hwaddr offset_end = LM_VRING_STATE_OFFSET + - virtio_pci_queue_mem_mult(proxy) * VIRTIO_QUEUE_MAX; - int qid; - - if (vdev == NULL) { - return; - } - - switch (addr) { - case LM_LOGGING_CTRL: - vdev->lm_logging_ctrl = val; - switch (val) { - case LM_DISABLE: - lm_disable(vdev); - break; - case LM_ENABLE: - lm_enable(vdev); - break; - default: - virtio_error(vdev, "Unsupport LM_LOGGING_CTRL value: %"PRIx64, - val); - break; - }; - - break; - case LM_BASE_ADDR_LOW: - vdev->lm_base_addr_low = val; - break; - case LM_BASE_ADDR_HIGH: - vdev->lm_base_addr_high = val; - break; - case LM_END_ADDR_LOW: - vdev->lm_end_addr_low = val; - break; - case LM_END_ADDR_HIGH: - vdev->lm_end_addr_high = val; - break; - default: - if (addr >= LM_VRING_STATE_OFFSET && addr <= offset_end) { - qid = (addr - LM_VRING_STATE_OFFSET) / - virtio_pci_queue_mem_mult(proxy); - virtio_queue_set_vring_states(vdev, qid, val); - } else - virtio_error(vdev, "Unsupport addr: %"PRIx64, addr); - break; - } -} - static uint64_t virtio_pci_common_read(void *opaque, hwaddr addr, unsigned size) { @@ -1972,15 +1823,6 @@ static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy, }, .endianness = DEVICE_LITTLE_ENDIAN, }; - static const MemoryRegionOps lm_ops = { - .read = virtio_pci_lm_read, - .write = virtio_pci_lm_write, - .impl = { - .min_access_size = 1, - .max_access_size = 4, - }, - .endianness = DEVICE_LITTLE_ENDIAN, - }; g_autoptr(GString) name = g_string_new(NULL); g_string_printf(name, "virtio-pci-common-%s", vdev_name); @@ -2017,14 +1859,6 @@ static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy, proxy, name->str, proxy->notify_pio.size); - if (proxy->flags & VIRTIO_PCI_FLAG_VDPA) { - g_string_printf(name, "virtio-pci-lm-%s", vdev_name); - memory_region_init_io(&proxy->lm.mr, OBJECT(proxy), - &lm_ops, - proxy, - name->str, - proxy->lm.size); - } } static void virtio_pci_modern_region_map(VirtIOPCIProxy *proxy, @@ -2187,10 +2021,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) virtio_pci_modern_mem_region_map(proxy, &proxy->isr, &cap); virtio_pci_modern_mem_region_map(proxy, &proxy->device, &cap); virtio_pci_modern_mem_region_map(proxy, &proxy->notify, ¬ify.cap); - if (proxy->flags & VIRTIO_PCI_FLAG_VDPA) { - memory_region_add_subregion(&proxy->modern_bar, - proxy->lm.offset, &proxy->lm.mr); - } if (modern_pio) { memory_region_init(&proxy->io_bar, OBJECT(proxy), @@ -2260,9 +2090,6 @@ static void virtio_pci_device_unplugged(DeviceState *d) virtio_pci_modern_mem_region_unmap(proxy, &proxy->isr); virtio_pci_modern_mem_region_unmap(proxy, &proxy->device); virtio_pci_modern_mem_region_unmap(proxy, &proxy->notify); - if (proxy->flags & VIRTIO_PCI_FLAG_VDPA) { - memory_region_del_subregion(&proxy->modern_bar, &proxy->lm.mr); - } if (modern_pio) { virtio_pci_modern_io_region_unmap(proxy, &proxy->notify_pio); } @@ -2317,17 +2144,9 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) proxy->notify_pio.type = VIRTIO_PCI_CAP_NOTIFY_CFG; /* subclasses can enforce modern, so do this unconditionally */ - if (!(proxy->flags & VIRTIO_PCI_FLAG_VDPA)) { - memory_region_init(&proxy->modern_bar, OBJECT(proxy), "virtio-pci", - /* PCI BAR regions must be powers of 2 */ - pow2ceil(proxy->notify.offset + proxy->notify.size)); - } else { - proxy->lm.offset = proxy->notify.offset + proxy->notify.size; - proxy->lm.size = 0x20 + VIRTIO_QUEUE_MAX * 4; - memory_region_init(&proxy->modern_bar, OBJECT(proxy), "virtio-pci", - /* PCI BAR regions must be powers of 2 */ - pow2ceil(proxy->lm.offset + proxy->lm.size)); - } + memory_region_init(&proxy->modern_bar, OBJECT(proxy), "virtio-pci", + /* PCI BAR regions must be powers of 2 */ + pow2ceil(proxy->notify.offset + proxy->notify.size)); if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) { proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; @@ -2482,8 +2301,6 @@ static Property virtio_pci_properties[] = { VIRTIO_PCI_FLAG_INIT_FLR_BIT, true), DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_AER_BIT, false), - DEFINE_PROP_BIT("vdpa", VirtIOPCIProxy, flags, - VIRTIO_PCI_FLAG_VDPA_BIT, false), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index fb6b4ccd83..d229755eae 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -3368,18 +3368,6 @@ static uint16_t virtio_queue_split_get_last_avail_idx(VirtIODevice *vdev, return vdev->vq[n].last_avail_idx; } -static uint32_t virtio_queue_split_get_vring_states(VirtIODevice *vdev, - int n) -{ - struct VirtQueue *vq = &vdev->vq[n]; - uint16_t avail, used; - - avail = vq->last_avail_idx; - used = vq->used_idx; - - return avail | (uint32_t)used << 16; -} - unsigned int virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n) { if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { @@ -3389,33 +3377,6 @@ unsigned int virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n) } } -unsigned int virtio_queue_get_vring_states(VirtIODevice *vdev, int n) -{ - if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { - return -1; - } else { - return virtio_queue_split_get_vring_states(vdev, n); - } -} - -static void virtio_queue_split_set_vring_states(VirtIODevice *vdev, - int n, uint32_t idx) -{ - struct VirtQueue *vq = &vdev->vq[n]; - vq->last_avail_idx = (uint16_t)(idx & 0xffff); - vq->shadow_avail_idx = (uint16_t)(idx & 0xffff); - vq->used_idx = (uint16_t)(idx >> 16); -} - -void virtio_queue_set_vring_states(VirtIODevice *vdev, int n, uint32_t idx) -{ - if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { - return; - } else { - virtio_queue_split_set_vring_states(vdev, n, idx); - } -} - static void virtio_queue_packed_set_last_avail_idx(VirtIODevice *vdev, int n, unsigned int idx) { diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h index 4d57a9c751..59d88018c1 100644 --- a/include/hw/virtio/virtio-pci.h +++ b/include/hw/virtio/virtio-pci.h @@ -43,7 +43,6 @@ enum { VIRTIO_PCI_FLAG_INIT_FLR_BIT, VIRTIO_PCI_FLAG_AER_BIT, VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, - VIRTIO_PCI_FLAG_VDPA_BIT, }; /* Need to activate work-arounds for buggy guests at vmstate load. */ @@ -90,9 +89,6 @@ enum { #define VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED \ (1 << VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT) -/* VDPA supported flags */ -#define VIRTIO_PCI_FLAG_VDPA (1 << VIRTIO_PCI_FLAG_VDPA_BIT) - typedef struct { MSIMessage msg; int virq; @@ -144,7 +140,6 @@ struct VirtIOPCIProxy { }; VirtIOPCIRegion regs[5]; }; - VirtIOPCIRegion lm; MemoryRegion modern_bar; MemoryRegion io_bar; uint32_t legacy_io_bar_idx; diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index b3c74a1bca..c8f72850bc 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -35,9 +35,6 @@ (0x1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) | \ (0x1ULL << VIRTIO_F_ANY_LAYOUT)) -#define LM_DISABLE 0x00 -#define LM_ENABLE 0x01 - struct VirtQueue; static inline hwaddr vring_align(hwaddr addr, @@ -98,11 +95,6 @@ enum virtio_device_endian { VIRTIO_DEVICE_ENDIAN_BIG, }; -typedef struct BitmapMemoryRegionCaches { - struct rcu_head rcu; - MemoryRegionCache bitmap; -} BitmapMemoryRegionCaches; - /** * struct VirtIODevice - common VirtIO structure * @name: name of the device @@ -136,14 +128,6 @@ struct VirtIODevice uint32_t generation; int nvectors; VirtQueue *vq; - uint8_t lm_logging_ctrl; - uint32_t lm_base_addr_low; - uint32_t lm_base_addr_high; - uint32_t lm_end_addr_low; - uint32_t lm_end_addr_high; - - BitmapMemoryRegionCaches *caches; - MemoryListener listener; uint16_t device_id; /* @vm_running: current VM running state via virtio_vmstate_change() */ @@ -395,11 +379,8 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n); hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n); hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n); unsigned int virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n); -unsigned int virtio_queue_get_vring_states(VirtIODevice *vdev, int n); void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, unsigned int idx); -void virtio_queue_set_vring_states(VirtIODevice *vdev, int n, - unsigned int idx); void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n); void virtio_queue_invalidate_signalled_used(VirtIODevice *vdev, int n); void virtio_queue_update_used_idx(VirtIODevice *vdev, int n); diff --git a/include/standard-headers/linux/virtio_pci.h b/include/standard-headers/linux/virtio_pci.h index 86733278ba..3e2bc2c97e 100644 --- a/include/standard-headers/linux/virtio_pci.h +++ b/include/standard-headers/linux/virtio_pci.h @@ -221,13 +221,6 @@ struct virtio_pci_cfg_cap { #define VIRTIO_PCI_COMMON_ADM_Q_IDX 60 #define VIRTIO_PCI_COMMON_ADM_Q_NUM 62 -#define LM_LOGGING_CTRL 0 -#define LM_BASE_ADDR_LOW 4 -#define LM_BASE_ADDR_HIGH 8 -#define LM_END_ADDR_LOW 12 -#define LM_END_ADDR_HIGH 16 -#define LM_VRING_STATE_OFFSET 0x20 - #endif /* VIRTIO_PCI_NO_MODERN */ /* Admin command status. */ From a45f09935c88ae352a5ec120418a8b2b36ec1daa Mon Sep 17 00:00:00 2001 From: Zheyu Ma Date: Fri, 22 Mar 2024 12:08:27 +0100 Subject: [PATCH 2/7] virtio-snd: Enhance error handling for invalid transfers This patch improves error handling in virtio_snd_handle_tx_xfer() and virtio_snd_handle_rx_xfer() in the VirtIO sound driver. Previously, 'goto' statements were used for error paths, leading to unnecessary processing and potential null pointer dereferences. Now, 'continue' is used to skip the rest of the current loop iteration for errors such as message size discrepancies or null streams, reducing crash risks. ASAN log illustrating the issue addressed: ERROR: AddressSanitizer: SEGV on unknown address 0x0000000000b4 #0 0x57cea39967b8 in qemu_mutex_lock_impl qemu/util/qemu-thread-posix.c:92:5 #1 0x57cea128c462 in qemu_mutex_lock qemu/include/qemu/thread.h:122:5 #2 0x57cea128d72f in qemu_lockable_lock qemu/include/qemu/lockable.h:95:5 #3 0x57cea128c294 in qemu_lockable_auto_lock qemu/include/qemu/lockable.h:105:5 #4 0x57cea1285eb2 in virtio_snd_handle_rx_xfer qemu/hw/audio/virtio-snd.c:1026:9 #5 0x57cea2caebbc in virtio_queue_notify_vq qemu/hw/virtio/virtio.c:2268:9 #6 0x57cea2cae412 in virtio_queue_host_notifier_read qemu/hw/virtio/virtio.c:3671:9 #7 0x57cea39822f1 in aio_dispatch_handler qemu/util/aio-posix.c:372:9 #8 0x57cea3979385 in aio_dispatch_handlers qemu/util/aio-posix.c:414:20 #9 0x57cea3978eb1 in aio_dispatch qemu/util/aio-posix.c:424:5 #10 0x57cea3a1eede in aio_ctx_dispatch qemu/util/async.c:360:5 Signed-off-by: Zheyu Ma Reviewed-by: Manos Pitsidianakis Message-Id: <20240322110827.568412-1-zheyuma97@gmail.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/audio/virtio-snd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c index e604d8f30c..30493f06a8 100644 --- a/hw/audio/virtio-snd.c +++ b/hw/audio/virtio-snd.c @@ -913,13 +913,13 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq) &hdr, sizeof(virtio_snd_pcm_xfer)); if (msg_sz != sizeof(virtio_snd_pcm_xfer)) { - goto tx_err; + continue; } stream_id = le32_to_cpu(hdr.stream_id); if (stream_id >= s->snd_conf.streams || s->pcm->streams[stream_id] == NULL) { - goto tx_err; + continue; } stream = s->pcm->streams[stream_id]; @@ -995,13 +995,13 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq) &hdr, sizeof(virtio_snd_pcm_xfer)); if (msg_sz != sizeof(virtio_snd_pcm_xfer)) { - goto rx_err; + continue; } stream_id = le32_to_cpu(hdr.stream_id); if (stream_id >= s->snd_conf.streams || !s->pcm->streams[stream_id]) { - goto rx_err; + continue; } stream = s->pcm->streams[stream_id]; From 731655f87f319fd06f27282c6cafbc2467ac8045 Mon Sep 17 00:00:00 2001 From: Manos Pitsidianakis Date: Sun, 24 Mar 2024 12:04:59 +0200 Subject: [PATCH 3/7] virtio-snd: rewrite invalid tx/rx message handling The current handling of invalid virtqueue elements inside the TX/RX virt queue handlers is wrong. They are added in a per-stream invalid queue to be processed after the handler is done examining each message, but the invalid message might not be specifying any stream_id; which means it's invalid to add it to any stream->invalid queue since stream could be NULL at this point. This commit moves the invalid queue to the VirtIOSound struct which guarantees there will always be a valid temporary place to store them inside the tx/rx handlers. The queue will be emptied before the handler returns, so the queue must be empty at any other point of the device's lifetime. Signed-off-by: Manos Pitsidianakis Message-Id: Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/audio/virtio-snd.c | 137 +++++++++++++++------------------- include/hw/audio/virtio-snd.h | 16 +++- 2 files changed, 77 insertions(+), 76 deletions(-) diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c index 30493f06a8..90d9a2796e 100644 --- a/hw/audio/virtio-snd.c +++ b/hw/audio/virtio-snd.c @@ -456,7 +456,6 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id) stream->s = s; qemu_mutex_init(&stream->queue_mutex); QSIMPLEQ_INIT(&stream->queue); - QSIMPLEQ_INIT(&stream->invalid); /* * stream_id >= s->snd_conf.streams was checked before so this is @@ -611,9 +610,6 @@ static size_t virtio_snd_pcm_get_io_msgs_count(VirtIOSoundPCMStream *stream) QSIMPLEQ_FOREACH_SAFE(buffer, &stream->queue, entry, next) { count += 1; } - QSIMPLEQ_FOREACH_SAFE(buffer, &stream->invalid, entry, next) { - count += 1; - } } return count; } @@ -831,47 +827,36 @@ static void virtio_snd_handle_event(VirtIODevice *vdev, VirtQueue *vq) trace_virtio_snd_handle_event(); } +/* + * Must only be called if vsnd->invalid is not empty. + */ static inline void empty_invalid_queue(VirtIODevice *vdev, VirtQueue *vq) { VirtIOSoundPCMBuffer *buffer = NULL; - VirtIOSoundPCMStream *stream = NULL; virtio_snd_pcm_status resp = { 0 }; VirtIOSound *vsnd = VIRTIO_SND(vdev); - bool any = false; - for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) { - stream = vsnd->pcm->streams[i]; - if (stream) { - any = false; - WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) { - while (!QSIMPLEQ_EMPTY(&stream->invalid)) { - buffer = QSIMPLEQ_FIRST(&stream->invalid); - if (buffer->vq != vq) { - break; - } - any = true; - resp.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG); - iov_from_buf(buffer->elem->in_sg, - buffer->elem->in_num, - 0, - &resp, - sizeof(virtio_snd_pcm_status)); - virtqueue_push(vq, - buffer->elem, - sizeof(virtio_snd_pcm_status)); - QSIMPLEQ_REMOVE_HEAD(&stream->invalid, entry); - virtio_snd_pcm_buffer_free(buffer); - } - if (any) { - /* - * Notify vq about virtio_snd_pcm_status responses. - * Buffer responses must be notified separately later. - */ - virtio_notify(vdev, vq); - } - } - } + g_assert(!QSIMPLEQ_EMPTY(&vsnd->invalid)); + + while (!QSIMPLEQ_EMPTY(&vsnd->invalid)) { + buffer = QSIMPLEQ_FIRST(&vsnd->invalid); + /* If buffer->vq != vq, our logic is fundamentally wrong, so bail out */ + g_assert(buffer->vq == vq); + + resp.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG); + iov_from_buf(buffer->elem->in_sg, + buffer->elem->in_num, + 0, + &resp, + sizeof(virtio_snd_pcm_status)); + virtqueue_push(vq, + buffer->elem, + sizeof(virtio_snd_pcm_status)); + QSIMPLEQ_REMOVE_HEAD(&vsnd->invalid, entry); + virtio_snd_pcm_buffer_free(buffer); } + /* Notify vq about virtio_snd_pcm_status responses. */ + virtio_notify(vdev, vq); } /* @@ -883,15 +868,14 @@ static inline void empty_invalid_queue(VirtIODevice *vdev, VirtQueue *vq) */ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq) { - VirtIOSound *s = VIRTIO_SND(vdev); - VirtIOSoundPCMStream *stream = NULL; + VirtIOSound *vsnd = VIRTIO_SND(vdev); VirtIOSoundPCMBuffer *buffer; VirtQueueElement *elem; size_t msg_sz, size; virtio_snd_pcm_xfer hdr; uint32_t stream_id; /* - * If any of the I/O messages are invalid, put them in stream->invalid and + * If any of the I/O messages are invalid, put them in vsnd->invalid and * return them after the for loop. */ bool must_empty_invalid_queue = false; @@ -901,7 +885,7 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq) } trace_virtio_snd_handle_tx_xfer(); - for (;;) { + for (VirtIOSoundPCMStream *stream = NULL;; stream = NULL) { elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); if (!elem) { break; @@ -913,16 +897,16 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq) &hdr, sizeof(virtio_snd_pcm_xfer)); if (msg_sz != sizeof(virtio_snd_pcm_xfer)) { - continue; + goto tx_err; } stream_id = le32_to_cpu(hdr.stream_id); - if (stream_id >= s->snd_conf.streams - || s->pcm->streams[stream_id] == NULL) { - continue; + if (stream_id >= vsnd->snd_conf.streams + || vsnd->pcm->streams[stream_id] == NULL) { + goto tx_err; } - stream = s->pcm->streams[stream_id]; + stream = vsnd->pcm->streams[stream_id]; if (stream->info.direction != VIRTIO_SND_D_OUTPUT) { goto tx_err; } @@ -942,13 +926,11 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq) continue; tx_err: - WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) { - must_empty_invalid_queue = true; - buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer)); - buffer->elem = elem; - buffer->vq = vq; - QSIMPLEQ_INSERT_TAIL(&stream->invalid, buffer, entry); - } + must_empty_invalid_queue = true; + buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer)); + buffer->elem = elem; + buffer->vq = vq; + QSIMPLEQ_INSERT_TAIL(&vsnd->invalid, buffer, entry); } if (must_empty_invalid_queue) { @@ -965,15 +947,14 @@ tx_err: */ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq) { - VirtIOSound *s = VIRTIO_SND(vdev); - VirtIOSoundPCMStream *stream = NULL; + VirtIOSound *vsnd = VIRTIO_SND(vdev); VirtIOSoundPCMBuffer *buffer; VirtQueueElement *elem; size_t msg_sz, size; virtio_snd_pcm_xfer hdr; uint32_t stream_id; /* - * if any of the I/O messages are invalid, put them in stream->invalid and + * if any of the I/O messages are invalid, put them in vsnd->invalid and * return them after the for loop. */ bool must_empty_invalid_queue = false; @@ -983,7 +964,7 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq) } trace_virtio_snd_handle_rx_xfer(); - for (;;) { + for (VirtIOSoundPCMStream *stream = NULL;; stream = NULL) { elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); if (!elem) { break; @@ -995,16 +976,16 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq) &hdr, sizeof(virtio_snd_pcm_xfer)); if (msg_sz != sizeof(virtio_snd_pcm_xfer)) { - continue; + goto rx_err; } stream_id = le32_to_cpu(hdr.stream_id); - if (stream_id >= s->snd_conf.streams - || !s->pcm->streams[stream_id]) { - continue; + if (stream_id >= vsnd->snd_conf.streams + || !vsnd->pcm->streams[stream_id]) { + goto rx_err; } - stream = s->pcm->streams[stream_id]; + stream = vsnd->pcm->streams[stream_id]; if (stream == NULL || stream->info.direction != VIRTIO_SND_D_INPUT) { goto rx_err; } @@ -1021,13 +1002,11 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq) continue; rx_err: - WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) { - must_empty_invalid_queue = true; - buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer)); - buffer->elem = elem; - buffer->vq = vq; - QSIMPLEQ_INSERT_TAIL(&stream->invalid, buffer, entry); - } + must_empty_invalid_queue = true; + buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer)); + buffer->elem = elem; + buffer->vq = vq; + QSIMPLEQ_INSERT_TAIL(&vsnd->invalid, buffer, entry); } if (must_empty_invalid_queue) { @@ -1127,6 +1106,7 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp) virtio_add_queue(vdev, 64, virtio_snd_handle_rx_xfer); qemu_mutex_init(&vsnd->cmdq_mutex); QTAILQ_INIT(&vsnd->cmdq); + QSIMPLEQ_INIT(&vsnd->invalid); for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) { status = virtio_snd_set_pcm_params(vsnd, i, &default_params); @@ -1376,13 +1356,20 @@ static void virtio_snd_unrealize(DeviceState *dev) static void virtio_snd_reset(VirtIODevice *vdev) { - VirtIOSound *s = VIRTIO_SND(vdev); + VirtIOSound *vsnd = VIRTIO_SND(vdev); virtio_snd_ctrl_command *cmd; - WITH_QEMU_LOCK_GUARD(&s->cmdq_mutex) { - while (!QTAILQ_EMPTY(&s->cmdq)) { - cmd = QTAILQ_FIRST(&s->cmdq); - QTAILQ_REMOVE(&s->cmdq, cmd, next); + /* + * Sanity check that the invalid buffer message queue is emptied at the end + * of every virtio_snd_handle_tx_xfer/virtio_snd_handle_rx_xfer call, and + * must be empty otherwise. + */ + g_assert(QSIMPLEQ_EMPTY(&vsnd->invalid)); + + WITH_QEMU_LOCK_GUARD(&vsnd->cmdq_mutex) { + while (!QTAILQ_EMPTY(&vsnd->cmdq)) { + cmd = QTAILQ_FIRST(&vsnd->cmdq); + QTAILQ_REMOVE(&vsnd->cmdq, cmd, next); virtio_snd_ctrl_cmd_free(cmd); } } diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h index 3d79181364..8dafedb276 100644 --- a/include/hw/audio/virtio-snd.h +++ b/include/hw/audio/virtio-snd.h @@ -151,7 +151,6 @@ struct VirtIOSoundPCMStream { QemuMutex queue_mutex; bool active; QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) queue; - QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) invalid; }; /* @@ -223,6 +222,21 @@ struct VirtIOSound { QemuMutex cmdq_mutex; QTAILQ_HEAD(, virtio_snd_ctrl_command) cmdq; bool processing_cmdq; + /* + * Convenience queue to keep track of invalid tx/rx queue messages inside + * the tx/rx callbacks. + * + * In the callbacks as a first step we are emptying the virtqueue to handle + * each message and we cannot add an invalid message back to the queue: we + * would re-process it in subsequent loop iterations. + * + * Instead, we add them to this queue and after finishing examining every + * virtqueue element, we inform the guest for each invalid message. + * + * This queue must be empty at all times except for inside the tx/rx + * callbacks. + */ + QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) invalid; }; struct virtio_snd_ctrl_command { From 2d9a31b3c27311eca1682cb2c076d7a300441960 Mon Sep 17 00:00:00 2001 From: Wafer Date: Sun, 7 Apr 2024 09:54:51 +0800 Subject: [PATCH 4/7] hw/virtio: Fix packed virtqueue flush used_idx MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the event of writing many chains of descriptors, the device must write just the id of the last buffer in the descriptor chain, skip forward the number of descriptors in the chain, and then repeat the operations for the rest of chains. Current QEMU code writes all the buffer ids consecutively, and then skips all the buffers altogether. This is a bug, and can be reproduced with a VirtIONet device with _F_MRG_RXBUB and without _F_INDIRECT_DESC: If a virtio-net device has the VIRTIO_NET_F_MRG_RXBUF feature but not the VIRTIO_RING_F_INDIRECT_DESC feature, 'VirtIONetQueue->rx_vq' will use the merge feature to store data in multiple 'elems'. The 'num_buffers' in the virtio header indicates how many elements are merged. If the value of 'num_buffers' is greater than 1, all the merged elements will be filled into the descriptor ring. The 'idx' of the elements should be the value of 'vq->used_idx' plus 'ndescs'. Fixes: 86044b24e8 ("virtio: basic packed virtqueue support") Acked-by: Eugenio Pérez Signed-off-by: Wafer Message-Id: <20240407015451.5228-2-wafer@jaguarmicro.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/virtio/virtio.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index d229755eae..c5bedca848 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -957,12 +957,20 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count) return; } + /* + * For indirect element's 'ndescs' is 1. + * For all other elemment's 'ndescs' is the + * number of descriptors chained by NEXT (as set in virtqueue_packed_pop). + * So When the 'elem' be filled into the descriptor ring, + * The 'idx' of this 'elem' shall be + * the value of 'vq->used_idx' plus the 'ndescs'. + */ + ndescs += vq->used_elems[0].ndescs; for (i = 1; i < count; i++) { - virtqueue_packed_fill_desc(vq, &vq->used_elems[i], i, false); + virtqueue_packed_fill_desc(vq, &vq->used_elems[i], ndescs, false); ndescs += vq->used_elems[i].ndescs; } virtqueue_packed_fill_desc(vq, &vq->used_elems[0], 0, true); - ndescs += vq->used_elems[0].ndescs; vq->inuse -= ndescs; vq->used_idx += ndescs; From 6ae72f609a21cfc56bf655cd4bcded5d07691ce7 Mon Sep 17 00:00:00 2001 From: lyx634449800 Date: Mon, 8 Apr 2024 10:00:03 +0800 Subject: [PATCH 5/7] vdpa-dev: Fix the issue of device status not updating when configuration interruption is triggered The set_config callback function vhost_vdpa_device_get_config in vdpa-dev does not fetch the current device status from the hardware device, causing the guest os to not receive the latest device status information. The hardware updates the config status of the vdpa device and then notifies the os. The guest os receives an interrupt notification, triggering a get_config access in the kernel, which then enters qemu internally. Ultimately, the vhost_vdpa_device_get_config function of vdpa-dev is called One scenario encountered is when the device needs to bring down the vdpa net device. After modifying the status field of virtio_net_config in the hardware, it sends an interrupt notification. However, the guest os always receives the STATUS field as VIRTIO_NET_S_LINK_UP. Signed-off-by: Yuxue Liu Acked-by: Jason Wang Message-Id: <20240408020003.1979-1-yuxue.liu@jaguarmicro.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/virtio/vdpa-dev.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c index 13e87f06f6..64b96b226c 100644 --- a/hw/virtio/vdpa-dev.c +++ b/hw/virtio/vdpa-dev.c @@ -195,7 +195,14 @@ static void vhost_vdpa_device_get_config(VirtIODevice *vdev, uint8_t *config) { VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev); + int ret; + ret = vhost_dev_get_config(&s->dev, s->config, s->config_size, + NULL); + if (ret < 0) { + error_report("get device config space failed"); + return; + } memcpy(config, s->config, s->config_size); } From f67d296b6ea0e946e4ca13a39c699ca13bd977b6 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 29 Mar 2024 21:37:54 +0300 Subject: [PATCH 6/7] vhost-user-blk: simplify and fix vhost_user_blk_handle_config_change Let's not care about what was changed and update the whole config, reasons: 1. config->geometry should be updated together with capacity, so we fix a bug. 2. Vhost-user protocol doesn't say anything about config change limitation. Silent ignore of changes doesn't seem to be correct. 3. vhost-user-vsock reads the whole config 4. on realize we don't do any checks on retrieved config, so no reason to care here Comment "valid for resize only" exists since introduction the whole hw/block/vhost-user-blk.c in commit 00343e4b54ba0685e9ebe928ec5713b0cf7f1d1c "vhost-user-blk: introduce a new vhost-user-blk host device", seems it was just an extra limitation. Also, let's notify guest unconditionally: 1. So does vhost-user-vsock 2. We are going to reuse the functionality in new cases when we do want to notify the guest unconditionally. So, no reason to create extra branches in the logic. Signed-off-by: Vladimir Sementsov-Ogievskiy Acked-by: Raphael Norwitz Message-Id: <20240329183758.3360733-2-vsementsov@yandex-team.ru> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/block/vhost-user-blk.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index 6a856ad51a..9e6bbc6950 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -91,7 +91,6 @@ static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config) static int vhost_user_blk_handle_config_change(struct vhost_dev *dev) { int ret; - struct virtio_blk_config blkcfg; VirtIODevice *vdev = dev->vdev; VHostUserBlk *s = VHOST_USER_BLK(dev->vdev); Error *local_err = NULL; @@ -100,19 +99,15 @@ static int vhost_user_blk_handle_config_change(struct vhost_dev *dev) return 0; } - ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg, + ret = vhost_dev_get_config(dev, (uint8_t *)&s->blkcfg, vdev->config_len, &local_err); if (ret < 0) { error_report_err(local_err); return ret; } - /* valid for resize only */ - if (blkcfg.capacity != s->blkcfg.capacity) { - s->blkcfg.capacity = blkcfg.capacity; - memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len); - virtio_notify_config(dev->vdev); - } + memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len); + virtio_notify_config(dev->vdev); return 0; } From e1999904a960c33b68fedf26dfb7b8e00abab8f2 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 29 Mar 2024 21:37:55 +0300 Subject: [PATCH 7/7] qdev-monitor: fix error message in find_device_state() This "hotpluggable" here is misleading. Actually we check is object a device or not. Let's drop the word. Suggested-by: Markus Armbruster Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Markus Armbruster Message-Id: <20240329183758.3360733-3-vsementsov@yandex-team.ru> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- system/qdev-monitor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c index c1243891c3..840177d19f 100644 --- a/system/qdev-monitor.c +++ b/system/qdev-monitor.c @@ -891,7 +891,7 @@ static DeviceState *find_device_state(const char *id, Error **errp) dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE); if (!dev) { - error_setg(errp, "%s is not a hotpluggable device", id); + error_setg(errp, "%s is not a device", id); return NULL; }