[PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Alvaro Karsz]

Implement the VIRTIO_BLK_F_LIFETIME feature for VirtIO block devices.

This commit introduces a new ioctl command, VBLK_LIFETIME.

VBLK_LIFETIME ioctl asks for the block device to provide lifetime
information by sending a VIRTIO_BLK_T_GET_LIFETIME command to the device.

lifetime information fields:

- pre_eol_info: specifies the percentage of reserved blocks that are
		consumed.
		optional values following virtio spec:
		*) 0 - undefined.
		*) 1 - normal, < 80% of reserved blocks are consumed.
		*) 2 - warning, 80% of reserved blocks are consumed.
		*) 3 - urgent, 90% of reserved blocks are consumed.

- device_lifetime_est_typ_a: this field refers to wear of SLC cells and
			     is provided in increments of 10used, and so
			     on, thru to 11 meaning estimated lifetime
			     exceeded. All values above 11 are reserved.

- device_lifetime_est_typ_b: this field refers to wear of MLC cells and is
			     provided with the same semantics as
			     device_lifetime_est_typ_a.

The data received from the device will be sent as is to the user.
No data check/decode is done by virtblk.

Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
--
v2:
	- Remove (void *) casting.
	- Fix comments format in virtio_blk.h.
	- Set ioprio value for legacy devices for REQ_OP_DRV_IN
	  operations.
--
---
 drivers/block/virtio_blk.c      | 100 ++++++++++++++++++++++++++++++--
 include/uapi/linux/virtio_blk.h |  32 ++++++++++
 2 files changed, 127 insertions(+), 5 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 19da5defd73..9aa37677b65 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -101,6 +101,18 @@ static inline blk_status_t virtblk_result(struct virtblk_req *vbr)
 	}
 }
 
+static inline int virtblk_ioctl_result(struct virtblk_req *vbr)
+{
+	switch (vbr->status) {
+	case VIRTIO_BLK_S_OK:
+		return 0;
+	case VIRTIO_BLK_S_UNSUPP:
+		return -ENOTTY;
+	default:
+		return -EIO;
+	}
+}
+
 static inline struct virtio_blk_vq *get_virtio_blk_vq(struct blk_mq_hw_ctx *hctx)
 {
 	struct virtio_blk *vblk = hctx->queue->queuedata;
@@ -218,6 +230,7 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
 	u32 type;
 
 	vbr->out_hdr.sector = 0;
+	vbr->out_hdr.ioprio = cpu_to_virtio32(vdev, req_get_ioprio(req));
 
 	switch (req_op(req)) {
 	case REQ_OP_READ:
@@ -244,15 +257,14 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
 		type = VIRTIO_BLK_T_SECURE_ERASE;
 		break;
 	case REQ_OP_DRV_IN:
-		type = VIRTIO_BLK_T_GET_ID;
-		break;
+		/* type is set in virtblk_get_id/virtblk_ioctl_lifetime */
+		return 0;
 	default:
 		WARN_ON_ONCE(1);
 		return BLK_STS_IOERR;
 	}
 
 	vbr->out_hdr.type = cpu_to_virtio32(vdev, type);
-	vbr->out_hdr.ioprio = cpu_to_virtio32(vdev, req_get_ioprio(req));
 
 	if (type == VIRTIO_BLK_T_DISCARD || type == VIRTIO_BLK_T_WRITE_ZEROES ||
 	    type == VIRTIO_BLK_T_SECURE_ERASE) {
@@ -459,12 +471,16 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
 	struct virtio_blk *vblk = disk->private_data;
 	struct request_queue *q = vblk->disk->queue;
 	struct request *req;
+	struct virtblk_req *vbr;
 	int err;
 
 	req = blk_mq_alloc_request(q, REQ_OP_DRV_IN, 0);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
+	vbr = blk_mq_rq_to_pdu(req);
+	vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_GET_ID);
+
 	err = blk_rq_map_kern(q, req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
 	if (err)
 		goto out;
@@ -508,6 +524,79 @@ static int virtblk_getgeo(struct block_device *bd, struct hd_geometry *geo)
 	return ret;
 }
 
+/* Get lifetime information from device */
+static int virtblk_ioctl_lifetime(struct virtio_blk *vblk, unsigned long arg)
+{
+	struct request_queue *q = vblk->disk->queue;
+	struct request *req = NULL;
+	struct virtblk_req *vbr;
+	struct virtio_blk_lifetime lifetime;
+	int ret;
+
+	/* The virtio_blk_lifetime struct fields follow virtio spec.
+	 * There is no check/decode on values received from the device.
+	 * The data is sent as is to the user.
+	 */
+
+	/* This ioctl is allowed only if VIRTIO_BLK_F_LIFETIME
+	 * feature is negotiated.
+	 */
+	if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_LIFETIME))
+		return -ENOTTY;
+
+	memset(&lifetime, 0, sizeof(lifetime));
+
+	req = blk_mq_alloc_request(q, REQ_OP_DRV_IN, 0);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
+
+	/* Write the correct type */
+	vbr = blk_mq_rq_to_pdu(req);
+	vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_GET_LIFETIME);
+
+	ret = blk_rq_map_kern(q, req, &lifetime, sizeof(lifetime), GFP_KERNEL);
+	if (ret)
+		goto out;
+
+	blk_execute_rq(req, false);
+
+	ret = virtblk_ioctl_result(blk_mq_rq_to_pdu(req));
+	if (ret)
+		goto out;
+
+	/* Pass the data to the user */
+	if (copy_to_user((void __user *)arg, &lifetime, sizeof(lifetime))) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+out:
+	blk_mq_free_request(req);
+	return ret;
+}
+
+static int virtblk_ioctl(struct block_device *bd, fmode_t mode,
+			     unsigned int cmd, unsigned long arg)
+{
+	struct virtio_blk *vblk = bd->bd_disk->private_data;
+	int ret;
+
+	mutex_lock(&vblk->vdev_mutex);
+
+	switch (cmd) {
+	case VBLK_LIFETIME:
+		ret = virtblk_ioctl_lifetime(vblk, arg);
+		break;
+	default:
+		ret = -ENOTTY;
+		break;
+	}
+
+	mutex_unlock(&vblk->vdev_mutex);
+
+	return ret;
+}
+
 static void virtblk_free_disk(struct gendisk *disk)
 {
 	struct virtio_blk *vblk = disk->private_data;
@@ -520,6 +609,7 @@ static void virtblk_free_disk(struct gendisk *disk)
 static const struct block_device_operations virtblk_fops = {
 	.owner  	= THIS_MODULE,
 	.getgeo		= virtblk_getgeo,
+	.ioctl		= virtblk_ioctl,
 	.free_disk	= virtblk_free_disk,
 };
 
@@ -1239,7 +1329,7 @@ static unsigned int features_legacy[] = {
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
 	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
-	VIRTIO_BLK_F_SECURE_ERASE,
+	VIRTIO_BLK_F_SECURE_ERASE, VIRTIO_BLK_F_LIFETIME,
 }
 ;
 static unsigned int features[] = {
@@ -1247,7 +1337,7 @@ static unsigned int features[] = {
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
 	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
-	VIRTIO_BLK_F_SECURE_ERASE,
+	VIRTIO_BLK_F_SECURE_ERASE, VIRTIO_BLK_F_LIFETIME,
 };
 
 static struct virtio_driver virtio_blk = {
diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index 58e70b24b50..c769930d269 100644
--- a/include/uapi/linux/virtio_blk.h
+++ b/include/uapi/linux/virtio_blk.h
@@ -40,6 +40,7 @@
 #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
 #define VIRTIO_BLK_F_DISCARD	13	/* DISCARD is supported */
 #define VIRTIO_BLK_F_WRITE_ZEROES	14	/* WRITE ZEROES is supported */
+#define VIRTIO_BLK_F_LIFETIME	15 /* Storage lifetime information is supported */
 #define VIRTIO_BLK_F_SECURE_ERASE	16 /* Secure Erase is supported */
 
 /* Legacy feature bits */
@@ -165,6 +166,9 @@ struct virtio_blk_config {
 /* Get device ID command */
 #define VIRTIO_BLK_T_GET_ID    8
 
+/* Get lifetime information command */
+#define VIRTIO_BLK_T_GET_LIFETIME 10
+
 /* Discard command */
 #define VIRTIO_BLK_T_DISCARD	11
 
@@ -206,6 +210,30 @@ struct virtio_blk_discard_write_zeroes {
 	__le32 flags;
 };
 
+/* Get lifetime information struct for each request */
+struct virtio_blk_lifetime {
+	/*
+	 * specifies the percentage of reserved blocks that are consumed.
+	 * optional values following virtio spec:
+	 * 0 - undefined
+	 * 1 - normal, < 80% of reserved blocks are consumed
+	 * 2 - warning, 80% of reserved blocks are consumed
+	 * 3 - urgent, 90% of reserved blocks are consumed
+	 */
+	__le16 pre_eol_info;
+	/*
+	 * this field refers to wear of SLC cells and is provided in increments of 10used,
+	 * and so on, thru to 11 meaning estimated lifetime exceeded. All values above 11
+	 * are reserved
+	 */
+	__le16 device_lifetime_est_typ_a;
+	/*
+	 * this field refers to wear of MLC cells and is provided with the same semantics as
+	 * device_lifetime_est_typ_a
+	 */
+	__le16 device_lifetime_est_typ_b;
+};
+
 #ifndef VIRTIO_BLK_NO_LEGACY
 struct virtio_scsi_inhdr {
 	__virtio32 errors;
@@ -219,4 +247,8 @@ struct virtio_scsi_inhdr {
 #define VIRTIO_BLK_S_OK		0
 #define VIRTIO_BLK_S_IOERR	1
 #define VIRTIO_BLK_S_UNSUPP	2
+
+/* Virtblk ioctl commands */
+#define VBLK_LIFETIME	_IOR('r', 0, struct virtio_blk_lifetime)
+
 #endif /* _LINUX_VIRTIO_BLK_H */
-- 
2.32.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Michael S. Tsirkin]

On Wed, Nov 23, 2022 at 10:41:46PM +0000, Chaitanya Kulkarni wrote:
> 
> > +/* Get lifetime information from device */
> > +static int virtblk_ioctl_lifetime(struct virtio_blk *vblk, unsigned long arg)
> > +{
> > +	struct request_queue *q = vblk->disk->queue;
> > +	struct request *req = NULL;
> > +	struct virtblk_req *vbr;
> > +	struct virtio_blk_lifetime lifetime;
> > +	int ret;
> > +
> > +	/* The virtio_blk_lifetime struct fields follow virtio spec.
> > +	 * There is no check/decode on values received from the device.
> > +	 * The data is sent as is to the user.
> > +	 */
> 
> Odd comment style, why not :-
> 
> 	/*
> 	 * The virtio_blk_lifetime struct fields follow virtio spec.
> 	 * There is no check/decode on values received from the device.
> 	 * The data is sent as is to the user.
> 	 */

most of virtio blk is like this. I don't really care much.

> > +
> > +	/* This ioctl is allowed only if VIRTIO_BLK_F_LIFETIME
> > +	 * feature is negotiated.
> > +	 */
> 
> above comment is redundant to the below code as following code is
> self explanatory.,..

i find it helpful personally - this point is important enough to
stress at cost of some duplication.

> > +	if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_LIFETIME))
> > +		return -ENOTTY;
> > +
> > +	memset(&lifetime, 0, sizeof(lifetime));
> > +
> > +	req = blk_mq_alloc_request(q, REQ_OP_DRV_IN, 0);
> > +	if (IS_ERR(req))
> > +		return PTR_ERR(req);
> > +
> > +	/* Write the correct type */
> 
> same here for above comment...

it's pretty harmless though

> > +	vbr = blk_mq_rq_to_pdu(req);
> > +	vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_GET_LIFETIME);
> > +
> > +	ret = blk_rq_map_kern(q, req, &lifetime, sizeof(lifetime), GFP_KERNEL);
> > +	if (ret)
> > +		goto out;
> > +
> > +	blk_execute_rq(req, false);
> > +
> > +	ret = virtblk_ioctl_result(blk_mq_rq_to_pdu(req));
> > +	if (ret)
> > +		goto out;
> > +
> > +	/* Pass the data to the user */
> > +	if (copy_to_user((void __user *)arg, &lifetime, sizeof(lifetime))) {
> > +		ret = -EFAULT;
> > +		goto out;
> 
> there nothing here to "goto out" following is sufficient I guess :-
> 
> if (copy_to_user((void __user *)arg, &lifetime, sizeof(lifetime)))
> 	ret = -EFAULT;

error handling with goto seems cleaner, easier to add code down
the road.

> > +	}
> > +
> > +out:
> > +	blk_mq_free_request(req);
> > +	return ret;
> > +}
> > +
> 
> [...]
> 
>    };
> >   
> > +/* Get lifetime information struct for each request */
> > +struct virtio_blk_lifetime {
> > +	/*
> > +	 * specifies the percentage of reserved blocks that are consumed.
> > +	 * optional values following virtio spec:
> > +	 * 0 - undefined
> > +	 * 1 - normal, < 80% of reserved blocks are consumed
> > +	 * 2 - warning, 80% of reserved blocks are consumed
> > +	 * 3 - urgent, 90% of reserved blocks are consumed
> > +	 */
> > +	__le16 pre_eol_info;
> > +	/*
> > +	 * this field refers to wear of SLC cells and is provided in increments of 10used,
> > +	 * and so on, thru to 11 meaning estimated lifetime exceeded. All values above 11
> > +	 * are reserved
> > +	 */
> > +	__le16 device_lifetime_est_typ_a;
> > +	/*
> > +	 * this field refers to wear of MLC cells and is provided with the same semantics as
> > +	 * device_lifetime_est_typ_a
> > +	 */
> > +	__le16 device_lifetime_est_typ_b;
> > +};
> > +
> 
> are you sure there won't be any new members ever be added in future ?
> to make it is futuresafe I'd add padding to this structure and keep
> the reserve bytes to the meaningful size...
> 
> -ck


virtio has feature bits for this kind of thing, we don't do padding.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Michael S. Tsirkin]

On Wed, Nov 23, 2022 at 10:22:26PM +0000, Chaitanya Kulkarni wrote:
> 
> > +/* Get lifetime information from device */
> > +static int virtblk_ioctl_lifetime(struct virtio_blk *vblk, unsigned long arg)
> > +{
> > +	struct request_queue *q = vblk->disk->queue;
> > +	struct request *req = NULL;
> > +	struct virtblk_req *vbr;
> > +	struct virtio_blk_lifetime lifetime;
> > +	int ret;
> > +
> > +	/* The virtio_blk_lifetime struct fields follow virtio spec.
> > +	 * There is no check/decode on values received from the device.
> > +	 * The data is sent as is to the user.
> > +	 */
> > +
> > +	/* This ioctl is allowed only if VIRTIO_BLK_F_LIFETIME
> > +	 * feature is negotiated.
> > +	 */
> > +	if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_LIFETIME))
> > +		return -ENOTTY;
> > +
> > +	memset(&lifetime, 0, sizeof(lifetime));
> > +
> 
> you can remove memset 0 call here and declare initialize struct var
> something like totally untested :-
> 
> 	struct virtio_blk_lifetime lifetime = { };
> 
> -ck

Yes, that's a bit cleaner, but there should be no space between {}:
	struct virtio_blk_lifetime lifetime = {};


-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Alvaro Karsz]

Thanks for your comments.

> > +     /* Pass the data to the user */
> > +     if (copy_to_user((void __user *)arg, &lifetime, sizeof(lifetime))) {
> > +             ret = -EFAULT;
> > +             goto out;
>
>
> there nothing here to "goto out" following is sufficient I guess :-


You're right, but like Michael said, it will be pretty easy to forget
adding a goto statement if adding more code to the function.

> > +     memset(&lifetime, 0, sizeof(lifetime));
> > +
>
>
> you can remove memset 0 call here and declare initialize struct var
> something like totally untested :-
>
>
>         struct virtio_blk_lifetime lifetime = { };


>
> Yes, that's a bit cleaner, but there should be no space between {}:
>         struct virtio_blk_lifetime lifetime = {};


I will fix it in the next version.

Alvaro
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 10666 bytes --]

On Mon, Nov 21, 2022 at 10:59:23AM +0200, Alvaro Karsz wrote:
> Implement the VIRTIO_BLK_F_LIFETIME feature for VirtIO block devices.
> 
> This commit introduces a new ioctl command, VBLK_LIFETIME.

How about naming it VBLK_GET_LIFETIME? It's clearer what the ioctl does
and it follows the name of virtio-blk request type.

> 
> VBLK_LIFETIME ioctl asks for the block device to provide lifetime
> information by sending a VIRTIO_BLK_T_GET_LIFETIME command to the device.
> 
> lifetime information fields:
> 
> - pre_eol_info: specifies the percentage of reserved blocks that are
> 		consumed.
> 		optional values following virtio spec:
> 		*) 0 - undefined.
> 		*) 1 - normal, < 80% of reserved blocks are consumed.
> 		*) 2 - warning, 80% of reserved blocks are consumed.
> 		*) 3 - urgent, 90% of reserved blocks are consumed.
> 
> - device_lifetime_est_typ_a: this field refers to wear of SLC cells and
> 			     is provided in increments of 10used, and so
> 			     on, thru to 11 meaning estimated lifetime
> 			     exceeded. All values above 11 are reserved.
> 
> - device_lifetime_est_typ_b: this field refers to wear of MLC cells and is
> 			     provided with the same semantics as
> 			     device_lifetime_est_typ_a.
> 
> The data received from the device will be sent as is to the user.
> No data check/decode is done by virtblk.
> 
> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> --
> v2:
> 	- Remove (void *) casting.
> 	- Fix comments format in virtio_blk.h.
> 	- Set ioprio value for legacy devices for REQ_OP_DRV_IN
> 	  operations.
> --
> ---
>  drivers/block/virtio_blk.c      | 100 ++++++++++++++++++++++++++++++--
>  include/uapi/linux/virtio_blk.h |  32 ++++++++++
>  2 files changed, 127 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 19da5defd73..9aa37677b65 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -101,6 +101,18 @@ static inline blk_status_t virtblk_result(struct virtblk_req *vbr)
>  	}
>  }
>  
> +static inline int virtblk_ioctl_result(struct virtblk_req *vbr)
> +{
> +	switch (vbr->status) {
> +	case VIRTIO_BLK_S_OK:
> +		return 0;
> +	case VIRTIO_BLK_S_UNSUPP:
> +		return -ENOTTY;

ENOTTY already has meaning for ioctl(2):

  ENOTTY fd is not associated with a character special device.

  ENOTTY The specified request does not apply to the kind of object that the file descriptor fd references.

Use ENOTSUP instead?

> +	default:
> +		return -EIO;
> +	}
> +}
> +
>  static inline struct virtio_blk_vq *get_virtio_blk_vq(struct blk_mq_hw_ctx *hctx)
>  {
>  	struct virtio_blk *vblk = hctx->queue->queuedata;
> @@ -218,6 +230,7 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
>  	u32 type;
>  
>  	vbr->out_hdr.sector = 0;
> +	vbr->out_hdr.ioprio = cpu_to_virtio32(vdev, req_get_ioprio(req));
>  
>  	switch (req_op(req)) {
>  	case REQ_OP_READ:
> @@ -244,15 +257,14 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
>  		type = VIRTIO_BLK_T_SECURE_ERASE;
>  		break;
>  	case REQ_OP_DRV_IN:
> -		type = VIRTIO_BLK_T_GET_ID;
> -		break;
> +		/* type is set in virtblk_get_id/virtblk_ioctl_lifetime */
> +		return 0;
>  	default:
>  		WARN_ON_ONCE(1);
>  		return BLK_STS_IOERR;
>  	}
>  
>  	vbr->out_hdr.type = cpu_to_virtio32(vdev, type);
> -	vbr->out_hdr.ioprio = cpu_to_virtio32(vdev, req_get_ioprio(req));
>  
>  	if (type == VIRTIO_BLK_T_DISCARD || type == VIRTIO_BLK_T_WRITE_ZEROES ||
>  	    type == VIRTIO_BLK_T_SECURE_ERASE) {
> @@ -459,12 +471,16 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
>  	struct virtio_blk *vblk = disk->private_data;
>  	struct request_queue *q = vblk->disk->queue;
>  	struct request *req;
> +	struct virtblk_req *vbr;
>  	int err;
>  
>  	req = blk_mq_alloc_request(q, REQ_OP_DRV_IN, 0);
>  	if (IS_ERR(req))
>  		return PTR_ERR(req);
>  
> +	vbr = blk_mq_rq_to_pdu(req);
> +	vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_GET_ID);
> +
>  	err = blk_rq_map_kern(q, req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
>  	if (err)
>  		goto out;
> @@ -508,6 +524,79 @@ static int virtblk_getgeo(struct block_device *bd, struct hd_geometry *geo)
>  	return ret;
>  }
>  
> +/* Get lifetime information from device */
> +static int virtblk_ioctl_lifetime(struct virtio_blk *vblk, unsigned long arg)
> +{
> +	struct request_queue *q = vblk->disk->queue;
> +	struct request *req = NULL;
> +	struct virtblk_req *vbr;
> +	struct virtio_blk_lifetime lifetime;
> +	int ret;
> +
> +	/* The virtio_blk_lifetime struct fields follow virtio spec.
> +	 * There is no check/decode on values received from the device.
> +	 * The data is sent as is to the user.
> +	 */

In terms of security, any process with access to the block device node
is allowed to get the lifetime?

Usually only privileged processes have access to block device nodes, but
I wanted to check whether anyone can think of a scenario where this is
not okay.

For example, a virtio-blk device may have a partition that an untrusted
process like a database or key-value store accesses. Can the untrusted
process read the lifetime information of the entire device?

> +
> +	/* This ioctl is allowed only if VIRTIO_BLK_F_LIFETIME
> +	 * feature is negotiated.
> +	 */
> +	if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_LIFETIME))
> +		return -ENOTTY;
> +
> +	memset(&lifetime, 0, sizeof(lifetime));
> +
> +	req = blk_mq_alloc_request(q, REQ_OP_DRV_IN, 0);
> +	if (IS_ERR(req))
> +		return PTR_ERR(req);
> +
> +	/* Write the correct type */
> +	vbr = blk_mq_rq_to_pdu(req);
> +	vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_GET_LIFETIME);
> +
> +	ret = blk_rq_map_kern(q, req, &lifetime, sizeof(lifetime), GFP_KERNEL);
> +	if (ret)
> +		goto out;
> +
> +	blk_execute_rq(req, false);
> +
> +	ret = virtblk_ioctl_result(blk_mq_rq_to_pdu(req));
> +	if (ret)
> +		goto out;
> +
> +	/* Pass the data to the user */
> +	if (copy_to_user((void __user *)arg, &lifetime, sizeof(lifetime))) {
> +		ret = -EFAULT;
> +		goto out;
> +	}

It's unusual for an ioctl to produce a struct that's not in CPU
endianness. I think the kernel should deal with endianness here.

> +
> +out:
> +	blk_mq_free_request(req);
> +	return ret;
> +}
> +
> +static int virtblk_ioctl(struct block_device *bd, fmode_t mode,
> +			     unsigned int cmd, unsigned long arg)
> +{
> +	struct virtio_blk *vblk = bd->bd_disk->private_data;
> +	int ret;
> +
> +	mutex_lock(&vblk->vdev_mutex);

We need to check that vblk->vdev is non-NULL before accessing it in
virtblk_ioctl_lifetime():

  if (!vblk->vdev) {
      mutex_unlock(&vblk->dev_mutex);
      return -ENXIO;
  }

Without the check I expect virtblk_ioctl_lifetime() to dereference a
NULL pointer.

> +
> +	switch (cmd) {
> +	case VBLK_LIFETIME:
> +		ret = virtblk_ioctl_lifetime(vblk, arg);
> +		break;
> +	default:
> +		ret = -ENOTTY;
> +		break;
> +	}
> +
> +	mutex_unlock(&vblk->vdev_mutex);
> +
> +	return ret;
> +}
> +
>  static void virtblk_free_disk(struct gendisk *disk)
>  {
>  	struct virtio_blk *vblk = disk->private_data;
> @@ -520,6 +609,7 @@ static void virtblk_free_disk(struct gendisk *disk)
>  static const struct block_device_operations virtblk_fops = {
>  	.owner  	= THIS_MODULE,
>  	.getgeo		= virtblk_getgeo,
> +	.ioctl		= virtblk_ioctl,
>  	.free_disk	= virtblk_free_disk,
>  };
>  
> @@ -1239,7 +1329,7 @@ static unsigned int features_legacy[] = {
>  	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
>  	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
>  	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
> -	VIRTIO_BLK_F_SECURE_ERASE,
> +	VIRTIO_BLK_F_SECURE_ERASE, VIRTIO_BLK_F_LIFETIME,
>  }
>  ;
>  static unsigned int features[] = {
> @@ -1247,7 +1337,7 @@ static unsigned int features[] = {
>  	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
>  	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
>  	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
> -	VIRTIO_BLK_F_SECURE_ERASE,
> +	VIRTIO_BLK_F_SECURE_ERASE, VIRTIO_BLK_F_LIFETIME,
>  };
>  
>  static struct virtio_driver virtio_blk = {
> diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
> index 58e70b24b50..c769930d269 100644
> --- a/include/uapi/linux/virtio_blk.h
> +++ b/include/uapi/linux/virtio_blk.h
> @@ -40,6 +40,7 @@
>  #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
>  #define VIRTIO_BLK_F_DISCARD	13	/* DISCARD is supported */
>  #define VIRTIO_BLK_F_WRITE_ZEROES	14	/* WRITE ZEROES is supported */
> +#define VIRTIO_BLK_F_LIFETIME	15 /* Storage lifetime information is supported */
>  #define VIRTIO_BLK_F_SECURE_ERASE	16 /* Secure Erase is supported */
>  
>  /* Legacy feature bits */
> @@ -165,6 +166,9 @@ struct virtio_blk_config {
>  /* Get device ID command */
>  #define VIRTIO_BLK_T_GET_ID    8
>  
> +/* Get lifetime information command */
> +#define VIRTIO_BLK_T_GET_LIFETIME 10
> +
>  /* Discard command */
>  #define VIRTIO_BLK_T_DISCARD	11
>  
> @@ -206,6 +210,30 @@ struct virtio_blk_discard_write_zeroes {
>  	__le32 flags;
>  };
>  
> +/* Get lifetime information struct for each request */
> +struct virtio_blk_lifetime {
> +	/*
> +	 * specifies the percentage of reserved blocks that are consumed.
> +	 * optional values following virtio spec:
> +	 * 0 - undefined
> +	 * 1 - normal, < 80% of reserved blocks are consumed
> +	 * 2 - warning, 80% of reserved blocks are consumed
> +	 * 3 - urgent, 90% of reserved blocks are consumed
> +	 */
> +	__le16 pre_eol_info;
> +	/*
> +	 * this field refers to wear of SLC cells and is provided in increments of 10used,
> +	 * and so on, thru to 11 meaning estimated lifetime exceeded. All values above 11
> +	 * are reserved
> +	 */
> +	__le16 device_lifetime_est_typ_a;
> +	/*
> +	 * this field refers to wear of MLC cells and is provided with the same semantics as
> +	 * device_lifetime_est_typ_a
> +	 */
> +	__le16 device_lifetime_est_typ_b;
> +};
> +
>  #ifndef VIRTIO_BLK_NO_LEGACY
>  struct virtio_scsi_inhdr {
>  	__virtio32 errors;
> @@ -219,4 +247,8 @@ struct virtio_scsi_inhdr {
>  #define VIRTIO_BLK_S_OK		0
>  #define VIRTIO_BLK_S_IOERR	1
>  #define VIRTIO_BLK_S_UNSUPP	2
> +
> +/* Virtblk ioctl commands */
> +#define VBLK_LIFETIME	_IOR('r', 0, struct virtio_blk_lifetime)
> +
>  #endif /* _LINUX_VIRTIO_BLK_H */
> -- 
> 2.32.0
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 4542 bytes --]

On Thu, Nov 24, 2022 at 01:42:37AM -0500, Michael S. Tsirkin wrote:
> On Wed, Nov 23, 2022 at 10:41:46PM +0000, Chaitanya Kulkarni wrote:
> > 
> > > +/* Get lifetime information from device */
> > > +static int virtblk_ioctl_lifetime(struct virtio_blk *vblk, unsigned long arg)
> > > +{
> > > +	struct request_queue *q = vblk->disk->queue;
> > > +	struct request *req = NULL;
> > > +	struct virtblk_req *vbr;
> > > +	struct virtio_blk_lifetime lifetime;
> > > +	int ret;
> > > +
> > > +	/* The virtio_blk_lifetime struct fields follow virtio spec.
> > > +	 * There is no check/decode on values received from the device.
> > > +	 * The data is sent as is to the user.
> > > +	 */
> > 
> > Odd comment style, why not :-
> > 
> > 	/*
> > 	 * The virtio_blk_lifetime struct fields follow virtio spec.
> > 	 * There is no check/decode on values received from the device.
> > 	 * The data is sent as is to the user.
> > 	 */
> 
> most of virtio blk is like this. I don't really care much.
> 
> > > +
> > > +	/* This ioctl is allowed only if VIRTIO_BLK_F_LIFETIME
> > > +	 * feature is negotiated.
> > > +	 */
> > 
> > above comment is redundant to the below code as following code is
> > self explanatory.,..
> 
> i find it helpful personally - this point is important enough to
> stress at cost of some duplication.
> 
> > > +	if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_LIFETIME))
> > > +		return -ENOTTY;
> > > +
> > > +	memset(&lifetime, 0, sizeof(lifetime));
> > > +
> > > +	req = blk_mq_alloc_request(q, REQ_OP_DRV_IN, 0);
> > > +	if (IS_ERR(req))
> > > +		return PTR_ERR(req);
> > > +
> > > +	/* Write the correct type */
> > 
> > same here for above comment...
> 
> it's pretty harmless though
> 
> > > +	vbr = blk_mq_rq_to_pdu(req);
> > > +	vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_GET_LIFETIME);
> > > +
> > > +	ret = blk_rq_map_kern(q, req, &lifetime, sizeof(lifetime), GFP_KERNEL);
> > > +	if (ret)
> > > +		goto out;
> > > +
> > > +	blk_execute_rq(req, false);
> > > +
> > > +	ret = virtblk_ioctl_result(blk_mq_rq_to_pdu(req));
> > > +	if (ret)
> > > +		goto out;
> > > +
> > > +	/* Pass the data to the user */
> > > +	if (copy_to_user((void __user *)arg, &lifetime, sizeof(lifetime))) {
> > > +		ret = -EFAULT;
> > > +		goto out;
> > 
> > there nothing here to "goto out" following is sufficient I guess :-
> > 
> > if (copy_to_user((void __user *)arg, &lifetime, sizeof(lifetime)))
> > 	ret = -EFAULT;
> 
> error handling with goto seems cleaner, easier to add code down
> the road.
> 
> > > +	}
> > > +
> > > +out:
> > > +	blk_mq_free_request(req);
> > > +	return ret;
> > > +}
> > > +
> > 
> > [...]
> > 
> >    };
> > >   
> > > +/* Get lifetime information struct for each request */
> > > +struct virtio_blk_lifetime {
> > > +	/*
> > > +	 * specifies the percentage of reserved blocks that are consumed.
> > > +	 * optional values following virtio spec:
> > > +	 * 0 - undefined
> > > +	 * 1 - normal, < 80% of reserved blocks are consumed
> > > +	 * 2 - warning, 80% of reserved blocks are consumed
> > > +	 * 3 - urgent, 90% of reserved blocks are consumed
> > > +	 */
> > > +	__le16 pre_eol_info;
> > > +	/*
> > > +	 * this field refers to wear of SLC cells and is provided in increments of 10used,
> > > +	 * and so on, thru to 11 meaning estimated lifetime exceeded. All values above 11
> > > +	 * are reserved
> > > +	 */
> > > +	__le16 device_lifetime_est_typ_a;
> > > +	/*
> > > +	 * this field refers to wear of MLC cells and is provided with the same semantics as
> > > +	 * device_lifetime_est_typ_a
> > > +	 */
> > > +	__le16 device_lifetime_est_typ_b;
> > > +};
> > > +
> > 
> > are you sure there won't be any new members ever be added in future ?
> > to make it is futuresafe I'd add padding to this structure and keep
> > the reserve bytes to the meaningful size...
> > 
> > -ck
> 
> 
> virtio has feature bits for this kind of thing, we don't do padding.

The struct defined in the VIRTIO spec is being reused in the ioctl
userspace ABI. While feature bits are used by VIRTIO, padding is common
in the Linux UAPI.

Adding padding would require decoupling the VIRTIO spec struct from the
ioctl struct. That's probably a good idea because I noticed endianness
is left to userspace due to the __le fields in the VIRTIO spec struct
and that's likely to cause userspace bugs/confusion.

I suggest defining a separate UAPI struct for this ioctl.

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Alvaro Karsz]

Thanks Stefan

> How about naming it VBLK_GET_LIFETIME? It's clearer what the ioctl does
> and it follows the name of virtio-blk request type.

You're right, I'll rename it.

> ENOTTY already has meaning for ioctl(2):
>
>
>   ENOTTY fd is not associated with a character special device.
>
>
>   ENOTTY The specified request does not apply to the kind of object that the file descriptor fd references.
>
>
> Use ENOTSUP instead?

 ENOTSUP seems like a better fit, I'll change the error code.

> In terms of security, any process with access to the block device node
> is allowed to get the lifetime?
>
>
> Usually only privileged processes have access to block device nodes, but
> I wanted to check whether anyone can think of a scenario where this is
> not okay.
>
>
> For example, a virtio-blk device may have a partition that an untrusted
> process like a database or key-value store accesses. Can the untrusted
> process read the lifetime information of the entire device?


I agree that only a privileged process should be able to read the lifetime.
I could add something like:
if (!capable(CAP_SYS_ADMIN))
        return -EPERM;

> It's unusual for an ioctl to produce a struct that's not in CPU
> endianness. I think the kernel should deal with endianness here.

The endianness was discussed in the first version:

> > After more thought, I think that the driver should handle the
> > virtio_blk_lifetime struct endianness.
> > Something like:
> > ...
> > lifetime.pre_eol_info = __le16_to_cpu(lifetime.pre_eol_info);
> > lifetime. device_lifetime_est_typ_a  = __le16_to_cpu(lifetime.
> > device_lifetime_est_typ_a);
> > lifetime. device_lifetime_est_typ_b  = __le16_to_cpu(lifetime.
> > device_lifetime_est_typ_b);
> >
> > if (copy_to_user((void __user *)arg, (void *)&lifetime,
> > ...
> >
> > What do you think?
>
>
> I think if you are going to pass struct virtio_blk_lifetime to
> userspace, better pass it as defined in the spec, in LE format.


I tend to agree that endianness should be taken care of in the kernel.

> We need to check that vblk->vdev is non-NULL before accessing it in
> virtblk_ioctl_lifetime():
>
>
>   if (!vblk->vdev) {
>       mutex_unlock(&vblk->dev_mutex);
>       return -ENXIO;
>   }
>
>
> Without the check I expect virtblk_ioctl_lifetime() to dereference a
> NULL pointer.

Right

Alvaro
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Alvaro Karsz]

> I suggest defining a separate UAPI struct for this ioctl.

Sounds fine to me.
I could use the following struct

struct virtio_blk_lifetime_ioctl {
        u16 pre_eol_info;
        u16 device_lifetime_est_typ_a;
        u16 device_lifetime_est_typ_b;
        u16 padding;
};
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 398 bytes --]

On Fri, Nov 25, 2022 at 12:09:45AM +0200, Alvaro Karsz wrote:
> > I suggest defining a separate UAPI struct for this ioctl.
> 
> Sounds fine to me.
> I could use the following struct
> 
> struct virtio_blk_lifetime_ioctl {
>         u16 pre_eol_info;
>         u16 device_lifetime_est_typ_a;
>         u16 device_lifetime_est_typ_b;
>         u16 padding;
> };

Looks good.

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Michael S. Tsirkin]

On Fri, Nov 25, 2022 at 12:09:45AM +0200, Alvaro Karsz wrote:
> > I suggest defining a separate UAPI struct for this ioctl.
> 
> Sounds fine to me.
> I could use the following struct
> 
> struct virtio_blk_lifetime_ioctl {
>         u16 pre_eol_info;
>         u16 device_lifetime_est_typ_a;
>         u16 device_lifetime_est_typ_b;
>         u16 padding;
> };

OK. What makes it virtio specific though? Don't any other block
devices report lifetime? And shouldn't they share the same
ioctl then? Maybe this belongs to a generic lifetime.h ?
Comments documenting meaning of the fields would also be good.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Michael S. Tsirkin]

On Tue, Nov 29, 2022 at 04:28:54AM +0000, Chaitanya Kulkarni wrote:
> On 11/28/22 11:07, Stefan Hajnoczi wrote:
> > On Fri, Nov 25, 2022 at 12:09:45AM +0200, Alvaro Karsz wrote:
> >>> I suggest defining a separate UAPI struct for this ioctl.
> >>
> >> Sounds fine to me.
> >> I could use the following struct
> >>
> >> struct virtio_blk_lifetime_ioctl {
> >>          u16 pre_eol_info;
> >>          u16 device_lifetime_est_typ_a;
> >>          u16 device_lifetime_est_typ_b;
> >>          u16 padding;
> >> };
> > 
> > Looks good.
> > 
> > Stefan
> 
> with above definition how can one extend existing IOCTL to
> have new members in future ?
> 
> e.g. from above structure
> type_a -> SLC
> type_b -> MLC
> 
> so if someone wants to add type_c info for Quad-level cell(QLC)
> flash memory then type_d and so forth then how can this be
> achieved without adding a new IOCTL since ?
> 
> OR
> 
> just adding new members to user API ioctl struct will work
> fine ?
> 
> -ck
> 


You can do this since ioctl encodes the struct size.
But if that is envisioned then you don't really need padding.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Alvaro Karsz]

So, we could create a block-general lifetime ioctl with many reserved
bytes, or create a virtio block specific ioctl without reserved bytes
at all.
I think that we should keep it virtio specific, and if a new lifetime
command is added to the spec with more fields, we could create a new
ioctl.
Does Everyone agree?

> I think if you are going to pass struct virtio_blk_lifetime to
> userspace, better pass it as defined in the spec, in LE format.

> It's unusual for an ioctl to produce a struct that's not in CPU
> endianness. I think the kernel should deal with endianness here.

I'm not sure how to proceed with the endianness matter..

Alvaro
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Michael S. Tsirkin]

On Sun, Dec 04, 2022 at 10:19:38AM +0200, Alvaro Karsz wrote:
> So, we could create a block-general lifetime ioctl with many reserved
> bytes, or create a virtio block specific ioctl without reserved bytes
> at all.


I don't see the connection. virtio would often pass through lifetime
info from the host. If other devices gain more info then it will make 
sense to add that to virtio, too.

> I think that we should keep it virtio specific, and if a new lifetime
> command is added to the spec with more fields, we could create a new
> ioctl.
> Does Everyone agree?

Depends. If we expect more types, then I think we
can solve this by passing an array of values.

> > I think if you are going to pass struct virtio_blk_lifetime to
> > userspace, better pass it as defined in the spec, in LE format.
> 
> > It's unusual for an ioctl to produce a struct that's not in CPU
> > endianness. I think the kernel should deal with endianness here.
> 
> I'm not sure how to proceed with the endianness matter..
> 
> Alvaro

If it's a generic ioctl then clearly it's native.
For a virtio specific one, we typically use LE and I would be
consistent.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Alvaro Karsz]

> I don't see the connection. virtio would often pass through lifetime
> info from the host. If other devices gain more info then it will make
> sense to add that to virtio, too.

> Depends. If we expect more types, then I think we
> can solve this by passing an array of values.


Good idea!

We could pass something like virtio_blk_lifetime_ioctl struct to userspace:

enum blk_lifetime_type {
         VIRTIO_BLK_LIFETIME_PRE_EOL_TYPE_A_B = 1,
};

struct virtio_blk_lifetime_element {
        void *data;
        enum blk_lifetime_type;
};

struct virtio_blk_lifetime_ioctl {
        struct virtio_blk_lifetime_element elements[];
        u32 elements_num;
};

If just VIRTIO_BLK_F_LIFETIME is negotiated, the array size is 1, and
the element type is VIRTIO_BLK_LIFETIME_PRE_EOL_TYPE_A_B, so the user
will know that this is a virtio_blk_lifetime struct.
This seems general enough to handle future additions and to handle out
of order types (if for example VIRTIO_BLK_LIFETIME is not negotiated
and VIRTIO_BLK_LIFETIME_FUTURE is).

The only downside I can think of is if we add new fields to the struct
virtio_blk_lifetime struct, instead of creating a new, dedicated
struct in the spec.
For example:

struct virtio_blk_lifetime {
  le16 pre_eol_info;
  le16 device_lifetime_est_typ_a;
  le16 device_lifetime_est_typ_b;
  le16 device_lifetime_est_typ_c;  //only if
VIRTIO_BLK_LIFETIME_FUTURE is negotiated
};

In this case, we may need to split it into 2 different structs, and
pass it as 2 elements to userspace.

What do you think?
Should I go ahead and create a new version?


Alvaro
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Michael S. Tsirkin]

On Sun, Dec 04, 2022 at 02:00:25PM +0200, Alvaro Karsz wrote:
> > I don't see the connection. virtio would often pass through lifetime
> > info from the host. If other devices gain more info then it will make
> > sense to add that to virtio, too.
> 
> > Depends. If we expect more types, then I think we
> > can solve this by passing an array of values.
> 
> 
> Good idea!
> 
> We could pass something like virtio_blk_lifetime_ioctl struct to userspace:
> 
> enum blk_lifetime_type {
>          VIRTIO_BLK_LIFETIME_PRE_EOL_TYPE_A_B = 1,
> };
> 
> struct virtio_blk_lifetime_element {
>         void *data;
>         enum blk_lifetime_type;
> };
> 
> struct virtio_blk_lifetime_ioctl {
>         struct virtio_blk_lifetime_element elements[];
>         u32 elements_num;
> };
> 
> If just VIRTIO_BLK_F_LIFETIME is negotiated, the array size is 1, and
> the element type is VIRTIO_BLK_LIFETIME_PRE_EOL_TYPE_A_B, so the user
> will know that this is a virtio_blk_lifetime struct.
> This seems general enough to handle future additions and to handle out
> of order types (if for example VIRTIO_BLK_LIFETIME is not negotiated
> and VIRTIO_BLK_LIFETIME_FUTURE is).
> 
> The only downside I can think of is if we add new fields to the struct
> virtio_blk_lifetime struct, instead of creating a new, dedicated
> struct in the spec.
> For example:
> 
> struct virtio_blk_lifetime {
>   le16 pre_eol_info;
>   le16 device_lifetime_est_typ_a;
>   le16 device_lifetime_est_typ_b;
>   le16 device_lifetime_est_typ_c;  //only if
> VIRTIO_BLK_LIFETIME_FUTURE is negotiated
> };
> 
> In this case, we may need to split it into 2 different structs, and
> pass it as 2 elements to userspace.
> 
> What do you think?
> Should I go ahead and create a new version?
> 
> 
> Alvaro

And now is this generic enough to disconnect from virtio and
make it a generic blk thing?

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Alvaro Karsz]

> And now is this generic enough to disconnect from virtio and
> make it a generic blk thing?

It could be generic enough if we drop the virtio structs and pass
single fields as elements.
The point is, we can easily make it generic enough, do we want to?

At the moment, there is at least 1 existing device-specific ioctl to
retrieve lifetime info (that I'm aware of),
MMC_IOC_CMD for a MMC device with MMC_SEND_EXT_CSD opcode.
So we will have duplication for MMC devices (for some of the lifetime fields).

Do you want it to be blk generic?

Alvaro
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Michael S. Tsirkin]

On Sun, Dec 04, 2022 at 04:37:28PM +0200, Alvaro Karsz wrote:
> > And now is this generic enough to disconnect from virtio and
> > make it a generic blk thing?
> 
> It could be generic enough if we drop the virtio structs and pass
> single fields as elements.
> The point is, we can easily make it generic enough, do we want to?
> 
> At the moment, there is at least 1 existing device-specific ioctl to
> retrieve lifetime info (that I'm aware of),
> MMC_IOC_CMD for a MMC device with MMC_SEND_EXT_CSD opcode.
> So we will have duplication for MMC devices (for some of the lifetime fields).
> 
> Do you want it to be blk generic?
> 
> Alvaro

I don't really know enough about this. I think you should CC some storage
mailing lists and relevant maintainers, to understand whether it's
likely other drivers will reuse a generic IOCTL.

My point is this, virtio will need to be implemented on top of a
physical device. If that device has a lifetime ioctl then it's easy to
run it on host and forward the data without poking at device specific
detail. But, that requires agreement from at least some host
device maintainers. If that's not agreed upon, I'd say let's
just get the current version in, LE endian, no padding, just
reflect virtio command to userspace as is.


-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Michael S. Tsirkin]

On Mon, Dec 05, 2022 at 05:30:34AM +0000, Chaitanya Kulkarni wrote:
> On 12/4/22 00:19, Alvaro Karsz wrote:
> > So, we could create a block-general lifetime ioctl with many reserved
> > bytes, or create a virtio block specific ioctl without reserved bytes
> > at all.
> > I think that we should keep it virtio specific, and if a new lifetime
> > command is added to the spec with more fields, we could create a new
> > ioctl.
> > Does Everyone agree?
> > 
> 
> Unless there are multiple drivers supporting same IOCTL and agreeing
> the same semantics I've not seen new block layer IOCTL was accepted
> you never know ..
> 
> Personally I'd not, unless I find other major drivers and specs
> agreeing on semantics..
> 
> -ck
> 


OK looks like we are in agreement we can keep it as is then.
If new types are added we'll add a new ioctl not a big deal.
Please do Cc storage mailing lists on the next version though.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization