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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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).


-- 
Alexey