[dpdk-dev,06/17] vhost: introduce API to fetch negotiated features

  Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_vhost/rte_vhost_version.map |  1 +
 lib/librte_vhost/rte_virtio_net.h      |  1 +
 lib/librte_vhost/vhost.c               | 12 ++++++++++++
 3 files changed, 14 insertions(+)
  

On 03/03/2017 10:51 AM, Yuanhan Liu wrote:
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
>  lib/librte_vhost/rte_vhost_version.map |  1 +
>  lib/librte_vhost/rte_virtio_net.h      |  1 +
>  lib/librte_vhost/vhost.c               | 12 ++++++++++++
>  3 files changed, 14 insertions(+)
>
> diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
> index b890da6..85a2796 100644
> --- a/lib/librte_vhost/rte_vhost_version.map
> +++ b/lib/librte_vhost/rte_vhost_version.map
> @@ -35,6 +35,7 @@ DPDK_17.05 {
>  	rte_vhost_driver_enable_features;
>  	rte_vhost_driver_get_features;
>  	rte_vhost_driver_set_features;
> +	rte_vhost_get_negotiated_features
>  	rte_vhost_get_vhost_memory;
>
>  } DPDK_16.07;
> diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h
> index eddf0f4..e7b1599 100644
> --- a/lib/librte_vhost/rte_virtio_net.h
> +++ b/lib/librte_vhost/rte_virtio_net.h
> @@ -210,5 +210,6 @@ uint16_t rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
>  	struct rte_mempool *mbuf_pool, struct rte_mbuf **pkts, uint16_t count);
>
>  int rte_vhost_get_vhost_memory(int vid, struct rte_vhost_memory **mem);
> +uint64_t rte_vhost_get_negotiated_features(int vid);
>
>  #endif /* _VIRTIO_NET_H_ */
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index eee229a..8046aef 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -340,6 +340,18 @@ struct virtio_net *
>  	return 0;
>  }
>
> +uint64_t
> +rte_vhost_get_negotiated_features(int vid)
> +{
> +	struct virtio_net *dev;
> +
> +	dev = get_device(vid);
> +	if (!dev)
> +		return -1;
> +
> +	return dev->features;
> +}
Are we sure the negotiation is done when we can get the device?

Maxime
  

On Tue, Mar 14, 2017 at 12:02:59PM +0100, Maxime Coquelin wrote:
> >+uint64_t
> >+rte_vhost_get_negotiated_features(int vid)
> >+{
> >+	struct virtio_net *dev;
> >+
> >+	dev = get_device(vid);
> >+	if (!dev)
> >+		return -1;
> >+
> >+	return dev->features;
> >+}
> Are we sure the negotiation is done when we can get the device?

Yes. However, one thing worth noting is that the features may change
after the new_device() callback. Notablely, when live migration starts
/ends, the VHOST_F_LOG_ALL will be set/cleared, respectively.

From that point of view, we need a new callback, something like
features_changed(), or live_migration_starts()? Or a better name?

	--yliu
  

On 03/16/2017 08:35 AM, Yuanhan Liu wrote:
> On Tue, Mar 14, 2017 at 12:02:59PM +0100, Maxime Coquelin wrote:
>>> +uint64_t
>>> +rte_vhost_get_negotiated_features(int vid)
>>> +{
>>> +	struct virtio_net *dev;
>>> +
>>> +	dev = get_device(vid);
>>> +	if (!dev)
>>> +		return -1;
>>> +
>>> +	return dev->features;
>>> +}
>> Are we sure the negotiation is done when we can get the device?
>
> Yes. However, one thing worth noting is that the features may change
> after the new_device() callback. Notablely, when live migration starts
> /ends, the VHOST_F_LOG_ALL will be set/cleared, respectively.

Good point.

> From that point of view, we need a new callback, something like
> features_changed(), or live_migration_starts()? Or a better name?

I would name it features_changed() to be future-proof (even if I don't
foresee other cases than live migration right now).

Thanks,
Maxime
  

On Thu, Mar 16, 2017 at 10:22:27AM +0100, Maxime Coquelin wrote:
> 
> 
> On 03/16/2017 08:35 AM, Yuanhan Liu wrote:
> >On Tue, Mar 14, 2017 at 12:02:59PM +0100, Maxime Coquelin wrote:
> >>>+uint64_t
> >>>+rte_vhost_get_negotiated_features(int vid)
> >>>+{
> >>>+	struct virtio_net *dev;
> >>>+
> >>>+	dev = get_device(vid);
> >>>+	if (!dev)
> >>>+		return -1;
> >>>+
> >>>+	return dev->features;
> >>>+}
> >>Are we sure the negotiation is done when we can get the device?
> >
> >Yes. However, one thing worth noting is that the features may change
> >after the new_device() callback. Notablely, when live migration starts
> >/ends, the VHOST_F_LOG_ALL will be set/cleared, respectively.
> 
> Good point.
> 
> >From that point of view, we need a new callback, something like
> >features_changed(), or live_migration_starts()? Or a better name?
> 
> I would name it features_changed() to be future-proof (even if I don't
> foresee other cases than live migration right now).

Right, it will be "features_changed()" then.

	--yliu