On 7/11/2023 3:34 PM, Jason Wang wrote:
On Tue, Jul 11, 2023 at 3:25 PM Eugenio Perez Martin
<epere...@redhat.com> wrote:
On Tue, Jul 11, 2023 at 9:05 AM Jason Wang <jasow...@redhat.com>
wrote:
>
> On Tue, Jul 11, 2023 at 12:09 PM Zhu, Lingshan
<lingshan....@intel.com> wrote:
> >
> >
> >
> > On 7/11/2023 10:50 AM, Jason Wang wrote:
> > > On Mon, Jul 10, 2023 at 4:53 PM Zhu Lingshan
<lingshan....@intel.com> wrote:
> > >> In the poweroff routine, no need to fetch last available index.
> > >>
> > > This is because there's no concept of shutdown in the vhost
layer, it
> > > only knows start and stop.
> > >
> > >> This commit also provides a better debug message in the vhost
> > >> caller vhost_virtqueue_stop,
> > > A separate patch is better.
> > OK
> > >
> > >> because if vhost does not fetch
> > >> the last avail idx successfully, maybe the device does not
> > >> suspend, vhost will sync last avail idx to vring used idx as a
> > >> work around, not a failure.
> > > This only happens if we return a negative value?
> > Yes
> > >
> > >> Signed-off-by: Zhu Lingshan <lingshan....@intel.com>
> > >> ---
> > >> hw/virtio/vhost-vdpa.c | 10 ++++++++++
> > >> hw/virtio/vhost.c | 2 +-
> > >> 2 files changed, 11 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > >> index 3c575a9a6e..10b445f64e 100644
> > >> --- a/hw/virtio/vhost-vdpa.c
> > >> +++ b/hw/virtio/vhost-vdpa.c
> > >> @@ -26,6 +26,7 @@
> > >> #include "cpu.h"
> > >> #include "trace.h"
> > >> #include "qapi/error.h"
> > >> +#include "sysemu/runstate.h"
> > >>
> > >> /*
> > >> * Return one past the end of the end of section. Be
careful with uint64_t
> > >> @@ -1391,6 +1392,15 @@ static int
vhost_vdpa_get_vring_base(struct vhost_dev *dev,
> > >> struct vhost_vdpa *v = dev->opaque;
> > >> int ret;
> > >>
> > >> + if (runstate_check(RUN_STATE_SHUTDOWN)) {
> > >> + /*
> > >> + * Some devices do not support this call properly,
> > >> + * and we don't need to retrieve the indexes
> > >> + * if it is shutting down
> > >> + */
> > >> + return 0;
> > > Checking runstate in the vhost code seems like a layer
violation.
> > >
> > > What happens without this patch?
> > vhost tries to fetch vring base,
> > vhost_vdpa needs suspend the device before retrieving
last_avail_idx.
> > However not all devices can support .suspend properly so this call
> > may fail.
>
> I think this is where I'm lost. If the device doesn't support
> suspending, any reason we only try to fix the case of shutdown?
>
> Btw, the fail is intended:
>
> if (!v->suspended) {
> /*
> * Cannot trust in value returned by device, let vhost
recover used
> * idx from guest.
> */
> return -1;
> }
>
The fail is intended, but to recover the last used idx, either from
device or from the guest, is only useful in the case of migration.
Note that we had userspace devices like VDUSE now, so it could be
useful in the case of a VDUSE daemon crash or reconnect.
This code blcok is for vhost_vdpa backend, and I think vduse is another
code path.
Return a guest used idx may be a good idea but as Eugenio pointed out
that may duplicate the code.
I think the main problem is the error message, actually. But I think
it has no use either to recover last_avail_idx or print a debug
message if we're not migrating. Another solution would be to recover
it from the guest at vhost_vdpa_get_vring_base, but I don't like the
duplication.
> And if we return to success here, will we go to set an uninitialized
> last avail idx?
>
It will be either the default value (is set to 0 at
__virtio_queue_reset) or the one received from a migration (at
virtio_load).
0 is even sub-optimal than the index used. Anyhow, VHOST_DEBUG should
not be enabled for production environments.
Returning 0 sounds like a queue reset, yes we can reset a queue if
failed to suspend it, I am not sure whther
0 is better than guest used idx.
I think we are not able to disable VHOST_DEBUG because customers can
build QEMU by their own.
Thanks
Thanks
Thanks!
> r = dev->vhost_ops->vhost_get_vring_base(dev, &state);
> if (r < 0) {
> ...
> }.else {
> virtio_queue_set_last_avail_idx(vdev, idx, state.num);
> }
>
> Thanks
>
> > Then vhost will print an error shows something failed.
> >
> > The error msg is confused, as stated in the commit log, restoring
> > last_avail_idx with guest used idx
> > is a workaround rather than a failure. And no needs to fetch
last_avail_idx
> > when power off.
> >
> > Thanks
> > >
> > > Thanks
> > >
> > >> + }
> > >> +
> > >> if (v->shadow_vqs_enabled) {
> > >> ring->num =
virtio_queue_get_last_avail_idx(dev->vdev, ring->index);
> > >> return 0;
> > >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > >> index 82394331bf..7dd90cff3a 100644
> > >> --- a/hw/virtio/vhost.c
> > >> +++ b/hw/virtio/vhost.c
> > >> @@ -1262,7 +1262,7 @@ void vhost_virtqueue_stop(struct
vhost_dev *dev,
> > >>
> > >> r = dev->vhost_ops->vhost_get_vring_base(dev, &state);
> > >> if (r < 0) {
> > >> - VHOST_OPS_DEBUG(r, "vhost VQ %u ring restore
failed: %d", idx, r);
> > >> + VHOST_OPS_DEBUG(r, "sync last avail idx to the
guest used idx for vhost VQ %u", idx);
> > >> /* Connection to the backend is broken, so let's
sync internal
> > >> * last avail idx to the device used idx.
> > >> */
> > >> --
> > >> 2.39.3
> > >>
> >
>
