Re: [Qemu-devel] [PATCH 1/2] vfio-pci: Introduce vfio_register_event_notifier helper

[Auger Eric <eric.auger@redhat.com>]

Hi Alexey,

On 1/18/19 5:14 AM, Alexey Kardashevskiy wrote:
> 
> 
> On 17/01/2019 20:16, Auger Eric wrote:
>> Hi Alexey, Cornelia,
>>
>> On 1/17/19 4:46 AM, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 12/01/2019 03:58, Eric Auger wrote:
>>>> The code used to attach the eventfd handler for the ERR and
>>>> REQ irq indices can be factorized into a helper. In subsequent
>>>> patches we will extend this helper to support other irq indices.
>>>>
>>>> We test the notification is allowed outside of the helper:
>>>> respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
>>>> Depending on the returned value we set vdev->pci_aer and
>>>> vdev->req_enabled. An error handle is introduced for future usage
>>>> although not strictly useful here.>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>>  hw/vfio/pci.c | 291 ++++++++++++++++++++++----------------------------
>>>>  1 file changed, 127 insertions(+), 164 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index c0cb1ec289..c589a4e666 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>>>>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>>>  }
>>>>  
>>>> +/*
>>>> + * vfio_register_event_notifier - setup/tear down eventfd
>>>> + * notification and handling for IRQ indices that span over
>>>> + * a single IRQ
>>>> + *
>>>> + * @vdev: VFIO device handle
>>>> + * @index: IRQ index the eventfd/handler is associated to
>>>> + * @target_state: true means notifier needs to be set up
>>>> + * @handler to attach if @target_state is true
>>>> + * @errp error handle
>>>> + */
>>>> +static int vfio_register_event_notifier(VFIOPCIDevice *vdev,
>>>> +                                        int index,
>>>> +                                        bool target_state,
>>>> +                                        void (*handler)(void *opaque),
>>>> +                                        Error **errp)
>>>> +{
>>>> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
>>>> +                                      .index = index };
>>>> +    struct vfio_irq_set *irq_set;
>>>> +    EventNotifier *notifier;
>>>> +    int argsz, ret = 0;
>>>> +    int32_t *pfd, fd;
>>>> +
>>>> +    switch (index) {
>>>
>>> I'd pass the notifier as a parameter as well so index/handler/notifier
>>> would walk together.
>>
>> I tend to agree with Cornelia. moving the notifier out of this helper
>> would remove some factorization and this way, the caller does not have
>> to care about it.
> 
> 
> Then why pass the handler? It also could go into this switch,
> vfio_register_event_notifier()/vfio_set_event_handler() is never called
> with more than one handler per index (or NULL but then target_state==false).
I don't have any strong opinion here. I will align with the majority's
opinion.

Thanks

Eric
> 
>