[dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Santosh Shukla]

Adding RTE_KDRV_VFIO_NOIOMMU mode in kernel driver. Also including
rte_vfio_is_noiommu() helper function. This function will parse
/sys/bus/pci/device/<bus_addr>/ and make sure that
- vfio noiommu mode set in kernel driver
- pci device attached to vfio-noiommu driver only

If both condition satisfies then set drv->kdrv = RTE_KDRV_VFIO_NOIOMMU

Also did similar changes in virtio_rd/wr, Changes applicable for virtio spec
0.95 only.

Signed-off-by: Santosh Shukla <sshukla@mvista.com>
---
v5--> v6:
- Include pci_dev == NULL check in pci_vfio_is_noiommu(), suggested by Anatoly.

v4--> v5:
- Removed virtio_xx_init_by_vfio and added new driver mode.
- Now no need to parse vfio interface in virtio. As pci_eal module will take of
  vfio-noiommu driver parsing for virtio or any other future device willing to
  use vfio-noiommu driver.

 drivers/net/virtio/virtio_pci.c            |   12 ++---
 lib/librte_eal/common/include/rte_pci.h    |    1 +
 lib/librte_eal/linuxapp/eal/eal_pci.c      |   13 +++--
 lib/librte_eal/linuxapp/eal/eal_pci_init.h |    1 +
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   72 ++++++++++++++++++++++++++++
 5 files changed, 90 insertions(+), 9 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index 0c29f1d..537c552 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -60,7 +60,7 @@ virtio_read_reg_1(struct virtio_hw *hw, uint64_t reg_offset)
 	struct rte_pci_device *dev;
 
 	dev = hw->dev;
-	if (dev->kdrv == RTE_KDRV_VFIO)
+	if (dev->kdrv == RTE_KDRV_VFIO_NOIOMMU)
 		ioport_inb(dev, reg_offset, &ret);
 	else
 		ret = inb(VIRTIO_PCI_REG_ADDR(hw, reg_offset));
@@ -75,7 +75,7 @@ virtio_read_reg_2(struct virtio_hw *hw, uint64_t reg_offset)
 	struct rte_pci_device *dev;
 
 	dev = hw->dev;
-	if (dev->kdrv == RTE_KDRV_VFIO)
+	if (dev->kdrv == RTE_KDRV_VFIO_NOIOMMU)
 		ioport_inw(dev, reg_offset, &ret);
 	else
 		ret = inw(VIRTIO_PCI_REG_ADDR(hw, reg_offset));
@@ -90,7 +90,7 @@ virtio_read_reg_4(struct virtio_hw *hw, uint64_t reg_offset)
 	struct rte_pci_device *dev;
 
 	dev = hw->dev;
-	if (dev->kdrv == RTE_KDRV_VFIO)
+	if (dev->kdrv == RTE_KDRV_VFIO_NOIOMMU)
 		ioport_inl(dev, reg_offset, &ret);
 	else
 		ret = inl(VIRTIO_PCI_REG_ADDR(hw, reg_offset));
@@ -104,7 +104,7 @@ virtio_write_reg_1(struct virtio_hw *hw, uint64_t reg_offset, uint8_t value)
 	struct rte_pci_device *dev;
 
 	dev = hw->dev;
-	if (dev->kdrv == RTE_KDRV_VFIO)
+	if (dev->kdrv == RTE_KDRV_VFIO_NOIOMMU)
 		ioport_outb_p(dev, reg_offset, value);
 	else
 		outb_p((unsigned char)value,
@@ -117,7 +117,7 @@ virtio_write_reg_2(struct virtio_hw *hw, uint64_t reg_offset, uint16_t value)
 	struct rte_pci_device *dev;
 
 	dev = hw->dev;
-	if (dev->kdrv == RTE_KDRV_VFIO)
+	if (dev->kdrv == RTE_KDRV_VFIO_NOIOMMU)
 		ioport_outw_p(dev, reg_offset, value);
 	else
 		outw_p((unsigned short)value,
@@ -130,7 +130,7 @@ virtio_write_reg_4(struct virtio_hw *hw, uint64_t reg_offset, uint32_t value)
 	struct rte_pci_device *dev;
 
 	dev = hw->dev;
-	if (dev->kdrv == RTE_KDRV_VFIO)
+	if (dev->kdrv == RTE_KDRV_VFIO_NOIOMMU)
 		ioport_outl_p(dev, reg_offset, value);
 	else
 		outl_p((unsigned int)value,
diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
index 0c667ff..2dbc658 100644
--- a/lib/librte_eal/common/include/rte_pci.h
+++ b/lib/librte_eal/common/include/rte_pci.h
@@ -149,6 +149,7 @@ enum rte_kernel_driver {
 	RTE_KDRV_VFIO,
 	RTE_KDRV_UIO_GENERIC,
 	RTE_KDRV_NIC_UIO,
+	RTE_KDRV_VFIO_NOIOMMU,
 	RTE_KDRV_NONE,
 };
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index eb503f0..2936497 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -131,6 +131,7 @@ rte_eal_pci_map_device(struct rte_pci_device *dev)
 	/* try mapping the NIC resources using VFIO if it exists */
 	switch (dev->kdrv) {
 	case RTE_KDRV_VFIO:
+	case RTE_KDRV_VFIO_NOIOMMU:
 #ifdef VFIO_PRESENT
 		if (pci_vfio_is_enabled())
 			ret = pci_vfio_map_resource(dev);
@@ -158,6 +159,7 @@ rte_eal_pci_unmap_device(struct rte_pci_device *dev)
 	/* try unmapping the NIC resources using VFIO if it exists */
 	switch (dev->kdrv) {
 	case RTE_KDRV_VFIO:
+	case RTE_KDRV_VFIO_NOIOMMU:
 		RTE_LOG(ERR, EAL, "Hotplug doesn't support vfio yet\n");
 		break;
 	case RTE_KDRV_IGB_UIO:
@@ -353,9 +355,12 @@ pci_scan_one(const char *dirname, uint16_t domain, uint8_t bus,
 	}
 
 	if (!ret) {
-		if (!strcmp(driver, "vfio-pci"))
-			dev->kdrv = RTE_KDRV_VFIO;
-		else if (!strcmp(driver, "igb_uio"))
+		if (!strcmp(driver, "vfio-pci")) {
+			if (pci_vfio_is_noiommu(dev) == 0)
+				dev->kdrv = RTE_KDRV_VFIO_NOIOMMU;
+			else
+				dev->kdrv = RTE_KDRV_VFIO;
+		} else if (!strcmp(driver, "igb_uio"))
 			dev->kdrv = RTE_KDRV_IGB_UIO;
 		else if (!strcmp(driver, "uio_pci_generic"))
 			dev->kdrv = RTE_KDRV_UIO_GENERIC;
@@ -630,6 +635,7 @@ int rte_eal_pci_read_bar(const struct rte_pci_device *device,
 
 	switch (device->kdrv) {
 	case RTE_KDRV_VFIO:
+	case RTE_KDRV_VFIO_NOIOMMU:
 		return pci_vfio_read_bar(intr_handle, buf, len,
 					 offset, bar_idx);
 	default:
@@ -647,6 +653,7 @@ int rte_eal_pci_write_bar(const struct rte_pci_device *device,
 
 	switch (device->kdrv) {
 	case RTE_KDRV_VFIO:
+	case RTE_KDRV_VFIO_NOIOMMU:
 		return pci_vfio_write_bar(intr_handle, buf, len,
 					  offset, bar_idx);
 	default:
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_init.h b/lib/librte_eal/linuxapp/eal/eal_pci_init.h
index 3bc592b..60b95d7 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_init.h
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_init.h
@@ -60,6 +60,7 @@ int pci_uio_write_config(const struct rte_intr_handle *intr_handle,
 
 int pci_vfio_enable(void);
 int pci_vfio_is_enabled(void);
+int pci_vfio_is_noiommu(struct rte_pci_device *pci_dev);
 int pci_vfio_mp_sync_setup(void);
 
 /* access config space */
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index df407ef..33f8808 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -973,4 +973,76 @@ pci_vfio_is_enabled(void)
 {
 	return vfio_cfg.vfio_enabled;
 }
+
+int
+pci_vfio_is_noiommu(struct rte_pci_device *pci_dev)
+{
+	FILE *fp;
+	struct rte_pci_addr *loc;
+	const char *path = "/sys/module/vfio/parameters/enable_unsafe_noiommu_mode";
+	char filename[PATH_MAX] = {0};
+	char buf[PATH_MAX] = {0};
+
+	/*
+	 * 1. chk vfio-noiommu mode set in kernel driver
+	 * 2. verify pci device attached to vfio-noiommu driver
+	 * example:
+	 * cd /sys/bus/pci/drivers/vfio-pci/<virtio_dev_addr>/iommu_group
+	 * > cat name
+	 * > vfio-noiommu --> means virtio_dev attached to vfio-noiommu driver
+	 */
+
+	if (pci_dev == NULL)
+		return -1;
+
+	fp = fopen(path, "r");
+	if (fp == NULL) {
+		RTE_LOG(ERR, EAL, "can't open %s\n", path);
+		return -1;
+	}
+
+	if (fread(buf, sizeof(char), 1, fp) != 1) {
+		RTE_LOG(ERR, EAL, "can't read from file %s\n", path);
+		fclose(fp);
+		return -1;
+	}
+
+	if (strncmp(buf, "Y", 1) != 0) {
+		RTE_LOG(ERR, EAL, "[%s]: vfio: noiommu mode not set\n", path);
+		fclose(fp);
+		return -1;
+	}
+
+	fclose(fp);
+
+	/* 2. chk whether attached driver is vfio-noiommu or not */
+	loc = &pci_dev->addr;
+	snprintf(filename, sizeof(filename),
+		     SYSFS_PCI_DEVICES "/" PCI_PRI_FMT "/iommu_group/name",
+		     loc->domain, loc->bus, loc->devid, loc->function);
+
+	/* check for vfio-noiommu */
+	fp = fopen(filename, "r");
+	if (fp == NULL) {
+		RTE_LOG(ERR, EAL, "can't open %s\n", filename);
+		return -1;
+	}
+
+	if (fread(buf, sizeof(char), sizeof("vfio-noiommu"), fp) !=
+		  sizeof("vfio-noiommu")) {
+		RTE_LOG(ERR, EAL, "can't read from file %s\n", filename);
+		fclose(fp);
+		return -1;
+	}
+
+	if (strncmp(buf, "vfio-noiommu", strlen("vfio-noiommu")) != 0) {
+		RTE_LOG(ERR, EAL, "not a vfio-noiommu driver\n");
+		fclose(fp);
+		return -1;
+	}
+
+	fclose(fp);
+
+	return 0;
+}
 #endif
-- 
1.7.9.5

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[David Marchand]

Santosh,

On Tue, Jan 19, 2016 at 7:57 PM, Santosh Shukla <sshukla@mvista.com> wrote:
> Adding RTE_KDRV_VFIO_NOIOMMU mode in kernel driver. Also including
> rte_vfio_is_noiommu() helper function. This function will parse
> /sys/bus/pci/device/<bus_addr>/ and make sure that
> - vfio noiommu mode set in kernel driver
> - pci device attached to vfio-noiommu driver only
>
> If both condition satisfies then set drv->kdrv = RTE_KDRV_VFIO_NOIOMMU
>
> Also did similar changes in virtio_rd/wr, Changes applicable for virtio spec
> 0.95 only.

This is a mode (specific to vfio), not a new kernel driver.

How come we need to distinguish between with/without iommu modes ?
Should not vfio behave the same way from an api point of view ?


Regards,
-- 
David Marchand

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Santosh Shukla]

On Thu, Jan 21, 2016 at 4:02 PM, David Marchand <david.marchand@6wind.com>
wrote:

> Santosh,
>
> On Tue, Jan 19, 2016 at 7:57 PM, Santosh Shukla <sshukla@mvista.com>
> wrote:
> > Adding RTE_KDRV_VFIO_NOIOMMU mode in kernel driver. Also including
> > rte_vfio_is_noiommu() helper function. This function will parse
> > /sys/bus/pci/device/<bus_addr>/ and make sure that
> > - vfio noiommu mode set in kernel driver
> > - pci device attached to vfio-noiommu driver only
> >
> > If both condition satisfies then set drv->kdrv = RTE_KDRV_VFIO_NOIOMMU
> >
> > Also did similar changes in virtio_rd/wr, Changes applicable for virtio
> spec
> > 0.95 only.
>
> This is a mode (specific to vfio), not a new kernel driver.
>
>
Yes, Specific to VFIO and this is why noiommu appended after vfio i.e..
__VFIO and __VFIO_NOIOMMU.


> How come we need to distinguish between with/without iommu modes ?
>

By default vfio framework assumes iommu i.,e., iommu present. Unless user
explicitly set "enable_unsafe_noiommu_mode" param. so in my opinion, we
care to parse vfio driver for _noiommu_ mode only.


> Should not vfio behave the same way from an api point of view ?
>
>
Yes It should. vfio gives similar file_ops i.e.. read/write/mmap/seek etc..
I am little confused on your question, do you see any issue in vfio bar
rd/wr api implementation?


> Regards,
> --
> David Marchand
>

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Thomas Monjalon]

2016-01-21 16:43, Santosh Shukla:
> David Marchand <david.marchand@6wind.com> wrote:
> > This is a mode (specific to vfio), not a new kernel driver.
> >
> Yes, Specific to VFIO and this is why noiommu appended after vfio i.e..
> __VFIO and __VFIO_NOIOMMU.

Woaaa! Your logic is really disappointing :)
Specific to VFIO => append _NOIOMMU
If it's for VFIO, it should be called VFIO (that's my logic).

> > How come we need to distinguish between with/without iommu modes ?
> 
> By default vfio framework assumes iommu i.,e., iommu present. Unless user
> explicitly set "enable_unsafe_noiommu_mode" param. so in my opinion, we
> care to parse vfio driver for _noiommu_ mode only.

Why do we care to parse noiommu only?
Even if virtio cannot work in an IOMMU case, there is no reason to add
a VFIO_NOIOMMU type here.

> > Should not vfio behave the same way from an api point of view ?
> >
> Yes It should. vfio gives similar file_ops i.e.. read/write/mmap/seek etc..
> I am little confused on your question, do you see any issue in vfio bar
> rd/wr api implementation?

I think you should just consider the VFIO API and let the noiommu option
as a kernel configuration detail.

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Santosh Shukla]

On Thu, Jan 21, 2016 at 4:58 PM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> 2016-01-21 16:43, Santosh Shukla:
>> David Marchand <david.marchand@6wind.com> wrote:
>> > This is a mode (specific to vfio), not a new kernel driver.
>> >
>> Yes, Specific to VFIO and this is why noiommu appended after vfio i.e..
>> __VFIO and __VFIO_NOIOMMU.
>
> Woaaa! Your logic is really disappointing :)
> Specific to VFIO => append _NOIOMMU
> If it's for VFIO, it should be called VFIO (that's my logic).
>
I am confused by reading your comment. vfio works for default iommu
and now with noiommu. drv->kdrv need to know driver mode for vfio
case. So that user can simply read drv->kdrv value in their driver and
accordingly use vfio rd/wr api for example {pread/pwrite}. This is how
rte_eal_pci_vfio_read/write_bar() api implemented.

And Yes it is called VFIO but with with specifics appended in it.

>> > How come we need to distinguish between with/without iommu modes ?
>>
>> By default vfio framework assumes iommu i.,e., iommu present. Unless user
>> explicitly set "enable_unsafe_noiommu_mode" param. so in my opinion, we
>> care to parse vfio driver for _noiommu_ mode only.
>
> Why do we care to parse noiommu only?

Because pmd drivers example virtio can work with vfio only in
_noiommu_ mode. In particular, virtio spec 0.95 / legacy virtio. So at
the initialization (example .. virtio-net) of such pmd driver, pmd
driver should know that vfio-with-noiommu mode enabled or not? for
that pmd driver simply checks drv->kdrv value. Currently virtio-net
pmd driver does resource parsing then resource init for interfaces
like UIO/ioport, I intend to do same but only parsing, as resource
init for vfio case already taken care by pci_xx_vfio_map() api in
virtio-net pmd driver (refer Yaun recently virtio 1.0 recently
submitted rte_eal_pci_map patch)

Also Yuan in one of earlier thread inclined to remove all the resource
parsing api from virtio-net pmd driver. Pl. refer this thread [1]

[1] http://dpdk.org/dev/patchwork/patch/9862/

> Even if virtio cannot work in an IOMMU case, there is no reason to add
> a VFIO_NOIOMMU type here.
>
>> > Should not vfio behave the same way from an api point of view ?
>> >
>> Yes It should. vfio gives similar file_ops i.e.. read/write/mmap/seek etc..
>> I am little confused on your question, do you see any issue in vfio bar
>> rd/wr api implementation?
>
> I think you should just consider the VFIO API and let the noiommu option
> as a kernel configuration detail.

vfio apis _are_ considered at low level rd/wr implementation, Has
nothing to do with iommu/noiommu mode. See pci_vfio_read/write_bar()
implementation, they are using pread64/pwrite64() vfio rd/wr api.

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Thomas Monjalon]

2016-01-21 17:34, Santosh Shukla:
> On Thu, Jan 21, 2016 at 4:58 PM, Thomas Monjalon
> <thomas.monjalon@6wind.com> wrote:
> > 2016-01-21 16:43, Santosh Shukla:
> >> David Marchand <david.marchand@6wind.com> wrote:
> >> > This is a mode (specific to vfio), not a new kernel driver.
> >> >
> >> Yes, Specific to VFIO and this is why noiommu appended after vfio i.e..
> >> __VFIO and __VFIO_NOIOMMU.
> >
> > Woaaa! Your logic is really disappointing :)
> > Specific to VFIO => append _NOIOMMU
> > If it's for VFIO, it should be called VFIO (that's my logic).
> >
> I am confused by reading your comment. vfio works for default iommu
> and now with noiommu. drv->kdrv need to know driver mode for vfio
> case. So that user can simply read drv->kdrv value in their driver and
> accordingly use vfio rd/wr api for example {pread/pwrite}. This is how
> rte_eal_pci_vfio_read/write_bar() api implemented.

Sorry I don't understand. Why EAL read/write functions should be different
depending of the VFIO mode?

> And Yes it is called VFIO but with with specifics appended in it.
> 
> >> > How come we need to distinguish between with/without iommu modes ?
> >>
> >> By default vfio framework assumes iommu i.,e., iommu present. Unless user
> >> explicitly set "enable_unsafe_noiommu_mode" param. so in my opinion, we
> >> care to parse vfio driver for _noiommu_ mode only.
> >
> > Why do we care to parse noiommu only?
> 
> Because pmd drivers example virtio can work with vfio only in
> _noiommu_ mode. In particular, virtio spec 0.95 / legacy virtio.

Please could you explain the limitation (except IOMMU availability)?

> So at
> the initialization (example .. virtio-net) of such pmd driver, pmd
> driver should know that vfio-with-noiommu mode enabled or not? for
> that pmd driver simply checks drv->kdrv value.

If a check is needed, I would prefer using your function
pci_vfio_is_noiommu() and remove driver modes from struct rte_kernel_driver.

> Currently virtio-net
> pmd driver does resource parsing then resource init for interfaces
> like UIO/ioport, I intend to do same but only parsing, as resource
> init for vfio case already taken care by pci_xx_vfio_map() api in
> virtio-net pmd driver (refer Yaun recently virtio 1.0 recently
> submitted rte_eal_pci_map patch)
> 
> Also Yuan in one of earlier thread inclined to remove all the resource
> parsing api from virtio-net pmd driver. Pl. refer this thread [1]
> 
> [1] http://dpdk.org/dev/patchwork/patch/9862/

Yes he said
"we should try to avoid getting UIO/VFIO stuff inside virtio pmd driver".
I agree we must try to have those abstractions in EAL.
The only exception seems to be the switch ioport/PCI bar to read/write
in virtio.

> > Even if virtio cannot work in an IOMMU case, there is no reason to add
> > a VFIO_NOIOMMU type here.
> >
> >> > Should not vfio behave the same way from an api point of view ?
> >> >
> >> Yes It should. vfio gives similar file_ops i.e.. read/write/mmap/seek etc..
> >> I am little confused on your question, do you see any issue in vfio bar
> >> rd/wr api implementation?
> >
> > I think you should just consider the VFIO API and let the noiommu option
> > as a kernel configuration detail.
> 
> vfio apis _are_ considered at low level rd/wr implementation, Has
> nothing to do with iommu/noiommu mode. See pci_vfio_read/write_bar()
> implementation, they are using pread64/pwrite64() vfio rd/wr api.

So you agree that the VFIO API abstract the iommu availability details?

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Santosh Shukla]

On Thu, Jan 21, 2016 at 8:16 PM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> 2016-01-21 17:34, Santosh Shukla:
>> On Thu, Jan 21, 2016 at 4:58 PM, Thomas Monjalon
>> <thomas.monjalon@6wind.com> wrote:
>> > 2016-01-21 16:43, Santosh Shukla:
>> >> David Marchand <david.marchand@6wind.com> wrote:
>> >> > This is a mode (specific to vfio), not a new kernel driver.
>> >> >
>> >> Yes, Specific to VFIO and this is why noiommu appended after vfio i.e..
>> >> __VFIO and __VFIO_NOIOMMU.
>> >
>> > Woaaa! Your logic is really disappointing :)
>> > Specific to VFIO => append _NOIOMMU
>> > If it's for VFIO, it should be called VFIO (that's my logic).
>> >
>> I am confused by reading your comment. vfio works for default iommu
>> and now with noiommu. drv->kdrv need to know driver mode for vfio
>> case. So that user can simply read drv->kdrv value in their driver and
>> accordingly use vfio rd/wr api for example {pread/pwrite}. This is how
>> rte_eal_pci_vfio_read/write_bar() api implemented.
>
> Sorry I don't understand. Why EAL read/write functions should be different
> depending of the VFIO mode?
>

no, EAL rd/wr functions are not different for vfio or vfio modes {same
for iommu or noiommu}. Pl. see pci_eal_read/write_bar() api. Those
apis currently used for VFIO, Irrespective of vfio mode. If required,
we can add UIO bar_rd/wr api too. pci_eal_rd/wr_bar() are abstract
apis. Underneath implementation can be vfio or uio type.

>> And Yes it is called VFIO but with with specifics appended in it.
>>
>> >> > How come we need to distinguish between with/without iommu modes ?
>> >>
>> >> By default vfio framework assumes iommu i.,e., iommu present. Unless user
>> >> explicitly set "enable_unsafe_noiommu_mode" param. so in my opinion, we
>> >> care to parse vfio driver for _noiommu_ mode only.
>> >
>> > Why do we care to parse noiommu only?
>>
>> Because pmd drivers example virtio can work with vfio only in
>> _noiommu_ mode. In particular, virtio spec 0.95 / legacy virtio.
>
> Please could you explain the limitation (except IOMMU availability)?
>

Ok.

I believe - we both agree that noiommu mode is a need for pmd drivers
like virtio, right? if so then other reason is implementation driven
i.e..

Pl. look at virtio_pci.c in this patch.. VIRTIO_RD/WR/_1/2/4()
implementation. They are in-use and applicable to  virtio spec 0.95,
so far support uio/ioport-way rd/wr. Now to support vfio-way rd/wr -
need to check drv->kdrv value, that value should be of vfio_noiommu
types __not__  generic _vfio types.

>> So at
>> the initialization (example .. virtio-net) of such pmd driver, pmd
>> driver should know that vfio-with-noiommu mode enabled or not? for
>> that pmd driver simply checks drv->kdrv value.
>
> If a check is needed, I would prefer using your function
> pci_vfio_is_noiommu() and remove driver modes from struct rte_kernel_driver.
>

I don't think calling pci_vfio_no_iommu() inside
virtio_reg_rd/wr_1/2/3() would be a good idea.

>> Currently virtio-net
>> pmd driver does resource parsing then resource init for interfaces
>> like UIO/ioport, I intend to do same but only parsing, as resource
>> init for vfio case already taken care by pci_xx_vfio_map() api in
>> virtio-net pmd driver (refer Yaun recently virtio 1.0 recently
>> submitted rte_eal_pci_map patch)
>>
>> Also Yuan in one of earlier thread inclined to remove all the resource
>> parsing api from virtio-net pmd driver. Pl. refer this thread [1]
>>
>> [1] http://dpdk.org/dev/patchwork/patch/9862/
>
> Yes he said
> "we should try to avoid getting UIO/VFIO stuff inside virtio pmd driver".
> I agree we must try to have those abstractions in EAL.

Right,

IMO vfio now has that abstraction in place and pmd driver only to
check driver type, which virtio-net pmd doing for vfio case. `

> The only exception seems to be the switch ioport/PCI bar to read/write
> in virtio.
>

Right, and only applicable for x86 arch, wont work for non-x86 archs.

>> > Even if virtio cannot work in an IOMMU case, there is no reason to add
>> > a VFIO_NOIOMMU type here.
>> >
>> >> > Should not vfio behave the same way from an api point of view ?
>> >> >
>> >> Yes It should. vfio gives similar file_ops i.e.. read/write/mmap/seek etc..
>> >> I am little confused on your question, do you see any issue in vfio bar
>> >> rd/wr api implementation?
>> >
>> > I think you should just consider the VFIO API and let the noiommu option
>> > as a kernel configuration detail.
>>
>> vfio apis _are_ considered at low level rd/wr implementation, Has
>> nothing to do with iommu/noiommu mode. See pci_vfio_read/write_bar()
>> implementation, they are using pread64/pwrite64() vfio rd/wr api.
>
> So you agree that the VFIO API abstract the iommu availability details?
>

didn't understood your question,
>

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Thomas Monjalon]

2016-01-21 22:47, Santosh Shukla:
> On Thu, Jan 21, 2016 at 8:16 PM, Thomas Monjalon
> <thomas.monjalon@6wind.com> wrote:
> > 2016-01-21 17:34, Santosh Shukla:
> >> On Thu, Jan 21, 2016 at 4:58 PM, Thomas Monjalon
> >> <thomas.monjalon@6wind.com> wrote:
> >> > 2016-01-21 16:43, Santosh Shukla:
> >> >> David Marchand <david.marchand@6wind.com> wrote:
> >> >> > This is a mode (specific to vfio), not a new kernel driver.
> >> >> >
> >> >> Yes, Specific to VFIO and this is why noiommu appended after vfio i.e..
> >> >> __VFIO and __VFIO_NOIOMMU.
> >> >
> >> > Woaaa! Your logic is really disappointing :)
> >> > Specific to VFIO => append _NOIOMMU
> >> > If it's for VFIO, it should be called VFIO (that's my logic).
> >> >
> >> I am confused by reading your comment. vfio works for default iommu
> >> and now with noiommu. drv->kdrv need to know driver mode for vfio
> >> case. So that user can simply read drv->kdrv value in their driver and
> >> accordingly use vfio rd/wr api for example {pread/pwrite}. This is how
> >> rte_eal_pci_vfio_read/write_bar() api implemented.
> >
> > Sorry I don't understand. Why EAL read/write functions should be different
> > depending of the VFIO mode?
> 
> no, EAL rd/wr functions are not different for vfio or vfio modes {same
> for iommu or noiommu}. Pl. see pci_eal_read/write_bar() api. Those
> apis currently used for VFIO, Irrespective of vfio mode. If required,
> we can add UIO bar_rd/wr api too. pci_eal_rd/wr_bar() are abstract
> apis. Underneath implementation can be vfio or uio type.

It means you agree the suffix _NOIOMMU is not needed?
It seems we go nowhere in this discussion. You said
"drv->kdrv need to know driver mode for vfio"
and after
"Those apis currently used for VFIO, Irrespective of vfio mode"
That's why I assume your first assumption was wrong.

> >> > Why do we care to parse noiommu only?
> >>
> >> Because pmd drivers example virtio can work with vfio only in
> >> _noiommu_ mode. In particular, virtio spec 0.95 / legacy virtio.
> >
> > Please could you explain the limitation (except IOMMU availability)?
> 
> Ok.
> 
> I believe - we both agree that noiommu mode is a need for pmd drivers
> like virtio, right? if so then other reason is implementation driven

No, noiommu is a need for some environment having no IOMMU.
But in my understanding, virtio could run with a nested IOMMU.

> i.e..
> 
> Pl. look at virtio_pci.c in this patch.. VIRTIO_RD/WR/_1/2/4()
> implementation. They are in-use and applicable to  virtio spec 0.95,
> so far support uio/ioport-way rd/wr. Now to support vfio-way rd/wr -
> need to check drv->kdrv value, that value should be of vfio_noiommu
> types __not__  generic _vfio types.

I still don't understand why it would not work with VFIO w/IOMMU.

> >> So at
> >> the initialization (example .. virtio-net) of such pmd driver, pmd
> >> driver should know that vfio-with-noiommu mode enabled or not? for
> >> that pmd driver simply checks drv->kdrv value.
> >
> > If a check is needed, I would prefer using your function
> > pci_vfio_is_noiommu() and remove driver modes from struct rte_kernel_driver.
> 
> I don't think calling pci_vfio_no_iommu() inside
> virtio_reg_rd/wr_1/2/3() would be a good idea.

Why? The value may be cached in the priv properties.

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Santosh Shukla]

On Mon, Jan 25, 2016 at 8:59 PM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> 2016-01-21 22:47, Santosh Shukla:
>> On Thu, Jan 21, 2016 at 8:16 PM, Thomas Monjalon
>> <thomas.monjalon@6wind.com> wrote:
>> > 2016-01-21 17:34, Santosh Shukla:
>> >> On Thu, Jan 21, 2016 at 4:58 PM, Thomas Monjalon
>> >> <thomas.monjalon@6wind.com> wrote:
>> >> > 2016-01-21 16:43, Santosh Shukla:
>> >> >> David Marchand <david.marchand@6wind.com> wrote:
>> >> >> > This is a mode (specific to vfio), not a new kernel driver.
>> >> >> >
>> >> >> Yes, Specific to VFIO and this is why noiommu appended after vfio i.e..
>> >> >> __VFIO and __VFIO_NOIOMMU.
>> >> >
>> >> > Woaaa! Your logic is really disappointing :)
>> >> > Specific to VFIO => append _NOIOMMU
>> >> > If it's for VFIO, it should be called VFIO (that's my logic).
>> >> >
>> >> I am confused by reading your comment. vfio works for default iommu
>> >> and now with noiommu. drv->kdrv need to know driver mode for vfio
>> >> case. So that user can simply read drv->kdrv value in their driver and
>> >> accordingly use vfio rd/wr api for example {pread/pwrite}. This is how
>> >> rte_eal_pci_vfio_read/write_bar() api implemented.
>> >
>> > Sorry I don't understand. Why EAL read/write functions should be different
>> > depending of the VFIO mode?
>>
>> no, EAL rd/wr functions are not different for vfio or vfio modes {same
>> for iommu or noiommu}. Pl. see pci_eal_read/write_bar() api. Those
>> apis currently used for VFIO, Irrespective of vfio mode. If required,
>> we can add UIO bar_rd/wr api too. pci_eal_rd/wr_bar() are abstract
>> apis. Underneath implementation can be vfio or uio type.
>
> It means you agree the suffix _NOIOMMU is not needed?
> It seems we go nowhere in this discussion. You said
> "drv->kdrv need to know driver mode for vfio"

In my observation, currently virtio work for vfio-noiommu, that's why
said drv->kdrv need to know vfio mode.

> and after
> "Those apis currently used for VFIO, Irrespective of vfio mode"
> That's why I assume your first assumption was wrong.
>

Newly introduced dpdk global api pci_eal_rd/wr_bar(),  can be used for
vfio and uio both; can be used for vfio w/IOMMU and vfio w/o IOMMU
both.

>> >> > Why do we care to parse noiommu only?
>> >>
>> >> Because pmd drivers example virtio can work with vfio only in
>> >> _noiommu_ mode. In particular, virtio spec 0.95 / legacy virtio.
>> >
>> > Please could you explain the limitation (except IOMMU availability)?
>>
>> Ok.
>>
>> I believe - we both agree that noiommu mode is a need for pmd drivers
>> like virtio, right? if so then other reason is implementation driven
>
> No, noiommu is a need for some environment having no IOMMU.
> But in my understanding, virtio could run with a nested IOMMU.
>

Interesting, like to understand nested one, I did tried in past by
passing "iommu=pt intel_iommu=on kvm-intel.nested=1" in cmdline for
x86 (for guest/host both), but virtio pci device binding to vfio-pci
driver fails. Tried on 4.2 kernel (qemu version 2.5), is it working
for >4.2 kernel/ qemu-version?

>> i.e..
>>
>> Pl. look at virtio_pci.c in this patch.. VIRTIO_RD/WR/_1/2/4()
>> implementation. They are in-use and applicable to  virtio spec 0.95,
>> so far support uio/ioport-way rd/wr. Now to support vfio-way rd/wr -
>> need to check drv->kdrv value, that value should be of vfio_noiommu
>> types __not__  generic _vfio types.
>
> I still don't understand why it would not work with VFIO w/IOMMU.
>

with vfio+iommu; binding virtio pci device to vfio-pci driver fail;
giving below error:
[   53.053464] VFIO - User Level meta-driver version: 0.3
[   73.077805] vfio-pci: probe of 0000:00:03.0 failed with error -22
[   73.077852] vfio-pci: probe of 0000:00:03.0 failed with error -22

vfio_pci_probe() --> vfio_iommu_group_get() --> iommu_group_get()
fails: iommu doesn't have group for virtio pci device.

In case of noiommu, it prepares the group / add device to iommu group,
so it passes.

Jason in other thread mentioned that he is working on virtio+iommu
approach [1], Patches are not merged and I am yet to evaluate his
patches for virtio pmd driver for iommu(+vfio). so wondering how
virtio pci device could work unless jason patches used?

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg337079.html

>> >> So at
>> >> the initialization (example .. virtio-net) of such pmd driver, pmd
>> >> driver should know that vfio-with-noiommu mode enabled or not? for
>> >> that pmd driver simply checks drv->kdrv value.
>> >
>> > If a check is needed, I would prefer using your function
>> > pci_vfio_is_noiommu() and remove driver modes from struct rte_kernel_driver.
>>
>> I don't think calling pci_vfio_no_iommu() inside
>> virtio_reg_rd/wr_1/2/3() would be a good idea.
>
> Why? The value may be cached in the priv properties.
>
pci_vfio_is_noiommu() parses /sys for
- enable_noiommu param
- attached driver name is vfio-noiommu or not.

It does file operation for that, I meant to say that calling this api
within register_rd/wr function is not correct. It would be better if
those low level register_rd/wr api only checks driver_types.

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Thomas Monjalon]

2016-01-26 15:56, Santosh Shukla:
> On Mon, Jan 25, 2016 at 8:59 PM, Thomas Monjalon
> <thomas.monjalon@6wind.com> wrote:
> > 2016-01-21 22:47, Santosh Shukla:
> >> On Thu, Jan 21, 2016 at 8:16 PM, Thomas Monjalon
> >> <thomas.monjalon@6wind.com> wrote:
> >> > 2016-01-21 17:34, Santosh Shukla:
> >> >> On Thu, Jan 21, 2016 at 4:58 PM, Thomas Monjalon
> >> >> <thomas.monjalon@6wind.com> wrote:
> >> >> > 2016-01-21 16:43, Santosh Shukla:
> >> >> >> David Marchand <david.marchand@6wind.com> wrote:
> >> >> >> > This is a mode (specific to vfio), not a new kernel driver.
> >> >> >> >
> >> >> >> Yes, Specific to VFIO and this is why noiommu appended after vfio i.e..
> >> >> >> __VFIO and __VFIO_NOIOMMU.
> >> >> >
> >> >> > Woaaa! Your logic is really disappointing :)
> >> >> > Specific to VFIO => append _NOIOMMU
> >> >> > If it's for VFIO, it should be called VFIO (that's my logic).
> >> >> >
> >> >> I am confused by reading your comment. vfio works for default iommu
> >> >> and now with noiommu. drv->kdrv need to know driver mode for vfio
> >> >> case. So that user can simply read drv->kdrv value in their driver and
> >> >> accordingly use vfio rd/wr api for example {pread/pwrite}. This is how
> >> >> rte_eal_pci_vfio_read/write_bar() api implemented.
> >> >
> >> > Sorry I don't understand. Why EAL read/write functions should be different
> >> > depending of the VFIO mode?
> >>
> >> no, EAL rd/wr functions are not different for vfio or vfio modes {same
> >> for iommu or noiommu}. Pl. see pci_eal_read/write_bar() api. Those
> >> apis currently used for VFIO, Irrespective of vfio mode. If required,
> >> we can add UIO bar_rd/wr api too. pci_eal_rd/wr_bar() are abstract
> >> apis. Underneath implementation can be vfio or uio type.
> >
> > It means you agree the suffix _NOIOMMU is not needed?
> > It seems we go nowhere in this discussion. You said
> > "drv->kdrv need to know driver mode for vfio"
> 
> In my observation, currently virtio work for vfio-noiommu, that's why
> said drv->kdrv need to know vfio mode.

It is your observation. It may change in near future.

> > and after
> > "Those apis currently used for VFIO, Irrespective of vfio mode"
> > That's why I assume your first assumption was wrong.
> >
> 
> Newly introduced dpdk global api pci_eal_rd/wr_bar(),  can be used for
> vfio and uio both; can be used for vfio w/IOMMU and vfio w/o IOMMU
> both.
> 
> >> >> > Why do we care to parse noiommu only?
> >> >>
> >> >> Because pmd drivers example virtio can work with vfio only in
> >> >> _noiommu_ mode. In particular, virtio spec 0.95 / legacy virtio.
> >> >
> >> > Please could you explain the limitation (except IOMMU availability)?
> >>
> >> Ok.
> >>
> >> I believe - we both agree that noiommu mode is a need for pmd drivers
> >> like virtio, right? if so then other reason is implementation driven
> >
> > No, noiommu is a need for some environment having no IOMMU.
> > But in my understanding, virtio could run with a nested IOMMU.
> 
> Interesting, like to understand nested one, I did tried in past by
> passing "iommu=pt intel_iommu=on kvm-intel.nested=1" in cmdline for
> x86 (for guest/host both), but virtio pci device binding to vfio-pci
> driver fails. Tried on 4.2 kernel (qemu version 2.5), is it working
> for >4.2 kernel/ qemu-version?

I haven't tried.

> >> i.e..
> >>
> >> Pl. look at virtio_pci.c in this patch.. VIRTIO_RD/WR/_1/2/4()
> >> implementation. They are in-use and applicable to  virtio spec 0.95,
> >> so far support uio/ioport-way rd/wr. Now to support vfio-way rd/wr -
> >> need to check drv->kdrv value, that value should be of vfio_noiommu
> >> types __not__  generic _vfio types.
> >
> > I still don't understand why it would not work with VFIO w/IOMMU.
> 
> with vfio+iommu; binding virtio pci device to vfio-pci driver fail;
> giving below error:
> [   53.053464] VFIO - User Level meta-driver version: 0.3
> [   73.077805] vfio-pci: probe of 0000:00:03.0 failed with error -22
> [   73.077852] vfio-pci: probe of 0000:00:03.0 failed with error -22
> 
> vfio_pci_probe() --> vfio_iommu_group_get() --> iommu_group_get()
> fails: iommu doesn't have group for virtio pci device.

Yes it fails when binding.
So the later check in the virtio PMD is useless.

> In case of noiommu, it prepares the group / add device to iommu group,
> so it passes.
> 
> Jason in other thread mentioned that he is working on virtio+iommu
> approach [1], Patches are not merged and I am yet to evaluate his
> patches for virtio pmd driver for iommu(+vfio). so wondering how
> virtio pci device could work unless jason patches used?
> 
> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg337079.html

I haven't tried nested IOMMU.
All this thread was about the kernel module in use, i.e. VFIO.
We are saying that virtio could work in both VFIO modes.
Furthermore restricting virtio to no-iommu mode doesn't bring
any improvement.
That's why I suggest to keep the initial semantic of kdrv and
not pollute it with VFIO modes.

> >> >> So at
> >> >> the initialization (example .. virtio-net) of such pmd driver, pmd
> >> >> driver should know that vfio-with-noiommu mode enabled or not? for
> >> >> that pmd driver simply checks drv->kdrv value.
> >> >
> >> > If a check is needed, I would prefer using your function
> >> > pci_vfio_is_noiommu() and remove driver modes from struct rte_kernel_driver.
> >>
> >> I don't think calling pci_vfio_no_iommu() inside
> >> virtio_reg_rd/wr_1/2/3() would be a good idea.
> >
> > Why? The value may be cached in the priv properties.
> >
> pci_vfio_is_noiommu() parses /sys for
> - enable_noiommu param
> - attached driver name is vfio-noiommu or not.
> 
> It does file operation for that, I meant to say that calling this api
> within register_rd/wr function is not correct. It would be better if
> those low level register_rd/wr api only checks driver_types.

Yes, that's why I said the return of pci_vfio_is_noiommu() may be cached
to keep efficiency.

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Santosh Shukla]

On Tue, Jan 26, 2016 at 6:30 PM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> 2016-01-26 15:56, Santosh Shukla:
>> On Mon, Jan 25, 2016 at 8:59 PM, Thomas Monjalon
>> <thomas.monjalon@6wind.com> wrote:
>> > 2016-01-21 22:47, Santosh Shukla:
>> >> On Thu, Jan 21, 2016 at 8:16 PM, Thomas Monjalon
>> >> <thomas.monjalon@6wind.com> wrote:
>> >> > 2016-01-21 17:34, Santosh Shukla:
>> >> >> On Thu, Jan 21, 2016 at 4:58 PM, Thomas Monjalon
>> >> >> <thomas.monjalon@6wind.com> wrote:
>> >> >> > 2016-01-21 16:43, Santosh Shukla:
>> >> >> >> David Marchand <david.marchand@6wind.com> wrote:
>> >> >> >> > This is a mode (specific to vfio), not a new kernel driver.
>> >> >> >> >
>> >> >> >> Yes, Specific to VFIO and this is why noiommu appended after vfio i.e..
>> >> >> >> __VFIO and __VFIO_NOIOMMU.
>> >> >> >
>> >> >> > Woaaa! Your logic is really disappointing :)
>> >> >> > Specific to VFIO => append _NOIOMMU
>> >> >> > If it's for VFIO, it should be called VFIO (that's my logic).
>> >> >> >
>> >> >> I am confused by reading your comment. vfio works for default iommu
>> >> >> and now with noiommu. drv->kdrv need to know driver mode for vfio
>> >> >> case. So that user can simply read drv->kdrv value in their driver and
>> >> >> accordingly use vfio rd/wr api for example {pread/pwrite}. This is how
>> >> >> rte_eal_pci_vfio_read/write_bar() api implemented.
>> >> >
>> >> > Sorry I don't understand. Why EAL read/write functions should be different
>> >> > depending of the VFIO mode?
>> >>
>> >> no, EAL rd/wr functions are not different for vfio or vfio modes {same
>> >> for iommu or noiommu}. Pl. see pci_eal_read/write_bar() api. Those
>> >> apis currently used for VFIO, Irrespective of vfio mode. If required,
>> >> we can add UIO bar_rd/wr api too. pci_eal_rd/wr_bar() are abstract
>> >> apis. Underneath implementation can be vfio or uio type.
>> >
>> > It means you agree the suffix _NOIOMMU is not needed?
>> > It seems we go nowhere in this discussion. You said
>> > "drv->kdrv need to know driver mode for vfio"
>>
>> In my observation, currently virtio work for vfio-noiommu, that's why
>> said drv->kdrv need to know vfio mode.
>
> It is your observation. It may change in near future.
>

so that mean till then, virtio support for non-x86 arch has to wait?
We have working model with vfio-noiommu, don't you think it make sense
to let vfio_noiommu implementation exist and later in-case
virtio+iommu gets mainline then switch to vfio __mode__ agnostic
approach. And for that All it takes to replace __noiommu suffix with
default.

>> > and after
>> > "Those apis currently used for VFIO, Irrespective of vfio mode"
>> > That's why I assume your first assumption was wrong.
>> >
>>
>> Newly introduced dpdk global api pci_eal_rd/wr_bar(),  can be used for
>> vfio and uio both; can be used for vfio w/IOMMU and vfio w/o IOMMU
>> both.
>>
>> >> >> > Why do we care to parse noiommu only?
>> >> >>
>> >> >> Because pmd drivers example virtio can work with vfio only in
>> >> >> _noiommu_ mode. In particular, virtio spec 0.95 / legacy virtio.
>> >> >
>> >> > Please could you explain the limitation (except IOMMU availability)?
>> >>
>> >> Ok.
>> >>
>> >> I believe - we both agree that noiommu mode is a need for pmd drivers
>> >> like virtio, right? if so then other reason is implementation driven
>> >
>> > No, noiommu is a need for some environment having no IOMMU.
>> > But in my understanding, virtio could run with a nested IOMMU.
>>
>> Interesting, like to understand nested one, I did tried in past by
>> passing "iommu=pt intel_iommu=on kvm-intel.nested=1" in cmdline for
>> x86 (for guest/host both), but virtio pci device binding to vfio-pci
>> driver fails. Tried on 4.2 kernel (qemu version 2.5), is it working
>> for >4.2 kernel/ qemu-version?
>
> I haven't tried.
>
>> >> i.e..
>> >>
>> >> Pl. look at virtio_pci.c in this patch.. VIRTIO_RD/WR/_1/2/4()
>> >> implementation. They are in-use and applicable to  virtio spec 0.95,
>> >> so far support uio/ioport-way rd/wr. Now to support vfio-way rd/wr -
>> >> need to check drv->kdrv value, that value should be of vfio_noiommu
>> >> types __not__  generic _vfio types.
>> >
>> > I still don't understand why it would not work with VFIO w/IOMMU.
>>
>> with vfio+iommu; binding virtio pci device to vfio-pci driver fail;
>> giving below error:
>> [   53.053464] VFIO - User Level meta-driver version: 0.3
>> [   73.077805] vfio-pci: probe of 0000:00:03.0 failed with error -22
>> [   73.077852] vfio-pci: probe of 0000:00:03.0 failed with error -22
>>
>> vfio_pci_probe() --> vfio_iommu_group_get() --> iommu_group_get()
>> fails: iommu doesn't have group for virtio pci device.
>
> Yes it fails when binding.
> So the later check in the virtio PMD is useless.
>

Which check?

>> In case of noiommu, it prepares the group / add device to iommu group,
>> so it passes.
>>
>> Jason in other thread mentioned that he is working on virtio+iommu
>> approach [1], Patches are not merged and I am yet to evaluate his
>> patches for virtio pmd driver for iommu(+vfio). so wondering how
>> virtio pci device could work unless jason patches used?
>>
>> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg337079.html
>
> I haven't tried nested IOMMU.
> All this thread was about the kernel module in use, i.e. VFIO.
> We are saying that virtio could work in both VFIO modes.

I agree and I am looking at stuff that works, vfio-noiommu is in
linus-next, and soon be in mainline. This series will let non-x86
archs to use virtio pmd driver in first place and could take benefits
of for example vhost, ovs-dpdk-vhost-user etc..

> Furthermore restricting virtio to no-iommu mode doesn't bring
> any improvement.

We're not __restricting__, as soon as virtio+iommu gets working state,
we'll simply replace __noiommu with default. Then its upto user to try
out virtio with vfio default or vfio_noiommu.

> That's why I suggest to keep the initial semantic of kdrv and
> not pollute it with VFIO modes.
>

I am okay to live with default and forget suffix __noiommu but there
are implementation problem which was discussed in other thread
- Virtio pmd driver should avoid interface parsing i.e.
virtio_resource_init_uio/vfio() etc.. For vfio case - We could easily
get rid of by moving /sys parsing to pci_eal layer, Right? If so then
virtio currently works with vfio-noiommu, it make sense to me that
pci_eal layer does parsing for pmd driver before that pmd driver get
initialized.

- Another case could be: iommu-less-pmd-driver. eal layer to do
parsing before updating drv->kdrv.

>> >> >> So at
>> >> >> the initialization (example .. virtio-net) of such pmd driver, pmd
>> >> >> driver should know that vfio-with-noiommu mode enabled or not? for
>> >> >> that pmd driver simply checks drv->kdrv value.
>> >> >
>> >> > If a check is needed, I would prefer using your function
>> >> > pci_vfio_is_noiommu() and remove driver modes from struct rte_kernel_driver.
>> >>
>> >> I don't think calling pci_vfio_no_iommu() inside
>> >> virtio_reg_rd/wr_1/2/3() would be a good idea.
>> >
>> > Why? The value may be cached in the priv properties.
>> >
>> pci_vfio_is_noiommu() parses /sys for
>> - enable_noiommu param
>> - attached driver name is vfio-noiommu or not.
>>
>> It does file operation for that, I meant to say that calling this api
>> within register_rd/wr function is not correct. It would be better if
>> those low level register_rd/wr api only checks driver_types.
>
> Yes, that's why I said the return of pci_vfio_is_noiommu() may be cached
> to keep efficiency.

I am not convinced though, Still find pmd driver checking driver_types
using drv->kdrv is better approach than introducing a new global
variable which may look something like;

At pci_eal layer ----
bool vfio_mode;
vfio_mode = pci_vfio_is_noiommu();

At virtio pmd driver layer ----
Checking value at vfio_mode variable before doing virtio_rd/wr for
vfio interface.

Instead virtio pmd driver doing

virtio_reg_rd/wr_1/2/4()
{
if (drv->kdrv == VFIO)
      do pread()/pwrite()
else
      in()/out()
}

is better approach.

Let me know if you still think former is better than latter then I'll
send patch revision right-away.

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Thomas Monjalon]

2016-01-26 19:35, Santosh Shukla:
> On Tue, Jan 26, 2016 at 6:30 PM, Thomas Monjalon
> <thomas.monjalon@6wind.com> wrote:
> > 2016-01-26 15:56, Santosh Shukla:
> >> In my observation, currently virtio work for vfio-noiommu, that's why
> >> said drv->kdrv need to know vfio mode.
> >
> > It is your observation. It may change in near future.
> 
> so that mean till then, virtio support for non-x86 arch has to wait?

No, absolutely not. virtio for non-x86 is welcome.

> We have working model with vfio-noiommu, don't you think it make sense
> to let vfio_noiommu implementation exist and later in-case
> virtio+iommu gets mainline then switch to vfio __mode__ agnostic
> approach. And for that All it takes to replace __noiommu suffix with
> default.

I'm just saying you should not touch the enum rte_kernel_driver.
RTE_KDRV_VFIO is a driver.
RTE_KDRV_VFIO_NOIOMMU is a mode.
As the VFIO API is the same in both modes, there is no reason to
distinguish them at this level.
Your patch adds the NOIOMMU case everywhere:
        case RTE_KDRV_VFIO:
+       case RTE_KDRV_VFIO_NOIOMMU:

I'll stop commenting here to let others give their opinion.

[...]
> >> with vfio+iommu; binding virtio pci device to vfio-pci driver fail;
> >> giving below error:
> >> [   53.053464] VFIO - User Level meta-driver version: 0.3
> >> [   73.077805] vfio-pci: probe of 0000:00:03.0 failed with error -22
> >> [   73.077852] vfio-pci: probe of 0000:00:03.0 failed with error -22
> >>
> >> vfio_pci_probe() --> vfio_iommu_group_get() --> iommu_group_get()
> >> fails: iommu doesn't have group for virtio pci device.
> >
> > Yes it fails when binding.
> > So the later check in the virtio PMD is useless.
> 
> Which check?

The check for VFIO noiommu only:
-       if (dev->kdrv == RTE_KDRV_VFIO)
+       if (dev->kdrv == RTE_KDRV_VFIO_NOIOMMU)

[...]
> > Furthermore restricting virtio to no-iommu mode doesn't bring
> > any improvement.
> 
> We're not __restricting__, as soon as virtio+iommu gets working state,
> we'll simply replace __noiommu with default. Then its upto user to try
> out virtio with vfio default or vfio_noiommu.

Yes it's up to user.
So your code should be
	if (dev->kdrv == RTE_KDRV_VFIO)

> > That's why I suggest to keep the initial semantic of kdrv and
> > not pollute it with VFIO modes.
> 
> I am okay to live with default and forget suffix __noiommu but there
> are implementation problem which was discussed in other thread
> - Virtio pmd driver should avoid interface parsing i.e.
> virtio_resource_init_uio/vfio() etc.. For vfio case - We could easily
> get rid of by moving /sys parsing to pci_eal layer, Right? If so then
> virtio currently works with vfio-noiommu, it make sense to me that
> pci_eal layer does parsing for pmd driver before that pmd driver get
> initialized.

Please reword. What is the problem?

> - Another case could be: iommu-less-pmd-driver. eal layer to do
> parsing before updating drv->kdrv.

[...]
> >> >> > If a check is needed, I would prefer using your function
> >> >> > pci_vfio_is_noiommu() and remove driver modes from struct rte_kernel_driver.
> >> >>
> >> >> I don't think calling pci_vfio_no_iommu() inside
> >> >> virtio_reg_rd/wr_1/2/3() would be a good idea.
> >> >
> >> > Why? The value may be cached in the priv properties.
> >> >
> >> pci_vfio_is_noiommu() parses /sys for
> >> - enable_noiommu param
> >> - attached driver name is vfio-noiommu or not.
> >>
> >> It does file operation for that, I meant to say that calling this api
> >> within register_rd/wr function is not correct. It would be better if
> >> those low level register_rd/wr api only checks driver_types.
> >
> > Yes, that's why I said the return of pci_vfio_is_noiommu() may be cached
> > to keep efficiency.
> 
> I am not convinced though, Still find pmd driver checking driver_types
> using drv->kdrv is better approach than introducing a new global
> variable which may look something like;

Not a global variable. A function in EAL layer. A variable in PMD priv.

> At pci_eal layer ----
> bool vfio_mode;
> vfio_mode = pci_vfio_is_noiommu();
> 
> At virtio pmd driver layer ----
> Checking value at vfio_mode variable before doing virtio_rd/wr for
> vfio interface.
> 
> Instead virtio pmd driver doing
> 
> virtio_reg_rd/wr_1/2/4()
> {
> if (drv->kdrv == VFIO)
>       do pread()/pwrite()
> else
>       in()/out()
> }
> 
> is better approach.
> 
> Let me know if you still think former is better than latter then I'll
> send patch revision right-away.

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Santosh Shukla]

On Tue, Jan 26, 2016 at 7:58 PM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> 2016-01-26 19:35, Santosh Shukla:
>> On Tue, Jan 26, 2016 at 6:30 PM, Thomas Monjalon
>> <thomas.monjalon@6wind.com> wrote:
>> > 2016-01-26 15:56, Santosh Shukla:
>> >> In my observation, currently virtio work for vfio-noiommu, that's why
>> >> said drv->kdrv need to know vfio mode.
>> >
>> > It is your observation. It may change in near future.
>>
>> so that mean till then, virtio support for non-x86 arch has to wait?
>
> No, absolutely not. virtio for non-x86 is welcome.
>
>> We have working model with vfio-noiommu, don't you think it make sense
>> to let vfio_noiommu implementation exist and later in-case
>> virtio+iommu gets mainline then switch to vfio __mode__ agnostic
>> approach. And for that All it takes to replace __noiommu suffix with
>> default.
>
> I'm just saying you should not touch the enum rte_kernel_driver.
> RTE_KDRV_VFIO is a driver.
> RTE_KDRV_VFIO_NOIOMMU is a mode.
> As the VFIO API is the same in both modes, there is no reason to
> distinguish them at this level.
> Your patch adds the NOIOMMU case everywhere:
>         case RTE_KDRV_VFIO:
> +       case RTE_KDRV_VFIO_NOIOMMU:
>
> I'll stop commenting here to let others give their opinion.
>
> [...]
>> >> with vfio+iommu; binding virtio pci device to vfio-pci driver fail;
>> >> giving below error:
>> >> [   53.053464] VFIO - User Level meta-driver version: 0.3
>> >> [   73.077805] vfio-pci: probe of 0000:00:03.0 failed with error -22
>> >> [   73.077852] vfio-pci: probe of 0000:00:03.0 failed with error -22
>> >>
>> >> vfio_pci_probe() --> vfio_iommu_group_get() --> iommu_group_get()
>> >> fails: iommu doesn't have group for virtio pci device.
>> >
>> > Yes it fails when binding.
>> > So the later check in the virtio PMD is useless.
>>
>> Which check?
>
> The check for VFIO noiommu only:
> -       if (dev->kdrv == RTE_KDRV_VFIO)
> +       if (dev->kdrv == RTE_KDRV_VFIO_NOIOMMU)
>
> [...]
>> > Furthermore restricting virtio to no-iommu mode doesn't bring
>> > any improvement.
>>
>> We're not __restricting__, as soon as virtio+iommu gets working state,
>> we'll simply replace __noiommu with default. Then its upto user to try
>> out virtio with vfio default or vfio_noiommu.
>
> Yes it's up to user.
> So your code should be
>         if (dev->kdrv == RTE_KDRV_VFIO)
>

Right,

>> > That's why I suggest to keep the initial semantic of kdrv and
>> > not pollute it with VFIO modes.
>>
>> I am okay to live with default and forget suffix __noiommu but there
>> are implementation problem which was discussed in other thread
>> - Virtio pmd driver should avoid interface parsing i.e.
>> virtio_resource_init_uio/vfio() etc.. For vfio case - We could easily
>> get rid of by moving /sys parsing to pci_eal layer, Right? If so then
>> virtio currently works with vfio-noiommu, it make sense to me that
>> pci_eal layer does parsing for pmd driver before that pmd driver get
>> initialized.
>
> Please reword. What is the problem?
>
>> - Another case could be: iommu-less-pmd-driver. eal layer to do
>> parsing before updating drv->kdrv.
>
> [...]
>> >> >> > If a check is needed, I would prefer using your function
>> >> >> > pci_vfio_is_noiommu() and remove driver modes from struct rte_kernel_driver.
>> >> >>
>> >> >> I don't think calling pci_vfio_no_iommu() inside
>> >> >> virtio_reg_rd/wr_1/2/3() would be a good idea.
>> >> >
>> >> > Why? The value may be cached in the priv properties.
>> >> >
>> >> pci_vfio_is_noiommu() parses /sys for
>> >> - enable_noiommu param
>> >> - attached driver name is vfio-noiommu or not.
>> >>
>> >> It does file operation for that, I meant to say that calling this api
>> >> within register_rd/wr function is not correct. It would be better if
>> >> those low level register_rd/wr api only checks driver_types.
>> >
>> > Yes, that's why I said the return of pci_vfio_is_noiommu() may be cached
>> > to keep efficiency.
>>
>> I am not convinced though, Still find pmd driver checking driver_types
>> using drv->kdrv is better approach than introducing a new global
>> variable which may look something like;
>
> Not a global variable. A function in EAL layer. A variable in PMD priv.
>

If we agreed to use condition (drv->kdrv == RTE_KDRV_VFIO);
then resource parsing for vfio {including vfio and vfio_noiommu both
case} is enforced in virtio pmd driver layer and that is contradicting
to what we agreed earlier in this[1] thread. Also we don't need a
function in EAL layer or a variable in PMD priv. Perhaps a private
function in virtio pmd which does parsing for vfio interface.

Thoughts?

[1] http://dpdk.org/dev/patchwork/patch/9862/

>> At pci_eal layer ----
>> bool vfio_mode;
>> vfio_mode = pci_vfio_is_noiommu();
>>
>> At virtio pmd driver layer ----
>> Checking value at vfio_mode variable before doing virtio_rd/wr for
>> vfio interface.
>>
>> Instead virtio pmd driver doing
>>
>> virtio_reg_rd/wr_1/2/4()
>> {
>> if (drv->kdrv == VFIO)
>>       do pread()/pwrite()
>> else
>>       in()/out()
>> }
>>
>> is better approach.
>>
>> Let me know if you still think former is better than latter then I'll
>> send patch revision right-away.
>
>

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Santosh Shukla]

On Tue, Jan 26, 2016 at 9:51 PM, Santosh Shukla <sshukla@mvista.com> wrote:
> On Tue, Jan 26, 2016 at 7:58 PM, Thomas Monjalon
> <thomas.monjalon@6wind.com> wrote:
>> 2016-01-26 19:35, Santosh Shukla:
>>> On Tue, Jan 26, 2016 at 6:30 PM, Thomas Monjalon
>>> <thomas.monjalon@6wind.com> wrote:
>>> > 2016-01-26 15:56, Santosh Shukla:
>>> >> In my observation, currently virtio work for vfio-noiommu, that's why
>>> >> said drv->kdrv need to know vfio mode.
>>> >
>>> > It is your observation. It may change in near future.
>>>
>>> so that mean till then, virtio support for non-x86 arch has to wait?
>>
>> No, absolutely not. virtio for non-x86 is welcome.
>>
>>> We have working model with vfio-noiommu, don't you think it make sense
>>> to let vfio_noiommu implementation exist and later in-case
>>> virtio+iommu gets mainline then switch to vfio __mode__ agnostic
>>> approach. And for that All it takes to replace __noiommu suffix with
>>> default.
>>
>> I'm just saying you should not touch the enum rte_kernel_driver.
>> RTE_KDRV_VFIO is a driver.
>> RTE_KDRV_VFIO_NOIOMMU is a mode.
>> As the VFIO API is the same in both modes, there is no reason to
>> distinguish them at this level.
>> Your patch adds the NOIOMMU case everywhere:
>>         case RTE_KDRV_VFIO:
>> +       case RTE_KDRV_VFIO_NOIOMMU:
>>
>> I'll stop commenting here to let others give their opinion.
>>
>> [...]
>>> >> with vfio+iommu; binding virtio pci device to vfio-pci driver fail;
>>> >> giving below error:
>>> >> [   53.053464] VFIO - User Level meta-driver version: 0.3
>>> >> [   73.077805] vfio-pci: probe of 0000:00:03.0 failed with error -22
>>> >> [   73.077852] vfio-pci: probe of 0000:00:03.0 failed with error -22
>>> >>
>>> >> vfio_pci_probe() --> vfio_iommu_group_get() --> iommu_group_get()
>>> >> fails: iommu doesn't have group for virtio pci device.
>>> >
>>> > Yes it fails when binding.
>>> > So the later check in the virtio PMD is useless.
>>>
>>> Which check?
>>
>> The check for VFIO noiommu only:
>> -       if (dev->kdrv == RTE_KDRV_VFIO)
>> +       if (dev->kdrv == RTE_KDRV_VFIO_NOIOMMU)
>>
>> [...]
>>> > Furthermore restricting virtio to no-iommu mode doesn't bring
>>> > any improvement.
>>>
>>> We're not __restricting__, as soon as virtio+iommu gets working state,
>>> we'll simply replace __noiommu with default. Then its upto user to try
>>> out virtio with vfio default or vfio_noiommu.
>>
>> Yes it's up to user.
>> So your code should be
>>         if (dev->kdrv == RTE_KDRV_VFIO)
>>
>
> Right,
>
>>> > That's why I suggest to keep the initial semantic of kdrv and
>>> > not pollute it with VFIO modes.
>>>
>>> I am okay to live with default and forget suffix __noiommu but there
>>> are implementation problem which was discussed in other thread
>>> - Virtio pmd driver should avoid interface parsing i.e.
>>> virtio_resource_init_uio/vfio() etc.. For vfio case - We could easily
>>> get rid of by moving /sys parsing to pci_eal layer, Right? If so then
>>> virtio currently works with vfio-noiommu, it make sense to me that
>>> pci_eal layer does parsing for pmd driver before that pmd driver get
>>> initialized.
>>
>> Please reword. What is the problem?
>>
>>> - Another case could be: iommu-less-pmd-driver. eal layer to do
>>> parsing before updating drv->kdrv.
>>
>> [...]
>>> >> >> > If a check is needed, I would prefer using your function
>>> >> >> > pci_vfio_is_noiommu() and remove driver modes from struct rte_kernel_driver.
>>> >> >>
>>> >> >> I don't think calling pci_vfio_no_iommu() inside
>>> >> >> virtio_reg_rd/wr_1/2/3() would be a good idea.
>>> >> >
>>> >> > Why? The value may be cached in the priv properties.
>>> >> >
>>> >> pci_vfio_is_noiommu() parses /sys for
>>> >> - enable_noiommu param
>>> >> - attached driver name is vfio-noiommu or not.
>>> >>
>>> >> It does file operation for that, I meant to say that calling this api
>>> >> within register_rd/wr function is not correct. It would be better if
>>> >> those low level register_rd/wr api only checks driver_types.
>>> >
>>> > Yes, that's why I said the return of pci_vfio_is_noiommu() may be cached
>>> > to keep efficiency.
>>>
>>> I am not convinced though, Still find pmd driver checking driver_types
>>> using drv->kdrv is better approach than introducing a new global
>>> variable which may look something like;
>>
>> Not a global variable. A function in EAL layer. A variable in PMD priv.
>>
>
> If we agreed to use condition (drv->kdrv == RTE_KDRV_VFIO);
> then resource parsing for vfio {including vfio and vfio_noiommu both
> case} is enforced in virtio pmd driver layer and that is contradicting
> to what we agreed earlier in this[1] thread. Also we don't need a
> function in EAL layer or a variable in PMD priv. Perhaps a private
> function in virtio pmd which does parsing for vfio interface.
>
> Thoughts?
>
> [1] http://dpdk.org/dev/patchwork/patch/9862/
>

Any comment/feedback on above approach?

>>> At pci_eal layer ----
>>> bool vfio_mode;
>>> vfio_mode = pci_vfio_is_noiommu();
>>>
>>> At virtio pmd driver layer ----
>>> Checking value at vfio_mode variable before doing virtio_rd/wr for
>>> vfio interface.
>>>
>>> Instead virtio pmd driver doing
>>>
>>> virtio_reg_rd/wr_1/2/4()
>>> {
>>> if (drv->kdrv == VFIO)
>>>       do pread()/pwrite()
>>> else
>>>       in()/out()
>>> }
>>>
>>> is better approach.
>>>
>>> Let me know if you still think former is better than latter then I'll
>>> send patch revision right-away.
>>
>>

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Santosh Shukla]

On Wed, Jan 27, 2016 at 4:11 PM, Santosh Shukla <sshukla@mvista.com> wrote:
> On Tue, Jan 26, 2016 at 9:51 PM, Santosh Shukla <sshukla@mvista.com> wrote:
>> On Tue, Jan 26, 2016 at 7:58 PM, Thomas Monjalon
>> <thomas.monjalon@6wind.com> wrote:
>>> 2016-01-26 19:35, Santosh Shukla:
>>>> On Tue, Jan 26, 2016 at 6:30 PM, Thomas Monjalon
>>>> <thomas.monjalon@6wind.com> wrote:
>>>> > 2016-01-26 15:56, Santosh Shukla:
>>>> >> In my observation, currently virtio work for vfio-noiommu, that's why
>>>> >> said drv->kdrv need to know vfio mode.
>>>> >
>>>> > It is your observation. It may change in near future.
>>>>
>>>> so that mean till then, virtio support for non-x86 arch has to wait?
>>>
>>> No, absolutely not. virtio for non-x86 is welcome.
>>>
>>>> We have working model with vfio-noiommu, don't you think it make sense
>>>> to let vfio_noiommu implementation exist and later in-case
>>>> virtio+iommu gets mainline then switch to vfio __mode__ agnostic
>>>> approach. And for that All it takes to replace __noiommu suffix with
>>>> default.
>>>
>>> I'm just saying you should not touch the enum rte_kernel_driver.
>>> RTE_KDRV_VFIO is a driver.
>>> RTE_KDRV_VFIO_NOIOMMU is a mode.
>>> As the VFIO API is the same in both modes, there is no reason to
>>> distinguish them at this level.
>>> Your patch adds the NOIOMMU case everywhere:
>>>         case RTE_KDRV_VFIO:
>>> +       case RTE_KDRV_VFIO_NOIOMMU:
>>>
>>> I'll stop commenting here to let others give their opinion.
>>>
>>> [...]
>>>> >> with vfio+iommu; binding virtio pci device to vfio-pci driver fail;
>>>> >> giving below error:
>>>> >> [   53.053464] VFIO - User Level meta-driver version: 0.3
>>>> >> [   73.077805] vfio-pci: probe of 0000:00:03.0 failed with error -22
>>>> >> [   73.077852] vfio-pci: probe of 0000:00:03.0 failed with error -22
>>>> >>
>>>> >> vfio_pci_probe() --> vfio_iommu_group_get() --> iommu_group_get()
>>>> >> fails: iommu doesn't have group for virtio pci device.
>>>> >
>>>> > Yes it fails when binding.
>>>> > So the later check in the virtio PMD is useless.
>>>>
>>>> Which check?
>>>
>>> The check for VFIO noiommu only:
>>> -       if (dev->kdrv == RTE_KDRV_VFIO)
>>> +       if (dev->kdrv == RTE_KDRV_VFIO_NOIOMMU)
>>>
>>> [...]
>>>> > Furthermore restricting virtio to no-iommu mode doesn't bring
>>>> > any improvement.
>>>>
>>>> We're not __restricting__, as soon as virtio+iommu gets working state,
>>>> we'll simply replace __noiommu with default. Then its upto user to try
>>>> out virtio with vfio default or vfio_noiommu.
>>>
>>> Yes it's up to user.
>>> So your code should be
>>>         if (dev->kdrv == RTE_KDRV_VFIO)
>>>
>>
>> Right,
>>
>>>> > That's why I suggest to keep the initial semantic of kdrv and
>>>> > not pollute it with VFIO modes.
>>>>
>>>> I am okay to live with default and forget suffix __noiommu but there
>>>> are implementation problem which was discussed in other thread
>>>> - Virtio pmd driver should avoid interface parsing i.e.
>>>> virtio_resource_init_uio/vfio() etc.. For vfio case - We could easily
>>>> get rid of by moving /sys parsing to pci_eal layer, Right? If so then
>>>> virtio currently works with vfio-noiommu, it make sense to me that
>>>> pci_eal layer does parsing for pmd driver before that pmd driver get
>>>> initialized.
>>>
>>> Please reword. What is the problem?
>>>
>>>> - Another case could be: iommu-less-pmd-driver. eal layer to do
>>>> parsing before updating drv->kdrv.
>>>
>>> [...]
>>>> >> >> > If a check is needed, I would prefer using your function
>>>> >> >> > pci_vfio_is_noiommu() and remove driver modes from struct rte_kernel_driver.
>>>> >> >>
>>>> >> >> I don't think calling pci_vfio_no_iommu() inside
>>>> >> >> virtio_reg_rd/wr_1/2/3() would be a good idea.
>>>> >> >
>>>> >> > Why? The value may be cached in the priv properties.
>>>> >> >
>>>> >> pci_vfio_is_noiommu() parses /sys for
>>>> >> - enable_noiommu param
>>>> >> - attached driver name is vfio-noiommu or not.
>>>> >>
>>>> >> It does file operation for that, I meant to say that calling this api
>>>> >> within register_rd/wr function is not correct. It would be better if
>>>> >> those low level register_rd/wr api only checks driver_types.
>>>> >
>>>> > Yes, that's why I said the return of pci_vfio_is_noiommu() may be cached
>>>> > to keep efficiency.
>>>>
>>>> I am not convinced though, Still find pmd driver checking driver_types
>>>> using drv->kdrv is better approach than introducing a new global
>>>> variable which may look something like;
>>>
>>> Not a global variable. A function in EAL layer. A variable in PMD priv.
>>>
>>
>> If we agreed to use condition (drv->kdrv == RTE_KDRV_VFIO);
>> then resource parsing for vfio {including vfio and vfio_noiommu both
>> case} is enforced in virtio pmd driver layer and that is contradicting
>> to what we agreed earlier in this[1] thread. Also we don't need a
>> function in EAL layer or a variable in PMD priv. Perhaps a private
>> function in virtio pmd which does parsing for vfio interface.
>>
>> Thoughts?
>>
>> [1] http://dpdk.org/dev/patchwork/patch/9862/
>>
>
> Any comment/feedback on above approach?
>

Since approach in this patch (i.e.. _noiommu suffix) is blocking patch
series acceptance, I revisited approach keeping concern raised by
Thomas/David in mind, So to summarize thread discussion;

1. virtio currently works for vfio+noiommu and likely will work for
vfio+iommu in near future.
2. So remove __noiommu suffix and always use default.
3. Introduce vfio resource parsing global function, That function
suppose to do parsing for default vfio case and for vfio-noiommu case.
This function will be used by pmd drivers for resource parsing purpose
example virtio.

Yuan won't be happy with 3) I guess, because he wanted to get rid of
interface parsing from pmd driver.

Thomas, if 1/2/3/ addresses your concern then I'll spin the series,

Thanks.



>>>> At pci_eal layer ----
>>>> bool vfio_mode;
>>>> vfio_mode = pci_vfio_is_noiommu();
>>>>
>>>> At virtio pmd driver layer ----
>>>> Checking value at vfio_mode variable before doing virtio_rd/wr for
>>>> vfio interface.
>>>>
>>>> Instead virtio pmd driver doing
>>>>
>>>> virtio_reg_rd/wr_1/2/4()
>>>> {
>>>> if (drv->kdrv == VFIO)
>>>>       do pread()/pwrite()
>>>> else
>>>>       in()/out()
>>>> }
>>>>
>>>> is better approach.
>>>>
>>>> Let me know if you still think former is better than latter then I'll
>>>> send patch revision right-away.
>>>
>>>

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Thomas Monjalon]

2016-01-27 21:02, Santosh Shukla:
> 1. virtio currently works for vfio+noiommu and likely will work for
> vfio+iommu in near future.
> 2. So remove __noiommu suffix and always use default.
> 3. Introduce vfio resource parsing global function, That function
> suppose to do parsing for default vfio case and for vfio-noiommu case.
> This function will be used by pmd drivers for resource parsing purpose
> example virtio.
> 
> Yuan won't be happy with 3) I guess, because he wanted to get rid of
> interface parsing from pmd driver.
> 
> Thomas, if 1/2/3/ addresses your concern then I'll spin the series,

I agree with 1/ and 2/.
Please, could you explain why 3/ is needed?

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Santosh Shukla]

On Wed, Jan 27, 2016 at 9:09 PM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> 2016-01-27 21:02, Santosh Shukla:
>> 1. virtio currently works for vfio+noiommu and likely will work for
>> vfio+iommu in near future.
>> 2. So remove __noiommu suffix and always use default.
>> 3. Introduce vfio resource parsing global function, That function
>> suppose to do parsing for default vfio case and for vfio-noiommu case.
>> This function will be used by pmd drivers for resource parsing purpose
>> example virtio.
>>
>> Yuan won't be happy with 3) I guess, because he wanted to get rid of
>> interface parsing from pmd driver.
>>
>> Thomas, if 1/2/3/ addresses your concern then I'll spin the series,
>
> I agree with 1/ and 2/.
> Please, could you explain why 3/ is needed?

Because someone should do resource parsing / validation before driver
does resource mapping/initialization. That someone could be either EAL
layer or driver itself.

In my case;
- driver is virtio
- resource is vfio interface

Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode

[Santosh Shukla]

On Wed, Jan 27, 2016 at 9:26 PM, Santosh Shukla <sshukla@mvista.com> wrote:
> On Wed, Jan 27, 2016 at 9:09 PM, Thomas Monjalon
> <thomas.monjalon@6wind.com> wrote:
>> 2016-01-27 21:02, Santosh Shukla:
>>> 1. virtio currently works for vfio+noiommu and likely will work for
>>> vfio+iommu in near future.
>>> 2. So remove __noiommu suffix and always use default.
>>> 3. Introduce vfio resource parsing global function, That function
>>> suppose to do parsing for default vfio case and for vfio-noiommu case.
>>> This function will be used by pmd drivers for resource parsing purpose
>>> example virtio.
>>>
>>> Yuan won't be happy with 3) I guess, because he wanted to get rid of
>>> interface parsing from pmd driver.
>>>
>>> Thomas, if 1/2/3/ addresses your concern then I'll spin the series,
>>
>> I agree with 1/ and 2/.
>> Please, could you explain why 3/ is needed?
>
> Because someone should do resource parsing / validation before driver
> does resource mapping/initialization. That someone could be either EAL
> layer or driver itself.
>
> In my case;
> - driver is virtio
> - resource is vfio interface

FWIW, removed 3) / Removed this patch entirely from this series,
Sending v6 version for effected patch [09/11]..