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. > > > > > > > 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. > > > > > 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
