Re: [dpdk-dev] [PATCH v2 02/44] bus/vdev: add driver IOVA VA mode requirement

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 1/20/21 4:32 PM, David Marchand wrote:
> On Tue, Jan 19, 2021 at 10:25 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> This patch adds driver flag in vdev bus driver so that
>> vdev drivers can require VA IOVA mode to be used, which
>> for example the case of Virtio-user PMD.
>>
>> The patch implements the .get_iommu_class() callback, that
>> is called before devices probing to determine the IOVA mode
>> to be used.
>>
>> It also adds a check right before the device is probed to
>> ensure compatible IOVa mode has been selected.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  drivers/bus/vdev/rte_bus_vdev.h |  4 ++++
>>  drivers/bus/vdev/vdev.c         | 31 +++++++++++++++++++++++++++++++
>>  2 files changed, 35 insertions(+)
>>
>> diff --git a/drivers/bus/vdev/rte_bus_vdev.h b/drivers/bus/vdev/rte_bus_vdev.h
>> index f99a41f825..c8b41e649c 100644
>> --- a/drivers/bus/vdev/rte_bus_vdev.h
>> +++ b/drivers/bus/vdev/rte_bus_vdev.h
>> @@ -113,8 +113,12 @@ struct rte_vdev_driver {
>>         rte_vdev_remove_t *remove;       /**< Virtual device remove function. */
>>         rte_vdev_dma_map_t *dma_map;     /**< Virtual device DMA map function. */
>>         rte_vdev_dma_unmap_t *dma_unmap; /**< Virtual device DMA unmap function. */
>> +       uint32_t drv_flags;                /**< Flags RTE_VDEV_DRV_*. */
> 
> This will probably get broken in the future, but for now, can you
> indent the comment in the same way as earlier lines?
> 
> 
> The ABI check will complain about this change so we need an exception.
> 
> rte_vdev_driver is exposed only through driver API.
> We could flag the whole structure like we did for ethdev.
> But there is also the alternative of just flagging the required
> symbols so that we won't miss later the inclusion of this structure in
> an API used by final users.
> How about:
> 
> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> index 1dc84fa74b..435913d908 100644
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> @@ -11,6 +11,8 @@
>  ; Explicit ignore for driver-only ABI
>  [suppress_type]
>          name = eth_dev_ops
> +[suppress_function]
> +        name_regexp = rte_vdev_(|un)register
> 
>  ; Ignore fields inserted in cacheline boundary of rte_cryptodev
>  [suppress_type]
> 
> 

This is fine by me.

>>  };
>>
>> +/** Device driver needs IOVA as VA and cannot work with IOVA as PA */
>> +#define RTE_VDEV_DRV_NEED_IOVA_AS_VA 0x0001
>> +
>>  /**
>>   * Register a virtual device driver.
>>   *
>> diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
>> index acfd78828f..56f15e8201 100644
>> --- a/drivers/bus/vdev/vdev.c
>> +++ b/drivers/bus/vdev/vdev.c
>> @@ -189,6 +189,7 @@ vdev_probe_all_drivers(struct rte_vdev_device *dev)
>>  {
>>         const char *name;
>>         struct rte_vdev_driver *driver;
>> +       enum rte_iova_mode iova_mode;
>>         int ret;
>>
>>         if (rte_dev_is_probed(&dev->device))
>> @@ -199,6 +200,14 @@ vdev_probe_all_drivers(struct rte_vdev_device *dev)
>>
>>         if (vdev_parse(name, &driver))
>>                 return -1;
>> +
>> +       iova_mode = rte_eal_iova_mode();
>> +       if ((driver->drv_flags & RTE_VDEV_DRV_NEED_IOVA_AS_VA) && (iova_mode == RTE_IOVA_PA)) {
>> +               VDEV_LOG(ERR, "%s requires VA IOVA mode but current mode is PA, not initializing",
>> +                               name);
>> +               return -1;
>> +       }
>> +
>>         ret = driver->probe(dev);
>>         if (ret == 0)
>>                 dev->device.driver = &driver->driver;
>> @@ -594,6 +603,27 @@ vdev_unplug(struct rte_device *dev)
>>         return rte_vdev_uninit(dev->name);
>>  }
>>
>> +static enum rte_iova_mode
>> +vdev_get_iommu_class(void)
>> +{
>> +       const char *name;
>> +       struct rte_vdev_device *dev;
>> +       struct rte_vdev_driver *driver;
>> +
>> +       TAILQ_FOREACH(dev, &vdev_device_list, next) {
>> +               name = rte_vdev_device_name(dev);
>> +               if (!name)
>> +                       continue;
> 
> Afaics, a device in vdev_device_list always has a name.

Indeed, I will remove the check in next revision.

Thanks,
Maxime

> 
>> +               if (vdev_parse(name, &driver))
>> +                       continue;
>> +
>> +               if (driver->drv_flags & RTE_VDEV_DRV_NEED_IOVA_AS_VA)
>> +                       return RTE_IOVA_VA;
>> +       }
>> +
>> +       return RTE_IOVA_DC;
>> +}
>> +
>>  static struct rte_bus rte_vdev_bus = {
>>         .scan = vdev_scan,
>>         .probe = vdev_probe,
>> @@ -603,6 +633,7 @@ static struct rte_bus rte_vdev_bus = {
>>         .parse = vdev_parse,
>>         .dma_map = vdev_dma_map,
>>         .dma_unmap = vdev_dma_unmap,
>> +       .get_iommu_class = vdev_get_iommu_class,
>>         .dev_iterate = rte_vdev_dev_iterate,
>>  };
>>
>> --
>> 2.29.2
>>
> 
>