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

["Gupta, Nipun" <nipun.gupta@amd.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
>