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

From: Cornelia Huck <cornelia.huck@de.ibm.com>
To: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
Cc: kvm@vger.kernel.org, linux-s390@vger.kernel.org,
	qemu-devel@nongnu.org, renxiaof@linux.vnet.ibm.com,
	borntraeger@de.ibm.com, agraf@suse.com,
	alex.williamson@redhat.com, pmorel@linux.vnet.ibm.com,
	pasic@linux.vnet.ibm.com, wkywang@linux.vnet.ibm.com
Subject: Re: [PATCH RFC v3 15/15] vfio: ccw: introduce support for ccw0
Date: Tue, 21 Feb 2017 16:47:33 +0100	[thread overview]
Message-ID: <20170221164733.105ed0f7.cornelia.huck@de.ibm.com> (raw)
In-Reply-To: <20170221085824.GN562@bjsdjshi@linux.vnet.ibm.com>

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;
> > > +}