[PATCH] bus/pci: fix legacy device IO port map in secondary process

[PATCH] bus/pci: fix legacy device IO port map in secondary process

[Wenwu Ma]

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

Re: [PATCH] bus/pci: fix legacy device IO port map in secondary process

[Gupta, Nipun]

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;

RE: [PATCH] bus/pci: fix legacy device IO port map in secondary process

[Ma, WenwuX]

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;

[PATCH v2] bus/pci: fix legacy device IO port map in secondary process

[Wenwu Ma]

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

Re: [PATCH v2] bus/pci: fix legacy device IO port map in secondary process

[Stephen Hemminger]

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.

RE: [PATCH v2] bus/pci: fix legacy device IO port map in secondary process

[Ma, WenwuX]

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

[PATCH v3] bus/pci: fix legacy device IO port map in secondary process

[Wenwu Ma]

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

Re: [PATCH v3] bus/pci: fix legacy device IO port map in secondary process

[Gupta, Nipun]

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

RE: [PATCH v3] bus/pci: fix legacy device IO port map in secondary process

[Ma, WenwuX]

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

[PATCH v4] bus/pci: fix legacy device IO port map in secondary process

[Wenwu Ma]

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

Re: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process

[Gupta, Nipun]

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>

RE: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process

[Ling, WeiX]

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

Re: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process

[David Marchand]

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

RE: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process

[Ma, WenwuX]

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

Re: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process

[David Marchand]

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

Re: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process

[Gupta, Nipun]

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

Re: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process

[David Marchand]

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

[PATCH v5] bus/pci: fix legacy device IO port map in secondary process

[Wenwu Ma]

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

Re: [PATCH v5] bus/pci: fix legacy device IO port map in secondary process

[Gupta, Nipun]

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

[PATCH v6] bus/pci: fix legacy device IO port map

[Mingjin Ye]

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

Re: [PATCH v6] bus/pci: fix legacy device IO port map

[Gupta, Nipun]

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>

[PATCH v7 0/2] fix legacy device missing region info

[Mingjin Ye]

Fill in missing region information for legacy devices in secondary
processes. And added rte_vfio_get_device_info API for EAL, which
makes it easier and more generic to get device information through
vfio driver.

Mingjin Ye (2):
  vfio: add get device info API
  bus/pci: fix legacy device missing region info

 drivers/bus/pci/linux/pci_vfio.c | 26 ++++++++++++++++++++++++++
 lib/eal/include/rte_vfio.h       | 29 +++++++++++++++++++++++++++++
 lib/eal/linux/eal_vfio.c         | 27 +++++++++++++++++++++++++++
 lib/eal/version.map              |  3 +++
 4 files changed, 85 insertions(+)

-- 
2.25.1

[PATCH v7 1/2] vfio: add get device info API

[Mingjin Ye]

This patch adds an API to support getting device information.

The driver can use the "rte_vfio_get_device_info" helper to get
device information from EAL.

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
 lib/eal/include/rte_vfio.h | 29 +++++++++++++++++++++++++++++
 lib/eal/linux/eal_vfio.c   | 27 +++++++++++++++++++++++++++
 lib/eal/version.map        |  3 +++
 3 files changed, 59 insertions(+)

diff --git a/lib/eal/include/rte_vfio.h b/lib/eal/include/rte_vfio.h
index 22832afd0f..50c4f5b617 100644
--- a/lib/eal/include/rte_vfio.h
+++ b/lib/eal/include/rte_vfio.h
@@ -17,6 +17,8 @@ extern "C" {
 #include <stdbool.h>
 #include <stdint.h>
 
+#include <rte_compat.h>
+
 /*
  * determine if VFIO is present on the system
  */
@@ -239,6 +241,33 @@ int
 rte_vfio_get_group_num(const char *sysfs_base,
 		      const char *dev_addr, int *iommu_group_num);
 
+/**
+ * Get device information
+ *
+ * This function is only relevant to linux and will return
+ * an error on BSD.
+ *
+ * @param sysfs_base
+ *   sysfs path prefix.
+ *
+ * @param dev_addr
+ *   device location.
+ *
+ * @param vfio_dev_fd
+ *   VFIO fd.
+ *
+ * @param device_info
+ *   Device information.
+ *
+ * @return
+ *   0 on success.
+ *   <0 on failure.
+ */
+__rte_experimental
+int
+rte_vfio_get_device_info(const char *sysfs_base, const char *dev_addr,
+			int *vfio_dev_fd, struct vfio_device_info *device_info);
+
 /**
  * Open a new VFIO container fd
  *
diff --git a/lib/eal/linux/eal_vfio.c b/lib/eal/linux/eal_vfio.c
index ad3c1654b2..05a0091006 100644
--- a/lib/eal/linux/eal_vfio.c
+++ b/lib/eal/linux/eal_vfio.c
@@ -1222,6 +1222,33 @@ vfio_set_iommu_type(int vfio_container_fd)
 	return NULL;
 }
 
+int
+rte_vfio_get_device_info(const char *sysfs_base, const char *dev_addr,
+			int *vfio_dev_fd, struct vfio_device_info *device_info)
+{
+	int ret;
+
+	if (!device_info || *vfio_dev_fd < 0)
+		return -1;
+
+	if (*vfio_dev_fd == 0) {
+		ret = rte_vfio_setup_device(sysfs_base, dev_addr,
+				vfio_dev_fd, device_info);
+		if (ret)
+			return -1;
+	} else {
+		ret = ioctl(*vfio_dev_fd, VFIO_DEVICE_GET_INFO, device_info);
+		if (ret) {
+			RTE_LOG(ERR, EAL, "%s cannot get device info, "
+					"error %i (%s)\n", dev_addr, errno,
+					strerror(errno));
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
 int
 vfio_has_supported_extensions(int vfio_container_fd)
 {
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 5e0cd47c82..c06ceaad50 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -393,6 +393,9 @@ EXPERIMENTAL {
 	# added in 23.07
 	rte_memzone_max_get;
 	rte_memzone_max_set;
+
+	# added in 23.11
+	rte_vfio_get_device_info; # WINDOWS_NO_EXPORT
 };
 
 INTERNAL {
-- 
2.25.1

RE: [PATCH v7 1/2] vfio: add get device info API

[Gupta, Nipun]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Mingjin Ye <mingjinx.ye@intel.com>
> Sent: Wednesday, November 22, 2023 3:53 PM
> To: dev@dpdk.org
> Cc: qiming.yang@intel.com; Mingjin Ye <mingjinx.ye@intel.com>; Anatoly
> Burakov <anatoly.burakov@intel.com>
> Subject: [PATCH v7 1/2] vfio: add get device info API
>
> This patch adds an API to support getting device information.
>
> The driver can use the "rte_vfio_get_device_info" helper to get
> device information from EAL.
>
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

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

Re: [PATCH v7 1/2] vfio: add get device info API

[Chenbo Xia]

On Nov 22, 2023, at 18:22, Mingjin Ye <mingjinx.ye@intel.com> wrote:
> 
> External email: Use caution opening links or attachments
> 
> 
> This patch adds an API to support getting device information.
> 
> The driver can use the "rte_vfio_get_device_info" helper to get
> device information from EAL.
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
> lib/eal/include/rte_vfio.h | 29 +++++++++++++++++++++++++++++
> lib/eal/linux/eal_vfio.c   | 27 +++++++++++++++++++++++++++
> lib/eal/version.map        |  3 +++
> 3 files changed, 59 insertions(+)
> 

Comments not addressed:

https://mails.dpdk.org/archives/dev/2023-November/282200.html

/Chenbo

RE: [PATCH v7 1/2] vfio: add get device info API

[Ye, MingjinX]

> -----Original Message-----
> From: Chenbo Xia <chenbox@nvidia.com>
> Sent: Wednesday, November 29, 2023 9:48 AM
> To: Ye, MingjinX <mingjinx.ye@intel.com>
> Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Burakov,
> Anatoly <anatoly.burakov@intel.com>
> Subject: Re: [PATCH v7 1/2] vfio: add get device info API
> 
> On Nov 22, 2023, at 18:22, Mingjin Ye <mingjinx.ye@intel.com> wrote:
> >
> > External email: Use caution opening links or attachments
> >
> >
> > This patch adds an API to support getting device information.
> >
> > The driver can use the "rte_vfio_get_device_info" helper to get device
> > information from EAL.
> >
> > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> > ---
> > lib/eal/include/rte_vfio.h | 29 +++++++++++++++++++++++++++++
> > lib/eal/linux/eal_vfio.c   | 27 +++++++++++++++++++++++++++
> > lib/eal/version.map        |  3 +++
> > 3 files changed, 59 insertions(+)
> >
> 
> Comments not addressed:
> 
> https://mails.dpdk.org/archives/dev/2023-November/282200.html

Sorry for not responding in time.

Experimental API, no CC stable.

This API is used essentially the same as the rte_vfio_setup_device API.
Reference: https://patchwork.dpdk.org/project/dpdk/patch/20231122102232.108299-3-mingjinx.ye@intel.com/

/Mingjin

[PATCH v7 2/2] bus/pci: fix legacy device missing region info

[Mingjin Ye]

If the legacy device in the primary process does not support mapping the
io port bar, the secondary process needs to proactively get the device
information and fil the region information.

Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
v7: Modify commit log
---
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..ee4cfb2598 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) != 0)
+				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

Re: [PATCH v7 2/2] bus/pci: fix legacy device missing region info

[Gupta, Nipun]

On 11/22/2023 3:52 PM, Mingjin Ye wrote:
> If the legacy device in the primary process does not support mapping the
> io port bar, the secondary process needs to proactively get the device
> information and fil the region information.
> 
> Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

I have already sent my Acked-by on v6. Once you receive an Ack, you need 
to add it in the subsequent versions.

For this one I am again doing for the committer.

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

Re: [PATCH v7 2/2] bus/pci: fix legacy device missing region info

[Thomas Monjalon]

24/11/2023 11:47, Gupta, Nipun:
> 
> On 11/22/2023 3:52 PM, Mingjin Ye wrote:
> > If the legacy device in the primary process does not support mapping the
> > io port bar, the secondary process needs to proactively get the device
> > information and fil the region information.
> > 
> > Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
> > 
> > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> 
> I have already sent my Acked-by on v6. Once you receive an Ack, you need 
> to add it in the subsequent versions.
> 
> For this one I am again doing for the committer.
> 
> Acked-by: Nipun Gupta <nipun.gupta@amd.com>

Series applied with a bit of rebase and styling fixes, thanks.

RE: [PATCH v7 0/2] fix legacy device missing region info

[Ye, MingjinX]

Hi,

can you please take a look at this patch.

Thanks,
Mingjin

> -----Original Message-----
> From: Ye, MingjinX <mingjinx.ye@intel.com>
> Sent: Wednesday, November 22, 2023 6:23 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Ye, MingjinX
> <mingjinx.ye@intel.com>
> Subject: [PATCH v7 0/2] fix legacy device missing region info
> 
> Fill in missing region information for legacy devices in secondary processes.
> And added rte_vfio_get_device_info API for EAL, which makes it easier and
> more generic to get device information through vfio driver.
> 
> Mingjin Ye (2):
>   vfio: add get device info API
>   bus/pci: fix legacy device missing region info
> 
>  drivers/bus/pci/linux/pci_vfio.c | 26 ++++++++++++++++++++++++++
>  lib/eal/include/rte_vfio.h       | 29 +++++++++++++++++++++++++++++
>  lib/eal/linux/eal_vfio.c         | 27 +++++++++++++++++++++++++++
>  lib/eal/version.map              |  3 +++
>  4 files changed, 85 insertions(+)
> 
> --
> 2.25.1