On Thu, Nov 25, 2021 at 06:27:15PM +0000, Parav Pandit wrote:
>
>
> > From: Eli Cohen <elic@xxxxxxxxxx>
> > Sent: Thursday, November 25, 2021 1:32 PM
> >
> > On Thu, Nov 25, 2021 at 07:29:18AM +0200, Parav Pandit wrote:
> > >
> > >
> > > > From: Eli Cohen <elic@xxxxxxxxxx>
> > > > Sent: Wednesday, November 24, 2021 10:40 PM
> > > >
> > > > Make sure to offer VIRTIO_NET_F_MTU since we configure the MTU based
> > > > on what was queried from the device.
> > > >
> > > > This allows the virtio driver to allocate large enough buffers based
> > > > on the reported MTU.
> > > >
> > > > Signed-off-by: Eli Cohen <elic@xxxxxxxxxx>
> > > > ---
> > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > index 465e832f2ad1..ed7a63e48335 100644
> > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > @@ -1956,6 +1956,7 @@ static u64 mlx5_vdpa_get_features(struct
> > > > vdpa_device *vdev)
> > > > ndev->mvdev.mlx_features |=
> > > > BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
> > > > ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MQ);
> > > > ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
> > > > + ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MTU);
> > > It is better to set this feature bit along with the writing the RO config.mtu
> > area, adjacent to query_mtu() call.
> >
> > Why is that so important? We always query the mtu and if the query fails, we
> > fail the initialization so it does not look critical.
> It is not so important. But it is more appropriate to set read only field and its associated feature bit at one place, which never changes.
> There is no need to perform OR operation for those many feature bits on every get_features() callback when they are immutable.
>
> So I wanted to move setting other 5 feature bits assignment at one place in device initialization time similar to how you have done VALID_FEATURES_MASK.
> Something like below.
> But again, its minor.
> If you happen to revise the series (I think you should for the supporting dumpit in future), please consider one time assignment like below.
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 63813fbb5f62..21802b25b0f5 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1890,11 +1890,6 @@ static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev)
> ndev->mvdev.mlx_features |= mlx_to_vritio_features(dev_features);
> if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0))
> ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VQ);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MQ);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
>
> print_features(mvdev, ndev->mvdev.mlx_features, false);
> return ndev->mvdev.mlx_features;
> @@ -2522,6 +2517,14 @@ static int event_handler(struct notifier_block *nb, unsigned long event, void *p
> return ret;
> }
>
> +#define DEFAULT_FEATURES \
> + BIT_ULL(VIRTIO_F_ACCESS_PLATFORM) | \
> + BIT_ULL(VIRTIO_NET_F_CTRL_VQ) | \
> + BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) \
> + BIT_ULL(VIRTIO_NET_F_MQ) \
> + BIT_ULL(VIRTIO_NET_F_STATUS) \
> + BIT_ULL(VIRTIO_NET_F_MTU)
> +
I would just open-code it, don't see what good does the macro do.
A single assignment is a bit clearer I think I agree,
there's not much of a difference.
> static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> const struct vdpa_dev_set_config *add_config)
> {
> @@ -2565,6 +2568,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> goto err_mtu;
>
> ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
> + ndev->mvdev.mlx_features = DEFAULT_FEATURES;
>
> if (get_link_state(mvdev))
> ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
