* [PATCH 1/2] vfio: ccw: bypass bad idaw address when fetching IDAL ccws
2017-10-11 2:38 [PATCH 0/2] vfio-ccw bugfix Dong Jia Shi
@ 2017-10-11 2:38 ` Dong Jia Shi
2017-10-16 9:09 ` Cornelia Huck
2017-10-11 2:38 ` [PATCH 2/2] vfio: ccw: validate the count field of a ccw before pinning Dong Jia Shi
2017-10-16 9:37 ` [PATCH 0/2] vfio-ccw bugfix Cornelia Huck
2 siblings, 1 reply; 6+ messages in thread
From: Dong Jia Shi @ 2017-10-11 2:38 UTC (permalink / raw)
To: linux-kernel, linux-s390, kvm
Cc: cohuck, borntraeger, bjsdjshi, pasic, pmorel
We currently returns the same error code (-EFAULT) to indicate two
different error cases:
1. a bug in vfio-ccw implementation has been found.
2. a buggy channel program has been detected.
This brings difficulty for userland program (specifically Qemu) to
handle.
Let's use -EFAULT to only indicate the first case. For the second
case, we simply hand over the ccws to lower level for further
handling.
Notice:
Once a bad idaw address is detected, the current behavior is to
suppress the ssch. With this fix, the channel program will be
accepted, and partial of the channel program (the part ahead of
the bad idaw) could possibly be executed by the device before
I/O conclusion.
Suggested-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
---
drivers/s390/cio/vfio_ccw_cp.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 5ccfdc80d0ec..722f8b8c7273 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -569,10 +569,6 @@ static int ccwchain_fetch_idal(struct ccwchain *chain,
for (i = 0; i < idaw_nr; i++) {
idaw_iova = *(idaws + i);
- if (IS_ERR_VALUE(idaw_iova)) {
- ret = -EFAULT;
- goto out_free_idaws;
- }
ret = pfn_array_alloc_pin(pat->pat_pa + i, cp->mdev,
idaw_iova, 1);
--
2.13.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] vfio: ccw: bypass bad idaw address when fetching IDAL ccws
2017-10-11 2:38 ` [PATCH 1/2] vfio: ccw: bypass bad idaw address when fetching IDAL ccws Dong Jia Shi
@ 2017-10-16 9:09 ` Cornelia Huck
0 siblings, 0 replies; 6+ messages in thread
From: Cornelia Huck @ 2017-10-16 9:09 UTC (permalink / raw)
To: Dong Jia Shi; +Cc: linux-kernel, linux-s390, kvm, borntraeger, pasic, pmorel
On Wed, 11 Oct 2017 04:38:21 +0200
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> We currently returns the same error code (-EFAULT) to indicate two
s/returns/return/
> different error cases:
> 1. a bug in vfio-ccw implementation has been found.
> 2. a buggy channel program has been detected.
>
> This brings difficulty for userland program (specifically Qemu) to
> handle.
>
> Let's use -EFAULT to only indicate the first case. For the second
> case, we simply hand over the ccws to lower level for further
> handling.
>
> Notice:
> Once a bad idaw address is detected, the current behavior is to
> suppress the ssch. With this fix, the channel program will be
> accepted, and partial of the channel program (the part ahead of
s/partial/part/
> the bad idaw) could possibly be executed by the device before
> I/O conclusion.
That actually sounds more sensible than the current behaviour.
I'll fix up the typos when applying.
>
> Suggested-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
> drivers/s390/cio/vfio_ccw_cp.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 5ccfdc80d0ec..722f8b8c7273 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -569,10 +569,6 @@ static int ccwchain_fetch_idal(struct ccwchain *chain,
>
> for (i = 0; i < idaw_nr; i++) {
> idaw_iova = *(idaws + i);
> - if (IS_ERR_VALUE(idaw_iova)) {
> - ret = -EFAULT;
> - goto out_free_idaws;
> - }
>
> ret = pfn_array_alloc_pin(pat->pat_pa + i, cp->mdev,
> idaw_iova, 1);
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] vfio: ccw: validate the count field of a ccw before pinning
2017-10-11 2:38 [PATCH 0/2] vfio-ccw bugfix Dong Jia Shi
2017-10-11 2:38 ` [PATCH 1/2] vfio: ccw: bypass bad idaw address when fetching IDAL ccws Dong Jia Shi
@ 2017-10-11 2:38 ` Dong Jia Shi
2017-10-16 9:14 ` Cornelia Huck
2017-10-16 9:37 ` [PATCH 0/2] vfio-ccw bugfix Cornelia Huck
2 siblings, 1 reply; 6+ messages in thread
From: Dong Jia Shi @ 2017-10-11 2:38 UTC (permalink / raw)
To: linux-kernel, linux-s390, kvm
Cc: cohuck, borntraeger, bjsdjshi, pasic, pmorel
If the count field of a ccw is zero, there is no need to
try to pin page(s) for it. Let's check the count value
before starting pinning operations.
Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
---
drivers/s390/cio/vfio_ccw_cp.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 722f8b8c7273..d8f98ad9b029 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -105,7 +105,10 @@ static int pfn_array_alloc_pin(struct pfn_array *pa, struct device *mdev,
{
int ret = 0;
- if (!len || pa->pa_nr)
+ if (!len)
+ return 0;
+
+ if (pa->pa_nr)
return -EINVAL;
pa->pa_iova = iova;
@@ -501,6 +504,16 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
ccw = chain->ch_ccw + idx;
+ if (!ccw->count) {
+ /*
+ * We just want the translation result of any direct ccw
+ * to be an IDA ccw, so let's add the IDA flag for it.
+ * Although the flag will be ignored by firmware.
+ */
+ ccw->flags |= CCW_FLAG_IDA;
+ return 0;
+ }
+
/*
* Pin data page(s) in memory.
* The number of pages actually is the count of the idaws which will be
@@ -541,6 +554,9 @@ static int ccwchain_fetch_idal(struct ccwchain *chain,
ccw = chain->ch_ccw + idx;
+ if (!ccw->count)
+ return 0;
+
/* Calculate size of idaws. */
ret = copy_from_iova(cp->mdev, &idaw_iova, ccw->cda, sizeof(idaw_iova));
if (ret)
--
2.13.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] vfio: ccw: validate the count field of a ccw before pinning
2017-10-11 2:38 ` [PATCH 2/2] vfio: ccw: validate the count field of a ccw before pinning Dong Jia Shi
@ 2017-10-16 9:14 ` Cornelia Huck
0 siblings, 0 replies; 6+ messages in thread
From: Cornelia Huck @ 2017-10-16 9:14 UTC (permalink / raw)
To: Dong Jia Shi; +Cc: linux-kernel, linux-s390, kvm, borntraeger, pasic, pmorel
On Wed, 11 Oct 2017 04:38:22 +0200
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> If the count field of a ccw is zero, there is no need to
> try to pin page(s) for it. Let's check the count value
> before starting pinning operations.
>
> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
> drivers/s390/cio/vfio_ccw_cp.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
This sounds reasonable, and I could not spot anything in the
architecture that speaks against simply setting the IDA flag. Hopefully
there's nothing buried out of sight :)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] vfio-ccw bugfix
2017-10-11 2:38 [PATCH 0/2] vfio-ccw bugfix Dong Jia Shi
2017-10-11 2:38 ` [PATCH 1/2] vfio: ccw: bypass bad idaw address when fetching IDAL ccws Dong Jia Shi
2017-10-11 2:38 ` [PATCH 2/2] vfio: ccw: validate the count field of a ccw before pinning Dong Jia Shi
@ 2017-10-16 9:37 ` Cornelia Huck
2 siblings, 0 replies; 6+ messages in thread
From: Cornelia Huck @ 2017-10-16 9:37 UTC (permalink / raw)
To: Dong Jia Shi; +Cc: linux-kernel, linux-s390, kvm, borntraeger, pasic, pmorel
On Wed, 11 Oct 2017 04:38:20 +0200
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> Some bugfixes according to recent discussion.
>
> Dong Jia Shi (2):
> vfio: ccw: bypass bad idaw address when fetching IDAL ccws
> vfio: ccw: validate the count field of a ccw before pinning
>
> drivers/s390/cio/vfio_ccw_cp.c | 22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
Thanks, applied.
^ permalink raw reply [flat|nested] 6+ messages in thread