netdev - Re: [PATCH vhost v4 03/10] virtio_ring: packed: structure the indirect desc table

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]

Message-ID: <CACGkMEtG-zY2kmzV1hzRoKWz21mQa1QuopbEeRNa2EbYw2cNgg@mail.gmail.com>
Date: Mon, 25 Mar 2024 15:07:21 +0800
From: Jason Wang <jasowang@...hat.com>
To: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Cc: virtualization@...ts.linux.dev, "Michael S. Tsirkin" <mst@...hat.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Subject: Re: [PATCH vhost v4 03/10] virtio_ring: packed: structure the
 indirect desc table

On Fri, Mar 22, 2024 at 3:58 PM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
>
> On Fri, 22 Mar 2024 13:15:10 +0800, Jason Wang <jasowang@...hat.com> wrote:
> > On Thu, Mar 21, 2024 at 4:29 PM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
> > >
> > > On Thu, 21 Mar 2024 12:47:18 +0800, Jason Wang <jasowang@...hat.com> wrote:
> > > > On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
> > > > >
> > > > > This commit structure the indirect desc table.
> > > > > Then we can get the desc num directly when doing unmap.
> > > > >
> > > > > And save the dma info to the struct, then the indirect
> > > > > will not use the dma fields of the desc_extra. The subsequent
> > > > > commits will make the dma fields are optional. But for
> > > > > the indirect case, we must record the dma info.
> > > > >
> > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
> > > > > ---
> > > > >  drivers/virtio/virtio_ring.c | 66 +++++++++++++++++++++---------------
> > > > >  1 file changed, 38 insertions(+), 28 deletions(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > index 0dfbd17e5a87..22a588bba166 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -72,9 +72,16 @@ struct vring_desc_state_split {
> > > > >         struct vring_desc *indir_desc;  /* Indirect descriptor, if any. */
> > > > >  };
> > > > >
> > > > > +struct vring_packed_desc_indir {
> > > > > +       dma_addr_t addr;                /* Descriptor Array DMA addr. */
> > > > > +       u32 len;                        /* Descriptor Array length. */
> > > > > +       u32 num;
> > > > > +       struct vring_packed_desc desc[];
> > > > > +};
> > > > > +
> > > > >  struct vring_desc_state_packed {
> > > > >         void *data;                     /* Data for callback. */
> > > > > -       struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */
> > > > > +       struct vring_packed_desc_indir *indir_desc; /* Indirect descriptor, if any. */
> > > >
> > > > Maybe it's better just to have a vring_desc_extra here.
> > >
> > >
> > > Do you mean replacing vring_packed_desc_indir by vring_desc_extra?
> >
> > Just add a vring_desc_extra in vring_desc_state_packed.
> >
> > >
> > > I am ok for that. But vring_desc_extra has two extra items:
> > >
> > >         u16 flags;                      /* Descriptor flags. */
> > >         u16 next;                       /* The next desc state in a list. */
> > >
> > > vring_packed_desc_indir has "desc". I think that is more convenient.
> > >
> > > So, I think vring_packed_desc_indir is appropriate.
> >
> > It reuses the existing structure so we had the chance to reuse the
> > helper.
>
> Do you mean vring_unmap_extra_packed()?

Yes.


>
> After last commit(virtio_ring: packed: remove double check of the unmap ops):
>
>         /* caller must check vring_need_unmap_buffer() */
>         static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
>                                              const struct vring_desc_extra *extra)
>         {
>                 u16 flags;
>
>                 flags = extra->flags;
>
>                 dma_unmap_page(vring_dma_dev(vq),
>                                extra->addr, extra->len,
>                                (flags & VRING_DESC_F_WRITE) ?
>                                DMA_FROM_DEVICE : DMA_TO_DEVICE);
>         }
>
> But we should call dma_unmap_single() for indirect desc.
>
> We know, dma_unmap_single() and dma_unmap_page() are same in essence.

Yes, it's worth tweaking in the future.

> So if we call dma_unmap_page for the indirect desc, we can reuse
> this function. But I do not prefer doing this.

Ok.

Thanks

>
> Thanks.
>
>
> > And it could be used for future chained indirect (if it turns
> > out to be necessary).
> >
> > Thanks
> >
> > > Or I missed something.
> > >
> > >
> > > Thanks.
> > >
> > >
> > > >
> > > > Thanks
> > > >
> > >
> >
> >
>