[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v3 15/15] vfio: ccw: introduce support for c
From: |
Dong Jia Shi |
Subject: |
Re: [Qemu-devel] [PATCH RFC v3 15/15] vfio: ccw: introduce support for ccw0 |
Date: |
Tue, 21 Feb 2017 16:58:24 +0800 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
* Cornelia Huck <address@hidden> [2017-02-20 19:59:01 +0100]:
> 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?
>
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!
--
Dong Jia
- [Qemu-devel] [PATCH RFC v3 08/15] vfio: ccw: handle ccw command request, (continued)
- [Qemu-devel] [PATCH RFC v3 08/15] vfio: ccw: handle ccw command request, Dong Jia Shi, 2017/02/17
- [Qemu-devel] [PATCH RFC v3 07/15] vfio: ccw: introduce ccw_io_region, Dong Jia Shi, 2017/02/17
- [Qemu-devel] [PATCH RFC v3 10/15] vfio: ccw: realize VFIO_DEVICE_RESET ioctl, Dong Jia Shi, 2017/02/17
- [Qemu-devel] [PATCH RFC v3 09/15] vfio: ccw: realize VFIO_DEVICE_GET_REGION_INFO ioctl, Dong Jia Shi, 2017/02/17
- [Qemu-devel] [PATCH RFC v3 11/15] vfio: ccw: realize VFIO_DEVICE_G(S)ET_IRQ_INFO ioctls, Dong Jia Shi, 2017/02/17
- [Qemu-devel] [PATCH RFC v3 12/15] vfio: ccw: return I/O results asynchronously, Dong Jia Shi, 2017/02/17
- [Qemu-devel] [PATCH RFC v3 13/15] vfio: ccw: introduce a finite state machine, Dong Jia Shi, 2017/02/17
- [Qemu-devel] [PATCH RFC v3 14/15] docs: add documentation for vfio-ccw, Dong Jia Shi, 2017/02/17
- [Qemu-devel] [PATCH RFC v3 15/15] vfio: ccw: introduce support for ccw0, Dong Jia Shi, 2017/02/17