Posted additional testing info with and without this patch in :
https://patchwork.ozlabs.org/project/openvswitch/patch/0c4c167ad644d3dda51b992e51ec1c27b8492457.1589605823.git.gmuth...@redhat.com/
Thanks,
Gowrishankar
On Fri, May 15, 2020 at 10:05 PM Gowrishankar Muthukrishnan <
gmuth...@redhat.com> wrote:
>
>
> On Thu, May 14, 2020 at 5:48 PM Maxime Coquelin <
> maxime.coque...@redhat.com> wrote:
>
>> Hi,
>>
>> On 5/14/20 1:56 PM, Gowrishankar Muthukrishnan wrote:
>> > Virtio pmd driver can not benefit from tso and csum offload
>> > as they are not included in negotiation check with host. Add
>> > them in virtio dev init and let negotiation decide the fate.
>> >
>> > Signed-off-by: Gowrishankar Muthukrishnan <gmuth...@redhat.com>
>>
>>
>> I think you don't need any patch to achieve that.
>> Disabling TSO and csum offload by default is done on-purpose, as it is
>> dependent on the application that drivers the Virtio PMD to support it.
>>
>> To enable offloads with Virtio PMD, your application has to enable by
>> setting the proper offloads flags in device's ethdev config. Doing that,
>> a re-negotiation will happen with the proper Virtio features added:
>>
>> Even before application (testpmd for dpdk pmd) or ethtool (kernel
> driver) to enable it, the feature should be agreed both by guest and host
> as commented further below.
>
> static int
>> virtio_dev_configure(struct rte_eth_dev *dev)
>> {
>> const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
>> const struct rte_eth_txmode *txmode = &dev->data->dev_conf.txmode;
>> ...
>> uint64_t rx_offloads = rxmode->offloads;
>> uint64_t tx_offloads = txmode->offloads;
>> ...
>>
>> if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
>> DEV_RX_OFFLOAD_TCP_CKSUM))
>> req_features |= (1ULL << VIRTIO_NET_F_GUEST_CSUM);
>>
>> if (rx_offloads & DEV_RX_OFFLOAD_TCP_LRO)
>> req_features |=
>> (1ULL << VIRTIO_NET_F_GUEST_TSO4) |
>> (1ULL << VIRTIO_NET_F_GUEST_TSO6);
>>
>> if (tx_offloads & (DEV_TX_OFFLOAD_UDP_CKSUM |
>> DEV_TX_OFFLOAD_TCP_CKSUM))
>> req_features |= (1ULL << VIRTIO_NET_F_CSUM);
>>
>> if (tx_offloads & DEV_TX_OFFLOAD_TCP_TSO)
>> req_features |=
>> (1ULL << VIRTIO_NET_F_HOST_TSO4) |
>> (1ULL << VIRTIO_NET_F_HOST_TSO6);
>>
>> /* if request features changed, reinit the device */
>> if (req_features != hw->req_guest_features) {
>> ret = virtio_init_device(dev, req_features);
>>
>
> In negotiation (virtio_negotiate_features), negotiating elements have to
> be in both guest (virtio hw->guest_features) as well as host(vtpci
> hw->get_features).
> When we do not have offload flags in hw->guest_features but host keeps
> ready, host will not be able to enable offload for that socket instance to
> guest, following set_features request from guest.
>
> hw->guest_features = vtpci_negotiate_features(hw, host_features << the
> list with offload that host is offering);
> vtpci_negotiate_features:
> features = host_features & hw->guest_features << which is default list
> of features w/o offload;
> VTPCI_OPS(hw)->set_features(hw, features);
>
> Again, as far as the host is considered, it again depends on
> whether vhostuser or vhostuserclient socket is connecting guest, but should
> not be a problem for guest.
>
> vhostuser socket:
> when backend tso is enabled (userspace tso), virtio bits are not removed
> in negotiation with the guest which is not an issue for enabling offload
> flags in front end negotiating set.
> otherwise, these bits are removed, so vnic/testpmd would not be able to
> set.
>
> vhostuserclient socket:
> supported features (including offload bits) are enabled when backend
> tso is enabled through dpdk vhost driver and hence vnic/testpmd will see
> the same in get features call.
>
> Am I missing something ?.
>
> Thanks,
> Gowrishankar
>
> }
>> ...
>>
>>
>>
>> > --
>> > This patch has been tested with TSO tests in OVS-DPDK:
>> >
>> > https://patchwork.ozlabs.org/project/openvswitch/list/?series=176886
>> >
>> > ## ------------------------------- ##
>> > ## openvswitch 2.13.90 test suite. ##
>> > ## ------------------------------- ##
>> >
>> > OVS-DPDK unit tests
>> >
>> > 1: OVS-DPDK - EAL init ok
>> > 2: OVS-DPDK - add standard DPDK port ok
>> > 3: OVS-DPDK - add vhost-user-client port ok
>> > 4: OVS-DPDK - ping vhost-user ports ok
>> > 5: OVS-DPDK - ping vhost-user-client ports ok
>> > 6: OVS-DPDK - validate tso negotiation ok
>> >
>> > ## ------------- ##
>> > ## Test results. ##
>> > ## ------------- ##
>> >
>> > All 6 tests were successful.
>> >
>> > ---
>> > drivers/net/virtio/virtio_ethdev.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/net/virtio/virtio_ethdev.c
>> b/drivers/net/virtio/virtio_ethdev.c
>> > index 044eb10..91f6f16 100644
>> > --- a/drivers/net/virtio/virtio_ethdev.c
>> > +++ b/drivers/net/virtio/virtio_ethdev.c
>> > @@ -1914,7 +1914,7 @@ static int virtio_dev_xstats_get_names(struct
>> rte_eth_dev *dev,
>> > }
>> >
>> > /* reset device and negotiate default features */
>> > - ret = virtio_init_device(eth_dev,
>> VIRTIO_PMD_DEFAULT_GUEST_FEATURES);
>> > + ret = virtio_init_device(eth_dev,
>> VIRTIO_PMD_SUPPORTED_GUEST_FEATURES);
>> > if (ret < 0)
>> > goto err_virtio_init;
>> >
>> > @@ -2064,7 +2064,7 @@ static int eth_virtio_pci_remove(struct
>> rte_pci_device *pci_dev)
>> > int ret;
>> >
>> > PMD_INIT_LOG(DEBUG, "configure");
>> > - req_features = VIRTIO_PMD_DEFAULT_GUEST_FEATURES;
>> > + req_features = VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
>> >
>> > if (rxmode->mq_mode != ETH_MQ_RX_NONE) {
>> > PMD_DRV_LOG(ERR,
>> >
>>
>>
>
> --
> Gowrishankar M
>
--
Gowrishankar M
