| Message ID | 20180525235509.11282-4-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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" >
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
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>
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
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"
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(-)