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

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

[Gowrishankar Muthukrishnan]

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@redhat.com>

--
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,
-- 
1.8.3.1

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

[Maxime Coquelin]

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@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:

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);
		if (ret < 0)
			return ret;
	}
...



> --
> 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,
> 

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

[Gowrishankar Muthukrishnan]

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

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

[Gowrishankar Muthukrishnan]

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@redhat.com> wrote:

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