Re: [PATCH v1 14/16] vfio/ccw: handle a guest Format-1 IDAL

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

On Mon, 2022-12-19 at 15:29 -0500, Matthew Rosato wrote:
> On 11/21/22 4:40 PM, Eric Farman wrote:
> > There are two scenarios that need to be addressed here.
> > 
> > First, an ORB that does NOT have the Format-2 IDAL bit set could
> > have both a direct-addressed CCW and an indirect-data-address CCW
> > chained together. This means that the IDA CCW will contain a
> > Format-1 IDAL, and can be easily converted to a 2K Format-2 IDAL.
> > But it also means that the direct-addressed CCW needs to be
> > converted to the same 2K Format-2 IDAL for consistency with the
> > ORB settings.
> > 
> > Secondly, a Format-1 IDAL is comprised of 31-bit addresses.
> > Thus, we need to cast this IDAL to a pointer of ints while
> > populating the list of addresses that are sent to vfio.
> > 
> > Since the result of both of these is the use of the 2K IDAL
> > variants, and the output of vfio-ccw is always a Format-2 IDAL
> > (in order to use 64-bit addresses), make sure that the correct
> > control bit gets set in the ORB when these scenarios occur.
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >  drivers/s390/cio/vfio_ccw_cp.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_cp.c
> > b/drivers/s390/cio/vfio_ccw_cp.c
> > index 90685cee85db..9527f3d8da77 100644
> > --- a/drivers/s390/cio/vfio_ccw_cp.c
> > +++ b/drivers/s390/cio/vfio_ccw_cp.c
> > @@ -222,6 +222,8 @@ static void convert_ccw0_to_ccw1(struct ccw1
> > *source, unsigned long len)
> >         }
> >  }
> >  
> > +#define idal_is_2k(_cp) (!_cp->orb.cmd.c64 || _cp->orb.cmd.i2k)
> > +
> >  /*
> >   * Helpers to operate ccwchain.
> >   */
> > @@ -504,8 +506,9 @@ static unsigned long *get_guest_idal(struct
> > ccw1 *ccw,
> >         struct vfio_device *vdev =
> >                 &container_of(cp, struct vfio_ccw_private, cp)-
> > >vdev;
> >         unsigned long *idaws;
> > +       unsigned int *idaws_f1;
> 
> I wonder if we should be using explicit u64/u32 here since we are
> dealing with hardware-architected data sizes and specifically want to
> index by 32- or 64-bits.  Honestly, there's probably a number of
> other spots in vfio-ccw where that might make sense so it would also
> be OK to look into that as a follow-on.

This is a fair point. I have some follow-on work to this series for
after the holidays, so I'm going to put this suggestion on that todo
list.

> 
> Otherwise, LGTM
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> >         int idal_len = idaw_nr * sizeof(*idaws);
> > -       int idaw_size = PAGE_SIZE;
> > +       int idaw_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
> >         int idaw_mask = ~(idaw_size - 1);
> >         int i, ret;
> >  
> > @@ -527,8 +530,10 @@ static unsigned long *get_guest_idal(struct
> > ccw1 *ccw,
> >                         for (i = 1; i < idaw_nr; i++)
> >                                 idaws[i] = (idaws[i - 1] +
> > idaw_size) & idaw_mask;
> >                 } else {
> > -                       kfree(idaws);
> > -                       return NULL;
> > +                       idaws_f1 = (unsigned int *)idaws;
> > +                       idaws_f1[0] = ccw->cda;
> > +                       for (i = 1; i < idaw_nr; i++)
> > +                               idaws_f1[i] = (idaws_f1[i - 1] +
> > idaw_size) & idaw_mask;
> >                 }
> >         }
> >  
> > @@ -593,6 +598,7 @@ static int ccwchain_fetch_ccw(struct ccw1 *ccw,
> >         struct vfio_device *vdev =
> >                 &container_of(cp, struct vfio_ccw_private, cp)-
> > >vdev;
> >         unsigned long *idaws;
> > +       unsigned int *idaws_f1;
> >         int ret;
> >         int idaw_nr;
> >         int i;
> > @@ -623,9 +629,12 @@ static int ccwchain_fetch_ccw(struct ccw1
> > *ccw,
> >          * Copy guest IDAWs into page_array, in case the memory
> > they
> >          * occupy is not contiguous.
> >          */
> > +       idaws_f1 = (unsigned int *)idaws;
> >         for (i = 0; i < idaw_nr; i++) {
> >                 if (cp->orb.cmd.c64)
> >                         pa->pa_iova[i] = idaws[i];
> > +               else
> > +                       pa->pa_iova[i] = idaws_f1[i];
> >         }
> >  
> >         if (ccw_does_data_transfer(ccw)) {
> > @@ -846,7 +855,11 @@ union orb *cp_get_orb(struct channel_program
> > *cp, struct subchannel *sch)
> >  
> >         /*
> >          * Everything built by vfio-ccw is a Format-2 IDAL.
> > +        * If the input was a Format-1 IDAL, indicate that
> > +        * 2K Format-2 IDAWs were created here.
> >          */
> > +       if (!orb->cmd.c64)
> > +               orb->cmd.i2k = 1;
> >         orb->cmd.c64 = 1;
> >  
> >         if (orb->cmd.lpm == 0)
>