Re: Commit broke 32-bit testpmd app

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 9/20/23 09:35, Maxime Coquelin wrote:
> Hi,
> 
> I tried to reproduce without success(see attached log).
> 
> I fail to reproduce because buf_iova fits into 32 bits in my case:
> (gdb) p /x *tx_pkts[0]
> 
> $4 = {
>    cacheline0 = 0x77b19ec0,
>    buf_addr = 0x77b19f40,
>    buf_iova = 0x49519f40,
>    rearm_data = 0x77b19ed0,
> 
> However, looking at your report, something like this would work for you:
> 
> diff --git a/drivers/net/virtio/virtqueue.h 
> b/drivers/net/virtio/virtqueue.h
> index 9d4aba11a3..38efbc517a 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -124,7 +124,7 @@ virtqueue_store_flags_packed(struct 
> vring_packed_desc *dp,
>    * (virtio-pci and virtio-user).
>    */
>   #define VIRTIO_MBUF_ADDR(mb, vq) \
> -       ((uint64_t)(*(uintptr_t *)((uintptr_t)(mb) + 
> (vq)->mbuf_addr_offset)))
> +       (*(uint64_t *)((uintptr_t)(mb) + (vq)->mbuf_addr_offset))
> 
> 
> The problem is that it would likely break Virtio-user en 32bits mode, as
> this is how it was initially implemented, and got fixed few years ago,
> as David hinted to me:
> 
> commit 260aae9ad9621e3e758f1443abb8fcbc25ece07c
> Author: Jianfeng Tan <jianfeng.tan@intel.com>
> Date:   Wed Apr 19 02:30:33 2017 +0000
> 
>      net/virtio-user: fix address on 32-bit system
> 
>      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.
> 
>      In case of virtio-user, we use buf_addr of mbuf to fill the
>      virtqueue desc addr. This is a regression bug. For 32-bit system,
>      the first 4 bytes of mbuf is buf_addr, with following 8 bytes for
>      buf_phyaddr. With below wrong definition, both buf_addr and lower
>      4 bytes buf_phyaddr are obtained to fill the virtqueue desc.
>        #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>
>      Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> 
> If my understanding is correct, on 32 bits, when mbuf->buf_addr is used
> (Virtio-user), we need to mask out the higher 4 bytes, while when using
> Virtio-pci we need the full 64 bits (as the physical addresses used as
> IOVA on the guest are 64 bits).

I posted a fix aiming at making it work for both Virtio-user and Virtio-
PCI 32 bits builds while not impacting 64 bits performance. Could you
please have a try and report feedback by replying to the patch?

Regards,
Maxime

> Regards,
> Maxime
> 
> On 9/13/23 15:24, Roger Melton (rmelton) wrote:
>> +Chris Brezovec
>>
>> Hi Maxime,
>>
>> Chris from our team is attending the DPDK Summit in Dublin this week. 
>> If you have some time available, we'd appreciate it if he could meet 
>> with you to discuss the 32bit virtio issue we are seeing.
>>
>> Regards,
>> Roger Melton
>>
>> On 9/6/23 2:57 PM, Dave Johnson (davejo) wrote:
>>>
>>> Hi Maxime,
>>>
>>> This email is regarding the following commit:
>>>
>>> https://github.com/DPDK/dpdk/commit/ba55c94a7ebc386d2288d6578ed57aad6cb92657
>>>
>>> A query had been sent previously on this topic (see below) indicating 
>>> this commit appears to have broken the 32-bit testpmd app and 
>>> impacted one of our products that runs as a 32-bit DPDK application.  
>>> We consequently backed the commit out of our product but would prefer 
>>> to get a fix for it.  In the earlier exchange, you had asked if we 
>>> were using virtio-pci or virtio-user (we are using virtio-pci) and 
>>> asked for logs which Sampath provided.  It’s been a while, so let me 
>>> now if you need me to send resend those logs or need any other 
>>> information.
>>>
>>> FWIW, I reproduced this using testpmd and noticed that this part of 
>>> the change seems to be the interesting part (in 
>>> drivers/net/virtio/virtqueue.h):
>>>
>>> /**
>>>
>>> * Return the IOVA (or virtual address in case of virtio-user) of mbuf
>>>
>>> * data buffer.
>>>
>>> *
>>>
>>> * The address is firstly casted to the word size (sizeof(uintptr_t))
>>>
>>> * before casting it to uint64_t. This is to make it work with different
>>>
>>> * combination of word size (64 bit and 32 bit) and virtio device
>>>
>>> * (virtio-pci and virtio-user).
>>>
>>> */
>>>
>>> #define VIRTIO_MBUF_ADDR(mb, vq) \
>>>
>>>       ((uint64_t)(*(uintptr_t *)((uintptr_t)(mb) + 
>>> (vq)->mbuf_addr_offset))
>>>
>>> If I revert just this part of the changeset (by re-using the 
>>> VIRTIO_MBUF_ADDR to return buf_iova which matches what it had used 
>>> previously), then 32-bit testpmd is able to receive traffic again:
>>>
>>> #define VIRTIO_MBUF_ADDR(mb, vq) (mb->buf_iova)
>>>
>>> Looking at the address produced by each of these, I see the address 
>>> is the same except that the casting results in the upper bits getting 
>>> cleared:
>>>
>>> Address from patch (nonworking case) = 0x58e7c900
>>>
>>> Address using buf_iova (working case) = 0x158e7c900
>>>
>>> ::
>>>
>>> Address from patch (nonworking case) = 0x58e7bfc0
>>>
>>> Address using buf_iova (working case) = 0x158e7bfc0
>>>
>>> ::
>>>
>>> Address from patch (nonworking case) = 0x58e7b680
>>>
>>> Address using buf_iova (working case) = 0x158e7b680
>>>
>>> ::
>>>
>>> Regards, Dave
>>>
>>> *From: *Sampath Peechu (speechu) <speechu@cisco.com>
>>> *Date: *Monday, January 30, 2023 at 3:29 PM
>>> *To: *Maxime Coquelin <maxime.coquelin@redhat.com>, 
>>> chenbo.xia@intel.com <chenbo.xia@intel.com>, dev@dpdk.org <dev@dpdk.org>
>>> *Cc: *Roger Melton (rmelton) <rmelton@cisco.com>, Malcolm Bumgardner 
>>> (mbumgard) <mbumgard@cisco.com>
>>> *Subject: *Re: Commit broke 32-bit testpmd app
>>>
>>> Hi Maxime,
>>>
>>> Could you please let us know if you got a chance to look at the 
>>> debugs logs I provided?
>>>
>>> Thanks,
>>>
>>> Sampath
>>>
>>> *From: *Sampath Peechu (speechu) <speechu@cisco.com>
>>> *Date: *Tuesday, December 6, 2022 at 1:08 PM
>>> *To: *Maxime Coquelin <maxime.coquelin@redhat.com>, 
>>> chenbo.xia@intel.com <chenbo.xia@intel.com>, dev@dpdk.org <dev@dpdk.org>
>>> *Cc: *Roger Melton (rmelton) <rmelton@cisco.com>
>>> *Subject: *Re: Commit broke 32-bit testpmd app
>>>
>>> Hi Maxime,
>>>
>>> Did you get a chance to look into this?
>>>
>>> Please let me know if you need anything else.
>>>
>>> Thanks,
>>>
>>> Sampath
>>>
>>> *From: *Sampath Peechu (speechu) <speechu@cisco.com>
>>> *Date: *Wednesday, November 23, 2022 at 5:15 PM
>>> *To: *Maxime Coquelin <maxime.coquelin@redhat.com>, 
>>> chenbo.xia@intel.com <chenbo.xia@intel.com>, dev@dpdk.org <dev@dpdk.org>
>>> *Cc: *Roger Melton (rmelton) <rmelton@cisco.com>
>>> *Subject: *Re: Commit broke 32-bit testpmd app
>>>
>>> Hi Maxime,
>>>
>>> I’m attaching the following for reference.
>>>
>>>   * Instructions for Centos8 test setup
>>>   * Diffs between the working and non-working versions (working
>>>     version has the problem commit backed out)
>>>   * Working logs (stats show that ping packets from neighbor VM can be
>>>     seen with both 64-bit and 32-bit apps)
>>>   * Non-working logs (stats show that ping packets from neighbor VM
>>>     are seen with 64-bit app but NOT seen with 32-bit app)
>>>
>>> ============================
>>>
>>> $ sudo ./usertools/dpdk-devbind.py --status
>>>
>>> Network devices using DPDK-compatible driver
>>>
>>> ============================================
>>>
>>> 0000:07:00.0 'Virtio network device 1041' drv=igb_uio unused=
>>>
>>> 0000:08:00.0 'Virtio network device 1041' drv=igb_uio unused=
>>>
>>> Network devices using kernel driver
>>>
>>> ===================================
>>>
>>> 0000:01:00.0 'Virtio network device 1041' if=enp1s0 drv=virtio-pci 
>>> unused=igb_uio *Active*
>>>
>>> …
>>>
>>> ===========================
>>>
>>> Thanks,
>>>
>>> Sampath
>>>
>>> *From: *Maxime Coquelin <maxime.coquelin@redhat.com>
>>> *Date: *Tuesday, November 22, 2022 at 4:24 AM
>>> *To: *Sampath Peechu (speechu) <speechu@cisco.com>, 
>>> chenbo.xia@intel.com <chenbo.xia@intel.com>, dev@dpdk.org <dev@dpdk.org>
>>> *Cc: *Roger Melton (rmelton) <rmelton@cisco.com>
>>> *Subject: *Re: Commit broke 32-bit testpmd app
>>>
>>> Hi,
>>>
>>> In my initial reply (see below), I also asked if you had logs to share.
>>> And wondered whether it happens with Virtio PCI or Virtio-user?
>>>
>>> Regards,
>>> Maxime
>>>
>>> On 11/16/22 00:30, Sampath Peechu (speechu) wrote:
>>> > ++ dev@dpdk.org <mailto:dev@dpdk.org <mailto:dev@dpdk.org>>
>>> >
>>> > *From: *Maxime Coquelin <maxime.coquelin@redhat.com>
>>> > *Date: *Tuesday, November 15, 2022 at 3:19 AM
>>> > *To: *Sampath Peechu (speechu) <speechu@cisco.com>, 
>>> chenbo.xia@intel.com
>>> > <chenbo.xia@intel.com>
>>> > *Cc: *Roger Melton (rmelton) <rmelton@cisco.com>
>>> > *Subject: *Re: Commit broke 32-bit testpmd app
>>> >
>>> > Hi Sampath,
>>> >
>>> >
>>> > Please add dev@dpdk.org, the upstream mailing list, if this is related
>>> > to the upstream DPDK project.If it is using RHEL DPDK package, please
>>> > use the appropriate support channels.
>>> >
>>> > On 11/14/22 23:55, Sampath Peechu (speechu) wrote:
>>> >  > Hi Virtio Maintainers team,
>>> >  >
>>> >  > This email is regarding the following commit.
>>> >  >
>>> >  >
>>> > 
>>> https://github.com/DPDK/dpdk/commit/ba55c94a7ebc386d2288d6578ed57aad6cb92657 <https://github.com/DPDK/dpdk/commit/ba55c94a7ebc386d2288d6578ed57aad6cb92657> <https://github.com/DPDK/dpdk/commit/ba55c94a7ebc386d2288d6578ed57aad6cb92657 <https://github.com/DPDK/dpdk/commit/ba55c94a7ebc386d2288d6578ed57aad6cb92657>>
>>> >  >
>>> >  > The above commit appears to have broken the 32-bit testpmd app (and
>>> >  > consequently impacted one of our products that runs as a 32-bit 
>>> DPDK
>>> >  > app). The 64-bit testpmd app does not appear to be impacted though.
>>> >
>>> > We'll need some logs to understand what is going on.
>>> > Does it happen with virtio-pci or virtio-user?
>>> >
>>> > Regards,
>>> > Maxime
>>> >
>>> >  > With the commit in place, we didn’t see any packets going 
>>> through at
>>> >  > all. After backing out the commit and rebuilding the 32-bit 
>>> testpmd app
>>> >  > in our test setup, we were able to pass traffic as expected.
>>> >  >
>>> >  > Could you please let us know if this is a known issue? And if 
>>> there is a
>>> >  > fix available for it?
>>> >  >
>>> >  > Thank you,
>>> >  >
>>> >  > Sampath Peechu
>>> >  >
>>> >  > Cisco Systems
>>> >  >
>>> >
>>>
>>