Re: [Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system

[John Snow]

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, >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 && 

Re: [Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system

[Philippe Mathieu-Daudé]

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, >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);

Re: [Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system

[John Snow]

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-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system

[Philippe Mathieu-Daudé]

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-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system

[John Snow]

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-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system

[Eric Blake]

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

[Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system

[John Snow]

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, >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,9 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
 IDEState *s;
 bool complete;
 
-#if defined(DEBUG_IDE)
-printf("ide: