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

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Fri, 17 Feb 2017 09:29:39 +0100
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> 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 <wkywang@linux.vnet.ibm.com>
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
>  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.