2016-01-12 15:57 GMT+08:00 Dmitry Fleytman <dmi...@daynix.com>: > >> On 12 Jan 2016, at 09:23 AM, Miao Yan <yanmiaob...@gmail.com> wrote: >> >> Hi Dmitry, >> >> 2016-01-12 14:43 GMT+08:00 Dmitry Fleytman <dmi...@daynix.com>: >>> >>>> On 12 Jan 2016, at 04:38 AM, Miao Yan <yanmiaob...@gmail.com> wrote: >>>> >>>> Turning debug printfs to trace points for register access >>> >>> Hello Miao! >>> >>> While I’m into adding trace points I don’t really like the decrease of logs >>> usability introduced by this patch. >> >> How about I add trace point and keep those debug logs ? > > I’d prefer the complete solution i.e. to replace all printouts with traces. > Otherwise it doesn’t make much sense.
OK, I get your point. Will investigate and prepare v2. Thanks. > >> >>> Current code produces clear human readable log that allows to trace >>> execution without looking into tables of commands and BAR layout. >>> >>> I’d say that every printout you removed should be replaced with a trace >>> point. >> >> The printfs that I removed are only for register accesses, which are already >> covered by trace. I didn't touch others in the code flow. > > I understand this. My point is that generic trace is less readable than a > number of specific ones, so do not drop specific printouts, convert those to > trace points. > >> >> Thanks, >> Miao >> >> >>> >>> Thanks, >>> Dmitry >>> >>>> >>>> Signed-off-by: Miao Yan <yanmiaob...@gmail.com> >>>> --- >>>> hw/net/vmxnet3.c | 68 >>>> +++++++++----------------------------------------------- >>>> trace-events | 6 +++++ >>>> 2 files changed, 16 insertions(+), 58 deletions(-) >>>> >>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c >>>> index 67abad3..e089037 100644 >>>> --- a/hw/net/vmxnet3.c >>>> +++ b/hw/net/vmxnet3.c >>>> @@ -32,6 +32,8 @@ >>>> #include "vmxnet_tx_pkt.h" >>>> #include "vmxnet_rx_pkt.h" >>>> >>>> +#include "trace.h" >>>> + >>>> #define PCI_DEVICE_ID_VMWARE_VMXNET3_REVISION 0x1 >>>> #define VMXNET3_MSIX_BAR_SIZE 0x2000 >>>> #define MIN_BUF_SIZE 60 >>>> @@ -1157,6 +1159,8 @@ vmxnet3_io_bar0_write(void *opaque, hwaddr addr, >>>> { >>>> VMXNET3State *s = opaque; >>>> >>>> + trace_vmxnet3_bar0_write(opaque, addr, val); >>>> + >>>> if (VMW_IS_MULTIREG_ADDR(addr, VMXNET3_REG_TXPROD, >>>> VMXNET3_DEVICE_MAX_TX_QUEUES, VMXNET3_REG_ALIGN)) { >>>> int tx_queue_idx = >>>> @@ -1171,9 +1175,6 @@ vmxnet3_io_bar0_write(void *opaque, hwaddr addr, >>>> VMXNET3_MAX_INTRS, VMXNET3_REG_ALIGN)) { >>>> int l = VMW_MULTIREG_IDX_BY_ADDR(addr, VMXNET3_REG_IMR, >>>> VMXNET3_REG_ALIGN); >>>> - >>>> - VMW_CBPRN("Interrupt mask for line %d written: 0x%" PRIx64, l, >>>> val); >>>> - >>>> vmxnet3_on_interrupt_mask_changed(s, l, val); >>>> return; >>>> } >>>> @@ -1184,9 +1185,6 @@ vmxnet3_io_bar0_write(void *opaque, hwaddr addr, >>>> VMXNET3_DEVICE_MAX_RX_QUEUES, VMXNET3_REG_ALIGN)) { >>>> return; >>>> } >>>> - >>>> - VMW_WRPRN("BAR0 unknown write [%" PRIx64 "] = %" PRIx64 ", size %d", >>>> - (uint64_t) addr, val, size); >>>> } >>>> >>>> static uint64_t >>>> @@ -1201,7 +1199,8 @@ vmxnet3_io_bar0_read(void *opaque, hwaddr addr, >>>> unsigned size) >>>> return s->interrupt_states[l].is_masked; >>>> } >>>> >>>> - VMW_CBPRN("BAR0 unknown read [%" PRIx64 "], size %d", addr, size); >>>> + trace_vmxnet3_bar0_read(opaque, addr, 0); >>>> + >>>> return 0; >>>> } >>>> >>>> @@ -1315,7 +1314,6 @@ static void vmxnet3_setup_rx_filtering(VMXNET3State >>>> *s) >>>> static uint32_t vmxnet3_get_interrupt_config(VMXNET3State *s) >>>> { >>>> uint32_t interrupt_mode = VMXNET3_IT_AUTO | (VMXNET3_IMM_AUTO << 2); >>>> - VMW_CFPRN("Interrupt config is 0x%X", interrupt_mode); >>>> return interrupt_mode; >>>> } >>>> >>>> @@ -1614,85 +1612,66 @@ static void vmxnet3_handle_command(VMXNET3State >>>> *s, uint64_t cmd) >>>> >>>> switch (cmd) { >>>> case VMXNET3_CMD_GET_PERM_MAC_HI: >>>> - VMW_CBPRN("Set: Get upper part of permanent MAC"); >>>> break; >>>> >>>> case VMXNET3_CMD_GET_PERM_MAC_LO: >>>> - VMW_CBPRN("Set: Get lower part of permanent MAC"); >>>> break; >>>> >>>> case VMXNET3_CMD_GET_STATS: >>>> - VMW_CBPRN("Set: Get device statistics"); >>>> vmxnet3_fill_stats(s); >>>> break; >>>> >>>> case VMXNET3_CMD_ACTIVATE_DEV: >>>> - VMW_CBPRN("Set: Activating vmxnet3 device"); >>>> vmxnet3_activate_device(s); >>>> break; >>>> >>>> case VMXNET3_CMD_UPDATE_RX_MODE: >>>> - VMW_CBPRN("Set: Update rx mode"); >>>> vmxnet3_update_rx_mode(s); >>>> break; >>>> >>>> case VMXNET3_CMD_UPDATE_VLAN_FILTERS: >>>> - VMW_CBPRN("Set: Update VLAN filters"); >>>> vmxnet3_update_vlan_filters(s); >>>> break; >>>> >>>> case VMXNET3_CMD_UPDATE_MAC_FILTERS: >>>> - VMW_CBPRN("Set: Update MAC filters"); >>>> vmxnet3_update_mcast_filters(s); >>>> break; >>>> >>>> case VMXNET3_CMD_UPDATE_FEATURE: >>>> - VMW_CBPRN("Set: Update features"); >>>> vmxnet3_update_features(s); >>>> break; >>>> >>>> case VMXNET3_CMD_UPDATE_PMCFG: >>>> - VMW_CBPRN("Set: Update power management config"); >>>> vmxnet3_update_pm_state(s); >>>> break; >>>> >>>> case VMXNET3_CMD_GET_LINK: >>>> - VMW_CBPRN("Set: Get link"); >>>> break; >>>> >>>> case VMXNET3_CMD_RESET_DEV: >>>> - VMW_CBPRN("Set: Reset device"); >>>> vmxnet3_reset(s); >>>> break; >>>> >>>> case VMXNET3_CMD_QUIESCE_DEV: >>>> - VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - deactivate the device"); >>>> vmxnet3_deactivate_device(s); >>>> break; >>>> >>>> case VMXNET3_CMD_GET_CONF_INTR: >>>> - VMW_CBPRN("Set: VMXNET3_CMD_GET_CONF_INTR - interrupt >>>> configuration"); >>>> break; >>>> >>>> case VMXNET3_CMD_GET_ADAPTIVE_RING_INFO: >>>> - VMW_CBPRN("Set: VMXNET3_CMD_GET_ADAPTIVE_RING_INFO - " >>>> - "adaptive ring info flags"); >>>> break; >>>> >>>> case VMXNET3_CMD_GET_DID_LO: >>>> - VMW_CBPRN("Set: Get lower part of device ID"); >>>> break; >>>> >>>> case VMXNET3_CMD_GET_DID_HI: >>>> - VMW_CBPRN("Set: Get upper part of device ID"); >>>> break; >>>> >>>> case VMXNET3_CMD_GET_DEV_EXTRA_INFO: >>>> - VMW_CBPRN("Set: Get device extra info"); >>>> break; >>>> >>>> default: >>>> - VMW_CBPRN("Received unknown command: %" PRIx64, cmd); >>>> break; >>>> } >>>> } >>>> @@ -1704,7 +1683,6 @@ static uint64_t >>>> vmxnet3_get_command_status(VMXNET3State *s) >>>> switch (s->last_command) { >>>> case VMXNET3_CMD_ACTIVATE_DEV: >>>> ret = (s->device_active) ? 0 : 1; >>>> - VMW_CFPRN("Device active: %" PRIx64, ret); >>>> break; >>>> >>>> case VMXNET3_CMD_RESET_DEV: >>>> @@ -1716,7 +1694,6 @@ static uint64_t >>>> vmxnet3_get_command_status(VMXNET3State *s) >>>> >>>> case VMXNET3_CMD_GET_LINK: >>>> ret = s->link_status_and_speed; >>>> - VMW_CFPRN("Link and speed: %" PRIx64, ret); >>>> break; >>>> >>>> case VMXNET3_CMD_GET_PERM_MAC_LO: >>>> @@ -1744,7 +1721,6 @@ static uint64_t >>>> vmxnet3_get_command_status(VMXNET3State *s) >>>> break; >>>> >>>> default: >>>> - VMW_WRPRN("Received request for unknown command: %x", >>>> s->last_command); >>>> ret = 0; >>>> break; >>>> } >>>> @@ -1765,7 +1741,6 @@ static void vmxnet3_ack_events(VMXNET3State *s, >>>> uint32_t val) >>>> { >>>> uint32_t events; >>>> >>>> - VMW_CBPRN("Clearing events: 0x%x", val); >>>> events = VMXNET3_READ_DRV_SHARED32(s->drv_shmem, ecr) & ~val; >>>> VMXNET3_WRITE_DRV_SHARED32(s->drv_shmem, ecr, events); >>>> } >>>> @@ -1778,23 +1753,19 @@ vmxnet3_io_bar1_write(void *opaque, >>>> { >>>> VMXNET3State *s = opaque; >>>> >>>> + trace_vmxnet3_bar1_write(opaque, addr, val); >>>> + >>>> switch (addr) { >>>> /* Vmxnet3 Revision Report Selection */ >>>> case VMXNET3_REG_VRRS: >>>> - VMW_CBPRN("Write BAR1 [VMXNET3_REG_VRRS] = %" PRIx64 ", size %d", >>>> - val, size); >>>> break; >>>> >>>> /* UPT Version Report Selection */ >>>> case VMXNET3_REG_UVRS: >>>> - VMW_CBPRN("Write BAR1 [VMXNET3_REG_UVRS] = %" PRIx64 ", size %d", >>>> - val, size); >>>> break; >>>> >>>> /* Driver Shared Address Low */ >>>> case VMXNET3_REG_DSAL: >>>> - VMW_CBPRN("Write BAR1 [VMXNET3_REG_DSAL] = %" PRIx64 ", size %d", >>>> - val, size); >>>> /* >>>> * Guest driver will first write the low part of the shared >>>> * memory address. We save it to temp variable and set the >>>> @@ -1809,8 +1780,6 @@ vmxnet3_io_bar1_write(void *opaque, >>>> >>>> /* Driver Shared Address High */ >>>> case VMXNET3_REG_DSAH: >>>> - VMW_CBPRN("Write BAR1 [VMXNET3_REG_DSAH] = %" PRIx64 ", size %d", >>>> - val, size); >>>> /* >>>> * Set the shared memory between guest driver and device. >>>> * We already should have low address part. >>>> @@ -1820,42 +1789,30 @@ vmxnet3_io_bar1_write(void *opaque, >>>> >>>> /* Command */ >>>> case VMXNET3_REG_CMD: >>>> - VMW_CBPRN("Write BAR1 [VMXNET3_REG_CMD] = %" PRIx64 ", size %d", >>>> - val, size); >>>> vmxnet3_handle_command(s, val); >>>> break; >>>> >>>> /* MAC Address Low */ >>>> case VMXNET3_REG_MACL: >>>> - VMW_CBPRN("Write BAR1 [VMXNET3_REG_MACL] = %" PRIx64 ", size %d", >>>> - val, size); >>>> s->temp_mac = val; >>>> break; >>>> >>>> /* MAC Address High */ >>>> case VMXNET3_REG_MACH: >>>> - VMW_CBPRN("Write BAR1 [VMXNET3_REG_MACH] = %" PRIx64 ", size %d", >>>> - val, size); >>>> vmxnet3_set_variable_mac(s, val, s->temp_mac); >>>> break; >>>> >>>> /* Interrupt Cause Register */ >>>> case VMXNET3_REG_ICR: >>>> - VMW_CBPRN("Write BAR1 [VMXNET3_REG_ICR] = %" PRIx64 ", size %d", >>>> - val, size); >>>> g_assert_not_reached(); >>>> break; >>>> >>>> /* Event Cause Register */ >>>> case VMXNET3_REG_ECR: >>>> - VMW_CBPRN("Write BAR1 [VMXNET3_REG_ECR] = %" PRIx64 ", size %d", >>>> - val, size); >>>> vmxnet3_ack_events(s, val); >>>> break; >>>> >>>> default: >>>> - VMW_CBPRN("Unknown Write to BAR1 [%" PRIx64 "] = %" PRIx64 ", >>>> size %d", >>>> - addr, val, size); >>>> break; >>>> } >>>> } >>>> @@ -1869,31 +1826,26 @@ vmxnet3_io_bar1_read(void *opaque, hwaddr addr, >>>> unsigned size) >>>> switch (addr) { >>>> /* Vmxnet3 Revision Report Selection */ >>>> case VMXNET3_REG_VRRS: >>>> - VMW_CBPRN("Read BAR1 [VMXNET3_REG_VRRS], size %d", size); >>>> ret = VMXNET3_DEVICE_REVISION; >>>> break; >>>> >>>> /* UPT Version Report Selection */ >>>> case VMXNET3_REG_UVRS: >>>> - VMW_CBPRN("Read BAR1 [VMXNET3_REG_UVRS], size %d", size); >>>> ret = VMXNET3_UPT_REVISION; >>>> break; >>>> >>>> /* Command */ >>>> case VMXNET3_REG_CMD: >>>> - VMW_CBPRN("Read BAR1 [VMXNET3_REG_CMD], size %d", size); >>>> ret = vmxnet3_get_command_status(s); >>>> break; >>>> >>>> /* MAC Address Low */ >>>> case VMXNET3_REG_MACL: >>>> - VMW_CBPRN("Read BAR1 [VMXNET3_REG_MACL], size %d", size); >>>> ret = vmxnet3_get_mac_low(&s->conf.macaddr); >>>> break; >>>> >>>> /* MAC Address High */ >>>> case VMXNET3_REG_MACH: >>>> - VMW_CBPRN("Read BAR1 [VMXNET3_REG_MACH], size %d", size); >>>> ret = vmxnet3_get_mac_high(&s->conf.macaddr); >>>> break; >>>> >>>> @@ -1902,7 +1854,6 @@ vmxnet3_io_bar1_read(void *opaque, hwaddr addr, >>>> unsigned size) >>>> * Used for legacy interrupts only so interrupt index always 0 >>>> */ >>>> case VMXNET3_REG_ICR: >>>> - VMW_CBPRN("Read BAR1 [VMXNET3_REG_ICR], size %d", size); >>>> if (vmxnet3_interrupt_asserted(s, 0)) { >>>> vmxnet3_clear_interrupt(s, 0); >>>> ret = true; >>>> @@ -1912,10 +1863,11 @@ vmxnet3_io_bar1_read(void *opaque, hwaddr addr, >>>> unsigned size) >>>> break; >>>> >>>> default: >>>> - VMW_CBPRN("Unknow read BAR1[%" PRIx64 "], %d bytes", addr, >>>> size); >>>> break; >>>> } >>>> >>>> + trace_vmxnet3_bar1_read(opaque, addr, ret); >>>> + >>>> return ret; >>>> } >>>> >>>> diff --git a/trace-events b/trace-events >>>> index 6f03638..48323e2 100644 >>>> --- a/trace-events >>>> +++ b/trace-events >>>> @@ -1375,6 +1375,12 @@ pcnet_mmio_readb(void *opaque, uint64_t addr, >>>> uint32_t val) "opaque=%p addr=%#"P >>>> pcnet_mmio_readw(void *opaque, uint64_t addr, uint32_t val) "opaque=%p >>>> addr=%#"PRIx64" val=0x%x" >>>> pcnet_mmio_readl(void *opaque, uint64_t addr, uint32_t val) "opaque=%p >>>> addr=%#"PRIx64" val=0x%x" >>>> >>>> +# hw/net/vmxnet3.c >>>> +vmxnet3_bar0_write(void *opaque, uint64_t addr, uint64_t val) "opaque=%p >>>> addr=0x%"PRIx64" val=0x%"PRIx64 >>>> +vmxnet3_bar0_read(void *opaque, uint64_t addr, uint64_t val) "opaque=%p >>>> addr=0x%"PRIx64" val=0x%"PRIx64 >>>> +vmxnet3_bar1_write(void *opaque, uint64_t addr, uint64_t val) "opaque=%p >>>> addr=0x%"PRIx64" val=0x%"PRIx64 >>>> +vmxnet3_bar1_read(void *opaque, uint64_t addr, uint64_t val) "opaque=%p >>>> addr=0x%"PRIx64" val=0x%"PRIx64 >>>> + >>>> # hw/intc/xics.c >>>> xics_icp_check_ipi(int server, uint8_t mfrr) "CPU %d can take IPI mfrr=%#x" >>>> xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr) "icp_accept: XIRR >>>> %#"PRIx32"->%#"PRIx32 >>>> -- >>>> 1.9.1 >>>> >>> >