Re: [ [PATCH v2] 00/13] Add virtio support in arm/arm64

[Santosh Shukla <sshukla@mvista.com>]

On Mon, Dec 14, 2015 at 8:01 PM, Jan Viktorin <viktorin@rehivetech.com> 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
--- 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);
                seg_num = header->num_buffers;

-----------------------------------------------------------------------------------

> Any other ideas are welcome.
>
> Regards
> Jan
>
> On Mon, 14 Dec 2015 18:30:19 +0530
> Santosh Shukla <sshukla@mvista.com> 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