* [PATCH v2 1/5] vfio: ccw: fix cleanup if cp_prefetch fails
2018-04-23 11:01 [PATCH v2 0/5] vfio: ccw: error handling fixes and improvements Dong Jia Shi
@ 2018-04-23 11:01 ` Dong Jia Shi
2018-04-23 11:38 ` Halil Pasic
` (2 more replies)
2018-04-23 11:01 ` [PATCH v2 2/5] vfio: ccw: shorten kernel doc description for pfn_array_pin() Dong Jia Shi
` (4 subsequent siblings)
5 siblings, 3 replies; 19+ messages in thread
From: Dong Jia Shi @ 2018-04-23 11:01 UTC (permalink / raw)
To: linux-kernel, linux-s390, kvm
Cc: cohuck, borntraeger, bjsdjshi, pasic, pmorel, Halil Pasic,
Dong Jia Shi
From: Halil Pasic <pasic@linux.vnet.ibm.com>
If the translation of a channel program fails, we may end up attempting
to clean up (free, unpin) stuff that never got translated (and allocated,
pinned) in the first place.
By adjusting the lengths of the chains accordingly (so the element that
failed, and all subsequent elements are excluded) cleanup activities
based on false assumptions can be avoided.
Let's make sure cp_free works properly after cp_prefetch returns with an
error by setting ch_len of a ccw chain to the number of the translated
CCWs on that chain.
Acked-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
---
drivers/s390/cio/vfio_ccw_cp.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 2c7550797ec2..62d66e195304 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -715,6 +715,10 @@ void cp_free(struct channel_program *cp)
* and stores the result to ccwchain list. @cp must have been
* initialized by a previous call with cp_init(). Otherwise, undefined
* behavior occurs.
+ * For each chain composing the channel program:
+ * - On entry ch_len holds the count of CCW to be translated.
+ * - On exit ch_len is adjusted to the count of successfully translated CCW.
+ * This allows cp_free to find in ch_len the count of CCW to free in a chain.
*
* The S/390 CCW Translation APIS (prefixed by 'cp_') are introduced
* as helpers to do ccw chain translation inside the kernel. Basically
@@ -749,11 +753,18 @@ int cp_prefetch(struct channel_program *cp)
for (idx = 0; idx < len; idx++) {
ret = ccwchain_fetch_one(chain, idx, cp);
if (ret)
- return ret;
+ goto out_err;
}
}
return 0;
+out_err:
+ /* Only cleanup the chain elements that were actually translated. */
+ chain->ch_len = idx;
+ list_for_each_entry_continue(chain, &cp->ccwchain_list, next) {
+ chain->ch_len = 0;
+ }
+ return ret;
}
/**
--
2.13.5
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/5] vfio: ccw: fix cleanup if cp_prefetch fails
2018-04-23 11:01 ` [PATCH v2 1/5] vfio: ccw: fix cleanup if cp_prefetch fails Dong Jia Shi
@ 2018-04-23 11:38 ` Halil Pasic
2018-04-23 11:40 ` Cornelia Huck
2018-04-24 9:31 ` Cornelia Huck
2 siblings, 0 replies; 19+ messages in thread
From: Halil Pasic @ 2018-04-23 11:38 UTC (permalink / raw)
To: Dong Jia Shi, linux-kernel, linux-s390, kvm
Cc: cohuck, borntraeger, bjsdjshi, pmorel, Halil Pasic
On 04/23/2018 01:01 PM, Dong Jia Shi wrote:
> From: Halil Pasic <pasic@linux.vnet.ibm.com>
>
> If the translation of a channel program fails, we may end up attempting
> to clean up (free, unpin) stuff that never got translated (and allocated,
> pinned) in the first place.
>
> By adjusting the lengths of the chains accordingly (so the element that
> failed, and all subsequent elements are excluded) cleanup activities
> based on false assumptions can be avoided.
>
> Let's make sure cp_free works properly after cp_prefetch returns with an
> error by setting ch_len of a ccw chain to the number of the translated
> CCWs on that chain.
>
> Acked-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
AFAIR we came to the conclusion that this one is stable
material. [https://www.spinics.net/lists/kvm/msg166629.html]
> ---
> drivers/s390/cio/vfio_ccw_cp.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 2c7550797ec2..62d66e195304 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -715,6 +715,10 @@ void cp_free(struct channel_program *cp)
> * and stores the result to ccwchain list. @cp must have been
> * initialized by a previous call with cp_init(). Otherwise, undefined
> * behavior occurs.
> + * For each chain composing the channel program:
> + * - On entry ch_len holds the count of CCW to be translated.
> + * - On exit ch_len is adjusted to the count of successfully translated CCW.
> + * This allows cp_free to find in ch_len the count of CCW to free in a chain.
> *
> * The S/390 CCW Translation APIS (prefixed by 'cp_') are introduced
> * as helpers to do ccw chain translation inside the kernel. Basically
> @@ -749,11 +753,18 @@ int cp_prefetch(struct channel_program *cp)
> for (idx = 0; idx < len; idx++) {
> ret = ccwchain_fetch_one(chain, idx, cp);
> if (ret)
> - return ret;
> + goto out_err;
> }
> }
>
> return 0;
> +out_err:
> + /* Only cleanup the chain elements that were actually translated. */
> + chain->ch_len = idx;
> + list_for_each_entry_continue(chain, &cp->ccwchain_list, next) {
> + chain->ch_len = 0;
> + }
> + return ret;
> }
>
> /**
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/5] vfio: ccw: fix cleanup if cp_prefetch fails
2018-04-23 11:01 ` [PATCH v2 1/5] vfio: ccw: fix cleanup if cp_prefetch fails Dong Jia Shi
2018-04-23 11:38 ` Halil Pasic
@ 2018-04-23 11:40 ` Cornelia Huck
2018-04-23 12:00 ` Halil Pasic
2018-04-24 9:31 ` Cornelia Huck
2 siblings, 1 reply; 19+ messages in thread
From: Cornelia Huck @ 2018-04-23 11:40 UTC (permalink / raw)
To: Dong Jia Shi
Cc: linux-kernel, linux-s390, kvm, borntraeger, bjsdjshi, pasic,
pmorel, Halil Pasic
On Mon, 23 Apr 2018 13:01:09 +0200
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> From: Halil Pasic <pasic@linux.vnet.ibm.com>
>
> If the translation of a channel program fails, we may end up attempting
> to clean up (free, unpin) stuff that never got translated (and allocated,
> pinned) in the first place.
>
> By adjusting the lengths of the chains accordingly (so the element that
> failed, and all subsequent elements are excluded) cleanup activities
> based on false assumptions can be avoided.
>
> Let's make sure cp_free works properly after cp_prefetch returns with an
> error by setting ch_len of a ccw chain to the number of the translated
> CCWs on that chain.
Should that be cc:stable? This problem has been there probably since we
introduced vfio-ccw, no?
>
> Acked-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
> drivers/s390/cio/vfio_ccw_cp.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 2c7550797ec2..62d66e195304 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -715,6 +715,10 @@ void cp_free(struct channel_program *cp)
> * and stores the result to ccwchain list. @cp must have been
> * initialized by a previous call with cp_init(). Otherwise, undefined
> * behavior occurs.
> + * For each chain composing the channel program:
> + * - On entry ch_len holds the count of CCW to be translated.
> + * - On exit ch_len is adjusted to the count of successfully translated CCW.
> + * This allows cp_free to find in ch_len the count of CCW to free in a chain.
s/CCW/CCWs/ (x3)?
Can change on applying.
> *
> * The S/390 CCW Translation APIS (prefixed by 'cp_') are introduced
> * as helpers to do ccw chain translation inside the kernel. Basically
> @@ -749,11 +753,18 @@ int cp_prefetch(struct channel_program *cp)
> for (idx = 0; idx < len; idx++) {
> ret = ccwchain_fetch_one(chain, idx, cp);
> if (ret)
> - return ret;
> + goto out_err;
> }
> }
>
> return 0;
> +out_err:
> + /* Only cleanup the chain elements that were actually translated. */
> + chain->ch_len = idx;
> + list_for_each_entry_continue(chain, &cp->ccwchain_list, next) {
> + chain->ch_len = 0;
> + }
> + return ret;
> }
>
> /**
Else, looks good.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/5] vfio: ccw: fix cleanup if cp_prefetch fails
2018-04-23 11:40 ` Cornelia Huck
@ 2018-04-23 12:00 ` Halil Pasic
0 siblings, 0 replies; 19+ messages in thread
From: Halil Pasic @ 2018-04-23 12:00 UTC (permalink / raw)
To: Cornelia Huck, Dong Jia Shi
Cc: linux-kernel, linux-s390, kvm, borntraeger, bjsdjshi, pmorel,
Halil Pasic
On 04/23/2018 01:40 PM, Cornelia Huck wrote:
> On Mon, 23 Apr 2018 13:01:09 +0200
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
>
>> From: Halil Pasic <pasic@linux.vnet.ibm.com>
>>
>> If the translation of a channel program fails, we may end up attempting
>> to clean up (free, unpin) stuff that never got translated (and allocated,
>> pinned) in the first place.
>>
>> By adjusting the lengths of the chains accordingly (so the element that
>> failed, and all subsequent elements are excluded) cleanup activities
>> based on false assumptions can be avoided.
>>
>> Let's make sure cp_free works properly after cp_prefetch returns with an
>> error by setting ch_len of a ccw chain to the number of the translated
>> CCWs on that chain.
>
> Should that be cc:stable? This problem has been there probably since we
> introduced vfio-ccw, no?
>
Seems emails crossed in flight. Yes I think it should
be cc stable and this was broken since the very beginning.
>>
>> Acked-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>> ---
>> drivers/s390/cio/vfio_ccw_cp.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
>> index 2c7550797ec2..62d66e195304 100644
>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>> @@ -715,6 +715,10 @@ void cp_free(struct channel_program *cp)
>> * and stores the result to ccwchain list. @cp must have been
>> * initialized by a previous call with cp_init(). Otherwise, undefined
>> * behavior occurs.
>> + * For each chain composing the channel program:
>> + * - On entry ch_len holds the count of CCW to be translated.
>> + * - On exit ch_len is adjusted to the count of successfully translated CCW.
>> + * This allows cp_free to find in ch_len the count of CCW to free in a chain.
>
> s/CCW/CCWs/ (x3)?
>
Nod.
> Can change on applying.
>
>> *
>> * The S/390 CCW Translation APIS (prefixed by 'cp_') are introduced
>> * as helpers to do ccw chain translation inside the kernel. Basically
>> @@ -749,11 +753,18 @@ int cp_prefetch(struct channel_program *cp)
>> for (idx = 0; idx < len; idx++) {
>> ret = ccwchain_fetch_one(chain, idx, cp);
>> if (ret)
>> - return ret;
>> + goto out_err;
>> }
>> }
>>
>> return 0;
>> +out_err:
>> + /* Only cleanup the chain elements that were actually translated. */
>> + chain->ch_len = idx;
>> + list_for_each_entry_continue(chain, &cp->ccwchain_list, next) {
>> + chain->ch_len = 0;
>> + }
>> + return ret;
>> }
>>
>> /**
>
> Else, looks good.
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/5] vfio: ccw: fix cleanup if cp_prefetch fails
2018-04-23 11:01 ` [PATCH v2 1/5] vfio: ccw: fix cleanup if cp_prefetch fails Dong Jia Shi
2018-04-23 11:38 ` Halil Pasic
2018-04-23 11:40 ` Cornelia Huck
@ 2018-04-24 9:31 ` Cornelia Huck
[not found] ` <20180425024341.GY12194@bjsdjshi@linux.vnet.ibm.com>
2 siblings, 1 reply; 19+ messages in thread
From: Cornelia Huck @ 2018-04-24 9:31 UTC (permalink / raw)
To: Dong Jia Shi
Cc: linux-kernel, linux-s390, kvm, borntraeger, bjsdjshi, pasic,
pmorel, Halil Pasic
On Mon, 23 Apr 2018 13:01:09 +0200
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> From: Halil Pasic <pasic@linux.vnet.ibm.com>
>
> If the translation of a channel program fails, we may end up attempting
> to clean up (free, unpin) stuff that never got translated (and allocated,
> pinned) in the first place.
>
> By adjusting the lengths of the chains accordingly (so the element that
> failed, and all subsequent elements are excluded) cleanup activities
> based on false assumptions can be avoided.
>
> Let's make sure cp_free works properly after cp_prefetch returns with an
> error by setting ch_len of a ccw chain to the number of the translated
> CCWs on that chain.
>
> Acked-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
> drivers/s390/cio/vfio_ccw_cp.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
Thanks, applied. I'll probably send a pull req after lunch.
[Some procedural notes: I've created a new vfio-ccw-fixes branch based
on the s390 fixes branch for easier handling. Things targeted for the
next release will go on the vfio-ccw branch on top of the s390 features
branch, as before. Does that work for everybody? (And I am the only
vfio-ccw maintainer with a kernel.org account, right?)]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 2/5] vfio: ccw: shorten kernel doc description for pfn_array_pin()
2018-04-23 11:01 [PATCH v2 0/5] vfio: ccw: error handling fixes and improvements Dong Jia Shi
2018-04-23 11:01 ` [PATCH v2 1/5] vfio: ccw: fix cleanup if cp_prefetch fails Dong Jia Shi
@ 2018-04-23 11:01 ` Dong Jia Shi
2018-04-23 11:44 ` Cornelia Huck
2018-04-23 11:01 ` [PATCH v2 3/5] vfio: ccw: refactor and improve pfn_array_alloc_pin() Dong Jia Shi
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Dong Jia Shi @ 2018-04-23 11:01 UTC (permalink / raw)
To: linux-kernel, linux-s390, kvm
Cc: cohuck, borntraeger, bjsdjshi, pasic, pmorel, Dong Jia Shi
The kernel doc description for usage of the struct pfn_array in
pfn_array_pin() is unnecessary long. Let's shorten it by describing
the contents of the struct pfn_array fields at the struct's definition
instead.
Suggested-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
---
drivers/s390/cio/vfio_ccw_cp.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 62d66e195304..e8fe7450702e 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -23,9 +23,13 @@
#define CCWCHAIN_LEN_MAX 256
struct pfn_array {
+ /* Starting guest physical I/O address. */
unsigned long pa_iova;
+ /* Array that stores PFNs of the pages need to pin. */
unsigned long *pa_iova_pfn;
+ /* Array that receives PFNs of the pages pinned. */
unsigned long *pa_pfn;
+ /* Number of pages to pin/pinned from @pa_iova. */
int pa_nr;
};
@@ -53,14 +57,8 @@ struct ccwchain {
* Attempt to pin user pages in memory.
*
* Usage of pfn_array:
- * @pa->pa_iova starting guest physical I/O address. Assigned by caller.
- * @pa->pa_iova_pfn array that stores PFNs of the pages need to pin. Allocated
- * by caller.
- * @pa->pa_pfn array that receives PFNs of the pages pinned. Allocated by
- * caller.
- * @pa->pa_nr number of pages from @pa->pa_iova to pin. Assigned by
- * caller.
- * number of pages pinned. Assigned by callee.
+ * Any field in this structure should be initialized by caller.
+ * We expect @pa->pa_nr > 0, and its value will be assigned by callee.
*
* Returns:
* Number of pages pinned on success.
--
2.13.5
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 2/5] vfio: ccw: shorten kernel doc description for pfn_array_pin()
2018-04-23 11:01 ` [PATCH v2 2/5] vfio: ccw: shorten kernel doc description for pfn_array_pin() Dong Jia Shi
@ 2018-04-23 11:44 ` Cornelia Huck
0 siblings, 0 replies; 19+ messages in thread
From: Cornelia Huck @ 2018-04-23 11:44 UTC (permalink / raw)
To: Dong Jia Shi
Cc: linux-kernel, linux-s390, kvm, borntraeger, bjsdjshi, pasic,
pmorel
On Mon, 23 Apr 2018 13:01:10 +0200
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> The kernel doc description for usage of the struct pfn_array in
> pfn_array_pin() is unnecessary long. Let's shorten it by describing
> the contents of the struct pfn_array fields at the struct's definition
> instead.
>
> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
> drivers/s390/cio/vfio_ccw_cp.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 62d66e195304..e8fe7450702e 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -23,9 +23,13 @@
> #define CCWCHAIN_LEN_MAX 256
>
> struct pfn_array {
> + /* Starting guest physical I/O address. */
> unsigned long pa_iova;
> + /* Array that stores PFNs of the pages need to pin. */
s/need to pin/needing to be pinned/ ? Or "we need to pin"? Or even
simply "to pin"?
(Pre-exiting, but we can as well fix it up. Can also do while applying.)
> unsigned long *pa_iova_pfn;
> + /* Array that receives PFNs of the pages pinned. */
> unsigned long *pa_pfn;
> + /* Number of pages to pin/pinned from @pa_iova. */
> int pa_nr;
> };
>
> @@ -53,14 +57,8 @@ struct ccwchain {
> * Attempt to pin user pages in memory.
> *
> * Usage of pfn_array:
> - * @pa->pa_iova starting guest physical I/O address. Assigned by caller.
> - * @pa->pa_iova_pfn array that stores PFNs of the pages need to pin. Allocated
> - * by caller.
> - * @pa->pa_pfn array that receives PFNs of the pages pinned. Allocated by
> - * caller.
> - * @pa->pa_nr number of pages from @pa->pa_iova to pin. Assigned by
> - * caller.
> - * number of pages pinned. Assigned by callee.
> + * Any field in this structure should be initialized by caller.
> + * We expect @pa->pa_nr > 0, and its value will be assigned by callee.
> *
> * Returns:
> * Number of pages pinned on success.
Otherwise, looks good.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 3/5] vfio: ccw: refactor and improve pfn_array_alloc_pin()
2018-04-23 11:01 [PATCH v2 0/5] vfio: ccw: error handling fixes and improvements Dong Jia Shi
2018-04-23 11:01 ` [PATCH v2 1/5] vfio: ccw: fix cleanup if cp_prefetch fails Dong Jia Shi
2018-04-23 11:01 ` [PATCH v2 2/5] vfio: ccw: shorten kernel doc description for pfn_array_pin() Dong Jia Shi
@ 2018-04-23 11:01 ` Dong Jia Shi
2018-04-23 11:01 ` [PATCH v2 4/5] vfio: ccw: set ccw->cda to NULL defensively Dong Jia Shi
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Dong Jia Shi @ 2018-04-23 11:01 UTC (permalink / raw)
To: linux-kernel, linux-s390, kvm
Cc: cohuck, borntraeger, bjsdjshi, pasic, pmorel, Dong Jia Shi
This refactors pfn_array_alloc_pin() and also improves it by adding
defensive code in error handling so that calling pfn_array_unpin_free()
after error return won't lead to problem. This mainly does:
1. Merge pfn_array_pin() into pfn_array_alloc_pin(), since there is no
other user of pfn_array_pin(). As a result, also remove kernel-doc
for pfn_array_pin() and add/update kernel-doc for pfn_array_alloc_pin()
and struct pfn_array.
2. For a vfio_pin_pages() failure, set pa->pa_nr to zero to indicate
zero pages were pinned.
3. Set pa->pa_iova_pfn to NULL right after it was freed.
Suggested-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 | 82 +++++++++++++++++++-----------------------
1 file changed, 36 insertions(+), 46 deletions(-)
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index e8fe7450702e..340804b45f6a 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -29,7 +29,7 @@ struct pfn_array {
unsigned long *pa_iova_pfn;
/* Array that receives PFNs of the pages pinned. */
unsigned long *pa_pfn;
- /* Number of pages to pin/pinned from @pa_iova. */
+ /* Number of pages pinned from @pa_iova. */
int pa_nr;
};
@@ -50,64 +50,33 @@ struct ccwchain {
};
/*
- * pfn_array_pin() - pin user pages in memory
+ * pfn_array_alloc_pin() - alloc memory for PFNs, then pin user pages in memory
* @pa: pfn_array on which to perform the operation
* @mdev: the mediated device to perform pin/unpin operations
+ * @iova: target guest physical address
+ * @len: number of bytes that should be pinned from @iova
*
- * Attempt to pin user pages in memory.
+ * Attempt to allocate memory for PFNs, and pin user pages in memory.
*
* Usage of pfn_array:
- * Any field in this structure should be initialized by caller.
- * We expect @pa->pa_nr > 0, and its value will be assigned by callee.
+ * We expect (pa_nr == 0) and (pa_iova_pfn == NULL), any field in
+ * this structure will be filled in by this function.
*
* Returns:
* Number of pages pinned on success.
- * If @pa->pa_nr is 0 or negative, returns 0.
+ * If @pa->pa_nr is not 0, or @pa->pa_iova_pfn is not NULL initially,
+ * returns -EINVAL.
* If no pages were pinned, returns -errno.
*/
-static int pfn_array_pin(struct pfn_array *pa, struct device *mdev)
-{
- int i, ret;
-
- if (pa->pa_nr <= 0) {
- pa->pa_nr = 0;
- return 0;
- }
-
- pa->pa_iova_pfn[0] = pa->pa_iova >> PAGE_SHIFT;
- for (i = 1; i < pa->pa_nr; i++)
- pa->pa_iova_pfn[i] = pa->pa_iova_pfn[i - 1] + 1;
-
- ret = vfio_pin_pages(mdev, pa->pa_iova_pfn, pa->pa_nr,
- IOMMU_READ | IOMMU_WRITE, pa->pa_pfn);
-
- if (ret > 0 && ret != pa->pa_nr) {
- vfio_unpin_pages(mdev, pa->pa_iova_pfn, ret);
- pa->pa_nr = 0;
- return 0;
- }
-
- return ret;
-}
-
-/* Unpin the pages before releasing the memory. */
-static void pfn_array_unpin_free(struct pfn_array *pa, struct device *mdev)
-{
- vfio_unpin_pages(mdev, pa->pa_iova_pfn, pa->pa_nr);
- pa->pa_nr = 0;
- kfree(pa->pa_iova_pfn);
-}
-
-/* Alloc memory for PFNs, then pin pages with them. */
static int pfn_array_alloc_pin(struct pfn_array *pa, struct device *mdev,
u64 iova, unsigned int len)
{
- int ret = 0;
+ int i, ret = 0;
if (!len)
return 0;
- if (pa->pa_nr)
+ if (pa->pa_nr || pa->pa_iova_pfn)
return -EINVAL;
pa->pa_iova = iova;
@@ -124,18 +93,39 @@ static int pfn_array_alloc_pin(struct pfn_array *pa, struct device *mdev,
return -ENOMEM;
pa->pa_pfn = pa->pa_iova_pfn + pa->pa_nr;
- ret = pfn_array_pin(pa, mdev);
+ pa->pa_iova_pfn[0] = pa->pa_iova >> PAGE_SHIFT;
+ for (i = 1; i < pa->pa_nr; i++)
+ pa->pa_iova_pfn[i] = pa->pa_iova_pfn[i - 1] + 1;
+
+ ret = vfio_pin_pages(mdev, pa->pa_iova_pfn, pa->pa_nr,
+ IOMMU_READ | IOMMU_WRITE, pa->pa_pfn);
- if (ret > 0)
- return ret;
- else if (!ret)
+ if (ret < 0) {
+ goto err_out;
+ } else if (ret > 0 && ret != pa->pa_nr) {
+ vfio_unpin_pages(mdev, pa->pa_iova_pfn, ret);
ret = -EINVAL;
+ goto err_out;
+ }
+
+ return ret;
+err_out:
+ pa->pa_nr = 0;
kfree(pa->pa_iova_pfn);
+ pa->pa_iova_pfn = NULL;
return ret;
}
+/* Unpin the pages before releasing the memory. */
+static void pfn_array_unpin_free(struct pfn_array *pa, struct device *mdev)
+{
+ vfio_unpin_pages(mdev, pa->pa_iova_pfn, pa->pa_nr);
+ pa->pa_nr = 0;
+ kfree(pa->pa_iova_pfn);
+}
+
static int pfn_array_table_init(struct pfn_array_table *pat, int nr)
{
pat->pat_pa = kcalloc(nr, sizeof(*pat->pat_pa), GFP_KERNEL);
--
2.13.5
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 4/5] vfio: ccw: set ccw->cda to NULL defensively
2018-04-23 11:01 [PATCH v2 0/5] vfio: ccw: error handling fixes and improvements Dong Jia Shi
` (2 preceding siblings ...)
2018-04-23 11:01 ` [PATCH v2 3/5] vfio: ccw: refactor and improve pfn_array_alloc_pin() Dong Jia Shi
@ 2018-04-23 11:01 ` Dong Jia Shi
2018-04-23 11:01 ` [PATCH v2 5/5] vfio: ccw: add traceponits for interesting error paths Dong Jia Shi
2018-04-23 11:32 ` [PATCH v2 0/5] vfio: ccw: error handling fixes and improvements Cornelia Huck
5 siblings, 0 replies; 19+ messages in thread
From: Dong Jia Shi @ 2018-04-23 11:01 UTC (permalink / raw)
To: linux-kernel, linux-s390, kvm
Cc: cohuck, borntraeger, bjsdjshi, pasic, pmorel, Dong Jia Shi
Let's avoid free on ccw->cda that points to a guest address
or an already freed memory area by setting it to NULL if memory
allocation didn't happen or failed.
Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
---
drivers/s390/cio/vfio_ccw_cp.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 340804b45f6a..1eef102c685e 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -491,7 +491,7 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
struct ccw1 *ccw;
struct pfn_array_table *pat;
unsigned long *idaws;
- int idaw_nr;
+ int ret;
ccw = chain->ch_ccw + idx;
@@ -511,18 +511,19 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
* needed when translating a direct ccw to a idal ccw.
*/
pat = chain->ch_pat + idx;
- if (pfn_array_table_init(pat, 1))
- return -ENOMEM;
- idaw_nr = pfn_array_alloc_pin(pat->pat_pa, cp->mdev,
- ccw->cda, ccw->count);
- if (idaw_nr < 0)
- return idaw_nr;
+ ret = pfn_array_table_init(pat, 1);
+ if (ret)
+ goto out_init;
+
+ ret = pfn_array_alloc_pin(pat->pat_pa, cp->mdev, ccw->cda, ccw->count);
+ if (ret < 0)
+ goto out_init;
/* Translate this direct ccw to a idal ccw. */
- idaws = kcalloc(idaw_nr, sizeof(*idaws), GFP_DMA | GFP_KERNEL);
+ idaws = kcalloc(ret, sizeof(*idaws), GFP_DMA | GFP_KERNEL);
if (!idaws) {
- pfn_array_table_unpin_free(pat, cp->mdev);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out_unpin;
}
ccw->cda = (__u32) virt_to_phys(idaws);
ccw->flags |= CCW_FLAG_IDA;
@@ -530,6 +531,12 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
pfn_array_table_idal_create_words(pat, idaws);
return 0;
+
+out_unpin:
+ pfn_array_table_unpin_free(pat, cp->mdev);
+out_init:
+ ccw->cda = 0;
+ return ret;
}
static int ccwchain_fetch_idal(struct ccwchain *chain,
@@ -559,7 +566,7 @@ static int ccwchain_fetch_idal(struct ccwchain *chain,
pat = chain->ch_pat + idx;
ret = pfn_array_table_init(pat, idaw_nr);
if (ret)
- return ret;
+ goto out_init;
/* Translate idal ccw to use new allocated idaws. */
idaws = kzalloc(idaw_len, GFP_DMA | GFP_KERNEL);
@@ -591,6 +598,8 @@ static int ccwchain_fetch_idal(struct ccwchain *chain,
kfree(idaws);
out_unpin:
pfn_array_table_unpin_free(pat, cp->mdev);
+out_init:
+ ccw->cda = 0;
return ret;
}
--
2.13.5
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 5/5] vfio: ccw: add traceponits for interesting error paths
2018-04-23 11:01 [PATCH v2 0/5] vfio: ccw: error handling fixes and improvements Dong Jia Shi
` (3 preceding siblings ...)
2018-04-23 11:01 ` [PATCH v2 4/5] vfio: ccw: set ccw->cda to NULL defensively Dong Jia Shi
@ 2018-04-23 11:01 ` Dong Jia Shi
2018-04-27 10:13 ` Cornelia Huck
2018-04-23 11:32 ` [PATCH v2 0/5] vfio: ccw: error handling fixes and improvements Cornelia Huck
5 siblings, 1 reply; 19+ messages in thread
From: Dong Jia Shi @ 2018-04-23 11:01 UTC (permalink / raw)
To: linux-kernel, linux-s390, kvm
Cc: cohuck, borntraeger, bjsdjshi, pasic, pmorel, Halil Pasic,
Dong Jia Shi
From: Halil Pasic <pasic@linux.vnet.ibm.com>
Add some tracepoints so we can inspect what is not working as is should.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
---
drivers/s390/cio/Makefile | 1 +
drivers/s390/cio/vfio_ccw_fsm.c | 16 +++++++-
drivers/s390/cio/vfio_ccw_trace.h | 77 +++++++++++++++++++++++++++++++++++++++
3 files changed, 93 insertions(+), 1 deletion(-)
create mode 100644 drivers/s390/cio/vfio_ccw_trace.h
diff --git a/drivers/s390/cio/Makefile b/drivers/s390/cio/Makefile
index a070ef0efe65..f230516abb96 100644
--- a/drivers/s390/cio/Makefile
+++ b/drivers/s390/cio/Makefile
@@ -5,6 +5,7 @@
# The following is required for define_trace.h to find ./trace.h
CFLAGS_trace.o := -I$(src)
+CFLAGS_vfio_ccw_fsm.o := -I$(src)
obj-y += airq.o blacklist.o chsc.o cio.o css.o chp.o idset.o isc.o \
fcx.o itcw.o crw.o ccwreq.o trace.o ioasm.o
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index ff6963ad6e39..90cab4e1436e 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -13,6 +13,9 @@
#include "ioasm.h"
#include "vfio_ccw_private.h"
+#define CREATE_TRACE_POINTS
+#include "vfio_ccw_trace.h"
+
static int fsm_io_helper(struct vfio_ccw_private *private)
{
struct subchannel *sch;
@@ -105,6 +108,10 @@ static void fsm_disabled_irq(struct vfio_ccw_private *private,
*/
cio_disable_subchannel(sch);
}
+inline struct subchannel_id get_schid(struct vfio_ccw_private *p)
+{
+ return p->sch->schid;
+}
/*
* Deal with the ccw command request from the userspace.
@@ -135,6 +142,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
goto err_out;
io_region->ret_code = cp_prefetch(&private->cp);
+ trace_vfio_ccw_cp_prefetch(get_schid(private),
+ io_region->ret_code);
if (io_region->ret_code) {
cp_free(&private->cp);
goto err_out;
@@ -142,11 +151,13 @@ static void fsm_io_request(struct vfio_ccw_private *private,
/* Start channel program and wait for I/O interrupt. */
io_region->ret_code = fsm_io_helper(private);
+ trace_vfio_ccw_fsm_io_helper(get_schid(private),
+ io_region->ret_code);
if (io_region->ret_code) {
cp_free(&private->cp);
goto err_out;
}
- return;
+ goto out;
} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
/* XXX: Handle halt. */
io_region->ret_code = -EOPNOTSUPP;
@@ -159,6 +170,9 @@ static void fsm_io_request(struct vfio_ccw_private *private,
err_out:
private->state = VFIO_CCW_STATE_IDLE;
+out:
+ trace_vfio_ccw_io_fctl(scsw->cmd.fctl, get_schid(private),
+ io_region->ret_code);
}
/*
diff --git a/drivers/s390/cio/vfio_ccw_trace.h b/drivers/s390/cio/vfio_ccw_trace.h
new file mode 100644
index 000000000000..25128331c285
--- /dev/null
+++ b/drivers/s390/cio/vfio_ccw_trace.h
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: GPL-2.0
+ * Tracepoints for vfio_ccw driver
+ *
+ * Copyright IBM Corp. 2018
+ *
+ * Author(s): Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
+ * Halil Pasic <pasic@linux.vnet.ibm.com>
+ */
+
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM vfio_ccw
+
+#if !defined(_VFIO_CCW_TRACE_) || defined(TRACE_HEADER_MULTI_READ)
+#define _VFIO_CCW_TRACE_
+
+#include <linux/tracepoint.h>
+
+DECLARE_EVENT_CLASS(vfio_ccw_schid_errno,
+ TP_PROTO(struct subchannel_id schid, int errno),
+ TP_ARGS(schid, errno),
+
+ TP_STRUCT__entry(
+ __field_struct(struct subchannel_id, schid)
+ __field(int, errno)
+ ),
+
+ TP_fast_assign(
+ __entry->schid = schid;
+ __entry->errno = errno;
+ ),
+
+ TP_printk("schid=%x.%x.%04x errno=%d", __entry->schid.cssid,
+ __entry->schid.ssid, __entry->schid.sch_no, __entry->errno)
+);
+
+DEFINE_EVENT(vfio_ccw_schid_errno, vfio_ccw_cp_prefetch,
+ TP_PROTO(struct subchannel_id schid, int errno),
+ TP_ARGS(schid, errno)
+);
+
+DEFINE_EVENT(vfio_ccw_schid_errno, vfio_ccw_fsm_io_helper,
+ TP_PROTO(struct subchannel_id schid, int errno),
+ TP_ARGS(schid, errno)
+);
+
+TRACE_EVENT(vfio_ccw_io_fctl,
+ TP_PROTO(int fctl, struct subchannel_id schid, int errno),
+ TP_ARGS(fctl, schid, errno),
+
+ TP_STRUCT__entry(
+ __field(int, fctl)
+ __field_struct(struct subchannel_id, schid)
+ __field(int, errno)
+ ),
+
+ TP_fast_assign(
+ __entry->fctl = fctl;
+ __entry->schid = schid;
+ __entry->errno = errno;
+ ),
+
+ TP_printk("schid=%x.%x.%04x fctl=%x errno=%d", __entry->schid.cssid,
+ __entry->schid.ssid, __entry->schid.sch_no,
+ __entry->fctl, __entry->errno)
+);
+
+#endif /* _VFIO_CCW_TRACE_ */
+
+/* This part must be outside protection */
+
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE vfio_ccw_trace
+
+#include <trace/define_trace.h>
--
2.13.5
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 5/5] vfio: ccw: add traceponits for interesting error paths
2018-04-23 11:01 ` [PATCH v2 5/5] vfio: ccw: add traceponits for interesting error paths Dong Jia Shi
@ 2018-04-27 10:13 ` Cornelia Huck
[not found] ` <20180428055023.GS5428@bjsdjshi@linux.vnet.ibm.com>
0 siblings, 1 reply; 19+ messages in thread
From: Cornelia Huck @ 2018-04-27 10:13 UTC (permalink / raw)
To: Dong Jia Shi
Cc: linux-kernel, linux-s390, kvm, borntraeger, bjsdjshi, pasic,
pmorel, Halil Pasic
On Mon, 23 Apr 2018 13:01:13 +0200
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
typo in subject: s/traceponits/tracepoints/
> From: Halil Pasic <pasic@linux.vnet.ibm.com>
>
> Add some tracepoints so we can inspect what is not working as is should.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
> drivers/s390/cio/Makefile | 1 +
> drivers/s390/cio/vfio_ccw_fsm.c | 16 +++++++-
> drivers/s390/cio/vfio_ccw_trace.h | 77 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 93 insertions(+), 1 deletion(-)
> create mode 100644 drivers/s390/cio/vfio_ccw_trace.h
> @@ -135,6 +142,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
> goto err_out;
>
> io_region->ret_code = cp_prefetch(&private->cp);
> + trace_vfio_ccw_cp_prefetch(get_schid(private),
> + io_region->ret_code);
> if (io_region->ret_code) {
> cp_free(&private->cp);
> goto err_out;
> @@ -142,11 +151,13 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>
> /* Start channel program and wait for I/O interrupt. */
> io_region->ret_code = fsm_io_helper(private);
> + trace_vfio_ccw_fsm_io_helper(get_schid(private),
> + io_region->ret_code);
> if (io_region->ret_code) {
> cp_free(&private->cp);
> goto err_out;
> }
> - return;
> + goto out;
> } else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
> /* XXX: Handle halt. */
> io_region->ret_code = -EOPNOTSUPP;
> @@ -159,6 +170,9 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>
> err_out:
> private->state = VFIO_CCW_STATE_IDLE;
> +out:
> + trace_vfio_ccw_io_fctl(scsw->cmd.fctl, get_schid(private),
> + io_region->ret_code);
> }
>
> /*
I really don't want to bikeshed, especially as some tracepoints are
better than no tracepoints, but...
We now trace fctl/schid/ret_code unconditionally (good).
We trace the outcome of cp_prefetch() and fsm_io_helper()
unconditionally. We don't, however, trace all things that may go wrong.
We have the tracepoint at the end, but it cannot tell us where the
error came from. Should we have tracepoints in every place (in this
function) that may generate an error? Only if there is an actual error?
Are the two enough for common debug scenarios?
Opinions? We can just go ahead with this and improve things later on, I
guess.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/5] vfio: ccw: error handling fixes and improvements
2018-04-23 11:01 [PATCH v2 0/5] vfio: ccw: error handling fixes and improvements Dong Jia Shi
` (4 preceding siblings ...)
2018-04-23 11:01 ` [PATCH v2 5/5] vfio: ccw: add traceponits for interesting error paths Dong Jia Shi
@ 2018-04-23 11:32 ` Cornelia Huck
5 siblings, 0 replies; 19+ messages in thread
From: Cornelia Huck @ 2018-04-23 11:32 UTC (permalink / raw)
To: Dong Jia Shi
Cc: linux-kernel, linux-s390, kvm, borntraeger, bjsdjshi, pasic,
pmorel
On Mon, 23 Apr 2018 13:01:08 +0200
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> Dear Reviewers,
>
> Here is a new version for this patch series.
>
> We didn't get agreement on patch #5 (#4 in v1) in the former cycle though,
> I made it based on my understanding. We can continue discussing on it.
>
> Changelog:
> v1->v2:
> - #1. Reworded commit message and comment, plus some typo fixes.
> - #2. New patch.
> - #3. Added the missing suggested-by Pierre.
> Fixed typos.
> Added sanity check on pa->pa_iova_pfn and updated comments accordingly.
> - #4. Removed unused idaw_nr.
> - #5. Replaced leading white spaces with TABs.
> Traced the function in anycase.
>
> Dong Jia Shi (3):
> vfio: ccw: shorten kernel doc description for pfn_array_pin()
> vfio: ccw: refactor and improve pfn_array_alloc_pin()
> vfio: ccw: set ccw->cda to NULL defensively
>
> Halil Pasic (2):
> vfio: ccw: fix cleanup if cp_prefetch fails
> vfio: ccw: add traceponits for interesting error paths
>
> drivers/s390/cio/Makefile | 1 +
> drivers/s390/cio/vfio_ccw_cp.c | 134 ++++++++++++++++++++------------------
> drivers/s390/cio/vfio_ccw_fsm.c | 16 ++++-
> drivers/s390/cio/vfio_ccw_trace.h | 77 ++++++++++++++++++++++
> 4 files changed, 164 insertions(+), 64 deletions(-)
> create mode 100644 drivers/s390/cio/vfio_ccw_trace.h
>
Out of this series, patch 1 is a fix, while the rest are improvements,
correct? So patch 1 would be material for 4.17 (and maybe stable?), the
rest for 4.18?
^ permalink raw reply [flat|nested] 19+ messages in thread