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

[Heiko Carstens <hca@linux.ibm.com>]

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 :)