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 B4A324CC0 for ; Tue, 20 Mar 2018 11:56:31 +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:56:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,335,1517904000"; d="scan'208";a="209760708" 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:56:28 -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> <2936e6e0-1ec3-d937-5e83-9938d76da814@intel.com> From: "Burakov, Anatoly" Message-ID: <4a1d2b63-ee35-1bc6-4607-ea3097200d25@intel.com> Date: Tue, 20 Mar 2018 10:56:28 +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: <2936e6e0-1ec3-d937-5e83-9938d76da814@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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:56:33 -0000 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