Re: [dpdk-dev] [PATCH v2 1/4] eal: add a new req notifier to eal interrupt

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/2/18 7:30 AM, Jeff Guo wrote:
> hi, andrew
>
> thanks for your review again, see comment as below.
>
> On 10/1/2018 5:46 PM, Andrew Rybchenko wrote:
>> On 9/30/18 5:16 PM, Jeff Guo wrote:
>>> Add a new req notifier in eal interrupt for enable vfio hotplug.
>>>
>>> Signed-off-by: Jeff Guo<jia.guo@intel.com>
>>> ---
>>> v2->v1:
>>> no change
>>> ---
>>>   lib/librte_eal/common/include/rte_eal_interrupts.h |  1 +
>>>   lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 71 
>>> ++++++++++++++++++++++
>>>   2 files changed, 72 insertions(+)
>>>
>>> diff --git a/lib/librte_eal/common/include/rte_eal_interrupts.h 
>>> b/lib/librte_eal/common/include/rte_eal_interrupts.h
>>> index 6eb4932..2c47738 100644
>>> --- a/lib/librte_eal/common/include/rte_eal_interrupts.h
>>> +++ b/lib/librte_eal/common/include/rte_eal_interrupts.h
>>> @@ -35,6 +35,7 @@ enum rte_intr_handle_type {
>>>       RTE_INTR_HANDLE_EXT,          /**< external handler */
>>>       RTE_INTR_HANDLE_VDEV,         /**< virtual device */
>>>       RTE_INTR_HANDLE_DEV_EVENT,    /**< device event handle */
>>> +    RTE_INTR_HANDLE_VFIO_REQ,  /**< vfio device handle (req) */
>>
>> Alignment looks inconsistent. Is there any reason?
>>
>
> ok, it should be consistent.
>
>
>>>       RTE_INTR_HANDLE_MAX /**< count of elements */
>>>   };
>>>   diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c 
>>> b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
>>> index 4076c6d..7f611b3 100644
>>> --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
>>> +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
>>> @@ -308,6 +308,64 @@ vfio_disable_msix(const struct rte_intr_handle 
>>> *intr_handle) {
>>>         return ret;
>>>   }
>>> +
>>> +/* enable req notifier */
>>> +static int
>>> +vfio_enable_req(const struct rte_intr_handle *intr_handle)
>>> +{
>>> +    int len, ret;
>>> +    char irq_set_buf[IRQ_SET_BUF_LEN];
>>
>> I see that it is copied from similar functions in the file, but
>> I'd suggest to initialize it with zeros using '= {};' just to make
>> sure that uninitialized on-stack data never go to kernel.
>>
>>> +    struct vfio_irq_set *irq_set;
>>> +    int *fd_ptr;
>>> +
>>> +    len = sizeof(irq_set_buf);
>>> +
>>> +    irq_set = (struct vfio_irq_set *) irq_set_buf;
>>> +    irq_set->argsz = len;
>>> +    irq_set->count = 1;
>>> +    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>>> +             VFIO_IRQ_SET_ACTION_TRIGGER;
>>> +    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
>>
>> It looks like it is the only difference (plus error log below) from
>> vfio_enable_msi(). May be it make sense to restructure code
>> to avoid duplication. Obviously it is not critical, but please, 
>> consider.
>>
>> Similar comment is applicable to vfio_disable_req() below.
>>
>
> make sense, and i found that there are many duplication could be 
> modify here, maybe we could just add param index type to
>
> some common function to enable/disable each type of the 
> interrupts/req. I suggest to make other patch set which is aim to modify
>
> eal interrupt to do it. The same as the your above comment about char 
> array initialize.  Do you agree with that?

Yes, I agree.

>
>>> +    irq_set->start = 0;
>>> +    fd_ptr = (int *) &irq_set->data;
>>> +    *fd_ptr = intr_handle->fd;
>>> +
>>> +    ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, 
>>> irq_set);
>>> +
>>> +    if (ret) {
>>> +        RTE_LOG(ERR, EAL, "Error enabling req interrupts for fd %d\n",
>>> +                        intr_handle->fd);
>>> +        return -1;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/* disable req notifier */
>>> +static int
>>> +vfio_disable_req(const struct rte_intr_handle *intr_handle)
>>> +{
>>> +    struct vfio_irq_set *irq_set;
>>> +    char irq_set_buf[IRQ_SET_BUF_LEN];
>>> +    int len, ret;
>>> +
>>> +    len = sizeof(struct vfio_irq_set);
>>> +
>>> +    irq_set = (struct vfio_irq_set *) irq_set_buf;
>>> +    irq_set->argsz = len;
>>> +    irq_set->count = 0;
>>> +    irq_set->flags = VFIO_IRQ_SET_DATA_NONE | 
>>> VFIO_IRQ_SET_ACTION_TRIGGER;
>>> +    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
>>> +    irq_set->start = 0;
>>> +
>>> +    ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, 
>>> irq_set);
>>> +
>>> +    if (ret)
>>> +        RTE_LOG(ERR, EAL, "Error disabling req interrupts for fd 
>>> %d\n",
>>> +            intr_handle->fd);
>>> +
>>> +    return ret;
>>> +}
>>>   #endif
>>>     static int
>>> @@ -556,6 +614,10 @@ rte_intr_enable(const struct rte_intr_handle 
>>> *intr_handle)
>>>           if (vfio_enable_intx(intr_handle))
>>>               return -1;
>>>           break;
>>> +    case RTE_INTR_HANDLE_VFIO_REQ:
>>> +        if (vfio_enable_req(intr_handle))
>>> +            return -1;
>>> +        break;
>>>   #endif
>>>       /* not used at this moment */
>>>       case RTE_INTR_HANDLE_DEV_EVENT:
>>> @@ -606,6 +668,11 @@ rte_intr_disable(const struct rte_intr_handle 
>>> *intr_handle)
>>>           if (vfio_disable_intx(intr_handle))
>>>               return -1;
>>>           break;
>>> +    case RTE_INTR_HANDLE_VFIO_REQ:
>>> +        if (vfio_disable_req(intr_handle))
>>> +            return -1;
>>> +        break;
>>> +
>>>   #endif
>>>       /* not used at this moment */
>>>       case RTE_INTR_HANDLE_DEV_EVENT:
>>> @@ -682,6 +749,10 @@ eal_intr_process_interrupts(struct epoll_event 
>>> *events, int nfds)
>>>               bytes_read = 0;
>>>               call = true;
>>>               break;
>>> +        case RTE_INTR_HANDLE_VFIO_REQ:
>>> +            bytes_read = 0;
>>> +            call = true;
>>> +            break;
>>>           default:
>>>               bytes_read = 1;
>>>               break;
>>