Lockup with --accel tcg,thread=single
I found a lockup in single threaded TCG, with OVMF bios, 16 CPUs. qemu-system-x86_64 --accel tcg,thread=single -smp cpus=16 -bios /usr/share/ovmf/OVMF.fd Using Ubuntu 18.04 LTS, default gnome desktop. There is no guest OS, there is no hard drive, just the OVMF firmware locks it up. (I narrowed it down to this from a much larger repro) Let that run for about 10 seconds or so, then attempt to Ctrl-C in the terminal that launched qemu. You should see the hung program thing (wait or force quit) appear on the qemu window, and the terminal just prints ^C without interrupting qemu. DO NOT click force quit or wait. Use killall -9 qemu-system-x86_64 to kill it if you must, gdb kill is also okay. If you force quit you will likely freeze the entire gnome desktop. This is what kmsg reports: https://gist.githubusercontent.com/doug65536/810e0471c11008c97edb4aa0d86ddd66/raw/40038a355542f0edb1f14a7c7fb7c2db60da0025/end%2520of%2520syslog%2520after%2520desktop%2520lockup, this is what syslog reports: https://gist.githubusercontent.com/doug65536/810e0471c11008c97edb4aa0d86ddd66/raw/40038a355542f0edb1f14a7c7fb7c2db60da0025/the%2520end%2520of%2520dmesg%2520after%2520desktop%2520lockup. Probably bugs in gnome but just warning about locking up your machines when reproducing. Peter Maydell helped me bisect it in IRC. Works fine at commit 1e8a98b53867f61 Fails at commit 9458a9a1df1a4c7 MTTCG works fine all the way up to master. Configure command line: ../qemu/configure --target-list=x86_64-softmmu,i386-softmmu --audio-drv-list=pa --enable-libusb --disable-libssh --enable-virglrenderer --enable-opengl --enable-debug The issue occurs without --enable-debug. I didn't strip the configure down though, it may not need all of those configure options exactly. OVMF from ubuntu package manager, package named ovmf, exact version is 0~20180205.c0d9813c-2ubuntu0.1 Stack trace at lockup at https://gist.githubusercontent.com/doug65536/810e0471c11008c97edb4aa0d86ddd66/raw/40038a355542f0edb1f14a7c7fb7c2db60da0025/backtrace%2520all
Lockup with --accel tcg,thread=single
I found a lockup in single threaded TCG, with OVMF bios, 16 CPUs. qemu-system-x86_64 --accel tcg,thread=single -smp cpus=16 -bios /usr/share/ovmf/OVMF.fd Using Ubuntu 18.04 LTS, default gnome desktop. There is no guest OS, there is no hard drive, just the OVMF firmware locks it up. (I narrowed it down to this from a much larger repro) Let that run for about 10 seconds or so, then attempt to Ctrl-C in the terminal that launched qemu. You should see the hung program thing (wait or force quit) appear on the qemu window, and the terminal just prints ^C without interrupting qemu. DO NOT click force quit or wait. Use killall -9 qemu-system-x86_64 to kill it if you must, gdb kill is also okay. If you force quit you will likely freeze the entire gnome desktop. This is what kmsg reports: https://gist.githubusercontent.com/doug65536/810e0471c11008c97edb4aa0d86ddd66/raw/40038a355542f0edb1f14a7c7fb7c2db60da0025/end%2520of%2520syslog%2520after%2520desktop%2520lockup, this is what syslog reports: https://gist.githubusercontent.com/doug65536/810e0471c11008c97edb4aa0d86ddd66/raw/40038a355542f0edb1f14a7c7fb7c2db60da0025/the%2520end%2520of%2520dmesg%2520after%2520desktop%2520lockup. Probably bugs in gnome but just warning about locking up your machines when reproducing. Peter Maydell helped me bisect it in IRC. Works fine at commit 1e8a98b53867f61 Fails at commit 9458a9a1df1a4c7 MTTCG works fine all the way up to master. Configure command line: ../qemu/configure --target-list=x86_64-softmmu,i386-softmmu --audio-drv-list=pa --enable-libusb --disable-libssh --enable-virglrenderer --enable-opengl --enable-debug The issue occurs without --enable-debug. I didn't strip the configure down though, it may not need all of those configure options exactly. OVMF from ubuntu package manager, package named ovmf, exact version is 0~20180205.c0d9813c-2ubuntu0.1 Stack trace at lockup at https://gist.githubusercontent.com/doug65536/810e0471c11008c97edb4aa0d86ddd66/raw/40038a355542f0edb1f14a7c7fb7c2db60da0025/backtrace%2520all
Re: [Qemu-devel] [PATCH] gdbstub: Fix i386/x86_64 machine description and add control registers
On Fri, Jan 25, 2019 at 6:22 AM Peter Maydell wrote: > > Thanks for this explanation -- the patch makes a lot more sense with it. > I'm confused though -- the XML we ship is basically what gdb itself > ships and uses internally: > > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/features/i386/i386.xml;h=beb1496d9773efcf0e0526dc540a5e206a2e21fc;hb=HEAD > > and that uses xi:include to pull in the other files. > So it seems odd that gdb can't parse the XML it is using > itself internally. Maybe QEMU is doing something else wrong > somewhere? > > I thought that too. I implemented gdbstub tracing a while ago and had it enabled (-trace gdb*). The binary data it sends is exactly what I expect. When I break into gdb, I checked the data and it looked correct. debug_xml even prints out what it's going to parse. Looks fine. GDB calls into an XML parser library, which I don't have built from source, so I didn't find precisely where it fails, only the high level log showing unexpected elements at nested feature.
Re: [Qemu-devel] [PATCH] gdbstub: Fix i386/x86_64 machine description and add control registers
Here is the sequence that led to this patch: - Debugging my kernel, I wished I could use conditional breapoints to put a condition on CR3 to make breakpoints in user processes only break when they are the active address space. - Knew it was no problem to add a register to GDB stub - Added CR3. While there, added CR0 and CR2 and CR4 and CR8 and EFER - Attempted to use it. Dreaded g packet error. - Setup to debug GDB. Step tdesc code. Find it failing and silently dropping description. - Changed register count in gdbstub to match what default machine description would want. No more error. GDB definitely dropping to default. - Learn more about GDB XML parser. Find it has a verbose parser log. set debug_xml=1 - Watch in horror as parser gets messed up parsing nesting due to feature including feature - Wonder if any QEMU machine descriptions xi:include actually work or if all architectures are lined up to coincidentally use the same register numbers as the default. - Run everything through XML validators that strictly enforce DTD and everything, no problem, totally valid. - Try several changes to make include work. Not working. - Move the content of the includes into the top level file so no include. Bingo, works. - Discover that fsbase and gsbase now working because the entire machine description is correctly implemented, makes the register window in qtcreator suddenly work great - Make sure 32 bit works too. Find a couple of issues. Fix. - Submit On Thu, Jan 24, 2019 at 4:59 PM Doug Gale wrote: > The machine description we send is being (silently) thrown on the floor by > GDB and GDB silently uses the default machine description. > > With current QEMU, if you debug gdb, and set debug_xml=1 and continue, > then attach to qemu gdbstub from the debugged gdb, you will see the xml > parse fail completely, and gdb will fall back to the default machine > description, silently, and changes to our xml (in qemu source code) have no > effect. They might as well be empty. > > The point of fixing the machine description was IDE's with GDB integration > will break on QEMU. The default machine description has fs_base, which > fails to be retrieved, whick breaks the whole register window (in > qt-creator at least, likely others). With my patch the register window > works perfectly. > > I didn't delete anything, I removed the superfluous nesting of files by > xi:include and moved the description into a single xml file. I added > fs_base, gs_base, k_gs_base, cr0/2/3/4/6, efer. > > Removing the nesting into xml includes fixes it because the xml parse > fails on unnecessary include indirections and placed the data inline. > > I tried lots of things to fix the nesting. After a while I asked, why > bother? It doesn't need to go through that include level of indirection > does it? > > This patch leads to another patch I want to submit later, which fixes the > broken real mode and protected mode debugging on x86_64. I want to add the > ability to turn off the "always 64 bit registers" hack that works around > 'g' packet too large. You fix that by patching gdb (which then reacts to > packet to large with realloc instead of freaking out), not by breaking real > and protected mode debugging on x86_64 target by forcing always-64-bit. My > intention is to default to current behavior and have a command line switch > that enables changing register sizes (fixing real and protected mode > debugging on x86_64). > > This patch includes a FORCE_64 define that will be replaced by a check for > the option that indicates patched gdb and "register size change ok mode". > > > On Thu, Jan 24, 2019 at 6:44 AM Peter Maydell > wrote: > >> On Thu, 24 Jan 2019 at 04:08, Doug Gale wrote: >> > >> > Signed-off-by: Doug Gale >> > --- >> > configure | 4 +- >> > gdb-xml/i386-32bit-core.xml | 65 --- >> > gdb-xml/i386-32bit-sse.xml | 52 - >> > gdb-xml/i386-32bit.xml | 184 ++- >> > gdb-xml/i386-64bit-core.xml | 73 - >> > gdb-xml/i386-64bit-sse.xml | 60 --- >> > gdb-xml/i386-64bit.xml | 210 +++- >> > target/i386/cpu.c | 4 +- >> > target/i386/gdbstub.c | 186 +++- >> > 9 files changed, 573 insertions(+), 265 deletions(-) >> > delete mode 100644 gdb-xml/i386-32bit-core.xml >> > delete mode 100644 gdb-xml/i386-32bit-sse.xml >> > delete mode 100644 gdb-xml/i386-64bit-core.xml >> > delete mode 100644 gdb-xml/i386-64bit-sse.xml >> >> Could you provide a commit message that explains what's >> wrong with the machine description we have (ie what bug >> or bugs this change is fixing) and why deleting half >> the xml files is the right way to fix it, please? >> >> Does the "add control registers" part need to be in >> the same patch, or is it a separate feature which >> could be in its own patch ? >> >> thanks >> -- PMM >> >
Re: [Qemu-devel] [PATCH] gdbstub: Fix i386/x86_64 machine description and add control registers
The machine description we send is being (silently) thrown on the floor by GDB and GDB silently uses the default machine description. With current QEMU, if you debug gdb, and set debug_xml=1 and continue, then attach to qemu gdbstub from the debugged gdb, you will see the xml parse fail completely, and gdb will fall back to the default machine description, silently, and changes to our xml (in qemu source code) have no effect. They might as well be empty. The point of fixing the machine description was IDE's with GDB integration will break on QEMU. The default machine description has fs_base, which fails to be retrieved, whick breaks the whole register window (in qt-creator at least, likely others). With my patch the register window works perfectly. I didn't delete anything, I removed the superfluous nesting of files by xi:include and moved the description into a single xml file. I added fs_base, gs_base, k_gs_base, cr0/2/3/4/6, efer. Removing the nesting into xml includes fixes it because the xml parse fails on wrote: > On Thu, 24 Jan 2019 at 04:08, Doug Gale wrote: > > > > Signed-off-by: Doug Gale > > --- > > configure | 4 +- > > gdb-xml/i386-32bit-core.xml | 65 --- > > gdb-xml/i386-32bit-sse.xml | 52 - > > gdb-xml/i386-32bit.xml | 184 ++- > > gdb-xml/i386-64bit-core.xml | 73 - > > gdb-xml/i386-64bit-sse.xml | 60 --- > > gdb-xml/i386-64bit.xml | 210 +++- > > target/i386/cpu.c | 4 +- > > target/i386/gdbstub.c | 186 +++- > > 9 files changed, 573 insertions(+), 265 deletions(-) > > delete mode 100644 gdb-xml/i386-32bit-core.xml > > delete mode 100644 gdb-xml/i386-32bit-sse.xml > > delete mode 100644 gdb-xml/i386-64bit-core.xml > > delete mode 100644 gdb-xml/i386-64bit-sse.xml > > Could you provide a commit message that explains what's > wrong with the machine description we have (ie what bug > or bugs this change is fixing) and why deleting half > the xml files is the right way to fix it, please? > > Does the "add control registers" part need to be in > the same patch, or is it a separate feature which > could be in its own patch ? > > thanks > -- PMM >
[Qemu-devel] [PATCH] gdbstub: Fix i386/x86_64 machine description and add control registers
Signed-off-by: Doug Gale --- configure | 4 +- gdb-xml/i386-32bit-core.xml | 65 --- gdb-xml/i386-32bit-sse.xml | 52 - gdb-xml/i386-32bit.xml | 184 ++- gdb-xml/i386-64bit-core.xml | 73 - gdb-xml/i386-64bit-sse.xml | 60 --- gdb-xml/i386-64bit.xml | 210 +++- target/i386/cpu.c | 4 +- target/i386/gdbstub.c | 186 +++- 9 files changed, 573 insertions(+), 265 deletions(-) delete mode 100644 gdb-xml/i386-32bit-core.xml delete mode 100644 gdb-xml/i386-32bit-sse.xml delete mode 100644 gdb-xml/i386-64bit-core.xml delete mode 100644 gdb-xml/i386-64bit-sse.xml diff --git a/configure b/configure index 4ea3f14883..c55a97b91c 100755 --- a/configure +++ b/configure @@ -7121,14 +7121,14 @@ TARGET_ABI_DIR="" case "$target_name" in i386) mttcg="yes" -gdb_xml_files="i386-32bit.xml i386-32bit-core.xml i386-32bit-sse.xml" + gdb_xml_files="i386-32bit.xml" target_compiler=$cross_cc_i386 target_compiler_cflags=$cross_cc_ccflags_i386 ;; x86_64) TARGET_BASE_ARCH=i386 mttcg="yes" -gdb_xml_files="i386-64bit.xml i386-64bit-core.xml i386-64bit-sse.xml" + gdb_xml_files="i386-64bit.xml" target_compiler=$cross_cc_x86_64 ;; alpha) diff --git a/gdb-xml/i386-32bit-core.xml b/gdb-xml/i386-32bit-core.xml deleted file mode 100644 index 7aeeeca3b2..00 --- a/gdb-xml/i386-32bit-core.xml +++ /dev/null @@ -1,65 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - diff --git a/gdb-xml/i386-32bit-sse.xml b/gdb-xml/i386-32bit-sse.xml deleted file mode 100644 index 57678473d6..00 --- a/gdb-xml/i386-32bit-sse.xml +++ /dev/null @@ -1,52 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - diff --git a/gdb-xml/i386-32bit.xml b/gdb-xml/i386-32bit.xml index 956fc7f45f..872fcea9c2 100644 --- a/gdb-xml/i386-32bit.xml +++ b/gdb-xml/i386-32bit.xml @@ -8,7 +8,185 @@ - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/gdb-xml/i386-64bit-core.xml b/gdb-xml/i386-64bit-core.xml deleted file mode 100644 index 5088d84ceb..00 --- a/gdb-xml/i386-64bit-core.xml +++ /dev/null @@ -1,73 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - diff --git a/gdb-xml/i386-64bit-sse.xml b/gdb-xml/i386-64bit-sse.xml deleted file mode 100644 index e86efc9ce5..00 --- a/gdb-xml/i386-64bit-sse.xml +++ /dev/null @@ -1,60 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - diff --git a/gdb-xml/i386-64bit.xml b/gdb-xml/i386-64bit.xml index 0b2f00ccbe..6d88969211 100644 --- a/gdb-xml/i386-64bit.xml +++ b/gdb-xml/i386-64bit.xml @@ -5,10 +5,212 @@ are permitted in any medium w
Re: [Qemu-devel] [PATCH] i386/monitor.c: make addresses canonical for "info mem" and "info tlb"
On Sun, Jun 17, 2018 at 4:40 AM, Doug Gale wrote:
> Correct the output of the "info mem" and "info tlb" monitor commands to
> correctly show canonical addresses.
>
> In 48-bit addressing mode, the upper 16 bits of linear addresses are
> equal to bit 47. In 57-bit addressing mode (LA57), the upper 7 bits of
> linear addresses are equal to bit 56.
>
> Signed-off-by: Doug Gale
> ---
> target/i386/monitor.c | 76 +--
> 1 file changed, 44 insertions(+), 32 deletions(-)
>
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index a890b3c2ab..99c97a63e2 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -35,21 +35,28 @@
> #include "sev_i386.h"
> #include "qapi/qapi-commands-misc.h"
>
> -
> -static void print_pte(Monitor *mon, CPUArchState *env, hwaddr addr,
> - hwaddr pte, hwaddr mask)
> +/* Perform linear address sign extension */
> +static hwaddr addr_canonical(CPUArchState *env, hwaddr addr)
> {
> #ifdef TARGET_X86_64
> if (env->cr[4] & CR4_LA57_MASK) {
> if (addr & (1ULL << 56)) {
> -addr |= -1LL << 57;
> +addr |= (hwaddr)-(1LL << 57);
> }
> } else {
> if (addr & (1ULL << 47)) {
> -addr |= -1LL << 48;
> +addr |= (hwaddr)-(1LL << 48);
> }
> }
> #endif
> +return addr;
> +}
> +
> +static void print_pte(Monitor *mon, CPUArchState *env, hwaddr addr,
> + hwaddr pte, hwaddr mask)
> +{
> +addr = addr_canonical(env, addr);
> +
> monitor_printf(mon, TARGET_FMT_plx ": " TARGET_FMT_plx
> " %c%c%c%c%c%c%c%c%c\n",
> addr,
> @@ -243,8 +250,8 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
> }
> }
>
> -static void mem_print(Monitor *mon, hwaddr *pstart,
> - int *plast_prot,
> +static void mem_print(Monitor *mon, CPUArchState *env,
> + hwaddr *pstart, int *plast_prot,
>hwaddr end, int prot)
> {
> int prot1;
> @@ -253,7 +260,9 @@ static void mem_print(Monitor *mon, hwaddr *pstart,
> if (*pstart != -1) {
> monitor_printf(mon, TARGET_FMT_plx "-" TARGET_FMT_plx " "
> TARGET_FMT_plx " %c%c%c\n",
> - *pstart, end, end - *pstart,
> + addr_canonical(env, *pstart),
> + addr_canonical(env, end),
> + addr_canonical(env, end - *pstart),
> prot1 & PG_USER_MASK ? 'u' : '-',
> 'r',
> prot1 & PG_RW_MASK ? 'w' : '-');
> @@ -283,7 +292,7 @@ static void mem_info_32(Monitor *mon, CPUArchState
> *env)
> if (pde & PG_PRESENT_MASK) {
> if ((pde & PG_PSE_MASK) && (env->cr[4] & CR4_PSE_MASK)) {
> prot = pde & (PG_USER_MASK | PG_RW_MASK |
> PG_PRESENT_MASK);
> -mem_print(mon, &start, &last_prot, end, prot);
> +mem_print(mon, env, &start, &last_prot, end, prot);
> } else {
> for(l2 = 0; l2 < 1024; l2++) {
> cpu_physical_memory_read((pde & ~0xfff) + l2 * 4,
> &pte, 4);
> @@ -295,16 +304,16 @@ static void mem_info_32(Monitor *mon, CPUArchState
> *env)
> } else {
> prot = 0;
> }
> -mem_print(mon, &start, &last_prot, end, prot);
> +mem_print(mon, env, &start, &last_prot, end, prot);
> }
> }
> } else {
> prot = 0;
> -mem_print(mon, &start, &last_prot, end, prot);
> +mem_print(mon, env, &start, &last_prot, end, prot);
> }
> }
> /* Flush last range */
> -mem_print(mon, &start, &last_prot, (hwaddr)1 << 32, 0);
> +mem_print(mon, env, &start, &last_prot, (hwaddr)1 << 32, 0);
> }
>
> static void mem_info_pae32(Monitor *mon, CPUArchState *env)
> @@ -332,7 +341,7 @@ static void mem_info_pae32(Monitor *mon, CPUArchState
> *env)
> if (pde & PG_PSE_MASK) {
> prot = pde & (PG_USER_MASK | PG_RW_MASK |
>PG_PRESENT_MASK);
> -
[Qemu-devel] [PATCH] i386/monitor.c: make addresses canonical for "info mem" and "info tlb"
Correct the output of the "info mem" and "info tlb" monitor commands to
correctly show canonical addresses.
In 48-bit addressing mode, the upper 16 bits of linear addresses are
equal to bit 47. In 57-bit addressing mode (LA57), the upper 7 bits of
linear addresses are equal to bit 56.
Signed-off-by: Doug Gale
---
target/i386/monitor.c | 76 +--
1 file changed, 44 insertions(+), 32 deletions(-)
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index a890b3c2ab..99c97a63e2 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -35,21 +35,28 @@
#include "sev_i386.h"
#include "qapi/qapi-commands-misc.h"
-
-static void print_pte(Monitor *mon, CPUArchState *env, hwaddr addr,
- hwaddr pte, hwaddr mask)
+/* Perform linear address sign extension */
+static hwaddr addr_canonical(CPUArchState *env, hwaddr addr)
{
#ifdef TARGET_X86_64
if (env->cr[4] & CR4_LA57_MASK) {
if (addr & (1ULL << 56)) {
-addr |= -1LL << 57;
+addr |= (hwaddr)-(1LL << 57);
}
} else {
if (addr & (1ULL << 47)) {
-addr |= -1LL << 48;
+addr |= (hwaddr)-(1LL << 48);
}
}
#endif
+return addr;
+}
+
+static void print_pte(Monitor *mon, CPUArchState *env, hwaddr addr,
+ hwaddr pte, hwaddr mask)
+{
+addr = addr_canonical(env, addr);
+
monitor_printf(mon, TARGET_FMT_plx ": " TARGET_FMT_plx
" %c%c%c%c%c%c%c%c%c\n",
addr,
@@ -243,8 +250,8 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
}
}
-static void mem_print(Monitor *mon, hwaddr *pstart,
- int *plast_prot,
+static void mem_print(Monitor *mon, CPUArchState *env,
+ hwaddr *pstart, int *plast_prot,
hwaddr end, int prot)
{
int prot1;
@@ -253,7 +260,9 @@ static void mem_print(Monitor *mon, hwaddr *pstart,
if (*pstart != -1) {
monitor_printf(mon, TARGET_FMT_plx "-" TARGET_FMT_plx " "
TARGET_FMT_plx " %c%c%c\n",
- *pstart, end, end - *pstart,
+ addr_canonical(env, *pstart),
+ addr_canonical(env, end),
+ addr_canonical(env, end - *pstart),
prot1 & PG_USER_MASK ? 'u' : '-',
'r',
prot1 & PG_RW_MASK ? 'w' : '-');
@@ -283,7 +292,7 @@ static void mem_info_32(Monitor *mon, CPUArchState *env)
if (pde & PG_PRESENT_MASK) {
if ((pde & PG_PSE_MASK) && (env->cr[4] & CR4_PSE_MASK)) {
prot = pde & (PG_USER_MASK | PG_RW_MASK | PG_PRESENT_MASK);
-mem_print(mon, &start, &last_prot, end, prot);
+mem_print(mon, env, &start, &last_prot, end, prot);
} else {
for(l2 = 0; l2 < 1024; l2++) {
cpu_physical_memory_read((pde & ~0xfff) + l2 * 4, &pte, 4);
@@ -295,16 +304,16 @@ static void mem_info_32(Monitor *mon, CPUArchState *env)
} else {
prot = 0;
}
-mem_print(mon, &start, &last_prot, end, prot);
+mem_print(mon, env, &start, &last_prot, end, prot);
}
}
} else {
prot = 0;
-mem_print(mon, &start, &last_prot, end, prot);
+mem_print(mon, env, &start, &last_prot, end, prot);
}
}
/* Flush last range */
-mem_print(mon, &start, &last_prot, (hwaddr)1 << 32, 0);
+mem_print(mon, env, &start, &last_prot, (hwaddr)1 << 32, 0);
}
static void mem_info_pae32(Monitor *mon, CPUArchState *env)
@@ -332,7 +341,7 @@ static void mem_info_pae32(Monitor *mon, CPUArchState *env)
if (pde & PG_PSE_MASK) {
prot = pde & (PG_USER_MASK | PG_RW_MASK |
PG_PRESENT_MASK);
-mem_print(mon, &start, &last_prot, end, prot);
+mem_print(mon, env, &start, &last_prot, end, prot);
} else {
pt_addr = pde & 0x3f000ULL;
for (l3 = 0; l3 < 512; l3++) {
@@ -345,21 +354,21 @@ static void mem_info_pae32(Monitor *mon, CPUArchState
*env)
} else {
prot = 0;
}
-mem_print(mon, &start, &a
[Qemu-devel] [Bug 1748296] Re: TCG throws Invalid Opcode when executing x86 BMI shlx instruction
** Description changed: I am unable to use BMI in my project when running under TCG. I narrowed the problem down to incorrect instruction decoding for BMI instructions (which have a 2 byte VEX prefix). The gen_sse function in translate.c reaches the goto label do_0f_38_fx, but b does not equal 0x1f7, 0x2f7, or 0x3f7, so the switch takes the default path and raises an invalid opcode exception. The code executes correctly and passes the test under KVM. I have created a complete repro here: https://github.com/doug65536/qemu- bmibug The makefile has the following utility targets: debug-kvm: Build and run the VM using KVM and wait for gdbstub attach run: Run the test case with TCG, make fails if the test fails. (It will fail) run-kvm: Run the test case with KVM, make fails if the test fails. (It will succeed) debug: Build and run the VM with TCG and wait for GDB attach - attach-gdb: Run GDB and attach to KVM gdbstub + attach-gdb: Run GDB and attach to QEMU gdbstub The VM runs with -cpu max. CPUID reports support for BMI, BMI2, and ABM. You can quickly verify the issue by executing `make run-kvm` to confirm that KVM passes, then `make run` to confirm that TCG fails. I believe the bug affects other BMI, BMI2, and ABM instructions, but I have only completely verified incorrect execution of SHLX. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1748296 Title: TCG throws Invalid Opcode when executing x86 BMI shlx instruction Status in QEMU: New Bug description: I am unable to use BMI in my project when running under TCG. I narrowed the problem down to incorrect instruction decoding for BMI instructions (which have a 2 byte VEX prefix). The gen_sse function in translate.c reaches the goto label do_0f_38_fx, but b does not equal 0x1f7, 0x2f7, or 0x3f7, so the switch takes the default path and raises an invalid opcode exception. The code executes correctly and passes the test under KVM. I have created a complete repro here: https://github.com/doug65536 /qemu-bmibug The makefile has the following utility targets: debug-kvm: Build and run the VM using KVM and wait for gdbstub attach run: Run the test case with TCG, make fails if the test fails. (It will fail) run-kvm: Run the test case with KVM, make fails if the test fails. (It will succeed) debug: Build and run the VM with TCG and wait for GDB attach attach-gdb: Run GDB and attach to QEMU gdbstub The VM runs with -cpu max. CPUID reports support for BMI, BMI2, and ABM. You can quickly verify the issue by executing `make run-kvm` to confirm that KVM passes, then `make run` to confirm that TCG fails. I believe the bug affects other BMI, BMI2, and ABM instructions, but I have only completely verified incorrect execution of SHLX. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1748296/+subscriptions
[Qemu-devel] [Bug 1748296] [NEW] TCG throws Invalid Opcode when executing x86 BMI shlx instruction
Public bug reported: I am unable to use BMI in my project when running under TCG. I narrowed the problem down to incorrect instruction decoding for BMI instructions (which have a 2 byte VEX prefix). The gen_sse function in translate.c reaches the goto label do_0f_38_fx, but b does not equal 0x1f7, 0x2f7, or 0x3f7, so the switch takes the default path and raises an invalid opcode exception. The code executes correctly and passes the test under KVM. I have created a complete repro here: https://github.com/doug65536/qemu- bmibug The makefile has the following utility targets: debug-kvm: Build and run the VM using KVM and wait for gdbstub attach run: Run the test case with TCG, make fails if the test fails. (It will fail) run-kvm: Run the test case with KVM, make fails if the test fails. (It will succeed) debug: Build and run the VM with TCG and wait for GDB attach attach-gdb: Run GDB and attach to KVM gdbstub The VM runs with -cpu max. CPUID reports support for BMI, BMI2, and ABM. You can quickly verify the issue by executing `make run-kvm` to confirm that KVM passes, then `make run` to confirm that TCG fails. I believe the bug affects other BMI, BMI2, and ABM instructions, but I have only completely verified incorrect execution of SHLX. ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1748296 Title: TCG throws Invalid Opcode when executing x86 BMI shlx instruction Status in QEMU: New Bug description: I am unable to use BMI in my project when running under TCG. I narrowed the problem down to incorrect instruction decoding for BMI instructions (which have a 2 byte VEX prefix). The gen_sse function in translate.c reaches the goto label do_0f_38_fx, but b does not equal 0x1f7, 0x2f7, or 0x3f7, so the switch takes the default path and raises an invalid opcode exception. The code executes correctly and passes the test under KVM. I have created a complete repro here: https://github.com/doug65536 /qemu-bmibug The makefile has the following utility targets: debug-kvm: Build and run the VM using KVM and wait for gdbstub attach run: Run the test case with TCG, make fails if the test fails. (It will fail) run-kvm: Run the test case with KVM, make fails if the test fails. (It will succeed) debug: Build and run the VM with TCG and wait for GDB attach attach-gdb: Run GDB and attach to KVM gdbstub The VM runs with -cpu max. CPUID reports support for BMI, BMI2, and ABM. You can quickly verify the issue by executing `make run-kvm` to confirm that KVM passes, then `make run` to confirm that TCG fails. I believe the bug affects other BMI, BMI2, and ABM instructions, but I have only completely verified incorrect execution of SHLX. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1748296/+subscriptions
Re: [Qemu-devel] [PATCH v2] Add AVX, AVX-512, MPX support to x86_cpu_dump_state
On Thu, Dec 7, 2017 at 5:37 PM, Richard Henderson <
richard.hender...@linaro.org> wrote:
> On 12/02/2017 10:35 PM, Doug Gale wrote:
> > Signed-off-by: Doug Gale
> > ---
> > Fix MSB LSB showing when SSE is disabled
> > target/i386/helper.c | 95 ++
> +++---
> > 1 file changed, 83 insertions(+), 12 deletions(-)
> >
> > diff --git a/target/i386/helper.c b/target/i386/helper.c
> > index f63eb3d3f4..03812b6e87 100644
> > --- a/target/i386/helper.c
> > +++ b/target/i386/helper.c
> > @@ -543,6 +543,7 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f,
> fprintf_function cpu_fprintf,
> > }
> > }
> > cpu_fprintf(f, "EFER=%016" PRIx64 "\n", env->efer);
> > +cpu_fprintf(f, "XCR0=%016" PRIx64 "\n", env->xcr0);
> > if (flags & CPU_DUMP_FPU) {
> > int fptag;
> > fptag = 0;
> > @@ -565,21 +566,91 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f,
> fprintf_function cpu_fprintf,
> > else
> > cpu_fprintf(f, " ");
> > }
> > -if (env->hflags & HF_CS64_MASK)
> > -nb = 16;
> > -else
> > +
> > +if (env->hflags & HF_CS64_MASK) {
> > +if (env->xcr0 & XSTATE_Hi16_ZMM_MASK) {
> > +/* AVX-512 32 registers enabled */
> > +nb = 32;
> > +} else {
> > +/* 64-bit mode, 16 registers */
> > +nb = 16;
> > +}
> > +} else {
> > +/* 32 bit mode, 8 registers */
> > nb = 8;
> > -for(i=0;i > -cpu_fprintf(f, "XMM%02d=%08x%08x%08x%08x",
> > -i,
> > -env->xmm_regs[i].ZMM_L(3),
> > -env->xmm_regs[i].ZMM_L(2),
> > -env->xmm_regs[i].ZMM_L(1),
> > -env->xmm_regs[i].ZMM_L(0));
> > -if ((i & 1) == 1)
> > +}
> > +
> > +/* sse register width in units of 64 bits */
> > +int zmm_width;
> > +char zmm_name;
> > +if (env->xcr0 & XSTATE_ZMM_Hi256_MASK) {
> > +/* 512-bit "ZMM" - AVX-512 registers enabled */
> > +zmm_width = 8;
> > +zmm_name = 'Z';
> > +} else if (env->xcr0 & XSTATE_YMM_MASK) {
> > +/* 256-bit "YMM" - AVX enabled */
> > +zmm_width = 4;
> > +zmm_name = 'Y';
> > +} else if (env->cr[4] & CR4_OSFXSR_MASK) {
> > +/* 128-bit "XMM" - SSE enabled */
> > +zmm_width = 2;
> > +zmm_name = 'X';
> > +} else {
> > +/* SSE not enabled */
> > +zmm_width = 0;
> > +zmm_name = 0;
>
> You should exit the function here ...
>
> > +}
> > +
> > +if (zmm_width > 0) {
> > +cpu_fprintf(f, " MSB%*sLSB\n",
> > +-(zmm_width * 16 + zmm_width - 7), "");
> > +}
> > +
> > +for (i = 0; i < nb; i++) {
> > +if (zmm_width == 0) {
> > +cpu_fprintf(f, "SSE not enabled\n");
> > +break;
> > +}
>
> ... rather than performing this test in a loop.
> Or indeed, printing anything at all.
>
> I assume this is in order to examine registers in the monitor under kvm?
>
>
> r~
>
Yes, to examine the registers in the monitor and improve the output of
things like -d int. I ran into an issue with
guest
code and I wanted to check XCR0 to see if AVX was enabled, and I didn't see
any AVX registers
, which was very misleading
.
I couldn't check XCR0 to see if it was enabled either.
I investigated and found that AVX was enabled, and the monitor output was
false. I added XCR0 and the entire AVX
/AVX-512
and MPX state to the
qemu
output.
Why would it exit the function there? Are you sure the other states I
further down won't be enabled? It prints the MPX state further down. I put
the test in the loop so I wouldn't have to indent that whole `for` loop
another level, to improve readability. Is the performance of that loop
really relevant? The fprintf's will take at least thousands of times more
CPU time than that predictable branch.
[Qemu-devel] [PATCH v2] Add AVX, AVX-512, MPX support to x86_cpu_dump_state
Signed-off-by: Doug Gale
---
Fix MSB LSB showing when SSE is disabled
target/i386/helper.c | 95 +---
1 file changed, 83 insertions(+), 12 deletions(-)
diff --git a/target/i386/helper.c b/target/i386/helper.c
index f63eb3d3f4..03812b6e87 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -543,6 +543,7 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f,
fprintf_function cpu_fprintf,
}
}
cpu_fprintf(f, "EFER=%016" PRIx64 "\n", env->efer);
+cpu_fprintf(f, "XCR0=%016" PRIx64 "\n", env->xcr0);
if (flags & CPU_DUMP_FPU) {
int fptag;
fptag = 0;
@@ -565,21 +566,91 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f,
fprintf_function cpu_fprintf,
else
cpu_fprintf(f, " ");
}
-if (env->hflags & HF_CS64_MASK)
-nb = 16;
-else
+
+if (env->hflags & HF_CS64_MASK) {
+if (env->xcr0 & XSTATE_Hi16_ZMM_MASK) {
+/* AVX-512 32 registers enabled */
+nb = 32;
+} else {
+/* 64-bit mode, 16 registers */
+nb = 16;
+}
+} else {
+/* 32 bit mode, 8 registers */
nb = 8;
-for(i=0;ixmm_regs[i].ZMM_L(3),
-env->xmm_regs[i].ZMM_L(2),
-env->xmm_regs[i].ZMM_L(1),
-env->xmm_regs[i].ZMM_L(0));
-if ((i & 1) == 1)
+}
+
+/* sse register width in units of 64 bits */
+int zmm_width;
+char zmm_name;
+if (env->xcr0 & XSTATE_ZMM_Hi256_MASK) {
+/* 512-bit "ZMM" - AVX-512 registers enabled */
+zmm_width = 8;
+zmm_name = 'Z';
+} else if (env->xcr0 & XSTATE_YMM_MASK) {
+/* 256-bit "YMM" - AVX enabled */
+zmm_width = 4;
+zmm_name = 'Y';
+} else if (env->cr[4] & CR4_OSFXSR_MASK) {
+/* 128-bit "XMM" - SSE enabled */
+zmm_width = 2;
+zmm_name = 'X';
+} else {
+/* SSE not enabled */
+zmm_width = 0;
+zmm_name = 0;
+}
+
+if (zmm_width > 0) {
+cpu_fprintf(f, " MSB%*sLSB\n",
+-(zmm_width * 16 + zmm_width - 7), "");
+}
+
+for (i = 0; i < nb; i++) {
+if (zmm_width == 0) {
+cpu_fprintf(f, "SSE not enabled\n");
+break;
+}
+
+cpu_fprintf(f, "%cMM%02d=", zmm_name, i);
+int qw;
+for (qw = zmm_width; qw > 0; qw -= 2) {
+/* ':' separator every 64 bits */
+cpu_fprintf(f, "%016" PRIx64 ":%016" PRIx64 "%s",
+env->xmm_regs[i].ZMM_Q(qw - 1),
+env->xmm_regs[i].ZMM_Q(qw - 2),
+qw > 2 ? ":" : "");
+}
+
+/* two registers per line for 128-bit registers */
+if (zmm_width > 2 || (i & 1)) {
cpu_fprintf(f, "\n");
-else
+} else {
cpu_fprintf(f, " ");
+}
+}
+
+if (env->xcr0 & XSTATE_OPMASK_MASK) {
+/* AVX-512 opmask registers */
+for (i = 0; i < 8; ++i) {
+cpu_fprintf(f, "K%d=%08" PRIx64 "%s", i, env->opmask_regs[i],
+ (i & 3) == 3 ? "\n" : " ");
+}
+}
+
+if (env->xcr0 & XSTATE_BNDREGS_MASK) {
+/* MPX bound registers */
+for (i = 0; i < 4; ++i) {
+cpu_fprintf(f, "BND%d=%016" PRIx64 ":%016" PRIx64 "%s",
+i, env->bnd_regs[i].ub, env->bnd_regs[i].lb,
+(i & 1) ? "\n" : " ");
+}
+}
+
+if (env->xcr0 & XSTATE_BNDCSR_MASK) {
+/* MPX bound status/config registers */
+cpu_fprintf(f, "BNDCFGU=%016" PRIx64 " BNDSTATUS=%016" PRIx64 "\n",
+env->bndcs_regs.cfgu, env->bndcs_regs.sts);
}
}
if (flags & CPU_DUMP_CODE) {
--
2.14.1
[Qemu-devel] [PATCH] Add AVX, AVX-512, MPX support to x86_cpu_dump_state
Signed-off-by: Doug Gale
---
target/i386/helper.c | 94 +---
1 file changed, 82 insertions(+), 12 deletions(-)
diff --git a/target/i386/helper.c b/target/i386/helper.c
index f63eb3d3f4..708fe13f2f 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -543,6 +543,7 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f,
fprintf_function cpu_fprintf,
}
}
cpu_fprintf(f, "EFER=%016" PRIx64 "\n", env->efer);
+cpu_fprintf(f, "XCR0=%016" PRIx64 "\n", env->xcr0);
if (flags & CPU_DUMP_FPU) {
int fptag;
fptag = 0;
@@ -565,21 +566,90 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f,
fprintf_function cpu_fprintf,
else
cpu_fprintf(f, " ");
}
-if (env->hflags & HF_CS64_MASK)
-nb = 16;
-else
+
+if (env->hflags & HF_CS64_MASK) {
+if (env->xcr0 & XSTATE_Hi16_ZMM_MASK) {
+/* AVX-512 32 registers enabled */
+nb = 32;
+} else {
+/* 64-bit mode, 16 registers */
+nb = 16;
+}
+} else {
+/* 32 bit mode, 8 registers */
nb = 8;
-for(i=0;ixmm_regs[i].ZMM_L(3),
-env->xmm_regs[i].ZMM_L(2),
-env->xmm_regs[i].ZMM_L(1),
-env->xmm_regs[i].ZMM_L(0));
-if ((i & 1) == 1)
+}
+
+/* sse register width in units of 64 bits */
+int zmm_width;
+char zmm_name;
+if (env->xcr0 & XSTATE_ZMM_Hi256_MASK) {
+/* 512-bit "ZMM" - AVX-512 registers enabled */
+zmm_width = 8;
+zmm_name = 'Z';
+} else if (env->xcr0 & XSTATE_YMM_MASK) {
+/* 256-bit "YMM" - AVX enabled */
+zmm_width = 4;
+zmm_name = 'Y';
+} else if (env->cr[4] & CR4_OSFXSR_MASK) {
+/* 128-bit "XMM" - SSE enabled */
+zmm_width = 2;
+zmm_name = 'X';
+} else {
+/* SSE not enabled */
+zmm_width = 0;
+zmm_name = 0;
+}
+
+
+cpu_fprintf(f, " MSB%*sLSB\n",
+-(zmm_width * 16 + zmm_width - 7), "");
+
+for (i = 0; i < nb; i++) {
+if (zmm_width == 0) {
+cpu_fprintf(f, "SSE not enabled\n");
+break;
+}
+
+cpu_fprintf(f, "%cMM%02d=", zmm_name, i);
+int qw;
+for (qw = zmm_width; qw > 0; qw -= 2) {
+/* ':' separator every 64 bits */
+cpu_fprintf(f, "%016" PRIx64 ":%016" PRIx64 "%s",
+env->xmm_regs[i].ZMM_Q(qw - 1),
+env->xmm_regs[i].ZMM_Q(qw - 2),
+qw > 2 ? ":" : "");
+}
+
+/* two registers per line for 128-bit registers */
+if (zmm_width > 2 || (i & 1)) {
cpu_fprintf(f, "\n");
-else
+} else {
cpu_fprintf(f, " ");
+}
+}
+
+if (env->xcr0 & XSTATE_OPMASK_MASK) {
+/* AVX-512 opmask registers */
+for (i = 0; i < 8; ++i) {
+cpu_fprintf(f, "K%d=%08" PRIx64 "%s", i, env->opmask_regs[i],
+ (i & 3) == 3 ? "\n" : " ");
+}
+}
+
+if (env->xcr0 & XSTATE_BNDREGS_MASK) {
+/* MPX bound registers */
+for (i = 0; i < 4; ++i) {
+cpu_fprintf(f, "BND%d=%016" PRIx64 ":%016" PRIx64 "%s",
+i, env->bnd_regs[i].ub, env->bnd_regs[i].lb,
+(i & 1) ? "\n" : " ");
+}
+}
+
+if (env->xcr0 & XSTATE_BNDCSR_MASK) {
+/* MPX bound status/config registers */
+cpu_fprintf(f, "BNDCFGU=%016" PRIx64 " BNDSTATUS=%016" PRIx64 "\n",
+env->bndcs_regs.cfgu, env->bndcs_regs.sts);
}
}
if (flags & CPU_DUMP_CODE) {
--
2.14.1
[Qemu-devel] [PATCH v4] gdbstub: add tracing
Signed-off-by: Doug Gale
---
Fix usage of %c in trace output, now uses 0x%02x
Fix possible sign extended char that could cause 0xfc to say 0xfffc
Add missing traces for hitting breakpoints, continuing, stepping
Fix incorrect dynamic check for tracing being enabled in hexdump
Fix missing braces around single line if body
Fix incorrectly indented return statement
Fix order of trace-events to be more tidy
gdbstub.c| 113 +--
trace-events | 28 +++
2 files changed, 106 insertions(+), 35 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 2a94030d3b..f1d51480f7 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -21,6 +21,7 @@
#include "qemu/error-report.h"
#include "qemu/cutils.h"
#include "cpu.h"
+#include "trace-root.h"
#ifdef CONFIG_USER_ONLY
#include "qemu.h"
#else
@@ -287,21 +288,6 @@ static int gdb_signal_to_target (int sig)
return -1;
}
-/* #define DEBUG_GDB */
-
-#ifdef DEBUG_GDB
-# define DEBUG_GDB_GATE 1
-#else
-# define DEBUG_GDB_GATE 0
-#endif
-
-#define gdb_debug(fmt, ...) do { \
-if (DEBUG_GDB_GATE) { \
-fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \
-} \
-} while (0)
-
-
typedef struct GDBRegisterState {
int base_reg;
int num_regs;
@@ -410,10 +396,13 @@ int use_gdb_syscalls(void)
/* Resume execution. */
static inline void gdb_continue(GDBState *s)
{
+
#ifdef CONFIG_USER_ONLY
s->running_state = 1;
+trace_gdbstub_op_continue();
#else
if (!runstate_needs_reset()) {
+trace_gdbstub_op_continue();
vm_start();
}
#endif
@@ -434,6 +423,7 @@ static int gdb_continue_partial(GDBState *s, char
*newstates)
*/
CPU_FOREACH(cpu) {
if (newstates[cpu->cpu_index] == 's') {
+trace_gdbstub_op_stepping(cpu->cpu_index);
cpu_single_step(cpu, sstep_flags);
}
}
@@ -452,11 +442,13 @@ static int gdb_continue_partial(GDBState *s, char
*newstates)
case 1:
break; /* nothing to do here */
case 's':
+trace_gdbstub_op_stepping(cpu->cpu_index);
cpu_single_step(cpu, sstep_flags);
cpu_resume(cpu);
flag = 1;
break;
case 'c':
+trace_gdbstub_op_continue_cpu(cpu->cpu_index);
cpu_resume(cpu);
flag = 1;
break;
@@ -538,12 +530,49 @@ static void hextomem(uint8_t *mem, const char *buf, int
len)
}
}
+static void hexdump(const char *buf, int len,
+void (*trace_fn)(size_t ofs, char const *text))
+{
+char line_buffer[3 * 16 + 4 + 16 + 1];
+
+size_t i;
+for (i = 0; i < len || (i & 0xF); ++i) {
+size_t byte_ofs = i & 15;
+
+if (byte_ofs == 0) {
+memset(line_buffer, ' ', 3 * 16 + 4 + 16);
+line_buffer[3 * 16 + 4 + 16] = 0;
+}
+
+size_t col_group = (i >> 2) & 3;
+size_t hex_col = byte_ofs * 3 + col_group;
+size_t txt_col = 3 * 16 + 4 + byte_ofs;
+
+if (i < len) {
+char value = buf[i];
+
+line_buffer[hex_col + 0] = tohex((value >> 4) & 0xF);
+line_buffer[hex_col + 1] = tohex((value >> 0) & 0xF);
+line_buffer[txt_col + 0] = (value >= ' ' && value < 127)
+? value
+: '.';
+}
+
+if (byte_ofs == 0xF)
+trace_fn(i & -16, line_buffer);
+}
+}
+
/* return -1 if error, 0 if OK */
-static int put_packet_binary(GDBState *s, const char *buf, int len)
+static int put_packet_binary(GDBState *s, const char *buf, int len, bool dump)
{
int csum, i;
uint8_t *p;
+if (dump && trace_event_get_state_backends(TRACE_GDBSTUB_IO_BINARYREPLY)) {
+hexdump(buf, len, trace_gdbstub_io_binaryreply);
+}
+
for(;;) {
p = s->last_packet;
*(p++) = '$';
@@ -576,9 +605,9 @@ static int put_packet_binary(GDBState *s, const char *buf,
int len)
/* return -1 if error, 0 if OK */
static int put_packet(GDBState *s, const char *buf)
{
-gdb_debug("reply='%s'\n", buf);
+trace_gdbstub_io_reply(buf);
-return put_packet_binary(s, buf, strlen(buf));
+return put_packet_binary(s, buf, strlen(buf), false);
}
/* Encode data using the encoding for 'x' packets. */
@@ -975,8 +1004,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
uint8_t *registers;
target_ulong addr, len;
-
-gdb_debug("command='%s'\n", line_buf);
+trace_gdbstub_io_command(line_buf);
p = line_buf;
ch = *p++;
@@ -999,7 +1027,7 @@ static int gdb_handle_packet(GDBState *s, const char
*li
Re: [Qemu-devel] [PATCH] gdbstub: add tracing
On Mon, Nov 27, 2017 at 5:00 AM, Markus Armbruster wrote:
> Drive-by question...
>
> Doug Gale writes:
>
>> Signed-off-by: Doug Gale
>> ---
>> gdbstub.c| 100
>> ++-
>> trace-events | 21 +
>> 2 files changed, 86 insertions(+), 35 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 2a94030d3b..a75f319bd0 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -21,6 +21,7 @@
>> #include "qemu/error-report.h"
>> #include "qemu/cutils.h"
>> #include "cpu.h"
>> +#include "trace-root.h"
>> #ifdef CONFIG_USER_ONLY
>> #include "qemu.h"
>> #else
>> @@ -287,21 +288,6 @@ static int gdb_signal_to_target (int sig)
>> return -1;
>> }
>>
>> -/* #define DEBUG_GDB */
>> -
>> -#ifdef DEBUG_GDB
>> -# define DEBUG_GDB_GATE 1
>> -#else
>> -# define DEBUG_GDB_GATE 0
>> -#endif
>> -
>> -#define gdb_debug(fmt, ...) do { \
>> -if (DEBUG_GDB_GATE) { \
>> -fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \
>> -} \
>> -} while (0)
>> -
>> -
>> typedef struct GDBRegisterState {
>> int base_reg;
>> int num_regs;
>> @@ -538,12 +524,47 @@ static void hextomem(uint8_t *mem, const char *buf,
>> int len)
>> }
>> }
>>
>> +static void hexdump(const char *buf, int len,
>> +void (*trace_fn)(size_t ofs, char const *text))
>> +{
>> +char line_buffer[3 * 16 + 4 + 16 + 1];
>> +
>> +for (size_t i = 0; i < len || (i & 0xF); ++i) {
>> +size_t byte_ofs = i & 15;
>> +
>> +if (byte_ofs == 0) {
>> +memset(line_buffer, ' ', 3 * 16 + 4 + 16);
>> +line_buffer[3 * 16 + 4 + 16] = 0;
>> +}
>> +
>> +size_t col_group = (i >> 2) & 3;
>> +size_t hex_col = byte_ofs * 3 + col_group;
>> +size_t txt_col = 3 * 16 + 4 + byte_ofs;
>> +
>> +if (i < len) {
>> +char value = buf[i];
>> +
>> +line_buffer[hex_col + 0] = tohex((value >> 4) & 0xF);
>> +line_buffer[hex_col + 1] = tohex((value >> 0) & 0xF);
>> +line_buffer[txt_col + 0] = (value >= ' ' && value < 127)
>> +? value
>> +: '.';
>> +}
>> +
>> +if (byte_ofs == 0xF)
>> +trace_fn(i & -16, line_buffer);
>> +}
>> +}
>
> Could existing qemu_hexdump() serve?
>
>> +
>> /* return -1 if error, 0 if OK */
>> -static int put_packet_binary(GDBState *s, const char *buf, int len)
>> +static int put_packet_binary(GDBState *s, const char *buf, int len, bool
>> dump)
>> {
>> int csum, i;
>> uint8_t *p;
>>
>> +if (TRACE_GDBSTUB_IO_BINARYREPLY_ENABLED && dump)
>> +hexdump(buf, len, trace_gdbstub_io_binaryreply);
>> +
>> for(;;) {
>> p = s->last_packet;
>> *(p++) = '$';
> [...]
It is incorrect to use qemu_hexdump, tracing isn't printf. The whole
point of this patch is to _not_ dump traces to printf. I have already
submitted tracing that used printf, and it was rejected with a request
to use trace_ infrastructure.
It might be a better idea to refactor qemu_hexdump to use this
algorithm and use a trace_fn callback which dumps lines to printf for
qemu_hexdump case. qemu_hexdump hammers printf with a couple of
characters at a time of output and uses several loops, this algorithm
generates whole lines and more efficiently does one I/O per line. This
algorithm also breaks the hex into groups of four bytes to make it
easy to visually find a given byte.
I opted not to do that refactor of qemu_hexdump to reduce the scope of
my patch. Should I do that and resubmit?
[Qemu-devel] [PATCH v3] gdbstub: add tracing
Signed-off-by: Doug Gale
---
gdbstub.c| 101 ++-
trace-events | 21 +
2 files changed, 87 insertions(+), 35 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 2a94030d3b..86482fa009 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -21,6 +21,7 @@
#include "qemu/error-report.h"
#include "qemu/cutils.h"
#include "cpu.h"
+#include "trace-root.h"
#ifdef CONFIG_USER_ONLY
#include "qemu.h"
#else
@@ -287,21 +288,6 @@ static int gdb_signal_to_target (int sig)
return -1;
}
-/* #define DEBUG_GDB */
-
-#ifdef DEBUG_GDB
-# define DEBUG_GDB_GATE 1
-#else
-# define DEBUG_GDB_GATE 0
-#endif
-
-#define gdb_debug(fmt, ...) do { \
-if (DEBUG_GDB_GATE) { \
-fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \
-} \
-} while (0)
-
-
typedef struct GDBRegisterState {
int base_reg;
int num_regs;
@@ -538,12 +524,48 @@ static void hextomem(uint8_t *mem, const char *buf, int
len)
}
}
+static void hexdump(const char *buf, int len,
+void (*trace_fn)(size_t ofs, char const *text))
+{
+char line_buffer[3 * 16 + 4 + 16 + 1];
+
+size_t i;
+for (i = 0; i < len || (i & 0xF); ++i) {
+size_t byte_ofs = i & 15;
+
+if (byte_ofs == 0) {
+memset(line_buffer, ' ', 3 * 16 + 4 + 16);
+line_buffer[3 * 16 + 4 + 16] = 0;
+}
+
+size_t col_group = (i >> 2) & 3;
+size_t hex_col = byte_ofs * 3 + col_group;
+size_t txt_col = 3 * 16 + 4 + byte_ofs;
+
+if (i < len) {
+char value = buf[i];
+
+line_buffer[hex_col + 0] = tohex((value >> 4) & 0xF);
+line_buffer[hex_col + 1] = tohex((value >> 0) & 0xF);
+line_buffer[txt_col + 0] = (value >= ' ' && value < 127)
+? value
+: '.';
+}
+
+if (byte_ofs == 0xF)
+trace_fn(i & -16, line_buffer);
+}
+}
+
/* return -1 if error, 0 if OK */
-static int put_packet_binary(GDBState *s, const char *buf, int len)
+static int put_packet_binary(GDBState *s, const char *buf, int len, bool dump)
{
int csum, i;
uint8_t *p;
+if (TRACE_GDBSTUB_IO_BINARYREPLY_ENABLED && dump)
+hexdump(buf, len, trace_gdbstub_io_binaryreply);
+
for(;;) {
p = s->last_packet;
*(p++) = '$';
@@ -576,9 +598,9 @@ static int put_packet_binary(GDBState *s, const char *buf,
int len)
/* return -1 if error, 0 if OK */
static int put_packet(GDBState *s, const char *buf)
{
-gdb_debug("reply='%s'\n", buf);
+trace_gdbstub_io_reply(buf);
-return put_packet_binary(s, buf, strlen(buf));
+return put_packet_binary(s, buf, strlen(buf), false);
}
/* Encode data using the encoding for 'x' packets. */
@@ -975,8 +997,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
uint8_t *registers;
target_ulong addr, len;
-
-gdb_debug("command='%s'\n", line_buf);
+trace_gdbstub_io_command(line_buf);
p = line_buf;
ch = *p++;
@@ -999,7 +1020,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
}
s->signal = 0;
gdb_continue(s);
- return RS_IDLE;
+return RS_IDLE;
case 'C':
s->signal = gdb_signal_to_target (strtoul(p, (char **)&p, 16));
if (s->signal == -1)
@@ -1045,7 +1066,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
}
cpu_single_step(s->c_cpu, sstep_flags);
gdb_continue(s);
- return RS_IDLE;
+return RS_IDLE;
case 'F':
{
target_ulong ret;
@@ -1267,6 +1288,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
len = snprintf((char *)mem_buf, sizeof(buf) / 2,
"CPU#%d [%s]", cpu->cpu_index,
cpu->halted ? "halted " : "running");
+trace_gdbstub_op_extra_info((char *)mem_buf);
memtohex(buf, mem_buf, len);
put_packet(s, buf);
}
@@ -1350,7 +1372,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
buf[0] = 'l';
len = memtox(buf + 1, xml + addr, total_len - addr);
}
-put_packet_binary(s, buf, len + 1);
+put_packet_binary(s, buf, len + 1, true);
break;
}
if (is_query_packet(p, "Attached", ':')) {
@@ -1407,6 +1429,8 @@ static void gdb_vm_state_change(void *opaque, int
running, RunState state)
type = "";
break;
}
+
[Qemu-devel] [PATCH v2] gdbstub: add tracing
Signed-off-by: Doug Gale
---
gdbstub.c| 101 ++-
trace-events | 21 +
2 files changed, 87 insertions(+), 35 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 2a94030d3b..86482fa009 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -21,6 +21,7 @@
#include "qemu/error-report.h"
#include "qemu/cutils.h"
#include "cpu.h"
+#include "trace-root.h"
#ifdef CONFIG_USER_ONLY
#include "qemu.h"
#else
@@ -287,21 +288,6 @@ static int gdb_signal_to_target (int sig)
return -1;
}
-/* #define DEBUG_GDB */
-
-#ifdef DEBUG_GDB
-# define DEBUG_GDB_GATE 1
-#else
-# define DEBUG_GDB_GATE 0
-#endif
-
-#define gdb_debug(fmt, ...) do { \
-if (DEBUG_GDB_GATE) { \
-fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \
-} \
-} while (0)
-
-
typedef struct GDBRegisterState {
int base_reg;
int num_regs;
@@ -538,12 +524,48 @@ static void hextomem(uint8_t *mem, const char *buf, int
len)
}
}
+static void hexdump(const char *buf, int len,
+void (*trace_fn)(size_t ofs, char const *text))
+{
+char line_buffer[3 * 16 + 4 + 16 + 1];
+
+size_t i;
+for (i = 0; i < len || (i & 0xF); ++i) {
+size_t byte_ofs = i & 15;
+
+if (byte_ofs == 0) {
+memset(line_buffer, ' ', 3 * 16 + 4 + 16);
+line_buffer[3 * 16 + 4 + 16] = 0;
+}
+
+size_t col_group = (i >> 2) & 3;
+size_t hex_col = byte_ofs * 3 + col_group;
+size_t txt_col = 3 * 16 + 4 + byte_ofs;
+
+if (i < len) {
+char value = buf[i];
+
+line_buffer[hex_col + 0] = tohex((value >> 4) & 0xF);
+line_buffer[hex_col + 1] = tohex((value >> 0) & 0xF);
+line_buffer[txt_col + 0] = (value >= ' ' && value < 127)
+? value
+: '.';
+}
+
+if (byte_ofs == 0xF)
+trace_fn(i & -16, line_buffer);
+}
+}
+
/* return -1 if error, 0 if OK */
-static int put_packet_binary(GDBState *s, const char *buf, int len)
+static int put_packet_binary(GDBState *s, const char *buf, int len, bool dump)
{
int csum, i;
uint8_t *p;
+if (TRACE_GDBSTUB_IO_BINARYREPLY_ENABLED && dump)
+hexdump(buf, len, trace_gdbstub_io_binaryreply);
+
for(;;) {
p = s->last_packet;
*(p++) = '$';
@@ -576,9 +598,9 @@ static int put_packet_binary(GDBState *s, const char *buf,
int len)
/* return -1 if error, 0 if OK */
static int put_packet(GDBState *s, const char *buf)
{
-gdb_debug("reply='%s'\n", buf);
+trace_gdbstub_io_reply(buf);
-return put_packet_binary(s, buf, strlen(buf));
+return put_packet_binary(s, buf, strlen(buf), false);
}
/* Encode data using the encoding for 'x' packets. */
@@ -975,8 +997,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
uint8_t *registers;
target_ulong addr, len;
-
-gdb_debug("command='%s'\n", line_buf);
+trace_gdbstub_io_command(line_buf);
p = line_buf;
ch = *p++;
@@ -999,7 +1020,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
}
s->signal = 0;
gdb_continue(s);
- return RS_IDLE;
+return RS_IDLE;
case 'C':
s->signal = gdb_signal_to_target (strtoul(p, (char **)&p, 16));
if (s->signal == -1)
@@ -1045,7 +1066,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
}
cpu_single_step(s->c_cpu, sstep_flags);
gdb_continue(s);
- return RS_IDLE;
+return RS_IDLE;
case 'F':
{
target_ulong ret;
@@ -1267,6 +1288,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
len = snprintf((char *)mem_buf, sizeof(buf) / 2,
"CPU#%d [%s]", cpu->cpu_index,
cpu->halted ? "halted " : "running");
+trace_gdbstub_op_extra_info((char *)mem_buf);
memtohex(buf, mem_buf, len);
put_packet(s, buf);
}
@@ -1350,7 +1372,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
buf[0] = 'l';
len = memtox(buf + 1, xml + addr, total_len - addr);
}
-put_packet_binary(s, buf, len + 1);
+put_packet_binary(s, buf, len + 1, true);
break;
}
if (is_query_packet(p, "Attached", ':')) {
@@ -1407,6 +1429,8 @@ static void gdb_vm_state_change(void *opaque, int
running, RunState state)
type = "";
break;
}
+
[Qemu-devel] [PATCH] gdbstub: add tracing
Signed-off-by: Doug Gale
---
gdbstub.c| 100 ++-
trace-events | 21 +
2 files changed, 86 insertions(+), 35 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 2a94030d3b..a75f319bd0 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -21,6 +21,7 @@
#include "qemu/error-report.h"
#include "qemu/cutils.h"
#include "cpu.h"
+#include "trace-root.h"
#ifdef CONFIG_USER_ONLY
#include "qemu.h"
#else
@@ -287,21 +288,6 @@ static int gdb_signal_to_target (int sig)
return -1;
}
-/* #define DEBUG_GDB */
-
-#ifdef DEBUG_GDB
-# define DEBUG_GDB_GATE 1
-#else
-# define DEBUG_GDB_GATE 0
-#endif
-
-#define gdb_debug(fmt, ...) do { \
-if (DEBUG_GDB_GATE) { \
-fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \
-} \
-} while (0)
-
-
typedef struct GDBRegisterState {
int base_reg;
int num_regs;
@@ -538,12 +524,47 @@ static void hextomem(uint8_t *mem, const char *buf, int
len)
}
}
+static void hexdump(const char *buf, int len,
+void (*trace_fn)(size_t ofs, char const *text))
+{
+char line_buffer[3 * 16 + 4 + 16 + 1];
+
+for (size_t i = 0; i < len || (i & 0xF); ++i) {
+size_t byte_ofs = i & 15;
+
+if (byte_ofs == 0) {
+memset(line_buffer, ' ', 3 * 16 + 4 + 16);
+line_buffer[3 * 16 + 4 + 16] = 0;
+}
+
+size_t col_group = (i >> 2) & 3;
+size_t hex_col = byte_ofs * 3 + col_group;
+size_t txt_col = 3 * 16 + 4 + byte_ofs;
+
+if (i < len) {
+char value = buf[i];
+
+line_buffer[hex_col + 0] = tohex((value >> 4) & 0xF);
+line_buffer[hex_col + 1] = tohex((value >> 0) & 0xF);
+line_buffer[txt_col + 0] = (value >= ' ' && value < 127)
+? value
+: '.';
+}
+
+if (byte_ofs == 0xF)
+trace_fn(i & -16, line_buffer);
+}
+}
+
/* return -1 if error, 0 if OK */
-static int put_packet_binary(GDBState *s, const char *buf, int len)
+static int put_packet_binary(GDBState *s, const char *buf, int len, bool dump)
{
int csum, i;
uint8_t *p;
+if (TRACE_GDBSTUB_IO_BINARYREPLY_ENABLED && dump)
+hexdump(buf, len, trace_gdbstub_io_binaryreply);
+
for(;;) {
p = s->last_packet;
*(p++) = '$';
@@ -576,9 +597,9 @@ static int put_packet_binary(GDBState *s, const char *buf,
int len)
/* return -1 if error, 0 if OK */
static int put_packet(GDBState *s, const char *buf)
{
-gdb_debug("reply='%s'\n", buf);
+trace_gdbstub_io_reply(buf);
-return put_packet_binary(s, buf, strlen(buf));
+return put_packet_binary(s, buf, strlen(buf), false);
}
/* Encode data using the encoding for 'x' packets. */
@@ -975,8 +996,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
uint8_t *registers;
target_ulong addr, len;
-
-gdb_debug("command='%s'\n", line_buf);
+trace_gdbstub_io_command(line_buf);
p = line_buf;
ch = *p++;
@@ -999,7 +1019,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
}
s->signal = 0;
gdb_continue(s);
- return RS_IDLE;
+return RS_IDLE;
case 'C':
s->signal = gdb_signal_to_target (strtoul(p, (char **)&p, 16));
if (s->signal == -1)
@@ -1045,7 +1065,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
}
cpu_single_step(s->c_cpu, sstep_flags);
gdb_continue(s);
- return RS_IDLE;
+return RS_IDLE;
case 'F':
{
target_ulong ret;
@@ -1267,6 +1287,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
len = snprintf((char *)mem_buf, sizeof(buf) / 2,
"CPU#%d [%s]", cpu->cpu_index,
cpu->halted ? "halted " : "running");
+trace_gdbstub_op_extra_info((char *)mem_buf);
memtohex(buf, mem_buf, len);
put_packet(s, buf);
}
@@ -1350,7 +1371,7 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
buf[0] = 'l';
len = memtox(buf + 1, xml + addr, total_len - addr);
}
-put_packet_binary(s, buf, len + 1);
+put_packet_binary(s, buf, len + 1, true);
break;
}
if (is_query_packet(p, "Attached", ':')) {
@@ -1407,6 +1428,8 @@ static void gdb_vm_state_change(void *opaque, int
running, RunState state)
type = "";
break;
}
+
Re: [Qemu-devel] [PATCH v5] nvme: Add tracing
Ping
On Thu, Nov 16, 2017 at 6:16 AM, Doug Gale wrote:
> I submitted it with git Nov 3 - the long lines issue with git-am
> should be resolved. Please let me know if there's still a problem.
>
> Thanks.
>
>
> On Fri, Nov 3, 2017 at 11:58 AM, Philippe Mathieu-Daudé
> wrote:
>> Cc'ing Trivial ;)
>>
>> On 11/03/2017 10:37 AM, Doug Gale wrote:
>>> Add trace output for commands, errors, and undefined behavior.
>>> Add guest error log output for undefined behavior.
>>> Report invalid undefined accesses to MMIO.
>>> Annotate unlikely error checks with unlikely.
>>>
>>> Signed-off-by: Doug Gale
>>> Reviewed-by: Philippe Mathieu-Daudé
>>> Reviewed-by: Stefan Hajnoczi
>>> ---
>>> changes v4 -> v5: add R-b tags
>>> hw/block/nvme.c | 349
>>> ++
>>> hw/block/trace-events | 93 ++
>>> 2 files changed, 390 insertions(+), 52 deletions(-)
>>>
>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>> index 441e21ed1f..4d98ed9fba 100644
>>> --- a/hw/block/nvme.c
>>> +++ b/hw/block/nvme.c
>>> @@ -34,8 +34,17 @@
>>> #include "qapi/visitor.h"
>>> #include "sysemu/block-backend.h"
>>>
>>> +#include "qemu/log.h"
>>> +#include "trace.h"
>>> #include "nvme.h"
>>>
>>> +#define NVME_GUEST_ERR(trace, fmt, ...) \
>>> +do { \
>>> +(trace_##trace)(__VA_ARGS__); \
>>> +qemu_log_mask(LOG_GUEST_ERROR, #trace \
>>> +" in %s: " fmt "\n", __func__, ## __VA_ARGS__); \
>>> +} while (0)
>>> +
>>> static void nvme_process_sq(void *opaque);
>>>
>>> static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
>>> @@ -86,10 +95,14 @@ static void nvme_isr_notify(NvmeCtrl *n, NvmeCQueue *cq)
>>> {
>>> if (cq->irq_enabled) {
>>> if (msix_enabled(&(n->parent_obj))) {
>>> +trace_nvme_irq_msix(cq->vector);
>>> msix_notify(&(n->parent_obj), cq->vector);
>>> } else {
>>> +trace_nvme_irq_pin();
>>> pci_irq_pulse(&n->parent_obj);
>>> }
>>> +} else {
>>> +trace_nvme_irq_masked();
>>> }
>>> }
>>>
>>> @@ -100,7 +113,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
>>> QEMUIOVector *iov, uint64_t prp1,
>>> trans_len = MIN(len, trans_len);
>>> int num_prps = (len >> n->page_bits) + 1;
>>>
>>> -if (!prp1) {
>>> +if (unlikely(!prp1)) {
>>> +trace_nvme_err_invalid_prp();
>>> return NVME_INVALID_FIELD | NVME_DNR;
>>> } else if (n->cmbsz && prp1 >= n->ctrl_mem.addr &&
>>> prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) {
>>> @@ -113,7 +127,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
>>> QEMUIOVector *iov, uint64_t prp1,
>>> }
>>> len -= trans_len;
>>> if (len) {
>>> -if (!prp2) {
>>> +if (unlikely(!prp2)) {
>>> +trace_nvme_err_invalid_prp2_missing();
>>> goto unmap;
>>> }
>>> if (len > n->page_size) {
>>> @@ -128,7 +143,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
>>> QEMUIOVector *iov, uint64_t prp1,
>>> uint64_t prp_ent = le64_to_cpu(prp_list[i]);
>>>
>>> if (i == n->max_prp_ents - 1 && len > n->page_size) {
>>> -if (!prp_ent || prp_ent & (n->page_size - 1)) {
>>> +if (unlikely(!prp_ent || prp_ent & (n->page_size -
>>> 1))) {
>>> +trace_nvme_err_invalid_prplist_ent(prp_ent);
>>> goto unmap;
>>> }
>>>
>>> @@ -140,7 +156,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
>>> QEMUIOVector *iov, uint64_t prp1,
>>> prp_ent = le64_to_cpu(prp_list[i]);
>>> }
>>>
>>> -if (!prp_ent || prp_ent & (n->page_size - 1)) {
>>> +if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
>>> +tr
Re: [Qemu-devel] [PATCH v5] nvme: Add tracing
I submitted it with git Nov 3 - the long lines issue with git-am
should be resolved. Please let me know if there's still a problem.
Thanks.
On Fri, Nov 3, 2017 at 11:58 AM, Philippe Mathieu-Daudé wrote:
> Cc'ing Trivial ;)
>
> On 11/03/2017 10:37 AM, Doug Gale wrote:
>> Add trace output for commands, errors, and undefined behavior.
>> Add guest error log output for undefined behavior.
>> Report invalid undefined accesses to MMIO.
>> Annotate unlikely error checks with unlikely.
>>
>> Signed-off-by: Doug Gale
>> Reviewed-by: Philippe Mathieu-Daudé
>> Reviewed-by: Stefan Hajnoczi
>> ---
>> changes v4 -> v5: add R-b tags
>> hw/block/nvme.c | 349
>> ++
>> hw/block/trace-events | 93 ++
>> 2 files changed, 390 insertions(+), 52 deletions(-)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index 441e21ed1f..4d98ed9fba 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -34,8 +34,17 @@
>> #include "qapi/visitor.h"
>> #include "sysemu/block-backend.h"
>>
>> +#include "qemu/log.h"
>> +#include "trace.h"
>> #include "nvme.h"
>>
>> +#define NVME_GUEST_ERR(trace, fmt, ...) \
>> +do { \
>> +(trace_##trace)(__VA_ARGS__); \
>> +qemu_log_mask(LOG_GUEST_ERROR, #trace \
>> +" in %s: " fmt "\n", __func__, ## __VA_ARGS__); \
>> +} while (0)
>> +
>> static void nvme_process_sq(void *opaque);
>>
>> static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
>> @@ -86,10 +95,14 @@ static void nvme_isr_notify(NvmeCtrl *n, NvmeCQueue *cq)
>> {
>> if (cq->irq_enabled) {
>> if (msix_enabled(&(n->parent_obj))) {
>> +trace_nvme_irq_msix(cq->vector);
>> msix_notify(&(n->parent_obj), cq->vector);
>> } else {
>> +trace_nvme_irq_pin();
>> pci_irq_pulse(&n->parent_obj);
>> }
>> +} else {
>> +trace_nvme_irq_masked();
>> }
>> }
>>
>> @@ -100,7 +113,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
>> QEMUIOVector *iov, uint64_t prp1,
>> trans_len = MIN(len, trans_len);
>> int num_prps = (len >> n->page_bits) + 1;
>>
>> -if (!prp1) {
>> +if (unlikely(!prp1)) {
>> +trace_nvme_err_invalid_prp();
>> return NVME_INVALID_FIELD | NVME_DNR;
>> } else if (n->cmbsz && prp1 >= n->ctrl_mem.addr &&
>> prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) {
>> @@ -113,7 +127,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
>> QEMUIOVector *iov, uint64_t prp1,
>> }
>> len -= trans_len;
>> if (len) {
>> -if (!prp2) {
>> +if (unlikely(!prp2)) {
>> +trace_nvme_err_invalid_prp2_missing();
>> goto unmap;
>> }
>> if (len > n->page_size) {
>> @@ -128,7 +143,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
>> QEMUIOVector *iov, uint64_t prp1,
>> uint64_t prp_ent = le64_to_cpu(prp_list[i]);
>>
>> if (i == n->max_prp_ents - 1 && len > n->page_size) {
>> -if (!prp_ent || prp_ent & (n->page_size - 1)) {
>> +if (unlikely(!prp_ent || prp_ent & (n->page_size - 1)))
>> {
>> +trace_nvme_err_invalid_prplist_ent(prp_ent);
>> goto unmap;
>> }
>>
>> @@ -140,7 +156,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
>> QEMUIOVector *iov, uint64_t prp1,
>> prp_ent = le64_to_cpu(prp_list[i]);
>> }
>>
>> -if (!prp_ent || prp_ent & (n->page_size - 1)) {
>> +if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
>> +trace_nvme_err_invalid_prplist_ent(prp_ent);
>> goto unmap;
>> }
>>
>> @@ -154,7 +171,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
>> QEMUIOVector *iov, uint64_t prp1,
>> i++;
>> }
>> } else {
>> -if (prp2 & (n->page_size - 1)) {
>> +if (unlikely(prp2 & (n->page_size - 1))) {
>> +
[Qemu-devel] [PATCH v5] nvme: Add tracing
Add trace output for commands, errors, and undefined behavior.
Add guest error log output for undefined behavior.
Report invalid undefined accesses to MMIO.
Annotate unlikely error checks with unlikely.
Signed-off-by: Doug Gale
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Stefan Hajnoczi
---
changes v4 -> v5: add R-b tags
hw/block/nvme.c | 349 ++
hw/block/trace-events | 93 ++
2 files changed, 390 insertions(+), 52 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 441e21ed1f..4d98ed9fba 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -34,8 +34,17 @@
#include "qapi/visitor.h"
#include "sysemu/block-backend.h"
+#include "qemu/log.h"
+#include "trace.h"
#include "nvme.h"
+#define NVME_GUEST_ERR(trace, fmt, ...) \
+do { \
+(trace_##trace)(__VA_ARGS__); \
+qemu_log_mask(LOG_GUEST_ERROR, #trace \
+" in %s: " fmt "\n", __func__, ## __VA_ARGS__); \
+} while (0)
+
static void nvme_process_sq(void *opaque);
static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
@@ -86,10 +95,14 @@ static void nvme_isr_notify(NvmeCtrl *n, NvmeCQueue *cq)
{
if (cq->irq_enabled) {
if (msix_enabled(&(n->parent_obj))) {
+trace_nvme_irq_msix(cq->vector);
msix_notify(&(n->parent_obj), cq->vector);
} else {
+trace_nvme_irq_pin();
pci_irq_pulse(&n->parent_obj);
}
+} else {
+trace_nvme_irq_masked();
}
}
@@ -100,7 +113,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector
*iov, uint64_t prp1,
trans_len = MIN(len, trans_len);
int num_prps = (len >> n->page_bits) + 1;
-if (!prp1) {
+if (unlikely(!prp1)) {
+trace_nvme_err_invalid_prp();
return NVME_INVALID_FIELD | NVME_DNR;
} else if (n->cmbsz && prp1 >= n->ctrl_mem.addr &&
prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) {
@@ -113,7 +127,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector
*iov, uint64_t prp1,
}
len -= trans_len;
if (len) {
-if (!prp2) {
+if (unlikely(!prp2)) {
+trace_nvme_err_invalid_prp2_missing();
goto unmap;
}
if (len > n->page_size) {
@@ -128,7 +143,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector
*iov, uint64_t prp1,
uint64_t prp_ent = le64_to_cpu(prp_list[i]);
if (i == n->max_prp_ents - 1 && len > n->page_size) {
-if (!prp_ent || prp_ent & (n->page_size - 1)) {
+if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
+trace_nvme_err_invalid_prplist_ent(prp_ent);
goto unmap;
}
@@ -140,7 +156,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector
*iov, uint64_t prp1,
prp_ent = le64_to_cpu(prp_list[i]);
}
-if (!prp_ent || prp_ent & (n->page_size - 1)) {
+if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
+trace_nvme_err_invalid_prplist_ent(prp_ent);
goto unmap;
}
@@ -154,7 +171,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector
*iov, uint64_t prp1,
i++;
}
} else {
-if (prp2 & (n->page_size - 1)) {
+if (unlikely(prp2 & (n->page_size - 1))) {
+trace_nvme_err_invalid_prp2_align(prp2);
goto unmap;
}
if (qsg->nsg) {
@@ -178,16 +196,20 @@ static uint16_t nvme_dma_read_prp(NvmeCtrl *n, uint8_t
*ptr, uint32_t len,
QEMUIOVector iov;
uint16_t status = NVME_SUCCESS;
+trace_nvme_dma_read(prp1, prp2);
+
if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {
return NVME_INVALID_FIELD | NVME_DNR;
}
if (qsg.nsg > 0) {
-if (dma_buf_read(ptr, len, &qsg)) {
+if (unlikely(dma_buf_read(ptr, len, &qsg))) {
+trace_nvme_err_invalid_dma();
status = NVME_INVALID_FIELD | NVME_DNR;
}
qemu_sglist_destroy(&qsg);
} else {
-if (qemu_iovec_to_buf(&iov, 0, ptr, len) != len) {
+if (unlikely(qemu_iovec_to_buf(&iov, 0, ptr, len) != len)) {
+trace_nvme_err_invalid_dma();
status = NVME_INVALID_FIELD | NVME_DNR;
}
qemu_iovec_destroy(&iov);
@@ -273,7 +295,8 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n, NvmeNamespace
*ns, NvmeCmd *cmd,
uint64_t aio_slba = slba << (data_shift - BDRV_SECTOR_BITS);
uint32_t aio_nlb = nlb <
[Qemu-devel] [PATCH] nvme: Add tracing
Add trace output for commands, errors, and undefined behavior.
Add guest error log output for undefined behavior.
Report invalid undefined accesses to MMIO.
Annotate unlikely error checks with unlikely.
Signed-off-by: Doug Gale
---
hw/block/nvme.c | 349 ++
hw/block/trace-events | 93 ++
2 files changed, 390 insertions(+), 52 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 441e21ed1f..4d98ed9fba 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -34,8 +34,17 @@
#include "qapi/visitor.h"
#include "sysemu/block-backend.h"
+#include "qemu/log.h"
+#include "trace.h"
#include "nvme.h"
+#define NVME_GUEST_ERR(trace, fmt, ...) \
+do { \
+(trace_##trace)(__VA_ARGS__); \
+qemu_log_mask(LOG_GUEST_ERROR, #trace \
+" in %s: " fmt "\n", __func__, ## __VA_ARGS__); \
+} while (0)
+
static void nvme_process_sq(void *opaque);
static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
@@ -86,10 +95,14 @@ static void nvme_isr_notify(NvmeCtrl *n, NvmeCQueue *cq)
{
if (cq->irq_enabled) {
if (msix_enabled(&(n->parent_obj))) {
+trace_nvme_irq_msix(cq->vector);
msix_notify(&(n->parent_obj), cq->vector);
} else {
+trace_nvme_irq_pin();
pci_irq_pulse(&n->parent_obj);
}
+} else {
+trace_nvme_irq_masked();
}
}
@@ -100,7 +113,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector
*iov, uint64_t prp1,
trans_len = MIN(len, trans_len);
int num_prps = (len >> n->page_bits) + 1;
-if (!prp1) {
+if (unlikely(!prp1)) {
+trace_nvme_err_invalid_prp();
return NVME_INVALID_FIELD | NVME_DNR;
} else if (n->cmbsz && prp1 >= n->ctrl_mem.addr &&
prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) {
@@ -113,7 +127,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector
*iov, uint64_t prp1,
}
len -= trans_len;
if (len) {
-if (!prp2) {
+if (unlikely(!prp2)) {
+trace_nvme_err_invalid_prp2_missing();
goto unmap;
}
if (len > n->page_size) {
@@ -128,7 +143,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector
*iov, uint64_t prp1,
uint64_t prp_ent = le64_to_cpu(prp_list[i]);
if (i == n->max_prp_ents - 1 && len > n->page_size) {
-if (!prp_ent || prp_ent & (n->page_size - 1)) {
+if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
+trace_nvme_err_invalid_prplist_ent(prp_ent);
goto unmap;
}
@@ -140,7 +156,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector
*iov, uint64_t prp1,
prp_ent = le64_to_cpu(prp_list[i]);
}
-if (!prp_ent || prp_ent & (n->page_size - 1)) {
+if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
+trace_nvme_err_invalid_prplist_ent(prp_ent);
goto unmap;
}
@@ -154,7 +171,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector
*iov, uint64_t prp1,
i++;
}
} else {
-if (prp2 & (n->page_size - 1)) {
+if (unlikely(prp2 & (n->page_size - 1))) {
+trace_nvme_err_invalid_prp2_align(prp2);
goto unmap;
}
if (qsg->nsg) {
@@ -178,16 +196,20 @@ static uint16_t nvme_dma_read_prp(NvmeCtrl *n, uint8_t
*ptr, uint32_t len,
QEMUIOVector iov;
uint16_t status = NVME_SUCCESS;
+trace_nvme_dma_read(prp1, prp2);
+
if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {
return NVME_INVALID_FIELD | NVME_DNR;
}
if (qsg.nsg > 0) {
-if (dma_buf_read(ptr, len, &qsg)) {
+if (unlikely(dma_buf_read(ptr, len, &qsg))) {
+trace_nvme_err_invalid_dma();
status = NVME_INVALID_FIELD | NVME_DNR;
}
qemu_sglist_destroy(&qsg);
} else {
-if (qemu_iovec_to_buf(&iov, 0, ptr, len) != len) {
+if (unlikely(qemu_iovec_to_buf(&iov, 0, ptr, len) != len)) {
+trace_nvme_err_invalid_dma();
status = NVME_INVALID_FIELD | NVME_DNR;
}
qemu_iovec_destroy(&iov);
@@ -273,7 +295,8 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n, NvmeNamespace
*ns, NvmeCmd *cmd,
uint64_t aio_slba = slba << (data_shift - BDRV_SECTOR_BITS);
uint32_t aio_nlb = nlb << (data_shift - BDRV_SECTOR_BITS);
-if (slba + nlb > ns->id_ns.nsze) {
+if (unlikely
[Qemu-devel] [PATCH v4] nvme: Add tracing
>From 0e27b5dca8f4f32a1b194e1b3544be77dd4f45d9 Mon Sep 17 00:00:00 2001
From: Doug Gale
Date: Mon, 30 Oct 2017 09:28:43 -0400
Subject: [PATCH] nvme: Add tracing
Add trace output for commands, errors, and undefined behavior.
Add guest error log output for undefined behavior.
Report invalid undefined accesses to MMIO.
Annotate unlikely error checks with unlikely.
Signed-off-by: Doug Gale
---
hw/block/nvme.c | 349 ++
hw/block/trace-events | 93 ++
2 files changed, 390 insertions(+), 52 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 441e21ed1f..4d98ed9fba 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -34,8 +34,17 @@
#include "qapi/visitor.h"
#include "sysemu/block-backend.h"
+#include "qemu/log.h"
+#include "trace.h"
#include "nvme.h"
+#define NVME_GUEST_ERR(trace, fmt, ...) \
+do { \
+(trace_##trace)(__VA_ARGS__); \
+qemu_log_mask(LOG_GUEST_ERROR, #trace \
+" in %s: " fmt "\n", __func__, ## __VA_ARGS__); \
+} while (0)
+
static void nvme_process_sq(void *opaque);
static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
@@ -86,10 +95,14 @@ static void nvme_isr_notify(NvmeCtrl *n, NvmeCQueue *cq)
{
if (cq->irq_enabled) {
if (msix_enabled(&(n->parent_obj))) {
+trace_nvme_irq_msix(cq->vector);
msix_notify(&(n->parent_obj), cq->vector);
} else {
+trace_nvme_irq_pin();
pci_irq_pulse(&n->parent_obj);
}
+} else {
+trace_nvme_irq_masked();
}
}
@@ -100,7 +113,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
trans_len = MIN(len, trans_len);
int num_prps = (len >> n->page_bits) + 1;
-if (!prp1) {
+if (unlikely(!prp1)) {
+trace_nvme_err_invalid_prp();
return NVME_INVALID_FIELD | NVME_DNR;
} else if (n->cmbsz && prp1 >= n->ctrl_mem.addr &&
prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) {
@@ -113,7 +127,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
}
len -= trans_len;
if (len) {
-if (!prp2) {
+if (unlikely(!prp2)) {
+trace_nvme_err_invalid_prp2_missing();
goto unmap;
}
if (len > n->page_size) {
@@ -128,7 +143,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
uint64_t prp_ent = le64_to_cpu(prp_list[i]);
if (i == n->max_prp_ents - 1 && len > n->page_size) {
-if (!prp_ent || prp_ent & (n->page_size - 1)) {
+if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
+trace_nvme_err_invalid_prplist_ent(prp_ent);
goto unmap;
}
@@ -140,7 +156,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
prp_ent = le64_to_cpu(prp_list[i]);
}
-if (!prp_ent || prp_ent & (n->page_size - 1)) {
+if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
+trace_nvme_err_invalid_prplist_ent(prp_ent);
goto unmap;
}
@@ -154,7 +171,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
i++;
}
} else {
-if (prp2 & (n->page_size - 1)) {
+if (unlikely(prp2 & (n->page_size - 1))) {
+trace_nvme_err_invalid_prp2_align(prp2);
goto unmap;
}
if (qsg->nsg) {
@@ -178,16 +196,20 @@ static uint16_t nvme_dma_read_prp(NvmeCtrl *n,
uint8_t *ptr, uint32_t len,
QEMUIOVector iov;
uint16_t status = NVME_SUCCESS;
+trace_nvme_dma_read(prp1, prp2);
+
if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {
return NVME_INVALID_FIELD | NVME_DNR;
}
if (qsg.nsg > 0) {
-if (dma_buf_read(ptr, len, &qsg)) {
+if (unlikely(dma_buf_read(ptr, len, &qsg))) {
+trace_nvme_err_invalid_dma();
status = NVME_INVALID_FIELD | NVME_DNR;
}
qemu_sglist_destroy(&qsg);
} else {
-if (qemu_iovec_to_buf(&iov, 0, ptr, len) != len) {
+if (unlikely(qemu_iovec_to_buf(&iov, 0, ptr, len) != len)) {
+trace_nvme_err_invalid_dma();
status = NVME_INVALID_FIELD | NVME_DNR;
}
qemu_iovec_destroy(&iov);
@@ -273,7 +295,8 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n,
NvmeNamespace *ns, NvmeCmd *cmd,
uint64_t aio_slba = slba << (data_shift - BDRV
Re: [Qemu-devel] nvme: Add tracing v3
On Fri, Oct 20, 2017 at 4:11 PM, Doug Gale wrote:
> On Thu, Oct 12, 2017 at 3:07 PM, Doug Gale wrote:
>
>> From c7f12a5949458fdd195b5e0b52f158e8f114f203 Mon Sep 17 00:00:00 2001
>> From: Doug Gale
>> Date: Thu, 12 Oct 2017 14:29:07 -0400
>> Subject: [PATCH] nvme: Add tracing
>>
>> Add trace output for commands, errors, and undefined behavior.
>> Add guest error log output for undefined behavior.
>> Report and ignore invalid undefined accesses to MMIO.
>> Annotate unlikely error checks with unlikely.
>>
>> Signed-off-by: Doug Gale
>> ---
>> hw/block/nvme.c | 347 ++
>>
>> hw/block/trace-events | 93 ++
>> 2 files changed, 387 insertions(+), 53 deletions(-)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index 9aa32692a3..adac19f5b0 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -34,8 +34,17 @@
>> #include "qapi/visitor.h"
>> #include "sysemu/block-backend.h"
>>
>> +#include "qemu/log.h"
>> +#include "trace.h"
>> #include "nvme.h"
>>
>> +#define NVME_GUEST_ERR(trace, fmt, ...) \
>> +do { \
>> +(trace_##trace)(__VA_ARGS__); \
>> +qemu_log_mask(LOG_GUEST_ERROR, #trace \
>> +" in %s: " fmt "\n", __func__, ## __VA_ARGS__); \
>> +} while (0)
>> +
>> static void nvme_process_sq(void *opaque);
>>
>> static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
>> @@ -86,10 +95,14 @@ static void nvme_isr_notify(NvmeCtrl *n, NvmeCQueue
>> *cq)
>> {
>> if (cq->irq_enabled) {
>> if (msix_enabled(&(n->parent_obj))) {
>> +trace_nvme_irq_msix(cq->vector);
>> msix_notify(&(n->parent_obj), cq->vector);
>> } else {
>> +trace_nvme_irq_pin();
>> pci_irq_pulse(&n->parent_obj);
>> }
>> +} else {
>> +trace_nvme_irq_masked();
>> }
>> }
>>
>> @@ -100,7 +113,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
>> QEMUIOVector *iov, uint64_t prp1,
>> trans_len = MIN(len, trans_len);
>> int num_prps = (len >> n->page_bits) + 1;
>>
>> -if (!prp1) {
>> +if (unlikely(!prp1)) {
>> +trace_nvme_err_invalid_prp();
>> return NVME_INVALID_FIELD | NVME_DNR;
>> } else if (n->cmbsz && prp1 >= n->ctrl_mem.addr &&
>> prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size))
>> {
>> @@ -113,7 +127,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
>> QEMUIOVector *iov, uint64_t prp1,
>> }
>> len -= trans_len;
>> if (len) {
>> -if (!prp2) {
>> +if (unlikely(!prp2)) {
>> +trace_nvme_err_invalid_prp2_missing();
>> goto unmap;
>> }
>> if (len > n->page_size) {
>> @@ -128,7 +143,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
>> QEMUIOVector *iov, uint64_t prp1,
>> uint64_t prp_ent = le64_to_cpu(prp_list[i]);
>>
>> if (i == n->max_prp_ents - 1 && len > n->page_size) {
>> -if (!prp_ent || prp_ent & (n->page_size - 1)) {
>> +if (unlikely(!prp_ent || prp_ent & (n->page_size -
>> 1))) {
>> +trace_nvme_err_invalid_prplist_ent(prp_ent);
>> goto unmap;
>> }
>>
>> @@ -140,7 +156,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
>> QEMUIOVector *iov, uint64_t prp1,
>> prp_ent = le64_to_cpu(prp_list[i]);
>> }
>>
>> -if (!prp_ent || prp_ent & (n->page_size - 1)) {
>> +if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
>> +trace_nvme_err_invalid_prplist_ent(prp_ent);
>> goto unmap;
>> }
>>
>> @@ -154,7 +171,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
>> QEMUIOVector *iov, uint64_t prp1,
>> i++;
>> }
>> } else {
>> -if (prp2 & (n->page_size - 1)) {
>> +if (unlikely(prp2 & (n->page_size - 1))) {
>> +trace_nvme_err_invalid_prp2_align(prp2);
>> goto unma
Re: [Qemu-devel] nvme: Add tracing v3
On Thu, Oct 12, 2017 at 3:07 PM, Doug Gale wrote:
> From c7f12a5949458fdd195b5e0b52f158e8f114f203 Mon Sep 17 00:00:00 2001
> From: Doug Gale
> Date: Thu, 12 Oct 2017 14:29:07 -0400
> Subject: [PATCH] nvme: Add tracing
>
> Add trace output for commands, errors, and undefined behavior.
> Add guest error log output for undefined behavior.
> Report and ignore invalid undefined accesses to MMIO.
> Annotate unlikely error checks with unlikely.
>
> Signed-off-by: Doug Gale
> ---
> hw/block/nvme.c | 347 ++
>
> hw/block/trace-events | 93 ++
> 2 files changed, 387 insertions(+), 53 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 9aa32692a3..adac19f5b0 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -34,8 +34,17 @@
> #include "qapi/visitor.h"
> #include "sysemu/block-backend.h"
>
> +#include "qemu/log.h"
> +#include "trace.h"
> #include "nvme.h"
>
> +#define NVME_GUEST_ERR(trace, fmt, ...) \
> +do { \
> +(trace_##trace)(__VA_ARGS__); \
> +qemu_log_mask(LOG_GUEST_ERROR, #trace \
> +" in %s: " fmt "\n", __func__, ## __VA_ARGS__); \
> +} while (0)
> +
> static void nvme_process_sq(void *opaque);
>
> static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
> @@ -86,10 +95,14 @@ static void nvme_isr_notify(NvmeCtrl *n, NvmeCQueue
> *cq)
> {
> if (cq->irq_enabled) {
> if (msix_enabled(&(n->parent_obj))) {
> +trace_nvme_irq_msix(cq->vector);
> msix_notify(&(n->parent_obj), cq->vector);
> } else {
> +trace_nvme_irq_pin();
> pci_irq_pulse(&n->parent_obj);
> }
> +} else {
> +trace_nvme_irq_masked();
> }
> }
>
> @@ -100,7 +113,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
> QEMUIOVector *iov, uint64_t prp1,
> trans_len = MIN(len, trans_len);
> int num_prps = (len >> n->page_bits) + 1;
>
> -if (!prp1) {
> +if (unlikely(!prp1)) {
> +trace_nvme_err_invalid_prp();
> return NVME_INVALID_FIELD | NVME_DNR;
> } else if (n->cmbsz && prp1 >= n->ctrl_mem.addr &&
> prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size))
> {
> @@ -113,7 +127,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
> QEMUIOVector *iov, uint64_t prp1,
> }
> len -= trans_len;
> if (len) {
> -if (!prp2) {
> +if (unlikely(!prp2)) {
> +trace_nvme_err_invalid_prp2_missing();
> goto unmap;
> }
> if (len > n->page_size) {
> @@ -128,7 +143,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
> QEMUIOVector *iov, uint64_t prp1,
> uint64_t prp_ent = le64_to_cpu(prp_list[i]);
>
> if (i == n->max_prp_ents - 1 && len > n->page_size) {
> -if (!prp_ent || prp_ent & (n->page_size - 1)) {
> +if (unlikely(!prp_ent || prp_ent & (n->page_size -
> 1))) {
> +trace_nvme_err_invalid_prplist_ent(prp_ent);
> goto unmap;
> }
>
> @@ -140,7 +156,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
> QEMUIOVector *iov, uint64_t prp1,
> prp_ent = le64_to_cpu(prp_list[i]);
> }
>
> -if (!prp_ent || prp_ent & (n->page_size - 1)) {
> +if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
> +trace_nvme_err_invalid_prplist_ent(prp_ent);
> goto unmap;
> }
>
> @@ -154,7 +171,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
> QEMUIOVector *iov, uint64_t prp1,
> i++;
> }
> } else {
> -if (prp2 & (n->page_size - 1)) {
> +if (unlikely(prp2 & (n->page_size - 1))) {
> +trace_nvme_err_invalid_prp2_align(prp2);
> goto unmap;
> }
> if (qsg->nsg) {
> @@ -178,16 +196,20 @@ static uint16_t nvme_dma_read_prp(NvmeCtrl *n,
> uint8_t *ptr, uint32_t len,
> QEMUIOVector iov;
> uint16_t status = NVME_SUCCESS;
>
> +trace_nvme_dma_read(prp1, prp2);
> +
> if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {
> return NVME_INVALID_FIELD | NVME_DNR;
> }
> if (qsg.nsg > 0) {
> -if (dma_buf_read(ptr, len, &q
[Qemu-devel] nvme: Add tracing v3
>From c7f12a5949458fdd195b5e0b52f158e8f114f203 Mon Sep 17 00:00:00 2001
From: Doug Gale
Date: Thu, 12 Oct 2017 14:29:07 -0400
Subject: [PATCH] nvme: Add tracing
Add trace output for commands, errors, and undefined behavior.
Add guest error log output for undefined behavior.
Report and ignore invalid undefined accesses to MMIO.
Annotate unlikely error checks with unlikely.
Signed-off-by: Doug Gale
---
hw/block/nvme.c | 347 ++
hw/block/trace-events | 93 ++
2 files changed, 387 insertions(+), 53 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9aa32692a3..adac19f5b0 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -34,8 +34,17 @@
#include "qapi/visitor.h"
#include "sysemu/block-backend.h"
+#include "qemu/log.h"
+#include "trace.h"
#include "nvme.h"
+#define NVME_GUEST_ERR(trace, fmt, ...) \
+do { \
+(trace_##trace)(__VA_ARGS__); \
+qemu_log_mask(LOG_GUEST_ERROR, #trace \
+" in %s: " fmt "\n", __func__, ## __VA_ARGS__); \
+} while (0)
+
static void nvme_process_sq(void *opaque);
static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
@@ -86,10 +95,14 @@ static void nvme_isr_notify(NvmeCtrl *n, NvmeCQueue *cq)
{
if (cq->irq_enabled) {
if (msix_enabled(&(n->parent_obj))) {
+trace_nvme_irq_msix(cq->vector);
msix_notify(&(n->parent_obj), cq->vector);
} else {
+trace_nvme_irq_pin();
pci_irq_pulse(&n->parent_obj);
}
+} else {
+trace_nvme_irq_masked();
}
}
@@ -100,7 +113,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
trans_len = MIN(len, trans_len);
int num_prps = (len >> n->page_bits) + 1;
-if (!prp1) {
+if (unlikely(!prp1)) {
+trace_nvme_err_invalid_prp();
return NVME_INVALID_FIELD | NVME_DNR;
} else if (n->cmbsz && prp1 >= n->ctrl_mem.addr &&
prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) {
@@ -113,7 +127,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
}
len -= trans_len;
if (len) {
-if (!prp2) {
+if (unlikely(!prp2)) {
+trace_nvme_err_invalid_prp2_missing();
goto unmap;
}
if (len > n->page_size) {
@@ -128,7 +143,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
uint64_t prp_ent = le64_to_cpu(prp_list[i]);
if (i == n->max_prp_ents - 1 && len > n->page_size) {
-if (!prp_ent || prp_ent & (n->page_size - 1)) {
+if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
+trace_nvme_err_invalid_prplist_ent(prp_ent);
goto unmap;
}
@@ -140,7 +156,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
prp_ent = le64_to_cpu(prp_list[i]);
}
-if (!prp_ent || prp_ent & (n->page_size - 1)) {
+if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
+trace_nvme_err_invalid_prplist_ent(prp_ent);
goto unmap;
}
@@ -154,7 +171,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
i++;
}
} else {
-if (prp2 & (n->page_size - 1)) {
+if (unlikely(prp2 & (n->page_size - 1))) {
+trace_nvme_err_invalid_prp2_align(prp2);
goto unmap;
}
if (qsg->nsg) {
@@ -178,16 +196,20 @@ static uint16_t nvme_dma_read_prp(NvmeCtrl *n,
uint8_t *ptr, uint32_t len,
QEMUIOVector iov;
uint16_t status = NVME_SUCCESS;
+trace_nvme_dma_read(prp1, prp2);
+
if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {
return NVME_INVALID_FIELD | NVME_DNR;
}
if (qsg.nsg > 0) {
-if (dma_buf_read(ptr, len, &qsg)) {
+if (unlikely(dma_buf_read(ptr, len, &qsg))) {
+trace_nvme_err_invalid_dma();
status = NVME_INVALID_FIELD | NVME_DNR;
}
qemu_sglist_destroy(&qsg);
} else {
-if (qemu_iovec_to_buf(&iov, 0, ptr, len) != len) {
+if (unlikely(qemu_iovec_to_buf(&iov, 0, ptr, len) != len)) {
+trace_nvme_err_invalid_dma();
status = NVME_INVALID_FIELD | NVME_DNR;
}
qemu_iovec_destroy(&iov);
@@ -273,7 +295,8 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n,
NvmeNamespace *ns, NvmeCmd *cmd,
uint64_t aio_slba = slba <<
Re: [Qemu-devel] Patch to add helpful tracing output for driver authors in NVMe emulation
On Tue, Oct 10, 2017 at 4:02 AM, Kevin Wolf wrote: > Am 10.10.2017 um 08:58 hat Markus Armbruster geschrieben: >> Doug Gale writes: >> >> > I used exclamations as a concise way of indicating that the driver did >> > something nonsensical, or horribly invalid, like something likely to >> > cause a memory corruption, trying to start the controller with a >> > nonsense configuration, providing invalid PRDs or writing to >> > unrecognized MMIO offsets that might hang or do something extremely >> > bad on a poorly implemented controller. Exclaiming is not shouting, I >> > thought shouting was when it was all uppercase. >> > >> > If a driver might trash memory or hang a controller, maybe it should >> > shout at the driver author or person investigating an unstable VM. >> > None of those messages with exclamations should ever happen. If any of >> > them are possible when the driver is correct, then I have made a >> > mistake. >> >> Please do not top-quote on this mailing list. >> >> Existing tracepoints do not use exclamation marks to signal severity. >> >> Consider using assertions for "if this happens, either the program's >> state is shot (and continuing is unsafe), or the author's mental state >> was shot (and continuing is probably unsafe, too)" conditions. >> Tracepoints are for tracing. > > Assertions are for checking that assumptions in qemu code hold true. > Here it's about bad guest code, and you can't let qemu abort for that. > > Tracing is the right tool to detect bad guest code, and I think it makes > sense to mark conditions that shouldn't happen with a correctly > operating guest driver. I'm not sure if an exclamation mark is the best > syntax for this, because I wouldn't have intuitively understood what > it's supposed to tell me. > > If we do use the exclamation mark, we should document it somewhere > (docs/devel/tracing.txt?) and make it a qemu-wide pattern. If not, we > should choose something else and still document it and use it > consistently. > > Kevin > Should I give up on indicating severity for now (removing the exclamations) and just group together the severe ones with a comment heading in the trace-events file? I added "" at the end of some traces to help possibly-incorrect parsing code find the end of the string reliably. Was that a good idea or is it okay and expected to end them with something like PRIx64 and drop the extra ""? I found a few cases of missing 0x that I will fix in the next version of this patch. Policy is to have 0x before every hex value, right? Also, so I'm clear, when I submit the patch I should put the patch first and put my message after the -- at the end? And use "nvme: Add tracing" for the commit message, And put nvme: Add tracing v3 in my subject line in a completely new email thread?
Re: [Qemu-devel] Patch to add helpful tracing output for driver authors in NVMe emulation
I used exclamations as a concise way of indicating that the driver did something nonsensical, or horribly invalid, like something likely to cause a memory corruption, trying to start the controller with a nonsense configuration, providing invalid PRDs or writing to unrecognized MMIO offsets that might hang or do something extremely bad on a poorly implemented controller. Exclaiming is not shouting, I thought shouting was when it was all uppercase. If a driver might trash memory or hang a controller, maybe it should shout at the driver author or person investigating an unstable VM. None of those messages with exclamations should ever happen. If any of them are possible when the driver is correct, then I have made a mistake. On Mon, Oct 9, 2017 at 11:52 AM, Eric Blake wrote: > On 10/07/2017 02:51 AM, Doug Gale wrote: >> Completely re-implemented patch, with significant improvements (now >> specifies values in several places I missed, also reduced the amount >> of redundant lines). I used the nvme_ as the tracing infrastructure >> prefix. Tested with -trace nvme_* on the qemu command line, worked for >> me. > > This information belongs... > >> >>>From 166f57458d60d363a10a0933c3e860985531ac96 Mon Sep 17 00:00:00 2001 >> From: Doug Gale >> Date: Thu, 5 Oct 2017 19:02:03 -0400 >> Subject: [PATCH] Add tracing output to NVMe emulation to help driver authors. >> >> This uses the tracing infrastructure using nvme_ as the prefix. >> >> Signed-off-by: Doug Gale >> --- > > ...here, after the --- separator. It is useful to the patch reviewer, > but does not need to be in the 'git log' history. The maintainers use > 'git am' to process incoming patches, which automatically prunes review > comments located in this location. > > Also, since this is a version 2 patch, it is best if your subject line > includes 'v2', and if you send the patch as a new top-level thread > rather than in-reply-to v1. This can be done with 'git send-email -v2'. > > The subject line is atypical; we tend to prefer 'topic: Short summary', > where you are missing the topic, you had a trailing dot that is not > typical, and where your line is longer than usual. A better subject > line might be: > > nvme: Add tracing output > > > For more helpful information on patch submission: > > https://wiki.qemu.org/Contribute/SubmitAPatch > > I didn't look closely at the patch itself, but did notice: > >> +nvme_mmio_start_failed(void) "setting controller enable bit failed!" >> +nvme_mmio_start_success(void) "setting controller enable bit succeeded" >> +nvme_mmio_stopped(void) "cleared controller enable bit" >> +nvme_mmio_shutdown_set(void) "shutdown bit set" >> +nvme_mmio_shutdown_cleared(void) "shutdown bit cleared" >> +nvme_mmio_ignored(uint64_t offset, uint64_t data) "invalid MMIO >> write, offset=0x%"PRIx64", data=%"PRIx64"!" > > You have a couple of traces with a trailing '!'; that is atypical, > because we don't need our traces to shout at the user. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org >
Re: [Qemu-devel] Patch to add helpful tracing output for driver authors in NVMe emulation
Completely re-implemented patch, with significant improvements (now
specifies values in several places I missed, also reduced the amount
of redundant lines). I used the nvme_ as the tracing infrastructure
prefix. Tested with -trace nvme_* on the qemu command line, worked for
me.
>From 166f57458d60d363a10a0933c3e860985531ac96 Mon Sep 17 00:00:00 2001
From: Doug Gale
Date: Thu, 5 Oct 2017 19:02:03 -0400
Subject: [PATCH] Add tracing output to NVMe emulation to help driver authors.
This uses the tracing infrastructure using nvme_ as the prefix.
Signed-off-by: Doug Gale
---
hw/block/nvme.c | 158 +-
hw/block/trace-events | 89
2 files changed, 233 insertions(+), 14 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9aa32692a3..3e3cd820a3 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -34,6 +34,7 @@
#include "qapi/visitor.h"
#include "sysemu/block-backend.h"
+#include "trace.h"
#include "nvme.h"
static void nvme_process_sq(void *opaque);
@@ -86,10 +87,14 @@ static void nvme_isr_notify(NvmeCtrl *n, NvmeCQueue *cq)
{
if (cq->irq_enabled) {
if (msix_enabled(&(n->parent_obj))) {
+trace_nvme_msix_intr(cq->vector);
msix_notify(&(n->parent_obj), cq->vector);
} else {
+trace_nvme_pin_intr();
pci_irq_pulse(&n->parent_obj);
}
+} else {
+trace_nvme_masked_intr();
}
}
@@ -101,6 +106,7 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
int num_prps = (len >> n->page_bits) + 1;
if (!prp1) {
+trace_nvme_invalid_prp();
return NVME_INVALID_FIELD | NVME_DNR;
} else if (n->cmbsz && prp1 >= n->ctrl_mem.addr &&
prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) {
@@ -114,6 +120,7 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
len -= trans_len;
if (len) {
if (!prp2) {
+trace_nvme_invalid_prp2_missing();
goto unmap;
}
if (len > n->page_size) {
@@ -129,6 +136,7 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
if (i == n->max_prp_ents - 1 && len > n->page_size) {
if (!prp_ent || prp_ent & (n->page_size - 1)) {
+trace_nvme_invalid_prplist_ent(prp_ent);
goto unmap;
}
@@ -141,6 +149,7 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
}
if (!prp_ent || prp_ent & (n->page_size - 1)) {
+trace_nvme_invalid_prplist_ent(prp_ent);
goto unmap;
}
@@ -155,6 +164,7 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
}
} else {
if (prp2 & (n->page_size - 1)) {
+trace_nvme_invalid_prp2_align(prp2);
goto unmap;
}
if (qsg->nsg) {
@@ -178,16 +188,20 @@ static uint16_t nvme_dma_read_prp(NvmeCtrl *n,
uint8_t *ptr, uint32_t len,
QEMUIOVector iov;
uint16_t status = NVME_SUCCESS;
+trace_nvme_dma_read(prp1, prp2);
+
if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {
return NVME_INVALID_FIELD | NVME_DNR;
}
if (qsg.nsg > 0) {
if (dma_buf_read(ptr, len, &qsg)) {
+trace_nvme_dma_too_short();
status = NVME_INVALID_FIELD | NVME_DNR;
}
qemu_sglist_destroy(&qsg);
} else {
if (qemu_iovec_to_buf(&iov, 0, ptr, len) != len) {
+trace_nvme_dma_too_short();
status = NVME_INVALID_FIELD | NVME_DNR;
}
qemu_iovec_destroy(&iov);
@@ -274,6 +288,7 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n,
NvmeNamespace *ns, NvmeCmd *cmd,
uint32_t aio_nlb = nlb << (data_shift - BDRV_SECTOR_BITS);
if (slba + nlb > ns->id_ns.nsze) {
+trace_nvme_invalid_lba_range(slba, nlb, ns->id_ns.nsze);
return NVME_LBA_RANGE | NVME_DNR;
}
@@ -301,8 +316,11 @@ static uint16_t nvme_rw(NvmeCtrl *n,
NvmeNamespace *ns, NvmeCmd *cmd,
int is_write = rw->opcode == NVME_CMD_WRITE ? 1 : 0;
enum BlockAcctType acct = is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ;
+trace_nvme_rw(is_write ? "write" : "read", nlb, data_size, slba);
+
if ((slba + nlb) > ns->id_ns.nsze) {
block_acct_invalid(blk_get_stats(n->conf.blk), acct);
+trace_nvme_invalid_lba_range(slba, nlb, ns->id_ns.nsze);
return NVME_LBA_RANGE | NVME_DNR;
}
@@ -337,6 +355,7 @@ static uint16_t nvme_io_cmd(Nv
Re: [Qemu-devel] Patch to add helpful tracing output for driver authors in NVMe emulation
I tried to get tracing to work before and I have never gotten it working. I'll give it another try and redo the patch with tracing infrastructure if necessary. The printf are nice because the dev can just look at the terminal where qemu is running. Can you view the trace output in realtime? When their code is sitting at a breakpoint in their driver, can they see the last thing that was traced right there? Or do they have to go run some cumbersome command and wade through it after an entire test run after the vm shut down? On Fri, Oct 6, 2017 at 9:54 AM, Daniel P. Berrange wrote: > On Fri, Oct 06, 2017 at 08:50:31AM -0500, Eric Blake wrote: >> On 10/05/2017 06:18 PM, Doug Gale wrote: >> > I added the tracing output in this patch to assist me in implementing >> > an NVMe driver. It helped tremendously. >> > >> >>From 1d19086cdef8d492929852d582cb41dcc5026f71 Mon Sep 17 00:00:00 2001 >> > From: Doug Gale >> > Date: Thu, 5 Oct 2017 19:02:03 -0400 >> > Subject: [PATCH] Add tracing output to NVMe emulation to help driver >> > authors. >> > >> > It is off by default, enable it by uncommenting #define DEBUG_NVME >> > or through CFLAGS >> > >> > Signed-off-by: Doug Gale >> > --- >> > hw/block/nvme.c | 191 >> > +++- >> > 1 file changed, 177 insertions(+), 14 deletions(-) >> > >> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c >> > index 9aa32692a3..74220c0171 100644 >> > --- a/hw/block/nvme.c >> > +++ b/hw/block/nvme.c >> > @@ -36,6 +36,14 @@ >> > >> > #include "nvme.h" >> > >> > +//#define DEBUG_NVME >> > + >> > +#ifdef DEBUG_NVME >> > +#define DPRINTF(fmt, ...) fprintf(stderr, "nvme: " fmt "\n", ## >> > __VA_ARGS__) >> > +#else >> > +#define DPRINTF(fmt, ...) ((void)0) >> > +#endif >> >> As Kevin said, using trace-events is nicer than fprintf(). But if you >> absolutely insist on fprintf(), then do NOT do it like this, because >> this leads to bit-rot. Instead, do it in a form that lets -Wformat >> checking work even when tracing is disabled: > > [snip] > > IMHO using of trace() instead of fprintf() is pretty much mandatory > at this point. We've been making a concious effort to replace fprintfs > with trace across code, so shouldn't add more fprintfs. It is just so > much more useful to be able to enable the debugging without having to > recompile the binary. > > Regards, > Daniel > -- > |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o-https://fstop138.berrange.com :| > |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[Qemu-devel] Patch to add helpful tracing output for driver authors in NVMe emulation
I added the tracing output in this patch to assist me in implementing
an NVMe driver. It helped tremendously.
>From 1d19086cdef8d492929852d582cb41dcc5026f71 Mon Sep 17 00:00:00 2001
From: Doug Gale
Date: Thu, 5 Oct 2017 19:02:03 -0400
Subject: [PATCH] Add tracing output to NVMe emulation to help driver authors.
It is off by default, enable it by uncommenting #define DEBUG_NVME
or through CFLAGS
Signed-off-by: Doug Gale
---
hw/block/nvme.c | 191 +++-
1 file changed, 177 insertions(+), 14 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9aa32692a3..74220c0171 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -36,6 +36,14 @@
#include "nvme.h"
+//#define DEBUG_NVME
+
+#ifdef DEBUG_NVME
+#define DPRINTF(fmt, ...) fprintf(stderr, "nvme: " fmt "\n", ## __VA_ARGS__)
+#else
+#define DPRINTF(fmt, ...) ((void)0)
+#endif
+
static void nvme_process_sq(void *opaque);
static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
@@ -86,10 +94,14 @@ static void nvme_isr_notify(NvmeCtrl *n, NvmeCQueue *cq)
{
if (cq->irq_enabled) {
if (msix_enabled(&(n->parent_obj))) {
+DPRINTF("raising MSI-X IRQ vector %u", cq->vector);
msix_notify(&(n->parent_obj), cq->vector);
} else {
+DPRINTF("pulsing IRQ pin");
pci_irq_pulse(&n->parent_obj);
}
+} else {
+DPRINTF("IRQ is masked");
}
}
@@ -101,9 +113,11 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
int num_prps = (len >> n->page_bits) + 1;
if (!prp1) {
+DPRINTF("Invalid PRP!");
return NVME_INVALID_FIELD | NVME_DNR;
} else if (n->cmbsz && prp1 >= n->ctrl_mem.addr &&
prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) {
+DPRINTF("PRP in controller memory");
qsg->nsg = 0;
qemu_iovec_init(iov, num_prps);
qemu_iovec_add(iov, (void *)&n->cmbuf[prp1 -
n->ctrl_mem.addr], trans_len);
@@ -168,6 +182,7 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
QEMUIOVector *iov, uint64_t prp1,
unmap:
qemu_sglist_destroy(qsg);
+DPRINTF("invalid SGL!");
return NVME_INVALID_FIELD | NVME_DNR;
}
@@ -178,16 +193,22 @@ static uint16_t nvme_dma_read_prp(NvmeCtrl *n,
uint8_t *ptr, uint32_t len,
QEMUIOVector iov;
uint16_t status = NVME_SUCCESS;
+DPRINTF("DMA read, prp1=0x%"PRIx64" prp2=0x%"PRIx64,
+prp1, prp2);
+
if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {
+DPRINTF("DMA read invalid PRP field!");
return NVME_INVALID_FIELD | NVME_DNR;
}
if (qsg.nsg > 0) {
if (dma_buf_read(ptr, len, &qsg)) {
+DPRINTF("DMA read invalid SGL field!");
status = NVME_INVALID_FIELD | NVME_DNR;
}
qemu_sglist_destroy(&qsg);
} else {
if (qemu_iovec_to_buf(&iov, 0, ptr, len) != len) {
+DPRINTF("invalid field!");
status = NVME_INVALID_FIELD | NVME_DNR;
}
qemu_iovec_destroy(&iov);
@@ -274,6 +295,7 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n,
NvmeNamespace *ns, NvmeCmd *cmd,
uint32_t aio_nlb = nlb << (data_shift - BDRV_SECTOR_BITS);
if (slba + nlb > ns->id_ns.nsze) {
+DPRINTF("Invalid LBA!");
return NVME_LBA_RANGE | NVME_DNR;
}
@@ -301,13 +323,19 @@ static uint16_t nvme_rw(NvmeCtrl *n,
NvmeNamespace *ns, NvmeCmd *cmd,
int is_write = rw->opcode == NVME_CMD_WRITE ? 1 : 0;
enum BlockAcctType acct = is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ;
+DPRINTF("%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64,
+is_write ? "write" : "read",
+nlb, data_size, slba);
+
if ((slba + nlb) > ns->id_ns.nsze) {
block_acct_invalid(blk_get_stats(n->conf.blk), acct);
+DPRINTF("Invalid LBA!");
return NVME_LBA_RANGE | NVME_DNR;
}
if (nvme_map_prp(&req->qsg, &req->iov, prp1, prp2, data_size, n)) {
block_acct_invalid(blk_get_stats(n->conf.blk), acct);
+DPRINTF("Invalid PRP!");
return NVME_INVALID_FIELD | NVME_DNR;
}
@@ -337,6 +365,7 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
uint32_t nsid = le32_to_cpu(cmd->nsid);
if (nsid == 0 || nsid > n->num_namespaces) {
+DPRINTF("Invalid namespace!");
return NVME_INVALID_NSID | NVME_DNR;
}
@@ -350,6 +379,7 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
case NVME_CMD_READ:
Re: [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive
Right, only GETLINE_* states write to the linebuf, so line_buf_index <
1 is correct. Updated patch:
>From 2e6c45260cae60bbae446bffe43f948ab002c529 Mon Sep 17 00:00:00 2001
From: Doug Gale
Date: Mon, 1 May 2017 12:22:10 -0400
Subject: [PATCH] gdbstub: implement remote debugging protocol escapes for
command receive
- decode escape sequences
- decompress run-length encoding escape sequences
- report command parsing problems to output when debug output is enabled
- reject packet checksums that are not valid hex digits
- compute the checksum based on the packet stream, not based on the
decoded packet
Tested with GDB and QtCreator integrated debugger on SMP QEMU instance.
Works for me.
Signed-off-by: Doug Gale
---
gdbstub.c | 108 --
1 file changed, 99 insertions(+), 9 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 9911153..dee0ff3 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -286,6 +286,8 @@ enum RSState {
RS_INACTIVE,
RS_IDLE,
RS_GETLINE,
+RS_GETLINE_ESC,
+RS_GETLINE_RLE,
RS_CHKSUM1,
RS_CHKSUM2,
};
@@ -296,7 +298,8 @@ typedef struct GDBState {
enum RSState state; /* parsing state */
char line_buf[MAX_PACKET_LENGTH];
int line_buf_index;
-int line_csum;
+int line_sum; /* running checksum */
+int line_csum; /* checksum at the end of the packet */
uint8_t last_packet[MAX_PACKET_LENGTH + 4];
int last_packet_len;
int signal;
@@ -1508,7 +1511,6 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb,
const char *fmt, ...)
static void gdb_read_byte(GDBState *s, int ch)
{
-int i, csum;
uint8_t reply;
#ifndef CONFIG_USER_ONLY
@@ -1542,35 +1544,123 @@ static void gdb_read_byte(GDBState *s, int ch)
switch(s->state) {
case RS_IDLE:
if (ch == '$') {
+/* start of command packet */
s->line_buf_index = 0;
+s->line_sum = 0;
s->state = RS_GETLINE;
+} else {
+#ifdef DEBUG_GDB
+printf("gdbstub received garbage between packets: 0x%x\n", ch);
+#endif
}
break;
case RS_GETLINE:
+if (ch == '}') {
+/* start escape sequence */
+s->state = RS_GETLINE_ESC;
+s->line_sum += ch;
+} else if (ch == '*') {
+/* start run length encoding sequence */
+s->state = RS_GETLINE_RLE;
+s->line_sum += ch;
+} else if (ch == '#') {
+/* end of command, start of checksum*/
+s->state = RS_CHKSUM1;
+} else if (s->line_buf_index >= sizeof(s->line_buf) - 1) {
+#ifdef DEBUG_GDB
+printf("gdbstub command buffer overrun, dropping command\n");
+#endif
+s->state = RS_IDLE;
+} else {
+/* unescaped command character */
+s->line_buf[s->line_buf_index++] = ch;
+s->line_sum += ch;
+}
+break;
+case RS_GETLINE_ESC:
if (ch == '#') {
-s->state = RS_CHKSUM1;
+/* unexpected end of command in escape sequence */
+s->state = RS_CHKSUM1;
} else if (s->line_buf_index >= sizeof(s->line_buf) - 1) {
+/* command buffer overrun */
+#ifdef DEBUG_GDB
+printf("gdbstub command buffer overrun, dropping command\n");
+#endif
s->state = RS_IDLE;
} else {
-s->line_buf[s->line_buf_index++] = ch;
+/* parse escaped character and leave escape state */
+s->line_buf[s->line_buf_index++] = ch ^ 0x20;
+s->line_sum += ch;
+s->state = RS_GETLINE;
+}
+break;
+case RS_GETLINE_RLE:
+if (ch < ' ') {
+/* invalid RLE count encoding */
+#ifdef DEBUG_GDB
+printf("gdbstub got invalid RLE count: 0x%x\n", ch);
+#endif
+s->state = RS_GETLINE;
+} else {
+/* decode repeat length */
+int repeat = (unsigned char)ch - ' ' + 3;
+if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) {
+/* that many repeats would overrun the command buffer */
+#ifdef DEBUG_GDB
+printf("gdbstub command buffer overrun,"
+ " dropping command\n");
+#endif
+s->state = RS_IDLE;
+} else if (s->line_buf_index < 1) {
+/* got a repeat but we have nothing to repeat */
+#ifdef DEBUG_GDB
+printf("gdbstub go
Re: [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive
Updated patch with comments addressed:
>From 6bce4e5c87c255f10b22d2bf6fc951dde2bbf457 Mon Sep 17 00:00:00 2001
From: Doug Gale
Date: Mon, 1 May 2017 12:22:10 -0400
Subject: [PATCH] gdbstub: implement remote debugging protocol escapes for
command receive
- decode escape sequences
- decompress run-length encoding escape sequences
- report command parsing problems to output when debug output is enabled
- reject packet checksums that are not valid hex digits
- compute the checksum based on the packet stream, not based on the
decoded packet
Tested with GDB and QtCreator integrated debugger on SMP QEMU instance.
Works for me.
Signed-off-by: Doug Gale
---
gdbstub.c | 108 --
1 file changed, 99 insertions(+), 9 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 9911153..3abaf7c 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -286,6 +286,8 @@ enum RSState {
RS_INACTIVE,
RS_IDLE,
RS_GETLINE,
+RS_GETLINE_ESC,
+RS_GETLINE_RLE,
RS_CHKSUM1,
RS_CHKSUM2,
};
@@ -296,7 +298,8 @@ typedef struct GDBState {
enum RSState state; /* parsing state */
char line_buf[MAX_PACKET_LENGTH];
int line_buf_index;
-int line_csum;
+int line_sum; /* running checksum */
+int line_csum; /* checksum at the end of the packet */
uint8_t last_packet[MAX_PACKET_LENGTH + 4];
int last_packet_len;
int signal;
@@ -1508,7 +1511,6 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb,
const char *fmt, ...)
static void gdb_read_byte(GDBState *s, int ch)
{
-int i, csum;
uint8_t reply;
#ifndef CONFIG_USER_ONLY
@@ -1542,35 +1544,123 @@ static void gdb_read_byte(GDBState *s, int ch)
switch(s->state) {
case RS_IDLE:
if (ch == '$') {
+/* start of command packet */
s->line_buf_index = 0;
+s->line_sum = 0;
s->state = RS_GETLINE;
+} else {
+#ifdef DEBUG_GDB
+printf("gdbstub received garbage between packets: 0x%x\n", ch);
+#endif
}
break;
case RS_GETLINE:
+if (ch == '}') {
+/* start escape sequence */
+s->state = RS_GETLINE_ESC;
+s->line_sum += ch;
+} else if (ch == '*') {
+/* start run length encoding sequence */
+s->state = RS_GETLINE_RLE;
+s->line_sum += ch;
+} else if (ch == '#') {
+/* end of command, start of checksum*/
+s->state = RS_CHKSUM1;
+} else if (s->line_buf_index >= sizeof(s->line_buf) - 1) {
+#ifdef DEBUG_GDB
+printf("gdbstub command buffer overrun, dropping command\n");
+#endif
+s->state = RS_IDLE;
+} else {
+/* unescaped command character */
+s->line_buf[s->line_buf_index++] = ch;
+s->line_sum += ch;
+}
+break;
+case RS_GETLINE_ESC:
if (ch == '#') {
-s->state = RS_CHKSUM1;
+/* unexpected end of command in escape sequence */
+s->state = RS_CHKSUM1;
} else if (s->line_buf_index >= sizeof(s->line_buf) - 1) {
+/* command buffer overrun */
+#ifdef DEBUG_GDB
+printf("gdbstub command buffer overrun, dropping command\n");
+#endif
s->state = RS_IDLE;
} else {
-s->line_buf[s->line_buf_index++] = ch;
+/* parse escaped character and leave escape state */
+s->line_buf[s->line_buf_index++] = ch ^ 0x20;
+s->line_sum += ch;
+s->state = RS_GETLINE;
+}
+break;
+case RS_GETLINE_RLE:
+if (ch < ' ') {
+/* invalid RLE count encoding */
+#ifdef DEBUG_GDB
+printf("gdbstub got invalid RLE count: 0x%x\n", ch);
+#endif
+s->state = RS_GETLINE;
+} else {
+/* decode repeat length */
+int repeat = (unsigned char)ch - ' ' + 3;
+if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) {
+/* that many repeats would overrun the command buffer */
+#ifdef DEBUG_GDB
+printf("gdbstub command buffer overrun,"
+ " dropping command\n");
+#endif
+s->state = RS_IDLE;
+} else if (s->line_buf_index < 2) {
+/* got a repeat but we have nothing to repeat */
+#ifdef DEBUG_GDB
+printf("gdbstub got invalid RLE sequence\n");
+#endif
+
Re: [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive
[Oops, forgot to reply all, resending...]
Yes, on second thought, <= 2 is off by one. [0] would be the '$', [1]
would be the repeated character, and [2] would be the '*'.
And yes, there is a missing s->state = RS_IDLE there. Good catch. I'll
post updated patch shortly...
On Fri, May 5, 2017 at 10:45 AM, Stefan Hajnoczi wrote:
> On Tue, May 02, 2017 at 10:32:40AM -0400, Doug Gale wrote:
>> +} else {
>> +/* decode repeat length */
>> +int repeat = (unsigned char)ch - ' ' + 3;
>> +if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) {
>> +/* that many repeats would overrun the command buffer */
>> +#ifdef DEBUG_GDB
>> +printf("gdbstub command buffer overrun,"
>> + " dropping command\n");
>> +#endif
>> +s->state = RS_IDLE;
>> +} else if (s->line_buf_index <= 2) {
>
> Why s->line_buf_index <= 2? I expected s->line_buf_index < 1 since we
> just need 1 character to clone for run-length decoding.
>
>> +/* got a repeat but we have nothing to repeat */
>> +#ifdef DEBUG_GDB
>> +printf("gdbstub got invalid RLE sequence\n");
>> +#endif
>> +} else {
>
> Missing s->state = RS_IDLE?
Re: [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive
Oops. Thanks, here's the patch inline.
>From c238752f10434970af8ef620ce3bf6c0e18a20b5 Mon Sep 17 00:00:00 2001
From: Doug Gale
Date: Mon, 1 May 2017 12:22:10 -0400
Subject: [PATCH] gdbstub: implement remote debugging protocol escapes for
command receive
- decode escape sequences
- decompress run-length encoding escape sequences
- report command parsing problems to output when debug output is enabled
- reject packet checksums that are not valid hex digits
- compute the checksum based on the packet stream, not based on the
decoded packet
Tested with GDB and QtCreator integrated debugger on SMP QEMU instance.
Works for me.
Signed-off-by: Doug Gale
---
gdbstub.c | 107 --
1 file changed, 98 insertions(+), 9 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 9911153..d6f1b0e 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -286,6 +286,8 @@ enum RSState {
RS_INACTIVE,
RS_IDLE,
RS_GETLINE,
+RS_GETLINE_ESC,
+RS_GETLINE_RLE,
RS_CHKSUM1,
RS_CHKSUM2,
};
@@ -296,7 +298,8 @@ typedef struct GDBState {
enum RSState state; /* parsing state */
char line_buf[MAX_PACKET_LENGTH];
int line_buf_index;
-int line_csum;
+int line_sum; /* running checksum */
+int line_csum; /* checksum at the end of the packet */
uint8_t last_packet[MAX_PACKET_LENGTH + 4];
int last_packet_len;
int signal;
@@ -1508,7 +1511,6 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb,
const char *fmt, ...)
static void gdb_read_byte(GDBState *s, int ch)
{
-int i, csum;
uint8_t reply;
#ifndef CONFIG_USER_ONLY
@@ -1542,35 +1544,122 @@ static void gdb_read_byte(GDBState *s, int ch)
switch(s->state) {
case RS_IDLE:
if (ch == '$') {
+/* start of command packet */
s->line_buf_index = 0;
+s->line_sum = 0;
s->state = RS_GETLINE;
+} else {
+#ifdef DEBUG_GDB
+printf("gdbstub received garbage between packets: 0x%x\n", ch);
+#endif
}
break;
case RS_GETLINE:
+if (ch == '}') {
+/* start escape sequence */
+s->state = RS_GETLINE_ESC;
+s->line_sum += ch;
+} else if (ch == '*') {
+/* start run length encoding sequence */
+s->state = RS_GETLINE_RLE;
+s->line_sum += ch;
+} else if (ch == '#') {
+/* end of command, start of checksum*/
+s->state = RS_CHKSUM1;
+} else if (s->line_buf_index >= sizeof(s->line_buf) - 1) {
+#ifdef DEBUG_GDB
+printf("gdbstub command buffer overrun, dropping command\n");
+#endif
+s->state = RS_IDLE;
+} else {
+/* unescaped command character */
+s->line_buf[s->line_buf_index++] = ch;
+s->line_sum += ch;
+}
+break;
+case RS_GETLINE_ESC:
if (ch == '#') {
-s->state = RS_CHKSUM1;
+/* unexpected end of command in escape sequence */
+s->state = RS_CHKSUM1;
} else if (s->line_buf_index >= sizeof(s->line_buf) - 1) {
+/* command buffer overrun */
+#ifdef DEBUG_GDB
+printf("gdbstub command buffer overrun, dropping command\n");
+#endif
s->state = RS_IDLE;
} else {
-s->line_buf[s->line_buf_index++] = ch;
+/* parse escaped character and leave escape state */
+s->line_buf[s->line_buf_index++] = ch ^ 0x20;
+s->line_sum += ch;
+s->state = RS_GETLINE;
+}
+break;
+case RS_GETLINE_RLE:
+if (ch < ' ') {
+/* invalid RLE count encoding */
+#ifdef DEBUG_GDB
+printf("gdbstub got invalid RLE count: 0x%x\n", ch);
+#endif
+s->state = RS_GETLINE;
+} else {
+/* decode repeat length */
+int repeat = (unsigned char)ch - ' ' + 3;
+if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) {
+/* that many repeats would overrun the command buffer */
+#ifdef DEBUG_GDB
+printf("gdbstub command buffer overrun,"
+ " dropping command\n");
+#endif
+s->state = RS_IDLE;
+} else if (s->line_buf_index <= 2) {
+/* got a repeat but we have nothing to repeat */
+#ifdef DEBUG_GDB
+printf("gdbstub got invalid RLE sequence\n");
+#endif
+
[Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive
The attached patch implements the GDB Remote Serial Protocol for command receive as per the documentation provided at https://sourceware.org/gdb/onlinedocs/gdb/Remote-Protocol.html#Remote-Protocol and from inspection of remote.c in the gdb source (the documentation didn't clearly document whether the packet or the unescaped data were used for the checksum, turns out the packet data is used for the checksum, as expected). get_maintainer.pl didn't find a maintainer for gdbstub.c, so I didn't cc any maintainers. patch Description: Binary data
