Re: [dpdk-dev] [PATCH v5] vfio: change to use generic multi-process channel

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 20-Mar-18 10:33 AM, Burakov, Anatoly wrote:
> On 19-Mar-18 6:53 AM, Tan, Jianfeng wrote:
>> Hi Anatoly,
>>
>> Thank you for the review. All your comments will be addressed in next 
>> version, except for below concern which might be taken care of in 
>> another patch if it also concerns you.
>>
>>> -----Original Message-----
>>> From: Burakov, Anatoly
>>> Sent: Wednesday, March 14, 2018 9:27 PM
>>> To: Tan, Jianfeng; dev@dpdk.org
>>> Cc: Richardson, Bruce; Ananyev, Konstantin; thomas@monjalon.net
>>> Subject: Re: [PATCH v5] vfio: change to use generic multi-process 
>>> channel
>> [...]
>>>
>>>> +    mp_req.len_param = sizeof(*p);
>>>> +    mp_req.num_fds = 0;
>>>> +
>>>> +    vfio_group_fd = -1;
>>>> +    if (rte_mp_request(&mp_req, &mp_reply, &ts) == 0 &&
>>>> +        mp_reply.nb_received == 1) {
>>>> +        mp_rep = &mp_reply.msgs[0];
>>>> +        p = (struct vfio_mp_param *)mp_rep->param;
>>>> +        if (p->result == SOCKET_OK && mp_rep->num_fds == 1) {
>>>> +            cur_grp->group_no = iommu_group_no;
>>>> +            vfio_group_fd = mp_rep->fds[0];
>>>> +            cur_grp->fd = vfio_group_fd;
>>>> +            vfio_cfg.vfio_active_groups++;
>>>>            }
>>>> +        free(mp_reply.msgs);
>>>>        }
>>>> -    return -1;
>>>> +
>>>> +    if (vfio_group_fd < 0)
>>>> +        RTE_LOG(ERR, EAL, "  cannot request group fd\n");
>>>> +    return vfio_group_fd;
>>>
>>> p->result can be SOCKET_NO_FD, in which case returned value should be
>>> zero. I think this is missing from this code. There probably should be
>>> an "else if (p->result == SOCKET_NO_FD)" clause that sets return 
>>> value to 0.
>>>
>>> You should be able to test this by trying to set up a device for VFIO
>>> that isn't bound to VFIO driver, in a secondary process.
>>
>> OK, I will fix this.
>>
>> But really, "zero" could be ambiguous as a fd could, theoretically, be 
>> zero too.
> 
> You're correct. Maybe return 0/-1 in case of success/failure and put fd 
> into a pointer? i.e.
> 
> int func(int *vfio_group_fd) {
> <...>
> *vfio_group_fd = fd;
> return 0;
> }

Or rather return 1/0/-1 depending on whether we got SOCKET_OK, 
SOCKET_NO_FD or SOCKET_ERR.
> 
>>
>> Thanks,
>> Jianfeng
>>
> 
> 


-- 
Thanks,
Anatoly