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

[Matan Azrad <matan@mellanox.com>]

From: Tiwei Bie
> 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.
> 

Ok, will remove.

> > > >>> +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.


Ok, will remove.

Thanks.