[dpdk-dev] [PATCH] net/virtio: include host features supported in guest

Gowrishankar Muthukrishnan gmuthukr at redhat.com
Sat May 16 07:57:55 CEST 2020


Posted additional testing info with and without this patch in :
https://patchwork.ozlabs.org/project/openvswitch/patch/0c4c167ad644d3dda51b992e51ec1c27b8492457.1589605823.git.gmuthukr@redhat.com/

Thanks,
Gowrishankar

On Fri, May 15, 2020 at 10:05 PM Gowrishankar Muthukrishnan <
gmuthukr at redhat.com> wrote:

>
>
> On Thu, May 14, 2020 at 5:48 PM Maxime Coquelin <
> maxime.coquelin at 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 <gmuthukr at 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


More information about the dev mailing list