[dpdk-dev,3/5] net/virtio-user: support to report net status
Checks
Commit Message
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
drivers/net/virtio/virtio_user/virtio_user_dev.c | 1 +
drivers/net/virtio/virtio_user_ethdev.c | 13 +++++++------
2 files changed, 8 insertions(+), 6 deletions(-)
Comments
On Fri, Mar 03, 2017 at 05:56:41PM +0000, Jianfeng Tan wrote:
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
Yet again, not a single work on explanation.
> ---
> drivers/net/virtio/virtio_user/virtio_user_dev.c | 1 +
> drivers/net/virtio/virtio_user_ethdev.c | 13 +++++++------
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 9777d6b..cc6f557 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -176,6 +176,7 @@ virtio_user_start_device(struct virtio_user_dev *dev, uint8_t portid)
> features &= ~(1ull << VIRTIO_NET_F_MAC);
> /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to know */
> features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
> + features &= ~(1ull << VIRTIO_NET_F_STATUS);
> ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES, &features);
> if (ret < 0)
> goto error;
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
> index fa79419..fbdd0a8 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -121,7 +121,8 @@ virtio_user_get_features(struct virtio_hw *hw)
> struct virtio_user_dev *dev = virtio_user_get_dev(hw);
>
> /* unmask feature bits defined in vhost user protocol */
> - return dev->device_features & VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
> + return (dev->device_features | (1 << VIRTIO_NET_F_STATUS))
> + & VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
Why not handle the features at virtio_user_dev_init()?
> }
>
> static void
> @@ -129,23 +130,23 @@ virtio_user_set_features(struct virtio_hw *hw, uint64_t features)
> {
> struct virtio_user_dev *dev = virtio_user_get_dev(hw);
>
> - dev->features = features & dev->device_features;
> + dev->features = features;
> }
>
> static uint8_t
> virtio_user_get_isr(struct virtio_hw *hw __rte_unused)
> {
> - /* When config interrupt happens, driver calls this function to query
> - * what kinds of change happen. Interrupt mode not supported for now.
> + /* rxq interrupts and config interrupt are separated in virtio-user,
> + * here we only report config change.
> */
> - return 0;
> + return VIRTIO_PCI_ISR_CONFIG;
> }
>
> static uint16_t
> virtio_user_set_config_irq(struct virtio_hw *hw __rte_unused,
> uint16_t vec __rte_unused)
> {
> - return VIRTIO_MSI_NO_VECTOR;
> + return 0;
And the above two changes have something to do with this patch (support
to report net statu)?
--yliu
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Friday, March 17, 2017 2:55 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; david.marchand@6wind.com
> Subject: Re: [PATCH 3/5] net/virtio-user: support to report net status
>
> On Fri, Mar 03, 2017 at 05:56:41PM +0000, Jianfeng Tan wrote:
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>
> Yet again, not a single work on explanation.
Sorry, will add in next version.
>
> > ---
> > drivers/net/virtio/virtio_user/virtio_user_dev.c | 1 +
> > drivers/net/virtio/virtio_user_ethdev.c | 13 +++++++------
> > 2 files changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > index 9777d6b..cc6f557 100644
> > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > @@ -176,6 +176,7 @@ virtio_user_start_device(struct virtio_user_dev
> *dev, uint8_t portid)
> > features &= ~(1ull << VIRTIO_NET_F_MAC);
> > /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to
> know */
> > features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
> > + features &= ~(1ull << VIRTIO_NET_F_STATUS);
> > ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES,
> &features);
> > if (ret < 0)
> > goto error;
> > diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> > index fa79419..fbdd0a8 100644
> > --- a/drivers/net/virtio/virtio_user_ethdev.c
> > +++ b/drivers/net/virtio/virtio_user_ethdev.c
> > @@ -121,7 +121,8 @@ virtio_user_get_features(struct virtio_hw *hw)
> > struct virtio_user_dev *dev = virtio_user_get_dev(hw);
> >
> > /* unmask feature bits defined in vhost user protocol */
> > - return dev->device_features &
> VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
> > + return (dev->device_features | (1 << VIRTIO_NET_F_STATUS))
> > + & VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
>
> Why not handle the features at virtio_user_dev_init()?
You mean add VIRTIO_NET_F_STATUS when get_features from device? Yes, we could do that there. But we originally add device_features to only record features supported by device.
>
> > }
> >
> > static void
> > @@ -129,23 +130,23 @@ virtio_user_set_features(struct virtio_hw *hw,
> uint64_t features)
> > {
> > struct virtio_user_dev *dev = virtio_user_get_dev(hw);
> >
> > - dev->features = features & dev->device_features;
> > + dev->features = features;
> > }
> >
> > static uint8_t
> > virtio_user_get_isr(struct virtio_hw *hw __rte_unused)
> > {
> > - /* When config interrupt happens, driver calls this function to query
> > - * what kinds of change happen. Interrupt mode not supported for
> now.
> > + /* rxq interrupts and config interrupt are separated in virtio-user,
> > + * here we only report config change.
> > */
> > - return 0;
> > + return VIRTIO_PCI_ISR_CONFIG;
> > }
> >
> > static uint16_t
> > virtio_user_set_config_irq(struct virtio_hw *hw __rte_unused,
> > uint16_t vec __rte_unused)
> > {
> > - return VIRTIO_MSI_NO_VECTOR;
> > + return 0;
>
> And the above two changes have something to do with this patch (support
> to report net statu)?
Yes, just pretend that irq/vec binding is configured successfully. When setting up config irq, the result is checked.
Thanks,
Jianfeng
On Mon, Mar 27, 2017 at 07:46:32AM +0000, Tan, Jianfeng wrote:
> > > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > > index 9777d6b..cc6f557 100644
> > > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > > @@ -176,6 +176,7 @@ virtio_user_start_device(struct virtio_user_dev
> > *dev, uint8_t portid)
> > > features &= ~(1ull << VIRTIO_NET_F_MAC);
> > > /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to
> > know */
> > > features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
> > > + features &= ~(1ull << VIRTIO_NET_F_STATUS);
> > > ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES,
> > &features);
> > > if (ret < 0)
> > > goto error;
> > > diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> > b/drivers/net/virtio/virtio_user_ethdev.c
> > > index fa79419..fbdd0a8 100644
> > > --- a/drivers/net/virtio/virtio_user_ethdev.c
> > > +++ b/drivers/net/virtio/virtio_user_ethdev.c
> > > @@ -121,7 +121,8 @@ virtio_user_get_features(struct virtio_hw *hw)
> > > struct virtio_user_dev *dev = virtio_user_get_dev(hw);
> > >
> > > /* unmask feature bits defined in vhost user protocol */
> > > - return dev->device_features &
> > VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
> > > + return (dev->device_features | (1 << VIRTIO_NET_F_STATUS))
> > > + & VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
> >
> > Why not handle the features at virtio_user_dev_init()?
>
> You mean add VIRTIO_NET_F_STATUS when get_features from device? Yes, we could do that there. But we originally add device_features to only record features supported by device.
>
Aren't you adding the F_STATUS features to this device?
--yliu
On 3/29/2017 2:33 PM, Yuanhan Liu wrote:
> On Mon, Mar 27, 2017 at 07:46:32AM +0000, Tan, Jianfeng wrote:
>>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>> index 9777d6b..cc6f557 100644
>>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>> @@ -176,6 +176,7 @@ virtio_user_start_device(struct virtio_user_dev
>>> *dev, uint8_t portid)
>>>> features &= ~(1ull << VIRTIO_NET_F_MAC);
>>>> /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to
>>> know */
>>>> features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
>>>> + features &= ~(1ull << VIRTIO_NET_F_STATUS);
>>>> ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES,
>>> &features);
>>>> if (ret < 0)
>>>> goto error;
>>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>>> b/drivers/net/virtio/virtio_user_ethdev.c
>>>> index fa79419..fbdd0a8 100644
>>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>>> @@ -121,7 +121,8 @@ virtio_user_get_features(struct virtio_hw *hw)
>>>> struct virtio_user_dev *dev = virtio_user_get_dev(hw);
>>>>
>>>> /* unmask feature bits defined in vhost user protocol */
>>>> - return dev->device_features &
>>> VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
>>>> + return (dev->device_features | (1 << VIRTIO_NET_F_STATUS))
>>>> + & VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
>>> Why not handle the features at virtio_user_dev_init()?
>> You mean add VIRTIO_NET_F_STATUS when get_features from device? Yes, we could do that there. But we originally add device_features to only record features supported by device.
>>
> Aren't you adding the F_STATUS features to this device?
>
For virtio driver, yes, we are adding F_STATUS feature so the it sees a
device supporting LSC. But for backend driver, we need hide this
feature, it happens when we set_features to the backend driver.
Thanks,
Jianfeng
On Wed, Mar 29, 2017 at 03:07:28PM +0800, Tan, Jianfeng wrote:
>
>
> On 3/29/2017 2:33 PM, Yuanhan Liu wrote:
> >On Mon, Mar 27, 2017 at 07:46:32AM +0000, Tan, Jianfeng wrote:
> >>>>diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>>index 9777d6b..cc6f557 100644
> >>>>--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>>+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>>@@ -176,6 +176,7 @@ virtio_user_start_device(struct virtio_user_dev
> >>>*dev, uint8_t portid)
> >>>> features &= ~(1ull << VIRTIO_NET_F_MAC);
> >>>> /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to
> >>>know */
> >>>> features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
> >>>>+ features &= ~(1ull << VIRTIO_NET_F_STATUS);
> >>>> ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES,
> >>>&features);
> >>>> if (ret < 0)
> >>>> goto error;
> >>>>diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> >>>b/drivers/net/virtio/virtio_user_ethdev.c
> >>>>index fa79419..fbdd0a8 100644
> >>>>--- a/drivers/net/virtio/virtio_user_ethdev.c
> >>>>+++ b/drivers/net/virtio/virtio_user_ethdev.c
> >>>>@@ -121,7 +121,8 @@ virtio_user_get_features(struct virtio_hw *hw)
> >>>> struct virtio_user_dev *dev = virtio_user_get_dev(hw);
> >>>>
> >>>> /* unmask feature bits defined in vhost user protocol */
> >>>>- return dev->device_features &
> >>>VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
> >>>>+ return (dev->device_features | (1 << VIRTIO_NET_F_STATUS))
> >>>>+ & VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
> >>>Why not handle the features at virtio_user_dev_init()?
> >>You mean add VIRTIO_NET_F_STATUS when get_features from device? Yes, we could do that there. But we originally add device_features to only record features supported by device.
> >>
> >Aren't you adding the F_STATUS features to this device?
> >
>
> For virtio driver, yes, we are adding F_STATUS feature so the it sees a
> device supporting LSC.
That means you are adding a device feature (F_STATUS) and reporting it to
the driver that this feature is always on, no matter whether the device
actually supports it or not? This looks wrong to me.
> But for backend driver, we need hide this feature, it
> happens when we set_features to the backend driver.
The feature negotion of virtio-user seems broken to me.
--yliu
On 3/29/2017 3:14 PM, Yuanhan Liu wrote:
> On Wed, Mar 29, 2017 at 03:07:28PM +0800, Tan, Jianfeng wrote:
>>
>> On 3/29/2017 2:33 PM, Yuanhan Liu wrote:
>>> On Mon, Mar 27, 2017 at 07:46:32AM +0000, Tan, Jianfeng wrote:
>>>>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>>>> index 9777d6b..cc6f557 100644
>>>>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>>>> @@ -176,6 +176,7 @@ virtio_user_start_device(struct virtio_user_dev
>>>>> *dev, uint8_t portid)
>>>>>> features &= ~(1ull << VIRTIO_NET_F_MAC);
>>>>>> /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to
>>>>> know */
>>>>>> features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
>>>>>> + features &= ~(1ull << VIRTIO_NET_F_STATUS);
>>>>>> ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES,
>>>>> &features);
>>>>>> if (ret < 0)
>>>>>> goto error;
>>>>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>>>>> b/drivers/net/virtio/virtio_user_ethdev.c
>>>>>> index fa79419..fbdd0a8 100644
>>>>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>>>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>>>>> @@ -121,7 +121,8 @@ virtio_user_get_features(struct virtio_hw *hw)
>>>>>> struct virtio_user_dev *dev = virtio_user_get_dev(hw);
>>>>>>
>>>>>> /* unmask feature bits defined in vhost user protocol */
>>>>>> - return dev->device_features &
>>>>> VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
>>>>>> + return (dev->device_features | (1 << VIRTIO_NET_F_STATUS))
>>>>>> + & VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
>>>>> Why not handle the features at virtio_user_dev_init()?
>>>> You mean add VIRTIO_NET_F_STATUS when get_features from device? Yes, we could do that there. But we originally add device_features to only record features supported by device.
>>>>
>>> Aren't you adding the F_STATUS features to this device?
>>>
>> For virtio driver, yes, we are adding F_STATUS feature so the it sees a
>> device supporting LSC.
> That means you are adding a device feature (F_STATUS) and reporting it to
> the driver that this feature is always on, no matter whether the device
> actually supports it or not? This looks wrong to me.
Why? IMO, device is not necessary to know this feature. For vhost-user,
except explicitly cutting down the unix connection, I don't see a
message of vhost-user protocol to report that.
>
>> But for backend driver, we need hide this feature, it
>> happens when we set_features to the backend driver.
> The feature negotion of virtio-user seems broken to me.
>
>
Why? vhost does not claim to support this feature, VHOST_SUPPORTED_FEATURES.
Thanks,
Jianfeng
On Wed, Mar 29, 2017 at 03:48:04PM +0800, Tan, Jianfeng wrote:
>
>
> On 3/29/2017 3:14 PM, Yuanhan Liu wrote:
> >On Wed, Mar 29, 2017 at 03:07:28PM +0800, Tan, Jianfeng wrote:
> >>
> >>On 3/29/2017 2:33 PM, Yuanhan Liu wrote:
> >>>On Mon, Mar 27, 2017 at 07:46:32AM +0000, Tan, Jianfeng wrote:
> >>>>>>diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>>>b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>>>>index 9777d6b..cc6f557 100644
> >>>>>>--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>>>>+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>>>>@@ -176,6 +176,7 @@ virtio_user_start_device(struct virtio_user_dev
> >>>>>*dev, uint8_t portid)
> >>>>>> features &= ~(1ull << VIRTIO_NET_F_MAC);
> >>>>>> /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to
> >>>>>know */
> >>>>>> features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
> >>>>>>+ features &= ~(1ull << VIRTIO_NET_F_STATUS);
> >>>>>> ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES,
> >>>>>&features);
> >>>>>> if (ret < 0)
> >>>>>> goto error;
> >>>>>>diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> >>>>>b/drivers/net/virtio/virtio_user_ethdev.c
> >>>>>>index fa79419..fbdd0a8 100644
> >>>>>>--- a/drivers/net/virtio/virtio_user_ethdev.c
> >>>>>>+++ b/drivers/net/virtio/virtio_user_ethdev.c
> >>>>>>@@ -121,7 +121,8 @@ virtio_user_get_features(struct virtio_hw *hw)
> >>>>>> struct virtio_user_dev *dev = virtio_user_get_dev(hw);
> >>>>>>
> >>>>>> /* unmask feature bits defined in vhost user protocol */
> >>>>>>- return dev->device_features &
> >>>>>VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
> >>>>>>+ return (dev->device_features | (1 << VIRTIO_NET_F_STATUS))
> >>>>>>+ & VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
> >>>>>Why not handle the features at virtio_user_dev_init()?
> >>>>You mean add VIRTIO_NET_F_STATUS when get_features from device? Yes, we could do that there. But we originally add device_features to only record features supported by device.
> >>>>
> >>>Aren't you adding the F_STATUS features to this device?
> >>>
> >>For virtio driver, yes, we are adding F_STATUS feature so the it sees a
> >>device supporting LSC.
> >That means you are adding a device feature (F_STATUS) and reporting it to
> >the driver that this feature is always on, no matter whether the device
> >actually supports it or not? This looks wrong to me.
>
> Why?
Because you were doing hack to make virtio-user work, while there is no
such hack in the QEMU virtio-pci implementation.
> IMO, device is not necessary to know this feature.
F_STATUS is a device feature. If such feature is not claimed to be
supported by the device, the driver should ignore it. But you are
unconditionally letting the driver handle it, even though the
virito-user device does not claim to support it.
Note that F_STATUS is set in the QEMU virtio-net PCI device.
--yliu
> For vhost-user,
> except explicitly cutting down the unix connection, I don't see a message of
> vhost-user protocol to report that.
>
> >
> >>But for backend driver, we need hide this feature, it
> >>happens when we set_features to the backend driver.
> >The feature negotion of virtio-user seems broken to me.
> >
> >
> Why?
> vhost does not claim to support this feature, VHOST_SUPPORTED_FEATURES.
>
> Thanks,
> Jianfeng
On 3/29/2017 4:00 PM, Yuanhan Liu wrote:
> On Wed, Mar 29, 2017 at 03:48:04PM +0800, Tan, Jianfeng wrote:
>>
>> On 3/29/2017 3:14 PM, Yuanhan Liu wrote:
>>> On Wed, Mar 29, 2017 at 03:07:28PM +0800, Tan, Jianfeng wrote:
>>>> On 3/29/2017 2:33 PM, Yuanhan Liu wrote:
>>>>> On Mon, Mar 27, 2017 at 07:46:32AM +0000, Tan, Jianfeng wrote:
>>>>>>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>>>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>>>>>> index 9777d6b..cc6f557 100644
>>>>>>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>>>>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>>>>>> @@ -176,6 +176,7 @@ virtio_user_start_device(struct virtio_user_dev
>>>>>>> *dev, uint8_t portid)
>>>>>>>> features &= ~(1ull << VIRTIO_NET_F_MAC);
>>>>>>>> /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to
>>>>>>> know */
>>>>>>>> features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
>>>>>>>> + features &= ~(1ull << VIRTIO_NET_F_STATUS);
>>>>>>>> ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES,
>>>>>>> &features);
>>>>>>>> if (ret < 0)
>>>>>>>> goto error;
>>>>>>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>>>>>>> b/drivers/net/virtio/virtio_user_ethdev.c
>>>>>>>> index fa79419..fbdd0a8 100644
>>>>>>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>>>>>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>>>>>>> @@ -121,7 +121,8 @@ virtio_user_get_features(struct virtio_hw *hw)
>>>>>>>> struct virtio_user_dev *dev = virtio_user_get_dev(hw);
>>>>>>>>
>>>>>>>> /* unmask feature bits defined in vhost user protocol */
>>>>>>>> - return dev->device_features &
>>>>>>> VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
>>>>>>>> + return (dev->device_features | (1 << VIRTIO_NET_F_STATUS))
>>>>>>>> + & VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
>>>>>>> Why not handle the features at virtio_user_dev_init()?
>>>>>> You mean add VIRTIO_NET_F_STATUS when get_features from device? Yes, we could do that there. But we originally add device_features to only record features supported by device.
>>>>>>
>>>>> Aren't you adding the F_STATUS features to this device?
>>>>>
>>>> For virtio driver, yes, we are adding F_STATUS feature so the it sees a
>>>> device supporting LSC.
>>> That means you are adding a device feature (F_STATUS) and reporting it to
>>> the driver that this feature is always on, no matter whether the device
>>> actually supports it or not? This looks wrong to me.
>> Why?
> Because you were doing hack to make virtio-user work, while there is no
> such hack in the QEMU virtio-pci implementation.
>
>> IMO, device is not necessary to know this feature.
> F_STATUS is a device feature. If such feature is not claimed to be
> supported by the device, the driver should ignore it. But you are
> unconditionally letting the driver handle it, even though the
> virito-user device does not claim to support it.
>
> Note that F_STATUS is set in the QEMU virtio-net PCI device.
In QEMU virtio-net PCI device, the device includes QEMU device emulation
+ vhost backend driver. This feature only shows at QEMU device
emulation, instead of vhost backend driver. This is why we did not see
this feature at any vhost backends.
And the embedded virtio-user actually substitutes QEMU device emulation
part, so this feature should end at this layer.
Let's look this in this way, if we let this feature go through to vhost
user, it will be treated as an error in vhost_user_set_features().
Thanks,
Jianfeng
>
> --yliu
>
>> For vhost-user,
>> except explicitly cutting down the unix connection, I don't see a message of
>> vhost-user protocol to report that.
>>
>>>> But for backend driver, we need hide this feature, it
>>>> happens when we set_features to the backend driver.
>>> The feature negotion of virtio-user seems broken to me.
>>>
>>>
>> Why?
>> vhost does not claim to support this feature, VHOST_SUPPORTED_FEATURES.
>>
>> Thanks,
>> Jianfeng
On Wed, Mar 29, 2017 at 04:33:20PM +0800, Tan, Jianfeng wrote:
>
>
> On 3/29/2017 4:00 PM, Yuanhan Liu wrote:
> >On Wed, Mar 29, 2017 at 03:48:04PM +0800, Tan, Jianfeng wrote:
> >>
> >>On 3/29/2017 3:14 PM, Yuanhan Liu wrote:
> >>>On Wed, Mar 29, 2017 at 03:07:28PM +0800, Tan, Jianfeng wrote:
> >>>>On 3/29/2017 2:33 PM, Yuanhan Liu wrote:
> >>>>>On Mon, Mar 27, 2017 at 07:46:32AM +0000, Tan, Jianfeng wrote:
> >>>>>>>>diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>>>>>b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>>>>>>index 9777d6b..cc6f557 100644
> >>>>>>>>--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>>>>>>+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>>>>>>@@ -176,6 +176,7 @@ virtio_user_start_device(struct virtio_user_dev
> >>>>>>>*dev, uint8_t portid)
> >>>>>>>> features &= ~(1ull << VIRTIO_NET_F_MAC);
> >>>>>>>> /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to
> >>>>>>>know */
> >>>>>>>> features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
> >>>>>>>>+ features &= ~(1ull << VIRTIO_NET_F_STATUS);
> >>>>>>>> ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES,
> >>>>>>>&features);
> >>>>>>>> if (ret < 0)
> >>>>>>>> goto error;
> >>>>>>>>diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> >>>>>>>b/drivers/net/virtio/virtio_user_ethdev.c
> >>>>>>>>index fa79419..fbdd0a8 100644
> >>>>>>>>--- a/drivers/net/virtio/virtio_user_ethdev.c
> >>>>>>>>+++ b/drivers/net/virtio/virtio_user_ethdev.c
> >>>>>>>>@@ -121,7 +121,8 @@ virtio_user_get_features(struct virtio_hw *hw)
> >>>>>>>> struct virtio_user_dev *dev = virtio_user_get_dev(hw);
> >>>>>>>>
> >>>>>>>> /* unmask feature bits defined in vhost user protocol */
> >>>>>>>>- return dev->device_features &
> >>>>>>>VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
> >>>>>>>>+ return (dev->device_features | (1 << VIRTIO_NET_F_STATUS))
> >>>>>>>>+ & VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
> >>>>>>>Why not handle the features at virtio_user_dev_init()?
> >>>>>>You mean add VIRTIO_NET_F_STATUS when get_features from device? Yes, we could do that there. But we originally add device_features to only record features supported by device.
> >>>>>>
> >>>>>Aren't you adding the F_STATUS features to this device?
> >>>>>
> >>>>For virtio driver, yes, we are adding F_STATUS feature so the it sees a
> >>>>device supporting LSC.
> >>>That means you are adding a device feature (F_STATUS) and reporting it to
> >>>the driver that this feature is always on, no matter whether the device
> >>>actually supports it or not? This looks wrong to me.
> >>Why?
> >Because you were doing hack to make virtio-user work, while there is no
> >such hack in the QEMU virtio-pci implementation.
> >
> >>IMO, device is not necessary to know this feature.
> >F_STATUS is a device feature. If such feature is not claimed to be
> >supported by the device, the driver should ignore it. But you are
> >unconditionally letting the driver handle it, even though the
> >virito-user device does not claim to support it.
> >
> >Note that F_STATUS is set in the QEMU virtio-net PCI device.
>
> In QEMU virtio-net PCI device, the device includes QEMU device emulation +
> vhost backend driver. This feature only shows at QEMU device emulation,
> instead of vhost backend driver. This is why we did not see this feature at
> any vhost backends.
> And the embedded virtio-user actually substitutes QEMU device emulation
> part, so this feature should end at this layer.
>
> Let's look this in this way, if we let this feature go through to vhost
> user, it will be treated as an error in vhost_user_set_features().
Again, why can't we (virtio-user) follow QEMU?
--yliu
On 3/29/2017 4:36 PM, Yuanhan Liu wrote:
> On Wed, Mar 29, 2017 at 04:33:20PM +0800, Tan, Jianfeng wrote:
>>
>> On 3/29/2017 4:00 PM, Yuanhan Liu wrote:
>>> On Wed, Mar 29, 2017 at 03:48:04PM +0800, Tan, Jianfeng wrote:
>>>> On 3/29/2017 3:14 PM, Yuanhan Liu wrote:
>>>>> On Wed, Mar 29, 2017 at 03:07:28PM +0800, Tan, Jianfeng wrote:
>>>>>> On 3/29/2017 2:33 PM, Yuanhan Liu wrote:
>>>>>>> On Mon, Mar 27, 2017 at 07:46:32AM +0000, Tan, Jianfeng wrote:
>>>>>>>>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>>>>>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>>>>>>>> index 9777d6b..cc6f557 100644
>>>>>>>>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>>>>>>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>>>>>>>> @@ -176,6 +176,7 @@ virtio_user_start_device(struct virtio_user_dev
>>>>>>>>> *dev, uint8_t portid)
>>>>>>>>>> features &= ~(1ull << VIRTIO_NET_F_MAC);
>>>>>>>>>> /* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to
>>>>>>>>> know */
>>>>>>>>>> features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
>>>>>>>>>> + features &= ~(1ull << VIRTIO_NET_F_STATUS);
>>>>>>>>>> ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES,
>>>>>>>>> &features);
>>>>>>>>>> if (ret < 0)
>>>>>>>>>> goto error;
>>>>>>>>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>>>>>>>>> b/drivers/net/virtio/virtio_user_ethdev.c
>>>>>>>>>> index fa79419..fbdd0a8 100644
>>>>>>>>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>>>>>>>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>>>>>>>>> @@ -121,7 +121,8 @@ virtio_user_get_features(struct virtio_hw *hw)
>>>>>>>>>> struct virtio_user_dev *dev = virtio_user_get_dev(hw);
>>>>>>>>>>
>>>>>>>>>> /* unmask feature bits defined in vhost user protocol */
>>>>>>>>>> - return dev->device_features &
>>>>>>>>> VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
>>>>>>>>>> + return (dev->device_features | (1 << VIRTIO_NET_F_STATUS))
>>>>>>>>>> + & VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
>>>>>>>>> Why not handle the features at virtio_user_dev_init()?
>>>>>>>> You mean add VIRTIO_NET_F_STATUS when get_features from device? Yes, we could do that there. But we originally add device_features to only record features supported by device.
>>>>>>>>
>>>>>>> Aren't you adding the F_STATUS features to this device?
>>>>>>>
>>>>>> For virtio driver, yes, we are adding F_STATUS feature so the it sees a
>>>>>> device supporting LSC.
>>>>> That means you are adding a device feature (F_STATUS) and reporting it to
>>>>> the driver that this feature is always on, no matter whether the device
>>>>> actually supports it or not? This looks wrong to me.
>>>> Why?
>>> Because you were doing hack to make virtio-user work, while there is no
>>> such hack in the QEMU virtio-pci implementation.
>>>
>>>> IMO, device is not necessary to know this feature.
>>> F_STATUS is a device feature. If such feature is not claimed to be
>>> supported by the device, the driver should ignore it. But you are
>>> unconditionally letting the driver handle it, even though the
>>> virito-user device does not claim to support it.
>>>
>>> Note that F_STATUS is set in the QEMU virtio-net PCI device.
>> In QEMU virtio-net PCI device, the device includes QEMU device emulation +
>> vhost backend driver. This feature only shows at QEMU device emulation,
>> instead of vhost backend driver. This is why we did not see this feature at
>> any vhost backends.
>> And the embedded virtio-user actually substitutes QEMU device emulation
>> part, so this feature should end at this layer.
>>
>> Let's look this in this way, if we let this feature go through to vhost
>> user, it will be treated as an error in vhost_user_set_features().
> Again, why can't we (virtio-user) follow QEMU?
Through F2F discussion, I got to know that you treat
virtio_user_ethdev.c as front driver part. That makes sense. I'll move
this change into vhost-user directory.
Thanks,
Jianfeng
@@ -176,6 +176,7 @@ virtio_user_start_device(struct virtio_user_dev *dev, uint8_t portid)
features &= ~(1ull << VIRTIO_NET_F_MAC);
/* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to know */
features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
+ features &= ~(1ull << VIRTIO_NET_F_STATUS);
ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES, &features);
if (ret < 0)
goto error;
@@ -121,7 +121,8 @@ virtio_user_get_features(struct virtio_hw *hw)
struct virtio_user_dev *dev = virtio_user_get_dev(hw);
/* unmask feature bits defined in vhost user protocol */
- return dev->device_features & VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
+ return (dev->device_features | (1 << VIRTIO_NET_F_STATUS))
+ & VIRTIO_PMD_SUPPORTED_GUEST_FEATURES;
}
static void
@@ -129,23 +130,23 @@ virtio_user_set_features(struct virtio_hw *hw, uint64_t features)
{
struct virtio_user_dev *dev = virtio_user_get_dev(hw);
- dev->features = features & dev->device_features;
+ dev->features = features;
}
static uint8_t
virtio_user_get_isr(struct virtio_hw *hw __rte_unused)
{
- /* When config interrupt happens, driver calls this function to query
- * what kinds of change happen. Interrupt mode not supported for now.
+ /* rxq interrupts and config interrupt are separated in virtio-user,
+ * here we only report config change.
*/
- return 0;
+ return VIRTIO_PCI_ISR_CONFIG;
}
static uint16_t
virtio_user_set_config_irq(struct virtio_hw *hw __rte_unused,
uint16_t vec __rte_unused)
{
- return VIRTIO_MSI_NO_VECTOR;
+ return 0;
}
static uint16_t