Re: [PATCH] net/virtio-user: fix not working on 32-bit system

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

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: stable@dpdk.org
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@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