On 1/10/2024 10:08 PM, Jason Wang wrote:
> On Thu, Jan 11, 2024 at 4:40 AM Steve Sistare <steven.sistare@xxxxxxxxxx> 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 mappings.
>> Define the new iotlb message type VHOST_IOTLB_REMAP to update the uaddr
>> 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@xxxxxxxxxx>
>> ---
>> 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 = v->vdpa;
>> + const struct vdpa_config_ops *ops = vdpa->config;
>> + u32 asid = iotlb_to_asid(iotlb);
>> + u64 start = msg->iova;
>> + u64 last = start + msg->size - 1;
>> + struct vhost_iotlb_map *map;
>> + int r = 0;
>> +
>> + if (msg->perm || !msg->size)
>> + return -EINVAL;
>> +
>> + map = vhost_iotlb_itree_first(iotlb, start, last);
>> + if (!map)
>> + return -ENOENT;
>> +
>> + if (map->start != start || map->last != last)
>> + return -EINVAL;
>> +
>> + /* batch will finish with remap. non-batch must do it now. */
>> + if (!v->in_batch)
>> + r = ops->set_map(vdpa, asid, iotlb);
>> + if (!r)
>> + map->addr = 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 physical
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 during live update.
I could compare the properties of the vma(s) for the old size@uaddr vs the vma for the
new, but that is more complicated and would be a maintenance headache. When 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.
- 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 vhost_dev *dev, u32 asid,
>> ops->set_map(vdpa, asid, iotlb);
>> v->in_batch = false;
>> break;
>> + case VHOST_IOTLB_REMAP:
>> + r = vhost_vdpa_process_iotlb_remap(v, iotlb, msg);
>> + break;
>> default:
>> r = -EINVAL;
>> break;
>> diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_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_REMAP 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 that
>> @@ -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 at iova.
>> + * The new uaddr must address the same memory object as originally mapped.
>> + * Failure to do so will result in user memory corruption and/or device
>> + * misbehavior. iova and size must match the arguments used to create the
>> + * an existing mapping. Protection is not changed, and perm must be 0.
>> + */
>> +#define VHOST_IOTLB_REMAP 7
>> __u8 type;
>> };
>>
>> --
>> 2.39.3
>>
>
