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@xxxxxxxxxx> > > > 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@xxxxxxxxxx> > > > > > > > > > > > > > > > > > > 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@xxxxxxxxxx> > > > > > > > > > Reviewed-by: Parav Pandit <parav@xxxxxxxxxx> > > > > > > > > > > > > > > > > > > > > > > > > Acked-by: Jason Wang <jasowang@xxxxxxxxxx> > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > 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@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
