[dpdk-dev] [PATCH v2 03/12] virtio: reinitialize the device in configure callback

2016-10-03 Thread Olivier Matz 
Add the ability to reset the virtio device in the configure callback
if the features flag changed since previous reset. This will be possible
with the introduction of offload support in next commits.

Signed-off-by: Olivier Matz 
---
 drivers/net/virtio/virtio_ethdev.c | 26 +++---
 drivers/net/virtio/virtio_pci.h|  1 +
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c 
b/drivers/net/virtio/virtio_ethdev.c
index b1056a1..fa56032 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1045,14 +1045,13 @@ virtio_vlan_filter_set(struct rte_eth_dev *dev, 
uint16_t vlan_id, int on)
 }

 static int
-virtio_negotiate_features(struct virtio_hw *hw)
+virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
 {
uint64_t host_features;

/* Prepare guest_features: feature that driver wants to support */
-   hw->guest_features = VIRTIO_PMD_GUEST_FEATURES;
PMD_INIT_LOG(DEBUG, "guest_features before negotiate = %" PRIx64,
-   hw->guest_features);
+   req_features);

/* Read device(host) feature bits */
host_features = hw->vtpci_ops->get_features(hw);
@@ -1063,6 +1062,7 @@ virtio_negotiate_features(struct virtio_hw *hw)
 * Negotiate features: Subset of device feature bits are written back
 * guest feature bits.
 */
+   hw->guest_features = req_features;
hw->guest_features = vtpci_negotiate_features(hw, host_features);
PMD_INIT_LOG(DEBUG, "features after negotiate = %" PRIx64,
hw->guest_features);
@@ -1081,6 +1081,8 @@ virtio_negotiate_features(struct virtio_hw *hw)
}
}

+   hw->req_guest_features = req_features;
+
return 0;
 }

@@ -1121,8 +1123,9 @@ rx_func_get(struct rte_eth_dev *eth_dev)
eth_dev->rx_pkt_burst = &virtio_recv_pkts;
 }

+/* reset device and renegotiate features if needed */
 static int
-virtio_init_device(struct rte_eth_dev *eth_dev)
+virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 {
struct virtio_hw *hw = eth_dev->data->dev_private;
struct virtio_net_config *config;
@@ -1137,7 +1140,7 @@ virtio_init_device(struct rte_eth_dev *eth_dev)

/* Tell the host we've known how to drive the device. */
vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
-   if (virtio_negotiate_features(hw) < 0)
+   if (virtio_negotiate_features(hw, req_features) < 0)
return -1;

/* If host does not support status then disable LSC */
@@ -1258,8 +1261,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)

eth_dev->data->dev_flags = dev_flags;

-   /* reset device and negotiate features */
-   ret = virtio_init_device(eth_dev);
+   /* reset device and negotiate default features */
+   ret = virtio_init_device(eth_dev, VIRTIO_PMD_GUEST_FEATURES);
if (ret < 0)
return ret;

@@ -1344,6 +1347,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 {
const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
struct virtio_hw *hw = dev->data->dev_private;
+   uint64_t req_features;
int ret;

PMD_INIT_LOG(DEBUG, "configure");
@@ -1353,6 +1357,14 @@ virtio_dev_configure(struct rte_eth_dev *dev)
return -EINVAL;
}

+   req_features = VIRTIO_PMD_GUEST_FEATURES;
+   /* 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;
+   }
+
/* Setup and start control queue */
if (vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_VQ)) {
ret = virtio_dev_cq_queue_setup(dev,
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 552166d..d1a7d1e 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -245,6 +245,7 @@ struct virtio_net_config;
 struct virtio_hw {
struct virtnet_ctl *cvq;
struct rte_pci_ioport io;
+   uint64_treq_guest_features;
uint64_tguest_features;
uint32_tmax_queue_pairs;
uint16_tvtnet_hdr_size;
-- 
2.8.1

[dpdk-dev] [PATCH v2 03/12] virtio: reinitialize the device in configure callback

2016-10-11 Thread Maxime Coquelin 


On 10/03/2016 11:00 AM, Olivier Matz wrote:
> Add the ability to reset the virtio device in the configure callback
> if the features flag changed since previous reset. This will be possible
> with the introduction of offload support in next commits.
>
> Signed-off-by: Olivier Matz 
> ---
>  drivers/net/virtio/virtio_ethdev.c | 26 +++---
>  drivers/net/virtio/virtio_pci.h|  1 +
>  2 files changed, 20 insertions(+), 7 deletions(-)

Looks good to me.
Reviewed-by: Maxime Coquelin 

Thanks,
Maxime

[dpdk-dev] [PATCH v2 03/12] virtio: reinitialize the device in configure callback

2016-10-12 Thread Yuanhan Liu 
On Mon, Oct 03, 2016 at 11:00:14AM +0200, Olivier Matz wrote:
> @@ -1344,6 +1347,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  {
>   const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
>   struct virtio_hw *hw = dev->data->dev_private;
> + uint64_t req_features;
>   int ret;
>  
>   PMD_INIT_LOG(DEBUG, "configure");
> @@ -1353,6 +1357,14 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>   return -EINVAL;
>   }
>  
> + req_features = VIRTIO_PMD_GUEST_FEATURES;
> + /* 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;
> + }

Why do you have to reset virtio here? This doesn't make too much sense
to me.

IIUC, you want to make sure those TSO related features being unset at
init time, and enable it (by doing reset) when it's asked to be enabled
(by rte_eth_dev_configure)?

Why not always setting those features? We could do the actual offloads
when:

- those features have been negoiated

- they are enabled through rte_eth_dev_configure

With that, I think we could avoid the reset here?

--yliu

[dpdk-dev] [PATCH v2 03/12] virtio: reinitialize the device in configure callback

2016-10-12 Thread Olivier MATZ 
Hello Yuanhan,

On 10/12/2016 04:41 PM, Yuanhan Liu wrote:
> On Mon, Oct 03, 2016 at 11:00:14AM +0200, Olivier Matz wrote:
>> @@ -1344,6 +1347,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>>   {
>>  const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
>>  struct virtio_hw *hw = dev->data->dev_private;
>> +uint64_t req_features;
>>  int ret;
>>
>>  PMD_INIT_LOG(DEBUG, "configure");
>> @@ -1353,6 +1357,14 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>>  return -EINVAL;
>>  }
>>
>> +req_features = VIRTIO_PMD_GUEST_FEATURES;
>> +/* 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;
>> +}
>
> Why do you have to reset virtio here? This doesn't make too much sense
> to me.
>
> IIUC, you want to make sure those TSO related features being unset at
> init time, and enable it (by doing reset) when it's asked to be enabled
> (by rte_eth_dev_configure)?
>
> Why not always setting those features? We could do the actual offloads
> when:
>
> - those features have been negoiated
>
> - they are enabled through rte_eth_dev_configure
>
> With that, I think we could avoid the reset here?

It would work for TX, since you decide to use or not the feature. But I 
think this won't work for RX: if you negociate LRO at init, the host may 
send you large packets, even if LRO is disabled in dev_configure.

Regards,
Olivier

[dpdk-dev] [PATCH v2 03/12] virtio: reinitialize the device in configure callback

2016-10-13 Thread Yuanhan Liu 
On Wed, Oct 12, 2016 at 06:01:25PM +0200, Olivier MATZ wrote:
> Hello Yuanhan,
> 
> On 10/12/2016 04:41 PM, Yuanhan Liu wrote:
> >On Mon, Oct 03, 2016 at 11:00:14AM +0200, Olivier Matz wrote:
> >>@@ -1344,6 +1347,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> >>  {
> >>const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
> >>struct virtio_hw *hw = dev->data->dev_private;
> >>+   uint64_t req_features;
> >>int ret;
> >>
> >>PMD_INIT_LOG(DEBUG, "configure");
> >>@@ -1353,6 +1357,14 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> >>return -EINVAL;
> >>}
> >>
> >>+   req_features = VIRTIO_PMD_GUEST_FEATURES;
> >>+   /* 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;
> >>+   }
> >
> >Why do you have to reset virtio here? This doesn't make too much sense
> >to me.
> >
> >IIUC, you want to make sure those TSO related features being unset at
> >init time, and enable it (by doing reset) when it's asked to be enabled
> >(by rte_eth_dev_configure)?
> >
> >Why not always setting those features? We could do the actual offloads
> >when:
> >
> >- those features have been negoiated
> >
> >- they are enabled through rte_eth_dev_configure
> >
> >With that, I think we could avoid the reset here?
> 
> It would work for TX, since you decide to use or not the feature. But I
> think this won't work for RX: if you negociate LRO at init, the host may
> send you large packets, even if LRO is disabled in dev_configure.

I see. Thanks.

Besides, I think you should return error when LRO is not negoiated
after the reset (say, when it's disabled through qemu command line)?

--yliu

[dpdk-dev] [PATCH v2 03/12] virtio: reinitialize the device in configure callback

2016-10-13 Thread Olivier MATZ 


On 10/13/2016 09:54 AM, Yuanhan Liu wrote:
> On Wed, Oct 12, 2016 at 06:01:25PM +0200, Olivier MATZ wrote:
>> Hello Yuanhan,
>>
>> On 10/12/2016 04:41 PM, Yuanhan Liu wrote:
>>> On Mon, Oct 03, 2016 at 11:00:14AM +0200, Olivier Matz wrote:
 @@ -1344,6 +1347,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
   {
const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
struct virtio_hw *hw = dev->data->dev_private;
 +  uint64_t req_features;
int ret;

PMD_INIT_LOG(DEBUG, "configure");
 @@ -1353,6 +1357,14 @@ virtio_dev_configure(struct rte_eth_dev *dev)
return -EINVAL;
}

 +  req_features = VIRTIO_PMD_GUEST_FEATURES;
 +  /* 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;
 +  }
>>>
>>> Why do you have to reset virtio here? This doesn't make too much sense
>>> to me.
>>>
>>> IIUC, you want to make sure those TSO related features being unset at
>>> init time, and enable it (by doing reset) when it's asked to be enabled
>>> (by rte_eth_dev_configure)?
>>>
>>> Why not always setting those features? We could do the actual offloads
>>> when:
>>>
>>> - those features have been negoiated
>>>
>>> - they are enabled through rte_eth_dev_configure
>>>
>>> With that, I think we could avoid the reset here?
>>
>> It would work for TX, since you decide to use or not the feature. But I
>> think this won't work for RX: if you negociate LRO at init, the host may
>> send you large packets, even if LRO is disabled in dev_configure.
>
> I see. Thanks.
>
> Besides, I think you should return error when LRO is not negoiated
> after the reset (say, when it's disabled through qemu command line)?

Good idea, I now return an error if offload cannot be negotiated.

Olivier