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 <ma...@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]
>
