[dpdk-dev] [PATCH v2 1/4] eal: add a new req notifier to eal interrupt
Andrew Rybchenko
arybchenko at solarflare.com
Tue Oct 2 08:51:30 CEST 2018
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 at 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;
>>
More information about the dev
mailing list