Commit broke 32-bit testpmd app

[Maxime Coquelin]

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 at 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 at dpdk.org

     Signed-off-by: Jianfeng Tan <jianfeng.tan at intel.com>
     Acked-by: Yuanhan Liu <yuanhan.liu at 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).

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 at cisco.com>
>> *Date: *Monday, January 30, 2023 at 3:29 PM
>> *To: *Maxime Coquelin <maxime.coquelin at redhat.com>, 
>> chenbo.xia at intel.com <chenbo.xia at intel.com>, dev at dpdk.org <dev at dpdk.org>
>> *Cc: *Roger Melton (rmelton) <rmelton at cisco.com>, Malcolm Bumgardner 
>> (mbumgard) <mbumgard at 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 at cisco.com>
>> *Date: *Tuesday, December 6, 2022 at 1:08 PM
>> *To: *Maxime Coquelin <maxime.coquelin at redhat.com>, 
>> chenbo.xia at intel.com <chenbo.xia at intel.com>, dev at dpdk.org <dev at dpdk.org>
>> *Cc: *Roger Melton (rmelton) <rmelton at 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 at cisco.com>
>> *Date: *Wednesday, November 23, 2022 at 5:15 PM
>> *To: *Maxime Coquelin <maxime.coquelin at redhat.com>, 
>> chenbo.xia at intel.com <chenbo.xia at intel.com>, dev at dpdk.org <dev at dpdk.org>
>> *Cc: *Roger Melton (rmelton) <rmelton at 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 at redhat.com>
>> *Date: *Tuesday, November 22, 2022 at 4:24 AM
>> *To: *Sampath Peechu (speechu) <speechu at cisco.com>, 
>> chenbo.xia at intel.com <chenbo.xia at intel.com>, dev at dpdk.org <dev at dpdk.org>
>> *Cc: *Roger Melton (rmelton) <rmelton at 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 at dpdk.org <mailto:dev at dpdk.org <mailto:dev at dpdk.org>>
>> >
>> > *From: *Maxime Coquelin <maxime.coquelin at redhat.com>
>> > *Date: *Tuesday, November 15, 2022 at 3:19 AM
>> > *To: *Sampath Peechu (speechu) <speechu at cisco.com>, 
>> chenbo.xia at intel.com
>> > <chenbo.xia at intel.com>
>> > *Cc: *Roger Melton (rmelton) <rmelton at cisco.com>
>> > *Subject: *Re: Commit broke 32-bit testpmd app
>> >
>> > Hi Sampath,
>> >
>> >
>> > Please add dev at 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
>> >  >
>> >
>>
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: virtio_32bits.log
Type: text/x-log
Size: 9729 bytes
Desc: not available
URL: <http://mails.dpdk.org/archives/dev/attachments/20230920/5baabfce/attachment.bin>