Re: [Qemu-block] [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 <address@hidden>
>>>> ---
>>>>  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é <address@hidden>
> 

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