Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end

Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end

[Jason Wang]

On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> The device already has a virtio status set by vhost_vdpa_init by the
> time vhost_vdpa_probe_cvq_isolation is called. vhost_vdpa_init set
> S_ACKNOWLEDGE and S_DRIVER, so it is invalid to just reset it.
>
> It is invalid to start the device after it, but all devices seems to be
> fine with it.  Fixing qemu so it follows virtio start procedure.
>
> Fixes: 152128d64697 ("vdpa: move CVQ isolation check to net_init_vhost_vdpa")
> Reported-by: Dragos Tatulea <dtatulea@nvidia.com>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  net/vhost-vdpa.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 9795306742..d7e2b714b4 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -1333,6 +1333,8 @@ static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
>  out:
>      status = 0;
>      ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> +    ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);

So if we fail after FEATURES_OK, this basically clears that bit. Spec
doesn't say it can or not, I wonder if a reset is better?

Btw, spec requires a read of status after setting FEATURES_OK, this
seems to be missed in the current code.

Thanks

>      return r;
>  }
>
> --
> 2.39.3
>

Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end

[Eugenio Perez Martin]

On Wed, Jul 26, 2023 at 4:07 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > The device already has a virtio status set by vhost_vdpa_init by the
> > time vhost_vdpa_probe_cvq_isolation is called. vhost_vdpa_init set
> > S_ACKNOWLEDGE and S_DRIVER, so it is invalid to just reset it.
> >
> > It is invalid to start the device after it, but all devices seems to be
> > fine with it.  Fixing qemu so it follows virtio start procedure.
> >
> > Fixes: 152128d64697 ("vdpa: move CVQ isolation check to net_init_vhost_vdpa")
> > Reported-by: Dragos Tatulea <dtatulea@nvidia.com>
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  net/vhost-vdpa.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index 9795306742..d7e2b714b4 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -1333,6 +1333,8 @@ static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
> >  out:
> >      status = 0;
> >      ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > +    ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
>
> So if we fail after FEATURES_OK, this basically clears that bit. Spec
> doesn't say it can or not, I wonder if a reset is better?
>

I don't follow this, the reset is just above the added code, isn't it?

> Btw, spec requires a read of status after setting FEATURES_OK, this
> seems to be missed in the current code.
>

I'm ok with that, but this patch does not touch that part.

To fix this properly we should:
- Expose vhost_vdpa_set_dev_features_fd as we did in previous versions
of the series that added vhost_vdpa_probe_cvq_isolation [1].
- Get status after vhost_vdpa_add_status, so both vhost start code and
this follows the standard properly.

Is it ok to do these on top of this patch?

Thanks!

[1] https://lore.kernel.org/qemu-devel/20230509154435.1410162-4-eperezma@redhat.com/


> Thanks
>
> >      return r;
> >  }
> >
> > --
> > 2.39.3
> >
>

Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end

[Jason Wang]

On Wed, Jul 26, 2023 at 2:27 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Wed, Jul 26, 2023 at 4:07 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >
> > > The device already has a virtio status set by vhost_vdpa_init by the
> > > time vhost_vdpa_probe_cvq_isolation is called. vhost_vdpa_init set
> > > S_ACKNOWLEDGE and S_DRIVER, so it is invalid to just reset it.
> > >
> > > It is invalid to start the device after it, but all devices seems to be
> > > fine with it.  Fixing qemu so it follows virtio start procedure.
> > >
> > > Fixes: 152128d64697 ("vdpa: move CVQ isolation check to net_init_vhost_vdpa")
> > > Reported-by: Dragos Tatulea <dtatulea@nvidia.com>
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  net/vhost-vdpa.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > index 9795306742..d7e2b714b4 100644
> > > --- a/net/vhost-vdpa.c
> > > +++ b/net/vhost-vdpa.c
> > > @@ -1333,6 +1333,8 @@ static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
> > >  out:
> > >      status = 0;
> > >      ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > > +    ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> >
> > So if we fail after FEATURES_OK, this basically clears that bit. Spec
> > doesn't say it can or not, I wonder if a reset is better?
> >
>
> I don't follow this, the reset is just above the added code, isn't it?

I meant for error path:

E.g:
    uint8_t status = VIRTIO_CONFIG_S_ACKNOWLEDGE |
                     VIRTIO_CONFIG_S_DRIVER |
                     VIRTIO_CONFIG_S_FEATURES_OK;
...
    r = ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
....
        if (cvq_group != -ENOTSUP) {
            r = cvq_group;
            goto out;
        }

out:
    status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
    ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);

We're basically clearing FEATURES_OK?

>
> > Btw, spec requires a read of status after setting FEATURES_OK, this
> > seems to be missed in the current code.
> >
>
> I'm ok with that, but this patch does not touch that part.
>
> To fix this properly we should:
> - Expose vhost_vdpa_set_dev_features_fd as we did in previous versions
> of the series that added vhost_vdpa_probe_cvq_isolation [1].
> - Get status after vhost_vdpa_add_status, so both vhost start code and
> this follows the standard properly.
>
> Is it ok to do these on top of this patch?

Fine.

Thanks

>
> Thanks!
>
> [1] https://lore.kernel.org/qemu-devel/20230509154435.1410162-4-eperezma@redhat.com/
>
>
> > Thanks
> >
> > >      return r;
> > >  }
> > >
> > > --
> > > 2.39.3
> > >
> >
>

Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end

[Eugenio Perez Martin]

On Mon, Jul 31, 2023 at 8:36 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Jul 26, 2023 at 2:27 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Wed, Jul 26, 2023 at 4:07 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > >
> > > > The device already has a virtio status set by vhost_vdpa_init by the
> > > > time vhost_vdpa_probe_cvq_isolation is called. vhost_vdpa_init set
> > > > S_ACKNOWLEDGE and S_DRIVER, so it is invalid to just reset it.
> > > >
> > > > It is invalid to start the device after it, but all devices seems to be
> > > > fine with it.  Fixing qemu so it follows virtio start procedure.
> > > >
> > > > Fixes: 152128d64697 ("vdpa: move CVQ isolation check to net_init_vhost_vdpa")
> > > > Reported-by: Dragos Tatulea <dtatulea@nvidia.com>
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > ---
> > > >  net/vhost-vdpa.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > index 9795306742..d7e2b714b4 100644
> > > > --- a/net/vhost-vdpa.c
> > > > +++ b/net/vhost-vdpa.c
> > > > @@ -1333,6 +1333,8 @@ static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
> > > >  out:
> > > >      status = 0;
> > > >      ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > > +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > > > +    ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > >
> > > So if we fail after FEATURES_OK, this basically clears that bit. Spec
> > > doesn't say it can or not, I wonder if a reset is better?
> > >
> >
> > I don't follow this, the reset is just above the added code, isn't it?
>
> I meant for error path:
>
> E.g:
>     uint8_t status = VIRTIO_CONFIG_S_ACKNOWLEDGE |
>                      VIRTIO_CONFIG_S_DRIVER |
>                      VIRTIO_CONFIG_S_FEATURES_OK;
> ...
>     r = ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> ....
>         if (cvq_group != -ENOTSUP) {
>             r = cvq_group;
>             goto out;
>         }
>
> out:
>     status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
>     ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
>
> We're basically clearing FEATURES_OK?
>

Yes, it is the state that previous functions (vhost_vdpa_init) set. We
need to leave it that way, either if the backend supports cvq
isolation or not, or in the case of an error. Not doing that way makes
vhost_dev_start (and vhost_vdpa_set_features) set the features before
setting VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER.
Otherwise, the guest can (and do) access to config space before
_S_ACKNOWLEDGE | _S_DRIVER.


> >
> > > Btw, spec requires a read of status after setting FEATURES_OK, this
> > > seems to be missed in the current code.
> > >
> >
> > I'm ok with that, but this patch does not touch that part.
> >
> > To fix this properly we should:
> > - Expose vhost_vdpa_set_dev_features_fd as we did in previous versions
> > of the series that added vhost_vdpa_probe_cvq_isolation [1].
> > - Get status after vhost_vdpa_add_status, so both vhost start code and
> > this follows the standard properly.
> >
> > Is it ok to do these on top of this patch?
>
> Fine.
>
> Thanks
>
> >
> > Thanks!
> >
> > [1] https://lore.kernel.org/qemu-devel/20230509154435.1410162-4-eperezma@redhat.com/
> >
> >
> > > Thanks
> > >
> > > >      return r;
> > > >  }
> > > >
> > > > --
> > > > 2.39.3
> > > >
> > >
> >
>

Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end

[Jason Wang]

On Mon, Jul 31, 2023 at 4:05 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Mon, Jul 31, 2023 at 8:36 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Jul 26, 2023 at 2:27 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Wed, Jul 26, 2023 at 4:07 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > >
> > > > > The device already has a virtio status set by vhost_vdpa_init by the
> > > > > time vhost_vdpa_probe_cvq_isolation is called. vhost_vdpa_init set
> > > > > S_ACKNOWLEDGE and S_DRIVER, so it is invalid to just reset it.
> > > > >
> > > > > It is invalid to start the device after it, but all devices seems to be
> > > > > fine with it.  Fixing qemu so it follows virtio start procedure.
> > > > >
> > > > > Fixes: 152128d64697 ("vdpa: move CVQ isolation check to net_init_vhost_vdpa")
> > > > > Reported-by: Dragos Tatulea <dtatulea@nvidia.com>
> > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > ---
> > > > >  net/vhost-vdpa.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > index 9795306742..d7e2b714b4 100644
> > > > > --- a/net/vhost-vdpa.c
> > > > > +++ b/net/vhost-vdpa.c
> > > > > @@ -1333,6 +1333,8 @@ static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
> > > > >  out:
> > > > >      status = 0;
> > > > >      ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > > > +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > > > > +    ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > >
> > > > So if we fail after FEATURES_OK, this basically clears that bit. Spec
> > > > doesn't say it can or not, I wonder if a reset is better?
> > > >
> > >
> > > I don't follow this, the reset is just above the added code, isn't it?
> >
> > I meant for error path:
> >
> > E.g:
> >     uint8_t status = VIRTIO_CONFIG_S_ACKNOWLEDGE |
> >                      VIRTIO_CONFIG_S_DRIVER |
> >                      VIRTIO_CONFIG_S_FEATURES_OK;
> > ...
> >     r = ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > ....
> >         if (cvq_group != -ENOTSUP) {
> >             r = cvq_group;
> >             goto out;
> >         }
> >
> > out:
> >     status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> >     ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> >
> > We're basically clearing FEATURES_OK?
> >
>
> Yes, it is the state that previous functions (vhost_vdpa_init) set. We
> need to leave it that way, either if the backend supports cvq
> isolation or not, or in the case of an error. Not doing that way makes
> vhost_dev_start (and vhost_vdpa_set_features) set the features before
> setting VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER.
> Otherwise, the guest can (and do) access to config space before
> _S_ACKNOWLEDGE | _S_DRIVER.

I'm not sure if it is supported by the spec or not (I meant clearing
the FEATURES_OK). Or maybe we need a reset here?

Thanks

>
>
> > >
> > > > Btw, spec requires a read of status after setting FEATURES_OK, this
> > > > seems to be missed in the current code.
> > > >
> > >
> > > I'm ok with that, but this patch does not touch that part.
> > >
> > > To fix this properly we should:
> > > - Expose vhost_vdpa_set_dev_features_fd as we did in previous versions
> > > of the series that added vhost_vdpa_probe_cvq_isolation [1].
> > > - Get status after vhost_vdpa_add_status, so both vhost start code and
> > > this follows the standard properly.
> > >
> > > Is it ok to do these on top of this patch?
> >
> > Fine.
> >
> > Thanks
> >
> > >
> > > Thanks!
> > >
> > > [1] https://lore.kernel.org/qemu-devel/20230509154435.1410162-4-eperezma@redhat.com/
> > >
> > >
> > > > Thanks
> > > >
> > > > >      return r;
> > > > >  }
> > > > >
> > > > > --
> > > > > 2.39.3
> > > > >
> > > >
> > >
> >
>

Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end

[Eugenio Perez Martin]

On Mon, Jul 31, 2023 at 10:42 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Jul 31, 2023 at 4:05 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Mon, Jul 31, 2023 at 8:36 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Wed, Jul 26, 2023 at 2:27 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Wed, Jul 26, 2023 at 4:07 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > >
> > > > > > The device already has a virtio status set by vhost_vdpa_init by the
> > > > > > time vhost_vdpa_probe_cvq_isolation is called. vhost_vdpa_init set
> > > > > > S_ACKNOWLEDGE and S_DRIVER, so it is invalid to just reset it.
> > > > > >
> > > > > > It is invalid to start the device after it, but all devices seems to be
> > > > > > fine with it.  Fixing qemu so it follows virtio start procedure.
> > > > > >
> > > > > > Fixes: 152128d64697 ("vdpa: move CVQ isolation check to net_init_vhost_vdpa")
> > > > > > Reported-by: Dragos Tatulea <dtatulea@nvidia.com>
> > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > ---
> > > > > >  net/vhost-vdpa.c | 2 ++
> > > > > >  1 file changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > index 9795306742..d7e2b714b4 100644
> > > > > > --- a/net/vhost-vdpa.c
> > > > > > +++ b/net/vhost-vdpa.c
> > > > > > @@ -1333,6 +1333,8 @@ static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
> > > > > >  out:
> > > > > >      status = 0;
> > > > > >      ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > > > > +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > > > > > +    ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > > >
> > > > > So if we fail after FEATURES_OK, this basically clears that bit. Spec
> > > > > doesn't say it can or not, I wonder if a reset is better?
> > > > >
> > > >
> > > > I don't follow this, the reset is just above the added code, isn't it?
> > >
> > > I meant for error path:
> > >
> > > E.g:
> > >     uint8_t status = VIRTIO_CONFIG_S_ACKNOWLEDGE |
> > >                      VIRTIO_CONFIG_S_DRIVER |
> > >                      VIRTIO_CONFIG_S_FEATURES_OK;
> > > ...
> > >     r = ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > ....
> > >         if (cvq_group != -ENOTSUP) {
> > >             r = cvq_group;
> > >             goto out;
> > >         }
> > >
> > > out:
> > >     status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > >     ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > >
> > > We're basically clearing FEATURES_OK?
> > >
> >
> > Yes, it is the state that previous functions (vhost_vdpa_init) set. We
> > need to leave it that way, either if the backend supports cvq
> > isolation or not, or in the case of an error. Not doing that way makes
> > vhost_dev_start (and vhost_vdpa_set_features) set the features before
> > setting VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER.
> > Otherwise, the guest can (and do) access to config space before
> > _S_ACKNOWLEDGE | _S_DRIVER.
>
> I'm not sure if it is supported by the spec or not (I meant clearing
> the FEATURES_OK). Or maybe we need a reset here?
>

Sorry, I'm still missing it :). The reset just above in all fail
paths. They go to "out" label, and the first ioctl reset the device,
the second set the VIRTIO_CONFIG_S_ACKNOWLEDGE |
VIRTIO_CONFIG_S_DRIVER.

> Thanks
>
> >
> >
> > > >
> > > > > Btw, spec requires a read of status after setting FEATURES_OK, this
> > > > > seems to be missed in the current code.
> > > > >
> > > >
> > > > I'm ok with that, but this patch does not touch that part.
> > > >
> > > > To fix this properly we should:
> > > > - Expose vhost_vdpa_set_dev_features_fd as we did in previous versions
> > > > of the series that added vhost_vdpa_probe_cvq_isolation [1].
> > > > - Get status after vhost_vdpa_add_status, so both vhost start code and
> > > > this follows the standard properly.
> > > >
> > > > Is it ok to do these on top of this patch?
> > >
> > > Fine.
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks!
> > > >
> > > > [1] https://lore.kernel.org/qemu-devel/20230509154435.1410162-4-eperezma@redhat.com/
> > > >
> > > >
> > > > > Thanks
> > > > >
> > > > > >      return r;
> > > > > >  }
> > > > > >
> > > > > > --
> > > > > > 2.39.3
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end

[Jason Wang]

On Mon, Jul 31, 2023 at 5:41 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Mon, Jul 31, 2023 at 10:42 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Mon, Jul 31, 2023 at 4:05 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Mon, Jul 31, 2023 at 8:36 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Wed, Jul 26, 2023 at 2:27 PM Eugenio Perez Martin
> > > > <eperezma@redhat.com> wrote:
> > > > >
> > > > > On Wed, Jul 26, 2023 at 4:07 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > > On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > > >
> > > > > > > The device already has a virtio status set by vhost_vdpa_init by the
> > > > > > > time vhost_vdpa_probe_cvq_isolation is called. vhost_vdpa_init set
> > > > > > > S_ACKNOWLEDGE and S_DRIVER, so it is invalid to just reset it.
> > > > > > >
> > > > > > > It is invalid to start the device after it, but all devices seems to be
> > > > > > > fine with it.  Fixing qemu so it follows virtio start procedure.
> > > > > > >
> > > > > > > Fixes: 152128d64697 ("vdpa: move CVQ isolation check to net_init_vhost_vdpa")
> > > > > > > Reported-by: Dragos Tatulea <dtatulea@nvidia.com>
> > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > > ---
> > > > > > >  net/vhost-vdpa.c | 2 ++
> > > > > > >  1 file changed, 2 insertions(+)
> > > > > > >
> > > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > > index 9795306742..d7e2b714b4 100644
> > > > > > > --- a/net/vhost-vdpa.c
> > > > > > > +++ b/net/vhost-vdpa.c
> > > > > > > @@ -1333,6 +1333,8 @@ static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
> > > > > > >  out:
> > > > > > >      status = 0;
> > > > > > >      ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > > > > > +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > > > > > > +    ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > > > >
> > > > > > So if we fail after FEATURES_OK, this basically clears that bit. Spec
> > > > > > doesn't say it can or not, I wonder if a reset is better?
> > > > > >
> > > > >
> > > > > I don't follow this, the reset is just above the added code, isn't it?
> > > >
> > > > I meant for error path:
> > > >
> > > > E.g:
> > > >     uint8_t status = VIRTIO_CONFIG_S_ACKNOWLEDGE |
> > > >                      VIRTIO_CONFIG_S_DRIVER |
> > > >                      VIRTIO_CONFIG_S_FEATURES_OK;
> > > > ...
> > > >     r = ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > > ....
> > > >         if (cvq_group != -ENOTSUP) {
> > > >             r = cvq_group;
> > > >             goto out;
> > > >         }
> > > >
> > > > out:
> > > >     status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > > >     ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > >
> > > > We're basically clearing FEATURES_OK?
> > > >
> > >
> > > Yes, it is the state that previous functions (vhost_vdpa_init) set. We
> > > need to leave it that way, either if the backend supports cvq
> > > isolation or not, or in the case of an error. Not doing that way makes
> > > vhost_dev_start (and vhost_vdpa_set_features) set the features before
> > > setting VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER.
> > > Otherwise, the guest can (and do) access to config space before
> > > _S_ACKNOWLEDGE | _S_DRIVER.
> >
> > I'm not sure if it is supported by the spec or not (I meant clearing
> > the FEATURES_OK). Or maybe we need a reset here?
> >
>
> Sorry, I'm still missing it :). The reset just above in all fail
> paths. They go to "out" label, and the first ioctl reset the device,
> the second set the VIRTIO_CONFIG_S_ACKNOWLEDGE |
> VIRTIO_CONFIG_S_DRIVER.

Just to make sure we are at the same page:

On error we basically do:

set_status(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER |
VIRTIO_CONFIG_S_FEATURES_OK);
...
set_status(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER)

So it means the device allows the driver to clear FEATURES_OK. But
spec is unclear whether or not this is supported. So I'm not sure it
is supported by all devices.

Thanks

>
> > Thanks
> >
> > >
> > >
> > > > >
> > > > > > Btw, spec requires a read of status after setting FEATURES_OK, this
> > > > > > seems to be missed in the current code.
> > > > > >
> > > > >
> > > > > I'm ok with that, but this patch does not touch that part.
> > > > >
> > > > > To fix this properly we should:
> > > > > - Expose vhost_vdpa_set_dev_features_fd as we did in previous versions
> > > > > of the series that added vhost_vdpa_probe_cvq_isolation [1].
> > > > > - Get status after vhost_vdpa_add_status, so both vhost start code and
> > > > > this follows the standard properly.
> > > > >
> > > > > Is it ok to do these on top of this patch?
> > > >
> > > > Fine.
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Thanks!
> > > > >
> > > > > [1] https://lore.kernel.org/qemu-devel/20230509154435.1410162-4-eperezma@redhat.com/
> > > > >
> > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >      return r;
> > > > > > >  }
> > > > > > >
> > > > > > > --
> > > > > > > 2.39.3
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end

[Jason Wang]

On Tue, Aug 1, 2023 at 11:28 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Jul 31, 2023 at 5:41 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Mon, Jul 31, 2023 at 10:42 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Mon, Jul 31, 2023 at 4:05 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Mon, Jul 31, 2023 at 8:36 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Wed, Jul 26, 2023 at 2:27 PM Eugenio Perez Martin
> > > > > <eperezma@redhat.com> wrote:
> > > > > >
> > > > > > On Wed, Jul 26, 2023 at 4:07 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > > >
> > > > > > > On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > > > >
> > > > > > > > The device already has a virtio status set by vhost_vdpa_init by the
> > > > > > > > time vhost_vdpa_probe_cvq_isolation is called. vhost_vdpa_init set
> > > > > > > > S_ACKNOWLEDGE and S_DRIVER, so it is invalid to just reset it.
> > > > > > > >
> > > > > > > > It is invalid to start the device after it, but all devices seems to be
> > > > > > > > fine with it.  Fixing qemu so it follows virtio start procedure.
> > > > > > > >
> > > > > > > > Fixes: 152128d64697 ("vdpa: move CVQ isolation check to net_init_vhost_vdpa")
> > > > > > > > Reported-by: Dragos Tatulea <dtatulea@nvidia.com>
> > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > > > ---
> > > > > > > >  net/vhost-vdpa.c | 2 ++
> > > > > > > >  1 file changed, 2 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > > > index 9795306742..d7e2b714b4 100644
> > > > > > > > --- a/net/vhost-vdpa.c
> > > > > > > > +++ b/net/vhost-vdpa.c
> > > > > > > > @@ -1333,6 +1333,8 @@ static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
> > > > > > > >  out:
> > > > > > > >      status = 0;
> > > > > > > >      ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > > > > > > +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > > > > > > > +    ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > > > > >
> > > > > > > So if we fail after FEATURES_OK, this basically clears that bit. Spec
> > > > > > > doesn't say it can or not, I wonder if a reset is better?
> > > > > > >
> > > > > >
> > > > > > I don't follow this, the reset is just above the added code, isn't it?
> > > > >
> > > > > I meant for error path:
> > > > >
> > > > > E.g:
> > > > >     uint8_t status = VIRTIO_CONFIG_S_ACKNOWLEDGE |
> > > > >                      VIRTIO_CONFIG_S_DRIVER |
> > > > >                      VIRTIO_CONFIG_S_FEATURES_OK;
> > > > > ...
> > > > >     r = ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > > > ....
> > > > >         if (cvq_group != -ENOTSUP) {
> > > > >             r = cvq_group;
> > > > >             goto out;
> > > > >         }
> > > > >
> > > > > out:
> > > > >     status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > > > >     ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> > > > >
> > > > > We're basically clearing FEATURES_OK?
> > > > >
> > > >
> > > > Yes, it is the state that previous functions (vhost_vdpa_init) set. We
> > > > need to leave it that way, either if the backend supports cvq
> > > > isolation or not, or in the case of an error. Not doing that way makes
> > > > vhost_dev_start (and vhost_vdpa_set_features) set the features before
> > > > setting VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER.
> > > > Otherwise, the guest can (and do) access to config space before
> > > > _S_ACKNOWLEDGE | _S_DRIVER.
> > >
> > > I'm not sure if it is supported by the spec or not (I meant clearing
> > > the FEATURES_OK). Or maybe we need a reset here?
> > >
> >
> > Sorry, I'm still missing it :). The reset just above in all fail
> > paths. They go to "out" label, and the first ioctl reset the device,
> > the second set the VIRTIO_CONFIG_S_ACKNOWLEDGE |
> > VIRTIO_CONFIG_S_DRIVER.
>
> Just to make sure we are at the same page:
>
> On error we basically do:
>
> set_status(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER |
> VIRTIO_CONFIG_S_FEATURES_OK);
> ...
> set_status(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER)
>
> So it means the device allows the driver to clear FEATURES_OK. But
> spec is unclear whether or not this is supported. So I'm not sure it
> is supported by all devices.

Ok, I think I miss the set_status(0), so this patch should be fine.

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

>
> Thanks
>
> >
> > > Thanks
> > >
> > > >
> > > >
> > > > > >
> > > > > > > Btw, spec requires a read of status after setting FEATURES_OK, this
> > > > > > > seems to be missed in the current code.
> > > > > > >
> > > > > >
> > > > > > I'm ok with that, but this patch does not touch that part.
> > > > > >
> > > > > > To fix this properly we should:
> > > > > > - Expose vhost_vdpa_set_dev_features_fd as we did in previous versions
> > > > > > of the series that added vhost_vdpa_probe_cvq_isolation [1].
> > > > > > - Get status after vhost_vdpa_add_status, so both vhost start code and
> > > > > > this follows the standard properly.
> > > > > >
> > > > > > Is it ok to do these on top of this patch?
> > > > >
> > > > > Fine.
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Thanks!
> > > > > >
> > > > > > [1] https://lore.kernel.org/qemu-devel/20230509154435.1410162-4-eperezma@redhat.com/
> > > > > >
> > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > >      return r;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > --
> > > > > > > > 2.39.3
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >

Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end

[Michael Tokarev]

26.07.2023 05:07, Jason Wang wrote:
> On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>>
>> The device already has a virtio status set by vhost_vdpa_init by the
>> time vhost_vdpa_probe_cvq_isolation is called. vhost_vdpa_init set
>> S_ACKNOWLEDGE and S_DRIVER, so it is invalid to just reset it.
>>
>> It is invalid to start the device after it, but all devices seems to be
>> fine with it.  Fixing qemu so it follows virtio start procedure.
>>
>> Fixes: 152128d64697 ("vdpa: move CVQ isolation check to net_init_vhost_vdpa")
>> Reported-by: Dragos Tatulea <dtatulea@nvidia.com>
>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>> ---
>>   net/vhost-vdpa.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 9795306742..d7e2b714b4 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -1333,6 +1333,8 @@ static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
>>   out:
>>       status = 0;
>>       ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
>> +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
>> +    ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);

Ping? Has this been forgotten, or not needed anymore?

(Just looking at the stable-to-apply queue)

/mjt