Re: [PATCH] vfio-ccw: Do not read region ret_code after write

[Eric Farman]

On 3/2/21 1:14 PM, Cornelia Huck wrote:
> On Mon,  1 Mar 2021 20:51:43 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
>
> > A pwrite() call returns the number of bytes written (or -1 on error),
> > and vfio-ccw compares this number with the size of the region to
> > determine if an error had occurred or not. If they are equal, the
> > code reads the ret_code field from the region. However, while the
> > kernel sets the ret_code field as necessary, the region and thus
> > this field is not "written back" to the user. So the value can only
> > be what it was initialized to, which is zero.
> >
> > Not harming anything, but it's a puzzle. Let's avoid the confusion
> > and just set the return code to zero for this case.
>
> Yes, ret_code seems to be pretty much useless for us: we don't even
> look at it when we read the region for interrupt handling. Thankfully,
> we don't seem to really need it, as we can rely on errno. (Probably
> worth double checking that this is indeed the case.)

I didn't see any scenario on either the kernel or qemu side that made me 
worried.

> I don't suppose we need to handle a hypothetical broken kernel that
> returns the wrong size with errno==0?

Well, that would be silly of it. :) But, since I'm in here, sure I can 
add in some suspenders.

Thanks,
Eric

> > Suggested-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >   hw/vfio/ccw.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> > index bc78a0ad76..bfd5fd07a5 100644
> > --- a/hw/vfio/ccw.c
> > +++ b/hw/vfio/ccw.c
> > @@ -106,7 +106,7 @@ again:
> >           error_report("vfio-ccw: write I/O region failed with errno=%d", 
> > errno);
> >           ret = -errno;
> >       } else {
> > -        ret = region->ret_code;
> > +        ret = 0;
> >       }
> >       switch (ret) {
> >       case 0:
> > @@ -194,7 +194,7 @@ again:
> >           error_report("vfio-ccw: write cmd region failed with errno=%d", 
> > errno);
> >           ret = -errno;
> >       } else {
> > -        ret = region->ret_code;
> > +        ret = 0;
> >       }
> >       switch (ret) {
> >       case 0:
> > @@ -234,7 +234,7 @@ again:
> >           error_report("vfio-ccw: write cmd region failed with errno=%d", 
> > errno);
> >           ret = -errno;
> >       } else {
> > -        ret = region->ret_code;
> > +        ret = 0;
> >       }
> >       switch (ret) {
> >       case 0: