Re: [dpdk-dev] [PATCH] bus/pci: fix IOVA as VA mode selection

Re: [dpdk-dev] [PATCH] bus/pci: fix IOVA as VA mode selection

[Jerin Jacob Kollanukkaran]

Not sure if it is problem from my email client or David email settings, I am getting the David email ONLY as HTML.
And outlook creating format issues when change to plain text on reply.
It looks like it is due to "Content-Type: multipart/alternative".

Please find inline reply.


From: David Marchand <david.marchand@redhat.com> 
Sent: Wednesday, July 10, 2019 1:40 PM
To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Burakov, Anatoly <anatoly.burakov@intel.com>
Cc: dev <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ben Walker <benjamin.walker@intel.com>
Subject: Re: [EXT] Re: [dpdk-dev] [PATCH] bus/pci: fix IOVA as VA mode selection

Hello guys,


> 
> Currently (again, disregarding your interpretation of how IOVA as VA works
> and looking at the actual commit history), we always seem to imply that IOVA
> as PA works for all devices, and we use IOVA_AS_VA flag to indicate that the
> device *also* supports IOVA as VA mode.
> 
> But we don't have any way to express a *requirement* for IOVA as VA mode
> - only for IOVA as PA mode. That is the purpose of the new flag. You are
> stating that the IOVA_AS_VA drv flag is an expression of that requirement,
> but that is not reflected in the codebase - our commit history indicates that
> we don't treat IOVA as VA as hard requirement whenever this flag is
> specified (and i would argue that we shouldn't).

> No objection to further classify it.

I propose to introduce:
* RTE_PCI_DRV_IOVA_AS_PA which means "the combination of the pmd+kmod+hw supports usage of Physical Addresses"
* RTE_PCI_DRV_IOVA_AS_VA which means "the combination of the pmd+kmod+hw supports usage of Virtual Addresses"

- For the pci bus, the algorigthm would be:

devices_want_pa = false
devices_want_va = false

Foreach pci device
  Skip blacklisted devices
  Skip unbound devices (i.e. we only consider devices bound to a known kernel driver)
  Skip unsupported devices (i.e. we only consider devices that have a pmd that supports them)

  If the combination pmd+kmod only supports VA (RTE_PCI_DRV_IOVA_AS_VA capability in driver flags), then devices_want_va = true
  Else if the combination pmd+kmod only supports PA (RTE_PCI_DRV_IOVA_AS_PA capability in driver flags), then devices_want_pa = true

If devices_want_va and !devices_want_pa
  return RTE_IOVA_VA
If devices_want_pa and !devices_want_va
  return RTE_IOVA_PA

return RTE_IOVA_DC

---------------
[Jerin] I am fine with introducing RTE_PCI_DRV_IOVA_AS_PA instead of RTE_PCI_DRV_IOVA_AS_DC(As I proposed earlier).
Only my concern is there may not be any PCIe device which is hitting the following.

If devices_want_pa and !devices_want_va
  return RTE_IOVA_PA

Assuming for i40e etc, You will change to RTE_PCI_DRV_IOVA_AS_PA | RTE_PCI_DRV_IOVA_AS_VA.
No strong option on RTE_PCI_DRV_IOVA_AS_PA vs RTE_PCI_DRV_IOVA_AS_DC.

---------------
Notes:
* the IOMMU limitations are considered as a per device/driver thing, since the kmod is the one that configures the system IOMMU,
* the case "devices_want_pa and devices_want_va" is considered as DC, we leave EAL decide based on the physical addresses availability because we can't comply with all present devices/drivers in the system.
  This means that at bus probe time for a device, we must add a check that the combination is fulfilled (and avoid this check in the drivers themselves).


- For the global bus code, that aggregates the different buses preferences, we need to do the same, while I suspect a bug at the moment.

The algorigthm:

buses_want_pa = false
buses_want_va = false

Foreach bus
  If the bus reports RTE_IOVA_VA, then buses_want_va = true
  Else if the bus reports RTE_IOVA_PA, then buses_want_pa = true

If buses_want_va and !buses_want_pa
  return RTE_IOVA_VA
If buses_want_pa and !buses_want_va
  return RTE_IOVA_PA

return RTE_IOVA_DC

- Finally at EAL level, we keep the current code.

----------------------------------

[Jerin] Algorithm look OK to me.  All of the following devices[1] added RTE_PCI_DRV_IOVA_AS_VA in the driver list
to run in VA mode to enable DPDK to run with out root privilege. But due to recent change it making as PA i.e
need root privilege to run.

May it is a separate topic to dicuss what would be default if the system has IOMMU and device is RTE_PCI_DRV_IOVA_AS_VA | RTE_PCI_DRV_IOVA_AS_PA.
I would say RTE_IOVA_VA. But I don’t know why it changed to RTE_IOVA_PA for hotplug or SPDK?


[1] http://patchwork.dpdk.org/patch/53206/
[2] http://patchwork.dpdk.org/patch/50274/
[3] http://patchwork.dpdk.org/patch/50991/
[4] http://patchwork.dpdk.org/patch/46134/


-----------------------------------

Hope I did not miss anything.
If we agree on this, I will send the changes and an update in the documentation.


-- 
David Marchand

Re: [dpdk-dev] [PATCH] bus/pci: fix IOVA as VA mode selection

[David Marchand]

On Mon, Jul 8, 2019 at 4:25 PM <jerinj@marvell.com> wrote:

> From: Jerin Jacob <jerinj@marvell.com>
>
> Existing logic fails to select IOVA mode as VA
> if driver request to enable IOVA as VA.
>
> IOVA as VA has more strict requirement than other modes,
> so enabling positive logic for IOVA as VA selection.
>
> This patch also updates the default IOVA mode as PA
> for PCI devices as it has to deal with DMA engines unlike
> the virtual devices that may need only IOVA as DC.
>

We have three cases:
- driver/hw supports IOVA as PA only
- driver/hw supports IOVA as both VA and PA
- driver/hw supports IOVA as VA only

Looking at the header:
/** Device driver supports IOVA as VA */
#define RTE_PCI_DRV_IOVA_AS_VA 0X0040

It clearly states that the driver supports IOVA as VA, and not requires.
With only one flag, we can't decide on those three cases and we might want
to introduce a new flag for your usecase.

But, in any case, this is too short for -rc1.
I prefer to nak this patch for now.
You can still set --iova-mode=va and we can try to find a solution for -rc2.


-- 
David Marchand

[dpdk-dev] [PATCH] bus/pci: fix IOVA as VA mode selection

[jerinj]

From: Jerin Jacob <jerinj@marvell.com>

Existing logic fails to select IOVA mode as VA
if driver request to enable IOVA as VA.

IOVA as VA has more strict requirement than other modes,
so enabling positive logic for IOVA as VA selection.

This patch also updates the default IOVA mode as PA
for PCI devices as it has to deal with DMA engines unlike
the virtual devices that may need only IOVA as DC.

Fixes: 703458e19c16 ("bus/pci: consider only usable devices for IOVA mode")

Signed-off-by: Jerin Jacob <jerinj@marvell.com>
---

If system has only IOVA as VA devices, with out this patch none of the
devices works on top of tree now. Request to review and close it for RC1.

---
 drivers/bus/pci/linux/pci.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 33c8ea7e9..99636831e 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -567,7 +567,7 @@ enum rte_iova_mode
 pci_device_iova_mode(const struct rte_pci_driver *pdrv,
 		     const struct rte_pci_device *pdev)
 {
-	enum rte_iova_mode iova_mode = RTE_IOVA_DC;
+	enum rte_iova_mode iova_mode = RTE_IOVA_PA;
 	static int iommu_no_va = -1;
 
 	switch (pdev->kdrv) {
@@ -581,8 +581,8 @@ pci_device_iova_mode(const struct rte_pci_driver *pdrv,
 			else
 				is_vfio_noiommu_enabled = 0;
 		}
-		if ((pdrv->drv_flags & RTE_PCI_DRV_IOVA_AS_VA) == 0) {
-			iova_mode = RTE_IOVA_PA;
+		if (pdrv->drv_flags & RTE_PCI_DRV_IOVA_AS_VA) {
+			iova_mode = RTE_IOVA_VA;
 		} else if (is_vfio_noiommu_enabled != 0) {
 			RTE_LOG(DEBUG, EAL, "Forcing to 'PA', vfio-noiommu mode configured\n");
 			iova_mode = RTE_IOVA_PA;
@@ -592,8 +592,8 @@ pci_device_iova_mode(const struct rte_pci_driver *pdrv,
 	}
 
 	case RTE_KDRV_NIC_MLX:
-		if ((pdrv->drv_flags & RTE_PCI_DRV_IOVA_AS_VA) == 0)
-			iova_mode = RTE_IOVA_PA;
+		if (pdrv->drv_flags & RTE_PCI_DRV_IOVA_AS_VA)
+			iova_mode = RTE_IOVA_VA;
 		break;
 
 	case RTE_KDRV_IGB_UIO:
-- 
2.22.0