Re: [PATCH v2 03/12] virtio: reinitialize the device in configure callback

[Olivier MATZ <olivier.matz@6wind.com>]

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