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

[Cornelia Huck]

On Fri, 18 Jan 2019 10:08:12 +0100
Auger Eric <address@hidden> 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 <address@hidden>
> >>>> ---
> >>>>  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.