| Message ID | 050943a6f5a427317ea64100bc2b4ec6394a4411.1561997809.git.alifm@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some vfio-ccw fixes | expand |
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?)
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
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
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/
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
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;
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(-)