Re: [PATCH v2 2/2] vhost-vdpa: add trace-events

[Laurent Vivier]

On 22/09/2020 04:09, Jason Wang wrote:
> 
> On 2020/9/21 下午9:04, Laurent Vivier wrote:
>> Add trace functionis in vhost-vdpa.c.
>>
>> All traces from this file can be enabled with '-trace vhost_vdpa*'.
>>
>> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   hw/virtio/trace-events | 29 ++++++++++++++
>>   hw/virtio/vhost-vdpa.c | 86 +++++++++++++++++++++++++++++++++++++++---
>>   2 files changed, 110 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
>> index 845200bf109d..2b453f77a4e3 100644
>> --- a/hw/virtio/trace-events
>> +++ b/hw/virtio/trace-events
>> @@ -22,6 +22,35 @@ vhost_user_postcopy_waker(const char *rb, uint64_t
>> rb_offset) "%s + 0x%"PRIx64
>>   vhost_user_postcopy_waker_found(uint64_t client_addr) "0x%"PRIx64
>>   vhost_user_postcopy_waker_nomatch(const char *rb, uint64_t
>> rb_offset) "%s + 0x%"PRIx64
>>   +# vhost-vdpa.c
>> +vhost_vdpa_listener_region_add(void *vdpa, uint64_t iova, uint64_t
>> llend, void *vaddr, bool readonly) "vdpa: %p iova 0x%"PRIx64" llend
>> 0x%"PRIx64" vaddr: %p read-only: %d"
>> +vhost_vdpa_listener_region_del(void *vdpa, uint64_t iova, uint64_t
>> llend) "vdpa: %p iova 0x%"PRIx64" llend 0x%"PRIx64
>> +vhost_vdpa_add_status(void *dev, uint8_t status) "dev: %p status:
>> 0x%"PRIx8
>> +vhost_vdpa_init(void *dev, void *vdpa) "dev: %p vdpa: %p"
>> +vhost_vdpa_cleanup(void *dev, void *vdpa) "dev: %p vdpa: %p"
>> +vhost_vdpa_memslots_limit(void *dev, int ret) "dev: %p = 0x%x"
>> +vhost_vdpa_set_mem_table(void *dev, uint32_t nregions, uint32_t
>> padding) "dev: %p nregions: %"PRIu32" padding: 0x%"PRIx32
>> +vhost_vdpa_dump_regions(void *dev, int i, uint64_t guest_phys_addr,
>> uint64_t memory_size, uint64_t userspace_addr, uint64_t flags_padding)
>> "dev: %p %d: guest_phys_addr: 0x%"PRIx64" memory_size: 0x%"PRIx64"
>> userspace_addr: 0x%"PRIx64" flags_padding: 0x%"PRIx64
>> +vhost_vdpa_set_features(void *dev, uint64_t features) "dev: %p
>> features: 0x%"PRIx64
>> +vhost_vdpa_get_device_id(void *dev, uint32_t device_id) "dev: %p
>> device_id %"PRIu32
>> +vhost_vdpa_reset_device(void *dev, uint8_t status) "dev: %p status:
>> 0x%"PRIx8
>> +vhost_vdpa_get_vq_index(void *dev, int idx, int vq_idx) "dev: %p idx:
>> %d vq idx: %d"
>> +vhost_vdpa_set_vring_ready(void *dev) "dev: %p"
>> +vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s"
>> +vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size,
>> uint32_t flags) "dev: %p offset: %"PRIu32" size: %"PRIu32" flags:
>> 0x%"PRIx32
>> +vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len)
>> "dev: %p config: %p config_len: %"PRIu32
>> +vhost_vdpa_dev_start(void *dev, bool started) "dev: %p started: %d"
>> +vhost_vdpa_set_log_base(void *dev, uint64_t base, unsigned long long
>> size, int refcnt, int fd, void *log) "dev: %p base: 0x%"PRIx64" size:
>> %llu refcnt: %d fd: %d log: %p"
>> +vhost_vdpa_set_vring_addr(void *dev, unsigned int index, unsigned int
>> flags, uint64_t desc_user_addr, uint64_t used_user_addr, uint64_t
>> avail_user_addr, uint64_t log_guest_addr) "dev: %p index: %u flags:
>> 0x%x desc_user_addr: 0x%"PRIx64" used_user_addr: 0x%"PRIx64"
>> avail_user_addr: 0x%"PRIx64" log_guest_addr: 0x%"PRIx64
>> +vhost_vdpa_set_vring_num(void *dev, unsigned int index, unsigned int
>> num) "dev: %p index: %u num: %u"
>> +vhost_vdpa_set_vring_base(void *dev, unsigned int index, unsigned int
>> num) "dev: %p index: %u num: %u"
>> +vhost_vdpa_get_vring_base(void *dev, unsigned int index, unsigned int
>> num) "dev: %p index: %u num: %u"
>> +vhost_vdpa_set_vring_kick(void *dev, unsigned int index, int fd)
>> "dev: %p index: %u fd: %d"
>> +vhost_vdpa_set_vring_call(void *dev, unsigned int index, int fd)
>> "dev: %p index: %u fd: %d"
> 
> 
> It's better to add set/get_vring_addr() and dma_map()/dma_unmap().

I'm adding dma_map()/dma_unmap().

set_vring_addr() is already in the list, and get_vring_addr() doesn't
exist. As I don't see the link with vhost_vdpa_set_vring_call() I would
keep it.

Did I miss something?

Thanks,
Laurent