| Message ID | 20200528130738.12816-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/hvm: Improve error information in handle_pio() | expand |
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.
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
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>
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
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; }
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(-)