Re: [Qemu-block] [Qemu-devel] [PATCH 6/9] AHCI: Replace DPRINTF with trace-events

[John Snow]

On 08/25/2017 09:17 AM, Philippe Mathieu-Daudé wrote:
> Hi John,
> 
> On 08/08/2017 03:33 PM, John Snow wrote:
>> There are a few hangers-on that will be dealt with individually
>> in forthcoming patches.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>   hw/ide/ahci.c       | 157
>> +++++++++++++++++++++++-----------------------------
>>   hw/ide/trace-events |  52 ++++++++++++++++-
>>   2 files changed, 119 insertions(+), 90 deletions(-)
>>

snip, snip

>>   diff --git a/hw/ide/trace-events b/hw/ide/trace-events
>> index 6e33cb3..e1a0457 100644
>> --- a/hw/ide/trace-events
>> +++ b/hw/ide/trace-events
>> @@ -52,4 +52,54 @@ ide_atapi_cmd_read(void *s, const char *method, int
>> lba, int nb_sectors) "IDESta
>>   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"
>> \ No newline at end of file
>> +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_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_port_write(void *s, int port, int offset, uint32_t val)
>> "ahci(%p)[%d]: port write @ 0x%x: 0x%08x"
>> +ahci_mem_read_32(void *s, uint64_t addr, uint32_t val) "ahci(%p): mem
>> read @ 0x%"PRIx64": 0x%08x"
> 
> can you use same format than ahci_mem_read()?
> 
> "ahci(%p): read%u @ 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
>> +ahci_mem_write_unknown(void *s, uint64_t addr) "ahci(%p): write to
>> unknown register 0x%"PRIx64
> 
> report the value written, eventually:
> 
> "ahci(%p): write%u @ 0x%"PRIx64": 0x%016"PRIx64" UNKNOWN"
> 

Not necessary here; it's reported by the more generic
trace_ahci_mem_write which gets all of the writes no matter where they
go. In this case, too, the write is actually ignored and doesn't wind up
going anywhere ...

... but I suppose it wouldn't hurt to know what that value would have
been. If we enable just this trace that will probably help illuminate
what the errant write is.

>> +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"
> 
> could be generic:
> 
> ahci_port(void *s, int port) "ahci(%p)[%d]: %s"
> 
> then
> 
> trace_ahci_port(s, port, "reset port");
> 

I don't disagree, just more trepidation on what that means for the trace
events which are otherwise all named exactly for the functions that call
them.

Also, the problem with combining these sorts of traces becomes one with
the DPRINTF we're replacing: you can't target individual sections of
code anymore, and you either turn on "ahci_port" or off.

If there is a better suggestion for avoiding the ahci(%p)[%d]: pattern
here over and over and over and over again I'd happily take it.

Stefan?

[...snip...]

>> +ahci_reset(void *s) "ahci(%p): HBA reset"
>> +allwinner_ahci_mem_read(void *s, void *a, uint64_t addr, uint64_t
>> val, unsigned size) "ahci(%p): read a=%p addr=0x%"HWADDR_PRIx"
>> val=0x%"PRIx64", size=%d"
> 
> is AllwinnerAHCIState useful?
> 

Only as far as it happens to be the argument to this function; of course
AHCIState is the real prize, but we have to fish it out of
AllwinnerAHCIState first.

> I'd rather use directly trace_ahci_mem_read(s, size, addr, val)
> >> +allwinner_ahci_mem_write(void *s, void *a, uint64_t addr, uint64_t
>> val, unsigned size) "ahci(%p): write a=%p addr=0x%"HWADDR_PRIx"
>> val=0x%"PRIx64", size=%d"
> 
> and trace_ahci_mem_write(s, size, addr, val)
> 
> Regards,
> 
> Phil.