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 <js...@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é <f4...@amsat.org>
