Re: [PATCH v1 15/16] vfio/ccw: don't group contiguous pages on 2K IDAWs

Matthew Rosato
2022-12-19T20:41:21Z

On 11/21/22 4:40 PM, Eric Farman wrote:
> The vfio_pin_pages() interface allows contiguous pages to be
> pinned as a single request, which is great for the 4K pages
> that are normally processed. Old IDA formats operate on 2K
> chunks, which makes this logic more difficult.
>
> Since these formats are rare, let's just invoke the page
> pinning one-at-a-time, instead of trying to group them.
> We can rework this code at a later date if needed.
>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> drivers/s390/cio/vfio_ccw_cp.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 9527f3d8da77..3829c346583c 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -88,7 +88,7 @@ static int page_array_alloc(struct page_array *pa, unsigned int len)
> * otherwise only clear pa->pa_nr
> */
> static void page_array_unpin(struct page_array *pa,
> - struct vfio_device *vdev, int pa_nr)
> + struct vfio_device *vdev, int pa_nr, bool unaligned)

Please add 'unaligned' the comment block above this function with a short explanation...

> {
> int unpinned = 0, npage = 1;
>
> @@ -97,7 +97,8 @@ static void page_array_unpin(struct page_array *pa,
> dma_addr_t *last = &first[npage];
>
> if (unpinned + npage < pa_nr &&
> - *first + npage * PAGE_SIZE == *last) {
> + *first + npage * PAGE_SIZE == *last &&
> + !unaligned) {
> npage++;
> continue;
> }
> @@ -119,7 +120,7 @@ static void page_array_unpin(struct page_array *pa,
> * If the pin request partially succeeds, or fails completely,
> * all pages are left unpinned and a negative error value is returned.
> */
> -static int page_array_pin(struct page_array *pa, struct vfio_device *vdev)
> +static int page_array_pin(struct page_array *pa, struct vfio_device *vdev, bool unaligned)

... Also here. Otherwise, I agree re-work can be done later since this is not a common case.

With those changes:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> {
> int pinned = 0, npage = 1;
> int ret = 0;
> @@ -129,7 +130,8 @@ static int page_array_pin(struct page_array *pa, struct vfio_device *vdev)
> dma_addr_t *last = &first[npage];
>
> if (pinned + npage < pa->pa_nr &&
> - *first + npage * PAGE_SIZE == *last) {
> + *first + npage * PAGE_SIZE == *last &&
> + !unaligned) {
> npage++;
> continue;
> }
> @@ -151,14 +153,14 @@ static int page_array_pin(struct page_array *pa, struct vfio_device *vdev)
> return ret;
>
> err_out:
> - page_array_unpin(pa, vdev, pinned);
> + page_array_unpin(pa, vdev, pinned, unaligned);
> return ret;
> }
>
> /* Unpin the pages before releasing the memory. */
> -static void page_array_unpin_free(struct page_array *pa, struct vfio_device *vdev)
> +static void page_array_unpin_free(struct page_array *pa, struct vfio_device *vdev, bool unaligned)
> {
> - page_array_unpin(pa, vdev, pa->pa_nr);
> + page_array_unpin(pa, vdev, pa->pa_nr, unaligned);
> kfree(pa->pa_page);
> kfree(pa->pa_iova);
> }
> @@ -638,7 +640,7 @@ static int ccwchain_fetch_ccw(struct ccw1 *ccw,
> }
>
> if (ccw_does_data_transfer(ccw)) {
> - ret = page_array_pin(pa, vdev);
> + ret = page_array_pin(pa, vdev, idal_is_2k(cp));
> if (ret < 0)
> goto out_unpin;
> } else {
> @@ -654,7 +656,7 @@ static int ccwchain_fetch_ccw(struct ccw1 *ccw,
> return 0;
>
> out_unpin:
> - page_array_unpin_free(pa, vdev);
> + page_array_unpin_free(pa, vdev, idal_is_2k(cp));
> out_free_idaws:
> kfree(idaws);
> out_init:
> @@ -752,7 +754,7 @@ void cp_free(struct channel_program *cp)
> cp->initialized = false;
> list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
> for (i = 0; i < chain->ch_len; i++) {
> - page_array_unpin_free(chain->ch_pa + i, vdev);
> + page_array_unpin_free(chain->ch_pa + i, vdev, idal_is_2k(cp));
> ccwchain_cda_free(chain, i);
> }
> ccwchain_free(chain);