* [PULL 1/7] Revert "hw/virtio: Add support for VDPA network simulation devices"
2024-04-09 7:32 [PULL 0/7] virtio,pc,pci: bugfixes Michael S. Tsirkin
@ 2024-04-09 7:32 ` Michael S. Tsirkin
2024-04-09 7:32 ` [PULL 2/7] virtio-snd: Enhance error handling for invalid transfers Michael S. Tsirkin
` (6 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2024-04-09 7:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang, Cornelia Huck, Paolo Bonzini
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 <jasowang@redhat.com>
Message-Id: <df6b6b465753e754a19459e8cd61416548f89a42.1712569644.git.mst@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/virtio/virtio-pci.h | 5 -
include/hw/virtio/virtio.h | 19 --
include/standard-headers/linux/virtio_pci.h | 7 -
hw/net/virtio-net.c | 16 --
hw/virtio/virtio-pci.c | 189 +-------------------
hw/virtio/virtio.c | 39 ----
MAINTAINERS | 5 -
docs/system/device-emulation.rst | 1 -
docs/system/devices/vdpa-net.rst | 121 -------------
9 files changed, 3 insertions(+), 399 deletions(-)
delete mode 100644 docs/system/devices/vdpa-net.rst
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. */
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/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 <chenh@yusur.tech>
-S: Maintained
-F: docs/system/devices/vdpa-net.rst
-
virtio-crypto
M: Gonglei <arei.gonglei@huawei.com>
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::
-
- <qemu:commandline xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'>
- <qemu:arg value='-device'/>
- <qemu:arg value='intel-iommu,intremap=on,device-iotlb=on,aw-bits=48'/>
- <qemu:arg value='-netdev'/>
- <qemu:arg value='tap,id=hostnet1,script=no,downscript=no,vhost=off'/>
- <qemu:arg value='-device'/>
- <qemu:arg value='virtio-net-pci,netdev=hostnet1,id=net1,mac=56:4a:b7:4f:4d:a9,bus=pci.6,addr=0x0,iommu_platform=on,ats=on,vdpa=on'/>
- </qemu:commandline>
-
-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.
-
-
-
--
MST
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PULL 2/7] virtio-snd: Enhance error handling for invalid transfers
2024-04-09 7:32 [PULL 0/7] virtio,pc,pci: bugfixes Michael S. Tsirkin
2024-04-09 7:32 ` [PULL 1/7] Revert "hw/virtio: Add support for VDPA network simulation devices" Michael S. Tsirkin
@ 2024-04-09 7:32 ` Michael S. Tsirkin
2024-04-09 7:32 ` [PULL 3/7] virtio-snd: rewrite invalid tx/rx message handling Michael S. Tsirkin
` (5 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2024-04-09 7:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Zheyu Ma, Manos Pitsidianakis, Gerd Hoffmann
From: Zheyu Ma <zheyuma97@gmail.com>
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 <zheyuma97@gmail.com>
Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Message-Id: <20240322110827.568412-1-zheyuma97@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
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];
--
MST
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PULL 3/7] virtio-snd: rewrite invalid tx/rx message handling
2024-04-09 7:32 [PULL 0/7] virtio,pc,pci: bugfixes Michael S. Tsirkin
2024-04-09 7:32 ` [PULL 1/7] Revert "hw/virtio: Add support for VDPA network simulation devices" Michael S. Tsirkin
2024-04-09 7:32 ` [PULL 2/7] virtio-snd: Enhance error handling for invalid transfers Michael S. Tsirkin
@ 2024-04-09 7:32 ` Michael S. Tsirkin
2024-04-09 7:32 ` [PULL 4/7] hw/virtio: Fix packed virtqueue flush used_idx Michael S. Tsirkin
` (4 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2024-04-09 7:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Manos Pitsidianakis, Gerd Hoffmann
From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
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 <manos.pitsidianakis@linaro.org>
Message-Id: <virtio-snd-rewrite-invalid-tx-rx-message-handling-v1.manos.pitsidianakis@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/audio/virtio-snd.h | 16 +++-
hw/audio/virtio-snd.c | 137 +++++++++++++++-------------------
2 files changed, 77 insertions(+), 76 deletions(-)
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 {
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);
}
}
--
MST
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PULL 4/7] hw/virtio: Fix packed virtqueue flush used_idx
2024-04-09 7:32 [PULL 0/7] virtio,pc,pci: bugfixes Michael S. Tsirkin
` (2 preceding siblings ...)
2024-04-09 7:32 ` [PULL 3/7] virtio-snd: rewrite invalid tx/rx message handling Michael S. Tsirkin
@ 2024-04-09 7:32 ` Michael S. Tsirkin
2024-04-09 17:40 ` Michael Tokarev
2024-04-09 7:32 ` [PULL 5/7] vdpa-dev: Fix the issue of device status not updating when configuration interruption is triggered Michael S. Tsirkin
` (3 subsequent siblings)
7 siblings, 1 reply; 13+ messages in thread
From: Michael S. Tsirkin @ 2024-04-09 7:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Wafer, Eugenio Pérez
From: Wafer <wafer@jaguarmicro.com>
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 <eperezma@redhat.com>
Signed-off-by: Wafer <wafer@jaguarmicro.com>
Message-Id: <20240407015451.5228-2-wafer@jaguarmicro.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
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;
--
MST
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PULL 4/7] hw/virtio: Fix packed virtqueue flush used_idx
2024-04-09 7:32 ` [PULL 4/7] hw/virtio: Fix packed virtqueue flush used_idx Michael S. Tsirkin
@ 2024-04-09 17:40 ` Michael Tokarev
2024-04-10 5:31 ` Eugenio Perez Martin
0 siblings, 1 reply; 13+ messages in thread
From: Michael Tokarev @ 2024-04-09 17:40 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel; +Cc: Wafer, Eugenio Pérez
09.04.2024 10:32, Michael S. Tsirkin wrote:
> From: Wafer <wafer@jaguarmicro.com>
>
> 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 <eperezma@redhat.com>
> Signed-off-by: Wafer <wafer@jaguarmicro.com>
> Message-Id: <20240407015451.5228-2-wafer@jaguarmicro.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/virtio/virtio.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
Is this a -stable material?
Thanks,
/mjt
> 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;
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PULL 4/7] hw/virtio: Fix packed virtqueue flush used_idx
2024-04-09 17:40 ` Michael Tokarev
@ 2024-04-10 5:31 ` Eugenio Perez Martin
0 siblings, 0 replies; 13+ messages in thread
From: Eugenio Perez Martin @ 2024-04-10 5:31 UTC (permalink / raw)
To: Michael Tokarev; +Cc: Michael S. Tsirkin, qemu-devel, Wafer
On Tue, Apr 9, 2024 at 7:40 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 09.04.2024 10:32, Michael S. Tsirkin wrote:
> > From: Wafer <wafer@jaguarmicro.com>
> >
> > 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 <eperezma@redhat.com>
> > Signed-off-by: Wafer <wafer@jaguarmicro.com>
> > Message-Id: <20240407015451.5228-2-wafer@jaguarmicro.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > hw/virtio/virtio.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
>
> Is this a -stable material?
>
Hi Michael,
Yes it is. It should be easy to backport but let me know if you need any help.
Thanks!
> Thanks,
>
> /mjt
>
> > 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;
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PULL 5/7] vdpa-dev: Fix the issue of device status not updating when configuration interruption is triggered
2024-04-09 7:32 [PULL 0/7] virtio,pc,pci: bugfixes Michael S. Tsirkin
` (3 preceding siblings ...)
2024-04-09 7:32 ` [PULL 4/7] hw/virtio: Fix packed virtqueue flush used_idx Michael S. Tsirkin
@ 2024-04-09 7:32 ` Michael S. Tsirkin
2024-04-09 17:43 ` Michael Tokarev
2024-04-09 7:32 ` [PULL 6/7] vhost-user-blk: simplify and fix vhost_user_blk_handle_config_change Michael S. Tsirkin
` (2 subsequent siblings)
7 siblings, 1 reply; 13+ messages in thread
From: Michael S. Tsirkin @ 2024-04-09 7:32 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, lyx634449800, Jason Wang, Eugenio Pérez,
Kevin Wolf, Richard Henderson
From: lyx634449800 <yuxue.liu@jaguarmicro.com>
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 <yuxue.liu@jaguarmicro.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20240408020003.1979-1-yuxue.liu@jaguarmicro.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
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);
}
--
MST
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PULL 5/7] vdpa-dev: Fix the issue of device status not updating when configuration interruption is triggered
2024-04-09 7:32 ` [PULL 5/7] vdpa-dev: Fix the issue of device status not updating when configuration interruption is triggered Michael S. Tsirkin
@ 2024-04-09 17:43 ` Michael Tokarev
2024-04-09 18:06 ` Michael Tokarev
0 siblings, 1 reply; 13+ messages in thread
From: Michael Tokarev @ 2024-04-09 17:43 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel
Cc: lyx634449800, Jason Wang, Eugenio Pérez, Kevin Wolf,
Richard Henderson
09.04.2024 10:32, Michael S. Tsirkin пишет:
> From: lyx634449800 <yuxue.liu@jaguarmicro.com>
>
> 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 <yuxue.liu@jaguarmicro.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
> Message-Id: <20240408020003.1979-1-yuxue.liu@jaguarmicro.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Ditto, is this a -stable material (for 8.2)?
Thanks,
/mjt
> 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);
> }
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PULL 5/7] vdpa-dev: Fix the issue of device status not updating when configuration interruption is triggered
2024-04-09 17:43 ` Michael Tokarev
@ 2024-04-09 18:06 ` Michael Tokarev
0 siblings, 0 replies; 13+ messages in thread
From: Michael Tokarev @ 2024-04-09 18:06 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel
Cc: lyx634449800, Jason Wang, Eugenio Pérez, Kevin Wolf,
Richard Henderson
09.04.2024 20:43, Michael Tokarev:
> 09.04.2024 10:32, Michael S. Tsirkin:
>> From: lyx634449800 <yuxue.liu@jaguarmicro.com>
>>
>> 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 <yuxue.liu@jaguarmicro.com>
>> Acked-by: Jason Wang <jasowang@redhat.com>
>> Message-Id: <20240408020003.1979-1-yuxue.liu@jaguarmicro.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Ditto, is this a -stable material (for 8.2)?
Yes it is, it has already been Cc'ed to qemu-stable. Please excuse me for
the noise.
/mjt
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PULL 6/7] vhost-user-blk: simplify and fix vhost_user_blk_handle_config_change
2024-04-09 7:32 [PULL 0/7] virtio,pc,pci: bugfixes Michael S. Tsirkin
` (4 preceding siblings ...)
2024-04-09 7:32 ` [PULL 5/7] vdpa-dev: Fix the issue of device status not updating when configuration interruption is triggered Michael S. Tsirkin
@ 2024-04-09 7:32 ` Michael S. Tsirkin
2024-04-09 7:32 ` [PULL 7/7] qdev-monitor: fix error message in find_device_state() Michael S. Tsirkin
2024-04-09 11:47 ` [PULL 0/7] virtio,pc,pci: bugfixes Peter Maydell
7 siblings, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2024-04-09 7:32 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Vladimir Sementsov-Ogievskiy, Raphael Norwitz,
Raphael Norwitz, Kevin Wolf, Hanna Reitz, qemu-block
From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
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 <vsementsov@yandex-team.ru>
Acked-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Message-Id: <20240329183758.3360733-2-vsementsov@yandex-team.ru>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
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;
}
--
MST
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PULL 7/7] qdev-monitor: fix error message in find_device_state()
2024-04-09 7:32 [PULL 0/7] virtio,pc,pci: bugfixes Michael S. Tsirkin
` (5 preceding siblings ...)
2024-04-09 7:32 ` [PULL 6/7] vhost-user-blk: simplify and fix vhost_user_blk_handle_config_change Michael S. Tsirkin
@ 2024-04-09 7:32 ` Michael S. Tsirkin
2024-04-09 11:47 ` [PULL 0/7] virtio,pc,pci: bugfixes Peter Maydell
7 siblings, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2024-04-09 7:32 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Vladimir Sementsov-Ogievskiy, Markus Armbruster,
Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost
From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
This "hotpluggable" here is misleading. Actually we check is object a
device or not. Let's drop the word.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20240329183758.3360733-3-vsementsov@yandex-team.ru>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
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;
}
--
MST
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PULL 0/7] virtio,pc,pci: bugfixes
2024-04-09 7:32 [PULL 0/7] virtio,pc,pci: bugfixes Michael S. Tsirkin
` (6 preceding siblings ...)
2024-04-09 7:32 ` [PULL 7/7] qdev-monitor: fix error message in find_device_state() Michael S. Tsirkin
@ 2024-04-09 11:47 ` Peter Maydell
7 siblings, 0 replies; 13+ messages in thread
From: Peter Maydell @ 2024-04-09 11:47 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Tue, 9 Apr 2024 at 08:32, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> The following changes since commit ce64e6224affb8b4e4b019f76d2950270b391af5:
>
> Merge tag 'qemu-sparc-20240404' of https://github.com/mcayland/qemu into staging (2024-04-04 15:28:06 +0100)
>
> are available in the Git repository at:
>
> https://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to e1999904a960c33b68fedf26dfb7b8e00abab8f2:
>
> qdev-monitor: fix error message in find_device_state() (2024-04-09 02:31:33 -0400)
>
> ----------------------------------------------------------------
> virtio,pc,pci: bugfixes
>
> Tiny fixes: important but mostly obvious ones. Revert VDPA network sim
> for this release as there are questions around it's maintainatiblity.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/9.0
for any user-visible changes.
-- PMM
^ permalink raw reply [flat|nested] 13+ messages in thread