DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Gupta, Nipun" <nipun.gupta@amd.com>
To: "Xia, Chenbo" <chenbo.xia@intel.com>,
	David Marchand <david.marchand@redhat.com>,
	"Li, Miao" <miao.li@intel.com>,
	Maxime Coquelin <maxime.coquelin@redhat.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [PATCH v2 1/2] net/virtio: fix legacy device IO port map in secondary process
Date: Fri, 7 Jul 2023 22:33:46 +0530	[thread overview]
Message-ID: <7926ad24-0c82-e2d0-7b59-52bed94bf210@amd.com> (raw)
In-Reply-To: <SN6PR11MB350475F703A8DE53FF6A5A4D9C29A@SN6PR11MB3504.namprd11.prod.outlook.com>



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
> 

  reply	other threads:[~2023-07-07 17:04 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-06-28  6:36 [PATCH " Miao Li
2023-06-28  6:36 ` [PATCH 2/2] bus/pci: add IO port region check before region map Miao Li
2023-06-29  2:26 ` [PATCH v2 1/2] net/virtio: fix legacy device IO port map in secondary process Miao Li
2023-06-29  2:26   ` [PATCH v2 2/2] bus/pci: add IO port region check before region map Miao Li
2023-07-03  1:19   ` [PATCH v2 1/2] net/virtio: fix legacy device IO port map in secondary process Ling, WeiX
2023-07-03  7:47   ` David Marchand
2023-07-03  8:54     ` Li, Miao
2023-07-03  8:57       ` David Marchand
2023-07-03  9:31         ` Xia, Chenbo
2023-07-07 17:03           ` Gupta, Nipun [this message]
2023-07-06  8:58     ` Xia, Chenbo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7926ad24-0c82-e2d0-7b59-52bed94bf210@amd.com \
    --to=nipun.gupta@amd.com \
    --cc=chenbo.xia@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=maxime.coquelin@redhat.com \
    --cc=miao.li@intel.com \
    --cc=stable@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).