diff mbox series

[V2,4/4] virtio/vdpa: clear the virtqueue state during probe

Message ID 20210602021043.39201-5-jasowang@redhat.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Jason Wang June 2, 2021, 2:10 a.m. UTC 
From: Eli Cohen <elic@nvidia.com>

Clear the available index as part of the initialization process to
clear and values that might be left from previous usage of the device.
For example, if the device was previously used by vhost_vdpa and now
probed by vhost_vdpa, you want to start with indices.

Fixes: c043b4a8cf3b ("virtio: introduce a vDPA based transport")
Signed-off-by: Eli Cohen <elic@nvidia.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_vdpa.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Eli Cohen June 2, 2021, 6:17 a.m. UTC | #1 
On Wed, Jun 02, 2021 at 10:10:43AM +0800, Jason Wang wrote:
> From: Eli Cohen <elic@nvidia.com>
> 
> Clear the available index as part of the initialization process to
> clear and values that might be left from previous usage of the device.
> For example, if the device was previously used by vhost_vdpa and now
> probed by vhost_vdpa, you want to start with indices.
> 
> Fixes: c043b4a8cf3b ("virtio: introduce a vDPA based transport")
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/virtio/virtio_vdpa.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> index e28acf482e0c..e1a141135992 100644
> --- a/drivers/virtio/virtio_vdpa.c
> +++ b/drivers/virtio/virtio_vdpa.c
> @@ -142,6 +142,8 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
>  	struct vdpa_callback cb;
>  	struct virtqueue *vq;
>  	u64 desc_addr, driver_addr, device_addr;
> +	/* Assume split virtqueue, switch to packed if necessary */
> +	struct vdpa_vq_state state = {0};
>  	unsigned long flags;
>  	u32 align, num;
>  	int err;
> @@ -191,6 +193,19 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
>  		goto err_vq;
>  	}
>  
> +	/* reset virtqueue state index */
> +	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> +		struct vdpa_vq_state_packed *s = &state.packed;
> +
> +		s->last_avail_counter = 1;
> +		s->last_avail_idx = 0;

It's already 0

> +		s->last_used_counter = 1;
> +		s->last_used_idx = 0;

already 0

> +	}
> +	err = ops->set_vq_state(vdpa, index, &state);
> +	if (err)
> +		goto err_vq;
> +
>  	ops->set_vq_ready(vdpa, index, 1);
>  
>  	vq->priv = info;
> -- 
> 2.25.1
>
Jason Wang June 2, 2021, 7:07 a.m. UTC | #2 
在 2021/6/2 下午2:17, Eli Cohen 写道:
> On Wed, Jun 02, 2021 at 10:10:43AM +0800, Jason Wang wrote:
>> From: Eli Cohen <elic@nvidia.com>
>>
>> Clear the available index as part of the initialization process to
>> clear and values that might be left from previous usage of the device.
>> For example, if the device was previously used by vhost_vdpa and now
>> probed by vhost_vdpa, you want to start with indices.
>>
>> Fixes: c043b4a8cf3b ("virtio: introduce a vDPA based transport")
>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   drivers/virtio/virtio_vdpa.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
>> index e28acf482e0c..e1a141135992 100644
>> --- a/drivers/virtio/virtio_vdpa.c
>> +++ b/drivers/virtio/virtio_vdpa.c
>> @@ -142,6 +142,8 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
>>   	struct vdpa_callback cb;
>>   	struct virtqueue *vq;
>>   	u64 desc_addr, driver_addr, device_addr;
>> +	/* Assume split virtqueue, switch to packed if necessary */
>> +	struct vdpa_vq_state state = {0};
>>   	unsigned long flags;
>>   	u32 align, num;
>>   	int err;
>> @@ -191,6 +193,19 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
>>   		goto err_vq;
>>   	}
>>   
>> +	/* reset virtqueue state index */
>> +	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
>> +		struct vdpa_vq_state_packed *s = &state.packed;
>> +
>> +		s->last_avail_counter = 1;
>> +		s->last_avail_idx = 0;
> It's already 0
>
>> +		s->last_used_counter = 1;
>> +		s->last_used_idx = 0;
> already 0


Yes, but for completeness and make code easy to read, it's no harm to 
keep them I think.

Thanks


>
>> +	}
>> +	err = ops->set_vq_state(vdpa, index, &state);
>> +	if (err)
>> +		goto err_vq;
>> +
>>   	ops->set_vq_ready(vdpa, index, 1);
>>   
>>   	vq->priv = info;
>> -- 
>> 2.25.1
>>
Eli Cohen June 2, 2021, 7:43 a.m. UTC | #3 
On Wed, Jun 02, 2021 at 03:07:00PM +0800, Jason Wang wrote:
> 
> 在 2021/6/2 下午2:17, Eli Cohen 写道:
> > On Wed, Jun 02, 2021 at 10:10:43AM +0800, Jason Wang wrote:
> > > From: Eli Cohen <elic@nvidia.com>
> > > 
> > > Clear the available index as part of the initialization process to
> > > clear and values that might be left from previous usage of the device.
> > > For example, if the device was previously used by vhost_vdpa and now
> > > probed by vhost_vdpa, you want to start with indices.
> > > 
> > > Fixes: c043b4a8cf3b ("virtio: introduce a vDPA based transport")
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >   drivers/virtio/virtio_vdpa.c | 15 +++++++++++++++
> > >   1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> > > index e28acf482e0c..e1a141135992 100644
> > > --- a/drivers/virtio/virtio_vdpa.c
> > > +++ b/drivers/virtio/virtio_vdpa.c
> > > @@ -142,6 +142,8 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
> > >   	struct vdpa_callback cb;
> > >   	struct virtqueue *vq;
> > >   	u64 desc_addr, driver_addr, device_addr;
> > > +	/* Assume split virtqueue, switch to packed if necessary */
> > > +	struct vdpa_vq_state state = {0};
> > >   	unsigned long flags;
> > >   	u32 align, num;
> > >   	int err;
> > > @@ -191,6 +193,19 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
> > >   		goto err_vq;
> > >   	}
> > > +	/* reset virtqueue state index */
> > > +	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> > > +		struct vdpa_vq_state_packed *s = &state.packed;
> > > +
> > > +		s->last_avail_counter = 1;
> > > +		s->last_avail_idx = 0;
> > It's already 0
> > 
> > > +		s->last_used_counter = 1;
> > > +		s->last_used_idx = 0;
> > already 0
> 
> 
> Yes, but for completeness and make code easy to read, it's no harm to keep
> them I think.
> 

OK.

> Thanks
> 
> 
> > 
> > > +	}
> > > +	err = ops->set_vq_state(vdpa, index, &state);
> > > +	if (err)
> > > +		goto err_vq;
> > > +
> > >   	ops->set_vq_ready(vdpa, index, 1);
> > >   	vq->priv = info;
> > > -- 
> > > 2.25.1
> > > 
>
diff mbox series

Patch

diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index e28acf482e0c..e1a141135992 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c
@@ -142,6 +142,8 @@  virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
 	struct vdpa_callback cb;
 	struct virtqueue *vq;
 	u64 desc_addr, driver_addr, device_addr;
+	/* Assume split virtqueue, switch to packed if necessary */
+	struct vdpa_vq_state state = {0};
 	unsigned long flags;
 	u32 align, num;
 	int err;
@@ -191,6 +193,19 @@  virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
 		goto err_vq;
 	}
 
+	/* reset virtqueue state index */
+	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
+		struct vdpa_vq_state_packed *s = &state.packed;
+
+		s->last_avail_counter = 1;
+		s->last_avail_idx = 0;
+		s->last_used_counter = 1;
+		s->last_used_idx = 0;
+	}
+	err = ops->set_vq_state(vdpa, index, &state);
+	if (err)
+		goto err_vq;
+
 	ops->set_vq_ready(vdpa, index, 1);
 
 	vq->priv = info;