All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 1/4] vdpa: Protect vdpa reset with cf_mutex
       [not found] <20220111072217.101836-1-elic@nvidia.com>
@ 2022-01-11  9:27 ` Si-Wei Liu
  2022-01-11 16:01 ` Michael S. Tsirkin
  1 sibling, 0 replies; 3+ messages in thread
From: Si-Wei Liu @ 2022-01-11  9:27 UTC (permalink / raw)
  To: Eli Cohen, mst, jasowang, virtualization; +Cc: lvivier, eperezma



On 1/10/2022 11:22 PM, Eli Cohen wrote:
> Call reset using the wrapper function vdpa_reset() to make sure the
> operation is serialized with cf_mutex.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   drivers/vhost/vdpa.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 6e7edaf2472b..fe0bbea4dab6 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -155,7 +155,6 @@ static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user *statusp)
>   static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
>   {
>   	struct vdpa_device *vdpa = v->vdpa;
> -	const struct vdpa_config_ops *ops = vdpa->config;
It is okay to remove this line from the patch, but a follow-up patch to 
remove the locking for get_status would need to reintroduce the same 
line. If possible please try post these two together in a same series.

Reviewed-by: Si-Wei Liu<si-wei.liu@oracle.com>

>   	u8 status, status_old;
>   	int ret, nvqs = v->nvqs;
>   	u16 i;
> @@ -177,7 +176,7 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
>   			vhost_vdpa_unsetup_vq_irq(v, i);
>   
>   	if (status == 0) {
> -		ret = ops->reset(vdpa);
> +		ret = vdpa_reset(vdpa);
>   		if (ret)
>   			return ret;
>   	} else

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH 1/4] vdpa: Protect vdpa reset with cf_mutex
       [not found] <20220111072217.101836-1-elic@nvidia.com>
  2022-01-11  9:27 ` [PATCH 1/4] vdpa: Protect vdpa reset with cf_mutex Si-Wei Liu
@ 2022-01-11 16:01 ` Michael S. Tsirkin
       [not found]   ` <20220111161238.GA168233@mtl-vdi-166.wap.labs.mlnx>
  1 sibling, 1 reply; 3+ messages in thread
From: Michael S. Tsirkin @ 2022-01-11 16:01 UTC (permalink / raw)
  To: Eli Cohen; +Cc: lvivier, virtualization, eperezma, si-wei.liu

On Tue, Jan 11, 2022 at 09:22:17AM +0200, Eli Cohen wrote:
> Call reset using the wrapper function vdpa_reset() to make sure the
> operation is serialized with cf_mutex.
> 
> Signed-off-by: Eli Cohen <elic@nvidia.com>

The motivation is vague here, this does not really explain.
Which operations are we trying to serialize?
Multiple reset requests from userspace?
Is anything broken right now without this patch?

> ---
>  drivers/vhost/vdpa.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 6e7edaf2472b..fe0bbea4dab6 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -155,7 +155,6 @@ static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user *statusp)
>  static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
>  {
>  	struct vdpa_device *vdpa = v->vdpa;
> -	const struct vdpa_config_ops *ops = vdpa->config;
>  	u8 status, status_old;
>  	int ret, nvqs = v->nvqs;
>  	u16 i;
> @@ -177,7 +176,7 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
>  			vhost_vdpa_unsetup_vq_irq(v, i);
>  
>  	if (status == 0) {
> -		ret = ops->reset(vdpa);
> +		ret = vdpa_reset(vdpa);
>  		if (ret)
>  			return ret;
>  	} else
> -- 
> 2.34.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH 1/4] vdpa: Protect vdpa reset with cf_mutex
       [not found]   ` <20220111161238.GA168233@mtl-vdi-166.wap.labs.mlnx>
@ 2022-01-11 16:36     ` Michael S. Tsirkin
  0 siblings, 0 replies; 3+ messages in thread
From: Michael S. Tsirkin @ 2022-01-11 16:36 UTC (permalink / raw)
  To: Eli Cohen; +Cc: lvivier, virtualization, eperezma, si-wei.liu

On Tue, Jan 11, 2022 at 06:12:38PM +0200, Eli Cohen wrote:
> On Tue, Jan 11, 2022 at 11:01:01AM -0500, Michael S. Tsirkin wrote:
> > On Tue, Jan 11, 2022 at 09:22:17AM +0200, Eli Cohen wrote:
> > > Call reset using the wrapper function vdpa_reset() to make sure the
> > > operation is serialized with cf_mutex.
> > > 
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > 
> > The motivation is vague here, this does not really explain.
> > Which operations are we trying to serialize?
> > Multiple reset requests from userspace?
> > Is anything broken right now without this patch?
> 
> vdpa_reset will reset the features which can be read or even be set
> through devlink (see vdpa_get_config_unlocked()). So this lock ensures
> serialization of the operations to ensure consitent value is read.

My point is that this is the kind of thing that needs to go into commit log.
A good log for a bugfix looks like this:


if XYZ triggers a vdpa reset while ABC is reading the features through
devlink, with the result that EFG.  Similarly for write which can lead
to HIJ.

Fix this by calling reset using the wrapper function vdpa_reset() to make sure the
operation is serialized with cf_mutex.

Fixes: <hash> ("subject")



> > 
> > > ---
> > >  drivers/vhost/vdpa.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > index 6e7edaf2472b..fe0bbea4dab6 100644
> > > --- a/drivers/vhost/vdpa.c
> > > +++ b/drivers/vhost/vdpa.c
> > > @@ -155,7 +155,6 @@ static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user *statusp)
> > >  static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
> > >  {
> > >  	struct vdpa_device *vdpa = v->vdpa;
> > > -	const struct vdpa_config_ops *ops = vdpa->config;
> > >  	u8 status, status_old;
> > >  	int ret, nvqs = v->nvqs;
> > >  	u16 i;
> > > @@ -177,7 +176,7 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
> > >  			vhost_vdpa_unsetup_vq_irq(v, i);
> > >  
> > >  	if (status == 0) {
> > > -		ret = ops->reset(vdpa);
> > > +		ret = vdpa_reset(vdpa);
> > >  		if (ret)
> > >  			return ret;
> > >  	} else
> > > -- 
> > > 2.34.1
> > 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-01-11 16:36 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20220111072217.101836-1-elic@nvidia.com>
2022-01-11  9:27 ` [PATCH 1/4] vdpa: Protect vdpa reset with cf_mutex Si-Wei Liu
2022-01-11 16:01 ` Michael S. Tsirkin
     [not found]   ` <20220111161238.GA168233@mtl-vdi-166.wap.labs.mlnx>
2022-01-11 16:36     ` Michael S. Tsirkin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.