[Qemu-devel] [PATCH 00/16] AHCI: tracing improvements

[Qemu-devel] [PATCH 00/16] AHCI: tracing improvements

[John Snow]

There's a bug I'm hunting down in AHCI at the moment,
so I made the AHCI tracing a little nicer again.

The bug is still at large, but no reason to hold up
tracing improvements.

In general, this set just adds register names so that
the read/write traces make more sense on their own without
having to memorize register offsets. It also splits read/write
traces into supported/unsupported subsets, so you can just
monitor for things that QEMU is likely doing wrong.

If nobody yells at me within, say, two weeks I'll just
merge these as a large trivial set.

John Snow (16):
  ahci: add port register enumeration
  ahci: modify ahci_port_read to use register numbers
  ahci: make port read traces more descriptive
  ahci: fix spacing damage on ahci_port_write
  ahci: combine identical clauses in port write
  ahci: modify ahci_port_write to use register numbers
  ahci: make port write traces more descriptive
  ahci: delete old port register address definitions
  ahci: add host register enumeration
  ahci: fix host register max address
  ahci: modify ahci_mem_read_32 to work on register numbers
  ahci: make mem_read_32 traces more descriptive
  ahci: fix spacing damage on ahci_mem_write
  ahci: adjust ahci_mem_write to work on registers
  ahci: delete old host register address definitions
  ahci: make ahci_mem_write traces more descriptive

 hw/ide/ahci.c          | 304 +++++++++++++++++++++++++++++--------------------
 hw/ide/ahci_internal.h |  63 ++++++----
 hw/ide/trace-events    |  13 ++-
 3 files changed, 231 insertions(+), 149 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 01/16] ahci: add port register enumeration

[John Snow]

Instead of tracking offsets, lets count the registers.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c          | 25 +++++++++++++++++++++++++
 hw/ide/ahci_internal.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index e22d7be05f..48130c6439 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -46,6 +46,31 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
 static void ahci_unmap_clb_address(AHCIDevice *ad);
 static void ahci_unmap_fis_address(AHCIDevice *ad);
 
+__attribute__((__unused__)) /* TODO */
+static const char *AHCIPortReg_lookup[AHCI_PORT_REG__COUNT] = {
+    [AHCI_PORT_REG_LST_ADDR]    = "PxCLB",
+    [AHCI_PORT_REG_LST_ADDR_HI] = "PxCLBU",
+    [AHCI_PORT_REG_FIS_ADDR]    = "PxFB",
+    [AHCI_PORT_REG_FIS_ADDR_HI] = "PxFBU",
+    [AHCI_PORT_REG_IRQ_STAT]    = "PxIS",
+    [AHCI_PORT_REG_IRQ_MASK]    = "PXIE",
+    [AHCI_PORT_REG_CMD]         = "PxCMD",
+    [7]                         = "Reserved",
+    [AHCI_PORT_REG_TFDATA]      = "PxTFD",
+    [AHCI_PORT_REG_SIG]         = "PxSIG",
+    [AHCI_PORT_REG_SCR_STAT]    = "PxSSTS",
+    [AHCI_PORT_REG_SCR_CTL]     = "PxSCTL",
+    [AHCI_PORT_REG_SCR_ERR]     = "PxSERR",
+    [AHCI_PORT_REG_SCR_ACT]     = "PxSACT",
+    [AHCI_PORT_REG_CMD_ISSUE]   = "PxCI",
+    [AHCI_PORT_REG_SCR_NOTIF]   = "PxSNTF",
+    [AHCI_PORT_REG_FIS_CTL]     = "PxFBS",
+    [AHCI_PORT_REG_DEV_SLEEP]   = "PxDEVSLP",
+    [18 ... 27]                 = "Reserved",
+    [AHCI_PORT_REG_VENDOR_1 ...
+     AHCI_PORT_REG_VENDOR_4]    = "PxVS",
+};
+
 static const char *AHCIPortIRQ_lookup[AHCI_PORT_IRQ__COUNT] = {
     [AHCI_PORT_IRQ_BIT_DHRS] = "DHRS",
     [AHCI_PORT_IRQ_BIT_PSS]  = "PSS",
diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index 1a25d6c039..eb7e1eefc0 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -74,6 +74,34 @@
 #define HOST_CAP_NCQ              (1 << 30) /* Native Command Queueing */
 #define HOST_CAP_64               (1U << 31) /* PCI DAC (64-bit DMA) support */
 
+/* registers for each SATA port */
+enum AHCIPortReg {
+    AHCI_PORT_REG_LST_ADDR    = 0, /* PxCLB: command list DMA addr */
+    AHCI_PORT_REG_LST_ADDR_HI = 1, /* PxCLBU: command list DMA addr hi */
+    AHCI_PORT_REG_FIS_ADDR    = 2, /* PxFB: FIS rx buf addr */
+    AHCI_PORT_REG_FIS_ADDR_HI = 3, /* PxFBU: FIX rx buf addr hi */
+    AHCI_PORT_REG_IRQ_STAT    = 4, /* PxIS: interrupt status */
+    AHCI_PORT_REG_IRQ_MASK    = 5, /* PxIE: interrupt enable/disable mask */
+    AHCI_PORT_REG_CMD         = 6, /* PxCMD: port command */
+    /* RESERVED */
+    AHCI_PORT_REG_TFDATA      = 8, /* PxTFD: taskfile data */
+    AHCI_PORT_REG_SIG         = 9, /* PxSIG: device TF signature */
+    AHCI_PORT_REG_SCR_STAT    = 10, /* PxSSTS: SATA phy register: SStatus */
+    AHCI_PORT_REG_SCR_CTL     = 11, /* PxSCTL: SATA phy register: SControl */
+    AHCI_PORT_REG_SCR_ERR     = 12, /* PxSERR: SATA phy register: SError */
+    AHCI_PORT_REG_SCR_ACT     = 13, /* PxSACT: SATA phy register: SActive */
+    AHCI_PORT_REG_CMD_ISSUE   = 14, /* PxCI: command issue */
+    AHCI_PORT_REG_SCR_NOTIF   = 15, /* PxSNTF: SATA phy register: SNotification */
+    AHCI_PORT_REG_FIS_CTL     = 16, /* PxFBS: Port multiplier switching ctl */
+    AHCI_PORT_REG_DEV_SLEEP   = 17, /* PxDEVSLP: device sleep control */
+    /* RESERVED */
+    AHCI_PORT_REG_VENDOR_1    = 28, /* PxVS: Vendor Specific */
+    AHCI_PORT_REG_VENDOR_2    = 29,
+    AHCI_PORT_REG_VENDOR_3    = 30,
+    AHCI_PORT_REG_VENDOR_4    = 31,
+    AHCI_PORT_REG__COUNT      = 32
+};
+
 /* registers for each SATA port */
 #define PORT_LST_ADDR             0x00 /* command list DMA addr */
 #define PORT_LST_ADDR_HI          0x04 /* command list DMA addr hi */
@@ -82,6 +110,7 @@
 #define PORT_IRQ_STAT             0x10 /* interrupt status */
 #define PORT_IRQ_MASK             0x14 /* interrupt enable/disable mask */
 #define PORT_CMD                  0x18 /* port command */
+
 #define PORT_TFDATA               0x20 /* taskfile data */
 #define PORT_SIG                  0x24 /* device TF signature */
 #define PORT_SCR_STAT             0x28 /* SATA phy register: SStatus */
-- 
2.14.3

[Qemu-devel] [PATCH 02/16] ahci: modify ahci_port_read to use register numbers

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 48130c6439..5187bf34ad 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -93,41 +93,42 @@ static const char *AHCIPortIRQ_lookup[AHCI_PORT_IRQ__COUNT] = {
     [AHCI_PORT_IRQ_BIT_CPDS] = "CPDS"
 };
 
-static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
+static uint32_t ahci_port_read(AHCIState *s, int port, int offset)
 {
     uint32_t val;
-    AHCIPortRegs *pr;
-    pr = &s->dev[port].port_regs;
+    AHCIPortRegs *pr = &s->dev[port].port_regs;
+    enum AHCIPortReg regnum = offset / sizeof(uint32_t);
+    assert(regnum < (AHCI_PORT_ADDR_OFFSET_LEN / sizeof(uint32_t)));
 
-    switch (offset) {
-    case PORT_LST_ADDR:
+    switch (regnum) {
+    case AHCI_PORT_REG_LST_ADDR:
         val = pr->lst_addr;
         break;
-    case PORT_LST_ADDR_HI:
+    case AHCI_PORT_REG_LST_ADDR_HI:
         val = pr->lst_addr_hi;
         break;
-    case PORT_FIS_ADDR:
+    case AHCI_PORT_REG_FIS_ADDR:
         val = pr->fis_addr;
         break;
-    case PORT_FIS_ADDR_HI:
+    case AHCI_PORT_REG_FIS_ADDR_HI:
         val = pr->fis_addr_hi;
         break;
-    case PORT_IRQ_STAT:
+    case AHCI_PORT_REG_IRQ_STAT:
         val = pr->irq_stat;
         break;
-    case PORT_IRQ_MASK:
+    case AHCI_PORT_REG_IRQ_MASK:
         val = pr->irq_mask;
         break;
-    case PORT_CMD:
+    case AHCI_PORT_REG_CMD:
         val = pr->cmd;
         break;
-    case PORT_TFDATA:
+    case AHCI_PORT_REG_TFDATA:
         val = pr->tfdata;
         break;
-    case PORT_SIG:
+    case AHCI_PORT_REG_SIG:
         val = pr->sig;
         break;
-    case PORT_SCR_STAT:
+    case AHCI_PORT_REG_SCR_STAT:
         if (s->dev[port].port.ifs[0].blk) {
             val = SATA_SCR_SSTATUS_DET_DEV_PRESENT_PHY_UP |
                   SATA_SCR_SSTATUS_SPD_GEN1 | SATA_SCR_SSTATUS_IPM_ACTIVE;
@@ -135,19 +136,18 @@ static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
             val = SATA_SCR_SSTATUS_DET_NODEV;
         }
         break;
-    case PORT_SCR_CTL:
+    case AHCI_PORT_REG_SCR_CTL:
         val = pr->scr_ctl;
         break;
-    case PORT_SCR_ERR:
+    case AHCI_PORT_REG_SCR_ERR:
         val = pr->scr_err;
         break;
-    case PORT_SCR_ACT:
+    case AHCI_PORT_REG_SCR_ACT:
         val = pr->scr_act;
         break;
-    case PORT_CMD_ISSUE:
+    case AHCI_PORT_REG_CMD_ISSUE:
         val = pr->cmd_issue;
         break;
-    case PORT_RESERVED:
     default:
         val = 0;
     }
-- 
2.14.3

[Qemu-devel] [PATCH 03/16] ahci: make port read traces more descriptive

[John Snow]

A trace is added to let us watch unimplemented registers specifically,
as these are more likely to cause us trouble. Otherwise, the port read
traces now tell us what register is getting hit, which is nicer.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c       | 5 +++--
 hw/ide/trace-events | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 5187bf34ad..57c80a2fe9 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -46,7 +46,6 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
 static void ahci_unmap_clb_address(AHCIDevice *ad);
 static void ahci_unmap_fis_address(AHCIDevice *ad);
 
-__attribute__((__unused__)) /* TODO */
 static const char *AHCIPortReg_lookup[AHCI_PORT_REG__COUNT] = {
     [AHCI_PORT_REG_LST_ADDR]    = "PxCLB",
     [AHCI_PORT_REG_LST_ADDR_HI] = "PxCLBU",
@@ -149,10 +148,12 @@ static uint32_t ahci_port_read(AHCIState *s, int port, int offset)
         val = pr->cmd_issue;
         break;
     default:
+        trace_ahci_port_read_unimpl(s, port, AHCIPortReg_lookup[regnum],
+                                    offset);
         val = 0;
     }
 
-    trace_ahci_port_read(s, port, offset, val);
+    trace_ahci_port_read(s, port, AHCIPortReg_lookup[regnum], offset, val);
     return val;
 }
 
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index 5c0e59ec42..ffde28615f 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -63,7 +63,8 @@ ide_atapi_cmd_read_dma_cb_aio(void *s, int lba, int n) "IDEState: %p; aio read:
 ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet) "IDEState: %p; limit=0x%x packet: %s"
 
 # hw/ide/ahci.c
-ahci_port_read(void *s, int port, int offset, uint32_t ret) "ahci(%p)[%d]: port read @ 0x%x: 0x%08x"
+ahci_port_read(void *s, int port, const char *reg, int offset, uint32_t ret) "ahci(%p)[%d]: port read [reg:%s] @ 0x%x: 0x%08x"
+ahci_port_read_unimpl(void *s, int port, const char *reg, int offset) "ahci(%p)[%d]: unimplemented port read [reg:%s] @ 0x%x"
 ahci_irq_raise(void *s) "ahci(%p): raise irq"
 ahci_irq_lower(void *s) "ahci(%p): lower irq"
 ahci_check_irq(void *s, uint32_t old, uint32_t new) "ahci(%p): check irq 0x%08x --> 0x%08x"
-- 
2.14.3

Re: [Qemu-devel] [PATCH 03/16] ahci: make port read traces more descriptive

[Philippe Mathieu-Daudé]

Hi John,

On 05/25/2018 08:54 PM, John Snow wrote:
> A trace is added to let us watch unimplemented registers specifically,
> as these are more likely to cause us trouble. Otherwise, the port read
> traces now tell us what register is getting hit, which is nicer.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/ahci.c       | 5 +++--
>  hw/ide/trace-events | 3 ++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 5187bf34ad..57c80a2fe9 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -46,7 +46,6 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
>  static void ahci_unmap_clb_address(AHCIDevice *ad);
>  static void ahci_unmap_fis_address(AHCIDevice *ad);
>  
> -__attribute__((__unused__)) /* TODO */
>  static const char *AHCIPortReg_lookup[AHCI_PORT_REG__COUNT] = {
>      [AHCI_PORT_REG_LST_ADDR]    = "PxCLB",
>      [AHCI_PORT_REG_LST_ADDR_HI] = "PxCLBU",
> @@ -149,10 +148,12 @@ static uint32_t ahci_port_read(AHCIState *s, int port, int offset)
>          val = pr->cmd_issue;
>          break;
>      default:
> +        trace_ahci_port_read_unimpl(s, port, AHCIPortReg_lookup[regnum],
> +                                    offset);

I personally prefer qemu_log_mask(LOG_UNIMP) here.

Rational is when trying firmware, one might want to know which
unimplemented features access the firmware, without having to dig into
trace events, and the "-d unimp" command line option just works.

(same apply for the write() function later in this series).

>          val = 0;
>      }
>  
> -    trace_ahci_port_read(s, port, offset, val);
> +    trace_ahci_port_read(s, port, AHCIPortReg_lookup[regnum], offset, val);
>      return val;
>  }
>  
> diff --git a/hw/ide/trace-events b/hw/ide/trace-events
> index 5c0e59ec42..ffde28615f 100644
> --- a/hw/ide/trace-events
> +++ b/hw/ide/trace-events
> @@ -63,7 +63,8 @@ ide_atapi_cmd_read_dma_cb_aio(void *s, int lba, int n) "IDEState: %p; aio read:
>  ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet) "IDEState: %p; limit=0x%x packet: %s"
>  
>  # hw/ide/ahci.c
> -ahci_port_read(void *s, int port, int offset, uint32_t ret) "ahci(%p)[%d]: port read @ 0x%x: 0x%08x"
> +ahci_port_read(void *s, int port, const char *reg, int offset, uint32_t ret) "ahci(%p)[%d]: port read [reg:%s] @ 0x%x: 0x%08x"
> +ahci_port_read_unimpl(void *s, int port, const char *reg, int offset) "ahci(%p)[%d]: unimplemented port read [reg:%s] @ 0x%x"
>  ahci_irq_raise(void *s) "ahci(%p): raise irq"
>  ahci_irq_lower(void *s) "ahci(%p): lower irq"
>  ahci_check_irq(void *s, uint32_t old, uint32_t new) "ahci(%p): check irq 0x%08x --> 0x%08x"
> 

Re: [Qemu-devel] [PATCH 03/16] ahci: make port read traces more descriptive

[John Snow]

On 05/26/2018 12:44 AM, Philippe Mathieu-Daudé wrote:
> Hi John,
> 
> On 05/25/2018 08:54 PM, John Snow wrote:
>> A trace is added to let us watch unimplemented registers specifically,
>> as these are more likely to cause us trouble. Otherwise, the port read
>> traces now tell us what register is getting hit, which is nicer.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  hw/ide/ahci.c       | 5 +++--
>>  hw/ide/trace-events | 3 ++-
>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 5187bf34ad..57c80a2fe9 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -46,7 +46,6 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
>>  static void ahci_unmap_clb_address(AHCIDevice *ad);
>>  static void ahci_unmap_fis_address(AHCIDevice *ad);
>>  
>> -__attribute__((__unused__)) /* TODO */
>>  static const char *AHCIPortReg_lookup[AHCI_PORT_REG__COUNT] = {
>>      [AHCI_PORT_REG_LST_ADDR]    = "PxCLB",
>>      [AHCI_PORT_REG_LST_ADDR_HI] = "PxCLBU",
>> @@ -149,10 +148,12 @@ static uint32_t ahci_port_read(AHCIState *s, int port, int offset)
>>          val = pr->cmd_issue;
>>          break;
>>      default:
>> +        trace_ahci_port_read_unimpl(s, port, AHCIPortReg_lookup[regnum],
>> +                                    offset);
> 
> I personally prefer qemu_log_mask(LOG_UNIMP) here.
> 
> Rational is when trying firmware, one might want to know which
> unimplemented features access the firmware, without having to dig into
> trace events, and the "-d unimp" command line option just works.
> 

For reads, it's ambiguous. Some of the registers we've specifically not
implemented because if they are unsupported they are supposed to return
0, which we do here. The default behavior is generally correct.

In this case, the trace really is just kind of an informative /
debugging statement we probably aren't too interested in unless we're
diagnosing some AHCI problem specifically -- and I don't want to turn on
all unimp messages to see these.

> (same apply for the write() function later in this series).
> 

For writes it's actually definitely unhandled and I might actually
prefer both the trace and the log -- if we support "-d unimp" on the
command line to hunt for known stubs getting probed, this is certainly
one of those places we want to know about.

--js

Re: [Qemu-devel] [PATCH 03/16] ahci: make port read traces more descriptive

[Philippe Mathieu-Daudé]

On 05/30/2018 05:17 PM, John Snow wrote:
> On 05/26/2018 12:44 AM, Philippe Mathieu-Daudé wrote:
>> Hi John,
>>
>> On 05/25/2018 08:54 PM, John Snow wrote:
>>> A trace is added to let us watch unimplemented registers specifically,
>>> as these are more likely to cause us trouble. Otherwise, the port read
>>> traces now tell us what register is getting hit, which is nicer.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  hw/ide/ahci.c       | 5 +++--
>>>  hw/ide/trace-events | 3 ++-
>>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>>> index 5187bf34ad..57c80a2fe9 100644
>>> --- a/hw/ide/ahci.c
>>> +++ b/hw/ide/ahci.c
>>> @@ -46,7 +46,6 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
>>>  static void ahci_unmap_clb_address(AHCIDevice *ad);
>>>  static void ahci_unmap_fis_address(AHCIDevice *ad);
>>>  
>>> -__attribute__((__unused__)) /* TODO */
>>>  static const char *AHCIPortReg_lookup[AHCI_PORT_REG__COUNT] = {
>>>      [AHCI_PORT_REG_LST_ADDR]    = "PxCLB",
>>>      [AHCI_PORT_REG_LST_ADDR_HI] = "PxCLBU",
>>> @@ -149,10 +148,12 @@ static uint32_t ahci_port_read(AHCIState *s, int port, int offset)
>>>          val = pr->cmd_issue;
>>>          break;
>>>      default:
>>> +        trace_ahci_port_read_unimpl(s, port, AHCIPortReg_lookup[regnum],
>>> +                                    offset);
>>
>> I personally prefer qemu_log_mask(LOG_UNIMP) here.
>>
>> Rational is when trying firmware, one might want to know which
>> unimplemented features access the firmware, without having to dig into
>> trace events, and the "-d unimp" command line option just works.
>>
> 
> For reads, it's ambiguous. Some of the registers we've specifically not
> implemented because if they are unsupported they are supposed to return
> 0, which we do here. The default behavior is generally correct.
> 
> In this case, the trace really is just kind of an informative /
> debugging statement we probably aren't too interested in unless we're
> diagnosing some AHCI problem specifically -- and I don't want to turn on
> all unimp messages to see these.
> 
>> (same apply for the write() function later in this series).
>>
> 
> For writes it's actually definitely unhandled and I might actually
> prefer both the trace and the log -- if we support "-d unimp" on the
> command line to hunt for known stubs getting probed, this is certainly
> one of those places we want to know about.

I agree to "both the trace and the log".

Whether or not you add qemu_log_mask(LOG_UNIMP) beyond the traces:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [PATCH 03/16] ahci: make port read traces more descriptive

[John Snow]

On 05/30/2018 05:28 PM, Philippe Mathieu-Daudé wrote:
> On 05/30/2018 05:17 PM, John Snow wrote:
>> On 05/26/2018 12:44 AM, Philippe Mathieu-Daudé wrote:
>>> Hi John,
>>>
>>> On 05/25/2018 08:54 PM, John Snow wrote:
>>>> A trace is added to let us watch unimplemented registers specifically,
>>>> as these are more likely to cause us trouble. Otherwise, the port read
>>>> traces now tell us what register is getting hit, which is nicer.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>  hw/ide/ahci.c       | 5 +++--
>>>>  hw/ide/trace-events | 3 ++-
>>>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>>>> index 5187bf34ad..57c80a2fe9 100644
>>>> --- a/hw/ide/ahci.c
>>>> +++ b/hw/ide/ahci.c
>>>> @@ -46,7 +46,6 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
>>>>  static void ahci_unmap_clb_address(AHCIDevice *ad);
>>>>  static void ahci_unmap_fis_address(AHCIDevice *ad);
>>>>  
>>>> -__attribute__((__unused__)) /* TODO */
>>>>  static const char *AHCIPortReg_lookup[AHCI_PORT_REG__COUNT] = {
>>>>      [AHCI_PORT_REG_LST_ADDR]    = "PxCLB",
>>>>      [AHCI_PORT_REG_LST_ADDR_HI] = "PxCLBU",
>>>> @@ -149,10 +148,12 @@ static uint32_t ahci_port_read(AHCIState *s, int port, int offset)
>>>>          val = pr->cmd_issue;
>>>>          break;
>>>>      default:
>>>> +        trace_ahci_port_read_unimpl(s, port, AHCIPortReg_lookup[regnum],
>>>> +                                    offset);
>>>
>>> I personally prefer qemu_log_mask(LOG_UNIMP) here.
>>>
>>> Rational is when trying firmware, one might want to know which
>>> unimplemented features access the firmware, without having to dig into
>>> trace events, and the "-d unimp" command line option just works.
>>>
>>
>> For reads, it's ambiguous. Some of the registers we've specifically not
>> implemented because if they are unsupported they are supposed to return
>> 0, which we do here. The default behavior is generally correct.
>>
>> In this case, the trace really is just kind of an informative /
>> debugging statement we probably aren't too interested in unless we're
>> diagnosing some AHCI problem specifically -- and I don't want to turn on
>> all unimp messages to see these.
>>
>>> (same apply for the write() function later in this series).
>>>
>>
>> For writes it's actually definitely unhandled and I might actually
>> prefer both the trace and the log -- if we support "-d unimp" on the
>> command line to hunt for known stubs getting probed, this is certainly
>> one of those places we want to know about.
> 
> I agree to "both the trace and the log".
> 
> Whether or not you add qemu_log_mask(LOG_UNIMP) beyond the traces:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 

1. Do you intend to R-B the whole series, or just this patch?
2. Is it your intention that I add the log to the read trace as well? My
inclination is to only add it to the write.

--js

[Qemu-devel] [PATCH 04/16] ahci: fix spacing damage on ahci_port_write

[John Snow]

Churn.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 142 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 71 insertions(+), 71 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 57c80a2fe9..2298e72833 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -279,85 +279,85 @@ static int ahci_cond_start_engines(AHCIDevice *ad)
     return 0;
 }
 
-static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
+static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
 {
     AHCIPortRegs *pr = &s->dev[port].port_regs;
 
     trace_ahci_port_write(s, port, offset, val);
     switch (offset) {
-        case PORT_LST_ADDR:
-            pr->lst_addr = val;
-            break;
-        case PORT_LST_ADDR_HI:
-            pr->lst_addr_hi = val;
-            break;
-        case PORT_FIS_ADDR:
-            pr->fis_addr = val;
-            break;
-        case PORT_FIS_ADDR_HI:
-            pr->fis_addr_hi = val;
-            break;
-        case PORT_IRQ_STAT:
-            pr->irq_stat &= ~val;
-            ahci_check_irq(s);
-            break;
-        case PORT_IRQ_MASK:
-            pr->irq_mask = val & 0xfdc000ff;
-            ahci_check_irq(s);
-            break;
-        case PORT_CMD:
-            /* Block any Read-only fields from being set;
-             * including LIST_ON and FIS_ON.
-             * The spec requires to set ICC bits to zero after the ICC change
-             * is done. We don't support ICC state changes, therefore always
-             * force the ICC bits to zero.
-             */
-            pr->cmd = (pr->cmd & PORT_CMD_RO_MASK) |
-                      (val & ~(PORT_CMD_RO_MASK|PORT_CMD_ICC_MASK));
+    case PORT_LST_ADDR:
+        pr->lst_addr = val;
+        break;
+    case PORT_LST_ADDR_HI:
+        pr->lst_addr_hi = val;
+        break;
+    case PORT_FIS_ADDR:
+        pr->fis_addr = val;
+        break;
+    case PORT_FIS_ADDR_HI:
+        pr->fis_addr_hi = val;
+        break;
+    case PORT_IRQ_STAT:
+        pr->irq_stat &= ~val;
+        ahci_check_irq(s);
+        break;
+    case PORT_IRQ_MASK:
+        pr->irq_mask = val & 0xfdc000ff;
+        ahci_check_irq(s);
+        break;
+    case PORT_CMD:
+        /* Block any Read-only fields from being set;
+         * including LIST_ON and FIS_ON.
+         * The spec requires to set ICC bits to zero after the ICC change
+         * is done. We don't support ICC state changes, therefore always
+         * force the ICC bits to zero.
+         */
+        pr->cmd = (pr->cmd & PORT_CMD_RO_MASK) |
+            (val & ~(PORT_CMD_RO_MASK|PORT_CMD_ICC_MASK));
 
-            /* Check FIS RX and CLB engines */
-            ahci_cond_start_engines(&s->dev[port]);
+        /* Check FIS RX and CLB engines */
+        ahci_cond_start_engines(&s->dev[port]);
 
-            /* XXX usually the FIS would be pending on the bus here and
-                   issuing deferred until the OS enables FIS receival.
-                   Instead, we only submit it once - which works in most
-                   cases, but is a hack. */
-            if ((pr->cmd & PORT_CMD_FIS_ON) &&
-                !s->dev[port].init_d2h_sent) {
-                ahci_init_d2h(&s->dev[port]);
-            }
+        /* XXX usually the FIS would be pending on the bus here and
+           issuing deferred until the OS enables FIS receival.
+           Instead, we only submit it once - which works in most
+           cases, but is a hack. */
+        if ((pr->cmd & PORT_CMD_FIS_ON) &&
+            !s->dev[port].init_d2h_sent) {
+            ahci_init_d2h(&s->dev[port]);
+        }
 
-            check_cmd(s, port);
-            break;
-        case PORT_TFDATA:
-            /* Read Only. */
-            break;
-        case PORT_SIG:
-            /* Read Only */
-            break;
-        case PORT_SCR_STAT:
-            /* Read Only */
-            break;
-        case PORT_SCR_CTL:
-            if (((pr->scr_ctl & AHCI_SCR_SCTL_DET) == 1) &&
-                ((val & AHCI_SCR_SCTL_DET) == 0)) {
-                ahci_reset_port(s, port);
-            }
-            pr->scr_ctl = val;
-            break;
-        case PORT_SCR_ERR:
-            pr->scr_err &= ~val;
-            break;
-        case PORT_SCR_ACT:
-            /* RW1 */
-            pr->scr_act |= val;
-            break;
-        case PORT_CMD_ISSUE:
-            pr->cmd_issue |= val;
-            check_cmd(s, port);
-            break;
-        default:
-            break;
+        check_cmd(s, port);
+        break;
+    case PORT_TFDATA:
+        /* Read Only. */
+        break;
+    case PORT_SIG:
+        /* Read Only */
+        break;
+    case PORT_SCR_STAT:
+        /* Read Only */
+        break;
+    case PORT_SCR_CTL:
+        if (((pr->scr_ctl & AHCI_SCR_SCTL_DET) == 1) &&
+            ((val & AHCI_SCR_SCTL_DET) == 0)) {
+            ahci_reset_port(s, port);
+        }
+        pr->scr_ctl = val;
+        break;
+    case PORT_SCR_ERR:
+        pr->scr_err &= ~val;
+        break;
+    case PORT_SCR_ACT:
+        /* RW1 */
+        pr->scr_act |= val;
+        break;
+    case PORT_CMD_ISSUE:
+        pr->cmd_issue |= val;
+        check_cmd(s, port);
+        break;
+    default:
+        break;
     }
 }
 
-- 
2.14.3

[Qemu-devel] [PATCH 05/16] ahci: combine identical clauses in port write

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 2298e72833..e9ca820d5e 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -330,11 +330,7 @@ static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
         check_cmd(s, port);
         break;
     case PORT_TFDATA:
-        /* Read Only. */
-        break;
     case PORT_SIG:
-        /* Read Only */
-        break;
     case PORT_SCR_STAT:
         /* Read Only */
         break;
-- 
2.14.3

[Qemu-devel] [PATCH 06/16] ahci: modify ahci_port_write to use register numbers

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index e9ca820d5e..1d93cd6806 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -282,30 +282,32 @@ static int ahci_cond_start_engines(AHCIDevice *ad)
 static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
 {
     AHCIPortRegs *pr = &s->dev[port].port_regs;
+    enum AHCIPortReg regnum = offset / sizeof(uint32_t);
+    assert(regnum < (AHCI_PORT_ADDR_OFFSET_LEN / sizeof(uint32_t)));
 
     trace_ahci_port_write(s, port, offset, val);
-    switch (offset) {
-    case PORT_LST_ADDR:
+    switch (regnum) {
+    case AHCI_PORT_REG_LST_ADDR:
         pr->lst_addr = val;
         break;
-    case PORT_LST_ADDR_HI:
+    case AHCI_PORT_REG_LST_ADDR_HI:
         pr->lst_addr_hi = val;
         break;
-    case PORT_FIS_ADDR:
+    case AHCI_PORT_REG_FIS_ADDR:
         pr->fis_addr = val;
         break;
-    case PORT_FIS_ADDR_HI:
+    case AHCI_PORT_REG_FIS_ADDR_HI:
         pr->fis_addr_hi = val;
         break;
-    case PORT_IRQ_STAT:
+    case AHCI_PORT_REG_IRQ_STAT:
         pr->irq_stat &= ~val;
         ahci_check_irq(s);
         break;
-    case PORT_IRQ_MASK:
+    case AHCI_PORT_REG_IRQ_MASK:
         pr->irq_mask = val & 0xfdc000ff;
         ahci_check_irq(s);
         break;
-    case PORT_CMD:
+    case AHCI_PORT_REG_CMD:
         /* Block any Read-only fields from being set;
          * including LIST_ON and FIS_ON.
          * The spec requires to set ICC bits to zero after the ICC change
@@ -329,26 +331,26 @@ static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
 
         check_cmd(s, port);
         break;
-    case PORT_TFDATA:
-    case PORT_SIG:
-    case PORT_SCR_STAT:
+    case AHCI_PORT_REG_TFDATA:
+    case AHCI_PORT_REG_SIG:
+    case AHCI_PORT_REG_SCR_STAT:
         /* Read Only */
         break;
-    case PORT_SCR_CTL:
+    case AHCI_PORT_REG_SCR_CTL:
         if (((pr->scr_ctl & AHCI_SCR_SCTL_DET) == 1) &&
             ((val & AHCI_SCR_SCTL_DET) == 0)) {
             ahci_reset_port(s, port);
         }
         pr->scr_ctl = val;
         break;
-    case PORT_SCR_ERR:
+    case AHCI_PORT_REG_SCR_ERR:
         pr->scr_err &= ~val;
         break;
-    case PORT_SCR_ACT:
+    case AHCI_PORT_REG_SCR_ACT:
         /* RW1 */
         pr->scr_act |= val;
         break;
-    case PORT_CMD_ISSUE:
+    case AHCI_PORT_REG_CMD_ISSUE:
         pr->cmd_issue |= val;
         check_cmd(s, port);
         break;
-- 
2.14.3

[Qemu-devel] [PATCH 07/16] ahci: make port write traces more descriptive

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c       | 4 +++-
 hw/ide/trace-events | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 1d93cd6806..61ac8e46c8 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -284,8 +284,8 @@ static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
     AHCIPortRegs *pr = &s->dev[port].port_regs;
     enum AHCIPortReg regnum = offset / sizeof(uint32_t);
     assert(regnum < (AHCI_PORT_ADDR_OFFSET_LEN / sizeof(uint32_t)));
+    trace_ahci_port_write(s, port, AHCIPortReg_lookup[regnum], offset, val);
 
-    trace_ahci_port_write(s, port, offset, val);
     switch (regnum) {
     case AHCI_PORT_REG_LST_ADDR:
         pr->lst_addr = val;
@@ -355,6 +355,8 @@ static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
         check_cmd(s, port);
         break;
     default:
+        trace_ahci_port_write_unimpl(s, port, AHCIPortReg_lookup[regnum],
+                                     offset, val);
         break;
     }
 }
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index ffde28615f..458e4a3e80 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -69,7 +69,8 @@ ahci_irq_raise(void *s) "ahci(%p): raise irq"
 ahci_irq_lower(void *s) "ahci(%p): lower irq"
 ahci_check_irq(void *s, uint32_t old, uint32_t new) "ahci(%p): check irq 0x%08x --> 0x%08x"
 ahci_trigger_irq(void *s, int port, const char *name, uint32_t val, uint32_t old, uint32_t new, uint32_t effective) "ahci(%p)[%d]: trigger irq +%s (0x%08x); irqstat: 0x%08x --> 0x%08x; effective: 0x%08x"
-ahci_port_write(void *s, int port, int offset, uint32_t val) "ahci(%p)[%d]: port write @ 0x%x: 0x%08x"
+ahci_port_write(void *s, int port, const char *reg, int offset, uint32_t val) "ahci(%p)[%d]: port write [reg:%s] @ 0x%x: 0x%08x"
+ahci_port_write_unimpl(void *s, int port, const char *reg, int offset, uint32_t val) "ahci(%p)[%d]: unimplemented port write [reg:%s] @ 0x%x: 0x%08x"
 ahci_mem_read_32(void *s, uint64_t addr, uint32_t val) "ahci(%p): mem read @ 0x%"PRIx64": 0x%08x"
 ahci_mem_read(void *s, unsigned size, uint64_t addr, uint64_t val) "ahci(%p): read%u @ 0x%"PRIx64": 0x%016"PRIx64
 ahci_mem_write(void *s, unsigned size, uint64_t addr, uint64_t val) "ahci(%p): write%u @ 0x%"PRIx64": 0x%016"PRIx64
-- 
2.14.3

[Qemu-devel] [PATCH 08/16] ahci: delete old port register address definitions

[John Snow]

They're now unused.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci_internal.h | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index eb7e1eefc0..db00c9aa39 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -102,24 +102,6 @@ enum AHCIPortReg {
     AHCI_PORT_REG__COUNT      = 32
 };
 
-/* registers for each SATA port */
-#define PORT_LST_ADDR             0x00 /* command list DMA addr */
-#define PORT_LST_ADDR_HI          0x04 /* command list DMA addr hi */
-#define PORT_FIS_ADDR             0x08 /* FIS rx buf addr */
-#define PORT_FIS_ADDR_HI          0x0c /* FIS rx buf addr hi */
-#define PORT_IRQ_STAT             0x10 /* interrupt status */
-#define PORT_IRQ_MASK             0x14 /* interrupt enable/disable mask */
-#define PORT_CMD                  0x18 /* port command */
-
-#define PORT_TFDATA               0x20 /* taskfile data */
-#define PORT_SIG                  0x24 /* device TF signature */
-#define PORT_SCR_STAT             0x28 /* SATA phy register: SStatus */
-#define PORT_SCR_CTL              0x2c /* SATA phy register: SControl */
-#define PORT_SCR_ERR              0x30 /* SATA phy register: SError */
-#define PORT_SCR_ACT              0x34 /* SATA phy register: SActive */
-#define PORT_CMD_ISSUE            0x38 /* command issue */
-#define PORT_RESERVED             0x3c /* reserved */
-
 /* Port interrupt bit descriptors */
 enum AHCIPortIRQ {
     AHCI_PORT_IRQ_BIT_DHRS = 0,
-- 
2.14.3

[Qemu-devel] [PATCH 09/16] ahci: add host register enumeration

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c          | 15 +++++++++++++++
 hw/ide/ahci_internal.h | 15 +++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 61ac8e46c8..674e06853e 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -46,6 +46,21 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
 static void ahci_unmap_clb_address(AHCIDevice *ad);
 static void ahci_unmap_fis_address(AHCIDevice *ad);
 
+__attribute__((__unused__)) /* TODO */
+static const char *AHCIHostReg_lookup[AHCI_HOST_REG__COUNT] = {
+    [AHCI_HOST_REG_CAP]        = "CAP",
+    [AHCI_HOST_REG_CTL]        = "GHC",
+    [AHCI_HOST_REG_IRQ_STAT]   = "IS",
+    [AHCI_HOST_REG_PORTS_IMPL] = "PI",
+    [AHCI_HOST_REG_VERSION]    = "VS",
+    [AHCI_HOST_REG_CCC_CTL]    = "CCC_CTL",
+    [AHCI_HOST_REG_CCC_PORTS]  = "CCC_PORTS",
+    [AHCI_HOST_REG_EM_LOC]     = "EM_LOC",
+    [AHCI_HOST_REG_EM_CTL]     = "EM_CTL",
+    [AHCI_HOST_REG_CAP2]       = "CAP2",
+    [AHCI_HOST_REG_BOHC]       = "BOHC",
+};
+
 static const char *AHCIPortReg_lookup[AHCI_PORT_REG__COUNT] = {
     [AHCI_PORT_REG_LST_ADDR]    = "PxCLB",
     [AHCI_PORT_REG_LST_ADDR_HI] = "PxCLBU",
diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index db00c9aa39..af366db6f3 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -61,6 +61,21 @@
 #define HOST_PORTS_IMPL           0x0c /* bitmap of implemented ports */
 #define HOST_VERSION              0x10 /* AHCI spec. version compliancy */
 
+enum AHCIHostReg {
+    AHCI_HOST_REG_CAP        = 0,  /* CAP: host capabilities */
+    AHCI_HOST_REG_CTL        = 1,  /* GHC: global host control */
+    AHCI_HOST_REG_IRQ_STAT   = 2,  /* IS: interrupt status */
+    AHCI_HOST_REG_PORTS_IMPL = 3,  /* PI: bitmap of implemented ports */
+    AHCI_HOST_REG_VERSION    = 4,  /* VS: AHCI spec. version compliancy */
+    AHCI_HOST_REG_CCC_CTL    = 5,  /* CCC_CTL: CCC Control */
+    AHCI_HOST_REG_CCC_PORTS  = 6,  /* CCC_PORTS: CCC Ports */
+    AHCI_HOST_REG_EM_LOC     = 7,  /* EM_LOC: Enclosure Mgmt Location */
+    AHCI_HOST_REG_EM_CTL     = 8,  /* EM_CTL: Enclosure Mgmt Control */
+    AHCI_HOST_REG_CAP2       = 9,  /* CAP2: host capabilities, extended */
+    AHCI_HOST_REG_BOHC       = 10, /* BOHC: firmare/os handoff ctrl & status */
+    AHCI_HOST_REG__COUNT     = 11
+};
+
 /* HOST_CTL bits */
 #define HOST_CTL_RESET            (1 << 0)  /* reset controller; self-clear */
 #define HOST_CTL_IRQ_EN           (1 << 1)  /* global IRQ enable */
-- 
2.14.3

[Qemu-devel] [PATCH 10/16] ahci: fix host register max address

[John Snow]

Yes, comment, it ought to be 0x2C.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci_internal.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index af366db6f3..f9dcf8b6e6 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -224,8 +224,7 @@ enum AHCIPortIRQ {
 #define SATA_SIGNATURE_CDROM               0xeb140101
 #define SATA_SIGNATURE_DISK                0x00000101
 
-#define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20
-                                            /* Shouldn't this be 0x2c? */
+#define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x2c
 
 #define AHCI_PORT_REGS_START_ADDR          0x100
 #define AHCI_PORT_ADDR_OFFSET_MASK         0x7f
-- 
2.14.3

[Qemu-devel] [PATCH 11/16] ahci: modify ahci_mem_read_32 to work on register numbers

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 674e06853e..4bcb613bf9 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -382,22 +382,27 @@ static uint64_t ahci_mem_read_32(void *opaque, hwaddr addr)
     uint32_t val = 0;
 
     if (addr < AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR) {
-        switch (addr) {
-        case HOST_CAP:
+        enum AHCIHostReg regnum = addr / 4;
+        assert(regnum < AHCI_HOST_REG__COUNT);
+
+        switch (regnum) {
+        case AHCI_HOST_REG_CAP:
             val = s->control_regs.cap;
             break;
-        case HOST_CTL:
+        case AHCI_HOST_REG_CTL:
             val = s->control_regs.ghc;
             break;
-        case HOST_IRQ_STAT:
+        case AHCI_HOST_REG_IRQ_STAT:
             val = s->control_regs.irqstatus;
             break;
-        case HOST_PORTS_IMPL:
+        case AHCI_HOST_REG_PORTS_IMPL:
             val = s->control_regs.impl;
             break;
-        case HOST_VERSION:
+        case AHCI_HOST_REG_VERSION:
             val = s->control_regs.version;
             break;
+        default:
+            break;
         }
     } else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
                (addr < (AHCI_PORT_REGS_START_ADDR +
-- 
2.14.3

[Qemu-devel] [PATCH 12/16] ahci: make mem_read_32 traces more descriptive

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c       | 7 +++++--
 hw/ide/trace-events | 3 +++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 4bcb613bf9..ea68950952 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -46,7 +46,6 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
 static void ahci_unmap_clb_address(AHCIDevice *ad);
 static void ahci_unmap_fis_address(AHCIDevice *ad);
 
-__attribute__((__unused__)) /* TODO */
 static const char *AHCIHostReg_lookup[AHCI_HOST_REG__COUNT] = {
     [AHCI_HOST_REG_CAP]        = "CAP",
     [AHCI_HOST_REG_CTL]        = "GHC",
@@ -402,13 +401,17 @@ static uint64_t ahci_mem_read_32(void *opaque, hwaddr addr)
             val = s->control_regs.version;
             break;
         default:
-            break;
+            trace_ahci_mem_read_32_host_unimpl(s, AHCIHostReg_lookup[regnum],
+                                               addr);
         }
+        trace_ahci_mem_read_32_host(s, AHCIHostReg_lookup[regnum], addr, val);
     } else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
                (addr < (AHCI_PORT_REGS_START_ADDR +
                 (s->ports * AHCI_PORT_ADDR_OFFSET_LEN)))) {
         val = ahci_port_read(s, (addr - AHCI_PORT_REGS_START_ADDR) >> 7,
                              addr & AHCI_PORT_ADDR_OFFSET_MASK);
+    } else {
+        trace_ahci_mem_read_32_default(s, addr, val);
     }
 
     trace_ahci_mem_read_32(s, addr, val);
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index 458e4a3e80..5bd8b2b1df 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -72,6 +72,9 @@ ahci_trigger_irq(void *s, int port, const char *name, uint32_t val, uint32_t old
 ahci_port_write(void *s, int port, const char *reg, int offset, uint32_t val) "ahci(%p)[%d]: port write [reg:%s] @ 0x%x: 0x%08x"
 ahci_port_write_unimpl(void *s, int port, const char *reg, int offset, uint32_t val) "ahci(%p)[%d]: unimplemented port write [reg:%s] @ 0x%x: 0x%08x"
 ahci_mem_read_32(void *s, uint64_t addr, uint32_t val) "ahci(%p): mem read @ 0x%"PRIx64": 0x%08x"
+ahci_mem_read_32_default(void *s, uint64_t addr, uint32_t val) "ahci(%p): mem read @ 0x%"PRIx64": 0x%08x"
+ahci_mem_read_32_host(void *s, const char *reg, uint64_t addr, uint32_t val) "ahci(%p): mem read [reg:%s] @ 0x%"PRIx64": 0x%08x"
+ahci_mem_read_32_host_unimpl(void *s, const char *reg, uint64_t addr) "ahci(%p): unimplemented mem read [reg:%s] @ 0x%"PRIx64
 ahci_mem_read(void *s, unsigned size, uint64_t addr, uint64_t val) "ahci(%p): read%u @ 0x%"PRIx64": 0x%016"PRIx64
 ahci_mem_write(void *s, unsigned size, uint64_t addr, uint64_t val) "ahci(%p): write%u @ 0x%"PRIx64": 0x%016"PRIx64
 ahci_mem_write_unknown(void *s, unsigned size, uint64_t addr, uint64_t val) "ahci(%p): write%u to unknown register 0x%"PRIx64": 0x%016"PRIx64
-- 
2.14.3

[Qemu-devel] [PATCH 13/16] ahci: fix spacing damage on ahci_mem_write

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 47 +++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index ea68950952..9225c4559f 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -465,37 +465,36 @@ static void ahci_mem_write(void *opaque, hwaddr addr,
 
     if (addr < AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR) {
         switch (addr) {
-            case HOST_CAP: /* R/WO, RO */
-                /* FIXME handle R/WO */
-                break;
-            case HOST_CTL: /* R/W */
-                if (val & HOST_CTL_RESET) {
-                    ahci_reset(s);
-                } else {
-                    s->control_regs.ghc = (val & 0x3) | HOST_CTL_AHCI_EN;
-                    ahci_check_irq(s);
-                }
-                break;
-            case HOST_IRQ_STAT: /* R/WC, RO */
-                s->control_regs.irqstatus &= ~val;
+        case HOST_CAP: /* R/WO, RO */
+            /* FIXME handle R/WO */
+            break;
+        case HOST_CTL: /* R/W */
+            if (val & HOST_CTL_RESET) {
+                ahci_reset(s);
+            } else {
+                s->control_regs.ghc = (val & 0x3) | HOST_CTL_AHCI_EN;
                 ahci_check_irq(s);
-                break;
-            case HOST_PORTS_IMPL: /* R/WO, RO */
-                /* FIXME handle R/WO */
-                break;
-            case HOST_VERSION: /* RO */
-                /* FIXME report write? */
-                break;
-            default:
-                trace_ahci_mem_write_unknown(s, size, addr, val);
+            }
+            break;
+        case HOST_IRQ_STAT: /* R/WC, RO */
+            s->control_regs.irqstatus &= ~val;
+            ahci_check_irq(s);
+            break;
+        case HOST_PORTS_IMPL: /* R/WO, RO */
+            /* FIXME handle R/WO */
+            break;
+        case HOST_VERSION: /* RO */
+            /* FIXME report write? */
+            break;
+        default:
+            trace_ahci_mem_write_unknown(s, size, addr, val);
         }
     } else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
                (addr < (AHCI_PORT_REGS_START_ADDR +
-                (s->ports * AHCI_PORT_ADDR_OFFSET_LEN)))) {
+                        (s->ports * AHCI_PORT_ADDR_OFFSET_LEN)))) {
         ahci_port_write(s, (addr - AHCI_PORT_REGS_START_ADDR) >> 7,
                         addr & AHCI_PORT_ADDR_OFFSET_MASK, val);
     }
-
 }
 
 static const MemoryRegionOps ahci_mem_ops = {
-- 
2.14.3

[Qemu-devel] [PATCH 14/16] ahci: adjust ahci_mem_write to work on registers

[John Snow]

Actually, this function looks pretty broken, but for now, let's finish
up what this series of commits came here to do.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 9225c4559f..cd834c563e 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -464,11 +464,14 @@ static void ahci_mem_write(void *opaque, hwaddr addr,
     }
 
     if (addr < AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR) {
-        switch (addr) {
-        case HOST_CAP: /* R/WO, RO */
+        enum AHCIHostReg regnum = addr / 4;
+        assert(regnum < AHCI_HOST_REG__COUNT);
+
+        switch (regnum) {
+        case AHCI_HOST_REG_CAP: /* R/WO, RO */
             /* FIXME handle R/WO */
             break;
-        case HOST_CTL: /* R/W */
+        case AHCI_HOST_REG_CTL: /* R/W */
             if (val & HOST_CTL_RESET) {
                 ahci_reset(s);
             } else {
@@ -476,14 +479,14 @@ static void ahci_mem_write(void *opaque, hwaddr addr,
                 ahci_check_irq(s);
             }
             break;
-        case HOST_IRQ_STAT: /* R/WC, RO */
+        case AHCI_HOST_REG_IRQ_STAT: /* R/WC, RO */
             s->control_regs.irqstatus &= ~val;
             ahci_check_irq(s);
             break;
-        case HOST_PORTS_IMPL: /* R/WO, RO */
+        case AHCI_HOST_REG_PORTS_IMPL: /* R/WO, RO */
             /* FIXME handle R/WO */
             break;
-        case HOST_VERSION: /* RO */
+        case AHCI_HOST_REG_VERSION: /* RO */
             /* FIXME report write? */
             break;
         default:
-- 
2.14.3

[Qemu-devel] [PATCH 15/16] ahci: delete old host register address definitions

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci_internal.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index f9dcf8b6e6..2953243929 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -55,12 +55,6 @@
 #define RX_FIS_UNK                0x60 /* offset of Unknown FIS data */
 
 /* global controller registers */
-#define HOST_CAP                  0x00 /* host capabilities */
-#define HOST_CTL                  0x04 /* global host control */
-#define HOST_IRQ_STAT             0x08 /* interrupt status */
-#define HOST_PORTS_IMPL           0x0c /* bitmap of implemented ports */
-#define HOST_VERSION              0x10 /* AHCI spec. version compliancy */
-
 enum AHCIHostReg {
     AHCI_HOST_REG_CAP        = 0,  /* CAP: host capabilities */
     AHCI_HOST_REG_CTL        = 1,  /* GHC: global host control */
-- 
2.14.3

[Qemu-devel] [PATCH 16/16] ahci: make ahci_mem_write traces more descriptive

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c       | 7 ++++++-
 hw/ide/trace-events | 4 +++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index cd834c563e..b74a871a83 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -490,13 +490,18 @@ static void ahci_mem_write(void *opaque, hwaddr addr,
             /* FIXME report write? */
             break;
         default:
-            trace_ahci_mem_write_unknown(s, size, addr, val);
+            trace_ahci_mem_write_host_unimpl(s, size, AHCIHostReg_lookup[regnum],
+                                             addr);
         }
+        trace_ahci_mem_write_host(s, size, AHCIHostReg_lookup[regnum],
+                                     addr, val);
     } else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
                (addr < (AHCI_PORT_REGS_START_ADDR +
                         (s->ports * AHCI_PORT_ADDR_OFFSET_LEN)))) {
         ahci_port_write(s, (addr - AHCI_PORT_REGS_START_ADDR) >> 7,
                         addr & AHCI_PORT_ADDR_OFFSET_MASK, val);
+    } else {
+        trace_ahci_mem_write_unimpl(s, size, addr, val);
     }
 }
 
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index 5bd8b2b1df..ad7195253f 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -77,7 +77,9 @@ ahci_mem_read_32_host(void *s, const char *reg, uint64_t addr, uint32_t val) "ah
 ahci_mem_read_32_host_unimpl(void *s, const char *reg, uint64_t addr) "ahci(%p): unimplemented mem read [reg:%s] @ 0x%"PRIx64
 ahci_mem_read(void *s, unsigned size, uint64_t addr, uint64_t val) "ahci(%p): read%u @ 0x%"PRIx64": 0x%016"PRIx64
 ahci_mem_write(void *s, unsigned size, uint64_t addr, uint64_t val) "ahci(%p): write%u @ 0x%"PRIx64": 0x%016"PRIx64
-ahci_mem_write_unknown(void *s, unsigned size, uint64_t addr, uint64_t val) "ahci(%p): write%u to unknown register 0x%"PRIx64": 0x%016"PRIx64
+ahci_mem_write_host_unimpl(void *s, unsigned size, const char *reg, uint64_t addr) "ahci(%p) unimplemented write%u [reg:%s] @ 0x%"PRIx64
+ahci_mem_write_host(void *s, unsigned size, const char *reg, uint64_t addr, uint64_t val) "ahci(%p) write%u [reg:%s] @ 0x%"PRIx64": 0x%016"PRIx64
+ahci_mem_write_unimpl(void *s, unsigned size, uint64_t addr, uint64_t val) "ahci(%p): write%u to unknown register 0x%"PRIx64": 0x%016"PRIx64
 ahci_set_signature(void *s, int port, uint8_t nsector, uint8_t sector, uint8_t lcyl, uint8_t hcyl, uint32_t sig) "ahci(%p)[%d]: set signature sector:0x%02x nsector:0x%02x lcyl:0x%02x hcyl:0x%02x (cumulatively: 0x%08x)"
 ahci_reset_port(void *s, int port) "ahci(%p)[%d]: reset port"
 ahci_unmap_fis_address_null(void *s, int port) "ahci(%p)[%d]: Attempt to unmap NULL FIS address"
-- 
2.14.3

Re: [Qemu-devel] [PATCH 00/16] AHCI: tracing improvements

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180525235509.11282-1-jsnow@redhat.com
Subject: [Qemu-devel] [PATCH 00/16] AHCI: tracing improvements

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/1527034517-7851-1-git-send-email-mjc@sifive.com -> patchew/1527034517-7851-1-git-send-email-mjc@sifive.com
 t [tag update]            patchew/1527266793-301361-1-git-send-email-mst@redhat.com -> patchew/1527266793-301361-1-git-send-email-mst@redhat.com
 * [new tag]               patchew/20180525235509.11282-1-jsnow@redhat.com -> patchew/20180525235509.11282-1-jsnow@redhat.com
Switched to a new branch 'test'
6e9eac25b6 ahci: make ahci_mem_write traces more descriptive
0e9178a8a3 ahci: delete old host register address definitions
3564c2a548 ahci: adjust ahci_mem_write to work on registers
cc62a192cf ahci: fix spacing damage on ahci_mem_write
b92ccc4dc0 ahci: make mem_read_32 traces more descriptive
c70ee985e4 ahci: modify ahci_mem_read_32 to work on register numbers
0c503fd064 ahci: fix host register max address
3296ccc062 ahci: add host register enumeration
69baf78e85 ahci: delete old port register address definitions
059ca5a176 ahci: make port write traces more descriptive
5907e28690 ahci: modify ahci_port_write to use register numbers
cc6578bfe2 ahci: combine identical clauses in port write
bd7bb50ca6 ahci: fix spacing damage on ahci_port_write
4f6e337a4f ahci: make port read traces more descriptive
c629998268 ahci: modify ahci_port_read to use register numbers
f01e729be1 ahci: add port register enumeration

=== OUTPUT BEGIN ===
Checking PATCH 1/16: ahci: add port register enumeration...
WARNING: line over 80 characters
#71: FILE: hw/ide/ahci_internal.h:94:
+    AHCI_PORT_REG_SCR_NOTIF   = 15, /* PxSNTF: SATA phy register: SNotification */

total: 0 errors, 1 warnings, 72 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/16: ahci: modify ahci_port_read to use register numbers...
Checking PATCH 3/16: ahci: make port read traces more descriptive...
Checking PATCH 4/16: ahci: fix spacing damage on ahci_port_write...
ERROR: spaces required around that '|' (ctx:VxV)
#83: FILE: hw/ide/ahci.c:316:
+            (val & ~(PORT_CMD_RO_MASK|PORT_CMD_ICC_MASK));
                                      ^

total: 1 errors, 0 warnings, 156 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 5/16: ahci: combine identical clauses in port write...
Checking PATCH 6/16: ahci: modify ahci_port_write to use register numbers...
Checking PATCH 7/16: ahci: make port write traces more descriptive...
Checking PATCH 8/16: ahci: delete old port register address definitions...
Checking PATCH 9/16: ahci: add host register enumeration...
Checking PATCH 10/16: ahci: fix host register max address...
Checking PATCH 11/16: ahci: modify ahci_mem_read_32 to work on register numbers...
Checking PATCH 12/16: ahci: make mem_read_32 traces more descriptive...
Checking PATCH 13/16: ahci: fix spacing damage on ahci_mem_write...
Checking PATCH 14/16: ahci: adjust ahci_mem_write to work on registers...
Checking PATCH 15/16: ahci: delete old host register address definitions...
Checking PATCH 16/16: ahci: make ahci_mem_write traces more descriptive...
WARNING: line over 80 characters
#18: FILE: hw/ide/ahci.c:493:
+            trace_ahci_mem_write_host_unimpl(s, size, AHCIHostReg_lookup[regnum],

total: 0 errors, 1 warnings, 29 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com