Re: [PATCH v1 13/16] vfio/ccw: allocate/populate the guest idal

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

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