| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <665c828e-6699-7688-cfea-b23b2b0f5fe3@redhat.com>
Date: Tue, 17 Apr 2018 10:24:32 +0800
From: Jason Wang <jasowang@...hat.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: Tiwei Bie <tiwei.bie@...el.com>, wexu@...hat.com,
virtualization@...ts.linux-foundation.org,
linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
jfreimann@...hat.com
Subject: Re: [RFC v2] virtio: support packed ring
On 2018年04月17日 10:17, Michael S. Tsirkin wrote:
> On Tue, Apr 17, 2018 at 10:11:58AM +0800, Jason Wang wrote:
>>
>> On 2018年04月13日 15:15, Tiwei Bie wrote:
>>> On Fri, Apr 13, 2018 at 12:30:24PM +0800, Jason Wang wrote:
>>>> On 2018年04月01日 22:12, Tiwei Bie wrote:
>>>>> Hello everyone,
>>>>>
>>>>> This RFC implements packed ring support for virtio driver.
>>>>>
>>>>> The code was tested with DPDK vhost (testpmd/vhost-PMD) implemented
>>>>> by Jens at http://dpdk.org/ml/archives/dev/2018-January/089417.html
>>>>> Minor changes are needed for the vhost code, e.g. to kick the guest.
>>>>>
>>>>> TODO:
>>>>> - Refinements and bug fixes;
>>>>> - Split into small patches;
>>>>> - Test indirect descriptor support;
>>>>> - Test/fix event suppression support;
>>>>> - Test devices other than net;
>>>>>
>>>>> RFC v1 -> RFC v2:
>>>>> - Add indirect descriptor support - compile test only;
>>>>> - Add event suppression supprt - compile test only;
>>>>> - Move vring_packed_init() out of uapi (Jason, MST);
>>>>> - Merge two loops into one in virtqueue_add_packed() (Jason);
>>>>> - Split vring_unmap_one() for packed ring and split ring (Jason);
>>>>> - Avoid using '%' operator (Jason);
>>>>> - Rename free_head -> next_avail_idx (Jason);
>>>>> - Add comments for virtio_wmb() in virtqueue_add_packed() (Jason);
>>>>> - Some other refinements and bug fixes;
>>>>>
>>>>> Thanks!
>>>>>
>>>>> Signed-off-by: Tiwei Bie <tiwei.bie@...el.com>
>>>>> ---
>>>>> drivers/virtio/virtio_ring.c | 1094 +++++++++++++++++++++++++++++-------
>>>>> include/linux/virtio_ring.h | 8 +-
>>>>> include/uapi/linux/virtio_config.h | 12 +-
>>>>> include/uapi/linux/virtio_ring.h | 61 ++
>>>>> 4 files changed, 980 insertions(+), 195 deletions(-)
>>> [...]
[...]
>>>> It looks to me we should examine RING_EVENT_FLAGS_DESC in desc_event_flags
>>>> instead of vq->event here. Spec does not forces to use evenf_off and
>>>> event_wrap if event index is enabled.
>>>>
>>>>> + // FIXME: fix this!
>>>>> + needs_kick = ((off_wrap >> 15) == vq->wrap_counter) &&
>>>>> + vring_need_event(off_wrap & ~(1<<15), new, old);
>>>> Why need a & here?
>>> Because wrap_counter (the most significant bit in off_wrap)
>>> isn't part of the index.
>>>
>>>>> + } else {
>>>> Need a smp_rmb() to make sure desc_event_flags was checked before flags.
>>> I don't get your point, if my understanding is correct,
>>> desc_event_flags is vq->vring_packed.device->flags. So
>>> what's the "flags"?
>> Sorry, I mean we need check device.flags before off_warp. So it needs an
>> smp_rmb() in the middle.
> It's best to just read all flags atomically as u32.
Yes it is.
>
>> It looks to me there's no guarantee that
>> VRING_EVENT_F_DESC is set if event index is supported.
>>
>>>>> + needs_kick = (vq->vring_packed.device->flags !=
>>>>> + cpu_to_virtio16(_vq->vdev, VRING_EVENT_F_DISABLE));
>>>>> + }
>>>>> + END_USE(vq);
>>>>> + return needs_kick;
>>>>> +}
>>> [...]
>>>>> +static int detach_buf_packed(struct vring_virtqueue *vq, unsigned int head,
>>>>> + void **ctx)
>>>>> +{
>>>>> + struct vring_packed_desc *desc;
>>>>> + unsigned int i, j;
>>>>> +
>>>>> + /* Clear data ptr. */
>>>>> + vq->desc_state[head].data = NULL;
>>>>> +
>>>>> + i = head;
>>>>> +
>>>>> + for (j = 0; j < vq->desc_state[head].num; j++) {
>>>>> + desc = &vq->vring_packed.desc[i];
>>>>> + vring_unmap_one_packed(vq, desc);
>>>>> + desc->flags = 0x0;
>>>> Looks like this is unnecessary.
>>> It's safer to zero it. If we don't zero it, after we
>>> call virtqueue_detach_unused_buf_packed() which calls
>>> this function, the desc is still available to the
>>> device.
>> Well detach_unused_buf_packed() should be called after device is stopped,
>> otherwise even if you try to clear, there will still be a window that device
>> may use it.
>>
>>>>> + i++;
>>>>> + if (i >= vq->vring_packed.num)
>>>>> + i = 0;
>>>>> + }
>>> [...]
>>>>> +static unsigned virtqueue_enable_cb_prepare_packed(struct virtqueue *_vq)
>>>>> +{
>>>>> + struct vring_virtqueue *vq = to_vvq(_vq);
>>>>> + u16 last_used_idx, wrap_counter, off_wrap;
>>>>> +
>>>>> + START_USE(vq);
>>>>> +
>>>>> + last_used_idx = vq->last_used_idx;
>>>>> + wrap_counter = vq->wrap_counter;
>>>>> +
>>>>> + if (last_used_idx > vq->next_avail_idx)
>>>>> + wrap_counter ^= 1;
>>>>> +
>>>>> + off_wrap = last_used_idx | (wrap_counter << 15);
>>>>> +
>>>>> + /* We optimistically turn back on interrupts, then check if there was
>>>>> + * more to do. */
>>>>> + /* Depending on the VIRTIO_RING_F_EVENT_IDX feature, we need to
>>>>> + * either clear the flags bit or point the event index at the next
>>>>> + * entry. Always do both to keep code simple. */
>>>>> + if (vq->event_flags_shadow == VRING_EVENT_F_DISABLE) {
>>>>> + vq->event_flags_shadow = vq->event ? VRING_EVENT_F_DESC:
>>>>> + VRING_EVENT_F_ENABLE;
>>>>> + vq->vring_packed.driver->flags = cpu_to_virtio16(_vq->vdev,
>>>>> + vq->event_flags_shadow);
>>>>> + }
>>>> A smp_wmb() is missed here?
>>>>
>>>>> + vq->vring_packed.driver->off_wrap = cpu_to_virtio16(_vq->vdev, off_wrap);
>>>> And according to the spec, it looks to me write a VRING_EVENT_F_ENABLE is
>>>> sufficient here.
>>> I didn't think much when implementing the event suppression
>>> for packed ring previously. After I saw your comments, I found
>>> something new. Indeed, unlike the split ring, for the packed
>>> ring, spec doesn't say we must use VRING_EVENT_F_DESC when
>>> EVENT_IDX is negotiated. So do you think below thought is
>>> right or makes sense?
>>>
>>> - For virtqueue_enable_cb_prepare(), we just need to enable
>>> the ring by setting flags to VRING_EVENT_F_ENABLE in any
>>> case.
>>>
>>> - We will try to use VRING_EVENT_F_DESC (if EVENT_IDX is
>>> negotiated) only when we want to delay the interrupts
>>> virtqueue_enable_cb_delayed().
>> This looks good to me.
> I suspect this will lead to extra interrupts if host is fast.
> So I think for now we should always use VRING_EVENT_F_DESC
> if EVENT_IDX is negotiated.
Right, so if this is true, maybe we'd better force this in the spec?
Thanks
>
> VRING_EVENT_F_DISABLE makes more sense to me.
>
[...]
Powered by blists - more mailing lists