Re: [PATCH V3 21/42] vfio/pci: export MSI functions

["Cédric Le Goater" <clg@redhat.com>]

On 5/20/25 16:56, Steven Sistare wrote:
> On 5/20/2025 1:52 AM, Cédric Le Goater wrote:
>> On 5/16/25 19:58, Steven Sistare wrote:
>>> On 5/16/2025 4:31 AM, Cédric Le Goater wrote:
>>>> On 5/12/25 17:32, Steve Sistare wrote:
>>>>> Export various MSI functions, for use by CPR in subsequent patches.
>>>>> No functional change.
>>>>>
>>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>>
>>>> Please rename this routines with a 'vfio_pci' prefix.
>>>
>>> Are you sure?  That makes sense for:
>>>    vfio_vector_init -> vfio_pci_vector_init
>>>
>>> but the rest already have msi or intx in the name which unambiguously
>>> means pci.  Adding pci_ seems unecessarily verbose:
>>
>> We are slowly defining an API for an internal VFIO library. I prefer
>> to ensure the interface is clean by changing the names of external
>> services to reflect the namespace they belong to.
>>
>> All routines are implemented in hw/vfio/pci.c and most take a
>> VFIOPCIDevice as first argument.
> 
> OK.  So this:
> 
> void vfio_pci_vector_init(VFIOPCIDevice *vdev, int nr);
> void vfio_pci_add_kvm_msi_virq(VFIOPCIDevice *vdev, VFIOMSIVector *vector,
>                                 int vector_n, bool msix);
> void vfio_pci_prepare_kvm_msi_virq_batch(VFIOPCIDevice *vdev);
> void vfio_pci_commit_kvm_msi_virq_batch(VFIOPCIDevice *vdev);
> bool vfio_pci_intx_enable(VFIOPCIDevice *vdev, Error **errp);
> 
>> vfio_msi_interrupt(), vfio_msix_vector_use() and
>> vfio_msix_vector_release() are rather low level routines.
>> I think we need a wrapper to avoid exposing them.
> 
> OK.  These will do the trick, defined in pci.c and exported to cpr.c:
> 
> void vfio_pci_msix_set_notifiers(VFIOPCIDevice *vdev)
> {
>      msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
>                                vfio_msix_vector_release, NULL);
> }
> 
> void vfio_pci_msi_set_handler(VFIOPCIDevice *vdev, int nr)
> {
>      VFIOMSIVector *vector = &vdev->msi_vectors[nr];
>      int fd = event_notifier_get_fd(&vector->interrupt);
> 
>      qemu_set_fd_handler(fd, vfio_msi_interrupt, NULL, vector);
> }


LGTM,

Thanks,

C.