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>
---
 drivers/bus/pci/linux/pci_vfio.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
  

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;
  

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;