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 <i.maxim...@samsung.com>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>> 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
> >>>>>>>
> >>>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >
> >
