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

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

[Eli Cohen]

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>
---
 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

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

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

[Eli Cohen]

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.

> 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/
 
> > ---
> >  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
> 

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
> > 

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

[Eli Cohen]

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.

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

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

> 
> > > > ---
> > > >  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
> > > 
> 

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
> > > > 
> > 

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

[Cindy Lu]

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.

> > >
> > > > > > ---
> > > > > >  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
> > > > >
> > >
>

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
> > > > > >
> > > >
> >

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

[Cindy Lu]

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

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
> > > > > > >
> > > > >
> > >
>

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
> > > > > > > >
> > > > > >
> > > >
> >

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

[Cindy Lu]

On Tue, Dec 1, 2020 at 7:32 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> 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?
>
I have tired, but guest will use it, here is some information from guest
[root@localhost ~]# cat /sys/bus/pci/devices/0000\:00\:04.0/virtio0/features
1110010000011000111100010000000010000000000000000000000000000000
[root@localhost ~]# ifconfig -a
eth0: flags=4098<BROADCAST,MULTICAST>  mtu 1500
        ether 00:00:00:00:00:00  txqueuelen 1000  (Ethernet)
        RX packets 0  bytes 0 (0.0 B)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 0  bytes 0 (0.0 B)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
For my understanding, this NIC should have MAC address by default ,
and this should not depend on
any other function/use case,for driver itself it's incorrect to give
an mac address with all 0 to  qemu without error/fail

Thanks
Cindy
> > 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
> > > > > > > > >
> > > > > > >
> > > > >
> > >
>

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

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

[Eli Cohen]

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.


> 
> 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
> 

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
> > 

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

[Eli Cohen]

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?

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

> 
> 
> 
> 
> > 
> > > 
> > > 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
> > > 
> 

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
> > > > 
> > 

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.
>

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.
> > 

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

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


>
>
>
>

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 ...


> > 
> > 
> > 
> > 

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

[Eli Cohen]

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.

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

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?

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

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

[Eli Cohen]

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. 

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

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 ...



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

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

[Eli Cohen]

On Thu, Dec 03, 2020 at 07:15:37AM -0500, Michael S. Tsirkin wrote:
> 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

Yes, the driver should always set VIRTIO_NET_F_MAC and provide a MAC -
either random or whatever configured using the vdpa too.

> - allow guest to set a mac
> 

This cannot work today. Moreover, if's a problem to allow the guest to
set the MAC becuase it can take over the regular mlx5 NIC's address.

> it's ok to support both ...
> 
> 
> 
> > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > 
> > > 
> 

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


>

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

[Eli Cohen]

On Fri, Dec 04, 2020 at 10:53:28AM +0800, Jason Wang wrote:
> 
> 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?
> 
The MAC assigned to VF is by the NIC implementation. Both ther regular
NIC driver and the VDPA implementation can co-exist so we can't use the
NIC's MAC for VDPA. We want to steer traffic based on its destination
MAC address to either VDPA or regular NIC.

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

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

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

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

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

[Eli Cohen]

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.

> > > - 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.

> > > 
> > > > > > ---
> > > > > >  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
> > > > > 
> > > 
> 

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
> > > > > > 
> > > > 
> >