On Thu, Sep 18, 2025 at 12:35 AM Eugenio Perez Martin
<epere...@redhat.com> wrote:
>
> On Wed, Sep 17, 2025 at 10:46 AM Jason Wang <jasow...@redhat.com> wrote:
> >
> > On Tue, Sep 16, 2025 at 9:09 PM Eugenio Pérez <epere...@redhat.com> wrote:
> > >
> > > This is a first step so we can make more than one different address
> > > spaces. No change on the colde flow intended.
> > >
> > > Acked-by: Jason Wang <jasow...@redhat.com>
> >
> > Sorry, I think I found something new.
> >
> > > Signed-off-by: Eugenio Pérez <epere...@redhat.com>
> > > ---
> > > drivers/vdpa/vdpa_user/vduse_dev.c | 114 +++++++++++++++--------------
> > > 1 file changed, 59 insertions(+), 55 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > index 9c12ae72abc2..b45b1d22784f 100644
> > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > @@ -86,6 +86,12 @@ struct vduse_umem {
> > > struct mm_struct *mm;
> > > };
> > >
> > > +struct vduse_as {
> > > + struct vduse_iova_domain *domain;
> > > + struct vduse_umem *umem;
> > > + struct mutex mem_lock;
> > > +};
> > > +
> > > struct vduse_vq_group_int {
> > > struct vduse_dev *dev;
> >
> > I would expect this has an indirection for as. E.g unsigned int as;
> >
>
> This is a change from the previous commit and *dev is needed for
> taking the rwlock of the vq_group -> asid relation anyway, as function
> like vduse_dev_sync_single_for_cpu could run at the same time as
> VHOST_VDPA_SET_GROUP_ASID.
>
> I'm ok with adding the "unsigned int as" indirection but it involves
> memory walking that is not needed.
I think it should be fine unless we can notice it during the
benchmark. What's more we can refactor vduse as without caring much
about the group in the future.
>
> > > };
> > > @@ -94,7 +100,7 @@ struct vduse_dev {
> > > struct vduse_vdpa *vdev;
> > > struct device *dev;
> > > struct vduse_virtqueue **vqs;
> > > - struct vduse_iova_domain *domain;
> > > + struct vduse_as as;
> >
> > This needs to be an array per title:
> >
> > "vduse: create vduse_as to make it an array"
> >
>
> I meant "to make it an array in next patches". Would it work if I just
> change the patch subject to:
>
> "create vduse_as to contain per-as members"
>
> And then specify that it will be converted to an array later in the
> patch message? Or do you prefer me just to squash the patches?
I would prefer either:
1) make it an array in this patch
or
2) squash this patch into the next one
to avoid unnecessary changes.
>
> > > char *name;
> > > struct mutex lock;
> > > spinlock_t msg_lock;
> > > @@ -122,9 +128,7 @@ struct vduse_dev {
> > > u32 vq_num;
> > > u32 vq_align;
> > > u32 ngroups;
> > > - struct vduse_umem *umem;
> > > struct vduse_vq_group_int *groups;
> > > - struct mutex mem_lock;
> > > unsigned int bounce_size;
> > > rwlock_t domain_lock;
> > > };
> > > @@ -439,7 +443,7 @@ static __poll_t vduse_dev_poll(struct file *file,
> > > poll_table *wait)
> > > static void vduse_dev_reset(struct vduse_dev *dev)
> > > {
> > > int i;
> > > - struct vduse_iova_domain *domain = dev->domain;
> > > + struct vduse_iova_domain *domain = dev->as.domain;
> >
> > This should be an iteration of all address spaces?
> >
>
> Yes, it will be in next patches.
>
> The rest of the comments have the same reply actually, as this patch
> just prepares the struct to make it an array in patches as small as
> possible.
Right.
Thanks
