Re: [PATCH V2] vdpa: allow provisioning device features

[Si-Wei Liu <si-wei.liu@xxxxxxxxxx>]

On 11/16/2022 7:33 PM, Jason Wang wrote:
> This patch allows device features to be provisioned via vdpa. This
> will be useful for preserving migration compatibility between source
> and destination:
>
> # vdpa dev add name dev1 mgmtdev pci/0000:02:00.0 device_features 0x300020000
Miss the actual "vdpa dev config show" command below
> # dev1: mac 52:54:00:12:34:56 link up link_announce false mtu 65535
>        negotiated_features CTRL_VQ VERSION_1 ACCESS_PLATFORM
>
> Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> ---
> Changes since v1:
> - Use uint64_t instead of __u64 for device_features
> - Fix typos and tweak the manpage
> - Add device_features to the help text
> ---
>   man/man8/vdpa-dev.8            | 15 +++++++++++++++
>   vdpa/include/uapi/linux/vdpa.h |  1 +
>   vdpa/vdpa.c                    | 32 +++++++++++++++++++++++++++++---
>   3 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/man/man8/vdpa-dev.8 b/man/man8/vdpa-dev.8
> index 9faf3838..43e5bf48 100644
> --- a/man/man8/vdpa-dev.8
> +++ b/man/man8/vdpa-dev.8
> @@ -31,6 +31,7 @@ vdpa-dev \- vdpa device configuration
>   .I NAME
>   .B mgmtdev
>   .I MGMTDEV
> +.RI "[ device_features " DEVICE_FEATURES " ]"
>   .RI "[ mac " MACADDR " ]"
>   .RI "[ mtu " MTU " ]"
>   .RI "[ max_vqp " MAX_VQ_PAIRS " ]"
> @@ -74,6 +75,15 @@ Name of the new vdpa device to add.
>   Name of the management device to use for device addition.
>   
>   .PP
> +.BI device_features " DEVICE_FEATURES"
> +Specifies the virtio device features bit-mask that is provisioned for the new vdpa device.
> +
> +The bits can be found under include/uapi/linux/virtio*h.
> +
> +see macros such as VIRTIO_F_ and VIRTIO_XXX(e.g NET)_F_ for specific bit values.
> +
> +This is optional.
Document the behavior when this attribute is missing? For e.g. inherit 
device features from parent device.

And what is the expected behavior when feature bit mask is off but the 
corresponding config attr (for e.g. mac, mtu, and max_vqp) is set? I 
think the previous behavior without device_features is that any config 
attr implies the presence of the specific corresponding feature (_F_MAC, 
_F_MTU, and _F_MQ). Should device_features override the other config 
attribute, or such combination is considered invalid thus should fail?

Thanks,
-Siwei

> +
>   .BI mac " MACADDR"
>   - specifies the mac address for the new vdpa device.
>   This is applicable only for the network type of vdpa device. This is optional.
> @@ -127,6 +137,11 @@ vdpa dev add name foo mgmtdev vdpa_sim_net
>   Add the vdpa device named foo on the management device vdpa_sim_net.
>   .RE
>   .PP
> +vdpa dev add name foo mgmtdev vdpa_sim_net device_features 0x300020000
> +.RS 4
> +Add the vdpa device named foo on the management device vdpa_sim_net with device_features of 0x300020000
> +.RE
> +.PP
>   vdpa dev add name foo mgmtdev vdpa_sim_net mac 00:11:22:33:44:55
>   .RS 4
>   Add the vdpa device named foo on the management device vdpa_sim_net with mac address of 00:11:22:33:44:55.
> diff --git a/vdpa/include/uapi/linux/vdpa.h b/vdpa/include/uapi/linux/vdpa.h
> index 94e4dad1..7c961991 100644
> --- a/vdpa/include/uapi/linux/vdpa.h
> +++ b/vdpa/include/uapi/linux/vdpa.h
> @@ -51,6 +51,7 @@ enum vdpa_attr {
>   	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
>   	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
>   	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
> +	VDPA_ATTR_DEV_FEATURES,                 /* u64 */
>   
>   	/* new attributes must be added above here */
>   	VDPA_ATTR_MAX,
> diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c
> index b73e40b4..d0ce5e22 100644
> --- a/vdpa/vdpa.c
> +++ b/vdpa/vdpa.c
> @@ -27,6 +27,7 @@
>   #define VDPA_OPT_VDEV_MTU		BIT(5)
>   #define VDPA_OPT_MAX_VQP		BIT(6)
>   #define VDPA_OPT_QUEUE_INDEX		BIT(7)
> +#define VDPA_OPT_VDEV_FEATURES		BIT(8)
>   
>   struct vdpa_opts {
>   	uint64_t present; /* flags of present items */
> @@ -38,6 +39,7 @@ struct vdpa_opts {
>   	uint16_t mtu;
>   	uint16_t max_vqp;
>   	uint32_t queue_idx;
> +	uint64_t device_features;
>   };
>   
>   struct vdpa {
> @@ -187,6 +189,17 @@ static int vdpa_argv_u32(struct vdpa *vdpa, int argc, char **argv,
>   	return get_u32(result, *argv, 10);
>   }
>   
> +static int vdpa_argv_u64_hex(struct vdpa *vdpa, int argc, char **argv,
> +			     uint64_t *result)
> +{
> +	if (argc <= 0 || !*argv) {
> +		fprintf(stderr, "number expected\n");
> +		return -EINVAL;
> +	}
> +
> +	return get_u64(result, *argv, 16);
> +}
> +
>   struct vdpa_args_metadata {
>   	uint64_t o_flag;
>   	const char *err_msg;
> @@ -244,6 +257,10 @@ static void vdpa_opts_put(struct nlmsghdr *nlh, struct vdpa *vdpa)
>   		mnl_attr_put_u16(nlh, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, opts->max_vqp);
>   	if (opts->present & VDPA_OPT_QUEUE_INDEX)
>   		mnl_attr_put_u32(nlh, VDPA_ATTR_DEV_QUEUE_INDEX, opts->queue_idx);
> +	if (opts->present & VDPA_OPT_VDEV_FEATURES) {
> +		mnl_attr_put_u64(nlh, VDPA_ATTR_DEV_FEATURES,
> +				opts->device_features);
> +	}
>   }
>   
>   static int vdpa_argv_parse(struct vdpa *vdpa, int argc, char **argv,
> @@ -329,6 +346,14 @@ static int vdpa_argv_parse(struct vdpa *vdpa, int argc, char **argv,
>   
>   			NEXT_ARG_FWD();
>   			o_found |= VDPA_OPT_QUEUE_INDEX;
> +		} else if (!strcmp(*argv, "device_features") &&
> +			   (o_optional & VDPA_OPT_VDEV_FEATURES)) {
> +			NEXT_ARG_FWD();
> +			err = vdpa_argv_u64_hex(vdpa, argc, argv,
> +						&opts->device_features);
> +			if (err)
> +				return err;
> +			o_found |= VDPA_OPT_VDEV_FEATURES;
>   		} else {
>   			fprintf(stderr, "Unknown option \"%s\"\n", *argv);
>   			return -EINVAL;
> @@ -615,8 +640,9 @@ static int cmd_mgmtdev(struct vdpa *vdpa, int argc, char **argv)
>   static void cmd_dev_help(void)
>   {
>   	fprintf(stderr, "Usage: vdpa dev show [ DEV ]\n");
> -	fprintf(stderr, "       vdpa dev add name NAME mgmtdev MANAGEMENTDEV [ mac MACADDR ] [ mtu MTU ]\n");
> -	fprintf(stderr, "                                                    [ max_vqp MAX_VQ_PAIRS ]\n");
> +	fprintf(stderr, "       vdpa dev add name NAME mgmtdevMANAGEMENTDEV [ device_features DEVICE_FEATURES]\n");
> +	fprintf(stderr, "                                                   [ mac MACADDR ] [ mtu MTU ]\n");
> +	fprintf(stderr, "                                                   [ max_vqp MAX_VQ_PAIRS ]\n");
>   	fprintf(stderr, "       vdpa dev del DEV\n");
>   	fprintf(stderr, "Usage: vdpa dev config COMMAND [ OPTIONS ]\n");
>   	fprintf(stderr, "Usage: vdpa dev vstats COMMAND\n");
> @@ -708,7 +734,7 @@ static int cmd_dev_add(struct vdpa *vdpa, int argc, char **argv)
>   	err = vdpa_argv_parse_put(nlh, vdpa, argc, argv,
>   				  VDPA_OPT_VDEV_MGMTDEV_HANDLE | VDPA_OPT_VDEV_NAME,
>   				  VDPA_OPT_VDEV_MAC | VDPA_OPT_VDEV_MTU |
> -				  VDPA_OPT_MAX_VQP);
> +				  VDPA_OPT_MAX_VQP | VDPA_OPT_VDEV_FEATURES);
>   	if (err)
>   		return err;
>   

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization