* [PATCH 1/4] vdpa: Add stop operation
2022-05-20 17:23 [PATCH 0/4] Implement vdpasim stop operation Eugenio Pérez
@ 2022-05-20 17:23 ` Eugenio Pérez
2022-05-21 10:13 ` Si-Wei Liu
2022-05-20 17:23 ` [PATCH 2/4] vhost-vdpa: introduce STOP backend feature bit Eugenio Pérez
` (2 subsequent siblings)
3 siblings, 1 reply; 30+ messages in thread
From: Eugenio Pérez @ 2022-05-20 17:23 UTC (permalink / raw)
To: virtualization, Jason Wang, kvm, Michael S. Tsirkin, netdev,
linux-kernel
Cc: Stefano Garzarella, Longpeng, Zhu Lingshan, martinh, hanand,
Si-Wei Liu, dinang, Eli Cohen, lvivier, pabloc, gautam.dawar,
Xie Yongji, habetsm.xilinx, Christophe JAILLET, tanuj.kamde,
eperezma, Wu Zongyong, martinpo, lulu, ecree.xilinx,
Parav Pandit, Dan Carpenter, Zhang Min
This operation is optional: It it's not implemented, backend feature bit
will not be exposed.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
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)
* @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);
--
2.27.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 1/4] vdpa: Add stop operation
2022-05-20 17:23 ` [PATCH 1/4] vdpa: Add " Eugenio Pérez
@ 2022-05-21 10:13 ` Si-Wei Liu
0 siblings, 0 replies; 30+ messages in thread
From: Si-Wei Liu @ 2022-05-21 10:13 UTC (permalink / raw)
To: Eugenio Pérez, virtualization, Jason Wang, kvm,
Michael S. Tsirkin, netdev, linux-kernel
Cc: Stefano Garzarella, Longpeng, Zhu Lingshan, martinh, hanand,
dinang, Eli Cohen, lvivier, pabloc, gautam.dawar, Xie Yongji,
habetsm.xilinx, Christophe JAILLET, tanuj.kamde, Wu Zongyong,
martinpo, lulu, ecree.xilinx, Parav Pandit, Dan Carpenter,
Zhang Min
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@redhat.com>
> ---
> 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.
--------------%<--------------
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);
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/4] vdpa: Add stop operation
@ 2022-05-21 10:13 ` Si-Wei Liu
0 siblings, 0 replies; 30+ messages in thread
From: Si-Wei Liu @ 2022-05-21 10:13 UTC (permalink / raw)
To: Eugenio Pérez, virtualization, Jason Wang, kvm,
Michael S. Tsirkin, netdev, linux-kernel
Cc: lvivier, Zhang Min, Eli Cohen, lulu, ecree.xilinx, gautam.dawar,
Christophe JAILLET, tanuj.kamde, martinh, Xie Yongji, hanand,
habetsm.xilinx, Dan Carpenter, martinpo, dinang, pabloc,
Longpeng, Zhu Lingshan, Wu Zongyong
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@redhat.com>
> ---
> 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.
--------------%<--------------
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);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/4] vdpa: Add stop operation
2022-05-21 10:13 ` Si-Wei Liu
(?)
@ 2022-05-23 19:20 ` Eugenio Perez Martin
2022-05-23 23:54 ` Si-Wei Liu
2022-05-24 7:09 ` Stefano Garzarella
-1 siblings, 2 replies; 30+ messages in thread
From: Eugenio Perez Martin @ 2022-05-23 19:20 UTC (permalink / raw)
To: Si-Wei Liu
Cc: virtualization, Jason Wang, kvm list, Michael S. Tsirkin, netdev,
linux-kernel, Stefano Garzarella, Longpeng, Zhu Lingshan,
Martin Petrus Hubertus Habets, Harpreet Singh Anand, dinang,
Eli Cohen, Laurent Vivier, pabloc, Dawar, Gautam, Xie Yongji,
habetsm.xilinx, Christophe JAILLET, tanuj.kamde, Wu Zongyong,
martinpo, Cindy Lu, ecree.xilinx, Parav Pandit, Dan Carpenter,
Zhang Min
On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> 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@redhat.com>
> > ---
> > 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) 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);
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/4] vdpa: Add stop operation
2022-05-23 19:20 ` Eugenio Perez Martin
@ 2022-05-23 23:54 ` Si-Wei Liu
2022-05-24 7:09 ` Stefano Garzarella
1 sibling, 0 replies; 30+ messages in thread
From: Si-Wei Liu @ 2022-05-23 23:54 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: virtualization, Jason Wang, kvm list, Michael S. Tsirkin, netdev,
linux-kernel, Stefano Garzarella, Longpeng, Zhu Lingshan,
Martin Petrus Hubertus Habets, Harpreet Singh Anand, dinang,
Eli Cohen, Laurent Vivier, pabloc, Dawar, Gautam, Xie Yongji,
habetsm.xilinx, Christophe JAILLET, tanuj.kamde, Wu Zongyong,
martinpo, Cindy Lu, ecree.xilinx, Parav Pandit, Dan Carpenter,
Zhang Min
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@oracle.com> 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@redhat.com>
>>> ---
>>> 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.
A follow-up question is what is the use of the `stop` argument of false,
does it require the device to support 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).
-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);
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/4] vdpa: Add stop operation
@ 2022-05-23 23:54 ` Si-Wei Liu
0 siblings, 0 replies; 30+ messages in thread
From: Si-Wei Liu @ 2022-05-23 23:54 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: tanuj.kamde, kvm list, Michael S. Tsirkin, virtualization,
Wu Zongyong, pabloc, Eli Cohen, Zhang Min, Cindy Lu,
Martin Petrus Hubertus Habets, Xie Yongji, dinang,
habetsm.xilinx, Longpeng, Dan Carpenter, Laurent Vivier,
Christophe JAILLET, netdev, linux-kernel, ecree.xilinx,
Harpreet Singh Anand, martinpo, Dawar, Gautam, Zhu Lingshan
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@oracle.com> 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@redhat.com>
>>> ---
>>> 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.
A follow-up question is what is the use of the `stop` argument of false,
does it require the device to support 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).
-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);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/4] vdpa: Add stop operation
2022-05-23 23:54 ` Si-Wei Liu
@ 2022-05-24 0:01 ` Si-Wei Liu
-1 siblings, 0 replies; 30+ messages in thread
From: Si-Wei Liu @ 2022-05-24 0:01 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: virtualization, Jason Wang, kvm list, Michael S. Tsirkin, netdev,
linux-kernel, Stefano Garzarella, Longpeng, Zhu Lingshan,
Martin Petrus Hubertus Habets, Harpreet Singh Anand, dinang,
Eli Cohen, Laurent Vivier, pabloc, Dawar, Gautam, Xie Yongji,
habetsm.xilinx, Christophe JAILLET, tanuj.kamde, Wu Zongyong,
martinpo, Cindy Lu, ecree.xilinx, Parav Pandit, Dan Carpenter,
Zhang Min
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@oracle.com>
>> 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@redhat.com>
>>>> ---
>>>> 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.
-Siwei
>
> A follow-up question is what is the use of the `stop` argument of
> false, does it require the device to support 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).
>
> -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);
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/4] vdpa: Add stop operation
@ 2022-05-24 0:01 ` Si-Wei Liu
0 siblings, 0 replies; 30+ messages in thread
From: Si-Wei Liu @ 2022-05-24 0:01 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: tanuj.kamde, kvm list, Michael S. Tsirkin, virtualization,
Wu Zongyong, pabloc, Eli Cohen, Zhang Min, Cindy Lu,
Martin Petrus Hubertus Habets, Xie Yongji, dinang,
habetsm.xilinx, Longpeng, Dan Carpenter, Laurent Vivier,
Christophe JAILLET, netdev, linux-kernel, ecree.xilinx,
Harpreet Singh Anand, martinpo, Dawar, Gautam, Zhu Lingshan
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@oracle.com>
>> 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@redhat.com>
>>>> ---
>>>> 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.
-Siwei
>
> A follow-up question is what is the use of the `stop` argument of
> false, does it require the device to support 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).
>
> -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);
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/4] vdpa: Add stop operation
2022-05-24 0:01 ` Si-Wei Liu
@ 2022-05-24 2:45 ` Jason Wang
-1 siblings, 0 replies; 30+ messages in thread
From: Jason Wang @ 2022-05-24 2:45 UTC (permalink / raw)
To: Si-Wei Liu, Eugenio Perez Martin
Cc: virtualization, kvm list, Michael S. Tsirkin, netdev,
linux-kernel, Stefano Garzarella, Longpeng, Zhu Lingshan,
Martin Petrus Hubertus Habets, Harpreet Singh Anand, dinang,
Eli Cohen, Laurent Vivier, pabloc, Dawar, Gautam, Xie Yongji,
habetsm.xilinx, Christophe JAILLET, tanuj.kamde, Wu Zongyong,
martinpo, Cindy Lu, ecree.xilinx, Parav Pandit, Dan Carpenter,
Zhang Min
在 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@oracle.com>
>>> 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@redhat.com>
>>>>> ---
>>>>> 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);
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/4] vdpa: Add stop operation
@ 2022-05-24 2:45 ` Jason Wang
0 siblings, 0 replies; 30+ messages in thread
From: Jason Wang @ 2022-05-24 2:45 UTC (permalink / raw)
To: Si-Wei Liu, Eugenio Perez Martin
Cc: tanuj.kamde, kvm list, Michael S. Tsirkin, virtualization,
Wu Zongyong, pabloc, Eli Cohen, Zhang Min, Cindy Lu,
Martin Petrus Hubertus Habets, Xie Yongji, dinang,
habetsm.xilinx, Longpeng, Dan Carpenter, Laurent Vivier,
Christophe JAILLET, netdev, linux-kernel, ecree.xilinx,
Harpreet Singh Anand, martinpo, Dawar, Gautam, Zhu Lingshan
在 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@oracle.com>
>>> 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@redhat.com>
>>>>> ---
>>>>> 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);
>>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/4] vdpa: Add stop operation
2022-05-24 0:01 ` Si-Wei Liu
(?)
(?)
@ 2022-05-24 7:38 ` Eugenio Perez Martin
-1 siblings, 0 replies; 30+ messages in thread
From: Eugenio Perez Martin @ 2022-05-24 7:38 UTC (permalink / raw)
To: Si-Wei Liu
Cc: virtualization, Jason Wang, kvm list, Michael S. Tsirkin, netdev,
linux-kernel, Stefano Garzarella, Longpeng, Zhu Lingshan,
Martin Petrus Hubertus Habets, Harpreet Singh Anand, dinang,
Eli Cohen, Laurent Vivier, pabloc, Dawar, Gautam, Xie Yongji,
habetsm.xilinx, Christophe JAILLET, tanuj.kamde, Wu Zongyong,
martinpo, Cindy Lu, ecree.xilinx, Parav Pandit, Dan Carpenter,
Zhang Min
On Tue, May 24, 2022 at 2:01 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> 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@oracle.com>
> >> 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@redhat.com>
> >>>> ---
> >>>> 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?
I'd let that be device-specific. For example, the net simulator
doesn't need to store them, since it cannot stop while processing
buffers. Other net devices can also decide to simply drop or re-submit
tx frames.
I can check for the block simulator if that's possible too. For
hardware vdpa block devices, this should be combined with the future
"get inflight buffers" call for sure.
> > 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.
Yes, for state set by the control vq a permanent SVQ is used only for
the cvq. For other things like config space vdpa already presents an
emulated one to the guest, so we're safe in that regard.
> 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.
>
Right. I'll add all of this to the proposal.
Thanks!
> -Siwei
>
> >
> > A follow-up question is what is the use of the `stop` argument of
> > false, does it require the device to support 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).
> >
> > -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);
> >
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/4] vdpa: Add stop operation
2022-05-23 19:20 ` Eugenio Perez Martin
@ 2022-05-24 7:09 ` Stefano Garzarella
2022-05-24 7:09 ` Stefano Garzarella
1 sibling, 0 replies; 30+ messages in thread
From: Stefano Garzarella @ 2022-05-24 7:09 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: Si-Wei Liu, virtualization, Jason Wang, kvm list,
Michael S. Tsirkin, netdev, linux-kernel, Longpeng, Zhu Lingshan,
Martin Petrus Hubertus Habets, Harpreet Singh Anand, dinang,
Eli Cohen, Laurent Vivier, pabloc, Dawar, Gautam, Xie Yongji,
habetsm.xilinx, Christophe JAILLET, tanuj.kamde, Wu Zongyong,
martinpo, Cindy Lu, ecree.xilinx, Parav Pandit, Dan Carpenter,
Zhang Min
On Mon, May 23, 2022 at 09:20:14PM +0200, Eugenio Perez Martin wrote:
>On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> 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@redhat.com>
>> > ---
>> > 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) that is required
>for restoring in the future.
For block devices wait for all in-flight requests could take several
time.
Could this be a problem if the caller gets stuck on this ioctl?
If it could be a problem, maybe we should use an eventfd to signal that
the device is successfully stopped.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/4] vdpa: Add stop operation
@ 2022-05-24 7:09 ` Stefano Garzarella
0 siblings, 0 replies; 30+ messages in thread
From: Stefano Garzarella @ 2022-05-24 7:09 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: tanuj.kamde, kvm list, Michael S. Tsirkin, virtualization,
Wu Zongyong, Si-Wei Liu, pabloc, Eli Cohen, Zhang Min, Cindy Lu,
Martin Petrus Hubertus Habets, Xie Yongji, dinang,
habetsm.xilinx, Longpeng, Dan Carpenter, Laurent Vivier,
Christophe JAILLET, netdev, linux-kernel, ecree.xilinx,
Harpreet Singh Anand, martinpo, Dawar, Gautam, Zhu Lingshan
On Mon, May 23, 2022 at 09:20:14PM +0200, Eugenio Perez Martin wrote:
>On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> 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@redhat.com>
>> > ---
>> > 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) that is required
>for restoring in the future.
For block devices wait for all in-flight requests could take several
time.
Could this be a problem if the caller gets stuck on this ioctl?
If it could be a problem, maybe we should use an eventfd to signal that
the device is successfully stopped.
Thanks,
Stefano
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/4] vdpa: Add stop operation
2022-05-24 7:09 ` Stefano Garzarella
(?)
@ 2022-05-24 7:42 ` Eugenio Perez Martin
2022-05-24 7:51 ` Stefano Garzarella
-1 siblings, 1 reply; 30+ messages in thread
From: Eugenio Perez Martin @ 2022-05-24 7:42 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Si-Wei Liu, virtualization, Jason Wang, kvm list,
Michael S. Tsirkin, netdev, linux-kernel, Longpeng, Zhu Lingshan,
Martin Petrus Hubertus Habets, Harpreet Singh Anand, dinang,
Eli Cohen, Laurent Vivier, pabloc, Dawar, Gautam, Xie Yongji,
habetsm.xilinx, Christophe JAILLET, tanuj.kamde, Wu Zongyong,
martinpo, Cindy Lu, ecree.xilinx, Parav Pandit, Dan Carpenter,
Zhang Min
On Tue, May 24, 2022 at 9:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Mon, May 23, 2022 at 09:20:14PM +0200, Eugenio Perez Martin wrote:
> >On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> 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@redhat.com>
> >> > ---
> >> > 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) that is required
> >for restoring in the future.
>
> For block devices wait for all in-flight requests could take several
> time.
>
> Could this be a problem if the caller gets stuck on this ioctl?
>
> If it could be a problem, maybe we should use an eventfd to signal that
> the device is successfully stopped.
>
For that particular problem I'd very much prefer to add directly an
ioctl to get the inflight descriptors. We know for sure we will need
them, and it will be cleaner in the long run.
As I understand the vdpa block simulator, there is no need to return
the inflight descriptors since all of the requests are processed in a
synchronous way. So, for this iteration, we could offer the stop
feature to qemu.
Other non-simulated devices would need it. Could it be delayed to
future development?
Thanks!
> Thanks,
> Stefano
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/4] vdpa: Add stop operation
2022-05-24 7:42 ` Eugenio Perez Martin
@ 2022-05-24 7:51 ` Stefano Garzarella
0 siblings, 0 replies; 30+ messages in thread
From: Stefano Garzarella @ 2022-05-24 7:51 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: Si-Wei Liu, virtualization, Jason Wang, kvm list,
Michael S. Tsirkin, netdev, linux-kernel, Longpeng, Zhu Lingshan,
Martin Petrus Hubertus Habets, Harpreet Singh Anand, dinang,
Eli Cohen, Laurent Vivier, pabloc, Dawar, Gautam, Xie Yongji,
habetsm.xilinx, Christophe JAILLET, tanuj.kamde, Wu Zongyong,
martinpo, Cindy Lu, ecree.xilinx, Parav Pandit, Dan Carpenter,
Zhang Min
On Tue, May 24, 2022 at 09:42:06AM +0200, Eugenio Perez Martin wrote:
>On Tue, May 24, 2022 at 9:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Mon, May 23, 2022 at 09:20:14PM +0200, Eugenio Perez Martin wrote:
>> >On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> 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@redhat.com>
>> >> > ---
>> >> > 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) that is required
>> >for restoring in the future.
>>
>> For block devices wait for all in-flight requests could take several
>> time.
>>
>> Could this be a problem if the caller gets stuck on this ioctl?
>>
>> If it could be a problem, maybe we should use an eventfd to signal that
>> the device is successfully stopped.
>>
>
>For that particular problem I'd very much prefer to add directly an
>ioctl to get the inflight descriptors. We know for sure we will need
>them, and it will be cleaner in the long run.
Makes sense!
>
>As I understand the vdpa block simulator, there is no need to return
>the inflight descriptors since all of the requests are processed in a
>synchronous way. So, for this iteration, we could offer the stop
>feature to qemu.
Right, the simulator handles everything synchronously.
>
>Other non-simulated devices would need it. Could it be delayed to
>future development?
Yep, sure, it sounds like you already have a plan, so no problem :-)
Thanks,
Stefano
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/4] vdpa: Add stop operation
@ 2022-05-24 7:51 ` Stefano Garzarella
0 siblings, 0 replies; 30+ messages in thread
From: Stefano Garzarella @ 2022-05-24 7:51 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: tanuj.kamde, kvm list, Michael S. Tsirkin, virtualization,
Wu Zongyong, Si-Wei Liu, pabloc, Eli Cohen, Zhang Min, Cindy Lu,
Martin Petrus Hubertus Habets, Xie Yongji, dinang,
habetsm.xilinx, Longpeng, Dan Carpenter, Laurent Vivier,
Christophe JAILLET, netdev, linux-kernel, ecree.xilinx,
Harpreet Singh Anand, martinpo, Dawar, Gautam, Zhu Lingshan
On Tue, May 24, 2022 at 09:42:06AM +0200, Eugenio Perez Martin wrote:
>On Tue, May 24, 2022 at 9:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Mon, May 23, 2022 at 09:20:14PM +0200, Eugenio Perez Martin wrote:
>> >On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> 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@redhat.com>
>> >> > ---
>> >> > 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) that is required
>> >for restoring in the future.
>>
>> For block devices wait for all in-flight requests could take several
>> time.
>>
>> Could this be a problem if the caller gets stuck on this ioctl?
>>
>> If it could be a problem, maybe we should use an eventfd to signal that
>> the device is successfully stopped.
>>
>
>For that particular problem I'd very much prefer to add directly an
>ioctl to get the inflight descriptors. We know for sure we will need
>them, and it will be cleaner in the long run.
Makes sense!
>
>As I understand the vdpa block simulator, there is no need to return
>the inflight descriptors since all of the requests are processed in a
>synchronous way. So, for this iteration, we could offer the stop
>feature to qemu.
Right, the simulator handles everything synchronously.
>
>Other non-simulated devices would need it. Could it be delayed to
>future development?
Yep, sure, it sounds like you already have a plan, so no problem :-)
Thanks,
Stefano
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 2/4] vhost-vdpa: introduce STOP backend feature bit
2022-05-20 17:23 [PATCH 0/4] Implement vdpasim stop operation Eugenio Pérez
2022-05-20 17:23 ` [PATCH 1/4] vdpa: Add " Eugenio Pérez
@ 2022-05-20 17:23 ` Eugenio Pérez
2022-05-21 10:24 ` Si-Wei Liu
2022-05-20 17:23 ` [PATCH 3/4] vhost-vdpa: uAPI to stop the device Eugenio Pérez
2022-05-20 17:23 ` [PATCH 4/4] vdpa_sim: Implement stop vdpa op Eugenio Pérez
3 siblings, 1 reply; 30+ messages in thread
From: Eugenio Pérez @ 2022-05-20 17:23 UTC (permalink / raw)
To: virtualization, Jason Wang, kvm, Michael S. Tsirkin, netdev,
linux-kernel
Cc: Stefano Garzarella, Longpeng, Zhu Lingshan, martinh, hanand,
Si-Wei Liu, dinang, Eli Cohen, lvivier, pabloc, gautam.dawar,
Xie Yongji, habetsm.xilinx, Christophe JAILLET, tanuj.kamde,
eperezma, Wu Zongyong, martinpo, lulu, ecree.xilinx,
Parav Pandit, Dan Carpenter, Zhang Min
Userland knows if it can stop the device or not by checking this feature
bit.
It's only offered if the vdpa driver backend implements the stop()
operation callback, and try to set it if the backend does not offer that
callback is an error.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
drivers/vhost/vdpa.c | 13 +++++++++++++
include/uapi/linux/vhost_types.h | 2 ++
2 files changed, 15 insertions(+)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 1f1d1c425573..a325bc259afb 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
return 0;
}
+static bool vhost_vdpa_can_stop(const struct vhost_vdpa *v)
+{
+ struct vdpa_device *vdpa = v->vdpa;
+ const struct vdpa_config_ops *ops = vdpa->config;
+
+ return ops->stop;
+}
+
static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
{
struct vdpa_device *vdpa = v->vdpa;
@@ -577,6 +585,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
return -EFAULT;
if (features & ~VHOST_VDPA_BACKEND_FEATURES)
return -EOPNOTSUPP;
+ if ((features & VHOST_BACKEND_F_STOP) &&
+ !vhost_vdpa_can_stop(v))
+ return -EOPNOTSUPP;
vhost_set_backend_features(&v->vdev, features);
return 0;
}
@@ -624,6 +635,8 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
break;
case VHOST_GET_BACKEND_FEATURES:
features = VHOST_VDPA_BACKEND_FEATURES;
+ if (vhost_vdpa_can_stop(v))
+ features |= VHOST_BACKEND_F_STOP;
if (copy_to_user(featurep, &features, sizeof(features)))
r = -EFAULT;
break;
diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
index 634cee485abb..2758e665791b 100644
--- a/include/uapi/linux/vhost_types.h
+++ b/include/uapi/linux/vhost_types.h
@@ -161,5 +161,7 @@ struct vhost_vdpa_iova_range {
* message
*/
#define VHOST_BACKEND_F_IOTLB_ASID 0x3
+/* Stop device from processing virtqueue buffers */
+#define VHOST_BACKEND_F_STOP 0x4
#endif
--
2.27.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 2/4] vhost-vdpa: introduce STOP backend feature bit
2022-05-20 17:23 ` [PATCH 2/4] vhost-vdpa: introduce STOP backend feature bit Eugenio Pérez
@ 2022-05-21 10:24 ` Si-Wei Liu
0 siblings, 0 replies; 30+ messages in thread
From: Si-Wei Liu @ 2022-05-21 10:24 UTC (permalink / raw)
To: Eugenio Pérez, virtualization, Jason Wang, kvm,
Michael S. Tsirkin, netdev, linux-kernel
Cc: Stefano Garzarella, Longpeng, Zhu Lingshan, martinh, hanand,
dinang, Eli Cohen, lvivier, pabloc, gautam.dawar, Xie Yongji,
habetsm.xilinx, Christophe JAILLET, tanuj.kamde, Wu Zongyong,
martinpo, lulu, ecree.xilinx, Parav Pandit, Dan Carpenter,
Zhang Min
On 5/20/2022 10:23 AM, Eugenio Pérez wrote:
> Userland knows if it can stop the device or not by checking this feature
> bit.
>
> It's only offered if the vdpa driver backend implements the stop()
> operation callback, and try to set it if the backend does not offer that
> callback is an error.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
> drivers/vhost/vdpa.c | 13 +++++++++++++
> include/uapi/linux/vhost_types.h | 2 ++
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 1f1d1c425573..a325bc259afb 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
> return 0;
> }
>
> +static bool vhost_vdpa_can_stop(const struct vhost_vdpa *v)
> +{
> + struct vdpa_device *vdpa = v->vdpa;
> + const struct vdpa_config_ops *ops = vdpa->config;
> +
> + return ops->stop;
> +}
> +
> static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
> {
> struct vdpa_device *vdpa = v->vdpa;
> @@ -577,6 +585,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> return -EFAULT;
> if (features & ~VHOST_VDPA_BACKEND_FEATURES)
> return -EOPNOTSUPP;
> + if ((features & VHOST_BACKEND_F_STOP) &&
VHOST_BACKEND_F_STOP is not part of VHOST_VDPA_BACKEND_FEATURES. There's
no chance for VHOST_BACKEND_F_STOP to get here.
-Siwei
> + !vhost_vdpa_can_stop(v))
> + return -EOPNOTSUPP;
> vhost_set_backend_features(&v->vdev, features);
> return 0;
> }
> @@ -624,6 +635,8 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> break;
> case VHOST_GET_BACKEND_FEATURES:
> features = VHOST_VDPA_BACKEND_FEATURES;
> + if (vhost_vdpa_can_stop(v))
> + features |= VHOST_BACKEND_F_STOP;
> if (copy_to_user(featurep, &features, sizeof(features)))
> r = -EFAULT;
> break;
> diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
> index 634cee485abb..2758e665791b 100644
> --- a/include/uapi/linux/vhost_types.h
> +++ b/include/uapi/linux/vhost_types.h
> @@ -161,5 +161,7 @@ struct vhost_vdpa_iova_range {
> * message
> */
> #define VHOST_BACKEND_F_IOTLB_ASID 0x3
> +/* Stop device from processing virtqueue buffers */
> +#define VHOST_BACKEND_F_STOP 0x4
>
> #endif
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 2/4] vhost-vdpa: introduce STOP backend feature bit
@ 2022-05-21 10:24 ` Si-Wei Liu
0 siblings, 0 replies; 30+ messages in thread
From: Si-Wei Liu @ 2022-05-21 10:24 UTC (permalink / raw)
To: Eugenio Pérez, virtualization, Jason Wang, kvm,
Michael S. Tsirkin, netdev, linux-kernel
Cc: lvivier, Zhang Min, Eli Cohen, lulu, ecree.xilinx, gautam.dawar,
Christophe JAILLET, tanuj.kamde, martinh, Xie Yongji, hanand,
habetsm.xilinx, Dan Carpenter, martinpo, dinang, pabloc,
Longpeng, Zhu Lingshan, Wu Zongyong
On 5/20/2022 10:23 AM, Eugenio Pérez wrote:
> Userland knows if it can stop the device or not by checking this feature
> bit.
>
> It's only offered if the vdpa driver backend implements the stop()
> operation callback, and try to set it if the backend does not offer that
> callback is an error.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
> drivers/vhost/vdpa.c | 13 +++++++++++++
> include/uapi/linux/vhost_types.h | 2 ++
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 1f1d1c425573..a325bc259afb 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
> return 0;
> }
>
> +static bool vhost_vdpa_can_stop(const struct vhost_vdpa *v)
> +{
> + struct vdpa_device *vdpa = v->vdpa;
> + const struct vdpa_config_ops *ops = vdpa->config;
> +
> + return ops->stop;
> +}
> +
> static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
> {
> struct vdpa_device *vdpa = v->vdpa;
> @@ -577,6 +585,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> return -EFAULT;
> if (features & ~VHOST_VDPA_BACKEND_FEATURES)
> return -EOPNOTSUPP;
> + if ((features & VHOST_BACKEND_F_STOP) &&
VHOST_BACKEND_F_STOP is not part of VHOST_VDPA_BACKEND_FEATURES. There's
no chance for VHOST_BACKEND_F_STOP to get here.
-Siwei
> + !vhost_vdpa_can_stop(v))
> + return -EOPNOTSUPP;
> vhost_set_backend_features(&v->vdev, features);
> return 0;
> }
> @@ -624,6 +635,8 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> break;
> case VHOST_GET_BACKEND_FEATURES:
> features = VHOST_VDPA_BACKEND_FEATURES;
> + if (vhost_vdpa_can_stop(v))
> + features |= VHOST_BACKEND_F_STOP;
> if (copy_to_user(featurep, &features, sizeof(features)))
> r = -EFAULT;
> break;
> diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
> index 634cee485abb..2758e665791b 100644
> --- a/include/uapi/linux/vhost_types.h
> +++ b/include/uapi/linux/vhost_types.h
> @@ -161,5 +161,7 @@ struct vhost_vdpa_iova_range {
> * message
> */
> #define VHOST_BACKEND_F_IOTLB_ASID 0x3
> +/* Stop device from processing virtqueue buffers */
> +#define VHOST_BACKEND_F_STOP 0x4
>
> #endif
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 2/4] vhost-vdpa: introduce STOP backend feature bit
2022-05-21 10:24 ` Si-Wei Liu
(?)
@ 2022-05-23 9:57 ` Eugenio Perez Martin
-1 siblings, 0 replies; 30+ messages in thread
From: Eugenio Perez Martin @ 2022-05-23 9:57 UTC (permalink / raw)
To: Si-Wei Liu
Cc: virtualization, Jason Wang, kvm list, Michael S. Tsirkin, netdev,
linux-kernel, Stefano Garzarella, Longpeng, Zhu Lingshan,
Martin Petrus Hubertus Habets, Harpreet Singh Anand, dinang,
Eli Cohen, Laurent Vivier, pabloc, Dawar, Gautam, Xie Yongji,
habetsm.xilinx, Christophe JAILLET, tanuj.kamde, Wu Zongyong,
martinpo, Cindy Lu, ecree.xilinx, Parav Pandit, Dan Carpenter,
Zhang Min
On Sat, May 21, 2022 at 12:25 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 5/20/2022 10:23 AM, Eugenio Pérez wrote:
> > Userland knows if it can stop the device or not by checking this feature
> > bit.
> >
> > It's only offered if the vdpa driver backend implements the stop()
> > operation callback, and try to set it if the backend does not offer that
> > callback is an error.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> > drivers/vhost/vdpa.c | 13 +++++++++++++
> > include/uapi/linux/vhost_types.h | 2 ++
> > 2 files changed, 15 insertions(+)
> >
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index 1f1d1c425573..a325bc259afb 100644
> > --- a/drivers/vhost/vdpa.c
> > +++ b/drivers/vhost/vdpa.c
> > @@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
> > return 0;
> > }
> >
> > +static bool vhost_vdpa_can_stop(const struct vhost_vdpa *v)
> > +{
> > + struct vdpa_device *vdpa = v->vdpa;
> > + const struct vdpa_config_ops *ops = vdpa->config;
> > +
> > + return ops->stop;
> > +}
> > +
> > static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
> > {
> > struct vdpa_device *vdpa = v->vdpa;
> > @@ -577,6 +585,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> > return -EFAULT;
> > if (features & ~VHOST_VDPA_BACKEND_FEATURES)
> > return -EOPNOTSUPP;
> > + if ((features & VHOST_BACKEND_F_STOP) &&
> VHOST_BACKEND_F_STOP is not part of VHOST_VDPA_BACKEND_FEATURES. There's
> no chance for VHOST_BACKEND_F_STOP to get here.
>
That's right. I think I missed to backport your patches about set
backend_features only once in my testing.
I will re-test with the latest qemu master, thanks!
> -Siwei
> > + !vhost_vdpa_can_stop(v))
> > + return -EOPNOTSUPP;
> > vhost_set_backend_features(&v->vdev, features);
> > return 0;
> > }
> > @@ -624,6 +635,8 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> > break;
> > case VHOST_GET_BACKEND_FEATURES:
> > features = VHOST_VDPA_BACKEND_FEATURES;
> > + if (vhost_vdpa_can_stop(v))
> > + features |= VHOST_BACKEND_F_STOP;
> > if (copy_to_user(featurep, &features, sizeof(features)))
> > r = -EFAULT;
> > break;
> > diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
> > index 634cee485abb..2758e665791b 100644
> > --- a/include/uapi/linux/vhost_types.h
> > +++ b/include/uapi/linux/vhost_types.h
> > @@ -161,5 +161,7 @@ struct vhost_vdpa_iova_range {
> > * message
> > */
> > #define VHOST_BACKEND_F_IOTLB_ASID 0x3
> > +/* Stop device from processing virtqueue buffers */
> > +#define VHOST_BACKEND_F_STOP 0x4
> >
> > #endif
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 3/4] vhost-vdpa: uAPI to stop the device
2022-05-20 17:23 [PATCH 0/4] Implement vdpasim stop operation Eugenio Pérez
2022-05-20 17:23 ` [PATCH 1/4] vdpa: Add " Eugenio Pérez
2022-05-20 17:23 ` [PATCH 2/4] vhost-vdpa: introduce STOP backend feature bit Eugenio Pérez
@ 2022-05-20 17:23 ` Eugenio Pérez
2022-05-21 5:20 ` kernel test robot
2022-05-21 8:36 ` Martin Habets
2022-05-20 17:23 ` [PATCH 4/4] vdpa_sim: Implement stop vdpa op Eugenio Pérez
3 siblings, 2 replies; 30+ messages in thread
From: Eugenio Pérez @ 2022-05-20 17:23 UTC (permalink / raw)
To: virtualization, Jason Wang, kvm, Michael S. Tsirkin, netdev,
linux-kernel
Cc: Stefano Garzarella, Longpeng, Zhu Lingshan, martinh, hanand,
Si-Wei Liu, dinang, Eli Cohen, lvivier, pabloc, gautam.dawar,
Xie Yongji, habetsm.xilinx, Christophe JAILLET, tanuj.kamde,
eperezma, Wu Zongyong, martinpo, lulu, ecree.xilinx,
Parav Pandit, Dan Carpenter, Zhang Min
The ioctl adds support for stop the device from userspace.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
drivers/vhost/vdpa.c | 18 ++++++++++++++++++
include/uapi/linux/vhost.h | 3 +++
2 files changed, 21 insertions(+)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index a325bc259afb..da4a8c709bc1 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -478,6 +478,21 @@ static long vhost_vdpa_get_vqs_count(struct vhost_vdpa *v, u32 __user *argp)
return 0;
}
+static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
+{
+ struct vdpa_device *vdpa = v->vdpa;
+ const struct vdpa_config_ops *ops = vdpa->config;
+ int stop;
+
+ if (!ops->stop)
+ return -EOPNOTSUPP;
+
+ if (copy_to_user(argp, &stop, sizeof(stop)))
+ return -EFAULT;
+
+ return ops->stop(vdpa, stop);
+}
+
static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
void __user *argp)
{
@@ -649,6 +664,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
case VHOST_VDPA_GET_VQS_COUNT:
r = vhost_vdpa_get_vqs_count(v, argp);
break;
+ case VHOST_STOP:
+ r = vhost_vdpa_stop(v, argp);
+ break;
default:
r = vhost_dev_ioctl(&v->vdev, cmd, argp);
if (r == -ENOIOCTLCMD)
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index cab645d4a645..e7526968ab0c 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -171,4 +171,7 @@
#define VHOST_VDPA_SET_GROUP_ASID _IOW(VHOST_VIRTIO, 0x7C, \
struct vhost_vring_state)
+/* Stop or resume a device so it does not process virtqueue requests anymore */
+#define VHOST_STOP _IOW(VHOST_VIRTIO, 0x7D, int)
+
#endif
--
2.27.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 3/4] vhost-vdpa: uAPI to stop the device
2022-05-20 17:23 ` [PATCH 3/4] vhost-vdpa: uAPI to stop the device Eugenio Pérez
@ 2022-05-21 5:20 ` kernel test robot
2022-05-21 8:36 ` Martin Habets
1 sibling, 0 replies; 30+ messages in thread
From: kernel test robot @ 2022-05-21 5:20 UTC (permalink / raw)
To: Eugenio Pérez, virtualization, Jason Wang, kvm,
Michael S. Tsirkin, netdev, linux-kernel
Cc: llvm, kbuild-all, Stefano Garzarella, Longpeng, Zhu Lingshan,
martinh, hanand, Si-Wei Liu, dinang, Eli Cohen, lvivier, pabloc,
gautam.dawar, Xie Yongji, habetsm.xilinx, Christophe JAILLET,
tanuj.kamde, eperezma, Wu Zongyong, martinpo, lulu, ecree.xilinx,
Parav Pandit, Dan Carpenter, Zhang Min
Hi "Eugenio,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mst-vhost/linux-next]
[also build test WARNING on next-20220520]
[cannot apply to horms-ipvs/master linus/master v5.18-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Eugenio-P-rez/Implement-vdpasim-stop-operation/20220521-012742
base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: hexagon-randconfig-r041-20220519 (https://download.01.org/0day-ci/archive/20220521/202205211317.rqYkIW8B-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project e00cbbec06c08dc616a0d52a20f678b8fbd4e304)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/bd6562e0d85e8d689d30226bcc0f43246e27e4b5
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Eugenio-P-rez/Implement-vdpasim-stop-operation/20220521-012742
git checkout bd6562e0d85e8d689d30226bcc0f43246e27e4b5
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/vhost/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/vhost/vdpa.c:493:25: warning: variable 'stop' is uninitialized when used here [-Wuninitialized]
return ops->stop(vdpa, stop);
^~~~
drivers/vhost/vdpa.c:485:10: note: initialize the variable 'stop' to silence this warning
int stop;
^
= 0
1 warning generated.
vim +/stop +493 drivers/vhost/vdpa.c
480
481 static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
482 {
483 struct vdpa_device *vdpa = v->vdpa;
484 const struct vdpa_config_ops *ops = vdpa->config;
485 int stop;
486
487 if (!ops->stop)
488 return -EOPNOTSUPP;
489
490 if (copy_to_user(argp, &stop, sizeof(stop)))
491 return -EFAULT;
492
> 493 return ops->stop(vdpa, stop);
494 }
495
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/4] vhost-vdpa: uAPI to stop the device
@ 2022-05-21 5:20 ` kernel test robot
0 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2022-05-21 5:20 UTC (permalink / raw)
To: Eugenio Pérez, virtualization, Jason Wang, kvm,
Michael S. Tsirkin, netdev, linux-kernel
Cc: tanuj.kamde, Dan Carpenter, llvm, Wu Zongyong, Si-Wei Liu,
pabloc, Eli Cohen, Zhang Min, lulu, Xie Yongji, martinh,
eperezma, dinang, habetsm.xilinx, Longpeng, lvivier,
Christophe JAILLET, kbuild-all, gautam.dawar, ecree.xilinx,
hanand, martinpo, Zhu Lingshan
Hi "Eugenio,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mst-vhost/linux-next]
[also build test WARNING on next-20220520]
[cannot apply to horms-ipvs/master linus/master v5.18-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Eugenio-P-rez/Implement-vdpasim-stop-operation/20220521-012742
base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: hexagon-randconfig-r041-20220519 (https://download.01.org/0day-ci/archive/20220521/202205211317.rqYkIW8B-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project e00cbbec06c08dc616a0d52a20f678b8fbd4e304)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/bd6562e0d85e8d689d30226bcc0f43246e27e4b5
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Eugenio-P-rez/Implement-vdpasim-stop-operation/20220521-012742
git checkout bd6562e0d85e8d689d30226bcc0f43246e27e4b5
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/vhost/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/vhost/vdpa.c:493:25: warning: variable 'stop' is uninitialized when used here [-Wuninitialized]
return ops->stop(vdpa, stop);
^~~~
drivers/vhost/vdpa.c:485:10: note: initialize the variable 'stop' to silence this warning
int stop;
^
= 0
1 warning generated.
vim +/stop +493 drivers/vhost/vdpa.c
480
481 static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
482 {
483 struct vdpa_device *vdpa = v->vdpa;
484 const struct vdpa_config_ops *ops = vdpa->config;
485 int stop;
486
487 if (!ops->stop)
488 return -EOPNOTSUPP;
489
490 if (copy_to_user(argp, &stop, sizeof(stop)))
491 return -EFAULT;
492
> 493 return ops->stop(vdpa, stop);
494 }
495
--
0-DAY CI Kernel Test Service
https://01.org/lkp
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] vhost-vdpa: uAPI to stop the device
2022-05-20 17:23 ` [PATCH 3/4] vhost-vdpa: uAPI to stop the device Eugenio Pérez
2022-05-21 5:20 ` kernel test robot
@ 2022-05-21 8:36 ` Martin Habets
2022-05-23 8:11 ` Eugenio Perez Martin
1 sibling, 1 reply; 30+ messages in thread
From: Martin Habets @ 2022-05-21 8:36 UTC (permalink / raw)
To: Eugenio Pérez
Cc: virtualization, Jason Wang, kvm, Michael S. Tsirkin, netdev,
linux-kernel, Stefano Garzarella, Longpeng, Zhu Lingshan,
martinh, hanand, Si-Wei Liu, dinang, Eli Cohen, lvivier, pabloc,
gautam.dawar, Xie Yongji, Christophe JAILLET, tanuj.kamde,
Wu Zongyong, martinpo, lulu, ecree.xilinx, Parav Pandit,
Dan Carpenter, Zhang Min
On Fri, May 20, 2022 at 07:23:24PM +0200, Eugenio Pérez wrote:
> The ioctl adds support for stop the device from userspace.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
> drivers/vhost/vdpa.c | 18 ++++++++++++++++++
> include/uapi/linux/vhost.h | 3 +++
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index a325bc259afb..da4a8c709bc1 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -478,6 +478,21 @@ static long vhost_vdpa_get_vqs_count(struct vhost_vdpa *v, u32 __user *argp)
> return 0;
> }
>
> +static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
> +{
> + struct vdpa_device *vdpa = v->vdpa;
> + const struct vdpa_config_ops *ops = vdpa->config;
> + int stop;
> +
> + if (!ops->stop)
> + return -EOPNOTSUPP;
> +
> + if (copy_to_user(argp, &stop, sizeof(stop)))
You want to use copy_from_user() here.
Martin
> + return -EFAULT;
> +
> + return ops->stop(vdpa, stop);
> +}
> +
> static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
> void __user *argp)
> {
> @@ -649,6 +664,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> case VHOST_VDPA_GET_VQS_COUNT:
> r = vhost_vdpa_get_vqs_count(v, argp);
> break;
> + case VHOST_STOP:
> + r = vhost_vdpa_stop(v, argp);
> + break;
> default:
> r = vhost_dev_ioctl(&v->vdev, cmd, argp);
> if (r == -ENOIOCTLCMD)
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index cab645d4a645..e7526968ab0c 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -171,4 +171,7 @@
> #define VHOST_VDPA_SET_GROUP_ASID _IOW(VHOST_VIRTIO, 0x7C, \
> struct vhost_vring_state)
>
> +/* Stop or resume a device so it does not process virtqueue requests anymore */
> +#define VHOST_STOP _IOW(VHOST_VIRTIO, 0x7D, int)
> +
> #endif
> --
> 2.27.0
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/4] vhost-vdpa: uAPI to stop the device
2022-05-21 8:36 ` Martin Habets
@ 2022-05-23 8:11 ` Eugenio Perez Martin
0 siblings, 0 replies; 30+ messages in thread
From: Eugenio Perez Martin @ 2022-05-23 8:11 UTC (permalink / raw)
To: Eugenio Pérez, virtualization, Jason Wang, kvm list,
Michael S. Tsirkin, netdev, linux-kernel, Stefano Garzarella,
Longpeng, Zhu Lingshan, Martin Petrus Hubertus Habets,
Harpreet Singh Anand, Si-Wei Liu, dinang, Eli Cohen,
Laurent Vivier, pabloc, Dawar, Gautam, Xie Yongji,
Christophe JAILLET, tanuj.kamde, Wu Zongyong, martinpo, Cindy Lu,
ecree.xilinx, Parav Pandit, Dan Carpenter, Zhang Min
On Sat, May 21, 2022 at 10:36 AM Martin Habets <habetsm.xilinx@gmail.com> wrote:
>
> On Fri, May 20, 2022 at 07:23:24PM +0200, Eugenio Pérez wrote:
> > The ioctl adds support for stop the device from userspace.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> > drivers/vhost/vdpa.c | 18 ++++++++++++++++++
> > include/uapi/linux/vhost.h | 3 +++
> > 2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index a325bc259afb..da4a8c709bc1 100644
> > --- a/drivers/vhost/vdpa.c
> > +++ b/drivers/vhost/vdpa.c
> > @@ -478,6 +478,21 @@ static long vhost_vdpa_get_vqs_count(struct vhost_vdpa *v, u32 __user *argp)
> > return 0;
> > }
> >
> > +static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
> > +{
> > + struct vdpa_device *vdpa = v->vdpa;
> > + const struct vdpa_config_ops *ops = vdpa->config;
> > + int stop;
> > +
> > + if (!ops->stop)
> > + return -EOPNOTSUPP;
> > +
> > + if (copy_to_user(argp, &stop, sizeof(stop)))
>
> You want to use copy_from_user() here.
>
Hi Martin,
You're right, I'll resend a new version with this fixed. Thanks!
> Martin
>
> > + return -EFAULT;
> > +
> > + return ops->stop(vdpa, stop);
> > +}
> > +
> > static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
> > void __user *argp)
> > {
> > @@ -649,6 +664,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> > case VHOST_VDPA_GET_VQS_COUNT:
> > r = vhost_vdpa_get_vqs_count(v, argp);
> > break;
> > + case VHOST_STOP:
> > + r = vhost_vdpa_stop(v, argp);
> > + break;
> > default:
> > r = vhost_dev_ioctl(&v->vdev, cmd, argp);
> > if (r == -ENOIOCTLCMD)
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index cab645d4a645..e7526968ab0c 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -171,4 +171,7 @@
> > #define VHOST_VDPA_SET_GROUP_ASID _IOW(VHOST_VIRTIO, 0x7C, \
> > struct vhost_vring_state)
> >
> > +/* Stop or resume a device so it does not process virtqueue requests anymore */
> > +#define VHOST_STOP _IOW(VHOST_VIRTIO, 0x7D, int)
> > +
> > #endif
> > --
> > 2.27.0
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 4/4] vdpa_sim: Implement stop vdpa op
2022-05-20 17:23 [PATCH 0/4] Implement vdpasim stop operation Eugenio Pérez
` (2 preceding siblings ...)
2022-05-20 17:23 ` [PATCH 3/4] vhost-vdpa: uAPI to stop the device Eugenio Pérez
@ 2022-05-20 17:23 ` Eugenio Pérez
2022-05-23 8:27 ` Stefano Garzarella
3 siblings, 1 reply; 30+ messages in thread
From: Eugenio Pérez @ 2022-05-20 17:23 UTC (permalink / raw)
To: virtualization, Jason Wang, kvm, Michael S. Tsirkin, netdev,
linux-kernel
Cc: Stefano Garzarella, Longpeng, Zhu Lingshan, martinh, hanand,
Si-Wei Liu, dinang, Eli Cohen, lvivier, pabloc, gautam.dawar,
Xie Yongji, habetsm.xilinx, Christophe JAILLET, tanuj.kamde,
eperezma, Wu Zongyong, martinpo, lulu, ecree.xilinx,
Parav Pandit, Dan Carpenter, Zhang Min
Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
that backend feature and userspace can effectively stop the device.
This is a must before get virtqueue indexes (base) for live migration,
since the device could modify them after userland gets them. There are
individual ways to perform that action for some devices
(VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
way to perform it for any vhost device (and, in particular, vhost-vdpa).
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +++++++++++++++++++++
drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 +
drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 +++
3 files changed, 25 insertions(+)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 50d721072beb..0515cf314bed 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -107,6 +107,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
for (i = 0; i < vdpasim->dev_attr.nas; i++)
vhost_iotlb_reset(&vdpasim->iommu[i]);
+ vdpasim->running = true;
spin_unlock(&vdpasim->iommu_lock);
vdpasim->features = 0;
@@ -505,6 +506,24 @@ static int vdpasim_reset(struct vdpa_device *vdpa)
return 0;
}
+static int vdpasim_stop(struct vdpa_device *vdpa, bool stop)
+{
+ struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+ int i;
+
+ spin_lock(&vdpasim->lock);
+ vdpasim->running = !stop;
+ if (vdpasim->running) {
+ /* Check for missed buffers */
+ for (i = 0; i < vdpasim->dev_attr.nvqs; ++i)
+ vdpasim_kick_vq(vdpa, i);
+
+ }
+ spin_unlock(&vdpasim->lock);
+
+ return 0;
+}
+
static size_t vdpasim_get_config_size(struct vdpa_device *vdpa)
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
@@ -694,6 +713,7 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
.get_status = vdpasim_get_status,
.set_status = vdpasim_set_status,
.reset = vdpasim_reset,
+ .stop = vdpasim_stop,
.get_config_size = vdpasim_get_config_size,
.get_config = vdpasim_get_config,
.set_config = vdpasim_set_config,
@@ -726,6 +746,7 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
.get_status = vdpasim_get_status,
.set_status = vdpasim_set_status,
.reset = vdpasim_reset,
+ .stop = vdpasim_stop,
.get_config_size = vdpasim_get_config_size,
.get_config = vdpasim_get_config,
.set_config = vdpasim_set_config,
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
index 622782e92239..061986f30911 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
@@ -66,6 +66,7 @@ struct vdpasim {
u32 generation;
u64 features;
u32 groups;
+ bool running;
/* spinlock to synchronize iommu table */
spinlock_t iommu_lock;
};
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
index 5125976a4df8..886449e88502 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
@@ -154,6 +154,9 @@ static void vdpasim_net_work(struct work_struct *work)
spin_lock(&vdpasim->lock);
+ if (!vdpasim->running)
+ goto out;
+
if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
goto out;
--
2.27.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 4/4] vdpa_sim: Implement stop vdpa op
2022-05-20 17:23 ` [PATCH 4/4] vdpa_sim: Implement stop vdpa op Eugenio Pérez
@ 2022-05-23 8:27 ` Stefano Garzarella
0 siblings, 0 replies; 30+ messages in thread
From: Stefano Garzarella @ 2022-05-23 8:27 UTC (permalink / raw)
To: Eugenio Pérez
Cc: virtualization, Jason Wang, kvm, Michael S. Tsirkin, netdev,
linux-kernel, Longpeng, Zhu Lingshan, martinh, hanand,
Si-Wei Liu, dinang, Eli Cohen, lvivier, pabloc, gautam.dawar,
Xie Yongji, habetsm.xilinx, Christophe JAILLET, tanuj.kamde,
Wu Zongyong, martinpo, lulu, ecree.xilinx, Parav Pandit,
Dan Carpenter, Zhang Min
On Fri, May 20, 2022 at 07:23:25PM +0200, Eugenio Pérez wrote:
>Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
>that backend feature and userspace can effectively stop the device.
>
>This is a must before get virtqueue indexes (base) for live migration,
>since the device could modify them after userland gets them. There are
>individual ways to perform that action for some devices
>(VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
>way to perform it for any vhost device (and, in particular, vhost-vdpa).
>
>Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>---
> drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +++++++++++++++++++++
> drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 +
> drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 +++
> 3 files changed, 25 insertions(+)
>
>diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>index 50d721072beb..0515cf314bed 100644
>--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
>+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>@@ -107,6 +107,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
> for (i = 0; i < vdpasim->dev_attr.nas; i++)
> vhost_iotlb_reset(&vdpasim->iommu[i]);
>
>+ vdpasim->running = true;
> spin_unlock(&vdpasim->iommu_lock);
>
> vdpasim->features = 0;
>@@ -505,6 +506,24 @@ static int vdpasim_reset(struct vdpa_device *vdpa)
> return 0;
> }
>
>+static int vdpasim_stop(struct vdpa_device *vdpa, bool stop)
>+{
>+ struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>+ int i;
>+
>+ spin_lock(&vdpasim->lock);
>+ vdpasim->running = !stop;
>+ if (vdpasim->running) {
>+ /* Check for missed buffers */
>+ for (i = 0; i < vdpasim->dev_attr.nvqs; ++i)
>+ vdpasim_kick_vq(vdpa, i);
>+
>+ }
>+ spin_unlock(&vdpasim->lock);
>+
>+ return 0;
>+}
>+
> static size_t vdpasim_get_config_size(struct vdpa_device *vdpa)
> {
> struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>@@ -694,6 +713,7 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
> .get_status = vdpasim_get_status,
> .set_status = vdpasim_set_status,
> .reset = vdpasim_reset,
>+ .stop = vdpasim_stop,
> .get_config_size = vdpasim_get_config_size,
> .get_config = vdpasim_get_config,
> .set_config = vdpasim_set_config,
>@@ -726,6 +746,7 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
> .get_status = vdpasim_get_status,
> .set_status = vdpasim_set_status,
> .reset = vdpasim_reset,
>+ .stop = vdpasim_stop,
> .get_config_size = vdpasim_get_config_size,
> .get_config = vdpasim_get_config,
> .set_config = vdpasim_set_config,
>diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
>index 622782e92239..061986f30911 100644
>--- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
>+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
>@@ -66,6 +66,7 @@ struct vdpasim {
> u32 generation;
> u64 features;
> u32 groups;
>+ bool running;
> /* spinlock to synchronize iommu table */
> spinlock_t iommu_lock;
> };
>diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
>index 5125976a4df8..886449e88502 100644
>--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
>+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
>@@ -154,6 +154,9 @@ static void vdpasim_net_work(struct work_struct *work)
>
> spin_lock(&vdpasim->lock);
>
>+ if (!vdpasim->running)
>+ goto out;
>+
It would be nice to do the same for vdpa_sim_blk as well.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 4/4] vdpa_sim: Implement stop vdpa op
@ 2022-05-23 8:27 ` Stefano Garzarella
0 siblings, 0 replies; 30+ messages in thread
From: Stefano Garzarella @ 2022-05-23 8:27 UTC (permalink / raw)
To: Eugenio Pérez
Cc: tanuj.kamde, kvm, Michael S. Tsirkin, virtualization,
Wu Zongyong, Si-Wei Liu, pabloc, Eli Cohen, Zhang Min, lulu,
martinh, Xie Yongji, dinang, habetsm.xilinx, Longpeng,
Dan Carpenter, lvivier, Christophe JAILLET, netdev, linux-kernel,
ecree.xilinx, hanand, martinpo, gautam.dawar, Zhu Lingshan
On Fri, May 20, 2022 at 07:23:25PM +0200, Eugenio Pérez wrote:
>Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
>that backend feature and userspace can effectively stop the device.
>
>This is a must before get virtqueue indexes (base) for live migration,
>since the device could modify them after userland gets them. There are
>individual ways to perform that action for some devices
>(VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
>way to perform it for any vhost device (and, in particular, vhost-vdpa).
>
>Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>---
> drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +++++++++++++++++++++
> drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 +
> drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 +++
> 3 files changed, 25 insertions(+)
>
>diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>index 50d721072beb..0515cf314bed 100644
>--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
>+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>@@ -107,6 +107,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
> for (i = 0; i < vdpasim->dev_attr.nas; i++)
> vhost_iotlb_reset(&vdpasim->iommu[i]);
>
>+ vdpasim->running = true;
> spin_unlock(&vdpasim->iommu_lock);
>
> vdpasim->features = 0;
>@@ -505,6 +506,24 @@ static int vdpasim_reset(struct vdpa_device *vdpa)
> return 0;
> }
>
>+static int vdpasim_stop(struct vdpa_device *vdpa, bool stop)
>+{
>+ struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>+ int i;
>+
>+ spin_lock(&vdpasim->lock);
>+ vdpasim->running = !stop;
>+ if (vdpasim->running) {
>+ /* Check for missed buffers */
>+ for (i = 0; i < vdpasim->dev_attr.nvqs; ++i)
>+ vdpasim_kick_vq(vdpa, i);
>+
>+ }
>+ spin_unlock(&vdpasim->lock);
>+
>+ return 0;
>+}
>+
> static size_t vdpasim_get_config_size(struct vdpa_device *vdpa)
> {
> struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>@@ -694,6 +713,7 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
> .get_status = vdpasim_get_status,
> .set_status = vdpasim_set_status,
> .reset = vdpasim_reset,
>+ .stop = vdpasim_stop,
> .get_config_size = vdpasim_get_config_size,
> .get_config = vdpasim_get_config,
> .set_config = vdpasim_set_config,
>@@ -726,6 +746,7 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
> .get_status = vdpasim_get_status,
> .set_status = vdpasim_set_status,
> .reset = vdpasim_reset,
>+ .stop = vdpasim_stop,
> .get_config_size = vdpasim_get_config_size,
> .get_config = vdpasim_get_config,
> .set_config = vdpasim_set_config,
>diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
>index 622782e92239..061986f30911 100644
>--- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
>+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
>@@ -66,6 +66,7 @@ struct vdpasim {
> u32 generation;
> u64 features;
> u32 groups;
>+ bool running;
> /* spinlock to synchronize iommu table */
> spinlock_t iommu_lock;
> };
>diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
>index 5125976a4df8..886449e88502 100644
>--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
>+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
>@@ -154,6 +154,9 @@ static void vdpasim_net_work(struct work_struct *work)
>
> spin_lock(&vdpasim->lock);
>
>+ if (!vdpasim->running)
>+ goto out;
>+
It would be nice to do the same for vdpa_sim_blk as well.
Thanks,
Stefano
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 4/4] vdpa_sim: Implement stop vdpa op
2022-05-23 8:27 ` Stefano Garzarella
(?)
@ 2022-05-23 19:07 ` Eugenio Perez Martin
-1 siblings, 0 replies; 30+ messages in thread
From: Eugenio Perez Martin @ 2022-05-23 19:07 UTC (permalink / raw)
To: Stefano Garzarella
Cc: virtualization, Jason Wang, kvm list, Michael S. Tsirkin, netdev,
linux-kernel, Longpeng, Zhu Lingshan,
Martin Petrus Hubertus Habets, Harpreet Singh Anand, Si-Wei Liu,
dinang, Eli Cohen, Laurent Vivier, pabloc, Dawar, Gautam,
Xie Yongji, habetsm.xilinx, Christophe JAILLET, tanuj.kamde,
Wu Zongyong, martinpo, Cindy Lu, ecree.xilinx, Parav Pandit,
Dan Carpenter, Zhang Min
On Mon, May 23, 2022 at 10:28 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, May 20, 2022 at 07:23:25PM +0200, Eugenio Pérez wrote:
> >Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> >that backend feature and userspace can effectively stop the device.
> >
> >This is a must before get virtqueue indexes (base) for live migration,
> >since the device could modify them after userland gets them. There are
> >individual ways to perform that action for some devices
> >(VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
> >way to perform it for any vhost device (and, in particular, vhost-vdpa).
> >
> >Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >---
> > drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +++++++++++++++++++++
> > drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 +
> > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 +++
> > 3 files changed, 25 insertions(+)
> >
> >diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >index 50d721072beb..0515cf314bed 100644
> >--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >@@ -107,6 +107,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
> > for (i = 0; i < vdpasim->dev_attr.nas; i++)
> > vhost_iotlb_reset(&vdpasim->iommu[i]);
> >
> >+ vdpasim->running = true;
> > spin_unlock(&vdpasim->iommu_lock);
> >
> > vdpasim->features = 0;
> >@@ -505,6 +506,24 @@ static int vdpasim_reset(struct vdpa_device *vdpa)
> > return 0;
> > }
> >
> >+static int vdpasim_stop(struct vdpa_device *vdpa, bool stop)
> >+{
> >+ struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> >+ int i;
> >+
> >+ spin_lock(&vdpasim->lock);
> >+ vdpasim->running = !stop;
> >+ if (vdpasim->running) {
> >+ /* Check for missed buffers */
> >+ for (i = 0; i < vdpasim->dev_attr.nvqs; ++i)
> >+ vdpasim_kick_vq(vdpa, i);
> >+
> >+ }
> >+ spin_unlock(&vdpasim->lock);
> >+
> >+ return 0;
> >+}
> >+
> > static size_t vdpasim_get_config_size(struct vdpa_device *vdpa)
> > {
> > struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> >@@ -694,6 +713,7 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
> > .get_status = vdpasim_get_status,
> > .set_status = vdpasim_set_status,
> > .reset = vdpasim_reset,
> >+ .stop = vdpasim_stop,
> > .get_config_size = vdpasim_get_config_size,
> > .get_config = vdpasim_get_config,
> > .set_config = vdpasim_set_config,
> >@@ -726,6 +746,7 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
> > .get_status = vdpasim_get_status,
> > .set_status = vdpasim_set_status,
> > .reset = vdpasim_reset,
> >+ .stop = vdpasim_stop,
> > .get_config_size = vdpasim_get_config_size,
> > .get_config = vdpasim_get_config,
> > .set_config = vdpasim_set_config,
> >diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> >index 622782e92239..061986f30911 100644
> >--- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
> >+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> >@@ -66,6 +66,7 @@ struct vdpasim {
> > u32 generation;
> > u64 features;
> > u32 groups;
> >+ bool running;
> > /* spinlock to synchronize iommu table */
> > spinlock_t iommu_lock;
> > };
> >diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> >index 5125976a4df8..886449e88502 100644
> >--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> >+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> >@@ -154,6 +154,9 @@ static void vdpasim_net_work(struct work_struct *work)
> >
> > spin_lock(&vdpasim->lock);
> >
> >+ if (!vdpasim->running)
> >+ goto out;
> >+
>
> It would be nice to do the same for vdpa_sim_blk as well.
>
Agree, it will be added in the next revision. If not, blk presents an
invalid backend feature bit.
Thanks!
> Thanks,
> Stefano
>
^ permalink raw reply [flat|nested] 30+ messages in thread