Received-SPF: pass (google.com: domain of linux-kernel+bounces-32314-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <1704919215-91319-1-git-send-email-steven.sistare@oracle.com>
 <1704919215-91319-6-git-send-email-steven.sistare@oracle.com>
 <CACGkMEsCzLNdRXu25tzhrnTVatM6pHwVXVUn7kE=3erVWQUL4g@mail.gmail.com> <700fc2e5-445d-46c4-bdde-0dd29bcdb2bc@oracle.com>
In-Reply-To: <700fc2e5-445d-46c4-bdde-0dd29bcdb2bc@oracle.com>
From: Jason Wang <jasowang@redhat.com>
Date: Mon, 22 Jan 2024 12:05:28 +0800
Message-ID: <CACGkMEsVNNJfUE7JVX6mYdrZ+uWxZYbdwJVOrxXGiK++WwA4Sg@mail.gmail.com>
Subject: Re: [RFC V1 05/13] vhost-vdpa: VHOST_IOTLB_REMAP
To: Steven Sistare <steven.sistare@oracle.com>
Cc: virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, 
	"Michael S. Tsirkin" <mst@redhat.com>, Si-Wei Liu <si-wei.liu@oracle.com>, 
	Eugenio Perez Martin <eperezma@redhat.com>, Xuan Zhuo <xuanzhuo@linux.alibaba.com>, 
	Dragos Tatulea <dtatulea@nvidia.com>, Eli Cohen <elic@nvidia.com>, 
	Xie Yongji <xieyongji@bytedance.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jan 18, 2024 at 4:32=E2=80=AFAM Steven Sistare
<steven.sistare@oracle.com> wrote:
>
> On 1/10/2024 10:08 PM, Jason Wang wrote:
> > On Thu, Jan 11, 2024 at 4:40=E2=80=AFAM Steve Sistare <steven.sistare@o=
racle.com> wrote:
> >>
> >> When device ownership is passed to a new process via VHOST_NEW_OWNER,
> >> some devices need to know the new userland addresses of the dma mappin=
gs.
> >> Define the new iotlb message type VHOST_IOTLB_REMAP to update the uadd=
r
> >> of a mapping.  The new uaddr must address the same memory object as
> >> originally mapped.
> >>
> >> The user must suspend the device before the old address is invalidated=
,
> >> and cannot resume it until after VHOST_IOTLB_REMAP is called, but this
> >> requirement is not enforced by the API.
> >>
> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >> ---
> >>  drivers/vhost/vdpa.c             | 34 +++++++++++++++++++++++++++++++=
+
> >>  include/uapi/linux/vhost_types.h | 11 ++++++++++-
> >>  2 files changed, 44 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> >> index faed6471934a..ec5ca20bd47d 100644
> >> --- a/drivers/vhost/vdpa.c
> >> +++ b/drivers/vhost/vdpa.c
> >> @@ -1219,6 +1219,37 @@ static int vhost_vdpa_pa_map(struct vhost_vdpa =
*v,
> >>
> >>  }
> >>
> >> +static int vhost_vdpa_process_iotlb_remap(struct vhost_vdpa *v,
> >> +                                         struct vhost_iotlb *iotlb,
> >> +                                         struct vhost_iotlb_msg *msg)
> >> +{
> >> +       struct vdpa_device *vdpa =3D v->vdpa;
> >> +       const struct vdpa_config_ops *ops =3D vdpa->config;
> >> +       u32 asid =3D iotlb_to_asid(iotlb);
> >> +       u64 start =3D msg->iova;
> >> +       u64 last =3D start + msg->size - 1;
> >> +       struct vhost_iotlb_map *map;
> >> +       int r =3D 0;
> >> +
> >> +       if (msg->perm || !msg->size)
> >> +               return -EINVAL;
> >> +
> >> +       map =3D vhost_iotlb_itree_first(iotlb, start, last);
> >> +       if (!map)
> >> +               return -ENOENT;
> >> +
> >> +       if (map->start !=3D start || map->last !=3D last)
> >> +               return -EINVAL;
> >> +
> >> +       /* batch will finish with remap.  non-batch must do it now. */
> >> +       if (!v->in_batch)
> >> +               r =3D ops->set_map(vdpa, asid, iotlb);
> >> +       if (!r)
> >> +               map->addr =3D msg->uaddr;
> >
> > I may miss something, for example for PA mapping,
> >
> > 1) need to convert uaddr into phys addr
> > 2) need to check whether the uaddr is backed by the same page or not?
>
> This code does not verify that the new size@uaddr points to the same phys=
ical
> pages as the old size@uaddr.  If the app screws up and they differ, then =
the app
> may corrupt its own memory, but no-one else's.
>
> It would be expensive for large memories to verify page by page, O(npages=
), and such
> verification lies on the critical path for virtual machine downtime durin=
g live update.
> I could compare the properties of the vma(s) for the old size@uaddr vs th=
e vma for the
> new, but that is more complicated and would be a maintenance headache.  W=
hen I submitted
> such code to Alex W when writing the equivalent patches for vfio, he said=
 don't check,
> correctness is the user's responsibility.

Ok, let's document this somewhere.

Thanks

>
> - Steve
>
> >> +
> >> +       return r;
> >> +}
> >> +
> >>  static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v,
> >>                                            struct vhost_iotlb *iotlb,
> >>                                            struct vhost_iotlb_msg *msg=
)
> >> @@ -1298,6 +1329,9 @@ static int vhost_vdpa_process_iotlb_msg(struct v=
host_dev *dev, u32 asid,
> >>                         ops->set_map(vdpa, asid, iotlb);
> >>                 v->in_batch =3D false;
> >>                 break;
> >> +       case VHOST_IOTLB_REMAP:
> >> +               r =3D vhost_vdpa_process_iotlb_remap(v, iotlb, msg);
> >> +               break;
> >>         default:
> >>                 r =3D -EINVAL;
> >>                 break;
> >> diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vho=
st_types.h
> >> index 9177843951e9..35908315ff55 100644
> >> --- a/include/uapi/linux/vhost_types.h
> >> +++ b/include/uapi/linux/vhost_types.h
> >> @@ -79,7 +79,7 @@ struct vhost_iotlb_msg {
> >>  /*
> >>   * VHOST_IOTLB_BATCH_BEGIN and VHOST_IOTLB_BATCH_END allow modifying
> >>   * multiple mappings in one go: beginning with
> >> - * VHOST_IOTLB_BATCH_BEGIN, followed by any number of
> >> + * VHOST_IOTLB_BATCH_BEGIN, followed by any number of VHOST_IOTLB_REM=
AP or
> >>   * VHOST_IOTLB_UPDATE messages, and ending with VHOST_IOTLB_BATCH_END=
.
> >>   * When one of these two values is used as the message type, the rest
> >>   * of the fields in the message are ignored. There's no guarantee tha=
t
> >> @@ -87,6 +87,15 @@ struct vhost_iotlb_msg {
> >>   */
> >>  #define VHOST_IOTLB_BATCH_BEGIN    5
> >>  #define VHOST_IOTLB_BATCH_END      6
> >> +
> >> +/*
> >> + * VHOST_IOTLB_REMAP registers a new uaddr for the existing mapping a=
t iova.
> >> + * The new uaddr must address the same memory object as originally ma=
pped.
> >> + * Failure to do so will result in user memory corruption and/or devi=
ce
> >> + * misbehavior.  iova and size must match the arguments used to creat=
e the
> >> + * an existing mapping.  Protection is not changed, and perm must be =
0.
> >> + */
> >> +#define VHOST_IOTLB_REMAP          7
> >>         __u8 type;
> >>  };
> >>
> >> --
> >> 2.39.3
> >>
> >
>