Hi Chenbo,
> -----Original Message-----
> From: Xia, Chenbo <chenbo....@intel.com>
> Sent: Wednesday, September 15, 2021 4:52 PM
> To: Li, Miao <miao...@intel.com>; dev@dpdk.org
> Cc: maxime.coque...@redhat.com
> Subject: RE: [PATCH 2/5] lib/vhost: implement rte_power_monitor API
>
> Hi Miao,
>
> > -----Original Message-----
> > From: Li, Miao <miao...@intel.com>
> > Sent: Friday, September 10, 2021 9:06 PM
> > To: dev@dpdk.org
> > Cc: Xia, Chenbo <chenbo....@intel.com>; maxime.coque...@redhat.com;
> > Li, Miao <miao...@intel.com>
> > Subject: [PATCH 2/5] lib/vhost: implement rte_power_monitor API
>
> Should be 'vhost: implement rte_power_monitor API'
I will modify it in the next version.
>
> >
> > This patch defines rte_vhost_power_monitor_cond which is used to pass
> > some information to vhost driver. The information is including the
> > address to monitor, the expected value, the mask to extract value read
> > from 'addr', the flag used to distinguish packed ring or split ring.
> > Vhost driver can use these information to fill rte_power_monitor_cond.
> >
> > Signed-off-by: Miao Li <miao...@intel.com>
> > ---
> > lib/vhost/rte_vhost.h | 33 +++++++++++++++++++++++++++++++++
> > lib/vhost/version.map | 3 +++
> > lib/vhost/vhost.c | 30 ++++++++++++++++++++++++++++++
> > 3 files changed, 66 insertions(+)
> >
> > diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h index
> > 8d875e9322..f58643b0a3 100644
> > --- a/lib/vhost/rte_vhost.h
> > +++ b/lib/vhost/rte_vhost.h
> > @@ -38,6 +38,8 @@ extern "C" {
> > #define RTE_VHOST_USER_ASYNC_COPY (1ULL << 7)
> > #define RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS (1ULL << 8)
> >
> > +#define VHOST_POWER_MONITOR_RING_PACKED (1ULL << 0)
>
> I'd say I don't quite like introducing this flag so that vhost lib app needs
> to know
> the vring is split or packed. I have another suggestion to do the same thing,
> please check below comment.
Yes, I agree with you. I will remove it in the next version.
>
> > +
> > /* Features. */
> > #ifndef VIRTIO_NET_F_GUEST_ANNOUNCE
> > #define VIRTIO_NET_F_GUEST_ANNOUNCE 21 @@ -292,6 +294,20 @@
> struct
> > vhost_device_ops {
> > void *reserved[1]; /**< Reserved for future extension */ };
> >
> > +/**
> > + * Power monitor condition.
> > + */
> > +struct rte_vhost_power_monitor_cond {
> > + volatile void *addr; /**< Address to monitor for changes */
> > + /**< If the `mask` is non-zero, location pointed
> > + * to by `addr` will be read and compared
> > + * against this value.
> > + */
> > + uint64_t val;
> > + uint64_t mask; /**< 64-bit mask to extract value read from `addr` */
> > + uint8_t flag; /**< if 1, vhost packed ring, otherwise split ring */
>
> What about define two values instead of the flag. One value for '(value & m)
> ==
> v ?' is True, another for False.
I think one value to represent true or false is enough. I will fix it in the
next version.
>
> > +};
> > +
> > /**
> > * Convert guest physical address to host virtual address
> > *
> > @@ -914,6 +930,23 @@ int rte_vhost_vring_call(int vid, uint16_t vring_idx);
> > */
> > uint32_t rte_vhost_rx_queue_count(int vid, uint16_t qid);
> >
> > +/**
> > + * Get power monitor address of the vhost device
> > + *
> > + * @param vid
> > + * vhost device ID
> > + * @param queue_id
> > + * vhost queue ID
> > + * @param pmc
> > + * power monitor condition
> > + * @return
> > + * 0 on success, -1 on failure
> > + */
> > +__rte_experimental
> > +int
> > +rte_vhost_get_monitor_addr(int vid, uint16_t queue_id,
> > + struct rte_vhost_power_monitor_cond *pmc);
> > +
> > /**
> > * Get log base and log size of the vhost device
> > *
> > diff --git a/lib/vhost/version.map b/lib/vhost/version.map index
> > c92a9d4962..0a9667ef1e 100644
> > --- a/lib/vhost/version.map
> > +++ b/lib/vhost/version.map
> > @@ -85,4 +85,7 @@ EXPERIMENTAL {
> > rte_vhost_async_channel_register_thread_unsafe;
> > rte_vhost_async_channel_unregister_thread_unsafe;
> > rte_vhost_clear_queue_thread_unsafe;
> > +
> > + # added in 21.11
> > + rte_vhost_get_monitor_addr;
> > };
> > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index
> > 355ff37651..f7374d3f94 100644
> > --- a/lib/vhost/vhost.c
> > +++ b/lib/vhost/vhost.c
> > @@ -1886,5 +1886,35 @@ int rte_vhost_async_get_inflight(int vid,
> > uint16_t
> > queue_id)
> > return ret;
> > }
> >
> > +int
> > +rte_vhost_get_monitor_addr(int vid, uint16_t queue_id,
> > + struct rte_vhost_power_monitor_cond *pmc) {
> > + struct virtio_net *dev = get_device(vid);
>
> Check dev is not NULL before accessing its member.
I will add dev and queue_id check here in the next version.
>
> > + struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
> > + if (vq == NULL)
> > + return -1;
> > + if (vq_is_packed(dev)) {
> > + struct vring_packed_desc *desc;
> > + desc = vq->desc_packed;
> > + pmc->addr = &desc[vq->last_avail_idx].flags;
> > + if (vq->avail_wrap_counter)
> > + pmc->val = VRING_DESC_F_AVAIL;
> > + else
> > + pmc->val = VRING_DESC_F_USED;
> > + pmc->mask = VRING_DESC_F_AVAIL | VRING_DESC_F_USED;
> > + pmc->flag = VHOST_POWER_MONITOR_RING_PACKED;
> > + } else {
> > + pmc->addr = &vq->avail->idx;
> > + pmc->val = vq->last_avail_idx & (vq->size - 1);
> > + pmc->mask = vq->size - 1;
> > + pmc->flag = 0;
> > + }
> > + if (pmc->addr == NULL)
> > + return -1;
>
> Is it possible that addr == NULL?
Yes, it is unnecessary. I will delete it in the next version.
Thanks,
Miao
>
> Thanks,
> Chenbo
>
> > +
> > + return 0;
> > +}
> > +
> > RTE_LOG_REGISTER_SUFFIX(vhost_config_log_level, config, INFO);
> > RTE_LOG_REGISTER_SUFFIX(vhost_data_log_level, data, WARNING);
> > --
> > 2.25.1
