Live migration of vdpa would typically require re-instate vdpa
device with an idential set of configs on the destination node,
same way as how source node created the device in the first place.
In order to allow live migration orchestration software to export the
initial set of vdpa attributes with which the device was created, it
will be useful if the vdpa tool can report the config on demand with
simple query. This will ease the orchestration software implementation
so that it doesn't have to keep track of vdpa config change, or have
to persist vdpa attributes across failure and recovery, in fear of
being killed due to accidental software error.
In this series, the initial device config for vdpa creation will be
exported via the "vdpa dev show" command. This is unlike the "vdpa
dev config show" command that usually goes with the live value in
the device config space, which is not reliable subject to the dynamics
of feature negotiation and possible change in device config space.
Examples:
1) Create vDPA by default without any config attribute
$ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0
$ vdpa dev show vdpa0
vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs 9 max_vq_size 256
$ vdpa dev -jp show vdpa0
{
"dev": {
"vdpa0": {
"type": "network",
"mgmtdev": "pci/0000:41:04.2",
"vendor_id": 5555,
"max_vqs": 9,
"max_vq_size": 256,
}
}
}
2) Create vDPA with config attribute(s) specified
$ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0 \
mac e4:11:c6:d3:45:f0 max_vq_pairs 4
$ vdpa dev show
vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs 9 max_vq_size 256
virtio_config: mac e4:11:c6:d3:45:f0 max_vq_pairs 4
$ vdpa dev -jp show
{
"dev": {
"vdpa0": {
"type": "network",
"mgmtdev": "pci/0000:41:04.2",
"vendor_id": 5555,
"max_vqs": 9,
"max_vq_size": 256,
"virtio_config": {
"mac": "e4:11:c6:d3:45:f0",
"max_vq_pairs": 4
}
}
}
}
---
v1 -> v2:
- Revised example output to export all config attributes under a
json object
---
Si-Wei Liu (4):
vdpa: save vdpa_dev_set_config in struct vdpa_device
vdpa: pass initial config to _vdpa_register_device()
vdpa: show dev config as-is in "vdpa dev show" output
vdpa: fix improper error message when adding vdpa dev
drivers/vdpa/ifcvf/ifcvf_main.c | 2 +-
drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +-
drivers/vdpa/vdpa.c | 63 +++++++++++++++++++++++++++++++++---
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 2 +-
drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 2 +-
drivers/vdpa/vdpa_user/vduse_dev.c | 2 +-
drivers/vdpa/virtio_pci/vp_vdpa.c | 3 +-
include/linux/vdpa.h | 26 ++++++++-------
8 files changed, 80 insertions(+), 22 deletions(-)
--
1.8.3.1
Just as _vdpa_register_device taking @nvqs as the number of queues
to feed userspace inquery via vdpa_dev_fill(), we can follow the
same to stash config attributes in struct vdpa_device at the time
of vdpa registration.
Signed-off-by: Si-Wei Liu <[email protected]>
---
drivers/vdpa/ifcvf/ifcvf_main.c | 2 +-
drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +-
drivers/vdpa/vdpa.c | 15 +++++++++++----
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 2 +-
drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 2 +-
drivers/vdpa/vdpa_user/vduse_dev.c | 2 +-
drivers/vdpa/virtio_pci/vp_vdpa.c | 3 ++-
include/linux/vdpa.h | 3 ++-
8 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index f9c0044..c54ab2c 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -771,7 +771,7 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
else
ret = dev_set_name(&vdpa_dev->dev, "vdpa%u", vdpa_dev->index);
- ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring);
+ ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring, config);
if (ret) {
put_device(&adapter->vdpa.dev);
IFCVF_ERR(pdev, "Failed to register to vDPA bus");
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 9091336..376082e 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -3206,7 +3206,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
mlx5_notifier_register(mdev, &ndev->nb);
ndev->nb_registered = true;
mvdev->vdev.mdev = &mgtdev->mgtdev;
- err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
+ err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1, add_config);
if (err)
goto err_reg;
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index febdc99..566c1c6 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -215,11 +215,16 @@ static int vdpa_name_match(struct device *dev, const void *data)
return (strcmp(dev_name(&vdev->dev), data) == 0);
}
-static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs)
+static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs,
+ const struct vdpa_dev_set_config *cfg)
{
struct device *dev;
vdev->nvqs = nvqs;
+ if (cfg)
+ vdev->vdev_cfg = *cfg;
+ else
+ vdev->vdev_cfg.mask = 0ULL;
lockdep_assert_held(&vdpa_dev_lock);
dev = bus_find_device(&vdpa_bus, NULL, dev_name(&vdev->dev), vdpa_name_match);
@@ -237,15 +242,17 @@ static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs)
* callback after setting up valid mgmtdev for this vdpa device.
* @vdev: the vdpa device to be registered to vDPA bus
* @nvqs: number of virtqueues supported by this device
+ * @cfg: initial config on vdpa device creation
*
* Return: Returns an error when fail to add device to vDPA bus
*/
-int _vdpa_register_device(struct vdpa_device *vdev, u32 nvqs)
+int _vdpa_register_device(struct vdpa_device *vdev, u32 nvqs,
+ const struct vdpa_dev_set_config *cfg)
{
if (!vdev->mdev)
return -EINVAL;
- return __vdpa_register_device(vdev, nvqs);
+ return __vdpa_register_device(vdev, nvqs, cfg);
}
EXPORT_SYMBOL_GPL(_vdpa_register_device);
@@ -262,7 +269,7 @@ int vdpa_register_device(struct vdpa_device *vdev, u32 nvqs)
int err;
down_write(&vdpa_dev_lock);
- err = __vdpa_register_device(vdev, nvqs);
+ err = __vdpa_register_device(vdev, nvqs, NULL);
up_write(&vdpa_dev_lock);
return err;
}
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index c6db1a1..5e1cebc 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -387,7 +387,7 @@ static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
if (IS_ERR(simdev))
return PTR_ERR(simdev);
- ret = _vdpa_register_device(&simdev->vdpa, VDPASIM_BLK_VQ_NUM);
+ ret = _vdpa_register_device(&simdev->vdpa, VDPASIM_BLK_VQ_NUM, config);
if (ret)
goto put_dev;
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
index c3cb225..06ef5a0 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
@@ -260,7 +260,7 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
vdpasim_net_setup_config(simdev, config);
- ret = _vdpa_register_device(&simdev->vdpa, VDPASIM_NET_VQ_NUM);
+ ret = _vdpa_register_device(&simdev->vdpa, VDPASIM_NET_VQ_NUM, config);
if (ret)
goto reg_err;
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 35dceee..6530fd2 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1713,7 +1713,7 @@ static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
if (ret)
return ret;
- ret = _vdpa_register_device(&dev->vdev->vdpa, dev->vq_num);
+ ret = _vdpa_register_device(&dev->vdev->vdpa, dev->vq_num, config);
if (ret) {
put_device(&dev->vdev->vdpa.dev);
return ret;
diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
index d448db0..ffdc90e 100644
--- a/drivers/vdpa/virtio_pci/vp_vdpa.c
+++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
@@ -538,7 +538,8 @@ static int vp_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
vp_vdpa->config_irq = VIRTIO_MSI_NO_VECTOR;
vp_vdpa->vdpa.mdev = &vp_vdpa_mgtdev->mgtdev;
- ret = _vdpa_register_device(&vp_vdpa->vdpa, vp_vdpa->queues);
+ ret = _vdpa_register_device(&vp_vdpa->vdpa, vp_vdpa->queues,
+ add_config);
if (ret) {
dev_err(&pdev->dev, "Failed to register to vdpa bus\n");
goto err;
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index f1838f5..b9d50e8 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -381,7 +381,8 @@ struct vdpa_device *__vdpa_alloc_device(struct device *parent,
int vdpa_register_device(struct vdpa_device *vdev, u32 nvqs);
void vdpa_unregister_device(struct vdpa_device *vdev);
-int _vdpa_register_device(struct vdpa_device *vdev, u32 nvqs);
+int _vdpa_register_device(struct vdpa_device *vdev, u32 nvqs,
+ const struct vdpa_dev_set_config *cfg);
void _vdpa_unregister_device(struct vdpa_device *vdev);
/**
--
1.8.3.1
Live migration of vdpa would typically require re-instate vdpa
device with an idential set of configs on the destination node,
same way as how source node created the device in the first
place. In order to save orchestration software from memorizing
and keeping track of vdpa config, it will be helpful if the vdpa
tool provides the aids for exporting the initial configs from
which vdpa device was created as-is. The "vdpa dev show" command
seems to be the right vehicle for that. It is unlike the "vdpa dev
config show" command output that usually goes with the live value
in the device config space, which is not quite reliable subject to
the dynamics of feature negotiation and possible change in device
config space.
Examples:
1) Create vDPA by default without any config attribute
$ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0
$ vdpa dev show vdpa0
vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs 9 max_vq_size 256
$ vdpa dev -jp show vdpa0
{
"dev": {
"vdpa0": {
"type": "network",
"mgmtdev": "pci/0000:41:04.2",
"vendor_id": 5555,
"max_vqs": 9,
"max_vq_size": 256,
}
}
}
2) Create vDPA with config attribute(s) specified
$ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0 \
mac e4:11:c6:d3:45:f0 max_vq_pairs 4
$ vdpa dev show
vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs 9 max_vq_size 256
virtio_config: mac e4:11:c6:d3:45:f0 max_vq_pairs 4
$ vdpa dev -jp show
{
"dev": {
"vdpa0": {
"type": "network",
"mgmtdev": "pci/0000:41:04.2",
"vendor_id": 5555,
"max_vqs": 9,
"max_vq_size": 256,
"virtio_config": {
"mac": "e4:11:c6:d3:45:f0",
"max_vq_pairs": 4
}
}
}
}
Signed-off-by: Si-Wei Liu <[email protected]>
---
drivers/vdpa/vdpa.c | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 566c1c6..91eca6d 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -677,6 +677,41 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i
}
static int
+vdpa_dev_cfgattrs_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 device_id)
+{
+ struct vdpa_dev_set_config *cfg = &vdev->vdev_cfg;
+ int err = -EMSGSIZE;
+
+ if (!cfg->mask)
+ return 0;
+
+ switch (device_id) {
+ case VIRTIO_ID_NET:
+ if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR)) != 0 &&
+ nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR,
+ sizeof(cfg->net.mac), cfg->net.mac))
+ return err;
+ if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) != 0 &&
+ nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, cfg->net.mtu))
+ return err;
+ if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) != 0 &&
+ nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
+ cfg->net.max_vq_pairs))
+ return err;
+ break;
+ default:
+ break;
+ }
+
+ if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) != 0 &&
+ nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES,
+ cfg->device_features, VDPA_ATTR_PAD))
+ return err;
+
+ return 0;
+}
+
+static int
vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq,
int flags, struct netlink_ext_ack *extack)
{
@@ -715,6 +750,10 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i
if (nla_put_u16(msg, VDPA_ATTR_DEV_MIN_VQ_SIZE, min_vq_size))
goto msg_err;
+ err = vdpa_dev_cfgattrs_fill(vdev, msg, device_id);
+ if (err)
+ goto msg_err;
+
genlmsg_end(msg, hdr);
return 0;
--
1.8.3.1
In order to allow live migration orchestration software to export the
initial set of vdpa attributes with which the device was created, it
will be useful if the vdpa tool can report the config on demand with
simple query. This will ease the orchestration software implementation
so that it doesn't have to keep track of vdpa config change, or have
to persist vdpa attributes across failure and recovery, in fear of
being killed due to accidental software error.
This commit attempts to make struct vdpa_device contain the struct
vdpa_dev_set_config, where all config attributes upon vdpa creation
are carried over. Which will be used in subsequent commits.
Signed-off-by: Si-Wei Liu <[email protected]>
---
include/linux/vdpa.h | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 6d0f5e4..f1838f5 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -58,6 +58,16 @@ struct vdpa_vq_state {
};
};
+struct vdpa_dev_set_config {
+ u64 device_features;
+ struct {
+ u8 mac[ETH_ALEN];
+ u16 mtu;
+ u16 max_vq_pairs;
+ } net;
+ u64 mask;
+};
+
struct vdpa_mgmt_dev;
/**
@@ -77,6 +87,8 @@ struct vdpa_vq_state {
* @nvqs: maximum number of supported virtqueues
* @mdev: management device pointer; caller must setup when registering device as part
* of dev_add() mgmtdev ops callback before invoking _vdpa_register_device().
+ * @vdev_cfg: initial device config on vdpa creation; useful when instantiate with
+ * the exact same config is needed.
*/
struct vdpa_device {
struct device dev;
@@ -91,6 +103,7 @@ struct vdpa_device {
struct vdpa_mgmt_dev *mdev;
unsigned int ngroups;
unsigned int nas;
+ struct vdpa_dev_set_config vdev_cfg;
};
/**
@@ -103,16 +116,6 @@ struct vdpa_iova_range {
u64 last;
};
-struct vdpa_dev_set_config {
- u64 device_features;
- struct {
- u8 mac[ETH_ALEN];
- u16 mtu;
- u16 max_vq_pairs;
- } net;
- u64 mask;
-};
-
/**
* Corresponding file area for device memory mapping
* @file: vma->vm_file for the mapping
--
1.8.3.1
In below example, before the fix, mtu attribute is supported
by the parent mgmtdev, but the error message showing "All
provided are not supported" is just misleading.
$ vdpa mgmtdev show
vdpasim_net:
supported_classes net
max_supported_vqs 3
dev_features MTU MAC CTRL_VQ CTRL_MAC_ADDR ANY_LAYOUT VERSION_1 ACCESS_PLATFORM
$ vdpa dev add mgmtdev vdpasim_net name vdpasim0 mtu 5000 max_vqp 2
Error: vdpa: All provided attributes are not supported.
kernel answers: Operation not supported
After fix, the relevant error message will be like:
$ vdpa dev add mgmtdev vdpasim_net name vdpasim0 mtu 5000 max_vqp 2
Error: vdpa: Some provided attributes are not supported.
kernel answers: Operation not supported
$ vdpa dev add mgmtdev vdpasim_net name vdpasim0 max_vqp 2
Error: vdpa: All provided attributes are not supported.
kernel answers: Operation not supported
Signed-off-by: Si-Wei Liu <[email protected]>
---
drivers/vdpa/vdpa.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 91eca6d..ff15e0a 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -629,13 +629,20 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
err = PTR_ERR(mdev);
goto err;
}
- if ((config.mask & mdev->config_attr_mask) != config.mask) {
+ if (config.mask && (config.mask & mdev->config_attr_mask) == 0) {
NL_SET_ERR_MSG_MOD(info->extack,
"All provided attributes are not supported");
err = -EOPNOTSUPP;
goto err;
}
+ if ((config.mask & mdev->config_attr_mask) != config.mask) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "Some provided attributes are not supported");
+ err = -EOPNOTSUPP;
+ goto err;
+ }
+
err = mdev->ops->dev_add(mdev, name, &config);
err:
up_write(&vdpa_dev_lock);
--
1.8.3.1
On Wed, Oct 19, 2022 at 8:56 AM Si-Wei Liu <[email protected]> wrote:
>
> In order to allow live migration orchestration software to export the
> initial set of vdpa attributes with which the device was created, it
> will be useful if the vdpa tool can report the config on demand with
> simple query. This will ease the orchestration software implementation
> so that it doesn't have to keep track of vdpa config change, or have
> to persist vdpa attributes across failure and recovery, in fear of
> being killed due to accidental software error.
>
> This commit attempts to make struct vdpa_device contain the struct
> vdpa_dev_set_config, where all config attributes upon vdpa creation
> are carried over. Which will be used in subsequent commits.
>
> Signed-off-by: Si-Wei Liu <[email protected]>
> ---
> include/linux/vdpa.h | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 6d0f5e4..f1838f5 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -58,6 +58,16 @@ struct vdpa_vq_state {
> };
> };
>
> +struct vdpa_dev_set_config {
> + u64 device_features;
> + struct {
> + u8 mac[ETH_ALEN];
> + u16 mtu;
> + u16 max_vq_pairs;
> + } net;
Not for this patch but I think there should be a union container for
this structure to make it usable for other types of devices.
> + u64 mask;
> +};
> +
> struct vdpa_mgmt_dev;
>
> /**
> @@ -77,6 +87,8 @@ struct vdpa_vq_state {
> * @nvqs: maximum number of supported virtqueues
> * @mdev: management device pointer; caller must setup when registering device as part
> * of dev_add() mgmtdev ops callback before invoking _vdpa_register_device().
> + * @vdev_cfg: initial device config on vdpa creation; useful when instantiate with
> + * the exact same config is needed.
Not a native speaker, but I guess it should be better named as "init_cfg"?
Thanks
> */
> struct vdpa_device {
> struct device dev;
> @@ -91,6 +103,7 @@ struct vdpa_device {
> struct vdpa_mgmt_dev *mdev;
> unsigned int ngroups;
> unsigned int nas;
> + struct vdpa_dev_set_config vdev_cfg;
> };
>
> /**
> @@ -103,16 +116,6 @@ struct vdpa_iova_range {
> u64 last;
> };
>
> -struct vdpa_dev_set_config {
> - u64 device_features;
> - struct {
> - u8 mac[ETH_ALEN];
> - u16 mtu;
> - u16 max_vq_pairs;
> - } net;
> - u64 mask;
> -};
> -
> /**
> * Corresponding file area for device memory mapping
> * @file: vma->vm_file for the mapping
> --
> 1.8.3.1
>
On Wed, Oct 19, 2022 at 8:56 AM Si-Wei Liu <[email protected]> wrote:
>
> Live migration of vdpa would typically require re-instate vdpa
> device with an idential set of configs on the destination node,
> same way as how source node created the device in the first
> place. In order to save orchestration software from memorizing
> and keeping track of vdpa config, it will be helpful if the vdpa
> tool provides the aids for exporting the initial configs from
> which vdpa device was created as-is. The "vdpa dev show" command
> seems to be the right vehicle for that. It is unlike the "vdpa dev
> config show" command output that usually goes with the live value
> in the device config space, which is not quite reliable subject to
> the dynamics of feature negotiation and possible change in device
> config space.
>
> Examples:
>
> 1) Create vDPA by default without any config attribute
>
> $ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0
> $ vdpa dev show vdpa0
> vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs 9 max_vq_size 256
> $ vdpa dev -jp show vdpa0
> {
> "dev": {
> "vdpa0": {
> "type": "network",
> "mgmtdev": "pci/0000:41:04.2",
> "vendor_id": 5555,
> "max_vqs": 9,
> "max_vq_size": 256,
> }
> }
> }
>
> 2) Create vDPA with config attribute(s) specified
>
> $ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0 \
> mac e4:11:c6:d3:45:f0 max_vq_pairs 4
> $ vdpa dev show
> vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs 9 max_vq_size 256
> virtio_config: mac e4:11:c6:d3:45:f0 max_vq_pairs 4
> $ vdpa dev -jp show
> {
> "dev": {
> "vdpa0": {
> "type": "network",
> "mgmtdev": "pci/0000:41:04.2",
> "vendor_id": 5555,
> "max_vqs": 9,
> "max_vq_size": 256,
> "virtio_config": {
> "mac": "e4:11:c6:d3:45:f0",
> "max_vq_pairs": 4
> }
> }
> }
> }
>
> Signed-off-by: Si-Wei Liu <[email protected]>
> ---
> drivers/vdpa/vdpa.c | 39 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 566c1c6..91eca6d 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -677,6 +677,41 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i
> }
>
> static int
> +vdpa_dev_cfgattrs_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 device_id)
> +{
> + struct vdpa_dev_set_config *cfg = &vdev->vdev_cfg;
> + int err = -EMSGSIZE;
> +
> + if (!cfg->mask)
> + return 0;
> +
> + switch (device_id) {
> + case VIRTIO_ID_NET:
> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR)) != 0 &&
> + nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR,
> + sizeof(cfg->net.mac), cfg->net.mac))
> + return err;
> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) != 0 &&
> + nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, cfg->net.mtu))
> + return err;
> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) != 0 &&
> + nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
> + cfg->net.max_vq_pairs))
> + return err;
> + break;
This makes me think if we can reuse the virtio_net_config structure
other than duplicate it slowly with a dedicated nested structure
inside vdpa_dev_set_config then we can reuse the
vdpa_dev_net_config_fill().
Thanks
> + default:
> + break;
> + }
> +
> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) != 0 &&
> + nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES,
> + cfg->device_features, VDPA_ATTR_PAD))
> + return err;
> +
> + return 0;
> +}
> +
> +static int
> vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq,
> int flags, struct netlink_ext_ack *extack)
> {
> @@ -715,6 +750,10 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i
> if (nla_put_u16(msg, VDPA_ATTR_DEV_MIN_VQ_SIZE, min_vq_size))
> goto msg_err;
>
> + err = vdpa_dev_cfgattrs_fill(vdev, msg, device_id);
> + if (err)
> + goto msg_err;
> +
> genlmsg_end(msg, hdr);
> return 0;
>
> --
> 1.8.3.1
>
On Wed, Oct 19, 2022 at 8:56 AM Si-Wei Liu <[email protected]> wrote:
>
> Just as _vdpa_register_device taking @nvqs as the number of queues
I wonder if it's better to embed nvqs in the config structure.
> to feed userspace inquery via vdpa_dev_fill(), we can follow the
> same to stash config attributes in struct vdpa_device at the time
> of vdpa registration.
>
> Signed-off-by: Si-Wei Liu <[email protected]>
> ---
> drivers/vdpa/ifcvf/ifcvf_main.c | 2 +-
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +-
> drivers/vdpa/vdpa.c | 15 +++++++++++----
> drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 2 +-
> drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 2 +-
> drivers/vdpa/vdpa_user/vduse_dev.c | 2 +-
> drivers/vdpa/virtio_pci/vp_vdpa.c | 3 ++-
> include/linux/vdpa.h | 3 ++-
> 8 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index f9c0044..c54ab2c 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -771,7 +771,7 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
> else
> ret = dev_set_name(&vdpa_dev->dev, "vdpa%u", vdpa_dev->index);
>
> - ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring);
> + ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring, config);
> if (ret) {
> put_device(&adapter->vdpa.dev);
> IFCVF_ERR(pdev, "Failed to register to vDPA bus");
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 9091336..376082e 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -3206,7 +3206,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> mlx5_notifier_register(mdev, &ndev->nb);
> ndev->nb_registered = true;
> mvdev->vdev.mdev = &mgtdev->mgtdev;
> - err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
> + err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1, add_config);
> if (err)
> goto err_reg;
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index febdc99..566c1c6 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -215,11 +215,16 @@ static int vdpa_name_match(struct device *dev, const void *data)
> return (strcmp(dev_name(&vdev->dev), data) == 0);
> }
>
> -static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs)
> +static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs,
> + const struct vdpa_dev_set_config *cfg)
> {
> struct device *dev;
>
> vdev->nvqs = nvqs;
> + if (cfg)
> + vdev->vdev_cfg = *cfg;
> + else
> + vdev->vdev_cfg.mask = 0ULL;
I think it would be nice if we can convert eni to use netlink then we
don't need any workaround like this.
Thanks
On Wed, Oct 19, 2022 at 8:56 AM Si-Wei Liu <[email protected]> wrote:
>
> In below example, before the fix, mtu attribute is supported
> by the parent mgmtdev, but the error message showing "All
> provided are not supported" is just misleading.
>
> $ vdpa mgmtdev show
> vdpasim_net:
> supported_classes net
> max_supported_vqs 3
> dev_features MTU MAC CTRL_VQ CTRL_MAC_ADDR ANY_LAYOUT VERSION_1 ACCESS_PLATFORM
>
> $ vdpa dev add mgmtdev vdpasim_net name vdpasim0 mtu 5000 max_vqp 2
> Error: vdpa: All provided attributes are not supported.
> kernel answers: Operation not supported
>
> After fix, the relevant error message will be like:
>
> $ vdpa dev add mgmtdev vdpasim_net name vdpasim0 mtu 5000 max_vqp 2
> Error: vdpa: Some provided attributes are not supported.
> kernel answers: Operation not supported
>
> $ vdpa dev add mgmtdev vdpasim_net name vdpasim0 max_vqp 2
> Error: vdpa: All provided attributes are not supported.
> kernel answers: Operation not supported
>
> Signed-off-by: Si-Wei Liu <[email protected]>
Acked-by: Jason Wang <[email protected]>
> ---
> drivers/vdpa/vdpa.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 91eca6d..ff15e0a 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -629,13 +629,20 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
> err = PTR_ERR(mdev);
> goto err;
> }
> - if ((config.mask & mdev->config_attr_mask) != config.mask) {
> + if (config.mask && (config.mask & mdev->config_attr_mask) == 0) {
> NL_SET_ERR_MSG_MOD(info->extack,
> "All provided attributes are not supported");
> err = -EOPNOTSUPP;
> goto err;
> }
>
> + if ((config.mask & mdev->config_attr_mask) != config.mask) {
> + NL_SET_ERR_MSG_MOD(info->extack,
> + "Some provided attributes are not supported");
> + err = -EOPNOTSUPP;
> + goto err;
> + }
> +
> err = mdev->ops->dev_add(mdev, name, &config);
> err:
> up_write(&vdpa_dev_lock);
> --
> 1.8.3.1
>
On 10/19/2022 10:20 PM, Jason Wang wrote:
> On Wed, Oct 19, 2022 at 8:56 AM Si-Wei Liu <[email protected]> wrote:
>> Just as _vdpa_register_device taking @nvqs as the number of queues
> I wonder if it's better to embed nvqs in the config structure.
Hmmm, the config structure is mostly for containing the configurables
specified in the 'vdpa dev add' command, while each field is
conditionally set and guarded by a corresponding mask bit. If @nvqs
needs to be folded into a structure, I feel it might be better to use
another struct for holding the informational fields (i.e. those are
read-only and always exist). But doing this would make @nvqs a weird
solo member in that struct with no extra benefit, and all the other
informational fields shown in the 'vdpa dev show' command would be
gotten from the device through config_ops directly. Maybe do this until
another read-only field comes around?
>
>> to feed userspace inquery via vdpa_dev_fill(), we can follow the
>> same to stash config attributes in struct vdpa_device at the time
>> of vdpa registration.
>>
>> Signed-off-by: Si-Wei Liu <[email protected]>
>> ---
>> drivers/vdpa/ifcvf/ifcvf_main.c | 2 +-
>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +-
>> drivers/vdpa/vdpa.c | 15 +++++++++++----
>> drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 2 +-
>> drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 2 +-
>> drivers/vdpa/vdpa_user/vduse_dev.c | 2 +-
>> drivers/vdpa/virtio_pci/vp_vdpa.c | 3 ++-
>> include/linux/vdpa.h | 3 ++-
>> 8 files changed, 20 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index f9c0044..c54ab2c 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -771,7 +771,7 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>> else
>> ret = dev_set_name(&vdpa_dev->dev, "vdpa%u", vdpa_dev->index);
>>
>> - ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring);
>> + ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring, config);
>> if (ret) {
>> put_device(&adapter->vdpa.dev);
>> IFCVF_ERR(pdev, "Failed to register to vDPA bus");
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index 9091336..376082e 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -3206,7 +3206,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>> mlx5_notifier_register(mdev, &ndev->nb);
>> ndev->nb_registered = true;
>> mvdev->vdev.mdev = &mgtdev->mgtdev;
>> - err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
>> + err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1, add_config);
>> if (err)
>> goto err_reg;
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> index febdc99..566c1c6 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -215,11 +215,16 @@ static int vdpa_name_match(struct device *dev, const void *data)
>> return (strcmp(dev_name(&vdev->dev), data) == 0);
>> }
>>
>> -static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs)
>> +static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs,
>> + const struct vdpa_dev_set_config *cfg)
>> {
>> struct device *dev;
>>
>> vdev->nvqs = nvqs;
>> + if (cfg)
>> + vdev->vdev_cfg = *cfg;
>> + else
>> + vdev->vdev_cfg.mask = 0ULL;
> I think it would be nice if we can convert eni to use netlink then we
> don't need any workaround like this.
Yes, Alibaba ENI is the only consumer of the old vdpa_register_device()
API without being ported to the netlink API. Not sure what is needed but
it seems another work to make netlink API committed to support a legacy
compatible model?
-Siwei
>
> Thanks
>
On 10/19/2022 10:25 PM, Jason Wang wrote:
> On Wed, Oct 19, 2022 at 8:56 AM Si-Wei Liu <[email protected]> wrote:
>> Live migration of vdpa would typically require re-instate vdpa
>> device with an idential set of configs on the destination node,
>> same way as how source node created the device in the first
>> place. In order to save orchestration software from memorizing
>> and keeping track of vdpa config, it will be helpful if the vdpa
>> tool provides the aids for exporting the initial configs from
>> which vdpa device was created as-is. The "vdpa dev show" command
>> seems to be the right vehicle for that. It is unlike the "vdpa dev
>> config show" command output that usually goes with the live value
>> in the device config space, which is not quite reliable subject to
>> the dynamics of feature negotiation and possible change in device
>> config space.
>>
>> Examples:
>>
>> 1) Create vDPA by default without any config attribute
>>
>> $ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0
>> $ vdpa dev show vdpa0
>> vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs 9 max_vq_size 256
>> $ vdpa dev -jp show vdpa0
>> {
>> "dev": {
>> "vdpa0": {
>> "type": "network",
>> "mgmtdev": "pci/0000:41:04.2",
>> "vendor_id": 5555,
>> "max_vqs": 9,
>> "max_vq_size": 256,
>> }
>> }
>> }
>>
>> 2) Create vDPA with config attribute(s) specified
>>
>> $ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0 \
>> mac e4:11:c6:d3:45:f0 max_vq_pairs 4
>> $ vdpa dev show
>> vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs 9 max_vq_size 256
>> virtio_config: mac e4:11:c6:d3:45:f0 max_vq_pairs 4
>> $ vdpa dev -jp show
>> {
>> "dev": {
>> "vdpa0": {
>> "type": "network",
>> "mgmtdev": "pci/0000:41:04.2",
>> "vendor_id": 5555,
>> "max_vqs": 9,
>> "max_vq_size": 256,
>> "virtio_config": {
>> "mac": "e4:11:c6:d3:45:f0",
>> "max_vq_pairs": 4
>> }
>> }
>> }
>> }
>>
>> Signed-off-by: Si-Wei Liu <[email protected]>
>> ---
>> drivers/vdpa/vdpa.c | 39 +++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> index 566c1c6..91eca6d 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -677,6 +677,41 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i
>> }
>>
>> static int
>> +vdpa_dev_cfgattrs_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 device_id)
>> +{
>> + struct vdpa_dev_set_config *cfg = &vdev->vdev_cfg;
>> + int err = -EMSGSIZE;
>> +
>> + if (!cfg->mask)
>> + return 0;
>> +
>> + switch (device_id) {
>> + case VIRTIO_ID_NET:
>> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR)) != 0 &&
>> + nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR,
>> + sizeof(cfg->net.mac), cfg->net.mac))
>> + return err;
>> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) != 0 &&
>> + nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, cfg->net.mtu))
>> + return err;
>> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) != 0 &&
>> + nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
>> + cfg->net.max_vq_pairs))
>> + return err;
>> + break;
> This makes me think if we can reuse the virtio_net_config structure
> other than duplicate it slowly with a dedicated nested structure
> inside vdpa_dev_set_config then we can reuse the
> vdpa_dev_net_config_fill().
Adding Parav.
I think for now the struct vdpa_dev_set_config has just a few fields, so
it's not very obvious. But from what I understand, the
vdpa_dev_set_config struct is designed to be built around vdpa
configurables, without getting it limited by what's exposed by the
virtio device config structure, such as virtio_net_config. For instance,
there could be possibility for vdpa user to specify the size of MAC
unicast or multicast address table, which is not defined anywhere in the
virtio_net_config. I think it's important to match such configuration
(which may not even be defined in spec) for src&dst vdpa devices
involving the live migration.
-Siwei
>
> Thanks
>
>> + default:
>> + break;
>> + }
>> +
>> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) != 0 &&
>> + nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES,
>> + cfg->device_features, VDPA_ATTR_PAD))
>> + return err;
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq,
>> int flags, struct netlink_ext_ack *extack)
>> {
>> @@ -715,6 +750,10 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i
>> if (nla_put_u16(msg, VDPA_ATTR_DEV_MIN_VQ_SIZE, min_vq_size))
>> goto msg_err;
>>
>> + err = vdpa_dev_cfgattrs_fill(vdev, msg, device_id);
>> + if (err)
>> + goto msg_err;
>> +
>> genlmsg_end(msg, hdr);
>> return 0;
>>
>> --
>> 1.8.3.1
>>
> From: Si-Wei Liu <[email protected]>
> Sent: Thursday, October 20, 2022 2:12 PM
>
>
> On 10/19/2022 10:25 PM, Jason Wang wrote:
> > On Wed, Oct 19, 2022 at 8:56 AM Si-Wei Liu <[email protected]>
> wrote:
> >> Live migration of vdpa would typically require re-instate vdpa device
> >> with an idential set of configs on the destination node, same way as
> >> how source node created the device in the first place. In order to
> >> save orchestration software from memorizing and keeping track of vdpa
> >> config, it will be helpful if the vdpa tool provides the aids for
> >> exporting the initial configs from which vdpa device was created
> >> as-is. The "vdpa dev show" command seems to be the right vehicle for
> >> that. It is unlike the "vdpa dev config show" command output that
> >> usually goes with the live value in the device config space, which is
> >> not quite reliable subject to the dynamics of feature negotiation and
> >> possible change in device config space.
> >>
> >> Examples:
> >>
> >> 1) Create vDPA by default without any config attribute
> >>
> >> $ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0 $ vdpa dev show
> >> vdpa0
> >> vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs
> 9
> >> max_vq_size 256 $ vdpa dev -jp show vdpa0 {
> >> "dev": {
> >> "vdpa0": {
> >> "type": "network",
> >> "mgmtdev": "pci/0000:41:04.2",
> >> "vendor_id": 5555,
> >> "max_vqs": 9,
> >> "max_vq_size": 256,
> >> }
> >> }
> >> }
> >>
> >> 2) Create vDPA with config attribute(s) specified
> >>
> >> $ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0 \
> >> mac e4:11:c6:d3:45:f0 max_vq_pairs 4 $ vdpa dev show
> >> vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs
> 9 max_vq_size 256
> >> virtio_config: mac e4:11:c6:d3:45:f0 max_vq_pairs 4 $ vdpa dev -jp
> >> show {
> >> "dev": {
> >> "vdpa0": {
> >> "type": "network",
> >> "mgmtdev": "pci/0000:41:04.2",
> >> "vendor_id": 5555,
> >> "max_vqs": 9,
> >> "max_vq_size": 256,
> >> "virtio_config": {
Since most config is related to virtio.
May be better to do
s/virtio_config/static_config or
s/virto_config/dev_config
This clearly indicates that this was the device static configuration.
> >> "mac": "e4:11:c6:d3:45:f0",
> >> "max_vq_pairs": 4
> >> }
> >> }
> >> }
> >> }
> >>
> >> Signed-off-by: Si-Wei Liu <[email protected]>
> >> ---
> >> drivers/vdpa/vdpa.c | 39 +++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 39 insertions(+)
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> >> 566c1c6..91eca6d 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -677,6 +677,41 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct
> sk_buff *skb, struct genl_info *i
> >> }
> >>
> >> static int
> >> +vdpa_dev_cfgattrs_fill(struct vdpa_device *vdev, struct sk_buff
> >> +*msg, u32 device_id) {
> >> + struct vdpa_dev_set_config *cfg = &vdev->vdev_cfg;
> >> + int err = -EMSGSIZE;
> >> +
> >> + if (!cfg->mask)
> >> + return 0;
> >> +
> >> + switch (device_id) {
> >> + case VIRTIO_ID_NET:
> >> + if ((cfg->mask &
> BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR)) != 0 &&
> >> + nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR,
> >> + sizeof(cfg->net.mac), cfg->net.mac))
> >> + return err;
> >> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) !=
> 0 &&
> >> + nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, cfg-
> >net.mtu))
> >> + return err;
> >> + if ((cfg->mask &
> BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) != 0 &&
> >> + nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
> >> + cfg->net.max_vq_pairs))
> >> + return err;
> >> + break;
> > This makes me think if we can reuse the virtio_net_config structure
> > other than duplicate it slowly with a dedicated nested structure
> > inside vdpa_dev_set_config then we can reuse the
> > vdpa_dev_net_config_fill().
> Adding Parav.
>
> I think for now the struct vdpa_dev_set_config has just a few fields, so it's
> not very obvious. But from what I understand, the vdpa_dev_set_config
> struct is designed to be built around vdpa configurables, without getting it
> limited by what's exposed by the virtio device config structure, such as
> virtio_net_config.
Sure. Vdpa_dev_set_config can expand for fields outside of virtio_net_config structure space, but it should be close to virtio spec definition like you described below or close to Linux kernel objects.
At present it can handle another 62 more fields, which I think is good enough for midterm.
> For instance, there could be possibility for vdpa user to
> specify the size of MAC unicast or multicast address table, which is not
> defined anywhere in the virtio_net_config. I think it's important to match
> such configuration (which may not even be defined in spec) for src&dst vdpa
> devices involving the live migration.
>
> -Siwei
> >
> > Thanks
> >
> >> + default:
> >> + break;
> >> + }
> >> +
> >> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) != 0 &&
> >> + nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES,
> >> + cfg->device_features, VDPA_ATTR_PAD))
> >> + return err;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int
> >> vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
> u32 seq,
> >> int flags, struct netlink_ext_ack *extack)
> >> {
> >> @@ -715,6 +750,10 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct
> sk_buff *skb, struct genl_info *i
> >> if (nla_put_u16(msg, VDPA_ATTR_DEV_MIN_VQ_SIZE, min_vq_size))
> >> goto msg_err;
> >>
> >> + err = vdpa_dev_cfgattrs_fill(vdev, msg, device_id);
> >> + if (err)
> >> + goto msg_err;
> >> +
> >> genlmsg_end(msg, hdr);
> >> return 0;
> >>
> >> --
> >> 1.8.3.1
> >>
On Fri, Oct 21, 2022 at 2:45 AM Si-Wei Liu <[email protected]> wrote:
>
>
>
> On 10/19/2022 10:20 PM, Jason Wang wrote:
> > On Wed, Oct 19, 2022 at 8:56 AM Si-Wei Liu <[email protected]> wrote:
> >> Just as _vdpa_register_device taking @nvqs as the number of queues
> > I wonder if it's better to embed nvqs in the config structure.
> Hmmm, the config structure is mostly for containing the configurables
> specified in the 'vdpa dev add' command, while each field is
> conditionally set and guarded by a corresponding mask bit. If @nvqs
> needs to be folded into a structure, I feel it might be better to use
> another struct for holding the informational fields (i.e. those are
> read-only and always exist). But doing this would make @nvqs a weird
> solo member in that struct with no extra benefit, and all the other
> informational fields shown in the 'vdpa dev show' command would be
> gotten from the device through config_ops directly. Maybe do this until
> another read-only field comes around?
That's fine.
>
> >
> >> to feed userspace inquery via vdpa_dev_fill(), we can follow the
> >> same to stash config attributes in struct vdpa_device at the time
> >> of vdpa registration.
> >>
> >> Signed-off-by: Si-Wei Liu <[email protected]>
> >> ---
> >> drivers/vdpa/ifcvf/ifcvf_main.c | 2 +-
> >> drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +-
> >> drivers/vdpa/vdpa.c | 15 +++++++++++----
> >> drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 2 +-
> >> drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 2 +-
> >> drivers/vdpa/vdpa_user/vduse_dev.c | 2 +-
> >> drivers/vdpa/virtio_pci/vp_vdpa.c | 3 ++-
> >> include/linux/vdpa.h | 3 ++-
> >> 8 files changed, 20 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> >> index f9c0044..c54ab2c 100644
> >> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> >> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> >> @@ -771,7 +771,7 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
> >> else
> >> ret = dev_set_name(&vdpa_dev->dev, "vdpa%u", vdpa_dev->index);
> >>
> >> - ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring);
> >> + ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring, config);
> >> if (ret) {
> >> put_device(&adapter->vdpa.dev);
> >> IFCVF_ERR(pdev, "Failed to register to vDPA bus");
> >> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >> index 9091336..376082e 100644
> >> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >> @@ -3206,7 +3206,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> >> mlx5_notifier_register(mdev, &ndev->nb);
> >> ndev->nb_registered = true;
> >> mvdev->vdev.mdev = &mgtdev->mgtdev;
> >> - err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
> >> + err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1, add_config);
> >> if (err)
> >> goto err_reg;
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >> index febdc99..566c1c6 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -215,11 +215,16 @@ static int vdpa_name_match(struct device *dev, const void *data)
> >> return (strcmp(dev_name(&vdev->dev), data) == 0);
> >> }
> >>
> >> -static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs)
> >> +static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs,
> >> + const struct vdpa_dev_set_config *cfg)
> >> {
> >> struct device *dev;
> >>
> >> vdev->nvqs = nvqs;
> >> + if (cfg)
> >> + vdev->vdev_cfg = *cfg;
> >> + else
> >> + vdev->vdev_cfg.mask = 0ULL;
> > I think it would be nice if we can convert eni to use netlink then we
> > don't need any workaround like this.
> Yes, Alibaba ENI is the only consumer of the old vdpa_register_device()
> API without being ported to the netlink API. Not sure what is needed but
> it seems another work to make netlink API committed to support a legacy
> compatible model?
It's only about the provisioning (which is kind of out of the spec).
So if I was not wrong, it should be something similar like the work
that Cindy has done, (per VF mgmtdev):
commit ffbda8e9df10d1784d5427ec199e7d8308e3763f
Author: Cindy Lu <[email protected]>
Date: Fri Apr 29 17:10:30 2022 +0800
vdpa/vp_vdpa : add vdpa tool support in vp_vdpa
Thanks
>
> -Siwei
>
> >
> > Thanks
> >
>
On 10/20/2022 7:51 PM, Jason Wang wrote:
> On Fri, Oct 21, 2022 at 2:45 AM Si-Wei Liu <[email protected]> wrote:
>>
>>
>> On 10/19/2022 10:20 PM, Jason Wang wrote:
>>> On Wed, Oct 19, 2022 at 8:56 AM Si-Wei Liu <[email protected]> wrote:
>>>> Just as _vdpa_register_device taking @nvqs as the number of queues
>>> I wonder if it's better to embed nvqs in the config structure.
>> Hmmm, the config structure is mostly for containing the configurables
>> specified in the 'vdpa dev add' command, while each field is
>> conditionally set and guarded by a corresponding mask bit. If @nvqs
>> needs to be folded into a structure, I feel it might be better to use
>> another struct for holding the informational fields (i.e. those are
>> read-only and always exist). But doing this would make @nvqs a weird
>> solo member in that struct with no extra benefit, and all the other
>> informational fields shown in the 'vdpa dev show' command would be
>> gotten from the device through config_ops directly. Maybe do this until
>> another read-only field comes around?
> That's fine.
>
>>>> to feed userspace inquery via vdpa_dev_fill(), we can follow the
>>>> same to stash config attributes in struct vdpa_device at the time
>>>> of vdpa registration.
>>>>
>>>> Signed-off-by: Si-Wei Liu <[email protected]>
>>>> ---
>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 2 +-
>>>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +-
>>>> drivers/vdpa/vdpa.c | 15 +++++++++++----
>>>> drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 2 +-
>>>> drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 2 +-
>>>> drivers/vdpa/vdpa_user/vduse_dev.c | 2 +-
>>>> drivers/vdpa/virtio_pci/vp_vdpa.c | 3 ++-
>>>> include/linux/vdpa.h | 3 ++-
>>>> 8 files changed, 20 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> index f9c0044..c54ab2c 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> @@ -771,7 +771,7 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>>>> else
>>>> ret = dev_set_name(&vdpa_dev->dev, "vdpa%u", vdpa_dev->index);
>>>>
>>>> - ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring);
>>>> + ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring, config);
>>>> if (ret) {
>>>> put_device(&adapter->vdpa.dev);
>>>> IFCVF_ERR(pdev, "Failed to register to vDPA bus");
>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>> index 9091336..376082e 100644
>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>> @@ -3206,7 +3206,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>>>> mlx5_notifier_register(mdev, &ndev->nb);
>>>> ndev->nb_registered = true;
>>>> mvdev->vdev.mdev = &mgtdev->mgtdev;
>>>> - err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
>>>> + err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1, add_config);
>>>> if (err)
>>>> goto err_reg;
>>>>
>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>> index febdc99..566c1c6 100644
>>>> --- a/drivers/vdpa/vdpa.c
>>>> +++ b/drivers/vdpa/vdpa.c
>>>> @@ -215,11 +215,16 @@ static int vdpa_name_match(struct device *dev, const void *data)
>>>> return (strcmp(dev_name(&vdev->dev), data) == 0);
>>>> }
>>>>
>>>> -static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs)
>>>> +static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs,
>>>> + const struct vdpa_dev_set_config *cfg)
>>>> {
>>>> struct device *dev;
>>>>
>>>> vdev->nvqs = nvqs;
>>>> + if (cfg)
>>>> + vdev->vdev_cfg = *cfg;
>>>> + else
>>>> + vdev->vdev_cfg.mask = 0ULL;
>>> I think it would be nice if we can convert eni to use netlink then we
>>> don't need any workaround like this.
>> Yes, Alibaba ENI is the only consumer of the old vdpa_register_device()
>> API without being ported to the netlink API. Not sure what is needed but
>> it seems another work to make netlink API committed to support a legacy
>> compatible model?
> It's only about the provisioning (which is kind of out of the spec).
> So if I was not wrong, it should be something similar like the work
> that Cindy has done, (per VF mgmtdev):
>
> commit ffbda8e9df10d1784d5427ec199e7d8308e3763f
> Author: Cindy Lu <[email protected]>
> Date: Fri Apr 29 17:10:30 2022 +0800
>
> vdpa/vp_vdpa : add vdpa tool support in vp_vdpa
OK, I was thinking of something else for e.g. support legacy driver
changing the default MAC with VIRTIO_NET_F_MAC provisioned. Then it
looks Alibaba eni_vdpa can only be provisioned without any config
attribute specified, thus unable to enjoy the other benefit from the
netlink API. Since it's kind of out of scope of my work, I'd like to
have the author of this driver (cc'ed) to make decision for it.
-Siwei
>
> Thanks
>
>> -Siwei
>>
>>> Thanks
>>>