[PATCH v1 0/2] Virtio: fix some vq allocation issues

[PATCH v1 0/2] Virtio: fix some vq allocation issues

[Wei Wang]

Some vqs don't need to be allocated when the related feature bits are
disabled. Callers notice the vq allocation layer by setting the related
names[i] to be NULL.

This patch series fixes the find_vqs implementations to handle this case.

Wei Wang (2):
  virtio_pci: use queue idx instead of array idx to set up the vq
  virtio: don't allocate vqs when names[i] = NULL

 drivers/misc/mic/vop/vop_main.c        |  9 +++++++--
 drivers/remoteproc/remoteproc_virtio.c |  9 +++++++--
 drivers/s390/virtio/virtio_ccw.c       | 12 +++++++++---
 drivers/virtio/virtio_mmio.c           |  9 +++++++--
 drivers/virtio/virtio_pci_common.c     |  8 ++++----
 5 files changed, 34 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH v1 1/2] virtio_pci: use queue idx instead of array idx to set up the vq

[Wei Wang]

When find_vqs, there will be no vq[i] allocation if its corresponding
names[i] is NULL. For example, the caller may pass in names[i] (i=4)
with names[2] being NULL because the related feature bit is turned off,
so technically there are 3 queues on the device, and name[4] should
correspond to the 3rd queue on the device.

So we use queue_idx as the queue index, which is increased only when the
queue exists.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 drivers/virtio/virtio_pci_common.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 465a6f5..d0584c0 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -285,7 +285,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	u16 msix_vec;
-	int i, err, nvectors, allocated_vectors;
+	int i, err, nvectors, allocated_vectors, queue_idx = 0;
 
 	vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
 	if (!vp_dev->vqs)
@@ -321,7 +321,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
 			msix_vec = allocated_vectors++;
 		else
 			msix_vec = VP_MSIX_VQ_VECTOR;
-		vqs[i] = vp_setup_vq(vdev, i, callbacks[i], names[i],
+		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
 				     ctx ? ctx[i] : false,
 				     msix_vec);
 		if (IS_ERR(vqs[i])) {
@@ -356,7 +356,7 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs,
 		const char * const names[], const bool *ctx)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	int i, err;
+	int i, err, queue_idx = 0;
 
 	vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
 	if (!vp_dev->vqs)
@@ -374,7 +374,7 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs,
 			vqs[i] = NULL;
 			continue;
 		}
-		vqs[i] = vp_setup_vq(vdev, i, callbacks[i], names[i],
+		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
 				     ctx ? ctx[i] : false,
 				     VIRTIO_MSI_NO_VECTOR);
 		if (IS_ERR(vqs[i])) {
-- 
2.7.4

Re: [PATCH v1 1/2] virtio_pci: use queue idx instead of array idx to set up the vq

[Cornelia Huck]

On Fri, 28 Dec 2018 10:26:25 +0800
Wei Wang <wei.w.wang@intel.com> wrote:

> When find_vqs, there will be no vq[i] allocation if its corresponding
> names[i] is NULL. For example, the caller may pass in names[i] (i=4)
> with names[2] being NULL because the related feature bit is turned off,
> so technically there are 3 queues on the device, and name[4] should
> correspond to the 3rd queue on the device.
> 
> So we use queue_idx as the queue index, which is increased only when the
> queue exists.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  drivers/virtio/virtio_pci_common.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v1 1/2] virtio_pci: use queue idx instead of array idx to set up the vq

[Christian Borntraeger]

Cc: stable@vger.kernel.org
Fixes: 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")


On 28.12.2018 03:26, Wei Wang wrote:
> When find_vqs, there will be no vq[i] allocation if its corresponding
> names[i] is NULL. For example, the caller may pass in names[i] (i=4)
> with names[2] being NULL because the related feature bit is turned off,
> so technically there are 3 queues on the device, and name[4] should
> correspond to the 3rd queue on the device.
> 
> So we use queue_idx as the queue index, which is increased only when the
> queue exists.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  drivers/virtio/virtio_pci_common.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 465a6f5..d0584c0 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -285,7 +285,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>  	u16 msix_vec;
> -	int i, err, nvectors, allocated_vectors;
> +	int i, err, nvectors, allocated_vectors, queue_idx = 0;
>  
>  	vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
>  	if (!vp_dev->vqs)
> @@ -321,7 +321,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
>  			msix_vec = allocated_vectors++;
>  		else
>  			msix_vec = VP_MSIX_VQ_VECTOR;
> -		vqs[i] = vp_setup_vq(vdev, i, callbacks[i], names[i],
> +		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
>  				     ctx ? ctx[i] : false,
>  				     msix_vec);
>  		if (IS_ERR(vqs[i])) {
> @@ -356,7 +356,7 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs,
>  		const char * const names[], const bool *ctx)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> -	int i, err;
> +	int i, err, queue_idx = 0;
>  
>  	vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
>  	if (!vp_dev->vqs)
> @@ -374,7 +374,7 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs,
>  			vqs[i] = NULL;
>  			continue;
>  		}
> -		vqs[i] = vp_setup_vq(vdev, i, callbacks[i], names[i],
> +		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
>  				     ctx ? ctx[i] : false,
>  				     VIRTIO_MSI_NO_VECTOR);
>  		if (IS_ERR(vqs[i])) {
> 

[PATCH v1 2/2] virtio: don't allocate vqs when names[i] = NULL

[Wei Wang]

Some vqs may not need to be allocated when their related feature bits
are disabled. So callers may pass in such vqs with "names = NULL".
Then we skip such vq allocations.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 drivers/misc/mic/vop/vop_main.c        |  9 +++++++--
 drivers/remoteproc/remoteproc_virtio.c |  9 +++++++--
 drivers/s390/virtio/virtio_ccw.c       | 12 +++++++++---
 drivers/virtio/virtio_mmio.c           |  9 +++++++--
 4 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index 6b212c8..2bfa3a9 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -394,16 +394,21 @@ static int vop_find_vqs(struct virtio_device *dev, unsigned nvqs,
 	struct _vop_vdev *vdev = to_vopvdev(dev);
 	struct vop_device *vpdev = vdev->vpdev;
 	struct mic_device_ctrl __iomem *dc = vdev->dc;
-	int i, err, retry;
+	int i, err, retry, queue_idx = 0;
 
 	/* We must have this many virtqueues. */
 	if (nvqs > ioread8(&vdev->desc->num_vq))
 		return -ENOENT;
 
 	for (i = 0; i < nvqs; ++i) {
+		if (!names[i]) {
+			vqs[i] = NULL;
+			continue;
+		}
+
 		dev_dbg(_vop_dev(vdev), "%s: %d: %s\n",
 			__func__, i, names[i]);
-		vqs[i] = vop_find_vq(dev, i, callbacks[i], names[i],
+		vqs[i] = vop_find_vq(dev, queue_idx++, callbacks[i], names[i],
 				     ctx ? ctx[i] : false);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 183fc42..2d7cd344 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -153,10 +153,15 @@ static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
 				 const bool * ctx,
 				 struct irq_affinity *desc)
 {
-	int i, ret;
+	int i, ret, queue_idx = 0;
 
 	for (i = 0; i < nvqs; ++i) {
-		vqs[i] = rp_find_vq(vdev, i, callbacks[i], names[i],
+		if (!names[i]) {
+			vqs[i] = NULL;
+			continue;
+		}
+
+		vqs[i] = rp_find_vq(vdev, queue_idx++, callbacks[i], names[i],
 				    ctx ? ctx[i] : false);
 		if (IS_ERR(vqs[i])) {
 			ret = PTR_ERR(vqs[i]);
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index fc9dbad..ae1d56d 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -635,7 +635,7 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 {
 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
 	unsigned long *indicatorp = NULL;
-	int ret, i;
+	int ret, i, queue_idx = 0;
 	struct ccw1 *ccw;
 
 	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
@@ -643,8 +643,14 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		return -ENOMEM;
 
 	for (i = 0; i < nvqs; ++i) {
-		vqs[i] = virtio_ccw_setup_vq(vdev, i, callbacks[i], names[i],
-					     ctx ? ctx[i] : false, ccw);
+		if (!names[i]) {
+			vqs[i] = NULL;
+			continue;
+		}
+
+		vqs[i] = virtio_ccw_setup_vq(vdev, queue_idx++, callbacks[i],
+					     names[i], ctx ? ctx[i] : false,
+					     ccw);
 		if (IS_ERR(vqs[i])) {
 			ret = PTR_ERR(vqs[i]);
 			vqs[i] = NULL;
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 4cd9ea5..d9dd0f78 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -468,7 +468,7 @@ static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
 	unsigned int irq = platform_get_irq(vm_dev->pdev, 0);
-	int i, err;
+	int i, err, queue_idx = 0;
 
 	err = request_irq(irq, vm_interrupt, IRQF_SHARED,
 			dev_name(&vdev->dev), vm_dev);
@@ -476,7 +476,12 @@ static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		return err;
 
 	for (i = 0; i < nvqs; ++i) {
-		vqs[i] = vm_setup_vq(vdev, i, callbacks[i], names[i],
+		if (!names[i]) {
+			vqs[i] = NULL;
+			continue;
+		}
+
+		vqs[i] = vm_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
 				     ctx ? ctx[i] : false);
 		if (IS_ERR(vqs[i])) {
 			vm_del_vqs(vdev);
-- 
2.7.4

Re: [PATCH v1 2/2] virtio: don't allocate vqs when names[i] = NULL

[Cornelia Huck]

On Fri, 28 Dec 2018 10:26:26 +0800
Wei Wang <wei.w.wang@intel.com> wrote:

> Some vqs may not need to be allocated when their related feature bits
> are disabled. So callers may pass in such vqs with "names = NULL".

s/=/==/ (here and in the subject)

> Then we skip such vq allocations.

> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  drivers/misc/mic/vop/vop_main.c        |  9 +++++++--
>  drivers/remoteproc/remoteproc_virtio.c |  9 +++++++--
>  drivers/s390/virtio/virtio_ccw.c       | 12 +++++++++---
>  drivers/virtio/virtio_mmio.c           |  9 +++++++--
>  4 files changed, 30 insertions(+), 9 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v1 2/2] virtio: don't allocate vqs when names[i] = NULL

[Christian Borntraeger]

Cc: stable@vger.kernel.org
Fixes: 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")

On 28.12.2018 03:26, Wei Wang wrote:
> Some vqs may not need to be allocated when their related feature bits
> are disabled. So callers may pass in such vqs with "names = NULL".
> Then we skip such vq allocations.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  drivers/misc/mic/vop/vop_main.c        |  9 +++++++--
>  drivers/remoteproc/remoteproc_virtio.c |  9 +++++++--
>  drivers/s390/virtio/virtio_ccw.c       | 12 +++++++++---
>  drivers/virtio/virtio_mmio.c           |  9 +++++++--
>  4 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
> index 6b212c8..2bfa3a9 100644
> --- a/drivers/misc/mic/vop/vop_main.c
> +++ b/drivers/misc/mic/vop/vop_main.c
> @@ -394,16 +394,21 @@ static int vop_find_vqs(struct virtio_device *dev, unsigned nvqs,
>  	struct _vop_vdev *vdev = to_vopvdev(dev);
>  	struct vop_device *vpdev = vdev->vpdev;
>  	struct mic_device_ctrl __iomem *dc = vdev->dc;
> -	int i, err, retry;
> +	int i, err, retry, queue_idx = 0;
>  
>  	/* We must have this many virtqueues. */
>  	if (nvqs > ioread8(&vdev->desc->num_vq))
>  		return -ENOENT;
>  
>  	for (i = 0; i < nvqs; ++i) {
> +		if (!names[i]) {
> +			vqs[i] = NULL;
> +			continue;
> +		}
> +
>  		dev_dbg(_vop_dev(vdev), "%s: %d: %s\n",
>  			__func__, i, names[i]);
> -		vqs[i] = vop_find_vq(dev, i, callbacks[i], names[i],
> +		vqs[i] = vop_find_vq(dev, queue_idx++, callbacks[i], names[i],
>  				     ctx ? ctx[i] : false);
>  		if (IS_ERR(vqs[i])) {
>  			err = PTR_ERR(vqs[i]);
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index 183fc42..2d7cd344 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -153,10 +153,15 @@ static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
>  				 const bool * ctx,
>  				 struct irq_affinity *desc)
>  {
> -	int i, ret;
> +	int i, ret, queue_idx = 0;
>  
>  	for (i = 0; i < nvqs; ++i) {
> -		vqs[i] = rp_find_vq(vdev, i, callbacks[i], names[i],
> +		if (!names[i]) {
> +			vqs[i] = NULL;
> +			continue;
> +		}
> +
> +		vqs[i] = rp_find_vq(vdev, queue_idx++, callbacks[i], names[i],
>  				    ctx ? ctx[i] : false);
>  		if (IS_ERR(vqs[i])) {
>  			ret = PTR_ERR(vqs[i]);
> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index fc9dbad..ae1d56d 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -635,7 +635,7 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  {
>  	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
>  	unsigned long *indicatorp = NULL;
> -	int ret, i;
> +	int ret, i, queue_idx = 0;
>  	struct ccw1 *ccw;
>  
>  	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> @@ -643,8 +643,14 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  		return -ENOMEM;
>  
>  	for (i = 0; i < nvqs; ++i) {
> -		vqs[i] = virtio_ccw_setup_vq(vdev, i, callbacks[i], names[i],
> -					     ctx ? ctx[i] : false, ccw);
> +		if (!names[i]) {
> +			vqs[i] = NULL;
> +			continue;
> +		}
> +
> +		vqs[i] = virtio_ccw_setup_vq(vdev, queue_idx++, callbacks[i],
> +					     names[i], ctx ? ctx[i] : false,
> +					     ccw);
>  		if (IS_ERR(vqs[i])) {
>  			ret = PTR_ERR(vqs[i]);
>  			vqs[i] = NULL;
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 4cd9ea5..d9dd0f78 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -468,7 +468,7 @@ static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  {
>  	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
>  	unsigned int irq = platform_get_irq(vm_dev->pdev, 0);
> -	int i, err;
> +	int i, err, queue_idx = 0;
>  
>  	err = request_irq(irq, vm_interrupt, IRQF_SHARED,
>  			dev_name(&vdev->dev), vm_dev);
> @@ -476,7 +476,12 @@ static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  		return err;
>  
>  	for (i = 0; i < nvqs; ++i) {
> -		vqs[i] = vm_setup_vq(vdev, i, callbacks[i], names[i],
> +		if (!names[i]) {
> +			vqs[i] = NULL;
> +			continue;
> +		}
> +
> +		vqs[i] = vm_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
>  				     ctx ? ctx[i] : false);
>  		if (IS_ERR(vqs[i])) {
>  			vm_del_vqs(vdev);
> 

Re: [PATCH v1 0/2] Virtio: fix some vq allocation issues

[Christian Borntraeger]

On 28.12.2018 03:26, Wei Wang wrote:
> Some vqs don't need to be allocated when the related feature bits are
> disabled. Callers notice the vq allocation layer by setting the related
> names[i] to be NULL.
> 
> This patch series fixes the find_vqs implementations to handle this case.

So the random crashes during boot are gone.
What still does not work is actually using the balloon.

So in the qemu monitor using lets say "balloon 1000"  will hang the guest.
Seems to be a deadlock in the virtio-ccw code.  We seem to call the
config code in the interrupt handler.

crash> bt
PID: 0      TASK: d9a400            CPU: 0   COMMAND: "swapper/0"
 LOWCORE INFO:
  -psw      : 0x0404c00180000000 0x0000000000116472
  -function : smp_yield_cpu at 116472
  -prefix   : 0x7fffc000
  -cpu timer: 0x7fffffcc8c0af5be
  -clock cmp: 0x720a4e4002831000
  -general registers:
     000000000000000000 000000000000000000
     0x000000000000009c 0x0000000000fac2b0
     0x0000000000000015 0xffffffffffffffe2
     0x000003e000100000 0x0000000000000001
     000000000000000000 0x0000000000000001
     0x00000000000003e8 0x000000000f85c020
     000000000000000000 0x0000000000000001
     0x0000000000116464 0x000003e00035fad0
  -access registers:
     0000000000 0000000000 0000000000 0000000000
     0000000000 0000000000 0000000000 0000000000
     0000000000 0000000000 0000000000 0000000000
     0000000000 0000000000 0000000000 0000000000
  -control registers:
     0x0080000014866a10 0x0000000000fbc007
     0x0000000000100140 000000000000000000
     0x000000000000ffff 0x0000000000100140
     0x0000000031000000 0x000000000f9281c3
     000000000000000000 000000000000000000
     000000000000000000 000000000000000000
     000000000000000000 0x0000000000fbc007
     0x00000000db000000 0x0000000000100280
  -floating point registers:
     000000000000000000 0x000002aa374b0298
     0x0000000000000001 0x0000000000000010
     0x00000000000001ae 0x000000000000000f
     0x000002aa46056010 0x000002aa460681c0
     0x000003ffd867d590 0x000003ffdca7c818
     0x000003ffd867d58f 0x000003fff6ffdc60
     0x000003ffd867dad8 0x000003ffdca7c5e8
     0x000003ffd867dadc 0x000003ffdca7c818

 #0 [3e00035faf8] arch_spin_lock_wait at a7bd52
 #1 [3e00035fb50] ccw_io_helper at 9130ea
 #2 [3e00035fbd0] virtio_ccw_get_config at 914a28
 #3 [3e00035fc30] virtballoon_changed at 76e776
 #4 [3e00035fc70] virtio_config_changed at 76aabc
 #5 [3e00035fca8] virtio_ccw_int_handler at 914ede
 #6 [3e00035fd18] ccw_device_irq at 8941d4
 #7 [3e00035fd48] do_cio_interrupt at 885906
 #8 [3e00035fd80] __handle_irq_event_percpu at 1b3c22
 #9 [3e00035fdf0] handle_irq_event_percpu at 1b3e1e
#10 [3e00035fe28] handle_percpu_irq at 1b87d8
#11 [3e00035fe58] generic_handle_irq at 1b2ce6
#12 [3e00035fe70] do_IRQ at 10c3b2
#13 [3e00035fea8] io_int_handler at a86b3c
 PSW:  0404c00180000000 00000000001034f6 (enabled_wait+70)
 GPRS: ffffffffffffffff 0000000000000000 000000007ff70200 0706c00180000000 
       000000000000000c 000001bf6f331c58 ffffffffffffffff 0000000000000000 
       0000000000000000 0000000000000000 0000000000000000 0000000000000001 
       000000007ff70200 0000000000a8b2f0 00000000001034f6 000003e000317e00 
 #0 [3e000317e28] arch_cpu_idle at 103842
 #1 [3e000317e48] do_idle at 17ad18
 #2 [3e000317e80] cpu_startup_entry at 17af16
 #3 [3e000317ea8] arch_call_rest_init at eac934


> 
> Wei Wang (2):
>   virtio_pci: use queue idx instead of array idx to set up the vq
>   virtio: don't allocate vqs when names[i] = NULL
> 
>  drivers/misc/mic/vop/vop_main.c        |  9 +++++++--
>  drivers/remoteproc/remoteproc_virtio.c |  9 +++++++--
>  drivers/s390/virtio/virtio_ccw.c       | 12 +++++++++---
>  drivers/virtio/virtio_mmio.c           |  9 +++++++--
>  drivers/virtio/virtio_pci_common.c     |  8 ++++----
>  5 files changed, 34 insertions(+), 13 deletions(-)
> 

RE: [PATCH v1 0/2] Virtio: fix some vq allocation issues

[Wang, Wei W]

On Friday, December 28, 2018 3:57 PM, Christian Borntraeger wrote:
> On 28.12.2018 03:26, Wei Wang wrote:
> > Some vqs don't need to be allocated when the related feature bits are
> > disabled. Callers notice the vq allocation layer by setting the
> > related names[i] to be NULL.
> >
> > This patch series fixes the find_vqs implementations to handle this case.
> 
> So the random crashes during boot are gone.
> What still does not work is actually using the balloon.
> 
> So in the qemu monitor using lets say "balloon 1000"  will hang the guest.
> Seems to be a deadlock in the virtio-ccw code.  We seem to call the config
> code in the interrupt handler.

Yes. It reads a config register from the interrupt handler. Do you know why ccw doesn't support it and has some internal lock that caused the deadlock issue?
 
Best,
Wei

Re: [PATCH v1 0/2] Virtio: fix some vq allocation issues

[Halil Pasic]

On Sat, 29 Dec 2018 02:45:49 +0000
"Wang, Wei W" <wei.w.wang@intel.com> wrote:

> On Friday, December 28, 2018 3:57 PM, Christian Borntraeger wrote:
> > On 28.12.2018 03:26, Wei Wang wrote:
> > > Some vqs don't need to be allocated when the related feature bits are
> > > disabled. Callers notice the vq allocation layer by setting the
> > > related names[i] to be NULL.
> > >
> > > This patch series fixes the find_vqs implementations to handle this case.
> > 
> > So the random crashes during boot are gone.
> > What still does not work is actually using the balloon.
> > 
> > So in the qemu monitor using lets say "balloon 1000"  will hang the guest.
> > Seems to be a deadlock in the virtio-ccw code.  We seem to call the config
> > code in the interrupt handler.
> 
> Yes. It reads a config register from the interrupt handler. Do you know why ccw doesn't support it and has some internal lock that caused the deadlock issue?
>  
> Best,
> Wei

I guess you are the first one trying to read virtio config from within
interrupt context. AFAICT this never worked.

About what happens. The apidoc of ccw_device_start() says it needs to be
called with the ccw device lock held, so ccw_io_helper() tries to take
it (since forever I guess). OTOH do_cio_interrupt() takes the subchannel
lock and io_subchannel_initialize_dev() makes the ccw device lock be the
subchannel lock. That means when one tries to get virtio config form
within a cio interrupt context we deadlock, because we try to take a lock
we already have.

That said, I don't think this limitation is by design (i.e. intended).
Maybe Connie can help us with that question. AFAIK we have nothing
documented regarding this (neither that can nor can't).

Obviously, there are multiple ways around this problem, and at the
moment I can't tell which would be my preferred one.

Regards,
Halil

RE: [PATCH v1 0/2] Virtio: fix some vq allocation issues

[Wang, Wei W]

On Sunday, December 30, 2018 2:06 PM, Halil Pasic wrote:
> 
> I guess you are the first one trying to read virtio config from within interrupt
> context. AFAICT this never worked.

I'm not sure about "never worked". It seems to work well with virtio-pci.
But looking forward to hearing a solid reason why reading config inside
the handler is forbidden (if that's true).

> About what happens. The apidoc of ccw_device_start() says it needs to be
> called with the ccw device lock held, so ccw_io_helper() tries to take it (since
> forever I guess). OTOH do_cio_interrupt() takes the subchannel lock and
> io_subchannel_initialize_dev() makes the ccw device lock be the subchannel
> lock. That means when one tries to get virtio config form within a cio
> interrupt context we deadlock, because we try to take a lock we already have.
> 
> That said, I don't think this limitation is by design (i.e. intended).
> Maybe Connie can help us with that question. AFAIK we have nothing
> documented regarding this (neither that can nor can't).
> 
> Obviously, there are multiple ways around this problem, and at the moment
> I can't tell which would be my preferred one.

Yes, it's also not difficult to tweak the virtio-balloon code to avoid that issue.
But if that's just an issue with ccw itself, I think it's better to tweak ccw and
remain virtio-balloon unchanged.

Best,
Wei

Re: [virtio-dev] RE: [PATCH v1 0/2] Virtio: fix some vq allocation issues

[Halil Pasic]

On Mon, 31 Dec 2018 06:03:51 +0000
"Wang, Wei W" <wei.w.wang@intel.com> wrote:

> On Sunday, December 30, 2018 2:06 PM, Halil Pasic wrote:
> > 
> > I guess you are the first one trying to read virtio config from within interrupt
> > context. AFAICT this never worked.
> 
> I'm not sure about "never worked". It seems to work well with virtio-pci.
> But looking forward to hearing a solid reason why reading config inside
> the handler is forbidden (if that's true).

By "never worked" I meant "never worked with virtio-ccw". Sorry
about the misunderstanding. Seems I've also failed to convey that I don't
know if reading config inside the handler is forbidden or not. So please
don't expect me providing the solid reasons you are looking forward to.

> 
> > About what happens. The apidoc of ccw_device_start() says it needs to be
> > called with the ccw device lock held, so ccw_io_helper() tries to take it (since
> > forever I guess). OTOH do_cio_interrupt() takes the subchannel lock and
> > io_subchannel_initialize_dev() makes the ccw device lock be the subchannel
> > lock. That means when one tries to get virtio config form within a cio
> > interrupt context we deadlock, because we try to take a lock we already have.
> > 
> > That said, I don't think this limitation is by design (i.e. intended).
> > Maybe Connie can help us with that question. AFAIK we have nothing
> > documented regarding this (neither that can nor can't).
> > 
> > Obviously, there are multiple ways around this problem, and at the moment
> > I can't tell which would be my preferred one.
> 
> Yes, it's also not difficult to tweak the virtio-balloon code to avoid that issue.
> But if that's just an issue with ccw itself, I think it's better to tweak ccw and
> remain virtio-balloon unchanged.
> 

As I said, at the moment I don't have a preference regarding the fix,
partly because I'm not sure if "reading config inside the handler" is OK
or not. Maybe Connie or Michael can help us here. I'm however sure that
commit 86a5597 "virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT"
breaks virtio-balloon with the ccw transport (i.e. effectively breaks 
virtio-balloon on s390): it used to work before and does not work
after.

AFAICT tweaking the balloon code may be simpler than tweaking the
virtio-ccw (transport code). ccw_io_helper() relies on getting
an interrupt when the issued IO is done. If virtio-ccw is buggy, it
needs to be fixed, but I'm not sure it is.

Regards,
Halil

 

Re: [virtio-dev] RE: [PATCH v1 0/2] Virtio: fix some vq allocation issues

[Cornelia Huck]

On Tue, 1 Jan 2019 00:40:19 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Mon, 31 Dec 2018 06:03:51 +0000
> "Wang, Wei W" <wei.w.wang@intel.com> wrote:
> 
> > On Sunday, December 30, 2018 2:06 PM, Halil Pasic wrote:  
> > > 
> > > I guess you are the first one trying to read virtio config from within interrupt
> > > context. AFAICT this never worked.  
> > 
> > I'm not sure about "never worked". It seems to work well with virtio-pci.
> > But looking forward to hearing a solid reason why reading config inside
> > the handler is forbidden (if that's true).  
> 
> By "never worked" I meant "never worked with virtio-ccw". Sorry
> about the misunderstanding. Seems I've also failed to convey that I don't
> know if reading config inside the handler is forbidden or not. So please
> don't expect me providing the solid reasons you are looking forward to.

It won't work with the current code, and this is all a bit ugly :( More
verbose explanation below.

> 
> >   
> > > About what happens. The apidoc of ccw_device_start() says it needs to be
> > > called with the ccw device lock held, so ccw_io_helper() tries to take it (since
> > > forever I guess). OTOH do_cio_interrupt() takes the subchannel lock and
> > > io_subchannel_initialize_dev() makes the ccw device lock be the subchannel
> > > lock. That means when one tries to get virtio config form within a cio
> > > interrupt context we deadlock, because we try to take a lock we already have.
> > > 
> > > That said, I don't think this limitation is by design (i.e. intended).
> > > Maybe Connie can help us with that question. AFAIK we have nothing
> > > documented regarding this (neither that can nor can't).

The main problem is that channel I/O is a fundamentally asynchronous
mechanism. As channel devices don't have the concept of config spaces
(or some other things that virtio needs), I decided to map
reading/writing the config space to channel commands. Starting I/O on a
subchannel always needs the lock (to avoid races on the subchannel),
and the asynchronous interrupt for that I/O needs the lock as well (for
the same reason; things like the scsw contain state that you want to
access without races). A config change also means that the subchannel
becomes state pending (and an interrupt is made pending), so the
subchannel lock is taken for that path as well. (Virtqueue
notifications are handled differently on modern QEMU, but that does not
come into play here.)

> > > 
> > > Obviously, there are multiple ways around this problem, and at the moment
> > > I can't tell which would be my preferred one.  
> > 
> > Yes, it's also not difficult to tweak the virtio-balloon code to avoid that issue.
> > But if that's just an issue with ccw itself, I think it's better to tweak ccw and
> > remain virtio-balloon unchanged.
> >   
> 
> As I said, at the moment I don't have a preference regarding the fix,
> partly because I'm not sure if "reading config inside the handler" is OK
> or not. Maybe Connie or Michael can help us here. I'm however sure that
> commit 86a5597 "virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT"
> breaks virtio-balloon with the ccw transport (i.e. effectively breaks 
> virtio-balloon on s390): it used to work before and does not work
> after.

Yes, that's unfortunate.

> 
> AFAICT tweaking the balloon code may be simpler than tweaking the
> virtio-ccw (transport code). ccw_io_helper() relies on getting
> an interrupt when the issued IO is done. If virtio-ccw is buggy, it
> needs to be fixed, but I'm not sure it is.

I would not call virtio-ccw buggy, but it has some constraints that
virtio-pci apparently doesn't have (and which did not show up so far;
e.g. virtio-blk schedules a work item on config change, so there's no
deadlock there.)

One way to get out of that constraint (don't interact with the config
space directly in the config changed handler) would be to schedule a
work item in virtio-ccw that calls virtio_config_changed() for the
device. My understanding is that delaying the notification to a work
queue would be fine.

From what I can see, that should make us safe on any modern QEMU (that
uses adapter interrupts). There still might be problems if we are using
classic subchannel interrupts for virtqueue notifications and the
handler interacts with the config space, but I'm not sure whether it is
worth spending time on that.

Re: [virtio-dev] RE: [PATCH v1 0/2] Virtio: fix some vq allocation issues

[Cornelia Huck]

On Wed, 2 Jan 2019 10:53:14 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 1 Jan 2019 00:40:19 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:

> > As I said, at the moment I don't have a preference regarding the fix,
> > partly because I'm not sure if "reading config inside the handler" is OK
> > or not. Maybe Connie or Michael can help us here. I'm however sure that
> > commit 86a5597 "virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT"
> > breaks virtio-balloon with the ccw transport (i.e. effectively breaks 
> > virtio-balloon on s390): it used to work before and does not work
> > after.  
> 
> Yes, that's unfortunate.
> 
> > 
> > AFAICT tweaking the balloon code may be simpler than tweaking the
> > virtio-ccw (transport code). ccw_io_helper() relies on getting
> > an interrupt when the issued IO is done. If virtio-ccw is buggy, it
> > needs to be fixed, but I'm not sure it is.  
> 
> I would not call virtio-ccw buggy, but it has some constraints that
> virtio-pci apparently doesn't have (and which did not show up so far;
> e.g. virtio-blk schedules a work item on config change, so there's no
> deadlock there.)
> 
> One way to get out of that constraint (don't interact with the config
> space directly in the config changed handler) would be to schedule a
> work item in virtio-ccw that calls virtio_config_changed() for the
> device. My understanding is that delaying the notification to a work
> queue would be fine.

Unfortunately, calling virtio_config_changed() from a work item is not
enough: That function takes the config_lock, and the virtio-ccw code to
get the config both needs to allocate some memory and call schedule :/

The best option really seems to be
- have virtio-balloon move the handling of the config change onto a
  workqueue or something like that, and
- document that you cannot read/write the virtio config space from an
  atomic context

Unless someone has a better idea?

Re: [virtio-dev] RE: [PATCH v1 0/2] Virtio: fix some vq allocation issues

[Halil Pasic]

On Wed, 2 Jan 2019 14:23:38 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 2 Jan 2019 10:53:14 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Tue, 1 Jan 2019 00:40:19 +0100
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > > As I said, at the moment I don't have a preference regarding the fix,
> > > partly because I'm not sure if "reading config inside the handler" is OK
> > > or not. Maybe Connie or Michael can help us here. I'm however sure that
> > > commit 86a5597 "virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT"
> > > breaks virtio-balloon with the ccw transport (i.e. effectively breaks 
> > > virtio-balloon on s390): it used to work before and does not work
> > > after.  
> > 
> > Yes, that's unfortunate.
> > 
> > > 
> > > AFAICT tweaking the balloon code may be simpler than tweaking the
> > > virtio-ccw (transport code). ccw_io_helper() relies on getting
> > > an interrupt when the issued IO is done. If virtio-ccw is buggy, it
> > > needs to be fixed, but I'm not sure it is.  
> > 
> > I would not call virtio-ccw buggy, but it has some constraints that
> > virtio-pci apparently doesn't have (and which did not show up so far;
> > e.g. virtio-blk schedules a work item on config change, so there's no
> > deadlock there.)
> > 
> > One way to get out of that constraint (don't interact with the config
> > space directly in the config changed handler) would be to schedule a
> > work item in virtio-ccw that calls virtio_config_changed() for the
> > device. My understanding is that delaying the notification to a work
> > queue would be fine.
> 
> Unfortunately, calling virtio_config_changed() from a work item is not
> enough: That function takes the config_lock, and the virtio-ccw code to
> get the config both needs to allocate some memory and call schedule :/
> 
> The best option really seems to be
> - have virtio-balloon move the handling of the config change onto a
>   workqueue or something like that, and
> - document that you cannot read/write the virtio config space from an
>   atomic context
> 
> Unless someone has a better idea?
> 

I wonder, would making config_lock a mutex suffice?

I've already hinted that a virtio-balloon side fix is probably the
simpler variant. 

I agree, let's fix the regression first, and think about wether to teach
virtio-ccw to allow config manipulation from virtio_config_changed() or
not later.

Regards,
Halil

Re: [virtio-dev] RE: [PATCH v1 0/2] Virtio: fix some vq allocation issues

[Cornelia Huck]

On Wed, 2 Jan 2019 16:59:19 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Wed, 2 Jan 2019 14:23:38 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Wed, 2 Jan 2019 10:53:14 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > On Tue, 1 Jan 2019 00:40:19 +0100
> > > Halil Pasic <pasic@linux.ibm.com> wrote:  

> > > > AFAICT tweaking the balloon code may be simpler than tweaking the
> > > > virtio-ccw (transport code). ccw_io_helper() relies on getting
> > > > an interrupt when the issued IO is done. If virtio-ccw is buggy, it
> > > > needs to be fixed, but I'm not sure it is.    
> > > 
> > > I would not call virtio-ccw buggy, but it has some constraints that
> > > virtio-pci apparently doesn't have (and which did not show up so far;
> > > e.g. virtio-blk schedules a work item on config change, so there's no
> > > deadlock there.)
> > > 
> > > One way to get out of that constraint (don't interact with the config
> > > space directly in the config changed handler) would be to schedule a
> > > work item in virtio-ccw that calls virtio_config_changed() for the
> > > device. My understanding is that delaying the notification to a work
> > > queue would be fine.  
> > 
> > Unfortunately, calling virtio_config_changed() from a work item is not
> > enough: That function takes the config_lock, and the virtio-ccw code to
> > get the config both needs to allocate some memory and call schedule :/
> > 
> > The best option really seems to be
> > - have virtio-balloon move the handling of the config change onto a
> >   workqueue or something like that, and
> > - document that you cannot read/write the virtio config space from an
> >   atomic context
> > 
> > Unless someone has a better idea?
> >   
> 
> I wonder, would making config_lock a mutex suffice?

Unless I'm mistaken, you can't take a mutex in an interrupt path.

> I've already hinted that a virtio-balloon side fix is probably the
> simpler variant. 

Yes, I think so as well.

> I agree, let's fix the regression first, and think about wether to teach
> virtio-ccw to allow config manipulation from virtio_config_changed() or
> not later.

Or whether we can tweak the virtio code instead. But I agree, let's get
things working again first.

Re: [virtio-dev] RE: [PATCH v1 0/2] Virtio: fix some vq allocation issues

[Halil Pasic]

On Wed, 2 Jan 2019 19:02:33 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 2 Jan 2019 16:59:19 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Wed, 2 Jan 2019 14:23:38 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > > On Wed, 2 Jan 2019 10:53:14 +0100
> > > Cornelia Huck <cohuck@redhat.com> wrote:
> > >   
> > > > On Tue, 1 Jan 2019 00:40:19 +0100
> > > > Halil Pasic <pasic@linux.ibm.com> wrote:  
> 
> > > > > AFAICT tweaking the balloon code may be simpler than tweaking the
> > > > > virtio-ccw (transport code). ccw_io_helper() relies on getting
> > > > > an interrupt when the issued IO is done. If virtio-ccw is buggy, it
> > > > > needs to be fixed, but I'm not sure it is.    
> > > > 
> > > > I would not call virtio-ccw buggy, but it has some constraints that
> > > > virtio-pci apparently doesn't have (and which did not show up so far;
> > > > e.g. virtio-blk schedules a work item on config change, so there's no
> > > > deadlock there.)
> > > > 
> > > > One way to get out of that constraint (don't interact with the config
> > > > space directly in the config changed handler) would be to schedule a
> > > > work item in virtio-ccw that calls virtio_config_changed() for the
> > > > device. My understanding is that delaying the notification to a work
> > > > queue would be fine.  
> > > 
> > > Unfortunately, calling virtio_config_changed() from a work item is not
> > > enough: That function takes the config_lock, and the virtio-ccw code to
> > > get the config both needs to allocate some memory and call schedule :/
> > > 
> > > The best option really seems to be
> > > - have virtio-balloon move the handling of the config change onto a
> > >   workqueue or something like that, and
> > > - document that you cannot read/write the virtio config space from an
> > >   atomic context
> > > 
> > > Unless someone has a better idea?
> > >   
> > 
> > I wonder, would making config_lock a mutex suffice?
> 
> Unless I'm mistaken, you can't take a mutex in an interrupt path.
>

I was too focused on virtio-ccw. We have the workqueue now, so it would
not be a problem for us, but for the other transports. Grrr

Re: [virtio-dev] RE: [PATCH v1 0/2] Virtio: fix some vq allocation issues

[Halil Pasic]

On Wed, 2 Jan 2019 10:53:14 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 1 Jan 2019 00:40:19 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Mon, 31 Dec 2018 06:03:51 +0000
> > "Wang, Wei W" <wei.w.wang@intel.com> wrote:
> > 
> > > On Sunday, December 30, 2018 2:06 PM, Halil Pasic wrote:  
> > > > 
> > > > I guess you are the first one trying to read virtio config from within interrupt
> > > > context. AFAICT this never worked.  
> > > 
> > > I'm not sure about "never worked". It seems to work well with virtio-pci.
> > > But looking forward to hearing a solid reason why reading config inside
> > > the handler is forbidden (if that's true).  
> > 
> > By "never worked" I meant "never worked with virtio-ccw". Sorry
> > about the misunderstanding. Seems I've also failed to convey that I don't
> > know if reading config inside the handler is forbidden or not. So please
> > don't expect me providing the solid reasons you are looking forward to.
> 
> It won't work with the current code, and this is all a bit ugly :( More
> verbose explanation below.
> 
> > 
> > >   
> > > > About what happens. The apidoc of ccw_device_start() says it needs to be
> > > > called with the ccw device lock held, so ccw_io_helper() tries to take it (since
> > > > forever I guess). OTOH do_cio_interrupt() takes the subchannel lock and
> > > > io_subchannel_initialize_dev() makes the ccw device lock be the subchannel
> > > > lock. That means when one tries to get virtio config form within a cio
> > > > interrupt context we deadlock, because we try to take a lock we already have.
> > > > 
> > > > That said, I don't think this limitation is by design (i.e. intended).
> > > > Maybe Connie can help us with that question. AFAIK we have nothing
> > > > documented regarding this (neither that can nor can't).
> 
> The main problem is that channel I/O is a fundamentally asynchronous
> mechanism. As channel devices don't have the concept of config spaces
> (or some other things that virtio needs), I decided to map
> reading/writing the config space to channel commands. Starting I/O on a
> subchannel always needs the lock (to avoid races on the subchannel),
> and the asynchronous interrupt for that I/O needs the lock as well (for
> the same reason; things like the scsw contain state that you want to
> access without races). A config change also means that the subchannel
> becomes state pending (and an interrupt is made pending), so the
> subchannel lock is taken for that path as well. (Virtqueue
> notifications are handled differently on modern QEMU, but that does not
> come into play here.)
> 

Besides locking (thinking along the lines that we work around the
lock problem somehow) there is also the new PSW which masks IO
interrupts. As I said, doing something about this seems non-trivial at
least.

> > > > 
> > > > Obviously, there are multiple ways around this problem, and at the moment
> > > > I can't tell which would be my preferred one.  
> > > 
> > > Yes, it's also not difficult to tweak the virtio-balloon code to avoid that issue.
> > > But if that's just an issue with ccw itself, I think it's better to tweak ccw and
> > > remain virtio-balloon unchanged.
> > >   
> > 
> > As I said, at the moment I don't have a preference regarding the fix,
> > partly because I'm not sure if "reading config inside the handler" is OK
> > or not. Maybe Connie or Michael can help us here. I'm however sure that
> > commit 86a5597 "virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT"
> > breaks virtio-balloon with the ccw transport (i.e. effectively breaks 
> > virtio-balloon on s390): it used to work before and does not work
> > after.
> 
> Yes, that's unfortunate.
> 
> > 
> > AFAICT tweaking the balloon code may be simpler than tweaking the
> > virtio-ccw (transport code). ccw_io_helper() relies on getting
> > an interrupt when the issued IO is done. If virtio-ccw is buggy, it
> > needs to be fixed, but I'm not sure it is.
> 
> I would not call virtio-ccw buggy, but it has some constraints that
> virtio-pci apparently doesn't have (and which did not show up so far;
> e.g. virtio-blk schedules a work item on config change, so there's no
> deadlock there.)

IMHO it is an internal API design thing. From the spirit of the virtio
standard perspective  a virtio-ccw device is a ccw device, and acts like
one. We don't support new IO form ccw device interrupt handler. So that's
quite OK. OTOH we probably do want a coherent in kernel virtio
interface. And if that one needs to account for all the quirks of any
transport, that is quite ugly.

> 
> One way to get out of that constraint (don't interact with the config
> space directly in the config changed handler) would be to schedule a
> work item in virtio-ccw that calls virtio_config_changed() for the
> device. My understanding is that delaying the notification to a work
> queue would be fine.
> 

That would get us out of irq context, but I read you found other
problems.

[..]

Regards,
Halil

Re: [PATCH v1 0/2] Virtio: fix some vq allocation issues

[Wei Wang]

On 12/28/2018 03:57 PM, Christian Borntraeger wrote:
>
> On 28.12.2018 03:26, Wei Wang wrote:
>> Some vqs don't need to be allocated when the related feature bits are
>> disabled. Callers notice the vq allocation layer by setting the related
>> names[i] to be NULL.
>>
>> This patch series fixes the find_vqs implementations to handle this case.
> So the random crashes during boot are gone.
> What still does not work is actually using the balloon.
>
> So in the qemu monitor using lets say "balloon 1000"  will hang the guest.
> Seems to be a deadlock in the virtio-ccw code.  We seem to call the
> config code in the interrupt handler.

Please try with applying both this series and the "virtio-balloon: tweak 
config_changed"
series that I just sent.
This series fixes the ccw booting issue and that series tries to fix the 
ccw deadlock issue.

Best,
Wei