[PULL 0/5] virtio/s390 patches for -next

[PULL 0/5] virtio/s390 patches for -next

[Cornelia Huck]

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

[PULL 1/5] tools/virtio/ringtest: fix run-on-all.sh for offline cpus

[Cornelia Huck]

From: Halil Pasic <pasic@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@linux.vnet.ibm.com>
Fixes: ef1b144d ("tools/virtio/ringtest: fix run-on-all.sh to work without
/dev/cpu")
Reviewed-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@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

[PULL 2/5] tools/virtio/ringtest: tweaks for s390

[Cornelia Huck]

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

Make ringtest work on s390 too.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Acked-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@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

[PULL 3/5] virtio/s390: support READ_STATUS command for virtio-ccw

[Cornelia Huck]

From: Pierre Morel <pmorel@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@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@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

[PULL 3/5] virtio/s390: support READ_STATUS command for virtio-ccw

[Cornelia Huck]

From: Pierre Morel <pmorel@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@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@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

[PULL 4/5] virtio/s390: add missing \n to end of dev_err message

[Cornelia Huck]

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

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

Signed-off-by: Colin Ian King <colin.king@canonical.com>
Message-Id: <20160927200844.16008-1-colin.king@canonical.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@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

[PULL 5/5] virtio/s390: virtio: constify virtio_config_ops structures

[Cornelia Huck]

From: Bhumika Goyal <bhumirks@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@p={...};

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

@bad@
position p!={r1.p,ok1.p};
identifier r1.i;
@@
i@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@gmail.com>
Message-Id: <1484333336-13443-1-git-send-email-bhumirks@gmail.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@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

[PULL 5/5] virtio/s390: virtio: constify virtio_config_ops structures

[Cornelia Huck]

From: Bhumika Goyal <bhumirks@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@p={...};

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

@bad@
position p!={r1.p,ok1.p};
identifier r1.i;
@@
i@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@gmail.com>
Message-Id: <1484333336-13443-1-git-send-email-bhumirks@gmail.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@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

Re: [PULL 5/5] virtio/s390: virtio: constify virtio_config_ops structures

[Michael S. Tsirkin]

On Mon, Jan 16, 2017 at 03:04:33PM +0100, Cornelia Huck wrote:
> From: Bhumika Goyal <bhumirks@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@p={...};
> 
> @ok1@
> identifier r1.i;
> position p;
> struct virtio_ccw_device x;
> @@
> x.vdev.config=&i@p
> 
> @bad@
> position p!={r1.p,ok1.p};
> identifier r1.i;
> @@
> i@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@gmail.com>
> Message-Id: <1484333336-13443-1-git-send-email-bhumirks@gmail.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@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

Re: [PULL 5/5] virtio/s390: virtio: constify virtio_config_ops structures

[Christian Borntraeger]

On 01/16/2017 03:29 PM, Michael S. Tsirkin wrote:
> On Mon, Jan 16, 2017 at 03:04:33PM +0100, Cornelia Huck wrote:
>> From: Bhumika Goyal <bhumirks@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@p={...};
>>
>> @ok1@
>> identifier r1.i;
>> position p;
>> struct virtio_ccw_device x;
>> @@
>> x.vdev.config=&i@p
>>
>> @bad@
>> position p!={r1.p,ok1.p};
>> identifier r1.i;
>> @@
>> i@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@gmail.com>
>> Message-Id: <1484333336-13443-1-git-send-email-bhumirks@gmail.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> I am frankly puzzled by this reporting on file sizes.
> So what does it mean that it's the same?

I think it means that the compiler cannot make any additional optimization
due to that being const. It still seems the right thing to do.

> 
>> ---
>>  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
> 

Re: [PULL 5/5] virtio/s390: virtio: constify virtio_config_ops structures

[Bhumika Goyal]

On Mon, Jan 16, 2017 at 7:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Jan 16, 2017 at 03:04:33PM +0100, Cornelia Huck wrote:
>> From: Bhumika Goyal <bhumirks@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@p={...};
>>
>> @ok1@
>> identifier r1.i;
>> position p;
>> struct virtio_ccw_device x;
>> @@
>> x.vdev.config=&i@p
>>
>> @bad@
>> position p!={r1.p,ok1.p};
>> identifier r1.i;
>> @@
>> i@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@gmail.com>
>> Message-Id: <1484333336-13443-1-git-send-email-bhumirks@gmail.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>
> I am frankly puzzled by this reporting on file sizes.
> So what does it mean that it's the same?
>

Ideally after making the structure const, bytes should move from the
data to the text segment of the memory. So a shift in the number of
bytes should be seen in the size details of the.o file after compiling
the changes. But for this patch no shift in bytes took place before
and after applying the changes.

Thanks,
Bhumika

>> ---
>>  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

Re: [PULL 5/5] virtio/s390: virtio: constify virtio_config_ops structures

[Bhumika Goyal]

On Mon, Jan 16, 2017 at 7:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Jan 16, 2017 at 03:04:33PM +0100, Cornelia Huck wrote:
>> From: Bhumika Goyal <bhumirks@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@p={...};
>>
>> @ok1@
>> identifier r1.i;
>> position p;
>> struct virtio_ccw_device x;
>> @@
>> x.vdev.config=&i@p
>>
>> @bad@
>> position p!={r1.p,ok1.p};
>> identifier r1.i;
>> @@
>> i@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@gmail.com>
>> Message-Id: <1484333336-13443-1-git-send-email-bhumirks@gmail.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>
> I am frankly puzzled by this reporting on file sizes.
> So what does it mean that it's the same?
>

Ideally after making the structure const, bytes should move from the
data to the text segment of the memory. So a shift in the number of
bytes should be seen in the size details of the.o file after compiling
the changes. But for this patch no shift in bytes took place before
and after applying the changes.

Thanks,
Bhumika

>> ---
>>  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

Re: [PULL 5/5] virtio/s390: virtio: constify virtio_config_ops structures

[Michael S. Tsirkin]

On Mon, Jan 16, 2017 at 08:58:34PM +0530, Bhumika Goyal wrote:
> On Mon, Jan 16, 2017 at 7:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Jan 16, 2017 at 03:04:33PM +0100, Cornelia Huck wrote:
> >> From: Bhumika Goyal <bhumirks@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@p={...};
> >>
> >> @ok1@
> >> identifier r1.i;
> >> position p;
> >> struct virtio_ccw_device x;
> >> @@
> >> x.vdev.config=&i@p
> >>
> >> @bad@
> >> position p!={r1.p,ok1.p};
> >> identifier r1.i;
> >> @@
> >> i@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@gmail.com>
> >> Message-Id: <1484333336-13443-1-git-send-email-bhumirks@gmail.com>
> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> >
> > I am frankly puzzled by this reporting on file sizes.
> > So what does it mean that it's the same?
> >
> 
> Ideally after making the structure const, bytes should move from the
> data to the text segment of the memory. So a shift in the number of
> bytes should be seen in the size details of the.o file after compiling
> the changes. But for this patch no shift in bytes took place before
> and after applying the changes.
> 
> Thanks,
> Bhumika
> 
> >> ---
> >>  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

Could that be because of alignment?
I'd say a list of symbols in each section would be
a better indication of what happened.

-- 
MST

Re: [PULL 5/5] virtio/s390: virtio: constify virtio_config_ops structures

[Bhumika Goyal]

On Mon, Jan 16, 2017 at 9:31 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Jan 16, 2017 at 08:58:34PM +0530, Bhumika Goyal wrote:
>> On Mon, Jan 16, 2017 at 7:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Mon, Jan 16, 2017 at 03:04:33PM +0100, Cornelia Huck wrote:
>> >> From: Bhumika Goyal <bhumirks@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@p={...};
>> >>
>> >> @ok1@
>> >> identifier r1.i;
>> >> position p;
>> >> struct virtio_ccw_device x;
>> >> @@
>> >> x.vdev.config=&i@p
>> >>
>> >> @bad@
>> >> position p!={r1.p,ok1.p};
>> >> identifier r1.i;
>> >> @@
>> >> i@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@gmail.com>
>> >> Message-Id: <1484333336-13443-1-git-send-email-bhumirks@gmail.com>
>> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> >> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> >
>> > I am frankly puzzled by this reporting on file sizes.
>> > So what does it mean that it's the same?
>> >
>>
>> Ideally after making the structure const, bytes should move from the
>> data to the text segment of the memory. So a shift in the number of
>> bytes should be seen in the size details of the.o file after compiling
>> the changes. But for this patch no shift in bytes took place before
>> and after applying the changes.
>>
>> Thanks,
>> Bhumika
>>
>> >> ---
>> >>  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
>
> Could that be because of alignment?
> I'd say a list of symbols in each section would be
> a better indication of what happened.
>

Yes, alignment issues can cause a change in  file size.

Thanks,
Bhumika

> --
> MST

Re: [PULL 5/5] virtio/s390: virtio: constify virtio_config_ops structures

[Michael S. Tsirkin]

On Mon, Jan 16, 2017 at 08:58:34PM +0530, Bhumika Goyal wrote:
> On Mon, Jan 16, 2017 at 7:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Jan 16, 2017 at 03:04:33PM +0100, Cornelia Huck wrote:
> >> From: Bhumika Goyal <bhumirks@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@p={...};
> >>
> >> @ok1@
> >> identifier r1.i;
> >> position p;
> >> struct virtio_ccw_device x;
> >> @@
> >> x.vdev.config=&i@p
> >>
> >> @bad@
> >> position p!={r1.p,ok1.p};
> >> identifier r1.i;
> >> @@
> >> i@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@gmail.com>
> >> Message-Id: <1484333336-13443-1-git-send-email-bhumirks@gmail.com>
> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> >
> > I am frankly puzzled by this reporting on file sizes.
> > So what does it mean that it's the same?
> >
> 
> Ideally after making the structure const, bytes should move from the
> data to the text segment of the memory. So a shift in the number of
> bytes should be seen in the size details of the.o file after compiling
> the changes. But for this patch no shift in bytes took place before
> and after applying the changes.
> 
> Thanks,
> Bhumika
> 
> >> ---
> >>  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

Could that be because of alignment?
I'd say a list of symbols in each section would be
a better indication of what happened.

-- 
MST

Re: [PULL 5/5] virtio/s390: virtio: constify virtio_config_ops structures

[Michael S. Tsirkin]

On Mon, Jan 16, 2017 at 03:04:33PM +0100, Cornelia Huck wrote:
> From: Bhumika Goyal <bhumirks@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@p={...};
> 
> @ok1@
> identifier r1.i;
> position p;
> struct virtio_ccw_device x;
> @@
> x.vdev.config=&i@p
> 
> @bad@
> position p!={r1.p,ok1.p};
> identifier r1.i;
> @@
> i@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@gmail.com>
> Message-Id: <1484333336-13443-1-git-send-email-bhumirks@gmail.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@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

Re: [PULL 0/5] virtio/s390 patches for -next

[Michael S. Tsirkin]

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