x86/hvm: Improve error information in handle_pio()
diff mbox series

Message ID 20200528130738.12816-1-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86/hvm: Improve error information in handle_pio()
Related show

Commit Message

Andrew Cooper May 28, 2020, 1:07 p.m. UTC
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(-)

Comments

Roger Pau Monné May 28, 2020, 4 p.m. UTC | #1
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.
Jan Beulich May 29, 2020, 2:46 p.m. UTC | #2
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
Jan Beulich May 29, 2020, 2:57 p.m. UTC | #3
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>
Andrew Cooper May 29, 2020, 3:09 p.m. UTC | #4
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

Patch
diff mbox series

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;
     }