From patchwork Mon Dec 14 16:09:51 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Santosh Shukla X-Patchwork-Id: 9543 Return-Path: X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 80C3B8D93; Mon, 14 Dec 2015 17:09:54 +0100 (CET) Received: from mail-pf0-f169.google.com (mail-pf0-f169.google.com [209.85.192.169]) by dpdk.org (Postfix) with ESMTP id D6D70376C for ; Mon, 14 Dec 2015 17:09:52 +0100 (CET) Received: by pfbu66 with SMTP id u66so63230619pfb.3 for ; Mon, 14 Dec 2015 08:09:52 -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=xveHf4k8xZ9O1HyRoOyJwqNqONZIargE1JMXs3kXtgg=; b=ulLiXSrT9ALJPwjjUKBRoBJCf2c9uwwUlXlPRlqn+XXGg1pbT01iwgsg+oSsDI1tLF 8tVDW3yE+hkHQOdypyan8aV/Ba0hY6sDA1AliV0umSxrMHzBpuUj7+3zj+4JGxYGQpQf SXjIFTqp4RCKqDkJTJyjjeklvYx9U69F4FZQUJqUY1vPcRf5sIwlsKsnTXAwH+gQ0ubY TYAcDoQPaH3DU3rPMn5tYTxxzFfbVAGi4S5BcwMmy48mLBRVbf0S+i4hzzt3Jq2LBsq9 b0xIviItesRpyqS2s+lb3wiL2+zCGhHEXphYroaJhTVd32B57ozLx0g4HSV5nD81/kQV Dqlw== 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=xveHf4k8xZ9O1HyRoOyJwqNqONZIargE1JMXs3kXtgg=; b=EoZW3/xQ6kYu/zDoXrr1yAo+MxSKmIq4N1bl17xfeL/A9+Nka9R77bRmzuY5yYEbkz q4Mm9iv+KcfSHN+ugKsnRuux4+znitkHA5iWDqviHCQrE6TL0OKbBZH3Xxh6i87HV8az 279Gc4x4GSrms8ZQSrDttu3+sc2nZ4ksjbW7obXydGuxM/1/MHHYXNqQhSabbG7GX2o8 aD2IhEwiA85dspkvD5HErxgEdnK2ziu7V7WVebhC+Vlwb8Odj2z6RH6KFyz5ofozuK8W Tsg6ZO08pMmwUGXJaOzA4rGPiTDejJcy0pRIOBzOVkS1JRyTJPEgPIGTXLBIOmxV6tJN 4l1Q== X-Gm-Message-State: ALoCoQlPeT9IC+nHwWhGUFQWrxRsBvTFiOH6EWNRceZxeaO5Sy3AsIaMwezhHD2aqVpZQh6lau+Y0qzmwJCGKuYUk8doIcm4xfhiyJqZuPiOuf8HAHUwj60= MIME-Version: 1.0 X-Received: by 10.98.71.138 with SMTP id p10mr37529936pfi.61.1450109392081; Mon, 14 Dec 2015 08:09:52 -0800 (PST) Received: by 10.66.13.233 with HTTP; Mon, 14 Dec 2015 08:09:51 -0800 (PST) In-Reply-To: <20151214153136.701b224c@pcviktorin.fit.vutbr.cz> References: <1450098032-21198-1-git-send-email-sshukla@mvista.com> <20151214153136.701b224c@pcviktorin.fit.vutbr.cz> Date: Mon, 14 Dec 2015 21:39:51 +0530 Message-ID: From: Santosh Shukla To: Jan Viktorin Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [ [PATCH v2] 00/13] Add virtio support in arm/arm64 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Dec 14, 2015 at 8:01 PM, Jan Viktorin wrote: > Hello, > > this patch set increases the number of warnings for the armv7 build. I > believe that most of them are false-positives. > > virtio_ethdev.c:729, GNU Make + GNU C Compiler (gcc), Priority: Normal > cast increases required alignment of target type [-Wcast-align] > > xstats[count].value = *(uint64_t *)(((char *)rxvq) + > > * Can you guarantee this is not an unaligned 64-bit access? > > virtio_ethdev.c:747, GNU Make + GNU C Compiler (gcc), Priority: Normal > cast increases required alignment of target type [-Wcast-align] > > ditto... > > virtio_pci.c:52, GNU Make + GNU C Compiler (gcc), Priority: Normal > cast increases required alignment of target type [-Wcast-align] > > *(uint32_t *)d = VIRTIO_READ_REG_4(hw, off); > > * I think, here we can live with unaligned_uint32_t, don't we? > Yes. > virtio_pci.c:55, GNU Make + GNU C Compiler (gcc), Priority: Normal > cast increases required alignment of target type [-Wcast-align] > > similar... > > virtio_pci.c:75, GNU Make + GNU C Compiler (gcc), Priority: Normal > cast increases required alignment of target type [-Wcast-align] > > similar... > > virtio_pci.c:78, GNU Make + GNU C Compiler (gcc), Priority: Normal > cast increases required alignment of target type [-Wcast-align] > > similar... > > virtio_ring.h:144, GNU Make + GNU C Compiler (gcc), Priority: Normal > cast increases required alignment of target type [-Wcast-align] > > vr->desc = (struct vring_desc *) p; > > * What can we do here? Should we annotate vring_desc to be aligned properly? > > virtio_ring.h:145, GNU Make + GNU C Compiler (gcc), Priority: Normal > cast increases required alignment of target type [-Wcast-align] > > similar... > > virtio_rxtx.c:733, GNU Make + GNU C Compiler (gcc), Priority: Normal > cast increases required alignment of target type [-Wcast-align] > > header = (struct virtio_net_hdr_mrg_rxbuf *)((char *)rxm->buf_addr + > RTE_PKTMBUF_HEADROOM - hdr_size); > > * No idea how to solve this... > > These warnings are in-general virtio-pmd warnings for armv7 case. Patchset enables virtio feature thus above warning noticed. I spent 10 min and noticed that most of those warning are types (uint64_t*) ((char *) ptr + offset). could avoid such warning by (uint64_t *) (uintptr) ((char*)ptr + offset) where ptr is void *, But this not the case in virtio code, most of such places ptr is uint8_t, keeping then void * then typecasting to ((char *)ptr + offset) then following proposed-way, I guess won't harm functionality and could keep armv7 happy. And yes they are all look false-positive to me too. Pasting code a snape which could get rid of above warning for armv7 case. Let me know your feedback, Others thought/comment welcome. Thanks! ---------------------------------------------------------- diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c index 8107aef..7ef4d13 100644 --- a/drivers/net/virtio/virtio_ethdev.c diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c index 8107aef..7ef4d13 100644 seg_num = header->num_buffers; ----------------------------------------------------------------------------------- > Any other ideas are welcome. > > Regards > Jan > > On Mon, 14 Dec 2015 18:30:19 +0530 > Santosh Shukla wrote: > >> This patch set add basic infrastrucure to run virtio-net-pci pmd driver for >> arm64/arm. Tested on ThunderX platfrom. Verified for existing dpdk(s) test >> applications like: >> - ovs-dpdk-vhost-user: across the VM's, for the use-cases like guest2guest and >> Host2Guest >> - testpmd application: Tested for max virtio-net-pci interface currently >> supported in kernel i.e. 31 interface. >> >> Builds successfully for armv7/v8/thunderX and x86_64/i686 platforms. Made sure >> that patch changes donot break for x86_64 case. Done similar tests for x86_64 >> too. >> >> Patch History: >> v2: >> - Removed ifdef arm.. clutter from igb_uio / virtio_ethedev files >> - Introduced rte_io.h header file in generic/ and arch specifics i.e.. for >> armv7 --> rte_io_32.h, for armv8 --> rte_io_64.h. >> - Removed RTE_ARCH_X86 ifdef clutter too and added rte_io.h header which nothing >> but wraps sys/io.h for x86_64 and i686 >> - Moved all the RTE_ARCH_ARM/64 dependancy for igb_uio case to separate header >> file named igbuio_ioport_misc.h. Now igb_uio.c will call only three function >> - igbuio_iomap >> - igbuio_ioport_register >> - igbuio_ioport_unregister >> - Moved ARM/64 specific definition to include/exec-env/rte_virt_ioport.h header >> - Included virtio_ioport.c/h; has all private and public api required to map >> iopci bar for non-x86 arch. Tested on thunderX and x86_64 both. >> Private api includes: >> - virtio_map_ioport >> - virtio_set_ioport_addr >> Public api includes: >> - virtio_ioport_init >> - virtio_ioport_unmap >> >> - Last patch is the miscllanious format specifier fix identifid for 64bit case >> during regression. >> >> >> v1: >> - First patch adds RTE_VIRTIO_INC_VECTOR config, much needed for archs like >> arm/arm64 as they don't support vectored implementation, also wont able to >> build. >> - Second patch is in-general fix for i686. >> - Third patch is to emulate x86-style of {in,out}[b,w,l] api support for armv7/v8. >> As virtio-net-pci pmd driver uses those apis for port rd/wr {b,w,l} >> - Fourth patch to enable VIRTIO_PMD feature in armv7/v8/thunderX config. >> - Fifth patch to disable iopl syscall, As arm/arm64 linux kernel doesn't support >> them. >> - Sixth patch introduces ioport memdevice called /dev/igb_ioport by which virtio >> pmd driver could able to rd/wr PCI_IOBAR. >> {applicable for arm/arm64 only, tested for arm64 as of now} >> >> >> Santosh Shukla (13): >> virtio: Introduce config RTE_VIRTIO_INC_VECTOR >> config: i686: set RTE_VIRTIO_INC_VECTOR=n >> rte_io: armv7/v8: Introduce api to emulate x86-style of PCI/ISA >> ioport access >> virtio_pci: use rte_io.h for non-x86 arch >> virtio: change io_base datatype from uint32_t to uint64_type >> config: armv7/v8: Enable RTE_LIBRTE_VIRTIO_PMD >> linuxapp: eal: arm: Always return 0 for rte_eal_iopl_init() >> rte_io: x86: Remove sys/io.h ifdef x86 clutter >> igb_uio: ioport: map iopci region for armv7/v8 >> include/exec-env: ioport: add rte_virt_ioport header file >> virtio_ioport: armv7/v8: mmap virtio iopci bar region >> virtio_ethdev: use virtio_ioport api at device init/close >> virtio_ethdev : fix format specifier error for 64bit addr case >> >> config/common_linuxapp | 1 + >> config/defconfig_arm-armv7a-linuxapp-gcc | 6 +- >> config/defconfig_arm64-armv8a-linuxapp-gcc | 6 +- >> config/defconfig_i686-native-linuxapp-gcc | 1 + >> config/defconfig_i686-native-linuxapp-icc | 1 + >> drivers/net/virtio/Makefile | 3 +- >> drivers/net/virtio/virtio_ethdev.c | 10 +- >> drivers/net/virtio/virtio_ioport.c | 163 ++++++++++++++++++++ >> drivers/net/virtio/virtio_ioport.h | 42 +++++ >> drivers/net/virtio/virtio_pci.h | 6 +- >> drivers/net/virtio/virtio_rxtx.c | 7 + >> lib/librte_eal/common/Makefile | 1 + >> lib/librte_eal/common/include/arch/arm/rte_io.h | 60 +++++++ >> lib/librte_eal/common/include/arch/arm/rte_io_32.h | 155 +++++++++++++++++++ >> lib/librte_eal/common/include/arch/arm/rte_io_64.h | 155 +++++++++++++++++++ >> lib/librte_eal/common/include/arch/x86/rte_io.h | 42 +++++ >> lib/librte_eal/common/include/generic/rte_io.h | 81 ++++++++++ >> lib/librte_eal/linuxapp/eal/Makefile | 2 +- >> lib/librte_eal/linuxapp/eal/eal.c | 7 +- >> .../eal/include/exec-env/rte_virt_ioport.h | 81 ++++++++++ >> lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 8 +- >> .../linuxapp/igb_uio/igbuio_ioport_misc.h | 133 ++++++++++++++++ >> 22 files changed, 957 insertions(+), 14 deletions(-) >> create mode 100644 drivers/net/virtio/virtio_ioport.c >> create mode 100644 drivers/net/virtio/virtio_ioport.h >> create mode 100644 lib/librte_eal/common/include/arch/arm/rte_io.h >> create mode 100644 lib/librte_eal/common/include/arch/arm/rte_io_32.h >> create mode 100644 lib/librte_eal/common/include/arch/arm/rte_io_64.h >> create mode 100644 lib/librte_eal/common/include/arch/x86/rte_io.h >> create mode 100644 lib/librte_eal/common/include/generic/rte_io.h >> create mode 100644 lib/librte_eal/linuxapp/eal/include/exec-env/rte_virt_ioport.h >> create mode 100644 lib/librte_eal/linuxapp/igb_uio/igbuio_ioport_misc.h >> > > > > -- > Jan Viktorin E-mail: Viktorin@RehiveTech.com > System Architect Web: www.RehiveTech.com > RehiveTech > Brno, Czech Republic --- a/drivers/net/virtio/virtio_ethdev.c +++ b/drivers/net/virtio/virtio_ethdev.c @@ -726,7 +726,7 @@ virtio_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstats *xstats, snprintf(xstats[count].name, sizeof(xstats[count].name), "rx_q%u_%s", i, rte_virtio_q_stat_strings[t].name); - xstats[count].value = *(uint64_t *)(((char *)rxvq) + + xstats[count].value = *(uint64_t *)(uintptr_t)(((char *)rxvq) + rte_virtio_q_stat_strings[t].offset); count++; } @@ -744,7 +744,7 @@ virtio_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstats *xstats, snprintf(xstats[count].name, sizeof(xstats[count].name), "tx_q%u_%s", i, rte_virtio_q_stat_strings[t].name); - xstats[count].value = *(uint64_t *)(((char *)txvq) + + xstats[count].value = *(uint64_t *)(uintptr_t)(((char *)txvq) + rte_virtio_q_stat_strings[t].offset); count++; } diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h index 447760a..f2f85b4 100644 --- a/drivers/net/virtio/virtio_ring.h +++ b/drivers/net/virtio/virtio_ring.h @@ -141,8 +141,8 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p, unsigned long align) { vr->num = num; - vr->desc = (struct vring_desc *) p; - vr->avail = (struct vring_avail *) (p + + vr->desc = (struct vring_desc *) (uintptr_t)p; + vr->avail = (struct vring_avail *) ((uintptr_t)p + num * sizeof(struct vring_desc)); vr->used = (void *) RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align); diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c index 23be1ff..d636e08 100644 --- a/drivers/net/virtio/virtio_rxtx.c +++ b/drivers/net/virtio/virtio_rxtx.c @@ -730,7 +730,7 @@ virtio_recv_mergeable_pkts(void *rx_queue, continue; } - header = (struct virtio_net_hdr_mrg_rxbuf *)((char *)rxm->buf_addr + + header = (struct virtio_net_hdr_mrg_rxbuf *)(uintptr_t)((char *)rxm->buf_addr + RTE_PKTMBUF_HEADROOM - hdr_size);