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

["Michael S. Tsirkin" <mst@redhat.com>]

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 <elic@nvidia.com>
> > > 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 <elic@nvidia.com>
> > > > > > > > >
> > > > > > > > > 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 <elic@nvidia.com>
> > > > > > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > > > > >
> > > > > > > >
> > > > > > > > Acked-by: Jason Wang <jasowang@redhat.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 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.

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

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