Re: [PATCH 1/4] vdpa: Add stop operation
From: Jason Wang
Date: Mon May 23 2022 - 22:46:41 EST
在 2022/5/24 08:01, Si-Wei Liu 写道:
On 5/23/2022 4:54 PM, Si-Wei Liu wrote:
On 5/23/2022 12:20 PM, Eugenio Perez Martin wrote:
On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
wrote:
On 5/20/2022 10:23 AM, Eugenio Pérez wrote:
This operation is optional: It it's not implemented, backend
feature bit
will not be exposed.
Signed-off-by: Eugenio Pérez <eperezma@xxxxxxxxxx>
---
include/linux/vdpa.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 15af802d41c4..ddfebc4e1e01 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -215,6 +215,11 @@ struct vdpa_map_file {
* @reset: Reset device
* @vdev: vdpa device
* Returns integer: success (0) or
error (< 0)
+ * @stop: Stop or resume the device (optional,
but it must
+ * be implemented if require device stop)
+ * @vdev: vdpa device
+ * @stop: stop (true), not stop (false)
+ * Returns integer: success (0) or
error (< 0)
Is this uAPI meant to address all use cases described in the full
blown
_F_STOP virtio spec proposal, such as:
--------------%<--------------
...... the device MUST finish any in flight
operations after the driver writes STOP. Depending on the device, it
can do it
in many ways as long as the driver can recover its normal operation
if it
resumes the device without the need of resetting it:
- Drain and wait for the completion of all pending requests until a
convenient avail descriptor. Ignore any other posterior
descriptor.
- Return a device-specific failure for these descriptors, so the
driver
can choose to retry or to cancel them.
- Mark them as done even if they are not, if the kind of device can
assume to lose them.
--------------%<--------------
Right, this is totally underspecified in this series.
I'll expand on it in the next version, but that text proposed to
virtio-comment was complicated and misleading. I find better to get
the previous version description. Would the next description work?
```
After the return of ioctl, the device MUST finish any pending
operations like
in flight requests. It must also preserve all the necessary state (the
virtqueue vring base plus the possible device specific states)
Hmmm, "possible device specific states" is a bit vague. Does it
require the device to save any device internal state that is not
defined in the virtio spec - such as any failed in-flight requests to
resubmit upon resume? Or you would lean on SVQ to intercept it in
depth and save it with some other means? I think network device also
has internal state such as flow steering state that needs bookkeeping
as well.
Noted that I understand you may introduce additional feature call
similar to VHOST_USER_GET_INFLIGHT_FD for (failed) in-flight request,
but since that's is a get interface, I assume the actual state
preserving should still take place in this STOP call.
Yes, I think so.
-Siwei
A follow-up question is what is the use of the `stop` argument of
false, does it require the device to support resume?
Yes, this is required by the hypervisor e.g for Qemu it supports vm
stop/resume.
I seem to recall this is something to abandon in favor of device
reset plus setting queue base/addr after. Or it's just a optional
feature that may be device specific (if one can do so in simple way).
Rest is more like a workarond consider we don't have a stop API.
Consider we don't add stop at the beginning, it can only be an optional
feature.
Thanks
-Siwei
that is required
for restoring in the future.
In the future, we will provide features similar to
VHOST_USER_GET_INFLIGHT_FD
so the device can save pending operations.
```
Thanks for pointing it out!
E.g. do I assume correctly all in flight requests are flushed after
return from this uAPI call? Or some of pending requests may be subject
to loss or failure? How does the caller/user specify these various
options (if there are) for device stop?
BTW, it would be nice to add the corresponding support to vdpa_sim_blk
as well to demo the stop handling. To just show it on vdpa-sim-net
IMHO
is perhaps not so convincing.
-Siwei
* @get_config_size: Get the size of the configuration space
includes
* fields that are conditional on
feature bits.
* @vdev: vdpa device
@@ -316,6 +321,7 @@ struct vdpa_config_ops {
u8 (*get_status)(struct vdpa_device *vdev);
void (*set_status)(struct vdpa_device *vdev, u8 status);
int (*reset)(struct vdpa_device *vdev);
+ int (*stop)(struct vdpa_device *vdev, bool stop);
size_t (*get_config_size)(struct vdpa_device *vdev);
void (*get_config)(struct vdpa_device *vdev, unsigned int
offset,
void *buf, unsigned int len);