Linux Virtualization - Jan 2017 - [PULL 3/5] virtio/s390: support READ_STATUS command for virtio-ccw

Michael,

the following patches have all been posted in the past. I've
collected them on top of your vhost branch -- please let me
know whether this works for you.

The following changes since commit 6bdf1e0efb04a1716373646cb6f35b73addca492:

  Makefile: drop -D__CHECK_ENDIAN__ from cflags (2016-12-16 00:13:43 +0200)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git
tags/virtio-s390-20170116

for you to fetch changes up to 364cf19a474501cf157f1d09895ebab71e603c77:

  virtio/s390: virtio: constify virtio_config_ops structures (2017-01-16
12:26:07 +0100)

----------------------------------------------------------------
- ringtest fixes to make it work on s390
- support new READ_STATUS command
- minor cleanups

----------------------------------------------------------------

Bhumika Goyal (1):
  virtio/s390: virtio: constify virtio_config_ops structures

Colin Ian King (1):
  virtio/s390: add missing \n to end of dev_err message

Halil Pasic (2):
  tools/virtio/ringtest: fix run-on-all.sh for offline cpus
  tools/virtio/ringtest: tweaks for s390

Pierre Morel (1):
  virtio/s390: support READ_STATUS command for virtio-ccw

 drivers/s390/virtio/virtio_ccw.c    | 29 +++++++++++++++++++++++++++--
 tools/virtio/ringtest/main.h        | 12 ++++++++++++
 tools/virtio/ringtest/run-on-all.sh |  5 +++--
 3 files changed, 42 insertions(+), 4 deletions(-)

-- 
2.11.0

From: Halil Pasic <pasic at linux.vnet.ibm.com>

Since ef1b144d ("tools/virtio/ringtest: fix run-on-all.sh to work
without /dev/cpu") run-on-all.sh uses seq 0 $HOST_AFFINITY as the list
of ids of the CPUs to run the command on (assuming ids of online CPUs
are consecutive and start from 0), where $HOST_AFFINITY is the highest
CPU id in the system previously determined using lscpu.  This can fail
on systems with offline CPUs.

Instead let's use lscpu to determine the list of online CPUs.

Signed-off-by: Halil Pasic <pasic at linux.vnet.ibm.com>
Fixes: ef1b144d ("tools/virtio/ringtest: fix run-on-all.sh to work without
/dev/cpu")
Reviewed-by: Sascha Silbe <silbe at linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
---
 tools/virtio/ringtest/run-on-all.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/virtio/ringtest/run-on-all.sh
b/tools/virtio/ringtest/run-on-all.sh
index 2e69ca812b4c..29b0d3920bfc 100755
--- a/tools/virtio/ringtest/run-on-all.sh
+++ b/tools/virtio/ringtest/run-on-all.sh
@@ -1,12 +1,13 @@
 #!/bin/sh
 
+CPUS_ONLINE=$(lscpu --online -p=cpu|grep -v -e '#')
 #use last CPU for host. Why not the first?
 #many devices tend to use cpu0 by default so
 #it tends to be busier
-HOST_AFFINITY=$(lscpu -p=cpu | tail -1)
+HOST_AFFINITY=$(echo "${CPUS_ONLINE}"|tail -n 1)
 
 #run command on all cpus
-for cpu in $(seq 0 $HOST_AFFINITY)
+for cpu in $CPUS_ONLINE
 do
 	#Don't run guest and host on same CPU
 	#It actually works ok if using signalling
-- 
2.11.0

From: Halil Pasic <pasic at linux.vnet.ibm.com>

Make ringtest work on s390 too.

Signed-off-by: Halil Pasic <pasic at linux.vnet.ibm.com>
Acked-by: Sascha Silbe <silbe at linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
---
 tools/virtio/ringtest/main.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/virtio/ringtest/main.h b/tools/virtio/ringtest/main.h
index 34e63cc4c572..14142faf040b 100644
--- a/tools/virtio/ringtest/main.h
+++ b/tools/virtio/ringtest/main.h
@@ -26,6 +26,16 @@ static inline void wait_cycles(unsigned long long cycles)
 #define VMEXIT_CYCLES 500
 #define VMENTRY_CYCLES 500
 
+#elif defined(__s390x__)
+static inline void wait_cycles(unsigned long long cycles)
+{
+	asm volatile("0: brctg %0,0b" : : "d" (cycles));
+}
+
+/* tweak me */
+#define VMEXIT_CYCLES 200
+#define VMENTRY_CYCLES 200
+
 #else
 static inline void wait_cycles(unsigned long long cycles)
 {
@@ -81,6 +91,8 @@ extern unsigned ring_size;
 /* Is there a portable way to do this? */
 #if defined(__x86_64__) || defined(__i386__)
 #define cpu_relax() asm ("rep; nop" ::: "memory")
+#elif defined(__s390x__)
+#define cpu_relax() barrier()
 #else
 #define cpu_relax() assert(0)
 #endif
-- 
2.11.0

From: Pierre Morel <pmorel at linux.vnet.ibm.com>

As virtio-1 introduced the possibility of the device manipulating the
status byte, revision 2 of the virtio-ccw transport introduced a means
of getting the status byte from the device via READ_STATUS. Let's wire
it up for revisions >= 2 and fall back to returning the stored status
byte if not supported.

Signed-off-by: Pierre Morel <pmorel at linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
---
 drivers/s390/virtio/virtio_ccw.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index 639ed4e6afd1..01e3dcfd7c64 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -145,6 +145,7 @@ static struct airq_info *airq_areas[MAX_AIRQ_AREAS];
 #define CCW_CMD_WRITE_CONF 0x21
 #define CCW_CMD_WRITE_STATUS 0x31
 #define CCW_CMD_READ_VQ_CONF 0x32
+#define CCW_CMD_READ_STATUS 0x72
 #define CCW_CMD_SET_IND_ADAPTER 0x73
 #define CCW_CMD_SET_VIRTIO_REV 0x83
 
@@ -160,6 +161,7 @@ static struct airq_info *airq_areas[MAX_AIRQ_AREAS];
 #define VIRTIO_CCW_DOING_SET_CONF_IND 0x04000000
 #define VIRTIO_CCW_DOING_SET_IND_ADAPTER 0x08000000
 #define VIRTIO_CCW_DOING_SET_VIRTIO_REV 0x10000000
+#define VIRTIO_CCW_DOING_READ_STATUS 0x20000000
 #define VIRTIO_CCW_INTPARM_MASK 0xffff0000
 
 static struct virtio_ccw_device *to_vc_device(struct virtio_device *vdev)
@@ -892,6 +894,28 @@ static void virtio_ccw_set_config(struct virtio_device
*vdev,
 static u8 virtio_ccw_get_status(struct virtio_device *vdev)
 {
 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
+	u8 old_status = *vcdev->status;
+	struct ccw1 *ccw;
+
+	if (vcdev->revision < 1)
+		return *vcdev->status;
+
+	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
+	if (!ccw)
+		return old_status;
+
+	ccw->cmd_code = CCW_CMD_READ_STATUS;
+	ccw->flags = 0;
+	ccw->count = sizeof(*vcdev->status);
+	ccw->cda = (__u32)(unsigned long)vcdev->status;
+	ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_READ_STATUS);
+/*
+ * If the channel program failed (should only happen if the device
+ * was hotunplugged, and then we clean up via the machine check
+ * handler anyway), vcdev->status was not overwritten and we just
+ * return the old status, which is fine.
+*/
+	kfree(ccw);
 
 	return *vcdev->status;
 }
@@ -987,6 +1011,7 @@ static void virtio_ccw_check_activity(struct
virtio_ccw_device *vcdev,
 		case VIRTIO_CCW_DOING_READ_CONFIG:
 		case VIRTIO_CCW_DOING_WRITE_CONFIG:
 		case VIRTIO_CCW_DOING_WRITE_STATUS:
+		case VIRTIO_CCW_DOING_READ_STATUS:
 		case VIRTIO_CCW_DOING_SET_VQ:
 		case VIRTIO_CCW_DOING_SET_IND:
 		case VIRTIO_CCW_DOING_SET_CONF_IND:
-- 
2.11.0

From: Colin Ian King <colin.king at canonical.com>

Trival fix, dev_err message is missing a \n, so add it.

Signed-off-by: Colin Ian King <colin.king at canonical.com>
Message-Id: <20160927200844.16008-1-colin.king at canonical.com>
Signed-off-by: Christian Borntraeger <borntraeger at de.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
---
 drivers/s390/virtio/virtio_ccw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index 01e3dcfd7c64..0672c6234ae8 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -454,7 +454,7 @@ static void virtio_ccw_del_vq(struct virtqueue *vq, struct
ccw1 *ccw)
 	 * This may happen on device detach.
 	 */
 	if (ret && (ret != -ENODEV))
-		dev_warn(&vq->vdev->dev, "Error %d while deleting queue
%d",
+		dev_warn(&vq->vdev->dev, "Error %d while deleting queue
%d\n",
 			 ret, index);
 
 	vring_del_virtqueue(vq);
-- 
2.11.0

From: Bhumika Goyal <bhumirks at gmail.com>

Declare virtio_config_ops structure as const as it is only stored in the
config field of a virtio_device structure. This field is of type const, so
virtio_config_ops structures having this property can be declared const.
Done using Coccinelle:

@r1 disable optional_qualifier@
identifier i;
position p;
@@
static struct virtio_config_ops i at p={...};

@ok1@
identifier r1.i;
position p;
struct virtio_ccw_device x;
@@
x.vdev.config=&i at p

@bad@
position p!={r1.p,ok1.p};
identifier r1.i;
@@
i at p

@depends on !bad disable optional_qualifier@
identifier r1.i;
@@
+const
struct virtio_config_ops i;

File size before and after applying the patch remains the same.
text	data	bss	dec	hex   filename
9235	296	32928	42459	a5db  drivers/s390/virtio/virtio_ccw.o

Signed-off-by: Bhumika Goyal <bhumirks at gmail.com>
Message-Id: <1484333336-13443-1-git-send-email-bhumirks at gmail.com>
Signed-off-by: Christian Borntraeger <borntraeger at de.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
---
 drivers/s390/virtio/virtio_ccw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index 0672c6234ae8..070c4da95f48 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -944,7 +944,7 @@ static void virtio_ccw_set_status(struct virtio_device
*vdev, u8 status)
 	kfree(ccw);
 }
 
-static struct virtio_config_ops virtio_ccw_config_ops = {
+static const struct virtio_config_ops virtio_ccw_config_ops = {
 	.get_features = virtio_ccw_get_features,
 	.finalize_features = virtio_ccw_finalize_features,
 	.get = virtio_ccw_get_config,
-- 
2.11.0

On Mon, Jan 16, 2017 at 03:04:28PM +0100, Cornelia Huck
wrote:
> Michael,
> 
> the following patches have all been posted in the past. I've
> collected them on top of your vhost branch -- please let me
> know whether this works for you.
Sure, thanks!
> The following changes since commit
6bdf1e0efb04a1716373646cb6f35b73addca492:
> 
>   Makefile: drop -D__CHECK_ENDIAN__ from cflags (2016-12-16 00:13:43 +0200)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git
tags/virtio-s390-20170116
> 
> for you to fetch changes up to 364cf19a474501cf157f1d09895ebab71e603c77:
> 
>   virtio/s390: virtio: constify virtio_config_ops structures (2017-01-16
12:26:07 +0100)
> 
> ----------------------------------------------------------------
> - ringtest fixes to make it work on s390
> - support new READ_STATUS command
> - minor cleanups
> 
> ----------------------------------------------------------------
> 
> Bhumika Goyal (1):
>   virtio/s390: virtio: constify virtio_config_ops structures
> 
> Colin Ian King (1):
>   virtio/s390: add missing \n to end of dev_err message
> 
> Halil Pasic (2):
>   tools/virtio/ringtest: fix run-on-all.sh for offline cpus
>   tools/virtio/ringtest: tweaks for s390
> 
> Pierre Morel (1):
>   virtio/s390: support READ_STATUS command for virtio-ccw
> 
>  drivers/s390/virtio/virtio_ccw.c    | 29 +++++++++++++++++++++++++++--
>  tools/virtio/ringtest/main.h        | 12 ++++++++++++
>  tools/virtio/ringtest/run-on-all.sh |  5 +++--
>  3 files changed, 42 insertions(+), 4 deletions(-)
> 
> -- 
> 2.11.0

On Mon, Jan 16, 2017 at 03:04:33PM +0100, Cornelia Huck
wrote:
> From: Bhumika Goyal <bhumirks at gmail.com>
> 
> Declare virtio_config_ops structure as const as it is only stored in the
> config field of a virtio_device structure. This field is of type const, so
> virtio_config_ops structures having this property can be declared const.
> Done using Coccinelle:
> 
> @r1 disable optional_qualifier@
> identifier i;
> position p;
> @@
> static struct virtio_config_ops i at p={...};
> 
> @ok1@
> identifier r1.i;
> position p;
> struct virtio_ccw_device x;
> @@
> x.vdev.config=&i at p
> 
> @bad@
> position p!={r1.p,ok1.p};
> identifier r1.i;
> @@
> i at p
> 
> @depends on !bad disable optional_qualifier@
> identifier r1.i;
> @@
> +const
> struct virtio_config_ops i;
> 
> File size before and after applying the patch remains the same.
> text	data	bss	dec	hex   filename
> 9235	296	32928	42459	a5db  drivers/s390/virtio/virtio_ccw.o
> 
> Signed-off-by: Bhumika Goyal <bhumirks at gmail.com>
> Message-Id: <1484333336-13443-1-git-send-email-bhumirks at gmail.com>
> Signed-off-by: Christian Borntraeger <borntraeger at de.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
I am frankly puzzled by this reporting on file sizes.
So what does it mean that it's the same?
> ---
>  drivers/s390/virtio/virtio_ccw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/virtio/virtio_ccw.c
b/drivers/s390/virtio/virtio_ccw.c
> index 0672c6234ae8..070c4da95f48 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -944,7 +944,7 @@ static void virtio_ccw_set_status(struct virtio_device
*vdev, u8 status)
>  	kfree(ccw);
>  }
>  
> -static struct virtio_config_ops virtio_ccw_config_ops = {
> +static const struct virtio_config_ops virtio_ccw_config_ops = {
>  	.get_features = virtio_ccw_get_features,
>  	.finalize_features = virtio_ccw_finalize_features,
>  	.get = virtio_ccw_get_config,
> -- 
> 2.11.0