[PATCH 1/2] net/virtio: fix legacy device IO port map in secondary process

[PATCH 1/2] net/virtio: fix legacy device IO port map in secondary process

[Miao Li]

When doing IO port map for legacy device in sencondary process,
vfio_cfg setup for leagacy device like vfio_group_fd and vfio_dev_fd
is missing. So, in sencondary process, rte_pci_map_device is added
for legacy device to setup vfio_cfg and fill region info like in
primary process.

Fixes: 512e27eeb743 ("net/virtio: move PCI specific dev init to PCI ethdev init")
Cc: stable@dpdk.org

Signed-off-by: Miao Li <miao.li@intel.com>
---
 drivers/net/virtio/virtio_pci_ethdev.c | 34 +++++++++++++-------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci_ethdev.c b/drivers/net/virtio/virtio_pci_ethdev.c
index 9b4b846f8a..dc11a6e82f 100644
--- a/drivers/net/virtio/virtio_pci_ethdev.c
+++ b/drivers/net/virtio/virtio_pci_ethdev.c
@@ -44,23 +44,23 @@ virtio_remap_pci(struct rte_pci_device *pci_dev, struct virtio_pci_dev *dev)
 {
 	struct virtio_hw *hw = &dev->hw;
 
-	if (dev->modern) {
-		/*
-		 * We don't have to re-parse the PCI config space, since
-		 * rte_pci_map_device() makes sure the mapped address
-		 * in secondary process would equal to the one mapped in
-		 * the primary process: error will be returned if that
-		 * requirement is not met.
-		 *
-		 * That said, we could simply reuse all cap pointers
-		 * (such as dev_cfg, common_cfg, etc.) parsed from the
-		 * primary process, which is stored in shared memory.
-		 */
-		if (rte_pci_map_device(pci_dev)) {
-			PMD_INIT_LOG(DEBUG, "failed to map pci device!");
-			return -1;
-		}
-	} else {
+	/*
+	 * We don't have to re-parse the PCI config space, since
+	 * rte_pci_map_device() makes sure the mapped address
+	 * in secondary process would equal to the one mapped in
+	 * the primary process: error will be returned if that
+	 * requirement is not met.
+	 *
+	 * That said, we could simply reuse all cap pointers
+	 * (such as dev_cfg, common_cfg, etc.) parsed from the
+	 * primary process, which is stored in shared memory.
+	 */
+	if (rte_pci_map_device(pci_dev)) {
+		PMD_INIT_LOG(DEBUG, "failed to map pci device!");
+		return -1;
+	}
+
+	if (!dev->modern) {
 		if (vtpci_legacy_ioport_map(hw) < 0)
 			return -1;
 	}
-- 
2.25.1

[PATCH 2/2] bus/pci: add IO port region check before region map

[Miao Li]

This patch adds IO port region check to skip region map
when doing IO port map for legacy device in sencondary process.

Fixes: 33604c31354a ("vfio: refactor PCI BAR mapping")
Cc: stable@dpdk.org

Signed-off-by: Miao Li <miao.li@intel.com>
---
 drivers/bus/pci/linux/pci_vfio.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index e634de8322..dd4ca46120 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -1099,6 +1099,15 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 	maps = vfio_res->maps;
 
 	for (i = 0; i < vfio_res->nb_maps; i++) {
+		/* chk for io port region */
+		ret = pci_vfio_is_ioport_bar(dev, vfio_dev_fd, i);
+		if (ret < 0) {
+			goto err_vfio_dev_fd;
+		} else if (ret) {
+			RTE_LOG(INFO, EAL, "Ignore mapping IO port bar(%d)\n", i);
+			continue;
+		}
+
 		if (maps[i].nr_areas > 0) {
 			ret = pci_vfio_sparse_mmap_bar(vfio_dev_fd, vfio_res, i, MAP_FIXED);
 			if (ret < 0) {
-- 
2.25.1

[PATCH v2 1/2] net/virtio: fix legacy device IO port map in secondary process

[Miao Li]

When doing IO port map for legacy device in secondary process,
vfio_cfg setup for legacy device like vfio_group_fd and vfio_dev_fd
is missing. So, in secondary process, rte_pci_map_device is added
for legacy device to setup vfio_cfg and fill in region info like in
primary process.

Fixes: 512e27eeb743 ("net/virtio: move PCI specific dev init to PCI ethdev init")
Cc: stable@dpdk.org

Signed-off-by: Miao Li <miao.li@intel.com>
---
 drivers/net/virtio/virtio_pci_ethdev.c | 34 +++++++++++++-------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci_ethdev.c b/drivers/net/virtio/virtio_pci_ethdev.c
index 9b4b846f8a..dc11a6e82f 100644
--- a/drivers/net/virtio/virtio_pci_ethdev.c
+++ b/drivers/net/virtio/virtio_pci_ethdev.c
@@ -44,23 +44,23 @@ virtio_remap_pci(struct rte_pci_device *pci_dev, struct virtio_pci_dev *dev)
 {
 	struct virtio_hw *hw = &dev->hw;
 
-	if (dev->modern) {
-		/*
-		 * We don't have to re-parse the PCI config space, since
-		 * rte_pci_map_device() makes sure the mapped address
-		 * in secondary process would equal to the one mapped in
-		 * the primary process: error will be returned if that
-		 * requirement is not met.
-		 *
-		 * That said, we could simply reuse all cap pointers
-		 * (such as dev_cfg, common_cfg, etc.) parsed from the
-		 * primary process, which is stored in shared memory.
-		 */
-		if (rte_pci_map_device(pci_dev)) {
-			PMD_INIT_LOG(DEBUG, "failed to map pci device!");
-			return -1;
-		}
-	} else {
+	/*
+	 * We don't have to re-parse the PCI config space, since
+	 * rte_pci_map_device() makes sure the mapped address
+	 * in secondary process would equal to the one mapped in
+	 * the primary process: error will be returned if that
+	 * requirement is not met.
+	 *
+	 * That said, we could simply reuse all cap pointers
+	 * (such as dev_cfg, common_cfg, etc.) parsed from the
+	 * primary process, which is stored in shared memory.
+	 */
+	if (rte_pci_map_device(pci_dev)) {
+		PMD_INIT_LOG(DEBUG, "failed to map pci device!");
+		return -1;
+	}
+
+	if (!dev->modern) {
 		if (vtpci_legacy_ioport_map(hw) < 0)
 			return -1;
 	}
-- 
2.25.1

[PATCH v2 2/2] bus/pci: add IO port region check before region map

[Miao Li]

This patch adds IO port region check to skip region map
when doing IO port map for legacy device in secondary process.

Fixes: 33604c31354a ("vfio: refactor PCI BAR mapping")
Cc: stable@dpdk.org

Signed-off-by: Miao Li <miao.li@intel.com>
---
 drivers/bus/pci/linux/pci_vfio.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index e634de8322..dd4ca46120 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -1099,6 +1099,15 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 	maps = vfio_res->maps;
 
 	for (i = 0; i < vfio_res->nb_maps; i++) {
+		/* chk for io port region */
+		ret = pci_vfio_is_ioport_bar(dev, vfio_dev_fd, i);
+		if (ret < 0) {
+			goto err_vfio_dev_fd;
+		} else if (ret) {
+			RTE_LOG(INFO, EAL, "Ignore mapping IO port bar(%d)\n", i);
+			continue;
+		}
+
 		if (maps[i].nr_areas > 0) {
 			ret = pci_vfio_sparse_mmap_bar(vfio_dev_fd, vfio_res, i, MAP_FIXED);
 			if (ret < 0) {
-- 
2.25.1

RE: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in secondary process

[Ling, WeiX]

> -----Original Message-----
> From: Miao Li <miao.li@intel.com>
> Sent: Thursday, June 29, 2023 10:27 AM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>;
> Xia, Chenbo <chenbo.xia@intel.com>; David Marchand
> <david.marchand@redhat.com>
> Subject: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in secondary
> process
> 
> When doing IO port map for legacy device in secondary process, vfio_cfg
> setup for legacy device like vfio_group_fd and vfio_dev_fd is missing. So, in
> secondary process, rte_pci_map_device is added for legacy device to setup
> vfio_cfg and fill in region info like in primary process.
> 
> Fixes: 512e27eeb743 ("net/virtio: move PCI specific dev init to PCI ethdev
> init")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Miao Li <miao.li@intel.com>
> ---

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

Re: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in secondary process

[David Marchand]

On Thu, Jun 29, 2023 at 4:27 AM Miao Li <miao.li@intel.com> wrote:
>
> When doing IO port map for legacy device in secondary process,
> vfio_cfg setup for legacy device like vfio_group_fd and vfio_dev_fd
> is missing. So, in secondary process, rte_pci_map_device is added
> for legacy device to setup vfio_cfg and fill in region info like in
> primary process.

I think, in legacy mode, there is no PCI mappable memory.
So there should be no need for this call to rte_pci_map_device.

What is missing is a vfio setup, is this correct?
I'd rather see this issue be fixed in the pci_vfio_ioport_map() function.


>> Fixes: 512e27eeb743 ("net/virtio: move PCI specific dev init to PCI ethdev init")

This commit only moved code, and at this point, there was no need for
a call to rte_pci_map_device in the secondary process case.
It seems unlikely this is a faulty change.

The recent addition on the vfio side seems a better culprit, but I am
fine with being proven wrong :-).


> Cc: stable@dpdk.org
>
> Signed-off-by: Miao Li <miao.li@intel.com>


-- 
David Marchand

RE: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in secondary process

[Li, Miao]

Hi,

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, July 3, 2023 3:48 PM
> To: Li, Miao <miao.li@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>
> Subject: Re: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in
> secondary process
> 
> On Thu, Jun 29, 2023 at 4:27 AM Miao Li <miao.li@intel.com> wrote:
> >
> > When doing IO port map for legacy device in secondary process,
> > vfio_cfg setup for legacy device like vfio_group_fd and vfio_dev_fd is
> > missing. So, in secondary process, rte_pci_map_device is added for
> > legacy device to setup vfio_cfg and fill in region info like in
> > primary process.
> 
> I think, in legacy mode, there is no PCI mappable memory.
> So there should be no need for this call to rte_pci_map_device.
> 
> What is missing is a vfio setup, is this correct?
> I'd rather see this issue be fixed in the pci_vfio_ioport_map() function.
> 
If adding vfio setup in the pci_vfio_ioport_map() function, vfio will be setup twice in primary process because rte_pci_map_device will be called for legacy device in primary process.
I add IO port region check to skip region map in the next patch.
> 
> >> Fixes: 512e27eeb743 ("net/virtio: move PCI specific dev init to PCI
> >> ethdev init")
> 
> This commit only moved code, and at this point, there was no need for a call to
> rte_pci_map_device in the secondary process case.
> It seems unlikely this is a faulty change.
> 
> The recent addition on the vfio side seems a better culprit, but I am fine with
> being proven wrong :-).
> 
Yes,  the fix commit is wrong, but not the recent addition commit on the vfio side. Because the root cause is missing a vfio setup. After adding recent addition commit, the uninitialized vfio_cfg info(like vfio_dev_fd, region info) will be used so this bug will be found. I think the correct fix commit will be 6d890f8ab512("net/virtio: fix multiple process support").
> 
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Miao Li <miao.li@intel.com>
> 
> 
> --
> David Marchand

Thanks,
Miao Li

Re: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in secondary process

[David Marchand]

On Mon, Jul 3, 2023 at 10:54 AM Li, Miao <miao.li@intel.com> wrote:
> > > When doing IO port map for legacy device in secondary process,
> > > vfio_cfg setup for legacy device like vfio_group_fd and vfio_dev_fd is
> > > missing. So, in secondary process, rte_pci_map_device is added for
> > > legacy device to setup vfio_cfg and fill in region info like in
> > > primary process.
> >
> > I think, in legacy mode, there is no PCI mappable memory.
> > So there should be no need for this call to rte_pci_map_device.
> >
> > What is missing is a vfio setup, is this correct?
> > I'd rather see this issue be fixed in the pci_vfio_ioport_map() function.
> >
> If adding vfio setup in the pci_vfio_ioport_map() function, vfio will be setup twice in primary process because rte_pci_map_device will be called for legacy device in primary process.
> I add IO port region check to skip region map in the next patch.

Well, something must be done so that it is not mapped twice, I did not
look into the details.
This current patch looks wrong to me and I understand this is not a
virtio only issue.


-- 
David Marchand

RE: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in secondary process

[Xia, Chenbo]

+Nipun

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, July 3, 2023 4:58 PM
> To: Li, Miao <miao.li@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>
> Subject: Re: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in
> secondary process
> 
> On Mon, Jul 3, 2023 at 10:54 AM Li, Miao <miao.li@intel.com> wrote:
> > > > When doing IO port map for legacy device in secondary process,
> > > > vfio_cfg setup for legacy device like vfio_group_fd and vfio_dev_fd
> is
> > > > missing. So, in secondary process, rte_pci_map_device is added for
> > > > legacy device to setup vfio_cfg and fill in region info like in
> > > > primary process.
> > >
> > > I think, in legacy mode, there is no PCI mappable memory.
> > > So there should be no need for this call to rte_pci_map_device.
> > >
> > > What is missing is a vfio setup, is this correct?
> > > I'd rather see this issue be fixed in the pci_vfio_ioport_map()
> function.
> > >
> > If adding vfio setup in the pci_vfio_ioport_map() function, vfio will be
> setup twice in primary process because rte_pci_map_device will be called
> for legacy device in primary process.
> > I add IO port region check to skip region map in the next patch.
> 
> Well, something must be done so that it is not mapped twice, I did not
> look into the details.
> This current patch looks wrong to me and I understand this is not a
> virtio only issue.

I think we could have some way to improve this:

1. Make rte_pci_map_device map either PIO or MMIO (Based on my knowledge, it's doable
for vfio. For UIO, I am no expert and not sure). For ioport, it's only about setting
up the ioport offset and size.
2. rte_pci_ioport_map may not be needed anymore.
3. struct rte_pci_ioport may not be needed anymore as the info could be saved in
struct rte_pci_device_internal.
4. ioport device uses bar #, len, offset to RW specific BAR.

Then for virtio device, either primary or secondary process only calls rte_pci_map_device
once.

Any comments?

Thanks,
Chenbo

> 
> 
> --
> David Marchand

RE: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in secondary process

[Xia, Chenbo]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, July 3, 2023 3:48 PM
> To: Li, Miao <miao.li@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>
> Subject: Re: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in
> secondary process
> 
> On Thu, Jun 29, 2023 at 4:27 AM Miao Li <miao.li@intel.com> wrote:
> >
> > When doing IO port map for legacy device in secondary process,
> > vfio_cfg setup for legacy device like vfio_group_fd and vfio_dev_fd
> > is missing. So, in secondary process, rte_pci_map_device is added
> > for legacy device to setup vfio_cfg and fill in region info like in
> > primary process.
> 
> I think, in legacy mode, there is no PCI mappable memory.
> So there should be no need for this call to rte_pci_map_device.
> 
> What is missing is a vfio setup, is this correct?
> I'd rather see this issue be fixed in the pci_vfio_ioport_map() function.

Thinking about this again: pci_vfio_ioport_map is defined to map specific ioport so
it does not make sense to do any device setup in such function. Any reason why
we can't call rte_pci_map_device in secondary/legacy? This function rte_pci_map_device
is defined to set-up device and set up BAR mapping if needed. Secondary process for any
driver needs set-up device and BAR mapping again (right?). For legacy device it can skip
the BAR mapping part, which rte_pci_map_device is already doing.

Any comments?

Thanks,
Chenbo

> 
> 
> >> Fixes: 512e27eeb743 ("net/virtio: move PCI specific dev init to PCI
> ethdev init")
> 
> This commit only moved code, and at this point, there was no need for
> a call to rte_pci_map_device in the secondary process case.
> It seems unlikely this is a faulty change.
> 
> The recent addition on the vfio side seems a better culprit, but I am
> fine with being proven wrong :-).
> 
> 
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Miao Li <miao.li@intel.com>
> 
> 
> --
> David Marchand

Re: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in secondary process

[Gupta, Nipun]

On 7/3/2023 3:01 PM, Xia, Chenbo wrote:
> +Nipun
> 
>> -----Original Message-----
>> From: David Marchand <david.marchand@redhat.com>
>> Sent: Monday, July 3, 2023 4:58 PM
>> To: Li, Miao <miao.li@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org; Maxime Coquelin
>> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>
>> Subject: Re: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in
>> secondary process
>>
>> On Mon, Jul 3, 2023 at 10:54 AM Li, Miao <miao.li@intel.com> wrote:
>>>>> When doing IO port map for legacy device in secondary process,
>>>>> vfio_cfg setup for legacy device like vfio_group_fd and vfio_dev_fd
>> is
>>>>> missing. So, in secondary process, rte_pci_map_device is added for
>>>>> legacy device to setup vfio_cfg and fill in region info like in
>>>>> primary process.
>>>>
>>>> I think, in legacy mode, there is no PCI mappable memory.
>>>> So there should be no need for this call to rte_pci_map_device.
>>>>
>>>> What is missing is a vfio setup, is this correct?
>>>> I'd rather see this issue be fixed in the pci_vfio_ioport_map()
>> function.
>>>>
>>> If adding vfio setup in the pci_vfio_ioport_map() function, vfio will be
>> setup twice in primary process because rte_pci_map_device will be called
>> for legacy device in primary process.
>>> I add IO port region check to skip region map in the next patch.
>>
>> Well, something must be done so that it is not mapped twice, I did not
>> look into the details.
>> This current patch looks wrong to me and I understand this is not a
>> virtio only issue.
> 
> I think we could have some way to improve this:
> 
> 1. Make rte_pci_map_device map either PIO or MMIO (Based on my knowledge, it's doable
> for vfio. For UIO, I am no expert and not sure). For ioport, it's only about setting
> up the ioport offset and size.
> 2. rte_pci_ioport_map may not be needed anymore.
> 3. struct rte_pci_ioport may not be needed anymore as the info could be saved in
> struct rte_pci_device_internal.
> 4. ioport device uses bar #, len, offset to RW specific BAR.
> 
> Then for virtio device, either primary or secondary process only calls rte_pci_map_device
> once.
> 
> Any comments?

Wouldn't a call to API rte_vfio_setup_device() to setup vfio_cfg, 
vfio_group_fd, vfio_dev_fd under a secondary process check suffice to 
handle IO port map for legacy device in secondary process?

I do not have much info on legacy Virtio device, and I am not clear on 
why and how for these devices rte_pci_map_device() would be called in 
case of primary process, but not in case of secondary process as 
mentioned by Miao Li?

Steps you have mentioned looks fine but note that this would cause an 
ABI breakage and as you mentioned may need changes in UIO (even I am not 
an expert in UIO).

Thanks,
Nipun

> 
> Thanks,
> Chenbo
> 
>>
>>
>> --
>> David Marchand
>