From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f52.google.com (mail-pa0-f52.google.com [209.85.220.52]) by dpdk.org (Postfix) with ESMTP id 160A0952 for ; Tue, 29 Sep 2015 21:25:35 +0200 (CEST) Received: by pacfv12 with SMTP id fv12so14726903pac.2 for ; Tue, 29 Sep 2015 12:25:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-type:content-transfer-encoding; bh=oC7YvTmBiRPJlTeH4kSc+ZqaWHNGKCBt0sJqdsDHYlc=; b=eSU6OE16kU4JDDd/v5fjP3xJQJoOB303sD2Takat16l4sEEh1WbpGUC+5DV3NYFPpb Sqwk8GWNrxVRNvUcltiuqQwqOrwrv+PZNB8ym/YAV3aC9QsDZJaRfTEh2Zq7UY416HW9 4UdYESTv6fOblSMAHJCGx1qq6U/pjWK3nFS//D6/hC/Atjwmx6Wj8wfm5e+VaSva2T6u 6Ibc1m2/uKSdaBt76p7jHJy2n2Ss7C89XbackTdCLFXX2/YVedSOw75Hgui2mvOp6Dwb s4kRhVSqvxmvEGtevDMlgRaMpyrj5w/s5HrSbikBQhAcAHX04RmbF7Qi4UyMzB3hCeEV U3RA== X-Gm-Message-State: ALoCoQkDbf4TvThlQlDQZ2lzlntu1H3LHYs+paZTr4tF1hHPPgrTZWIFF5zh5X9nZffnajeC+XSO X-Received: by 10.68.201.168 with SMTP id kb8mr551521pbc.95.1443554734133; Tue, 29 Sep 2015 12:25:34 -0700 (PDT) Received: from urahara (static-50-53-82-155.bvtn.or.frontiernet.net. [50.53.82.155]) by smtp.gmail.com with ESMTPSA id ux7sm22280936pac.10.2015.09.29.12.25.33 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 29 Sep 2015 12:25:33 -0700 (PDT) Date: Tue, 29 Sep 2015 12:25:42 -0700 From: Stephen Hemminger To: Neil Horman Message-ID: <20150929122542.419781cc@urahara> In-Reply-To: <20150310131428.GB7873@hmsreliant.think-freely.org> References: <1425912999-13118-1-git-send-email-david.marchand@6wind.com> <1425912999-13118-3-git-send-email-david.marchand@6wind.com> <20150310131428.GB7873@hmsreliant.think-freely.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH 2/2] virtio: change io privilege level as early as possible 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, 29 Sep 2015 19:25:35 -0000 On Tue, 10 Mar 2015 09:14:28 -0400 Neil Horman wrote: > On Mon, Mar 09, 2015 at 03:56:39PM +0100, David Marchand wrote: > > Playing with virtio link status triggers a segfault because of an incorrect io > > privilege level. > > > > To reproduce the problem, virtio device must be bound to igb_uio to have lsc > > enabled. > > > > $ lspci |grep Ethernet > > 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device > > $ modprobe uio > > $ insmod ./x86_64-native-linuxapp-gcc/kmod/igb_uio.ko > > $ echo 0000:00:03.0 > /sys/bus/pci/devices/0000\:00\:03.0/driver/unbind > > $ echo 1af4 1000 > /sys/bus/pci/drivers/igb_uio/new_id > > $ ./x86_64-native-linuxapp-gcc/app/testpmd -c 0x6 -n 3 -w 0000:00:03.0 -- -i --txqflags=0xf01 > > [snip] > > EAL: PCI device 0000:00:03.0 on NUMA socket -1 > > EAL: probe driver: 1af4:1000 rte_virtio_pmd > > Interactive-mode selected > > Configuring Port 0 (socket 0) > > Port 0: DE:AD:DE:01:02:03 > > Checking link statuses... > > Port 0 Link Up - speed 10000 Mbps - full-duplex > > Done > > testpmd> > > > > Then, from qemu monitor: > > (qemu) set_link virtio-net-pci.0 off > > > > testpmd> Segmentation fault > > > > A call to rte_eal_iopl_init() in a specific constructor has been added so that, > > on Linux, iopl() is called asap: > > - for statically linked virtio pmd, the constructor will be called at binary > > start, > > - for shared virtio pmd (loaded using -d eal option), it will be called at > > dlopen() in rte_eal_init(). > > > > On linux, iopl() effects are inherited by children threads, so calling it in a > > constructor will ensure that any thread / process created after rte_eal_init() > > inherits the io privilege level needed by the pmd. > > > > We keep call to rte_eal_iopl_init() in the pmd init macro so that the pmd will > > not register if the io privilege level is insufficient. > > > > Besides, I moved rte_virtio_pmd_init() and related structure near its calling > > site, so that all init code can be looked at in a glance. > > The only change here is a new comment. > > > > Signed-off-by: David Marchand > > --- > > lib/librte_pmd_virtio/virtio_ethdev.c | 71 ++++++++++++++++++++------------- > > 1 file changed, 43 insertions(+), 28 deletions(-) > > > > diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c > > index d239083..29332fa 100644 > > --- a/lib/librte_pmd_virtio/virtio_ethdev.c > > +++ b/lib/librte_pmd_virtio/virtio_ethdev.c > > @@ -1228,34 +1228,6 @@ eth_virtio_dev_init(__rte_unused struct eth_driver *eth_drv, > > return 0; > > } > > > > -static struct eth_driver rte_virtio_pmd = { > > - { > > - .name = "rte_virtio_pmd", > > - .id_table = pci_id_virtio_map, > > - }, > > - .eth_dev_init = eth_virtio_dev_init, > > - .dev_private_size = sizeof(struct virtio_hw), > > -}; > > - > > -/* > > - * Driver initialization routine. > > - * Invoked once at EAL init time. > > - * Register itself as the [Poll Mode] Driver of PCI virtio devices. > > - * Returns 0 on success. > > - */ > > -static int > > -rte_virtio_pmd_init(const char *name __rte_unused, > > - const char *param __rte_unused) > > -{ > > - if (rte_eal_iopl_init() != 0) { > > - PMD_INIT_LOG(ERR, "IOPL call failed - cannot use virtio PMD"); > > - return -1; > > - } > > - > > - rte_eth_driver_register(&rte_virtio_pmd); > > - return 0; > > -} > > - > > /* > > * Only 1 queue is supported, no queue release related operation > > */ > > @@ -1484,6 +1456,49 @@ __rte_unused uint8_t is_rx) > > return 0; > > } > > > > +/* > > + * Early init function, this is needed so that iopl() on linux is done before > > + * rte_eal_init (since it creates other threads that must inherit from the one > > + * that calls rte_eal_init). > > + */ > > +static void __attribute__((constructor, used)) > > +rte_virtio_early_init(void) > > +{ > > + /* return value is checked at pmd init time, no need to check here, see > > + * below. */ > > + rte_eal_iopl_init(); > > +} > > + > > I don't see how this works for all cases. The constructor is called once when > the library is first loaded. What if you have multiple independent (i.e. not > forked children) processes that are using the dpdk in parallel? Only the > process that triggered the library load will have io permissions set > appropriately. I think what you need is to have every application that expects > to call through the transmit path or poll the receive path call iopl, which I > think speaks to having this requirement documented, so each application can call > iopl prior to calling fork/daemonize/etc. > > Neil I am still seeing this problem with DPDK 2.0 and 2.1. It seems to me that doing the iopl init in eal_init is the only safe way. Other workaround is to have application calling iopl_init before eal_init but that kind of violates the current method of all things being initialized by eal_init