[PATCH v7 16/46] virtio_blk: v1.0 support

Based on patch by Cornelia Huck.

Note: for consistency, and to avoid sparse errors,
      convert all fields, even those no longer in use
      for virtio v1.0.

Signed-off-by: Cornelia Huck <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/uapi/linux/virtio_blk.h | 15 ++++-----
 drivers/block/virtio_blk.c      | 70 ++++++++++++++++++++++++-----------------
 2 files changed, 49 insertions(+), 36 deletions(-)

diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index 9ad67b2..247c8ba 100644
--- a/include/uapi/linux/virtio_blk.h
+++ b/include/uapi/linux/virtio_blk.h

@@ -28,6 +28,7 @@
 #include <linux/types.h>
 #include <linux/virtio_ids.h>
 #include <linux/virtio_config.h>
+#include <linux/virtio_types.h>
 
 /* Feature bits */
 #define VIRTIO_BLK_F_BARRIER	0	/* Does host support barriers? */

@@ -114,18 +115,18 @@ struct virtio_blk_config {
 /* This is the first element of the read scatter-gather list. */
 struct virtio_blk_outhdr {
 	/* VIRTIO_BLK_T* */
-	__u32 type;
+	__virtio32 type;
 	/* io priority. */
-	__u32 ioprio;
+	__virtio32 ioprio;
 	/* Sector (ie. 512 byte offset) */
-	__u64 sector;
+	__virtio64 sector;
 };
 
 struct virtio_scsi_inhdr {
-	__u32 errors;
-	__u32 data_len;
-	__u32 sense_len;
-	__u32 residual;
+	__virtio32 errors;
+	__virtio32 data_len;
+	__virtio32 sense_len;
+	__virtio32 residual;
 };
 
 /* And this is the final byte of the write scatter-gather list. */

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index c6a27d5..f601f16 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c

@@ -80,7 +80,7 @@ static int __virtblk_add_req(struct virtqueue *vq,
 {
 	struct scatterlist hdr, status, cmd, sense, inhdr, *sgs[6];
 	unsigned int num_out = 0, num_in = 0;
-	int type = vbr->out_hdr.type & ~VIRTIO_BLK_T_OUT;
+	__virtio32 type = vbr->out_hdr.type & ~cpu_to_virtio32(vq->vdev, VIRTIO_BLK_T_OUT);
 
 	sg_init_one(&hdr, &vbr->out_hdr, sizeof(vbr->out_hdr));
 	sgs[num_out++] = &hdr;

@@ -91,19 +91,19 @@ static int __virtblk_add_req(struct virtqueue *vq,
 	 * block, and before the normal inhdr we put the sense data and the
 	 * inhdr with additional status information.
 	 */
-	if (type == VIRTIO_BLK_T_SCSI_CMD) {
+	if (type == cpu_to_virtio32(vq->vdev, VIRTIO_BLK_T_SCSI_CMD)) {
 		sg_init_one(&cmd, vbr->req->cmd, vbr->req->cmd_len);
 		sgs[num_out++] = &cmd;
 	}
 
 	if (have_data) {
-		if (vbr->out_hdr.type & VIRTIO_BLK_T_OUT)
+		if (vbr->out_hdr.type & cpu_to_virtio32(vq->vdev, VIRTIO_BLK_T_OUT))
 			sgs[num_out++] = data_sg;
 		else
 			sgs[num_out + num_in++] = data_sg;
 	}
 
-	if (type == VIRTIO_BLK_T_SCSI_CMD) {
+	if (type == cpu_to_virtio32(vq->vdev, VIRTIO_BLK_T_SCSI_CMD)) {
 		sg_init_one(&sense, vbr->req->sense, SCSI_SENSE_BUFFERSIZE);
 		sgs[num_out + num_in++] = &sense;
 		sg_init_one(&inhdr, &vbr->in_hdr, sizeof(vbr->in_hdr));

@@ -119,12 +119,13 @@ static int __virtblk_add_req(struct virtqueue *vq,
 static inline void virtblk_request_done(struct request *req)
 {
 	struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
+	struct virtio_blk *vblk = req->q->queuedata;
 	int error = virtblk_result(vbr);
 
 	if (req->cmd_type == REQ_TYPE_BLOCK_PC) {
-		req->resid_len = vbr->in_hdr.residual;
-		req->sense_len = vbr->in_hdr.sense_len;
-		req->errors = vbr->in_hdr.errors;
+		req->resid_len = virtio32_to_cpu(vblk->vdev, vbr->in_hdr.residual);
+		req->sense_len = virtio32_to_cpu(vblk->vdev, vbr->in_hdr.sense_len);
+		req->errors = virtio32_to_cpu(vblk->vdev, vbr->in_hdr.errors);
 	} else if (req->cmd_type == REQ_TYPE_SPECIAL) {
 		req->errors = (error != 0);
 	}

@@ -173,25 +174,25 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req,
 
 	vbr->req = req;
 	if (req->cmd_flags & REQ_FLUSH) {
-		vbr->out_hdr.type = VIRTIO_BLK_T_FLUSH;
+		vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_FLUSH);
 		vbr->out_hdr.sector = 0;
-		vbr->out_hdr.ioprio = req_get_ioprio(vbr->req);
+		vbr->out_hdr.ioprio = cpu_to_virtio32(vblk->vdev, req_get_ioprio(vbr->req));
 	} else {
 		switch (req->cmd_type) {
 		case REQ_TYPE_FS:
 			vbr->out_hdr.type = 0;
-			vbr->out_hdr.sector = blk_rq_pos(vbr->req);
-			vbr->out_hdr.ioprio = req_get_ioprio(vbr->req);
+			vbr->out_hdr.sector = cpu_to_virtio64(vblk->vdev, blk_rq_pos(vbr->req));
+			vbr->out_hdr.ioprio = cpu_to_virtio32(vblk->vdev, req_get_ioprio(vbr->req));
 			break;
 		case REQ_TYPE_BLOCK_PC:
-			vbr->out_hdr.type = VIRTIO_BLK_T_SCSI_CMD;
+			vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_SCSI_CMD);
 			vbr->out_hdr.sector = 0;
-			vbr->out_hdr.ioprio = req_get_ioprio(vbr->req);
+			vbr->out_hdr.ioprio = cpu_to_virtio32(vblk->vdev, req_get_ioprio(vbr->req));
 			break;
 		case REQ_TYPE_SPECIAL:
-			vbr->out_hdr.type = VIRTIO_BLK_T_GET_ID;
+			vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_GET_ID);
 			vbr->out_hdr.sector = 0;
-			vbr->out_hdr.ioprio = req_get_ioprio(vbr->req);
+			vbr->out_hdr.ioprio = cpu_to_virtio32(vblk->vdev, req_get_ioprio(vbr->req));
 			break;
 		default:
 			/* We don't put anything else in the queue. */

@@ -204,9 +205,9 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req,
 	num = blk_rq_map_sg(hctx->queue, vbr->req, vbr->sg);
 	if (num) {
 		if (rq_data_dir(vbr->req) == WRITE)
-			vbr->out_hdr.type |= VIRTIO_BLK_T_OUT;
+			vbr->out_hdr.type |= cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_OUT);
 		else
-			vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
+			vbr->out_hdr.type |= cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_IN);
 	}
 
 	spin_lock_irqsave(&vblk->vqs[qid].lock, flags);

@@ -476,7 +477,8 @@ static int virtblk_get_cache_mode(struct virtio_device *vdev)
 				   struct virtio_blk_config, wce,
 				   &writeback);
 	if (err)
-		writeback = virtio_has_feature(vdev, VIRTIO_BLK_F_WCE);
+		writeback = virtio_has_feature(vdev, VIRTIO_BLK_F_WCE) ||
+		            virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
 
 	return writeback;
 }

@@ -821,25 +823,35 @@ static const struct virtio_device_id id_table[] = {
 	{ 0 },
 };
 
-static unsigned int features[] = {
+static unsigned int features_legacy[] = {
 	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
 	VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ,
+}
+;
+static unsigned int features[] = {
+	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
+	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
+	VIRTIO_BLK_F_TOPOLOGY,
+	VIRTIO_BLK_F_MQ,
+	VIRTIO_F_VERSION_1,
 };
 
 static struct virtio_driver virtio_blk = {
-	.feature_table		= features,
-	.feature_table_size	= ARRAY_SIZE(features),
-	.driver.name		= KBUILD_MODNAME,
-	.driver.owner		= THIS_MODULE,
-	.id_table		= id_table,
-	.probe			= virtblk_probe,
-	.remove			= virtblk_remove,
-	.config_changed		= virtblk_config_changed,
+	.feature_table			= features,
+	.feature_table_size		= ARRAY_SIZE(features),
+	.feature_table_legacy		= features_legacy,
+	.feature_table_size_legacy	= ARRAY_SIZE(features_legacy),
+	.driver.name			= KBUILD_MODNAME,
+	.driver.owner			= THIS_MODULE,
+	.id_table			= id_table,
+	.probe				= virtblk_probe,
+	.remove				= virtblk_remove,
+	.config_changed			= virtblk_config_changed,
 #ifdef CONFIG_PM_SLEEP
-	.freeze			= virtblk_freeze,
-	.restore		= virtblk_restore,
+	.freeze				= virtblk_freeze,
+	.restore			= virtblk_restore,
 #endif
 };
 

-- 
MST

Based on patch by Cornelia Huck.

Note: for consistency, and to avoid sparse errors,
      convert all fields, even those no longer in use
      for virtio v1.0.

Signed-off-by: Cornelia Huck <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

...

-static unsigned int features[] = {
+static unsigned int features_legacy[] = {
 	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
 	VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ,
+}
+;
+static unsigned int features[] = {
+	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
+	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
+	VIRTIO_BLK_F_TOPOLOGY,
+	VIRTIO_BLK_F_MQ,
+	VIRTIO_F_VERSION_1,

We can fit this into less lines, like done for features_legacy.

I was asking myself if we could do the conversion of the statical values
somehow upfront, to reduce the patch size and avoid cpu_to_virtio.* at those
places.

Otherwise looks good to me.

On Mon, Dec 01, 2014 at 09:16:41AM +0100, David Hildenbrand wrote:

quoted

Based on patch by Cornelia Huck.

Note: for consistency, and to avoid sparse errors,
      convert all fields, even those no longer in use
      for virtio v1.0.

Signed-off-by: Cornelia Huck <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

...

quoted

-static unsigned int features[] = {
+static unsigned int features_legacy[] = {
 	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
 	VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ,
+}
+;
+static unsigned int features[] = {
+	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
+	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
+	VIRTIO_BLK_F_TOPOLOGY,
+	VIRTIO_BLK_F_MQ,
+	VIRTIO_F_VERSION_1,

We can fit this into less lines, like done for features_legacy.

I was asking myself if we could do the conversion of the statical values
somehow upfront, to reduce the patch size and avoid cpu_to_virtio.* at those
places.

Otherwise looks good to me.

I don't see how we can reduce the patch size.
For BE architectures it's dynamic, so at best the values
will become macros/incline functions taking a flag.

For some places on data path, it might be worth it
to cache the correct value e.g. as part of device
structure. This replaces a branch with a memory load,
so the gain would have to be measured, best done
separately?

-- 
MST

On Mon, Dec 01, 2014 at 09:16:41AM +0100, David Hildenbrand wrote:

quoted

Based on patch by Cornelia Huck.

Note: for consistency, and to avoid sparse errors,
      convert all fields, even those no longer in use
      for virtio v1.0.

Signed-off-by: Cornelia Huck <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

...

quoted

-static unsigned int features[] = {
+static unsigned int features_legacy[] = {
 	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
 	VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ,
+}
+;
+static unsigned int features[] = {
+	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
+	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
+	VIRTIO_BLK_F_TOPOLOGY,
+	VIRTIO_BLK_F_MQ,
+	VIRTIO_F_VERSION_1,

We can fit this into less lines, like done for features_legacy.

Wrt packing code more tightly, I did it like this to
make it easier to compare the arrays.
Each flag is on the same line in original and new array.

I was asking myself if we could do the conversion of the statical values
somehow upfront, to reduce the patch size and avoid cpu_to_virtio.* at those
places.

Otherwise looks good to me.

On Mon, Dec 01, 2014 at 09:16:41AM +0100, David Hildenbrand wrote:

quoted

quoted

Based on patch by Cornelia Huck.

Note: for consistency, and to avoid sparse errors,
      convert all fields, even those no longer in use
      for virtio v1.0.

Signed-off-by: Cornelia Huck <redacted>
Signed-off-by: Michael S. Tsirkin <redacted>

...

quoted

-static unsigned int features[] = {
+static unsigned int features_legacy[] = {
 	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
 	VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ,
+}
+;
+static unsigned int features[] = {
+	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
+	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
+	VIRTIO_BLK_F_TOPOLOGY,
+	VIRTIO_BLK_F_MQ,
+	VIRTIO_F_VERSION_1,

We can fit this into less lines, like done for features_legacy.

Wrt packing code more tightly, I did it like this to
make it easier to compare the arrays.
Each flag is on the same line in original and new array.

This just looks inconsistent to me.

1. features_legacy is tightly packed
2. half of features is tightly packed

So either all tightly packed or put every item on a single line. At least
that's what I would do :)

quoted

I was asking myself if we could do the conversion of the statical values
somehow upfront, to reduce the patch size and avoid cpu_to_virtio.* at those
places.

Otherwise looks good to me.

On Mon, 1 Dec 2014 11:01:36 +0100
David Hildenbrand [off-list ref] wrote:

quoted

On Mon, Dec 01, 2014 at 09:16:41AM +0100, David Hildenbrand wrote:

quoted

quoted

Based on patch by Cornelia Huck.

Note: for consistency, and to avoid sparse errors,
      convert all fields, even those no longer in use
      for virtio v1.0.

Signed-off-by: Cornelia Huck <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

...

quoted

-static unsigned int features[] = {
+static unsigned int features_legacy[] = {
 	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
 	VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ,
+}
+;
+static unsigned int features[] = {
+	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
+	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
+	VIRTIO_BLK_F_TOPOLOGY,
+	VIRTIO_BLK_F_MQ,
+	VIRTIO_F_VERSION_1,

We can fit this into less lines, like done for features_legacy.

Wrt packing code more tightly, I did it like this to
make it easier to compare the arrays.
Each flag is on the same line in original and new array.

This just looks inconsistent to me.

1. features_legacy is tightly packed
2. half of features is tightly packed

So either all tightly packed or put every item on a single line. At least
that's what I would do :)

I agree with the reasoning that this makes it easy to compare legacy
vs. standard at a glance, so I vote for keeping it this way.

On Mon, 1 Dec 2014 11:26:58 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

For some places on data path, it might be worth it
to cache the correct value e.g. as part of device
structure. This replaces a branch with a memory load,
so the gain would have to be measured, best done
separately?

I think we'll want to do some measuring once the basic structure is
in place anyway. We should make sure that e.g. s390 only takes minor
hit due to all that swapping that is needed for standard-compliant
devices. Caching the value might certainly help in some paths.

On Mon, Dec 01, 2014 at 12:33:15PM +0100, Cornelia Huck wrote:

On Mon, 1 Dec 2014 11:26:58 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

For some places on data path, it might be worth it
to cache the correct value e.g. as part of device
structure. This replaces a branch with a memory load,
so the gain would have to be measured, best done
separately?

I think we'll want to do some measuring once the basic structure is
in place anyway.

What's meant by in place here?

We should make sure that e.g. s390 only takes minor
hit due to all that swapping that is needed for standard-compliant
devices. Caching the value might certainly help in some paths.

Well, this is queued in linux-next for 3.19, so
now's the time to do it :)

On Mon, 1 Dec 2014 13:46:45 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

On Mon, Dec 01, 2014 at 12:33:15PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 11:26:58 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

For some places on data path, it might be worth it
to cache the correct value e.g. as part of device
structure. This replaces a branch with a memory load,
so the gain would have to be measured, best done
separately?

I think we'll want to do some measuring once the basic structure is
in place anyway.

What's meant by in place here?

That this patchset is ready :)

quoted

We should make sure that e.g. s390 only takes minor
hit due to all that swapping that is needed for standard-compliant
devices. Caching the value might certainly help in some paths.

Well, this is queued in linux-next for 3.19, so
now's the time to do it :)

So much to do, so little time...

I'm still feeling a bit uncomfortable with some of the changes
(virtio-scsi etc.) as I have not been able to test them yet (as there's
no converted qemu for these yet). The virtio-net and virtio-blk changes
seem sane, though, and virtio-ccw should be fine as well.

OTOH, it's not like we're introducing new external interfaces, so later
rework should be fine.

On Mon, Dec 01, 2014 at 01:02:58PM +0100, Cornelia Huck wrote:

On Mon, 1 Dec 2014 13:46:45 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Mon, Dec 01, 2014 at 12:33:15PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 11:26:58 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

For some places on data path, it might be worth it
to cache the correct value e.g. as part of device
structure. This replaces a branch with a memory load,
so the gain would have to be measured, best done
separately?

I think we'll want to do some measuring once the basic structure is
in place anyway.

What's meant by in place here?

That this patchset is ready :)

quoted

quoted

We should make sure that e.g. s390 only takes minor
hit due to all that swapping that is needed for standard-compliant
devices. Caching the value might certainly help in some paths.

Well, this is queued in linux-next for 3.19, so
now's the time to do it :)

So much to do, so little time...

I'm still feeling a bit uncomfortable with some of the changes
(virtio-scsi etc.) as I have not been able to test them yet (as there's
no converted qemu for these yet). The virtio-net and virtio-blk changes
seem sane, though, and virtio-ccw should be fine as well.

OTOH, it's not like we're introducing new external interfaces, so later
rework should be fine.

Right. I'll send a revision with virtio console and the rest of devices
shortly.

On Mon, Dec 01, 2014 at 01:02:58PM +0100, Cornelia Huck wrote:

On Mon, 1 Dec 2014 13:46:45 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Mon, Dec 01, 2014 at 12:33:15PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 11:26:58 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

For some places on data path, it might be worth it
to cache the correct value e.g. as part of device
structure. This replaces a branch with a memory load,
so the gain would have to be measured, best done
separately?

I think we'll want to do some measuring once the basic structure is
in place anyway.

What's meant by in place here?

That this patchset is ready :)

Also it's ready to the level where benchmarking is possible, right?  I
don't think you should wait until we finish polishing up commit
messages.

quoted

quoted

We should make sure that e.g. s390 only takes minor
hit due to all that swapping that is needed for standard-compliant
devices. Caching the value might certainly help in some paths.

Well, this is queued in linux-next for 3.19, so
now's the time to do it :)

So much to do, so little time...

I'm still feeling a bit uncomfortable with some of the changes
(virtio-scsi etc.) as I have not been able to test them yet (as there's
no converted qemu for these yet). The virtio-net and virtio-blk changes
seem sane, though, and virtio-ccw should be fine as well.

OTOH, it's not like we're introducing new external interfaces, so later
rework should be fine.

On Mon, 1 Dec 2014 14:34:55 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

On Mon, Dec 01, 2014 at 01:02:58PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 13:46:45 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Mon, Dec 01, 2014 at 12:33:15PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 11:26:58 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

For some places on data path, it might be worth it
to cache the correct value e.g. as part of device
structure. This replaces a branch with a memory load,
so the gain would have to be measured, best done
separately?

I think we'll want to do some measuring once the basic structure is
in place anyway.

What's meant by in place here?

That this patchset is ready :)

Also it's ready to the level where benchmarking is possible, right?  I
don't think you should wait until we finish polishing up commit
messages.

My point is that I haven't even found time yet to test this
thouroughly :(

On Mon, Dec 01, 2014 at 01:40:36PM +0100, Cornelia Huck wrote:

On Mon, 1 Dec 2014 14:34:55 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Mon, Dec 01, 2014 at 01:02:58PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 13:46:45 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Mon, Dec 01, 2014 at 12:33:15PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 11:26:58 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

For some places on data path, it might be worth it
to cache the correct value e.g. as part of device
structure. This replaces a branch with a memory load,
so the gain would have to be measured, best done
separately?

I think we'll want to do some measuring once the basic structure is
in place anyway.

What's meant by in place here?

That this patchset is ready :)

Also it's ready to the level where benchmarking is possible, right?  I
don't think you should wait until we finish polishing up commit
messages.

My point is that I haven't even found time yet to test this
thouroughly :(

If my experience shows anything, it's unlikely we'll get appropriate
testing without code being upstream first.
That's why I pushed on with sparse tagging btw.
This way we can be reasonably sure we didn't miss some path.

-- 
MST

On Mon, 1 Dec 2014 14:51:26 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

On Mon, Dec 01, 2014 at 01:40:36PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 14:34:55 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Mon, Dec 01, 2014 at 01:02:58PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 13:46:45 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Mon, Dec 01, 2014 at 12:33:15PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 11:26:58 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

For some places on data path, it might be worth it
to cache the correct value e.g. as part of device
structure. This replaces a branch with a memory load,
so the gain would have to be measured, best done
separately?

I think we'll want to do some measuring once the basic structure is
in place anyway.

What's meant by in place here?

That this patchset is ready :)

Also it's ready to the level where benchmarking is possible, right?  I
don't think you should wait until we finish polishing up commit
messages.

My point is that I haven't even found time yet to test this
thouroughly :(

If my experience shows anything, it's unlikely we'll get appropriate
testing without code being upstream first.
That's why I pushed on with sparse tagging btw.
This way we can be reasonably sure we didn't miss some path.

I know that I'm likely the only one to test ccw (unless I manage to get
some other also-busy people to try this out).

What's the status of virtio-pci, btw? Can people actually test this
sanely?

On Mon, Dec 01, 2014 at 02:00:04PM +0100, Cornelia Huck wrote:

On Mon, 1 Dec 2014 14:51:26 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Mon, Dec 01, 2014 at 01:40:36PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 14:34:55 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Mon, Dec 01, 2014 at 01:02:58PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 13:46:45 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Mon, Dec 01, 2014 at 12:33:15PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 11:26:58 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

For some places on data path, it might be worth it
to cache the correct value e.g. as part of device
structure. This replaces a branch with a memory load,
so the gain would have to be measured, best done
separately?

I think we'll want to do some measuring once the basic structure is
in place anyway.

What's meant by in place here?

That this patchset is ready :)

Also it's ready to the level where benchmarking is possible, right?  I
don't think you should wait until we finish polishing up commit
messages.

My point is that I haven't even found time yet to test this
thouroughly :(

If my experience shows anything, it's unlikely we'll get appropriate
testing without code being upstream first.
That's why I pushed on with sparse tagging btw.
This way we can be reasonably sure we didn't miss some path.

I know that I'm likely the only one to test ccw (unless I manage to get
some other also-busy people to try this out).

What's the status of virtio-pci, btw? Can people actually test this
sanely?

Sure, I'm testing that it's not broken by these patches.
Others can do so, too.

Once ccw is done on host and guest (will be complete after I
send v8), it will be easier to add virtio 1.0 for more transports.

OTOH if we require that everything is ready and perfect before merging
anything we'll never get anywhere.

-- 
MST

On Mon, 1 Dec 2014 15:47:19 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

On Mon, Dec 01, 2014 at 02:00:04PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 14:51:26 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Mon, Dec 01, 2014 at 01:40:36PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 14:34:55 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Mon, Dec 01, 2014 at 01:02:58PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 13:46:45 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Mon, Dec 01, 2014 at 12:33:15PM +0100, Cornelia Huck wrote:

quoted

On Mon, 1 Dec 2014 11:26:58 +0200
"Michael S. Tsirkin" [off-list ref] wrote:

quoted

For some places on data path, it might be worth it
to cache the correct value e.g. as part of device
structure. This replaces a branch with a memory load,
so the gain would have to be measured, best done
separately?

I think we'll want to do some measuring once the basic structure is
in place anyway.

What's meant by in place here?

That this patchset is ready :)

Also it's ready to the level where benchmarking is possible, right?  I
don't think you should wait until we finish polishing up commit
messages.

My point is that I haven't even found time yet to test this
thouroughly :(

If my experience shows anything, it's unlikely we'll get appropriate
testing without code being upstream first.
That's why I pushed on with sparse tagging btw.
This way we can be reasonably sure we didn't miss some path.

I know that I'm likely the only one to test ccw (unless I manage to get
some other also-busy people to try this out).

What's the status of virtio-pci, btw? Can people actually test this
sanely?

Sure, I'm testing that it's not broken by these patches.
Others can do so, too.

So basically just regression testing, right?

Once ccw is done on host and guest (will be complete after I
send v8), it will be easier to add virtio 1.0 for more transports.

OTOH if we require that everything is ready and perfect before merging
anything we'll never get anywhere.

I'm not looking for perfect, I'm just trying to juggle testing this +
doing qemu changes + various other stuff that is eating my time :)