[PATCH v2] ioreq: don't wrongly claim "success" in ioreq_send_buffered()

Jan Beulich posted 1 patch 1 month, 3 weeks ago
Failed in applying to current master (apply log)
[PATCH v2] ioreq: don't wrongly claim "success" in ioreq_send_buffered()
Posted by Jan Beulich 1 month, 3 weeks ago
Returning a literal number is a bad idea anyway when all other returns
use IOREQ_STATUS_* values. The function is dead on Arm, and mapping to
X86EMUL_OKAY is surely wrong on x86.

Fixes: f6bf39f84f82 ("x86/hvm: add support for broadcast of buffered ioreqs...")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Shouldn't IOREQ_READ requests also be rejected here, for the result of
a read not possibly coming from anywhere, yet a (bogus) caller then
assuming some data was actually returned?

ioreq_send_buffered() being built on Arm is a violation of Misra rule
2.1, which apparently Eclair doesn't flag (the rule is marked clean).
---
v2: Use IOREQ_STATUS_UNHANDLED.

--- unstable.orig/xen/common/ioreq.c	2024-09-30 12:22:03.759445625 +0200
+++ unstable/xen/common/ioreq.c	2024-09-30 12:24:06.516408920 +0200
@@ -1175,7 +1175,7 @@ static int ioreq_send_buffered(struct io
         return IOREQ_STATUS_UNHANDLED;
 
     /*
-     * Return 0 for the cases we can't deal with:
+     * Return UNHANDLED for the cases we can't deal with:
      *  - 'addr' is only a 20-bit field, so we cannot address beyond 1MB
      *  - we cannot buffer accesses to guest memory buffers, as the guest
      *    may expect the memory buffer to be synchronously accessed
@@ -1183,7 +1183,7 @@ static int ioreq_send_buffered(struct io
      *    support data_is_ptr we do not waste space for the count field either
      */
     if ( (p->addr > 0xfffffUL) || p->data_is_ptr || (p->count != 1) )
-        return 0;
+        return IOREQ_STATUS_UNHANDLED;
 
     switch ( p->size )
     {
Re: [PATCH v2] ioreq: don't wrongly claim "success" in ioreq_send_buffered()
Posted by Julien Grall 1 month, 2 weeks ago
Hi Jan,

On 30/09/2024 11:28, Jan Beulich wrote:
> Returning a literal number is a bad idea anyway when all other returns
> use IOREQ_STATUS_* values. The function is dead on Arm, and mapping to
> X86EMUL_OKAY is surely wrong on x86.
> 
> Fixes: f6bf39f84f82 ("x86/hvm: add support for broadcast of buffered ioreqs...")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall