On Fri, 22 Mar 2024 13:15:10 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> On Thu, Mar 21, 2024 at 4:29 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, 21 Mar 2024 12:47:18 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
> > > > ---
> > > > 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 surprise to here that.
Do you mean this: #1
struct vring_desc_state_packed {
void *data; /* Data for callback. */
struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */
u16 num; /* Descriptor list length. */
u16 last; /* The last desc state in a list. */
struct vring_desc_extra desc_extra;
};
Then desc_extra is included by desc_state. I do not think so.
I guess you mean this: #2
struct vring_desc_state_packed {
void *data; /* Data for callback. */
struct vring_desc_extra *indir_desc; /* Indirect descriptor, if any. */
u16 num; /* Descriptor list length. */
u16 last; /* The last desc state in a list. */
};
indir_desc pointers to memory:
|struct vring_desc_extra | struct vring_packed_desc desc[] |
>
> >
> > 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.
Which helper?
But, if you mean #2. I am ok.
Thanks.
> And it could be used for future chained indirect (if it turns
> out to be necessary).
>
> Thanks
>
> > Or I missed something.
> >
> >
> > Thanks.
> >
> >
> > >
> > > Thanks
> > >
> >
>
>
