Re: Re: [PATCH V1] vdpa_sim: reset must not run

[Stefano Garzarella]

On Mon, Jan 22, 2024 at 11:47:22AM +0100, Eugenio Perez Martin wrote:
> On Mon, Jan 22, 2024 at 11:22 AM Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote:
> > On Wed, Jan 17, 2024 at 11:23:23AM -0800, Steve Sistare wrote:
> > >vdpasim_do_reset sets running to true, which is wrong, as it allows
> > >vdpasim_kick_vq to post work requests before the device has been
> > >configured.  To fix, do not set running until VIRTIO_CONFIG_S_FEATURES_OK
> > >is set.
> > >
> > >Fixes: 0c89e2a3a9d0 ("vdpa_sim: Implement suspend vdpa op")
> > >Signed-off-by: Steve Sistare <steven.sistare@xxxxxxxxxx>
> > >Reviewed-by: Eugenio Pérez <eperezma@xxxxxxxxxx>
> > >---
> > > drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > >diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > >index be2925d0d283..6304cb0b4770 100644
> > >--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > >+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > >@@ -160,7 +160,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim, u32 flags)
> > >               }
> > >       }
> > >
> > >-      vdpasim->running = true;
> > >+      vdpasim->running = false;
> > >       spin_unlock(&vdpasim->iommu_lock);
> > >
> > >       vdpasim->features = 0;
> > >@@ -483,6 +483,7 @@ static void vdpasim_set_status(struct vdpa_device *vdpa, u8 status)
> > >
> > >       mutex_lock(&vdpasim->mutex);
> > >       vdpasim->status = status;
> > >+      vdpasim->running = (status & VIRTIO_CONFIG_S_FEATURES_OK) != 0;
> > >       mutex_unlock(&vdpasim->mutex);
> >
> > Should we do something similar also in vdpasim_resume() ?
> >
> > I mean something like this:
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > index be2925d0d283..55e4633d5442 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > @@ -520,7 +520,7 @@ static int vdpasim_resume(struct vdpa_device *vdpa)
> >          int i;
> >
> >          mutex_lock(&vdpasim->mutex);
> > -       vdpasim->running = true;
> > +       vdpasim->running = (vdpasim->status & VIRTIO_CONFIG_S_FEATURES_OK) != 0;
> >
> >          if (vdpasim->pending_kick) {
> >                  /* Process pending descriptors */
> >
> > Thanks,
> > Stefano
>
> The suspend and resume operation should not be called before
> DRIVER_OK, so maybe we should add that protection at
> drivers/vhost/vdpa.c actually?

Yeah, I think so!

Anyway, IMHO we should at least return an error in vdpa_sim if 
vdpasim_suspend/resume are called before DRIVER_OK (in another patch of 
course).

Stefano