| Message ID | 20170217082939.33208-16-bjsdjshi@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
* Cornelia Huck <cornelia.huck@de.ibm.com> [2017-02-20 19:59:01 +0100]: > 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? > Yes, this is an unnecessary move. I overthought to give the user the freedom to make their own choice. Will remove this. > > + > > 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. > Ok. > > + > > /* > > * 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. > Cool! Thanks for pointing out this! Transfer in Channel, 15-73, PoP: --------------------8<----------------------------- The contents of the second half of the format-0 CCW, bit positions 32-63, are ignored. Similarly, the con- tents of bit positions 0-3 of the format-0 CCW are ignored. Bit positions 0-3 and 8-32 of the format-1 CCW must contain zeros; otherwise, a program-check condition is generated. -------------------->8----------------------------- I will fix according to the above description. I also checked the code in qemu. It seems that it is not compliant with the second paragraph above. Could I send out a separated patch to fix that? > > + } > > + > > + 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 :) > No problem. Since I will remove CONFIG_VFIO_CCW_CCW0, this will disappear either. > > > > /* > > * 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 > Ok. > > > > 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. Thansk for the comments. Will do!
On Tue, 21 Feb 2017 16:58:24 +0800 Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > * Cornelia Huck <cornelia.huck@de.ibm.com> [2017-02-20 19:59:01 +0100]: > > > 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(-) > > > +#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. > > > Cool! Thanks for pointing out this! > > Transfer in Channel, 15-73, PoP: > --------------------8<----------------------------- > The contents of the second half of the format-0 CCW, > bit positions 32-63, are ignored. Similarly, the con- > tents of bit positions 0-3 of the format-0 CCW are > ignored. > > Bit positions 0-3 and 8-32 of the format-1 CCW must > contain zeros; otherwise, a program-check condition > is generated. > -------------------->8----------------------------- > > I will fix according to the above description. Yes. It's a bit unfortunate that we have to inspect the ccws, but it probably can't be helped if we want to be architecture compliant. > > I also checked the code in qemu. It seems that it is not compliant with > the second paragraph above. Could I send out a separated patch to fix > that? Please do, bugfixes are welcome :) > > > > + } > > > + > > > + return ret; > > > +}
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. + 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 + /* * 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++; + } + } + + 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 /* * 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 INIT_LIST_HEAD(&cp->ccwchain_list); memcpy(&cp->orb, orb, sizeof(*orb));