From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Tan, Jianfeng" Subject: Re: bug: virtio PMD sends malformed packets for 32-bit processes on 64-bit kernel Date: Mon, 26 Jun 2017 18:15:39 +0800 Message-ID: <474626a8-6bfa-da37-39a1-58ca27c89b9c@intel.com> References: <1498143499.8893.38.camel@oneaccess-net.com> <1498464846.4049.9.camel@oneaccess-net.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: John Sucaet , "dev@dpdk.org" To: Frederico Cadete , "yuanhan.liu@linux.intel.com" , "maxime.coquelin@redhat.com" Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 0F4122C12 for ; Mon, 26 Jun 2017 12:15:41 +0200 (CEST) In-Reply-To: <1498464846.4049.9.camel@oneaccess-net.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 6/26/2017 4:14 PM, Frederico Cadete wrote: > On Fri, 2017-06-23 at 23:36 +0800, Tan, Jianfeng wrote: >> Hi Cadete, >> >> >> On 6/22/2017 10:58 PM, Frederico Cadete wrote: >>> Hello, >>> >>> I believe commit 260aae9a [1] has introduced a regression for the >>> case >>> of 32-bit process running on a 64-bit kernel. >>> >>> The commit is effectively casting mbuf->buf_physaddr to uintptr_t >>> before dereferencing it. It truncates the physical address to the >>> width >>> of the process's uint, and in the the aforementioned combination >>> this >>> loses important bits. >>> >>> I can confirm this under GDB. When virtqueue_enqueue_xmit is >>> filling in >>> start_dp, I get this result: >>> >>> (gdb) p /x cookie->buf_physaddr >>> $5 = 0x12f94a000 >>> (gdb) p /x start_dp[idx].addr >>> $6 = 0x2f94a080 >> Now you are testing a virtio-pci device and app is compiled into a >> 32-bit executable on 64-bit VM system? > Exactly. Furthermore, this bug is only visible if you give the virtual > machine enough memory for the mbuf's physiscal address to be above the > 4GB mark. That's an important information. > >>> >>> On my system, I capture the packet that goes out to the host and I >>> see >>> it has the correct size but the content is all-zeroes. >>> >>> I would like to propose a patch that would work for all supported >>> combinations of user/kernel bitwidth *and* virtio-pci/virtio-user. >>> But >>> I don't really see how that could work, given virtio-user tries to >>> store a physical address in rte_mbuf's "void *buf_addr", and this >>> is >>> not always big enough for a physical address. >> virtio-user does not store a physical address in rte_mbuf's "void >> *buf_addr", instead, it uses this field in rte_mbuf to fill desc's >> addr >> which is always 64bit long. > Oh, that's right. Sorry about that. > > In that case I guess that the issue is that the conversion is assuming > that on 32-bit apps only 4 bytes are necessary, even in the case of > virtio-pci and 64-bit physaddr. > > Would you say that this is how vring_desc's addr should be filled? > > | 32-bit app | 64-bit app | > ------------+-----------------------+ -----------------------+ > virtio-pci | buf_physaddr, 8 bytes | buf_physaddr, 8 bytes | > virtio-user | buf_addr, 4 bytes | buf_addr, 8 bytes | > > I believe that the issue is that after commit 260aae9a, for virtio-pci > and 32-bit app it is taking 4 bytes instead of 8. Aha, yes, that's the issue! Great analysis. After Bruce's commit 586ec205bcbbb ("mbuf: fix 64-bit address alignment in 32-bit builds"), we can fix this issue by fetching 8 bytes at all cases. But unfortunately, that commit is not back-ported to v17.02.1. I wonder if we can back-port Bruce's patch with a new patch to fix this problem? Any opinions from others? Thanks, Jianfeng > >>> Any suggestions on if and how this could be fixed? >>> >>> Meanwhile, the bug affects dpdk 17.05, 17.02.1 and master. Users >>> not >>> requiring virtio-user support can avoid it by setting >>> CONFIG_VIRTIO_USER=n during compilation. >>> >>> Best regards, >>> Frederico Cadete