On 07/03/2019 05:30 AM, Cornelia Huck wrote:
On Tue, 2 Jul 2019 11:11:47 -0400
Eric Farman <farman@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
---
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
