Re: [dpdk-dev] [PATCH v3 1/4] bus/vmbus: add hyper-v virtual bus support

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 18 Apr 2018 14:16:56 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 4/17/2018 10:53 PM, Stephen Hemminger wrote:
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > 
> > This patch adds support for an additional bus type Virtual Machine BUS
> > (VMBUS) on Microsoft Hyper-V in Windows 10, Windows Server 2016
> > and Azure. Most of this code was extracted from FreeBSD and some of
> > this is from earlier code donated by Brocade.
> > 
> > Only Linux is supported at present, but the code is split
> > to allow future FreeBSD and Windows support.
> > 
> > The bus support relies on the uio_hv_generic driver from Linux
> > kernel 4.16. Multiple queue support requires additional sysfs
> > interfaces which is in kernel 5.0 (a.k.a 4.17).
> > 
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > ---
> >  MAINTAINERS                                 |   3 +
> >  config/common_base                          |   5 +
> >  config/common_linuxapp                      |   4 +
> >  drivers/bus/Makefile                        |   1 +
> >  drivers/bus/vmbus/Makefile                  |  36 ++
> >  drivers/bus/vmbus/linux/Makefile            |   3 +
> >  drivers/bus/vmbus/linux/vmbus_bus.c         | 355 +++++++++++++++++
> >  drivers/bus/vmbus/linux/vmbus_uio.c         | 381 ++++++++++++++++++
> >  drivers/bus/vmbus/private.h                 | 132 +++++++
> >  drivers/bus/vmbus/rte_bus_vmbus.h           | 400 +++++++++++++++++++
> >  drivers/bus/vmbus/rte_bus_vmbus_version.map |  28 ++
> >  drivers/bus/vmbus/rte_vmbus_reg.h           | 344 +++++++++++++++++
> >  drivers/bus/vmbus/vmbus_bufring.c           | 241 ++++++++++++
> >  drivers/bus/vmbus/vmbus_channel.c           | 405 ++++++++++++++++++++
> >  drivers/bus/vmbus/vmbus_common.c            | 286 ++++++++++++++
> >  drivers/bus/vmbus/vmbus_common_uio.c        | 232 +++++++++++
> >  mk/rte.app.mk                               |   1 +  
> 
> <...>
> 
> > @@ -381,6 +381,9 @@ VDEV bus driver
> >  M: Jianfeng Tan <jianfeng.tan@intel.com>
> >  F: drivers/bus/vdev/
> >  
> > +VMBUS bus driver
> > +M: Stephen Hemminger <sthemmin@microsoft.com>
> > +F: drivers/bus/vmbus/  
> 
> We have methods to mark APIs as experimental but not for drivers, we have put "-
> EXPERIMENTAL" suffix in MAINTAINER file before, same thing can be done here.

Ok, will add this to v4

> 
> > @@ -37,3 +37,7 @@ CONFIG_RTE_LIBRTE_DPAA2_MEMPOOL=y
> >  CONFIG_RTE_LIBRTE_DPAA2_PMD=y
> >  CONFIG_RTE_LIBRTE_PMD_DPAA2_EVENTDEV=y
> >  CONFIG_RTE_LIBRTE_PMD_DPAA2_SEC=y
> > +
> > +# Hyper-V Virtual Machine bus and drivers
> > +CONFIG_RTE_LIBRTE_VMBUS=y  
> 
> We tend to disable the component that has external dependency, because of uuid
> library dependency I believe vmbus should be disabled by default.

Ok. But libuuid is really trivial.

> > +LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
> > +LDLIBS += -lrte_ethdev -lrte_vmbus -luuid  
> 
> -lrte_vmbus is causing build error for shared build, there is no library like
> that, looks like can be dropped.

Will check, haven't modified that in a while.

> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause */
> > +EXPERIMENTAL {
> > +	global:
> > +
> > +	rte_vmbus_chan_close;
> > +	rte_vmbus_chan_open;
> > +	rte_vmbus_chan_recv;
> > +	rte_vmbus_chan_recv_raw;
> > +	rte_vmbus_chan_rx_empty;
> > +	rte_vmbus_chan_send;
> > +	rte_vmbus_chan_send_sglist;
> > +	rte_vmbus_chan_signal_tx;
> > +	rte_vmbus_irq_mask;
> > +	rte_vmbus_irq_read;
> > +	rte_vmbus_irq_unmask;
> > +	rte_vmbus_map_device;
> > +	rte_vmbus_max_channels;
> > +	rte_vmbus_probe;
> > +	rte_vmbus_probe_one;
> > +	rte_vmbus_register;
> > +	rte_vmbus_scan;
> > +	rte_vmbus_sub_channel_index;
> > +	rte_vmbus_subchan_open;
> > +	rte_vmbus_unmap_device;
> > +	rte_vmbus_unregister;
> > +
> > +	local: *;
> > +};  
> 
> Just to confirm, these APIs are for bus drivers, not for applications, right? If
> so we already don't guarantee comptiblity in this level, perhaps adding
> __experimental flag to these APIs is overkill but no harm.

Yes, these are internal bus api's.   I will take off experimental from
the bus part. It makes life simpler to have it gone.