On Thu, Apr 13, 2017 at 02:12:56PM +0000, Jianfeng Tan wrote:
> virtio-user cannot work on 32-bit system as higher 32-bit of the
> addr field (64-bit) in the desc is filled with non-zero value
> which should not happen for a 32-bit system.
>
> This is a regression bug. For 32-bit system, the first 4 bytes
> is the virtual address, with following 8 bytes pointing to
> physical addr.
It took me a while to understand that you were trying to say "the first
4 bytes __of mbuf__ is ...".
> With below wrong definition, both virtual address
> and lower 4 bytes of physical addr are obtained.
Again, it's not complete. Something like "in the case of virtio-user,
buf_addr will be used for filling the desc addr, ...". will make it much
easier to understand.
> #define VIRTIO_MBUF_ADDR(mb, vq) \
> (*(uint64_t *)((uintptr_t)(mb) + (vq)->offset))
>
> Fixes: 25f80d108780 ("net/virtio: fix packet corruption")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Jianfeng Tan <jianfeng....@intel.com>
> ---
> drivers/net/virtio/virtqueue.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index f9e3736..f43ea70 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -72,7 +72,8 @@ struct rte_mbuf;
> * Return the physical address (or virtual address in case of
> * virtio-user) of mbuf data buffer.
> */
> -#define VIRTIO_MBUF_ADDR(mb, vq) (*(uint64_t *)((uintptr_t)(mb) +
> (vq)->offset))
> +#define VIRTIO_MBUF_ADDR(mb, vq) \
> + ((uint64_t)((uintptr_t)(*(void **)((uintptr_t)(mb) + (vq)->offset))))
The "void **" cast makes it a bit complex (thus hard to read). I think
following should work?
(uint64_t(*(uintptr_t *)((uintptr_t)(mb) + (vq)->offset)))
Besides, it deserves a comment.
--yliu
> #else
> #define VIRTIO_MBUF_ADDR(mb, vq) ((mb)->buf_physaddr)
> #endif
> --
> 2.7.4
