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

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

[Michael S. Tsirkin]

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 <lulu@redhat.com>
> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> Signed-off-by: Eli Cohen <elic@nvidia.com>

I didn't realise it's possible to use VF in two ways
with and without vdpa.
Could you include a bit more description on the failure
mode?
Is switching to a random mac for such an unusual
configuration really justified?
It looks like changing a MAC could break some guests,
can it not?

> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 1fa6fcac8299..80d06d958b8b 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1955,10 +1955,7 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)
>  	if (err)
>  		goto err_mtu;
>  
> -	err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> -	if (err)
> -		goto err_mtu;
> -
> +	eth_random_addr(config->mac);
>  	mvdev->vdev.dma_dev = mdev->device;
>  	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
>  	if (err)
> -- 
> 2.26.2

_______________________________________________
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

[Michael S. Tsirkin]

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 <lulu@redhat.com>
> > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > 
> > 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?

> > 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-saeedm@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)?
- why won't device MAC change from guest point of view?


> > > ---
> > >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index 1fa6fcac8299..80d06d958b8b 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -1955,10 +1955,7 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)
> > >  	if (err)
> > >  		goto err_mtu;
> > >  
> > > -	err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> > > -	if (err)
> > > -		goto err_mtu;
> > > -
> > > +	eth_random_addr(config->mac);
> > >  	mvdev->vdev.dma_dev = mdev->device;
> > >  	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> > >  	if (err)
> > > -- 
> > > 2.26.2
> > 

_______________________________________________
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

[Michael S. Tsirkin]

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 <lulu@redhat.com>
> > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > 
> > > > 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-saeedm@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?

> > 
> > > > > ---
> > > > >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > > > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > index 1fa6fcac8299..80d06d958b8b 100644
> > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > @@ -1955,10 +1955,7 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)
> > > > >  	if (err)
> > > > >  		goto err_mtu;
> > > > >  
> > > > > -	err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> > > > > -	if (err)
> > > > > -		goto err_mtu;
> > > > > -
> > > > > +	eth_random_addr(config->mac);
> > > > >  	mvdev->vdev.dma_dev = mdev->device;
> > > > >  	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> > > > >  	if (err)
> > > > > -- 
> > > > > 2.26.2
> > > > 
> > 

_______________________________________________
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

[Michael S. Tsirkin]

On Mon, Nov 30, 2020 at 01:51:06PM +0200, Eli Cohen wrote:
> On Mon, Nov 30, 2020 at 04:33:09AM -0500, 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 <lulu@redhat.com>
> > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > 
> > > > > > 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-saeedm@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.
> > 
> 
> OK, let's leave it for now. I will push along with the steering patches.
> The meaning is the the VDPA net device instance is create with a MAC of
> all zeros which also mean the link is down. You can set a MAC which will
> let the link come up.
> The vdpa driver will not get a callback as I
> stated but since current mode of steering directs all traffic to the
> vdpa instance it will work. In the future this must be fixed.

So at the moment the MAC is in the config space and that is read during
probe.  So I guess userspace will do that before passing device to
guest.

> 
> > > > - 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?
> > 
> 
> vdpa config operation set_config() should be called whenever the MAC is
> changed, e.g. when administrator of the vdpa net device changes the mac.
> This does not happen which is a bug.

I am not sure that's a good interface for that, I think set_config
is for guest writes into config space.
Will let Jason comment once patches are posted.

> > > > 
> > > > > > > ---
> > > > > > >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > > > > > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > index 1fa6fcac8299..80d06d958b8b 100644
> > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > @@ -1955,10 +1955,7 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)
> > > > > > >  	if (err)
> > > > > > >  		goto err_mtu;
> > > > > > >  
> > > > > > > -	err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> > > > > > > -	if (err)
> > > > > > > -		goto err_mtu;
> > > > > > > -
> > > > > > > +	eth_random_addr(config->mac);
> > > > > > >  	mvdev->vdev.dma_dev = mdev->device;
> > > > > > >  	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> > > > > > >  	if (err)
> > > > > > > -- 
> > > > > > > 2.26.2
> > > > > > 
> > > > 
> > 

_______________________________________________
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

[Michael S. Tsirkin]

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 <mst@redhat.com> 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 <lulu@redhat.com>
> > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > >
> > > > > > 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-saeedm@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 ...

> > > >
> > > > > > > ---
> > > > > > >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > > > > > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > index 1fa6fcac8299..80d06d958b8b 100644
> > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > @@ -1955,10 +1955,7 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)
> > > > > > >       if (err)
> > > > > > >               goto err_mtu;
> > > > > > >
> > > > > > > -     err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> > > > > > > -     if (err)
> > > > > > > -             goto err_mtu;
> > > > > > > -
> > > > > > > +     eth_random_addr(config->mac);
> > > > > > >       mvdev->vdev.dma_dev = mdev->device;
> > > > > > >       err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> > > > > > >       if (err)
> > > > > > > --
> > > > > > > 2.26.2
> > > > > >
> > > >
> >

_______________________________________________
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

[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 <mst@redhat.com> 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 <mst@redhat.com> 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 <lulu@redhat.com>
> > > > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > > >
> > > > > > > > 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-saeedm@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 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.
> 2. mlx driver checks the mac address and if this mac is 0, return fail
> to qemu, but this need to change the UAPI.
> 3. mlx driver it shelf should get an correct mac address while it init.
> 4. add check in qemu get_config function  , if there is not F_MAC Then
> ignore the mac address from mlx driver
> 
> not sure which method is more suitable ?
> 
> Thanks
> Cindy
> 
> 
> 
> > > > > >
> > > > > > > > > ---
> > > > > > > > >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > > > > > > > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > index 1fa6fcac8299..80d06d958b8b 100644
> > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > @@ -1955,10 +1955,7 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)
> > > > > > > > >       if (err)
> > > > > > > > >               goto err_mtu;
> > > > > > > > >
> > > > > > > > > -     err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> > > > > > > > > -     if (err)
> > > > > > > > > -             goto err_mtu;
> > > > > > > > > -
> > > > > > > > > +     eth_random_addr(config->mac);
> > > > > > > > >       mvdev->vdev.dma_dev = mdev->device;
> > > > > > > > >       err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> > > > > > > > >       if (err)
> > > > > > > > > --
> > > > > > > > > 2.26.2
> > > > > > > >
> > > > > >
> > > >
> >

_______________________________________________
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

[Jason Wang]

On 2020/12/1 下午5:23, Cindy Lu wrote:
> On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> 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 <lulu@redhat.com>
>>>>>>>>> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
>>>>>>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>>>>>> 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-saeedm@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 value, so we're probably fine.

Thanks


> 3. mlx driver it shelf should get an correct mac address while it init.
> 4. add check in qemu get_config function  , if there is not F_MAC Then
> ignore the mac address from mlx driver
>
> not sure which method is more suitable ?
>
> Thanks
> Cindy
>
>
>
>>>>>>>>> ---
>>>>>>>>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
>>>>>>>>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>>>>>> index 1fa6fcac8299..80d06d958b8b 100644
>>>>>>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>>>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>>>>>> @@ -1955,10 +1955,7 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)
>>>>>>>>>        if (err)
>>>>>>>>>                goto err_mtu;
>>>>>>>>>
>>>>>>>>> -     err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
>>>>>>>>> -     if (err)
>>>>>>>>> -             goto err_mtu;
>>>>>>>>> -
>>>>>>>>> +     eth_random_addr(config->mac);
>>>>>>>>>        mvdev->vdev.dma_dev = mdev->device;
>>>>>>>>>        err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
>>>>>>>>>        if (err)
>>>>>>>>> --
>>>>>>>>> 2.26.2

_______________________________________________
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

[Michael S. Tsirkin]

On Wed, Dec 02, 2020 at 07:57:14AM +0200, Eli Cohen wrote:
> On Wed, Dec 02, 2020 at 12:18:36PM +0800, Jason Wang wrote:
> > 
> > On 2020/12/1 下午5:23, Cindy Lu wrote:
> > > On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> 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 <lulu@redhat.com>
> > > > > > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > > > > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > > > > > 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-saeedm@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:
> 
> Thinking it over, at least in mlx5, I should always advertise
> VIRTIO_NET_F_MAC and set a non zero MAC value. The source of the MAC can
> be either randomly generated value by mlx5_vdpa or by a management tool.
> This is important becauase we should not let the VM modify the MAC. If
> we do it can set a MAC value identical to the mlx5 NIC driver and can
> kidnap traffic that was not destined to it.
> 
> In addition, when VIRTIO_NET_F_MAC is published, attempts to change the
> MAC address from the VM should result in error.

That is not what the spec says though.
VIRTIO_NET_F_MAC only says whether mac is valid in the config space.
Whether guest can control that depends on VIRTIO_NET_F_CTRL_MAC_ADDR:

	The VIRTIO_NET_CTRL_MAC_ADDR_SET command is used to set the default MAC address which rx
	filtering accepts (and if VIRTIO_NET_F_MAC_ADDR has been negotiated, this will be reflected in mac in
	config space).
	The command-specific-data for VIRTIO_NET_CTRL_MAC_ADDR_SET is the 6-byte MAC address.




> 
> > 
> > 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 value,
> > so we're probably fine.
> > 
> > Thanks
> > 
> > 
> > > 3. mlx driver it shelf should get an correct mac address while it init.
> > > 4. add check in qemu get_config function  , if there is not F_MAC Then
> > > ignore the mac address from mlx driver
> > > 
> > > not sure which method is more suitable ?
> > > 
> > > Thanks
> > > Cindy
> > > 
> > > 
> > > 
> > > > > > > > > > > ---
> > > > > > > > > > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > > > > > > > > > >   1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > > index 1fa6fcac8299..80d06d958b8b 100644
> > > > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > > @@ -1955,10 +1955,7 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)
> > > > > > > > > > >        if (err)
> > > > > > > > > > >                goto err_mtu;
> > > > > > > > > > > 
> > > > > > > > > > > -     err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> > > > > > > > > > > -     if (err)
> > > > > > > > > > > -             goto err_mtu;
> > > > > > > > > > > -
> > > > > > > > > > > +     eth_random_addr(config->mac);
> > > > > > > > > > >        mvdev->vdev.dma_dev = mdev->device;
> > > > > > > > > > >        err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> > > > > > > > > > >        if (err)
> > > > > > > > > > > --
> > > > > > > > > > > 2.26.2
> > 

_______________________________________________
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

[Michael S. Tsirkin]

On Wed, Dec 02, 2020 at 02:12:41PM +0200, Eli Cohen wrote:
> On Wed, Dec 02, 2020 at 04:23:11AM -0500, Michael S. Tsirkin wrote:
> > On Wed, Dec 02, 2020 at 07:57:14AM +0200, Eli Cohen wrote:
> > > On Wed, Dec 02, 2020 at 12:18:36PM +0800, Jason Wang wrote:
> > > > 
> > > > On 2020/12/1 下午5:23, Cindy Lu wrote:
> > > > > On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> 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 <lulu@redhat.com>
> > > > > > > > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > > > > > > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > > > > > > > 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-saeedm@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:
> > > 
> > > Thinking it over, at least in mlx5, I should always advertise
> > > VIRTIO_NET_F_MAC and set a non zero MAC value. The source of the MAC can
> > > be either randomly generated value by mlx5_vdpa or by a management tool.
> > > This is important becauase we should not let the VM modify the MAC. If
> > > we do it can set a MAC value identical to the mlx5 NIC driver and can
> > > kidnap traffic that was not destined to it.
> > > 
> > > In addition, when VIRTIO_NET_F_MAC is published, attempts to change the
> > > MAC address from the VM should result in error.
> > 
> > That is not what the spec says though.
> > VIRTIO_NET_F_MAC only says whether mac is valid in the config space.
> > Whether guest can control that depends on VIRTIO_NET_F_CTRL_MAC_ADDR:
> > 
> > 	The VIRTIO_NET_CTRL_MAC_ADDR_SET command is used to set the default MAC address which rx
> > 	filtering accepts (and if VIRTIO_NET_F_MAC_ADDR has been negotiated, this will be reflected in mac in
> > 	config space).
> > 	The command-specific-data for VIRTIO_NET_CTRL_MAC_ADDR_SET is the 6-byte MAC address.
> 
> Two questions here:
> 1. Now we don't have support for control virtqueue. Yet, we must filter
> packets based on MAC, what do you suggest to do here?

How about an ioctl to pass the mac to the device?
Maybe mirroring the control vq struct format ...

> 2. When control virtqueue is implemented, which admin entity is allowed
> to change the MAC filtering table?

It's up to the hypervisor: if VIRTIO_NET_F_CTRL_MAC_ADDR
is enabled this means hypervisor trusts the guest with full
access to NIC network.
If it's clear then mac must be set by hardware or the hypervisor.

> > 
> > 
> > 
> > 
> > > 
> > > > 
> > > > 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 value,
> > > > so we're probably fine.
> > > > 
> > > > Thanks
> > > > 
> > > > 
> > > > > 3. mlx driver it shelf should get an correct mac address while it init.
> > > > > 4. add check in qemu get_config function  , if there is not F_MAC Then
> > > > > ignore the mac address from mlx driver
> > > > > 
> > > > > not sure which method is more suitable ?
> > > > > 
> > > > > Thanks
> > > > > Cindy
> > > > > 
> > > > > 
> > > > > 
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > > > > > > > > > > > >   1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > > > > > > > > > 
> > > > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > > > > index 1fa6fcac8299..80d06d958b8b 100644
> > > > > > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > > > > @@ -1955,10 +1955,7 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)
> > > > > > > > > > > > >        if (err)
> > > > > > > > > > > > >                goto err_mtu;
> > > > > > > > > > > > > 
> > > > > > > > > > > > > -     err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> > > > > > > > > > > > > -     if (err)
> > > > > > > > > > > > > -             goto err_mtu;
> > > > > > > > > > > > > -
> > > > > > > > > > > > > +     eth_random_addr(config->mac);
> > > > > > > > > > > > >        mvdev->vdev.dma_dev = mdev->device;
> > > > > > > > > > > > >        err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> > > > > > > > > > > > >        if (err)
> > > > > > > > > > > > > --
> > > > > > > > > > > > > 2.26.2
> > > > 
> > 

_______________________________________________
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

[Jason Wang]

On 2020/12/2 下午8:17, Michael S. Tsirkin wrote:
> On Wed, Dec 02, 2020 at 02:12:41PM +0200, Eli Cohen wrote:
>> On Wed, Dec 02, 2020 at 04:23:11AM -0500, Michael S. Tsirkin wrote:
>>> On Wed, Dec 02, 2020 at 07:57:14AM +0200, Eli Cohen wrote:
>>>> On Wed, Dec 02, 2020 at 12:18:36PM +0800, Jason Wang wrote:
>>>>> On 2020/12/1 下午5:23, Cindy Lu wrote:
>>>>>> On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin<mst@redhat.com>  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<mst@redhat.com>  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<lulu@redhat.com>
>>>>>>>>>>>>>> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
>>>>>>>>>>>>>> Signed-off-by: Eli Cohen<elic@nvidia.com>
>>>>>>>>>>>>> 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-saeedm@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:
>>>> Thinking it over, at least in mlx5, I should always advertise
>>>> VIRTIO_NET_F_MAC and set a non zero MAC value. The source of the MAC can
>>>> be either randomly generated value by mlx5_vdpa or by a management tool.
>>>> This is important becauase we should not let the VM modify the MAC. If
>>>> we do it can set a MAC value identical to the mlx5 NIC driver and can
>>>> kidnap traffic that was not destined to it.
>>>>
>>>> In addition, when VIRTIO_NET_F_MAC is published, attempts to change the
>>>> MAC address from the VM should result in error.
>>> That is not what the spec says though.
>>> VIRTIO_NET_F_MAC only says whether mac is valid in the config space.
>>> Whether guest can control that depends on VIRTIO_NET_F_CTRL_MAC_ADDR:
>>>
>>> 	The VIRTIO_NET_CTRL_MAC_ADDR_SET command is used to set the default MAC address which rx
>>> 	filtering accepts (and if VIRTIO_NET_F_MAC_ADDR has been negotiated, this will be reflected in mac in
>>> 	config space).
>>> 	The command-specific-data for VIRTIO_NET_CTRL_MAC_ADDR_SET is the 6-byte MAC address.
>> Two questions here:
>> 1. Now we don't have support for control virtqueue. Yet, we must filter
>> packets based on MAC, what do you suggest to do here?
> How about an ioctl to pass the mac to the device?
> Maybe mirroring the control vq struct format ...


I think we'd better avoid such ad-hoc ioctls to make vhost-vDPA type 
independent. And doing this means we need to seek another way for 
virtio-vdpa drivers.

Thanks


>
>> 2. When control virtqueue is implemented, which admin entity is allowed
>> to change the MAC filtering table?
> It's up to the hypervisor: if VIRTIO_NET_F_CTRL_MAC_ADDR
> is enabled this means hypervisor trusts the guest with full
> access to NIC network.
> If it's clear then mac must be set by hardware or the hypervisor.
>

_______________________________________________
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

[Michael S. Tsirkin]

On Wed, Dec 02, 2020 at 09:00:07PM +0800, Jason Wang wrote:
> 
> On 2020/12/2 下午8:17, Michael S. Tsirkin wrote:
> > On Wed, Dec 02, 2020 at 02:12:41PM +0200, Eli Cohen wrote:
> > > On Wed, Dec 02, 2020 at 04:23:11AM -0500, Michael S. Tsirkin wrote:
> > > > On Wed, Dec 02, 2020 at 07:57:14AM +0200, Eli Cohen wrote:
> > > > > On Wed, Dec 02, 2020 at 12:18:36PM +0800, Jason Wang wrote:
> > > > > > On 2020/12/1 下午5:23, Cindy Lu wrote:
> > > > > > > On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin<mst@redhat.com>  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<mst@redhat.com>  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<lulu@redhat.com>
> > > > > > > > > > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > > > > > > > > > > > > > Signed-off-by: Eli Cohen<elic@nvidia.com>
> > > > > > > > > > > > > > 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-saeedm@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:
> > > > > Thinking it over, at least in mlx5, I should always advertise
> > > > > VIRTIO_NET_F_MAC and set a non zero MAC value. The source of the MAC can
> > > > > be either randomly generated value by mlx5_vdpa or by a management tool.
> > > > > This is important becauase we should not let the VM modify the MAC. If
> > > > > we do it can set a MAC value identical to the mlx5 NIC driver and can
> > > > > kidnap traffic that was not destined to it.
> > > > > 
> > > > > In addition, when VIRTIO_NET_F_MAC is published, attempts to change the
> > > > > MAC address from the VM should result in error.
> > > > That is not what the spec says though.
> > > > VIRTIO_NET_F_MAC only says whether mac is valid in the config space.
> > > > Whether guest can control that depends on VIRTIO_NET_F_CTRL_MAC_ADDR:
> > > > 
> > > > 	The VIRTIO_NET_CTRL_MAC_ADDR_SET command is used to set the default MAC address which rx
> > > > 	filtering accepts (and if VIRTIO_NET_F_MAC_ADDR has been negotiated, this will be reflected in mac in
> > > > 	config space).
> > > > 	The command-specific-data for VIRTIO_NET_CTRL_MAC_ADDR_SET is the 6-byte MAC address.
> > > Two questions here:
> > > 1. Now we don't have support for control virtqueue. Yet, we must filter
> > > packets based on MAC, what do you suggest to do here?
> > How about an ioctl to pass the mac to the device?
> > Maybe mirroring the control vq struct format ...
> 
> 
> I think we'd better avoid such ad-hoc ioctls to make vhost-vDPA type
> independent.

Fundamentally this is about handling some VQs in QEMU, right?
Maybe a generic ioctl along the lines of "CTRL_VQ" passing
vq number and a command buffer from guest?
Seems generic enough for you?

> And doing this means we need to seek another way for
> virtio-vdpa drivers.
> 
> Thanks
> 
> 
> > 
> > > 2. When control virtqueue is implemented, which admin entity is allowed
> > > to change the MAC filtering table?
> > It's up to the hypervisor: if VIRTIO_NET_F_CTRL_MAC_ADDR
> > is enabled this means hypervisor trusts the guest with full
> > access to NIC network.
> > If it's clear then mac must be set by hardware or the hypervisor.
> > 

_______________________________________________
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

[Jason Wang]

On 2020/12/2 下午9:07, Michael S. Tsirkin wrote:
>>>> Two questions here:
>>>> 1. Now we don't have support for control virtqueue. Yet, we must filter
>>>> packets based on MAC, what do you suggest to do here?
>>> How about an ioctl to pass the mac to the device?
>>> Maybe mirroring the control vq struct format ...
>> I think we'd better avoid such ad-hoc ioctls to make vhost-vDPA type
>> independent.
> Fundamentally this is about handling some VQs in QEMU, right?
> Maybe a generic ioctl along the lines of "CTRL_VQ" passing
> vq number and a command buffer from guest?
> Seems generic enough for you?
>

It looks to me you want to invent a synchronized API (or vDPA config 
ops) for submitting virtio descriptors.

Several issues I can think for this:

1) control vq allows the request to be handled asynchronously
2) we still need a way to isolate the DMA if there's a hardware 
virtqueue for the device that use DMA
3) new vDPA config operations need to be invented, new uAPI for 
vhost-vDPA, new virtio config ops for virtio-vDPA

It looks to me we can overcome 1) and 2) if we just stick to a virtqueue 
interface in vhost-vDPA as I proposed in [1]. For issue 3) it also 
requires much less work.

Thanks

[1] https://lkml.org/lkml/2020/9/23/1243

_______________________________________________
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

[Jason Wang]

On 2020/12/2 下午5:23, Michael S. Tsirkin wrote:
> On Wed, Dec 02, 2020 at 07:57:14AM +0200, Eli Cohen wrote:
>> On Wed, Dec 02, 2020 at 12:18:36PM +0800, Jason Wang wrote:
>>> On 2020/12/1 下午5:23, Cindy Lu wrote:
>>>> On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin<mst@redhat.com>  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<mst@redhat.com>  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<lulu@redhat.com>
>>>>>>>>>>>> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
>>>>>>>>>>>> Signed-off-by: Eli Cohen<elic@nvidia.com>
>>>>>>>>>>> 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-saeedm@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:
>> Thinking it over, at least in mlx5, I should always advertise
>> VIRTIO_NET_F_MAC and set a non zero MAC value. The source of the MAC can
>> be either randomly generated value by mlx5_vdpa or by a management tool.
>> This is important becauase we should not let the VM modify the MAC. If
>> we do it can set a MAC value identical to the mlx5 NIC driver and can
>> kidnap traffic that was not destined to it.
>>
>> In addition, when VIRTIO_NET_F_MAC is published, attempts to change the
>> MAC address from the VM should result in error.
> That is not what the spec says though.
> VIRTIO_NET_F_MAC only says whether mac is valid in the config space.
> Whether guest can control that depends on VIRTIO_NET_F_CTRL_MAC_ADDR:
>
> 	The VIRTIO_NET_CTRL_MAC_ADDR_SET command is used to set the default MAC address which rx
> 	filtering accepts (and if VIRTIO_NET_F_MAC_ADDR has been negotiated, this will be reflected in mac in
> 	config space).
> 	The command-specific-data for VIRTIO_NET_CTRL_MAC_ADDR_SET is the 6-byte MAC address.


Consider VIRTIO_NET_CTRL_MAC_ADDR_SET is not supported now. What Eli 
proposed here should work?

Thanks


>
>
>
>

_______________________________________________
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

[Michael S. Tsirkin]

On Wed, Dec 02, 2020 at 09:48:25PM +0800, Jason Wang wrote:
> 
> On 2020/12/2 下午5:23, Michael S. Tsirkin wrote:
> > On Wed, Dec 02, 2020 at 07:57:14AM +0200, Eli Cohen wrote:
> > > On Wed, Dec 02, 2020 at 12:18:36PM +0800, Jason Wang wrote:
> > > > On 2020/12/1 下午5:23, Cindy Lu wrote:
> > > > > On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin<mst@redhat.com>  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<mst@redhat.com>  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<lulu@redhat.com>
> > > > > > > > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > > > > > > > > > > > Signed-off-by: Eli Cohen<elic@nvidia.com>
> > > > > > > > > > > > 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-saeedm@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:
> > > Thinking it over, at least in mlx5, I should always advertise
> > > VIRTIO_NET_F_MAC and set a non zero MAC value. The source of the MAC can
> > > be either randomly generated value by mlx5_vdpa or by a management tool.
> > > This is important becauase we should not let the VM modify the MAC. If
> > > we do it can set a MAC value identical to the mlx5 NIC driver and can
> > > kidnap traffic that was not destined to it.
> > > 
> > > In addition, when VIRTIO_NET_F_MAC is published, attempts to change the
> > > MAC address from the VM should result in error.
> > That is not what the spec says though.
> > VIRTIO_NET_F_MAC only says whether mac is valid in the config space.
> > Whether guest can control that depends on VIRTIO_NET_F_CTRL_MAC_ADDR:
> > 
> > 	The VIRTIO_NET_CTRL_MAC_ADDR_SET command is used to set the default MAC address which rx
> > 	filtering accepts (and if VIRTIO_NET_F_MAC_ADDR has been negotiated, this will be reflected in mac in
> > 	config space).
> > 	The command-specific-data for VIRTIO_NET_CTRL_MAC_ADDR_SET is the 6-byte MAC address.
> 
> 
> Consider VIRTIO_NET_CTRL_MAC_ADDR_SET is not supported now. What Eli
> proposed here should work?
> 
> Thanks
> 

We can have management set a MAC address. Randomizing it in kernel
does not seem like a reasonable policy to me ...


> > 
> > 
> > 
> > 

_______________________________________________
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

[Michael S. Tsirkin]

On Thu, Dec 03, 2020 at 08:49:28AM +0200, Eli Cohen wrote:
> On Wed, Dec 02, 2020 at 05:00:22PM -0500, Michael S. Tsirkin wrote:
> > On Wed, Dec 02, 2020 at 09:48:25PM +0800, Jason Wang wrote:
> > > 
> > > On 2020/12/2 下午5:23, Michael S. Tsirkin wrote:
> > > > On Wed, Dec 02, 2020 at 07:57:14AM +0200, Eli Cohen wrote:
> > > > > On Wed, Dec 02, 2020 at 12:18:36PM +0800, Jason Wang wrote:
> > > > > > On 2020/12/1 下午5:23, Cindy Lu wrote:
> > > > > > > On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin<mst@redhat.com>  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<mst@redhat.com>  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<lulu@redhat.com>
> > > > > > > > > > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > > > > > > > > > > > > > Signed-off-by: Eli Cohen<elic@nvidia.com>
> > > > > > > > > > > > > > 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-saeedm@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:
> > > > > Thinking it over, at least in mlx5, I should always advertise
> > > > > VIRTIO_NET_F_MAC and set a non zero MAC value. The source of the MAC can
> > > > > be either randomly generated value by mlx5_vdpa or by a management tool.
> > > > > This is important becauase we should not let the VM modify the MAC. If
> > > > > we do it can set a MAC value identical to the mlx5 NIC driver and can
> > > > > kidnap traffic that was not destined to it.
> > > > > 
> > > > > In addition, when VIRTIO_NET_F_MAC is published, attempts to change the
> > > > > MAC address from the VM should result in error.
> > > > That is not what the spec says though.
> > > > VIRTIO_NET_F_MAC only says whether mac is valid in the config space.
> > > > Whether guest can control that depends on VIRTIO_NET_F_CTRL_MAC_ADDR:
> > > > 
> > > > 	The VIRTIO_NET_CTRL_MAC_ADDR_SET command is used to set the default MAC address which rx
> > > > 	filtering accepts (and if VIRTIO_NET_F_MAC_ADDR has been negotiated, this will be reflected in mac in
> > > > 	config space).
> > > > 	The command-specific-data for VIRTIO_NET_CTRL_MAC_ADDR_SET is the 6-byte MAC address.
> > > 
> > > 
> > > Consider VIRTIO_NET_CTRL_MAC_ADDR_SET is not supported now. What Eli
> > > proposed here should work?
> > > 
> > > Thanks
> > > 
> > 
> > We can have management set a MAC address. Randomizing it in kernel
> > does not seem like a reasonable policy to me ...
> > 
> 
> This manangement should be the VDPA tool that Parav is pushing. We can
> use it to set a MAC chosen by the user. The mlx5 vdpa driver can then
> use that MAC instead of randomizing a value. If no admin value is given
> we can use a random MAC.

IIUC in this model devices are created by this tool, right?
Why not require the MAC when device is created?

> > 
> > > > 
> > > > 
> > > > 
> > > > 
> > 

_______________________________________________
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

[Michael S. Tsirkin]

On Thu, Dec 03, 2020 at 02:09:29PM +0200, Eli Cohen wrote:
> On Thu, Dec 03, 2020 at 05:44:17AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Dec 03, 2020 at 08:49:28AM +0200, Eli Cohen wrote:
> > > On Wed, Dec 02, 2020 at 05:00:22PM -0500, Michael S. Tsirkin wrote:
> > > > On Wed, Dec 02, 2020 at 09:48:25PM +0800, Jason Wang wrote:
> > > > > 
> > > > > On 2020/12/2 下午5:23, Michael S. Tsirkin wrote:
> > > > > > On Wed, Dec 02, 2020 at 07:57:14AM +0200, Eli Cohen wrote:
> > > > > > > On Wed, Dec 02, 2020 at 12:18:36PM +0800, Jason Wang wrote:
> > > > > > > > On 2020/12/1 下午5:23, Cindy Lu wrote:
> > > > > > > > > On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin<mst@redhat.com>  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<mst@redhat.com>  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<lulu@redhat.com>
> > > > > > > > > > > > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > > > > > > > > > > > > > > > Signed-off-by: Eli Cohen<elic@nvidia.com>
> > > > > > > > > > > > > > > > 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-saeedm@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:
> > > > > > > Thinking it over, at least in mlx5, I should always advertise
> > > > > > > VIRTIO_NET_F_MAC and set a non zero MAC value. The source of the MAC can
> > > > > > > be either randomly generated value by mlx5_vdpa or by a management tool.
> > > > > > > This is important becauase we should not let the VM modify the MAC. If
> > > > > > > we do it can set a MAC value identical to the mlx5 NIC driver and can
> > > > > > > kidnap traffic that was not destined to it.
> > > > > > > 
> > > > > > > In addition, when VIRTIO_NET_F_MAC is published, attempts to change the
> > > > > > > MAC address from the VM should result in error.
> > > > > > That is not what the spec says though.
> > > > > > VIRTIO_NET_F_MAC only says whether mac is valid in the config space.
> > > > > > Whether guest can control that depends on VIRTIO_NET_F_CTRL_MAC_ADDR:
> > > > > > 
> > > > > > 	The VIRTIO_NET_CTRL_MAC_ADDR_SET command is used to set the default MAC address which rx
> > > > > > 	filtering accepts (and if VIRTIO_NET_F_MAC_ADDR has been negotiated, this will be reflected in mac in
> > > > > > 	config space).
> > > > > > 	The command-specific-data for VIRTIO_NET_CTRL_MAC_ADDR_SET is the 6-byte MAC address.
> > > > > 
> > > > > 
> > > > > Consider VIRTIO_NET_CTRL_MAC_ADDR_SET is not supported now. What Eli
> > > > > proposed here should work?
> > > > > 
> > > > > Thanks
> > > > > 
> > > > 
> > > > We can have management set a MAC address. Randomizing it in kernel
> > > > does not seem like a reasonable policy to me ...
> > > > 
> > > 
> > > This manangement should be the VDPA tool that Parav is pushing. We can
> > > use it to set a MAC chosen by the user. The mlx5 vdpa driver can then
> > > use that MAC instead of randomizing a value. If no admin value is given
> > > we can use a random MAC.
> > 
> > IIUC in this model devices are created by this tool, right?
> > Why not require the MAC when device is created?
> > 
> 
> It is mentioned in Parav's patchset that this will be coming in a
> subsequent patch to his vdpa tool. 

So I think kernel has two options:
- require a mac when device is created, we supply it to guest
- allow guest to set a mac

it's ok to support both ...



> > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > 
> > 

_______________________________________________
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

[Jason Wang]

On 2020/12/3 下午8:24, Eli Cohen wrote:
>>> It is mentioned in Parav's patchset that this will be coming in a
>>> subsequent patch to his vdpa tool.
>> So I think kernel has two options:
>> - require a mac when device is created, we supply it to guest
> Yes, the driver should always set VIRTIO_NET_F_MAC and provide a MAC -
> either random or whatever configured using the vdpa too.


A questions here, I think current mlx5 vdpa works for VF only. So I 
think the VF should have a given MAC? If yes, can we use that MAC?

Thanks


>

_______________________________________________
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

[Michael S. Tsirkin]

On Wed, Dec 02, 2020 at 12:18:36PM +0800, Jason Wang wrote:
> 
> On 2020/12/1 下午5:23, Cindy Lu wrote:
> > On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> 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 <lulu@redhat.com>
> > > > > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > > > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > > > > 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-saeedm@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

To me this looks like a spec violation.

If the driver negotiates the VIRTIO_NET_F_MAC feature, the driver MUST set
the physical address of the NIC to \field{mac}.  Otherwise, it SHOULD
use a locally-administered MAC address (see \hyperref[intro:IEEE 802]{IEEE 802},
``9.2 48-bit universal LAN MAC addresses'').

While not said explicitly, the assumption I think is that the local
MAC is not a local one.


> 2) advertise VIRTIO_NET_F_MAC
> 3) set the random mac to vDPA through set_config

that part looks wrong to me. Setting mac through set_config was
a pre-virtio-1.0 way to send mac to device. In 1.0 we have
VIRTIO_NET_CTRL_MAC_ADDR_SET for that:


	When using the legacy interface, \field{mac} is driver-writable
	which provided a way for drivers to update the MAC without
	negotiating VIRTIO_NET_F_CTRL_MAC_ADDR.




> 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 value,
> so we're probably fine.
> 
> Thanks
> 
> 
> > 3. mlx driver it shelf should get an correct mac address while it init.
> > 4. add check in qemu get_config function  , if there is not F_MAC Then
> > ignore the mac address from mlx driver
> > 
> > not sure which method is more suitable ?
> > 
> > Thanks
> > Cindy
> > 
> > 
> > 
> > > > > > > > > > ---
> > > > > > > > > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > > > > > > > > >   1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > index 1fa6fcac8299..80d06d958b8b 100644
> > > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > @@ -1955,10 +1955,7 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)
> > > > > > > > > >        if (err)
> > > > > > > > > >                goto err_mtu;
> > > > > > > > > > 
> > > > > > > > > > -     err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> > > > > > > > > > -     if (err)
> > > > > > > > > > -             goto err_mtu;
> > > > > > > > > > -
> > > > > > > > > > +     eth_random_addr(config->mac);
> > > > > > > > > >        mvdev->vdev.dma_dev = mdev->device;
> > > > > > > > > >        err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> > > > > > > > > >        if (err)
> > > > > > > > > > --
> > > > > > > > > > 2.26.2

_______________________________________________
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

[Jason Wang]

On 2020/12/2 下午5:30, Michael S. Tsirkin wrote:
> On Wed, Dec 02, 2020 at 12:18:36PM +0800, Jason Wang wrote:
>> On 2020/12/1 下午5:23, Cindy Lu wrote:
>>> On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> 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 <lulu@redhat.com>
>>>>>>>>>>> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
>>>>>>>>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>>>>>>>> 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-saeedm@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
> To me this looks like a spec violation.
>
> If the driver negotiates the VIRTIO_NET_F_MAC feature, the driver MUST set
> the physical address of the NIC to \field{mac}.  Otherwise, it SHOULD
> use a locally-administered MAC address (see \hyperref[intro:IEEE 802]{IEEE 802},
> ``9.2 48-bit universal LAN MAC addresses'').


One question here, what did "set" mean here consider the mac is given by 
the device itself?


>
> While not said explicitly, the assumption I think is that the local
> MAC is not a local one.
>
>
>> 2) advertise VIRTIO_NET_F_MAC
>> 3) set the random mac to vDPA through set_config
> that part looks wrong to me. Setting mac through set_config was
> a pre-virtio-1.0 way to send mac to device. In 1.0 we have
> VIRTIO_NET_CTRL_MAC_ADDR_SET for that:
>
>
> 	When using the legacy interface, \field{mac} is driver-writable
> 	which provided a way for drivers to update the MAC without
> 	negotiating VIRTIO_NET_F_CTRL_MAC_ADDR.


Looks like it doesn't prevent us from doing so. Otherwise this brings an 
implicit dependency for control virtqueue if we want to support 1.0?

Thanks


>
>
>
>> 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 value,
>> so we're probably fine.
>>
>> Thanks
>>
>>
>>> 3. mlx driver it shelf should get an correct mac address while it init.
>>> 4. add check in qemu get_config function  , if there is not F_MAC Then
>>> ignore the mac address from mlx driver
>>>
>>> not sure which method is more suitable ?
>>>
>>> Thanks
>>> Cindy
>>>
>>>
>>>
>>>>>>>>>>> ---
>>>>>>>>>>>    drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
>>>>>>>>>>>    1 file changed, 1 insertion(+), 4 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>>>>>>>> index 1fa6fcac8299..80d06d958b8b 100644
>>>>>>>>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>>>>>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>>>>>>>> @@ -1955,10 +1955,7 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)
>>>>>>>>>>>         if (err)
>>>>>>>>>>>                 goto err_mtu;
>>>>>>>>>>>
>>>>>>>>>>> -     err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
>>>>>>>>>>> -     if (err)
>>>>>>>>>>> -             goto err_mtu;
>>>>>>>>>>> -
>>>>>>>>>>> +     eth_random_addr(config->mac);
>>>>>>>>>>>         mvdev->vdev.dma_dev = mdev->device;
>>>>>>>>>>>         err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
>>>>>>>>>>>         if (err)
>>>>>>>>>>> --
>>>>>>>>>>> 2.26.2

_______________________________________________
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

[Michael S. Tsirkin]

On Wed, Dec 02, 2020 at 08:56:37PM +0800, Jason Wang wrote:
> 
> On 2020/12/2 下午5:30, Michael S. Tsirkin wrote:
> > On Wed, Dec 02, 2020 at 12:18:36PM +0800, Jason Wang wrote:
> > > On 2020/12/1 下午5:23, Cindy Lu wrote:
> > > > On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> 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 <lulu@redhat.com>
> > > > > > > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > > > > > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > > > > > > 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-saeedm@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
> > To me this looks like a spec violation.
> > 
> > If the driver negotiates the VIRTIO_NET_F_MAC feature, the driver MUST set
> > the physical address of the NIC to \field{mac}.  Otherwise, it SHOULD
> > use a locally-administered MAC address (see \hyperref[intro:IEEE 802]{IEEE 802},
> > ``9.2 48-bit universal LAN MAC addresses'').
> 
> 
> One question here, what did "set" mean here consider the mac is given by the
> device itself?
> 


That is my understanding, and this seems to be what linux guests do.

> > 
> > While not said explicitly, the assumption I think is that the local
> > MAC is not a local one.
> > 
> > 
> > > 2) advertise VIRTIO_NET_F_MAC
> > > 3) set the random mac to vDPA through set_config
> > that part looks wrong to me. Setting mac through set_config was
> > a pre-virtio-1.0 way to send mac to device. In 1.0 we have
> > VIRTIO_NET_CTRL_MAC_ADDR_SET for that:
> > 
> > 
> > 	When using the legacy interface, \field{mac} is driver-writable
> > 	which provided a way for drivers to update the MAC without
> > 	negotiating VIRTIO_NET_F_CTRL_MAC_ADDR.
> 
> 
> Looks like it doesn't prevent us from doing so.

From writing into mac?
Yes it does:
	Device configuration fields are listed below, they are read-only for a driver. The \field{mac} address field
	always exists (though is only valid if VIRTIO_NET_F_MAC is set), and
	\field{status} only exists if VIRTIO_NET_F_STATUS is set.


> Otherwise this brings an
> implicit dependency for control virtqueue if we want to support 1.0?
> 
> Thanks

With 1.0 you either need VIRTIO_NET_F_CTRL_MAC_ADDR or VIRTIO_NET_F_MAC.


> 
> > 
> > 
> > 
> > > 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 value,
> > > so we're probably fine.
> > > 
> > > Thanks
> > > 
> > > 
> > > > 3. mlx driver it shelf should get an correct mac address while it init.
> > > > 4. add check in qemu get_config function  , if there is not F_MAC Then
> > > > ignore the mac address from mlx driver
> > > > 
> > > > not sure which method is more suitable ?
> > > > 
> > > > Thanks
> > > > Cindy
> > > > 
> > > > 
> > > > 
> > > > > > > > > > > > ---
> > > > > > > > > > > >    drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > > > > > > > > > > >    1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > > > > > > > > 
> > > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > > > index 1fa6fcac8299..80d06d958b8b 100644
> > > > > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > > > @@ -1955,10 +1955,7 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)
> > > > > > > > > > > >         if (err)
> > > > > > > > > > > >                 goto err_mtu;
> > > > > > > > > > > > 
> > > > > > > > > > > > -     err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> > > > > > > > > > > > -     if (err)
> > > > > > > > > > > > -             goto err_mtu;
> > > > > > > > > > > > -
> > > > > > > > > > > > +     eth_random_addr(config->mac);
> > > > > > > > > > > >         mvdev->vdev.dma_dev = mdev->device;
> > > > > > > > > > > >         err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> > > > > > > > > > > >         if (err)
> > > > > > > > > > > > --
> > > > > > > > > > > > 2.26.2

_______________________________________________
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

[Jason Wang]

On 2020/12/2 下午9:04, Michael S. Tsirkin wrote:
> On Wed, Dec 02, 2020 at 08:56:37PM +0800, Jason Wang wrote:
>> On 2020/12/2 下午5:30, Michael S. Tsirkin wrote:
>>> On Wed, Dec 02, 2020 at 12:18:36PM +0800, Jason Wang wrote:
>>>> On 2020/12/1 下午5:23, Cindy Lu wrote:
>>>>> On Mon, Nov 30, 2020 at 11:33 PM Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> 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 <lulu@redhat.com>
>>>>>>>>>>>>> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
>>>>>>>>>>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>>>>>>>>>> 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-saeedm@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
>>> To me this looks like a spec violation.
>>>
>>> If the driver negotiates the VIRTIO_NET_F_MAC feature, the driver MUST set
>>> the physical address of the NIC to \field{mac}.  Otherwise, it SHOULD
>>> use a locally-administered MAC address (see \hyperref[intro:IEEE 802]{IEEE 802},
>>> ``9.2 48-bit universal LAN MAC addresses'').
>>
>> One question here, what did "set" mean here consider the mac is given by the
>> device itself?
>>
>
> That is my understanding, and this seems to be what linux guests do.
>
>>> While not said explicitly, the assumption I think is that the local
>>> MAC is not a local one.
>>>
>>>
>>>> 2) advertise VIRTIO_NET_F_MAC
>>>> 3) set the random mac to vDPA through set_config
>>> that part looks wrong to me. Setting mac through set_config was
>>> a pre-virtio-1.0 way to send mac to device. In 1.0 we have
>>> VIRTIO_NET_CTRL_MAC_ADDR_SET for that:
>>>
>>>
>>> 	When using the legacy interface, \field{mac} is driver-writable
>>> 	which provided a way for drivers to update the MAC without
>>> 	negotiating VIRTIO_NET_F_CTRL_MAC_ADDR.
>>
>> Looks like it doesn't prevent us from doing so.
>  From writing into mac?
> Yes it does:
> 	Device configuration fields are listed below, they are read-only for a driver. The \field{mac} address field
> 	always exists (though is only valid if VIRTIO_NET_F_MAC is set), and
> 	\field{status} only exists if VIRTIO_NET_F_STATUS is set.


Okay, so this is a hint that the vDPA parent needs to check and ignore 
the write if version 1 is negotiated.


>
>
>> Otherwise this brings an
>> implicit dependency for control virtqueue if we want to support 1.0?
>>
>> Thanks
> With 1.0 you either need VIRTIO_NET_F_CTRL_MAC_ADDR or VIRTIO_NET_F_MAC.
>
>

Ok, so it looks to me VIRTIO_NET_F_MAC is the only choice for mlx5 
consider control vq is not supported.

Thanks


>>>
>>>
>>>> 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 value,
>>>> so we're probably fine.
>>>>
>>>> Thanks
>>>>
>>>>
>>>>> 3. mlx driver it shelf should get an correct mac address while it init.
>>>>> 4. add check in qemu get_config function  , if there is not F_MAC Then
>>>>> ignore the mac address from mlx driver
>>>>>
>>>>> not sure which method is more suitable ?
>>>>>
>>>>> Thanks
>>>>> Cindy
>>>>>
>>>>>
>>>>>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>     drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
>>>>>>>>>>>>>     1 file changed, 1 insertion(+), 4 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>>>>>>>>>> index 1fa6fcac8299..80d06d958b8b 100644
>>>>>>>>>>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>>>>>>>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>>>>>>>>>> @@ -1955,10 +1955,7 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)
>>>>>>>>>>>>>          if (err)
>>>>>>>>>>>>>                  goto err_mtu;
>>>>>>>>>>>>>
>>>>>>>>>>>>> -     err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
>>>>>>>>>>>>> -     if (err)
>>>>>>>>>>>>> -             goto err_mtu;
>>>>>>>>>>>>> -
>>>>>>>>>>>>> +     eth_random_addr(config->mac);
>>>>>>>>>>>>>          mvdev->vdev.dma_dev = mdev->device;
>>>>>>>>>>>>>          err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
>>>>>>>>>>>>>          if (err)
>>>>>>>>>>>>> --
>>>>>>>>>>>>> 2.26.2

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