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.au...@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 > >