Re: [Qemu-devel] [PATCH] virtio: add support for in-order feature

[Ilya Maximets]

On 13.08.2018 18:35, Michael S. Tsirkin wrote:
> On Mon, Aug 13, 2018 at 06:28:06PM +0300, Ilya Maximets wrote:
>> On 13.08.2018 12:56, Michael S. Tsirkin wrote:
>>> On Mon, Aug 13, 2018 at 10:55:23AM +0300, Ilya Maximets wrote:
>>>> On 10.08.2018 22:19, Michael S. Tsirkin wrote:
>>>>> On Fri, Aug 10, 2018 at 02:04:47PM +0300, Ilya Maximets wrote:
>>>>>> On 10.08.2018 12:34, Michael S. Tsirkin wrote:
>>>>>>> On Fri, Aug 10, 2018 at 11:28:47AM +0300, Ilya Maximets wrote:
>>>>>>>> On 10.08.2018 01:51, Michael S. Tsirkin wrote:
>>>>>>>>> On Thu, Aug 09, 2018 at 07:54:37PM +0300, Ilya Maximets wrote:
>>>>>>>>>> New feature bit for in-order feature of the upcoming
>>>>>>>>>> virtio 1.1. It's already supported by DPDK vhost-user
>>>>>>>>>> and virtio implementations. These changes required to
>>>>>>>>>> allow feature negotiation.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Ilya Maximets <address@hidden>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> I just wanted to test this new feature in DPDK but failed
>>>>>>>>>> to found required patch for QEMU side. So, I implemented it.
>>>>>>>>>> At least it will be helpful for someone like me, who wants
>>>>>>>>>> to evaluate VIRTIO_F_IN_ORDER with DPDK.
>>>>>>>>>>
>>>>>>>>>>  hw/net/vhost_net.c                             |  1 +
>>>>>>>>>>  include/hw/virtio/virtio.h                     | 12 +++++++-----
>>>>>>>>>>  include/standard-headers/linux/virtio_config.h |  7 +++++++
>>>>>>>>>>  3 files changed, 15 insertions(+), 5 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>>>>>>>>> index e037db6..86879c5 100644
>>>>>>>>>> --- a/hw/net/vhost_net.c
>>>>>>>>>> +++ b/hw/net/vhost_net.c
>>>>>>>>>> @@ -78,6 +78,7 @@ static const int user_feature_bits[] = {
>>>>>>>>>>      VIRTIO_NET_F_MRG_RXBUF,
>>>>>>>>>>      VIRTIO_NET_F_MTU,
>>>>>>>>>>      VIRTIO_F_IOMMU_PLATFORM,
>>>>>>>>>> +    VIRTIO_F_IN_ORDER,
>>>>>>>>>>  
>>>>>>>>>>      /* This bit implies RARP isn't sent by QEMU out of band */
>>>>>>>>>>      VIRTIO_NET_F_GUEST_ANNOUNCE,
>>>>>>>>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>>>>>>>>> index 9c1fa07..a422025 100644
>>>>>>>>>> --- a/include/hw/virtio/virtio.h
>>>>>>>>>> +++ b/include/hw/virtio/virtio.h
>>>>>>>>>> @@ -254,16 +254,18 @@ typedef struct virtio_input_conf 
>>>>>>>>>> virtio_input_conf;
>>>>>>>>>>  typedef struct VirtIOSCSIConf VirtIOSCSIConf;
>>>>>>>>>>  typedef struct VirtIORNGConf VirtIORNGConf;
>>>>>>>>>>  
>>>>>>>>>> -#define DEFINE_VIRTIO_COMMON_FEATURES(_state, _field) \
>>>>>>>>>> +#define DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)     \
>>>>>>>>>>      DEFINE_PROP_BIT64("indirect_desc", _state, _field,    \
>>>>>>>>>>                        VIRTIO_RING_F_INDIRECT_DESC, true), \
>>>>>>>>>>      DEFINE_PROP_BIT64("event_idx", _state, _field,        \
>>>>>>>>>>                        VIRTIO_RING_F_EVENT_IDX, true),     \
>>>>>>>>>>      DEFINE_PROP_BIT64("notify_on_empty", _state, _field,  \
>>>>>>>>>> -                      VIRTIO_F_NOTIFY_ON_EMPTY, true), \
>>>>>>>>>> -    DEFINE_PROP_BIT64("any_layout", _state, _field, \
>>>>>>>>>> -                      VIRTIO_F_ANY_LAYOUT, true), \
>>>>>>>>>> -    DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
>>>>>>>>>> +                      VIRTIO_F_NOTIFY_ON_EMPTY, true),    \
>>>>>>>>>> +    DEFINE_PROP_BIT64("any_layout", _state, _field,       \
>>>>>>>>>> +                      VIRTIO_F_ANY_LAYOUT, true),         \
>>>>>>>>>> +    DEFINE_PROP_BIT64("in_order", _state, _field,         \
>>>>>>>>>> +                      VIRTIO_F_IN_ORDER, true),           \
>>>>>>>>>> +    DEFINE_PROP_BIT64("iommu_platform", _state, _field,   \
>>>>>>>>>>                        VIRTIO_F_IOMMU_PLATFORM, false)
>>>>>>>>>
>>>>>>>>> Is in_order really right for all virtio devices?
>>>>>>>>
>>>>>>>> I see nothing device specific in this feature. It just specifies
>>>>>>>> some restrictions on the descriptors handling. All virtio devices
>>>>>>>> could use it to have performance benefits. Also, upcoming packed
>>>>>>>> rings should give a good performance boost in case of enabled
>>>>>>>> in-order feature. And packed rings RFC [1] implements
>>>>>>>> VIRTIO_F_RING_PACKED for all virtio devices. So, I see no issues
>>>>>>>> in enabling in-order negotiation for all of them.
>>>>>>>>
>>>>>>>> What do you think?
>>>>>>>>
>>>>>>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg01028.html
>>>>>>>>
>>>>>>>> Best regards, Ilya Maximets.
>>>>>>>
>>>>>>> If guest assumes in-order use of buffers but device uses them out of
>>>>>>> order then guest will crash. So there's a missing piece where
>>>>>>> you actually make devices use buffers in order when the flag is set.
>>>>>>
>>>>>> I thought that feature negotiation is the mechanism that should
>>>>>> protect us from situations like that. Isn't it?
>>>>>> If device negotiates in-order feature, when it MUST (as described
>>>>>> in spec) use buffers in the same order in which they have been
>>>>>> available.
>>>>>
>>>>> Exactly. And your patch does nothing to ensure that,
>>>
>>> Let me elaborate. Your patch adds an in order property to
>>> all virtio devices. When set, guests will assume that
>>> all buffers are used in the order they have been made
>>> available. However, IIUC current virtio code in qemu
>>> sometimes uses buffers out of order. Therefore
>>> with your patch applied devices behave out of spec.
>>>
>>> You need to either make sure the flag forces in-order
>>> behaviour, or limit the flag to devices which are
>>> in-order.
>>>
>>
>> OK. Finally, I got your point. Thanks for explanation.
>> So, we have a bunch of qemu virtio devices that doesn't
>> support this option.
>>
>> I see 2 options that could be not so hard to implement:
>>
>> 1. Set the default value of 'in_order' property to 'false'.
>>    In this case, It'll be the users responsibility to ensure
>>    that device that he wants to use supports this feature.
> 
> I don't much like this option, I don't see how is the user
> supposed to know.

Sure. This is not obvious. At least user could know that if
some specific vhost backend will be used.
But yes, this is not a good option.

> 
>> 2. Remove the "in_order" property bit and enable the feature
>>    for now only for virtio-net device
> 
> I suspect at least serial, balloon, rng can all support this.

Hmm. I'll try to check, but this could take some time.

> 
> BTW you also need to disable for compat machine types.

OK. This will require some internal config option?

> 
>> somewhere in
>>    "virtio_net_get_features".
> 
> Is there something that guarantees this for virtio-net (without vhost)?

I'm not sure. Need to check.

> 
>> Probably, only for vhost enabled
>>    devices, i.e. just before "vhost_net_get_features".
> 
> vhost should probably expose this feature from the backend,
> should it not?

Yes. You're right. But there is an issue here. That is the most
confusing part. vhost is not able to expose any new feature that
wasn't already set in "host_features". That's why we need to
add this flag to "features" before calling "vhost_net_get_features".
This comes from the implementation of "vhost_get_features" in vhost.c.

The origin of this is that "host_features" used in a two ways:
1. as a set of features supported by internal device implementation.
   (without vhost)
2. as a set of features available for negotiation with vhost.

I want to enable vhost negotiation with in-order feature, but I can't
add the property flag to "host_features", because internal
implementation may not support it.

Thinking about how to solve this situation, maybe it's better to
make the "vhost-internal" backend with its own set of supported
features? In this case we'll be able to use "host_features" in
a second way (as a set of features available for negotiation) and
every backend (including "internal" one) will negotiate features
that it supports.

What do you think?

> 
>> What do you think ?
> 
> BTW there's also another option 3. queue buffers somewhere and mark them
> used in order.

I'll look at this option.

> 
>>>
>>>> Are you requesting to validate every single ring operation?
>>>>
>>>> Anyway,
>>>> Buggy/malicious device is able to crash guest in a variety of ways.
>>>> Device that flags support of the feature, but breaks this promise,
>>>> IMHO, is buggy or malicious. So, why we need to protect from these
>>>> devices only for this particular feature flag?
>>>>
>>>> If your HW is broken, you're replacing it with a better one.
>>>
>>>
>>> This isn't what I was trying to say but in any case I'd like to point
>>> out that generally we would prefer guests to be able to validate device
>>> input at least optionally. It's useful for use-cases such as SEV.
>>>
>>> Besides, a guest crashing in the field isn't necessarily user or
>>> developer friendly.
>>>
>>> However it's a separate discussion about guest behaviours that
>>> would refer to a guest not host change.
>>>
>>>> Do you have an example where both (device and driver) are compliant
>>>> to virtio spec and something goes wrong?
>>>
>>> My point was that with your patch qemu isn't compliant.
>>>
>>>>> or limit to devices which use buffers in order.
>>>> Do you have a full list?
>>>>
>>>> Negotiation works well with current patch applied.
>>>
>>> I think that's only true because guests ignore the in-order
>>> flag even if negotiated. Once guests start relying on the
>>> in-order flag.
>>>
>>>> If device doesn't
>>>> support feature, the driver is not able to negotiate it. If device
>>>> supports it, the driver is able to use this feature.
>>>> So, what is the point?
>>>
>>> The point is that the feature implies a new promise about
>>> device behaviour. You set the flag but don't change qemu
>>> so it does not fulfill the promise.
>>>
>>>> The feature flag VIRTIO_F_IN_ORDER is a common flag for all the
>>>> devices. If not, maybe you need to fix the virtio spec?
>>>
>>> Any device can make the promise. The point is that current qemu
>>> code doesn't fulfill it for all device.
>>>
>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>>  hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
>>>>>>>>>> diff --git a/include/standard-headers/linux/virtio_config.h 
>>>>>>>>>> b/include/standard-headers/linux/virtio_config.h
>>>>>>>>>> index b777069..d20398c 100644
>>>>>>>>>> --- a/include/standard-headers/linux/virtio_config.h
>>>>>>>>>> +++ b/include/standard-headers/linux/virtio_config.h
>>>>>>>>>> @@ -71,4 +71,11 @@
>>>>>>>>>>   * this is for compatibility with legacy systems.
>>>>>>>>>>   */
>>>>>>>>>>  #define VIRTIO_F_IOMMU_PLATFORM             33
>>>>>>>>>> +
>>>>>>>>>> +/*
>>>>>>>>>> + * Inorder feature indicates that all buffers are used by the device
>>>>>>>>>> + * in the same order in which they have been made available.
>>>>>>>>>> + */
>>>>>>>>>> +#define VIRTIO_F_IN_ORDER 35
>>>>>>>>>> +
>>>>>>>>>>  #endif /* _LINUX_VIRTIO_CONFIG_H */
>>>>>>>>>> -- 
>>>>>>>>>> 2.7.4