Received-SPF: pass (google.com: domain of linux-kernel+bounces-29423-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <700fc2e5-445d-46c4-bdde-0dd29bcdb2bc@oracle.com>
Date: Wed, 17 Jan 2024 15:31:27 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC V1 05/13] vhost-vdpa: VHOST_IOTLB_REMAP
Content-Language: en-US
To: Jason Wang <jasowang@redhat.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>
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>
From: Steven Sistare <steven.sistare@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <CACGkMEsCzLNdRXu25tzhrnTVatM6pHwVXVUn7kE=3erVWQUL4g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?SW9nWjB1RHhneWNoWlRpc1ZheHYxSzJsT2FxTmtia0dvRVFPbU9jUnlESnZB?=
 =?utf-8?B?cUs2TXI3NGRvd09SajYyM2txV1ZldzNKY1dvSDF6RW00eTZ2ZjJiTEt2V21I?=
 =?utf-8?B?a0dzeHp3OUVkbHAreWlQNUo3OCtpajh2SE5GWnRyNkFZeTdGVHVXR1g0Z2I4?=
 =?utf-8?B?emw3dE1PNFhZYlJXSjFUQzZnaDZzaitZUENiUFlIS2Rnam5xaGc0ZmVIak9z?=
 =?utf-8?B?KzZGOFl1d1ZoNWYzdjBwZWhaQlVvaTN4d25wNHNUT1pZVUxXU0xpdEI2NDlp?=
 =?utf-8?B?OStES1l0MVQ4eDZUYlJkekpyUGgwenVDa1AwKzBXVjE0L0ZxbTFDWnV0OTA4?=
 =?utf-8?B?bDUxN05Gb0dEbklUK1dGTFlBdjJBTkwzbldwditNdXI3UW95Y2pRSHZDL3p0?=
 =?utf-8?B?MEpVZGs2dEJNdlNhZGVPUlFFWDdONVRkNEtLNitjTnJuT1hHT1ptbkxNRFJj?=
 =?utf-8?B?NFJRTGVGU1FJSGhCNnVKYkp2Misvbk8wK3ZpY2NXRDgvS0hRWUpyeC83ZU9r?=
 =?utf-8?B?eGdlUDZYRmhMT3lpMVRZaDVCK0hvNi9nbEtuQzI2SHZVSjRIN3ZDb042Zyty?=
 =?utf-8?B?bjJ2clo0NTJURjJqWlQwWUZqd3FhaFlLYWlrN3FKYVVJTmEzVUN1YkJUbzVD?=
 =?utf-8?B?Y3pudkJWTGprcU5KYUNQaG55ZnhzUFFCVmYvcnlEQVRkNkcrRkdsaW5wQXV2?=
 =?utf-8?B?eVdzcS9rbENiem1hT1pPTklxcy9Va3p0R1BOaXZXZlp6aWd5OUR4NUdFa1lq?=
 =?utf-8?B?a2FRaWh3UFdkb0syOFZrajIyMGhDSFRLOHhwYzVPcGdzUi9id2g1TG9qN3BJ?=
 =?utf-8?B?ajIzVFZ5cjkxTGpnWHlvWUhCMm5FMFArME9uTWd0K3oxbm96c1hOWHlvZ0VB?=
 =?utf-8?B?V3pIQmRRQzZDS1VHWk03Yk9ONGN5cXMwMzh4RURHY09wUFpyNXdob3YvS0h5?=
 =?utf-8?B?aFEvTGJKRVZrNmVhRXhya1JZbGxzUkYvZzBKYmFGb0MrNXl1N3l1eC9CVWVy?=
 =?utf-8?B?Ty9EY0pHMjAxeW9KeWV6T2VCcXlPM3RnZXNVRVhEZm4rYk5CVTdURWlqUHMv?=
 =?utf-8?B?NGthTHVyTDl2NXF0NEhoMyt1azkvYkUzeE14eGhlVkxJd3dZSjB2RVhlNlJi?=
 =?utf-8?B?Tk1XOGRVQzRUUDhiUkNpa0EwbklyZ3lkQnp5RGF6QTBmSVE0SU9BeEFZcE1F?=
 =?utf-8?B?V3kwWUYwcmNxdHlydTJHUmVrTUJDZVlvUlhyZlROZTd3UzJkU0lBdDdBaWZZ?=
 =?utf-8?B?NzVkZW5TZSs2bHNwLzRlakZIbFl2RVZqYlpVbG9rNmNZTzB6emlBY0dBK0s5?=
 =?utf-8?B?Y2I4Z3UzR1dBZEtRQ2Zzb2dBY01kNmJaZldaZ1orVVNtbTl2SS9vZ2o3VHZj?=
 =?utf-8?B?MXlkYk1CZjM1SERZRFhDdDEzckdRYzJYbGpWQnZ1TFNEdzlXYWpRcGJ3Nmtk?=
 =?utf-8?B?TkZUc3pMa3RrM3Z3ZkpOM1ZvV3IwZE1vR2JTbUdLSGp6cnpVeldkcFRCSE4v?=
 =?utf-8?B?d1RTcjBZQkM2ajk5STh5VGpYV2FMeHd3bzYrK3FVM20rdzJTQmNUSHczbEVq?=
 =?utf-8?B?YlJtTkZYUzhsdE9OSzZTbklEOVQ0KzFsNzJTMVlPaSs1T0owR2N3cThaS2lM?=
 =?utf-8?B?djZZZzYxQld0ekxpNnNLWUpUWEtiaHVjT2o0bmh3OC9NZm5xQWFFYzV5Tkdu?=
 =?utf-8?B?SWQ0WVBtSWdTQ1pQYllLMVJKdnBiVFJLYk9JYjN6bDBoQ1M4ZnFKSTdiTkxK?=
 =?utf-8?B?bllROG5MTzZEOTc1ZTBUcVl3LzRDYzhEWlF1UldsdGNNamY0Q0FnT1k4NmFa?=
 =?utf-8?B?SUxOUmh6cmZiT2JJZTNZcjJTRDBid3dzc254WTRmOWFkaG10WXdoL1g5TW40?=
 =?utf-8?B?cnVBKzJxUnBCVG5yd2FnNlByMzEyam5WaURKSHJDS2pFVjBhYS9uUmYvZHJa?=
 =?utf-8?B?c3liNG9hMnd5Znl6bXdQeUw4R2VLZHB5WUpPUlA3ZjFaOE9mcHlubmlRcHJZ?=
 =?utf-8?B?WStzcXJxSGc4bGtEUEZCN1VKSEdBMEt3TlZDKzFHZ0tRV2JleUlablNQYkJG?=
 =?utf-8?B?ZHV3MFFyeWpjMzhyemUvVldjQnNjSU5TWGFuKzBhMlo2Vk1DdTBpZS9Qcktk?=
 =?utf-8?B?cE5xdEpocXMwQUxSa3UwNklXUUxxSXVRWXQrS0x5aStMclJzNE5za2szVm1V?=
 =?utf-8?B?alE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	n4pEOXEifLiLzyAQV3DAFNDhr4wd3pE0FXkPrWsfzLqGfvKwsaKlZMFxQSyg7DkpBojNZ7QeKBt0Mszn0TMOgca1YvjiXeYpvDYPw4X2V9UX8bGeUmpO2B2TPrw2wtabvK6eFY6KwrrKgpdnQGIdfMWMaXY7lwO6lJ7KIHMwTLwbmZptPtPUzfsnPLU/np9xn6LlHUw8qK2t8y11zOwpa+XTndGobR87EgZfluVx9kEMnFBm7lhJr6n1w0H7IhNxoM4lt50vRS/koAkqJanEbNp5gyTE3264a5TD6rqhnkp0EgjT6ASTAtMjQvXYhHE4A5Utf3pWtcDQb13g+VOH79qzTFr5SYUElfKiq76ATGvdJ7YmbvhNkgopkeS20EuSm7uRof5d9SEwHyu1FRLWRjM8h/PGmykJBM06HrBiHKnzKTyXPtKa17O593zrHSR85FdEbgdKnhbLnZ8A6ytQF5NpFX4rq+2/trSocFtCIaJ2YmbgvBxHwEQWXMJeCC/L6neucD8xmBrJsbLmpXXY4vs9bhzhhnXPSo/t6YyplsKErQQSJBlPsFBz4a4VtVJy97a1FkTDc3uRt3+yTxwqAL3tq6RhwbG+ZyNG2vJrn+A=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b9460590-e483-4a3f-063f-08dc179b498c
X-MS-Exchange-CrossTenant-AuthSource: SA2PR10MB4684.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jan 2024 20:31:30.2152
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: kd/CABBRf0fR9ah4DeEkF9BMnLPbAUn8RpUracBOI5PEogGDpnmhRyrS9nUtx3r/aOOlm1iAJy5n1hLvz0S0eUhuVn7RPaCOnRBG3Kgsnvs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR10MB5942
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-01-17_12,2024-01-17_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 phishscore=0
 malwarescore=0 adultscore=0 mlxscore=0 bulkscore=0 mlxlogscore=999
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2401170150
X-Proofpoint-GUID: bEEYHprD-IlZaPn3vq3boXnEvlyONS5F
X-Proofpoint-ORIG-GUID: bEEYHprD-IlZaPn3vq3boXnEvlyONS5F

On 1/10/2024 10:08 PM, Jason Wang wrote:
> On Thu, Jan 11, 2024 at 4:40 AM Steve Sistare <steven.sistare@oracle.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 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@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 = 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
>>
>