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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 1/8/20 1:42 PM, 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?
> 
>>>>> +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?
> 
>>>>> +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?

Vhost user protocol specification and virtio specification
(since some features directly correspond to virtio features).

[1]
https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html
[2] https://qemu.weilnetz.de/doc/interop/vhost-user.html

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

IMO it is one of best practices in DPDK which should be used
by other classes as well.

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

More formal description simplifies search and helps driver
developers and maintainers a lot.

>> [snip]
>