1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v3 0/9] IDE: replace printfs with tracing
  4. View patch

[Qemu-devel] [PATCH v3 5/9] IDE: replace DEBUG_AIO with trace events

John Snow posted 9 patches 8 years, 5 months ago
[Qemu-devel] [PATCH v3 5/9] IDE: replace DEBUG_AIO with trace events
Posted by John Snow 8 years, 5 months ago 
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/atapi.c            |  5 +----
 hw/ide/core.c             | 24 +++++++++++++++++-------
 hw/ide/trace-events       |  3 +++
 include/hw/ide/internal.h |  6 ++++--
 4 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 9b84a1b..c0509c8 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -416,10 +416,7 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
         s->io_buffer_size = n * 2048;
         data_offset = 0;
     }
-#ifdef DEBUG_AIO
-    printf("aio_read_cd: lba=%u n=%d\n", s->lba, n);
-#endif
-
+    trace_ide_atapi_cmd_read_dma_cb_aio(s, s->lba, n);
     s->bus->dma->iov.iov_base = (void *)(s->io_buffer + data_offset);
     s->bus->dma->iov.iov_len = n * ATAPI_SECTOR_SIZE;
     qemu_iovec_init_external(&s->bus->dma->qiov, &s->bus->dma->iov, 1);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 82a19b1..2cc2a08 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -58,6 +58,21 @@ static const int smart_attributes[][12] = {
     { 190,  0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00, 0x00, 0x32},
 };
 
+const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT] = {
+    [IDE_DMA_READ] = "DMA READ",
+    [IDE_DMA_WRITE] = "DMA WRITE",
+    [IDE_DMA_TRIM] = "DMA TRIM",
+    [IDE_DMA_ATAPI] = "DMA ATAPI"
+};
+
+static const char *IDE_DMA_CMD_str(enum ide_dma_cmd enval)
+{
+    if (enval >= 0 && enval < IDE_DMA__COUNT) {
+        return IDE_DMA_CMD_lookup[enval];
+    }
+    return "DMA UNKNOWN CMD";
+}
+
 static void ide_dummy_transfer_stop(IDEState *s);
 
 static void padstr(char *str, const char *src, int len)
@@ -860,10 +875,7 @@ static void ide_dma_cb(void *opaque, int ret)
         goto eot;
     }
 
-#ifdef DEBUG_AIO
-    printf("ide_dma_cb: sector_num=%" PRId64 " n=%d, cmd_cmd=%d\n",
-           sector_num, n, s->dma_cmd);
-#endif
+    trace_ide_dma_cb(s, sector_num, n, IDE_DMA_CMD_str(s->dma_cmd));
 
     if ((s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) &&
         !ide_sect_range_ok(s, sector_num, n)) {
@@ -2391,9 +2403,7 @@ void ide_bus_reset(IDEBus *bus)
 
     /* pending async DMA */
     if (bus->dma->aiocb) {
-#ifdef DEBUG_AIO
-        printf("aio_cancel\n");
-#endif
+        trace_ide_bus_reset_aio();
         blk_aio_cancel(bus->dma->aiocb);
         bus->dma->aiocb = NULL;
     }
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index 8c79a6c..cc8949c 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -18,6 +18,8 @@ ide_cancel_dma_sync_remaining(void) "draining all remaining requests"
 ide_sector_read(int64_t sector_num, int nsectors) "sector=%"PRId64" nsectors=%d"
 ide_sector_write(int64_t sector_num, int nsectors) "sector=%"PRId64" nsectors=%d"
 ide_reset(void *s) "IDEstate %p"
+ide_bus_reset_aio(void) "aio_cancel"
+ide_dma_cb(void *s, int64_t sector_num, int n, const char *dma) "IDEState %p; sector_num=%"PRId64" n=%d cmd=%s"
 
 # BMDMA HBAs:
 
@@ -51,5 +53,6 @@ ide_atapi_cmd_reply_end_new(void *s, int status) "IDEState: %p; new transfer sta
 ide_atapi_cmd_check_status(void *s) "IDEState: %p"
 ide_atapi_cmd_read(void *s, const char *method, int lba, int nb_sectors) "IDEState: %p; read %s: LBA=%d nb_sectors=%d"
 ide_atapi_cmd(void *s, uint8_t cmd) "IDEState: %p; cmd: 0x%02x"
+ide_atapi_cmd_read_dma_cb_aio(void *s, int lba, int n) "IDEState: %p; aio read: lba=%d n=%d"
 # Warning: Verbose
 ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet) "IDEState: %p; limit=0x%x packet: %s"
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 74efe8a..db9fde0 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -14,7 +14,6 @@
 #include "block/scsi.h"
 
 /* debug IDE devices */
-//#define DEBUG_AIO
 #define USE_DMA_CDROM
 
 typedef struct IDEBus IDEBus;
@@ -333,12 +332,15 @@ struct unreported_events {
 };
 
 enum ide_dma_cmd {
-    IDE_DMA_READ,
+    IDE_DMA_READ = 0,
     IDE_DMA_WRITE,
     IDE_DMA_TRIM,
     IDE_DMA_ATAPI,
+    IDE_DMA__COUNT
 };
 
+extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];
+
 #define ide_cmd_is_read(s) \
 	((s)->dma_cmd == IDE_DMA_READ)
 
-- 
2.9.5
Re: [Qemu-devel] [PATCH v3 5/9] IDE: replace DEBUG_AIO with trace events
Posted by Philippe Mathieu-Daudé 8 years, 5 months ago 
On 08/31/2017 09:14 PM, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   hw/ide/atapi.c            |  5 +----
>   hw/ide/core.c             | 24 +++++++++++++++++-------
>   hw/ide/trace-events       |  3 +++
>   include/hw/ide/internal.h |  6 ++++--
>   4 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 9b84a1b..c0509c8 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -416,10 +416,7 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
>           s->io_buffer_size = n * 2048;
>           data_offset = 0;
>       }
> -#ifdef DEBUG_AIO
> -    printf("aio_read_cd: lba=%u n=%d\n", s->lba, n);
> -#endif
> -
> +    trace_ide_atapi_cmd_read_dma_cb_aio(s, s->lba, n);
>       s->bus->dma->iov.iov_base = (void *)(s->io_buffer + data_offset);
>       s->bus->dma->iov.iov_len = n * ATAPI_SECTOR_SIZE;
>       qemu_iovec_init_external(&s->bus->dma->qiov, &s->bus->dma->iov, 1);
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 82a19b1..2cc2a08 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -58,6 +58,21 @@ static const int smart_attributes[][12] = {
>       { 190,  0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00, 0x00, 0x32},
>   };
>   
> +const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT] = {
> +    [IDE_DMA_READ] = "DMA READ",
> +    [IDE_DMA_WRITE] = "DMA WRITE",
> +    [IDE_DMA_TRIM] = "DMA TRIM",
> +    [IDE_DMA_ATAPI] = "DMA ATAPI"
> +};
> +
> +static const char *IDE_DMA_CMD_str(enum ide_dma_cmd enval)
> +{
> +    if (enval >= 0 && enval < IDE_DMA__COUNT) {
> +        return IDE_DMA_CMD_lookup[enval];
> +    }
> +    return "DMA UNKNOWN CMD";
> +}
> +
>   static void ide_dummy_transfer_stop(IDEState *s);
>   
>   static void padstr(char *str, const char *src, int len)
> @@ -860,10 +875,7 @@ static void ide_dma_cb(void *opaque, int ret)
>           goto eot;
>       }
>   
> -#ifdef DEBUG_AIO
> -    printf("ide_dma_cb: sector_num=%" PRId64 " n=%d, cmd_cmd=%d\n",
> -           sector_num, n, s->dma_cmd);
> -#endif
> +    trace_ide_dma_cb(s, sector_num, n, IDE_DMA_CMD_str(s->dma_cmd));
>   
>       if ((s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) &&
>           !ide_sect_range_ok(s, sector_num, n)) {
> @@ -2391,9 +2403,7 @@ void ide_bus_reset(IDEBus *bus)
>   
>       /* pending async DMA */
>       if (bus->dma->aiocb) {
> -#ifdef DEBUG_AIO
> -        printf("aio_cancel\n");
> -#endif
> +        trace_ide_bus_reset_aio();
>           blk_aio_cancel(bus->dma->aiocb);
>           bus->dma->aiocb = NULL;
>       }
> diff --git a/hw/ide/trace-events b/hw/ide/trace-events
> index 8c79a6c..cc8949c 100644
> --- a/hw/ide/trace-events
> +++ b/hw/ide/trace-events
> @@ -18,6 +18,8 @@ ide_cancel_dma_sync_remaining(void) "draining all remaining requests"
>   ide_sector_read(int64_t sector_num, int nsectors) "sector=%"PRId64" nsectors=%d"
>   ide_sector_write(int64_t sector_num, int nsectors) "sector=%"PRId64" nsectors=%d"
>   ide_reset(void *s) "IDEstate %p"
> +ide_bus_reset_aio(void) "aio_cancel"
> +ide_dma_cb(void *s, int64_t sector_num, int n, const char *dma) "IDEState %p; sector_num=%"PRId64" n=%d cmd=%s"
>   
>   # BMDMA HBAs:
>   
> @@ -51,5 +53,6 @@ ide_atapi_cmd_reply_end_new(void *s, int status) "IDEState: %p; new transfer sta
>   ide_atapi_cmd_check_status(void *s) "IDEState: %p"
>   ide_atapi_cmd_read(void *s, const char *method, int lba, int nb_sectors) "IDEState: %p; read %s: LBA=%d nb_sectors=%d"
>   ide_atapi_cmd(void *s, uint8_t cmd) "IDEState: %p; cmd: 0x%02x"
> +ide_atapi_cmd_read_dma_cb_aio(void *s, int lba, int n) "IDEState: %p; aio read: lba=%d n=%d"
>   # Warning: Verbose
>   ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet) "IDEState: %p; limit=0x%x packet: %s"
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index 74efe8a..db9fde0 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -14,7 +14,6 @@
>   #include "block/scsi.h"
>   
>   /* debug IDE devices */
> -//#define DEBUG_AIO
>   #define USE_DMA_CDROM
>   
>   typedef struct IDEBus IDEBus;
> @@ -333,12 +332,15 @@ struct unreported_events {
>   };
>   
>   enum ide_dma_cmd {
> -    IDE_DMA_READ,
> +    IDE_DMA_READ = 0,
>       IDE_DMA_WRITE,
>       IDE_DMA_TRIM,
>       IDE_DMA_ATAPI,
> +    IDE_DMA__COUNT
>   };
>   
> +extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];
> +
>   #define ide_cmd_is_read(s) \
>   	((s)->dma_cmd == IDE_DMA_READ)
>   
>
Re: [Qemu-devel] [PATCH v3 5/9] IDE: replace DEBUG_AIO with trace events
Posted by John Snow 8 years, 5 months ago 

On 08/31/2017 08:27 PM, Philippe Mathieu-Daudé wrote:
> On 08/31/2017 09:14 PM, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 

TY, and btw the reasoning behind what I went with:

Since s->dma_cmd is itself a type `enum ide_dma_cmd` then by definition
s->dma_cmd (even if corrupted or wrong) must fit inside the width of
that type. Performing the range checking in the getter is adequate for
protecting the table in this case, if I understand Laszlo's rationales
correctly.

In case case of the AHCI table, I do not use a getter since the IRQBIT
property is passed in statically for each instance and shouldn't have a
chance to get out-of-spec. (If it does, something MASSIVELY bad has
corrupted everything, and then we've got worse problems.)

Thanks,
John

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.