[PATCH] s390/vfio_ccw: Fix target addresses of TIC CCWs

[PATCH] s390/vfio_ccw: Fix target addresses of TIC CCWs

[Eric Farman]

The processing of a Transfer-In-Channel (TIC) CCW requires locating
the target of the CCW in the channel program, and updating the
address to reflect what will actually be sent to hardware.

An error exists where the 64-bit virtual address is truncated to
32-bits (variable "cda") when performing this math. Since s390
addresses of that size are 31-bits, this leaves that additional
bit enabled such that the resulting I/O triggers a channel
program check. This shows up occasionally when booting a KVM
guest from a passthrough DASD device:

  ..snip...
  Interrupt Response Block Data:
  : 0x0000000000003990
      Function Ctrl : [Start]
      Activity Ctrl :
      Status Ctrl : [Alert] [Primary] [Secondary] [Status-Pending]
      Device Status :
      Channel Status : [Program-Check]
      cpa=: 0x00000000008d0018
      prev_ccw=: 0x0000000000000000
      this_ccw=: 0x0000000000000000
  ...snip...
  dasd-ipl: Failed to run IPL1 channel program

The channel program address of "0x008d0018" in the IRB doesn't
look wrong, but tracing the CCWs shows the offending bit enabled:

  ccw=0x0000012e808d0000 cda=00a0b030
  ccw=0x0000012e808d0008 cda=00a0b038
  ccw=0x0000012e808d0010 cda=808d0008
  ccw=0x0000012e808d0018 cda=00a0b040

Fix the calculation of the TIC CCW's data address such that it points
to a valid 31-bit address regardless of the input address.

Fixes: bd36cfbbb9e1 ("s390/vfio_ccw_cp: use new address translation helpers")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 6e5c508b1e07..fd8cb052f096 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -495,8 +495,9 @@ static int ccwchain_fetch_tic(struct ccw1 *ccw,
 	list_for_each_entry(iter, &cp->ccwchain_list, next) {
 		ccw_head = iter->ch_iova;
 		if (is_cpa_within_range(ccw->cda, ccw_head, iter->ch_len)) {
-			cda = (u64)iter->ch_ccw + dma32_to_u32(ccw->cda) - ccw_head;
-			ccw->cda = u32_to_dma32(cda);
+			/* Calculate offset of TIC target */
+			cda = dma32_to_u32(ccw->cda) - ccw_head;
+			ccw->cda = virt_to_dma32(iter->ch_ccw) + cda;
 			return 0;
 		}
 	}
-- 
2.40.1

Re: [PATCH] s390/vfio_ccw: Fix target addresses of TIC CCWs

[Heiko Carstens]

On Thu, Jun 27, 2024 at 10:07:40PM +0200, Eric Farman wrote:
> The processing of a Transfer-In-Channel (TIC) CCW requires locating
> the target of the CCW in the channel program, and updating the
> address to reflect what will actually be sent to hardware.
> 
> An error exists where the 64-bit virtual address is truncated to
> 32-bits (variable "cda") when performing this math. Since s390

...

> Fix the calculation of the TIC CCW's data address such that it points
> to a valid 31-bit address regardless of the input address.
> 
> Fixes: bd36cfbbb9e1 ("s390/vfio_ccw_cp: use new address translation helpers")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 6e5c508b1e07..fd8cb052f096 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -495,8 +495,9 @@ static int ccwchain_fetch_tic(struct ccw1 *ccw,
>  	list_for_each_entry(iter, &cp->ccwchain_list, next) {
>  		ccw_head = iter->ch_iova;
>  		if (is_cpa_within_range(ccw->cda, ccw_head, iter->ch_len)) {
> -			cda = (u64)iter->ch_ccw + dma32_to_u32(ccw->cda) - ccw_head;
> -			ccw->cda = u32_to_dma32(cda);
> +			/* Calculate offset of TIC target */
> +			cda = dma32_to_u32(ccw->cda) - ccw_head;
> +			ccw->cda = virt_to_dma32(iter->ch_ccw) + cda;

I would suggest to rename cda to "offset", since that reflects what it is
now. Also this code needs to take care of type checking, which will fail now
due to dma32_t type (try "make C=1 drivers/s390/cio/vfio_ccw_cp.o).

You could write the above as:

			ccw->cda = virt_to_dma32((void *)iter->ch_ccw + cda);

Note that somebody :) introduced a similar bug in cp_update_scsw(). I guess
you could add this hunk to your patch:

@@ -915,7 +915,7 @@ void cp_update_scsw(struct channel_program *cp, union scsw *scsw)
 	 * in the ioctl directly. Path status changes etc.
 	 */
 	list_for_each_entry(chain, &cp->ccwchain_list, next) {
-		ccw_head = (u32)(u64)chain->ch_ccw;
+		ccw_head = (__force u32)virt_to_dma32(chain->ch_ccw);
 		/*
 		 * On successful execution, cpa points just beyond the end
 		 * of the chain.

Furthermore it looks to me like the ch_iova member of struct ccwchain should
get a dma32_t type instead of u64. The same applies to quite a few variables
to the code. I could give this a try, but I think it would be better if
somebody who knows what he is doing would address this :)

Re: [PATCH] s390/vfio_ccw: Fix target addresses of TIC CCWs

[Eric Farman]

On Fri, 2024-06-28 at 14:17 +0200, Heiko Carstens wrote:
> On Thu, Jun 27, 2024 at 10:07:40PM +0200, Eric Farman wrote:
> > The processing of a Transfer-In-Channel (TIC) CCW requires locating
> > the target of the CCW in the channel program, and updating the
> > address to reflect what will actually be sent to hardware.
> > 
> > An error exists where the 64-bit virtual address is truncated to
> > 32-bits (variable "cda") when performing this math. Since s390
> 
> ...
> 
> > Fix the calculation of the TIC CCW's data address such that it
> > points
> > to a valid 31-bit address regardless of the input address.
> > 
> > Fixes: bd36cfbbb9e1 ("s390/vfio_ccw_cp: use new address translation
> > helpers")
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >  drivers/s390/cio/vfio_ccw_cp.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_cp.c
> > b/drivers/s390/cio/vfio_ccw_cp.c
> > index 6e5c508b1e07..fd8cb052f096 100644
> > --- a/drivers/s390/cio/vfio_ccw_cp.c
> > +++ b/drivers/s390/cio/vfio_ccw_cp.c
> > @@ -495,8 +495,9 @@ static int ccwchain_fetch_tic(struct ccw1 *ccw,
> >  	list_for_each_entry(iter, &cp->ccwchain_list, next) {
> >  		ccw_head = iter->ch_iova;
> >  		if (is_cpa_within_range(ccw->cda, ccw_head, iter-
> > >ch_len)) {
> > -			cda = (u64)iter->ch_ccw +
> > dma32_to_u32(ccw->cda) - ccw_head;
> > -			ccw->cda = u32_to_dma32(cda);
> > +			/* Calculate offset of TIC target */
> > +			cda = dma32_to_u32(ccw->cda) - ccw_head;
> > +			ccw->cda = virt_to_dma32(iter->ch_ccw) +
> > cda;
> 
> I would suggest to rename cda to "offset", since that reflects what
> it is
> now. Also this code needs to take care of type checking, which will
> fail now
> due to dma32_t type (try "make C=1 drivers/s390/cio/vfio_ccw_cp.o).

Argh, I missed that. Sorry.

> 
> You could write the above as:
> 
> 			ccw->cda = virt_to_dma32((void *)iter-
> >ch_ccw + cda);
> 
> Note that somebody :) introduced a similar bug in cp_update_scsw(). 

:)

I was poking at that code yesterday because it seemed suspect, but as I
wasn't getting an explicit failure (versus the CPC generated by hw), I
opted to leave it for now. I agree they should both be fixed up.

> I guess
> you could add this hunk to your patch:
> 
> @@ -915,7 +915,7 @@ void cp_update_scsw(struct channel_program *cp,
> union scsw *scsw)
>  	 * in the ioctl directly. Path status changes etc.
>  	 */
>  	list_for_each_entry(chain, &cp->ccwchain_list, next) {
> -		ccw_head = (u32)(u64)chain->ch_ccw;
> +		ccw_head = (__force u32)virt_to_dma32(chain-
> >ch_ccw);
>  		/*
>  		 * On successful execution, cpa points just beyond
> the end
>  		 * of the chain.
> 
> Furthermore it looks to me like the ch_iova member of struct ccwchain
> should
> get a dma32_t type instead of u64. The same applies to quite a few
> variables
> to the code. 

Agreed. I started this some time back after the IDAW code got reworked,
but have been sidetracked. The problem with ch_iova is more apparent
after the dma32 stuff.

> I could give this a try, but I think it would be better if
> somebody who knows what he is doing would address this :)

I'll finish them up. But v2 will have to wait until after my holiday.
Thanks for reminding me of the typechecking!

Re: [PATCH] s390/vfio_ccw: Fix target addresses of TIC CCWs

[Heiko Carstens]

On Fri, Jun 28, 2024 at 09:31:56AM -0400, Eric Farman wrote:
> > > dma32_to_u32(ccw->cda) - ccw_head;
> > > -			ccw->cda = u32_to_dma32(cda);
> > > +			/* Calculate offset of TIC target */
> > > +			cda = dma32_to_u32(ccw->cda) - ccw_head;
> > > +			ccw->cda = virt_to_dma32(iter->ch_ccw) +
> > > cda;
> > 
> > I would suggest to rename cda to "offset", since that reflects what
> > it is
> > now. Also this code needs to take care of type checking, which will
> > fail now
> > due to dma32_t type (try "make C=1 drivers/s390/cio/vfio_ccw_cp.o).

...

> I was poking at that code yesterday because it seemed suspect, but as I
> wasn't getting an explicit failure (versus the CPC generated by hw), I
> opted to leave it for now. I agree they should both be fixed up.

...

> > I guess
> > you could add this hunk to your patch:
> > 
> > @@ -915,7 +915,7 @@ void cp_update_scsw(struct channel_program *cp,
> > union scsw *scsw)
> >  	 * in the ioctl directly. Path status changes etc.
> >  	 */
> >  	list_for_each_entry(chain, &cp->ccwchain_list, next) {
> > -		ccw_head = (u32)(u64)chain->ch_ccw;
> > +		ccw_head = (__force u32)virt_to_dma32(chain-
> > >ch_ccw);
> >  		/*
> >  		 * On successful execution, cpa points just beyond
> > the end
> >  		 * of the chain.

...

> > Furthermore it looks to me like the ch_iova member of struct ccwchain
> > should
> > get a dma32_t type instead of u64. The same applies to quite a few
> > variables
> > to the code. 
> 
> Agreed. I started this some time back after the IDAW code got reworked,
> but have been sidetracked. The problem with ch_iova is more apparent
> after the dma32 stuff.
> 
> > I could give this a try, but I think it would be better if
> > somebody who knows what he is doing would address this :)
> 
> I'll finish them up. But v2 will have to wait until after my holiday.
> Thanks for reminding me of the typechecking!

I hope you didn't get me wrong: from my point of view we want one or
two small patches (the above hunks), which fix the bugs, if you
agree.

And then address the type checking stuff at a later point in time.

(btw: your mailer adds lot's of extra line wraps)

Re: [PATCH] s390/vfio_ccw: Fix target addresses of TIC CCWs

[Eric Farman]

On Fri, 2024-06-28 at 15:40 +0200, Heiko Carstens wrote:
> On Fri, Jun 28, 2024 at 09:31:56AM -0400, Eric Farman wrote:
> > > > dma32_to_u32(ccw->cda) - ccw_head;
> > > > -			ccw->cda = u32_to_dma32(cda);
> > > > +			/* Calculate offset of TIC target */
> > > > +			cda = dma32_to_u32(ccw->cda) - ccw_head;
> > > > +			ccw->cda = virt_to_dma32(iter->ch_ccw) +
> > > > cda;
> > > 
> > > I would suggest to rename cda to "offset", since that reflects what
> > > it is
> > > now. Also this code needs to take care of type checking, which will
> > > fail now
> > > due to dma32_t type (try "make C=1 drivers/s390/cio/vfio_ccw_cp.o).
> 
> ...
> 
> > I was poking at that code yesterday because it seemed suspect, but as I
> > wasn't getting an explicit failure (versus the CPC generated by hw), I
> > opted to leave it for now. I agree they should both be fixed up.
> 
> ...
> 
> > > I guess
> > > you could add this hunk to your patch:
> > > 
> > > @@ -915,7 +915,7 @@ void cp_update_scsw(struct channel_program *cp,
> > > union scsw *scsw)
> > >  	 * in the ioctl directly. Path status changes etc.
> > >  	 */
> > >  	list_for_each_entry(chain, &cp->ccwchain_list, next) {
> > > -		ccw_head = (u32)(u64)chain->ch_ccw;
> > > +		ccw_head = (__force u32)virt_to_dma32(chain-
> > > > ch_ccw);
> > >  		/*
> > >  		 * On successful execution, cpa points just beyond
> > > the end
> > >  		 * of the chain.
> 
> ...
> 
> > > Furthermore it looks to me like the ch_iova member of struct ccwchain
> > > should
> > > get a dma32_t type instead of u64. The same applies to quite a few
> > > variables
> > > to the code. 
> > 
> > Agreed. I started this some time back after the IDAW code got reworked,
> > but have been sidetracked. The problem with ch_iova is more apparent
> > after the dma32 stuff.
> > 
> > > I could give this a try, but I think it would be better if
> > > somebody who knows what he is doing would address this :)
> > 
> > I'll finish them up. But v2 will have to wait until after my holiday.
> > Thanks for reminding me of the typechecking!
> 
> I hope you didn't get me wrong: from my point of view we want one or
> two small patches (the above hunks), which fix the bugs, if you
> agree.
> 
> And then address the type checking stuff at a later point in time.

-ENOCOFFEE. Sorry, I did misunderstand. I'll send the small patches
later this morning.

> 
> (btw: your mailer adds lot's of extra line wraps)
> 

Ugh. Thanks for the heads up.