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

[Jerin Jacob Kollanukkaran <jerinj@marvell.com>]

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Tuesday, July 9, 2019 8:07 PM
> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; David Marchand
> <david.marchand@redhat.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
issue.
> >>
> >> I wouldn't classify this as "needing" IOVA. "Need" implies it cannot
> >> work without it, whereas in this case it's more of a "highly
> >> recommended" rather than "need".
> >
> > It is "need" as performance is horrible without it as is per packet SW
> translation.
> > A "need" for DPDK performance perspective.
> 
> Would the driver fail to initialize if it detects running as IOVA as PA?

Yes.
https://git.dpdk.org/dpdk/tree/drivers/net/octeontx2/otx2_ethdev.c#n1191

> Also, some other use cases will also require IOVA as PA while having full
> IOMMU support. An example of this would be systems with limited IOMMU
> width (such as VM's) - even though the IOMMU is technically supported, we
> may not have the necessary address width to run all devices in IOVA as VA
> mode, and would need to fall back to IOVA as PA.
> Since we cannot *require* IOVA as VA in current codebase, any driver that
> expects IOVA as VA to always be enabled will presumably not work.
> 
> >
> > Again, it is not device attribute, it is system attribute.
> 
> If it's a system attribute, why is it a device driver flag then? The system may
> or may not support IOMMU, the device itself probably doesn't care since bus
> address looks the same in both cases, *but the driver
> might* (such as would be in your case - requiring IOVA as VA and disallowing
> IOVA as PA for performance reasons).

Agree.

> 
> 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.

How about the following

1) Change RTE_PCI_DRV_IOVA_AS_VA as RTE_PCI_DRV_IOVA_AS_DC
It is same as existing RTE_PCI_DRV_IOVA_AS_VA. Meaning driver don't care IOVA as PA or VA.
2) Introduce RTE_PCI_DRV_NEED_IOVA_AS_VA(Driver needs IOVA as VA)
This would selected for octeontx device "drivers"
3) Change existing driver's "drv_flags" as RTE_PCI_DRV_IOVA_AS_DC if it can work
with PA and VA(literally all exiting drivers which currently has RTE_PCI_DRV_IOVA_AS_VA excluding the octeontx drivers)

In pci_device_iova_mode()

if (drv->flags & RTE_PCI_DRV_IOVA_AS_DC)
	iova_mode = RTE_IOVA_DC;
else if (drv->flags & RTE_PCI_DRV_NEED_IOVA_AS_VA)
	iova_mode = RTE_IOVA_VA;
else
	iova_mode = RTE_IOVA_PA;

I can submit the patch if above is OK.

> >>
> >>> # With top of tree, Currently it never runs in IOVA as VA mode.
> >>> That’s a separate problem to fix. Which effect all the devices
> >>> Currently supporting RTE_PCI_DRV_IOVA_AS_VA. Ie even though
> Device
> >>> support RTE_PCI_DRV_IOVA_AS_VA, it is not running With IOMMU
> >>> protection and/or root privilege is required to run DPDK.
> >
> > What's your view on this existing problem?
> 
> My view would be to always run in IOVA as VA by default and only falling
> back to IOVA as PA if there is a need to do that. Yet, it seems that whenever i
> try to bring this up, the response (not necessarily from you, so this is not
> directed at you specifically) seems to be that because of hotplug, we have to
> start in the "safest" (from device support point of
> view) mode - that is, in IOVA as PA. Seeing how, as you claim, some devices
> require IOVA as VA, then IOVA as PA is no longer the "safe"
> default that all devices will support. Perhaps we can use this opportunity to
> finally make IOVA as VA the default :)

I was thinking to use VA as default if system/device/driver supports.
That’s the reason for the original patch to have VA selection in
pci code itself. But it makes sense to move that up.

Not related to this patch, Why hotplug prefers IOVA as PA? Now, If I understand it correctly, to accommodate
Hotplug for SPDK the pci_device_iova_mode() made it as DC so that
common code can pick PA.

I have no strong opinion on this, if it help for SPDK then no issue in keeping
default as PA in case of DC.