| Message ID | 20221121214056.1187700-14-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/ccw: channel program cleanup | expand |
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@linux.ibm.com> > --- > 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)) {
On Mon, 2022-12-19 at 15:14 -0500, Matthew Rosato wrote: > 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@linux.ibm.com> > > --- > > 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. Yes, good idea. > > > + } > > + } 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. As you note, this goes away in the next patch so I didn't put a comment in place. But while putting in the ERR_PTR/PTR_ERR stuff, I opted to return EOPNOTSUPP here instead of NULL, so we get an error equal to the current fence instead of an ENOMEM. So that'll be in v2. Thanks, Eric > > > + 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)) { >
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; + } + } 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 { + 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)) {
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@linux.ibm.com> --- drivers/s390/cio/vfio_ccw_cp.c | 72 +++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 22 deletions(-)