DPDK patches and discussions
 help / color / mirror / Atom feed
From: Neil Horman <nhorman@tuxdriver.com>
To: David Marchand <david.marchand@6wind.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 2/2] virtio: change io privilege level as early as possible
Date: Tue, 10 Mar 2015 09:14:28 -0400
Message-ID: <20150310131428.GB7873@hmsreliant.think-freely.org> (raw)
In-Reply-To: <1425912999-13118-3-git-send-email-david.marchand@6wind.com>

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 <david.marchand@6wind.com>
> ---
>  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

  reply	other threads:[~2015-03-10 13:14 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-06  0:45 [dpdk-dev] [PATCH 0/2] virtio: bugfixes Stephen Hemminger
2015-03-06  0:45 ` [dpdk-dev] [PATCH 1/2] virtio: initialize iopl when device is initialized Stephen Hemminger
2015-03-06  3:41   ` Ouyang, Changchun
2015-03-06 16:20     ` Stephen Hemminger
2015-03-06 16:33       ` David Marchand
2015-03-06 16:55         ` Stephen Hemminger
2015-03-06 22:04           ` David Marchand
2015-03-06 23:43             ` Stephen Hemminger
2015-03-07  6:53               ` David Marchand
2015-03-09 11:05       ` David Marchand
2015-03-09 14:56         ` [dpdk-dev] [PATCH 0/2] fix virtio interrupt handling David Marchand
2015-03-09 14:56           ` [dpdk-dev] [PATCH 1/2] eal/linux: move plugin load to very start of eal init David Marchand
2015-03-09 15:21             ` Neil Horman
     [not found]               ` <CALwxeUs4hPbYDPBUfz9u2AoiCoj_wwTsAyj=_1xxzuT6LLW6nw@mail.gmail.com>
2015-03-10 10:55                 ` Neil Horman
2015-10-14  0:05                   ` Stephen Hemminger
2015-10-14  9:55                     ` David Marchand
2015-03-09 14:56           ` [dpdk-dev] [PATCH 2/2] virtio: change io privilege level as early as possible David Marchand
2015-03-10 13:14             ` Neil Horman [this message]
2015-09-29 19:25               ` Stephen Hemminger
2015-09-30  8:28                 ` David Marchand
2015-09-30 14:52                   ` Neil Horman
2015-09-30 15:37                     ` Thomas Monjalon
2015-09-30 17:26                       ` Stephen Hemminger
2015-10-01 11:25                       ` Neil Horman
2015-10-12 20:08                         ` Stephen Hemminger
2015-10-14  0:07                         ` Stephen Hemminger
2015-10-14  8:00                           ` David Marchand
2015-10-14  9:49                             ` David Marchand
2015-10-14  9:50                               ` [dpdk-dev] [PATCH] eal: move interrupt init after device init David Marchand
2015-10-14 11:32                                 ` David Marchand
2015-10-20 21:22                                   ` Thomas Monjalon
2015-07-29 17:26       ` [dpdk-dev] [PATCH 1/2] virtio: initialize iopl when device is initialized Thomas Monjalon
2015-03-06  0:45 ` [dpdk-dev] [PATCH 2/2] virtio: allow running w/o vlan filtering Stephen Hemminger
2015-03-06  3:39   ` Ouyang, Changchun
2015-03-06 16:24     ` Stephen Hemminger
2015-07-29 12:56       ` Thomas Monjalon
2015-07-30  1:23         ` Ouyang, Changchun
2015-08-04 12:51         ` Vincent JARDIN
2015-08-05  1:01           ` Ouyang, Changchun
2015-08-05  1:22             ` Stephen Hemminger
2015-08-05 10:49             ` Vincent JARDIN
2015-10-21 13:58               ` Thomas Monjalon
2017-02-15  8:38                 ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150310131428.GB7873@hmsreliant.think-freely.org \
    --to=nhorman@tuxdriver.com \
    --cc=david.marchand@6wind.com \
    --cc=dev@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git