[PATCH] x86/hvm: Improve error information in handle_pio()

[PATCH] x86/hvm: Improve error information in handle_pio()

[Andrew Cooper]

domain_crash() should always have a message which emitted even in release
builds, so something more useful than this is presented.

  (XEN) domain_crash called from io.c:171
  (XEN) domain_crash called from io.c:171
  (XEN) domain_crash called from io.c:171
  ...

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Part of a bug reported by Marek.  Something else is wonky in the IO emulation
state, and preventing us from yielding to the scheduler so the domain can
progress with being shut down.
---
 xen/arch/x86/hvm/io.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index a5b0a23f06..4e468bfb6b 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -167,7 +167,9 @@ bool handle_pio(uint16_t port, unsigned int size, int dir)
         break;
 
     default:
-        gdprintk(XENLOG_ERR, "Weird HVM ioemulation status %d.\n", rc);
+        gprintk(XENLOG_ERR, "Unexpected PIO status %d, port %#x %s 0x%0*lx\n",
+                rc, port, dir == IOREQ_WRITE ? "write" : "read",
+                size * 2, data & ((1ul << (size * 8)) - 1));
         domain_crash(curr->domain);
         return false;
     }
-- 
2.11.0

Re: [PATCH] x86/hvm: Improve error information in handle_pio()

[Roger Pau Monné]

On Thu, May 28, 2020 at 02:07:38PM +0100, Andrew Cooper wrote:
> domain_crash() should always have a message which emitted even in release
> builds, so something more useful than this is presented.
> 
>   (XEN) domain_crash called from io.c:171
>   (XEN) domain_crash called from io.c:171
>   (XEN) domain_crash called from io.c:171
>   ...
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> 
> Part of a bug reported by Marek.  Something else is wonky in the IO emulation
> state, and preventing us from yielding to the scheduler so the domain can
> progress with being shut down.
> ---
>  xen/arch/x86/hvm/io.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index a5b0a23f06..4e468bfb6b 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -167,7 +167,9 @@ bool handle_pio(uint16_t port, unsigned int size, int dir)
>          break;
>  
>      default:
> -        gdprintk(XENLOG_ERR, "Weird HVM ioemulation status %d.\n", rc);
> +        gprintk(XENLOG_ERR, "Unexpected PIO status %d, port %#x %s 0x%0*lx\n",
> +                rc, port, dir == IOREQ_WRITE ? "write" : "read",
> +                size * 2, data & ((1ul << (size * 8)) - 1));

I wonder, should data be initialized to 0 in order to prevent writing
garbage here if the buffer is not filled in the read case?

Note sure it's better to print garbage or just 0 in that case, as in
both cases it won't be possible to figure out if it's real data or
just the emulation didn't get to fill it (unless the specific error
path is checked).

Thanks, Roger.

Re: [PATCH] x86/hvm: Improve error information in handle_pio()

[Jan Beulich]

On 28.05.2020 15:07, Andrew Cooper wrote:
> domain_crash() should always have a message which emitted even in release
> builds, so something more useful than this is presented.
> 
>   (XEN) domain_crash called from io.c:171
>   (XEN) domain_crash called from io.c:171
>   (XEN) domain_crash called from io.c:171
>   ...
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -167,7 +167,9 @@ bool handle_pio(uint16_t port, unsigned int size, int dir)
>          break;
>  
>      default:
> -        gdprintk(XENLOG_ERR, "Weird HVM ioemulation status %d.\n", rc);
> +        gprintk(XENLOG_ERR, "Unexpected PIO status %d, port %#x %s 0x%0*lx\n",
> +                rc, port, dir == IOREQ_WRITE ? "write" : "read",
> +                size * 2, data & ((1ul << (size * 8)) - 1));

I agree with Roger that potentially logging rubbish for IOREQ_READ
may be confusing, so initializing "data" might end up being better.
Perhaps simply drop (or put in a comment) the
"if ( dir == IOREQ_WRITE )" at the top of the function? (As an
aside, it's also odd for "data" to be "unsigned long" rather than
just "unsigned int" or, less preferable, "uint32_t".)

Jan

Re: [PATCH] x86/hvm: Improve error information in handle_pio()

[Jan Beulich]

On 28.05.2020 15:07, Andrew Cooper wrote:
> domain_crash() should always have a message which emitted even in release

Oh, forgot to say: The wording here looks somewhat strange (and thus
unclear) to me.

> builds, so something more useful than this is presented.
> 
>   (XEN) domain_crash called from io.c:171
>   (XEN) domain_crash called from io.c:171
>   (XEN) domain_crash called from io.c:171
>   ...
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH] x86/hvm: Improve error information in handle_pio()

[Andrew Cooper]

On 29/05/2020 15:57, Jan Beulich wrote:
> On 28.05.2020 15:07, Andrew Cooper wrote:
>> domain_crash() should always have a message which emitted even in release
> Oh, forgot to say: The wording here looks somewhat strange (and thus
> unclear) to me.

It should read "which is emitted", but this is basically a stopgap fix
seeing as I still haven't adjusted domain_crash() to force a message to
be provided.

~Andrew