Parav Pandit
2021-Mar-03 03:59 UTC
[PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address
Hi Eli,> From: Eli Cohen <elic at 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 at 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 at nvidia.com> > > > > > > > Reviewed-by: Parav Pandit <parav at nvidia.com> > > > > > > > > > > > > > > > > > > Acked-by: Jason Wang <jasowang at 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
Michael S. Tsirkin
2021-Mar-03 09:35 UTC
[PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address
On Wed, Mar 03, 2021 at 03:59:50AM +0000, Parav Pandit wrote:> Hi Eli, > > > From: Eli Cohen <elic at 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 at 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 at nvidia.com> > > > > > > > > Reviewed-by: Parav Pandit <parav at nvidia.com> > > > > > > > > > > > > > > > > > > > > > Acked-by: Jason Wang <jasowang at 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:55well 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