Re: [PATCH net v4] virtio_net: Do not send RSS key if it is not supported
On Wed, 3 Apr 2024 08:43:12 -0700, Breno Leitao wrote:
> There is a bug when setting the RSS options in virtio_net that can break
> the whole machine, getting the kernel into an infinite loop.
>
> Running the following command in any QEMU virtual machine with virtionet
> will reproduce this problem:
>
> # ethtool -X eth0 hfunc toeplitz
>
> This is how the problem happens:
>
> 1) ethtool_set_rxfh() calls virtnet_set_rxfh()
>
> 2) virtnet_set_rxfh() calls virtnet_commit_rss_command()
>
> 3) virtnet_commit_rss_command() populates 4 entries for the rss
> scatter-gather
>
> 4) Since the command above does not have a key, then the last
> scatter-gatter entry will be zeroed, since rss_key_size == 0.
> sg_buf_size = vi->rss_key_size;
>
> 5) This buffer is passed to qemu, but qemu is not happy with a buffer
> with zero length, and do the following in virtqueue_map_desc() (QEMU
> function):
>
> if (!sz) {
> virtio_error(vdev, "virtio: zero sized buffers are not allowed");
>
> 6) virtio_error() (also QEMU function) set the device as broken
>
> vdev->broken = true;
>
> 7) Qemu bails out, and do not repond this crazy kernel.
>
> 8) The kernel is waiting for the response to come back (function
> virtnet_send_command())
>
> 9) The kernel is waiting doing the following :
>
> while (!virtqueue_get_buf(vi->cvq, ) &&
>!virtqueue_is_broken(vi->cvq))
> cpu_relax();
>
> 10) None of the following functions above is true, thus, the kernel
> loops here forever. Keeping in mind that virtqueue_is_broken() does
> not look at the qemu `vdev->broken`, so, it never realizes that the
> vitio is broken at QEMU side.
>
> Fix it by not sending RSS commands if the feature is not available in
> the device.
>
> Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
> Cc: sta...@vger.kernel.org
> Cc: qemu-devel@nongnu.org
> Signed-off-by: Breno Leitao
> Reviewed-by: Heng Qi
Reviewed-by: Xuan Zhuo
> ---
> Changelog:
>
> V2:
> * Moved from creating a valid packet, by rejecting the request
> completely.
> V3:
> * Got some good feedback from and Xuan Zhuo and Heng Qi, and reworked
> the rejection path.
> V4:
> * Added a comment in an "if" clause, as suggested by Michael S. Tsirkin.
>
> ---
> drivers/net/virtio_net.c | 26 ++
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index c22d1118a133..115c3c5414f2 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3807,6 +3807,7 @@ static int virtnet_set_rxfh(struct net_device *dev,
> struct netlink_ext_ack *extack)
> {
> struct virtnet_info *vi = netdev_priv(dev);
> + bool update = false;
> int i;
>
> if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE &&
> @@ -3814,13 +3815,28 @@ static int virtnet_set_rxfh(struct net_device *dev,
> return -EOPNOTSUPP;
>
> if (rxfh->indir) {
> + if (!vi->has_rss)
> + return -EOPNOTSUPP;
> +
> for (i = 0; i < vi->rss_indir_table_size; ++i)
> vi->ctrl->rss.indirection_table[i] = rxfh->indir[i];
> + update = true;
> }
> - if (rxfh->key)
> +
> + if (rxfh->key) {
> + /* If either _F_HASH_REPORT or _F_RSS are negotiated, the
> + * device provides hash calculation capabilities, that is,
> + * hash_key is configured.
> + */
> + if (!vi->has_rss && !vi->has_rss_hash_report)
> + return -EOPNOTSUPP;
> +
> memcpy(vi->ctrl->rss.key, rxfh->key, vi->rss_key_size);
> + update = true;
> + }
>
> - virtnet_commit_rss_command(vi);
> + if (update)
> + virtnet_commit_rss_command(vi);
>
> return 0;
> }
> @@ -4729,13 +4745,15 @@ static int virtnet_probe(struct virtio_device *vdev)
> if (virtio_has_feature(vdev, VIRTIO_NET_F_HASH_REPORT))
> vi->has_rss_hash_report = true;
>
> - if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS))
> + if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS)) {
> vi->has_rss = true;
>
> - if (vi->has_rss || vi->has_rss_hash_report) {
> vi->rss_indir_table_size =
> virtio_cread16(vdev, offsetof(struct virtio_net_config,
> rss_max_indirection_table_length));
> + }
> +
> + if (vi->has_rss || vi->has_rss_hash_report) {
> vi->rss_key_size =
> virtio_cread8(vdev, offsetof(struct virtio_net_config,
> rss_max_key_size));
>
> --
> 2.43.0
>
Re: [PATCH] virtio_net: Do not send RSS key if it is not supported
On Mon, 25 Mar 2024 04:26:08 -0700, Breno Leitao wrote:
> Hello Xuan,
>
> On Mon, Mar 25, 2024 at 01:57:53PM +0800, Xuan Zhuo wrote:
> > On Fri, 22 Mar 2024 03:21:21 -0700, Breno Leitao wrote:
> > > Hello Xuan,
> > >
> > > On Fri, Mar 22, 2024 at 10:00:22AM +0800, Xuan Zhuo wrote:
> > > > On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao
> > > > wrote:
> > >
> > > > > 4) Since the command above does not have a key, then the last
> > > > >scatter-gatter entry will be zeroed, since rss_key_size == 0.
> > > > > sg_buf_size = vi->rss_key_size;
> > > >
> > > >
> > > >
> > > > if (vi->has_rss || vi->has_rss_hash_report) {
> > > > vi->rss_indir_table_size =
> > > > virtio_cread16(vdev, offsetof(struct
> > > > virtio_net_config,
> > > > rss_max_indirection_table_length));
> > > > vi->rss_key_size =
> > > > virtio_cread8(vdev, offsetof(struct
> > > > virtio_net_config, rss_max_key_size));
> > > >
> > > > vi->rss_hash_types_supported =
> > > > virtio_cread32(vdev, offsetof(struct
> > > > virtio_net_config, supported_hash_types));
> > > > vi->rss_hash_types_supported &=
> > > > ~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX |
> > > > VIRTIO_NET_RSS_HASH_TYPE_TCP_EX |
> > > > VIRTIO_NET_RSS_HASH_TYPE_UDP_EX);
> > > >
> > > > dev->hw_features |= NETIF_F_RXHASH;
> > > > }
> > > >
> > > >
> > > > vi->rss_key_size is initiated here, I wonder if there is something
> > > > wrong?
> > >
> > > Not really, the code above is never executed (in my machines). This is
> > > because `vi->has_rss` and `vi->has_rss_hash_report` are both unset.
> > >
> > > Looking further, vdev does not have the VIRTIO_NET_F_RSS and
> > > VIRTIO_NET_F_HASH_REPORT features.
> > >
> > > Also, when I run `ethtool -x`, I got:
> > >
> > > # ethtool -x eth0
> > > RX flow hash indirection table for eth0 with 1 RX ring(s):
> > > Operation not supported
> > > RSS hash key:
> > > Operation not supported
> > > RSS hash function:
> > > toeplitz: on
> > > xor: off
> > > crc32: off
> >
> >
> > The spec saies:
> > Note that if the device offers VIRTIO_NET_F_HASH_REPORT, even if it
> > supports only one pair of virtqueues, it MUST support at least one of
> > commands of VIRTIO_NET_CTRL_MQ class to configure reported hash
> > parameters:
> >
> > If the device offers VIRTIO_NET_F_RSS, it MUST support
> > VIRTIO_NET_CTRL_MQ_RSS_CONFIG command per 5.1.6.5.7.1.
> >
> > Otherwise the device MUST support VIRTIO_NET_CTRL_MQ_HASH_CONFIG command
> > per 5.1.6.5.6.4.
> >
> >
> > So if we have not anyone of `vi->has_rss` and `vi->has_rss_hash_report`,
> > we should return from virtnet_set_rxfh directly.
>
> Makes sense. Although it is not clear to me how vi->has_rss_hash_report
> is related here, but, I am convinced that we shouldn't do any RSS
> operation if the device doesn't have the RSS feature, i.e, vi->has_rss
> is false.
>
> That said, I am thinking about something like this. How does it sound?
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 5a7700b103f8..8c1ad7361cf2 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3780,6 +3780,9 @@ static int virtnet_set_rxfh(struct net_device
> *dev,
> struct virtnet_info *vi = netdev_priv(dev);
> int i;
>
> + if (!vi->has_rss)
> + return -EOPNOTSUPP;
> +
Should we check has_rss_hash_report?
@Heng Qi
Could you help us?
Thanks.
> if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE &&
> rxfh->hfunc != ETH_RSS_HASH_TOP)
> return -EOPNOTSUPP;
>
> Thanks!
Re: [PATCH] virtio_net: Do not send RSS key if it is not supported
On Fri, 22 Mar 2024 03:21:21 -0700, Breno Leitao wrote:
> Hello Xuan,
>
> On Fri, Mar 22, 2024 at 10:00:22AM +0800, Xuan Zhuo wrote:
> > On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao wrote:
>
> > > 4) Since the command above does not have a key, then the last
> > >scatter-gatter entry will be zeroed, since rss_key_size == 0.
> > > sg_buf_size = vi->rss_key_size;
> >
> >
> >
> > if (vi->has_rss || vi->has_rss_hash_report) {
> > vi->rss_indir_table_size =
> > virtio_cread16(vdev, offsetof(struct virtio_net_config,
> > rss_max_indirection_table_length));
> > vi->rss_key_size =
> > virtio_cread8(vdev, offsetof(struct virtio_net_config,
> > rss_max_key_size));
> >
> > vi->rss_hash_types_supported =
> > virtio_cread32(vdev, offsetof(struct virtio_net_config,
> > supported_hash_types));
> > vi->rss_hash_types_supported &=
> > ~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX |
> > VIRTIO_NET_RSS_HASH_TYPE_TCP_EX |
> > VIRTIO_NET_RSS_HASH_TYPE_UDP_EX);
> >
> > dev->hw_features |= NETIF_F_RXHASH;
> > }
> >
> >
> > vi->rss_key_size is initiated here, I wonder if there is something wrong?
>
> Not really, the code above is never executed (in my machines). This is
> because `vi->has_rss` and `vi->has_rss_hash_report` are both unset.
>
> Looking further, vdev does not have the VIRTIO_NET_F_RSS and
> VIRTIO_NET_F_HASH_REPORT features.
>
> Also, when I run `ethtool -x`, I got:
>
> # ethtool -x eth0
> RX flow hash indirection table for eth0 with 1 RX ring(s):
> Operation not supported
> RSS hash key:
> Operation not supported
> RSS hash function:
> toeplitz: on
> xor: off
> crc32: off
The spec saies:
Note that if the device offers VIRTIO_NET_F_HASH_REPORT, even if it
supports only one pair of virtqueues, it MUST support at least one of
commands of VIRTIO_NET_CTRL_MQ class to configure reported hash
parameters:
If the device offers VIRTIO_NET_F_RSS, it MUST support
VIRTIO_NET_CTRL_MQ_RSS_CONFIG command per 5.1.6.5.7.1.
Otherwise the device MUST support VIRTIO_NET_CTRL_MQ_HASH_CONFIG command
per 5.1.6.5.6.4.
So if we have not anyone of `vi->has_rss` and `vi->has_rss_hash_report`,
we should return from virtnet_set_rxfh directly.
Thanks.
Re: qemu fuzz crash in virtio_net_queue_reset()
On Wed, 20 Mar 2024 00:24:37 +0300, "Vladimir Sementsov-Ogievskiy" wrote: > Hi all! > > From fuzzing I've got a fuzz-data, which produces the following crash: > > qemu-fuzz-x86_64: ../hw/net/virtio-net.c:134: void > flush_or_purge_queued_packets(NetClientState *): Assertion > `!virtio_net_get_subqueue(nc)->async_tx.elem' failed. > ==2172308== ERROR: libFuzzer: deadly signal > #0 0x5bd8c748b5a1 in __sanitizer_print_stack_trace > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x26f05a1) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > #1 0x5bd8c73fde38 in fuzzer::PrintStackTrace() > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2662e38) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > #2 0x5bd8c73e38b3 in fuzzer::Fuzzer::CrashCallback() > (/home/settlements/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x26488b3) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > #3 0x739eec84251f (/lib/x86_64-linux-gnu/libc.so.6+0x4251f) (BuildId: > c289da5071a3399de893d2af81d6a30c62646e1e) > #4 0x739eec8969fb in __pthread_kill_implementation > nptl/./nptl/pthread_kill.c:43:17 > #5 0x739eec8969fb in __pthread_kill_internal > nptl/./nptl/pthread_kill.c:78:10 > #6 0x739eec8969fb in pthread_kill nptl/./nptl/pthread_kill.c:89:10 > #7 0x739eec842475 in gsignal signal/../sysdeps/posix/raise.c:26:13 > #8 0x739eec8287f2 in abort stdlib/./stdlib/abort.c:79:7 > #9 0x739eec82871a in __assert_fail_base assert/./assert/assert.c:92:3 > #10 0x739eec839e95 in __assert_fail assert/./assert/assert.c:101:3 > #11 0x5bd8c995d9e2 in flush_or_purge_queued_packets > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/net/virtio-net.c:134:5 > #12 0x5bd8c9918a5f in virtio_net_queue_reset > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/net/virtio-net.c:563:5 > #13 0x5bd8c9b724e5 in virtio_queue_reset > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/virtio/virtio.c:2492:9 > #14 0x5bd8c8bcfb7c in virtio_pci_common_write > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/virtio/virtio-pci.c:1372:13 > #15 0x5bd8c9e19cf3 in memory_region_write_accessor > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/memory.c:492:5 > #16 0x5bd8c9e19631 in access_with_adjusted_size > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/memory.c:554:18 > #17 0x5bd8c9e17f3c in memory_region_dispatch_write > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/memory.c:1514:16 > #18 0x5bd8c9ea3bbe in flatview_write_continue > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/physmem.c:2825:23 > #19 0x5bd8c9e91aab in flatview_write > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/physmem.c:2867:12 > #20 0x5bd8c9e91568 in address_space_write > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/physmem.c:2963:18 > #21 0x5bd8c74c8a90 in __wrap_qtest_writeq > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/qtest_wrappers.c:187:9 > #22 0x5bd8c74dc4da in op_write > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/generic_fuzz.c:487:13 > #23 0x5bd8c74d942e in generic_fuzz > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/generic_fuzz.c:714:17 > #24 0x5bd8c74c016e in LLVMFuzzerTestOneInput > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/fuzz.c:152:5 > #25 0x5bd8c73e4e43 in fuzzer::Fuzzer::ExecuteCallback(unsigned char > const*, unsigned long) > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2649e43) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > #26 0x5bd8c73cebbf in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, > unsigned long) > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2633bbf) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > #27 0x5bd8c73d4916 in fuzzer::FuzzerDriver(int*, char***, int > (*)(unsigned char const*, unsigned long)) > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2639916) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > #28 0x5bd8c73fe732 in main > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2663732) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > #29 0x739eec829d8f in __libc_start_call_main > csu/../sysdeps/nptl/libc_start_call_main.h:58:16 > #30 0x739eec829e3f in __libc_start_main csu/../csu/libc-start.c:392:3 > #31 0x5bd8c73c9484 in _start > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x262e484) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > > > > How to reproduce: > ./configure --target-list=x86_64-softmmu --enable-debug --disable-docs > --cc=clang --cxx=clang++ --enable-fuzzing --enable-sanitizers --enable-slirp > make -j20 qemu-fuzz-x86_64 > ./build/qemu-fuzz-x86_64 --fuzz-target=generic-fuzz-virtio-net-pci-slirp >
Re: [PATCH] virtio_net: Do not send RSS key if it is not supported
On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao wrote:
> There is a bug when setting the RSS options in virtio_net that can break
> the whole machine, getting the kernel into an infinite loop.
>
> Running the following command in any QEMU virtual machine with virtionet
> will reproduce this problem:
>
> # ethtool -X eth0 hfunc toeplitz
>
> This is how the problem happens:
>
> 1) ethtool_set_rxfh() calls virtnet_set_rxfh()
>
> 2) virtnet_set_rxfh() calls virtnet_commit_rss_command()
>
> 3) virtnet_commit_rss_command() populates 4 entries for the rss
>scatter-gather
>
> 4) Since the command above does not have a key, then the last
>scatter-gatter entry will be zeroed, since rss_key_size == 0.
> sg_buf_size = vi->rss_key_size;
if (vi->has_rss || vi->has_rss_hash_report) {
vi->rss_indir_table_size =
virtio_cread16(vdev, offsetof(struct virtio_net_config,
rss_max_indirection_table_length));
vi->rss_key_size =
virtio_cread8(vdev, offsetof(struct virtio_net_config,
rss_max_key_size));
vi->rss_hash_types_supported =
virtio_cread32(vdev, offsetof(struct virtio_net_config,
supported_hash_types));
vi->rss_hash_types_supported &=
~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX |
VIRTIO_NET_RSS_HASH_TYPE_TCP_EX |
VIRTIO_NET_RSS_HASH_TYPE_UDP_EX);
dev->hw_features |= NETIF_F_RXHASH;
}
vi->rss_key_size is initiated here, I wonder if there is something wrong?
Thanks.
>
> 5) This buffer is passed to qemu, but qemu is not happy with a buffer
>with zero length, and do the following in virtqueue_map_desc() (QEMU
>function):
>
> if (!sz) {
> virtio_error(vdev, "virtio: zero sized buffers are not allowed");
>
> 6) virtio_error() (also QEMU function) set the device as broken
>
> vdev->broken = true;
>
> 7) Qemu bails out, and do not repond this crazy kernel.
>
> 8) The kernel is waiting for the response to come back (function
>virtnet_send_command())
>
> 9) The kernel is waiting doing the following :
>
> while (!virtqueue_get_buf(vi->cvq, ) &&
> !virtqueue_is_broken(vi->cvq))
> cpu_relax();
>
> 10) None of the following functions above is true, thus, the kernel
> loops here forever. Keeping in mind that virtqueue_is_broken() does
> not look at the qemu `vdev->broken`, so, it never realizes that the
> vitio is broken at QEMU side.
>
> Fix it by not sending the key scatter-gatter key if it is not set.
>
> Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
> Signed-off-by: Breno Leitao
> Cc: sta...@vger.kernel.org
> Cc: qemu-devel@nongnu.org
> ---
> drivers/net/virtio_net.c | 16 +---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d7ce4a1011ea..5a7700b103f8 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3041,11 +3041,16 @@ static int virtnet_set_ringparam(struct net_device
> *dev,
> static bool virtnet_commit_rss_command(struct virtnet_info *vi)
> {
> struct net_device *dev = vi->dev;
> + int has_key = vi->rss_key_size;
> struct scatterlist sgs[4];
> unsigned int sg_buf_size;
> + int nents = 3;
> +
> + if (has_key)
> + nents += 1;
>
> /* prepare sgs */
> - sg_init_table(sgs, 4);
> + sg_init_table(sgs, nents);
>
> sg_buf_size = offsetof(struct virtio_net_ctrl_rss, indirection_table);
> sg_set_buf([0], >ctrl->rss, sg_buf_size);
> @@ -3057,8 +3062,13 @@ static bool virtnet_commit_rss_command(struct
> virtnet_info *vi)
> - offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
> sg_set_buf([2], >ctrl->rss.max_tx_vq, sg_buf_size);
>
> - sg_buf_size = vi->rss_key_size;
> - sg_set_buf([3], vi->ctrl->rss.key, sg_buf_size);
> + if (has_key) {
> + /* Only populate if key is available, otherwise
> + * populating a buffer with zero size breaks virtio
> + */
> + sg_buf_size = vi->rss_key_size;
> + sg_set_buf([3], vi->ctrl->rss.key, sg_buf_size);
> + }
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
> vi->has_rss ? VIRTIO_NET_CTRL_MQ_RSS_CONFIG
> --
> 2.43.0
>
Re: [PATCH] virtio-net: not enable vq reset feature unconditionally
On Mon, 8 May 2023 11:09:46 +0200, Eugenio Perez Martin
wrote:
> On Sat, May 6, 2023 at 4:25 AM Xuan Zhuo wrote:
> >
> > On Thu, 4 May 2023 12:14:47 +0200, =?utf-8?q?Eugenio_P=C3=A9rez?=
> > wrote:
> > > The commit 93a97dc5200a ("virtio-net: enable vq reset feature") enables
> > > unconditionally vq reset feature as long as the device is emulated.
> > > This makes impossible to actually disable the feature, and it causes
> > > migration problems from qemu version previous than 7.2.
> > >
> > > The entire final commit is unneeded as device system already enable or
> > > disable the feature properly.
> > >
> > > This reverts commit 93a97dc5200a95e63b99cb625f20b7ae802ba413.
> > > Fixes: 93a97dc5200a ("virtio-net: enable vq reset feature")
> > > Signed-off-by: Eugenio Pérez
> > >
> > > ---
> > > Tested by checking feature bit at /sys/devices/pci.../virtio0/features
> > > enabling and disabling queue_reset virtio-net feature and vhost=on/off
> > > on net device backend.
> >
> > Do you mean that this feature cannot be closed?
> >
> > I tried to close in the guest, it was successful.
> >
>
> I'm not sure what you mean with close. If the device dataplane is
> emulated in qemu (vhost=off), I'm not able to make the device not
> offer it.
>
> > In addition, in this case, could you try to repair the problem instead of
> > directly revert.
> >
>
> I'm not following this. The revert is not to always disable the feature.
>
> By default, the feature is enabled. If cmdline states queue_reset=on,
> the feature is enabled. That is true both before and after applying
> this patch.
>
> However, in qemu master, queue_reset=off keeps enabling this feature
> on the device. It happens that there is a commit explicitly doing
> that, so I'm reverting it.
>
> Let me know if that makes sense to you.
I got it.
Looks like it's indeed a bug.
Reviewed-by: Xuan Zhuo
Thanks.
>
> Thanks!
>
Re: [PATCH] virtio-net: not enable vq reset feature unconditionally
On Mon, 8 May 2023 14:44:21 +0800, Jason Wang wrote:
> On Sun, May 7, 2023 at 2:01 PM Michael S. Tsirkin wrote:
> >
> > On Sat, May 06, 2023 at 10:13:36AM +0800, Xuan Zhuo wrote:
> > > On Thu, 4 May 2023 12:14:47 +0200, =?utf-8?q?Eugenio_P=C3=A9rez?=
> > > wrote:
> > > > The commit 93a97dc5200a ("virtio-net: enable vq reset feature") enables
> > > > unconditionally vq reset feature as long as the device is emulated.
> > > > This makes impossible to actually disable the feature, and it causes
> > > > migration problems from qemu version previous than 7.2.
> > > >
> > > > The entire final commit is unneeded as device system already enable or
> > > > disable the feature properly.
> > > >
> > > > This reverts commit 93a97dc5200a95e63b99cb625f20b7ae802ba413.
> > > > Fixes: 93a97dc5200a ("virtio-net: enable vq reset feature")
> > > > Signed-off-by: Eugenio Pérez
> > > >
> > > > ---
> > > > Tested by checking feature bit at /sys/devices/pci.../virtio0/features
> > > > enabling and disabling queue_reset virtio-net feature and vhost=on/off
> > > > on net device backend.
> > >
> > > Do you mean that this feature cannot be closed?
> > >
> > > I tried to close in the guest, it was successful.
> > >
> > > In addition, in this case, could you try to repair the problem instead of
> > > directly revert.
> > >
> > > Thanks.
> >
> > What does you patch accomplish though? If it's not needed
> > let's not do it.
>
> It looks to me the unconditional set of this feature breaks the
> migration of pre 7.2 machines.
>
> Also we probably need to make ring_reset as false by default, or
> compat it for pre 7.2 machines.
>
> DEFINE_PROP_BIT64("queue_reset", _state, _field, \
> VIRTIO_F_RING_RESET, true)
>
Do you mean this?
Thanks
commit 69e1c14aa22284f933a6ea134b96d5cb5a88a94d
Author: Kangjie Xu
Date: Mon Oct 17 17:25:47 2022 +0800
virtio: core: vq reset feature negotation support
A a new command line parameter "queue_reset" is added.
Meanwhile, the vq reset feature is disabled for pre-7.2 machines.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
Message-Id: <20221017092558.111082-5-xuanz...@linux.alibaba.com>
Reviewed-by: Michael S. Tsirkin
Signed-off-by: Michael S. Tsirkin
diff --git a/hw/core/machine.c b/hw/core/machine.c
index aa520e74a8..907fa78ff0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -40,7 +40,9 @@
#include "hw/virtio/virtio-pci.h"
#include "qom/object_interfaces.h"
-GlobalProperty hw_compat_7_1[] = {};
+GlobalProperty hw_compat_7_1[] = {
+{ "virtio-device", "queue_reset", "false" },
+};
const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
GlobalProperty hw_compat_7_0[] = {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index b00b3fcf31..1423dba379 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -313,7 +313,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
VIRTIO_F_IOMMU_PLATFORM, false), \
DEFINE_PROP_BIT64("packed", _state, _field, \
- VIRTIO_F_RING_PACKED, false)
+ VIRTIO_F_RING_PACKED, false), \
+DEFINE_PROP_BIT64("queue_reset", _state, _field, \
+ VIRTIO_F_RING_RESET, true)
hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
>
> Thanks
>
> >
> > > > ---
> > > > hw/net/virtio-net.c | 1 -
> > > > 1 file changed, 1 deletion(-)
> > > >
> > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > index 53e1c32643..4ea33b6e2e 100644
> > > > --- a/hw/net/virtio-net.c
> > > > +++ b/hw/net/virtio-net.c
> > > > @@ -805,7 +805,6 @@ static uint64_t
> > > > virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> > > > }
> > > >
> > > > if (!get_vhost_net(nc->peer)) {
> > > > -virtio_add_feature(, VIRTIO_F_RING_RESET);
> > > > return features;
> > > > }
> > > >
> > > > --
> > > > 2.31.1
> > > >
> >
>
Re: [PATCH] virtio-net: not enable vq reset feature unconditionally
On Thu, 4 May 2023 12:14:47 +0200, =?utf-8?q?Eugenio_P=C3=A9rez?=
wrote:
> The commit 93a97dc5200a ("virtio-net: enable vq reset feature") enables
> unconditionally vq reset feature as long as the device is emulated.
> This makes impossible to actually disable the feature, and it causes
> migration problems from qemu version previous than 7.2.
>
> The entire final commit is unneeded as device system already enable or
> disable the feature properly.
>
> This reverts commit 93a97dc5200a95e63b99cb625f20b7ae802ba413.
> Fixes: 93a97dc5200a ("virtio-net: enable vq reset feature")
> Signed-off-by: Eugenio Pérez
>
> ---
> Tested by checking feature bit at /sys/devices/pci.../virtio0/features
> enabling and disabling queue_reset virtio-net feature and vhost=on/off
> on net device backend.
Do you mean that this feature cannot be closed?
I tried to close in the guest, it was successful.
In addition, in this case, could you try to repair the problem instead of
directly revert.
Thanks.
> ---
> hw/net/virtio-net.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 53e1c32643..4ea33b6e2e 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -805,7 +805,6 @@ static uint64_t virtio_net_get_features(VirtIODevice
> *vdev, uint64_t features,
> }
>
> if (!get_vhost_net(nc->peer)) {
> -virtio_add_feature(, VIRTIO_F_RING_RESET);
> return features;
> }
>
> --
> 2.31.1
>
Re: [PATCH v2 07/10] contrib/gitdm: add Alibaba to the domain-map
On Fri, 10 Mar 2023 18:03:29 +, =?utf-8?q?Alex_Benn=C3=A9e?= wrote: > This replaces the previous attempt to add c-sky.com so I've dropped > the review/ack tags. Group everything under Alibaba now. > > Added as requested by LIU Zhiwei. > > Signed-off-by: Alex Bennée > Cc: LIU Zhiwei > Cc: Xuan Zhuo > Cc: Guo Ren Reviewed-by: Xuan Zhuo Thanks. > --- > contrib/gitdm/domain-map| 1 + > contrib/gitdm/group-map-alibaba | 7 +++ > gitdm.config| 1 + > 3 files changed, 9 insertions(+) > create mode 100644 contrib/gitdm/group-map-alibaba > > diff --git a/contrib/gitdm/domain-map b/contrib/gitdm/domain-map > index 0261533990..e678c23a9c 100644 > --- a/contrib/gitdm/domain-map > +++ b/contrib/gitdm/domain-map > @@ -4,6 +4,7 @@ > # This maps email domains to nice easy to read company names > # > > +linux.alibaba.com Alibaba > amazon.com Amazon > amazon.co.ukAmazon > amd.com AMD > diff --git a/contrib/gitdm/group-map-alibaba b/contrib/gitdm/group-map-alibaba > new file mode 100644 > index 00..0ebbe6b06e > --- /dev/null > +++ b/contrib/gitdm/group-map-alibaba > @@ -0,0 +1,7 @@ > +# > +# Alibaba contributors including its subsidiaries > +# > + > +# c-sky.com, now part of T-Head, wholly-owned entity of Alibaba Group > +ren_...@c-sky.com > +zhiwei_...@c-sky.com > diff --git a/gitdm.config b/gitdm.config > index 4b52ee47be..6908ddbd19 100644 > --- a/gitdm.config > +++ b/gitdm.config > @@ -31,6 +31,7 @@ EmailMap contrib/gitdm/domain-map > # identifiable corporate emails. Please keep this list sorted. > # > > +GroupMap contrib/gitdm/group-map-alibaba Alibaba > GroupMap contrib/gitdm/group-map-cadence Cadence Design Systems > GroupMap contrib/gitdm/group-map-codeweavers CodeWeavers > GroupMap contrib/gitdm/group-map-facebook Facebook > -- > 2.39.2 >
[PATCH v2 1/2] virtio_net: virtio_net_tx_complete() stop flush new packets for purge operation
For async tx, virtio_net_tx_complete() is called when purge or flush
operation is done. But for purge operation, we should not try to flush
new packet from tx queue. The purge operation means we will stop the
queue soon.
Signed-off-by: Xuan Zhuo
---
hw/net/virtio-net.c | 32 ++--
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3ae909041a..6daa1e5ac1 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2601,21 +2601,25 @@ static void virtio_net_tx_complete(NetClientState *nc,
ssize_t len)
q->async_tx.elem = NULL;
virtio_queue_set_notification(q->tx_vq, 1);
-ret = virtio_net_flush_tx(q);
-if (ret >= n->tx_burst) {
-/*
- * the flush has been stopped by tx_burst
- * we will not receive notification for the
- * remainining part, so re-schedule
- */
-virtio_queue_set_notification(q->tx_vq, 0);
-if (q->tx_bh) {
-qemu_bh_schedule(q->tx_bh);
-} else {
-timer_mod(q->tx_timer,
- qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + n->tx_timeout);
+
+/* len == 0 means purge, we should not flush new tx packets. */
+if (len) {
+ret = virtio_net_flush_tx(q);
+if (ret >= n->tx_burst) {
+/*
+ * the flush has been stopped by tx_burst
+ * we will not receive notification for the
+ * remainining part, so re-schedule
+ */
+virtio_queue_set_notification(q->tx_vq, 0);
+if (q->tx_bh) {
+qemu_bh_schedule(q->tx_bh);
+} else {
+timer_mod(q->tx_timer,
+ qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
n->tx_timeout);
+}
+q->tx_waiting = 1;
}
-q->tx_waiting = 1;
}
}
--
2.32.0.3.g01195cf9f
[PATCH v2 0/2] virtio: fix for assertion failure: virtio_net_get_subqueue(nc)->async_tx.elem failed
In the current design, we stop the device from operating on the vring during per-queue reset by resetting the structure VirtQueue. But before the reset operation, when recycling some resources, we should stop referencing new vring resources. This bug is caused by this reason. https://gitlab.com/qemu-project/qemu/-/issues/1451 Before we reset the structure, we called the ->queue_reset callback to let the device reclaim resources. Here virtio-net tries to release the packets sent asynchronously, but during this process virtio_net_flush_tx() will be called, and new data will be sent again. This leads to asserted. assert(!virtio_net_get_subqueue(nc)->async_tx.elem); v2: 1. fix by stop flush inside virtio_net_tx_complete() when purge packets. v1: 1. rename "reset" to disabled_by_reset 2. add api: virtio_queue_reset_state() Xuan Zhuo (2): virtio_net: virtio_net_tx_complete() stop flush new packets for purge operation virtio_net: just purge tx when dev/queue reset hw/net/virtio-net.c | 34 +++--- 1 file changed, 19 insertions(+), 15 deletions(-) -- 2.32.0.3.g01195cf9f
[PATCH v2 2/2] virtio_net: just purge tx when dev/queue reset
When dev/queue reset, we should just purge all packet, not try to flush
the async packets. When flush these async packets, the
callback(virtio_net_tx_complete) will try to flush new packets from tx
queue.
Fixes: 7dc6be52 ("virtio-net: support queue reset")
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1451
Reported-by: Alexander Bulekov
Signed-off-by: Xuan Zhuo
---
hw/net/virtio-net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 6daa1e5ac1..2ac6d3dad9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -570,7 +570,7 @@ static void virtio_net_queue_reset(VirtIODevice *vdev,
uint32_t queue_index)
vhost_net_virtqueue_reset(vdev, nc, queue_index);
}
-flush_or_purge_queued_packets(nc);
+qemu_purge_queued_packets(nc);
}
static void virtio_net_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
--
2.32.0.3.g01195cf9f
Re: [PATCH v1 2/2] virtio-net: virtio_net_flush_tx() check for per-queue reset
On Mon, 30 Jan 2023 15:49:36 +0800, Jason Wang wrote: > On Mon, Jan 30, 2023 at 1:32 PM Michael S. Tsirkin wrote: > > > > On Mon, Jan 30, 2023 at 10:15:12AM +0800, Xuan Zhuo wrote: > > > On Sun, 29 Jan 2023 07:15:47 -0500, "Michael S. Tsirkin" > > > wrote: > > > > On Sun, Jan 29, 2023 at 08:03:42PM +0800, Xuan Zhuo wrote: > > > > > On Sun, 29 Jan 2023 06:57:29 -0500, "Michael S. Tsirkin" > > > > > wrote: > > > > > > On Sun, Jan 29, 2023 at 04:23:08PM +0800, Xuan Zhuo wrote: > > > > > > > On Sun, 29 Jan 2023 03:12:12 -0500, "Michael S. Tsirkin" > > > > > > > wrote: > > > > > > > > On Sun, Jan 29, 2023 at 03:28:28PM +0800, Xuan Zhuo wrote: > > > > > > > > > On Sun, 29 Jan 2023 02:25:43 -0500, "Michael S. Tsirkin" > > > > > > > > > wrote: > > > > > > > > > > On Sun, Jan 29, 2023 at 10:51:50AM +0800, Xuan Zhuo wrote: > > > > > > > > > > > Check whether it is per-queue reset state in > > > > > > > > > > > virtio_net_flush_tx(). > > > > > > > > > > > > > > > > > > > > > > Before per-queue reset, we need to recover async tx > > > > > > > > > > > resources. At this > > > > > > > > > > > time, virtio_net_flush_tx() is called, but we should not > > > > > > > > > > > try to send > > > > > > > > > > > new packets, so virtio_net_flush_tx() should check the > > > > > > > > > > > current > > > > > > > > > > > per-queue reset state. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > What does "at this time" mean here? > > > > > > > > > > Do you in fact mean it's called from > > > > > > > > > > flush_or_purge_queued_packets? > > > > > > > > > > > > > > > > > > Yes > > > > > > > > > > > > > > > > > > virtio_queue_reset > > > > > > > > > k->queue_reset > > > > > > > > > virtio_net_queue_reset > > > > > > > > > flush_or_purge_queued_packets > > > > > > > > > qemu_flush_or_purge_queued_packets > > > > > > > > > . > > > > > > > > > (callback) > > > > > > > > > virtio_net_tx_complete > > > > > > > > > virtio_net_flush_tx > > > > > > > > > <-- here send new packet. We need stop it. > > > > > > > > > > > > > > > > > > > > > > > > > > > Because it is inside the callback, I can't pass information > > > > > > > > > through the stack. I > > > > > > > > > originally thought it was a general situation, so I wanted to > > > > > > > > > put it in > > > > > > > > > struct VirtQueue. > > > > > > > > > > > > > > > > > > If it is not very suitable, it may be better to put it in > > > > > > > > > VirtIONetQueue. > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > Hmm maybe. Another idea: isn't virtio_net_tx_complete called > > > > > > > > with length 0 here? Are there other cases where length is 0? > > > > > > > > > > > > > > > > > > > > > > > > > > What does the call stack look like? > > > > > > > > > > > > > > > > > > > > If yes introducing a vq state just so virtio_net_flush_tx > > > > > > > > > > knows we are in the process of reset would be a bad idea. > > > > > > > > > > We want something much more local, ideally on stack even ... > > > > > > > > > > > > > &
Re: [PATCH v1 2/2] virtio-net: virtio_net_flush_tx() check for per-queue reset
On Tue, 31 Jan 2023 11:27:42 +0800, Jason Wang wrote: > On Mon, Jan 30, 2023 at 6:26 PM Xuan Zhuo wrote: > > > > On Mon, 30 Jan 2023 16:40:08 +0800, Jason Wang wrote: > > > On Mon, Jan 30, 2023 at 4:03 PM Xuan Zhuo > > > wrote: > > > > > > > > On Mon, 30 Jan 2023 15:49:36 +0800, Jason Wang > > > > wrote: > > > > > On Mon, Jan 30, 2023 at 1:32 PM Michael S. Tsirkin > > > > > wrote: > > > > > > > > > > > > On Mon, Jan 30, 2023 at 10:15:12AM +0800, Xuan Zhuo wrote: > > > > > > > On Sun, 29 Jan 2023 07:15:47 -0500, "Michael S. Tsirkin" > > > > > > > wrote: > > > > > > > > On Sun, Jan 29, 2023 at 08:03:42PM +0800, Xuan Zhuo wrote: > > > > > > > > > On Sun, 29 Jan 2023 06:57:29 -0500, "Michael S. Tsirkin" > > > > > > > > > wrote: > > > > > > > > > > On Sun, Jan 29, 2023 at 04:23:08PM +0800, Xuan Zhuo wrote: > > > > > > > > > > > On Sun, 29 Jan 2023 03:12:12 -0500, "Michael S. Tsirkin" > > > > > > > > > > > wrote: > > > > > > > > > > > > On Sun, Jan 29, 2023 at 03:28:28PM +0800, Xuan Zhuo > > > > > > > > > > > > wrote: > > > > > > > > > > > > > On Sun, 29 Jan 2023 02:25:43 -0500, "Michael S. > > > > > > > > > > > > > Tsirkin" wrote: > > > > > > > > > > > > > > On Sun, Jan 29, 2023 at 10:51:50AM +0800, Xuan Zhuo > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > Check whether it is per-queue reset state in > > > > > > > > > > > > > > > virtio_net_flush_tx(). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Before per-queue reset, we need to recover async > > > > > > > > > > > > > > > tx resources. At this > > > > > > > > > > > > > > > time, virtio_net_flush_tx() is called, but we > > > > > > > > > > > > > > > should not try to send > > > > > > > > > > > > > > > new packets, so virtio_net_flush_tx() should > > > > > > > > > > > > > > > check the current > > > > > > > > > > > > > > > per-queue reset state. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > What does "at this time" mean here? > > > > > > > > > > > > > > Do you in fact mean it's called from > > > > > > > > > > > > > > flush_or_purge_queued_packets? > > > > > > > > > > > > > > > > > > > > > > > > > > Yes > > > > > > > > > > > > > > > > > > > > > > > > > > virtio_queue_reset > > > > > > > > > > > > > k->queue_reset > > > > > > > > > > > > > virtio_net_queue_reset > > > > > > > > > > > > > flush_or_purge_queued_packets > > > > > > > > > > > > > > > > > > > > > > > > > > qemu_flush_or_purge_queued_packets > > > > > > > > > > > > > . > > > > > > > > > > > > > (callback) > > > > > > > > > > > > > virtio_net_tx_complete > > > > > > > > > > > > > > > > > > > > > > > > > > virtio_net_flush_tx <-- here send new packet. We need > > > > > > > > > > > > > stop it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Be
Re: [PATCH v1 2/2] virtio-net: virtio_net_flush_tx() check for per-queue reset
On Mon, 30 Jan 2023 16:40:08 +0800, Jason Wang wrote: > On Mon, Jan 30, 2023 at 4:03 PM Xuan Zhuo wrote: > > > > On Mon, 30 Jan 2023 15:49:36 +0800, Jason Wang wrote: > > > On Mon, Jan 30, 2023 at 1:32 PM Michael S. Tsirkin > > > wrote: > > > > > > > > On Mon, Jan 30, 2023 at 10:15:12AM +0800, Xuan Zhuo wrote: > > > > > On Sun, 29 Jan 2023 07:15:47 -0500, "Michael S. Tsirkin" > > > > > wrote: > > > > > > On Sun, Jan 29, 2023 at 08:03:42PM +0800, Xuan Zhuo wrote: > > > > > > > On Sun, 29 Jan 2023 06:57:29 -0500, "Michael S. Tsirkin" > > > > > > > wrote: > > > > > > > > On Sun, Jan 29, 2023 at 04:23:08PM +0800, Xuan Zhuo wrote: > > > > > > > > > On Sun, 29 Jan 2023 03:12:12 -0500, "Michael S. Tsirkin" > > > > > > > > > wrote: > > > > > > > > > > On Sun, Jan 29, 2023 at 03:28:28PM +0800, Xuan Zhuo wrote: > > > > > > > > > > > On Sun, 29 Jan 2023 02:25:43 -0500, "Michael S. Tsirkin" > > > > > > > > > > > wrote: > > > > > > > > > > > > On Sun, Jan 29, 2023 at 10:51:50AM +0800, Xuan Zhuo > > > > > > > > > > > > wrote: > > > > > > > > > > > > > Check whether it is per-queue reset state in > > > > > > > > > > > > > virtio_net_flush_tx(). > > > > > > > > > > > > > > > > > > > > > > > > > > Before per-queue reset, we need to recover async tx > > > > > > > > > > > > > resources. At this > > > > > > > > > > > > > time, virtio_net_flush_tx() is called, but we should > > > > > > > > > > > > > not try to send > > > > > > > > > > > > > new packets, so virtio_net_flush_tx() should check > > > > > > > > > > > > > the current > > > > > > > > > > > > > per-queue reset state. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > What does "at this time" mean here? > > > > > > > > > > > > Do you in fact mean it's called from > > > > > > > > > > > > flush_or_purge_queued_packets? > > > > > > > > > > > > > > > > > > > > > > Yes > > > > > > > > > > > > > > > > > > > > > > virtio_queue_reset > > > > > > > > > > > k->queue_reset > > > > > > > > > > > virtio_net_queue_reset > > > > > > > > > > > flush_or_purge_queued_packets > > > > > > > > > > > > > > > > > > > > > > qemu_flush_or_purge_queued_packets > > > > > > > > > > > . > > > > > > > > > > > (callback) > > > > > > > > > > > virtio_net_tx_complete > > > > > > > > > > > > > > > > > > > > > > virtio_net_flush_tx <-- here send new packet. We need > > > > > > > > > > > stop it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Because it is inside the callback, I can't pass > > > > > > > > > > > information through the stack. I > > > > > > > > > > > originally thought it was a general situation, so I > > > > > > > > > > > wanted to put it in > > > > > > > > > > > struct VirtQueue. > > > > > > > > > > > > > > > > > > > > > > If it is not very suitable, it may be better to put it in > > > > > > > > > > > VirtIONetQueue. > > > > > > > > > > > > > > > &
Re: [PATCH v1 2/2] virtio-net: virtio_net_flush_tx() check for per-queue reset
On Mon, 30 Jan 2023 15:49:36 +0800, Jason Wang wrote: > On Mon, Jan 30, 2023 at 1:32 PM Michael S. Tsirkin wrote: > > > > On Mon, Jan 30, 2023 at 10:15:12AM +0800, Xuan Zhuo wrote: > > > On Sun, 29 Jan 2023 07:15:47 -0500, "Michael S. Tsirkin" > > > wrote: > > > > On Sun, Jan 29, 2023 at 08:03:42PM +0800, Xuan Zhuo wrote: > > > > > On Sun, 29 Jan 2023 06:57:29 -0500, "Michael S. Tsirkin" > > > > > wrote: > > > > > > On Sun, Jan 29, 2023 at 04:23:08PM +0800, Xuan Zhuo wrote: > > > > > > > On Sun, 29 Jan 2023 03:12:12 -0500, "Michael S. Tsirkin" > > > > > > > wrote: > > > > > > > > On Sun, Jan 29, 2023 at 03:28:28PM +0800, Xuan Zhuo wrote: > > > > > > > > > On Sun, 29 Jan 2023 02:25:43 -0500, "Michael S. Tsirkin" > > > > > > > > > wrote: > > > > > > > > > > On Sun, Jan 29, 2023 at 10:51:50AM +0800, Xuan Zhuo wrote: > > > > > > > > > > > Check whether it is per-queue reset state in > > > > > > > > > > > virtio_net_flush_tx(). > > > > > > > > > > > > > > > > > > > > > > Before per-queue reset, we need to recover async tx > > > > > > > > > > > resources. At this > > > > > > > > > > > time, virtio_net_flush_tx() is called, but we should not > > > > > > > > > > > try to send > > > > > > > > > > > new packets, so virtio_net_flush_tx() should check the > > > > > > > > > > > current > > > > > > > > > > > per-queue reset state. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > What does "at this time" mean here? > > > > > > > > > > Do you in fact mean it's called from > > > > > > > > > > flush_or_purge_queued_packets? > > > > > > > > > > > > > > > > > > Yes > > > > > > > > > > > > > > > > > > virtio_queue_reset > > > > > > > > > k->queue_reset > > > > > > > > > virtio_net_queue_reset > > > > > > > > > flush_or_purge_queued_packets > > > > > > > > > qemu_flush_or_purge_queued_packets > > > > > > > > > . > > > > > > > > > (callback) > > > > > > > > > virtio_net_tx_complete > > > > > > > > > virtio_net_flush_tx > > > > > > > > > <-- here send new packet. We need stop it. > > > > > > > > > > > > > > > > > > > > > > > > > > > Because it is inside the callback, I can't pass information > > > > > > > > > through the stack. I > > > > > > > > > originally thought it was a general situation, so I wanted to > > > > > > > > > put it in > > > > > > > > > struct VirtQueue. > > > > > > > > > > > > > > > > > > If it is not very suitable, it may be better to put it in > > > > > > > > > VirtIONetQueue. > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > Hmm maybe. Another idea: isn't virtio_net_tx_complete called > > > > > > > > with length 0 here? Are there other cases where length is 0? > > > > > > > > > > > > > > > > > > > > > > > > > > What does the call stack look like? > > > > > > > > > > > > > > > > > > > > If yes introducing a vq state just so virtio_net_flush_tx > > > > > > > > > > knows we are in the process of reset would be a bad idea. > > > > > > > > > > We want something much more local, ideally on stack even ... > > > > > > > > > > > > > &
Re: [PATCH v1 2/2] virtio-net: virtio_net_flush_tx() check for per-queue reset
On Mon, 30 Jan 2023 11:01:40 +0800, Jason Wang wrote:
> On Sun, Jan 29, 2023 at 3:44 PM Xuan Zhuo wrote:
> >
> > On Sun, 29 Jan 2023 14:23:21 +0800, Jason Wang wrote:
> > > On Sun, Jan 29, 2023 at 10:52 AM Xuan Zhuo
> > > wrote:
> > > >
> > > > Check whether it is per-queue reset state in virtio_net_flush_tx().
> > > >
> > > > Before per-queue reset, we need to recover async tx resources. At this
> > > > time, virtio_net_flush_tx() is called, but we should not try to send
> > > > new packets, so virtio_net_flush_tx() should check the current
> > > > per-queue reset state.
> > > >
> > > > Fixes: 7dc6be52 ("virtio-net: support queue reset")
> > > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1451
> > > > Reported-by: Alexander Bulekov
> > > > Signed-off-by: Xuan Zhuo
> > > > ---
> > > > hw/net/virtio-net.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > index 3ae909041a..fba6451a50 100644
> > > > --- a/hw/net/virtio-net.c
> > > > +++ b/hw/net/virtio-net.c
> > > > @@ -2627,7 +2627,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue
> > > > *q)
> > > > VirtQueueElement *elem;
> > > > int32_t num_packets = 0;
> > > > int queue_index = vq2q(virtio_get_queue_index(q->tx_vq));
> > > > -if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > > +if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) ||
> > > > +virtio_queue_reset_state(q->tx_vq)) {
> > >
> > > We have other places that check DRIVER_OK do we need to check queue
> > > reset as well?
> >
> > I checked it again. I still think that the location of other checking
> > DRIVER_OK
> > does not need to check the queue reset.
>
> For example, if we don't disable can_receive() when the queue is
> reset, it means rx may go for virtio_net_receive_rcu(). It means the
> Qemu is still trying to process the traffic from the network backend
> like tap which may waste cpu cycles.
>
> I think the correct way is to return false when the queue is reset in
> can_receive(), then the backend poll will be disabled (e.g TAP). When
> the queue is enabled again, qemu_flush_queued_packets() will wake up
> the backend polling.
>
> Having had time to check other places but it would be better to
> mention why it doesn't need a check in the changelog.
static bool virtio_net_can_receive(NetClientState *nc)
{
VirtIONet *n = qemu_get_nic_opaque(nc);
VirtIODevice *vdev = VIRTIO_DEVICE(n);
VirtIONetQueue *q = virtio_net_get_subqueue(nc);
if (!vdev->vm_running) {
return false;
}
if (nc->queue_index >= n->curr_queue_pairs) {
return false;
}
if (!virtio_queue_ready(q->rx_vq) ||
!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
return false;
}
return true;
}
int virtio_queue_ready(VirtQueue *vq)
{
return vq->vring.avail != 0;
}
static void __virtio_queue_reset(VirtIODevice *vdev, uint32_t i)
{
vdev->vq[i].vring.desc = 0;
vdev->vq[i].vring.avail = 0;
vdev->vq[i].vring.used = 0;
vdev->vq[i].last_avail_idx = 0;
vdev->vq[i].shadow_avail_idx = 0;
vdev->vq[i].used_idx = 0;
vdev->vq[i].last_avail_wrap_counter = true;
vdev->vq[i].shadow_avail_wrap_counter = true;
vdev->vq[i].used_wrap_counter = true;
virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
vdev->vq[i].signalled_used = 0;
vdev->vq[i].signalled_used_valid = false;
vdev->vq[i].notification = true;
vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
vdev->vq[i].inuse = 0;
virtio_virtqueue_reset_region_cache(>vq[i]);
}
In the implementation of Per-Queue Reset, for RX, we stop RX by setting
vdev->vq[i].vring.avail to 0.
Then callback can_receive will return False.
Thanks.
>
> Thanks
>
> >
> > Thanks.
> >
> >
> > >
> > > E.g:
> > > virtio_net_can_receive()
> > > virtio_net_tx_{timer|bh}()
> > >
> > > Thanks
> > >
> > > > return num_packets;
> > > > }
> > > >
> > > > --
> > > > 2.32.0.3.g01195cf9f
> > > >
> > >
> >
>
Re: [PATCH v1 2/2] virtio-net: virtio_net_flush_tx() check for per-queue reset
On Sun, 29 Jan 2023 07:15:47 -0500, "Michael S. Tsirkin"
wrote:
> On Sun, Jan 29, 2023 at 08:03:42PM +0800, Xuan Zhuo wrote:
> > On Sun, 29 Jan 2023 06:57:29 -0500, "Michael S. Tsirkin"
> > wrote:
> > > On Sun, Jan 29, 2023 at 04:23:08PM +0800, Xuan Zhuo wrote:
> > > > On Sun, 29 Jan 2023 03:12:12 -0500, "Michael S. Tsirkin"
> > > > wrote:
> > > > > On Sun, Jan 29, 2023 at 03:28:28PM +0800, Xuan Zhuo wrote:
> > > > > > On Sun, 29 Jan 2023 02:25:43 -0500, "Michael S. Tsirkin"
> > > > > > wrote:
> > > > > > > On Sun, Jan 29, 2023 at 10:51:50AM +0800, Xuan Zhuo wrote:
> > > > > > > > Check whether it is per-queue reset state in
> > > > > > > > virtio_net_flush_tx().
> > > > > > > >
> > > > > > > > Before per-queue reset, we need to recover async tx resources.
> > > > > > > > At this
> > > > > > > > time, virtio_net_flush_tx() is called, but we should not try to
> > > > > > > > send
> > > > > > > > new packets, so virtio_net_flush_tx() should check the current
> > > > > > > > per-queue reset state.
> > > > > > >
> > > > > > >
> > > > > > > What does "at this time" mean here?
> > > > > > > Do you in fact mean it's called from
> > > > > > > flush_or_purge_queued_packets?
> > > > > >
> > > > > > Yes
> > > > > >
> > > > > > virtio_queue_reset
> > > > > > k->queue_reset
> > > > > > virtio_net_queue_reset
> > > > > > flush_or_purge_queued_packets
> > > > > > qemu_flush_or_purge_queued_packets
> > > > > > .
> > > > > > (callback)
> > > > > > virtio_net_tx_complete
> > > > > > virtio_net_flush_tx <--
> > > > > > here send new packet. We need stop it.
> > > > > >
> > > > > >
> > > > > > Because it is inside the callback, I can't pass information through
> > > > > > the stack. I
> > > > > > originally thought it was a general situation, so I wanted to put
> > > > > > it in
> > > > > > struct VirtQueue.
> > > > > >
> > > > > > If it is not very suitable, it may be better to put it in
> > > > > > VirtIONetQueue.
> > > > > >
> > > > > > Thanks.
> > > > >
> > > > > Hmm maybe. Another idea: isn't virtio_net_tx_complete called
> > > > > with length 0 here? Are there other cases where length is 0?
> > > > >
> > > > >
> > > > > > > What does the call stack look like?
> > > > > > >
> > > > > > > If yes introducing a vq state just so virtio_net_flush_tx
> > > > > > > knows we are in the process of reset would be a bad idea.
> > > > > > > We want something much more local, ideally on stack even ...
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Fixes: 7dc6be52 ("virtio-net: support queue reset")
> > > > > > > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1451
> > > > > > > > Reported-by: Alexander Bulekov
> > > > > > > > Signed-off-by: Xuan Zhuo
> > > > > > > > ---
> > > > > > > > hw/net/virtio-net.c | 3 ++-
> > > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > > > > > index 3ae909041a..fba6451a50 100644
> > > > > > > > --- a/hw/net/virtio-net.c
> > > > > > > > +++ b/hw/net/virtio-net.c
> > > > > > > > @@ -2627,7 +2627,8 @@ static int32_t
> > > > > > > > virtio_net_flush_tx(VirtIONetQueue *q)
> > > > > > > > VirtQueueElement *elem;
> > >
Re: [PATCH v1 2/2] virtio-net: virtio_net_flush_tx() check for per-queue reset
On Sun, 29 Jan 2023 06:57:29 -0500, "Michael S. Tsirkin"
wrote:
> On Sun, Jan 29, 2023 at 04:23:08PM +0800, Xuan Zhuo wrote:
> > On Sun, 29 Jan 2023 03:12:12 -0500, "Michael S. Tsirkin"
> > wrote:
> > > On Sun, Jan 29, 2023 at 03:28:28PM +0800, Xuan Zhuo wrote:
> > > > On Sun, 29 Jan 2023 02:25:43 -0500, "Michael S. Tsirkin"
> > > > wrote:
> > > > > On Sun, Jan 29, 2023 at 10:51:50AM +0800, Xuan Zhuo wrote:
> > > > > > Check whether it is per-queue reset state in virtio_net_flush_tx().
> > > > > >
> > > > > > Before per-queue reset, we need to recover async tx resources. At
> > > > > > this
> > > > > > time, virtio_net_flush_tx() is called, but we should not try to send
> > > > > > new packets, so virtio_net_flush_tx() should check the current
> > > > > > per-queue reset state.
> > > > >
> > > > >
> > > > > What does "at this time" mean here?
> > > > > Do you in fact mean it's called from flush_or_purge_queued_packets?
> > > >
> > > > Yes
> > > >
> > > > virtio_queue_reset
> > > > k->queue_reset
> > > > virtio_net_queue_reset
> > > > flush_or_purge_queued_packets
> > > > qemu_flush_or_purge_queued_packets
> > > > .
> > > > (callback)
> > > > virtio_net_tx_complete
> > > > virtio_net_flush_tx <--
> > > > here send new packet. We need stop it.
> > > >
> > > >
> > > > Because it is inside the callback, I can't pass information through the
> > > > stack. I
> > > > originally thought it was a general situation, so I wanted to put it in
> > > > struct VirtQueue.
> > > >
> > > > If it is not very suitable, it may be better to put it in
> > > > VirtIONetQueue.
> > > >
> > > > Thanks.
> > >
> > > Hmm maybe. Another idea: isn't virtio_net_tx_complete called
> > > with length 0 here? Are there other cases where length is 0?
> > >
> > >
> > > > > What does the call stack look like?
> > > > >
> > > > > If yes introducing a vq state just so virtio_net_flush_tx
> > > > > knows we are in the process of reset would be a bad idea.
> > > > > We want something much more local, ideally on stack even ...
> > > > >
> > > > >
> > > > > >
> > > > > > Fixes: 7dc6be52 ("virtio-net: support queue reset")
> > > > > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1451
> > > > > > Reported-by: Alexander Bulekov
> > > > > > Signed-off-by: Xuan Zhuo
> > > > > > ---
> > > > > > hw/net/virtio-net.c | 3 ++-
> > > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > > > index 3ae909041a..fba6451a50 100644
> > > > > > --- a/hw/net/virtio-net.c
> > > > > > +++ b/hw/net/virtio-net.c
> > > > > > @@ -2627,7 +2627,8 @@ static int32_t
> > > > > > virtio_net_flush_tx(VirtIONetQueue *q)
> > > > > > VirtQueueElement *elem;
> > > > > > int32_t num_packets = 0;
> > > > > > int queue_index = vq2q(virtio_get_queue_index(q->tx_vq));
> > > > > > -if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > > > > +if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) ||
> > > > > > +virtio_queue_reset_state(q->tx_vq)) {
> > >
> > > btw this sounds like you are asking it to reset some state.
> > >
> > > > > > return num_packets;
> > >
> > > and then
> > >
> > > ret = virtio_net_flush_tx(q);
> > > if (ret >= n->tx_burst)
> > >
> > >
> > > will reschedule automatically won't it?
> > >
> > > also why check in virtio_net_flush_tx and not virtio_net_tx_complete?
> >
> > virtio_net_flush_tx may been called by timer.
> >
> > Thanks.
>
> timer won't run while flush_or_purge_queued_packets is in progress.
Is timer not executed during the VMEXIT process? Otherwise, we still have to
consider that after the flush_or_purge_queued_packets, this process before the
structure is cleared.
Even if it can be processed in virtio_net_tx_complete, is there any good way?
This is a callback, it is not convenient to pass the parameters.
Thanks
>
> > >
> > >
> > > > > > }
> > > > > >
> > > > > > --
> > > > > > 2.32.0.3.g01195cf9f
> > > > >
> > >
>
Re: [PATCH v1 2/2] virtio-net: virtio_net_flush_tx() check for per-queue reset
On Sun, 29 Jan 2023 07:15:47 -0500, "Michael S. Tsirkin"
wrote:
> On Sun, Jan 29, 2023 at 08:03:42PM +0800, Xuan Zhuo wrote:
> > On Sun, 29 Jan 2023 06:57:29 -0500, "Michael S. Tsirkin"
> > wrote:
> > > On Sun, Jan 29, 2023 at 04:23:08PM +0800, Xuan Zhuo wrote:
> > > > On Sun, 29 Jan 2023 03:12:12 -0500, "Michael S. Tsirkin"
> > > > wrote:
> > > > > On Sun, Jan 29, 2023 at 03:28:28PM +0800, Xuan Zhuo wrote:
> > > > > > On Sun, 29 Jan 2023 02:25:43 -0500, "Michael S. Tsirkin"
> > > > > > wrote:
> > > > > > > On Sun, Jan 29, 2023 at 10:51:50AM +0800, Xuan Zhuo wrote:
> > > > > > > > Check whether it is per-queue reset state in
> > > > > > > > virtio_net_flush_tx().
> > > > > > > >
> > > > > > > > Before per-queue reset, we need to recover async tx resources.
> > > > > > > > At this
> > > > > > > > time, virtio_net_flush_tx() is called, but we should not try to
> > > > > > > > send
> > > > > > > > new packets, so virtio_net_flush_tx() should check the current
> > > > > > > > per-queue reset state.
> > > > > > >
> > > > > > >
> > > > > > > What does "at this time" mean here?
> > > > > > > Do you in fact mean it's called from
> > > > > > > flush_or_purge_queued_packets?
> > > > > >
> > > > > > Yes
> > > > > >
> > > > > > virtio_queue_reset
> > > > > > k->queue_reset
> > > > > > virtio_net_queue_reset
> > > > > > flush_or_purge_queued_packets
> > > > > > qemu_flush_or_purge_queued_packets
> > > > > > .
> > > > > > (callback)
> > > > > > virtio_net_tx_complete
> > > > > > virtio_net_flush_tx <--
> > > > > > here send new packet. We need stop it.
> > > > > >
> > > > > >
> > > > > > Because it is inside the callback, I can't pass information through
> > > > > > the stack. I
> > > > > > originally thought it was a general situation, so I wanted to put
> > > > > > it in
> > > > > > struct VirtQueue.
> > > > > >
> > > > > > If it is not very suitable, it may be better to put it in
> > > > > > VirtIONetQueue.
> > > > > >
> > > > > > Thanks.
> > > > >
> > > > > Hmm maybe. Another idea: isn't virtio_net_tx_complete called
> > > > > with length 0 here? Are there other cases where length is 0?
> > > > >
> > > > >
> > > > > > > What does the call stack look like?
> > > > > > >
> > > > > > > If yes introducing a vq state just so virtio_net_flush_tx
> > > > > > > knows we are in the process of reset would be a bad idea.
> > > > > > > We want something much more local, ideally on stack even ...
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Fixes: 7dc6be52 ("virtio-net: support queue reset")
> > > > > > > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1451
> > > > > > > > Reported-by: Alexander Bulekov
> > > > > > > > Signed-off-by: Xuan Zhuo
> > > > > > > > ---
> > > > > > > > hw/net/virtio-net.c | 3 ++-
> > > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > > > > > index 3ae909041a..fba6451a50 100644
> > > > > > > > --- a/hw/net/virtio-net.c
> > > > > > > > +++ b/hw/net/virtio-net.c
> > > > > > > > @@ -2627,7 +2627,8 @@ static int32_t
> > > > > > > > virtio_net_flush_tx(VirtIONetQueue *q)
> > > > > > > > VirtQueueElement *elem;
> > >
Re: [PATCH v1 2/2] virtio-net: virtio_net_flush_tx() check for per-queue reset
On Sun, 29 Jan 2023 03:12:12 -0500, "Michael S. Tsirkin"
wrote:
> On Sun, Jan 29, 2023 at 03:28:28PM +0800, Xuan Zhuo wrote:
> > On Sun, 29 Jan 2023 02:25:43 -0500, "Michael S. Tsirkin"
> > wrote:
> > > On Sun, Jan 29, 2023 at 10:51:50AM +0800, Xuan Zhuo wrote:
> > > > Check whether it is per-queue reset state in virtio_net_flush_tx().
> > > >
> > > > Before per-queue reset, we need to recover async tx resources. At this
> > > > time, virtio_net_flush_tx() is called, but we should not try to send
> > > > new packets, so virtio_net_flush_tx() should check the current
> > > > per-queue reset state.
> > >
> > >
> > > What does "at this time" mean here?
> > > Do you in fact mean it's called from flush_or_purge_queued_packets?
> >
> > Yes
> >
> > virtio_queue_reset
> > k->queue_reset
> > virtio_net_queue_reset
> > flush_or_purge_queued_packets
> > qemu_flush_or_purge_queued_packets
> > .
> > (callback) virtio_net_tx_complete
> > virtio_net_flush_tx <-- here
> > send new packet. We need stop it.
> >
> >
> > Because it is inside the callback, I can't pass information through the
> > stack. I
> > originally thought it was a general situation, so I wanted to put it in
> > struct VirtQueue.
> >
> > If it is not very suitable, it may be better to put it in VirtIONetQueue.
> >
> > Thanks.
>
> Hmm maybe. Another idea: isn't virtio_net_tx_complete called
> with length 0 here? Are there other cases where length is 0?
>
>
> > > What does the call stack look like?
> > >
> > > If yes introducing a vq state just so virtio_net_flush_tx
> > > knows we are in the process of reset would be a bad idea.
> > > We want something much more local, ideally on stack even ...
> > >
> > >
> > > >
> > > > Fixes: 7dc6be52 ("virtio-net: support queue reset")
> > > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1451
> > > > Reported-by: Alexander Bulekov
> > > > Signed-off-by: Xuan Zhuo
> > > > ---
> > > > hw/net/virtio-net.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > index 3ae909041a..fba6451a50 100644
> > > > --- a/hw/net/virtio-net.c
> > > > +++ b/hw/net/virtio-net.c
> > > > @@ -2627,7 +2627,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue
> > > > *q)
> > > > VirtQueueElement *elem;
> > > > int32_t num_packets = 0;
> > > > int queue_index = vq2q(virtio_get_queue_index(q->tx_vq));
> > > > -if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > > +if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) ||
> > > > +virtio_queue_reset_state(q->tx_vq)) {
>
> btw this sounds like you are asking it to reset some state.
>
> > > > return num_packets;
>
> and then
>
> ret = virtio_net_flush_tx(q);
> if (ret >= n->tx_burst)
>
>
> will reschedule automatically won't it?
>
> also why check in virtio_net_flush_tx and not virtio_net_tx_complete?
virtio_net_flush_tx may been called by timer.
Thanks.
>
>
> > > > }
> > > >
> > > > --
> > > > 2.32.0.3.g01195cf9f
> > >
>
Re: [PATCH v1 2/2] virtio-net: virtio_net_flush_tx() check for per-queue reset
On Sun, 29 Jan 2023 14:23:21 +0800, Jason Wang wrote:
> On Sun, Jan 29, 2023 at 10:52 AM Xuan Zhuo wrote:
> >
> > Check whether it is per-queue reset state in virtio_net_flush_tx().
> >
> > Before per-queue reset, we need to recover async tx resources. At this
> > time, virtio_net_flush_tx() is called, but we should not try to send
> > new packets, so virtio_net_flush_tx() should check the current
> > per-queue reset state.
> >
> > Fixes: 7dc6be52 ("virtio-net: support queue reset")
> > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1451
> > Reported-by: Alexander Bulekov
> > Signed-off-by: Xuan Zhuo
> > ---
> > hw/net/virtio-net.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 3ae909041a..fba6451a50 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -2627,7 +2627,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> > VirtQueueElement *elem;
> > int32_t num_packets = 0;
> > int queue_index = vq2q(virtio_get_queue_index(q->tx_vq));
> > -if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > +if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) ||
> > +virtio_queue_reset_state(q->tx_vq)) {
>
> We have other places that check DRIVER_OK do we need to check queue
> reset as well?
I checked it again. I still think that the location of other checking DRIVER_OK
does not need to check the queue reset.
Thanks.
>
> E.g:
> virtio_net_can_receive()
> virtio_net_tx_{timer|bh}()
>
> Thanks
>
> > return num_packets;
> > }
> >
> > --
> > 2.32.0.3.g01195cf9f
> >
>
Re: [PATCH v1 1/2] virtio: struct VirtQueue introduce reset
On Sun, 29 Jan 2023 02:37:22 -0500, "Michael S. Tsirkin"
wrote:
> On Sun, Jan 29, 2023 at 03:15:16PM +0800, Xuan Zhuo wrote:
> > On Sun, 29 Jan 2023 02:12:36 -0500, "Michael S. Tsirkin"
> > wrote:
> > >
> > > subject seems wrong.
> >
> >
> > Will fix.
> >
> >
> > >
> > > On Sun, Jan 29, 2023 at 10:51:49AM +0800, Xuan Zhuo wrote:
> > > > In the current design, we stop the device from operating on the vring
> > > > during per-queue reset by resetting the structure VirtQueue.
> > > >
> > > > But before the reset operation, when recycling some resources, we should
> > > > stop referencing new vring resources. For example, when recycling
> > > > virtio-net's asynchronous sending resources, virtio-net should be able
> > > > to perceive that the current queue is in the per-queue reset state, and
> > > > stop sending new packets from the tx queue.
> > > >
> > > > Signed-off-by: Xuan Zhuo
> > > > ---
> > > > hw/virtio/virtio.c | 15 +++
> > > > include/hw/virtio/virtio.h | 1 +
> > > > 2 files changed, 16 insertions(+)
> > > >
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index f35178f5fc..c954f2a2b3 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -142,6 +142,8 @@ struct VirtQueue
> > > > /* Notification enabled? */
> > > > bool notification;
> > > >
> > > > +bool disabled_by_reset;
> > > > +
> > > > uint16_t queue_index;
> > > >
> > > > unsigned int inuse;
> > > > @@ -2079,6 +2081,12 @@ void virtio_queue_reset(VirtIODevice *vdev,
> > > > uint32_t queue_index)
> > > > {
> > > > VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > > >
> > > > +/*
> > > > + * Mark this queue is per-queue reset status. The device should
> > > > release the
> > > > + * references of the vring, and not refer more new vring item.
> > >
> > > items
> >
> >
> > Will fix.
> >
> > >
> > > > + */
> > > > +vdev->vq[queue_index].disabled_by_reset = true;
> > > > +
> > > > if (k->queue_reset) {
> > > > k->queue_reset(vdev, queue_index);
> > > > }
> > >
> > > can we set this after calling queue_reset? For symmetry with enable.
> >
> >
> > In fact, queue_reset() will check it.
> >
>
> when you disable you first set it then disable.
> so when we are not 100% ready it's already set.
> when you enable you first clear it then enable.
> so we are not 100% ready but it's no longer set.
> inconsistent.
>
>
> > >
> > > > @@ -2102,11 +2110,18 @@ void virtio_queue_enable(VirtIODevice *vdev,
> > > > uint32_t queue_index)
> > > > }
> > > > */
> > > >
> > > > +vdev->vq[queue_index].disabled_by_reset = false;
> > > > +
> > > > if (k->queue_enable) {
> > > > k->queue_enable(vdev, queue_index);
> > > > }
> > > > }
> > > >
> > > > +bool virtio_queue_reset_state(VirtQueue *vq)
> > > > +{
> > > > +return vq->disabled_by_reset;
> > > > +}
> > > > +
> > > > void virtio_reset(void *opaque)
> > > > {
> > > > VirtIODevice *vdev = opaque;
> > > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > > > index 77c6c55929..00e91af7c4 100644
> > > > --- a/include/hw/virtio/virtio.h
> > > > +++ b/include/hw/virtio/virtio.h
> > > > @@ -319,6 +319,7 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t
> > > > val);
> > > > void virtio_reset(void *opaque);
> > > > void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
> > > > void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
> > > > +bool virtio_queue_reset_state(VirtQueue *vq);
> > > > void virtio_update_irq(VirtIODevice *vdev);
> > > > int virtio_set_features(VirtIODevice *vdev, uint64_t val);
> > >
> > > OK I guess ... what about migration. This state won't be
> > > set correctly will it?
> >
> > I think it has no effect. After the reset, there is actually no state. We
> > can
> > migrate.
> >
> > The current variable is only used by ->queue_reset().
> >
> > Thanks.
> >
>
> Yea maybe it works for this bug but ... yack. This means the state has
> no logic consistency. It's just there because you found a bug and
> wanted to fix it.
> An ultra specific
> bool this_weird_state_fuzzer_gets_in_issue_1451;
> is hard to maintain, not happy :(
I agree.
Thanks.
>
>
> > >
> > >
> > > >
> > > > --
> > > > 2.32.0.3.g01195cf9f
> > >
>
Re: [PATCH v1 2/2] virtio-net: virtio_net_flush_tx() check for per-queue reset
On Sun, 29 Jan 2023 02:25:43 -0500, "Michael S. Tsirkin"
wrote:
> On Sun, Jan 29, 2023 at 10:51:50AM +0800, Xuan Zhuo wrote:
> > Check whether it is per-queue reset state in virtio_net_flush_tx().
> >
> > Before per-queue reset, we need to recover async tx resources. At this
> > time, virtio_net_flush_tx() is called, but we should not try to send
> > new packets, so virtio_net_flush_tx() should check the current
> > per-queue reset state.
>
>
> What does "at this time" mean here?
> Do you in fact mean it's called from flush_or_purge_queued_packets?
Yes
virtio_queue_reset
k->queue_reset
virtio_net_queue_reset
flush_or_purge_queued_packets
qemu_flush_or_purge_queued_packets
.
(callback) virtio_net_tx_complete
virtio_net_flush_tx <-- here
send new packet. We need stop it.
Because it is inside the callback, I can't pass information through the stack. I
originally thought it was a general situation, so I wanted to put it in
struct VirtQueue.
If it is not very suitable, it may be better to put it in VirtIONetQueue.
Thanks.
> What does the call stack look like?
>
> If yes introducing a vq state just so virtio_net_flush_tx
> knows we are in the process of reset would be a bad idea.
> We want something much more local, ideally on stack even ...
>
>
> >
> > Fixes: 7dc6be52 ("virtio-net: support queue reset")
> > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1451
> > Reported-by: Alexander Bulekov
> > Signed-off-by: Xuan Zhuo
> > ---
> > hw/net/virtio-net.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 3ae909041a..fba6451a50 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -2627,7 +2627,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> > VirtQueueElement *elem;
> > int32_t num_packets = 0;
> > int queue_index = vq2q(virtio_get_queue_index(q->tx_vq));
> > -if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > +if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) ||
> > +virtio_queue_reset_state(q->tx_vq)) {
> > return num_packets;
> > }
> >
> > --
> > 2.32.0.3.g01195cf9f
>
Re: [PATCH v1 1/2] virtio: struct VirtQueue introduce reset
On Sun, 29 Jan 2023 02:12:36 -0500, "Michael S. Tsirkin"
wrote:
>
> subject seems wrong.
Will fix.
>
> On Sun, Jan 29, 2023 at 10:51:49AM +0800, Xuan Zhuo wrote:
> > In the current design, we stop the device from operating on the vring
> > during per-queue reset by resetting the structure VirtQueue.
> >
> > But before the reset operation, when recycling some resources, we should
> > stop referencing new vring resources. For example, when recycling
> > virtio-net's asynchronous sending resources, virtio-net should be able
> > to perceive that the current queue is in the per-queue reset state, and
> > stop sending new packets from the tx queue.
> >
> > Signed-off-by: Xuan Zhuo
> > ---
> > hw/virtio/virtio.c | 15 +++
> > include/hw/virtio/virtio.h | 1 +
> > 2 files changed, 16 insertions(+)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index f35178f5fc..c954f2a2b3 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -142,6 +142,8 @@ struct VirtQueue
> > /* Notification enabled? */
> > bool notification;
> >
> > +bool disabled_by_reset;
> > +
> > uint16_t queue_index;
> >
> > unsigned int inuse;
> > @@ -2079,6 +2081,12 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t
> > queue_index)
> > {
> > VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> >
> > +/*
> > + * Mark this queue is per-queue reset status. The device should
> > release the
> > + * references of the vring, and not refer more new vring item.
>
> items
Will fix.
>
> > + */
> > +vdev->vq[queue_index].disabled_by_reset = true;
> > +
> > if (k->queue_reset) {
> > k->queue_reset(vdev, queue_index);
> > }
>
> can we set this after calling queue_reset? For symmetry with enable.
In fact, queue_reset() will check it.
>
> > @@ -2102,11 +2110,18 @@ void virtio_queue_enable(VirtIODevice *vdev,
> > uint32_t queue_index)
> > }
> > */
> >
> > +vdev->vq[queue_index].disabled_by_reset = false;
> > +
> > if (k->queue_enable) {
> > k->queue_enable(vdev, queue_index);
> > }
> > }
> >
> > +bool virtio_queue_reset_state(VirtQueue *vq)
> > +{
> > +return vq->disabled_by_reset;
> > +}
> > +
> > void virtio_reset(void *opaque)
> > {
> > VirtIODevice *vdev = opaque;
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 77c6c55929..00e91af7c4 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -319,6 +319,7 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val);
> > void virtio_reset(void *opaque);
> > void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
> > void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
> > +bool virtio_queue_reset_state(VirtQueue *vq);
> > void virtio_update_irq(VirtIODevice *vdev);
> > int virtio_set_features(VirtIODevice *vdev, uint64_t val);
>
> OK I guess ... what about migration. This state won't be
> set correctly will it?
I think it has no effect. After the reset, there is actually no state. We can
migrate.
The current variable is only used by ->queue_reset().
Thanks.
>
>
> >
> > --
> > 2.32.0.3.g01195cf9f
>
[PATCH v1 0/2] virtio: fix for assertion failure: virtio_net_get_subqueue(nc)->async_tx.elem failed
In the current design, we stop the device from operating on the vring during per-queue reset by resetting the structure VirtQueue. But before the reset operation, when recycling some resources, we should stop referencing new vring resources. This bug is caused by this reason. https://gitlab.com/qemu-project/qemu/-/issues/1451 Before we reset the structure, we called the ->queue_reset callback to let the device reclaim resources. Here virtio-net tries to release the packets sent asynchronously, but during this process virtio_net_flush_tx() will be called, and new data will be sent again. This leads to asserted. assert(!virtio_net_get_subqueue(nc)->async_tx.elem); This patch set introduce new item "reset" into struct VirtQueue, then device can know this virtqueue is per-queue reset state. v1: 1. rename "reset" to disabled_by_reset 2. add api: virtio_queue_reset_state() Xuan Zhuo (2): virtio: struct VirtQueue introduce reset virtio-net: virtio_net_flush_tx() check for per-queue reset hw/net/virtio-net.c| 3 ++- hw/virtio/virtio.c | 15 +++ include/hw/virtio/virtio.h | 1 + 3 files changed, 18 insertions(+), 1 deletion(-) -- 2.32.0.3.g01195cf9f
[PATCH v1 1/2] virtio: struct VirtQueue introduce reset
In the current design, we stop the device from operating on the vring
during per-queue reset by resetting the structure VirtQueue.
But before the reset operation, when recycling some resources, we should
stop referencing new vring resources. For example, when recycling
virtio-net's asynchronous sending resources, virtio-net should be able
to perceive that the current queue is in the per-queue reset state, and
stop sending new packets from the tx queue.
Signed-off-by: Xuan Zhuo
---
hw/virtio/virtio.c | 15 +++
include/hw/virtio/virtio.h | 1 +
2 files changed, 16 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f35178f5fc..c954f2a2b3 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -142,6 +142,8 @@ struct VirtQueue
/* Notification enabled? */
bool notification;
+bool disabled_by_reset;
+
uint16_t queue_index;
unsigned int inuse;
@@ -2079,6 +2081,12 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t
queue_index)
{
VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+/*
+ * Mark this queue is per-queue reset status. The device should release the
+ * references of the vring, and not refer more new vring item.
+ */
+vdev->vq[queue_index].disabled_by_reset = true;
+
if (k->queue_reset) {
k->queue_reset(vdev, queue_index);
}
@@ -2102,11 +2110,18 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t
queue_index)
}
*/
+vdev->vq[queue_index].disabled_by_reset = false;
+
if (k->queue_enable) {
k->queue_enable(vdev, queue_index);
}
}
+bool virtio_queue_reset_state(VirtQueue *vq)
+{
+return vq->disabled_by_reset;
+}
+
void virtio_reset(void *opaque)
{
VirtIODevice *vdev = opaque;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 77c6c55929..00e91af7c4 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -319,6 +319,7 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val);
void virtio_reset(void *opaque);
void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
+bool virtio_queue_reset_state(VirtQueue *vq);
void virtio_update_irq(VirtIODevice *vdev);
int virtio_set_features(VirtIODevice *vdev, uint64_t val);
--
2.32.0.3.g01195cf9f
[PATCH v1 2/2] virtio-net: virtio_net_flush_tx() check for per-queue reset
Check whether it is per-queue reset state in virtio_net_flush_tx().
Before per-queue reset, we need to recover async tx resources. At this
time, virtio_net_flush_tx() is called, but we should not try to send
new packets, so virtio_net_flush_tx() should check the current
per-queue reset state.
Fixes: 7dc6be52 ("virtio-net: support queue reset")
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1451
Reported-by: Alexander Bulekov
Signed-off-by: Xuan Zhuo
---
hw/net/virtio-net.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3ae909041a..fba6451a50 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2627,7 +2627,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
VirtQueueElement *elem;
int32_t num_packets = 0;
int queue_index = vq2q(virtio_get_queue_index(q->tx_vq));
-if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) ||
+virtio_queue_reset_state(q->tx_vq)) {
return num_packets;
}
--
2.32.0.3.g01195cf9f
Re: [PATCH 2/3] virtio: struct VirtQueue introduce reset
On Sat, 28 Jan 2023 05:22:05 -0500, "Michael S. Tsirkin"
wrote:
> On Sat, Jan 28, 2023 at 03:17:23PM +0800, Xuan Zhuo wrote:
> > In the current design, we stop the device from operating on the vring
> > during per-queue reset by resetting the structure VirtQueue.
> >
> > But before the reset operation, when recycling some resources, we should
> > stop referencing new vring resources. For example, when recycling
> > virtio-net's asynchronous sending resources, virtio-net should be able
> > to perceive that the current queue is in the per-queue reset state, and
> > stop sending new packets from the tx queue.
> >
> > Signed-off-by: Xuan Zhuo
> > ---
> > hw/virtio/virtio.c | 8
> > include/hw/virtio/virtio.h | 3 +++
> > 2 files changed, 11 insertions(+)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 03077b2ecf..907d5b8bde 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -2030,6 +2030,12 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t
> > queue_index)
> > {
> > VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> >
> > +/*
> > + * Mark this queue is per-queue reset status. The device should
> > release the
> > + * references of the vring, and not refer more new vring item.
> > + */
> > +vdev->vq[queue_index].reset = true;
> > +
> > if (k->queue_reset) {
> > k->queue_reset(vdev, queue_index);
> > }
> > @@ -2053,6 +2059,8 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t
> > queue_index)
> > }
> > */
> >
> > +vdev->vq[queue_index].reset = false;
> > +
> > if (k->queue_enable) {
> > k->queue_enable(vdev, queue_index);
> > }
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 1c0d77c670..b888538d09 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -251,6 +251,9 @@ struct VirtQueue {
> > /* Notification enabled? */
> > bool notification;
> >
> > +/* Per-Queue Reset status */
> > +bool reset;
> > +
> > uint16_t queue_index;
> >
>
> Reset state makes no sense. It seems to imply queue_reset
> in the spec. And for extra fun there's "reset" in the pci
> proxy which means "virtio_queue_reset is in progress" - I have no
> idea what uses it though - it is not guest visible. First what is it?
> It actually means "queue has been reset and not has not been enabled since".
> So disabled_by_reset maybe?
In fact, when reading this, the queue has not been reset,
so prepare_for_reset?
>
> Second this hack helps make the change minimal
> so it's helpful for stable, but it's ugly in that it
> duplicates the reverse of enabled value - we don't really
> care what disabled it in practice.
>
> With the fixups above I can apply so it's easier to backport, but later
> a patch on top should clean it all up, perhaps by adding
> "enabled" in VirtQueue. We should also get rid of "reset" in the proxy
> unless there's some way it's useful which I don't currently see.
>
I have some confusion, I don't understand what you mean.
Why did we remove the "reset" in the proxy?
I agree to rename the "reset".
Thanks.
>
>
> > unsigned int inuse;
> > --
> > 2.32.0.3.g01195cf9f
>
Re: [PATCH 1/3] virtio: move struct VirtQueue to include file
On Sat, 28 Jan 2023 05:23:46 -0500, "Michael S. Tsirkin"
wrote:
> On Sat, Jan 28, 2023 at 03:17:22PM +0800, Xuan Zhuo wrote:
> > This patch move struct VirtQueue into virtio.h.
> >
> > In order to implement Queue Reset, we have to record the queue reset
> > status of in struct VirtQueue and provide it to device.
> >
> > Signed-off-by: Xuan Zhuo
>
> So add an API please, no need to move the struct.
> This patch will go away then.
OK.
Thanks.
>
> > ---
> > hw/virtio/virtio.c | 49 ---
> > include/hw/virtio/virtio.h | 52 --
> > 2 files changed, 50 insertions(+), 51 deletions(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index f35178f5fc..03077b2ecf 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -101,60 +101,11 @@ typedef struct VRingMemoryRegionCaches {
> > MemoryRegionCache used;
> > } VRingMemoryRegionCaches;
> >
> > -typedef struct VRing
> > -{
> > -unsigned int num;
> > -unsigned int num_default;
> > -unsigned int align;
> > -hwaddr desc;
> > -hwaddr avail;
> > -hwaddr used;
> > -VRingMemoryRegionCaches *caches;
> > -} VRing;
> > -
> > typedef struct VRingPackedDescEvent {
> > uint16_t off_wrap;
> > uint16_t flags;
> > } VRingPackedDescEvent ;
> >
> > -struct VirtQueue
> > -{
> > -VRing vring;
> > -VirtQueueElement *used_elems;
> > -
> > -/* Next head to pop */
> > -uint16_t last_avail_idx;
> > -bool last_avail_wrap_counter;
> > -
> > -/* Last avail_idx read from VQ. */
> > -uint16_t shadow_avail_idx;
> > -bool shadow_avail_wrap_counter;
> > -
> > -uint16_t used_idx;
> > -bool used_wrap_counter;
> > -
> > -/* Last used index value we have signalled on */
> > -uint16_t signalled_used;
> > -
> > -/* Last used index value we have signalled on */
> > -bool signalled_used_valid;
> > -
> > -/* Notification enabled? */
> > -bool notification;
> > -
> > -uint16_t queue_index;
> > -
> > -unsigned int inuse;
> > -
> > -uint16_t vector;
> > -VirtIOHandleOutput handle_output;
> > -VirtIODevice *vdev;
> > -EventNotifier guest_notifier;
> > -EventNotifier host_notifier;
> > -bool host_notifier_enabled;
> > -QLIST_ENTRY(VirtQueue) node;
> > -};
> > -
> > const char *virtio_device_names[] = {
> > [VIRTIO_ID_NET] = "virtio-net",
> > [VIRTIO_ID_BLOCK] = "virtio-blk",
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 77c6c55929..1c0d77c670 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -214,6 +214,56 @@ struct VirtioDeviceClass {
> > struct vhost_dev *(*get_vhost)(VirtIODevice *vdev);
> > };
> >
> > +typedef struct VRingMemoryRegionCaches VRingMemoryRegionCaches;
> > +typedef void (*VirtIOHandleOutput)(VirtIODevice *, VirtQueue *);
> > +
> > +typedef struct VRing {
> > +unsigned int num;
> > +unsigned int num_default;
> > +unsigned int align;
> > +hwaddr desc;
> > +hwaddr avail;
> > +hwaddr used;
> > +VRingMemoryRegionCaches *caches;
> > +} VRing;
> > +
> > +struct VirtQueue {
> > +VRing vring;
> > +VirtQueueElement *used_elems;
> > +
> > +/* Next head to pop */
> > +uint16_t last_avail_idx;
> > +bool last_avail_wrap_counter;
> > +
> > +/* Last avail_idx read from VQ. */
> > +uint16_t shadow_avail_idx;
> > +bool shadow_avail_wrap_counter;
> > +
> > +uint16_t used_idx;
> > +bool used_wrap_counter;
> > +
> > +/* Last used index value we have signalled on */
> > +uint16_t signalled_used;
> > +
> > +/* Last used index value we have signalled on */
> > +bool signalled_used_valid;
> > +
> > +/* Notification enabled? */
> > +bool notification;
> > +
> > +uint16_t queue_index;
> > +
> > +unsigned int inuse;
> > +
> > +uint16_t vector;
> > +VirtIOHandleOutput handle_output;
> > +VirtIODevice *vdev;
> > +EventNotifier guest_notifier;
> > +EventNotifier host_notifier;
> > +bool host_notifier_enabled;
> > +QLIST_ENTRY(VirtQueue) node;
> > +};
> > +
> > void virtio_instance_init_common(Object *proxy_obj, void *data,
> > size_t vdev_size, const char *vdev_name);
> >
> > @@ -226,8 +276,6 @@ void virtio_error(VirtIODevice *vdev, const char *fmt,
> > ...) G_GNUC_PRINTF(2, 3);
> > /* Set the child bus name. */
> > void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
> >
> > -typedef void (*VirtIOHandleOutput)(VirtIODevice *, VirtQueue *);
> > -
> > VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
> > VirtIOHandleOutput handle_output);
> >
> > --
> > 2.32.0.3.g01195cf9f
>
[PATCH 1/3] virtio: move struct VirtQueue to include file
This patch move struct VirtQueue into virtio.h.
In order to implement Queue Reset, we have to record the queue reset
status of in struct VirtQueue and provide it to device.
Signed-off-by: Xuan Zhuo
---
hw/virtio/virtio.c | 49 ---
include/hw/virtio/virtio.h | 52 --
2 files changed, 50 insertions(+), 51 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f35178f5fc..03077b2ecf 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -101,60 +101,11 @@ typedef struct VRingMemoryRegionCaches {
MemoryRegionCache used;
} VRingMemoryRegionCaches;
-typedef struct VRing
-{
-unsigned int num;
-unsigned int num_default;
-unsigned int align;
-hwaddr desc;
-hwaddr avail;
-hwaddr used;
-VRingMemoryRegionCaches *caches;
-} VRing;
-
typedef struct VRingPackedDescEvent {
uint16_t off_wrap;
uint16_t flags;
} VRingPackedDescEvent ;
-struct VirtQueue
-{
-VRing vring;
-VirtQueueElement *used_elems;
-
-/* Next head to pop */
-uint16_t last_avail_idx;
-bool last_avail_wrap_counter;
-
-/* Last avail_idx read from VQ. */
-uint16_t shadow_avail_idx;
-bool shadow_avail_wrap_counter;
-
-uint16_t used_idx;
-bool used_wrap_counter;
-
-/* Last used index value we have signalled on */
-uint16_t signalled_used;
-
-/* Last used index value we have signalled on */
-bool signalled_used_valid;
-
-/* Notification enabled? */
-bool notification;
-
-uint16_t queue_index;
-
-unsigned int inuse;
-
-uint16_t vector;
-VirtIOHandleOutput handle_output;
-VirtIODevice *vdev;
-EventNotifier guest_notifier;
-EventNotifier host_notifier;
-bool host_notifier_enabled;
-QLIST_ENTRY(VirtQueue) node;
-};
-
const char *virtio_device_names[] = {
[VIRTIO_ID_NET] = "virtio-net",
[VIRTIO_ID_BLOCK] = "virtio-blk",
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 77c6c55929..1c0d77c670 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -214,6 +214,56 @@ struct VirtioDeviceClass {
struct vhost_dev *(*get_vhost)(VirtIODevice *vdev);
};
+typedef struct VRingMemoryRegionCaches VRingMemoryRegionCaches;
+typedef void (*VirtIOHandleOutput)(VirtIODevice *, VirtQueue *);
+
+typedef struct VRing {
+unsigned int num;
+unsigned int num_default;
+unsigned int align;
+hwaddr desc;
+hwaddr avail;
+hwaddr used;
+VRingMemoryRegionCaches *caches;
+} VRing;
+
+struct VirtQueue {
+VRing vring;
+VirtQueueElement *used_elems;
+
+/* Next head to pop */
+uint16_t last_avail_idx;
+bool last_avail_wrap_counter;
+
+/* Last avail_idx read from VQ. */
+uint16_t shadow_avail_idx;
+bool shadow_avail_wrap_counter;
+
+uint16_t used_idx;
+bool used_wrap_counter;
+
+/* Last used index value we have signalled on */
+uint16_t signalled_used;
+
+/* Last used index value we have signalled on */
+bool signalled_used_valid;
+
+/* Notification enabled? */
+bool notification;
+
+uint16_t queue_index;
+
+unsigned int inuse;
+
+uint16_t vector;
+VirtIOHandleOutput handle_output;
+VirtIODevice *vdev;
+EventNotifier guest_notifier;
+EventNotifier host_notifier;
+bool host_notifier_enabled;
+QLIST_ENTRY(VirtQueue) node;
+};
+
void virtio_instance_init_common(Object *proxy_obj, void *data,
size_t vdev_size, const char *vdev_name);
@@ -226,8 +276,6 @@ void virtio_error(VirtIODevice *vdev, const char *fmt, ...)
G_GNUC_PRINTF(2, 3);
/* Set the child bus name. */
void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
-typedef void (*VirtIOHandleOutput)(VirtIODevice *, VirtQueue *);
-
VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
VirtIOHandleOutput handle_output);
--
2.32.0.3.g01195cf9f
[PATCH 0/3] virtio: fix for assertion failure: virtio_net_get_subqueue(nc)->async_tx.elem failed
In the current design, we stop the device from operating on the vring during per-queue reset by resetting the structure VirtQueue. But before the reset operation, when recycling some resources, we should stop referencing new vring resources. This bug is caused by this reason. https://gitlab.com/qemu-project/qemu/-/issues/1451 Before we reset the structure, we called the ->queue_reset callback to let the device reclaim resources. Here virtio-net tries to release the packets sent asynchronously, but during this process virtio_net_flush_tx() will be called, and new data will be sent again. This leads to asserted. assert(!virtio_net_get_subqueue(nc)->async_tx.elem); This patch set introduce new item "reset" into struct VirtQueue, then device can know this virtqueue is per-queue reset state. Xuan Zhuo (3): virtio: move struct VirtQueue to include file virtio: struct VirtQueue introduce reset virtio-net: virtio_net_flush_tx() check for per-queue reset hw/net/virtio-net.c| 2 +- hw/virtio/virtio.c | 57 ++ include/hw/virtio/virtio.h | 55 ++-- 3 files changed, 62 insertions(+), 52 deletions(-) -- 2.32.0.3.g01195cf9f
[PATCH 3/3] virtio-net: virtio_net_flush_tx() check for per-queue reset
Check whether it is per-queue reset state in virtio_net_flush_tx().
Before per-queue reset, we need to recover async tx resources. At this
time, virtio_net_flush_tx() is called, but we should not try to send
new packets, so virtio_net_flush_tx() should check the current
per-queue reset state.
Fixes: 7dc6be52 ("virtio-net: support queue reset")
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1451
Reported-by: Alexander Bulekov
Signed-off-by: Xuan Zhuo
---
hw/net/virtio-net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3ae909041a..e7e651e915 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2627,7 +2627,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
VirtQueueElement *elem;
int32_t num_packets = 0;
int queue_index = vq2q(virtio_get_queue_index(q->tx_vq));
-if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) || q->tx_vq->reset) {
return num_packets;
}
--
2.32.0.3.g01195cf9f
[PATCH 2/3] virtio: struct VirtQueue introduce reset
In the current design, we stop the device from operating on the vring
during per-queue reset by resetting the structure VirtQueue.
But before the reset operation, when recycling some resources, we should
stop referencing new vring resources. For example, when recycling
virtio-net's asynchronous sending resources, virtio-net should be able
to perceive that the current queue is in the per-queue reset state, and
stop sending new packets from the tx queue.
Signed-off-by: Xuan Zhuo
---
hw/virtio/virtio.c | 8
include/hw/virtio/virtio.h | 3 +++
2 files changed, 11 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 03077b2ecf..907d5b8bde 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2030,6 +2030,12 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t
queue_index)
{
VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+/*
+ * Mark this queue is per-queue reset status. The device should release the
+ * references of the vring, and not refer more new vring item.
+ */
+vdev->vq[queue_index].reset = true;
+
if (k->queue_reset) {
k->queue_reset(vdev, queue_index);
}
@@ -2053,6 +2059,8 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t
queue_index)
}
*/
+vdev->vq[queue_index].reset = false;
+
if (k->queue_enable) {
k->queue_enable(vdev, queue_index);
}
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 1c0d77c670..b888538d09 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -251,6 +251,9 @@ struct VirtQueue {
/* Notification enabled? */
bool notification;
+/* Per-Queue Reset status */
+bool reset;
+
uint16_t queue_index;
unsigned int inuse;
--
2.32.0.3.g01195cf9f
[PATCH v1] virtio-net: fix for heap-buffer-overflow
Run shell script:
cat << EOF | valgrind qemu-system-i386 -display none -machine accel=qtest,
-m \
512M -M q35 -nodefaults -device virtio-net,netdev=net0 -netdev \
user,id=net0 -qtest stdio
outl 0xcf8 0x8810
outl 0xcfc 0xc000
outl 0xcf8 0x8804
outl 0xcfc 0x01
outl 0xc00d 0x0200
outl 0xcf8 0x8890
outb 0xcfc 0x4
outl 0xcf8 0x8889
outl 0xcfc 0x1c00
outl 0xcf8 0x8893
outw 0xcfc 0x100
EOF
Got:
==68666== Invalid read of size 8
==68666==at 0x688536: virtio_net_queue_enable (virtio-net.c:575)
==68666==by 0x6E31AE: memory_region_write_accessor (memory.c:492)
==68666==by 0x6E098D: access_with_adjusted_size (memory.c:554)
==68666==by 0x6E4DB3: memory_region_dispatch_write (memory.c:1521)
==68666==by 0x6E31AE: memory_region_write_accessor (memory.c:492)
==68666==by 0x6E098D: access_with_adjusted_size (memory.c:554)
==68666==by 0x6E4DB3: memory_region_dispatch_write (memory.c:1521)
==68666==by 0x6EBCD3: flatview_write_continue (physmem.c:2820)
==68666==by 0x6EBFBF: flatview_write (physmem.c:2862)
==68666==by 0x6EF5E7: address_space_write (physmem.c:2958)
==68666==by 0x6DFDEC: cpu_outw (ioport.c:70)
==68666==by 0x6F6DF0: qtest_process_command (qtest.c:480)
==68666== Address 0x29087fe8 is 24 bytes after a block of size 416 in
arena "client"
That is reported by Alexander Bulekov.
https://gitlab.com/qemu-project/qemu/-/issues/1309
Here, the queue_index is the index of the cvq, but in some cases cvq
does not have the corresponding NetClientState, so overflow appears.
I add a check here, ignore illegal queue_index and cvq queue_index.
Note the queue_index is below the VIRTIO_QUEUE_MAX but greater or equal
than cvq index could hit this. Other devices are similar.
Signed-off-by: Xuan Zhuo
---
hw/net/virtio-net.c| 18 --
include/hw/virtio/virtio.h | 2 ++
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 975bbc22f9..88f25709d6 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -549,7 +549,14 @@ static RxFilterInfo
*virtio_net_query_rxfilter(NetClientState *nc)
static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
{
VirtIONet *n = VIRTIO_NET(vdev);
-NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
+NetClientState *nc;
+
+/* validate queue_index and skip for cvq */
+if (queue_index >= n->max_queue_pairs * 2) {
+return;
+}
+
+nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
if (!nc->peer) {
return;
@@ -566,9 +573,16 @@ static void virtio_net_queue_reset(VirtIODevice *vdev,
uint32_t queue_index)
static void virtio_net_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
{
VirtIONet *n = VIRTIO_NET(vdev);
-NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
+NetClientState *nc;
int r;
+/* validate queue_index and skip for cvq */
+if (queue_index >= n->max_queue_pairs * 2) {
+return;
+}
+
+nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
+
if (!nc->peer || !vdev->vhost_started) {
return;
}
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 1423dba379..6e7c0c09f7 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -148,7 +148,9 @@ struct VirtioDeviceClass {
void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
void (*reset)(VirtIODevice *vdev);
void (*set_status)(VirtIODevice *vdev, uint8_t val);
+/* Device must validate queue_index. */
void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
+/* Device must validate queue_index. */
void (*queue_enable)(VirtIODevice *vdev, uint32_t queue_index);
/* For transitional devices, this is a bitmap of features
* that are only exposed on the legacy interface but not
--
2.32.0.3.g01195cf9f
Re: [PATCH] virtio-net: fix for heap-buffer-overflow
On Thu, 10 Nov 2022 17:18:23 +0800, Jason Wang wrote:
> On Thu, Nov 10, 2022 at 4:28 PM Xuan Zhuo wrote:
> >
> > Run shell script:
> >
> > cat << EOF | valgrind qemu-system-i386 -display none -machine
> > accel=qtest, -m \
> > 512M -M q35 -nodefaults -device virtio-net,netdev=net0 -netdev \
> > user,id=net0 -qtest stdio
> > outl 0xcf8 0x8810
> > outl 0xcfc 0xc000
> > outl 0xcf8 0x8804
> > outl 0xcfc 0x01
> > outl 0xc00d 0x0200
> > outl 0xcf8 0x8890
> > outb 0xcfc 0x4
> > outl 0xcf8 0x8889
> > outl 0xcfc 0x1c00
> > outl 0xcf8 0x8893
> > outw 0xcfc 0x100
> > EOF
> >
> > Got:
> > ==68666== Invalid read of size 8
> > ==68666==at 0x688536: virtio_net_queue_enable (virtio-net.c:575)
> > ==68666==by 0x6E31AE: memory_region_write_accessor (memory.c:492)
> > ==68666==by 0x6E098D: access_with_adjusted_size (memory.c:554)
> > ==68666==by 0x6E4DB3: memory_region_dispatch_write (memory.c:1521)
> > ==68666==by 0x6E31AE: memory_region_write_accessor (memory.c:492)
> > ==68666==by 0x6E098D: access_with_adjusted_size (memory.c:554)
> > ==68666==by 0x6E4DB3: memory_region_dispatch_write (memory.c:1521)
> > ==68666==by 0x6EBCD3: flatview_write_continue (physmem.c:2820)
> > ==68666==by 0x6EBFBF: flatview_write (physmem.c:2862)
> > ==68666==by 0x6EF5E7: address_space_write (physmem.c:2958)
> > ==68666==by 0x6DFDEC: cpu_outw (ioport.c:70)
> > ==68666==by 0x6F6DF0: qtest_process_command (qtest.c:480)
> > ==68666== Address 0x29087fe8 is 24 bytes after a block of size 416 in
> > arena "client"
> >
> > That is reported by Alexander Bulekov.
> > https://gitlab.com/qemu-project/qemu/-/issues/1309
> >
> > Here, the queue_index is the index of the cvq, but cvq does not have the
> > corresponding NetClientState,
>
> This is not necessarily truth for some backends like vhost-vDPA.
Oh, I ignored it.
>
> > so overflow appears.
>
> Note that this is guest trigger-able, so anything that is below the
> VIRTIO_QUEUE_MAX but greater or equal than cvq index could hit this.
Yes
>
> >
> > I add a check here, ignore illegal queue_index and cvq queue_index.
> >
> > Signed-off-by: Xuan Zhuo
> > ---
> > hw/net/virtio-net.c | 18 --
> > 1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 975bbc22f9..88f25709d6 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -549,7 +549,14 @@ static RxFilterInfo
> > *virtio_net_query_rxfilter(NetClientState *nc)
> > static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t
> > queue_index)
>
> If we require the VirtioDeviceClass to validate the index, let's add a
> document there. Or we can let the transport to validate this.
My understanding, it can not be verified in transport for the time being, so
add some instructions first.
Thanks.
>
> Thanks
>
> > {
> > VirtIONet *n = VIRTIO_NET(vdev);
> > -NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> > +NetClientState *nc;
> > +
> > +/* validate queue_index and skip for cvq */
> > +if (queue_index >= n->max_queue_pairs * 2) {
> > +return;
> > +}
> > +
> > +nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> >
> > if (!nc->peer) {
> > return;
> > @@ -566,9 +573,16 @@ static void virtio_net_queue_reset(VirtIODevice *vdev,
> > uint32_t queue_index)
> > static void virtio_net_queue_enable(VirtIODevice *vdev, uint32_t
> > queue_index)
> > {
> > VirtIONet *n = VIRTIO_NET(vdev);
> > -NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> > +NetClientState *nc;
> > int r;
> >
> > +/* validate queue_index and skip for cvq */
> > +if (queue_index >= n->max_queue_pairs * 2) {
> > +return;
> > +}
> > +
> > +nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> > +
> > if (!nc->peer || !vdev->vhost_started) {
> > return;
> > }
> > --
> > 2.32.0.3.g01195cf9f
> >
>
[PATCH] virtio-net: fix for heap-buffer-overflow
Run shell script:
cat << EOF | valgrind qemu-system-i386 -display none -machine accel=qtest,
-m \
512M -M q35 -nodefaults -device virtio-net,netdev=net0 -netdev \
user,id=net0 -qtest stdio
outl 0xcf8 0x8810
outl 0xcfc 0xc000
outl 0xcf8 0x8804
outl 0xcfc 0x01
outl 0xc00d 0x0200
outl 0xcf8 0x8890
outb 0xcfc 0x4
outl 0xcf8 0x8889
outl 0xcfc 0x1c00
outl 0xcf8 0x8893
outw 0xcfc 0x100
EOF
Got:
==68666== Invalid read of size 8
==68666==at 0x688536: virtio_net_queue_enable (virtio-net.c:575)
==68666==by 0x6E31AE: memory_region_write_accessor (memory.c:492)
==68666==by 0x6E098D: access_with_adjusted_size (memory.c:554)
==68666==by 0x6E4DB3: memory_region_dispatch_write (memory.c:1521)
==68666==by 0x6E31AE: memory_region_write_accessor (memory.c:492)
==68666==by 0x6E098D: access_with_adjusted_size (memory.c:554)
==68666==by 0x6E4DB3: memory_region_dispatch_write (memory.c:1521)
==68666==by 0x6EBCD3: flatview_write_continue (physmem.c:2820)
==68666==by 0x6EBFBF: flatview_write (physmem.c:2862)
==68666==by 0x6EF5E7: address_space_write (physmem.c:2958)
==68666==by 0x6DFDEC: cpu_outw (ioport.c:70)
==68666==by 0x6F6DF0: qtest_process_command (qtest.c:480)
==68666== Address 0x29087fe8 is 24 bytes after a block of size 416 in
arena "client"
That is reported by Alexander Bulekov.
https://gitlab.com/qemu-project/qemu/-/issues/1309
Here, the queue_index is the index of the cvq, but cvq does not have the
corresponding NetClientState, so overflow appears.
I add a check here, ignore illegal queue_index and cvq queue_index.
Signed-off-by: Xuan Zhuo
---
hw/net/virtio-net.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 975bbc22f9..88f25709d6 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -549,7 +549,14 @@ static RxFilterInfo
*virtio_net_query_rxfilter(NetClientState *nc)
static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
{
VirtIONet *n = VIRTIO_NET(vdev);
-NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
+NetClientState *nc;
+
+/* validate queue_index and skip for cvq */
+if (queue_index >= n->max_queue_pairs * 2) {
+return;
+}
+
+nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
if (!nc->peer) {
return;
@@ -566,9 +573,16 @@ static void virtio_net_queue_reset(VirtIODevice *vdev,
uint32_t queue_index)
static void virtio_net_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
{
VirtIONet *n = VIRTIO_NET(vdev);
-NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
+NetClientState *nc;
int r;
+/* validate queue_index and skip for cvq */
+if (queue_index >= n->max_queue_pairs * 2) {
+return;
+}
+
+nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
+
if (!nc->peer || !vdev->vhost_started) {
return;
}
--
2.32.0.3.g01195cf9f
Re: QEMU | Heap-overflow in virtio_net_queue_enable (#1309)
On Thu, 10 Nov 2022 13:18:54 +0800, Jason Wang wrote:
> On Thu, Nov 10, 2022 at 1:12 PM Michael S. Tsirkin wrote:
> >
> > Xuan Zhuo pls take a look ASAP.
> >
> > On Thu, Nov 10, 2022 at 03:04:41AM +, Alexander Bulekov (@a1xndr) wrote:
> > Alexander Bulekov created an issue: #1309
> >
> > Hello,
> >
> > I bisected this to 7f863302 ("virtio-net: support queue_enable"). CC:
> > @mstredhat @jasowang (could not find Kangjie Xu or Xuan Zhuo gitlab
> > accounts).
>
> Looks like we need to validate queue_index or queue_sel before calling
> device specific queue enable here.
The queue_index here should be index of the cvq.
We cannot check on the upper level, because there is no actual queue num in the
upper layer VirtIODevice.
Thanks.
>
> Thanks
>
> >
> > Reproducer
> >
> > cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \
> > 512M -M q35 -nodefaults -device virtio-net,netdev=net0 -netdev \
> > user,id=net0 -qtest stdio
> > outl 0xcf8 0x8810
> > outl 0xcfc 0xc000
> > outl 0xcf8 0x8804
> > outl 0xcfc 0x01
> > outl 0xc00d 0x0200
> > outl 0xcf8 0x8890
> > outb 0xcfc 0x4
> > outl 0xcf8 0x8889
> > outl 0xcfc 0x1c00
> > outl 0xcf8 0x8893
> > outw 0xcfc 0x100
> > EOF
> >
> > Stack-Trace
> >
> > ==374==ERROR: AddressSanitizer: heap-buffer-overflow on address
> > 0x6141a9f8 at pc 0x55db851032b1 bp 0x7ffe639914c0 sp 0x7ffe639914b8
> > READ of size 8 at 0x6141a9f8 thread T0
> > #0 0x55db851032b0 in virtio_net_queue_enable
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../hw/net/virtio-net.c:572:14
> > #1 0x55db85361748 in memory_region_write_accessor
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/memory.c:493:5
> > #2 0x55db8536129a in access_with_adjusted_size
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/memory.c:555:18
> > #3 0x55db85360c03 in memory_region_dispatch_write
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/memory.c
> > #4 0x55db8485e11f in virtio_address_space_write
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../hw/virtio/virtio-pci.c:592:5
> > #5 0x55db8485e11f in virtio_write_config
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../hw/virtio/virtio-pci.c:670:13
> > #6 0x55db844de82a in pci_host_config_write_common
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../hw/pci/pci_host.c:85:5
> > #7 0x55db85361748 in memory_region_write_accessor
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/memory.c:493:5
> > #8 0x55db8536129a in access_with_adjusted_size
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/memory.c:555:18
> > #9 0x55db85360c03 in memory_region_dispatch_write
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/memory.c
> > #10 0x55db853ad390 in flatview_write_continue
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/physmem.c:2825:23
> > #11 0x55db853a4833 in flatview_write
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/physmem.c:2867:12
> > #12 0x55db853a4543 in address_space_write
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/physmem.c:2963:18
> > #13 0x55db85354567 in cpu_outw
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/ioport.c:70:5
> > #14 0x55db853b8129 in qtest_process_command
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/qtest.c:480:13
> > #15 0x55db853b6cb8 in qtest_process_inbuf
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/qtest.c:802:9
> > #16 0x55db85a3e284 in fd_chr_read
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../chardev/char-fd.c:72:9
> > #17 0x7f7f528c8a9e in g_main_context_dispatch
> > (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x53a9e) (BuildId:
> > 1697a734f1bc7448cd8772689a1c439343f062f7)
> > #18 0x55db85cc1f33 in glib_pollfds_poll
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../util/main-loop.c:297:9
> > #19 0x55db85cc1f33 in os_host_main_loop_wait
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../util/main-loop.c:320:5
> > #20 0x55db85cc1f33 in main_loop_wait
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../util/main-loop.c:606:11
> > #21 0x55db849163a6 in qemu_main_loop
> > /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/runstate.c:739:9
> > #22 0x55db83d54105 in q
Re: QEMU | Heap-overflow in virtio_net_queue_enable (#1309)
On Thu, 10 Nov 2022 00:11:00 -0500, "Michael S. Tsirkin"
wrote:
> Xuan Zhuo pls take a look ASAP.
>
> On Thu, Nov 10, 2022 at 03:04:41AM +, Alexander Bulekov (@a1xndr) wrote:
> Alexander Bulekov created an issue: #1309
>
> Hello,
>
> I bisected this to 7f863302 ("virtio-net: support queue_enable"). CC:
> @mstredhat @jasowang (could not find Kangjie Xu or Xuan Zhuo gitlab accounts).
>
> Reproducer
>
> cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \
> 512M -M q35 -nodefaults -device virtio-net,netdev=net0 -netdev \
> user,id=net0 -qtest stdio
> outl 0xcf8 0x8810
> outl 0xcfc 0xc000
> outl 0xcf8 0x8804
> outl 0xcfc 0x01
> outl 0xc00d 0x0200
> outl 0xcf8 0x8890
> outb 0xcfc 0x4
> outl 0xcf8 0x8889
> outl 0xcfc 0x1c00
> outl 0xcf8 0x8893
> outw 0xcfc 0x100
> EOF
Hi, I don't reproduce this problem, need valgrind?
Thanks.
sudo sh test.sh
[I 0.00] OPENED
[R +0.014069] outl 0xcf8 0x8810
[S +0.014089] OK
OK
[R +0.014100] outl 0xcfc 0xc000
[S +0.014113] OK
OK
[R +0.014117] outl 0xcf8 0x8804
[S +0.014125] OK
OK
[R +0.014133] outl 0xcfc 0x01
[S +0.014210] OK
OK
[R +0.014215] outl 0xc00d 0x0200
[S +0.014222] OK
OK
[R +0.014226] outl 0xcf8 0x8890
[S +0.014233] OK
OK
[R +0.014240] outb 0xcfc 0x4
[S +0.014247] OK
OK
[R +0.014252] outl 0xcf8 0x8889
[S +0.014259] OK
OK
[R +0.014266] outl 0xcfc 0x1c00
[S +0.014275] OK
OK
[R +0.014279] outl 0xcf8 0x8893
[S +0.014288] OK
OK
[R +0.014292] outw 0xcfc 0x100
[S +0.014304] OK
OK
[I +0.014319] CLOSED
^Cqemu-system-i386: GLib: g_timer_elapsed: assertion 'timer != NULL'
failed
[I +0.00] CLOSED
>
> Stack-Trace
>
> ==374==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x6141a9f8 at pc 0x55db851032b1 bp 0x7ffe639914c0 sp 0x7ffe639914b8
> READ of size 8 at 0x6141a9f8 thread T0
> #0 0x55db851032b0 in virtio_net_queue_enable
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../hw/net/virtio-net.c:572:14
> #1 0x55db85361748 in memory_region_write_accessor
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/memory.c:493:5
> #2 0x55db8536129a in access_with_adjusted_size
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/memory.c:555:18
> #3 0x55db85360c03 in memory_region_dispatch_write
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/memory.c
> #4 0x55db8485e11f in virtio_address_space_write
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../hw/virtio/virtio-pci.c:592:5
> #5 0x55db8485e11f in virtio_write_config
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../hw/virtio/virtio-pci.c:670:13
> #6 0x55db844de82a in pci_host_config_write_common
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../hw/pci/pci_host.c:85:5
> #7 0x55db85361748 in memory_region_write_accessor
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/memory.c:493:5
> #8 0x55db8536129a in access_with_adjusted_size
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/memory.c:555:18
> #9 0x55db85360c03 in memory_region_dispatch_write
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/memory.c
> #10 0x55db853ad390 in flatview_write_continue
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/physmem.c:2825:23
> #11 0x55db853a4833 in flatview_write
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/physmem.c:2867:12
> #12 0x55db853a4543 in address_space_write
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/physmem.c:2963:18
> #13 0x55db85354567 in cpu_outw
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/ioport.c:70:5
> #14 0x55db853b8129 in qtest_process_command
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/qtest.c:480:13
> #15 0x55db853b6cb8 in qtest_process_inbuf
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../softmmu/qtest.c:802:9
> #16 0x55db85a3e284 in fd_chr_read
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../chardev/char-fd.c:72:9
> #17 0x7f7f528c8a9e in g_main_context_dispatch
> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x53a9e) (BuildId:
> 1697a734f1bc7448cd8772689a1c439343f062f7)
> #18 0x55db85cc1f33 in glib_pollfds_poll
> /home/alxndr/Development/qemu-demo/qemu/build-asan/../util/main-loop.c:297:9
> #19 0x55db85cc1f33 in os_host_main_loop_wait
> /home/alxnd
Re: [PATCH] virtio: remove the excess virtio features check
On Wed, 9 Nov 2022 09:46:18 -0500, "Michael S. Tsirkin" wrote:
> On Wed, Nov 09, 2022 at 07:10:21PM +0800, Xuan Zhuo wrote:
> > In virtio_queue_enable(), we checked virtio feature VIRTIO_F_VERSION_1.
> >
> > This check is not necessary, and conflict with SeaBIOS. The problem
> > appeared in SeaBIOS. But we also remove this check.
> >
> > Link: https://www.mail-archive.com/qemu-devel@nongnu.org/msg920538.html
> > Signed-off-by: Xuan Zhuo
> > ---
> > hw/virtio/virtio.c | 5 -
> > 1 file changed, 5 deletions(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 9683b2e158..701e23ea6a 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -2499,11 +2499,6 @@ void virtio_queue_enable(VirtIODevice *vdev,
> > uint32_t queue_index)
> > {
> > VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> >
> > -if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> > -error_report("queue_enable is only suppported in devices of virtio
> > "
> > - "1.0 or later.");
> > -}
> > -
> > if (k->queue_enable) {
> > k->queue_enable(vdev, queue_index);
> > }
>
> Well this warning turned out to be actually useful.
> Let's see whether we can fix seabios in time for release.
> If yes I would just make it LOG_GUEST_ERR and limit to
> latest machine type but not drop completely.
OK.
>
> > --
> > 2.32.0.3.g01195cf9f
>
[PATCH] virtio: remove the excess virtio features check
In virtio_queue_enable(), we checked virtio feature VIRTIO_F_VERSION_1.
This check is not necessary, and conflict with SeaBIOS. The problem
appeared in SeaBIOS. But we also remove this check.
Link: https://www.mail-archive.com/qemu-devel@nongnu.org/msg920538.html
Signed-off-by: Xuan Zhuo
---
hw/virtio/virtio.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 9683b2e158..701e23ea6a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2499,11 +2499,6 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t
queue_index)
{
VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
-if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
-error_report("queue_enable is only suppported in devices of virtio "
- "1.0 or later.");
-}
-
if (k->queue_enable) {
k->queue_enable(vdev, queue_index);
}
--
2.32.0.3.g01195cf9f
Re: [PULL v4 29/83] virtio: introduce virtio_queue_enable()
On Wed, 9 Nov 2022 02:04:17 -0500, "Michael S. Tsirkin" wrote:
> On Wed, Nov 09, 2022 at 02:56:01PM +0800, Xuan Zhuo wrote:
> > On Wed, 9 Nov 2022 14:55:03 +0800, Jason Wang wrote:
> > >
> > > 在 2022/11/9 14:51, Michael S. Tsirkin 写道:
> > > > On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
> > > >> On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi
> > > >> wrote:
> > > >>> On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin
> > > >>> wrote:
> > > >>>> From: Kangjie Xu
> > > >>>>
> > > >>>> Introduce the interface queue_enable() in VirtioDeviceClass and the
> > > >>>> fucntion virtio_queue_enable() in virtio, it can be called when
> > > >>>> VIRTIO_PCI_COMMON_Q_ENABLE is written and related virtqueue can be
> > > >>>> started. It only supports the devices of virtio 1 or later. The
> > > >>>> not-supported devices can only start the virtqueue when DRIVER_OK.
> > > >>>>
> > > >>>> Signed-off-by: Kangjie Xu
> > > >>>> Signed-off-by: Xuan Zhuo
> > > >>>> Acked-by: Jason Wang
> > > >>>> Message-Id: <20221017092558.111082-4-xuanz...@linux.alibaba.com>
> > > >>>> Reviewed-by: Michael S. Tsirkin
> > > >>>> Signed-off-by: Michael S. Tsirkin
> > > >>>> ---
> > > >>>> include/hw/virtio/virtio.h | 2 ++
> > > >>>> hw/virtio/virtio.c | 14 ++
> > > >>>> 2 files changed, 16 insertions(+)
> > > >>>>
> > > >>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > > >>>> index 74d76c1dbc..b00b3fcf31 100644
> > > >>>> --- a/include/hw/virtio/virtio.h
> > > >>>> +++ b/include/hw/virtio/virtio.h
> > > >>>> @@ -149,6 +149,7 @@ struct VirtioDeviceClass {
> > > >>>> void (*reset)(VirtIODevice *vdev);
> > > >>>> void (*set_status)(VirtIODevice *vdev, uint8_t val);
> > > >>>> void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
> > > >>>> +void (*queue_enable)(VirtIODevice *vdev, uint32_t queue_index);
> > > >>>> /* For transitional devices, this is a bitmap of features
> > > >>>>* that are only exposed on the legacy interface but not
> > > >>>>* the modern one.
> > > >>>> @@ -288,6 +289,7 @@ int
> > > >>>> virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
> > > >>>> int virtio_set_status(VirtIODevice *vdev, uint8_t val);
> > > >>>> void virtio_reset(void *opaque);
> > > >>>> void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
> > > >>>> +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
> > > >>>> void virtio_update_irq(VirtIODevice *vdev);
> > > >>>> int virtio_set_features(VirtIODevice *vdev, uint64_t val);
> > > >>>>
> > > >>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > >>>> index cf5f9ca387..9683b2e158 100644
> > > >>>> --- a/hw/virtio/virtio.c
> > > >>>> +++ b/hw/virtio/virtio.c
> > > >>>> @@ -2495,6 +2495,20 @@ void virtio_queue_reset(VirtIODevice *vdev,
> > > >>>> uint32_t queue_index)
> > > >>>> __virtio_queue_reset(vdev, queue_index);
> > > >>>> }
> > > >>>>
> > > >>>> +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
> > > >>>> +{
> > > >>>> +VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > > >>>> +
> > > >>>> +if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> > > >>>> +error_report("queue_enable is only suppported in devices of
> > > >>>> virtio "
> > > >>>> + "1.0 or later.");
> > > >>> Why is this triggering here? Maybe virtio_queue_enable() is called too
> > > >>> early. I have verified that the Linux guest driver sets VERSION_1. I
> > > >>> didn't check what SeaBIOS does.
> > > >> Looks like a bug, we should check device features here at least and it
> > > >> should be guest errors instead of error_report() here.
> > > >>
> > > >> Thanks
> > > > But it's weird, queue enable is written before guest features?
> > > > How come?
> > >
> > >
> > > Or queue_enable is written when the driver doesn't negotiate VERSION_1?
> >
> > Is this a bug?
> >
> > Or is it allowed in some cases?
> >
> > I feel weird too.
> >
> > Thanks.
>
> Weren't you able to reproduce?
> I suggest
> - write a bios patch to make it spec compliant
> - check UEFI to make sure it's spec compliant
> - ask bios/uefi maintainers whether they can include the patch for this
> release
It looks very interesting, I am happy to study it.
> - add a patch to drop the warning - we don't really need it
I sent this patch first.
Thanks.
>
>
> > >
> > > Thanks
> > >
> > >
> > > >
> > > >>> $ build/qemu-system-x86_64 -M accel=kvm -m 1G -cpu host -blockdev
> > > >>> file,node-name=drive0,filename=test.img -device
> > > >>> virtio-blk-pci,drive=drive0
> > > >>> qemu: queue_enable is only suppported in devices of virtio 1.0 or
> > > >>> later.
> > > >>>
> > > >>> Stefan
> > > >>>
> > >
>
Re: [PULL v4 29/83] virtio: introduce virtio_queue_enable()
On Wed, 9 Nov 2022 02:01:38 -0500, "Michael S. Tsirkin" wrote:
> On Wed, Nov 09, 2022 at 02:48:29PM +0800, Xuan Zhuo wrote:
> > On Wed, 9 Nov 2022 01:39:32 -0500, "Michael S. Tsirkin"
> > wrote:
> > > On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
> > > > On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi
> > > > wrote:
> > > > >
> > > > > On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin
> > > > > wrote:
> > > > > >
> > > > > > From: Kangjie Xu
> > > > > >
> > > > > > Introduce the interface queue_enable() in VirtioDeviceClass and the
> > > > > > fucntion virtio_queue_enable() in virtio, it can be called when
> > > > > > VIRTIO_PCI_COMMON_Q_ENABLE is written and related virtqueue can be
> > > > > > started. It only supports the devices of virtio 1 or later. The
> > > > > > not-supported devices can only start the virtqueue when DRIVER_OK.
> > > > > >
> > > > > > Signed-off-by: Kangjie Xu
> > > > > > Signed-off-by: Xuan Zhuo
> > > > > > Acked-by: Jason Wang
> > > > > > Message-Id: <20221017092558.111082-4-xuanz...@linux.alibaba.com>
> > > > > > Reviewed-by: Michael S. Tsirkin
> > > > > > Signed-off-by: Michael S. Tsirkin
> > > > > > ---
> > > > > > include/hw/virtio/virtio.h | 2 ++
> > > > > > hw/virtio/virtio.c | 14 ++
> > > > > > 2 files changed, 16 insertions(+)
> > > > > >
> > > > > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > > > > > index 74d76c1dbc..b00b3fcf31 100644
> > > > > > --- a/include/hw/virtio/virtio.h
> > > > > > +++ b/include/hw/virtio/virtio.h
> > > > > > @@ -149,6 +149,7 @@ struct VirtioDeviceClass {
> > > > > > void (*reset)(VirtIODevice *vdev);
> > > > > > void (*set_status)(VirtIODevice *vdev, uint8_t val);
> > > > > > void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
> > > > > > +void (*queue_enable)(VirtIODevice *vdev, uint32_t queue_index);
> > > > > > /* For transitional devices, this is a bitmap of features
> > > > > > * that are only exposed on the legacy interface but not
> > > > > > * the modern one.
> > > > > > @@ -288,6 +289,7 @@ int
> > > > > > virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
> > > > > > int virtio_set_status(VirtIODevice *vdev, uint8_t val);
> > > > > > void virtio_reset(void *opaque);
> > > > > > void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
> > > > > > +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
> > > > > > void virtio_update_irq(VirtIODevice *vdev);
> > > > > > int virtio_set_features(VirtIODevice *vdev, uint64_t val);
> > > > > >
> > > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > > > index cf5f9ca387..9683b2e158 100644
> > > > > > --- a/hw/virtio/virtio.c
> > > > > > +++ b/hw/virtio/virtio.c
> > > > > > @@ -2495,6 +2495,20 @@ void virtio_queue_reset(VirtIODevice *vdev,
> > > > > > uint32_t queue_index)
> > > > > > __virtio_queue_reset(vdev, queue_index);
> > > > > > }
> > > > > >
> > > > > > +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
> > > > > > +{
> > > > > > +VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > > > > > +
> > > > > > +if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> > > > > > +error_report("queue_enable is only suppported in devices
> > > > > > of virtio "
> > > > > > + "1.0 or later.");
> > > > >
> > > > > Why is this triggering here? Maybe virtio_queue_enable() is called too
> > > > > early. I have verified that the Linux guest driver sets VERSION_1. I
> > > > > didn't check what SeaBIOS does.
> > > >
> > > > Looks like a bug, we should check device features here at least and it
> > > > should be guest errors instead of error_report() here.
> > > >
> > > > Thanks
> > > >
> > >
> > > I suspect we should just drop this print. Kangjie?
> >
> >
> > I think it is.
> >
> > At that time, this inspection was only added at hand, and theoretically it
> > should not be performed.
> >
> > I am responsible for this patch set now.
> >
> > hi, Michael,
> >
> > What should I do, do I send a new version again?
> >
> > Thanks.
>
> I debugged it and replied separately. Can you check EFI drivers too?
OK.
Thanks.
>
> > >
> > >
> > > > > $ build/qemu-system-x86_64 -M accel=kvm -m 1G -cpu host -blockdev
> > > > > file,node-name=drive0,filename=test.img -device
> > > > > virtio-blk-pci,drive=drive0
> > > > > qemu: queue_enable is only suppported in devices of virtio 1.0 or
> > > > > later.
> > > > >
> > > > > Stefan
> > > > >
> > >
>
Re: [PULL v4 29/83] virtio: introduce virtio_queue_enable()
On Wed, 9 Nov 2022 14:55:03 +0800, Jason Wang wrote:
>
> 在 2022/11/9 14:51, Michael S. Tsirkin 写道:
> > On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
> >> On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi wrote:
> >>> On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin wrote:
> >>>> From: Kangjie Xu
> >>>>
> >>>> Introduce the interface queue_enable() in VirtioDeviceClass and the
> >>>> fucntion virtio_queue_enable() in virtio, it can be called when
> >>>> VIRTIO_PCI_COMMON_Q_ENABLE is written and related virtqueue can be
> >>>> started. It only supports the devices of virtio 1 or later. The
> >>>> not-supported devices can only start the virtqueue when DRIVER_OK.
> >>>>
> >>>> Signed-off-by: Kangjie Xu
> >>>> Signed-off-by: Xuan Zhuo
> >>>> Acked-by: Jason Wang
> >>>> Message-Id: <20221017092558.111082-4-xuanz...@linux.alibaba.com>
> >>>> Reviewed-by: Michael S. Tsirkin
> >>>> Signed-off-by: Michael S. Tsirkin
> >>>> ---
> >>>> include/hw/virtio/virtio.h | 2 ++
> >>>> hw/virtio/virtio.c | 14 ++
> >>>> 2 files changed, 16 insertions(+)
> >>>>
> >>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >>>> index 74d76c1dbc..b00b3fcf31 100644
> >>>> --- a/include/hw/virtio/virtio.h
> >>>> +++ b/include/hw/virtio/virtio.h
> >>>> @@ -149,6 +149,7 @@ struct VirtioDeviceClass {
> >>>> void (*reset)(VirtIODevice *vdev);
> >>>> void (*set_status)(VirtIODevice *vdev, uint8_t val);
> >>>> void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
> >>>> +void (*queue_enable)(VirtIODevice *vdev, uint32_t queue_index);
> >>>> /* For transitional devices, this is a bitmap of features
> >>>>* that are only exposed on the legacy interface but not
> >>>>* the modern one.
> >>>> @@ -288,6 +289,7 @@ int virtio_queue_set_host_notifier_mr(VirtIODevice
> >>>> *vdev, int n,
> >>>> int virtio_set_status(VirtIODevice *vdev, uint8_t val);
> >>>> void virtio_reset(void *opaque);
> >>>> void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
> >>>> +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
> >>>> void virtio_update_irq(VirtIODevice *vdev);
> >>>> int virtio_set_features(VirtIODevice *vdev, uint64_t val);
> >>>>
> >>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >>>> index cf5f9ca387..9683b2e158 100644
> >>>> --- a/hw/virtio/virtio.c
> >>>> +++ b/hw/virtio/virtio.c
> >>>> @@ -2495,6 +2495,20 @@ void virtio_queue_reset(VirtIODevice *vdev,
> >>>> uint32_t queue_index)
> >>>> __virtio_queue_reset(vdev, queue_index);
> >>>> }
> >>>>
> >>>> +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
> >>>> +{
> >>>> +VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> >>>> +
> >>>> +if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> >>>> +error_report("queue_enable is only suppported in devices of
> >>>> virtio "
> >>>> + "1.0 or later.");
> >>> Why is this triggering here? Maybe virtio_queue_enable() is called too
> >>> early. I have verified that the Linux guest driver sets VERSION_1. I
> >>> didn't check what SeaBIOS does.
> >> Looks like a bug, we should check device features here at least and it
> >> should be guest errors instead of error_report() here.
> >>
> >> Thanks
> > But it's weird, queue enable is written before guest features?
> > How come?
>
>
> Or queue_enable is written when the driver doesn't negotiate VERSION_1?
Is this a bug?
Or is it allowed in some cases?
I feel weird too.
Thanks.
>
> Thanks
>
>
> >
> >>> $ build/qemu-system-x86_64 -M accel=kvm -m 1G -cpu host -blockdev
> >>> file,node-name=drive0,filename=test.img -device
> >>> virtio-blk-pci,drive=drive0
> >>> qemu: queue_enable is only suppported in devices of virtio 1.0 or later.
> >>>
> >>> Stefan
> >>>
>
Re: [PULL v4 29/83] virtio: introduce virtio_queue_enable()
On Wed, 9 Nov 2022 01:39:32 -0500, "Michael S. Tsirkin" wrote:
> On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
> > On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi wrote:
> > >
> > > On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin wrote:
> > > >
> > > > From: Kangjie Xu
> > > >
> > > > Introduce the interface queue_enable() in VirtioDeviceClass and the
> > > > fucntion virtio_queue_enable() in virtio, it can be called when
> > > > VIRTIO_PCI_COMMON_Q_ENABLE is written and related virtqueue can be
> > > > started. It only supports the devices of virtio 1 or later. The
> > > > not-supported devices can only start the virtqueue when DRIVER_OK.
> > > >
> > > > Signed-off-by: Kangjie Xu
> > > > Signed-off-by: Xuan Zhuo
> > > > Acked-by: Jason Wang
> > > > Message-Id: <20221017092558.111082-4-xuanz...@linux.alibaba.com>
> > > > Reviewed-by: Michael S. Tsirkin
> > > > Signed-off-by: Michael S. Tsirkin
> > > > ---
> > > > include/hw/virtio/virtio.h | 2 ++
> > > > hw/virtio/virtio.c | 14 ++
> > > > 2 files changed, 16 insertions(+)
> > > >
> > > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > > > index 74d76c1dbc..b00b3fcf31 100644
> > > > --- a/include/hw/virtio/virtio.h
> > > > +++ b/include/hw/virtio/virtio.h
> > > > @@ -149,6 +149,7 @@ struct VirtioDeviceClass {
> > > > void (*reset)(VirtIODevice *vdev);
> > > > void (*set_status)(VirtIODevice *vdev, uint8_t val);
> > > > void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
> > > > +void (*queue_enable)(VirtIODevice *vdev, uint32_t queue_index);
> > > > /* For transitional devices, this is a bitmap of features
> > > > * that are only exposed on the legacy interface but not
> > > > * the modern one.
> > > > @@ -288,6 +289,7 @@ int virtio_queue_set_host_notifier_mr(VirtIODevice
> > > > *vdev, int n,
> > > > int virtio_set_status(VirtIODevice *vdev, uint8_t val);
> > > > void virtio_reset(void *opaque);
> > > > void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
> > > > +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
> > > > void virtio_update_irq(VirtIODevice *vdev);
> > > > int virtio_set_features(VirtIODevice *vdev, uint64_t val);
> > > >
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index cf5f9ca387..9683b2e158 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -2495,6 +2495,20 @@ void virtio_queue_reset(VirtIODevice *vdev,
> > > > uint32_t queue_index)
> > > > __virtio_queue_reset(vdev, queue_index);
> > > > }
> > > >
> > > > +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
> > > > +{
> > > > +VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > > > +
> > > > +if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> > > > +error_report("queue_enable is only suppported in devices of
> > > > virtio "
> > > > + "1.0 or later.");
> > >
> > > Why is this triggering here? Maybe virtio_queue_enable() is called too
> > > early. I have verified that the Linux guest driver sets VERSION_1. I
> > > didn't check what SeaBIOS does.
> >
> > Looks like a bug, we should check device features here at least and it
> > should be guest errors instead of error_report() here.
> >
> > Thanks
> >
>
> I suspect we should just drop this print. Kangjie?
I think it is.
At that time, this inspection was only added at hand, and theoretically it
should not be performed.
I am responsible for this patch set now.
hi, Michael,
What should I do, do I send a new version again?
Thanks.
>
>
> > > $ build/qemu-system-x86_64 -M accel=kvm -m 1G -cpu host -blockdev
> > > file,node-name=drive0,filename=test.img -device
> > > virtio-blk-pci,drive=drive0
> > > qemu: queue_enable is only suppported in devices of virtio 1.0 or later.
> > >
> > > Stefan
> > >
>
[PATCH v6 06/15] virtio-pci: support queue enable
From: Kangjie Xu
PCI devices support device specific vq enable.
Based on this function, the driver can re-enable the virtqueue after the
virtqueue is reset.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/virtio/virtio-pci.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index a41c1dfe71..9d948e21f6 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1342,6 +1342,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr
addr,
proxy->vqs[vdev->queue_sel].used[0]);
proxy->vqs[vdev->queue_sel].enabled = 1;
proxy->vqs[vdev->queue_sel].reset = 0;
+virtio_queue_enable(vdev, vdev->queue_sel);
} else {
virtio_error(vdev, "wrong value for queue_enable %"PRIx64, val);
}
--
2.32.0.3.g01195cf9f
[PATCH v6 05/15] virtio-pci: support queue reset
PCI devices support vq reset.
Based on this function, the driver can adjust the size of the ring, and
quickly recycle the buffer in the ring.
The migration of the virtio devices will not happen during a reset
operation. This is becuase the global iothread lock is held. Migration
thread also needs the lock. As a result, when migration of virtio
devices starts, the 'reset' status of VirtIOPCIQueue will always be 0.
Thus, we do not need to add it in vmstate_virtio_pci_modern_queue_state.
Signed-off-by: Xuan Zhuo
Signed-off-by: Kangjie Xu
Acked-by: Jason Wang
---
hw/virtio/virtio-pci.c | 15 +++
include/hw/virtio/virtio-pci.h | 5 +
2 files changed, 20 insertions(+)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index e7d80242b7..a41c1dfe71 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1251,6 +1251,9 @@ static uint64_t virtio_pci_common_read(void *opaque,
hwaddr addr,
case VIRTIO_PCI_COMMON_Q_USEDHI:
val = proxy->vqs[vdev->queue_sel].used[1];
break;
+case VIRTIO_PCI_COMMON_Q_RESET:
+val = proxy->vqs[vdev->queue_sel].reset;
+break;
default:
val = 0;
}
@@ -1338,6 +1341,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr
addr,
((uint64_t)proxy->vqs[vdev->queue_sel].used[1]) << 32 |
proxy->vqs[vdev->queue_sel].used[0]);
proxy->vqs[vdev->queue_sel].enabled = 1;
+proxy->vqs[vdev->queue_sel].reset = 0;
} else {
virtio_error(vdev, "wrong value for queue_enable %"PRIx64, val);
}
@@ -1360,6 +1364,16 @@ static void virtio_pci_common_write(void *opaque, hwaddr
addr,
case VIRTIO_PCI_COMMON_Q_USEDHI:
proxy->vqs[vdev->queue_sel].used[1] = val;
break;
+case VIRTIO_PCI_COMMON_Q_RESET:
+if (val == 1) {
+proxy->vqs[vdev->queue_sel].reset = 1;
+
+virtio_queue_reset(vdev, vdev->queue_sel);
+
+proxy->vqs[vdev->queue_sel].reset = 0;
+proxy->vqs[vdev->queue_sel].enabled = 0;
+}
+break;
default:
break;
}
@@ -1954,6 +1968,7 @@ static void virtio_pci_reset(DeviceState *qdev)
for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
proxy->vqs[i].enabled = 0;
+proxy->vqs[i].reset = 0;
proxy->vqs[i].num = 0;
proxy->vqs[i].desc[0] = proxy->vqs[i].desc[1] = 0;
proxy->vqs[i].avail[0] = proxy->vqs[i].avail[1] = 0;
diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
index 2446dcd9ae..938799e8f6 100644
--- a/include/hw/virtio/virtio-pci.h
+++ b/include/hw/virtio/virtio-pci.h
@@ -117,6 +117,11 @@ typedef struct VirtIOPCIRegion {
typedef struct VirtIOPCIQueue {
uint16_t num;
bool enabled;
+ /*
+ * No need to migrate the reset status, because it is always 0
+ * when the migration starts.
+ */
+ bool reset;
uint32_t desc[2];
uint32_t avail[2];
uint32_t used[2];
--
2.32.0.3.g01195cf9f
[PATCH v6 14/15] vhost: vhost-kernel: enable vq reset feature
From: Kangjie Xu
Add virtqueue reset feature for vhost-kernel.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/net/vhost_net.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index d2926e2ed6..53b2fac4f6 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -47,6 +47,7 @@ static const int kernel_feature_bits[] = {
VIRTIO_NET_F_MTU,
VIRTIO_F_IOMMU_PLATFORM,
VIRTIO_F_RING_PACKED,
+VIRTIO_F_RING_RESET,
VIRTIO_NET_F_HASH_REPORT,
VHOST_INVALID_FEATURE_BIT
};
--
2.32.0.3.g01195cf9f
[PATCH v6 12/15] virtio-net: support queue reset
virtio-net and vhost-kernel implement queue reset.
Queued packets in the corresponding queue pair are flushed
or purged.
For virtio-net, userspace datapath will be disabled later in
__virtio_queue_reset(). It will set addr of vring to 0 and idx to 0.
Thus, virtio_net_receive() and virtio_net_flush_tx() will not receive
or send packets.
For vhost-net, the datapath will be disabled in vhost_net_virtqueue_reset().
Signed-off-by: Xuan Zhuo
Signed-off-by: Kangjie Xu
Acked-by: Jason Wang
---
hw/net/virtio-net.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 4ace57fa6d..8feeb032b4 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -546,6 +546,23 @@ static RxFilterInfo
*virtio_net_query_rxfilter(NetClientState *nc)
return info;
}
+static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
+{
+VirtIONet *n = VIRTIO_NET(vdev);
+NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
+
+if (!nc->peer) {
+return;
+}
+
+if (get_vhost_net(nc->peer) &&
+nc->peer->info->type == NET_CLIENT_DRIVER_TAP) {
+vhost_net_virtqueue_reset(vdev, nc, queue_index);
+}
+
+flush_or_purge_queued_packets(nc);
+}
+
static void virtio_net_reset(VirtIODevice *vdev)
{
VirtIONet *n = VIRTIO_NET(vdev);
@@ -3784,6 +3801,7 @@ static void virtio_net_class_init(ObjectClass *klass,
void *data)
vdc->set_features = virtio_net_set_features;
vdc->bad_features = virtio_net_bad_features;
vdc->reset = virtio_net_reset;
+vdc->queue_reset = virtio_net_queue_reset;
vdc->set_status = virtio_net_set_status;
vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
--
2.32.0.3.g01195cf9f
[PATCH v6 10/15] vhost-net: vhost-kernel: introduce vhost_net_virtqueue_restart()
From: Kangjie Xu
Introduce vhost_net_virtqueue_restart(), which can restart the
specific virtqueue when the vhost net started running before.
If it fails to restart the virtqueue, the device will be stopped.
Here we do not reuse vhost_net_start_one() or vhost_dev_start()
because they work at queue pair level. The mem table and features
do not change, so we can call the vhost_virtqueue_start() to
restart a specific queue.
This patch only considers the case of vhost-kernel, when
NetClientDriver is NET_CLIENT_DRIVER_TAP.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/net/vhost_net-stub.c | 6 +
hw/net/vhost_net.c | 53 +
include/net/vhost_net.h | 2 ++
3 files changed, 61 insertions(+)
diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c
index 2d745e359c..9f7daae99c 100644
--- a/hw/net/vhost_net-stub.c
+++ b/hw/net/vhost_net-stub.c
@@ -107,3 +107,9 @@ void vhost_net_virtqueue_reset(VirtIODevice *vdev,
NetClientState *nc,
{
}
+
+int vhost_net_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
+int vq_index)
+{
+return 0;
+}
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 8beecb4d22..d2926e2ed6 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -34,6 +34,7 @@
#include "standard-headers/linux/virtio_ring.h"
#include "hw/virtio/vhost.h"
#include "hw/virtio/virtio-bus.h"
+#include "linux-headers/linux/vhost.h"
/* Features supported by host kernel. */
@@ -556,3 +557,55 @@ void vhost_net_virtqueue_reset(VirtIODevice *vdev,
NetClientState *nc,
net->dev.vqs + idx,
net->dev.vq_index + idx);
}
+
+int vhost_net_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
+int vq_index)
+{
+VHostNetState *net = get_vhost_net(nc->peer);
+const VhostOps *vhost_ops = net->dev.vhost_ops;
+struct vhost_vring_file file = { };
+int idx, r;
+
+if (!net->dev.started) {
+return -EBUSY;
+}
+
+/* should only be called after backend is connected */
+assert(vhost_ops);
+
+idx = vhost_ops->vhost_get_vq_index(>dev, vq_index);
+
+r = vhost_virtqueue_start(>dev,
+ vdev,
+ net->dev.vqs + idx,
+ net->dev.vq_index + idx);
+if (r < 0) {
+goto err_start;
+}
+
+if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
+file.index = idx;
+file.fd = net->backend;
+r = vhost_net_set_backend(>dev, );
+if (r < 0) {
+r = -errno;
+goto err_start;
+}
+}
+
+return 0;
+
+err_start:
+error_report("Error when restarting the queue.");
+
+if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
+file.fd = VHOST_FILE_UNBIND;
+file.index = idx;
+int r = vhost_net_set_backend(>dev, );
+assert(r >= 0);
+}
+
+vhost_dev_stop(>dev, vdev);
+
+return r;
+}
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 85d85a4957..40b9a40074 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -50,4 +50,6 @@ int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
int vq_index);
+int vhost_net_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
+int vq_index);
#endif
--
2.32.0.3.g01195cf9f
[PATCH v6 09/15] vhost-net: vhost-kernel: introduce vhost_net_virtqueue_reset()
From: Kangjie Xu
Introduce vhost_virtqueue_reset(), which can reset the specific
virtqueue in the device. Then it will unmap vrings and the desc
of the virtqueue.
Here we do not reuse the vhost_net_stop_one() or vhost_dev_stop(),
because they work at queue pair level. We do not use
vhost_virtqueue_stop() because it may stop the device in the
backend.
This patch only considers the case of vhost-kernel, when
NetClientDriver is NET_CLIENT_DRIVER_TAP.
Furthermore, we do not need net->nc->info->poll() because
it enables userspace datapath and we want to stop all
datapaths for this reset virtqueue here.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/net/vhost_net-stub.c | 6 ++
hw/net/vhost_net.c | 25 +
include/net/vhost_net.h | 2 ++
3 files changed, 33 insertions(+)
diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c
index 89d71cfb8e..2d745e359c 100644
--- a/hw/net/vhost_net-stub.c
+++ b/hw/net/vhost_net-stub.c
@@ -101,3 +101,9 @@ int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu)
{
return 0;
}
+
+void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
+ int vq_index)
+{
+
+}
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index d28f8b974b..8beecb4d22 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -531,3 +531,28 @@ int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu)
return vhost_ops->vhost_net_set_mtu(>dev, mtu);
}
+
+void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
+ int vq_index)
+{
+VHostNetState *net = get_vhost_net(nc->peer);
+const VhostOps *vhost_ops = net->dev.vhost_ops;
+struct vhost_vring_file file = { .fd = -1 };
+int idx;
+
+/* should only be called after backend is connected */
+assert(vhost_ops);
+
+idx = vhost_ops->vhost_get_vq_index(>dev, vq_index);
+
+if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
+file.index = idx;
+int r = vhost_net_set_backend(>dev, );
+assert(r >= 0);
+}
+
+vhost_virtqueue_stop(>dev,
+ vdev,
+ net->dev.vqs + idx,
+ net->dev.vq_index + idx);
+}
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 387e913e4e..85d85a4957 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -48,4 +48,6 @@ uint64_t vhost_net_get_acked_features(VHostNetState *net);
int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
+void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
+ int vq_index);
#endif
--
2.32.0.3.g01195cf9f
[PATCH v6 03/15] virtio: introduce virtio_queue_enable()
From: Kangjie Xu
Introduce the interface queue_enable() in VirtioDeviceClass and the
fucntion virtio_queue_enable() in virtio, it can be called when
VIRTIO_PCI_COMMON_Q_ENABLE is written and related virtqueue can be
started. It only supports the devices of virtio 1 or later. The
not-supported devices can only start the virtqueue when DRIVER_OK.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/virtio/virtio.c | 14 ++
include/hw/virtio/virtio.h | 2 ++
2 files changed, 16 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index cf5f9ca387..9683b2e158 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2495,6 +2495,20 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t
queue_index)
__virtio_queue_reset(vdev, queue_index);
}
+void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
+{
+VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+
+if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+error_report("queue_enable is only suppported in devices of virtio "
+ "1.0 or later.");
+}
+
+if (k->queue_enable) {
+k->queue_enable(vdev, queue_index);
+}
+}
+
void virtio_reset(void *opaque)
{
VirtIODevice *vdev = opaque;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 74d76c1dbc..b00b3fcf31 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -149,6 +149,7 @@ struct VirtioDeviceClass {
void (*reset)(VirtIODevice *vdev);
void (*set_status)(VirtIODevice *vdev, uint8_t val);
void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
+void (*queue_enable)(VirtIODevice *vdev, uint32_t queue_index);
/* For transitional devices, this is a bitmap of features
* that are only exposed on the legacy interface but not
* the modern one.
@@ -288,6 +289,7 @@ int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev,
int n,
int virtio_set_status(VirtIODevice *vdev, uint8_t val);
void virtio_reset(void *opaque);
void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
+void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
void virtio_update_irq(VirtIODevice *vdev);
int virtio_set_features(VirtIODevice *vdev, uint64_t val);
--
2.32.0.3.g01195cf9f
[PATCH v6 00/15] Support VIRTIO_F_RING_RESET for virtio-net, vhost-net kernel in virtio pci-modern
The virtio queue reset function has already been defined in the virtio spec 1.2. The relevant virtio spec information is here: https://github.com/oasis-tcs/virtio-spec/issues/124 https://github.com/oasis-tcs/virtio-spec/issues/139 This patch set is to support this function in QEMU. It consists of several parts: 1. Patches 1-7 are the basic interfaces for vq reset in virtio and virtio-pci. 2. Patches 8-11 support vq reset and vq restart for vhost-kernel. 3. Patches 12-14 support vq reset and vq restart for virtio-net. 5. Patch 15 enables the vq reset feature for vhost-kernel. The process of virtqueue reset can be concluded as: 1. The virtqueue is disabled when VIRTIO_PCI_COMMON_Q_RESET is written. 2. Then the virtqueue can be optionally restarted(re-enabled). Since this patch set involves multiple modules and seems a bit messy, we briefly describe the calling process for different modes below. virtio-net: 1. VIRTIO_PCI_COMMON_Q_RESET is written [virtio-pci] -> virtio_queue_reset() [virtio] -> virtio_net_queue_reset() [virtio-net] -> __virtio_queue_reset() 2. VIRTIO_PCI_COMMON_Q_ENABLE is written [virtio-pci] -> set enabled, reset status of vq. vhost-kernel: 1. VIRTIO_PCI_COMMON_Q_RESET is written [virtio-pci] -> virtio_queue_reset() [virtio] -> virtio_net_queue_reset() [virtio-net] -> vhost_net_virtqueue_stop() [vhost-net] -> vhost_net_set_backend() [vhost] -> vhost_virtqueue_stop() -> __virtio_queue_reset() 2. VIRTIO_PCI_COMMON_Q_ENABLE is written [virtio-pci] -> virtio_queue_enable() [virtio] -> virtio_net_queue_enable() [virtio-net] -> vhost_net_virtqueue_restart() [vhost-net] -> vhost_virtqueue_start() [vhost] -> vhost_net_set_backend() -> set enabled, reset status of vq. Test environment and method: Host: 5.19.0-rc3 (With vq reset support) Qemu: QEMU emulator version 7.0.50 Guest: 5.19.0-rc3 (With vq reset support) Test Cmd: ethtool -g eth1; ethtool -G eth1 rx $1 tx $2; ethtool -g eth1; The drvier can resize the virtio queue, then virtio queue reset function should be triggered. The default is split mode, modify Qemu virtio-net to add PACKED feature to test packed mode. Guest Kernel Patch: https://lore.kernel.org/bpf/20220801063902.129329-1-xuanz...@linux.alibaba.com/ Host Kernel Patch: https://lore.kernel.org/bpf/20220825085610.80315-1-kangjie...@linux.alibaba.com/ Looking forward to your review and comments. Thanks. changelog: v6: 1. fix for compile fail for mingw 32 by adding funcs inside vhost_net-stub.c 2. add one new commit to add vq reset support inside virtio-net (not wait for vhost-user) v5: 1. vhost_net_virtqueue_restart() use -EBUSY replace -ENOTSUP. @Jason 2. reuse VHOST_FILE_UNBIND. @Jason v4: 1. Add explanation for preventing userspace datapath in virtio-net. 2. Return error when vhost is not started in vhost_net_virtqueue_restart(). 3. Reset the virtqueue in the device reusing vhost_virtqueue_stop(). 4. Disable queue reset feature for pre-7.2 machine. v3: 1. Remove support for vhost-user in this series and refactor the code. 2. Rename 'vhost_net_virtqueue_stop' to 'vhost_net_virtqueue_reset'. 3. Make PCI transport ready before device ready when queue_enabled is set to true. 4. Add some comments. v2: 1. Add support for vhost-net kernel scenario. 2. Add a new vhost-user message VHOST_USER_RESET_VRING. 3. Add migration compatibility for virtqueue reset. Kangjie Xu (10): virtio: introduce virtio_queue_enable() virtio: core: vq reset feature negotation support virtio-pci: support queue enable vhost: expose vhost_virtqueue_start() vhost: expose vhost_virtqueue_stop() vhost-net: vhost-kernel: introduce vhost_net_virtqueue_reset() vhost-net: vhost-kernel: introduce vhost_net_virtqueue_restart() virtio-net: introduce flush_or_purge_queued_packets() virtio-net: support queue_enable vhost: vhost-kernel: enable vq reset feature Xuan Zhuo (5): virtio: introduce __virtio_queue_reset() virtio: introduce virtio_queue_reset() virtio-pci: support queue reset virtio-net: support queue reset virtio-net: enable vq reset feature hw/core/machine.c | 4 +- hw/net/vhost_net-stub.c| 12 ++ hw/net/vhost_net.c | 79 ++ hw/net/virtio-net.c| 57 +--- hw/virtio/vhost.c | 16 +++ hw/virtio/virtio-pci.c | 16 +++ hw/virtio/virtio.c | 62 +++--- include/hw/virtio/vhost.h | 5 +++ include/hw/virtio/virtio-pci.h | 5 +++ include/hw/virtio/virtio.h | 8 +++- include/net/vhost_net.h| 4 ++ 11 files changed, 236 insertions(+), 32 deletions(-) -- 2.32.0.3.g01195cf9f
[PATCH v6 04/15] virtio: core: vq reset feature negotation support
From: Kangjie Xu
A a new command line parameter "queue_reset" is added.
Meanwhile, the vq reset feature is disabled for pre-7.2 machines.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/core/machine.c | 4 +++-
include/hw/virtio/virtio.h | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index aa520e74a8..907fa78ff0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -40,7 +40,9 @@
#include "hw/virtio/virtio-pci.h"
#include "qom/object_interfaces.h"
-GlobalProperty hw_compat_7_1[] = {};
+GlobalProperty hw_compat_7_1[] = {
+{ "virtio-device", "queue_reset", "false" },
+};
const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
GlobalProperty hw_compat_7_0[] = {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index b00b3fcf31..1423dba379 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -313,7 +313,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
VIRTIO_F_IOMMU_PLATFORM, false), \
DEFINE_PROP_BIT64("packed", _state, _field, \
- VIRTIO_F_RING_PACKED, false)
+ VIRTIO_F_RING_PACKED, false), \
+DEFINE_PROP_BIT64("queue_reset", _state, _field, \
+ VIRTIO_F_RING_RESET, true)
hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
--
2.32.0.3.g01195cf9f
[PATCH v6 02/15] virtio: introduce virtio_queue_reset()
Introduce a new interface function virtio_queue_reset() to implement
reset for vq.
Add a new callback to VirtioDeviceClass for queue reset operation for
each child device.
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/virtio/virtio.c | 11 +++
include/hw/virtio/virtio.h | 2 ++
2 files changed, 13 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 6f42fcadd7..cf5f9ca387 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2484,6 +2484,17 @@ static void __virtio_queue_reset(VirtIODevice *vdev,
uint32_t i)
virtio_virtqueue_reset_region_cache(>vq[i]);
}
+void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
+{
+VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+
+if (k->queue_reset) {
+k->queue_reset(vdev, queue_index);
+}
+
+__virtio_queue_reset(vdev, queue_index);
+}
+
void virtio_reset(void *opaque)
{
VirtIODevice *vdev = opaque;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index f41b4a7e64..74d76c1dbc 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -148,6 +148,7 @@ struct VirtioDeviceClass {
void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
void (*reset)(VirtIODevice *vdev);
void (*set_status)(VirtIODevice *vdev, uint8_t val);
+void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
/* For transitional devices, this is a bitmap of features
* that are only exposed on the legacy interface but not
* the modern one.
@@ -286,6 +287,7 @@ int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev,
int n,
MemoryRegion *mr, bool assign);
int virtio_set_status(VirtIODevice *vdev, uint8_t val);
void virtio_reset(void *opaque);
+void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
void virtio_update_irq(VirtIODevice *vdev);
int virtio_set_features(VirtIODevice *vdev, uint64_t val);
--
2.32.0.3.g01195cf9f
[PATCH v6 13/15] virtio-net: support queue_enable
From: Kangjie Xu
Support queue_enable in vhost-kernel scenario. It can be called when
a vq reset operation has been performed and the vq is restared.
It should be noted that we can restart the vq when the vhost has
already started. When launching a new vhost device, the vhost is not
started and all vqs are not initalized until VIRTIO_PCI_COMMON_STATUS
is written. Thus, we should use vhost_started to differentiate the
two cases: vq reset and device start.
Currently it only supports vhost-kernel.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/net/virtio-net.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 8feeb032b4..f5adba45d5 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -563,6 +563,26 @@ static void virtio_net_queue_reset(VirtIODevice *vdev,
uint32_t queue_index)
flush_or_purge_queued_packets(nc);
}
+static void virtio_net_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
+{
+VirtIONet *n = VIRTIO_NET(vdev);
+NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
+int r;
+
+if (!nc->peer || !vdev->vhost_started) {
+return;
+}
+
+if (get_vhost_net(nc->peer) &&
+nc->peer->info->type == NET_CLIENT_DRIVER_TAP) {
+r = vhost_net_virtqueue_restart(vdev, nc, queue_index);
+if (r < 0) {
+error_report("unable to restart vhost net virtqueue: %d, "
+"when resetting the queue", queue_index);
+}
+}
+}
+
static void virtio_net_reset(VirtIODevice *vdev)
{
VirtIONet *n = VIRTIO_NET(vdev);
@@ -3802,6 +3822,7 @@ static void virtio_net_class_init(ObjectClass *klass,
void *data)
vdc->bad_features = virtio_net_bad_features;
vdc->reset = virtio_net_reset;
vdc->queue_reset = virtio_net_queue_reset;
+vdc->queue_enable = virtio_net_queue_enable;
vdc->set_status = virtio_net_set_status;
vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
--
2.32.0.3.g01195cf9f
[PATCH v6 08/15] vhost: expose vhost_virtqueue_stop()
From: Kangjie Xu
Expose vhost_virtqueue_stop(), we need to use it when resetting a
virtqueue.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/virtio/vhost.c | 8
include/hw/virtio/vhost.h | 2 ++
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 788d0a0679..d1c4c20b8c 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1201,10 +1201,10 @@ fail_alloc_desc:
return r;
}
-static void vhost_virtqueue_stop(struct vhost_dev *dev,
-struct VirtIODevice *vdev,
-struct vhost_virtqueue *vq,
-unsigned idx)
+void vhost_virtqueue_stop(struct vhost_dev *dev,
+ struct VirtIODevice *vdev,
+ struct vhost_virtqueue *vq,
+ unsigned idx)
{
int vhost_vq_index = dev->vhost_ops->vhost_get_vq_index(dev, idx);
struct vhost_vring_state state = {
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 0054a695dc..353252ac3e 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -299,6 +299,8 @@ int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t
iova, int write);
int vhost_virtqueue_start(struct vhost_dev *dev, struct VirtIODevice *vdev,
struct vhost_virtqueue *vq, unsigned idx);
+void vhost_virtqueue_stop(struct vhost_dev *dev, struct VirtIODevice *vdev,
+ struct vhost_virtqueue *vq, unsigned idx);
void vhost_dev_reset_inflight(struct vhost_inflight *inflight);
void vhost_dev_free_inflight(struct vhost_inflight *inflight);
--
2.32.0.3.g01195cf9f
[PATCH v6 11/15] virtio-net: introduce flush_or_purge_queued_packets()
From: Kangjie Xu
Introduce the fucntion flush_or_purge_queued_packets(), it will be
used in device reset and virtqueue reset. Therefore, we extract the
common logic as a new function.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/net/virtio-net.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index e9f696b4cf..4ace57fa6d 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -124,6 +124,16 @@ static int vq2q(int queue_index)
return queue_index / 2;
}
+static void flush_or_purge_queued_packets(NetClientState *nc)
+{
+if (!nc->peer) {
+return;
+}
+
+qemu_flush_or_purge_queued_packets(nc->peer, true);
+assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
+}
+
/* TODO
* - we could suppress RX interrupt if we were so inclined.
*/
@@ -566,12 +576,7 @@ static void virtio_net_reset(VirtIODevice *vdev)
/* Flush any async TX */
for (i = 0; i < n->max_queue_pairs; i++) {
-NetClientState *nc = qemu_get_subqueue(n->nic, i);
-
-if (nc->peer) {
-qemu_flush_or_purge_queued_packets(nc->peer, true);
-assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
-}
+flush_or_purge_queued_packets(qemu_get_subqueue(n->nic, i));
}
}
--
2.32.0.3.g01195cf9f
[PATCH v6 15/15] virtio-net: enable vq reset feature
Add virtqueue reset feature for virtio-net
Signed-off-by: Xuan Zhuo
---
hw/net/virtio-net.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f5adba45d5..975bbc22f9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -788,6 +788,7 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev,
uint64_t features,
}
if (!get_vhost_net(nc->peer)) {
+virtio_add_feature(, VIRTIO_F_RING_RESET);
return features;
}
--
2.32.0.3.g01195cf9f
[PATCH v6 07/15] vhost: expose vhost_virtqueue_start()
From: Kangjie Xu
Expose vhost_virtqueue_start(), we need to use it when restarting a
virtqueue.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/virtio/vhost.c | 8
include/hw/virtio/vhost.h | 3 +++
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 5185c15295..788d0a0679 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1081,10 +1081,10 @@ out:
return ret;
}
-static int vhost_virtqueue_start(struct vhost_dev *dev,
-struct VirtIODevice *vdev,
-struct vhost_virtqueue *vq,
-unsigned idx)
+int vhost_virtqueue_start(struct vhost_dev *dev,
+ struct VirtIODevice *vdev,
+ struct vhost_virtqueue *vq,
+ unsigned idx)
{
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
VirtioBusState *vbus = VIRTIO_BUS(qbus);
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index d7eb557885..0054a695dc 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -297,6 +297,9 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write);
+int vhost_virtqueue_start(struct vhost_dev *dev, struct VirtIODevice *vdev,
+ struct vhost_virtqueue *vq, unsigned idx);
+
void vhost_dev_reset_inflight(struct vhost_inflight *inflight);
void vhost_dev_free_inflight(struct vhost_inflight *inflight);
void vhost_dev_save_inflight(struct vhost_inflight *inflight, QEMUFile *f);
--
2.32.0.3.g01195cf9f
[PATCH v6 01/15] virtio: introduce __virtio_queue_reset()
Separate the logic of vq reset. This logic will be called directly
later.
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/virtio/virtio.c | 37 +
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 808446b4c9..6f42fcadd7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2464,6 +2464,26 @@ static enum virtio_device_endian
virtio_current_cpu_endian(void)
}
}
+static void __virtio_queue_reset(VirtIODevice *vdev, uint32_t i)
+{
+vdev->vq[i].vring.desc = 0;
+vdev->vq[i].vring.avail = 0;
+vdev->vq[i].vring.used = 0;
+vdev->vq[i].last_avail_idx = 0;
+vdev->vq[i].shadow_avail_idx = 0;
+vdev->vq[i].used_idx = 0;
+vdev->vq[i].last_avail_wrap_counter = true;
+vdev->vq[i].shadow_avail_wrap_counter = true;
+vdev->vq[i].used_wrap_counter = true;
+virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
+vdev->vq[i].signalled_used = 0;
+vdev->vq[i].signalled_used_valid = false;
+vdev->vq[i].notification = true;
+vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
+vdev->vq[i].inuse = 0;
+virtio_virtqueue_reset_region_cache(>vq[i]);
+}
+
void virtio_reset(void *opaque)
{
VirtIODevice *vdev = opaque;
@@ -2495,22 +2515,7 @@ void virtio_reset(void *opaque)
virtio_notify_vector(vdev, vdev->config_vector);
for(i = 0; i < VIRTIO_QUEUE_MAX; i++) {
-vdev->vq[i].vring.desc = 0;
-vdev->vq[i].vring.avail = 0;
-vdev->vq[i].vring.used = 0;
-vdev->vq[i].last_avail_idx = 0;
-vdev->vq[i].shadow_avail_idx = 0;
-vdev->vq[i].used_idx = 0;
-vdev->vq[i].last_avail_wrap_counter = true;
-vdev->vq[i].shadow_avail_wrap_counter = true;
-vdev->vq[i].used_wrap_counter = true;
-virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
-vdev->vq[i].signalled_used = 0;
-vdev->vq[i].signalled_used_valid = false;
-vdev->vq[i].notification = true;
-vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
-vdev->vq[i].inuse = 0;
-virtio_virtqueue_reset_region_cache(>vq[i]);
+__virtio_queue_reset(vdev, i);
}
}
--
2.32.0.3.g01195cf9f
Re: [PATCH v2 3/6] vhost-net: vhost-user: update vhost_net_virtqueue_reset()
On Thu, 15 Sep 2022 10:12:11 +0800, Jason Wang wrote:
> On Wed, Sep 14, 2022 at 2:21 PM Xuan Zhuo wrote:
> >
> > On Wed, 14 Sep 2022 11:13:29 +0800, Jason Wang wrote:
> > >
> > > 在 2022/9/12 11:10, Kangjie Xu 写道:
> > > > Update vhost_net_virtqueue_reset() for vhost-user scenario.
> > > >
> > > > In order to reuse some functions, we process the idx for
> > > > vhost-user scenario because vhost_get_vq_index behave
> > > > differently for vhost-user.
> > > >
> > > > Signed-off-by: Kangjie Xu
> > > > Signed-off-by: Xuan Zhuo
> > > > ---
> > > > hw/net/vhost_net.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > index ea896ea75b..25e5665489 100644
> > > > --- a/hw/net/vhost_net.c
> > > > +++ b/hw/net/vhost_net.c
> > > > @@ -545,6 +545,9 @@ void vhost_net_virtqueue_reset(VirtIODevice *vdev,
> > > > NetClientState *nc,
> > > > assert(vhost_ops);
> > > >
> > > > idx = vhost_ops->vhost_get_vq_index(>dev, vq_index);
> > > > +if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> > > > +idx -= net->dev.vq_index;
> > > > +}
> > >
> > >
> > > This looks tricky. Any reason we can't simply use vq_index for both
> > > vhost-user and vhost-net?
> >
> >
> > static int vhost_user_get_vq_index(struct vhost_dev *dev, int idx)
> > {
> > assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
> >
> > return idx;
> > }
> >
> >
> > static int vhost_kernel_get_vq_index(struct vhost_dev *dev, int idx)
> > {
> > assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
> >
> > return idx - dev->vq_index;
> > }
> >
> > The implementation of these two callbacks is different. The structure of
> > the two
> > scenarios is different. We may need to do some optimizations in the future.
>
> Yes, but what I meant is, you do
>
> idx -= net->dev.vq_index;
>
> and then
>
> net->dev.vq_index + idx
>
> This is a hint that we should have a better organization of the code.
Rethink about this.
If I don't do this "idx -= net->dev.vq_index".
Then, it is necessary to call vhost_virtqueue_stop() separately and once again
"net->dev.vqs + idx - net->dev.vq_index"
vhost_virtqueue_stop(>dev,
vdev,
net->dev.vqs + idx - net->dev.vq_index,
idx);
Thanks.
>
> Thanks
>
> >
> > Thanks.
> >
> >
> > >
> > > Thanks
> > >
> > >
> > > >
> > > > if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
> > > > file.index = idx;
> > >
> >
>
Re: [PATCH v2 3/6] vhost-net: vhost-user: update vhost_net_virtqueue_reset()
On Thu, 15 Sep 2022 10:12:11 +0800, Jason Wang wrote:
> On Wed, Sep 14, 2022 at 2:21 PM Xuan Zhuo wrote:
> >
> > On Wed, 14 Sep 2022 11:13:29 +0800, Jason Wang wrote:
> > >
> > > 在 2022/9/12 11:10, Kangjie Xu 写道:
> > > > Update vhost_net_virtqueue_reset() for vhost-user scenario.
> > > >
> > > > In order to reuse some functions, we process the idx for
> > > > vhost-user scenario because vhost_get_vq_index behave
> > > > differently for vhost-user.
> > > >
> > > > Signed-off-by: Kangjie Xu
> > > > Signed-off-by: Xuan Zhuo
> > > > ---
> > > > hw/net/vhost_net.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > index ea896ea75b..25e5665489 100644
> > > > --- a/hw/net/vhost_net.c
> > > > +++ b/hw/net/vhost_net.c
> > > > @@ -545,6 +545,9 @@ void vhost_net_virtqueue_reset(VirtIODevice *vdev,
> > > > NetClientState *nc,
> > > > assert(vhost_ops);
> > > >
> > > > idx = vhost_ops->vhost_get_vq_index(>dev, vq_index);
> > > > +if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> > > > +idx -= net->dev.vq_index;
> > > > +}
> > >
> > >
> > > This looks tricky. Any reason we can't simply use vq_index for both
> > > vhost-user and vhost-net?
> >
> >
> > static int vhost_user_get_vq_index(struct vhost_dev *dev, int idx)
> > {
> > assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
> >
> > return idx;
> > }
> >
> >
> > static int vhost_kernel_get_vq_index(struct vhost_dev *dev, int idx)
> > {
> > assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
> >
> > return idx - dev->vq_index;
> > }
> >
> > The implementation of these two callbacks is different. The structure of
> > the two
> > scenarios is different. We may need to do some optimizations in the future.
>
> Yes, but what I meant is, you do
>
> idx -= net->dev.vq_index;
>
> and then
>
> net->dev.vq_index + idx
>
> This is a hint that we should have a better organization of the code.
I see.
Will fix this.
Thanks.
>
> Thanks
>
> >
> > Thanks.
> >
> >
> > >
> > > Thanks
> > >
> > >
> > > >
> > > > if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
> > > > file.index = idx;
> > >
> >
>
Re: [PATCH v2 3/6] vhost-net: vhost-user: update vhost_net_virtqueue_reset()
On Wed, 14 Sep 2022 11:13:29 +0800, Jason Wang wrote:
>
> 在 2022/9/12 11:10, Kangjie Xu 写道:
> > Update vhost_net_virtqueue_reset() for vhost-user scenario.
> >
> > In order to reuse some functions, we process the idx for
> > vhost-user scenario because vhost_get_vq_index behave
> > differently for vhost-user.
> >
> > Signed-off-by: Kangjie Xu
> > Signed-off-by: Xuan Zhuo
> > ---
> > hw/net/vhost_net.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index ea896ea75b..25e5665489 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -545,6 +545,9 @@ void vhost_net_virtqueue_reset(VirtIODevice *vdev,
> > NetClientState *nc,
> > assert(vhost_ops);
> >
> > idx = vhost_ops->vhost_get_vq_index(>dev, vq_index);
> > +if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> > +idx -= net->dev.vq_index;
> > +}
>
>
> This looks tricky. Any reason we can't simply use vq_index for both
> vhost-user and vhost-net?
static int vhost_user_get_vq_index(struct vhost_dev *dev, int idx)
{
assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
return idx;
}
static int vhost_kernel_get_vq_index(struct vhost_dev *dev, int idx)
{
assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
return idx - dev->vq_index;
}
The implementation of these two callbacks is different. The structure of the two
scenarios is different. We may need to do some optimizations in the future.
Thanks.
>
> Thanks
>
>
> >
> > if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
> > file.index = idx;
>
[PATCH v5 14/15] virtio-net: support queue_enable
From: Kangjie Xu
Support queue_enable in vhost-kernel scenario. It can be called when
a vq reset operation has been performed and the vq is restared.
It should be noted that we can restart the vq when the vhost has
already started. When launching a new vhost device, the vhost is not
started and all vqs are not initalized until VIRTIO_PCI_COMMON_STATUS
is written. Thus, we should use vhost_started to differentiate the
two cases: vq reset and device start.
Currently it only supports vhost-kernel.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/net/virtio-net.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index d774a3e652..7817206596 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -557,6 +557,26 @@ static void virtio_net_queue_reset(VirtIODevice *vdev,
uint32_t queue_index)
flush_or_purge_queued_packets(nc);
}
+static void virtio_net_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
+{
+VirtIONet *n = VIRTIO_NET(vdev);
+NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
+int r;
+
+if (!nc->peer || !vdev->vhost_started) {
+return;
+}
+
+if (get_vhost_net(nc->peer) &&
+nc->peer->info->type == NET_CLIENT_DRIVER_TAP) {
+r = vhost_net_virtqueue_restart(vdev, nc, queue_index);
+if (r < 0) {
+error_report("unable to restart vhost net virtqueue: %d, "
+"when resetting the queue", queue_index);
+}
+}
+}
+
static void virtio_net_reset(VirtIODevice *vdev)
{
VirtIONet *n = VIRTIO_NET(vdev);
@@ -3802,6 +3822,7 @@ static void virtio_net_class_init(ObjectClass *klass,
void *data)
vdc->bad_features = virtio_net_bad_features;
vdc->reset = virtio_net_reset;
vdc->queue_reset = virtio_net_queue_reset;
+vdc->queue_enable = virtio_net_queue_enable;
vdc->set_status = virtio_net_set_status;
vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
--
2.32.0.3.g01195cf9f
[PATCH v5 09/15] vhost: expose vhost_virtqueue_stop()
From: Kangjie Xu
Expose vhost_virtqueue_stop(), we need to use it when resetting a
virtqueue.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/virtio/vhost.c | 8
include/hw/virtio/vhost.h | 2 ++
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 7900bc81ab..5407f60226 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1201,10 +1201,10 @@ fail_alloc_desc:
return r;
}
-static void vhost_virtqueue_stop(struct vhost_dev *dev,
-struct VirtIODevice *vdev,
-struct vhost_virtqueue *vq,
-unsigned idx)
+void vhost_virtqueue_stop(struct vhost_dev *dev,
+ struct VirtIODevice *vdev,
+ struct vhost_virtqueue *vq,
+ unsigned idx)
{
int vhost_vq_index = dev->vhost_ops->vhost_get_vq_index(dev, idx);
struct vhost_vring_state state = {
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index b092f57d98..2b168b2269 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -281,6 +281,8 @@ int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t
iova, int write);
int vhost_virtqueue_start(struct vhost_dev *dev, struct VirtIODevice *vdev,
struct vhost_virtqueue *vq, unsigned idx);
+void vhost_virtqueue_stop(struct vhost_dev *dev, struct VirtIODevice *vdev,
+ struct vhost_virtqueue *vq, unsigned idx);
void vhost_dev_reset_inflight(struct vhost_inflight *inflight);
void vhost_dev_free_inflight(struct vhost_inflight *inflight);
--
2.32.0.3.g01195cf9f
[PATCH v5 00/15] Support VIRTIO_F_RING_RESET for virtio-net, vhost-net kernel in virtio pci-modern
The virtio queue reset function has already been defined in the virtio spec 1.2. The relevant virtio spec information is here: https://github.com/oasis-tcs/virtio-spec/issues/124 https://github.com/oasis-tcs/virtio-spec/issues/139 This patch set is to support this function in QEMU. It consists of several parts: 1. Patches 1-7 are the basic interfaces for vq reset in virtio and virtio-pci. 2. Patches 8-11 support vq reset and vq restart for vhost-kernel. 3. Patches 12-14 support vq reset and vq restart for virtio-net. 5. Patch 15 enables the vq reset feature for vhost-kernel. The process of virtqueue reset can be concluded as: 1. The virtqueue is disabled when VIRTIO_PCI_COMMON_Q_RESET is written. 2. Then the virtqueue can be optionally restarted(re-enabled). Since this patch set involves multiple modules and seems a bit messy, we briefly describe the calling process for different modes below. virtio-net: 1. VIRTIO_PCI_COMMON_Q_RESET is written [virtio-pci] -> virtio_queue_reset() [virtio] -> virtio_net_queue_reset() [virtio-net] -> __virtio_queue_reset() 2. VIRTIO_PCI_COMMON_Q_ENABLE is written [virtio-pci] -> set enabled, reset status of vq. vhost-kernel: 1. VIRTIO_PCI_COMMON_Q_RESET is written [virtio-pci] -> virtio_queue_reset() [virtio] -> virtio_net_queue_reset() [virtio-net] -> vhost_net_virtqueue_stop() [vhost-net] -> vhost_net_set_backend() [vhost] -> vhost_virtqueue_stop() -> __virtio_queue_reset() 2. VIRTIO_PCI_COMMON_Q_ENABLE is written [virtio-pci] -> virtio_queue_enable() [virtio] -> virtio_net_queue_enable() [virtio-net] -> vhost_net_virtqueue_restart() [vhost-net] -> vhost_virtqueue_start() [vhost] -> vhost_net_set_backend() -> set enabled, reset status of vq. Test environment and method: Host: 5.19.0-rc3 (With vq reset support) Qemu: QEMU emulator version 7.0.50 Guest: 5.19.0-rc3 (With vq reset support) Test Cmd: ethtool -g eth1; ethtool -G eth1 rx $1 tx $2; ethtool -g eth1; The drvier can resize the virtio queue, then virtio queue reset function should be triggered. The default is split mode, modify Qemu virtio-net to add PACKED feature to test packed mode. Guest Kernel Patch: https://lore.kernel.org/bpf/20220801063902.129329-1-xuanz...@linux.alibaba.com/ Host Kernel Patch: https://lore.kernel.org/bpf/20220825085610.80315-1-kangjie...@linux.alibaba.com/ Looking forward to your review and comments. Thanks. changelog: v5: 1. vhost_net_virtqueue_restart() use -EBUSY replace -ENOTSUP. @Jason 2. reuse VHOST_FILE_UNBIND. @Jason v4: 1. Add explanation for preventing userspace datapath in virtio-net. 2. Return error when vhost is not started in vhost_net_virtqueue_restart(). 3. Reset the virtqueue in the device reusing vhost_virtqueue_stop(). 4. Disable queue reset feature for pre-7.2 machine. v3: 1. Remove support for vhost-user in this series and refactor the code. 2. Rename 'vhost_net_virtqueue_stop' to 'vhost_net_virtqueue_reset'. 3. Make PCI transport ready before device ready when queue_enabled is set to true. 4. Add some comments. v2: 1. Add support for vhost-net kernel scenario. 2. Add a new vhost-user message VHOST_USER_RESET_VRING. 3. Add migration compatibility for virtqueue reset. Kangjie Xu (10): virtio: introduce virtio_queue_enable() virtio: core: vq reset feature negotation support virtio-pci: support queue enable vhost: expose vhost_virtqueue_start() vhost: expose vhost_virtqueue_stop() vhost-net: vhost-kernel: introduce vhost_net_virtqueue_reset() vhost-net: vhost-kernel: introduce vhost_net_virtqueue_restart() virtio-net: introduce flush_or_purge_queued_packets() virtio-net: support queue_enable vhost: vhost-kernel: enable vq reset feature Xuan Zhuo (5): virtio: sync relevant definitions with linux virtio: introduce __virtio_queue_reset() virtio: introduce virtio_queue_reset() virtio-pci: support queue reset virtio-net: support queue reset hw/core/machine.c | 4 +- hw/net/vhost_net.c| 79 +++ hw/net/virtio-net.c | 56 +++-- hw/virtio/vhost.c | 16 ++-- hw/virtio/virtio-pci.c| 16 hw/virtio/virtio.c| 62 +++ include/hw/virtio/vhost.h | 5 ++ include/hw/virtio/virtio-pci.h| 5 ++ include/hw/virtio/virtio.h| 8 +- include/net/vhost_net.h | 4 + .../standard-headers/linux/virtio_config.h| 5 ++ include/standard-headers/linux/virtio_pci.h | 2 + 12 files changed, 230 insertions(+), 32 deletions(-) -- 2.32.0.3.g01195cf9f
[PATCH v5 13/15] virtio-net: support queue reset
virtio-net and vhost-kernel implement queue reset.
Queued packets in the corresponding queue pair are flushed
or purged.
For virtio-net, userspace datapath will be disabled later in
__virtio_queue_reset(). It will set addr of vring to 0 and idx to 0.
Thus, virtio_net_receive() and virtio_net_flush_tx() will not receive
or send packets.
For vhost-net, the datapath will be disabled in vhost_net_virtqueue_reset().
Signed-off-by: Xuan Zhuo
Signed-off-by: Kangjie Xu
Acked-by: Jason Wang
---
hw/net/virtio-net.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 27b59c0ad6..d774a3e652 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -540,6 +540,23 @@ static RxFilterInfo
*virtio_net_query_rxfilter(NetClientState *nc)
return info;
}
+static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
+{
+VirtIONet *n = VIRTIO_NET(vdev);
+NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
+
+if (!nc->peer) {
+return;
+}
+
+if (get_vhost_net(nc->peer) &&
+nc->peer->info->type == NET_CLIENT_DRIVER_TAP) {
+vhost_net_virtqueue_reset(vdev, nc, queue_index);
+}
+
+flush_or_purge_queued_packets(nc);
+}
+
static void virtio_net_reset(VirtIODevice *vdev)
{
VirtIONet *n = VIRTIO_NET(vdev);
@@ -3784,6 +3801,7 @@ static void virtio_net_class_init(ObjectClass *klass,
void *data)
vdc->set_features = virtio_net_set_features;
vdc->bad_features = virtio_net_bad_features;
vdc->reset = virtio_net_reset;
+vdc->queue_reset = virtio_net_queue_reset;
vdc->set_status = virtio_net_set_status;
vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
--
2.32.0.3.g01195cf9f
[PATCH v5 11/15] vhost-net: vhost-kernel: introduce vhost_net_virtqueue_restart()
From: Kangjie Xu
Introduce vhost_net_virtqueue_restart(), which can restart the
specific virtqueue when the vhost net started running before.
If it fails to restart the virtqueue, the device will be stopped.
Here we do not reuse vhost_net_start_one() or vhost_dev_start()
because they work at queue pair level. The mem table and features
do not change, so we can call the vhost_virtqueue_start() to
restart a specific queue.
This patch only considers the case of vhost-kernel, when
NetClientDriver is NET_CLIENT_DRIVER_TAP.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
---
hw/net/vhost_net.c | 53 +
include/net/vhost_net.h | 2 ++
2 files changed, 55 insertions(+)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 8beecb4d22..d2926e2ed6 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -34,6 +34,7 @@
#include "standard-headers/linux/virtio_ring.h"
#include "hw/virtio/vhost.h"
#include "hw/virtio/virtio-bus.h"
+#include "linux-headers/linux/vhost.h"
/* Features supported by host kernel. */
@@ -556,3 +557,55 @@ void vhost_net_virtqueue_reset(VirtIODevice *vdev,
NetClientState *nc,
net->dev.vqs + idx,
net->dev.vq_index + idx);
}
+
+int vhost_net_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
+int vq_index)
+{
+VHostNetState *net = get_vhost_net(nc->peer);
+const VhostOps *vhost_ops = net->dev.vhost_ops;
+struct vhost_vring_file file = { };
+int idx, r;
+
+if (!net->dev.started) {
+return -EBUSY;
+}
+
+/* should only be called after backend is connected */
+assert(vhost_ops);
+
+idx = vhost_ops->vhost_get_vq_index(>dev, vq_index);
+
+r = vhost_virtqueue_start(>dev,
+ vdev,
+ net->dev.vqs + idx,
+ net->dev.vq_index + idx);
+if (r < 0) {
+goto err_start;
+}
+
+if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
+file.index = idx;
+file.fd = net->backend;
+r = vhost_net_set_backend(>dev, );
+if (r < 0) {
+r = -errno;
+goto err_start;
+}
+}
+
+return 0;
+
+err_start:
+error_report("Error when restarting the queue.");
+
+if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
+file.fd = VHOST_FILE_UNBIND;
+file.index = idx;
+int r = vhost_net_set_backend(>dev, );
+assert(r >= 0);
+}
+
+vhost_dev_stop(>dev, vdev);
+
+return r;
+}
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 85d85a4957..40b9a40074 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -50,4 +50,6 @@ int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
int vq_index);
+int vhost_net_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
+int vq_index);
#endif
--
2.32.0.3.g01195cf9f
[PATCH v5 07/15] virtio-pci: support queue enable
From: Kangjie Xu
PCI devices support device specific vq enable.
Based on this function, the driver can re-enable the virtqueue after the
virtqueue is reset.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/virtio/virtio-pci.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 79b9e641dd..a53b5d9f1e 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1342,6 +1342,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr
addr,
proxy->vqs[vdev->queue_sel].used[0]);
proxy->vqs[vdev->queue_sel].enabled = 1;
proxy->vqs[vdev->queue_sel].reset = 0;
+virtio_queue_enable(vdev, vdev->queue_sel);
} else {
virtio_error(vdev, "wrong value for queue_enable %"PRIx64, val);
}
--
2.32.0.3.g01195cf9f
[PATCH v5 01/15] virtio: sync relevant definitions with linux
This is updated using scripts/update-linux-headers.sh.
Added VIRTIO_F_RING_RESET, VIRTIO_PCI_COMMON_Q_RESET. It came from here:
https://github.com/oasis-tcs/virtio-spec/issues/124
https://github.com/oasis-tcs/virtio-spec/issues/139
Add VIRTIO_PCI_COMMON_Q_NDATA, which comes from here:
https://github.com/oasis-tcs/virtio-spec/issues/89
Signed-off-by: Xuan Zhuo
Signed-off-by: Kangjie Xu
Acked-by: Jason Wang
---
include/standard-headers/linux/virtio_config.h | 5 +
include/standard-headers/linux/virtio_pci.h| 2 ++
2 files changed, 7 insertions(+)
diff --git a/include/standard-headers/linux/virtio_config.h
b/include/standard-headers/linux/virtio_config.h
index 7acd8d4abc..47a7eef5e4 100644
--- a/include/standard-headers/linux/virtio_config.h
+++ b/include/standard-headers/linux/virtio_config.h
@@ -96,4 +96,9 @@
* Does the device support Single Root I/O Virtualization?
*/
#define VIRTIO_F_SR_IOV37
+
+/*
+ * This feature indicates that the driver can reset a queue individually.
+ */
+#define VIRTIO_F_RING_RESET40
#endif /* _LINUX_VIRTIO_CONFIG_H */
diff --git a/include/standard-headers/linux/virtio_pci.h
b/include/standard-headers/linux/virtio_pci.h
index db7a8e2fcb..be912cfc95 100644
--- a/include/standard-headers/linux/virtio_pci.h
+++ b/include/standard-headers/linux/virtio_pci.h
@@ -202,6 +202,8 @@ struct virtio_pci_cfg_cap {
#define VIRTIO_PCI_COMMON_Q_AVAILHI44
#define VIRTIO_PCI_COMMON_Q_USEDLO 48
#define VIRTIO_PCI_COMMON_Q_USEDHI 52
+#define VIRTIO_PCI_COMMON_Q_NDATA 56
+#define VIRTIO_PCI_COMMON_Q_RESET 58
#endif /* VIRTIO_PCI_NO_MODERN */
--
2.32.0.3.g01195cf9f
[PATCH v5 03/15] virtio: introduce virtio_queue_reset()
Introduce a new interface function virtio_queue_reset() to implement
reset for vq.
Add a new callback to VirtioDeviceClass for queue reset operation for
each child device.
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/virtio/virtio.c | 11 +++
include/hw/virtio/virtio.h | 2 ++
2 files changed, 13 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 67d54832a9..0e9d41366f 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2039,6 +2039,17 @@ static void __virtio_queue_reset(VirtIODevice *vdev,
uint32_t i)
virtio_virtqueue_reset_region_cache(>vq[i]);
}
+void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
+{
+VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+
+if (k->queue_reset) {
+k->queue_reset(vdev, queue_index);
+}
+
+__virtio_queue_reset(vdev, queue_index);
+}
+
void virtio_reset(void *opaque)
{
VirtIODevice *vdev = opaque;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index db1c0ddf6b..879394299b 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -130,6 +130,7 @@ struct VirtioDeviceClass {
void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
void (*reset)(VirtIODevice *vdev);
void (*set_status)(VirtIODevice *vdev, uint8_t val);
+void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
/* For transitional devices, this is a bitmap of features
* that are only exposed on the legacy interface but not
* the modern one.
@@ -268,6 +269,7 @@ int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev,
int n,
MemoryRegion *mr, bool assign);
int virtio_set_status(VirtIODevice *vdev, uint8_t val);
void virtio_reset(void *opaque);
+void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
void virtio_update_irq(VirtIODevice *vdev);
int virtio_set_features(VirtIODevice *vdev, uint64_t val);
--
2.32.0.3.g01195cf9f
[PATCH v5 06/15] virtio-pci: support queue reset
PCI devices support vq reset.
Based on this function, the driver can adjust the size of the ring, and
quickly recycle the buffer in the ring.
The migration of the virtio devices will not happen during a reset
operation. This is becuase the global iothread lock is held. Migration
thread also needs the lock. As a result, when migration of virtio
devices starts, the 'reset' status of VirtIOPCIQueue will always be 0.
Thus, we do not need to add it in vmstate_virtio_pci_modern_queue_state.
Signed-off-by: Xuan Zhuo
Signed-off-by: Kangjie Xu
Acked-by: Jason Wang
---
hw/virtio/virtio-pci.c | 15 +++
include/hw/virtio/virtio-pci.h | 5 +
2 files changed, 20 insertions(+)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index a50c5a57d7..79b9e641dd 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1251,6 +1251,9 @@ static uint64_t virtio_pci_common_read(void *opaque,
hwaddr addr,
case VIRTIO_PCI_COMMON_Q_USEDHI:
val = proxy->vqs[vdev->queue_sel].used[1];
break;
+case VIRTIO_PCI_COMMON_Q_RESET:
+val = proxy->vqs[vdev->queue_sel].reset;
+break;
default:
val = 0;
}
@@ -1338,6 +1341,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr
addr,
((uint64_t)proxy->vqs[vdev->queue_sel].used[1]) << 32 |
proxy->vqs[vdev->queue_sel].used[0]);
proxy->vqs[vdev->queue_sel].enabled = 1;
+proxy->vqs[vdev->queue_sel].reset = 0;
} else {
virtio_error(vdev, "wrong value for queue_enable %"PRIx64, val);
}
@@ -1360,6 +1364,16 @@ static void virtio_pci_common_write(void *opaque, hwaddr
addr,
case VIRTIO_PCI_COMMON_Q_USEDHI:
proxy->vqs[vdev->queue_sel].used[1] = val;
break;
+case VIRTIO_PCI_COMMON_Q_RESET:
+if (val == 1) {
+proxy->vqs[vdev->queue_sel].reset = 1;
+
+virtio_queue_reset(vdev, vdev->queue_sel);
+
+proxy->vqs[vdev->queue_sel].reset = 0;
+proxy->vqs[vdev->queue_sel].enabled = 0;
+}
+break;
default:
break;
}
@@ -1954,6 +1968,7 @@ static void virtio_pci_reset(DeviceState *qdev)
for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
proxy->vqs[i].enabled = 0;
+proxy->vqs[i].reset = 0;
proxy->vqs[i].num = 0;
proxy->vqs[i].desc[0] = proxy->vqs[i].desc[1] = 0;
proxy->vqs[i].avail[0] = proxy->vqs[i].avail[1] = 0;
diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
index 2446dcd9ae..938799e8f6 100644
--- a/include/hw/virtio/virtio-pci.h
+++ b/include/hw/virtio/virtio-pci.h
@@ -117,6 +117,11 @@ typedef struct VirtIOPCIRegion {
typedef struct VirtIOPCIQueue {
uint16_t num;
bool enabled;
+ /*
+ * No need to migrate the reset status, because it is always 0
+ * when the migration starts.
+ */
+ bool reset;
uint32_t desc[2];
uint32_t avail[2];
uint32_t used[2];
--
2.32.0.3.g01195cf9f
[PATCH v5 15/15] vhost: vhost-kernel: enable vq reset feature
From: Kangjie Xu
Add virtqueue reset feature for vhost-kernel.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
---
hw/net/vhost_net.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index d2926e2ed6..53b2fac4f6 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -47,6 +47,7 @@ static const int kernel_feature_bits[] = {
VIRTIO_NET_F_MTU,
VIRTIO_F_IOMMU_PLATFORM,
VIRTIO_F_RING_PACKED,
+VIRTIO_F_RING_RESET,
VIRTIO_NET_F_HASH_REPORT,
VHOST_INVALID_FEATURE_BIT
};
--
2.32.0.3.g01195cf9f
[PATCH v5 12/15] virtio-net: introduce flush_or_purge_queued_packets()
From: Kangjie Xu
Introduce the fucntion flush_or_purge_queued_packets(), it will be
used in device reset and virtqueue reset. Therefore, we extract the
common logic as a new function.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/net/virtio-net.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index dd0d056fde..27b59c0ad6 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -118,6 +118,16 @@ static int vq2q(int queue_index)
return queue_index / 2;
}
+static void flush_or_purge_queued_packets(NetClientState *nc)
+{
+if (!nc->peer) {
+return;
+}
+
+qemu_flush_or_purge_queued_packets(nc->peer, true);
+assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
+}
+
/* TODO
* - we could suppress RX interrupt if we were so inclined.
*/
@@ -560,12 +570,7 @@ static void virtio_net_reset(VirtIODevice *vdev)
/* Flush any async TX */
for (i = 0; i < n->max_queue_pairs; i++) {
-NetClientState *nc = qemu_get_subqueue(n->nic, i);
-
-if (nc->peer) {
-qemu_flush_or_purge_queued_packets(nc->peer, true);
-assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
-}
+flush_or_purge_queued_packets(qemu_get_subqueue(n->nic, i));
}
}
--
2.32.0.3.g01195cf9f
[PATCH v5 08/15] vhost: expose vhost_virtqueue_start()
From: Kangjie Xu
Expose vhost_virtqueue_start(), we need to use it when restarting a
virtqueue.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/virtio/vhost.c | 8
include/hw/virtio/vhost.h | 3 +++
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index f758f177bb..7900bc81ab 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1081,10 +1081,10 @@ out:
return ret;
}
-static int vhost_virtqueue_start(struct vhost_dev *dev,
-struct VirtIODevice *vdev,
-struct vhost_virtqueue *vq,
-unsigned idx)
+int vhost_virtqueue_start(struct vhost_dev *dev,
+ struct VirtIODevice *vdev,
+ struct vhost_virtqueue *vq,
+ unsigned idx)
{
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
VirtioBusState *vbus = VIRTIO_BUS(qbus);
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index a346f23d13..b092f57d98 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -279,6 +279,9 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write);
+int vhost_virtqueue_start(struct vhost_dev *dev, struct VirtIODevice *vdev,
+ struct vhost_virtqueue *vq, unsigned idx);
+
void vhost_dev_reset_inflight(struct vhost_inflight *inflight);
void vhost_dev_free_inflight(struct vhost_inflight *inflight);
void vhost_dev_save_inflight(struct vhost_inflight *inflight, QEMUFile *f);
--
2.32.0.3.g01195cf9f
[PATCH v5 05/15] virtio: core: vq reset feature negotation support
From: Kangjie Xu
A a new command line parameter "queue_reset" is added.
Meanwhile, the vq reset feature is disabled for pre-7.2 machines.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/core/machine.c | 4 +++-
include/hw/virtio/virtio.h | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index aa520e74a8..907fa78ff0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -40,7 +40,9 @@
#include "hw/virtio/virtio-pci.h"
#include "qom/object_interfaces.h"
-GlobalProperty hw_compat_7_1[] = {};
+GlobalProperty hw_compat_7_1[] = {
+{ "virtio-device", "queue_reset", "false" },
+};
const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
GlobalProperty hw_compat_7_0[] = {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 085997d8f3..ed3ecbef80 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -295,7 +295,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
VIRTIO_F_IOMMU_PLATFORM, false), \
DEFINE_PROP_BIT64("packed", _state, _field, \
- VIRTIO_F_RING_PACKED, false)
+ VIRTIO_F_RING_PACKED, false), \
+DEFINE_PROP_BIT64("queue_reset", _state, _field, \
+ VIRTIO_F_RING_RESET, true)
hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
--
2.32.0.3.g01195cf9f
[PATCH v5 10/15] vhost-net: vhost-kernel: introduce vhost_net_virtqueue_reset()
From: Kangjie Xu
Introduce vhost_virtqueue_reset(), which can reset the specific
virtqueue in the device. Then it will unmap vrings and the desc
of the virtqueue.
Here we do not reuse the vhost_net_stop_one() or vhost_dev_stop(),
because they work at queue pair level. We do not use
vhost_virtqueue_stop() because it may stop the device in the
backend.
This patch only considers the case of vhost-kernel, when
NetClientDriver is NET_CLIENT_DRIVER_TAP.
Furthermore, we do not need net->nc->info->poll() because
it enables userspace datapath and we want to stop all
datapaths for this reset virtqueue here.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/net/vhost_net.c | 25 +
include/net/vhost_net.h | 2 ++
2 files changed, 27 insertions(+)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index d28f8b974b..8beecb4d22 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -531,3 +531,28 @@ int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu)
return vhost_ops->vhost_net_set_mtu(>dev, mtu);
}
+
+void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
+ int vq_index)
+{
+VHostNetState *net = get_vhost_net(nc->peer);
+const VhostOps *vhost_ops = net->dev.vhost_ops;
+struct vhost_vring_file file = { .fd = -1 };
+int idx;
+
+/* should only be called after backend is connected */
+assert(vhost_ops);
+
+idx = vhost_ops->vhost_get_vq_index(>dev, vq_index);
+
+if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
+file.index = idx;
+int r = vhost_net_set_backend(>dev, );
+assert(r >= 0);
+}
+
+vhost_virtqueue_stop(>dev,
+ vdev,
+ net->dev.vqs + idx,
+ net->dev.vq_index + idx);
+}
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 387e913e4e..85d85a4957 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -48,4 +48,6 @@ uint64_t vhost_net_get_acked_features(VHostNetState *net);
int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
+void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
+ int vq_index);
#endif
--
2.32.0.3.g01195cf9f
[PATCH v5 04/15] virtio: introduce virtio_queue_enable()
From: Kangjie Xu
Introduce the interface queue_enable() in VirtioDeviceClass and the
fucntion virtio_queue_enable() in virtio, it can be called when
VIRTIO_PCI_COMMON_Q_ENABLE is written and related virtqueue can be
started. It only supports the devices of virtio 1 or later. The
not-supported devices can only start the virtqueue when DRIVER_OK.
Signed-off-by: Kangjie Xu
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/virtio/virtio.c | 14 ++
include/hw/virtio/virtio.h | 2 ++
2 files changed, 16 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 0e9d41366f..141f18c633 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2050,6 +2050,20 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t
queue_index)
__virtio_queue_reset(vdev, queue_index);
}
+void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
+{
+VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+
+if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+error_report("queue_enable is only suppported in devices of virtio "
+ "1.0 or later.");
+}
+
+if (k->queue_enable) {
+k->queue_enable(vdev, queue_index);
+}
+}
+
void virtio_reset(void *opaque)
{
VirtIODevice *vdev = opaque;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 879394299b..085997d8f3 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -131,6 +131,7 @@ struct VirtioDeviceClass {
void (*reset)(VirtIODevice *vdev);
void (*set_status)(VirtIODevice *vdev, uint8_t val);
void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
+void (*queue_enable)(VirtIODevice *vdev, uint32_t queue_index);
/* For transitional devices, this is a bitmap of features
* that are only exposed on the legacy interface but not
* the modern one.
@@ -270,6 +271,7 @@ int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev,
int n,
int virtio_set_status(VirtIODevice *vdev, uint8_t val);
void virtio_reset(void *opaque);
void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
+void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
void virtio_update_irq(VirtIODevice *vdev);
int virtio_set_features(VirtIODevice *vdev, uint64_t val);
--
2.32.0.3.g01195cf9f
[PATCH v5 02/15] virtio: introduce __virtio_queue_reset()
Separate the logic of vq reset. This logic will be called directly
later.
Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
---
hw/virtio/virtio.c | 37 +
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5d607aeaa0..67d54832a9 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2019,6 +2019,26 @@ static enum virtio_device_endian
virtio_current_cpu_endian(void)
}
}
+static void __virtio_queue_reset(VirtIODevice *vdev, uint32_t i)
+{
+vdev->vq[i].vring.desc = 0;
+vdev->vq[i].vring.avail = 0;
+vdev->vq[i].vring.used = 0;
+vdev->vq[i].last_avail_idx = 0;
+vdev->vq[i].shadow_avail_idx = 0;
+vdev->vq[i].used_idx = 0;
+vdev->vq[i].last_avail_wrap_counter = true;
+vdev->vq[i].shadow_avail_wrap_counter = true;
+vdev->vq[i].used_wrap_counter = true;
+virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
+vdev->vq[i].signalled_used = 0;
+vdev->vq[i].signalled_used_valid = false;
+vdev->vq[i].notification = true;
+vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
+vdev->vq[i].inuse = 0;
+virtio_virtqueue_reset_region_cache(>vq[i]);
+}
+
void virtio_reset(void *opaque)
{
VirtIODevice *vdev = opaque;
@@ -2050,22 +2070,7 @@ void virtio_reset(void *opaque)
virtio_notify_vector(vdev, vdev->config_vector);
for(i = 0; i < VIRTIO_QUEUE_MAX; i++) {
-vdev->vq[i].vring.desc = 0;
-vdev->vq[i].vring.avail = 0;
-vdev->vq[i].vring.used = 0;
-vdev->vq[i].last_avail_idx = 0;
-vdev->vq[i].shadow_avail_idx = 0;
-vdev->vq[i].used_idx = 0;
-vdev->vq[i].last_avail_wrap_counter = true;
-vdev->vq[i].shadow_avail_wrap_counter = true;
-vdev->vq[i].used_wrap_counter = true;
-virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
-vdev->vq[i].signalled_used = 0;
-vdev->vq[i].signalled_used_valid = false;
-vdev->vq[i].notification = true;
-vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
-vdev->vq[i].inuse = 0;
-virtio_virtqueue_reset_region_cache(>vq[i]);
+__virtio_queue_reset(vdev, i);
}
}
--
2.32.0.3.g01195cf9f
Re: [PATCH 00/24] Support VIRTIO_F_RING_RESET for virtio-net, vhost-user, vhost-kernel in virtio pci-modern
On Tue, 16 Aug 2022 02:22:16 -0400, "Michael S. Tsirkin" wrote: > On Tue, Aug 16, 2022 at 02:15:57PM +0800, Xuan Zhuo wrote: > > On Tue, 16 Aug 2022 02:14:10 -0400, "Michael S. Tsirkin" > > wrote: > > > On Tue, Aug 16, 2022 at 09:06:12AM +0800, Kangjie Xu wrote: > > > > The virtio queue reset function has already been defined in the virtio > > > > spec 1.2. > > > > The relevant virtio spec information is here: > > > > > > > > https://github.com/oasis-tcs/virtio-spec/issues/124 > > > > https://github.com/oasis-tcs/virtio-spec/issues/139 > > > > > > > > This patch set is to support this function in QEMU. It consists of > > > > several parts: > > > > 1. Patches 1-7 are the basic interfaces for vq reset in virtio and > > > > virtio-pci. > > > > 2. Patches 8-12 support vq stop and vq restart for vhost-kernel. > > > > 3. Patches 13-19 support vq stop and vq restart for vhost-user. > > > > 4. Patches 20-22 support vq reset and re-enable for virtio-net. > > > > 5. Patches 23-24 enable the vq reset feature for vhost-kernel and > > > > vhost-user. > > > > > > > > The process of virtqueue reset can be concluded as: > > > > 1. The virtqueue is disabled when VIRTIO_PCI_COMMON_Q_RESET is written. > > > > 2. Then the virtqueue can be optionally restarted(re-enabled). > > > > > > > > Since this patch set involves multiple modules and seems a bit messy, > > > > we briefly describe the > > > > calling process for different modes below. > > > > virtio-net: > > > > 1. VIRTIO_PCI_COMMON_Q_RESET is written [virtio-pci] > > > > -> virtio_queue_reset() [virtio] > > > > -> virtio_net_queue_reset() [virtio-net] > > > > -> __virtio_queue_reset() > > > > 2. VIRTIO_PCI_COMMON_Q_ENABLE is written [virtio-pci] > > > > -> set enabled, reset status of vq. > > > > > > > > vhost-kernel: > > > > 1. VIRTIO_PCI_COMMON_Q_RESET is written [virtio-pci] > > > > -> virtio_queue_reset() [virtio] > > > > -> virtio_net_queue_reset() [virtio-net] > > > > -> vhost_net_virtqueue_stop() [vhost-net] > > > > -> vhost_net_set_backend() [vhost] > > > > -> vhost_dev_virtqueue_stop() > > > > -> vhost_virtqueue_unmap() > > > > -> __virtio_queue_reset() > > > > 2. VIRTIO_PCI_COMMON_Q_ENABLE is written [virtio-pci] > > > > -> virtio_queue_enable() [virtio] > > > > -> virtio_net_queue_enable() [virtio-net] > > > > -> vhost_net_virtqueue_restart() [vhost-net] > > > > -> vhost_dev_virtqueue_restart() [vhost] > > > > -> vhost_virtqueue_start() > > > > -> vhost_net_set_backend() > > > > -> set enabled, reset status of vq. > > > > > > > > vhost-user: > > > > 1. VIRTIO_PCI_COMMON_Q_RESET is written [virtio-pci] > > > > -> virtio_queue_reset() [virtio] > > > > -> virtio_net_queue_reset() [virtio-net] > > > > -> vhost_net_virtqueue_stop() [vhost-net] > > > > -> vhost_dev_virtqueue_stop() [vhost] > > > > -> vhost_user_reset_vring() [vhost-user] > > > > -> send VHOST_USER_RESET_VRING to the device > > > > -> vhost_virtqueue_unmap() > > > > -> __virtio_queue_reset() > > > > 2. VIRTIO_PCI_COMMON_Q_ENABLE is written [virtio-pci] > > > > -> virtio_queue_enable() [virtio] > > > > -> virtio_net_queue_enable() [virtio-net] > > > > -> vhost_net_virtqueue_restart() [vhost-net] > > > > -> vhost_dev_virtqueue_restart() [vhost] > > > > -> vhost_virtqueue_start() > > > > -> vhost_user_set_single_vring_enable [vhost-user] > > > > -> send VHOST_USER_SET_VRING_ENABLE to the > > > > device > > > > -> set enabled, reset status of vq. > > > > > > > > > > > > Test environment: > > > > Host: 5.19.0-rc3 (With vq reset support) > > > >
Re: [PATCH 00/24] Support VIRTIO_F_RING_RESET for virtio-net, vhost-user, vhost-kernel in virtio pci-modern
> > Host Kernel Patch: > > > > https://github.com/middaywords/linux/commit/19a91e0d7167b2031e46078c6215c213b89cb2c3 > > > > Looking forward to your review and comments. Thanks. > > > > Kangjie Xu (19): > > virtio: introduce virtio_queue_enable() > > virtio: core: vq reset feature negotation support > > virtio-pci: support queue enable > > vhost: extract the logic of unmapping the vrings and desc > > vhost: introduce vhost_dev_virtqueue_stop() > > vhost: introduce vhost_dev_virtqueue_restart() > > vhost-net: vhost-kernel: introduce vhost_net_virtqueue_stop() > > vhost-net: vhost-kernel: introduce vhost_net_virtqueue_restart() > > docs: vhost-user: add VHOST_USER_RESET_VRING message > > vhost-user: introduce vhost_reset_vring() interface > > vhost-user: add op to enable or disable a single vring > > vhost: vhost-user: update vhost_dev_virtqueue_stop() > > vhost: vhost-user: update vhost_dev_virtqueue_restart() > > vhost-net: vhost-user: update vhost_net_virtqueue_stop() > > vhost-net: vhost-user: update vhost_net_virtqueue_restart() > > virtio-net: introduce flush_or_purge_queued_packets() > > virtio-net: support queue_enable > > vhost: vhost-kernel: enable vq reset feature > > vhost: vhost-user: enable vq reset feature > > > > Xuan Zhuo (5): > > virtio: sync relevant definitions with linux > > virtio: introduce __virtio_queue_reset() > > virtio: introduce virtio_queue_reset() > > virtio-pci: support queue reset > > virtio-net: support queue reset > > > > docs/interop/vhost-user.rst | 10 +++ > > hw/core/machine.c | 1 + > > hw/net/vhost_net.c| 79 +++ > > hw/net/virtio-net.c | 58 -- > > hw/virtio/vhost-user.c| 67 ++-- > > hw/virtio/vhost.c | 79 +-- > > hw/virtio/virtio-pci.c| 20 + > > hw/virtio/virtio.c| 62 +++ > > include/hw/virtio/vhost-backend.h | 6 ++ > > include/hw/virtio/vhost.h | 5 ++ > > include/hw/virtio/virtio-pci.h| 1 + > > include/hw/virtio/virtio.h| 8 +- > > include/net/vhost_net.h | 4 + > > .../standard-headers/linux/virtio_config.h| 5 ++ > > include/standard-headers/linux/virtio_pci.h | 2 + > > 15 files changed, 371 insertions(+), 36 deletions(-) > > > > -- > > 2.32.0 >
