> 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. > >> 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 >>> >>