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
