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

[Jeff Guo <jia.guo@intel.com>]

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?


>> +	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;
>