Re: [dpdk-dev] bug: virtio PMD sends malformed packets for 32-bit processes on 64-bit kernel

[Frederico Cadete <Frederico.Cadete-ext@oneaccess-net.com>]

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