> From: Michael S. Tsirkin <mst@xxxxxxxxxx> > 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@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. 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 _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
