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

["Gupta, Nipun" <nipun.gupta@amd.com>]

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