Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] virtio_blk: Add support for lifetime feature
@ 2021-04-16 19:47 Enrico Granata
  2021-04-20  7:01 ` Christoph Hellwig
  0 siblings, 1 reply; 5+ messages in thread
From: Enrico Granata @ 2021-04-16 19:47 UTC (permalink / raw)
  To: mst, jasowang, pbonzini, stefanha, axboe, virtualization,
	linux-block, linux-kernel
  Cc: egranata

The VirtIO TC has adopted a new feature in virtio-blk enabling
discovery of lifetime information.

This commit adds support for the VIRTIO_BLK_T_LIFETIME command
to the virtio_blk driver, and adds two new attributes to the
sysfs entry for virtio_blk:
* pre_eol_info
* life_time

which are defined in the same manner as the files of the same name
for the eMMC driver, in line with the VirtIO specification.

Signed-off-by: Enrico Granata <egranata@google.com>
---
Changes in v2:
  - Removed redudnant buffer size checks
  - Renamed variables for consistency
  - Cleaned up endianness

 drivers/block/virtio_blk.c      | 75 +++++++++++++++++++++++++++++++--
 include/uapi/linux/virtio_blk.h | 11 +++++
 2 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index b9fa3ef5b57c..31969f680a9f 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -246,14 +246,15 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
 		unmap = !(req->cmd_flags & REQ_NOUNMAP);
 		break;
 	case REQ_OP_DRV_IN:
-		type = VIRTIO_BLK_T_GET_ID;
-		break;
+		break; /* type already set for custom requests */
 	default:
 		WARN_ON_ONCE(1);
 		return BLK_STS_IOERR;
 	}
 
-	vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, type);
+	if (req_op(req) != REQ_OP_DRV_IN)
+		vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, type);
+
 	vbr->out_hdr.sector = type ?
 		0 : cpu_to_virtio64(vblk->vdev, blk_rq_pos(req));
 	vbr->out_hdr.ioprio = cpu_to_virtio32(vblk->vdev, req_get_ioprio(req));
@@ -310,11 +311,14 @@ 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_get_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)
@@ -327,6 +331,34 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
 	return err;
 }
 
+static int virtblk_get_lifetime(struct gendisk *disk, struct virtio_blk_lifetime *lifetime)
+{
+	struct virtio_blk *vblk = disk->private_data;
+	struct request_queue *q = vblk->disk->queue;
+	struct request *req;
+	struct virtblk_req *vbr;
+	int err;
+
+	if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_LIFETIME))
+		return -EOPNOTSUPP;
+
+	req = blk_get_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_LIFETIME);
+
+	err = blk_rq_map_kern(q, req, lifetime, sizeof(*lifetime), GFP_KERNEL);
+	if (err)
+		goto out;
+
+	blk_execute_rq(vblk->disk, req, false);
+	err = blk_status_to_errno(virtblk_result(blk_mq_rq_to_pdu(req)));
+out:
+	blk_put_request(req);
+	return err;
+}
+
 static void virtblk_get(struct virtio_blk *vblk)
 {
 	refcount_inc(&vblk->refs);
@@ -435,6 +467,40 @@ static ssize_t serial_show(struct device *dev,
 
 static DEVICE_ATTR_RO(serial);
 
+static ssize_t pre_eol_info_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct gendisk *disk = dev_to_disk(dev);
+	struct virtio_blk_lifetime lft;
+	int err;
+
+	err = virtblk_get_lifetime(disk, &lft);
+	if (err)
+		return 0;
+
+	return sprintf(buf, "0x%02x\n", le16_to_cpu(lft.pre_eol_info));
+}
+
+static DEVICE_ATTR_RO(pre_eol_info);
+
+static ssize_t life_time_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct gendisk *disk = dev_to_disk(dev);
+	struct virtio_blk_lifetime lft;
+	int err;
+
+	err = virtblk_get_lifetime(disk, &lft);
+	if (err)
+		return 0;
+
+	return sprintf(buf, "0x%02x 0x%02x\n",
+			le16_to_cpu(lft.device_life_time_est_typ_a),
+			le16_to_cpu(lft.device_life_time_est_typ_b));
+}
+
+static DEVICE_ATTR_RO(life_time);
+
 /* The queue's logical block size must be set before calling this */
 static void virtblk_update_capacity(struct virtio_blk *vblk, bool resize)
 {
@@ -638,6 +704,8 @@ static DEVICE_ATTR_RW(cache_type);
 
 static struct attribute *virtblk_attrs[] = {
 	&dev_attr_serial.attr,
+	&dev_attr_pre_eol_info.attr,
+	&dev_attr_life_time.attr,
 	&dev_attr_cache_type.attr,
 	NULL,
 };
@@ -984,6 +1052,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_LIFETIME,
 };
 
 static struct virtio_driver virtio_blk = {
diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index d888f013d9ff..bbd3978b9d08 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 /* LIFETIME is supported */
 
 /* Legacy feature bits */
 #ifndef VIRTIO_BLK_NO_LEGACY
@@ -149,6 +150,9 @@ struct virtio_blk_config {
 /* Get device ID command */
 #define VIRTIO_BLK_T_GET_ID    8
 
+/* Get device lifetime command */
+#define VIRTIO_BLK_T_GET_LIFETIME 10
+
 /* Discard command */
 #define VIRTIO_BLK_T_DISCARD	11
 
@@ -196,6 +200,13 @@ struct virtio_scsi_inhdr {
 };
 #endif /* !VIRTIO_BLK_NO_LEGACY */
 
+/* Lifetime information for virtio_blk device */
+struct virtio_blk_lifetime {
+	__le16 pre_eol_info;
+	__le16 device_life_time_est_typ_a;
+	__le16 device_life_time_est_typ_b;
+};
+
 /* And this is the final byte of the write scatter-gather list. */
 #define VIRTIO_BLK_S_OK		0
 #define VIRTIO_BLK_S_IOERR	1
-- 
2.31.1.368.gbe11c130af-goog


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] virtio_blk: Add support for lifetime feature
  2021-04-16 19:47 [PATCH v2] virtio_blk: Add support for lifetime feature Enrico Granata
@ 2021-04-20  7:01 ` Christoph Hellwig
  2021-04-20 10:08   ` Michael S. Tsirkin
  0 siblings, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2021-04-20  7:01 UTC (permalink / raw)
  To: Enrico Granata
  Cc: mst, jasowang, pbonzini, stefanha, axboe, virtualization,
	linux-block, linux-kernel

Just to despit my 2 cents again:  I think the way this is specified
in the virtio spec is actively harmful and we should not suport it in
Linux.

If others override me we at least need to require a detailed
documentation of these fields as the virto spec does not provide it.

Please also do not add pointless over 80 character lines, and follow
the one value per sysfs file rule.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] virtio_blk: Add support for lifetime feature
  2021-04-20  7:01 ` Christoph Hellwig
@ 2021-04-20 10:08   ` Michael S. Tsirkin
  2021-04-20 16:28     ` Enrico Granata
  2021-04-20 16:41     ` Cornelia Huck
  0 siblings, 2 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2021-04-20 10:08 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Enrico Granata, jasowang, pbonzini, stefanha, axboe,
	virtualization, linux-block, linux-kernel

On Tue, Apr 20, 2021 at 08:01:29AM +0100, Christoph Hellwig wrote:
> Just to despit my 2 cents again:  I think the way this is specified
> in the virtio spec is actively harmful and we should not suport it in
> Linux.
> 
> If others override me we at least need to require a detailed
> documentation of these fields as the virto spec does not provide it.
> 
> Please also do not add pointless over 80 character lines, and follow
> the one value per sysfs file rule.

Enrico would you like to raise the issues with the virtio TC
for resolution?

-- 
MST


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] virtio_blk: Add support for lifetime feature
  2021-04-20 10:08   ` Michael S. Tsirkin
@ 2021-04-20 16:28     ` Enrico Granata
  2021-04-20 16:41     ` Cornelia Huck
  1 sibling, 0 replies; 5+ messages in thread
From: Enrico Granata @ 2021-04-20 16:28 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Christoph Hellwig, jasowang, pbonzini, Stefan Hajnoczi, axboe,
	virtualization, linux-block, Linux Kernel Mailing List

I prepared a proposed patch to address these concerns:
https://lists.oasis-open.org/archives/virtio-dev/202104/msg00007.html

Feedback will be much appreciated

Thanks,
- Enrico

On Tue, Apr 20, 2021 at 4:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Apr 20, 2021 at 08:01:29AM +0100, Christoph Hellwig wrote:
> > Just to despit my 2 cents again:  I think the way this is specified
> > in the virtio spec is actively harmful and we should not suport it in
> > Linux.
> >
> > If others override me we at least need to require a detailed
> > documentation of these fields as the virto spec does not provide it.
> >
> > Please also do not add pointless over 80 character lines, and follow
> > the one value per sysfs file rule.
>
> Enrico would you like to raise the issues with the virtio TC
> for resolution?
>
> --
> MST
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] virtio_blk: Add support for lifetime feature
  2021-04-20 10:08   ` Michael S. Tsirkin
  2021-04-20 16:28     ` Enrico Granata
@ 2021-04-20 16:41     ` Cornelia Huck
  1 sibling, 0 replies; 5+ messages in thread
From: Cornelia Huck @ 2021-04-20 16:41 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Christoph Hellwig, axboe, linux-kernel, virtualization,
	linux-block, stefanha, pbonzini, Enrico Granata

On Tue, 20 Apr 2021 06:08:29 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Apr 20, 2021 at 08:01:29AM +0100, Christoph Hellwig wrote:
> > Just to despit my 2 cents again:  I think the way this is specified
> > in the virtio spec is actively harmful and we should not suport it in
> > Linux.
> > 
> > If others override me we at least need to require a detailed
> > documentation of these fields as the virto spec does not provide it.
> > 
> > Please also do not add pointless over 80 character lines, and follow
> > the one value per sysfs file rule.  
> 
> Enrico would you like to raise the issues with the virtio TC
> for resolution?
> 

FWIW, I've opened https://github.com/oasis-tcs/virtio-spec/issues/106
to track this.


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-16 19:47 [PATCH v2] virtio_blk: Add support for lifetime feature Enrico Granata
2021-04-20  7:01 ` Christoph Hellwig
2021-04-20 10:08   ` Michael S. Tsirkin
2021-04-20 16:28     ` Enrico Granata
2021-04-20 16:41     ` Cornelia Huck

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git