Michael S. Tsirkin
2021-Mar-03 09:29 UTC
[PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address
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 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]. > > > 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
Parav Pandit
2021-Mar-03 10:01 UTC
[PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address
> From: Michael S. Tsirkin <mst at 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 <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]. > > > > > 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