> 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)
+
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
