Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write

From: Cornelia Huck <cohuck@redhat.com>
To: Halil Pasic <pasic@linux.ibm.com>
Cc: thuth@redhat.com, frankja@linux.ibm.com,
	Pierre Morel <pmorel@linux.ibm.com>,
	mst@redhat.com, david@redhat.com, richard.henderson@linaro.org,
	qemu-devel@nongnu.org, borntraeger@de.ibm.com,
	qemu-s390x@nongnu.org, marcandre.lureau@redhat.com,
	pbonzini@redhat.com, imbrenda@linux.ibm.com
Subject: Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
Date: Thu, 8 Apr 2021 10:53:02 +0200	[thread overview]
Message-ID: <20210408105302.598f46b8.cohuck@redhat.com> (raw)
In-Reply-To: <20210407185426.257ed03d.pasic@linux.ibm.com>

On Wed, 7 Apr 2021 18:54:26 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Wed, 7 Apr 2021 13:41:57 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> > > Here, however, I'm not sure. Returning a negative error here is fine,
> > > but handle_payload_3270_write (not changed in this patch) seems to
> > > match everything to -EIO. Shouldn't it just be propagated, and maybe 0
> > > mapped to -EIO only? If I'm not confused, we'll end up mapping every
> > > error to intervention required.    
> > 
> > I know very little about the 3270 but in my opinion accessing memory is 
> > not the problem of the device but of the subchannel.
> > So we should certainly propagate the error instead of returning -EIO for 
> > any error.  
> 
> I agree, what condition needs to be indicated when we encounter an
> invalid CCW, IDAW or MIDAW address is clear. In the PoP this is
> described under Chapter 16. I/O Interruptions > Subchannel-Status Word >
> Subchannel-Status Field > Program Check in the paragraphs on "Invalid
> IDAW or MIDAW Address and "Invalid CCW Address".
> 
> My guess about handle_payload_3270_write() is that IMHO the initial 3270
> emulation code was, let's say of mixed quality in the first place, so
> wouldn't seek some hidden meaning/sense, behind the -EIO. IMHO first
> mapping architectural conditions to "errno-style" error codes, passing
> these around, and then mapping them back is a litle non-intuitive. If one
> looks at the purpose of handle_payload_3270_write(), it is basically
> emulating an IO data transfer from the device into the main storage. If
> that sort of operation. So with -EIO the original author probably wanted
> to say: hey there was an input/output error, which ain't that wrong. The
> problem is that somewhere up the call stack EIO gets interpreted in a
> very peculiar way, and along with other errnos mapped to CIO
> architecture stuff like SCSW bits.

It's not quite clear to me what the 3270 is supposed to do (I remember
some requirements for the aforementioned intervention required, but
posting unit checks does not look like the right thing to do for all of
the possible errors.)

Let's just loop the error through? I doubt that this is the last
problem in the 3270 code :) Fortunately, it is a rather obscure device
to be used with QEMU.