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@xxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx>
>
> > 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)
>
