Re: [dpdk-dev] [PATCH v1 2/3] doc: add vDPA feature table

[Tiwei Bie <tiwei.bie@intel.com>]

On Wed, Jan 08, 2020 at 10:42:48AM +0000, Matan Azrad wrote:
> Hi all
> 
> Thanks very much for the review.
> Please see below.
> 
> From: Andrew Rybchenko
> > On 1/8/20 8:28 AM, Tiwei Bie wrote:
> > > On Tue, Jan 07, 2020 at 06:39:36PM +0100, Maxime Coquelin wrote:
> > >> On 12/25/19 4:19 PM, Matan Azrad wrote:
> > >>> Add vDPA devices features table and explanation.
> > >>>
> > >>> Any vDPA driver can add its own supported features by ading a new
> > >>> ini file to the features directory in doc/guides/vdpadevs/features.
> > >>>
> > >>> Signed-off-by: Matan Azrad <matan@mellanox.com>
> > >>> ---
> > >>>  doc/guides/conf.py                        |  5 +++
> > >>>  doc/guides/vdpadevs/features/default.ini  | 55
> > >>> ++++++++++++++++++++++++++
> > doc/guides/vdpadevs/features_overview.rst | 65
> > +++++++++++++++++++++++++++++++
> > >>>  doc/guides/vdpadevs/index.rst             |  1 +
> > >>>  4 files changed, 126 insertions(+)
> > >>>  create mode 100644 doc/guides/vdpadevs/features/default.ini
> > >>>  create mode 100644 doc/guides/vdpadevs/features_overview.rst
> > >>>
> > >>> diff --git a/doc/guides/conf.py b/doc/guides/conf.py index
> > >>> 0892c06..c368fa5 100644
> > >>> --- a/doc/guides/conf.py
> > >>> +++ b/doc/guides/conf.py
> > >>> @@ -401,6 +401,11 @@ def setup(app):
> > >>>                              'Features',
> > >>>                              'Features availability in compression drivers',
> > >>>                              'Feature')
> > >>> +    table_file = dirname(__file__) +
> > '/vdpadevs/overview_feature_table.txt'
> > >>> +    generate_overview_table(table_file, 1,
> > >>> +                            'Features',
> > >>> +                            'Features availability in vDPA drivers',
> > >>> +                            'Feature')
> > >>>
> > >>>      if LooseVersion(sphinx_version) < LooseVersion('1.3.1'):
> > >>>          print('Upgrade sphinx to version >= 1.3.1 for '
> > >>> diff --git a/doc/guides/vdpadevs/features/default.ini
> > >>> b/doc/guides/vdpadevs/features/default.ini
> > >>> new file mode 100644
> > >>> index 0000000..a3e0bc7
> > >>> --- /dev/null
> > >>> +++ b/doc/guides/vdpadevs/features/default.ini
> > >>> @@ -0,0 +1,55 @@
> > >>> +;
> > >>> +; Features of a default vDPA driver.
> > >>> +;
> > >>> +; This file defines the features that are valid for inclusion in ;
> > >>> +the other driver files and also the order that they appear in ; the
> > >>> +features table in the documentation. The feature description ;
> > >>> +string should not exceed feature_str_len defined in conf.py.
> > >>> +;
> > >> I think some entries below could be removed for vDPA.
> > > +1
> > >
> > >>> +[Features]
> > >>> +csum                 =
> > >>> +guest csum           =
> > >>> +mac                  =
> > >>> +gso                  =
> > >>> +guest tso4           =
> > >>> +guest tso6           =
> > >>> +ecn                  =
> > >>> +ufo                  =
> > >>> +host tso4            =
> > >>> +host tso6            =
> > >>> +mrg rxbuf            =
> > >>> +ctrl vq              =
> > >>> +ctrl rx              =
> > >>> +any layout           =
> > >>> +guest announce       =
> > >>> +mq                   =
> > >>> +version 1            =
> > >>> +log all              =
> > >>> +protocol features    =
> > > We may not need to list this. The proto * would imply it.
> 
> So can you explain why this flag is exposed by the vhost features?

This feature is needed in vhost-user to allow master and
slave to negotiate protocol features in a backward compatible
way. Supports of any proto features would imply the support
of this feature. If we want to shorten this list, it can be
a good candidate for removal.

> 
> > >>> +indirect desc        =
> > >>> +event idx            =
> > >>> +mtu                  =
> > >>> +in_order             =
> > >>> +IOMMU platform       =
> > >>> +packed               =
> > >>> +proto mq             =
> > >>> +proto log shmfd      =
> > >>> +proto rarp           =
> > >>> +proto reply ack      =
> > >>> +proto slave req      =
> > > Ditto. This feature is to be used by other features.
> > > Features like host notifier would imply it.
> 
> So can you explain why this flag is exposed by the vhost protocol features?

This feature allows master and slave to setup a slave channel
in a backward compatible way. Having a slave channel between
master and slave without any other features using it isn't very
useful. I.e. this feature is supposed to be used by the features
like pagefault, host notifier. And supports of these features
would imply the support of this feature as well. So it can be a
good candidate for removal to shorten this list.

> 
> > >>> +proto crypto session =
> > > We don't need to list this before we officially support the crypto
> > > vDPA device.
> > >
> 
> Ok, will remove.
> 
> > >>> +proto host notifier  =
> > >>> +proto pagefault      =
> > >>> +Multiprocess aware   =
> > > There is no support for this in library currently.
> > > To support it, we need to sync vhost fds and messages among processes.
> > >
> 
> Ok, will remove. 
> 
> > >>> +BSD nic_uio          =
> > >>> +Linux UIO            =
> > >> E.g. UIO, which cannot be used since vDPA requires an IOMMU.
> 
> Ok, will remove.
> 
> > >>> +Linux VFIO           =
> > >>> +Other kdrv           =
> > >>> +ARMv7                =
> > >>> +ARMv8                =
> > >>> +Power8               =
> > >>> +x86-32               =
> > >>> +x86-64               =
> > >>> +Usage doc            =
> > >>> +Design doc           =
> > >>> +Perf doc             =
> > >>> \ No newline at end of file
> > >>> diff --git a/doc/guides/vdpadevs/features_overview.rst
> > >>> b/doc/guides/vdpadevs/features_overview.rst
> > >>> new file mode 100644
> > >>> index 0000000..c7745b7
> > >>> --- /dev/null
> > >>> +++ b/doc/guides/vdpadevs/features_overview.rst
> > >>> @@ -0,0 +1,65 @@
> > >>> +..  SPDX-License-Identifier: BSD-3-Clause
> > >>> +    Copyright 2019 Mellanox Technologies, Ltd
> > >>> +
> > >>> +Overview of vDPA drivers features
> > >>> +=================================
> > >>> +
> > >>> +This section explains the supported features that are listed in the table
> > below.
> > >>> +
> > >>> +  * csum - Device can handle packets with partial checksum.
> > >>> +  * guest csum - Guest can handle packets with partial checksum.
> > >>> +  * mac - Device has given MAC address.
> > >>> +  * gso - Device can handle packets with any GSO type.
> > >>> +  * guest tso4 - Guest can receive TSOv4.
> > >>> +  * guest tso6 - Guest can receive TSOv6.
> > >>> +  * ecn - Device can receive TSO with ECN.
> > >>> +  * ufo - Device can receive UFO.
> > >>> +  * host tso4 - Device can receive TSOv4.
> > >>> +  * host tso6 - Device can receive TSOv6.
> > >>> +  * mrg rxbuf - Guest can merge receive buffers.
> > >>> +  * ctrl vq - Control channel is available.
> > >>> +  * ctrl rx - Control channel RX mode support.
> > >>> +  * any layout - Device can handle any descriptor layout.
> > >>> +  * guest announce - Guest can send gratuitous packets.
> > >>> +  * mq - Device supports Receive Flow Steering.
> > >>> +  * version 1 - v1.0 compliant.
> > >>> +  * log all - Device can log all write descriptors (live migration).
> > >>> +  * protocol features - Protocol features negotiation support.
> > >>> +  * indirect desc - Indirect buffer descriptors support.
> > >>> +  * event idx - Support for avail_idx and used_idx fields.
> > >>> +  * mtu - Host can advise the guest with its maximum supported MTU.
> > >>> +  * in_order - Device can use descriptors in ring order.
> > >>> +  * IOMMU platform - Device support IOMMU addresses.
> > >>> +  * packed - Device support packed virtio queues.
> > >>> +  * proto mq - Support the number of queues query.
> > >>> +  * proto log shmfd - Guest support setting log base.
> > >>> +  * proto rarp - Host can broadcast a fake RARP after live migration.
> > >>> +  * proto reply ack - Host support requested operation status ack.
> > >>> +  * proto slave req - Allow the slave to make requests to the master.
> > >>> +  * proto crypto session - Support crypto session creation.
> > >>> +  * proto host notifier - Host can register memory region based host
> > notifiers.
> > >>> +  * proto pagefault - Slave expose page-fault FD for migration process.
> > >>> +  * Multiprocess aware - Driver can be used for primary-secondary
> > process model.
> > >>> +  * BSD nic_uio - BSD ``nic_uio`` module supported.
> > >>> +  * Linux UIO - Works with ``igb_uio`` kernel module.
> > >>> +  * Linux VFIO - Works with ``vfio-pci`` kernel module.
> > >>> +  * Other kdrv - Kernel module other than above ones supported.
> > >>> +  * ARMv7 - Support armv7 architecture.
> > >>> +  * ARMv8 - Support armv8a (64bit) architecture.
> > >>> +  * Power8 - Support PowerPC architecture.
> > >>> +  * x86-32 - Support 32bits x86 architecture.
> > >>> +  * x86-64 - Support 64bits x86 architecture.
> > >>> +  * Usage doc - Documentation describes usage, In
> > ``doc/guides/vdpadevs/``.
> > >>> +  * Design doc - Documentation describes design. In
> > ``doc/guides/vdpadevs/``.
> > >>> +  * Perf doc - Documentation describes performance values, In
> > ``doc/perf/``.
> > 
> > Are you going to put Y mark for all these features in v20.02 release cycle?
> 
> No.
> 
> > Basically the question is: is it OK to have features that no driver supports?
> > "Dead" features do not look nice.
> > I would say yes for architecture support since it is better to list all
> > architectures supported by DPDK and make it clear which architectures are
> > supported by particular vDPA driver.
> > May be it is OK for features which are directly correspond to virtio/vhost
> > features (of course, it is very useful to know spec compliance).
> > In this case I think it would be very helpful to add references to spec
> > sections.
> 
> These features are supported in vhost library. So I think it may sense to add them.
> I think, especially in vDPA case, It is good for the vhost users to see what is supported and what is not supported by the vDPA driver.
> 
> Which spec are you talking about?
> 
> > Also I like doc/guides/nics/features.rst and would like to know why the
> > practice is not used here. I'm talking about features description format.
> 
> Yes, but except nic class, no class uses it.
> Maybe it is because there are not a lot of different ways to add the configuration\capability.
> Here, for example, most of them are just in feature bitmap.
>  
> > [snip]
>