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

[Cornelia Huck <cohuck@redhat.com>]

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/