Re: [PATCH] virtio-net: not enable vq reset feature unconditionally

[Eugenio Perez Martin]

On Mon, May 8, 2023 at 8:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, May 08, 2023 at 07:31:35PM +0200, Eugenio Perez Martin wrote:
> > On Mon, May 8, 2023 at 12:22 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, May 08, 2023 at 11:09:46AM +0200, Eugenio Perez Martin wrote:
> > > > On Sat, May 6, 2023 at 4:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 
> > > > wrote:
> > > > >
> > > > > On Thu,  4 May 2023 12:14:47 +0200, =?utf-8?q?Eugenio_P=C3=A9rez?= 
> > > > > <eperezma@redhat.com> 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 <eperezma@redhat.com>
> > > > > >
> > > > > > ---
> > > > > > 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.
> > > >
> > > > Thanks!
> > >
> > >
> > > question is this:
> > >
> > >     DEFINE_PROP_BIT64("queue_reset", _state, _field, \
> > >                       VIRTIO_F_RING_RESET, true)
> > >
> > >
> > >
> > > don't we need compat for 7.2 and back for this property?
> > >
> >
> > I think that part is already covered by commit 69e1c14aa222 ("virtio:
> > core: vq reset feature negotation support"). In that regard, maybe we
> > can simplify the patch message simply stating that queue_reset=off
> > does not work.
> >
> > Thanks!
>
> that compat for 7.1 and not 7.2 though? is that correct?
>

Yes. queue_reset support was added for 7.2 and there are cases where
it can be on or off, like using vhost=on.

If a new rhel 7.2 guest starts with cmdline vhost=off queue_reset=off,
both the guest and device model still see queue_reset=on, so they will
migrate with queue_reset=on. In the case of a migration to a current
qemu master with cmdline vhost=off queue_reset=off, dst qemu will
complain and block the migration (untested).

I think this is the least bad of all the bad behaviors, as a sudden
change to honor cmdline will cause a change of the device features
that the guest sees. I'm not sure if there are better ways to
accomplish this.

Maybe I'm missing something?

Thanks!