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

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

[Eric Farman]

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.

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

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

[Cornelia Huck]

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 don't suppose we need to handle a hypothetical broken kernel that
returns the wrong size with errno==0?

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

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