From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f46.google.com (mail-pa0-f46.google.com [209.85.220.46]) by dpdk.org (Postfix) with ESMTP id 5061A95A0 for ; Wed, 27 Jan 2016 11:41:07 +0100 (CET) Received: by mail-pa0-f46.google.com with SMTP id uo6so2990475pac.1 for ; Wed, 27 Jan 2016 02:41:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mvista-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=cUqqV2KrMK8sb3Mq1dwsUCdF9rnZ6sFOTneevgv3Kys=; b=BI6dm9wYBH1EmmYHDGaayNbx8BZBABHxT8OFUbobIVwd6uGwCiZgyv13qrENVWJg8h xMpMUtAq1krbTv+dLmDu4QiH9tTu2zRdr4sfjyux3ZQXU/O+MD/0tcA+ttbfVB19l6Gg XAYPmitJCJISQiVeFjSLNh0VmqIU48dWZvYajpk65Er9zjxJGHsiLUt8GNIE0Mz9EOKv eSBM3WUY1nIS4bNnwfNpUexavVrt3OFwJvuAZ+fI92Kz9zrSknVXcOfuRhkkrfAY26Ap AIa3Zt9FLUjmec9BbVy+TfZq2ANIrXkMmn/cHchf5ulPRNT13z0s1KwKlOp6X4zwnEfk U+lA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=cUqqV2KrMK8sb3Mq1dwsUCdF9rnZ6sFOTneevgv3Kys=; b=l3TPkNEpeiwEahdXHsktKOcgZM4k2aFcZIewRkXnE7IpaencDm25453y3MIRQrTsUC 4bzPdHxDVI8N9wqswzAyrYXM+zvgrdiw0ot8P7eIBtUiP5vJtq5m959EkBzD/BybogGT SJyy2WEKziaA1s3gfR1Ogzon5JXUkrtoChAuZk3YJ5FvIBoELc6Mw7dqmZS1GuGaHqQa g0irhb6gI2qLYlPdMVdCE0GF3j1FQmHNDWGz/u9/eURjI48BF0ojZz+0sVZYXehf6IWw KPegn/08aYG4mlrAuTLHSGkS5msenuceg3tTGaFWme7UM5bo05dGNq4Xi5Dx+pnXqCGn FkKQ== X-Gm-Message-State: AG10YOSb5Z+5zb+RBRcN5XjRnjkdtpyEvRWqexhHljPzfdN7/9SKkb4DvFrHRNlwkAZEJoTGAzRrtu9gPxrGsDqf MIME-Version: 1.0 X-Received: by 10.66.161.227 with SMTP id xv3mr41508116pab.117.1453891266755; Wed, 27 Jan 2016 02:41:06 -0800 (PST) Received: by 10.66.196.81 with HTTP; Wed, 27 Jan 2016 02:41:06 -0800 (PST) In-Reply-To: References: <1453229842-15310-1-git-send-email-sshukla@mvista.com> <6703609.YCn1Se5Uby@xps13> <2443301.RnaAh4IIhO@xps13> Date: Wed, 27 Jan 2016 16:11:06 +0530 Message-ID: From: Santosh Shukla To: Thomas Monjalon Content-Type: text/plain; charset=UTF-8 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: Wed, 27 Jan 2016 10:41:07 -0000 On Tue, Jan 26, 2016 at 9:51 PM, Santosh Shukla wrote: > On Tue, Jan 26, 2016 at 7:58 PM, Thomas Monjalon > wrote: >> 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) >> > > Right, > >>> > 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. >> > > If we agreed to use condition (drv->kdrv == RTE_KDRV_VFIO); > then resource parsing for vfio {including vfio and vfio_noiommu both > case} is enforced in virtio pmd driver layer and that is contradicting > to what we agreed earlier in this[1] thread. Also we don't need a > function in EAL layer or a variable in PMD priv. Perhaps a private > function in virtio pmd which does parsing for vfio interface. > > Thoughts? > > [1] http://dpdk.org/dev/patchwork/patch/9862/ > Any comment/feedback on above approach? >>> 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. >> >>