Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system
On 08/25/2017 09:33 AM, Philippe Mathieu-Daudé wrote: > Hi John, > > On 08/08/2017 03:32 PM, John Snow wrote: >> Out with the old, in with the new. >> >> Signed-off-by: John Snow >> --- >> Makefile.objs | 1 + >> hw/ide/cmd646.c | 10 +++- >> hw/ide/core.c | 65 >> +++ >> hw/ide/pci.c | 17 - >> hw/ide/piix.c | 11 >> hw/ide/trace-events | 33 >> hw/ide/via.c | 10 +++- >> include/hw/ide/internal.h | 1 - >> 8 files changed, 78 insertions(+), 70 deletions(-) >> create mode 100644 hw/ide/trace-events >> >> diff --git a/Makefile.objs b/Makefile.objs >> index 24a4ea0..967c092 100644 >> --- a/Makefile.objs >> +++ b/Makefile.objs >> @@ -153,6 +153,7 @@ trace-events-subdirs += hw/acpi >> trace-events-subdirs += hw/arm >> trace-events-subdirs += hw/alpha >> trace-events-subdirs += hw/xen >> +trace-events-subdirs += hw/ide >> trace-events-subdirs += ui >> trace-events-subdirs += audio >> trace-events-subdirs += net >> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c >> index 9ebb8d4..86b2a8f 100644 >> --- a/hw/ide/cmd646.c >> +++ b/hw/ide/cmd646.c >> @@ -32,6 +32,7 @@ >> #include "sysemu/dma.h" >> #include "hw/ide/pci.h" >> +#include "trace.h" >> /* CMD646 specific */ >> #define CFR0x50 >> @@ -195,9 +196,8 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr, >> val = 0xff; >> break; >> } >> -#ifdef DEBUG_IDE >> -printf("bmdma: readb " TARGET_FMT_plx " : 0x%02x\n", addr, val); >> -#endif >> + >> +trace_bmdma_read_cmd646(addr, val); >> return val; >> } >> @@ -211,9 +211,7 @@ static void bmdma_write(void *opaque, hwaddr addr, >> return; >> } >> -#ifdef DEBUG_IDE >> -printf("bmdma: writeb " TARGET_FMT_plx " : 0x%" PRIx64 "\n", >> addr, val); >> -#endif >> +trace_bmdma_write_cmd646(addr, val); >> switch(addr & 3) { >> case 0: >> bmdma_cmd_writeb(bm, val); >> diff --git a/hw/ide/core.c b/hw/ide/core.c >> index 0b48b64..53fa084 100644 >> --- a/hw/ide/core.c >> +++ b/hw/ide/core.c >> @@ -36,6 +36,7 @@ >> #include "qemu/cutils.h" >> #include "hw/ide/internal.h" >> +#include "trace.h" >> /* These values were based on a Seagate ST3500418AS but have been >> modified >> to make more sense in QEMU */ >> @@ -656,10 +657,7 @@ void ide_cancel_dma_sync(IDEState *s) >>* write requests) pending and we can avoid to drain. */ >> QLIST_FOREACH(req, &s->buffered_requests, list) { >> if (!req->orphaned) { >> -#ifdef DEBUG_IDE >> -printf("%s: invoking cb %p of buffered request %p with" >> - " -ECANCELED\n", __func__, req->original_cb, req); >> -#endif >> +trace_ide_cancel_dma_sync_buffered(req->original_cb, req); >> req->original_cb(req->original_opaque, -ECANCELED); >> } >> req->orphaned = true; >> @@ -678,9 +676,7 @@ void ide_cancel_dma_sync(IDEState *s) >>* aio operation with preadv/pwritev. >>*/ >> if (s->bus->dma->aiocb) { >> -#ifdef DEBUG_IDE >> -printf("%s: draining all remaining requests", __func__); >> -#endif >> +trace_ide_cancel_dma_sync_remaining(); >> blk_drain(s->blk); >> assert(s->bus->dma->aiocb == NULL); >> } >> @@ -741,9 +737,7 @@ static void ide_sector_read(IDEState *s) >> n = s->req_nb_sectors; >> } >> -#if defined(DEBUG_IDE) >> -printf("sector=%" PRId64 "\n", sector_num); >> -#endif >> +trace_ide_sector_read(sector_num, n); >> if (!ide_sect_range_ok(s, sector_num, n)) { >> ide_rw_error(s); >> @@ -1005,14 +999,14 @@ static void ide_sector_write(IDEState *s) >> s->status = READY_STAT | SEEK_STAT | BUSY_STAT; >> sector_num = ide_get_sector(s); >> -#if defined(DEBUG_IDE) >> -printf("sector=%" PRId64 "\n", sector_num); >> -#endif >> + >> n = s->nsector; >> if (n > s->req_nb_sectors) { >> n = s->req_nb_sectors; >> } >> +trace_ide_sector_write(sector_num, n); >> + >> if (!ide_sect_range_ok(s, sector_num, n)) { >> ide_rw_error(s); >> block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_WRITE); >> @@ -1186,18 +1180,17 @@ static void ide_clear_hob(IDEBus *bus) >> void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val) >> { >> IDEBus *bus = opaque; >> +IDEState *s = idebus_active_if(bus); >> +int reg_num = addr & 7; >> -#ifdef DEBUG_IDE >> -printf("IDE: write addr=0x%x val=0x%02x\n", addr, val); >> -#endif >> - >> -addr &= 7; >> +trace_ide_ioport_write(addr, val, bus, s); >> /* ignore writes to command block while busy with previous >> command */ >> -if (addr != 7 && (idebus_active_if(bus)->status & >> (BUSY_STAT|DRQ_STAT))) >> +if (reg_num != 7 && (s->status & (BU
Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system
Hi John, On 08/08/2017 03:32 PM, John Snow wrote: Out with the old, in with the new. Signed-off-by: John Snow --- Makefile.objs | 1 + hw/ide/cmd646.c | 10 +++- hw/ide/core.c | 65 +++ hw/ide/pci.c | 17 - hw/ide/piix.c | 11 hw/ide/trace-events | 33 hw/ide/via.c | 10 +++- include/hw/ide/internal.h | 1 - 8 files changed, 78 insertions(+), 70 deletions(-) create mode 100644 hw/ide/trace-events diff --git a/Makefile.objs b/Makefile.objs index 24a4ea0..967c092 100644 --- a/Makefile.objs +++ b/Makefile.objs @@ -153,6 +153,7 @@ trace-events-subdirs += hw/acpi trace-events-subdirs += hw/arm trace-events-subdirs += hw/alpha trace-events-subdirs += hw/xen +trace-events-subdirs += hw/ide trace-events-subdirs += ui trace-events-subdirs += audio trace-events-subdirs += net diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c index 9ebb8d4..86b2a8f 100644 --- a/hw/ide/cmd646.c +++ b/hw/ide/cmd646.c @@ -32,6 +32,7 @@ #include "sysemu/dma.h" #include "hw/ide/pci.h" +#include "trace.h" /* CMD646 specific */ #define CFR 0x50 @@ -195,9 +196,8 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr, val = 0xff; break; } -#ifdef DEBUG_IDE -printf("bmdma: readb " TARGET_FMT_plx " : 0x%02x\n", addr, val); -#endif + +trace_bmdma_read_cmd646(addr, val); return val; } @@ -211,9 +211,7 @@ static void bmdma_write(void *opaque, hwaddr addr, return; } -#ifdef DEBUG_IDE -printf("bmdma: writeb " TARGET_FMT_plx " : 0x%" PRIx64 "\n", addr, val); -#endif +trace_bmdma_write_cmd646(addr, val); switch(addr & 3) { case 0: bmdma_cmd_writeb(bm, val); diff --git a/hw/ide/core.c b/hw/ide/core.c index 0b48b64..53fa084 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -36,6 +36,7 @@ #include "qemu/cutils.h" #include "hw/ide/internal.h" +#include "trace.h" /* These values were based on a Seagate ST3500418AS but have been modified to make more sense in QEMU */ @@ -656,10 +657,7 @@ void ide_cancel_dma_sync(IDEState *s) * write requests) pending and we can avoid to drain. */ QLIST_FOREACH(req, &s->buffered_requests, list) { if (!req->orphaned) { -#ifdef DEBUG_IDE -printf("%s: invoking cb %p of buffered request %p with" - " -ECANCELED\n", __func__, req->original_cb, req); -#endif +trace_ide_cancel_dma_sync_buffered(req->original_cb, req); req->original_cb(req->original_opaque, -ECANCELED); } req->orphaned = true; @@ -678,9 +676,7 @@ void ide_cancel_dma_sync(IDEState *s) * aio operation with preadv/pwritev. */ if (s->bus->dma->aiocb) { -#ifdef DEBUG_IDE -printf("%s: draining all remaining requests", __func__); -#endif +trace_ide_cancel_dma_sync_remaining(); blk_drain(s->blk); assert(s->bus->dma->aiocb == NULL); } @@ -741,9 +737,7 @@ static void ide_sector_read(IDEState *s) n = s->req_nb_sectors; } -#if defined(DEBUG_IDE) -printf("sector=%" PRId64 "\n", sector_num); -#endif +trace_ide_sector_read(sector_num, n); if (!ide_sect_range_ok(s, sector_num, n)) { ide_rw_error(s); @@ -1005,14 +999,14 @@ static void ide_sector_write(IDEState *s) s->status = READY_STAT | SEEK_STAT | BUSY_STAT; sector_num = ide_get_sector(s); -#if defined(DEBUG_IDE) -printf("sector=%" PRId64 "\n", sector_num); -#endif + n = s->nsector; if (n > s->req_nb_sectors) { n = s->req_nb_sectors; } +trace_ide_sector_write(sector_num, n); + if (!ide_sect_range_ok(s, sector_num, n)) { ide_rw_error(s); block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_WRITE); @@ -1186,18 +1180,17 @@ static void ide_clear_hob(IDEBus *bus) void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val) { IDEBus *bus = opaque; +IDEState *s = idebus_active_if(bus); +int reg_num = addr & 7; -#ifdef DEBUG_IDE -printf("IDE: write addr=0x%x val=0x%02x\n", addr, val); -#endif - -addr &= 7; +trace_ide_ioport_write(addr, val, bus, s); /* ignore writes to command block while busy with previous command */ -if (addr != 7 && (idebus_active_if(bus)->status & (BUSY_STAT|DRQ_STAT))) +if (reg_num != 7 && (s->status & (BUSY_STAT|DRQ_STAT))) { return; +} -switch(addr) { +switch(reg_num) { case 0: break; case 1: @@ -1253,9 +1246,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val) static void ide_reset(IDEState *s) { -#ifdef DEBUG_IDE -printf("ide: reset\n"); -#endif +trace_ide_reset(s); if (s->pio_aiocb) { blk_aio_cancel(s->pio_aiocb); @@ -2013,10 +2004
Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system
On 08/08/2017 04:00 PM, Eric Blake wrote: > On 08/08/2017 01:32 PM, John Snow wrote: >> Out with the old, in with the new. >> >> Signed-off-by: John Snow >> --- > >> hw/ide/piix.c | 11 >> hw/ide/trace-events | 33 >> hw/ide/via.c | 10 +++- > > Hmm - should we tweak scripts/git.orderfile to prioritize trace-events > over .c files? Then again, right now it prioritizes all .c files before > anything that didn't match, so that things like trace-events will at > least avoid falling in the middle of a patch if you use the project's > orderfile. > >> +++ b/hw/ide/cmd646.c >> @@ -32,6 +32,7 @@ >> #include "sysemu/dma.h" >> >> #include "hw/ide/pci.h" >> +#include "trace.h" >> >> /* CMD646 specific */ >> #define CFR 0x50 >> @@ -195,9 +196,8 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr, >> val = 0xff; >> break; >> } >> -#ifdef DEBUG_IDE >> -printf("bmdma: readb " TARGET_FMT_plx " : 0x%02x\n", addr, val); >> -#endif > > Yay for killing code prone to bitrot. > >> +++ b/hw/ide/core.c > >> @@ -2054,18 +2044,18 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val) >> } > >> hob = 0; >> -switch(addr) { >> +switch(reg_num) { > > Worth fixing the style to put space after switch while touching this? > >> +++ b/hw/ide/trace-events >> @@ -0,0 +1,33 @@ >> +# See docs/devel/tracing.txt for syntax documentation. >> + >> +# hw/ide/core.c > >> + >> +# hw/ide/pci.c >> +bmdma_reset(void) "" > > An empty trace? Do all the backends support it? > Not the first instance of this, so I'm assuming yes.
Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system
On 08/08/2017 05:00 PM, Eric Blake wrote: On 08/08/2017 01:32 PM, John Snow wrote: Out with the old, in with the new. Signed-off-by: John Snow --- hw/ide/piix.c | 11 hw/ide/trace-events | 33 hw/ide/via.c | 10 +++- Hmm - should we tweak scripts/git.orderfile to prioritize trace-events over .c files? Then again, right now it prioritizes all .c files before anything that didn't match, so that things like trace-events will at least avoid falling in the middle of a patch if you use the project's orderfile. It sounds like a good idea, although I'd rather prioritize .c, having trace-events at bottom. At least we can agree about top-to-bottom scripting here :)
Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system
On 08/08/2017 04:00 PM, Eric Blake wrote: > On 08/08/2017 01:32 PM, John Snow wrote: >> Out with the old, in with the new. >> >> Signed-off-by: John Snow >> --- > >> hw/ide/piix.c | 11 >> hw/ide/trace-events | 33 >> hw/ide/via.c | 10 +++- > > Hmm - should we tweak scripts/git.orderfile to prioritize trace-events > over .c files? Then again, right now it prioritizes all .c files before > anything that didn't match, so that things like trace-events will at > least avoid falling in the middle of a patch if you use the project's > orderfile. > Sorry! >> +++ b/hw/ide/cmd646.c >> @@ -32,6 +32,7 @@ >> #include "sysemu/dma.h" >> >> #include "hw/ide/pci.h" >> +#include "trace.h" >> >> /* CMD646 specific */ >> #define CFR 0x50 >> @@ -195,9 +196,8 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr, >> val = 0xff; >> break; >> } >> -#ifdef DEBUG_IDE >> -printf("bmdma: readb " TARGET_FMT_plx " : 0x%02x\n", addr, val); >> -#endif > > Yay for killing code prone to bitrot. > Yup, seeya later. >> +++ b/hw/ide/core.c > >> @@ -2054,18 +2044,18 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val) >> } > >> hob = 0; >> -switch(addr) { >> +switch(reg_num) { > > Worth fixing the style to put space after switch while touching this? > Yes. >> +++ b/hw/ide/trace-events >> @@ -0,0 +1,33 @@ >> +# See docs/devel/tracing.txt for syntax documentation. >> + >> +# hw/ide/core.c > >> + >> +# hw/ide/pci.c >> +bmdma_reset(void) "" > > An empty trace? Do all the backends support it? > Oh, I don't know... I guess I can just repeat the function name in here, or add something arbitrary. >> +# hw/ide/cmd646.c > > Worth sorting the sections by filename? > Oh, you mean alphabetically? I'd like to keep the BMDMA HBA tracers near each other, but otherwise I can, yes. > Whether or not you tweak based on my nits, > > Reviewed-by: Eric Blake > There's likely to be many nits, so I'll accept all the critique. John
Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system
On 08/08/2017 01:32 PM, John Snow wrote: > Out with the old, in with the new. > > Signed-off-by: John Snow > --- > hw/ide/piix.c | 11 > hw/ide/trace-events | 33 > hw/ide/via.c | 10 +++- Hmm - should we tweak scripts/git.orderfile to prioritize trace-events over .c files? Then again, right now it prioritizes all .c files before anything that didn't match, so that things like trace-events will at least avoid falling in the middle of a patch if you use the project's orderfile. > +++ b/hw/ide/cmd646.c > @@ -32,6 +32,7 @@ > #include "sysemu/dma.h" > > #include "hw/ide/pci.h" > +#include "trace.h" > > /* CMD646 specific */ > #define CFR 0x50 > @@ -195,9 +196,8 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr, > val = 0xff; > break; > } > -#ifdef DEBUG_IDE > -printf("bmdma: readb " TARGET_FMT_plx " : 0x%02x\n", addr, val); > -#endif Yay for killing code prone to bitrot. > +++ b/hw/ide/core.c > @@ -2054,18 +2044,18 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val) > } > hob = 0; > -switch(addr) { > +switch(reg_num) { Worth fixing the style to put space after switch while touching this? > +++ b/hw/ide/trace-events > @@ -0,0 +1,33 @@ > +# See docs/devel/tracing.txt for syntax documentation. > + > +# hw/ide/core.c > + > +# hw/ide/pci.c > +bmdma_reset(void) "" An empty trace? Do all the backends support it? > +# hw/ide/cmd646.c Worth sorting the sections by filename? Whether or not you tweak based on my nits, Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature