[RFC v1 0/4] Some vfio-ccw fixes

[RFC v1 0/4] Some vfio-ccw fixes

[Farhan Ali]

Hi,

While trying to chase down the problem regarding the stacktraces,
I have also found some minor problems in the code.

Would appreciate any review or feedback regarding them.

Thanks
Farhan 

Farhan Ali (4):
  vfio-ccw: Set orb.cmd.c64 before calling ccwchain_handle_ccw
  vfio-ccw: No need to call cp_free on an error in cp_init
  vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn
  vfio-ccw: Don't call cp_free if we are processing a channel program

 drivers/s390/cio/vfio_ccw_cp.c  | 12 ++++++------
 drivers/s390/cio/vfio_ccw_drv.c |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.7.4

[RFC v1 1/4] vfio-ccw: Set orb.cmd.c64 before calling ccwchain_handle_ccw

[Farhan Ali]

Because ccwchain_handle_ccw calls ccwchain_calc_length and
as per the comment we should set orb.cmd.c64 before calling
ccwchanin_calc_length.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index d6a8dff..5ac4c1e 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -640,16 +640,16 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
 	memcpy(&cp->orb, orb, sizeof(*orb));
 	cp->mdev = mdev;
 
-	/* Build a ccwchain for the first CCW segment */
-	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
-	if (ret)
-		cp_free(cp);
-
 	/* It is safe to force: if not set but idals used
 	 * ccwchain_calc_length returns an error.
 	 */
 	cp->orb.cmd.c64 = 1;
 
+	/* Build a ccwchain for the first CCW segment */
+	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
+	if (ret)
+		cp_free(cp);
+
 	if (!ret)
 		cp->initialized = true;
 
-- 
2.7.4

[RFC v1 2/4] vfio-ccw: No need to call cp_free on an error in cp_init

[Farhan Ali]

We don't set cp->initialized to true so calling cp_free
will just return and not do anything.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 5ac4c1e..cab1be9 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
 
 	/* Build a ccwchain for the first CCW segment */
 	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
-	if (ret)
-		cp_free(cp);
 
 	if (!ret)
 		cp->initialized = true;
-- 
2.7.4

[RFC v1 3/4] vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn

[Farhan Ali]

So we clean up correctly.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index cab1be9..c5655de 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -72,8 +72,10 @@ static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len)
 				  sizeof(*pa->pa_iova_pfn) +
 				  sizeof(*pa->pa_pfn),
 				  GFP_KERNEL);
-	if (unlikely(!pa->pa_iova_pfn))
+	if (unlikely(!pa->pa_iova_pfn)) {
+		pa->pa_nr = 0;
 		return -ENOMEM;
+	}
 	pa->pa_pfn = pa->pa_iova_pfn + pa->pa_nr;
 
 	pa->pa_iova_pfn[0] = pa->pa_iova >> PAGE_SHIFT;
-- 
2.7.4

[RFC v1 4/4] vfio-ccw: Don't call cp_free if we are processing a channel program

[Farhan Ali]

There is a small window where it's possible that we could be working
on an interrupt (queued in the workqueue) and setting up a channel
program (i.e allocating memory, pinning pages, translating address).
This can lead to allocating and freeing the channel program at the
same time and can cause memory corruption.

Let's not call cp_free if we are currently processing a channel program.
The only way we know for sure that we don't have a thread setting
up a channel program is when the state is set to VFIO_CCW_STATE_CP_PENDING.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 4e3a903..0357165 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -92,7 +92,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
 		     (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
 	if (scsw_is_solicited(&irb->scsw)) {
 		cp_update_scsw(&private->cp, &irb->scsw);
-		if (is_final)
+		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)
 			cp_free(&private->cp);
 	}
 	mutex_lock(&private->io_mutex);
-- 
2.7.4

Re: [RFC v1 1/4] vfio-ccw: Set orb.cmd.c64 before calling ccwchain_handle_ccw

[Cornelia Huck]

On Mon,  1 Jul 2019 12:23:43 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> Because ccwchain_handle_ccw calls ccwchain_calc_length and
> as per the comment we should set orb.cmd.c64 before calling
> ccwchanin_calc_length.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index d6a8dff..5ac4c1e 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -640,16 +640,16 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
>  	memcpy(&cp->orb, orb, sizeof(*orb));
>  	cp->mdev = mdev;
>  
> -	/* Build a ccwchain for the first CCW segment */
> -	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
> -	if (ret)
> -		cp_free(cp);
> -
>  	/* It is safe to force: if not set but idals used
>  	 * ccwchain_calc_length returns an error.
>  	 */
>  	cp->orb.cmd.c64 = 1;
>  
> +	/* Build a ccwchain for the first CCW segment */
> +	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
> +	if (ret)
> +		cp_free(cp);
> +
>  	if (!ret)
>  		cp->initialized = true;
>  

Hm... has this ever been correct, or did this break only with the
recent refactorings?

(IOW, what should Fixes: point to?)

Re: [RFC v1 2/4] vfio-ccw: No need to call cp_free on an error in cp_init

[Cornelia Huck]

On Mon,  1 Jul 2019 12:23:44 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> We don't set cp->initialized to true so calling cp_free
> will just return and not do anything.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 5ac4c1e..cab1be9 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
>  
>  	/* Build a ccwchain for the first CCW segment */
>  	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
> -	if (ret)
> -		cp_free(cp);

Makes sense; hopefully ccwchain_handle_ccw() cleans up correctly on
error :) (I think it does)

Maybe add a comment

/* ccwchain_handle_ccw() already cleans up on error */

so we don't stumble over this in the future?

(Also, does this want a Fixes: tag?)

>  
>  	if (!ret)
>  		cp->initialized = true;

Re: [RFC v1 3/4] vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn

[Cornelia Huck]

On Mon,  1 Jul 2019 12:23:45 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> So we clean up correctly.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index cab1be9..c5655de 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -72,8 +72,10 @@ static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len)
>  				  sizeof(*pa->pa_iova_pfn) +
>  				  sizeof(*pa->pa_pfn),
>  				  GFP_KERNEL);
> -	if (unlikely(!pa->pa_iova_pfn))
> +	if (unlikely(!pa->pa_iova_pfn)) {
> +		pa->pa_nr = 0;
>  		return -ENOMEM;
> +	}
>  	pa->pa_pfn = pa->pa_iova_pfn + pa->pa_nr;
>  
>  	pa->pa_iova_pfn[0] = pa->pa_iova >> PAGE_SHIFT;

This looks like an older error -- can you give a Fixes: tag? (Yeah, I
know I sound like a broken record wrt that tag... :)

Re: [RFC v1 4/4] vfio-ccw: Don't call cp_free if we are processing a channel program

[Cornelia Huck]

On Mon,  1 Jul 2019 12:23:46 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> There is a small window where it's possible that we could be working
> on an interrupt (queued in the workqueue) and setting up a channel
> program (i.e allocating memory, pinning pages, translating address).
> This can lead to allocating and freeing the channel program at the
> same time and can cause memory corruption.

This can only happen if the interrupt is for a halt/clear operation,
right?

> 
> Let's not call cp_free if we are currently processing a channel program.
> The only way we know for sure that we don't have a thread setting
> up a channel program is when the state is set to VFIO_CCW_STATE_CP_PENDING.

I have looked through the code again and I think you are right.

> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 4e3a903..0357165 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -92,7 +92,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>  		     (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
>  	if (scsw_is_solicited(&irb->scsw)) {
>  		cp_update_scsw(&private->cp, &irb->scsw);
> -		if (is_final)
> +		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)

Do we actually want to call cp_update_scsw() unconditionally?

At this point, we know that we have a solicited interrupt; that may be
for several reasons:
- Interrupt for something we issued via ssch; it makes sense to update
  the scsw with the cpa address.
- Interrupt for a csch; the cpa address will be unpredictable, even if
  we did a ssch before. cp_update_scsw() hopefully can deal with that?
  Given that its purpose is to translate the cpa back, any
  unpredictable value in the scsw should be fine in the end.
- Interrupt for a hsch after we did a ssch; the cpa might be valid (see
  figure 16-6).
- Interrupt for a hsch without a prior ssch; we'll end up with an
  unpredictable cpa, again.

So I *think* we're fine with calling cp_update_scsw() in all cases,
even if there's junk in the cpa of the scsw we get from the hardware.
Opinions?

>  			cp_free(&private->cp);
>  	}
>  	mutex_lock(&private->io_mutex);

Re: [RFC v1 1/4] vfio-ccw: Set orb.cmd.c64 before calling ccwchain_handle_ccw

[Farhan Ali]

On 07/02/2019 04:26 AM, Cornelia Huck wrote:
> On Mon,  1 Jul 2019 12:23:43 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> Because ccwchain_handle_ccw calls ccwchain_calc_length and
>> as per the comment we should set orb.cmd.c64 before calling
>> ccwchanin_calc_length.
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_cp.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
>> index d6a8dff..5ac4c1e 100644
>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>> @@ -640,16 +640,16 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
>>   	memcpy(&cp->orb, orb, sizeof(*orb));
>>   	cp->mdev = mdev;
>>   
>> -	/* Build a ccwchain for the first CCW segment */
>> -	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
>> -	if (ret)
>> -		cp_free(cp);
>> -
>>   	/* It is safe to force: if not set but idals used
>>   	 * ccwchain_calc_length returns an error.
>>   	 */
>>   	cp->orb.cmd.c64 = 1;
>>   
>> +	/* Build a ccwchain for the first CCW segment */
>> +	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
>> +	if (ret)
>> +		cp_free(cp);
>> +
>>   	if (!ret)
>>   		cp->initialized = true;
>>   
> 
> Hm... has this ever been correct, or did this break only with the
> recent refactorings?
> 
> (IOW, what should Fixes: point to?)
> 
> 

I think it was correct before some of the new refactoring we did. But we 
do need to set before calling ccwchain_calc_length, because the function 
does have a check for orb.cmd.64. I will see which exact commit did it.

Thanks
Farhan

Re: [RFC v1 2/4] vfio-ccw: No need to call cp_free on an error in cp_init

[Farhan Ali]

On 07/02/2019 04:42 AM, Cornelia Huck wrote:
> On Mon,  1 Jul 2019 12:23:44 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> We don't set cp->initialized to true so calling cp_free
>> will just return and not do anything.
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_cp.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
>> index 5ac4c1e..cab1be9 100644
>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>> @@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
>>   
>>   	/* Build a ccwchain for the first CCW segment */
>>   	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
>> -	if (ret)
>> -		cp_free(cp);
> 
> Makes sense; hopefully ccwchain_handle_ccw() cleans up correctly on
> error :) (I think it does)
> 

I have checked that it does as well, but wouldn't hurt if someone else 
also glances over once again :)

> Maybe add a comment
> 
> /* ccwchain_handle_ccw() already cleans up on error */
> 
> so we don't stumble over this in the future?

Sure.

> 
> (Also, does this want a Fixes: tag?)

This might warrant a fixes tag as well.
> 
>>   
>>   	if (!ret)
>>   		cp->initialized = true;
> 
> 

Re: [RFC v1 3/4] vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn

[Farhan Ali]

On 07/02/2019 04:45 AM, Cornelia Huck wrote:
> On Mon,  1 Jul 2019 12:23:45 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> So we clean up correctly.
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_cp.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
>> index cab1be9..c5655de 100644
>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>> @@ -72,8 +72,10 @@ static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len)
>>   				  sizeof(*pa->pa_iova_pfn) +
>>   				  sizeof(*pa->pa_pfn),
>>   				  GFP_KERNEL);
>> -	if (unlikely(!pa->pa_iova_pfn))
>> +	if (unlikely(!pa->pa_iova_pfn)) {
>> +		pa->pa_nr = 0;
>>   		return -ENOMEM;
>> +	}
>>   	pa->pa_pfn = pa->pa_iova_pfn + pa->pa_nr;
>>   
>>   	pa->pa_iova_pfn[0] = pa->pa_iova >> PAGE_SHIFT;
> 
> This looks like an older error -- can you give a Fixes: tag? (Yeah, I
> know I sound like a broken record wrt that tag... :)
> 
Yes, this is an older error. And yup I will add a fixes tag :)

Re: [RFC v1 1/4] vfio-ccw: Set orb.cmd.c64 before calling ccwchain_handle_ccw

[Eric Farman]

On 7/2/19 9:56 AM, Farhan Ali wrote:
> 
> 
> On 07/02/2019 04:26 AM, Cornelia Huck wrote:
>> On Mon,  1 Jul 2019 12:23:43 -0400
>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>
>>> Because ccwchain_handle_ccw calls ccwchain_calc_length and
>>> as per the comment we should set orb.cmd.c64 before calling
>>> ccwchanin_calc_length.
>>>
>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>>> ---
>>>   drivers/s390/cio/vfio_ccw_cp.c | 10 +++++-----
>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c
>>> b/drivers/s390/cio/vfio_ccw_cp.c
>>> index d6a8dff..5ac4c1e 100644
>>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>>> @@ -640,16 +640,16 @@ int cp_init(struct channel_program *cp, struct
>>> device *mdev, union orb *orb)
>>>       memcpy(&cp->orb, orb, sizeof(*orb));
>>>       cp->mdev = mdev;
>>>   -    /* Build a ccwchain for the first CCW segment */
>>> -    ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
>>> -    if (ret)
>>> -        cp_free(cp);
>>> -
>>>       /* It is safe to force: if not set but idals used
>>>        * ccwchain_calc_length returns an error.
>>>        */
>>>       cp->orb.cmd.c64 = 1;
>>>   +    /* Build a ccwchain for the first CCW segment */
>>> +    ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
>>> +    if (ret)
>>> +        cp_free(cp);
>>> +
>>>       if (!ret)
>>>           cp->initialized = true;
>>>   
>>
>> Hm... has this ever been correct, or did this break only with the
>> recent refactorings?
>>
>> (IOW, what should Fixes: point to?)

Yeah, that looks like it should blame my refactoring.

>>
>>
> 
> I think it was correct before some of the new refactoring we did. But we
> do need to set before calling ccwchain_calc_length, because the function
> does have a check for orb.cmd.64. I will see which exact commit did it.

I get why that check exists, but does anyone know why it's buried in
ccwchain_calc_length()?  Is it simply because ccwchain_calc_length()
assumes to be working on Format-1 CCWs?  I don't think that routine
cares if it's an IDA or not, an it'd be nice if we could put a check for
the supported IDA formats somewhere up front.

> 
> Thanks
> Farhan

Re: [RFC v1 2/4] vfio-ccw: No need to call cp_free on an error in cp_init

[Eric Farman]

On 7/2/19 9:58 AM, Farhan Ali wrote:
> 
> 
> On 07/02/2019 04:42 AM, Cornelia Huck wrote:
>> On Mon,  1 Jul 2019 12:23:44 -0400
>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>
>>> We don't set cp->initialized to true so calling cp_free
>>> will just return and not do anything.
>>>
>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>>> ---
>>>   drivers/s390/cio/vfio_ccw_cp.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c
>>> b/drivers/s390/cio/vfio_ccw_cp.c
>>> index 5ac4c1e..cab1be9 100644
>>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>>> @@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct
>>> device *mdev, union orb *orb)
>>>         /* Build a ccwchain for the first CCW segment */
>>>       ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
>>> -    if (ret)
>>> -        cp_free(cp);
>>
>> Makes sense; hopefully ccwchain_handle_ccw() cleans up correctly on
>> error :) (I think it does)
>>
> 
> I have checked that it does as well, but wouldn't hurt if someone else
> also glances over once again :)

Oh noes.  What happens once we start encountering TICs?  If we do:

ccwchain_handle_ccw()	(OK)
ccwchain_loop_tic()	(OK)
ccwchain_handle_ccw()	(FAIL)

The first _handle_ccw() will have added a ccwchain to the cp list, which
doesn't appear to get cleaned up now.  That used to be done in cp_init()
until I squashed cp_free and cp_unpin_free.  :(

> 
>> Maybe add a comment
>>
>> /* ccwchain_handle_ccw() already cleans up on error */
>>
>> so we don't stumble over this in the future?
> 
> Sure.
> 
>>
>> (Also, does this want a Fixes: tag?)
> 
> This might warrant a fixes tag as well.
>>
>>>         if (!ret)
>>>           cp->initialized = true;
>>
>>

Re: [RFC v1 3/4] vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn

[Eric Farman]

On 7/2/19 10:07 AM, Farhan Ali wrote:
> 
> 
> On 07/02/2019 04:45 AM, Cornelia Huck wrote:
>> On Mon,  1 Jul 2019 12:23:45 -0400
>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>
>>> So we clean up correctly.

You mean, "so we don't try to call vfio_unpin_pages()" ?

>>>
>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>>> ---
>>>   drivers/s390/cio/vfio_ccw_cp.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c
>>> b/drivers/s390/cio/vfio_ccw_cp.c
>>> index cab1be9..c5655de 100644
>>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>>> @@ -72,8 +72,10 @@ static int pfn_array_alloc(struct pfn_array *pa,
>>> u64 iova, unsigned int len)
>>>                     sizeof(*pa->pa_iova_pfn) +
>>>                     sizeof(*pa->pa_pfn),
>>>                     GFP_KERNEL);
>>> -    if (unlikely(!pa->pa_iova_pfn))
>>> +    if (unlikely(!pa->pa_iova_pfn)) {
>>> +        pa->pa_nr = 0;
>>>           return -ENOMEM;
>>> +    }
>>>       pa->pa_pfn = pa->pa_iova_pfn + pa->pa_nr;
>>>         pa->pa_iova_pfn[0] = pa->pa_iova >> PAGE_SHIFT;
>>
>> This looks like an older error -- can you give a Fixes: tag? (Yeah, I
>> know I sound like a broken record wrt that tag... :)
>>
> Yes, this is an older error. And yup I will add a fixes tag :)

Seems okay.

Reviewed-by: Eric Farman <farman@linux.ibm.com>

Re: [RFC v1 2/4] vfio-ccw: No need to call cp_free on an error in cp_init

[Farhan Ali]

On 07/02/2019 12:15 PM, Eric Farman wrote:
> 
> 
> On 7/2/19 9:58 AM, Farhan Ali wrote:
>>
>>
>> On 07/02/2019 04:42 AM, Cornelia Huck wrote:
>>> On Mon,  1 Jul 2019 12:23:44 -0400
>>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>>
>>>> We don't set cp->initialized to true so calling cp_free
>>>> will just return and not do anything.
>>>>
>>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>>>> ---
>>>>    drivers/s390/cio/vfio_ccw_cp.c | 2 --
>>>>    1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c
>>>> b/drivers/s390/cio/vfio_ccw_cp.c
>>>> index 5ac4c1e..cab1be9 100644
>>>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>>>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>>>> @@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct
>>>> device *mdev, union orb *orb)
>>>>          /* Build a ccwchain for the first CCW segment */
>>>>        ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
>>>> -    if (ret)
>>>> -        cp_free(cp);
>>>
>>> Makes sense; hopefully ccwchain_handle_ccw() cleans up correctly on
>>> error :) (I think it does)
>>>
>>
>> I have checked that it does as well, but wouldn't hurt if someone else
>> also glances over once again :)
> 
> Oh noes.  What happens once we start encountering TICs?  If we do:
> 
> ccwchain_handle_ccw()	(OK)
> ccwchain_loop_tic()	(OK)
> ccwchain_handle_ccw()	(FAIL)
> 
> The first _handle_ccw() will have added a ccwchain to the cp list, which
> doesn't appear to get cleaned up now.  That used to be done in cp_init()
> until I squashed cp_free and cp_unpin_free.  :(

Yup, you are right we are not freeing the chain correctly. Will fix it 
in v2.
> 
>>
>>> Maybe add a comment
>>>
>>> /* ccwchain_handle_ccw() already cleans up on error */
>>>
>>> so we don't stumble over this in the future?
>>
>> Sure.
>>
>>>
>>> (Also, does this want a Fixes: tag?)
>>
>> This might warrant a fixes tag as well.
>>>
>>>>          if (!ret)
>>>>            cp->initialized = true;
>>>
>>>
> 

Re: [RFC v1 1/4] vfio-ccw: Set orb.cmd.c64 before calling ccwchain_handle_ccw

[Cornelia Huck]

On Tue, 2 Jul 2019 11:11:47 -0400
Eric Farman <farman@linux.ibm.com> wrote:

> On 7/2/19 9:56 AM, Farhan Ali wrote:
> > 
> > 
> > On 07/02/2019 04:26 AM, Cornelia Huck wrote:  
> >> On Mon,  1 Jul 2019 12:23:43 -0400
> >> Farhan Ali <alifm@linux.ibm.com> wrote:
> >>  
> >>> Because ccwchain_handle_ccw calls ccwchain_calc_length and
> >>> as per the comment we should set orb.cmd.c64 before calling
> >>> ccwchanin_calc_length.
> >>>
> >>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> >>> ---
> >>>   drivers/s390/cio/vfio_ccw_cp.c | 10 +++++-----
> >>>   1 file changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c
> >>> b/drivers/s390/cio/vfio_ccw_cp.c
> >>> index d6a8dff..5ac4c1e 100644
> >>> --- a/drivers/s390/cio/vfio_ccw_cp.c
> >>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> >>> @@ -640,16 +640,16 @@ int cp_init(struct channel_program *cp, struct
> >>> device *mdev, union orb *orb)
> >>>       memcpy(&cp->orb, orb, sizeof(*orb));
> >>>       cp->mdev = mdev;
> >>>   -    /* Build a ccwchain for the first CCW segment */
> >>> -    ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
> >>> -    if (ret)
> >>> -        cp_free(cp);
> >>> -
> >>>       /* It is safe to force: if not set but idals used
> >>>        * ccwchain_calc_length returns an error.
> >>>        */
> >>>       cp->orb.cmd.c64 = 1;
> >>>   +    /* Build a ccwchain for the first CCW segment */
> >>> +    ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
> >>> +    if (ret)
> >>> +        cp_free(cp);
> >>> +
> >>>       if (!ret)
> >>>           cp->initialized = true;
> >>>     
> >>
> >> Hm... has this ever been correct, or did this break only with the
> >> recent refactorings?
> >>
> >> (IOW, what should Fixes: point to?)  
> 
> Yeah, that looks like it should blame my refactoring.
> 
> >>
> >>  
> > 
> > I think it was correct before some of the new refactoring we did. But we
> > do need to set before calling ccwchain_calc_length, because the function
> > does have a check for orb.cmd.64. I will see which exact commit did it.  
> 
> I get why that check exists, but does anyone know why it's buried in
> ccwchain_calc_length()?  Is it simply because ccwchain_calc_length()
> assumes to be working on Format-1 CCWs?  I don't think that routine
> cares if it's an IDA or not, an it'd be nice if we could put a check for
> the supported IDA formats somewhere up front.

The more I stare at this code, the more confused I get :(

Apparently we want to allow the guest to specify an orb without cmd.c64
set, as this is fine as long as the channel program does not use idals.
However, we _do_ want to reject it if cmd.c64 is not set, but idals are
used; so we actually _don't_ want to force this before the processing.
We just want the flag in the orb to be set when we do the ssch.

So it seems that the comment does not really talk about what
ccwchain_calc_length _will_ do, but what it _generally_ does (and, in
this case, already would have done.)

If my understanding is correct, maybe we should reword the comment
instead? i.e. s/returns/would have returned/

Re: [RFC v1 1/4] vfio-ccw: Set orb.cmd.c64 before calling ccwchain_handle_ccw

[Farhan Ali]

On 07/03/2019 05:30 AM, Cornelia Huck wrote:
> On Tue, 2 Jul 2019 11:11:47 -0400
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> On 7/2/19 9:56 AM, Farhan Ali wrote:
>>>
>>>
>>> On 07/02/2019 04:26 AM, Cornelia Huck wrote:
>>>> On Mon,  1 Jul 2019 12:23:43 -0400
>>>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>>>   
>>>>> Because ccwchain_handle_ccw calls ccwchain_calc_length and
>>>>> as per the comment we should set orb.cmd.c64 before calling
>>>>> ccwchanin_calc_length.
>>>>>
>>>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>>>>> ---
>>>>>    drivers/s390/cio/vfio_ccw_cp.c | 10 +++++-----
>>>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c
>>>>> b/drivers/s390/cio/vfio_ccw_cp.c
>>>>> index d6a8dff..5ac4c1e 100644
>>>>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>>>>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>>>>> @@ -640,16 +640,16 @@ int cp_init(struct channel_program *cp, struct
>>>>> device *mdev, union orb *orb)
>>>>>        memcpy(&cp->orb, orb, sizeof(*orb));
>>>>>        cp->mdev = mdev;
>>>>>    -    /* Build a ccwchain for the first CCW segment */
>>>>> -    ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
>>>>> -    if (ret)
>>>>> -        cp_free(cp);
>>>>> -
>>>>>        /* It is safe to force: if not set but idals used
>>>>>         * ccwchain_calc_length returns an error.
>>>>>         */
>>>>>        cp->orb.cmd.c64 = 1;
>>>>>    +    /* Build a ccwchain for the first CCW segment */
>>>>> +    ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
>>>>> +    if (ret)
>>>>> +        cp_free(cp);
>>>>> +
>>>>>        if (!ret)
>>>>>            cp->initialized = true;
>>>>>      
>>>>
>>>> Hm... has this ever been correct, or did this break only with the
>>>> recent refactorings?
>>>>
>>>> (IOW, what should Fixes: point to?)
>>
>> Yeah, that looks like it should blame my refactoring.
>>
>>>>
>>>>   
>>>
>>> I think it was correct before some of the new refactoring we did. But we
>>> do need to set before calling ccwchain_calc_length, because the function
>>> does have a check for orb.cmd.64. I will see which exact commit did it.
>>
>> I get why that check exists, but does anyone know why it's buried in
>> ccwchain_calc_length()?  Is it simply because ccwchain_calc_length()
>> assumes to be working on Format-1 CCWs?  I don't think that routine
>> cares if it's an IDA or not, an it'd be nice if we could put a check for
>> the supported IDA formats somewhere up front.
> 
> The more I stare at this code, the more confused I get :(

That makes 2 of us :(

> 
> Apparently we want to allow the guest to specify an orb without cmd.c64
> set, as this is fine as long as the channel program does not use idals.
> However, we _do_ want to reject it if cmd.c64 is not set, but idals are
> used; so we actually _don't_ want to force this before the processing.
> We just want the flag in the orb to be set when we do the ssch.
> 
> So it seems that the comment does not really talk about what
> ccwchain_calc_length _will_ do, but what it _generally_ does (and, in
> this case, already would have done.)
> 
> If my understanding is correct, maybe we should reword the comment
> instead? i.e. s/returns/would have returned/
> 
> 
I think you are right. But then should we move this to ccw_fetch_direct? 
It might be easier to understand the code, since that's where we are 
converting guest ccws to host idals?

Thanks
Farhan