[v4,02/44] bus/vdev: add driver IOVA VA mode requirement
Checks
Commit Message
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, and adds a check right before the device is
probed to ensure compatible IOVA mode has been selected.
It also adds a ABI exception rule to accommodate with an
update on the driver registration API
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
devtools/libabigail.abignore | 2 ++
drivers/bus/vdev/rte_bus_vdev.h | 4 ++++
drivers/bus/vdev/vdev.c | 29 +++++++++++++++++++++++++++++
3 files changed, 35 insertions(+)
Comments
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, January 26, 2021 6:16 PM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com;
> amorenoz@redhat.com; david.marchand@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH v4 02/44] bus/vdev: add driver IOVA VA mode requirement
>
> 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, and adds a check right before the device is
> probed to ensure compatible IOVA mode has been selected.
>
> It also adds a ABI exception rule to accommodate with an
> update on the driver registration API
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> devtools/libabigail.abignore | 2 ++
> drivers/bus/vdev/rte_bus_vdev.h | 4 ++++
> drivers/bus/vdev/vdev.c | 29 +++++++++++++++++++++++++++++
> 3 files changed, 35 insertions(+)
>
> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> index 1dc84fa74b..170304c876 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]
> diff --git a/drivers/bus/vdev/rte_bus_vdev.h b/drivers/bus/vdev/rte_bus_vdev.h
> index f99a41f825..fc315d10fa 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_*. */
> };
>
> +/** 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..9a673347ae 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,25 @@ 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 (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 +631,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
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
On Tue, Jan 26, 2021 at 11:16 AM 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, and adds a check right before the device is
> probed to ensure compatible IOVA mode has been selected.
>
> It also adds a ABI exception rule to accommodate with an
> update on the driver registration API
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> devtools/libabigail.abignore | 2 ++
> drivers/bus/vdev/rte_bus_vdev.h | 4 ++++
> drivers/bus/vdev/vdev.c | 29 +++++++++++++++++++++++++++++
> 3 files changed, 35 insertions(+)
>
> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> index 1dc84fa74b..170304c876 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]
Ray,
Are you okay with this exception?
Thanks.
On 26/01/2021 12:50, David Marchand wrote:
> On Tue, Jan 26, 2021 at 11:16 AM 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, and adds a check right before the device is
>> probed to ensure compatible IOVA mode has been selected.
>>
>> It also adds a ABI exception rule to accommodate with an
>> update on the driver registration API
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>> ---
>> devtools/libabigail.abignore | 2 ++
>> drivers/bus/vdev/rte_bus_vdev.h | 4 ++++
>> drivers/bus/vdev/vdev.c | 29 +++++++++++++++++++++++++++++
>> 3 files changed, 35 insertions(+)
>>
>> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
>> index 1dc84fa74b..170304c876 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]
>
> Ray,
> Are you okay with this exception?
Ask a perhaps silly question,
shouldn't rte_vdev_register & rte_vdev_unregister have been INTERNAL in any case?
> Thanks.
>
Ray K
On Tue, Jan 26, 2021 at 2:23 PM Kinsella, Ray <mdr@ashroe.eu> wrote:
> >> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> >> index 1dc84fa74b..170304c876 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]
> >
> > Ray,
> > Are you okay with this exception?
>
> Ask a perhaps silly question,
> shouldn't rte_vdev_register & rte_vdev_unregister have been INTERNAL in any case?
I discussed with Thomas earlier.
The INTERNAL exception rule we have suppresses changes on symbols
already versioned INTERNAL.
If we mark these two symbols INTERNAL now, they are part of the stable
v21 ABI in any case.
libabigail will still complain about them disappearing.
$ abidiff --suppr
/home/dmarchan/dpdk/devtools/../devtools/libabigail.abignore
--no-added-syms --headers-dir1
/home/dmarchan/abi/v20.11/build-gcc-shared/usr/local/include
--headers-dir2 /home/dmarchan/builds/build-gcc-shared/install/usr/local/include
/home/dmarchan/abi/v20.11/build-gcc-shared/dump/librte_bus_vdev.dump
/home/dmarchan/builds/build-gcc-shared/install/dump/librte_bus_vdev.dump
Functions changes summary: 2 Removed, 0 Changed, 0 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
2 Removed functions:
[D] 'function void rte_vdev_register(rte_vdev_driver*)'
{rte_vdev_register@@DPDK_21}
[D] 'function void rte_vdev_unregister(rte_vdev_driver*)'
{rte_vdev_unregister@@DPDK_21}
We will need an exception in any case for them.
On 26/01/2021 14:40, David Marchand wrote:
> On Tue, Jan 26, 2021 at 2:23 PM Kinsella, Ray <mdr@ashroe.eu> wrote:
>>>> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
>>>> index 1dc84fa74b..170304c876 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]
>>>
>>> Ray,
>>> Are you okay with this exception?
>>
>> Ask a perhaps silly question,
>> shouldn't rte_vdev_register & rte_vdev_unregister have been INTERNAL in any case?
>
> I discussed with Thomas earlier.
>
> The INTERNAL exception rule we have suppresses changes on symbols
> already versioned INTERNAL.
> If we mark these two symbols INTERNAL now, they are part of the stable
> v21 ABI in any case.
> libabigail will still complain about them disappearing.
>
> $ abidiff --suppr
> /home/dmarchan/dpdk/devtools/../devtools/libabigail.abignore
> --no-added-syms --headers-dir1
> /home/dmarchan/abi/v20.11/build-gcc-shared/usr/local/include
> --headers-dir2 /home/dmarchan/builds/build-gcc-shared/install/usr/local/include
> /home/dmarchan/abi/v20.11/build-gcc-shared/dump/librte_bus_vdev.dump
> /home/dmarchan/builds/build-gcc-shared/install/dump/librte_bus_vdev.dump
> Functions changes summary: 2 Removed, 0 Changed, 0 Added functions
> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
>
> 2 Removed functions:
>
> [D] 'function void rte_vdev_register(rte_vdev_driver*)'
> {rte_vdev_register@@DPDK_21}
> [D] 'function void rte_vdev_unregister(rte_vdev_driver*)'
> {rte_vdev_unregister@@DPDK_21}
>
> We will need an exception in any case for them.
>
Agreed, I didn't miss that are still going to need the exception.
If we agree that everything that is in rte_vdev_bus should be internal.
We can also fix that, while we are aware of it.
The rule above gets my +1 and I will fix rte_vdev_bus.
On Tue, Jan 26, 2021 at 11:16 AM 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, and adds a check right before the device is
> probed to ensure compatible IOVA mode has been selected.
>
> It also adds a ABI exception rule to accommodate with an
> update on the driver registration API
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
I only suggested some changes.
This patch looks good to me.
Can you change this Sob as a Acked-by?
Thanks Maxime.
On 1/27/21 9:23 AM, David Marchand wrote:
> On Tue, Jan 26, 2021 at 11:16 AM 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, and adds a check right before the device is
>> probed to ensure compatible IOVA mode has been selected.
>>
>> It also adds a ABI exception rule to accommodate with an
>> update on the driver registration API
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>
> I only suggested some changes.
> This patch looks good to me.
> Can you change this Sob as a Acked-by?
Sure, I can do that.
Thanks!
Maxime
> Thanks Maxime.
>
@@ -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]
@@ -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_*. */
};
+/** 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.
*
@@ -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,25 @@ 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 (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 +631,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,
};