From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 7625044CF for ; Tue, 20 Mar 2018 11:33:03 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Mar 2018 03:33:02 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,335,1517904000"; d="scan'208";a="209755808" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.112]) ([10.237.220.112]) by orsmga005.jf.intel.com with ESMTP; 20 Mar 2018 03:33:00 -0700 To: "Tan, Jianfeng" , "dev@dpdk.org" Cc: "Richardson, Bruce" , "Ananyev, Konstantin" , "thomas@monjalon.net" References: <1515643654-129489-5-git-send-email-jianfeng.tan@intel.com> <1520175456-52990-1-git-send-email-jianfeng.tan@intel.com> From: "Burakov, Anatoly" Message-ID: <2936e6e0-1ec3-d937-5e83-9938d76da814@intel.com> Date: Tue, 20 Mar 2018 10:33:00 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v5] vfio: change to use generic multi-process channel X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Mar 2018 10:33:04 -0000 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; } > > Thanks, > Jianfeng > -- Thanks, Anatoly