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

Message ID 050943a6f5a427317ea64100bc2b4ec6394a4411.1561997809.git.alifm@linux.ibm.com
State New
Headers show
Series
  • Some vfio-ccw fixes
Related show

Commit Message

Farhan Ali July 1, 2019, 4:23 p.m. UTC
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(-)

Comments

Cornelia Huck July 2, 2019, 8:26 a.m. UTC | #1
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?)
Farhan Ali July 2, 2019, 1:56 p.m. UTC | #2
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
Eric Farman July 2, 2019, 3:11 p.m. UTC | #3
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
Cornelia Huck July 3, 2019, 9:30 a.m. UTC | #4
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/
Farhan Ali July 8, 2019, 1:34 p.m. UTC | #5
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

Patch
diff mbox series

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;