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

[David Marchand <david.marchand@redhat.com>]

On Wed, Oct 18, 2023 at 2:39 PM Gupta, Nipun <nipun.gupta@amd.com> wrote:
> 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, c() 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.

Even if rte_vfio_setup_device() is reentrant, there is still the
question when to call rte_vfio_release_device().


>
> 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?

Ok, I think I understand your suggestion.

Do we have a reference to the vfio device fd stored somewhere in the
pci device object?
I don't think it is the case, but if the pci layer keeps a reference
to it (it would be populated/reset during
rte_pci_map_device/rte_pci_unmap_device), then the ioport code can
call the VFIO_DEVICE_GET_INFO ioctl() similarly to what is done for
irq msi info, and  there is no need for a new EAL api.

For the case when this device fd is not available (no previous call to
rte_pci_map_device()), then the ioport code can call
rte_vfio_setup_device() / rte_vfio_release_device().

Is this what you have in mind?


-- 
David Marchand