Re: [Qemu-devel] [PATCH] net/vmxnet3: trace support for register access

[Miao Yan]

2016-01-12 15:57 GMT+08:00 Dmitry Fleytman <address@hidden>:
>
>> On 12 Jan 2016, at 09:23 AM, Miao Yan <address@hidden> wrote:
>>
>> Hi Dmitry,
>>
>> 2016-01-12 14:43 GMT+08:00 Dmitry Fleytman <address@hidden>:
>>>
>>>> On 12 Jan 2016, at 04:38 AM, Miao Yan <address@hidden> 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 <address@hidden>
>>>> ---
>>>> 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
>>>>
>>>
>