On 11/21/22 4:40 PM, Eric Farman wrote:
> Today, we allocate memory for a list of IDAWs, and if the CCW
> being processed contains an IDAL we read that data from the guest
> into that space. We then copy each IDAW into the pa_iova array,
> or fabricate that pa_iova array with a list of addresses based
> on a direct-addressed CCW.
>
> Combine the reading of the guest IDAL with the creation of a
> pseudo-IDAL for direct-addressed CCWs, so that both CCW types
> have a "guest" IDAL that can be populated straight into the
> pa_iova array.
>
> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
> ---
> drivers/s390/cio/vfio_ccw_cp.c | 72 +++++++++++++++++++++++-----------
> 1 file changed, 50 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 6839e7195182..90685cee85db 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -192,11 +192,12 @@ static inline void page_array_idal_create_words(struct page_array *pa,
> * idaw.
> */
>
> - for (i = 0; i < pa->pa_nr; i++)
> + for (i = 0; i < pa->pa_nr; i++) {
> idaws[i] = page_to_phys(pa->pa_page[i]);
>
> - /* Adjust the first IDAW, since it may not start on a page boundary */
> - idaws[0] += pa->pa_iova[0] & (PAGE_SIZE - 1);
> + /* Incorporate any offset from each starting address */
> + idaws[i] += pa->pa_iova[i] & (PAGE_SIZE - 1);
> + }
> }
>
> static void convert_ccw0_to_ccw1(struct ccw1 *source, unsigned long len)
> @@ -496,6 +497,44 @@ static int ccwchain_fetch_tic(struct ccw1 *ccw,
> return -EFAULT;
> }
>
> +static unsigned long *get_guest_idal(struct ccw1 *ccw,
> + struct channel_program *cp,
> + int idaw_nr)
> +{
> + struct vfio_device *vdev =
> + &container_of(cp, struct vfio_ccw_private, cp)->vdev;
> + unsigned long *idaws;
> + int idal_len = idaw_nr * sizeof(*idaws);
> + int idaw_size = PAGE_SIZE;
> + int idaw_mask = ~(idaw_size - 1);
> + int i, ret;
> +
> + idaws = kcalloc(idaw_nr, sizeof(*idaws), GFP_DMA | GFP_KERNEL);
> + if (!idaws)
> + return NULL;
> +
> + if (ccw_is_idal(ccw)) {
> + /* Copy IDAL from guest */
> + ret = vfio_dma_rw(vdev, ccw->cda, idaws, idal_len, false);
> + if (ret) {
> + kfree(idaws);
> + return NULL;
As discussed off-list, for debug purposes consider using something like ERR_PTR of the vfio_dma_rw error return here rather than NULL.
> + }
> + } else {
> + /* Fabricate an IDAL based off CCW data address */
> + if (cp->orb.cmd.c64) {
> + idaws[0] = ccw->cda;
> + for (i = 1; i < idaw_nr; i++)
> + idaws[i] = (idaws[i - 1] + idaw_size) & idaw_mask;
> + } else {
If anyone else is reviewing and stumbles on this, I was initially wondering why we bail here with no obvious explanation - was going to ask for a comment here but it looks like this else gets replaced next patch with implementation for format-1.
> + kfree(idaws);
> + return NULL;
> + }
> + }
> +
> + return idaws;
> +}
> +
> /*
> * ccw_count_idaws() - Calculate the number of IDAWs needed to transfer
> * a specified amount of data
> @@ -555,7 +594,7 @@ static int ccwchain_fetch_ccw(struct ccw1 *ccw,
> &container_of(cp, struct vfio_ccw_private, cp)->vdev;
> unsigned long *idaws;
> int ret;
> - int idaw_nr, idal_len;
> + int idaw_nr;
> int i;
>
> /* Calculate size of IDAL */
> @@ -563,10 +602,8 @@ static int ccwchain_fetch_ccw(struct ccw1 *ccw,
> if (idaw_nr < 0)
> return idaw_nr;
>
> - idal_len = idaw_nr * sizeof(*idaws);
> -
> /* Allocate an IDAL from host storage */
> - idaws = kcalloc(idaw_nr, sizeof(*idaws), GFP_DMA | GFP_KERNEL);
> + idaws = get_guest_idal(ccw, cp, idaw_nr);
> if (!idaws) {
> ret = -ENOMEM;
> goto out_init;
> @@ -582,22 +619,13 @@ static int ccwchain_fetch_ccw(struct ccw1 *ccw,
> if (ret < 0)
> goto out_free_idaws;
>
> - if (ccw_is_idal(ccw)) {
> - /* Copy guest IDAL into host IDAL */
> - ret = vfio_dma_rw(vdev, ccw->cda, idaws, idal_len, false);
> - if (ret)
> - goto out_unpin;
> -
> - /*
> - * Copy guest IDAWs into page_array, in case the memory they
> - * occupy is not contiguous.
> - */
> - for (i = 0; i < idaw_nr; i++)
> + /*
> + * Copy guest IDAWs into page_array, in case the memory they
> + * occupy is not contiguous.
> + */
> + for (i = 0; i < idaw_nr; i++) {
> + if (cp->orb.cmd.c64)
> pa->pa_iova[i] = idaws[i];
> - } else {
> - pa->pa_iova[0] = ccw->cda;
> - for (i = 1; i < pa->pa_nr; i++)
> - pa->pa_iova[i] = pa->pa_iova[i - 1] + PAGE_SIZE;
> }
>
> if (ccw_does_data_transfer(ccw)) {
