Re: [dpdk-dev] [PATCH v12 1/2] kni: add IOVA=VA mode support

[David Marchand <david.marchand@redhat.com>]

On Tue, Nov 5, 2019 at 12:05 PM <vattunuru@marvell.com> wrote:
>
> From: Vamsi Attunuru <vattunuru@marvell.com>
>
> Current KNI implementation only operates in IOVA_PA mode
> patch adds required functionality to enable KNI in
> IOVA_VA mode.
>
> KNI can operate in IOVA_VA mode when this mode is requested
> using eal option (`iova-mode=va`) or when bus iommu scheme is
> selected as RTE_IOVA_VA.
>
> During KNI creation, app's iova_mode details are passed to
> the KNI kernel module, accordingly kernel module translates
> PA/IOVA addresses to KVA and vice-versa.
>
> Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
> Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  doc/guides/prog_guide/kernel_nic_interface.rst    |  9 +++++++
>  doc/guides/rel_notes/release_19_11.rst            |  5 ++++
>  lib/librte_eal/linux/eal/eal.c                    | 29 +++++++++++++----------
>  lib/librte_eal/linux/eal/include/rte_kni_common.h |  1 +
>  lib/librte_kni/rte_kni.c                          |  7 ++----
>  5 files changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/doc/guides/prog_guide/kernel_nic_interface.rst b/doc/guides/prog_guide/kernel_nic_interface.rst
> index 2fd58e1..f869493 100644
> --- a/doc/guides/prog_guide/kernel_nic_interface.rst
> +++ b/doc/guides/prog_guide/kernel_nic_interface.rst
> @@ -300,6 +300,15 @@ The sk_buff is then freed and the mbuf sent in the tx_q FIFO.
>  The DPDK TX thread dequeues the mbuf and sends it to the PMD via ``rte_eth_tx_burst()``.
>  It then puts the mbuf back in the cache.
>
> +IOVA = VA: Support
> +------------------
> +
> +KNI operates in IOVA_VA scheme when
> +
> +- LINUX_VERSION_CODE >= KERNEL_VERSION(4, 6, 0) and
> +- eal option `iova-mode=va` is passed or bus IOVA scheme in the DPDK is selected
> +  as RTE_IOVA_VA.
> +
>  Ethtool
>  -------
>
> diff --git a/doc/guides/rel_notes/release_19_11.rst b/doc/guides/rel_notes/release_19_11.rst
> index ae8e7b2..e5d2996 100644
> --- a/doc/guides/rel_notes/release_19_11.rst
> +++ b/doc/guides/rel_notes/release_19_11.rst
> @@ -231,6 +231,11 @@ New Features
>    * Added a console command to testpmd app, ``show port (port_id) ptypes`` which
>      gives ability to print port supported ptypes in different protocol layers.
>
> +* **Added IOVA as VA support for KNI.**
> +
> +  Added IOVA = VA support for KNI, KNI can operate in IOVA = VA mode when
> +  `iova-mode=va` eal option is passed to the application or when bus IOVA
> +  scheme is selected as RTE_IOVA_VA.

Add a mention about the kernel requirement here too please.

Missing newline.

>
>  Removed Items
>  -------------
> diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c
> index 9e2d50c..a1c5bf6 100644
> --- a/lib/librte_eal/linux/eal/eal.c
> +++ b/lib/librte_eal/linux/eal/eal.c
> @@ -922,6 +922,19 @@ static int rte_eal_vfio_setup(void)
>  }
>  #endif
>
> +static enum rte_iova_mode
> +rte_eal_kni_get_iova_mode(enum rte_iova_mode iova_mode)
> +{
> +       if (iova_mode == RTE_IOVA_VA) {
> +#if KERNEL_VERSION(4, 6, 0) > LINUX_VERSION_CODE

I understood that IOVA as VA is possible with kernel >= 4.6.
Should it be < ?


> +               iova_mode = RTE_IOVA_PA;
> +               RTE_LOG(WARNING, EAL, "Forcing IOVA as 'PA' because KNI module does not support VA\n");
> +#endif
> +       }
> +
> +       return iova_mode;
> +}
> +
>  static void rte_eal_init_alert(const char *msg)
>  {
>         fprintf(stderr, "EAL: FATAL: %s\n", msg);
> @@ -1085,24 +1098,16 @@ rte_eal_init(int argc, char **argv)
>                                 RTE_LOG(DEBUG, EAL, "IOMMU is not available, selecting IOVA as PA mode.\n");
>                         }
>                 }
> -#ifdef RTE_LIBRTE_KNI

You removed this part, but we don't have to consider kni things if
RTE_LIBRTE_KNI is not compiled in.


> -               /* Workaround for KNI which requires physical address to work */
> -               if (iova_mode == RTE_IOVA_VA &&
> -                               rte_eal_check_module("rte_kni") == 1) {
> -                       if (phys_addrs) {

The check on physical address availability has been lost.


> -                               iova_mode = RTE_IOVA_PA;
> -                               RTE_LOG(WARNING, EAL, "Forcing IOVA as 'PA' because KNI module is loaded\n");
> -                       } else {
> -                               RTE_LOG(DEBUG, EAL, "KNI can not work since physical addresses are unavailable\n");
> -                       }
> -               }
> -#endif
>                 rte_eal_get_configuration()->iova_mode = iova_mode;
>         } else {
>                 rte_eal_get_configuration()->iova_mode =
>                         internal_config.iova_mode;
>         }
>
> +       if (rte_eal_check_module("rte_kni") == 1)
> +               rte_eal_get_configuration()->iova_mode =
> +                               rte_eal_kni_get_iova_mode(rte_eal_iova_mode());
> +

You are forcing this even if the user specified a iova mode.


In the end, can't we just change the check on the workaround?
Something like:

@@ -1085,7 +1085,7 @@ rte_eal_init(int argc, char **argv)
                                RTE_LOG(DEBUG, EAL, "IOMMU is not
available, selecting IOVA as PA mode.\n");
                        }
                }
-#ifdef RTE_LIBRTE_KNI
+#if defined(RTE_LIBRTE_KNI) && KERNEL_VERSION(4, 6, 0) < LINUX_VERSION_CODE
                /* Workaround for KNI which requires physical address to work */
                if (iova_mode == RTE_IOVA_VA &&
                                rte_eal_check_module("rte_kni") == 1) {


It would be the only change in the whole file lib/librte_eal/linux/eal/eal.c


>         if (rte_eal_iova_mode() == RTE_IOVA_PA && !phys_addrs) {
>                 rte_eal_init_alert("Cannot use IOVA as 'PA' since physical addresses are not available");
>                 rte_errno = EINVAL;
> diff --git a/lib/librte_eal/linux/eal/include/rte_kni_common.h b/lib/librte_eal/linux/eal/include/rte_kni_common.h
> index 46f75a7..2427a96 100644
> --- a/lib/librte_eal/linux/eal/include/rte_kni_common.h
> +++ b/lib/librte_eal/linux/eal/include/rte_kni_common.h
> @@ -125,6 +125,7 @@ struct rte_kni_device_info {
>         unsigned int min_mtu;
>         unsigned int max_mtu;
>         uint8_t mac_addr[6];
> +       uint8_t iova_mode;
>  };
>
>  #define KNI_DEVICE "kni"
> diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
> index 7fbcf22..7221280 100644
> --- a/lib/librte_kni/rte_kni.c
> +++ b/lib/librte_kni/rte_kni.c
> @@ -97,11 +97,6 @@ static volatile int kni_fd = -1;
>  int
>  rte_kni_init(unsigned int max_kni_ifaces __rte_unused)
>  {

Should this check still apply with kernel < 4.6?
Like:
+ #if KERNEL_VERSION(4, 6, 0) < LINUX_VERSION_CODE
> -       if (rte_eal_iova_mode() != RTE_IOVA_PA) {
> -               RTE_LOG(ERR, KNI, "KNI requires IOVA as PA\n");
> -               return -1;
> -       }
> -
+ #endif


>         /* Check FD and open */
>         if (kni_fd < 0) {
>                 kni_fd = open("/dev/" KNI_DEVICE, O_RDWR);
> @@ -302,6 +297,8 @@ rte_kni_alloc(struct rte_mempool *pktmbuf_pool,
>         kni->group_id = conf->group_id;
>         kni->mbuf_size = conf->mbuf_size;
>
> +       dev_info.iova_mode = (rte_eal_iova_mode() == RTE_IOVA_VA) ? 1 : 0;
> +
>         ret = ioctl(kni_fd, RTE_KNI_IOCTL_CREATE, &dev_info);
>         if (ret < 0)
>                 goto ioctl_fail;
> --
> 2.8.4
>


-- 
David Marchand