RE: [PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address

(off-list ancestor, not in this archive)

From: Michael S. Tsirkin <mst@redhat.com>
Sent: Wednesday, March 3, 2021 2:59 PM

On Wed, Mar 03, 2021 at 08:33:50AM +0200, Eli Cohen wrote:
On Wed, Mar 03, 2021 at 05:59:50AM +0200, Parav Pandit wrote:
Hi Eli,

From: Eli Cohen <redacted>
Sent: Tuesday, March 2, 2021 11:09 AM

On Mon, Mar 01, 2021 at 11:12:33AM -0500, Michael S. Tsirkin wrote:
On Mon, Mar 01, 2021 at 03:19:51PM +0200, Eli Cohen wrote:
On Mon, Mar 01, 2021 at 08:09:48AM -0500, Michael S. Tsirkin wrote:
On Mon, Mar 01, 2021 at 09:08:28AM +0200, Eli Cohen wrote:
On Wed, Feb 24, 2021 at 05:11:23PM +0800, Jason Wang wrote:
On 2021/2/24 2:18 下午, Parav Pandit wrote:
From: Eli Cohen <redacted>

Use a randomly generated MAC address to be applied in
case it is not configured by management tool.

The value queried through
mlx5_query_nic_vport_mac_address()
is not relelavnt to vdpa since it is the mac that
should be used by the regular NIC driver.

Signed-off-by: Eli Cohen <redacted>
Reviewed-by: Parav Pandit <redacted>

Acked-by: Jason Wang <redacted>


---
  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 b67bba581dfd..ece2183e7b20 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -2005,10 +2005,7 @@ static int
mlx5_vdpa_dev_add(struct
vdpa_mgmt_dev *v_mdev, const char *name)
  	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);
I think this patch is missing setting VIRTIO_NET_F_MTU. I
will post v2 with the other fixes in this series.
I don't really understand why this is a good idea.

If userspace wants a random mac it can set it, with this
patch it is impossible to know whether the mac is a hardware
one (which will be persistent e.g. across reboots) or a random
one.
You can still get a persistent MAC set by the vdpa tool. e.g

vdpa dev config set vdpa0 mac 00:11:22:33:44:55

If you don't use vdpa tool, the device assigns a random MAC.
This MAC is used to filter imcoming unicast traffic (done in a
subsequent
patch).
Well previously device learned the MAC from outgoing traffic and
used that for the filter.
No, was is added only in the last series that Parav send. Before
that the device did not filter and forwarded any packet that was
forwarded to it buy the eswitch.

Is changing that to a random value really all that useful as a
default?  Why not do the randomization in userspace?
I think we want management entity to set the MAC, that's the VDPA
tool.
So as things are right now (with the last series applied), the
vdpa tool is the tool to assign MAC addresses and if it doesn't, a
device randomly generated MAC applies. Currently MAC addresses set
by qemu command line are ignored (set_config is not implementd).
We can allow this but this will override vdpa tool configuration.
I believe its incorrect to always do random generated mac as of this
patch.
This is because, doing so ignores any admin config done by the sysadmin
on the switch (ovs side) using [1].
Well, when this patch was sent, we still had thoughts to let mlx5e NIC
and the VDPA to co-exist on the same function (VF or SF). Now that
we're off this idea you can become tempted to use the MAC address
configured for the NIC but I don't think it's a good idea. We already
have a dedicated tool to configure MAC for VDPA so let's use it.
Right. And users can decide to reuse the hardware MAC if they want to.
Right.
If user chose to reuse the hw mac set by the eswitch, so let them use it.
When smartnic is used, some users do not trust compute side for network attributes configuration.
So those users to configure the MAC from eswitch along with ovs policy.
I think Sean gave one example of it.

And user can choose to override it via vdpa tool/qemu.

I am not sure doing only one_way fits all.
Switch/ovs side configuring the mac along with policy seems fine to me.
But this may not fit the case for those who doesn't have eswitch.
Jason offline or in previous thread mentioned a use case to create multiple vdpa device from single PF/VF.
At Mellanox at least we don't see that use case at moment given its attached to eswitch.


So if user choose to configure using eswitch config, mlx5_vnet to honor
that.
And if user prefers to override is using vdpa tool or set_config from
QEMU side, so be it.
I agree that we should let the user to configure the MAC through qemu
argument when booting the VM. So I'll add this for the next spin of
this series.
OK so
- if MAC is configured it is used
- if not configured 0 is reported to userspace

fair summary?
LGTM.
Eli?

Hence, instead of reporting all zeros, mlx5 should query own vport
context and publish that mac in the config layout and rx steering side.
If user choose to override it, config layout and rx rules will have to
updated on such config.
[1] devlink port function set pci/0000:03:00.0/<port_id_of_sf/vf>/
hw_addr 00:11:22:33:44:55
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help