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

[Michael S. Tsirkin]

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.

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

BTW you also need to disable for compat machine types.

> somewhere in
>    "virtio_net_get_features".

Is there something that guarantees this for virtio-net (without vhost)?

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

> What do you think ?

BTW there's also another option 3. queue buffers somewhere and mark them
used in order.

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