From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out4-smtp.messagingengine.com (out4-smtp.messagingengine.com [66.111.4.28]) by dpdk.org (Postfix) with ESMTP id 379AA1B1A2 for ; Mon, 30 Oct 2017 12:24:18 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id C32F820DAA; Mon, 30 Oct 2017 07:24:17 -0400 (EDT) Received: from frontend2 ([10.202.2.161]) by compute1.internal (MEProxy); Mon, 30 Oct 2017 07:24:17 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc; s=mesmtp; bh=Vkv0BSdFPJipPIny38JR03uVvk 4oJMP0ILA7qY5AUyM=; b=BjeitkcvrlSM1t4e98Fnc+BknFvIDF+uDu8HSH7XOD mZ0fr2xjVaq4vpRmU4VRBzvMDruBxOxm1dphUGLK/uTCwTqTVc53g4taMZYGckn9 sjCN7TGZ3K/9APfZuVorXypL+k4cW2eAfrMPo/JbpzxvjFwbozlY2mqQuAp+amgF k= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=Vkv0BS dFPJipPIny38JR03uVvk4oJMP0ILA7qY5AUyM=; b=V6ssZeIPEcukDjUm2TyFdN F7Yv9UhCvLkUwoEvdpYNpMDpcxyypGcVbE01mA+P347JVnUSJoJmzQX5Xi1KNQRP sSqJ96zL7oji00hKp3FjEcqwtFnacHcqUhs3o4dNt0uQCU/sQixKwM3O10x8blWJ yZhnT0cQpEFvX8NFSemP1yk+KeBt1BFf/uSEhgnAin/rSyHjHokhDRHy3yNVlnrc ptcreEq74G++qM4GKkWdSj3TbX2HqAzpn4fimyTfxJyjJvSKmzzHY4F8d4HVSrVP ykDyyYuqpBwiSCo5mB3qa2isR7Yy4zgjw4Z4sXOpejOxri+4aaFCV1ns1v5Forrw == X-ME-Sender: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 77F5F24784; Mon, 30 Oct 2017 07:24:17 -0400 (EDT) From: Thomas Monjalon To: =?ISO-8859-1?Q?Ga=EBtan?= Rivet Cc: dev@dpdk.org, Ferruh Yigit , Jerin Jacob Date: Mon, 30 Oct 2017 12:24:16 +0100 Message-ID: <1799529.Vp2fhR3gtB@xps> In-Reply-To: <3786691.UAh344p4j6@xps> References: <20171028062053.6615-1-jerin.jacob@caviumnetworks.com> <20171030091736.GH10890@bidouze.vm.6wind.com> <3786691.UAh344p4j6@xps> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" Subject: Re: [dpdk-dev] [PATCH] bus/pci: fix vfio mode 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: Mon, 30 Oct 2017 11:24:18 -0000 30/10/2017 10:27, Thomas Monjalon: > 30/10/2017 10:17, Ga=EBtan Rivet: > > Hi Ferruh, > >=20 > > On Mon, Oct 30, 2017 at 02:00:43AM -0700, Ferruh Yigit wrote: > > > On 10/30/2017 1:06 AM, Ga=EBtan Rivet wrote: > > > > Hi Jerin, > > > >=20 > > > > On Sat, Oct 28, 2017 at 11:50:52AM +0530, Jerin Jacob wrote: > > > >> The definition of VFIO_PRESENT is "eal_vfio.h", Fail to > > > >> include eal_vfio.h will result in disabling vfio. > > > >> > > > >> Fixes: 279b581c897d ("vfio: expose functions") > > > >> > > > >=20 > > > > Thanks for the fix, sorry for VFIO. > > > > I tried to let go of VFIO_PRESENT in the PCI patchset, unfortunatel= y I did > > > > not do a good-enough job. > > > >=20 > > > > Instead of reinstating the dependency on the private eal_vfio.h hea= der, > > > > I'd suggest replacing all VFIO_PRESENT references within the PCI bu= s by > > > > RTE_EAL_VFIO, and make the pci_vfio.c compilation depend on it with= in > > > > the linux Makefile. Something like: > > > >=20 > > > > ---8<--- > > > >=20 > > > > grep -rl VFIO_PRESENT drivers/bus/pci/linux/ |while read -r file > > > > do sed -i 's;VFIO_PRESENT;RTE_EAL_VFIO;' $file > > > > done > > >=20 > > > VFIO_PRESENT is the combination of the if user enabled VFIO and if Li= nux kernel > > > supports it. > > >=20 > > > Why not add same check and VFIO_PRESENT definition to rte_vfio.h: > > >=20 > > > --- a/lib/librte_eal/common/include/rte_vfio.h > > > +++ b/lib/librte_eal/common/include/rte_vfio.h > > > @@ -34,7 +34,13 @@ > > > #ifndef _RTE_VFIO_H_ > > > #define _RTE_VFIO_H_ > > >=20 > > > +#if !defined(VFIO_PRESENT) && defined(RTE_EAL_VFIO) > > > +#include > > > +#if LINUX_VERSION_CODE >=3D KERNEL_VERSION(3, 6, 0) > > > #include > > > +#define VFIO_PRESENT > > > +#endif /* >=3D 3.6.0 */ > > > +#endif > > > + > > > +#ifdef VFIO_PRESENT > > >=20 > > > ... Need to wrap here in case VFIO disabled ... > > >=20 > > > #endif > > >=20 > >=20 > > It would work indeed. > >=20 > > I mostly dislike having whole compilation units disabled silently on a > > linux version check. I think that if someone wanted to support kernels < > > 3.6, they ought to do the work of disabling RTE_EAL_VFIO. > >=20 > > A build error could be thrown to help those users toward the right > > solution, but I think that the meaning of having this option enabled > > should be enforced: if it is enabled, it is compiled. If dependencies > > are not met, then the option should be disabled. >=20 > +1 to avoid implicit disabling. To make it clear, we can disable VFIO automatically if not supported by the kernel at compilation time, but there should be a warning.