patches for DPDK stable branches
 help / color / mirror / Atom feed
* [PATCH] bus/pci: fix legacy device IO port map in secondary process
@ 2023-08-07  1:58 Wenwu Ma
  2023-08-13  6:15 ` Gupta, Nipun
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Wenwu Ma @ 2023-08-07  1:58 UTC (permalink / raw)
  To: nipun.gupta, dev
  Cc: david.marchand, maxime.coquelin, chenbo.xia, miao.li, weix.ling,
	Wenwu Ma, stable

When doing IO port mapping for legacy device
in secondary process, the region information
is missing, so, we need to refill it.

Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
Cc: stable@dpdk.org

Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
---
 drivers/bus/pci/linux/pci_vfio.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index e634de8322..eea1c9851e 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -1306,6 +1306,11 @@ int
 pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
 		    struct rte_pci_ioport *p)
 {
+	struct vfio_device_info device_info = { .argsz = sizeof(device_info) };
+	char pci_addr[PATH_MAX] = {0};
+	int vfio_dev_fd;
+	struct rte_pci_addr *loc = &dev->addr;
+	int ret;
 	uint64_t size, offset;
 
 	if (bar < VFIO_PCI_BAR0_REGION_INDEX ||
@@ -1314,6 +1319,22 @@ pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
 		return -1;
 	}
 
+	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
+		/* store PCI address string */
+		snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
+				loc->domain, loc->bus, loc->devid, loc->function);
+
+		ret = rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
+						&vfio_dev_fd, &device_info);
+		if (ret)
+			return -1;
+
+		ret = pci_vfio_fill_regions(dev, vfio_dev_fd, &device_info);
+		if (ret)
+			return -1;
+
+	}
+
 	if (pci_vfio_get_region(dev, bar, &size, &offset) != 0) {
 		RTE_LOG(ERR, EAL, "Cannot get offset of region %d.\n", bar);
 		return -1;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] bus/pci: fix legacy device IO port map in secondary process
  2023-08-07  1:58 [PATCH] bus/pci: fix legacy device IO port map in secondary process Wenwu Ma
@ 2023-08-13  6:15 ` Gupta, Nipun
  2023-08-16  1:11   ` Ma, WenwuX
  2023-08-21  1:27 ` [PATCH v2] " Wenwu Ma
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Gupta, Nipun @ 2023-08-13  6:15 UTC (permalink / raw)
  To: Wenwu Ma, dev
  Cc: david.marchand, maxime.coquelin, chenbo.xia, miao.li, weix.ling, stable

Hi Wenwu,

On 8/7/2023 7:28 AM, Wenwu Ma wrote:
> When doing IO port mapping for legacy device
> in secondary process, the region information
> is missing, so, we need to refill it.
> 
> Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
> ---
>   drivers/bus/pci/linux/pci_vfio.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index e634de8322..eea1c9851e 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -1306,6 +1306,11 @@ int
>   pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
>   		    struct rte_pci_ioport *p)
>   {
> +	struct vfio_device_info device_info = { .argsz = sizeof(device_info) };
> +	char pci_addr[PATH_MAX] = {0};
> +	int vfio_dev_fd;
> +	struct rte_pci_addr *loc = &dev->addr;
> +	int ret;
>   	uint64_t size, offset;
>   
>   	if (bar < VFIO_PCI_BAR0_REGION_INDEX ||
> @@ -1314,6 +1319,22 @@ pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
>   		return -1;
>   	}
>   
> +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> +		/* store PCI address string */
> +		snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
> +				loc->domain, loc->bus, loc->devid, loc->function);
> +
> +		ret = rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
> +						&vfio_dev_fd, &device_info);

This looks better than fixing in "virtio_remap_pci".

Ideally, these API should be called irrespective of PRIMARY or SECONDARY 
process here. Miao Li mentioned earlier that "rte_pci_map_device" is 
called from primary process, is it via "virtio_read_caps" API? Isn't 
there any other way to detect if it is a virtio legacy device without 
calling "rte_pci_map_device"?


> +		if (ret)
> +			return -1;
> +
> +		ret = pci_vfio_fill_regions(dev, vfio_dev_fd, &device_info);
> +		if (ret)
> +			return -1;

Corresponding cleanup required in "pci_vfio_ioport_unmap"?

Thanks,
Nipun

> +
> +	}
> +
>   	if (pci_vfio_get_region(dev, bar, &size, &offset) != 0) {
>   		RTE_LOG(ERR, EAL, "Cannot get offset of region %d.\n", bar);
>   		return -1;

^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [PATCH] bus/pci: fix legacy device IO port map in secondary process
  2023-08-13  6:15 ` Gupta, Nipun
@ 2023-08-16  1:11   ` Ma, WenwuX
  0 siblings, 0 replies; 21+ messages in thread
From: Ma, WenwuX @ 2023-08-16  1:11 UTC (permalink / raw)
  To: dev
  Cc: david.marchand, maxime.coquelin, Xia, Chenbo, Li, Miao, Ling,
	WeiX, stable

Hi,

> -----Original Message-----
> From: Gupta, Nipun <nipun.gupta@amd.com>
> Sent: 2023年8月13日 14:15
> To: Ma, WenwuX <wenwux.ma@intel.com>; dev@dpdk.org
> Cc: david.marchand@redhat.com; maxime.coquelin@redhat.com; Xia,
> Chenbo <chenbo.xia@intel.com>; Li, Miao <miao.li@intel.com>; Ling, WeiX
> <weix.ling@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH] bus/pci: fix legacy device IO port map in secondary
> process
> 
> Hi Wenwu,
> 
> On 8/7/2023 7:28 AM, Wenwu Ma wrote:
> > When doing IO port mapping for legacy device in secondary process, the
> > region information is missing, so, we need to refill it.
> >
> > Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel
> > value")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
> > ---
> >   drivers/bus/pci/linux/pci_vfio.c | 21 +++++++++++++++++++++
> >   1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > b/drivers/bus/pci/linux/pci_vfio.c
> > index e634de8322..eea1c9851e 100644
> > --- a/drivers/bus/pci/linux/pci_vfio.c
> > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > @@ -1306,6 +1306,11 @@ int
> >   pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
> >   		    struct rte_pci_ioport *p)
> >   {
> > +	struct vfio_device_info device_info = { .argsz = sizeof(device_info) };
> > +	char pci_addr[PATH_MAX] = {0};
> > +	int vfio_dev_fd;
> > +	struct rte_pci_addr *loc = &dev->addr;
> > +	int ret;
> >   	uint64_t size, offset;
> >
> >   	if (bar < VFIO_PCI_BAR0_REGION_INDEX || @@ -1314,6 +1319,22
> @@
> > pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
> >   		return -1;
> >   	}
> >
> > +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > +		/* store PCI address string */
> > +		snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
> > +				loc->domain, loc->bus, loc->devid, loc-
> >function);
> > +
> > +		ret = rte_vfio_setup_device(rte_pci_get_sysfs_path(),
> pci_addr,
> > +						&vfio_dev_fd, &device_info);
> 
> This looks better than fixing in "virtio_remap_pci".
> 
> Ideally, these API should be called irrespective of PRIMARY or SECONDARY
> process here. Miao Li mentioned earlier that "rte_pci_map_device" is called
> from primary process, is it via "virtio_read_caps" API? Isn't there any other
> way to detect if it is a virtio legacy device without calling
> "rte_pci_map_device"?
> 
1. yes, it is via "virtio_read_caps".

2. if (pdev->region[bar].size == 0 && pdev->region[bar].offset == 0)
    Do you think it's better this way?

> 
> > +		if (ret)
> > +			return -1;
> > +
> > +		ret = pci_vfio_fill_regions(dev, vfio_dev_fd, &device_info);
> > +		if (ret)
> > +			return -1;
> 
> Corresponding cleanup required in "pci_vfio_ioport_unmap"?
> 
Yes, we need release group fd, I will add it.

Thanks.

> Thanks,
> Nipun
> 
> > +
> > +	}
> > +
> >   	if (pci_vfio_get_region(dev, bar, &size, &offset) != 0) {
> >   		RTE_LOG(ERR, EAL, "Cannot get offset of region %d.\n", bar);
> >   		return -1;

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2] bus/pci: fix legacy device IO port map in secondary process
  2023-08-07  1:58 [PATCH] bus/pci: fix legacy device IO port map in secondary process Wenwu Ma
  2023-08-13  6:15 ` Gupta, Nipun
@ 2023-08-21  1:27 ` Wenwu Ma
  2023-08-21  2:53   ` Stephen Hemminger
  2023-08-22  2:18 ` [PATCH v3] " Wenwu Ma
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Wenwu Ma @ 2023-08-21  1:27 UTC (permalink / raw)
  To: nipun.gupta, dev
  Cc: david.marchand, maxime.coquelin, chenbo.xia, miao.li, weix.ling,
	Wenwu Ma, stable

When doing IO port mapping for legacy device
in secondary process, the region information
is missing, so, we need to refill it.

Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
Cc: stable@dpdk.org

Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
---
 v2: add release of device in pci_vfio_ioport_unmap

---
 drivers/bus/pci/linux/pci_vfio.c | 43 ++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index e634de8322..f2cc55c431 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -1306,6 +1306,11 @@ int
 pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
 		    struct rte_pci_ioport *p)
 {
+	struct vfio_device_info device_info = { .argsz = sizeof(device_info) };
+	char pci_addr[PATH_MAX] = {0};
+	int vfio_dev_fd;
+	struct rte_pci_addr *loc = &dev->addr;
+	int ret;
 	uint64_t size, offset;
 
 	if (bar < VFIO_PCI_BAR0_REGION_INDEX ||
@@ -1314,6 +1319,22 @@ pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
 		return -1;
 	}
 
+	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
+		/* store PCI address string */
+		snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
+				loc->domain, loc->bus, loc->devid, loc->function);
+
+		ret = rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
+						&vfio_dev_fd, &device_info);
+		if (ret)
+			return -1;
+
+		ret = pci_vfio_fill_regions(dev, vfio_dev_fd, &device_info);
+		if (ret)
+			return -1;
+
+	}
+
 	if (pci_vfio_get_region(dev, bar, &size, &offset) != 0) {
 		RTE_LOG(ERR, EAL, "Cannot get offset of region %d.\n", bar);
 		return -1;
@@ -1361,8 +1382,26 @@ pci_vfio_ioport_write(struct rte_pci_ioport *p,
 int
 pci_vfio_ioport_unmap(struct rte_pci_ioport *p)
 {
-	RTE_SET_USED(p);
-	return -1;
+	char pci_addr[PATH_MAX] = {0};
+	struct rte_pci_addr *loc = &p->dev->addr;
+	int ret, vfio_dev_fd;
+
+	/* store PCI address string */
+	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
+			loc->domain, loc->bus, loc->devid, loc->function);
+
+	vfio_dev_fd = rte_intr_dev_fd_get(p->dev->intr_handle);
+	if (vfio_dev_fd < 0)
+		return -1;
+
+	ret = rte_vfio_release_device(rte_pci_get_sysfs_path(), pci_addr,
+				      vfio_dev_fd);
+	if (ret < 0) {
+		RTE_LOG(ERR, EAL, "Cannot release VFIO device\n");
+		return ret;
+	}
+
+	return 0;
 }
 
 int
-- 
2.25.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2] bus/pci: fix legacy device IO port map in secondary process
  2023-08-21  1:27 ` [PATCH v2] " Wenwu Ma
@ 2023-08-21  2:53   ` Stephen Hemminger
  2023-08-22  2:13     ` Ma, WenwuX
  0 siblings, 1 reply; 21+ messages in thread
From: Stephen Hemminger @ 2023-08-21  2:53 UTC (permalink / raw)
  To: Wenwu Ma
  Cc: nipun.gupta, dev, david.marchand, maxime.coquelin, chenbo.xia,
	miao.li, weix.ling, stable

On Mon, 21 Aug 2023 09:27:07 +0800
Wenwu Ma <wenwux.ma@intel.com> wrote:

> +	struct vfio_device_info device_info = { .argsz = sizeof(device_info) };
> +	char pci_addr[PATH_MAX] = {0}

Not sure if some tools will complain about initializing chars as zero.
Anyway, why bother since you are using it with snprintf.

Also, the new variables that are only used in the secondary case
should be declared in that if() not for whole function.




^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [PATCH v2] bus/pci: fix legacy device IO port map in secondary process
  2023-08-21  2:53   ` Stephen Hemminger
@ 2023-08-22  2:13     ` Ma, WenwuX
  0 siblings, 0 replies; 21+ messages in thread
From: Ma, WenwuX @ 2023-08-22  2:13 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: nipun.gupta, dev, david.marchand, maxime.coquelin, Xia, Chenbo,
	Li, Miao, Ling, WeiX, stable

Hi,

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: 2023年8月21日 10:53
> To: Ma, WenwuX <wenwux.ma@intel.com>
> Cc: nipun.gupta@amd.com; dev@dpdk.org; david.marchand@redhat.com;
> maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; Li,
> Miao <miao.li@intel.com>; Ling, WeiX <weix.ling@intel.com>;
> stable@dpdk.org
> Subject: Re: [PATCH v2] bus/pci: fix legacy device IO port map in secondary
> process
> 
> On Mon, 21 Aug 2023 09:27:07 +0800
> Wenwu Ma <wenwux.ma@intel.com> wrote:
> 
> > +	struct vfio_device_info device_info = { .argsz = sizeof(device_info) };
> > +	char pci_addr[PATH_MAX] = {0}
> 
> Not sure if some tools will complain about initializing chars as zero.
> Anyway, why bother since you are using it with snprintf.
> 
> Also, the new variables that are only used in the secondary case should be
> declared in that if() not for whole function.
> 
> 
Ok, thanks


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v3] bus/pci: fix legacy device IO port map in secondary process
  2023-08-07  1:58 [PATCH] bus/pci: fix legacy device IO port map in secondary process Wenwu Ma
  2023-08-13  6:15 ` Gupta, Nipun
  2023-08-21  1:27 ` [PATCH v2] " Wenwu Ma
@ 2023-08-22  2:18 ` Wenwu Ma
  2023-08-28  6:06   ` Gupta, Nipun
  2023-08-30  5:07 ` [PATCH v4] " Wenwu Ma
  2023-10-24  2:00 ` [PATCH v5] " Wenwu Ma
  4 siblings, 1 reply; 21+ messages in thread
From: Wenwu Ma @ 2023-08-22  2:18 UTC (permalink / raw)
  To: nipun.gupta, dev
  Cc: david.marchand, maxime.coquelin, chenbo.xia, miao.li, weix.ling,
	Wenwu Ma, stable

When doing IO port mapping for legacy device
in secondary process, the region information
is missing, so, we need to refill it.

Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
Cc: stable@dpdk.org

Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
---
v3:
 - adjusting variable settings
v2:
 - add release of device in pci_vfio_ioport_unmap

---
 drivers/bus/pci/linux/pci_vfio.c | 43 ++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index e634de8322..5ef26c98d1 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -1314,6 +1314,27 @@ pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
 		return -1;
 	}
 
+	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
+		struct vfio_device_info device_info = { .argsz = sizeof(device_info) };
+		char pci_addr[PATH_MAX];
+		int vfio_dev_fd;
+		struct rte_pci_addr *loc = &dev->addr;
+		int ret;
+		/* store PCI address string */
+		snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
+				loc->domain, loc->bus, loc->devid, loc->function);
+
+		ret = rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
+						&vfio_dev_fd, &device_info);
+		if (ret)
+			return -1;
+
+		ret = pci_vfio_fill_regions(dev, vfio_dev_fd, &device_info);
+		if (ret)
+			return -1;
+
+	}
+
 	if (pci_vfio_get_region(dev, bar, &size, &offset) != 0) {
 		RTE_LOG(ERR, EAL, "Cannot get offset of region %d.\n", bar);
 		return -1;
@@ -1361,8 +1382,26 @@ pci_vfio_ioport_write(struct rte_pci_ioport *p,
 int
 pci_vfio_ioport_unmap(struct rte_pci_ioport *p)
 {
-	RTE_SET_USED(p);
-	return -1;
+	char pci_addr[PATH_MAX] = {0};
+	struct rte_pci_addr *loc = &p->dev->addr;
+	int ret, vfio_dev_fd;
+
+	/* store PCI address string */
+	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
+			loc->domain, loc->bus, loc->devid, loc->function);
+
+	vfio_dev_fd = rte_intr_dev_fd_get(p->dev->intr_handle);
+	if (vfio_dev_fd < 0)
+		return -1;
+
+	ret = rte_vfio_release_device(rte_pci_get_sysfs_path(), pci_addr,
+				      vfio_dev_fd);
+	if (ret < 0) {
+		RTE_LOG(ERR, EAL, "Cannot release VFIO device\n");
+		return ret;
+	}
+
+	return 0;
 }
 
 int
-- 
2.25.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] bus/pci: fix legacy device IO port map in secondary process
  2023-08-22  2:18 ` [PATCH v3] " Wenwu Ma
@ 2023-08-28  6:06   ` Gupta, Nipun
  2023-08-29  8:00     ` Ma, WenwuX
  0 siblings, 1 reply; 21+ messages in thread
From: Gupta, Nipun @ 2023-08-28  6:06 UTC (permalink / raw)
  To: Wenwu Ma, dev
  Cc: david.marchand, maxime.coquelin, chenbo.xia, miao.li, weix.ling, stable

Hi Wenwu

On 8/22/2023 7:48 AM, Wenwu Ma wrote:
> When doing IO port mapping for legacy device
> in secondary process, the region information
> is missing, so, we need to refill it.

Please use 72 columns in the commit log

> 
> Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
> ---
> v3:
>   - adjusting variable settings
> v2:
>   - add release of device in pci_vfio_ioport_unmap
> 
> ---
>   drivers/bus/pci/linux/pci_vfio.c | 43 ++++++++++++++++++++++++++++++--
>   1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index e634de8322..5ef26c98d1 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -1314,6 +1314,27 @@ pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
>   		return -1;
>   	}
>   
> +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {

One thing I am not fully convinced is that why VFIO setup is not 
required for primary process here?

Thanks,
Nipun

^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [PATCH v3] bus/pci: fix legacy device IO port map in secondary process
  2023-08-28  6:06   ` Gupta, Nipun
@ 2023-08-29  8:00     ` Ma, WenwuX
  0 siblings, 0 replies; 21+ messages in thread
From: Ma, WenwuX @ 2023-08-29  8:00 UTC (permalink / raw)
  To: Gupta, Nipun, dev
  Cc: david.marchand, maxime.coquelin, Xia, Chenbo, Li, Miao, Ling,
	WeiX, stable



> -----Original Message-----
> From: Gupta, Nipun <nipun.gupta@amd.com>
> Sent: 2023年8月28日 14:07
> To: Ma, WenwuX <wenwux.ma@intel.com>; dev@dpdk.org
> Cc: david.marchand@redhat.com; maxime.coquelin@redhat.com; Xia,
> Chenbo <chenbo.xia@intel.com>; Li, Miao <miao.li@intel.com>; Ling, WeiX
> <weix.ling@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v3] bus/pci: fix legacy device IO port map in secondary
> process
> 
> Hi Wenwu
> 
> On 8/22/2023 7:48 AM, Wenwu Ma wrote:
> > When doing IO port mapping for legacy device in secondary process, the
> > region information is missing, so, we need to refill it.
> 
> Please use 72 columns in the commit log
> 
Ok
> >
> > Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel
> > value")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
> > ---
> > v3:
> >   - adjusting variable settings
> > v2:
> >   - add release of device in pci_vfio_ioport_unmap
> >
> > ---
> >   drivers/bus/pci/linux/pci_vfio.c | 43
> ++++++++++++++++++++++++++++++--
> >   1 file changed, 41 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > b/drivers/bus/pci/linux/pci_vfio.c
> > index e634de8322..5ef26c98d1 100644
> > --- a/drivers/bus/pci/linux/pci_vfio.c
> > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > @@ -1314,6 +1314,27 @@ pci_vfio_ioport_map(struct rte_pci_device *dev,
> int bar,
> >   		return -1;
> >   	}
> >
> > +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> 
> One thing I am not fully convinced is that why VFIO setup is not required for
> primary process here?
> 
In primary process, if virtio is not modern mode, it will call rte_pci_map_device and  pci_vfio_ioport_map, VFIO setup will be called in rte_pci_map_device, so it cannot be called in pci_vfio_ioport_map again.

pci_vfio_ioport_map will not be called if the virtio is modern mode.

> Thanks,
> Nipun

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v4] bus/pci: fix legacy device IO port map in secondary process
  2023-08-07  1:58 [PATCH] bus/pci: fix legacy device IO port map in secondary process Wenwu Ma
                   ` (2 preceding siblings ...)
  2023-08-22  2:18 ` [PATCH v3] " Wenwu Ma
@ 2023-08-30  5:07 ` Wenwu Ma
  2023-09-04 15:06   ` Gupta, Nipun
                     ` (2 more replies)
  2023-10-24  2:00 ` [PATCH v5] " Wenwu Ma
  4 siblings, 3 replies; 21+ messages in thread
From: Wenwu Ma @ 2023-08-30  5:07 UTC (permalink / raw)
  To: nipun.gupta, dev
  Cc: david.marchand, maxime.coquelin, chenbo.xia, miao.li, weix.ling,
	Wenwu Ma, stable

When doing IO port mapping for legacy device in secondary process, the
region information is missing, so, we need to refill it.

Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
Cc: stable@dpdk.org

Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
---
v4:
 - adjusting commit log
v3:
 - adjusting variable settings
v2:
 - add release of device in pci_vfio_ioport_unmap

---
 drivers/bus/pci/linux/pci_vfio.c | 43 ++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index e634de8322..5ef26c98d1 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -1314,6 +1314,27 @@ pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
 		return -1;
 	}
 
+	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
+		struct vfio_device_info device_info = { .argsz = sizeof(device_info) };
+		char pci_addr[PATH_MAX];
+		int vfio_dev_fd;
+		struct rte_pci_addr *loc = &dev->addr;
+		int ret;
+		/* store PCI address string */
+		snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
+				loc->domain, loc->bus, loc->devid, loc->function);
+
+		ret = rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
+						&vfio_dev_fd, &device_info);
+		if (ret)
+			return -1;
+
+		ret = pci_vfio_fill_regions(dev, vfio_dev_fd, &device_info);
+		if (ret)
+			return -1;
+
+	}
+
 	if (pci_vfio_get_region(dev, bar, &size, &offset) != 0) {
 		RTE_LOG(ERR, EAL, "Cannot get offset of region %d.\n", bar);
 		return -1;
@@ -1361,8 +1382,26 @@ pci_vfio_ioport_write(struct rte_pci_ioport *p,
 int
 pci_vfio_ioport_unmap(struct rte_pci_ioport *p)
 {
-	RTE_SET_USED(p);
-	return -1;
+	char pci_addr[PATH_MAX] = {0};
+	struct rte_pci_addr *loc = &p->dev->addr;
+	int ret, vfio_dev_fd;
+
+	/* store PCI address string */
+	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
+			loc->domain, loc->bus, loc->devid, loc->function);
+
+	vfio_dev_fd = rte_intr_dev_fd_get(p->dev->intr_handle);
+	if (vfio_dev_fd < 0)
+		return -1;
+
+	ret = rte_vfio_release_device(rte_pci_get_sysfs_path(), pci_addr,
+				      vfio_dev_fd);
+	if (ret < 0) {
+		RTE_LOG(ERR, EAL, "Cannot release VFIO device\n");
+		return ret;
+	}
+
+	return 0;
 }
 
 int
-- 
2.34.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process
  2023-08-30  5:07 ` [PATCH v4] " Wenwu Ma
@ 2023-09-04 15:06   ` Gupta, Nipun
  2023-09-05  3:10   ` Ling, WeiX
  2023-10-03 10:21   ` David Marchand
  2 siblings, 0 replies; 21+ messages in thread
From: Gupta, Nipun @ 2023-09-04 15:06 UTC (permalink / raw)
  To: Wenwu Ma, dev
  Cc: david.marchand, maxime.coquelin, chenbo.xia, miao.li, weix.ling, stable



On 8/30/2023 10:37 AM, Wenwu Ma wrote:
> When doing IO port mapping for legacy device in secondary process, the
> region information is missing, so, we need to refill it.
> 
> Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>

Acked-by: Nipun Gupta <nipun.gupta@amd.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process
  2023-08-30  5:07 ` [PATCH v4] " Wenwu Ma
  2023-09-04 15:06   ` Gupta, Nipun
@ 2023-09-05  3:10   ` Ling, WeiX
  2023-10-03 10:21   ` David Marchand
  2 siblings, 0 replies; 21+ messages in thread
From: Ling, WeiX @ 2023-09-05  3:10 UTC (permalink / raw)
  To: Ma, WenwuX, nipun.gupta, dev
  Cc: david.marchand, maxime.coquelin, Xia, Chenbo, Li, Miao, stable

> -----Original Message-----
> From: Ma, WenwuX <wenwux.ma@intel.com>
> Sent: Wednesday, August 30, 2023 1:07 PM
> To: nipun.gupta@amd.com; dev@dpdk.org
> Cc: david.marchand@redhat.com; maxime.coquelin@redhat.com; Xia,
> Chenbo <chenbo.xia@intel.com>; Li, Miao <miao.li@intel.com>; Ling, WeiX
> <weix.ling@intel.com>; Ma, WenwuX <wenwux.ma@intel.com>;
> stable@dpdk.org
> Subject: [PATCH v4] bus/pci: fix legacy device IO port map in secondary
> process
> 
> When doing IO port mapping for legacy device in secondary process, the
> region information is missing, so, we need to refill it.
> 
> Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>

Tested-by: Wei Ling <weix.ling@intel.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process
  2023-08-30  5:07 ` [PATCH v4] " Wenwu Ma
  2023-09-04 15:06   ` Gupta, Nipun
  2023-09-05  3:10   ` Ling, WeiX
@ 2023-10-03 10:21   ` David Marchand
  2023-10-09  3:06     ` Ma, WenwuX
  2 siblings, 1 reply; 21+ messages in thread
From: David Marchand @ 2023-10-03 10:21 UTC (permalink / raw)
  To: Wenwu Ma, nipun.gupta, chenbo.xia
  Cc: dev, maxime.coquelin, miao.li, weix.ling, stable

On Wed, Aug 30, 2023 at 7:07 AM Wenwu Ma <wenwux.ma@intel.com> wrote:
>
> When doing IO port mapping for legacy device in secondary process, the
> region information is missing, so, we need to refill it.
>
> Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
> Cc: stable@dpdk.org
>
> Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
>
> ---
>  drivers/bus/pci/linux/pci_vfio.c | 43 ++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index e634de8322..5ef26c98d1 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -1314,6 +1314,27 @@ pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
>                 return -1;
>         }
>
> +       if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> +               struct vfio_device_info device_info = { .argsz = sizeof(device_info) };
> +               char pci_addr[PATH_MAX];
> +               int vfio_dev_fd;
> +               struct rte_pci_addr *loc = &dev->addr;
> +               int ret;
> +               /* store PCI address string */
> +               snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
> +                               loc->domain, loc->bus, loc->devid, loc->function);
> +
> +               ret = rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
> +                                               &vfio_dev_fd, &device_info);

From a pci bus API pov, nothing prevents a driver from mixing memory
mapped with vfio and ioport resources (iow, calls to
rte_pci_map_resource() and rte_pci_ioport_map()).
IOW, it may not be the case with the net/virtio driver but, in theory,
rte_pci_ioport_map()/pci_vfio_ioport_map() may be called after a
rte_pci_map_resource() call.

In a similar manner, from the API pov,
rte_pci_ioport_map()/pci_vfio_ioport_map() may be called for multiple
bars.

In summary, nothing in this patch checks that vfio has been configured
already and I think we need a refcount to handle those situations.

Nipun, Chenbo, WDYT?


> +               if (ret)
> +                       return -1;

ret value is not used, so there is no need for this variable.

if (rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
        &vfio_dev_fd, &device_info) != 0)
    return -1;

> +
> +               ret = pci_vfio_fill_regions(dev, vfio_dev_fd, &device_info);
> +               if (ret)
> +                       return -1;

Same here, ret is not needed.


> +
> +       }
> +
>         if (pci_vfio_get_region(dev, bar, &size, &offset) != 0) {
>                 RTE_LOG(ERR, EAL, "Cannot get offset of region %d.\n", bar);
>                 return -1;
> @@ -1361,8 +1382,26 @@ pci_vfio_ioport_write(struct rte_pci_ioport *p,
>  int
>  pci_vfio_ioport_unmap(struct rte_pci_ioport *p)
>  {
> -       RTE_SET_USED(p);
> -       return -1;
> +       char pci_addr[PATH_MAX] = {0};
> +       struct rte_pci_addr *loc = &p->dev->addr;
> +       int ret, vfio_dev_fd;
> +
> +       /* store PCI address string */
> +       snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
> +                       loc->domain, loc->bus, loc->devid, loc->function);
> +
> +       vfio_dev_fd = rte_intr_dev_fd_get(p->dev->intr_handle);
> +       if (vfio_dev_fd < 0)
> +               return -1;

This check is odd and does not seem related.


> +
> +       ret = rte_vfio_release_device(rte_pci_get_sysfs_path(), pci_addr,
> +                                     vfio_dev_fd);
> +       if (ret < 0) {
> +               RTE_LOG(ERR, EAL, "Cannot release VFIO device\n");
> +               return ret;
> +       }


-- 
David Marchand


^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process
  2023-10-03 10:21   ` David Marchand
@ 2023-10-09  3:06     ` Ma, WenwuX
  2023-10-18 10:05       ` David Marchand
  0 siblings, 1 reply; 21+ messages in thread
From: Ma, WenwuX @ 2023-10-09  3:06 UTC (permalink / raw)
  To: David Marchand, nipun.gupta, chenbo.xia
  Cc: dev, maxime.coquelin, Li, Miao, Ling, WeiX, stable

Hi,

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: 2023年10月3日 18:21
> To: Ma, WenwuX <wenwux.ma@intel.com>; nipun.gupta@amd.com;
> chenbo.xia@outlook.com
> Cc: dev@dpdk.org; maxime.coquelin@redhat.com; Li, Miao
> <miao.li@intel.com>; Ling, WeiX <weix.ling@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v4] bus/pci: fix legacy device IO port map in secondary
> process
> 
> On Wed, Aug 30, 2023 at 7:07 AM Wenwu Ma <wenwux.ma@intel.com>
> wrote:
> >
> > When doing IO port mapping for legacy device in secondary process, the
> > region information is missing, so, we need to refill it.
> >
> > Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel
> > value")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
> >
> > ---
> >  drivers/bus/pci/linux/pci_vfio.c | 43
> > ++++++++++++++++++++++++++++++--
> >  1 file changed, 41 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > b/drivers/bus/pci/linux/pci_vfio.c
> > index e634de8322..5ef26c98d1 100644
> > --- a/drivers/bus/pci/linux/pci_vfio.c
> > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > @@ -1314,6 +1314,27 @@ pci_vfio_ioport_map(struct rte_pci_device
> *dev, int bar,
> >                 return -1;
> >         }
> >
> > +       if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > +               struct vfio_device_info device_info = { .argsz = sizeof(device_info) };
> > +               char pci_addr[PATH_MAX];
> > +               int vfio_dev_fd;
> > +               struct rte_pci_addr *loc = &dev->addr;
> > +               int ret;
> > +               /* store PCI address string */
> > +               snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
> > +                               loc->domain, loc->bus, loc->devid,
> > + loc->function);
> > +
> > +               ret = rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
> > +                                               &vfio_dev_fd,
> > + &device_info);
> 
> From a pci bus API pov, nothing prevents a driver from mixing memory
> mapped with vfio and ioport resources (iow, calls to
> rte_pci_map_resource() and rte_pci_ioport_map()).
> IOW, it may not be the case with the net/virtio driver but, in theory,
> rte_pci_ioport_map()/pci_vfio_ioport_map() may be called after a
> rte_pci_map_resource() call.
> 
> In a similar manner, from the API pov,
> rte_pci_ioport_map()/pci_vfio_ioport_map() may be called for multiple bars.
> 
> In summary, nothing in this patch checks that vfio has been configured already
> and I think we need a refcount to handle those situations.
> 
We call rte_vfio_setup_device just to get device info, we can call rte_vfio_release_device as soon as pci_vfio_fill_regions is done.
This avoids reference counting operations, do you think it works?

> Nipun, Chenbo, WDYT?
> 
> 
> > +               if (ret)
> > +                       return -1;
> 
> ret value is not used, so there is no need for this variable.
> 
> if (rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
>         &vfio_dev_fd, &device_info) != 0)
>     return -1;
> 
> > +
> > +               ret = pci_vfio_fill_regions(dev, vfio_dev_fd, &device_info);
> > +               if (ret)
> > +                       return -1;
> 
> Same here, ret is not needed.
> 
> 
> > +
> > +       }
> > +
> >         if (pci_vfio_get_region(dev, bar, &size, &offset) != 0) {
> >                 RTE_LOG(ERR, EAL, "Cannot get offset of region %d.\n", bar);
> >                 return -1;
> > @@ -1361,8 +1382,26 @@ pci_vfio_ioport_write(struct rte_pci_ioport *p,
> > int  pci_vfio_ioport_unmap(struct rte_pci_ioport *p)  {
> > -       RTE_SET_USED(p);
> > -       return -1;
> > +       char pci_addr[PATH_MAX] = {0};
> > +       struct rte_pci_addr *loc = &p->dev->addr;
> > +       int ret, vfio_dev_fd;
> > +
> > +       /* store PCI address string */
> > +       snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
> > +                       loc->domain, loc->bus, loc->devid,
> > + loc->function);
> > +
> > +       vfio_dev_fd = rte_intr_dev_fd_get(p->dev->intr_handle);
> > +       if (vfio_dev_fd < 0)
> > +               return -1;
> 
> This check is odd and does not seem related.
> 
> 
> > +
> > +       ret = rte_vfio_release_device(rte_pci_get_sysfs_path(), pci_addr,
> > +                                     vfio_dev_fd);
> > +       if (ret < 0) {
> > +               RTE_LOG(ERR, EAL, "Cannot release VFIO device\n");
> > +               return ret;
> > +       }
> 
> 
> --
> David Marchand


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process
  2023-10-09  3:06     ` Ma, WenwuX
@ 2023-10-18 10:05       ` David Marchand
  2023-10-18 12:38         ` Gupta, Nipun
  0 siblings, 1 reply; 21+ messages in thread
From: David Marchand @ 2023-10-18 10:05 UTC (permalink / raw)
  To: Ma, WenwuX, nipun.gupta, chenbo.xia
  Cc: dev, maxime.coquelin, Li, Miao, Ling, WeiX, stable

On Mon, Oct 9, 2023 at 5:06 AM Ma, WenwuX <wenwux.ma@intel.com> wrote:
> > From a pci bus API pov, nothing prevents a driver from mixing memory
> > mapped with vfio and ioport resources (iow, calls to
> > rte_pci_map_resource() and rte_pci_ioport_map()).
> > IOW, it may not be the case with the net/virtio driver but, in theory,
> > rte_pci_ioport_map()/pci_vfio_ioport_map() may be called after a
> > rte_pci_map_resource() call.
> >
> > In a similar manner, from the API pov,
> > rte_pci_ioport_map()/pci_vfio_ioport_map() may be called for multiple bars.
> >
> > In summary, nothing in this patch checks that vfio has been configured already
> > and I think we need a refcount to handle those situations.
> >
> We call rte_vfio_setup_device just to get device info, we can call rte_vfio_release_device as soon as pci_vfio_fill_regions is done.
> This avoids reference counting operations, do you think it works?

Afaics, rte_vfio_setup_device should not be called if a call to
rte_pci_map_device for this device was successful (rte_pci_map_device
itself calls rte_vfio_setup_device).
And as a consequence, calling rte_vfio_release_device cannot be done
unconditionnally neither.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process
  2023-10-18 10:05       ` David Marchand
@ 2023-10-18 12:38         ` Gupta, Nipun
  2023-10-18 13:44           ` David Marchand
  0 siblings, 1 reply; 21+ messages in thread
From: Gupta, Nipun @ 2023-10-18 12:38 UTC (permalink / raw)
  To: David Marchand, Ma, WenwuX, chenbo.xia
  Cc: dev, maxime.coquelin, Li, Miao, Ling, WeiX, stable



On 10/18/2023 3:35 PM, David Marchand wrote:
> On Mon, Oct 9, 2023 at 5:06 AM Ma, WenwuX <wenwux.ma@intel.com> wrote:
>>>  From a pci bus API pov, nothing prevents a driver from mixing memory
>>> mapped with vfio and ioport resources (iow, calls to
>>> rte_pci_map_resource() and rte_pci_ioport_map()).
>>> IOW, it may not be the case with the net/virtio driver but, in theory,
>>> rte_pci_ioport_map()/pci_vfio_ioport_map() may be called after a
>>> rte_pci_map_resource() call.
>>>
>>> In a similar manner, from the API pov,
>>> rte_pci_ioport_map()/pci_vfio_ioport_map() may be called for multiple bars.
>>>
>>> In summary, nothing in this patch checks that vfio has been configured already
>>> and I think we need a refcount to handle those situations.
>>>
>> We call rte_vfio_setup_device just to get device info, we can call rte_vfio_release_device as soon as pci_vfio_fill_regions is done.
>> This avoids reference counting operations, do you think it works?
> 
> Afaics, rte_vfio_setup_device should not be called if a call to
> rte_pci_map_device for this device was successful (rte_pci_map_device
> itself calls rte_vfio_setup_device).
> And as a consequence, calling rte_vfio_release_device cannot be done
> unconditionnally neither.

Hi David,

AFAIU, rte_vfio_setup_device() is written as re-entrant and does not 
create the DMA mapping again if it is already done for the iommu group.

When this API is called again either for a device within the same group 
or from the device for which it is already called, it mainly only does 
the work for device info get. Though not the best thing to use like 
this, but if this is called multiple times it should not have any 
negative impact.

As Wenmu mention that they need only device info from VFIO, a separate 
API to get device info can be added in eal_vfio.c/h. The device info 
portion of rte_vfio_setup_device() can be moved out to a new API, and 
rte_vfio_setup_device() can call this new API too?

Thanks,
Nipun

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process
  2023-10-18 12:38         ` Gupta, Nipun
@ 2023-10-18 13:44           ` David Marchand
  0 siblings, 0 replies; 21+ messages in thread
From: David Marchand @ 2023-10-18 13:44 UTC (permalink / raw)
  To: Gupta, Nipun
  Cc: Ma, WenwuX, chenbo.xia, dev, maxime.coquelin, Li, Miao, Ling,
	WeiX, stable

On Wed, Oct 18, 2023 at 2:39 PM Gupta, Nipun <nipun.gupta@amd.com> wrote:
> On 10/18/2023 3:35 PM, David Marchand wrote:
> > On Mon, Oct 9, 2023 at 5:06 AM Ma, WenwuX <wenwux.ma@intel.com> wrote:
> >>>  From a pci bus API pov, nothing prevents a driver from mixing memory
> >>> mapped with vfio and ioport resources (iow, calls to
> >>> rte_pci_map_resource() and rte_pci_ioport_map()).
> >>> IOW, it may not be the case with the net/virtio driver but, in theory,
> >>> rte_pci_ioport_map()/pci_vfio_ioport_map() may be called after a
> >>> rte_pci_map_resource() call.
> >>>
> >>> In a similar manner, from the API pov,
> >>> rte_pci_ioport_map()/pci_vfio_ioport_map() may be called for multiple bars.
> >>>
> >>> In summary, nothing in this patch checks that vfio has been configured already
> >>> and I think we need a refcount to handle those situations.
> >>>
> >> We call rte_vfio_setup_device just to get device info, we can call rte_vfio_release_device as soon as pci_vfio_fill_regions is done.
> >> This avoids reference counting operations, do you think it works?
> >
> > Afaics, rte_vfio_setup_device should not be called if a call to
> > rte_pci_map_device for this device was successful (rte_pci_map_device
> > itself calls rte_vfio_setup_device).
> > And as a consequence, calling rte_vfio_release_device cannot be done
> > unconditionnally neither.
>
> Hi David,
>
> AFAIU, c() is written as re-entrant and does not
> create the DMA mapping again if it is already done for the iommu group.
>
> When this API is called again either for a device within the same group
> or from the device for which it is already called, it mainly only does
> the work for device info get. Though not the best thing to use like
> this, but if this is called multiple times it should not have any
> negative impact.

Even if rte_vfio_setup_device() is reentrant, there is still the
question when to call rte_vfio_release_device().


>
> As Wenmu mention that they need only device info from VFIO, a separate
> API to get device info can be added in eal_vfio.c/h. The device info
> portion of rte_vfio_setup_device() can be moved out to a new API, and
> rte_vfio_setup_device() can call this new API too?

Ok, I think I understand your suggestion.

Do we have a reference to the vfio device fd stored somewhere in the
pci device object?
I don't think it is the case, but if the pci layer keeps a reference
to it (it would be populated/reset during
rte_pci_map_device/rte_pci_unmap_device), then the ioport code can
call the VFIO_DEVICE_GET_INFO ioctl() similarly to what is done for
irq msi info, and  there is no need for a new EAL api.

For the case when this device fd is not available (no previous call to
rte_pci_map_device()), then the ioport code can call
rte_vfio_setup_device() / rte_vfio_release_device().

Is this what you have in mind?


-- 
David Marchand


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v5] bus/pci: fix legacy device IO port map in secondary process
  2023-08-07  1:58 [PATCH] bus/pci: fix legacy device IO port map in secondary process Wenwu Ma
                   ` (3 preceding siblings ...)
  2023-08-30  5:07 ` [PATCH v4] " Wenwu Ma
@ 2023-10-24  2:00 ` Wenwu Ma
  2023-10-27 12:50   ` Gupta, Nipun
  2023-11-14 10:24   ` [PATCH v6] bus/pci: fix legacy device IO port map Mingjin Ye
  4 siblings, 2 replies; 21+ messages in thread
From: Wenwu Ma @ 2023-10-24  2:00 UTC (permalink / raw)
  To: nipun.gupta, anatoly.burakov, dev
  Cc: david.marchand, maxime.coquelin, linglix.chen, Wenwu Ma, stable

When doing IO port mapping for legacy device in secondary process, the
region information is missing, so, we need to refill it.

Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
Cc: stable@dpdk.org

Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
---
v5:
 - adding checks to vfio setup
v4:
 - adjusting commit log
v3:
 - adjusting variable settings
v2:
 - add release of device in pci_vfio_ioport_unmap

---
 drivers/bus/pci/linux/pci_vfio.c | 52 ++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 3f3201daf2..a1f7cc2421 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -1230,6 +1230,36 @@ pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
 		return -1;
 	}
 
+	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
+		struct vfio_device_info device_info = { .argsz = sizeof(device_info) };
+		char pci_addr[PATH_MAX];
+		int vfio_dev_fd;
+		struct rte_pci_addr *loc = &dev->addr;
+		/* store PCI address string */
+		snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
+				loc->domain, loc->bus, loc->devid, loc->function);
+
+		vfio_dev_fd = rte_intr_dev_fd_get(dev->intr_handle);
+		if (vfio_dev_fd < 0)
+			return -1;
+
+		if (vfio_dev_fd == 0) {
+			if (rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
+					&vfio_dev_fd, &device_info))
+				return -1;
+
+			/* we need save vfio_dev_fd, so it can be used during release */
+			if (rte_intr_dev_fd_set(dev->intr_handle, vfio_dev_fd))
+				return -1;
+		} else {
+			if (ioctl(vfio_dev_fd, VFIO_DEVICE_GET_INFO, &device_info))
+				return -1;
+		}
+
+		if (pci_vfio_fill_regions(dev, vfio_dev_fd, &device_info))
+			return -1;
+	}
+
 	if (pci_vfio_get_region(dev, bar, &size, &offset) != 0) {
 		RTE_LOG(ERR, EAL, "Cannot get offset of region %d.\n", bar);
 		return -1;
@@ -1277,8 +1307,26 @@ pci_vfio_ioport_write(struct rte_pci_ioport *p,
 int
 pci_vfio_ioport_unmap(struct rte_pci_ioport *p)
 {
-	RTE_SET_USED(p);
-	return -1;
+	char pci_addr[PATH_MAX] = {0};
+	struct rte_pci_addr *loc = &p->dev->addr;
+	int ret, vfio_dev_fd;
+
+	/* store PCI address string */
+	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
+			loc->domain, loc->bus, loc->devid, loc->function);
+
+	vfio_dev_fd = rte_intr_dev_fd_get(p->dev->intr_handle);
+	if (vfio_dev_fd < 0)
+		return -1;
+
+	ret = rte_vfio_release_device(rte_pci_get_sysfs_path(), pci_addr,
+				      vfio_dev_fd);
+	if (ret < 0) {
+		RTE_LOG(ERR, EAL, "Cannot release VFIO device\n");
+		return ret;
+	}
+
+	return 0;
 }
 
 int
-- 
2.34.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v5] bus/pci: fix legacy device IO port map in secondary process
  2023-10-24  2:00 ` [PATCH v5] " Wenwu Ma
@ 2023-10-27 12:50   ` Gupta, Nipun
  2023-11-14 10:24   ` [PATCH v6] bus/pci: fix legacy device IO port map Mingjin Ye
  1 sibling, 0 replies; 21+ messages in thread
From: Gupta, Nipun @ 2023-10-27 12:50 UTC (permalink / raw)
  To: Wenwu Ma, anatoly.burakov, dev
  Cc: david.marchand, maxime.coquelin, linglix.chen, stable



On 10/24/2023 7:30 AM, Wenwu Ma wrote:
> When doing IO port mapping for legacy device in secondary process, the
> region information is missing, so, we need to refill it.
> 
> Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
> ---
> v5:
>   - adding checks to vfio setup
> v4:
>   - adjusting commit log
> v3:
>   - adjusting variable settings
> v2:
>   - add release of device in pci_vfio_ioport_unmap
> 
> ---
>   drivers/bus/pci/linux/pci_vfio.c | 52 ++++++++++++++++++++++++++++++--
>   1 file changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index 3f3201daf2..a1f7cc2421 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -1230,6 +1230,36 @@ pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
>   		return -1;
>   	}
>   
> +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> +		struct vfio_device_info device_info = { .argsz = sizeof(device_info) };
> +		char pci_addr[PATH_MAX];
> +		int vfio_dev_fd;
> +		struct rte_pci_addr *loc = &dev->addr;
> +		/* store PCI address string */
> +		snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
> +				loc->domain, loc->bus, loc->devid, loc->function);
> +
> +		vfio_dev_fd = rte_intr_dev_fd_get(dev->intr_handle);
> +		if (vfio_dev_fd < 0)
> +			return -1;
> +
> +		if (vfio_dev_fd == 0) {
> +			if (rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
> +					&vfio_dev_fd, &device_info))
> +				return -1;
> +
> +			/* we need save vfio_dev_fd, so it can be used during release */
> +			if (rte_intr_dev_fd_set(dev->intr_handle, vfio_dev_fd))
> +				return -1;
> +		} else {
> +			if (ioctl(vfio_dev_fd, VFIO_DEVICE_GET_INFO, &device_info))
> +				return -1;
> +		}

Code upto here is to get device info, IMO we should have a separate API 
for this in VFIO layer which can be used from any driver, rather than 
being limited here.

This can go into a new API like rte_vfio_get_device_info() in eal_vfio.c.

Thanks,
Nipun

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v6] bus/pci: fix legacy device IO port map
  2023-10-24  2:00 ` [PATCH v5] " Wenwu Ma
  2023-10-27 12:50   ` Gupta, Nipun
@ 2023-11-14 10:24   ` Mingjin Ye
  2023-11-15 11:26     ` Gupta, Nipun
  1 sibling, 1 reply; 21+ messages in thread
From: Mingjin Ye @ 2023-11-14 10:24 UTC (permalink / raw)
  To: dev
  Cc: qiming.yang, Mingjin Ye, stable, Anatoly Burakov, Chenbo Xia,
	Nipun Gupta

When doing IO port mapping for legacy device in secondary process, the
region information is missing, so, we need to refill it.

Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
v6:
 - split patch
v5:
 - adding checks to vfio setup
v4:
 - adjusting commit log
v3:
 - adjusting variable settings
v2:
 - add release of device in pci_vfio_ioport_unmap
---
 drivers/bus/pci/linux/pci_vfio.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 3f3201daf2..a18161c27b 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -1230,6 +1230,32 @@ pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
 		return -1;
 	}
 
+	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
+		struct vfio_device_info device_info = { .argsz = sizeof(device_info) };
+		char pci_addr[PATH_MAX];
+		int vfio_dev_fd;
+		struct rte_pci_addr *loc = &dev->addr;
+
+		/* store PCI address string */
+		snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
+				loc->domain, loc->bus, loc->devid, loc->function);
+
+		vfio_dev_fd = rte_intr_dev_fd_get(dev->intr_handle);
+		if (vfio_dev_fd < 0) {
+			return -1;
+		} else if (vfio_dev_fd == 0) {
+			if (rte_vfio_get_device_info(rte_pci_get_sysfs_path(), pci_addr,
+				&vfio_dev_fd, &device_info) != 0)
+				return -1;
+			/* we need save vfio_dev_fd, so it can be used during release */
+			if (rte_intr_dev_fd_set(dev->intr_handle, vfio_dev_fd))
+				return -1;
+
+			if (pci_vfio_fill_regions(dev, vfio_dev_fd, &device_info) != 0)
+				return -1;
+		}
+	}
+
 	if (pci_vfio_get_region(dev, bar, &size, &offset) != 0) {
 		RTE_LOG(ERR, EAL, "Cannot get offset of region %d.\n", bar);
 		return -1;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6] bus/pci: fix legacy device IO port map
  2023-11-14 10:24   ` [PATCH v6] bus/pci: fix legacy device IO port map Mingjin Ye
@ 2023-11-15 11:26     ` Gupta, Nipun
  0 siblings, 0 replies; 21+ messages in thread
From: Gupta, Nipun @ 2023-11-15 11:26 UTC (permalink / raw)
  To: Mingjin Ye, dev; +Cc: qiming.yang, stable, Anatoly Burakov, Chenbo Xia



On 11/14/2023 3:54 PM, Mingjin Ye wrote:
> When doing IO port mapping for legacy device in secondary process, the
> region information is missing, so, we need to refill it.
> 
> Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
> v6:
>   - split patch
> v5:
>   - adding checks to vfio setup
> v4:
>   - adjusting commit log
> v3:
>   - adjusting variable settings
> v2:
>   - add release of device in pci_vfio_ioport_unmap
> ---
>   drivers/bus/pci/linux/pci_vfio.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index 3f3201daf2..a18161c27b 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -1230,6 +1230,32 @@ pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
>   		return -1;
>   	}
>   
> +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {

Please add a comment why we are doing this in secondary only. With this 
change/comment incorporated

Acked-by: Nipun Gupta <nipun.gupta@amd.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2023-11-15 11:26 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-07  1:58 [PATCH] bus/pci: fix legacy device IO port map in secondary process Wenwu Ma
2023-08-13  6:15 ` Gupta, Nipun
2023-08-16  1:11   ` Ma, WenwuX
2023-08-21  1:27 ` [PATCH v2] " Wenwu Ma
2023-08-21  2:53   ` Stephen Hemminger
2023-08-22  2:13     ` Ma, WenwuX
2023-08-22  2:18 ` [PATCH v3] " Wenwu Ma
2023-08-28  6:06   ` Gupta, Nipun
2023-08-29  8:00     ` Ma, WenwuX
2023-08-30  5:07 ` [PATCH v4] " Wenwu Ma
2023-09-04 15:06   ` Gupta, Nipun
2023-09-05  3:10   ` Ling, WeiX
2023-10-03 10:21   ` David Marchand
2023-10-09  3:06     ` Ma, WenwuX
2023-10-18 10:05       ` David Marchand
2023-10-18 12:38         ` Gupta, Nipun
2023-10-18 13:44           ` David Marchand
2023-10-24  2:00 ` [PATCH v5] " Wenwu Ma
2023-10-27 12:50   ` Gupta, Nipun
2023-11-14 10:24   ` [PATCH v6] bus/pci: fix legacy device IO port map Mingjin Ye
2023-11-15 11:26     ` Gupta, Nipun

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).