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

[Eric Farman <farman@linux.ibm.com>]

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!