On Fri, 18 Jan 2019 10:08:12 +0100 Auger Eric <eric.au...@redhat.com> wrote:
> 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. If we are sure that the same index/notifier will always use the same handler when setting (i.e., not a different handler if some feature is available), we could also move getting it to the switch statement. OTOH, passing in a handler is a common pattern. Don't really have a strong opinion here.