From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f181.google.com (mail-pf0-f181.google.com [209.85.192.181]) by dpdk.org (Postfix) with ESMTP id 717C89252 for ; Tue, 26 Jan 2016 17:21:23 +0100 (CET) Received: by mail-pf0-f181.google.com with SMTP id q63so103864735pfb.1 for ; Tue, 26 Jan 2016 08:21:23 -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=RL8UIyudnA9+rzRS+DxZ1shvkqx/ZYtua/mYOVeavYw=; b=BAht1KGBNJACpoLo+SkTzb9V7IyppWYExQT/Cat/oZMysm+kR7ST0kCwgwIxePEwSM sh/8Wqgs5+n5bLACA1xxT7+/cexoBBQBzRp6ySvFZqdWhIljeHHF0Kso1nRfozHKwvWH kg5IocfD079/xxvWWpc9/fQAj8DTR15wJzBiYQOsOXFKXSIJTcbknrqrEtAGo0PH3/66 iHmET4yuvax1MRHiTI6ANI8qkXMyg4JGAnuKQtuAwXG83YIqH7sn8fy3QFWG2q4wkltJ gSKWX+6RtmF7Dusvft4aGdZB7XJYjwzaAqNNTbmSDr6Hj67xj/KOp/qbEQ7szHnMxbpJ 4D9A== 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=RL8UIyudnA9+rzRS+DxZ1shvkqx/ZYtua/mYOVeavYw=; b=O9CgGTsLdxHrcSnQNCMHM0nZRtWboJJ/zvOy0faV4huhVzuJZph9TX+0w3Cs/mK61+ 13SNQ7y1TTxrHCXKj3QMi7/ZFW+OVBWg2zxKS5pu8TZDobMqRAxofebZTi973wOsD9oZ RlGY+d5lprmYPD0EUsGZJzauhkDv3sZ76UL0VjXFS0yFKfs8g25JNq6FCCqbopxMpAJn YW8EAX3hRdFhwVmHK0FrNOEljNzluJIbXRfbd3bpA6ZdjfJtNHtBcmLiA6O4p1/t/J6p TYTA3PsdF7KXIGN+xTaJz7xCFv10T9SAbBeM8+t4HNlxSqdGSaNEFli+2A5YQ/JU7nvC g2kw== X-Gm-Message-State: AG10YOQTGvxUxyoY2CvnOlM+YuasFUu+cFAcq6JGusfARmuizm1HGk5C56rM1djxCo96b502FuuFUWUJ/jbbPZ6G MIME-Version: 1.0 X-Received: by 10.98.42.81 with SMTP id q78mr34801647pfq.142.1453825282574; Tue, 26 Jan 2016 08:21:22 -0800 (PST) Received: by 10.66.196.81 with HTTP; Tue, 26 Jan 2016 08:21:22 -0800 (PST) In-Reply-To: <2443301.RnaAh4IIhO@xps13> References: <1453229842-15310-1-git-send-email-sshukla@mvista.com> <6703609.YCn1Se5Uby@xps13> <2443301.RnaAh4IIhO@xps13> Date: Tue, 26 Jan 2016 21:51:22 +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: Tue, 26 Jan 2016 16:21:23 -0000 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/ >> 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. > >