Re: [PATCH v2] hw/net/virtio-net: add support for notification coalescing

[Eugenio Perez Martin <eperezma@redhat.com>]

On Mon, Mar 23, 2026 at 9:39 AM Koushik Dutta <kdutta@redhat.com> wrote:
>
> Hi Eugenio,
> Regarding the point about not enabling VIRTIO_NET_F_NOTF_COAL unconditionally: since feature negotiation happens during VM bootup and coalescing is configured via ethtool after the VM has already booted, how would we handle this under a conditional property?
>
> If the feature isn't negotiated at the start, we wouldn't be able to enable it dynamically later. Could you clarify how you'd like the command-line regulation to interact with the guest's ability to enable it later?

Replying at the position of the comment, marked with [1]. Please
interleave your reply for the next time, as it makes revision easier
:) [2].

> ------------------------------------------------------------
>
> > @@ -37,6 +37,7 @@ bool vhost_svq_valid_features(uint64_t features, Error **errp)
> >          case VIRTIO_RING_F_INDIRECT_DESC:
> >              continue;
> >
> > +        case VIRTIO_F_RING_RESET:
>
> This patch does not handle VIRTIO_F_RING_RESET, so this should not be enabled.
>
> => I included VIRTIO_F_RING_RESET in the valid features list because the vDPA host device offers it by default; without it, SVQ fails the feature negotiation check during initialization.
> qemu-system-x86_64: -netdev type=vhost-vdpa,vhostdev=/dev/vhost-vdpa-0,id=vhost-vdpa0,x-svq=on: SVQ Invalid device feature flags, offer: 0x1e0010330bfffa7, ok: 0x1e0000330bfffa7
>

Since you're testing the feature with nested virtualization, you can
disable the features in the L0 QEMU with:
-device virtio-net-pci,...,queue_reset=off,iommu_platform=off,guest_uso4=off,guest_uso6=off,host_uso=off,guest_announce=off

> Regards,
> Koushik Dutta
>
> On Thu, Mar 12, 2026 at 3:47 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>>
>> On Wed, Mar 11, 2026 at 2:28 PM Koushik Dutta <kdutta@redhat.com> wrote:
>> >

[...]

>> > +
>> > +static void virtio_net_add_queue(VirtIONet *n, int index)
>> > +{
>> > +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>> > +
>> > +    n->vqs[index].rx_vq =
>> > +        virtio_add_queue(vdev,
>> > +                         n->net_conf.rx_queue_size,
>> > +                         virtio_net_handle_rx);
>> > +
>> > +    n->vqs[index].tx_vq =
>> > +        virtio_add_queue(vdev,
>> > +                         n->net_conf.tx_queue_size,
>> > +                         virtio_net_handle_tx_dispatch);
>> > +
>> > +    n->vqs[index].tx_timer =
>> > +        timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> > +                     virtio_net_tx_timer,
>> > +                     &n->vqs[index]);
>>
>> We should avoid creating a new timer if it will not be used by
>> default. Create it only if cmdline enables it or the guest enable it
>> via control vq.
>>

[1]

I'm proposing we switch this to something like:

if (!strcmp(n->net_conf.tx, "timer")) {
  n->vqs[index].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
virtio_net_tx_timer, &n->vqs[index]);
}

And, in the CVQ code that you created,

if (n->tx_coal_usecs) {
  n->vqs[index].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
virtio_net_tx_timer, &n->vqs[index]);
}

And the same applies to RX timers.

[2] https://subspace.kernel.org/etiquette.html#do-not-top-post-when-replying