* Re: [PATCH v1 1/7] vdpa: Provide interface to read driver features
[not found] ` <20211208201430.73720-2-elic@nvidia.com>
@ 2021-12-08 23:57 ` Si-Wei Liu
[not found] ` <20211209064702.GA108239@mtl-vdi-166.wap.labs.mlnx>
2021-12-09 5:33 ` [PATCH v1 1/7] vdpa: Provide interface to read driver features Jason Wang
1 sibling, 1 reply; 22+ messages in thread
From: Si-Wei Liu @ 2021-12-08 23:57 UTC (permalink / raw)
To: Eli Cohen, mst, jasowang, virtualization; +Cc: lvivier, eperezma
On 12/8/2021 12:14 PM, Eli Cohen wrote:
> Provide an interface to read the negotiated features. This is needed
> when building the netlink message in vdpa_dev_net_config_fill().
>
> Also fix the implementation of vdpa_dev_net_config_fill() to use the
> negotiated features instead of the device features.
Are we sure the use of device feature is a bug rather than expected
behavior? How do users determine the configured number of queue pairs at
any point in case of a non-multiqueue supporting guest/driver or that
the device is being reset (where actual_features == 0)? Maybe we should
differentiate the static device config against driver/device running
state here. I thought the change to vdpa_dev_net_config_fill() in this
patch would break vdpa tool user's expectation that the value of max_vqp
config is a fixed value since the vdpa creation time.
Perhaps worth adding another attribute to represent the running state
(cur_max_qps) based on the negotiated features.
-Siwei
>
> To make APIs clearer, make the following name changes to struct
> vdpa_config_ops so they better describe their operations:
>
> get_features -> get_device_features
> set_features -> set_driver_features
>
> Finally, add get_driver_features to return the negotiated features and
> add implementation to all the upstream drivers.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
> drivers/vdpa/alibaba/eni_vdpa.c | 16 ++++++++++++----
> drivers/vdpa/ifcvf/ifcvf_main.c | 16 ++++++++++++----
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 16 ++++++++++++----
> drivers/vdpa/vdpa.c | 2 +-
> drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +++++++++++++++------
> drivers/vdpa/vdpa_user/vduse_dev.c | 16 ++++++++++++----
> drivers/vdpa/virtio_pci/vp_vdpa.c | 16 ++++++++++++----
> drivers/vhost/vdpa.c | 2 +-
> drivers/virtio/virtio_vdpa.c | 2 +-
> include/linux/vdpa.h | 14 +++++++++-----
> 10 files changed, 87 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c
> index 3f788794571a..ac0975660f4d 100644
> --- a/drivers/vdpa/alibaba/eni_vdpa.c
> +++ b/drivers/vdpa/alibaba/eni_vdpa.c
> @@ -58,7 +58,7 @@ static struct virtio_pci_legacy_device *vdpa_to_ldev(struct vdpa_device *vdpa)
> return &eni_vdpa->ldev;
> }
>
> -static u64 eni_vdpa_get_features(struct vdpa_device *vdpa)
> +static u64 eni_vdpa_get_device_features(struct vdpa_device *vdpa)
> {
> struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
> u64 features = vp_legacy_get_features(ldev);
> @@ -69,7 +69,7 @@ static u64 eni_vdpa_get_features(struct vdpa_device *vdpa)
> return features;
> }
>
> -static int eni_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> +static int eni_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
> {
> struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
>
> @@ -84,6 +84,13 @@ static int eni_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> return 0;
> }
>
> +static u64 eni_vdpa_get_driver_features(struct vdpa_device *vdpa)
> +{
> + struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
> +
> + return vp_legacy_get_driver_features(ldev);
> +}
> +
> static u8 eni_vdpa_get_status(struct vdpa_device *vdpa)
> {
> struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
> @@ -401,8 +408,9 @@ static void eni_vdpa_set_config_cb(struct vdpa_device *vdpa,
> }
>
> static const struct vdpa_config_ops eni_vdpa_ops = {
> - .get_features = eni_vdpa_get_features,
> - .set_features = eni_vdpa_set_features,
> + .get_device_features = eni_vdpa_get_device_features,
> + .set_driver_features = eni_vdpa_set_driver_features,
> + .get_driver_features = eni_vdpa_get_driver_features,
> .get_status = eni_vdpa_get_status,
> .set_status = eni_vdpa_set_status,
> .reset = eni_vdpa_reset,
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 6dc75ca70b37..6a6a0a462392 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -169,7 +169,7 @@ static struct ifcvf_hw *vdpa_to_vf(struct vdpa_device *vdpa_dev)
> return &adapter->vf;
> }
>
> -static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev)
> +static u64 ifcvf_vdpa_get_device_features(struct vdpa_device *vdpa_dev)
> {
> struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> @@ -187,7 +187,7 @@ static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev)
> return features;
> }
>
> -static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, u64 features)
> +static int ifcvf_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 features)
> {
> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> int ret;
> @@ -201,6 +201,13 @@ static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, u64 features)
> return 0;
> }
>
> +static u64 ifcvf_vdpa_get_driver_features(struct vdpa_device *vdpa_dev)
> +{
> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> +
> + return vf->req_features;
> +}
> +
> static u8 ifcvf_vdpa_get_status(struct vdpa_device *vdpa_dev)
> {
> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> @@ -443,8 +450,9 @@ static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_devic
> * implemented set_map()/dma_map()/dma_unmap()
> */
> static const struct vdpa_config_ops ifc_vdpa_ops = {
> - .get_features = ifcvf_vdpa_get_features,
> - .set_features = ifcvf_vdpa_set_features,
> + .get_device_features = ifcvf_vdpa_get_device_features,
> + .set_driver_features = ifcvf_vdpa_set_driver_features,
> + .get_driver_features = ifcvf_vdpa_get_driver_features,
> .get_status = ifcvf_vdpa_get_status,
> .set_status = ifcvf_vdpa_set_status,
> .reset = ifcvf_vdpa_reset,
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 63813fbb5f62..ce2e13135dd8 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1880,7 +1880,7 @@ static u64 mlx_to_vritio_features(u16 dev_features)
> return result;
> }
>
> -static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev)
> +static u64 mlx5_vdpa_get_device_features(struct vdpa_device *vdev)
> {
> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> @@ -1972,7 +1972,7 @@ static void update_cvq_info(struct mlx5_vdpa_dev *mvdev)
> }
> }
>
> -static int mlx5_vdpa_set_features(struct vdpa_device *vdev, u64 features)
> +static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> {
> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> @@ -2339,6 +2339,13 @@ static int mlx5_get_vq_irq(struct vdpa_device *vdv, u16 idx)
> return -EOPNOTSUPP;
> }
>
> +static u64 mlx5_vdpa_get_driver_features(struct vdpa_device *vdev)
> +{
> + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> +
> + return mvdev->actual_features;
> +}
> +
> static const struct vdpa_config_ops mlx5_vdpa_ops = {
> .set_vq_address = mlx5_vdpa_set_vq_address,
> .set_vq_num = mlx5_vdpa_set_vq_num,
> @@ -2351,8 +2358,9 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> .get_vq_notification = mlx5_get_vq_notification,
> .get_vq_irq = mlx5_get_vq_irq,
> .get_vq_align = mlx5_vdpa_get_vq_align,
> - .get_features = mlx5_vdpa_get_features,
> - .set_features = mlx5_vdpa_set_features,
> + .get_device_features = mlx5_vdpa_get_device_features,
> + .set_driver_features = mlx5_vdpa_set_driver_features,
> + .get_driver_features = mlx5_vdpa_get_driver_features,
> .set_config_cb = mlx5_vdpa_set_config_cb,
> .get_vq_num_max = mlx5_vdpa_get_vq_num_max,
> .get_device_id = mlx5_vdpa_get_device_id,
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 7332a74a4b00..c37d384c0f33 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -733,7 +733,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
> return -EMSGSIZE;
>
> - features = vdev->config->get_features(vdev);
> + features = vdev->config->get_driver_features(vdev);
>
> return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> }
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index 41b0cd17fcba..ddbe142af09a 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -399,14 +399,14 @@ static u32 vdpasim_get_vq_align(struct vdpa_device *vdpa)
> return VDPASIM_QUEUE_ALIGN;
> }
>
> -static u64 vdpasim_get_features(struct vdpa_device *vdpa)
> +static u64 vdpasim_get_device_features(struct vdpa_device *vdpa)
> {
> struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>
> return vdpasim->dev_attr.supported_features;
> }
>
> -static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
> +static int vdpasim_set_driver_features(struct vdpa_device *vdpa, u64 features)
> {
> struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>
> @@ -419,6 +419,13 @@ static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
> return 0;
> }
>
> +static u64 vdpasim_get_driver_features(struct vdpa_device *vdpa)
> +{
> + struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> +
> + return vdpasim->features;
> +}
> +
> static void vdpasim_set_config_cb(struct vdpa_device *vdpa,
> struct vdpa_callback *cb)
> {
> @@ -613,8 +620,9 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
> .set_vq_state = vdpasim_set_vq_state,
> .get_vq_state = vdpasim_get_vq_state,
> .get_vq_align = vdpasim_get_vq_align,
> - .get_features = vdpasim_get_features,
> - .set_features = vdpasim_set_features,
> + .get_device_features = vdpasim_get_device_features,
> + .set_driver_features = vdpasim_set_driver_features,
> + .get_driver_features = vdpasim_get_driver_features,
> .set_config_cb = vdpasim_set_config_cb,
> .get_vq_num_max = vdpasim_get_vq_num_max,
> .get_device_id = vdpasim_get_device_id,
> @@ -642,8 +650,9 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
> .set_vq_state = vdpasim_set_vq_state,
> .get_vq_state = vdpasim_get_vq_state,
> .get_vq_align = vdpasim_get_vq_align,
> - .get_features = vdpasim_get_features,
> - .set_features = vdpasim_set_features,
> + .get_device_features = vdpasim_get_device_features,
> + .set_driver_features = vdpasim_set_driver_features,
> + .get_driver_features = vdpasim_get_driver_features,
> .set_config_cb = vdpasim_set_config_cb,
> .get_vq_num_max = vdpasim_get_vq_num_max,
> .get_device_id = vdpasim_get_device_id,
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index c9204c62f339..1e65af6ab9ae 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -573,14 +573,14 @@ static u32 vduse_vdpa_get_vq_align(struct vdpa_device *vdpa)
> return dev->vq_align;
> }
>
> -static u64 vduse_vdpa_get_features(struct vdpa_device *vdpa)
> +static u64 vduse_vdpa_get_device_features(struct vdpa_device *vdpa)
> {
> struct vduse_dev *dev = vdpa_to_vduse(vdpa);
>
> return dev->device_features;
> }
>
> -static int vduse_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> +static int vduse_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
> {
> struct vduse_dev *dev = vdpa_to_vduse(vdpa);
>
> @@ -588,6 +588,13 @@ static int vduse_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> return 0;
> }
>
> +static u64 vduse_vdpa_get_driver_features(struct vdpa_device *vdpa)
> +{
> + struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> +
> + return dev->driver_features;
> +}
> +
> static void vduse_vdpa_set_config_cb(struct vdpa_device *vdpa,
> struct vdpa_callback *cb)
> {
> @@ -720,8 +727,9 @@ static const struct vdpa_config_ops vduse_vdpa_config_ops = {
> .set_vq_state = vduse_vdpa_set_vq_state,
> .get_vq_state = vduse_vdpa_get_vq_state,
> .get_vq_align = vduse_vdpa_get_vq_align,
> - .get_features = vduse_vdpa_get_features,
> - .set_features = vduse_vdpa_set_features,
> + .get_device_features = vduse_vdpa_get_device_features,
> + .set_driver_features = vduse_vdpa_set_driver_features,
> + .get_driver_features = vduse_vdpa_get_driver_features,
> .set_config_cb = vduse_vdpa_set_config_cb,
> .get_vq_num_max = vduse_vdpa_get_vq_num_max,
> .get_device_id = vduse_vdpa_get_device_id,
> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
> index e3ff7875e123..97285bc05e47 100644
> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> @@ -53,14 +53,14 @@ static struct virtio_pci_modern_device *vdpa_to_mdev(struct vdpa_device *vdpa)
> return &vp_vdpa->mdev;
> }
>
> -static u64 vp_vdpa_get_features(struct vdpa_device *vdpa)
> +static u64 vp_vdpa_get_device_features(struct vdpa_device *vdpa)
> {
> struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
>
> return vp_modern_get_features(mdev);
> }
>
> -static int vp_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> +static int vp_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
> {
> struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
>
> @@ -69,6 +69,13 @@ static int vp_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> return 0;
> }
>
> +static u64 vp_vdpa_get_driver_features(struct vdpa_device *vdpa)
> +{
> + struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
> +
> + return vp_modern_get_features(mdev);
> +}
> +
> static u8 vp_vdpa_get_status(struct vdpa_device *vdpa)
> {
> struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
> @@ -415,8 +422,9 @@ vp_vdpa_get_vq_notification(struct vdpa_device *vdpa, u16 qid)
> }
>
> static const struct vdpa_config_ops vp_vdpa_ops = {
> - .get_features = vp_vdpa_get_features,
> - .set_features = vp_vdpa_set_features,
> + .get_device_features = vp_vdpa_get_device_features,
> + .set_driver_features = vp_vdpa_set_driver_features,
> + .get_driver_features = vp_vdpa_get_driver_features,
> .get_status = vp_vdpa_get_status,
> .set_status = vp_vdpa_set_status,
> .reset = vp_vdpa_reset,
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 29cced1cd277..6d087a4fb581 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -262,7 +262,7 @@ static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
> const struct vdpa_config_ops *ops = vdpa->config;
> u64 features;
>
> - features = ops->get_features(vdpa);
> + features = ops->get_device_features(vdpa);
>
> if (copy_to_user(featurep, &features, sizeof(features)))
> return -EFAULT;
> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> index f85f860bc10b..a84b04ba3195 100644
> --- a/drivers/virtio/virtio_vdpa.c
> +++ b/drivers/virtio/virtio_vdpa.c
> @@ -308,7 +308,7 @@ static u64 virtio_vdpa_get_features(struct virtio_device *vdev)
> struct vdpa_device *vdpa = vd_get_vdpa(vdev);
> const struct vdpa_config_ops *ops = vdpa->config;
>
> - return ops->get_features(vdpa);
> + return ops->get_device_features(vdpa);
> }
>
> static int virtio_vdpa_finalize_features(struct virtio_device *vdev)
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index c3011ccda430..db24317d61b6 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -169,14 +169,17 @@ struct vdpa_map_file {
> * for the device
> * @vdev: vdpa device
> * Returns virtqueue algin requirement
> - * @get_features: Get virtio features supported by the device
> + * @get_device_features: Get virtio features supported by the device
> * @vdev: vdpa device
> * Returns the virtio features support by the
> * device
> - * @set_features: Set virtio features supported by the driver
> + * @set_driver_features: Set virtio features supported by the driver
> * @vdev: vdpa device
> * @features: feature support by the driver
> * Returns integer: success (0) or error (< 0)
> + * @get_driver_features: Get virtio features in action
> + * @vdev: vdpa device
> + * Returns the virtio features accepted
> * @set_config_cb: Set the config interrupt callback
> * @vdev: vdpa device
> * @cb: virtio-vdev interrupt callback structure
> @@ -276,8 +279,9 @@ struct vdpa_config_ops {
>
> /* Device ops */
> u32 (*get_vq_align)(struct vdpa_device *vdev);
> - u64 (*get_features)(struct vdpa_device *vdev);
> - int (*set_features)(struct vdpa_device *vdev, u64 features);
> + u64 (*get_device_features)(struct vdpa_device *vdev);
> + int (*set_driver_features)(struct vdpa_device *vdev, u64 features);
> + u64 (*get_driver_features)(struct vdpa_device *vdev);
> void (*set_config_cb)(struct vdpa_device *vdev,
> struct vdpa_callback *cb);
> u16 (*get_vq_num_max)(struct vdpa_device *vdev);
> @@ -395,7 +399,7 @@ static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
> const struct vdpa_config_ops *ops = vdev->config;
>
> vdev->features_valid = true;
> - return ops->set_features(vdev, features);
> + return ops->set_driver_features(vdev, features);
> }
>
> void vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 2/7] vdpa/mlx5: Distribute RX virtqueues in RQT object
[not found] ` <20211208201430.73720-3-elic@nvidia.com>
@ 2021-12-09 0:12 ` Si-Wei Liu
[not found] ` <20211209065504.GB108239@mtl-vdi-166.wap.labs.mlnx>
0 siblings, 1 reply; 22+ messages in thread
From: Si-Wei Liu @ 2021-12-09 0:12 UTC (permalink / raw)
To: Eli Cohen, mst, jasowang, virtualization; +Cc: lvivier, eperezma
On 12/8/2021 12:14 PM, Eli Cohen wrote:
> Distribute the available rx virtqueues amongst the available RQT
> entries.
>
> RQTs require to have a power of two entries. When creating or modifying
> the RQT, use the lowest number of power of two entries that is not less
> than the number of rx virtqueues. Distribute them in the available
> entries such that some virtqueus may be referenced twice.
>
> This allows to configure any number of virtqueue pairs when multiqueue
> is used.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 30 +++++++-----------------------
> 1 file changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index ce2e13135dd8..e1a8a790f213 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1261,17 +1261,10 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
> MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
> list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> - for (i = 0, j = 0; j < max_rqt; j++) {
> - if (!ndev->vqs[j].initialized)
> - continue;
Why the !initialized check is dropped from the new code?
> -
> - if (!vq_is_tx(ndev->vqs[j].index)) {
> - list[i] = cpu_to_be32(ndev->vqs[j].virtq_id);
> - i++;
> - }
> - }
> - MLX5_SET(rqtc, rqtc, rqt_actual_size, i);
> + for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> + list[i] = cpu_to_be32(ndev->vqs[j % ndev->mvdev.max_vqs].virtq_id);
>
> + MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);
> kfree(in);
> if (err)
> @@ -1292,7 +1285,7 @@ static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
> int i, j;
> int err;
>
> - max_rqt = min_t(int, ndev->cur_num_vqs / 2,
> + max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
> 1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> if (max_rqt < 1)
> return -EOPNOTSUPP;
> @@ -1308,16 +1301,10 @@ static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
> MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
>
> list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> - for (i = 0, j = 0; j < num; j++) {
> - if (!ndev->vqs[j].initialized)
> - continue;
Ditto.
-Siwei
> + for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> + list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
>
> - if (!vq_is_tx(ndev->vqs[j].index)) {
> - list[i] = cpu_to_be32(ndev->vqs[j].virtq_id);
> - i++;
> - }
> - }
> - MLX5_SET(rqtc, rqtc, rqt_actual_size, i);
> + MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, ndev->res.rqtn);
> kfree(in);
> if (err)
> @@ -1581,9 +1568,6 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> break;
> }
>
> - if (newqps & (newqps - 1))
> - break;
> -
> if (!change_num_qps(mvdev, newqps))
> status = VIRTIO_NET_OK;
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 3/7] vdpa: Allow to configure max data virtqueues
[not found] ` <20211208201430.73720-4-elic@nvidia.com>
@ 2021-12-09 0:25 ` Si-Wei Liu
2021-12-09 5:36 ` Jason Wang
0 siblings, 1 reply; 22+ messages in thread
From: Si-Wei Liu @ 2021-12-09 0:25 UTC (permalink / raw)
To: Eli Cohen, mst, jasowang, virtualization; +Cc: lvivier, eperezma
On 12/8/2021 12:14 PM, Eli Cohen wrote:
> Add netlink support to configure the max virtqueue pairs for a device.
> At least one pair is required. The maximum is dictated by the device.
>
> Example:
>
> $ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 max_vqp 5
Not this patch, but I think there should be a mega attribute defined
ahead to specify the virtio class/type to create vdpa instance with.
Like the existing ones e.g. mac_addr and mtu, max_vqp is also vdpa net
specific attribute.
-Siwei
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
> v0 -> v1:
> 1. fix typo
> 2. move max_vq_pairs to net specific struct
>
> drivers/vdpa/vdpa.c | 14 +++++++++++++-
> include/linux/vdpa.h | 1 +
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index c37d384c0f33..3bf016e03512 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -480,7 +480,8 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
> }
>
> #define VDPA_DEV_NET_ATTRS_MASK ((1 << VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
> - (1 << VDPA_ATTR_DEV_NET_CFG_MTU))
> + (1 << VDPA_ATTR_DEV_NET_CFG_MTU) | \
> + (1 << VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
>
> static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *info)
> {
> @@ -506,6 +507,17 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
> nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]);
> config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MTU);
> }
> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]) {
> + config.net.max_vq_pairs =
> + nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]);
> + if (!config.net.max_vq_pairs) {
> + NL_SET_ERR_MSG_MOD(info->extack,
> + "At least one pair of VQs is required");
> + err = -EINVAL;
> + goto err;
> + }
> + config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
> + }
>
> /* Skip checking capability if user didn't prefer to configure any
> * device networking attributes. It is likely that user might have used
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index db24317d61b6..b62032573780 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -99,6 +99,7 @@ struct vdpa_dev_set_config {
> struct {
> u8 mac[ETH_ALEN];
> u16 mtu;
> + u16 max_vq_pairs;
> } net;
> u64 mask;
> };
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 6/7] vdpa: Add support for querying control virtqueue index
[not found] ` <20211208201430.73720-7-elic@nvidia.com>
@ 2021-12-09 0:59 ` Si-Wei Liu
2021-12-09 5:44 ` Jason Wang
1 sibling, 0 replies; 22+ messages in thread
From: Si-Wei Liu @ 2021-12-09 0:59 UTC (permalink / raw)
To: Eli Cohen, mst, jasowang, virtualization; +Cc: lvivier, eperezma
On 12/8/2021 12:14 PM, Eli Cohen wrote:
> Add netlink attribute and callback function to query the control VQ
> index of a device.
>
> Example:
>
> $ vdpa dev config show vdpa-a
> vdpa-a: mac 00:00:00:00:88:88 link up link_announce false max_vq_pairs 5 \
> mtu 9000 ctrl_vq_idx 10
First, I am not sure if there's value or a case showing that expose and
trace the guest ctrl_vq_idx value (running state) to admin users turns
out to be useful. Previously I thought you want to expose it to QEMU but
it seems this is a bit redundant, which can be deduced from max_vqp
passing up to QEMU. Second, I don't feel running states such as
link_announce and ctrl_vq_idx are qualified to be configuration field
that can be displayed in 'vdpa dev config show'. Could you please
clarify the scope for what kind of info this command should cover?
-Siwei
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
> v0 -> v1:
> 1. Use logic defined in the spec to deduce virtqueue index.
>
> drivers/vdpa/vdpa.c | 25 +++++++++++++++++++++++++
> include/uapi/linux/vdpa.h | 1 +
> 2 files changed, 26 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 3bf016e03512..b4d4b8a7ca4e 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -712,6 +712,27 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct sk_buff *msg, struct netlink_callba
> return msg->len;
> }
>
> +static int vdpa_dev_net_ctrl_vq_fill(struct vdpa_device *vdev,
> + struct sk_buff *msg,
> + struct virtio_net_config *config,
> + u64 features)
> +{
> + u16 N;
> +
> + /* control VQ index, if available, is deduced according to the logic
> + * described in the virtio spec in section 5.1.2
> + */
> + if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> + return 0;
> +
> + if (features & BIT_ULL(VIRTIO_NET_F_MQ))
> + N = le16_to_cpu(config->max_virtqueue_pairs);
> + else
> + N = 1;
> +
> + return nla_put_u16(msg, VDPA_ATTR_DEV_CTRL_VQ_IDX, 2 * N);
> +}
> +
> static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
> struct sk_buff *msg, u64 features,
> const struct virtio_net_config *config)
> @@ -730,6 +751,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> struct virtio_net_config config = {};
> u64 features;
> u16 val_u16;
> + int err;
>
> vdpa_get_config(vdev, 0, &config, sizeof(config));
>
> @@ -746,6 +768,9 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> return -EMSGSIZE;
>
> features = vdev->config->get_driver_features(vdev);
> + err = vdpa_dev_net_ctrl_vq_fill(vdev, msg, &config, features);
> + if (err)
> + return err;
>
> return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> }
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index a252f06f9dfd..2e3a7f89f42d 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -34,6 +34,7 @@ enum vdpa_attr {
> VDPA_ATTR_DEV_MAX_VQS, /* u32 */
> VDPA_ATTR_DEV_MAX_VQ_SIZE, /* u16 */
> VDPA_ATTR_DEV_MIN_VQ_SIZE, /* u16 */
> + VDPA_ATTR_DEV_CTRL_VQ_IDX, /* u16 */
>
> VDPA_ATTR_DEV_NET_CFG_MACADDR, /* binary */
> VDPA_ATTR_DEV_NET_STATUS, /* u8 */
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/7] vdpa: Provide interface to read driver features
[not found] ` <20211208201430.73720-2-elic@nvidia.com>
2021-12-08 23:57 ` [PATCH v1 1/7] vdpa: Provide interface to read driver features Si-Wei Liu
@ 2021-12-09 5:33 ` Jason Wang
[not found] ` <20211209070654.GD108239@mtl-vdi-166.wap.labs.mlnx>
1 sibling, 1 reply; 22+ messages in thread
From: Jason Wang @ 2021-12-09 5:33 UTC (permalink / raw)
To: Eli Cohen; +Cc: Laurent Vivier, mst, virtualization, eperezma, Si-Wei Liu
On Thu, Dec 9, 2021 at 4:14 AM Eli Cohen <elic@nvidia.com> wrote:
>
> Provide an interface to read the negotiated features. This is needed
> when building the netlink message in vdpa_dev_net_config_fill().
>
> Also fix the implementation of vdpa_dev_net_config_fill() to use the
> negotiated features instead of the device features.
>
> To make APIs clearer, make the following name changes to struct
> vdpa_config_ops so they better describe their operations:
>
> get_features -> get_device_features
> set_features -> set_driver_features
>
> Finally, add get_driver_features to return the negotiated features and
> add implementation to all the upstream drivers.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
> drivers/vdpa/alibaba/eni_vdpa.c | 16 ++++++++++++----
> drivers/vdpa/ifcvf/ifcvf_main.c | 16 ++++++++++++----
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 16 ++++++++++++----
> drivers/vdpa/vdpa.c | 2 +-
> drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +++++++++++++++------
> drivers/vdpa/vdpa_user/vduse_dev.c | 16 ++++++++++++----
> drivers/vdpa/virtio_pci/vp_vdpa.c | 16 ++++++++++++----
> drivers/vhost/vdpa.c | 2 +-
> drivers/virtio/virtio_vdpa.c | 2 +-
> include/linux/vdpa.h | 14 +++++++++-----
> 10 files changed, 87 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c
> index 3f788794571a..ac0975660f4d 100644
> --- a/drivers/vdpa/alibaba/eni_vdpa.c
> +++ b/drivers/vdpa/alibaba/eni_vdpa.c
> @@ -58,7 +58,7 @@ static struct virtio_pci_legacy_device *vdpa_to_ldev(struct vdpa_device *vdpa)
> return &eni_vdpa->ldev;
> }
>
> -static u64 eni_vdpa_get_features(struct vdpa_device *vdpa)
> +static u64 eni_vdpa_get_device_features(struct vdpa_device *vdpa)
> {
> struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
> u64 features = vp_legacy_get_features(ldev);
> @@ -69,7 +69,7 @@ static u64 eni_vdpa_get_features(struct vdpa_device *vdpa)
> return features;
> }
>
> -static int eni_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> +static int eni_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
> {
> struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
>
> @@ -84,6 +84,13 @@ static int eni_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> return 0;
> }
>
> +static u64 eni_vdpa_get_driver_features(struct vdpa_device *vdpa)
> +{
> + struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
> +
> + return vp_legacy_get_driver_features(ldev);
> +}
> +
> static u8 eni_vdpa_get_status(struct vdpa_device *vdpa)
> {
> struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
> @@ -401,8 +408,9 @@ static void eni_vdpa_set_config_cb(struct vdpa_device *vdpa,
> }
>
> static const struct vdpa_config_ops eni_vdpa_ops = {
> - .get_features = eni_vdpa_get_features,
> - .set_features = eni_vdpa_set_features,
> + .get_device_features = eni_vdpa_get_device_features,
> + .set_driver_features = eni_vdpa_set_driver_features,
> + .get_driver_features = eni_vdpa_get_driver_features,
> .get_status = eni_vdpa_get_status,
> .set_status = eni_vdpa_set_status,
> .reset = eni_vdpa_reset,
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 6dc75ca70b37..6a6a0a462392 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -169,7 +169,7 @@ static struct ifcvf_hw *vdpa_to_vf(struct vdpa_device *vdpa_dev)
> return &adapter->vf;
> }
>
> -static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev)
> +static u64 ifcvf_vdpa_get_device_features(struct vdpa_device *vdpa_dev)
> {
> struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> @@ -187,7 +187,7 @@ static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev)
> return features;
> }
>
> -static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, u64 features)
> +static int ifcvf_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 features)
> {
> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> int ret;
> @@ -201,6 +201,13 @@ static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, u64 features)
> return 0;
> }
>
> +static u64 ifcvf_vdpa_get_driver_features(struct vdpa_device *vdpa_dev)
> +{
> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> +
> + return vf->req_features;
> +}
> +
> static u8 ifcvf_vdpa_get_status(struct vdpa_device *vdpa_dev)
> {
> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> @@ -443,8 +450,9 @@ static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_devic
> * implemented set_map()/dma_map()/dma_unmap()
> */
> static const struct vdpa_config_ops ifc_vdpa_ops = {
> - .get_features = ifcvf_vdpa_get_features,
> - .set_features = ifcvf_vdpa_set_features,
> + .get_device_features = ifcvf_vdpa_get_device_features,
> + .set_driver_features = ifcvf_vdpa_set_driver_features,
> + .get_driver_features = ifcvf_vdpa_get_driver_features,
> .get_status = ifcvf_vdpa_get_status,
> .set_status = ifcvf_vdpa_set_status,
> .reset = ifcvf_vdpa_reset,
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 63813fbb5f62..ce2e13135dd8 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1880,7 +1880,7 @@ static u64 mlx_to_vritio_features(u16 dev_features)
> return result;
> }
>
> -static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev)
> +static u64 mlx5_vdpa_get_device_features(struct vdpa_device *vdev)
> {
> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> @@ -1972,7 +1972,7 @@ static void update_cvq_info(struct mlx5_vdpa_dev *mvdev)
> }
> }
>
> -static int mlx5_vdpa_set_features(struct vdpa_device *vdev, u64 features)
> +static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> {
> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> @@ -2339,6 +2339,13 @@ static int mlx5_get_vq_irq(struct vdpa_device *vdv, u16 idx)
> return -EOPNOTSUPP;
> }
>
> +static u64 mlx5_vdpa_get_driver_features(struct vdpa_device *vdev)
> +{
> + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> +
> + return mvdev->actual_features;
> +}
> +
> static const struct vdpa_config_ops mlx5_vdpa_ops = {
> .set_vq_address = mlx5_vdpa_set_vq_address,
> .set_vq_num = mlx5_vdpa_set_vq_num,
> @@ -2351,8 +2358,9 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> .get_vq_notification = mlx5_get_vq_notification,
> .get_vq_irq = mlx5_get_vq_irq,
> .get_vq_align = mlx5_vdpa_get_vq_align,
> - .get_features = mlx5_vdpa_get_features,
> - .set_features = mlx5_vdpa_set_features,
> + .get_device_features = mlx5_vdpa_get_device_features,
> + .set_driver_features = mlx5_vdpa_set_driver_features,
> + .get_driver_features = mlx5_vdpa_get_driver_features,
> .set_config_cb = mlx5_vdpa_set_config_cb,
> .get_vq_num_max = mlx5_vdpa_get_vq_num_max,
> .get_device_id = mlx5_vdpa_get_device_id,
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 7332a74a4b00..c37d384c0f33 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -733,7 +733,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
> return -EMSGSIZE;
>
> - features = vdev->config->get_features(vdev);
> + features = vdev->config->get_driver_features(vdev);
>
> return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> }
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index 41b0cd17fcba..ddbe142af09a 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -399,14 +399,14 @@ static u32 vdpasim_get_vq_align(struct vdpa_device *vdpa)
> return VDPASIM_QUEUE_ALIGN;
> }
>
> -static u64 vdpasim_get_features(struct vdpa_device *vdpa)
> +static u64 vdpasim_get_device_features(struct vdpa_device *vdpa)
> {
> struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>
> return vdpasim->dev_attr.supported_features;
> }
>
> -static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
> +static int vdpasim_set_driver_features(struct vdpa_device *vdpa, u64 features)
> {
> struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>
> @@ -419,6 +419,13 @@ static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
> return 0;
> }
>
> +static u64 vdpasim_get_driver_features(struct vdpa_device *vdpa)
> +{
> + struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> +
> + return vdpasim->features;
> +}
> +
> static void vdpasim_set_config_cb(struct vdpa_device *vdpa,
> struct vdpa_callback *cb)
> {
> @@ -613,8 +620,9 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
> .set_vq_state = vdpasim_set_vq_state,
> .get_vq_state = vdpasim_get_vq_state,
> .get_vq_align = vdpasim_get_vq_align,
> - .get_features = vdpasim_get_features,
> - .set_features = vdpasim_set_features,
> + .get_device_features = vdpasim_get_device_features,
> + .set_driver_features = vdpasim_set_driver_features,
> + .get_driver_features = vdpasim_get_driver_features,
> .set_config_cb = vdpasim_set_config_cb,
> .get_vq_num_max = vdpasim_get_vq_num_max,
> .get_device_id = vdpasim_get_device_id,
> @@ -642,8 +650,9 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
> .set_vq_state = vdpasim_set_vq_state,
> .get_vq_state = vdpasim_get_vq_state,
> .get_vq_align = vdpasim_get_vq_align,
> - .get_features = vdpasim_get_features,
> - .set_features = vdpasim_set_features,
> + .get_device_features = vdpasim_get_device_features,
> + .set_driver_features = vdpasim_set_driver_features,
> + .get_driver_features = vdpasim_get_driver_features,
> .set_config_cb = vdpasim_set_config_cb,
> .get_vq_num_max = vdpasim_get_vq_num_max,
> .get_device_id = vdpasim_get_device_id,
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index c9204c62f339..1e65af6ab9ae 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -573,14 +573,14 @@ static u32 vduse_vdpa_get_vq_align(struct vdpa_device *vdpa)
> return dev->vq_align;
> }
>
> -static u64 vduse_vdpa_get_features(struct vdpa_device *vdpa)
> +static u64 vduse_vdpa_get_device_features(struct vdpa_device *vdpa)
> {
> struct vduse_dev *dev = vdpa_to_vduse(vdpa);
>
> return dev->device_features;
> }
>
> -static int vduse_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> +static int vduse_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
> {
> struct vduse_dev *dev = vdpa_to_vduse(vdpa);
>
> @@ -588,6 +588,13 @@ static int vduse_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> return 0;
> }
>
> +static u64 vduse_vdpa_get_driver_features(struct vdpa_device *vdpa)
> +{
> + struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> +
> + return dev->driver_features;
> +}
> +
> static void vduse_vdpa_set_config_cb(struct vdpa_device *vdpa,
> struct vdpa_callback *cb)
> {
> @@ -720,8 +727,9 @@ static const struct vdpa_config_ops vduse_vdpa_config_ops = {
> .set_vq_state = vduse_vdpa_set_vq_state,
> .get_vq_state = vduse_vdpa_get_vq_state,
> .get_vq_align = vduse_vdpa_get_vq_align,
> - .get_features = vduse_vdpa_get_features,
> - .set_features = vduse_vdpa_set_features,
> + .get_device_features = vduse_vdpa_get_device_features,
> + .set_driver_features = vduse_vdpa_set_driver_features,
> + .get_driver_features = vduse_vdpa_get_driver_features,
> .set_config_cb = vduse_vdpa_set_config_cb,
> .get_vq_num_max = vduse_vdpa_get_vq_num_max,
> .get_device_id = vduse_vdpa_get_device_id,
> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
> index e3ff7875e123..97285bc05e47 100644
> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> @@ -53,14 +53,14 @@ static struct virtio_pci_modern_device *vdpa_to_mdev(struct vdpa_device *vdpa)
> return &vp_vdpa->mdev;
> }
>
> -static u64 vp_vdpa_get_features(struct vdpa_device *vdpa)
> +static u64 vp_vdpa_get_device_features(struct vdpa_device *vdpa)
> {
> struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
>
> return vp_modern_get_features(mdev);
> }
>
> -static int vp_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> +static int vp_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
> {
> struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
>
> @@ -69,6 +69,13 @@ static int vp_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> return 0;
> }
>
> +static u64 vp_vdpa_get_driver_features(struct vdpa_device *vdpa)
> +{
> + struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
> +
> + return vp_modern_get_features(mdev);
> +}
I think this should be vp_modern_get_driver_features().
> +
> static u8 vp_vdpa_get_status(struct vdpa_device *vdpa)
> {
> struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
> @@ -415,8 +422,9 @@ vp_vdpa_get_vq_notification(struct vdpa_device *vdpa, u16 qid)
> }
>
> static const struct vdpa_config_ops vp_vdpa_ops = {
> - .get_features = vp_vdpa_get_features,
> - .set_features = vp_vdpa_set_features,
> + .get_device_features = vp_vdpa_get_device_features,
> + .set_driver_features = vp_vdpa_set_driver_features,
> + .get_driver_features = vp_vdpa_get_driver_features,
> .get_status = vp_vdpa_get_status,
> .set_status = vp_vdpa_set_status,
> .reset = vp_vdpa_reset,
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 29cced1cd277..6d087a4fb581 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -262,7 +262,7 @@ static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
> const struct vdpa_config_ops *ops = vdpa->config;
> u64 features;
>
> - features = ops->get_features(vdpa);
> + features = ops->get_device_features(vdpa);
>
> if (copy_to_user(featurep, &features, sizeof(features)))
> return -EFAULT;
> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> index f85f860bc10b..a84b04ba3195 100644
> --- a/drivers/virtio/virtio_vdpa.c
> +++ b/drivers/virtio/virtio_vdpa.c
> @@ -308,7 +308,7 @@ static u64 virtio_vdpa_get_features(struct virtio_device *vdev)
> struct vdpa_device *vdpa = vd_get_vdpa(vdev);
> const struct vdpa_config_ops *ops = vdpa->config;
>
> - return ops->get_features(vdpa);
> + return ops->get_device_features(vdpa);
> }
>
> static int virtio_vdpa_finalize_features(struct virtio_device *vdev)
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index c3011ccda430..db24317d61b6 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -169,14 +169,17 @@ struct vdpa_map_file {
> * for the device
> * @vdev: vdpa device
> * Returns virtqueue algin requirement
> - * @get_features: Get virtio features supported by the device
> + * @get_device_features: Get virtio features supported by the device
> * @vdev: vdpa device
> * Returns the virtio features support by the
> * device
> - * @set_features: Set virtio features supported by the driver
> + * @set_driver_features: Set virtio features supported by the driver
> * @vdev: vdpa device
> * @features: feature support by the driver
> * Returns integer: success (0) or error (< 0)
> + * @get_driver_features: Get virtio features in action
Maybe "Get virtio driver features .." is better.
Thanks
> + * @vdev: vdpa device
> + * Returns the virtio features accepted
> * @set_config_cb: Set the config interrupt callback
> * @vdev: vdpa device
> * @cb: virtio-vdev interrupt callback structure
> @@ -276,8 +279,9 @@ struct vdpa_config_ops {
>
> /* Device ops */
> u32 (*get_vq_align)(struct vdpa_device *vdev);
> - u64 (*get_features)(struct vdpa_device *vdev);
> - int (*set_features)(struct vdpa_device *vdev, u64 features);
> + u64 (*get_device_features)(struct vdpa_device *vdev);
> + int (*set_driver_features)(struct vdpa_device *vdev, u64 features);
> + u64 (*get_driver_features)(struct vdpa_device *vdev);
> void (*set_config_cb)(struct vdpa_device *vdev,
> struct vdpa_callback *cb);
> u16 (*get_vq_num_max)(struct vdpa_device *vdev);
> @@ -395,7 +399,7 @@ static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
> const struct vdpa_config_ops *ops = vdev->config;
>
> vdev->features_valid = true;
> - return ops->set_features(vdev, features);
> + return ops->set_driver_features(vdev, features);
> }
>
> void vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
> --
> 2.33.1
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 3/7] vdpa: Allow to configure max data virtqueues
2021-12-09 0:25 ` [PATCH v1 3/7] vdpa: Allow to configure max data virtqueues Si-Wei Liu
@ 2021-12-09 5:36 ` Jason Wang
2021-12-09 21:50 ` Si-Wei Liu
0 siblings, 1 reply; 22+ messages in thread
From: Jason Wang @ 2021-12-09 5:36 UTC (permalink / raw)
To: Si-Wei Liu; +Cc: Laurent Vivier, mst, virtualization, eperezma, Eli Cohen
On Thu, Dec 9, 2021 at 8:25 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 12/8/2021 12:14 PM, Eli Cohen wrote:
> > Add netlink support to configure the max virtqueue pairs for a device.
> > At least one pair is required. The maximum is dictated by the device.
> >
> > Example:
> >
> > $ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 max_vqp 5
> Not this patch, but I think there should be a mega attribute defined
> ahead to specify the virtio class/type to create vdpa instance with.
> Like the existing ones e.g. mac_addr and mtu, max_vqp is also vdpa net
> specific attribute.
Probably, but this only works for the case when a single parent is
allowed to create different types of devices. It looks to me the
current model to have a per type parent.
Thanks
>
> -Siwei
>
> >
> > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > ---
> > v0 -> v1:
> > 1. fix typo
> > 2. move max_vq_pairs to net specific struct
> >
> > drivers/vdpa/vdpa.c | 14 +++++++++++++-
> > include/linux/vdpa.h | 1 +
> > 2 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > index c37d384c0f33..3bf016e03512 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -480,7 +480,8 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
> > }
> >
> > #define VDPA_DEV_NET_ATTRS_MASK ((1 << VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
> > - (1 << VDPA_ATTR_DEV_NET_CFG_MTU))
> > + (1 << VDPA_ATTR_DEV_NET_CFG_MTU) | \
> > + (1 << VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
> >
> > static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *info)
> > {
> > @@ -506,6 +507,17 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
> > nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]);
> > config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MTU);
> > }
> > + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]) {
> > + config.net.max_vq_pairs =
> > + nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]);
> > + if (!config.net.max_vq_pairs) {
> > + NL_SET_ERR_MSG_MOD(info->extack,
> > + "At least one pair of VQs is required");
> > + err = -EINVAL;
> > + goto err;
> > + }
> > + config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
> > + }
> >
> > /* Skip checking capability if user didn't prefer to configure any
> > * device networking attributes. It is likely that user might have used
> > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> > index db24317d61b6..b62032573780 100644
> > --- a/include/linux/vdpa.h
> > +++ b/include/linux/vdpa.h
> > @@ -99,6 +99,7 @@ struct vdpa_dev_set_config {
> > struct {
> > u8 mac[ETH_ALEN];
> > u16 mtu;
> > + u16 max_vq_pairs;
> > } net;
> > u64 mask;
> > };
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 5/7] vdpa/mlx5: Support configuring max data virtqueue pairs
[not found] ` <20211208201430.73720-6-elic@nvidia.com>
@ 2021-12-09 5:43 ` Jason Wang
0 siblings, 0 replies; 22+ messages in thread
From: Jason Wang @ 2021-12-09 5:43 UTC (permalink / raw)
To: Eli Cohen; +Cc: Laurent Vivier, mst, virtualization, eperezma, Si-Wei Liu
On Thu, Dec 9, 2021 at 4:15 AM Eli Cohen <elic@nvidia.com> wrote:
>
> Check whether the max number of data virtqueue pairs was provided when a
> adding a new device and verify the new value does not exceed device
> capabilities.
>
> In addition, change the arrays holding virtqueue and callback contexts
> to be dynamically allocated.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Acked-by: Jason Wang <jasowang@redhat.com>
> ---
> v0->v1:
> 1. Remove requirement to configure power of two virtqueue pairs
> 2. Fix cleanup if kcalloc failed
>
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 43 +++++++++++++++++++++----------
> 1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 1ec29879fc2c..4f0b8bba8b58 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -131,11 +131,6 @@ struct mlx5_vdpa_virtqueue {
> struct mlx5_vq_restore_info ri;
> };
>
> -/* We will remove this limitation once mlx5_vdpa_alloc_resources()
> - * provides for driver space allocation
> - */
> -#define MLX5_MAX_SUPPORTED_VQS 16
> -
> static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
> {
> if (unlikely(idx > mvdev->max_idx))
> @@ -148,8 +143,8 @@ struct mlx5_vdpa_net {
> struct mlx5_vdpa_dev mvdev;
> struct mlx5_vdpa_net_resources res;
> struct virtio_net_config config;
> - struct mlx5_vdpa_virtqueue vqs[MLX5_MAX_SUPPORTED_VQS];
> - struct vdpa_callback event_cbs[MLX5_MAX_SUPPORTED_VQS + 1];
> + struct mlx5_vdpa_virtqueue *vqs;
> + struct vdpa_callback *event_cbs;
>
> /* Serialize vq resources creation and destruction. This is required
> * since memory map might change and we need to destroy and create
> @@ -1218,7 +1213,7 @@ static void suspend_vqs(struct mlx5_vdpa_net *ndev)
> {
> int i;
>
> - for (i = 0; i < MLX5_MAX_SUPPORTED_VQS; i++)
> + for (i = 0; i < ndev->mvdev.max_vqs; i++)
> suspend_vq(ndev, &ndev->vqs[i]);
> }
>
> @@ -1245,7 +1240,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
> int i, j;
> int err;
>
> - max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
> + max_rqt = min_t(int, roundup_pow_of_two(ndev->mvdev.max_vqs / 2),
> 1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> if (max_rqt < 1)
> return -EOPNOTSUPP;
> @@ -2219,7 +2214,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
> clear_vqs_ready(ndev);
> mlx5_vdpa_destroy_mr(&ndev->mvdev);
> ndev->mvdev.status = 0;
> - memset(ndev->event_cbs, 0, sizeof(ndev->event_cbs));
> + memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
> ndev->mvdev.actual_features = 0;
> ++mvdev->generation;
> if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
> @@ -2292,6 +2287,8 @@ static void mlx5_vdpa_free(struct vdpa_device *vdev)
> }
> mlx5_vdpa_free_resources(&ndev->mvdev);
> mutex_destroy(&ndev->reslock);
> + kfree(ndev->event_cbs);
> + kfree(ndev->vqs);
> }
>
> static struct vdpa_notification_area mlx5_get_vq_notification(struct vdpa_device *vdev, u16 idx)
> @@ -2537,15 +2534,33 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> return -EOPNOTSUPP;
> }
>
> - /* we save one virtqueue for control virtqueue should we require it */
> max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
> - max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS);
> + if (max_vqs < 2) {
> + dev_warn(mdev->device,
> + "%d virtqueues are supported. At least 2 are required\n",
> + max_vqs);
> + return -EAGAIN;
> + }
> +
> + if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) {
> + if (add_config->net.max_vq_pairs > max_vqs / 2)
> + return -EINVAL;
> + max_vqs = min_t(u32, max_vqs, 2 * add_config->net.max_vq_pairs);
> + } else {
> + max_vqs = 2;
> + }
>
> ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev, mdev->device, &mlx5_vdpa_ops,
> name, false);
> if (IS_ERR(ndev))
> return PTR_ERR(ndev);
>
> + ndev->vqs = kcalloc(max_vqs, sizeof(*ndev->vqs), GFP_KERNEL);
> + ndev->event_cbs = kcalloc(max_vqs + 1, sizeof(*ndev->event_cbs), GFP_KERNEL);
> + if (!ndev->vqs || !ndev->event_cbs) {
> + err = -ENOMEM;
> + goto err_alloc;
> + }
> ndev->mvdev.max_vqs = max_vqs;
> mvdev = &ndev->mvdev;
> mvdev->mdev = mdev;
> @@ -2626,6 +2641,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> mlx5_mpfs_del_mac(pfmdev, config->mac);
> err_mtu:
> mutex_destroy(&ndev->reslock);
> +err_alloc:
> put_device(&mvdev->vdev.dev);
> return err;
> }
> @@ -2668,7 +2684,8 @@ static int mlx5v_probe(struct auxiliary_device *adev,
> mgtdev->mgtdev.ops = &mdev_ops;
> mgtdev->mgtdev.device = mdev->device;
> mgtdev->mgtdev.id_table = id_table;
> - mgtdev->mgtdev.config_attr_mask = BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
> + mgtdev->mgtdev.config_attr_mask = BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |
> + BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
> mgtdev->madev = madev;
>
> err = vdpa_mgmtdev_register(&mgtdev->mgtdev);
> --
> 2.33.1
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 6/7] vdpa: Add support for querying control virtqueue index
[not found] ` <20211208201430.73720-7-elic@nvidia.com>
2021-12-09 0:59 ` [PATCH v1 6/7] vdpa: Add support for querying control virtqueue index Si-Wei Liu
@ 2021-12-09 5:44 ` Jason Wang
[not found] ` <20211209070904.GE108239@mtl-vdi-166.wap.labs.mlnx>
1 sibling, 1 reply; 22+ messages in thread
From: Jason Wang @ 2021-12-09 5:44 UTC (permalink / raw)
To: Eli Cohen; +Cc: Laurent Vivier, mst, virtualization, eperezma, Si-Wei Liu
On Thu, Dec 9, 2021 at 4:15 AM Eli Cohen <elic@nvidia.com> wrote:
>
> Add netlink attribute and callback function to query the control VQ
> index of a device.
>
> Example:
>
> $ vdpa dev config show vdpa-a
> vdpa-a: mac 00:00:00:00:88:88 link up link_announce false max_vq_pairs 5 \
> mtu 9000 ctrl_vq_idx 10
I still wonder about the motivation for this. And as discussed, the
ctrl_vq_idx varies depending on whether MQ is negotiated.
Thanks
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
> v0 -> v1:
> 1. Use logic defined in the spec to deduce virtqueue index.
>
> drivers/vdpa/vdpa.c | 25 +++++++++++++++++++++++++
> include/uapi/linux/vdpa.h | 1 +
> 2 files changed, 26 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 3bf016e03512..b4d4b8a7ca4e 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -712,6 +712,27 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct sk_buff *msg, struct netlink_callba
> return msg->len;
> }
>
> +static int vdpa_dev_net_ctrl_vq_fill(struct vdpa_device *vdev,
> + struct sk_buff *msg,
> + struct virtio_net_config *config,
> + u64 features)
> +{
> + u16 N;
> +
> + /* control VQ index, if available, is deduced according to the logic
> + * described in the virtio spec in section 5.1.2
> + */
> + if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> + return 0;
> +
> + if (features & BIT_ULL(VIRTIO_NET_F_MQ))
> + N = le16_to_cpu(config->max_virtqueue_pairs);
> + else
> + N = 1;
> +
> + return nla_put_u16(msg, VDPA_ATTR_DEV_CTRL_VQ_IDX, 2 * N);
> +}
> +
> static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
> struct sk_buff *msg, u64 features,
> const struct virtio_net_config *config)
> @@ -730,6 +751,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> struct virtio_net_config config = {};
> u64 features;
> u16 val_u16;
> + int err;
>
> vdpa_get_config(vdev, 0, &config, sizeof(config));
>
> @@ -746,6 +768,9 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> return -EMSGSIZE;
>
> features = vdev->config->get_driver_features(vdev);
> + err = vdpa_dev_net_ctrl_vq_fill(vdev, msg, &config, features);
> + if (err)
> + return err;
>
> return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> }
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index a252f06f9dfd..2e3a7f89f42d 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -34,6 +34,7 @@ enum vdpa_attr {
> VDPA_ATTR_DEV_MAX_VQS, /* u32 */
> VDPA_ATTR_DEV_MAX_VQ_SIZE, /* u16 */
> VDPA_ATTR_DEV_MIN_VQ_SIZE, /* u16 */
> + VDPA_ATTR_DEV_CTRL_VQ_IDX, /* u16 */
>
> VDPA_ATTR_DEV_NET_CFG_MACADDR, /* binary */
> VDPA_ATTR_DEV_NET_STATUS, /* u8 */
> --
> 2.33.1
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 6/7] vdpa: Add support for querying control virtqueue index
[not found] ` <20211209070904.GE108239@mtl-vdi-166.wap.labs.mlnx>
@ 2021-12-09 7:55 ` Jason Wang
[not found] ` <20211209080414.GA110931@mtl-vdi-166.wap.labs.mlnx>
0 siblings, 1 reply; 22+ messages in thread
From: Jason Wang @ 2021-12-09 7:55 UTC (permalink / raw)
To: Eli Cohen; +Cc: Laurent Vivier, mst, virtualization, eperezma, Si-Wei Liu
On Thu, Dec 9, 2021 at 3:09 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Thu, Dec 09, 2021 at 01:44:56PM +0800, Jason Wang wrote:
> > On Thu, Dec 9, 2021 at 4:15 AM Eli Cohen <elic@nvidia.com> wrote:
> > >
> > > Add netlink attribute and callback function to query the control VQ
> > > index of a device.
> > >
> > > Example:
> > >
> > > $ vdpa dev config show vdpa-a
> > > vdpa-a: mac 00:00:00:00:88:88 link up link_announce false max_vq_pairs 5 \
> > > mtu 9000 ctrl_vq_idx 10
> >
> >
> > I still wonder about the motivation for this.
> To be able to show the stats for CVQ.
Right.
>
> > And as discussed, the
> > ctrl_vq_idx varies depending on whether MQ is negotiated.
>
> I handle this according to the spec and it depends on MQ.
Yes, but there could be a chance that the cvq index changes after the
vdpa dev config show.
Thanks
>
> >
> > Thanks
> >
> > >
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > ---
> > > v0 -> v1:
> > > 1. Use logic defined in the spec to deduce virtqueue index.
> > >
> > > drivers/vdpa/vdpa.c | 25 +++++++++++++++++++++++++
> > > include/uapi/linux/vdpa.h | 1 +
> > > 2 files changed, 26 insertions(+)
> > >
> > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > index 3bf016e03512..b4d4b8a7ca4e 100644
> > > --- a/drivers/vdpa/vdpa.c
> > > +++ b/drivers/vdpa/vdpa.c
> > > @@ -712,6 +712,27 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct sk_buff *msg, struct netlink_callba
> > > return msg->len;
> > > }
> > >
> > > +static int vdpa_dev_net_ctrl_vq_fill(struct vdpa_device *vdev,
> > > + struct sk_buff *msg,
> > > + struct virtio_net_config *config,
> > > + u64 features)
> > > +{
> > > + u16 N;
> > > +
> > > + /* control VQ index, if available, is deduced according to the logic
> > > + * described in the virtio spec in section 5.1.2
> > > + */
> > > + if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > + return 0;
> > > +
> > > + if (features & BIT_ULL(VIRTIO_NET_F_MQ))
> > > + N = le16_to_cpu(config->max_virtqueue_pairs);
> > > + else
> > > + N = 1;
> > > +
> > > + return nla_put_u16(msg, VDPA_ATTR_DEV_CTRL_VQ_IDX, 2 * N);
> > > +}
> > > +
> > > static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
> > > struct sk_buff *msg, u64 features,
> > > const struct virtio_net_config *config)
> > > @@ -730,6 +751,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> > > struct virtio_net_config config = {};
> > > u64 features;
> > > u16 val_u16;
> > > + int err;
> > >
> > > vdpa_get_config(vdev, 0, &config, sizeof(config));
> > >
> > > @@ -746,6 +768,9 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> > > return -EMSGSIZE;
> > >
> > > features = vdev->config->get_driver_features(vdev);
> > > + err = vdpa_dev_net_ctrl_vq_fill(vdev, msg, &config, features);
> > > + if (err)
> > > + return err;
> > >
> > > return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> > > }
> > > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > > index a252f06f9dfd..2e3a7f89f42d 100644
> > > --- a/include/uapi/linux/vdpa.h
> > > +++ b/include/uapi/linux/vdpa.h
> > > @@ -34,6 +34,7 @@ enum vdpa_attr {
> > > VDPA_ATTR_DEV_MAX_VQS, /* u32 */
> > > VDPA_ATTR_DEV_MAX_VQ_SIZE, /* u16 */
> > > VDPA_ATTR_DEV_MIN_VQ_SIZE, /* u16 */
> > > + VDPA_ATTR_DEV_CTRL_VQ_IDX, /* u16 */
> > >
> > > VDPA_ATTR_DEV_NET_CFG_MACADDR, /* binary */
> > > VDPA_ATTR_DEV_NET_STATUS, /* u8 */
> > > --
> > > 2.33.1
> > >
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/7] vdpa: Provide interface to read driver features
[not found] ` <20211209070654.GD108239@mtl-vdi-166.wap.labs.mlnx>
@ 2021-12-09 7:55 ` Jason Wang
0 siblings, 0 replies; 22+ messages in thread
From: Jason Wang @ 2021-12-09 7:55 UTC (permalink / raw)
To: Eli Cohen; +Cc: Laurent Vivier, mst, virtualization, eperezma, Si-Wei Liu
On Thu, Dec 9, 2021 at 3:07 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Thu, Dec 09, 2021 at 01:33:01PM +0800, Jason Wang wrote:
> > On Thu, Dec 9, 2021 at 4:14 AM Eli Cohen <elic@nvidia.com> wrote:
> > >
> > > Provide an interface to read the negotiated features. This is needed
> > > when building the netlink message in vdpa_dev_net_config_fill().
> > >
> > > Also fix the implementation of vdpa_dev_net_config_fill() to use the
> > > negotiated features instead of the device features.
> > >
> > > To make APIs clearer, make the following name changes to struct
> > > vdpa_config_ops so they better describe their operations:
> > >
> > > get_features -> get_device_features
> > > set_features -> set_driver_features
[...]
> > > + * @get_driver_features: Get virtio features in action
> >
> > Maybe "Get virtio driver features .." is better.
>
> I hope the name does not become too long.
> Which one would you favor?
>
> get_vio_driver_features
> get_virtio_drv_features
> get_virtio_driver_features
The name is fine, I mean the comment might be
"Get the virtio driver features in action"
Thanks
>
> >
> > Thanks
> >
> > > + * @vdev: vdpa device
> > > + * Returns the virtio features accepted
> > > * @set_config_cb: Set the config interrupt callback
> > > * @vdev: vdpa device
> > > * @cb: virtio-vdev interrupt callback structure
> > > @@ -276,8 +279,9 @@ struct vdpa_config_ops {
> > >
> > > /* Device ops */
> > > u32 (*get_vq_align)(struct vdpa_device *vdev);
> > > - u64 (*get_features)(struct vdpa_device *vdev);
> > > - int (*set_features)(struct vdpa_device *vdev, u64 features);
> > > + u64 (*get_device_features)(struct vdpa_device *vdev);
> > > + int (*set_driver_features)(struct vdpa_device *vdev, u64 features);
> > > + u64 (*get_driver_features)(struct vdpa_device *vdev);
> > > void (*set_config_cb)(struct vdpa_device *vdev,
> > > struct vdpa_callback *cb);
> > > u16 (*get_vq_num_max)(struct vdpa_device *vdev);
> > > @@ -395,7 +399,7 @@ static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
> > > const struct vdpa_config_ops *ops = vdev->config;
> > >
> > > vdev->features_valid = true;
> > > - return ops->set_features(vdev, features);
> > > + return ops->set_driver_features(vdev, features);
> > > }
> > >
> > > void vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
> > > --
> > > 2.33.1
> > >
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 6/7] vdpa: Add support for querying control virtqueue index
[not found] ` <20211209080414.GA110931@mtl-vdi-166.wap.labs.mlnx>
@ 2021-12-09 8:17 ` Jason Wang
[not found] ` <20211209082618.GA111658@mtl-vdi-166.wap.labs.mlnx>
0 siblings, 1 reply; 22+ messages in thread
From: Jason Wang @ 2021-12-09 8:17 UTC (permalink / raw)
To: Eli Cohen; +Cc: Laurent Vivier, mst, virtualization, eperezma, Si-Wei Liu
On Thu, Dec 9, 2021 at 4:04 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Thu, Dec 09, 2021 at 03:55:48PM +0800, Jason Wang wrote:
> > On Thu, Dec 9, 2021 at 3:09 PM Eli Cohen <elic@nvidia.com> wrote:
> > >
> > > On Thu, Dec 09, 2021 at 01:44:56PM +0800, Jason Wang wrote:
> > > > On Thu, Dec 9, 2021 at 4:15 AM Eli Cohen <elic@nvidia.com> wrote:
> > > > >
> > > > > Add netlink attribute and callback function to query the control VQ
> > > > > index of a device.
> > > > >
> > > > > Example:
> > > > >
> > > > > $ vdpa dev config show vdpa-a
> > > > > vdpa-a: mac 00:00:00:00:88:88 link up link_announce false max_vq_pairs 5 \
> > > > > mtu 9000 ctrl_vq_idx 10
> > > >
> > > >
> > > > I still wonder about the motivation for this.
> > > To be able to show the stats for CVQ.
> >
> > Right.
> >
> > >
> > > > And as discussed, the
> > > > ctrl_vq_idx varies depending on whether MQ is negotiated.
> > >
> > > I handle this according to the spec and it depends on MQ.
> >
> > Yes, but there could be a chance that the cvq index changes after the
> > vdpa dev config show.
> >
>
> It depends on:
> VIRTIO_NET_F_CTRL_VQ
> VIRTIO_NET_F_MQ
>
> which can change only if the features are re-negotiated and that happens
> on driver in itialization.
>
> And on max_virtqueue_pairs which is also set at driver initialization.
>
> So I don't see any real issue here. Am I missing something?
No. I meant technically there could be a re-negotiation that happens
in the middle:
1) vdpa dev config show
2) feature renegotiation which change the cvq index
3) getting cvq stats
So the cvq index might be changed. E.g it could be changed from a cvq
to a rx queue. It might be easier to make the get index and stats
atomic.
Thanks
>
> > Thanks
> >
> > >
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > ---
> > > > > v0 -> v1:
> > > > > 1. Use logic defined in the spec to deduce virtqueue index.
> > > > >
> > > > > drivers/vdpa/vdpa.c | 25 +++++++++++++++++++++++++
> > > > > include/uapi/linux/vdpa.h | 1 +
> > > > > 2 files changed, 26 insertions(+)
> > > > >
> > > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > > > index 3bf016e03512..b4d4b8a7ca4e 100644
> > > > > --- a/drivers/vdpa/vdpa.c
> > > > > +++ b/drivers/vdpa/vdpa.c
> > > > > @@ -712,6 +712,27 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct sk_buff *msg, struct netlink_callba
> > > > > return msg->len;
> > > > > }
> > > > >
> > > > > +static int vdpa_dev_net_ctrl_vq_fill(struct vdpa_device *vdev,
> > > > > + struct sk_buff *msg,
> > > > > + struct virtio_net_config *config,
> > > > > + u64 features)
> > > > > +{
> > > > > + u16 N;
> > > > > +
> > > > > + /* control VQ index, if available, is deduced according to the logic
> > > > > + * described in the virtio spec in section 5.1.2
> > > > > + */
> > > > > + if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > > + return 0;
> > > > > +
> > > > > + if (features & BIT_ULL(VIRTIO_NET_F_MQ))
> > > > > + N = le16_to_cpu(config->max_virtqueue_pairs);
> > > > > + else
> > > > > + N = 1;
> > > > > +
> > > > > + return nla_put_u16(msg, VDPA_ATTR_DEV_CTRL_VQ_IDX, 2 * N);
> > > > > +}
> > > > > +
> > > > > static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
> > > > > struct sk_buff *msg, u64 features,
> > > > > const struct virtio_net_config *config)
> > > > > @@ -730,6 +751,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> > > > > struct virtio_net_config config = {};
> > > > > u64 features;
> > > > > u16 val_u16;
> > > > > + int err;
> > > > >
> > > > > vdpa_get_config(vdev, 0, &config, sizeof(config));
> > > > >
> > > > > @@ -746,6 +768,9 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> > > > > return -EMSGSIZE;
> > > > >
> > > > > features = vdev->config->get_driver_features(vdev);
> > > > > + err = vdpa_dev_net_ctrl_vq_fill(vdev, msg, &config, features);
> > > > > + if (err)
> > > > > + return err;
> > > > >
> > > > > return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> > > > > }
> > > > > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > > > > index a252f06f9dfd..2e3a7f89f42d 100644
> > > > > --- a/include/uapi/linux/vdpa.h
> > > > > +++ b/include/uapi/linux/vdpa.h
> > > > > @@ -34,6 +34,7 @@ enum vdpa_attr {
> > > > > VDPA_ATTR_DEV_MAX_VQS, /* u32 */
> > > > > VDPA_ATTR_DEV_MAX_VQ_SIZE, /* u16 */
> > > > > VDPA_ATTR_DEV_MIN_VQ_SIZE, /* u16 */
> > > > > + VDPA_ATTR_DEV_CTRL_VQ_IDX, /* u16 */
> > > > >
> > > > > VDPA_ATTR_DEV_NET_CFG_MACADDR, /* binary */
> > > > > VDPA_ATTR_DEV_NET_STATUS, /* u8 */
> > > > > --
> > > > > 2.33.1
> > > > >
> > > >
> > >
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 6/7] vdpa: Add support for querying control virtqueue index
[not found] ` <20211209082618.GA111658@mtl-vdi-166.wap.labs.mlnx>
@ 2021-12-09 9:12 ` Jason Wang
[not found] ` <20211209092053.GA113385@mtl-vdi-166.wap.labs.mlnx>
0 siblings, 1 reply; 22+ messages in thread
From: Jason Wang @ 2021-12-09 9:12 UTC (permalink / raw)
To: Eli Cohen; +Cc: Laurent Vivier, mst, virtualization, eperezma, Si-Wei Liu
On Thu, Dec 9, 2021 at 4:26 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Thu, Dec 09, 2021 at 04:17:22PM +0800, Jason Wang wrote:
> > On Thu, Dec 9, 2021 at 4:04 PM Eli Cohen <elic@nvidia.com> wrote:
> > >
> > > On Thu, Dec 09, 2021 at 03:55:48PM +0800, Jason Wang wrote:
> > > > On Thu, Dec 9, 2021 at 3:09 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > >
> > > > > On Thu, Dec 09, 2021 at 01:44:56PM +0800, Jason Wang wrote:
> > > > > > On Thu, Dec 9, 2021 at 4:15 AM Eli Cohen <elic@nvidia.com> wrote:
> > > > > > >
> > > > > > > Add netlink attribute and callback function to query the control VQ
> > > > > > > index of a device.
> > > > > > >
> > > > > > > Example:
> > > > > > >
> > > > > > > $ vdpa dev config show vdpa-a
> > > > > > > vdpa-a: mac 00:00:00:00:88:88 link up link_announce false max_vq_pairs 5 \
> > > > > > > mtu 9000 ctrl_vq_idx 10
> > > > > >
> > > > > >
> > > > > > I still wonder about the motivation for this.
> > > > > To be able to show the stats for CVQ.
> > > >
> > > > Right.
> > > >
> > > > >
> > > > > > And as discussed, the
> > > > > > ctrl_vq_idx varies depending on whether MQ is negotiated.
> > > > >
> > > > > I handle this according to the spec and it depends on MQ.
> > > >
> > > > Yes, but there could be a chance that the cvq index changes after the
> > > > vdpa dev config show.
> > > >
> > >
> > > It depends on:
> > > VIRTIO_NET_F_CTRL_VQ
> > > VIRTIO_NET_F_MQ
> > >
> > > which can change only if the features are re-negotiated and that happens
> > > on driver in itialization.
> > >
> > > And on max_virtqueue_pairs which is also set at driver initialization.
> > >
> > > So I don't see any real issue here. Am I missing something?
> >
> > No. I meant technically there could be a re-negotiation that happens
> > in the middle:
> >
> > 1) vdpa dev config show
> > 2) feature renegotiation which change the cvq index
> > 3) getting cvq stats
> >
> > So the cvq index might be changed. E.g it could be changed from a cvq
> > to a rx queue. It might be easier to make the get index and stats
> > atomic.
> >
>
> The interface to read VQ stats is based on the user providing the index
> and getting the statistics.
>
> If we want to follow your suggestion then we need maybe to use a
> slightly modified command:
>
> For regular VQ:
> dpa dev vstats show vdpa-a qidx 0
>
> For CVQ:
> vdpa dev vstats show vdpa-a cvq
This might be better, but we need to make sure cvq only works for the
device that has a cvq.
>
> And that raises another question. Do we want to report the CVQ index in
> config show?
If we go with stats show vdpa-a cvq, we don't need to report the cvq index.
Thanks
>
> > Thanks
> >
> > >
> > > > Thanks
> > > >
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > > ---
> > > > > > > v0 -> v1:
> > > > > > > 1. Use logic defined in the spec to deduce virtqueue index.
> > > > > > >
> > > > > > > drivers/vdpa/vdpa.c | 25 +++++++++++++++++++++++++
> > > > > > > include/uapi/linux/vdpa.h | 1 +
> > > > > > > 2 files changed, 26 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > > > > > index 3bf016e03512..b4d4b8a7ca4e 100644
> > > > > > > --- a/drivers/vdpa/vdpa.c
> > > > > > > +++ b/drivers/vdpa/vdpa.c
> > > > > > > @@ -712,6 +712,27 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct sk_buff *msg, struct netlink_callba
> > > > > > > return msg->len;
> > > > > > > }
> > > > > > >
> > > > > > > +static int vdpa_dev_net_ctrl_vq_fill(struct vdpa_device *vdev,
> > > > > > > + struct sk_buff *msg,
> > > > > > > + struct virtio_net_config *config,
> > > > > > > + u64 features)
> > > > > > > +{
> > > > > > > + u16 N;
> > > > > > > +
> > > > > > > + /* control VQ index, if available, is deduced according to the logic
> > > > > > > + * described in the virtio spec in section 5.1.2
> > > > > > > + */
> > > > > > > + if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > > > > + return 0;
> > > > > > > +
> > > > > > > + if (features & BIT_ULL(VIRTIO_NET_F_MQ))
> > > > > > > + N = le16_to_cpu(config->max_virtqueue_pairs);
> > > > > > > + else
> > > > > > > + N = 1;
> > > > > > > +
> > > > > > > + return nla_put_u16(msg, VDPA_ATTR_DEV_CTRL_VQ_IDX, 2 * N);
> > > > > > > +}
> > > > > > > +
> > > > > > > static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
> > > > > > > struct sk_buff *msg, u64 features,
> > > > > > > const struct virtio_net_config *config)
> > > > > > > @@ -730,6 +751,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> > > > > > > struct virtio_net_config config = {};
> > > > > > > u64 features;
> > > > > > > u16 val_u16;
> > > > > > > + int err;
> > > > > > >
> > > > > > > vdpa_get_config(vdev, 0, &config, sizeof(config));
> > > > > > >
> > > > > > > @@ -746,6 +768,9 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> > > > > > > return -EMSGSIZE;
> > > > > > >
> > > > > > > features = vdev->config->get_driver_features(vdev);
> > > > > > > + err = vdpa_dev_net_ctrl_vq_fill(vdev, msg, &config, features);
> > > > > > > + if (err)
> > > > > > > + return err;
> > > > > > >
> > > > > > > return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> > > > > > > }
> > > > > > > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > > > > > > index a252f06f9dfd..2e3a7f89f42d 100644
> > > > > > > --- a/include/uapi/linux/vdpa.h
> > > > > > > +++ b/include/uapi/linux/vdpa.h
> > > > > > > @@ -34,6 +34,7 @@ enum vdpa_attr {
> > > > > > > VDPA_ATTR_DEV_MAX_VQS, /* u32 */
> > > > > > > VDPA_ATTR_DEV_MAX_VQ_SIZE, /* u16 */
> > > > > > > VDPA_ATTR_DEV_MIN_VQ_SIZE, /* u16 */
> > > > > > > + VDPA_ATTR_DEV_CTRL_VQ_IDX, /* u16 */
> > > > > > >
> > > > > > > VDPA_ATTR_DEV_NET_CFG_MACADDR, /* binary */
> > > > > > > VDPA_ATTR_DEV_NET_STATUS, /* u8 */
> > > > > > > --
> > > > > > > 2.33.1
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 3/7] vdpa: Allow to configure max data virtqueues
2021-12-09 5:36 ` Jason Wang
@ 2021-12-09 21:50 ` Si-Wei Liu
2021-12-10 2:29 ` Jason Wang
0 siblings, 1 reply; 22+ messages in thread
From: Si-Wei Liu @ 2021-12-09 21:50 UTC (permalink / raw)
To: Jason Wang; +Cc: Laurent Vivier, mst, virtualization, eperezma, Eli Cohen
On 12/8/2021 9:36 PM, Jason Wang wrote:
> On Thu, Dec 9, 2021 at 8:25 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 12/8/2021 12:14 PM, Eli Cohen wrote:
>>> Add netlink support to configure the max virtqueue pairs for a device.
>>> At least one pair is required. The maximum is dictated by the device.
>>>
>>> Example:
>>>
>>> $ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 max_vqp 5
>> Not this patch, but I think there should be a mega attribute defined
>> ahead to specify the virtio class/type to create vdpa instance with.
>> Like the existing ones e.g. mac_addr and mtu, max_vqp is also vdpa net
>> specific attribute.
> Probably, but this only works for the case when a single parent is
> allowed to create different types of devices. It looks to me the
> current model to have a per type parent.
Yes, that is the current situation and implementation, although the
model does allow multi-type parent through
VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES. Regardless, even though with
single-type parent, so far on vdpa creation there's no validation done
yet against which class/type the parent can support. The max-vqp is
essentially vdpa network device only, but command line users don't have
a means to infer it is structured this way, and/or which vdpa parent may
support this config attribute. That said, were the command line usage
structured like below, I would have less worry.
$ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 net.max_vqp 5
Alternatively, if the goal is to keep the attribute flat, we may show
the mapping for the parent capability and the supported class:
$ vdpa mgmtdev capab show
pci/0000:41:00.2:
class: net
mac: off
mtu: on
max_mtu: 9000
max_vqp: 1
auxiliary/mlx5_core.sf.1:
class: net
mac: on
mtu: off
max_mtu: 1500
max_vqp: 256
Thanks,
-Siwei
>
> Thanks
>
>> -Siwei
>>
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> ---
>>> v0 -> v1:
>>> 1. fix typo
>>> 2. move max_vq_pairs to net specific struct
>>>
>>> drivers/vdpa/vdpa.c | 14 +++++++++++++-
>>> include/linux/vdpa.h | 1 +
>>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index c37d384c0f33..3bf016e03512 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -480,7 +480,8 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
>>> }
>>>
>>> #define VDPA_DEV_NET_ATTRS_MASK ((1 << VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
>>> - (1 << VDPA_ATTR_DEV_NET_CFG_MTU))
>>> + (1 << VDPA_ATTR_DEV_NET_CFG_MTU) | \
>>> + (1 << VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
>>>
>>> static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *info)
>>> {
>>> @@ -506,6 +507,17 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
>>> nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]);
>>> config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MTU);
>>> }
>>> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]) {
>>> + config.net.max_vq_pairs =
>>> + nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]);
>>> + if (!config.net.max_vq_pairs) {
>>> + NL_SET_ERR_MSG_MOD(info->extack,
>>> + "At least one pair of VQs is required");
>>> + err = -EINVAL;
>>> + goto err;
>>> + }
>>> + config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
>>> + }
>>>
>>> /* Skip checking capability if user didn't prefer to configure any
>>> * device networking attributes. It is likely that user might have used
>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>> index db24317d61b6..b62032573780 100644
>>> --- a/include/linux/vdpa.h
>>> +++ b/include/linux/vdpa.h
>>> @@ -99,6 +99,7 @@ struct vdpa_dev_set_config {
>>> struct {
>>> u8 mac[ETH_ALEN];
>>> u16 mtu;
>>> + u16 max_vq_pairs;
>>> } net;
>>> u64 mask;
>>> };
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/7] vdpa: Provide interface to read driver features
[not found] ` <20211209064702.GA108239@mtl-vdi-166.wap.labs.mlnx>
@ 2021-12-09 22:29 ` Si-Wei Liu
2021-12-12 9:45 ` Michael S. Tsirkin
[not found] ` <20211212133854.GA8840@mtl-vdi-166.wap.labs.mlnx>
0 siblings, 2 replies; 22+ messages in thread
From: Si-Wei Liu @ 2021-12-09 22:29 UTC (permalink / raw)
To: Eli Cohen; +Cc: lvivier, mst, virtualization, eperezma
On 12/8/2021 10:47 PM, Eli Cohen wrote:
> On Wed, Dec 08, 2021 at 03:57:21PM -0800, Si-Wei Liu wrote:
>>
>> On 12/8/2021 12:14 PM, Eli Cohen wrote:
>>> Provide an interface to read the negotiated features. This is needed
>>> when building the netlink message in vdpa_dev_net_config_fill().
>>>
>>> Also fix the implementation of vdpa_dev_net_config_fill() to use the
>>> negotiated features instead of the device features.
>> Are we sure the use of device feature is a bug rather than expected
>> behavior?
> I think so since in vdpa_dev_net_config_fill() we're returning the
> current configuration so you you don't want to mislead the user with
> wrong information.
You seem to imply vdpa_dev_net_config_fill() should return the
current/running driver config rather than the initial setting of the
device side before feature negotiation kicks in. This seems to work for
the running link status which is propagated to the configuration space,
but how do you infer the other untrackable running config the driver is
operating, such as mtu, the effective value of which is not written back
to config space or propagated back to vdpa backend?
>
>> How do users determine the configured number of queue pairs at any
>> point in case of a non-multiqueue supporting guest/driver or that the device
>> is being reset (where actual_features == 0)?
> I would think if you read during reset (which is really a short period
> of time), you get what there is at the moment.
I would stress out the case if guest not supporting multi-queue. For e.g
guest firmware might only support single queue without control queue,
which is not considered to be transient. Since the validity of the value
connects to feature negotiation, how does the host admin user infer
feature negotiation is complete? What is the administrative value if
they have to keep track of the change without knowing the effective
negotiation status?
Thanks,
-Siwei
>
>> Maybe we should differentiate
>> the static device config against driver/device running state here. I thought
>> the change to vdpa_dev_net_config_fill() in this patch would break vdpa tool
>> user's expectation that the value of max_vqp config is a fixed value since
>> the vdpa creation time.
> That was not intended to be fixed AFAIU.
>
>> Perhaps worth adding another attribute to represent the running state
>> (cur_max_qps) based on the negotiated features.
>>
> You can get that information by running e.g. ethtool -l ens1.
>
>> -Siwei
>>
>>> To make APIs clearer, make the following name changes to struct
>>> vdpa_config_ops so they better describe their operations:
>>>
>>> get_features -> get_device_features
>>> set_features -> set_driver_features
>>>
>>> Finally, add get_driver_features to return the negotiated features and
>>> add implementation to all the upstream drivers.
>>>
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> ---
>>> drivers/vdpa/alibaba/eni_vdpa.c | 16 ++++++++++++----
>>> drivers/vdpa/ifcvf/ifcvf_main.c | 16 ++++++++++++----
>>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 16 ++++++++++++----
>>> drivers/vdpa/vdpa.c | 2 +-
>>> drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +++++++++++++++------
>>> drivers/vdpa/vdpa_user/vduse_dev.c | 16 ++++++++++++----
>>> drivers/vdpa/virtio_pci/vp_vdpa.c | 16 ++++++++++++----
>>> drivers/vhost/vdpa.c | 2 +-
>>> drivers/virtio/virtio_vdpa.c | 2 +-
>>> include/linux/vdpa.h | 14 +++++++++-----
>>> 10 files changed, 87 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c
>>> index 3f788794571a..ac0975660f4d 100644
>>> --- a/drivers/vdpa/alibaba/eni_vdpa.c
>>> +++ b/drivers/vdpa/alibaba/eni_vdpa.c
>>> @@ -58,7 +58,7 @@ static struct virtio_pci_legacy_device *vdpa_to_ldev(struct vdpa_device *vdpa)
>>> return &eni_vdpa->ldev;
>>> }
>>> -static u64 eni_vdpa_get_features(struct vdpa_device *vdpa)
>>> +static u64 eni_vdpa_get_device_features(struct vdpa_device *vdpa)
>>> {
>>> struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
>>> u64 features = vp_legacy_get_features(ldev);
>>> @@ -69,7 +69,7 @@ static u64 eni_vdpa_get_features(struct vdpa_device *vdpa)
>>> return features;
>>> }
>>> -static int eni_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
>>> +static int eni_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
>>> {
>>> struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
>>> @@ -84,6 +84,13 @@ static int eni_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
>>> return 0;
>>> }
>>> +static u64 eni_vdpa_get_driver_features(struct vdpa_device *vdpa)
>>> +{
>>> + struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
>>> +
>>> + return vp_legacy_get_driver_features(ldev);
>>> +}
>>> +
>>> static u8 eni_vdpa_get_status(struct vdpa_device *vdpa)
>>> {
>>> struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
>>> @@ -401,8 +408,9 @@ static void eni_vdpa_set_config_cb(struct vdpa_device *vdpa,
>>> }
>>> static const struct vdpa_config_ops eni_vdpa_ops = {
>>> - .get_features = eni_vdpa_get_features,
>>> - .set_features = eni_vdpa_set_features,
>>> + .get_device_features = eni_vdpa_get_device_features,
>>> + .set_driver_features = eni_vdpa_set_driver_features,
>>> + .get_driver_features = eni_vdpa_get_driver_features,
>>> .get_status = eni_vdpa_get_status,
>>> .set_status = eni_vdpa_set_status,
>>> .reset = eni_vdpa_reset,
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> index 6dc75ca70b37..6a6a0a462392 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> @@ -169,7 +169,7 @@ static struct ifcvf_hw *vdpa_to_vf(struct vdpa_device *vdpa_dev)
>>> return &adapter->vf;
>>> }
>>> -static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev)
>>> +static u64 ifcvf_vdpa_get_device_features(struct vdpa_device *vdpa_dev)
>>> {
>>> struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>> @@ -187,7 +187,7 @@ static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev)
>>> return features;
>>> }
>>> -static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, u64 features)
>>> +static int ifcvf_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 features)
>>> {
>>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>> int ret;
>>> @@ -201,6 +201,13 @@ static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, u64 features)
>>> return 0;
>>> }
>>> +static u64 ifcvf_vdpa_get_driver_features(struct vdpa_device *vdpa_dev)
>>> +{
>>> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>> +
>>> + return vf->req_features;
>>> +}
>>> +
>>> static u8 ifcvf_vdpa_get_status(struct vdpa_device *vdpa_dev)
>>> {
>>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>> @@ -443,8 +450,9 @@ static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_devic
>>> * implemented set_map()/dma_map()/dma_unmap()
>>> */
>>> static const struct vdpa_config_ops ifc_vdpa_ops = {
>>> - .get_features = ifcvf_vdpa_get_features,
>>> - .set_features = ifcvf_vdpa_set_features,
>>> + .get_device_features = ifcvf_vdpa_get_device_features,
>>> + .set_driver_features = ifcvf_vdpa_set_driver_features,
>>> + .get_driver_features = ifcvf_vdpa_get_driver_features,
>>> .get_status = ifcvf_vdpa_get_status,
>>> .set_status = ifcvf_vdpa_set_status,
>>> .reset = ifcvf_vdpa_reset,
>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> index 63813fbb5f62..ce2e13135dd8 100644
>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> @@ -1880,7 +1880,7 @@ static u64 mlx_to_vritio_features(u16 dev_features)
>>> return result;
>>> }
>>> -static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev)
>>> +static u64 mlx5_vdpa_get_device_features(struct vdpa_device *vdev)
>>> {
>>> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>> struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> @@ -1972,7 +1972,7 @@ static void update_cvq_info(struct mlx5_vdpa_dev *mvdev)
>>> }
>>> }
>>> -static int mlx5_vdpa_set_features(struct vdpa_device *vdev, u64 features)
>>> +static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>>> {
>>> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>> struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> @@ -2339,6 +2339,13 @@ static int mlx5_get_vq_irq(struct vdpa_device *vdv, u16 idx)
>>> return -EOPNOTSUPP;
>>> }
>>> +static u64 mlx5_vdpa_get_driver_features(struct vdpa_device *vdev)
>>> +{
>>> + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>> +
>>> + return mvdev->actual_features;
>>> +}
>>> +
>>> static const struct vdpa_config_ops mlx5_vdpa_ops = {
>>> .set_vq_address = mlx5_vdpa_set_vq_address,
>>> .set_vq_num = mlx5_vdpa_set_vq_num,
>>> @@ -2351,8 +2358,9 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
>>> .get_vq_notification = mlx5_get_vq_notification,
>>> .get_vq_irq = mlx5_get_vq_irq,
>>> .get_vq_align = mlx5_vdpa_get_vq_align,
>>> - .get_features = mlx5_vdpa_get_features,
>>> - .set_features = mlx5_vdpa_set_features,
>>> + .get_device_features = mlx5_vdpa_get_device_features,
>>> + .set_driver_features = mlx5_vdpa_set_driver_features,
>>> + .get_driver_features = mlx5_vdpa_get_driver_features,
>>> .set_config_cb = mlx5_vdpa_set_config_cb,
>>> .get_vq_num_max = mlx5_vdpa_get_vq_num_max,
>>> .get_device_id = mlx5_vdpa_get_device_id,
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 7332a74a4b00..c37d384c0f33 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -733,7 +733,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
>>> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
>>> return -EMSGSIZE;
>>> - features = vdev->config->get_features(vdev);
>>> + features = vdev->config->get_driver_features(vdev);
>>> return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
>>> }
>>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>> index 41b0cd17fcba..ddbe142af09a 100644
>>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>> @@ -399,14 +399,14 @@ static u32 vdpasim_get_vq_align(struct vdpa_device *vdpa)
>>> return VDPASIM_QUEUE_ALIGN;
>>> }
>>> -static u64 vdpasim_get_features(struct vdpa_device *vdpa)
>>> +static u64 vdpasim_get_device_features(struct vdpa_device *vdpa)
>>> {
>>> struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>>> return vdpasim->dev_attr.supported_features;
>>> }
>>> -static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
>>> +static int vdpasim_set_driver_features(struct vdpa_device *vdpa, u64 features)
>>> {
>>> struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>>> @@ -419,6 +419,13 @@ static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
>>> return 0;
>>> }
>>> +static u64 vdpasim_get_driver_features(struct vdpa_device *vdpa)
>>> +{
>>> + struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>>> +
>>> + return vdpasim->features;
>>> +}
>>> +
>>> static void vdpasim_set_config_cb(struct vdpa_device *vdpa,
>>> struct vdpa_callback *cb)
>>> {
>>> @@ -613,8 +620,9 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
>>> .set_vq_state = vdpasim_set_vq_state,
>>> .get_vq_state = vdpasim_get_vq_state,
>>> .get_vq_align = vdpasim_get_vq_align,
>>> - .get_features = vdpasim_get_features,
>>> - .set_features = vdpasim_set_features,
>>> + .get_device_features = vdpasim_get_device_features,
>>> + .set_driver_features = vdpasim_set_driver_features,
>>> + .get_driver_features = vdpasim_get_driver_features,
>>> .set_config_cb = vdpasim_set_config_cb,
>>> .get_vq_num_max = vdpasim_get_vq_num_max,
>>> .get_device_id = vdpasim_get_device_id,
>>> @@ -642,8 +650,9 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
>>> .set_vq_state = vdpasim_set_vq_state,
>>> .get_vq_state = vdpasim_get_vq_state,
>>> .get_vq_align = vdpasim_get_vq_align,
>>> - .get_features = vdpasim_get_features,
>>> - .set_features = vdpasim_set_features,
>>> + .get_device_features = vdpasim_get_device_features,
>>> + .set_driver_features = vdpasim_set_driver_features,
>>> + .get_driver_features = vdpasim_get_driver_features,
>>> .set_config_cb = vdpasim_set_config_cb,
>>> .get_vq_num_max = vdpasim_get_vq_num_max,
>>> .get_device_id = vdpasim_get_device_id,
>>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>>> index c9204c62f339..1e65af6ab9ae 100644
>>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>>> @@ -573,14 +573,14 @@ static u32 vduse_vdpa_get_vq_align(struct vdpa_device *vdpa)
>>> return dev->vq_align;
>>> }
>>> -static u64 vduse_vdpa_get_features(struct vdpa_device *vdpa)
>>> +static u64 vduse_vdpa_get_device_features(struct vdpa_device *vdpa)
>>> {
>>> struct vduse_dev *dev = vdpa_to_vduse(vdpa);
>>> return dev->device_features;
>>> }
>>> -static int vduse_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
>>> +static int vduse_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
>>> {
>>> struct vduse_dev *dev = vdpa_to_vduse(vdpa);
>>> @@ -588,6 +588,13 @@ static int vduse_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
>>> return 0;
>>> }
>>> +static u64 vduse_vdpa_get_driver_features(struct vdpa_device *vdpa)
>>> +{
>>> + struct vduse_dev *dev = vdpa_to_vduse(vdpa);
>>> +
>>> + return dev->driver_features;
>>> +}
>>> +
>>> static void vduse_vdpa_set_config_cb(struct vdpa_device *vdpa,
>>> struct vdpa_callback *cb)
>>> {
>>> @@ -720,8 +727,9 @@ static const struct vdpa_config_ops vduse_vdpa_config_ops = {
>>> .set_vq_state = vduse_vdpa_set_vq_state,
>>> .get_vq_state = vduse_vdpa_get_vq_state,
>>> .get_vq_align = vduse_vdpa_get_vq_align,
>>> - .get_features = vduse_vdpa_get_features,
>>> - .set_features = vduse_vdpa_set_features,
>>> + .get_device_features = vduse_vdpa_get_device_features,
>>> + .set_driver_features = vduse_vdpa_set_driver_features,
>>> + .get_driver_features = vduse_vdpa_get_driver_features,
>>> .set_config_cb = vduse_vdpa_set_config_cb,
>>> .get_vq_num_max = vduse_vdpa_get_vq_num_max,
>>> .get_device_id = vduse_vdpa_get_device_id,
>>> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
>>> index e3ff7875e123..97285bc05e47 100644
>>> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
>>> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
>>> @@ -53,14 +53,14 @@ static struct virtio_pci_modern_device *vdpa_to_mdev(struct vdpa_device *vdpa)
>>> return &vp_vdpa->mdev;
>>> }
>>> -static u64 vp_vdpa_get_features(struct vdpa_device *vdpa)
>>> +static u64 vp_vdpa_get_device_features(struct vdpa_device *vdpa)
>>> {
>>> struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
>>> return vp_modern_get_features(mdev);
>>> }
>>> -static int vp_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
>>> +static int vp_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
>>> {
>>> struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
>>> @@ -69,6 +69,13 @@ static int vp_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
>>> return 0;
>>> }
>>> +static u64 vp_vdpa_get_driver_features(struct vdpa_device *vdpa)
>>> +{
>>> + struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
>>> +
>>> + return vp_modern_get_features(mdev);
>>> +}
>>> +
>>> static u8 vp_vdpa_get_status(struct vdpa_device *vdpa)
>>> {
>>> struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
>>> @@ -415,8 +422,9 @@ vp_vdpa_get_vq_notification(struct vdpa_device *vdpa, u16 qid)
>>> }
>>> static const struct vdpa_config_ops vp_vdpa_ops = {
>>> - .get_features = vp_vdpa_get_features,
>>> - .set_features = vp_vdpa_set_features,
>>> + .get_device_features = vp_vdpa_get_device_features,
>>> + .set_driver_features = vp_vdpa_set_driver_features,
>>> + .get_driver_features = vp_vdpa_get_driver_features,
>>> .get_status = vp_vdpa_get_status,
>>> .set_status = vp_vdpa_set_status,
>>> .reset = vp_vdpa_reset,
>>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>>> index 29cced1cd277..6d087a4fb581 100644
>>> --- a/drivers/vhost/vdpa.c
>>> +++ b/drivers/vhost/vdpa.c
>>> @@ -262,7 +262,7 @@ static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
>>> const struct vdpa_config_ops *ops = vdpa->config;
>>> u64 features;
>>> - features = ops->get_features(vdpa);
>>> + features = ops->get_device_features(vdpa);
>>> if (copy_to_user(featurep, &features, sizeof(features)))
>>> return -EFAULT;
>>> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
>>> index f85f860bc10b..a84b04ba3195 100644
>>> --- a/drivers/virtio/virtio_vdpa.c
>>> +++ b/drivers/virtio/virtio_vdpa.c
>>> @@ -308,7 +308,7 @@ static u64 virtio_vdpa_get_features(struct virtio_device *vdev)
>>> struct vdpa_device *vdpa = vd_get_vdpa(vdev);
>>> const struct vdpa_config_ops *ops = vdpa->config;
>>> - return ops->get_features(vdpa);
>>> + return ops->get_device_features(vdpa);
>>> }
>>> static int virtio_vdpa_finalize_features(struct virtio_device *vdev)
>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>> index c3011ccda430..db24317d61b6 100644
>>> --- a/include/linux/vdpa.h
>>> +++ b/include/linux/vdpa.h
>>> @@ -169,14 +169,17 @@ struct vdpa_map_file {
>>> * for the device
>>> * @vdev: vdpa device
>>> * Returns virtqueue algin requirement
>>> - * @get_features: Get virtio features supported by the device
>>> + * @get_device_features: Get virtio features supported by the device
>>> * @vdev: vdpa device
>>> * Returns the virtio features support by the
>>> * device
>>> - * @set_features: Set virtio features supported by the driver
>>> + * @set_driver_features: Set virtio features supported by the driver
>>> * @vdev: vdpa device
>>> * @features: feature support by the driver
>>> * Returns integer: success (0) or error (< 0)
>>> + * @get_driver_features: Get virtio features in action
>>> + * @vdev: vdpa device
>>> + * Returns the virtio features accepted
>>> * @set_config_cb: Set the config interrupt callback
>>> * @vdev: vdpa device
>>> * @cb: virtio-vdev interrupt callback structure
>>> @@ -276,8 +279,9 @@ struct vdpa_config_ops {
>>> /* Device ops */
>>> u32 (*get_vq_align)(struct vdpa_device *vdev);
>>> - u64 (*get_features)(struct vdpa_device *vdev);
>>> - int (*set_features)(struct vdpa_device *vdev, u64 features);
>>> + u64 (*get_device_features)(struct vdpa_device *vdev);
>>> + int (*set_driver_features)(struct vdpa_device *vdev, u64 features);
>>> + u64 (*get_driver_features)(struct vdpa_device *vdev);
>>> void (*set_config_cb)(struct vdpa_device *vdev,
>>> struct vdpa_callback *cb);
>>> u16 (*get_vq_num_max)(struct vdpa_device *vdev);
>>> @@ -395,7 +399,7 @@ static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
>>> const struct vdpa_config_ops *ops = vdev->config;
>>> vdev->features_valid = true;
>>> - return ops->set_features(vdev, features);
>>> + return ops->set_driver_features(vdev, features);
>>> }
>>> void vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 6/7] vdpa: Add support for querying control virtqueue index
[not found] ` <20211209092053.GA113385@mtl-vdi-166.wap.labs.mlnx>
@ 2021-12-10 2:27 ` Jason Wang
2021-12-13 3:05 ` Jason Wang
1 sibling, 0 replies; 22+ messages in thread
From: Jason Wang @ 2021-12-10 2:27 UTC (permalink / raw)
To: Eli Cohen; +Cc: Laurent Vivier, mst, virtualization, eperezma, Si-Wei Liu
On Thu, Dec 9, 2021 at 5:21 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Thu, Dec 09, 2021 at 05:12:01PM +0800, Jason Wang wrote:
> > On Thu, Dec 9, 2021 at 4:26 PM Eli Cohen <elic@nvidia.com> wrote:
> > >
> > > On Thu, Dec 09, 2021 at 04:17:22PM +0800, Jason Wang wrote:
> > > > On Thu, Dec 9, 2021 at 4:04 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > >
> > > > > On Thu, Dec 09, 2021 at 03:55:48PM +0800, Jason Wang wrote:
> > > > > > On Thu, Dec 9, 2021 at 3:09 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > > > >
> > > > > > > On Thu, Dec 09, 2021 at 01:44:56PM +0800, Jason Wang wrote:
> > > > > > > > On Thu, Dec 9, 2021 at 4:15 AM Eli Cohen <elic@nvidia.com> wrote:
> > > > > > > > >
> > > > > > > > > Add netlink attribute and callback function to query the control VQ
> > > > > > > > > index of a device.
> > > > > > > > >
> > > > > > > > > Example:
> > > > > > > > >
> > > > > > > > > $ vdpa dev config show vdpa-a
> > > > > > > > > vdpa-a: mac 00:00:00:00:88:88 link up link_announce false max_vq_pairs 5 \
> > > > > > > > > mtu 9000 ctrl_vq_idx 10
> > > > > > > >
> > > > > > > >
> > > > > > > > I still wonder about the motivation for this.
> > > > > > > To be able to show the stats for CVQ.
> > > > > >
> > > > > > Right.
> > > > > >
> > > > > > >
> > > > > > > > And as discussed, the
> > > > > > > > ctrl_vq_idx varies depending on whether MQ is negotiated.
> > > > > > >
> > > > > > > I handle this according to the spec and it depends on MQ.
> > > > > >
> > > > > > Yes, but there could be a chance that the cvq index changes after the
> > > > > > vdpa dev config show.
> > > > > >
> > > > >
> > > > > It depends on:
> > > > > VIRTIO_NET_F_CTRL_VQ
> > > > > VIRTIO_NET_F_MQ
> > > > >
> > > > > which can change only if the features are re-negotiated and that happens
> > > > > on driver in itialization.
> > > > >
> > > > > And on max_virtqueue_pairs which is also set at driver initialization.
> > > > >
> > > > > So I don't see any real issue here. Am I missing something?
> > > >
> > > > No. I meant technically there could be a re-negotiation that happens
> > > > in the middle:
> > > >
> > > > 1) vdpa dev config show
> > > > 2) feature renegotiation which change the cvq index
> > > > 3) getting cvq stats
> > > >
> > > > So the cvq index might be changed. E.g it could be changed from a cvq
> > > > to a rx queue. It might be easier to make the get index and stats
> > > > atomic.
> > > >
> > >
> > > The interface to read VQ stats is based on the user providing the index
> > > and getting the statistics.
> > >
> > > If we want to follow your suggestion then we need maybe to use a
> > > slightly modified command:
> > >
> > > For regular VQ:
> > > dpa dev vstats show vdpa-a qidx 0
> > >
> > > For CVQ:
> > > vdpa dev vstats show vdpa-a cvq
> >
> > This might be better, but we need to make sure cvq only works for the
> > device that has a cvq.
> >
>
> OK.
>
> > >
> > > And that raises another question. Do we want to report the CVQ index in
> > > config show?
> >
> > If we go with stats show vdpa-a cvq, we don't need to report the cvq index.
> >
>
> Maybe we should leave some kind of indication as to whether there is a
> CVQ. Maybe not the index itself by just "CVQ" so a user will know if
> it's worth trying to read cvq stats.
>
> Otherwise, the user will attempt to read and fail.
> What do you think?
I agree.
Thanks
>
> > Thanks
> >
> > >
> > > > Thanks
> > > >
> > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > > > > ---
> > > > > > > > > v0 -> v1:
> > > > > > > > > 1. Use logic defined in the spec to deduce virtqueue index.
> > > > > > > > >
> > > > > > > > > drivers/vdpa/vdpa.c | 25 +++++++++++++++++++++++++
> > > > > > > > > include/uapi/linux/vdpa.h | 1 +
> > > > > > > > > 2 files changed, 26 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > > > > > > > index 3bf016e03512..b4d4b8a7ca4e 100644
> > > > > > > > > --- a/drivers/vdpa/vdpa.c
> > > > > > > > > +++ b/drivers/vdpa/vdpa.c
> > > > > > > > > @@ -712,6 +712,27 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct sk_buff *msg, struct netlink_callba
> > > > > > > > > return msg->len;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > +static int vdpa_dev_net_ctrl_vq_fill(struct vdpa_device *vdev,
> > > > > > > > > + struct sk_buff *msg,
> > > > > > > > > + struct virtio_net_config *config,
> > > > > > > > > + u64 features)
> > > > > > > > > +{
> > > > > > > > > + u16 N;
> > > > > > > > > +
> > > > > > > > > + /* control VQ index, if available, is deduced according to the logic
> > > > > > > > > + * described in the virtio spec in section 5.1.2
> > > > > > > > > + */
> > > > > > > > > + if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > > > > > > + return 0;
> > > > > > > > > +
> > > > > > > > > + if (features & BIT_ULL(VIRTIO_NET_F_MQ))
> > > > > > > > > + N = le16_to_cpu(config->max_virtqueue_pairs);
> > > > > > > > > + else
> > > > > > > > > + N = 1;
> > > > > > > > > +
> > > > > > > > > + return nla_put_u16(msg, VDPA_ATTR_DEV_CTRL_VQ_IDX, 2 * N);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
> > > > > > > > > struct sk_buff *msg, u64 features,
> > > > > > > > > const struct virtio_net_config *config)
> > > > > > > > > @@ -730,6 +751,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> > > > > > > > > struct virtio_net_config config = {};
> > > > > > > > > u64 features;
> > > > > > > > > u16 val_u16;
> > > > > > > > > + int err;
> > > > > > > > >
> > > > > > > > > vdpa_get_config(vdev, 0, &config, sizeof(config));
> > > > > > > > >
> > > > > > > > > @@ -746,6 +768,9 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> > > > > > > > > return -EMSGSIZE;
> > > > > > > > >
> > > > > > > > > features = vdev->config->get_driver_features(vdev);
> > > > > > > > > + err = vdpa_dev_net_ctrl_vq_fill(vdev, msg, &config, features);
> > > > > > > > > + if (err)
> > > > > > > > > + return err;
> > > > > > > > >
> > > > > > > > > return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> > > > > > > > > }
> > > > > > > > > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > > > > > > > > index a252f06f9dfd..2e3a7f89f42d 100644
> > > > > > > > > --- a/include/uapi/linux/vdpa.h
> > > > > > > > > +++ b/include/uapi/linux/vdpa.h
> > > > > > > > > @@ -34,6 +34,7 @@ enum vdpa_attr {
> > > > > > > > > VDPA_ATTR_DEV_MAX_VQS, /* u32 */
> > > > > > > > > VDPA_ATTR_DEV_MAX_VQ_SIZE, /* u16 */
> > > > > > > > > VDPA_ATTR_DEV_MIN_VQ_SIZE, /* u16 */
> > > > > > > > > + VDPA_ATTR_DEV_CTRL_VQ_IDX, /* u16 */
> > > > > > > > >
> > > > > > > > > VDPA_ATTR_DEV_NET_CFG_MACADDR, /* binary */
> > > > > > > > > VDPA_ATTR_DEV_NET_STATUS, /* u8 */
> > > > > > > > > --
> > > > > > > > > 2.33.1
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 3/7] vdpa: Allow to configure max data virtqueues
2021-12-09 21:50 ` Si-Wei Liu
@ 2021-12-10 2:29 ` Jason Wang
[not found] ` <20211213064453.GD41423@mtl-vdi-166.wap.labs.mlnx>
0 siblings, 1 reply; 22+ messages in thread
From: Jason Wang @ 2021-12-10 2:29 UTC (permalink / raw)
To: Si-Wei Liu; +Cc: Laurent Vivier, mst, virtualization, eperezma, Eli Cohen
On Fri, Dec 10, 2021 at 5:51 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 12/8/2021 9:36 PM, Jason Wang wrote:
> > On Thu, Dec 9, 2021 at 8:25 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> >>
> >>
> >> On 12/8/2021 12:14 PM, Eli Cohen wrote:
> >>> Add netlink support to configure the max virtqueue pairs for a device.
> >>> At least one pair is required. The maximum is dictated by the device.
> >>>
> >>> Example:
> >>>
> >>> $ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 max_vqp 5
> >> Not this patch, but I think there should be a mega attribute defined
> >> ahead to specify the virtio class/type to create vdpa instance with.
> >> Like the existing ones e.g. mac_addr and mtu, max_vqp is also vdpa net
> >> specific attribute.
> > Probably, but this only works for the case when a single parent is
> > allowed to create different types of devices. It looks to me the
> > current model to have a per type parent.
> Yes, that is the current situation and implementation, although the
> model does allow multi-type parent through
> VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES.
Right, so I agree with you, we need to specify the class first.
> Regardless, even though with
> single-type parent, so far on vdpa creation there's no validation done
> yet against which class/type the parent can support. The max-vqp is
> essentially vdpa network device only, but command line users don't have
> a means to infer it is structured this way, and/or which vdpa parent may
> support this config attribute. That said, were the command line usage
> structured like below, I would have less worry.
>
> $ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 net.max_vqp 5
It might be easier to specify class as a dedicated parameter, since we
may want to specify mtu and mac.
Thanks
>
> Alternatively, if the goal is to keep the attribute flat, we may show
> the mapping for the parent capability and the supported class:
>
> $ vdpa mgmtdev capab show
> pci/0000:41:00.2:
> class: net
> mac: off
> mtu: on
> max_mtu: 9000
> max_vqp: 1
> auxiliary/mlx5_core.sf.1:
> class: net
> mac: on
> mtu: off
> max_mtu: 1500
> max_vqp: 256
>
> Thanks,
> -Siwei
>
> >
> > Thanks
> >
> >> -Siwei
> >>
> >>> Signed-off-by: Eli Cohen <elic@nvidia.com>
> >>> ---
> >>> v0 -> v1:
> >>> 1. fix typo
> >>> 2. move max_vq_pairs to net specific struct
> >>>
> >>> drivers/vdpa/vdpa.c | 14 +++++++++++++-
> >>> include/linux/vdpa.h | 1 +
> >>> 2 files changed, 14 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >>> index c37d384c0f33..3bf016e03512 100644
> >>> --- a/drivers/vdpa/vdpa.c
> >>> +++ b/drivers/vdpa/vdpa.c
> >>> @@ -480,7 +480,8 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
> >>> }
> >>>
> >>> #define VDPA_DEV_NET_ATTRS_MASK ((1 << VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
> >>> - (1 << VDPA_ATTR_DEV_NET_CFG_MTU))
> >>> + (1 << VDPA_ATTR_DEV_NET_CFG_MTU) | \
> >>> + (1 << VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
> >>>
> >>> static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *info)
> >>> {
> >>> @@ -506,6 +507,17 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
> >>> nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]);
> >>> config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MTU);
> >>> }
> >>> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]) {
> >>> + config.net.max_vq_pairs =
> >>> + nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]);
> >>> + if (!config.net.max_vq_pairs) {
> >>> + NL_SET_ERR_MSG_MOD(info->extack,
> >>> + "At least one pair of VQs is required");
> >>> + err = -EINVAL;
> >>> + goto err;
> >>> + }
> >>> + config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
> >>> + }
> >>>
> >>> /* Skip checking capability if user didn't prefer to configure any
> >>> * device networking attributes. It is likely that user might have used
> >>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> >>> index db24317d61b6..b62032573780 100644
> >>> --- a/include/linux/vdpa.h
> >>> +++ b/include/linux/vdpa.h
> >>> @@ -99,6 +99,7 @@ struct vdpa_dev_set_config {
> >>> struct {
> >>> u8 mac[ETH_ALEN];
> >>> u16 mtu;
> >>> + u16 max_vq_pairs;
> >>> } net;
> >>> u64 mask;
> >>> };
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 7/7] vdpa/mlx5: Restore cur_num_vqs in case of failure in change_num_qps()
[not found] ` <20211208201430.73720-8-elic@nvidia.com>
@ 2021-12-10 8:17 ` Si-Wei Liu
0 siblings, 0 replies; 22+ messages in thread
From: Si-Wei Liu @ 2021-12-10 8:17 UTC (permalink / raw)
To: Eli Cohen, mst, jasowang, virtualization; +Cc: lvivier, eperezma
On 12/8/2021 12:14 PM, Eli Cohen wrote:
> Restore ndev->cur_num_vqs to the original value in case change_num_qps()
> fails.
>
> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> Acked-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 4f0b8bba8b58..2d37240e8bfc 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1539,6 +1539,8 @@ static int change_num_qps(struct mlx5_vdpa_dev *mvdev, int newqps)
> for (--i; i >= cur_qps; --i)
Should the condition be i >= cur_qps*2 instead?
> teardown_vq(ndev, &ndev->vqs[i]);
>
> + ndev->cur_num_vqs = 2 * cur_qps;
> +
> return err;
> }
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 2/7] vdpa/mlx5: Distribute RX virtqueues in RQT object
[not found] ` <20211209065504.GB108239@mtl-vdi-166.wap.labs.mlnx>
@ 2021-12-10 8:26 ` Si-Wei Liu
0 siblings, 0 replies; 22+ messages in thread
From: Si-Wei Liu @ 2021-12-10 8:26 UTC (permalink / raw)
To: Eli Cohen; +Cc: lvivier, mst, virtualization, eperezma
On 12/8/2021 10:55 PM, Eli Cohen wrote:
> On Wed, Dec 08, 2021 at 04:12:33PM -0800, Si-Wei Liu wrote:
>>
>> On 12/8/2021 12:14 PM, Eli Cohen wrote:
>>> Distribute the available rx virtqueues amongst the available RQT
>>> entries.
>>>
>>> RQTs require to have a power of two entries. When creating or modifying
>>> the RQT, use the lowest number of power of two entries that is not less
>>> than the number of rx virtqueues. Distribute them in the available
>>> entries such that some virtqueus may be referenced twice.
>>>
>>> This allows to configure any number of virtqueue pairs when multiqueue
>>> is used.
>>>
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> ---
>>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 30 +++++++-----------------------
>>> 1 file changed, 7 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> index ce2e13135dd8..e1a8a790f213 100644
>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> @@ -1261,17 +1261,10 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>>> MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
>>> MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
>>> list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
>>> - for (i = 0, j = 0; j < max_rqt; j++) {
>>> - if (!ndev->vqs[j].initialized)
>>> - continue;
>> Why the !initialized check is dropped from the new code?
> We now access the vqs array with j % ndev->mvdev.max_vqs and that keeps
> us in the range of valid indices.
OK. I thought the previous check was more for defensive coding and it
doesn't bother if keeping it. Anyway,
Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
>
>>> -
>>> - if (!vq_is_tx(ndev->vqs[j].index)) {
>>> - list[i] = cpu_to_be32(ndev->vqs[j].virtq_id);
>>> - i++;
>>> - }
>>> - }
>>> - MLX5_SET(rqtc, rqtc, rqt_actual_size, i);
>>> + for (i = 0, j = 0; i < max_rqt; i++, j += 2)
>>> + list[i] = cpu_to_be32(ndev->vqs[j % ndev->mvdev.max_vqs].virtq_id);
>>> + MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
>>> err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);
>>> kfree(in);
>>> if (err)
>>> @@ -1292,7 +1285,7 @@ static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
>>> int i, j;
>>> int err;
>>> - max_rqt = min_t(int, ndev->cur_num_vqs / 2,
>>> + max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
>>> 1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
>>> if (max_rqt < 1)
>>> return -EOPNOTSUPP;
>>> @@ -1308,16 +1301,10 @@ static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
>>> MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
>>> list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
>>> - for (i = 0, j = 0; j < num; j++) {
>>> - if (!ndev->vqs[j].initialized)
>>> - continue;
>> Ditto.
> Same as above.
>
>> -Siwei
>>
>>> + for (i = 0, j = 0; i < max_rqt; i++, j += 2)
>>> + list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
>>> - if (!vq_is_tx(ndev->vqs[j].index)) {
>>> - list[i] = cpu_to_be32(ndev->vqs[j].virtq_id);
>>> - i++;
>>> - }
>>> - }
>>> - MLX5_SET(rqtc, rqtc, rqt_actual_size, i);
>>> + MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
>>> err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, ndev->res.rqtn);
>>> kfree(in);
>>> if (err)
>>> @@ -1581,9 +1568,6 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>>> break;
>>> }
>>> - if (newqps & (newqps - 1))
>>> - break;
>>> -
>>> if (!change_num_qps(mvdev, newqps))
>>> status = VIRTIO_NET_OK;
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/7] vdpa: Provide interface to read driver features
2021-12-09 22:29 ` Si-Wei Liu
@ 2021-12-12 9:45 ` Michael S. Tsirkin
[not found] ` <20211212133854.GA8840@mtl-vdi-166.wap.labs.mlnx>
1 sibling, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2021-12-12 9:45 UTC (permalink / raw)
To: Si-Wei Liu; +Cc: lvivier, virtualization, eperezma, Eli Cohen
On Thu, Dec 09, 2021 at 02:29:17PM -0800, Si-Wei Liu wrote:
>
>
> On 12/8/2021 10:47 PM, Eli Cohen wrote:
> > On Wed, Dec 08, 2021 at 03:57:21PM -0800, Si-Wei Liu wrote:
> > >
> > > On 12/8/2021 12:14 PM, Eli Cohen wrote:
> > > > Provide an interface to read the negotiated features. This is needed
> > > > when building the netlink message in vdpa_dev_net_config_fill().
> > > >
> > > > Also fix the implementation of vdpa_dev_net_config_fill() to use the
> > > > negotiated features instead of the device features.
> > > Are we sure the use of device feature is a bug rather than expected
> > > behavior?
> > I think so since in vdpa_dev_net_config_fill() we're returning the
> > current configuration so you you don't want to mislead the user with
> > wrong information.
> You seem to imply vdpa_dev_net_config_fill() should return the
> current/running driver config rather than the initial setting of the device
> side before feature negotiation kicks in. This seems to work for the running
> link status which is propagated to the configuration space, but how do you
> infer the other untrackable running config the driver is operating, such as
> mtu, the effective value of which is not written back to config space or
> propagated back to vdpa backend?
>
> >
> > > How do users determine the configured number of queue pairs at any
> > > point in case of a non-multiqueue supporting guest/driver or that the device
> > > is being reset (where actual_features == 0)?
> > I would think if you read during reset (which is really a short period
> > of time), you get what there is at the moment.
> I would stress out the case if guest not supporting multi-queue. For e.g
> guest firmware might only support single queue without control queue, which
> is not considered to be transient. Since the validity of the value connects
> to feature negotiation, how does the host admin user infer feature
> negotiation is complete? What is the administrative value if they have to
> keep track of the change without knowing the effective negotiation status?
>
> Thanks,
> -Siwei
Right now I think the assumption is that features can be
overwritten multiple times.
I would say the way to detect feature negotiation is complete for
a modern guest would be by using set_status.
Legacy guests also kick before set_status, once they do
feature negotiation is complete.
Having said all that, ability to read config before FEATURES_OK
only caused us pain so far. I think we should at least discourage it
at the spec level.
> >
> > > Maybe we should differentiate
> > > the static device config against driver/device running state here. I thought
> > > the change to vdpa_dev_net_config_fill() in this patch would break vdpa tool
> > > user's expectation that the value of max_vqp config is a fixed value since
> > > the vdpa creation time.
> > That was not intended to be fixed AFAIU.
> >
> > > Perhaps worth adding another attribute to represent the running state
> > > (cur_max_qps) based on the negotiated features.
> > >
> > You can get that information by running e.g. ethtool -l ens1.
> >
> > > -Siwei
> > >
> > > > To make APIs clearer, make the following name changes to struct
> > > > vdpa_config_ops so they better describe their operations:
> > > >
> > > > get_features -> get_device_features
> > > > set_features -> set_driver_features
> > > >
> > > > Finally, add get_driver_features to return the negotiated features and
> > > > add implementation to all the upstream drivers.
> > > >
> > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > ---
> > > > drivers/vdpa/alibaba/eni_vdpa.c | 16 ++++++++++++----
> > > > drivers/vdpa/ifcvf/ifcvf_main.c | 16 ++++++++++++----
> > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 16 ++++++++++++----
> > > > drivers/vdpa/vdpa.c | 2 +-
> > > > drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +++++++++++++++------
> > > > drivers/vdpa/vdpa_user/vduse_dev.c | 16 ++++++++++++----
> > > > drivers/vdpa/virtio_pci/vp_vdpa.c | 16 ++++++++++++----
> > > > drivers/vhost/vdpa.c | 2 +-
> > > > drivers/virtio/virtio_vdpa.c | 2 +-
> > > > include/linux/vdpa.h | 14 +++++++++-----
> > > > 10 files changed, 87 insertions(+), 34 deletions(-)
> > > >
> > > > diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c
> > > > index 3f788794571a..ac0975660f4d 100644
> > > > --- a/drivers/vdpa/alibaba/eni_vdpa.c
> > > > +++ b/drivers/vdpa/alibaba/eni_vdpa.c
> > > > @@ -58,7 +58,7 @@ static struct virtio_pci_legacy_device *vdpa_to_ldev(struct vdpa_device *vdpa)
> > > > return &eni_vdpa->ldev;
> > > > }
> > > > -static u64 eni_vdpa_get_features(struct vdpa_device *vdpa)
> > > > +static u64 eni_vdpa_get_device_features(struct vdpa_device *vdpa)
> > > > {
> > > > struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
> > > > u64 features = vp_legacy_get_features(ldev);
> > > > @@ -69,7 +69,7 @@ static u64 eni_vdpa_get_features(struct vdpa_device *vdpa)
> > > > return features;
> > > > }
> > > > -static int eni_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> > > > +static int eni_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
> > > > {
> > > > struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
> > > > @@ -84,6 +84,13 @@ static int eni_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> > > > return 0;
> > > > }
> > > > +static u64 eni_vdpa_get_driver_features(struct vdpa_device *vdpa)
> > > > +{
> > > > + struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
> > > > +
> > > > + return vp_legacy_get_driver_features(ldev);
> > > > +}
> > > > +
> > > > static u8 eni_vdpa_get_status(struct vdpa_device *vdpa)
> > > > {
> > > > struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
> > > > @@ -401,8 +408,9 @@ static void eni_vdpa_set_config_cb(struct vdpa_device *vdpa,
> > > > }
> > > > static const struct vdpa_config_ops eni_vdpa_ops = {
> > > > - .get_features = eni_vdpa_get_features,
> > > > - .set_features = eni_vdpa_set_features,
> > > > + .get_device_features = eni_vdpa_get_device_features,
> > > > + .set_driver_features = eni_vdpa_set_driver_features,
> > > > + .get_driver_features = eni_vdpa_get_driver_features,
> > > > .get_status = eni_vdpa_get_status,
> > > > .set_status = eni_vdpa_set_status,
> > > > .reset = eni_vdpa_reset,
> > > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> > > > index 6dc75ca70b37..6a6a0a462392 100644
> > > > --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> > > > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> > > > @@ -169,7 +169,7 @@ static struct ifcvf_hw *vdpa_to_vf(struct vdpa_device *vdpa_dev)
> > > > return &adapter->vf;
> > > > }
> > > > -static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev)
> > > > +static u64 ifcvf_vdpa_get_device_features(struct vdpa_device *vdpa_dev)
> > > > {
> > > > struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
> > > > struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> > > > @@ -187,7 +187,7 @@ static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev)
> > > > return features;
> > > > }
> > > > -static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, u64 features)
> > > > +static int ifcvf_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 features)
> > > > {
> > > > struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> > > > int ret;
> > > > @@ -201,6 +201,13 @@ static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, u64 features)
> > > > return 0;
> > > > }
> > > > +static u64 ifcvf_vdpa_get_driver_features(struct vdpa_device *vdpa_dev)
> > > > +{
> > > > + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> > > > +
> > > > + return vf->req_features;
> > > > +}
> > > > +
> > > > static u8 ifcvf_vdpa_get_status(struct vdpa_device *vdpa_dev)
> > > > {
> > > > struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> > > > @@ -443,8 +450,9 @@ static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_devic
> > > > * implemented set_map()/dma_map()/dma_unmap()
> > > > */
> > > > static const struct vdpa_config_ops ifc_vdpa_ops = {
> > > > - .get_features = ifcvf_vdpa_get_features,
> > > > - .set_features = ifcvf_vdpa_set_features,
> > > > + .get_device_features = ifcvf_vdpa_get_device_features,
> > > > + .set_driver_features = ifcvf_vdpa_set_driver_features,
> > > > + .get_driver_features = ifcvf_vdpa_get_driver_features,
> > > > .get_status = ifcvf_vdpa_get_status,
> > > > .set_status = ifcvf_vdpa_set_status,
> > > > .reset = ifcvf_vdpa_reset,
> > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > index 63813fbb5f62..ce2e13135dd8 100644
> > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > @@ -1880,7 +1880,7 @@ static u64 mlx_to_vritio_features(u16 dev_features)
> > > > return result;
> > > > }
> > > > -static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev)
> > > > +static u64 mlx5_vdpa_get_device_features(struct vdpa_device *vdev)
> > > > {
> > > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > @@ -1972,7 +1972,7 @@ static void update_cvq_info(struct mlx5_vdpa_dev *mvdev)
> > > > }
> > > > }
> > > > -static int mlx5_vdpa_set_features(struct vdpa_device *vdev, u64 features)
> > > > +static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> > > > {
> > > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > @@ -2339,6 +2339,13 @@ static int mlx5_get_vq_irq(struct vdpa_device *vdv, u16 idx)
> > > > return -EOPNOTSUPP;
> > > > }
> > > > +static u64 mlx5_vdpa_get_driver_features(struct vdpa_device *vdev)
> > > > +{
> > > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > +
> > > > + return mvdev->actual_features;
> > > > +}
> > > > +
> > > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > > @@ -2351,8 +2358,9 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > .get_vq_notification = mlx5_get_vq_notification,
> > > > .get_vq_irq = mlx5_get_vq_irq,
> > > > .get_vq_align = mlx5_vdpa_get_vq_align,
> > > > - .get_features = mlx5_vdpa_get_features,
> > > > - .set_features = mlx5_vdpa_set_features,
> > > > + .get_device_features = mlx5_vdpa_get_device_features,
> > > > + .set_driver_features = mlx5_vdpa_set_driver_features,
> > > > + .get_driver_features = mlx5_vdpa_get_driver_features,
> > > > .set_config_cb = mlx5_vdpa_set_config_cb,
> > > > .get_vq_num_max = mlx5_vdpa_get_vq_num_max,
> > > > .get_device_id = mlx5_vdpa_get_device_id,
> > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > > index 7332a74a4b00..c37d384c0f33 100644
> > > > --- a/drivers/vdpa/vdpa.c
> > > > +++ b/drivers/vdpa/vdpa.c
> > > > @@ -733,7 +733,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> > > > if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
> > > > return -EMSGSIZE;
> > > > - features = vdev->config->get_features(vdev);
> > > > + features = vdev->config->get_driver_features(vdev);
> > > > return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> > > > }
> > > > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > > > index 41b0cd17fcba..ddbe142af09a 100644
> > > > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > > > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > > > @@ -399,14 +399,14 @@ static u32 vdpasim_get_vq_align(struct vdpa_device *vdpa)
> > > > return VDPASIM_QUEUE_ALIGN;
> > > > }
> > > > -static u64 vdpasim_get_features(struct vdpa_device *vdpa)
> > > > +static u64 vdpasim_get_device_features(struct vdpa_device *vdpa)
> > > > {
> > > > struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> > > > return vdpasim->dev_attr.supported_features;
> > > > }
> > > > -static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
> > > > +static int vdpasim_set_driver_features(struct vdpa_device *vdpa, u64 features)
> > > > {
> > > > struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> > > > @@ -419,6 +419,13 @@ static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
> > > > return 0;
> > > > }
> > > > +static u64 vdpasim_get_driver_features(struct vdpa_device *vdpa)
> > > > +{
> > > > + struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> > > > +
> > > > + return vdpasim->features;
> > > > +}
> > > > +
> > > > static void vdpasim_set_config_cb(struct vdpa_device *vdpa,
> > > > struct vdpa_callback *cb)
> > > > {
> > > > @@ -613,8 +620,9 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
> > > > .set_vq_state = vdpasim_set_vq_state,
> > > > .get_vq_state = vdpasim_get_vq_state,
> > > > .get_vq_align = vdpasim_get_vq_align,
> > > > - .get_features = vdpasim_get_features,
> > > > - .set_features = vdpasim_set_features,
> > > > + .get_device_features = vdpasim_get_device_features,
> > > > + .set_driver_features = vdpasim_set_driver_features,
> > > > + .get_driver_features = vdpasim_get_driver_features,
> > > > .set_config_cb = vdpasim_set_config_cb,
> > > > .get_vq_num_max = vdpasim_get_vq_num_max,
> > > > .get_device_id = vdpasim_get_device_id,
> > > > @@ -642,8 +650,9 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
> > > > .set_vq_state = vdpasim_set_vq_state,
> > > > .get_vq_state = vdpasim_get_vq_state,
> > > > .get_vq_align = vdpasim_get_vq_align,
> > > > - .get_features = vdpasim_get_features,
> > > > - .set_features = vdpasim_set_features,
> > > > + .get_device_features = vdpasim_get_device_features,
> > > > + .set_driver_features = vdpasim_set_driver_features,
> > > > + .get_driver_features = vdpasim_get_driver_features,
> > > > .set_config_cb = vdpasim_set_config_cb,
> > > > .get_vq_num_max = vdpasim_get_vq_num_max,
> > > > .get_device_id = vdpasim_get_device_id,
> > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > index c9204c62f339..1e65af6ab9ae 100644
> > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > @@ -573,14 +573,14 @@ static u32 vduse_vdpa_get_vq_align(struct vdpa_device *vdpa)
> > > > return dev->vq_align;
> > > > }
> > > > -static u64 vduse_vdpa_get_features(struct vdpa_device *vdpa)
> > > > +static u64 vduse_vdpa_get_device_features(struct vdpa_device *vdpa)
> > > > {
> > > > struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> > > > return dev->device_features;
> > > > }
> > > > -static int vduse_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> > > > +static int vduse_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
> > > > {
> > > > struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> > > > @@ -588,6 +588,13 @@ static int vduse_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> > > > return 0;
> > > > }
> > > > +static u64 vduse_vdpa_get_driver_features(struct vdpa_device *vdpa)
> > > > +{
> > > > + struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> > > > +
> > > > + return dev->driver_features;
> > > > +}
> > > > +
> > > > static void vduse_vdpa_set_config_cb(struct vdpa_device *vdpa,
> > > > struct vdpa_callback *cb)
> > > > {
> > > > @@ -720,8 +727,9 @@ static const struct vdpa_config_ops vduse_vdpa_config_ops = {
> > > > .set_vq_state = vduse_vdpa_set_vq_state,
> > > > .get_vq_state = vduse_vdpa_get_vq_state,
> > > > .get_vq_align = vduse_vdpa_get_vq_align,
> > > > - .get_features = vduse_vdpa_get_features,
> > > > - .set_features = vduse_vdpa_set_features,
> > > > + .get_device_features = vduse_vdpa_get_device_features,
> > > > + .set_driver_features = vduse_vdpa_set_driver_features,
> > > > + .get_driver_features = vduse_vdpa_get_driver_features,
> > > > .set_config_cb = vduse_vdpa_set_config_cb,
> > > > .get_vq_num_max = vduse_vdpa_get_vq_num_max,
> > > > .get_device_id = vduse_vdpa_get_device_id,
> > > > diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > > index e3ff7875e123..97285bc05e47 100644
> > > > --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > > +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > > @@ -53,14 +53,14 @@ static struct virtio_pci_modern_device *vdpa_to_mdev(struct vdpa_device *vdpa)
> > > > return &vp_vdpa->mdev;
> > > > }
> > > > -static u64 vp_vdpa_get_features(struct vdpa_device *vdpa)
> > > > +static u64 vp_vdpa_get_device_features(struct vdpa_device *vdpa)
> > > > {
> > > > struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
> > > > return vp_modern_get_features(mdev);
> > > > }
> > > > -static int vp_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> > > > +static int vp_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
> > > > {
> > > > struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
> > > > @@ -69,6 +69,13 @@ static int vp_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> > > > return 0;
> > > > }
> > > > +static u64 vp_vdpa_get_driver_features(struct vdpa_device *vdpa)
> > > > +{
> > > > + struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
> > > > +
> > > > + return vp_modern_get_features(mdev);
> > > > +}
> > > > +
> > > > static u8 vp_vdpa_get_status(struct vdpa_device *vdpa)
> > > > {
> > > > struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
> > > > @@ -415,8 +422,9 @@ vp_vdpa_get_vq_notification(struct vdpa_device *vdpa, u16 qid)
> > > > }
> > > > static const struct vdpa_config_ops vp_vdpa_ops = {
> > > > - .get_features = vp_vdpa_get_features,
> > > > - .set_features = vp_vdpa_set_features,
> > > > + .get_device_features = vp_vdpa_get_device_features,
> > > > + .set_driver_features = vp_vdpa_set_driver_features,
> > > > + .get_driver_features = vp_vdpa_get_driver_features,
> > > > .get_status = vp_vdpa_get_status,
> > > > .set_status = vp_vdpa_set_status,
> > > > .reset = vp_vdpa_reset,
> > > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > > index 29cced1cd277..6d087a4fb581 100644
> > > > --- a/drivers/vhost/vdpa.c
> > > > +++ b/drivers/vhost/vdpa.c
> > > > @@ -262,7 +262,7 @@ static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
> > > > const struct vdpa_config_ops *ops = vdpa->config;
> > > > u64 features;
> > > > - features = ops->get_features(vdpa);
> > > > + features = ops->get_device_features(vdpa);
> > > > if (copy_to_user(featurep, &features, sizeof(features)))
> > > > return -EFAULT;
> > > > diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> > > > index f85f860bc10b..a84b04ba3195 100644
> > > > --- a/drivers/virtio/virtio_vdpa.c
> > > > +++ b/drivers/virtio/virtio_vdpa.c
> > > > @@ -308,7 +308,7 @@ static u64 virtio_vdpa_get_features(struct virtio_device *vdev)
> > > > struct vdpa_device *vdpa = vd_get_vdpa(vdev);
> > > > const struct vdpa_config_ops *ops = vdpa->config;
> > > > - return ops->get_features(vdpa);
> > > > + return ops->get_device_features(vdpa);
> > > > }
> > > > static int virtio_vdpa_finalize_features(struct virtio_device *vdev)
> > > > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> > > > index c3011ccda430..db24317d61b6 100644
> > > > --- a/include/linux/vdpa.h
> > > > +++ b/include/linux/vdpa.h
> > > > @@ -169,14 +169,17 @@ struct vdpa_map_file {
> > > > * for the device
> > > > * @vdev: vdpa device
> > > > * Returns virtqueue algin requirement
> > > > - * @get_features: Get virtio features supported by the device
> > > > + * @get_device_features: Get virtio features supported by the device
> > > > * @vdev: vdpa device
> > > > * Returns the virtio features support by the
> > > > * device
> > > > - * @set_features: Set virtio features supported by the driver
> > > > + * @set_driver_features: Set virtio features supported by the driver
> > > > * @vdev: vdpa device
> > > > * @features: feature support by the driver
> > > > * Returns integer: success (0) or error (< 0)
> > > > + * @get_driver_features: Get virtio features in action
> > > > + * @vdev: vdpa device
> > > > + * Returns the virtio features accepted
> > > > * @set_config_cb: Set the config interrupt callback
> > > > * @vdev: vdpa device
> > > > * @cb: virtio-vdev interrupt callback structure
> > > > @@ -276,8 +279,9 @@ struct vdpa_config_ops {
> > > > /* Device ops */
> > > > u32 (*get_vq_align)(struct vdpa_device *vdev);
> > > > - u64 (*get_features)(struct vdpa_device *vdev);
> > > > - int (*set_features)(struct vdpa_device *vdev, u64 features);
> > > > + u64 (*get_device_features)(struct vdpa_device *vdev);
> > > > + int (*set_driver_features)(struct vdpa_device *vdev, u64 features);
> > > > + u64 (*get_driver_features)(struct vdpa_device *vdev);
> > > > void (*set_config_cb)(struct vdpa_device *vdev,
> > > > struct vdpa_callback *cb);
> > > > u16 (*get_vq_num_max)(struct vdpa_device *vdev);
> > > > @@ -395,7 +399,7 @@ static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
> > > > const struct vdpa_config_ops *ops = vdev->config;
> > > > vdev->features_valid = true;
> > > > - return ops->set_features(vdev, features);
> > > > + return ops->set_driver_features(vdev, features);
> > > > }
> > > > void vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 6/7] vdpa: Add support for querying control virtqueue index
[not found] ` <20211209092053.GA113385@mtl-vdi-166.wap.labs.mlnx>
2021-12-10 2:27 ` Jason Wang
@ 2021-12-13 3:05 ` Jason Wang
1 sibling, 0 replies; 22+ messages in thread
From: Jason Wang @ 2021-12-13 3:05 UTC (permalink / raw)
To: Eli Cohen; +Cc: Laurent Vivier, mst, virtualization, eperezma, Si-Wei Liu
On Thu, Dec 9, 2021 at 5:21 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Thu, Dec 09, 2021 at 05:12:01PM +0800, Jason Wang wrote:
> > On Thu, Dec 9, 2021 at 4:26 PM Eli Cohen <elic@nvidia.com> wrote:
> > >
> > > On Thu, Dec 09, 2021 at 04:17:22PM +0800, Jason Wang wrote:
> > > > On Thu, Dec 9, 2021 at 4:04 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > >
> > > > > On Thu, Dec 09, 2021 at 03:55:48PM +0800, Jason Wang wrote:
> > > > > > On Thu, Dec 9, 2021 at 3:09 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > > > >
> > > > > > > On Thu, Dec 09, 2021 at 01:44:56PM +0800, Jason Wang wrote:
> > > > > > > > On Thu, Dec 9, 2021 at 4:15 AM Eli Cohen <elic@nvidia.com> wrote:
> > > > > > > > >
> > > > > > > > > Add netlink attribute and callback function to query the control VQ
> > > > > > > > > index of a device.
> > > > > > > > >
> > > > > > > > > Example:
> > > > > > > > >
> > > > > > > > > $ vdpa dev config show vdpa-a
> > > > > > > > > vdpa-a: mac 00:00:00:00:88:88 link up link_announce false max_vq_pairs 5 \
> > > > > > > > > mtu 9000 ctrl_vq_idx 10
> > > > > > > >
> > > > > > > >
> > > > > > > > I still wonder about the motivation for this.
> > > > > > > To be able to show the stats for CVQ.
> > > > > >
> > > > > > Right.
> > > > > >
> > > > > > >
> > > > > > > > And as discussed, the
> > > > > > > > ctrl_vq_idx varies depending on whether MQ is negotiated.
> > > > > > >
> > > > > > > I handle this according to the spec and it depends on MQ.
> > > > > >
> > > > > > Yes, but there could be a chance that the cvq index changes after the
> > > > > > vdpa dev config show.
> > > > > >
> > > > >
> > > > > It depends on:
> > > > > VIRTIO_NET_F_CTRL_VQ
> > > > > VIRTIO_NET_F_MQ
> > > > >
> > > > > which can change only if the features are re-negotiated and that happens
> > > > > on driver in itialization.
> > > > >
> > > > > And on max_virtqueue_pairs which is also set at driver initialization.
> > > > >
> > > > > So I don't see any real issue here. Am I missing something?
> > > >
> > > > No. I meant technically there could be a re-negotiation that happens
> > > > in the middle:
> > > >
> > > > 1) vdpa dev config show
> > > > 2) feature renegotiation which change the cvq index
> > > > 3) getting cvq stats
> > > >
> > > > So the cvq index might be changed. E.g it could be changed from a cvq
> > > > to a rx queue. It might be easier to make the get index and stats
> > > > atomic.
> > > >
> > >
> > > The interface to read VQ stats is based on the user providing the index
> > > and getting the statistics.
> > >
> > > If we want to follow your suggestion then we need maybe to use a
> > > slightly modified command:
> > >
> > > For regular VQ:
> > > dpa dev vstats show vdpa-a qidx 0
> > >
> > > For CVQ:
> > > vdpa dev vstats show vdpa-a cvq
> >
> > This might be better, but we need to make sure cvq only works for the
> > device that has a cvq.
> >
>
> OK.
>
> > >
> > > And that raises another question. Do we want to report the CVQ index in
> > > config show?
> >
> > If we go with stats show vdpa-a cvq, we don't need to report the cvq index.
> >
>
> Maybe we should leave some kind of indication as to whether there is a
> CVQ. Maybe not the index itself by just "CVQ" so a user will know if
> it's worth trying to read cvq stats.
>
> Otherwise, the user will attempt to read and fail.
> What do you think?
It should work.
Thanks
>
> > Thanks
> >
> > >
> > > > Thanks
> > > >
> > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > > > > ---
> > > > > > > > > v0 -> v1:
> > > > > > > > > 1. Use logic defined in the spec to deduce virtqueue index.
> > > > > > > > >
> > > > > > > > > drivers/vdpa/vdpa.c | 25 +++++++++++++++++++++++++
> > > > > > > > > include/uapi/linux/vdpa.h | 1 +
> > > > > > > > > 2 files changed, 26 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > > > > > > > index 3bf016e03512..b4d4b8a7ca4e 100644
> > > > > > > > > --- a/drivers/vdpa/vdpa.c
> > > > > > > > > +++ b/drivers/vdpa/vdpa.c
> > > > > > > > > @@ -712,6 +712,27 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct sk_buff *msg, struct netlink_callba
> > > > > > > > > return msg->len;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > +static int vdpa_dev_net_ctrl_vq_fill(struct vdpa_device *vdev,
> > > > > > > > > + struct sk_buff *msg,
> > > > > > > > > + struct virtio_net_config *config,
> > > > > > > > > + u64 features)
> > > > > > > > > +{
> > > > > > > > > + u16 N;
> > > > > > > > > +
> > > > > > > > > + /* control VQ index, if available, is deduced according to the logic
> > > > > > > > > + * described in the virtio spec in section 5.1.2
> > > > > > > > > + */
> > > > > > > > > + if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > > > > > > + return 0;
> > > > > > > > > +
> > > > > > > > > + if (features & BIT_ULL(VIRTIO_NET_F_MQ))
> > > > > > > > > + N = le16_to_cpu(config->max_virtqueue_pairs);
> > > > > > > > > + else
> > > > > > > > > + N = 1;
> > > > > > > > > +
> > > > > > > > > + return nla_put_u16(msg, VDPA_ATTR_DEV_CTRL_VQ_IDX, 2 * N);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
> > > > > > > > > struct sk_buff *msg, u64 features,
> > > > > > > > > const struct virtio_net_config *config)
> > > > > > > > > @@ -730,6 +751,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> > > > > > > > > struct virtio_net_config config = {};
> > > > > > > > > u64 features;
> > > > > > > > > u16 val_u16;
> > > > > > > > > + int err;
> > > > > > > > >
> > > > > > > > > vdpa_get_config(vdev, 0, &config, sizeof(config));
> > > > > > > > >
> > > > > > > > > @@ -746,6 +768,9 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> > > > > > > > > return -EMSGSIZE;
> > > > > > > > >
> > > > > > > > > features = vdev->config->get_driver_features(vdev);
> > > > > > > > > + err = vdpa_dev_net_ctrl_vq_fill(vdev, msg, &config, features);
> > > > > > > > > + if (err)
> > > > > > > > > + return err;
> > > > > > > > >
> > > > > > > > > return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> > > > > > > > > }
> > > > > > > > > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > > > > > > > > index a252f06f9dfd..2e3a7f89f42d 100644
> > > > > > > > > --- a/include/uapi/linux/vdpa.h
> > > > > > > > > +++ b/include/uapi/linux/vdpa.h
> > > > > > > > > @@ -34,6 +34,7 @@ enum vdpa_attr {
> > > > > > > > > VDPA_ATTR_DEV_MAX_VQS, /* u32 */
> > > > > > > > > VDPA_ATTR_DEV_MAX_VQ_SIZE, /* u16 */
> > > > > > > > > VDPA_ATTR_DEV_MIN_VQ_SIZE, /* u16 */
> > > > > > > > > + VDPA_ATTR_DEV_CTRL_VQ_IDX, /* u16 */
> > > > > > > > >
> > > > > > > > > VDPA_ATTR_DEV_NET_CFG_MACADDR, /* binary */
> > > > > > > > > VDPA_ATTR_DEV_NET_STATUS, /* u8 */
> > > > > > > > > --
> > > > > > > > > 2.33.1
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/7] vdpa: Provide interface to read driver featuresy
[not found] ` <20211212133854.GA8840@mtl-vdi-166.wap.labs.mlnx>
@ 2021-12-13 22:34 ` Si-Wei Liu
0 siblings, 0 replies; 22+ messages in thread
From: Si-Wei Liu @ 2021-12-13 22:34 UTC (permalink / raw)
To: Eli Cohen; +Cc: lvivier, mst, virtualization, eperezma
On 12/12/2021 5:38 AM, Eli Cohen wrote:
> On Thu, Dec 09, 2021 at 02:29:17PM -0800, Si-Wei Liu wrote:
>>
>> On 12/8/2021 10:47 PM, Eli Cohen wrote:
>>> On Wed, Dec 08, 2021 at 03:57:21PM -0800, Si-Wei Liu wrote:
>>>> On 12/8/2021 12:14 PM, Eli Cohen wrote:
>>>>> Provide an interface to read the negotiated features. This is needed
>>>>> when building the netlink message in vdpa_dev_net_config_fill().
>>>>>
>>>>> Also fix the implementation of vdpa_dev_net_config_fill() to use the
>>>>> negotiated features instead of the device features.
>>>> Are we sure the use of device feature is a bug rather than expected
>>>> behavior?
>>> I think so since in vdpa_dev_net_config_fill() we're returning the
>>> current configuration so you you don't want to mislead the user with
>>> wrong information.
>> You seem to imply vdpa_dev_net_config_fill() should return the
>> current/running driver config rather than the initial setting of the device
>> side before feature negotiation kicks in. This seems to work for the running
>> link status which is propagated to the configuration space, but how do you
>> infer the other untrackable running config the driver is operating, such as
>> mtu, the effective value of which is not written back to config space or
>> propagated back to vdpa backend?
>>
> I would think MTU should be configured by devlink before adding the
> device and reading should also be done using devlink. This is not
> supported currently and I am trying to have this supported although at
> least for ConnectX 6DX it might require to add firmware support.
Thanks for keeping me posted on MTU support. But again, what you said is
device side capability/config that is made static once vdpa is created.
What I'm concerned about is driver running config that is made by the
guest. Such as the effective number of queues via "ethtool -L" done in
the guest will show up in "vdpa dev show config".
However the mtu effective value for driver running config set by guest
"ip link set mtu ..." command won't show up in "vdpa dev show config".
This is on contrary with other vdpa config, which is why I think it'd be
quite confusing and misleading to show them all with mixed nomenclature
in the same place.
-Siwei
>
>>>> How do users determine the configured number of queue pairs at any
>>>> point in case of a non-multiqueue supporting guest/driver or that the device
>>>> is being reset (where actual_features == 0)?
>>> I would think if you read during reset (which is really a short period
>>> of time), you get what there is at the moment.
>> I would stress out the case if guest not supporting multi-queue. For e.g
>> guest firmware might only support single queue without control queue, which
>> is not considered to be transient. Since the validity of the value connects
>> to feature negotiation, how does the host admin user infer feature
>> negotiation is complete? What is the administrative value if they have to
>> keep track of the change without knowing the effective negotiation status?
> So you want to know how many VQ pairs the device supports? I think it is
> appropriate to return this information in VDPA_ATTR_DEV_MAX_VQS.
> Currently the returned value is not the device capapbilty but rather the
> device configured value.
> Does that make sense to you?
>
>> Thanks,
>> -Siwei
>>>> Maybe we should differentiate
>>>> the static device config against driver/device running state here. I thought
>>>> the change to vdpa_dev_net_config_fill() in this patch would break vdpa tool
>>>> user's expectation that the value of max_vqp config is a fixed value since
>>>> the vdpa creation time.
>>> That was not intended to be fixed AFAIU.
>>>
>>>> Perhaps worth adding another attribute to represent the running state
>>>> (cur_max_qps) based on the negotiated features.
>>>>
>>> You can get that information by running e.g. ethtool -l ens1.
>>>
>>>> -Siwei
>>>>
>>>>> To make APIs clearer, make the following name changes to struct
>>>>> vdpa_config_ops so they better describe their operations:
>>>>>
>>>>> get_features -> get_device_features
>>>>> set_features -> set_driver_features
>>>>>
>>>>> Finally, add get_driver_features to return the negotiated features and
>>>>> add implementation to all the upstream drivers.
>>>>>
>>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>>> ---
>>>>> drivers/vdpa/alibaba/eni_vdpa.c | 16 ++++++++++++----
>>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 16 ++++++++++++----
>>>>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 16 ++++++++++++----
>>>>> drivers/vdpa/vdpa.c | 2 +-
>>>>> drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +++++++++++++++------
>>>>> drivers/vdpa/vdpa_user/vduse_dev.c | 16 ++++++++++++----
>>>>> drivers/vdpa/virtio_pci/vp_vdpa.c | 16 ++++++++++++----
>>>>> drivers/vhost/vdpa.c | 2 +-
>>>>> drivers/virtio/virtio_vdpa.c | 2 +-
>>>>> include/linux/vdpa.h | 14 +++++++++-----
>>>>> 10 files changed, 87 insertions(+), 34 deletions(-)
>>>>>
>>>>> diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c
>>>>> index 3f788794571a..ac0975660f4d 100644
>>>>> --- a/drivers/vdpa/alibaba/eni_vdpa.c
>>>>> +++ b/drivers/vdpa/alibaba/eni_vdpa.c
>>>>> @@ -58,7 +58,7 @@ static struct virtio_pci_legacy_device *vdpa_to_ldev(struct vdpa_device *vdpa)
>>>>> return &eni_vdpa->ldev;
>>>>> }
>>>>> -static u64 eni_vdpa_get_features(struct vdpa_device *vdpa)
>>>>> +static u64 eni_vdpa_get_device_features(struct vdpa_device *vdpa)
>>>>> {
>>>>> struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
>>>>> u64 features = vp_legacy_get_features(ldev);
>>>>> @@ -69,7 +69,7 @@ static u64 eni_vdpa_get_features(struct vdpa_device *vdpa)
>>>>> return features;
>>>>> }
>>>>> -static int eni_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
>>>>> +static int eni_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
>>>>> {
>>>>> struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
>>>>> @@ -84,6 +84,13 @@ static int eni_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
>>>>> return 0;
>>>>> }
>>>>> +static u64 eni_vdpa_get_driver_features(struct vdpa_device *vdpa)
>>>>> +{
>>>>> + struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
>>>>> +
>>>>> + return vp_legacy_get_driver_features(ldev);
>>>>> +}
>>>>> +
>>>>> static u8 eni_vdpa_get_status(struct vdpa_device *vdpa)
>>>>> {
>>>>> struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
>>>>> @@ -401,8 +408,9 @@ static void eni_vdpa_set_config_cb(struct vdpa_device *vdpa,
>>>>> }
>>>>> static const struct vdpa_config_ops eni_vdpa_ops = {
>>>>> - .get_features = eni_vdpa_get_features,
>>>>> - .set_features = eni_vdpa_set_features,
>>>>> + .get_device_features = eni_vdpa_get_device_features,
>>>>> + .set_driver_features = eni_vdpa_set_driver_features,
>>>>> + .get_driver_features = eni_vdpa_get_driver_features,
>>>>> .get_status = eni_vdpa_get_status,
>>>>> .set_status = eni_vdpa_set_status,
>>>>> .reset = eni_vdpa_reset,
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> index 6dc75ca70b37..6a6a0a462392 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> @@ -169,7 +169,7 @@ static struct ifcvf_hw *vdpa_to_vf(struct vdpa_device *vdpa_dev)
>>>>> return &adapter->vf;
>>>>> }
>>>>> -static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev)
>>>>> +static u64 ifcvf_vdpa_get_device_features(struct vdpa_device *vdpa_dev)
>>>>> {
>>>>> struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>>>>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>> @@ -187,7 +187,7 @@ static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev)
>>>>> return features;
>>>>> }
>>>>> -static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, u64 features)
>>>>> +static int ifcvf_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 features)
>>>>> {
>>>>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>> int ret;
>>>>> @@ -201,6 +201,13 @@ static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, u64 features)
>>>>> return 0;
>>>>> }
>>>>> +static u64 ifcvf_vdpa_get_driver_features(struct vdpa_device *vdpa_dev)
>>>>> +{
>>>>> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>> +
>>>>> + return vf->req_features;
>>>>> +}
>>>>> +
>>>>> static u8 ifcvf_vdpa_get_status(struct vdpa_device *vdpa_dev)
>>>>> {
>>>>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>> @@ -443,8 +450,9 @@ static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_devic
>>>>> * implemented set_map()/dma_map()/dma_unmap()
>>>>> */
>>>>> static const struct vdpa_config_ops ifc_vdpa_ops = {
>>>>> - .get_features = ifcvf_vdpa_get_features,
>>>>> - .set_features = ifcvf_vdpa_set_features,
>>>>> + .get_device_features = ifcvf_vdpa_get_device_features,
>>>>> + .set_driver_features = ifcvf_vdpa_set_driver_features,
>>>>> + .get_driver_features = ifcvf_vdpa_get_driver_features,
>>>>> .get_status = ifcvf_vdpa_get_status,
>>>>> .set_status = ifcvf_vdpa_set_status,
>>>>> .reset = ifcvf_vdpa_reset,
>>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>> index 63813fbb5f62..ce2e13135dd8 100644
>>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>> @@ -1880,7 +1880,7 @@ static u64 mlx_to_vritio_features(u16 dev_features)
>>>>> return result;
>>>>> }
>>>>> -static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev)
>>>>> +static u64 mlx5_vdpa_get_device_features(struct vdpa_device *vdev)
>>>>> {
>>>>> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>>>> struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>>>> @@ -1972,7 +1972,7 @@ static void update_cvq_info(struct mlx5_vdpa_dev *mvdev)
>>>>> }
>>>>> }
>>>>> -static int mlx5_vdpa_set_features(struct vdpa_device *vdev, u64 features)
>>>>> +static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>>>>> {
>>>>> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>>>> struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>>>> @@ -2339,6 +2339,13 @@ static int mlx5_get_vq_irq(struct vdpa_device *vdv, u16 idx)
>>>>> return -EOPNOTSUPP;
>>>>> }
>>>>> +static u64 mlx5_vdpa_get_driver_features(struct vdpa_device *vdev)
>>>>> +{
>>>>> + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>>>> +
>>>>> + return mvdev->actual_features;
>>>>> +}
>>>>> +
>>>>> static const struct vdpa_config_ops mlx5_vdpa_ops = {
>>>>> .set_vq_address = mlx5_vdpa_set_vq_address,
>>>>> .set_vq_num = mlx5_vdpa_set_vq_num,
>>>>> @@ -2351,8 +2358,9 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
>>>>> .get_vq_notification = mlx5_get_vq_notification,
>>>>> .get_vq_irq = mlx5_get_vq_irq,
>>>>> .get_vq_align = mlx5_vdpa_get_vq_align,
>>>>> - .get_features = mlx5_vdpa_get_features,
>>>>> - .set_features = mlx5_vdpa_set_features,
>>>>> + .get_device_features = mlx5_vdpa_get_device_features,
>>>>> + .set_driver_features = mlx5_vdpa_set_driver_features,
>>>>> + .get_driver_features = mlx5_vdpa_get_driver_features,
>>>>> .set_config_cb = mlx5_vdpa_set_config_cb,
>>>>> .get_vq_num_max = mlx5_vdpa_get_vq_num_max,
>>>>> .get_device_id = mlx5_vdpa_get_device_id,
>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>>> index 7332a74a4b00..c37d384c0f33 100644
>>>>> --- a/drivers/vdpa/vdpa.c
>>>>> +++ b/drivers/vdpa/vdpa.c
>>>>> @@ -733,7 +733,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
>>>>> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
>>>>> return -EMSGSIZE;
>>>>> - features = vdev->config->get_features(vdev);
>>>>> + features = vdev->config->get_driver_features(vdev);
>>>>> return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
>>>>> }
>>>>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>>>> index 41b0cd17fcba..ddbe142af09a 100644
>>>>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>>>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>>>> @@ -399,14 +399,14 @@ static u32 vdpasim_get_vq_align(struct vdpa_device *vdpa)
>>>>> return VDPASIM_QUEUE_ALIGN;
>>>>> }
>>>>> -static u64 vdpasim_get_features(struct vdpa_device *vdpa)
>>>>> +static u64 vdpasim_get_device_features(struct vdpa_device *vdpa)
>>>>> {
>>>>> struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>>>>> return vdpasim->dev_attr.supported_features;
>>>>> }
>>>>> -static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
>>>>> +static int vdpasim_set_driver_features(struct vdpa_device *vdpa, u64 features)
>>>>> {
>>>>> struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>>>>> @@ -419,6 +419,13 @@ static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
>>>>> return 0;
>>>>> }
>>>>> +static u64 vdpasim_get_driver_features(struct vdpa_device *vdpa)
>>>>> +{
>>>>> + struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>>>>> +
>>>>> + return vdpasim->features;
>>>>> +}
>>>>> +
>>>>> static void vdpasim_set_config_cb(struct vdpa_device *vdpa,
>>>>> struct vdpa_callback *cb)
>>>>> {
>>>>> @@ -613,8 +620,9 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
>>>>> .set_vq_state = vdpasim_set_vq_state,
>>>>> .get_vq_state = vdpasim_get_vq_state,
>>>>> .get_vq_align = vdpasim_get_vq_align,
>>>>> - .get_features = vdpasim_get_features,
>>>>> - .set_features = vdpasim_set_features,
>>>>> + .get_device_features = vdpasim_get_device_features,
>>>>> + .set_driver_features = vdpasim_set_driver_features,
>>>>> + .get_driver_features = vdpasim_get_driver_features,
>>>>> .set_config_cb = vdpasim_set_config_cb,
>>>>> .get_vq_num_max = vdpasim_get_vq_num_max,
>>>>> .get_device_id = vdpasim_get_device_id,
>>>>> @@ -642,8 +650,9 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
>>>>> .set_vq_state = vdpasim_set_vq_state,
>>>>> .get_vq_state = vdpasim_get_vq_state,
>>>>> .get_vq_align = vdpasim_get_vq_align,
>>>>> - .get_features = vdpasim_get_features,
>>>>> - .set_features = vdpasim_set_features,
>>>>> + .get_device_features = vdpasim_get_device_features,
>>>>> + .set_driver_features = vdpasim_set_driver_features,
>>>>> + .get_driver_features = vdpasim_get_driver_features,
>>>>> .set_config_cb = vdpasim_set_config_cb,
>>>>> .get_vq_num_max = vdpasim_get_vq_num_max,
>>>>> .get_device_id = vdpasim_get_device_id,
>>>>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>>>>> index c9204c62f339..1e65af6ab9ae 100644
>>>>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>>>>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>>>>> @@ -573,14 +573,14 @@ static u32 vduse_vdpa_get_vq_align(struct vdpa_device *vdpa)
>>>>> return dev->vq_align;
>>>>> }
>>>>> -static u64 vduse_vdpa_get_features(struct vdpa_device *vdpa)
>>>>> +static u64 vduse_vdpa_get_device_features(struct vdpa_device *vdpa)
>>>>> {
>>>>> struct vduse_dev *dev = vdpa_to_vduse(vdpa);
>>>>> return dev->device_features;
>>>>> }
>>>>> -static int vduse_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
>>>>> +static int vduse_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
>>>>> {
>>>>> struct vduse_dev *dev = vdpa_to_vduse(vdpa);
>>>>> @@ -588,6 +588,13 @@ static int vduse_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
>>>>> return 0;
>>>>> }
>>>>> +static u64 vduse_vdpa_get_driver_features(struct vdpa_device *vdpa)
>>>>> +{
>>>>> + struct vduse_dev *dev = vdpa_to_vduse(vdpa);
>>>>> +
>>>>> + return dev->driver_features;
>>>>> +}
>>>>> +
>>>>> static void vduse_vdpa_set_config_cb(struct vdpa_device *vdpa,
>>>>> struct vdpa_callback *cb)
>>>>> {
>>>>> @@ -720,8 +727,9 @@ static const struct vdpa_config_ops vduse_vdpa_config_ops = {
>>>>> .set_vq_state = vduse_vdpa_set_vq_state,
>>>>> .get_vq_state = vduse_vdpa_get_vq_state,
>>>>> .get_vq_align = vduse_vdpa_get_vq_align,
>>>>> - .get_features = vduse_vdpa_get_features,
>>>>> - .set_features = vduse_vdpa_set_features,
>>>>> + .get_device_features = vduse_vdpa_get_device_features,
>>>>> + .set_driver_features = vduse_vdpa_set_driver_features,
>>>>> + .get_driver_features = vduse_vdpa_get_driver_features,
>>>>> .set_config_cb = vduse_vdpa_set_config_cb,
>>>>> .get_vq_num_max = vduse_vdpa_get_vq_num_max,
>>>>> .get_device_id = vduse_vdpa_get_device_id,
>>>>> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
>>>>> index e3ff7875e123..97285bc05e47 100644
>>>>> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
>>>>> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
>>>>> @@ -53,14 +53,14 @@ static struct virtio_pci_modern_device *vdpa_to_mdev(struct vdpa_device *vdpa)
>>>>> return &vp_vdpa->mdev;
>>>>> }
>>>>> -static u64 vp_vdpa_get_features(struct vdpa_device *vdpa)
>>>>> +static u64 vp_vdpa_get_device_features(struct vdpa_device *vdpa)
>>>>> {
>>>>> struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
>>>>> return vp_modern_get_features(mdev);
>>>>> }
>>>>> -static int vp_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
>>>>> +static int vp_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
>>>>> {
>>>>> struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
>>>>> @@ -69,6 +69,13 @@ static int vp_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
>>>>> return 0;
>>>>> }
>>>>> +static u64 vp_vdpa_get_driver_features(struct vdpa_device *vdpa)
>>>>> +{
>>>>> + struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
>>>>> +
>>>>> + return vp_modern_get_features(mdev);
>>>>> +}
>>>>> +
>>>>> static u8 vp_vdpa_get_status(struct vdpa_device *vdpa)
>>>>> {
>>>>> struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
>>>>> @@ -415,8 +422,9 @@ vp_vdpa_get_vq_notification(struct vdpa_device *vdpa, u16 qid)
>>>>> }
>>>>> static const struct vdpa_config_ops vp_vdpa_ops = {
>>>>> - .get_features = vp_vdpa_get_features,
>>>>> - .set_features = vp_vdpa_set_features,
>>>>> + .get_device_features = vp_vdpa_get_device_features,
>>>>> + .set_driver_features = vp_vdpa_set_driver_features,
>>>>> + .get_driver_features = vp_vdpa_get_driver_features,
>>>>> .get_status = vp_vdpa_get_status,
>>>>> .set_status = vp_vdpa_set_status,
>>>>> .reset = vp_vdpa_reset,
>>>>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>>>>> index 29cced1cd277..6d087a4fb581 100644
>>>>> --- a/drivers/vhost/vdpa.c
>>>>> +++ b/drivers/vhost/vdpa.c
>>>>> @@ -262,7 +262,7 @@ static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
>>>>> const struct vdpa_config_ops *ops = vdpa->config;
>>>>> u64 features;
>>>>> - features = ops->get_features(vdpa);
>>>>> + features = ops->get_device_features(vdpa);
>>>>> if (copy_to_user(featurep, &features, sizeof(features)))
>>>>> return -EFAULT;
>>>>> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
>>>>> index f85f860bc10b..a84b04ba3195 100644
>>>>> --- a/drivers/virtio/virtio_vdpa.c
>>>>> +++ b/drivers/virtio/virtio_vdpa.c
>>>>> @@ -308,7 +308,7 @@ static u64 virtio_vdpa_get_features(struct virtio_device *vdev)
>>>>> struct vdpa_device *vdpa = vd_get_vdpa(vdev);
>>>>> const struct vdpa_config_ops *ops = vdpa->config;
>>>>> - return ops->get_features(vdpa);
>>>>> + return ops->get_device_features(vdpa);
>>>>> }
>>>>> static int virtio_vdpa_finalize_features(struct virtio_device *vdev)
>>>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>>>> index c3011ccda430..db24317d61b6 100644
>>>>> --- a/include/linux/vdpa.h
>>>>> +++ b/include/linux/vdpa.h
>>>>> @@ -169,14 +169,17 @@ struct vdpa_map_file {
>>>>> * for the device
>>>>> * @vdev: vdpa device
>>>>> * Returns virtqueue algin requirement
>>>>> - * @get_features: Get virtio features supported by the device
>>>>> + * @get_device_features: Get virtio features supported by the device
>>>>> * @vdev: vdpa device
>>>>> * Returns the virtio features support by the
>>>>> * device
>>>>> - * @set_features: Set virtio features supported by the driver
>>>>> + * @set_driver_features: Set virtio features supported by the driver
>>>>> * @vdev: vdpa device
>>>>> * @features: feature support by the driver
>>>>> * Returns integer: success (0) or error (< 0)
>>>>> + * @get_driver_features: Get virtio features in action
>>>>> + * @vdev: vdpa device
>>>>> + * Returns the virtio features accepted
>>>>> * @set_config_cb: Set the config interrupt callback
>>>>> * @vdev: vdpa device
>>>>> * @cb: virtio-vdev interrupt callback structure
>>>>> @@ -276,8 +279,9 @@ struct vdpa_config_ops {
>>>>> /* Device ops */
>>>>> u32 (*get_vq_align)(struct vdpa_device *vdev);
>>>>> - u64 (*get_features)(struct vdpa_device *vdev);
>>>>> - int (*set_features)(struct vdpa_device *vdev, u64 features);
>>>>> + u64 (*get_device_features)(struct vdpa_device *vdev);
>>>>> + int (*set_driver_features)(struct vdpa_device *vdev, u64 features);
>>>>> + u64 (*get_driver_features)(struct vdpa_device *vdev);
>>>>> void (*set_config_cb)(struct vdpa_device *vdev,
>>>>> struct vdpa_callback *cb);
>>>>> u16 (*get_vq_num_max)(struct vdpa_device *vdev);
>>>>> @@ -395,7 +399,7 @@ static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
>>>>> const struct vdpa_config_ops *ops = vdev->config;
>>>>> vdev->features_valid = true;
>>>>> - return ops->set_features(vdev, features);
>>>>> + return ops->set_driver_features(vdev, features);
>>>>> }
>>>>> void vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 3/7] vdpa: Allow to configure max data virtqueues
[not found] ` <20211213130734.GB44427@mtl-vdi-166.wap.labs.mlnx>
@ 2021-12-14 0:47 ` Si-Wei Liu
0 siblings, 0 replies; 22+ messages in thread
From: Si-Wei Liu @ 2021-12-14 0:47 UTC (permalink / raw)
To: Eli Cohen, Jason Wang; +Cc: Laurent Vivier, mst, virtualization, eperezma
On 12/13/2021 5:07 AM, Eli Cohen wrote:
> On Mon, Dec 13, 2021 at 08:44:53AM +0200, Eli Cohen wrote:
>> On Fri, Dec 10, 2021 at 10:29:43AM +0800, Jason Wang wrote:
>>> On Fri, Dec 10, 2021 at 5:51 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>>>
>>>>
>>>> On 12/8/2021 9:36 PM, Jason Wang wrote:
>>>>> On Thu, Dec 9, 2021 at 8:25 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>>>>>
>>>>>> On 12/8/2021 12:14 PM, Eli Cohen wrote:
>>>>>>> Add netlink support to configure the max virtqueue pairs for a device.
>>>>>>> At least one pair is required. The maximum is dictated by the device.
>>>>>>>
>>>>>>> Example:
>>>>>>>
>>>>>>> $ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 max_vqp 5
>>>>>> Not this patch, but I think there should be a mega attribute defined
>>>>>> ahead to specify the virtio class/type to create vdpa instance with.
>>>>>> Like the existing ones e.g. mac_addr and mtu, max_vqp is also vdpa net
>>>>>> specific attribute.
>>>>> Probably, but this only works for the case when a single parent is
>>>>> allowed to create different types of devices. It looks to me the
>>>>> current model to have a per type parent.
>>>> Yes, that is the current situation and implementation, although the
>>>> model does allow multi-type parent through
>>>> VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES.
>>> Right, so I agree with you, we need to specify the class first.
>>>
>> Or maybe fail device creation if the parent device does not support net
>> class.
> BTW, we already have the mechanism in place to enforce that. If a device
> does not support configuration of max_vqp, it will not set
> VDPA_ATTR_DEV_NET_CFG_MACADDR in its config_attr_mask so you will not be
> able add it if you attempt to specify max_vqp upon creation.
Yes, but that is another level of validation down to the specific class
of vdpa config. It should be completely fine for user not to specify
max_vqp or mac address when vdpa is created. Though they may not create
the expected vdpa class as they wish in the first place if the class
validation is missing.
Having said, I guess it would be great if users who want to create vdpa
can get known of the parent's supported class and capability ahead
through some mgmtdev command, like what I suggested earlier:
$ vdpa mgmtdev capab show
pci/0000:41:00.2:
class: net
mac: off
mtu: on
max_mtu: 9000
max_vqp: 1
auxiliary/mlx5_core.sf.1:
class: net
mac: on
mtu: off
max_mtu: 1500
max_vqp: 256
Regards,
-Siwei
>
>> You check first the supported class with "vdpa mgmtdev show" and if net
>> is supported you may use max_vqp.
>>
>>>> Regardless, even though with
>>>> single-type parent, so far on vdpa creation there's no validation done
>>>> yet against which class/type the parent can support. The max-vqp is
>>>> essentially vdpa network device only, but command line users don't have
>>>> a means to infer it is structured this way, and/or which vdpa parent may
>>>> support this config attribute. That said, were the command line usage
>>>> structured like below, I would have less worry.
>>>>
>>>> $ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 net.max_vqp 5
>>> It might be easier to specify class as a dedicated parameter, since we
>>> may want to specify mtu and mac.
>>>
>>> Thanks
>>>
>>>> Alternatively, if the goal is to keep the attribute flat, we may show
>>>> the mapping for the parent capability and the supported class:
>>>>
>>>> $ vdpa mgmtdev capab show
>>>> pci/0000:41:00.2:
>>>> class: net
>>>> mac: off
>>>> mtu: on
>>>> max_mtu: 9000
>>>> max_vqp: 1
>>>> auxiliary/mlx5_core.sf.1:
>>>> class: net
>>>> mac: on
>>>> mtu: off
>>>> max_mtu: 1500
>>>> max_vqp: 256
>>>>
>>>> Thanks,
>>>> -Siwei
>>>>
>>>>> Thanks
>>>>>
>>>>>> -Siwei
>>>>>>
>>>>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>>>>> ---
>>>>>>> v0 -> v1:
>>>>>>> 1. fix typo
>>>>>>> 2. move max_vq_pairs to net specific struct
>>>>>>>
>>>>>>> drivers/vdpa/vdpa.c | 14 +++++++++++++-
>>>>>>> include/linux/vdpa.h | 1 +
>>>>>>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>>>>> index c37d384c0f33..3bf016e03512 100644
>>>>>>> --- a/drivers/vdpa/vdpa.c
>>>>>>> +++ b/drivers/vdpa/vdpa.c
>>>>>>> @@ -480,7 +480,8 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
>>>>>>> }
>>>>>>>
>>>>>>> #define VDPA_DEV_NET_ATTRS_MASK ((1 << VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
>>>>>>> - (1 << VDPA_ATTR_DEV_NET_CFG_MTU))
>>>>>>> + (1 << VDPA_ATTR_DEV_NET_CFG_MTU) | \
>>>>>>> + (1 << VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
>>>>>>>
>>>>>>> static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *info)
>>>>>>> {
>>>>>>> @@ -506,6 +507,17 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
>>>>>>> nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]);
>>>>>>> config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MTU);
>>>>>>> }
>>>>>>> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]) {
>>>>>>> + config.net.max_vq_pairs =
>>>>>>> + nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]);
>>>>>>> + if (!config.net.max_vq_pairs) {
>>>>>>> + NL_SET_ERR_MSG_MOD(info->extack,
>>>>>>> + "At least one pair of VQs is required");
>>>>>>> + err = -EINVAL;
>>>>>>> + goto err;
>>>>>>> + }
>>>>>>> + config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
>>>>>>> + }
>>>>>>>
>>>>>>> /* Skip checking capability if user didn't prefer to configure any
>>>>>>> * device networking attributes. It is likely that user might have used
>>>>>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>>>>>> index db24317d61b6..b62032573780 100644
>>>>>>> --- a/include/linux/vdpa.h
>>>>>>> +++ b/include/linux/vdpa.h
>>>>>>> @@ -99,6 +99,7 @@ struct vdpa_dev_set_config {
>>>>>>> struct {
>>>>>>> u8 mac[ETH_ALEN];
>>>>>>> u16 mtu;
>>>>>>> + u16 max_vq_pairs;
>>>>>>> } net;
>>>>>>> u64 mask;
>>>>>>> };
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-12-14 0:47 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20211208201430.73720-1-elic@nvidia.com>
[not found] ` <20211208201430.73720-3-elic@nvidia.com>
2021-12-09 0:12 ` [PATCH v1 2/7] vdpa/mlx5: Distribute RX virtqueues in RQT object Si-Wei Liu
[not found] ` <20211209065504.GB108239@mtl-vdi-166.wap.labs.mlnx>
2021-12-10 8:26 ` Si-Wei Liu
[not found] ` <20211208201430.73720-4-elic@nvidia.com>
2021-12-09 0:25 ` [PATCH v1 3/7] vdpa: Allow to configure max data virtqueues Si-Wei Liu
2021-12-09 5:36 ` Jason Wang
2021-12-09 21:50 ` Si-Wei Liu
2021-12-10 2:29 ` Jason Wang
[not found] ` <20211213064453.GD41423@mtl-vdi-166.wap.labs.mlnx>
[not found] ` <20211213130734.GB44427@mtl-vdi-166.wap.labs.mlnx>
2021-12-14 0:47 ` Si-Wei Liu
[not found] ` <20211208201430.73720-2-elic@nvidia.com>
2021-12-08 23:57 ` [PATCH v1 1/7] vdpa: Provide interface to read driver features Si-Wei Liu
[not found] ` <20211209064702.GA108239@mtl-vdi-166.wap.labs.mlnx>
2021-12-09 22:29 ` Si-Wei Liu
2021-12-12 9:45 ` Michael S. Tsirkin
[not found] ` <20211212133854.GA8840@mtl-vdi-166.wap.labs.mlnx>
2021-12-13 22:34 ` [PATCH v1 1/7] vdpa: Provide interface to read driver featuresy Si-Wei Liu
2021-12-09 5:33 ` [PATCH v1 1/7] vdpa: Provide interface to read driver features Jason Wang
[not found] ` <20211209070654.GD108239@mtl-vdi-166.wap.labs.mlnx>
2021-12-09 7:55 ` Jason Wang
[not found] ` <20211208201430.73720-6-elic@nvidia.com>
2021-12-09 5:43 ` [PATCH v1 5/7] vdpa/mlx5: Support configuring max data virtqueue pairs Jason Wang
[not found] ` <20211208201430.73720-7-elic@nvidia.com>
2021-12-09 0:59 ` [PATCH v1 6/7] vdpa: Add support for querying control virtqueue index Si-Wei Liu
2021-12-09 5:44 ` Jason Wang
[not found] ` <20211209070904.GE108239@mtl-vdi-166.wap.labs.mlnx>
2021-12-09 7:55 ` Jason Wang
[not found] ` <20211209080414.GA110931@mtl-vdi-166.wap.labs.mlnx>
2021-12-09 8:17 ` Jason Wang
[not found] ` <20211209082618.GA111658@mtl-vdi-166.wap.labs.mlnx>
2021-12-09 9:12 ` Jason Wang
[not found] ` <20211209092053.GA113385@mtl-vdi-166.wap.labs.mlnx>
2021-12-10 2:27 ` Jason Wang
2021-12-13 3:05 ` Jason Wang
[not found] ` <20211208201430.73720-8-elic@nvidia.com>
2021-12-10 8:17 ` [PATCH v1 7/7] vdpa/mlx5: Restore cur_num_vqs in case of failure in change_num_qps() Si-Wei Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.