Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end
26.07.2023 05:07, Jason Wang wrote:
On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez 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
Signed-off-by: Eugenio Pérez
---
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 = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
+ioctl(device_fd, VHOST_VDPA_SET_STATUS, );
Ping? Has this been forgotten, or not needed anymore?
(Just looking at the stable-to-apply queue)
/mjt
Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end
On Tue, Aug 1, 2023 at 11:28 AM Jason Wang wrote:
>
> On Mon, Jul 31, 2023 at 5:41 PM Eugenio Perez Martin
> wrote:
> >
> > On Mon, Jul 31, 2023 at 10:42 AM Jason Wang wrote:
> > >
> > > On Mon, Jul 31, 2023 at 4:05 PM Eugenio Perez Martin
> > > wrote:
> > > >
> > > > On Mon, Jul 31, 2023 at 8:36 AM Jason Wang wrote:
> > > > >
> > > > > On Wed, Jul 26, 2023 at 2:27 PM Eugenio Perez Martin
> > > > > wrote:
> > > > > >
> > > > > > On Wed, Jul 26, 2023 at 4:07 AM Jason Wang
> > > > > > wrote:
> > > > > > >
> > > > > > > On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez
> > > > > > > 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
> > > > > > > > Signed-off-by: Eugenio Pérez
> > > > > > > > ---
> > > > > > > > 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 = VIRTIO_CONFIG_S_ACKNOWLEDGE |
> > > > > > > > VIRTIO_CONFIG_S_DRIVER;
> > > > > > > > +ioctl(device_fd, VHOST_VDPA_SET_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, );
> > > > >
> > > > > 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, );
> > > > >
> > > > > 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
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].
> > > > > > -
Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end
On Mon, Jul 31, 2023 at 5:41 PM Eugenio Perez Martin
wrote:
>
> On Mon, Jul 31, 2023 at 10:42 AM Jason Wang wrote:
> >
> > On Mon, Jul 31, 2023 at 4:05 PM Eugenio Perez Martin
> > wrote:
> > >
> > > On Mon, Jul 31, 2023 at 8:36 AM Jason Wang wrote:
> > > >
> > > > On Wed, Jul 26, 2023 at 2:27 PM Eugenio Perez Martin
> > > > wrote:
> > > > >
> > > > > On Wed, Jul 26, 2023 at 4:07 AM Jason Wang
> > > > > wrote:
> > > > > >
> > > > > > On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez
> > > > > > 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
> > > > > > > Signed-off-by: Eugenio Pérez
> > > > > > > ---
> > > > > > > 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 = VIRTIO_CONFIG_S_ACKNOWLEDGE |
> > > > > > > VIRTIO_CONFIG_S_DRIVER;
> > > > > > > +ioctl(device_fd, VHOST_VDPA_SET_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, );
> > > >
> > > > 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, );
> > > >
> > > > 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-epere...@redhat.com/
> > > > >
> > > > >
> > > > > > Thanks
> > > > > >
> > > > > > > return r;
>
Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end
On Mon, Jul 31, 2023 at 10:42 AM Jason Wang wrote:
>
> On Mon, Jul 31, 2023 at 4:05 PM Eugenio Perez Martin
> wrote:
> >
> > On Mon, Jul 31, 2023 at 8:36 AM Jason Wang wrote:
> > >
> > > On Wed, Jul 26, 2023 at 2:27 PM Eugenio Perez Martin
> > > wrote:
> > > >
> > > > On Wed, Jul 26, 2023 at 4:07 AM Jason Wang wrote:
> > > > >
> > > > > On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez
> > > > > 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
> > > > > > Signed-off-by: Eugenio Pérez
> > > > > > ---
> > > > > > 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 = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > > > > > +ioctl(device_fd, VHOST_VDPA_SET_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, );
> > >
> > > 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, );
> > >
> > > 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-epere...@redhat.com/
> > > >
> > > >
> > > > > Thanks
> > > > >
> > > > > > return r;
> > > > > > }
> > > > > >
> > > > > > --
> > > > > > 2.39.3
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end
On Mon, Jul 31, 2023 at 4:05 PM Eugenio Perez Martin
wrote:
>
> On Mon, Jul 31, 2023 at 8:36 AM Jason Wang wrote:
> >
> > On Wed, Jul 26, 2023 at 2:27 PM Eugenio Perez Martin
> > wrote:
> > >
> > > On Wed, Jul 26, 2023 at 4:07 AM Jason Wang wrote:
> > > >
> > > > On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez
> > > > 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
> > > > > Signed-off-by: Eugenio Pérez
> > > > > ---
> > > > > 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 = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > > > > +ioctl(device_fd, VHOST_VDPA_SET_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, );
> >
> > 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, );
> >
> > 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-epere...@redhat.com/
> > >
> > >
> > > > Thanks
> > > >
> > > > > return r;
> > > > > }
> > > > >
> > > > > --
> > > > > 2.39.3
> > > > >
> > > >
> > >
> >
>
Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end
On Mon, Jul 31, 2023 at 8:36 AM Jason Wang wrote:
>
> On Wed, Jul 26, 2023 at 2:27 PM Eugenio Perez Martin
> wrote:
> >
> > On Wed, Jul 26, 2023 at 4:07 AM Jason Wang wrote:
> > >
> > > On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez 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
> > > > Signed-off-by: Eugenio Pérez
> > > > ---
> > > > 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 = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > > > +ioctl(device_fd, VHOST_VDPA_SET_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, );
>
> 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, );
>
> 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-epere...@redhat.com/
> >
> >
> > > Thanks
> > >
> > > > return r;
> > > > }
> > > >
> > > > --
> > > > 2.39.3
> > > >
> > >
> >
>
Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end
On Wed, Jul 26, 2023 at 2:27 PM Eugenio Perez Martin
wrote:
>
> On Wed, Jul 26, 2023 at 4:07 AM Jason Wang wrote:
> >
> > On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez 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
> > > Signed-off-by: Eugenio Pérez
> > > ---
> > > 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 = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > > +ioctl(device_fd, VHOST_VDPA_SET_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, );
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, );
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-epere...@redhat.com/
>
>
> > Thanks
> >
> > > return r;
> > > }
> > >
> > > --
> > > 2.39.3
> > >
> >
>
Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end
On Wed, Jul 26, 2023 at 4:07 AM Jason Wang wrote:
>
> On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez 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
> > Signed-off-by: Eugenio Pérez
> > ---
> > 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 = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > +ioctl(device_fd, VHOST_VDPA_SET_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-epere...@redhat.com/
> Thanks
>
> > return r;
> > }
> >
> > --
> > 2.39.3
> >
>
Re: [PATCH] vdpa: set old virtio status at cvq isolation probing end
On Wed, Jul 26, 2023 at 2:21 AM Eugenio Pérez 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
> Signed-off-by: Eugenio Pérez
> ---
> 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 = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> +ioctl(device_fd, VHOST_VDPA_SET_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
>
