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

[Eric Farman <farman@linux.ibm.com>]

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:
>