linux-kernel - Re: [RFC v3 0/5] virtio: support packed ring

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 27 Apr 2018 07:18:03 +0300
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Tiwei Bie <tiwei.bie@...el.com>,
        virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        wexu@...hat.com, jfreimann@...hat.com
Subject: Re: [RFC v3 0/5] virtio: support packed ring

On Fri, Apr 27, 2018 at 11:56:05AM +0800, Jason Wang wrote:
> 
> 
> On 2018年04月25日 13:15, Tiwei Bie wrote:
> > Hello everyone,
> > 
> > This RFC implements packed ring support in virtio driver.
> > 
> > Some simple functional tests have been done with Jason's
> > packed ring implementation in vhost:
> > 
> > https://lkml.org/lkml/2018/4/23/12
> > 
> > Both of ping and netperf worked as expected (with EVENT_IDX
> > disabled). But there are below known issues:
> > 
> > 1. Reloading the guest driver will break the Tx/Rx;
> 
> Will have a look at this issue.
> 
> > 2. Zeroing the flags when detaching a used desc will
> >     break the guest -> host path.
> 
> I still think zeroing flags is unnecessary or even a bug. At host, I track
> last observed avail wrap counter and detect avail like (what is suggested in
> the example code in the spec):
> 
> static bool desc_is_avail(struct vhost_virtqueue *vq, __virtio16 flags)
> {
>        bool avail = flags & cpu_to_vhost16(vq, DESC_AVAIL);
> 
>        return avail == vq->avail_wrap_counter;
> }
> 
> So zeroing wrap can not work with this obviously.
> 
> Thanks

I agree. I think what one should do is flip the available bit.

> > 
> > Some simple functional tests have also been done with
> > Wei's packed ring implementation in QEMU:
> > 
> > http://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg00342.html
> > 
> > Both of ping and netperf worked as expected (with EVENT_IDX
> > disabled). Reloading the guest driver also worked as expected.
> > 
> > TODO:
> > - Refinements (for code and commit log) and bug fixes;
> > - Discuss/fix/test EVENT_IDX support;
> > - Test devices other than net;
> > 
> > RFC v2 -> RFC v3:
> > - Split into small patches (Jason);
> > - Add helper virtqueue_use_indirect() (Jason);
> > - Just set id for the last descriptor of a list (Jason);
> > - Calculate the prev in virtqueue_add_packed() (Jason);
> > - Fix/improve desc suppression code (Jason/MST);
> > - Refine the code layout for XXX_split/packed and wrappers (MST);
> > - Fix the comments and API in uapi (MST);
> > - Remove the BUG_ON() for indirect (Jason);
> > - Some other refinements and bug fixes;
> > 
> > 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!
> > 
> > Tiwei Bie (5):
> >    virtio: add packed ring definitions
> >    virtio_ring: support creating packed ring
> >    virtio_ring: add packed ring support
> >    virtio_ring: add event idx support in packed ring
> >    virtio_ring: enable packed ring
> > 
> >   drivers/virtio/virtio_ring.c       | 1271 ++++++++++++++++++++++++++++--------
> >   include/linux/virtio_ring.h        |    8 +-
> >   include/uapi/linux/virtio_config.h |   12 +-
> >   include/uapi/linux/virtio_ring.h   |   36 +
> >   4 files changed, 1049 insertions(+), 278 deletions(-)
> >