From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f46.google.com (mail-wm0-f46.google.com [74.125.82.46]) by dpdk.org (Postfix) with ESMTP id 6B49C9209 for ; Tue, 26 Jan 2016 15:29:42 +0100 (CET) Received: by mail-wm0-f46.google.com with SMTP id 123so108266108wmz.0 for ; Tue, 26 Jan 2016 06:29:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:organization:user-agent :in-reply-to:references:mime-version:content-transfer-encoding :content-type; bh=a1mzLvAEkLbBg/jAClyBoXr0sRwEuV2tgCyec35+EK8=; b=Ka1h51eNpDT+I86IDRlpBtBXtnbuFiUUYATfQq3rGPxjirspw+X+3lIlapGDBB/Awx Xi/PIBgs48I8aadwUVev69QHGD+xCerPUME11DP+cI0OcOcv0Tm24GObdCCgYrkAxl+y xtqtBAFc6qDSk3QQZsxgOB8pQVM8IU9z1Wbtp3sCzlkLBXe4mmtF2ZJ0Bn8N6XQwqBfz YWZMpu64p0uttRcCq9qwQF5QaO7ESEoNtFh7Xk3lQXkmXvz/aJKEDoanCwZLZMhM4+DA 0xi4/I4OfTf2oSMkMW3QpW/exIa9kPXxkdLmLLE/YHFOx5xhhB1AmaVcJq3M6NC8UBuy ukBQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=a1mzLvAEkLbBg/jAClyBoXr0sRwEuV2tgCyec35+EK8=; b=bOHslDQkmpkDFSlYj+CfcgIbI8yDMgoQMSrjk7+aj0S4Bjvry8r3fHFuiCNK19JVIZ tCSHgGXj371909GAXFNltGRz9Up5yWx7naGeNO59bMQWjh6FeZOGQ8OdU+j4lI3y38xs EdF04413kjd6Z8VC5089elDokrjQRezQcBoGYiBHWbdGyClfa7bJwNI8R2WtBWVZcbnt s45rT7dDIoxi4HEYWOLa3h481cL3yZNRa8l9vvdPlzOT+dPM3MLQwPKmG9cQJAm0BX0e Ouq12j3/yKLzj2vAeBg1pBCmFeHcQhRsm5piq8V1+IVezaNRfwKUYYi5Y64zlV5LLt++ 2dQQ== X-Gm-Message-State: AG10YORY5p+Az+7GecXrcVQWPsRc4JM8tgK8tfvzKGxKSRNtTGzKT5W+EuqkHBpZIhPDlJsf X-Received: by 10.28.51.17 with SMTP id z17mr25848337wmz.26.1453818582262; Tue, 26 Jan 2016 06:29:42 -0800 (PST) Received: from xps13.localnet (136-92-190-109.dsl.ovh.fr. [109.190.92.136]) by smtp.gmail.com with ESMTPSA id l67sm3775955wmf.11.2016.01.26.06.29.41 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 26 Jan 2016 06:29:41 -0800 (PST) From: Thomas Monjalon To: Santosh Shukla Date: Tue, 26 Jan 2016 15:28:34 +0100 Message-ID: <2443301.RnaAh4IIhO@xps13> Organization: 6WIND User-Agent: KMail/4.14.10 (Linux/4.1.6-1-ARCH; KDE/4.14.11; x86_64; ; ) In-Reply-To: References: <1453229842-15310-1-git-send-email-sshukla@mvista.com> <6703609.YCn1Se5Uby@xps13> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Cc: "Michael S. Tsirkin" , dev@dpdk.org, Alex Williamson Subject: Re: [dpdk-dev] [PATCH v6 08/11] eal: pci: introduce RTE_KDRV_VFIO_NOIOMMUi driver mode X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Jan 2016 14:29:42 -0000 2016-01-26 19:35, Santosh Shukla: > On Tue, Jan 26, 2016 at 6:30 PM, Thomas Monjalon > wrote: > > 2016-01-26 15:56, Santosh Shukla: > >> In my observation, currently virtio work for vfio-noiommu, that's why > >> said drv->kdrv need to know vfio mode. > > > > It is your observation. It may change in near future. > > so that mean till then, virtio support for non-x86 arch has to wait? No, absolutely not. virtio for non-x86 is welcome. > We have working model with vfio-noiommu, don't you think it make sense > to let vfio_noiommu implementation exist and later in-case > virtio+iommu gets mainline then switch to vfio __mode__ agnostic > approach. And for that All it takes to replace __noiommu suffix with > default. I'm just saying you should not touch the enum rte_kernel_driver. RTE_KDRV_VFIO is a driver. RTE_KDRV_VFIO_NOIOMMU is a mode. As the VFIO API is the same in both modes, there is no reason to distinguish them at this level. Your patch adds the NOIOMMU case everywhere: case RTE_KDRV_VFIO: + case RTE_KDRV_VFIO_NOIOMMU: I'll stop commenting here to let others give their opinion. [...] > >> with vfio+iommu; binding virtio pci device to vfio-pci driver fail; > >> giving below error: > >> [ 53.053464] VFIO - User Level meta-driver version: 0.3 > >> [ 73.077805] vfio-pci: probe of 0000:00:03.0 failed with error -22 > >> [ 73.077852] vfio-pci: probe of 0000:00:03.0 failed with error -22 > >> > >> vfio_pci_probe() --> vfio_iommu_group_get() --> iommu_group_get() > >> fails: iommu doesn't have group for virtio pci device. > > > > Yes it fails when binding. > > So the later check in the virtio PMD is useless. > > Which check? The check for VFIO noiommu only: - if (dev->kdrv == RTE_KDRV_VFIO) + if (dev->kdrv == RTE_KDRV_VFIO_NOIOMMU) [...] > > Furthermore restricting virtio to no-iommu mode doesn't bring > > any improvement. > > We're not __restricting__, as soon as virtio+iommu gets working state, > we'll simply replace __noiommu with default. Then its upto user to try > out virtio with vfio default or vfio_noiommu. Yes it's up to user. So your code should be if (dev->kdrv == RTE_KDRV_VFIO) > > That's why I suggest to keep the initial semantic of kdrv and > > not pollute it with VFIO modes. > > I am okay to live with default and forget suffix __noiommu but there > are implementation problem which was discussed in other thread > - Virtio pmd driver should avoid interface parsing i.e. > virtio_resource_init_uio/vfio() etc.. For vfio case - We could easily > get rid of by moving /sys parsing to pci_eal layer, Right? If so then > virtio currently works with vfio-noiommu, it make sense to me that > pci_eal layer does parsing for pmd driver before that pmd driver get > initialized. Please reword. What is the problem? > - Another case could be: iommu-less-pmd-driver. eal layer to do > parsing before updating drv->kdrv. [...] > >> >> > If a check is needed, I would prefer using your function > >> >> > pci_vfio_is_noiommu() and remove driver modes from struct rte_kernel_driver. > >> >> > >> >> I don't think calling pci_vfio_no_iommu() inside > >> >> virtio_reg_rd/wr_1/2/3() would be a good idea. > >> > > >> > Why? The value may be cached in the priv properties. > >> > > >> pci_vfio_is_noiommu() parses /sys for > >> - enable_noiommu param > >> - attached driver name is vfio-noiommu or not. > >> > >> It does file operation for that, I meant to say that calling this api > >> within register_rd/wr function is not correct. It would be better if > >> those low level register_rd/wr api only checks driver_types. > > > > Yes, that's why I said the return of pci_vfio_is_noiommu() may be cached > > to keep efficiency. > > I am not convinced though, Still find pmd driver checking driver_types > using drv->kdrv is better approach than introducing a new global > variable which may look something like; Not a global variable. A function in EAL layer. A variable in PMD priv. > At pci_eal layer ---- > bool vfio_mode; > vfio_mode = pci_vfio_is_noiommu(); > > At virtio pmd driver layer ---- > Checking value at vfio_mode variable before doing virtio_rd/wr for > vfio interface. > > Instead virtio pmd driver doing > > virtio_reg_rd/wr_1/2/4() > { > if (drv->kdrv == VFIO) > do pread()/pwrite() > else > in()/out() > } > > is better approach. > > Let me know if you still think former is better than latter then I'll > send patch revision right-away.