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 03:59:50AM +0000, 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].
> 
> 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.
> 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

well it has reported all zeros to mean "learning bridge" for a
so while I suspect changing that is an ABI change, though
a minor one ...

If you do change it please add some other way to find out
whether it still learns from outgoing traffic
(ie whether mac spoofing is enabled).

-- 
MST

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