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

[John Snow <jsnow@redhat.com>]

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