Re: [dpdk-dev] [PATCH v3] vhost: add header check in dequeue offload

["Wang, Xiao W" <xiao.w.wang@intel.com>]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, April 1, 2021 8:04 PM
> To: Wang, Xiao W <xiao.w.wang@intel.com>
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Liu, Yong <yong.liu@intel.com>; dev
> <dev@dpdk.org>; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> dpdk stable <stable@dpdk.org>
> Subject: Re: [PATCH v3] vhost: add header check in dequeue offload
> 
> On Wed, Mar 17, 2021 at 7:50 AM Xiao Wang <xiao.w.wang@intel.com>
> wrote:
> >
> > When parsing the virtio net header and packet header for dequeue offload,
> > we need to perform sanity check on the packet header to ensure:
> >   - No out-of-boundary memory access.
> >   - The packet header and virtio_net header are valid and aligned.
> >
> > Fixes: d0cf91303d73 ("vhost: add Tx offload capabilities")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
> 
> I spent some time digging on this topic.
> 
> Afaiu the offload API, vhost is not supposed to populate tx offloads.
> I would drop this whole parse_ethernet function and replace
> vhost_dequeue_offload with what virtio does on the rx side.
> 
> Please have a look at this series (especially the last patch):
> http://patchwork.dpdk.org/project/dpdk/list/?series=16052
> 
> 
> Thanks.
> 
> --
> David Marchand

+Yang ,Yi into this loop who may have comments especially from OVS perspective on CKSUM/TSO/TSO in tunnel/etc..

I think the original vhost implementation here is to help pass virtio's offload request onto the next output port, either physical device or a virtio device.
If we go with series http://patchwork.dpdk.org/project/dpdk/list/?series=16052, then virtual switch need to do an extra translation on the flags:
e.g. PKT_RX_LRO --> PKT_TX_TCP_SEG. The question is that a packet marked with PKT_RX_LRO may come from different types of ports (non-vhost), how vSwitch can tell if TSO request should be set for this packet at transmission?

If I think from an endpoint app's perspective, I'm inclined to agree with your series. If I think from a switch/router's perspective, I'm inclined to keep the current implementation. Maybe we can add PKT_RX_L4_CKSUM_NONE/PKT_RX_LRO flags into the current implementation, seems this method can cover both scenarios.

BRs,
Xiao