Re: [Qemu-devel] [PATCH RFC v3 15/15] vfio: ccw: introduce support for ccw0

[Cornelia Huck]

On Fri, 17 Feb 2017 09:29:39 +0100
Dong Jia Shi <address@hidden> wrote:

> Although Linux does not use format-0 channel command words (CCW0)
> these are a non-optional part of the platform spec, and for the sake
> of platform compliance, and possibly some non-Linux guests, we have
> to support CCW0.
> 
> Making the kernel execute a format 0 channel program is too much hassle
> because we would need to allocate and use memory which can be addressed
> by 24 bit physical addresses (because of CCW0.cda). So we implement CCW0
> support by translating the channel program into an equivalent CCW1
> program instead.
> 
> Signed-off-by: Kai Yue Wang <address@hidden>
> Signed-off-by: Dong Jia Shi <address@hidden>
> ---
>  arch/s390/Kconfig              |  7 +++++
>  drivers/s390/cio/vfio_ccw_cp.c | 58 
> ++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 32008b8..f25d077 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -680,6 +680,13 @@ config VFIO_CCW
>         To compile this driver as a module, choose M here: the
>         module will be called vfio_ccw.
> 
> +config VFIO_CCW_CCW0
> +     def_bool n
> +     prompt "Support for CCW0 translation"
> +     depends on VFIO_CCW
> +     help
> +       Enable translation for CCW0 programs for VFIO-CCW subchannels.

Hm... if ccw0 is non-optional for the architecture, why are you making
it optional then?

> +
>  endmenu
> 
>  menu "Dump support"
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 16bbb54..b0a8bc05 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -15,6 +15,26 @@
> 
>  #include "vfio_ccw_cp.h"
> 
> +#ifdef CONFIG_VFIO_CCW_CCW0
> +/**
> + * struct ccw0 - channel command word
> + * @cmd_code: command code
> + * @cda: data address
> + * @flags: flags, like IDA addressing, etc.
> + * @reserved: will be ignored
> + * @count: byte count
> + *
> + * The format-0 ccw structure.
> + */
> +struct ccw0 {
> +     __u8 cmd_code;
> +     __u32 cda : 24;
> +     __u8  flags;
> +     __u8  reserved;
> +     __u16 count;
> +} __packed __aligned(8);
> +#endif

Would it be better to move this definition next to the struct ccw1
definition? Even if the kernel does not build format-0 ccws, it makes
sense from a logical POV.

> +
>  /*
>   * Max length for ccw chain.
>   * XXX: Limit to 256, need to check more?
> @@ -243,12 +263,42 @@ static long copy_from_iova(struct device *mdev,
>       return l;
>  }
> 
> +#ifdef CONFIG_VFIO_CCW_CCW0
> +static long copy_ccw_from_iova(struct channel_program *cp,
> +                            struct ccw1 *to, u64 iova,
> +                            unsigned long len)
> +{
> +     struct ccw0 ccw0;
> +     struct ccw1 *pccw1;
> +     int ret;
> +     int i;
> +
> +     ret = copy_from_iova(cp->mdev, to, iova, len * sizeof(struct ccw1));
> +     if (ret)
> +             return ret;
> +
> +     if (!cp->orb.cmd.fmt) {
> +             pccw1 = to;
> +             for (i = 0; i < len; i++) {
> +                     ccw0 = *(struct ccw0 *)pccw1;
> +                     pccw1->cmd_code = ccw0.cmd_code;
> +                     pccw1->flags = ccw0.flags;
> +                     pccw1->count = ccw0.count;
> +                     pccw1->cda = ccw0.cda;
> +                     pccw1++;
> +             }

IIRC there are one or two very subtle differences between what format-0
and what format-1 ccws allow (see the ccw interpretation in qemu --
probably easier than combing through the PoP). We should either check
this or add a comment why we don't.

> +     }
> +
> +     return ret;
> +}
> +#else
>  static long copy_ccw_from_iova(struct channel_program *cp,
>                              struct ccw1 *to, u64 iova,
>                              unsigned long len)
>  {
>       return copy_from_iova(cp->mdev, to, iova, len * sizeof(struct ccw1));
>  }
> +#endif

This would be nicer without #ifdefs :)

> 
>  /*
>   * Helpers to operate ccwchain.
> @@ -616,10 +666,14 @@ int cp_init(struct channel_program *cp, struct device 
> *mdev, union orb *orb)
>        * Only support prefetch enable mode now.
>        * Only support 64bit addressing idal.
>        * Only support 4k IDAW.
> -      * Only support ccw1.
>        */
> -     if (!orb->cmd.pfch || !orb->cmd.c64 || orb->cmd.i2k || !orb->cmd.fmt)
> +     if (!orb->cmd.pfch || !orb->cmd.c64 || orb->cmd.i2k)
> +             return -EOPNOTSUPP;
> +
> +#ifndef CONFIG_VFIO_CCW_CCW0
> +     if (!orb->cmd.fmt)
>               return -EOPNOTSUPP;
> +#endif

dito

> 
>       INIT_LIST_HEAD(&cp->ccwchain_list);
>       memcpy(&cp->orb, orb, sizeof(*orb));

The "translate format-0 into format-1 ccws" approach is fine, but I
think you need to double check the subtle differences I mentioned.