Re: [PATCH 6/6] vhost-user: Have reset_status fall back to reset
On 24.07.23 20:04, Stefan Hajnoczi wrote:
On Fri, Jul 21, 2023 at 04:16:07PM +0200, Hanna Czenczek wrote:
On 20.07.23 18:03, Stefan Hajnoczi wrote:
On Wed, Jul 19, 2023 at 04:27:58PM +0200, Hanna Czenczek wrote:
On 19.07.23 16:11, Hanna Czenczek wrote:
On 18.07.23 17:10, Stefan Hajnoczi wrote:
On Tue, Jul 11, 2023 at 05:52:28PM +0200, Hanna Czenczek wrote:
The only user of vhost_user_reset_status() is vhost_dev_stop(), which
only uses it as a fall-back to stop the back-end if it does not support
SUSPEND. However, vhost-user's implementation is a no-op unless the
back-end supports SET_STATUS.
vhost-vdpa's implementation instead just calls
vhost_vdpa_reset_device(), implying that it's OK to fully reset the
device if SET_STATUS is not supported.
To be fair, vhost_vdpa_reset_device() does nothing but to set
the status
to zero. However, that may well be because vhost-vdpa has no method
besides this to reset a device. In contrast, vhost-user has
RESET_DEVICE and a RESET_OWNER, which can be used instead.
While it is not entirely clear from documentation or git logs, from
discussions and the order of vhost-user protocol features, it
appears to
me as if RESET_OWNER originally had no real meaning for vhost-user, and
was thus used to signal a device reset to the back-end. Then,
RESET_DEVICE was introduced, to have a well-defined dedicated reset
command. Finally, vhost-user received full STATUS support, including
SET_STATUS, so setting the device status to 0 is now the preferred way
of resetting a device. Still, RESET_DEVICE and RESET_OWNER should
remain valid as fall-backs.
Therefore, have vhost_user_reset_status() fall back to
vhost_user_reset_device() if the back-end has no STATUS support.
Signed-off-by: Hanna Czenczek
---
hw/virtio/vhost-user.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 4507de5a92..53a881ec2a 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2833,6 +2833,8 @@ static void vhost_user_reset_status(struct
vhost_dev *dev)
if (virtio_has_feature(dev->protocol_features,
VHOST_USER_PROTOCOL_F_STATUS)) {
vhost_user_set_status(dev, 0);
+ } else {
+ vhost_user_reset_device(dev);
}
}
Did you check whether DPDK treats setting the status to 0 as equivalent
to RESET_DEVICE?
If it doesn’t, what’s even the point of using reset_status?
Sorry, I’m being unclear, and I think this may be important because it ties
into the question from patch 1, what qemu is even trying to do by running
SET_STATUS(0) vhost_dev_stop(), so here’s what gave me the impression that
SET_STATUS(0) and RESET_DEVICE should be equivalent:
vhost-vdpa.c runs SET_STATUS(0) in a function called
vhost_vdpa_reset_device(). This is one thing that gave me the impression
that this is about an actual full reset.
Another is the whole discussion that we’ve had. vhost_dev_stop() does not
call a `vhost_reset_device()` function, it calls `vhost_reset_status()`.
Still, we were always talking about resetting the device.
There is some hacky stuff with struct vhost_dev's vq_index_end and
multi-queue devices. I think it's because multi-queue vhost-net device
consist of many vhost_devs and NetClientStates, so certain vhost
operations are skipped unless this is the "first" or "last" vhost_dev
from a large aggregate vhost-net device. That might be responsible for
part of the weirdness.
It doesn’t make sense to me that vDPA would provide no function to fully
reset a device, while vhost-user does. Being able to reset a device sounds
vital to me. This also gave me the impression that SET_STATUS(0) on vDPA at
least is functionally equivalent to a full device reset.
Maybe SET_STATUS(0) does mean a full device reset on vDPA, but not on
vhost-user. That would be a real shame, so I assumed this would not be the
case; that SET_STATUS(0) does the same thing on both protocols.
Yes, exactly. It has the real VIRTIO spec meaning in vDPA. In vhost-user
it's currently only used by DPDK as a hint for when device
initialization is complete:
https://github.com/DPDK/dpdk/commit/41d201804c4c44738168e2d247d3b1780845faa1
FWIW, now the code is a bit different.
https://github.com/DPDK/dpdk/commit/671cc679a5fcd26705bb20ddc13b93e665719054
has added a RESET interpretation for the status field, i.e. when it is 0.
It doesn’t do anything, but at least DPDK seems to agree that SET_STATUS(0)
is a reset.
That patch adds diagnostics but does not perform any action for
SET_STATUS 0. DPDK's vhost_user_reset_owner() is still the only place
where the device is actually reset.
That’s what I said, it doesn’t do anything, but the diagnostics agree
that it is a RESET.
QEMU cannot switch to just
SET_STATUS 0, it still needs to send RESET_DEVICE/RESET_OWNER.
That is what I questioned below: We currently *do not* call
RESET_DEVICE/RESET_OWNER. This patch is not about switching to
SET_STATUS(0), it is
Re: [PATCH 6/6] vhost-user: Have reset_status fall back to reset
On Fri, Jul 21, 2023 at 04:16:07PM +0200, Hanna Czenczek wrote:
> On 20.07.23 18:03, Stefan Hajnoczi wrote:
> > On Wed, Jul 19, 2023 at 04:27:58PM +0200, Hanna Czenczek wrote:
> > > On 19.07.23 16:11, Hanna Czenczek wrote:
> > > > On 18.07.23 17:10, Stefan Hajnoczi wrote:
> > > > > On Tue, Jul 11, 2023 at 05:52:28PM +0200, Hanna Czenczek wrote:
> > > > > > The only user of vhost_user_reset_status() is vhost_dev_stop(),
> > > > > > which
> > > > > > only uses it as a fall-back to stop the back-end if it does not
> > > > > > support
> > > > > > SUSPEND. However, vhost-user's implementation is a no-op unless the
> > > > > > back-end supports SET_STATUS.
> > > > > >
> > > > > > vhost-vdpa's implementation instead just calls
> > > > > > vhost_vdpa_reset_device(), implying that it's OK to fully reset the
> > > > > > device if SET_STATUS is not supported.
> > > > > >
> > > > > > To be fair, vhost_vdpa_reset_device() does nothing but to set
> > > > > > the status
> > > > > > to zero. However, that may well be because vhost-vdpa has no method
> > > > > > besides this to reset a device. In contrast, vhost-user has
> > > > > > RESET_DEVICE and a RESET_OWNER, which can be used instead.
> > > > > >
> > > > > > While it is not entirely clear from documentation or git logs, from
> > > > > > discussions and the order of vhost-user protocol features, it
> > > > > > appears to
> > > > > > me as if RESET_OWNER originally had no real meaning for vhost-user,
> > > > > > and
> > > > > > was thus used to signal a device reset to the back-end. Then,
> > > > > > RESET_DEVICE was introduced, to have a well-defined dedicated reset
> > > > > > command. Finally, vhost-user received full STATUS support,
> > > > > > including
> > > > > > SET_STATUS, so setting the device status to 0 is now the preferred
> > > > > > way
> > > > > > of resetting a device. Still, RESET_DEVICE and RESET_OWNER should
> > > > > > remain valid as fall-backs.
> > > > > >
> > > > > > Therefore, have vhost_user_reset_status() fall back to
> > > > > > vhost_user_reset_device() if the back-end has no STATUS support.
> > > > > >
> > > > > > Signed-off-by: Hanna Czenczek
> > > > > > ---
> > > > > > hw/virtio/vhost-user.c | 2 ++
> > > > > > 1 file changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > > > index 4507de5a92..53a881ec2a 100644
> > > > > > --- a/hw/virtio/vhost-user.c
> > > > > > +++ b/hw/virtio/vhost-user.c
> > > > > > @@ -2833,6 +2833,8 @@ static void vhost_user_reset_status(struct
> > > > > > vhost_dev *dev)
> > > > > > if (virtio_has_feature(dev->protocol_features,
> > > > > > VHOST_USER_PROTOCOL_F_STATUS)) {
> > > > > > vhost_user_set_status(dev, 0);
> > > > > > + } else {
> > > > > > + vhost_user_reset_device(dev);
> > > > > > }
> > > > > > }
> > > > > Did you check whether DPDK treats setting the status to 0 as
> > > > > equivalent
> > > > > to RESET_DEVICE?
> > > > If it doesn’t, what’s even the point of using reset_status?
> > > Sorry, I’m being unclear, and I think this may be important because it
> > > ties
> > > into the question from patch 1, what qemu is even trying to do by running
> > > SET_STATUS(0) vhost_dev_stop(), so here’s what gave me the impression that
> > > SET_STATUS(0) and RESET_DEVICE should be equivalent:
> > >
> > > vhost-vdpa.c runs SET_STATUS(0) in a function called
> > > vhost_vdpa_reset_device(). This is one thing that gave me the impression
> > > that this is about an actual full reset.
> > >
> > > Another is the whole discussion that we’ve had. vhost_dev_stop() does not
> > > call a `vhost_reset_device()` function, it calls `vhost_reset_status()`.
> > > Still, we were always talking about resetting the device.
> > There is some hacky stuff with struct vhost_dev's vq_index_end and
> > multi-queue devices. I think it's because multi-queue vhost-net device
> > consist of many vhost_devs and NetClientStates, so certain vhost
> > operations are skipped unless this is the "first" or "last" vhost_dev
> > from a large aggregate vhost-net device. That might be responsible for
> > part of the weirdness.
> >
> > > It doesn’t make sense to me that vDPA would provide no function to fully
> > > reset a device, while vhost-user does. Being able to reset a device
> > > sounds
> > > vital to me. This also gave me the impression that SET_STATUS(0) on vDPA
> > > at
> > > least is functionally equivalent to a full device reset.
> > >
> > >
> > > Maybe SET_STATUS(0) does mean a full device reset on vDPA, but not on
> > > vhost-user. That would be a real shame, so I assumed this would not be
> > > the
> > > case; that SET_STATUS(0) does the same thing on both protocols.
> > Yes, exactly. It has the real VIRTIO spec meaning in vDPA. In vhost-user
> > it's currently only used by DPDK as a hint for when device
> > initialization is complete:
> >
Re: [PATCH 6/6] vhost-user: Have reset_status fall back to reset
On 20.07.23 18:03, Stefan Hajnoczi wrote:
On Wed, Jul 19, 2023 at 04:27:58PM +0200, Hanna Czenczek wrote:
On 19.07.23 16:11, Hanna Czenczek wrote:
On 18.07.23 17:10, Stefan Hajnoczi wrote:
On Tue, Jul 11, 2023 at 05:52:28PM +0200, Hanna Czenczek wrote:
The only user of vhost_user_reset_status() is vhost_dev_stop(), which
only uses it as a fall-back to stop the back-end if it does not support
SUSPEND. However, vhost-user's implementation is a no-op unless the
back-end supports SET_STATUS.
vhost-vdpa's implementation instead just calls
vhost_vdpa_reset_device(), implying that it's OK to fully reset the
device if SET_STATUS is not supported.
To be fair, vhost_vdpa_reset_device() does nothing but to set
the status
to zero. However, that may well be because vhost-vdpa has no method
besides this to reset a device. In contrast, vhost-user has
RESET_DEVICE and a RESET_OWNER, which can be used instead.
While it is not entirely clear from documentation or git logs, from
discussions and the order of vhost-user protocol features, it
appears to
me as if RESET_OWNER originally had no real meaning for vhost-user, and
was thus used to signal a device reset to the back-end. Then,
RESET_DEVICE was introduced, to have a well-defined dedicated reset
command. Finally, vhost-user received full STATUS support, including
SET_STATUS, so setting the device status to 0 is now the preferred way
of resetting a device. Still, RESET_DEVICE and RESET_OWNER should
remain valid as fall-backs.
Therefore, have vhost_user_reset_status() fall back to
vhost_user_reset_device() if the back-end has no STATUS support.
Signed-off-by: Hanna Czenczek
---
hw/virtio/vhost-user.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 4507de5a92..53a881ec2a 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2833,6 +2833,8 @@ static void vhost_user_reset_status(struct
vhost_dev *dev)
if (virtio_has_feature(dev->protocol_features,
VHOST_USER_PROTOCOL_F_STATUS)) {
vhost_user_set_status(dev, 0);
+ } else {
+ vhost_user_reset_device(dev);
}
}
Did you check whether DPDK treats setting the status to 0 as equivalent
to RESET_DEVICE?
If it doesn’t, what’s even the point of using reset_status?
Sorry, I’m being unclear, and I think this may be important because it ties
into the question from patch 1, what qemu is even trying to do by running
SET_STATUS(0) vhost_dev_stop(), so here’s what gave me the impression that
SET_STATUS(0) and RESET_DEVICE should be equivalent:
vhost-vdpa.c runs SET_STATUS(0) in a function called
vhost_vdpa_reset_device(). This is one thing that gave me the impression
that this is about an actual full reset.
Another is the whole discussion that we’ve had. vhost_dev_stop() does not
call a `vhost_reset_device()` function, it calls `vhost_reset_status()`.
Still, we were always talking about resetting the device.
There is some hacky stuff with struct vhost_dev's vq_index_end and
multi-queue devices. I think it's because multi-queue vhost-net device
consist of many vhost_devs and NetClientStates, so certain vhost
operations are skipped unless this is the "first" or "last" vhost_dev
from a large aggregate vhost-net device. That might be responsible for
part of the weirdness.
It doesn’t make sense to me that vDPA would provide no function to fully
reset a device, while vhost-user does. Being able to reset a device sounds
vital to me. This also gave me the impression that SET_STATUS(0) on vDPA at
least is functionally equivalent to a full device reset.
Maybe SET_STATUS(0) does mean a full device reset on vDPA, but not on
vhost-user. That would be a real shame, so I assumed this would not be the
case; that SET_STATUS(0) does the same thing on both protocols.
Yes, exactly. It has the real VIRTIO spec meaning in vDPA. In vhost-user
it's currently only used by DPDK as a hint for when device
initialization is complete:
https://github.com/DPDK/dpdk/commit/41d201804c4c44738168e2d247d3b1780845faa1
FWIW, now the code is a bit different.
https://github.com/DPDK/dpdk/commit/671cc679a5fcd26705bb20ddc13b93e665719054
has added a RESET interpretation for the status field, i.e. when it is
0. It doesn’t do anything, but at least DPDK seems to agree that
SET_STATUS(0) is a reset.
The virtio specification says “Writing 0 into this field resets the device.”
about the device_status field.
This also makes sense, because the device_status field is basically used to
tell the device that a driver has taken control. If reset, this indicates
the driver has given up control, and to me this is a point where a device
should fully reset itself.
So all in all, I can’t see the rationale why any implementation that
supports SET_STATUS would decide to treat SET_STATUS(0) not as equivalent or
a superset of RESET_DEVICE. I may be wrong, and this might explain a whole
deal about what kind of
Re: [PATCH 6/6] vhost-user: Have reset_status fall back to reset
On Wed, Jul 19, 2023 at 04:27:58PM +0200, Hanna Czenczek wrote:
> On 19.07.23 16:11, Hanna Czenczek wrote:
> > On 18.07.23 17:10, Stefan Hajnoczi wrote:
> > > On Tue, Jul 11, 2023 at 05:52:28PM +0200, Hanna Czenczek wrote:
> > > > The only user of vhost_user_reset_status() is vhost_dev_stop(), which
> > > > only uses it as a fall-back to stop the back-end if it does not support
> > > > SUSPEND. However, vhost-user's implementation is a no-op unless the
> > > > back-end supports SET_STATUS.
> > > >
> > > > vhost-vdpa's implementation instead just calls
> > > > vhost_vdpa_reset_device(), implying that it's OK to fully reset the
> > > > device if SET_STATUS is not supported.
> > > >
> > > > To be fair, vhost_vdpa_reset_device() does nothing but to set
> > > > the status
> > > > to zero. However, that may well be because vhost-vdpa has no method
> > > > besides this to reset a device. In contrast, vhost-user has
> > > > RESET_DEVICE and a RESET_OWNER, which can be used instead.
> > > >
> > > > While it is not entirely clear from documentation or git logs, from
> > > > discussions and the order of vhost-user protocol features, it
> > > > appears to
> > > > me as if RESET_OWNER originally had no real meaning for vhost-user, and
> > > > was thus used to signal a device reset to the back-end. Then,
> > > > RESET_DEVICE was introduced, to have a well-defined dedicated reset
> > > > command. Finally, vhost-user received full STATUS support, including
> > > > SET_STATUS, so setting the device status to 0 is now the preferred way
> > > > of resetting a device. Still, RESET_DEVICE and RESET_OWNER should
> > > > remain valid as fall-backs.
> > > >
> > > > Therefore, have vhost_user_reset_status() fall back to
> > > > vhost_user_reset_device() if the back-end has no STATUS support.
> > > >
> > > > Signed-off-by: Hanna Czenczek
> > > > ---
> > > > hw/virtio/vhost-user.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > index 4507de5a92..53a881ec2a 100644
> > > > --- a/hw/virtio/vhost-user.c
> > > > +++ b/hw/virtio/vhost-user.c
> > > > @@ -2833,6 +2833,8 @@ static void vhost_user_reset_status(struct
> > > > vhost_dev *dev)
> > > > if (virtio_has_feature(dev->protocol_features,
> > > > VHOST_USER_PROTOCOL_F_STATUS)) {
> > > > vhost_user_set_status(dev, 0);
> > > > + } else {
> > > > + vhost_user_reset_device(dev);
> > > > }
> > > > }
> > > Did you check whether DPDK treats setting the status to 0 as equivalent
> > > to RESET_DEVICE?
> >
> > If it doesn’t, what’s even the point of using reset_status?
>
> Sorry, I’m being unclear, and I think this may be important because it ties
> into the question from patch 1, what qemu is even trying to do by running
> SET_STATUS(0) vhost_dev_stop(), so here’s what gave me the impression that
> SET_STATUS(0) and RESET_DEVICE should be equivalent:
>
> vhost-vdpa.c runs SET_STATUS(0) in a function called
> vhost_vdpa_reset_device(). This is one thing that gave me the impression
> that this is about an actual full reset.
>
> Another is the whole discussion that we’ve had. vhost_dev_stop() does not
> call a `vhost_reset_device()` function, it calls `vhost_reset_status()`.
> Still, we were always talking about resetting the device.
There is some hacky stuff with struct vhost_dev's vq_index_end and
multi-queue devices. I think it's because multi-queue vhost-net device
consist of many vhost_devs and NetClientStates, so certain vhost
operations are skipped unless this is the "first" or "last" vhost_dev
from a large aggregate vhost-net device. That might be responsible for
part of the weirdness.
>
> It doesn’t make sense to me that vDPA would provide no function to fully
> reset a device, while vhost-user does. Being able to reset a device sounds
> vital to me. This also gave me the impression that SET_STATUS(0) on vDPA at
> least is functionally equivalent to a full device reset.
>
>
> Maybe SET_STATUS(0) does mean a full device reset on vDPA, but not on
> vhost-user. That would be a real shame, so I assumed this would not be the
> case; that SET_STATUS(0) does the same thing on both protocols.
Yes, exactly. It has the real VIRTIO spec meaning in vDPA. In vhost-user
it's currently only used by DPDK as a hint for when device
initialization is complete:
https://github.com/DPDK/dpdk/commit/41d201804c4c44738168e2d247d3b1780845faa1
> The virtio specification says “Writing 0 into this field resets the device.”
> about the device_status field.
>
> This also makes sense, because the device_status field is basically used to
> tell the device that a driver has taken control. If reset, this indicates
> the driver has given up control, and to me this is a point where a device
> should fully reset itself.
>
> So all in all, I can’t see the rationale why any implementation that
> supports SET_STATUS would decide to treat
Re: [PATCH 6/6] vhost-user: Have reset_status fall back to reset
On 19.07.23 16:11, Hanna Czenczek wrote:
On 18.07.23 17:10, Stefan Hajnoczi wrote:
On Tue, Jul 11, 2023 at 05:52:28PM +0200, Hanna Czenczek wrote:
The only user of vhost_user_reset_status() is vhost_dev_stop(), which
only uses it as a fall-back to stop the back-end if it does not support
SUSPEND. However, vhost-user's implementation is a no-op unless the
back-end supports SET_STATUS.
vhost-vdpa's implementation instead just calls
vhost_vdpa_reset_device(), implying that it's OK to fully reset the
device if SET_STATUS is not supported.
To be fair, vhost_vdpa_reset_device() does nothing but to set the
status
to zero. However, that may well be because vhost-vdpa has no method
besides this to reset a device. In contrast, vhost-user has
RESET_DEVICE and a RESET_OWNER, which can be used instead.
While it is not entirely clear from documentation or git logs, from
discussions and the order of vhost-user protocol features, it
appears to
me as if RESET_OWNER originally had no real meaning for vhost-user, and
was thus used to signal a device reset to the back-end. Then,
RESET_DEVICE was introduced, to have a well-defined dedicated reset
command. Finally, vhost-user received full STATUS support, including
SET_STATUS, so setting the device status to 0 is now the preferred way
of resetting a device. Still, RESET_DEVICE and RESET_OWNER should
remain valid as fall-backs.
Therefore, have vhost_user_reset_status() fall back to
vhost_user_reset_device() if the back-end has no STATUS support.
Signed-off-by: Hanna Czenczek
---
hw/virtio/vhost-user.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 4507de5a92..53a881ec2a 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2833,6 +2833,8 @@ static void vhost_user_reset_status(struct
vhost_dev *dev)
if (virtio_has_feature(dev->protocol_features,
VHOST_USER_PROTOCOL_F_STATUS)) {
vhost_user_set_status(dev, 0);
+ } else {
+ vhost_user_reset_device(dev);
}
}
Did you check whether DPDK treats setting the status to 0 as equivalent
to RESET_DEVICE?
If it doesn’t, what’s even the point of using reset_status?
Sorry, I’m being unclear, and I think this may be important because it
ties into the question from patch 1, what qemu is even trying to do by
running SET_STATUS(0) vhost_dev_stop(), so here’s what gave me the
impression that SET_STATUS(0) and RESET_DEVICE should be equivalent:
vhost-vdpa.c runs SET_STATUS(0) in a function called
vhost_vdpa_reset_device(). This is one thing that gave me the
impression that this is about an actual full reset.
Another is the whole discussion that we’ve had. vhost_dev_stop() does
not call a `vhost_reset_device()` function, it calls
`vhost_reset_status()`. Still, we were always talking about resetting
the device.
It doesn’t make sense to me that vDPA would provide no function to fully
reset a device, while vhost-user does. Being able to reset a device
sounds vital to me. This also gave me the impression that SET_STATUS(0)
on vDPA at least is functionally equivalent to a full device reset.
Maybe SET_STATUS(0) does mean a full device reset on vDPA, but not on
vhost-user. That would be a real shame, so I assumed this would not be
the case; that SET_STATUS(0) does the same thing on both protocols.
The virtio specification says “Writing 0 into this field resets the
device.” about the device_status field.
This also makes sense, because the device_status field is basically used
to tell the device that a driver has taken control. If reset, this
indicates the driver has given up control, and to me this is a point
where a device should fully reset itself.
So all in all, I can’t see the rationale why any implementation that
supports SET_STATUS would decide to treat SET_STATUS(0) not as
equivalent or a superset of RESET_DEVICE. I may be wrong, and this
might explain a whole deal about what kind of background operations we
hope to stop with SET_STATUS(0).
Hanna
Re: [PATCH 6/6] vhost-user: Have reset_status fall back to reset
On 18.07.23 17:10, Stefan Hajnoczi wrote:
On Tue, Jul 11, 2023 at 05:52:28PM +0200, Hanna Czenczek wrote:
The only user of vhost_user_reset_status() is vhost_dev_stop(), which
only uses it as a fall-back to stop the back-end if it does not support
SUSPEND. However, vhost-user's implementation is a no-op unless the
back-end supports SET_STATUS.
vhost-vdpa's implementation instead just calls
vhost_vdpa_reset_device(), implying that it's OK to fully reset the
device if SET_STATUS is not supported.
To be fair, vhost_vdpa_reset_device() does nothing but to set the status
to zero. However, that may well be because vhost-vdpa has no method
besides this to reset a device. In contrast, vhost-user has
RESET_DEVICE and a RESET_OWNER, which can be used instead.
While it is not entirely clear from documentation or git logs, from
discussions and the order of vhost-user protocol features, it appears to
me as if RESET_OWNER originally had no real meaning for vhost-user, and
was thus used to signal a device reset to the back-end. Then,
RESET_DEVICE was introduced, to have a well-defined dedicated reset
command. Finally, vhost-user received full STATUS support, including
SET_STATUS, so setting the device status to 0 is now the preferred way
of resetting a device. Still, RESET_DEVICE and RESET_OWNER should
remain valid as fall-backs.
Therefore, have vhost_user_reset_status() fall back to
vhost_user_reset_device() if the back-end has no STATUS support.
Signed-off-by: Hanna Czenczek
---
hw/virtio/vhost-user.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 4507de5a92..53a881ec2a 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2833,6 +2833,8 @@ static void vhost_user_reset_status(struct vhost_dev *dev)
if (virtio_has_feature(dev->protocol_features,
VHOST_USER_PROTOCOL_F_STATUS)) {
vhost_user_set_status(dev, 0);
+} else {
+vhost_user_reset_device(dev);
}
}
Did you check whether DPDK treats setting the status to 0 as equivalent
to RESET_DEVICE?
If it doesn’t, what’s even the point of using reset_status?
I will investigate, but if there’s a difference, that makes the whole
reset_* thing even more questionable to me than it has already been so far.
Hanna
My understanding is that SET_STATUS is mostly ignored by vhost-user
back-ends today. Even those that implement it may not treat SET_STATUS 0
as equivalent to RESET_DEVICE.
If you decide it's safe to make this change, please also update
vhost-user.rst to document that front-ends should use SET_STATUS 0,
RESET_DEVICE, and RESET_OWNER in order of preference.
Stefan
Re: [PATCH 6/6] vhost-user: Have reset_status fall back to reset
On Tue, Jul 11, 2023 at 05:52:28PM +0200, Hanna Czenczek wrote:
> The only user of vhost_user_reset_status() is vhost_dev_stop(), which
> only uses it as a fall-back to stop the back-end if it does not support
> SUSPEND. However, vhost-user's implementation is a no-op unless the
> back-end supports SET_STATUS.
>
> vhost-vdpa's implementation instead just calls
> vhost_vdpa_reset_device(), implying that it's OK to fully reset the
> device if SET_STATUS is not supported.
>
> To be fair, vhost_vdpa_reset_device() does nothing but to set the status
> to zero. However, that may well be because vhost-vdpa has no method
> besides this to reset a device. In contrast, vhost-user has
> RESET_DEVICE and a RESET_OWNER, which can be used instead.
>
> While it is not entirely clear from documentation or git logs, from
> discussions and the order of vhost-user protocol features, it appears to
> me as if RESET_OWNER originally had no real meaning for vhost-user, and
> was thus used to signal a device reset to the back-end. Then,
> RESET_DEVICE was introduced, to have a well-defined dedicated reset
> command. Finally, vhost-user received full STATUS support, including
> SET_STATUS, so setting the device status to 0 is now the preferred way
> of resetting a device. Still, RESET_DEVICE and RESET_OWNER should
> remain valid as fall-backs.
>
> Therefore, have vhost_user_reset_status() fall back to
> vhost_user_reset_device() if the back-end has no STATUS support.
>
> Signed-off-by: Hanna Czenczek
> ---
> hw/virtio/vhost-user.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 4507de5a92..53a881ec2a 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -2833,6 +2833,8 @@ static void vhost_user_reset_status(struct vhost_dev
> *dev)
> if (virtio_has_feature(dev->protocol_features,
> VHOST_USER_PROTOCOL_F_STATUS)) {
> vhost_user_set_status(dev, 0);
> +} else {
> +vhost_user_reset_device(dev);
> }
> }
Did you check whether DPDK treats setting the status to 0 as equivalent
to RESET_DEVICE?
My understanding is that SET_STATUS is mostly ignored by vhost-user
back-ends today. Even those that implement it may not treat SET_STATUS 0
as equivalent to RESET_DEVICE.
If you decide it's safe to make this change, please also update
vhost-user.rst to document that front-ends should use SET_STATUS 0,
RESET_DEVICE, and RESET_OWNER in order of preference.
Stefan
signature.asc
Description: PGP signature
[PATCH 6/6] vhost-user: Have reset_status fall back to reset
The only user of vhost_user_reset_status() is vhost_dev_stop(), which
only uses it as a fall-back to stop the back-end if it does not support
SUSPEND. However, vhost-user's implementation is a no-op unless the
back-end supports SET_STATUS.
vhost-vdpa's implementation instead just calls
vhost_vdpa_reset_device(), implying that it's OK to fully reset the
device if SET_STATUS is not supported.
To be fair, vhost_vdpa_reset_device() does nothing but to set the status
to zero. However, that may well be because vhost-vdpa has no method
besides this to reset a device. In contrast, vhost-user has
RESET_DEVICE and a RESET_OWNER, which can be used instead.
While it is not entirely clear from documentation or git logs, from
discussions and the order of vhost-user protocol features, it appears to
me as if RESET_OWNER originally had no real meaning for vhost-user, and
was thus used to signal a device reset to the back-end. Then,
RESET_DEVICE was introduced, to have a well-defined dedicated reset
command. Finally, vhost-user received full STATUS support, including
SET_STATUS, so setting the device status to 0 is now the preferred way
of resetting a device. Still, RESET_DEVICE and RESET_OWNER should
remain valid as fall-backs.
Therefore, have vhost_user_reset_status() fall back to
vhost_user_reset_device() if the back-end has no STATUS support.
Signed-off-by: Hanna Czenczek
---
hw/virtio/vhost-user.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 4507de5a92..53a881ec2a 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2833,6 +2833,8 @@ static void vhost_user_reset_status(struct vhost_dev *dev)
if (virtio_has_feature(dev->protocol_features,
VHOST_USER_PROTOCOL_F_STATUS)) {
vhost_user_set_status(dev, 0);
+} else {
+vhost_user_reset_device(dev);
}
}
--
2.41.0
