date:20201201

Re: [External] Re: [PATCH 0/7] Introduce vdpa management tool

2020-12-01 Thread Jason Wang



On 2020/12/2 下午2:24, Parav Pandit wrote:



From: Jason Wang 
Sent: Wednesday, December 2, 2020 11:21 AM

On 2020/12/2 下午12:53, Parav Pandit wrote:

From: Yongji Xie 
Sent: Wednesday, December 2, 2020 9:00 AM

On Tue, Dec 1, 2020 at 11:59 PM Parav Pandit  wrote:



From: Yongji Xie 
Sent: Tuesday, December 1, 2020 7:49 PM

On Tue, Dec 1, 2020 at 7:32 PM Parav Pandit 

wrote:



From: Yongji Xie 
Sent: Tuesday, December 1, 2020 3:26 PM

On Tue, Dec 1, 2020 at 2:25 PM Jason Wang



wrote:

On 2020/11/30 下午3:07, Yongji Xie wrote:

Thanks for adding me, Jason!

Now I'm working on a v2 patchset for VDUSE (vDPA Device in
Userspace) [1]. This tool is very useful for the vduse device.
So I'm considering integrating this into my v2 patchset.
But there is one problem：

In this tool, vdpa device config action and enable action are
combined into one netlink msg: VDPA_CMD_DEV_NEW. But in

vduse

case, it needs to be splitted because a chardev should be
created and opened by a userspace process before we enable
the vdpa device (call vdpa_register_device()).

So I'd like to know whether it's possible (or have some
plans) to add two new netlink msgs something like:
VDPA_CMD_DEV_ENABLE

and

VDPA_CMD_DEV_DISABLE to make the config path more

flexible.

Actually, we've discussed such intermediate step in some early
discussion. It looks to me VDUSE could be one of the users of

this.

Or I wonder whether we can switch to use anonymous
inode(fd) for VDUSE then fetching it via an
VDUSE_GET_DEVICE_FD

ioctl?

Yes, we can. Actually the current implementation in VDUSE is
like this.  But seems like this is still a intermediate step.
The fd should be binded to a name or something else which need
to be configured before.

The name could be specified via the netlink. It looks to me the
real issue is that until the device is connected with a
userspace, it can't be used. So we also need to fail the
enabling if it doesn't

opened.

Yes, that's true. So you mean we can firstly try to fetch the fd
binded to a name/vduse_id via an VDUSE_GET_DEVICE_FD, then

use

the name/vduse_id as a attribute to create vdpa device? It looks
fine to

me.

I probably do not well understand. I tried reading patch [1] and
few things

do not look correct as below.

Creating the vdpa device on the bus device and destroying the
device from

the workqueue seems unnecessary and racy.

It seems vduse driver needs
This is something should be done as part of the vdpa dev add
command,

instead of connecting two sides separately and ensuring race free
access to it.

So VDUSE_DEV_START and VDUSE_DEV_STOP should possibly be

avoided.

Yes, we can avoid these two ioctls with the help of the management

tool.

$ vdpa dev add parentdev vduse_mgmtdev type net name foo2

When above command is executed it creates necessary vdpa device
foo2

on the bus.

When user binds foo2 device with the vduse driver, in the probe(),
it

creates respective char device to access it from user space.


I see. So vduse cannot work with any existing vdpa devices like ifc,
mlx5 or

netdevsim.

It has its own implementation similar to fuse with its own backend of

choice.

More below.


But vduse driver is not a vdpa bus driver. It works like vdpasim
driver, but offloads the data plane and control plane to a user space

process.

In that case to draw parallel lines,

1. netdevsim:
(a) create resources in kernel sw
(b) datapath simulates in kernel

2. ifc + mlx5 vdpa dev:
(a) creates resource in hw
(b) data path is in hw

3. vduse:
(a) creates resources in userspace sw
(b) data path is in user space.
hence creates data path resources for user space.
So char device is created, removed as result of vdpa device creation.

For example,
$ vdpa dev add parentdev vduse_mgmtdev type net name foo2

Above command will create char device for user space.

Similar command for ifc/mlx5 would have created similar channel for
rest of

the config commands in hw.

vduse channel = char device, eventfd etc.
ifc/mlx5 hw channel = bar, irq, command interface etc Netdev sim
channel = sw direct calls

Does it make sense?

In my understanding, to make vdpa work, we need a backend (datapath
resources) and a frontend (a vdpa device attached to a vdpa bus). In
the above example, it looks like we use the command "vdpa dev add ..."
   to create a backend, so do we need another command to create a

frontend?

For block device there is certainly some backend to process the IOs.
Sometimes backend to be setup first, before its front end is exposed.
"vdpa dev add" is the front end command who connects to the backend

(implicitly) for network device.

vhost->vdpa_block_device->backend_io_processor (usr,hw,kernel).

And it needs a way to connect to backend when explicitly specified during

creation time.

Something like,
$ vdpa dev add parentdev vdpa_vduse type block name foo3 handle



In above example some vendor device specific unique handle is passed

based on backend setup in hardware/user space.

In below 3 examples, vdpa

[PATCH] vhost_vdpa: return -EFAULT if copy_to_user() fails

2020-12-01 Thread Dan Carpenter

The copy_to_user() function returns the number of bytes remaining to be
copied but this should return -EFAULT to the user.

Fixes: 1b48dc03e575 ("vhost: vdpa: report iova range")
Signed-off-by: Dan Carpenter 
---
 drivers/vhost/vdpa.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index d6a37b66770b..ef688c8c0e0e 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -344,7 +344,9 @@ static long vhost_vdpa_get_iova_range(struct vhost_vdpa *v, 
u32 __user *argp)
.last = v->range.last,
};
 
-   return copy_to_user(argp, , sizeof(range));
+   if (copy_to_user(argp, , sizeof(range)))
+   return -EFAULT;
+   return 0;
 }
 
 static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
-- 
2.29.2

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

RE: [External] Re: [PATCH 0/7] Introduce vdpa management tool

2020-12-01 Thread Parav Pandit



> From: Jason Wang 
> Sent: Wednesday, December 2, 2020 11:21 AM
> 
> On 2020/12/2 下午12:53, Parav Pandit wrote:
> >
> >> From: Yongji Xie 
> >> Sent: Wednesday, December 2, 2020 9:00 AM
> >>
> >> On Tue, Dec 1, 2020 at 11:59 PM Parav Pandit  wrote:
> >>>
> >>>
>  From: Yongji Xie 
>  Sent: Tuesday, December 1, 2020 7:49 PM
> 
>  On Tue, Dec 1, 2020 at 7:32 PM Parav Pandit 
> wrote:
> >
> >
> >> From: Yongji Xie 
> >> Sent: Tuesday, December 1, 2020 3:26 PM
> >>
> >> On Tue, Dec 1, 2020 at 2:25 PM Jason Wang
> 
>  wrote:
> >>>
> >>> On 2020/11/30 下午3:07, Yongji Xie wrote:
> >> Thanks for adding me, Jason!
> >>
> >> Now I'm working on a v2 patchset for VDUSE (vDPA Device in
> >> Userspace) [1]. This tool is very useful for the vduse device.
> >> So I'm considering integrating this into my v2 patchset.
> >> But there is one problem：
> >>
> >> In this tool, vdpa device config action and enable action are
> >> combined into one netlink msg: VDPA_CMD_DEV_NEW. But in
>  vduse
> >> case, it needs to be splitted because a chardev should be
> >> created and opened by a userspace process before we enable
> >> the vdpa device (call vdpa_register_device()).
> >>
> >> So I'd like to know whether it's possible (or have some
> >> plans) to add two new netlink msgs something like:
> >> VDPA_CMD_DEV_ENABLE
> >> and
> >> VDPA_CMD_DEV_DISABLE to make the config path more
> flexible.
> >>
> > Actually, we've discussed such intermediate step in some early
> > discussion. It looks to me VDUSE could be one of the users of
> >> this.
> > Or I wonder whether we can switch to use anonymous
> > inode(fd) for VDUSE then fetching it via an
> > VDUSE_GET_DEVICE_FD
> >> ioctl?
>  Yes, we can. Actually the current implementation in VDUSE is
>  like this.  But seems like this is still a intermediate step.
>  The fd should be binded to a name or something else which need
>  to be configured before.
> >>>
> >>> The name could be specified via the netlink. It looks to me the
> >>> real issue is that until the device is connected with a
> >>> userspace, it can't be used. So we also need to fail the
> >>> enabling if it doesn't
>  opened.
> >> Yes, that's true. So you mean we can firstly try to fetch the fd
> >> binded to a name/vduse_id via an VDUSE_GET_DEVICE_FD, then
> use
> >> the name/vduse_id as a attribute to create vdpa device? It looks
> >> fine to
> >> me.
> > I probably do not well understand. I tried reading patch [1] and
> > few things
>  do not look correct as below.
> > Creating the vdpa device on the bus device and destroying the
> > device from
>  the workqueue seems unnecessary and racy.
> > It seems vduse driver needs
> > This is something should be done as part of the vdpa dev add
> > command,
>  instead of connecting two sides separately and ensuring race free
>  access to it.
> > So VDUSE_DEV_START and VDUSE_DEV_STOP should possibly be
> avoided.
> >
>  Yes, we can avoid these two ioctls with the help of the management
> tool.
> 
> > $ vdpa dev add parentdev vduse_mgmtdev type net name foo2
> >
> > When above command is executed it creates necessary vdpa device
> > foo2
>  on the bus.
> > When user binds foo2 device with the vduse driver, in the probe(),
> > it
>  creates respective char device to access it from user space.
> 
> >>> I see. So vduse cannot work with any existing vdpa devices like ifc,
> >>> mlx5 or
> >> netdevsim.
> >>> It has its own implementation similar to fuse with its own backend of
> choice.
> >>> More below.
> >>>
>  But vduse driver is not a vdpa bus driver. It works like vdpasim
>  driver, but offloads the data plane and control plane to a user space
> process.
> >>> In that case to draw parallel lines,
> >>>
> >>> 1. netdevsim:
> >>> (a) create resources in kernel sw
> >>> (b) datapath simulates in kernel
> >>>
> >>> 2. ifc + mlx5 vdpa dev:
> >>> (a) creates resource in hw
> >>> (b) data path is in hw
> >>>
> >>> 3. vduse:
> >>> (a) creates resources in userspace sw
> >>> (b) data path is in user space.
> >>> hence creates data path resources for user space.
> >>> So char device is created, removed as result of vdpa device creation.
> >>>
> >>> For example,
> >>> $ vdpa dev add parentdev vduse_mgmtdev type net name foo2
> >>>
> >>> Above command will create char device for user space.
> >>>
> >>> Similar command for ifc/mlx5 would have created similar channel for
> >>> rest of
> >> the config commands in hw.
> >>> vduse channel = char device, eventfd etc.
> >>> ifc/mlx5 hw channel = bar, irq, command interface etc Netdev sim
> >>> channel = sw direct calls
> >>>
> >>> Does it make

Re: [External] Re: [PATCH 0/7] Introduce vdpa management tool

2020-12-01 Thread Jason Wang



On 2020/12/2 下午12:53, Parav Pandit wrote:



From: Yongji Xie 
Sent: Wednesday, December 2, 2020 9:00 AM

On Tue, Dec 1, 2020 at 11:59 PM Parav Pandit  wrote:




From: Yongji Xie 
Sent: Tuesday, December 1, 2020 7:49 PM

On Tue, Dec 1, 2020 at 7:32 PM Parav Pandit  wrote:




From: Yongji Xie 
Sent: Tuesday, December 1, 2020 3:26 PM

On Tue, Dec 1, 2020 at 2:25 PM Jason Wang 

wrote:


On 2020/11/30 下午3:07, Yongji Xie wrote:

Thanks for adding me, Jason!

Now I'm working on a v2 patchset for VDUSE (vDPA Device in
Userspace) [1]. This tool is very useful for the vduse device.
So I'm considering integrating this into my v2 patchset.
But there is one problem：

In this tool, vdpa device config action and enable action
are combined into one netlink msg: VDPA_CMD_DEV_NEW. But
in

vduse

case, it needs to be splitted because a chardev should be
created and opened by a userspace process before we enable
the vdpa device (call vdpa_register_device()).

So I'd like to know whether it's possible (or have some
plans) to add two new netlink msgs something like:
VDPA_CMD_DEV_ENABLE

and

VDPA_CMD_DEV_DISABLE to make the config path more flexible.


Actually, we've discussed such intermediate step in some
early discussion. It looks to me VDUSE could be one of the users of

this.

Or I wonder whether we can switch to use anonymous
inode(fd) for VDUSE then fetching it via an VDUSE_GET_DEVICE_FD

ioctl?

Yes, we can. Actually the current implementation in VDUSE is
like this.  But seems like this is still a intermediate step.
The fd should be binded to a name or something else which
need to be configured before.


The name could be specified via the netlink. It looks to me
the real issue is that until the device is connected with a
userspace, it can't be used. So we also need to fail the
enabling if it doesn't

opened.

Yes, that's true. So you mean we can firstly try to fetch the fd
binded to a name/vduse_id via an VDUSE_GET_DEVICE_FD, then use
the name/vduse_id as a attribute to create vdpa device? It looks fine to

me.

I probably do not well understand. I tried reading patch [1] and
few things

do not look correct as below.

Creating the vdpa device on the bus device and destroying the
device from

the workqueue seems unnecessary and racy.

It seems vduse driver needs
This is something should be done as part of the vdpa dev add
command,

instead of connecting two sides separately and ensuring race free
access to it.

So VDUSE_DEV_START and VDUSE_DEV_STOP should possibly be avoided.


Yes, we can avoid these two ioctls with the help of the management tool.


$ vdpa dev add parentdev vduse_mgmtdev type net name foo2

When above command is executed it creates necessary vdpa device
foo2

on the bus.

When user binds foo2 device with the vduse driver, in the probe(),
it

creates respective char device to access it from user space.


I see. So vduse cannot work with any existing vdpa devices like ifc, mlx5 or

netdevsim.

It has its own implementation similar to fuse with its own backend of choice.
More below.


But vduse driver is not a vdpa bus driver. It works like vdpasim
driver, but offloads the data plane and control plane to a user space process.

In that case to draw parallel lines,

1. netdevsim:
(a) create resources in kernel sw
(b) datapath simulates in kernel

2. ifc + mlx5 vdpa dev:
(a) creates resource in hw
(b) data path is in hw

3. vduse:
(a) creates resources in userspace sw
(b) data path is in user space.
hence creates data path resources for user space.
So char device is created, removed as result of vdpa device creation.

For example,
$ vdpa dev add parentdev vduse_mgmtdev type net name foo2

Above command will create char device for user space.

Similar command for ifc/mlx5 would have created similar channel for rest of

the config commands in hw.

vduse channel = char device, eventfd etc.
ifc/mlx5 hw channel = bar, irq, command interface etc Netdev sim
channel = sw direct calls

Does it make sense?

In my understanding, to make vdpa work, we need a backend (datapath
resources) and a frontend (a vdpa device attached to a vdpa bus). In the above
example, it looks like we use the command "vdpa dev add ..."
  to create a backend, so do we need another command to create a frontend?


For block device there is certainly some backend to process the IOs.
Sometimes backend to be setup first, before its front end is exposed.
"vdpa dev add" is the front end command who connects to the backend 
(implicitly) for network device.

vhost->vdpa_block_device->backend_io_processor (usr,hw,kernel).

And it needs a way to connect to backend when explicitly specified during 
creation time.
Something like,
$ vdpa dev add parentdev vdpa_vduse type block name foo3 handle 
In above example some vendor device specific unique handle is passed based on 
backend setup in hardware/user space.

In below 3 examples, vdpa block simulator is connecting to backend block or 
file.

$ vdpa dev add parentdev vdpa_blocksim type block name

Re: [External] Re: [PATCH 0/7] Introduce vdpa management tool

2020-12-01 Thread Jason Wang



On 2020/12/1 下午5:55, Yongji Xie wrote:

On Tue, Dec 1, 2020 at 2:25 PM Jason Wang  wrote:


On 2020/11/30 下午3:07, Yongji Xie wrote:

Thanks for adding me, Jason!

Now I'm working on a v2 patchset for VDUSE (vDPA Device in Userspace)
[1]. This tool is very useful for the vduse device. So I'm considering
integrating this into my v2 patchset. But there is one problem：

In this tool, vdpa device config action and enable action are combined
into one netlink msg: VDPA_CMD_DEV_NEW. But in vduse case, it needs to
be splitted because a chardev should be created and opened by a
userspace process before we enable the vdpa device (call
vdpa_register_device()).

So I'd like to know whether it's possible (or have some plans) to add
two new netlink msgs something like: VDPA_CMD_DEV_ENABLE and
VDPA_CMD_DEV_DISABLE to make the config path more flexible.


Actually, we've discussed such intermediate step in some early
discussion. It looks to me VDUSE could be one of the users of this.

Or I wonder whether we can switch to use anonymous inode(fd) for VDUSE
then fetching it via an VDUSE_GET_DEVICE_FD ioctl?


Yes, we can. Actually the current implementation in VDUSE is like
this.  But seems like this is still a intermediate step. The fd should
be binded to a name or something else which need to be configured
before.


The name could be specified via the netlink. It looks to me the real
issue is that until the device is connected with a userspace, it can't
be used. So we also need to fail the enabling if it doesn't opened.


Yes, that's true. So you mean we can firstly try to fetch the fd
binded to a name/vduse_id via an VDUSE_GET_DEVICE_FD, then use the
name/vduse_id as a attribute to create vdpa device? It looks fine to
me.



Yes, something like this. The anonymous fd will be created during 
dev_add() and the fd will be carried in the msg to userspace.


Thanks




Thanks,
Yongji



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

RE: [External] Re: [PATCH 0/7] Introduce vdpa management tool

2020-12-01 Thread Parav Pandit



> From: Yongji Xie 
> Sent: Wednesday, December 2, 2020 9:00 AM
> 
> On Tue, Dec 1, 2020 at 11:59 PM Parav Pandit  wrote:
> >
> >
> >
> > > From: Yongji Xie 
> > > Sent: Tuesday, December 1, 2020 7:49 PM
> > >
> > > On Tue, Dec 1, 2020 at 7:32 PM Parav Pandit  wrote:
> > > >
> > > >
> > > >
> > > > > From: Yongji Xie 
> > > > > Sent: Tuesday, December 1, 2020 3:26 PM
> > > > >
> > > > > On Tue, Dec 1, 2020 at 2:25 PM Jason Wang 
> > > wrote:
> > > > > >
> > > > > >
> > > > > > On 2020/11/30 下午3:07, Yongji Xie wrote:
> > > > > > >>> Thanks for adding me, Jason!
> > > > > > >>>
> > > > > > >>> Now I'm working on a v2 patchset for VDUSE (vDPA Device in
> > > > > > >>> Userspace) [1]. This tool is very useful for the vduse device.
> > > > > > >>> So I'm considering integrating this into my v2 patchset.
> > > > > > >>> But there is one problem：
> > > > > > >>>
> > > > > > >>> In this tool, vdpa device config action and enable action
> > > > > > >>> are combined into one netlink msg: VDPA_CMD_DEV_NEW. But
> > > > > > >>> in
> > > vduse
> > > > > > >>> case, it needs to be splitted because a chardev should be
> > > > > > >>> created and opened by a userspace process before we enable
> > > > > > >>> the vdpa device (call vdpa_register_device()).
> > > > > > >>>
> > > > > > >>> So I'd like to know whether it's possible (or have some
> > > > > > >>> plans) to add two new netlink msgs something like:
> > > > > > >>> VDPA_CMD_DEV_ENABLE
> > > > > and
> > > > > > >>> VDPA_CMD_DEV_DISABLE to make the config path more flexible.
> > > > > > >>>
> > > > > > >> Actually, we've discussed such intermediate step in some
> > > > > > >> early discussion. It looks to me VDUSE could be one of the users 
> > > > > > >> of
> this.
> > > > > > >>
> > > > > > >> Or I wonder whether we can switch to use anonymous
> > > > > > >> inode(fd) for VDUSE then fetching it via an VDUSE_GET_DEVICE_FD
> ioctl?
> > > > > > >>
> > > > > > > Yes, we can. Actually the current implementation in VDUSE is
> > > > > > > like this.  But seems like this is still a intermediate step.
> > > > > > > The fd should be binded to a name or something else which
> > > > > > > need to be configured before.
> > > > > >
> > > > > >
> > > > > > The name could be specified via the netlink. It looks to me
> > > > > > the real issue is that until the device is connected with a
> > > > > > userspace, it can't be used. So we also need to fail the
> > > > > > enabling if it doesn't
> > > opened.
> > > > > >
> > > > >
> > > > > Yes, that's true. So you mean we can firstly try to fetch the fd
> > > > > binded to a name/vduse_id via an VDUSE_GET_DEVICE_FD, then use
> > > > > the name/vduse_id as a attribute to create vdpa device? It looks fine 
> > > > > to
> me.
> > > >
> > > > I probably do not well understand. I tried reading patch [1] and
> > > > few things
> > > do not look correct as below.
> > > > Creating the vdpa device on the bus device and destroying the
> > > > device from
> > > the workqueue seems unnecessary and racy.
> > > >
> > > > It seems vduse driver needs
> > > > This is something should be done as part of the vdpa dev add
> > > > command,
> > > instead of connecting two sides separately and ensuring race free
> > > access to it.
> > > >
> > > > So VDUSE_DEV_START and VDUSE_DEV_STOP should possibly be avoided.
> > > >
> > >
> > > Yes, we can avoid these two ioctls with the help of the management tool.
> > >
> > > > $ vdpa dev add parentdev vduse_mgmtdev type net name foo2
> > > >
> > > > When above command is executed it creates necessary vdpa device
> > > > foo2
> > > on the bus.
> > > > When user binds foo2 device with the vduse driver, in the probe(),
> > > > it
> > > creates respective char device to access it from user space.
> > >
> > I see. So vduse cannot work with any existing vdpa devices like ifc, mlx5 or
> netdevsim.
> > It has its own implementation similar to fuse with its own backend of 
> > choice.
> > More below.
> >
> > > But vduse driver is not a vdpa bus driver. It works like vdpasim
> > > driver, but offloads the data plane and control plane to a user space 
> > > process.
> >
> > In that case to draw parallel lines,
> >
> > 1. netdevsim:
> > (a) create resources in kernel sw
> > (b) datapath simulates in kernel
> >
> > 2. ifc + mlx5 vdpa dev:
> > (a) creates resource in hw
> > (b) data path is in hw
> >
> > 3. vduse:
> > (a) creates resources in userspace sw
> > (b) data path is in user space.
> > hence creates data path resources for user space.
> > So char device is created, removed as result of vdpa device creation.
> >
> > For example,
> > $ vdpa dev add parentdev vduse_mgmtdev type net name foo2
> >
> > Above command will create char device for user space.
> >
> > Similar command for ifc/mlx5 would have created similar channel for rest of
> the config commands in hw.
> > vduse channel = char device, eventfd etc.
> > ifc/mlx5 hw channel = bar, irq, command interface etc Netdev sim
> > channel = sw direct calls
> >
> > Does it

Re: [PATCH] vdpa/mlx5: Use random MAC for the vdpa net instance

2020-12-01 Thread Jason Wang

On 2020/12/1 下午5:23, Cindy Lu wrote:

On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin wrote:

On Mon, Nov 30, 2020 at 06:41:45PM +0800, Cindy Lu wrote:

On Mon, Nov 30, 2020 at 5:33 PM Michael S. Tsirkin wrote:

On Mon, Nov 30, 2020 at 11:27:59AM +0200, Eli Cohen wrote:

On Mon, Nov 30, 2020 at 04:00:51AM -0500, Michael S. Tsirkin wrote:

On Mon, Nov 30, 2020 at 08:27:46AM +0200, Eli Cohen wrote:

On Sun, Nov 29, 2020 at 03:08:22PM -0500, Michael S. Tsirkin wrote:

On Sun, Nov 29, 2020 at 08:43:51AM +0200, Eli Cohen wrote:

We should not try to use the VF MAC address as that is used by the
regular (e.g. mlx5_core) NIC implementation. Instead, use a random
generated MAC address.

Suggested by: Cindy Lu
Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
Signed-off-by: Eli Cohen

I didn't realise it's possible to use VF in two ways
with and without vdpa.

Using a VF you can create quite a few resources, e.g. send queues
recieve queues, virtio_net queues etc. So you can possibly create
several instances of vdpa net devices and nic net devices.

Could you include a bit more description on the failure
mode?

Well, using the MAC address of the nic vport is wrong since that is the
MAC of the regular NIC implementation of mlx5_core.

Right but ATM it doesn't coexist with vdpa so what's the problem?

This call is wrong: mlx5_query_nic_vport_mac_address()

Is switching to a random mac for such an unusual
configuration really justified?

Since I can't use the NIC's MAC address, I have two options:
1. To get the MAC address as was chosen by the user administering the
NIC. This should invoke the set_config callback. Unfortunately this
is not implemented yet.

2. Use a random MAC address. This is OK since if (1) is implemented it
can always override this random configuration.

It looks like changing a MAC could break some guests,
can it not?

No, it will not. The current version of mlx5 VDPA does not allow regular
NIC driver and VDPA to co-exist. I have patches ready that enable that
from steering point of view. I will post them here once other patches on
which they depend will be merged.

https://patchwork.ozlabs.org/project/netdev/patch/20201120230339.651609-12-sae...@nvidia.com/

Could you be more explicit on the following points:
- which configuration is broken ATM (as in, two device have identical
macs? any other issues)?

The only wrong thing is the call to mlx5_query_nic_vport_mac_address().
It's not breaking anything yet is wrong. The random MAC address setting
is required for the steering patches.

Okay so I'm not sure the Fixes tag at least is appropriate if it's a
dependency of a new feature.

- why won't device MAC change from guest point of view?

It's lack of implementation in qemu as far as I know.

Sorry not sure I understand. What's not implemented in QEMU?

HI Michael, there are some bug in qemu to set_config, this will fix in future,
But this patch is still needed, because without this patch the mlx
driver will give an 0 mac address to qemu
and qemu will overwrite the default mac address. This will cause traffic down.

Hmm the patch description says VF mac address, not 0 address. Confused.
If there's no mac we can clear VIRTIO_NET_F_MAC and have guest
use a random value ...

I'm not sure this can work for all types of vDPA (e.g it could not be a
learning bridge in the swtich).

hi Michael，
I have tried as your suggestion, seems even remove the
VIRTIO_NET_F_MAC the qemu will still call get_cinfig and overwrite the
default address in VM,

This looks a bug in qemu, in guest driver we had:

/* Configuration may specify what MAC to use. Otherwise random. */
if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC))
virtio_cread_bytes(vdev,
offsetof(struct virtio_net_config, mac),
dev->dev_addr, dev->addr_len);
else
eth_hw_addr_random(dev);

this process is like
vdpa _init -->qemu call get_config ->mlx driver will give an mac
address with all 0-->
qemu will not check this mac address and use it --> overwrite the mac
address in qemu

So for my understanding there are several method to fix this problem

1, qemu check the mac address, if the mac address is all 0, qemu will
ignore it and set the random mac address to mlx driver.

So my understanding is that, if mac address is all 0, vDPA parent should
not advertise VIRTIO_NET_F_MAC. And qemu should emulate this feature as
you did:

1) get a random mac
2) advertise VIRTIO_NET_F_MAC
3) set the random mac to vDPA through set_config
4) advertise the random mac to emulated config to guest

2. mlx driver checks the mac address and if this mac is 0, return fail
to qemu, but this need to change the UAPI.

uAPI is probably fine since ioctl can fail. We can change the to allow
the set_config to fail but virito spec doesn't have a way to advertise
the error in this case. Anyway, the driver only risk itself for setting
a wrong

Re: [GIT PULL] vhost,vdpa: bugfixes

2020-12-01 Thread pr-tracker-bot

The pull request you sent on Mon, 30 Nov 2020 03:50:10 -0500:

> https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/f43691b59fae581ca83349539c686ecf4a01e42d

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 00/10] vhost/qemu: thread per IO SCSI vq

2020-12-01 Thread Stefan Hajnoczi

On Tue, Dec 01, 2020 at 02:45:18PM +0100, Stefano Garzarella wrote:
> On Tue, Dec 01, 2020 at 12:59:43PM +, Stefan Hajnoczi wrote:
> > On Fri, Nov 20, 2020 at 07:31:08AM -0500, Michael S. Tsirkin wrote:
> > > On Fri, Nov 20, 2020 at 08:45:49AM +, Stefan Hajnoczi wrote:
> > > > On Thu, Nov 19, 2020 at 5:08 PM Stefan Hajnoczi  
> > > > wrote:
> > > > >
> > > > > On Thu, Nov 19, 2020 at 4:43 PM Mike Christie
> > > > >  wrote:
> > > > > >
> > > > > > On 11/19/20 10:24 AM, Stefan Hajnoczi wrote:
> > > > > > > On Thu, Nov 19, 2020 at 4:13 PM Mike Christie
> > > > > > >  wrote:
> > > > > > >>
> > > > > > >> On 11/19/20 8:46 AM, Michael S. Tsirkin wrote:
> > > > > > >>> On Wed, Nov 18, 2020 at 11:31:17AM +, Stefan Hajnoczi wrote:
> > > > > > > struct vhost_run_worker_info {
> > > > > > >  struct timespec *timeout;
> > > > > > >  sigset_t *sigmask;
> > > > > > >
> > > > > > >  /* List of virtqueues to process */
> > > > > > >  unsigned nvqs;
> > > > > > >  unsigned vqs[];
> > > > > > > };
> > > > > > >
> > > > > > > /* This blocks until the timeout is reached, a signal is 
> > > > > > > received, or
> > > > > > > the vhost device is destroyed */
> > > > > > > int ret = ioctl(vhost_fd, VHOST_RUN_WORKER, );
> > > > > > >
> > > > > > > As you can see, userspace isn't involved with dealing with the
> > > > > > > requests. It just acts as a thread donor to the vhost driver.
> > > > > > >
> > > > > > > We would want the VHOST_RUN_WORKER calls to be infrequent to 
> > > > > > > avoid the
> > > > > > > penalty of switching into the kernel, copying in the arguments, 
> > > > > > > etc.
> > > > > >
> > > > > > I didn't get this part. Why have the timeout? When the timeout 
> > > > > > expires,
> > > > > > does userspace just call right back down to the kernel or does it do
> > > > > > some sort of processing/operation?
> > > > > >
> > > > > > You could have your worker function run from that ioctl wait for a
> > > > > > signal or a wake up call from the vhost_work/poll functions.
> > > > >
> > > > > An optional timeout argument is common in blocking interfaces like
> > > > > poll(2), recvmmsg(2), etc.
> > > > >
> > > > > Although something can send a signal to the thread instead,
> > > > > implementing that in an application is more awkward than passing a
> > > > > struct timespec.
> > > > >
> > > > > Compared to other blocking calls we don't expect
> > > > > ioctl(VHOST_RUN_WORKER) to return soon, so maybe the timeout will
> > > > > rarely be used and can be dropped from the interface.
> > > > >
> > > > > BTW the code I posted wasn't a carefully thought out proposal :). The
> > > > > details still need to be considered and I'm going to be offline for
> > > > > the next week so maybe someone else can think it through in the
> > > > > meantime.
> > > >
> > > > One final thought before I'm offline for a week. If
> > > > ioctl(VHOST_RUN_WORKER) is specific to a single vhost device instance
> > > > then it's hard to support poll-mode (busy waiting) workers because
> > > > each device instance consumes a whole CPU. If we stick to an interface
> > > > where the kernel manages the worker threads then it's easier to share
> > > > workers between devices for polling.
> > > 
> > > 
> > > Yes that is the reason vhost did its own reason in the first place.
> > > 
> > > 
> > > I am vaguely thinking about poll(2) or a similar interface,
> > > which can wait for an event on multiple FDs.
> > 
> > I can imagine how using poll(2) would work from a userspace perspective,
> > but on the kernel side I don't think it can be implemented cleanly.
> > poll(2) is tied to the file_operations->poll() callback and
> > read/write/error events. Not to mention there isn't a way to substitue
> > the vhost worker thread function instead of scheduling out the current
> > thread while waiting for poll fd events.
> > 
> > But maybe ioctl(VHOST_WORKER_RUN) can do it:
> > 
> >  struct vhost_run_worker_dev {
> >  int vhostfd;  /* /dev/vhost-TYPE fd */
> >  unsigned nvqs;/* number of virtqueues in vqs[] */
> >  unsigned vqs[];   /* virtqueues to process */
> >  };
> > 
> >  struct vhost_run_worker_info {
> >   struct timespec *timeout;
> >   sigset_t *sigmask;
> > 
> >   unsigned ndevices;
> >   struct vhost_run_worker_dev *devices[];
> >  };
> > 
> > In the simple case userspace sets ndevices to 1 and we just handle
> > virtqueues for the current device.
> > 
> > In the fancier shared worker thread case the userspace process has the
> > vhost fds of all the devices it is processing and passes them to
> > ioctl(VHOST_WORKER_RUN) via struct vhost_run_worker_dev elements.
> 
> Which fd will be used for this IOCTL? One of the 'vhostfd' or we should
> create a new /dev/vhost-workers (or something similar)?
> 
> Maybe the new device will be cleaner and can be reused also for other stuff
> (I'm thinking about vDPA software devices).
> 
> > 
> > From a security perspective it means the userspace thread

RE: [External] Re: [PATCH 0/7] Introduce vdpa management tool

2020-12-01 Thread Parav Pandit



> From: Yongji Xie 
> Sent: Tuesday, December 1, 2020 7:49 PM
> 
> On Tue, Dec 1, 2020 at 7:32 PM Parav Pandit  wrote:
> >
> >
> >
> > > From: Yongji Xie 
> > > Sent: Tuesday, December 1, 2020 3:26 PM
> > >
> > > On Tue, Dec 1, 2020 at 2:25 PM Jason Wang 
> wrote:
> > > >
> > > >
> > > > On 2020/11/30 下午3:07, Yongji Xie wrote:
> > > > >>> Thanks for adding me, Jason!
> > > > >>>
> > > > >>> Now I'm working on a v2 patchset for VDUSE (vDPA Device in
> > > > >>> Userspace) [1]. This tool is very useful for the vduse device.
> > > > >>> So I'm considering integrating this into my v2 patchset. But
> > > > >>> there is one problem：
> > > > >>>
> > > > >>> In this tool, vdpa device config action and enable action are
> > > > >>> combined into one netlink msg: VDPA_CMD_DEV_NEW. But in
> vduse
> > > > >>> case, it needs to be splitted because a chardev should be
> > > > >>> created and opened by a userspace process before we enable the
> > > > >>> vdpa device (call vdpa_register_device()).
> > > > >>>
> > > > >>> So I'd like to know whether it's possible (or have some plans)
> > > > >>> to add two new netlink msgs something like:
> > > > >>> VDPA_CMD_DEV_ENABLE
> > > and
> > > > >>> VDPA_CMD_DEV_DISABLE to make the config path more flexible.
> > > > >>>
> > > > >> Actually, we've discussed such intermediate step in some early
> > > > >> discussion. It looks to me VDUSE could be one of the users of this.
> > > > >>
> > > > >> Or I wonder whether we can switch to use anonymous inode(fd)
> > > > >> for VDUSE then fetching it via an VDUSE_GET_DEVICE_FD ioctl?
> > > > >>
> > > > > Yes, we can. Actually the current implementation in VDUSE is
> > > > > like this.  But seems like this is still a intermediate step.
> > > > > The fd should be binded to a name or something else which need
> > > > > to be configured before.
> > > >
> > > >
> > > > The name could be specified via the netlink. It looks to me the
> > > > real issue is that until the device is connected with a userspace,
> > > > it can't be used. So we also need to fail the enabling if it doesn't
> opened.
> > > >
> > >
> > > Yes, that's true. So you mean we can firstly try to fetch the fd
> > > binded to a name/vduse_id via an VDUSE_GET_DEVICE_FD, then use the
> > > name/vduse_id as a attribute to create vdpa device? It looks fine to me.
> >
> > I probably do not well understand. I tried reading patch [1] and few things
> do not look correct as below.
> > Creating the vdpa device on the bus device and destroying the device from
> the workqueue seems unnecessary and racy.
> >
> > It seems vduse driver needs
> > This is something should be done as part of the vdpa dev add command,
> instead of connecting two sides separately and ensuring race free access to
> it.
> >
> > So VDUSE_DEV_START and VDUSE_DEV_STOP should possibly be avoided.
> >
> 
> Yes, we can avoid these two ioctls with the help of the management tool.
> 
> > $ vdpa dev add parentdev vduse_mgmtdev type net name foo2
> >
> > When above command is executed it creates necessary vdpa device foo2
> on the bus.
> > When user binds foo2 device with the vduse driver, in the probe(), it
> creates respective char device to access it from user space.
>
I see. So vduse cannot work with any existing vdpa devices like ifc, mlx5 or 
netdevsim.
It has its own implementation similar to fuse with its own backend of choice.
More below.

> But vduse driver is not a vdpa bus driver. It works like vdpasim driver, but
> offloads the data plane and control plane to a user space process.

In that case to draw parallel lines,

1. netdevsim:
(a) create resources in kernel sw
(b) datapath simulates in kernel

2. ifc + mlx5 vdpa dev:
(a) creates resource in hw
(b) data path is in hw

3. vduse:
(a) creates resources in userspace sw
(b) data path is in user space.
hence creates data path resources for user space.
So char device is created, removed as result of vdpa device creation.

For example,
$ vdpa dev add parentdev vduse_mgmtdev type net name foo2

Above command will create char device for user space.

Similar command for ifc/mlx5 would have created similar channel for rest of the 
config commands in hw.
vduse channel = char device, eventfd etc.
ifc/mlx5 hw channel = bar, irq, command interface etc
Netdev sim channel = sw direct calls

Does it make sense?
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 000/141] Fix fall-through warnings for Clang

2020-12-01 Thread Dan Carpenter

On Sun, Nov 22, 2020 at 08:17:03AM -0800, Kees Cook wrote:
> On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote:
> > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote:
> > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote:  
> > > > > This series aims to fix almost all remaining fall-through warnings in
> > > > > order to enable -Wimplicit-fallthrough for Clang.
> > > > > 
> > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly
> > > > > add multiple break/goto/return/fallthrough statements instead of just
> > > > > letting the code fall through to the next case.
> > > > > 
> > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this
> > > > > change[1] is meant to be reverted at some point. So, this patch helps
> > > > > to move in that direction.
> > > > > 
> > > > > Something important to mention is that there is currently a 
> > > > > discrepancy
> > > > > between GCC and Clang when dealing with switch fall-through to empty 
> > > > > case
> > > > > statements or to cases that only contain a break/continue/return
> > > > > statement[2][3][4].  
> > > > 
> > > > Are we sure we want to make this change? Was it discussed before?
> > > > 
> > > > Are there any bugs Clangs puritanical definition of fallthrough helped
> > > > find?
> > > > 
> > > > IMVHO compiler warnings are supposed to warn about issues that could
> > > > be bugs. Falling through to default: break; can hardly be a bug?!  
> > > 
> > > It's certainly a place where the intent is not always clear. I think
> > > this makes all the cases unambiguous, and doesn't impact the machine
> > > code, since the compiler will happily optimize away any behavioral
> > > redundancy.
> > 
> > If none of the 140 patches here fix a real bug, and there is no change
> > to machine code then it sounds to me like a W=2 kind of a warning.
> 
> FWIW, this series has found at least one bug so far:
> https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/

This is a fallthrough to a return and not to a break.  That should
trigger a warning.  The fallthrough to a break should not generate a
warning.

The bug we're trying to fix is "missing break statement" but if the
result of the bug is "we hit a break statement" then now we're just
talking about style.  GCC should limit itself to warning about
potentially buggy code.

regards,
dan carpenter
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 000/141] Fix fall-through warnings for Clang

2020-12-01 Thread Dan Carpenter

On Mon, Nov 23, 2020 at 05:32:51PM -0800, Nick Desaulniers wrote:
> On Sun, Nov 22, 2020 at 8:17 AM Kees Cook  wrote:
> >
> > On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > > If none of the 140 patches here fix a real bug, and there is no change
> > > to machine code then it sounds to me like a W=2 kind of a warning.
> >
> > FWIW, this series has found at least one bug so far:
> > https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/
> 
> So looks like the bulk of these are:
> switch (x) {
>   case 0:
> ++x;
>   default:
> break;
> }

This should not generate a warning.

> 
> I have a patch that fixes those up for clang:
> https://reviews.llvm.org/D91895
> 
> There's 3 other cases that don't quite match between GCC and Clang I
> observe in the kernel:
> switch (x) {
>   case 0:
> ++x;
>   default:
> goto y;
> }
> y:;

This should generate a warning.

> 
> switch (x) {
>   case 0:
> ++x;
>   default:
> return;
> }

Warn for this.


> 
> switch (x) {
>   case 0:
> ++x;
>   default:
> ;
> }

Don't warn for this.

If adding a break statement changes the flow of the code then warn about
potentially missing break statements, but if it doesn't change anything
then don't warn about it.

regards,
dan carpenter
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 00/10] vhost/qemu: thread per IO SCSI vq

2020-12-01 Thread Stefano Garzarella

On Tue, Dec 01, 2020 at 12:59:43PM +, Stefan Hajnoczi wrote:

On Fri, Nov 20, 2020 at 07:31:08AM -0500, Michael S. Tsirkin wrote:

On Fri, Nov 20, 2020 at 08:45:49AM +, Stefan Hajnoczi wrote:
> On Thu, Nov 19, 2020 at 5:08 PM Stefan Hajnoczi  wrote:
> >
> > On Thu, Nov 19, 2020 at 4:43 PM Mike Christie
> >  wrote:
> > >
> > > On 11/19/20 10:24 AM, Stefan Hajnoczi wrote:
> > > > On Thu, Nov 19, 2020 at 4:13 PM Mike Christie
> > > >  wrote:
> > > >>
> > > >> On 11/19/20 8:46 AM, Michael S. Tsirkin wrote:
> > > >>> On Wed, Nov 18, 2020 at 11:31:17AM +, Stefan Hajnoczi wrote:
> > > > struct vhost_run_worker_info {
> > > >  struct timespec *timeout;
> > > >  sigset_t *sigmask;
> > > >
> > > >  /* List of virtqueues to process */
> > > >  unsigned nvqs;
> > > >  unsigned vqs[];
> > > > };
> > > >
> > > > /* This blocks until the timeout is reached, a signal is received, or
> > > > the vhost device is destroyed */
> > > > int ret = ioctl(vhost_fd, VHOST_RUN_WORKER, );
> > > >
> > > > As you can see, userspace isn't involved with dealing with the
> > > > requests. It just acts as a thread donor to the vhost driver.
> > > >
> > > > We would want the VHOST_RUN_WORKER calls to be infrequent to avoid the
> > > > penalty of switching into the kernel, copying in the arguments, etc.
> > >
> > > I didn't get this part. Why have the timeout? When the timeout expires,
> > > does userspace just call right back down to the kernel or does it do
> > > some sort of processing/operation?
> > >
> > > You could have your worker function run from that ioctl wait for a
> > > signal or a wake up call from the vhost_work/poll functions.
> >
> > An optional timeout argument is common in blocking interfaces like
> > poll(2), recvmmsg(2), etc.
> >
> > Although something can send a signal to the thread instead,
> > implementing that in an application is more awkward than passing a
> > struct timespec.
> >
> > Compared to other blocking calls we don't expect
> > ioctl(VHOST_RUN_WORKER) to return soon, so maybe the timeout will
> > rarely be used and can be dropped from the interface.
> >
> > BTW the code I posted wasn't a carefully thought out proposal :). The
> > details still need to be considered and I'm going to be offline for
> > the next week so maybe someone else can think it through in the
> > meantime.
>
> One final thought before I'm offline for a week. If
> ioctl(VHOST_RUN_WORKER) is specific to a single vhost device instance
> then it's hard to support poll-mode (busy waiting) workers because
> each device instance consumes a whole CPU. If we stick to an interface
> where the kernel manages the worker threads then it's easier to share
> workers between devices for polling.

Yes that is the reason vhost did its own reason in the first place.

I am vaguely thinking about poll(2) or a similar interface,
which can wait for an event on multiple FDs.

I can imagine how using poll(2) would work from a userspace perspective,
but on the kernel side I don't think it can be implemented cleanly.
poll(2) is tied to the file_operations->poll() callback and
read/write/error events. Not to mention there isn't a way to substitue
the vhost worker thread function instead of scheduling out the current
thread while waiting for poll fd events.

But maybe ioctl(VHOST_WORKER_RUN) can do it:

 struct vhost_run_worker_dev {
 int vhostfd;  /* /dev/vhost-TYPE fd */
 unsigned nvqs;/* number of virtqueues in vqs[] */
 unsigned vqs[];   /* virtqueues to process */
 };

 struct vhost_run_worker_info {
  struct timespec *timeout;
  sigset_t *sigmask;

  unsigned ndevices;
  struct vhost_run_worker_dev *devices[];
 };

In the simple case userspace sets ndevices to 1 and we just handle
virtqueues for the current device.

In the fancier shared worker thread case the userspace process has the
vhost fds of all the devices it is processing and passes them to
ioctl(VHOST_WORKER_RUN) via struct vhost_run_worker_dev elements.

Which fd will be used for this IOCTL? One of the 'vhostfd' or we should 
create a new /dev/vhost-workers (or something similar)?

Maybe the new device will be cleaner and can be reused also for other 
stuff (I'm thinking about vDPA software devices).

From a security perspective it means the userspace thread has access to
all vhost devices (because it has their fds).

I'm not sure how the mm is supposed to work. The devices might be
associated with different userspace processes (guests) and therefore
have different virtual memory.

Maybe in this case we should do something similar to io_uring SQPOLL 
kthread where kthread_use_mm()/kthread_unuse_mm() is used to switch 
virtual memory spaces.

After writing, I saw that we already do it this in the vhost_worker() in 
drivers/vhost/vhost.c

Just wanted to push this discussion along a little further. I'm buried
under emails and probably wont be very active over the next few days.

I think ioctl(VHOST_WORKER_RUN) might

Re: [PATCH 00/10] vhost/qemu: thread per IO SCSI vq

2020-12-01 Thread Stefan Hajnoczi

On Fri, Nov 20, 2020 at 07:31:08AM -0500, Michael S. Tsirkin wrote:
> On Fri, Nov 20, 2020 at 08:45:49AM +, Stefan Hajnoczi wrote:
> > On Thu, Nov 19, 2020 at 5:08 PM Stefan Hajnoczi  wrote:
> > >
> > > On Thu, Nov 19, 2020 at 4:43 PM Mike Christie
> > >  wrote:
> > > >
> > > > On 11/19/20 10:24 AM, Stefan Hajnoczi wrote:
> > > > > On Thu, Nov 19, 2020 at 4:13 PM Mike Christie
> > > > >  wrote:
> > > > >>
> > > > >> On 11/19/20 8:46 AM, Michael S. Tsirkin wrote:
> > > > >>> On Wed, Nov 18, 2020 at 11:31:17AM +, Stefan Hajnoczi wrote:
> > > > > struct vhost_run_worker_info {
> > > > >  struct timespec *timeout;
> > > > >  sigset_t *sigmask;
> > > > >
> > > > >  /* List of virtqueues to process */
> > > > >  unsigned nvqs;
> > > > >  unsigned vqs[];
> > > > > };
> > > > >
> > > > > /* This blocks until the timeout is reached, a signal is received, or
> > > > > the vhost device is destroyed */
> > > > > int ret = ioctl(vhost_fd, VHOST_RUN_WORKER, );
> > > > >
> > > > > As you can see, userspace isn't involved with dealing with the
> > > > > requests. It just acts as a thread donor to the vhost driver.
> > > > >
> > > > > We would want the VHOST_RUN_WORKER calls to be infrequent to avoid the
> > > > > penalty of switching into the kernel, copying in the arguments, etc.
> > > >
> > > > I didn't get this part. Why have the timeout? When the timeout expires,
> > > > does userspace just call right back down to the kernel or does it do
> > > > some sort of processing/operation?
> > > >
> > > > You could have your worker function run from that ioctl wait for a
> > > > signal or a wake up call from the vhost_work/poll functions.
> > >
> > > An optional timeout argument is common in blocking interfaces like
> > > poll(2), recvmmsg(2), etc.
> > >
> > > Although something can send a signal to the thread instead,
> > > implementing that in an application is more awkward than passing a
> > > struct timespec.
> > >
> > > Compared to other blocking calls we don't expect
> > > ioctl(VHOST_RUN_WORKER) to return soon, so maybe the timeout will
> > > rarely be used and can be dropped from the interface.
> > >
> > > BTW the code I posted wasn't a carefully thought out proposal :). The
> > > details still need to be considered and I'm going to be offline for
> > > the next week so maybe someone else can think it through in the
> > > meantime.
> > 
> > One final thought before I'm offline for a week. If
> > ioctl(VHOST_RUN_WORKER) is specific to a single vhost device instance
> > then it's hard to support poll-mode (busy waiting) workers because
> > each device instance consumes a whole CPU. If we stick to an interface
> > where the kernel manages the worker threads then it's easier to share
> > workers between devices for polling.
> 
> 
> Yes that is the reason vhost did its own reason in the first place.
> 
> 
> I am vaguely thinking about poll(2) or a similar interface,
> which can wait for an event on multiple FDs.

I can imagine how using poll(2) would work from a userspace perspective,
but on the kernel side I don't think it can be implemented cleanly.
poll(2) is tied to the file_operations->poll() callback and
read/write/error events. Not to mention there isn't a way to substitue
the vhost worker thread function instead of scheduling out the current
thread while waiting for poll fd events.

But maybe ioctl(VHOST_WORKER_RUN) can do it:

  struct vhost_run_worker_dev {
  int vhostfd;  /* /dev/vhost-TYPE fd */
  unsigned nvqs;/* number of virtqueues in vqs[] */
  unsigned vqs[];   /* virtqueues to process */
  };

  struct vhost_run_worker_info {
   struct timespec *timeout;
   sigset_t *sigmask;

   unsigned ndevices;
   struct vhost_run_worker_dev *devices[];
  };

In the simple case userspace sets ndevices to 1 and we just handle
virtqueues for the current device.

In the fancier shared worker thread case the userspace process has the
vhost fds of all the devices it is processing and passes them to
ioctl(VHOST_WORKER_RUN) via struct vhost_run_worker_dev elements.

>From a security perspective it means the userspace thread has access to
all vhost devices (because it has their fds).

I'm not sure how the mm is supposed to work. The devices might be
associated with different userspace processes (guests) and therefore
have different virtual memory.

Just wanted to push this discussion along a little further. I'm buried
under emails and probably wont be very active over the next few days.

Stefan


signature.asc
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [External] Re: [PATCH 0/7] Introduce vdpa management tool

2020-12-01 Thread Parav Pandit

> From: Yongji Xie 
> Sent: Tuesday, December 1, 2020 3:26 PM
> 
> On Tue, Dec 1, 2020 at 2:25 PM Jason Wang  wrote:
> >
> >
> > On 2020/11/30 下午3:07, Yongji Xie wrote:
> > >>> Thanks for adding me, Jason!
> > >>>
> > >>> Now I'm working on a v2 patchset for VDUSE (vDPA Device in
> > >>> Userspace) [1]. This tool is very useful for the vduse device. So
> > >>> I'm considering integrating this into my v2 patchset. But there is
> > >>> one problem：
> > >>>
> > >>> In this tool, vdpa device config action and enable action are
> > >>> combined into one netlink msg: VDPA_CMD_DEV_NEW. But in vduse
> > >>> case, it needs to be splitted because a chardev should be created
> > >>> and opened by a userspace process before we enable the vdpa device
> > >>> (call vdpa_register_device()).
> > >>>
> > >>> So I'd like to know whether it's possible (or have some plans) to
> > >>> add two new netlink msgs something like: VDPA_CMD_DEV_ENABLE
> and
> > >>> VDPA_CMD_DEV_DISABLE to make the config path more flexible.
> > >>>
> > >> Actually, we've discussed such intermediate step in some early
> > >> discussion. It looks to me VDUSE could be one of the users of this.
> > >>
> > >> Or I wonder whether we can switch to use anonymous inode(fd) for
> > >> VDUSE then fetching it via an VDUSE_GET_DEVICE_FD ioctl?
> > >>
> > > Yes, we can. Actually the current implementation in VDUSE is like
> > > this.  But seems like this is still a intermediate step. The fd
> > > should be binded to a name or something else which need to be
> > > configured before.
> >
> >
> > The name could be specified via the netlink. It looks to me the real
> > issue is that until the device is connected with a userspace, it can't
> > be used. So we also need to fail the enabling if it doesn't opened.
> >
> 
> Yes, that's true. So you mean we can firstly try to fetch the fd binded to a
> name/vduse_id via an VDUSE_GET_DEVICE_FD, then use the
> name/vduse_id as a attribute to create vdpa device? It looks fine to me.

I probably do not well understand. I tried reading patch [1] and few things do 
not look correct as below.
Creating the vdpa device on the bus device and destroying the device from the 
workqueue seems unnecessary and racy.

It seems vduse driver needs 
This is something should be done as part of the vdpa dev add command, instead 
of connecting two sides separately and ensuring race free access to it.

So VDUSE_DEV_START and VDUSE_DEV_STOP should possibly be avoided.

$ vdpa dev add parentdev vduse_mgmtdev type net name foo2

When above command is executed it creates necessary vdpa device foo2 on the bus.
When user binds foo2 device with the vduse driver, in the probe(), it creates 
respective char device to access it from user space.
Depending on which driver foo2 device is bound it, it can be used, either via 
(a) existing vhost stack  or (b) some vdpa Netdev driver? (not sure its current 
state), or (c) vduse user space.

This will have sane model to me without races unless I am missing something 
fundamental here.
This way there are not two ways to create vdpa devices from user space.
Consumers can be of different types (vhost, vduse etc) of the bus device as 
above mentioned.

[1] https://www.spinics.net/lists/linux-mm/msg231581.html

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

Re: [PATCH] vdpa/mlx5: Use random MAC for the vdpa net instance

2020-12-01 Thread Michael S. Tsirkin

On Tue, Dec 01, 2020 at 05:23:18PM +0800, Cindy Lu wrote:
> On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin  wrote:
> >
> > On Mon, Nov 30, 2020 at 06:41:45PM +0800, Cindy Lu wrote:
> > > On Mon, Nov 30, 2020 at 5:33 PM Michael S. Tsirkin  
> > > wrote:
> > > >
> > > > On Mon, Nov 30, 2020 at 11:27:59AM +0200, Eli Cohen wrote:
> > > > > On Mon, Nov 30, 2020 at 04:00:51AM -0500, Michael S. Tsirkin wrote:
> > > > > > On Mon, Nov 30, 2020 at 08:27:46AM +0200, Eli Cohen wrote:
> > > > > > > On Sun, Nov 29, 2020 at 03:08:22PM -0500, Michael S. Tsirkin 
> > > > > > > wrote:
> > > > > > > > On Sun, Nov 29, 2020 at 08:43:51AM +0200, Eli Cohen wrote:
> > > > > > > > > We should not try to use the VF MAC address as that is used 
> > > > > > > > > by the
> > > > > > > > > regular (e.g. mlx5_core) NIC implementation. Instead, use a 
> > > > > > > > > random
> > > > > > > > > generated MAC address.
> > > > > > > > >
> > > > > > > > > Suggested by: Cindy Lu 
> > > > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for 
> > > > > > > > > supported mlx5 devices")
> > > > > > > > > Signed-off-by: Eli Cohen 
> > > > > > > >
> > > > > > > > I didn't realise it's possible to use VF in two ways
> > > > > > > > with and without vdpa.
> > > > > > >
> > > > > > > Using a VF you can create quite a few resources, e.g. send queues
> > > > > > > recieve queues, virtio_net queues etc. So you can possibly create
> > > > > > > several instances of vdpa net devices and nic net devices.
> > > > > > >
> > > > > > > > Could you include a bit more description on the failure
> > > > > > > > mode?
> > > > > > >
> > > > > > > Well, using the MAC address of the nic vport is wrong since that 
> > > > > > > is the
> > > > > > > MAC of the regular NIC implementation of mlx5_core.
> > > > > >
> > > > > > Right but ATM it doesn't coexist with vdpa so what's the problem?
> > > > > >
> > > > >
> > > > > This call is wrong:  mlx5_query_nic_vport_mac_address()
> > > > >
> > > > > > > > Is switching to a random mac for such an unusual
> > > > > > > > configuration really justified?
> > > > > > >
> > > > > > > Since I can't use the NIC's MAC address, I have two options:
> > > > > > > 1. To get the MAC address as was chosen by the user administering 
> > > > > > > the
> > > > > > >NIC. This should invoke the set_config callback. Unfortunately 
> > > > > > > this
> > > > > > >is not implemented yet.
> > > > > > >
> > > > > > > 2. Use a random MAC address. This is OK since if (1) is 
> > > > > > > implemented it
> > > > > > >can always override this random configuration.
> > > > > > >
> > > > > > > > It looks like changing a MAC could break some guests,
> > > > > > > > can it not?
> > > > > > > >
> > > > > > >
> > > > > > > No, it will not. The current version of mlx5 VDPA does not allow 
> > > > > > > regular
> > > > > > > NIC driver and VDPA to co-exist. I have patches ready that enable 
> > > > > > > that
> > > > > > > from steering point of view. I will post them here once other 
> > > > > > > patches on
> > > > > > > which they depend will be merged.
> > > > > > >
> > > > > > > https://patchwork.ozlabs.org/project/netdev/patch/20201120230339.651609-12-sae...@nvidia.com/
> > > > > >
> > > > > > Could you be more explicit on the following points:
> > > > > > - which configuration is broken ATM (as in, two device have 
> > > > > > identical
> > > > > >   macs? any other issues)?
> > > > >
> > > > > The only wrong thing is the call to  
> > > > > mlx5_query_nic_vport_mac_address().
> > > > > It's not breaking anything yet is wrong. The random MAC address 
> > > > > setting
> > > > > is required for the steering patches.
> > > >
> > > > Okay so I'm not sure the Fixes tag at least is appropriate if it's a
> > > > dependency of a new feature.
> > > >
> > > > > > - why won't device MAC change from guest point of view?
> > > > > >
> > > > >
> > > > > It's lack of implementation in qemu as far as I know.
> > > >
> > > > Sorry not sure I understand. What's not implemented in QEMU?
> > > >
> > > HI Michael, there are some bug in qemu to set_config, this will fix in 
> > > future,
> > > But this patch is still needed, because without this patch the mlx
> > > driver will give an 0 mac address to qemu
> > > and qemu will overwrite the default mac address.  This will cause traffic 
> > > down.
> >
> > Hmm the patch description says VF mac address, not 0 address. Confused.
> > If there's no mac we can clear VIRTIO_NET_F_MAC and have guest
> > use a random value ...
> >
> hi Michael，
> I have tried as your suggestion, seems even remove the
> VIRTIO_NET_F_MAC the qemu will still call get_cinfig and overwrite the
> default address in  VM,
> this process is like
> vdpa _init -->qemu call get_config ->mlx driver will give  an mac
> address with all 0-->
> qemu will not check this mac address and use it --> overwrite the mac
> address in qemu

Right but guest will ignore it then, right?

> So for my understanding there are several method to fix this

Re: [PATCH v2 12/17] vdpa_sim: add get_config callback in vdpasim_dev_attr

2020-12-01 Thread Stefano Garzarella


On Tue, Dec 01, 2020 at 11:44:19AM +0800, Jason Wang wrote:


On 2020/11/30 下午10:14, Stefano Garzarella wrote:

On Mon, Nov 30, 2020 at 11:25:31AM +0800, Jason Wang wrote:


On 2020/11/26 下午10:49, Stefano Garzarella wrote:

The get_config callback can be used by the device to fill the
config structure.
The callback will be invoked in vdpasim_get_config() before copying
bytes into caller buffer.

Move vDPA-net config updates from vdpasim_set_features() in the
new vdpasim_net_get_config() callback.

Signed-off-by: Stefano Garzarella 
---
 drivers/vdpa/vdpa_sim/vdpa_sim.c | 33 +++-
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c 
b/drivers/vdpa/vdpa_sim/vdpa_sim.c

index c07ddf6af720..8b87ce0485b6 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -58,6 +58,8 @@ struct vdpasim_virtqueue {
 #define VDPASIM_NET_FEATURES    (VDPASIM_FEATURES | \
  (1ULL << VIRTIO_NET_F_MAC))
+struct vdpasim;
+
 struct vdpasim_dev_attr {
 u64 supported_features;
 size_t config_size;
@@ -65,6 +67,7 @@ struct vdpasim_dev_attr {
 u32 id;
 work_func_t work_fn;
+    void (*get_config)(struct vdpasim *vdpasim, void *config);
 };
 /* State of each vdpasim device */
@@ -520,8 +523,6 @@ static u64 vdpasim_get_features(struct 
vdpa_device *vdpa)
 static int vdpasim_set_features(struct vdpa_device *vdpa, u64 
features)

 {
 struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
-    struct virtio_net_config *config =
-    (struct virtio_net_config *)vdpasim->config;
 /* DMA mapping must be done by driver */
 if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
@@ -529,15 +530,6 @@ static int vdpasim_set_features(struct 
vdpa_device *vdpa, u64 features)
 vdpasim->features = features & 
vdpasim->dev_attr.supported_features;
-    /* We generally only know whether guest is using the legacy 
interface
- * here, so generally that's the earliest we can set config 
fields.

- * Note: We actually require VIRTIO_F_ACCESS_PLATFORM above which
- * implies VIRTIO_F_VERSION_1, but let's not try to be 
clever here.

- */
-
-    config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
-    config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
-    memcpy(config->mac, macaddr_buf, ETH_ALEN);
 return 0;
 }
@@ -593,8 +585,12 @@ static void vdpasim_get_config(struct 
vdpa_device *vdpa, unsigned int offset,

 {
 struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
-    if (offset + len < vdpasim->dev_attr.config_size)
-    memcpy(buf, vdpasim->config + offset, len);
+    if (offset + len > vdpasim->dev_attr.config_size)
+    return;
+
+    vdpasim->dev_attr.get_config(vdpasim, vdpasim->config);
+
+    memcpy(buf, vdpasim->config + offset, len);
 }



I wonder how much value we can get from vdpasim->config consider 
we've already had get_config() method.


Is it possible to copy to the buffer directly here?


I had thought of eliminating it too, but then I wanted to do something 
similar to what we do in QEMU (hw/virtio/virtio.c), leaving in the 
simulator core the buffer, the memory copy (handling offset and len), 
and the boundary checks.


In this way each device should simply fill the entire configuration 
and we can avoid code duplication.


Storing the configuration in the core may also be useful if some 
device needs to support config writes.



I think in that way we need should provide config_write().


Yes, I'll add set_config() callback.






Do you think it makes sense, or is it better to move everything in the 
device?



I prefer to do that in the device but it's also fine keep what the 
patch has done.


Okay, for now I'll keep it and add the set_config() callback, but I'm 
open to move it in the device.


Thanks,
Stefano

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

Re: [PATCH v2 01/20] drm/amdgpu: Fix trailing whitespaces

2020-12-01 Thread Christian König


Reviewed-by: Christian König  on patch #1 and #15.

Acked-by: Christian König  on patch #2 and #16.

Regards,
Christian.

Am 01.12.20 um 11:35 schrieb Thomas Zimmermann:

Adhere to kernel coding style.

Signed-off-by: Thomas Zimmermann 
Acked-by: Alex Deucher 
Acked-by: Sam Ravnborg 
Cc: Alex Deucher 
Cc: Christian König 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 5f304425c948..da23c0f21311 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4922,8 +4922,8 @@ pci_ers_result_t amdgpu_pci_error_detected(struct pci_dev 
*pdev, pci_channel_sta
case pci_channel_io_normal:
return PCI_ERS_RESULT_CAN_RECOVER;
/* Fatal error, prepare for slot reset */
-   case pci_channel_io_frozen: 
-   /*  
+   case pci_channel_io_frozen:
+   /*
 * Cancel and wait for all TDRs in progress if failing to
 * set  adev->in_gpu_reset in amdgpu_device_lock_adev
 *
@@ -5014,7 +5014,7 @@ pci_ers_result_t amdgpu_pci_slot_reset(struct pci_dev 
*pdev)
goto out;
}
  
-	adev->in_pci_err_recovery = true;	

+   adev->in_pci_err_recovery = true;
r = amdgpu_device_pre_asic_reset(adev, NULL, _full_reset);
adev->in_pci_err_recovery = false;
if (r)


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

40 matches

Mail list logo