Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <d71dc008-9920-463d-948a-0375c5754810@oracle.com>
Date:   Tue, 24 Oct 2023 09:25:30 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 5/7] vhost-vdpa: clean iotlb map during reset for older
 userspace
Content-Language: en-US
From:   Si-Wei Liu <si-wei.liu@oracle.com>
To:     Jason Wang <jasowang@redhat.com>
Cc:     mst@redhat.com, eperezma@redhat.com, sgarzare@redhat.com,
        dtatulea@nvidia.com, virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org
References: <1697880319-4937-1-git-send-email-si-wei.liu@oracle.com>
 <1697880319-4937-6-git-send-email-si-wei.liu@oracle.com>
 <CACGkMEun7pnq5zMBH=_X+H9p_=HK6+5WHRp9A2prRe2Na-qmvQ@mail.gmail.com>
 <0ad99060-119b-4465-b611-868f693aea02@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <0ad99060-119b-4465-b611-868f693aea02@oracle.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SVkwS3pIdTBLNUVKMnI1VjhRWXZyZlhWL1RONS9HbGNXOWptSHoxYmppSnNL?=
 =?utf-8?B?YzBHQ2VBa1hOVE16d3BTVTlVQ3lXSUdoUFB4Z3Y4UzlMK2gzYXdVbWFaS2RP?=
 =?utf-8?B?a3RRTStpRElYTnR6VXZtQ0VXckNCL0l1V1JJNTFkZ0pWZHpUYzExMFFqLzlv?=
 =?utf-8?B?M1N1ZkdnZHhnb1dlU3ppNC8yRWprRkNwRFB3NWZtWUlZNyt2a0V3SGRvTTky?=
 =?utf-8?B?ekk4NnJxQ1E2dVBybWdnN0NCaCtQUjBPZm9lYkF4SzlVQWVoMlNnajh6cWV3?=
 =?utf-8?B?Yy9GMkgwMVB1ZTIvb2Z0dXhJOGl0UGFMY1ZSQ3gvUlBmT2NvVStNS3Brc2gr?=
 =?utf-8?B?ZGlCRWZvbTFCUkpZZjZpeFVVUTBCM25VdHRMdzZhY2gyc25rbkwyQTdlQnB4?=
 =?utf-8?B?TlMxRVlFeW55UlU0N050TXdJL3d6ZmhBZmVNYml2UEJJN1dsakZ4c2JmU253?=
 =?utf-8?B?Y3Q2c0dQaVdZc3k5TnpVZzRHd2F4bTZIcEVnWUJhMi93Y3h2Z2pzdWZHL3BR?=
 =?utf-8?B?WTB4Sk1ZOXZ5Qml0K1Z3NnROdjRpT2dPeUErUHJ5ZWpjZWxzSTgwU2o1cis5?=
 =?utf-8?B?a3pCbDMvbnlxeWJobjc0bERZTEZhUzdQU3BhTzNtUkZPbWh0K3E2TzhxMnp3?=
 =?utf-8?B?TGM0N1VsT2RJbFBMbHRLVkYrbVNhTExqYURJeTNoa2pFajF5Q0ltOERTRjl2?=
 =?utf-8?B?WlNhSlVyWWFLNWNldkR4RUNURDVCYkJmVXd2Y292ZzcyMmxvMms3amhyNXg1?=
 =?utf-8?B?a0VDMmRhbEZtRm9ITFg3ZVg1dFdZSGRUTWl4ODFWRVArZUlFY0xLeWc3OTYx?=
 =?utf-8?B?QW0yNHZvNDREN0xrOHJhbERJcGxyVktpVWRHK3BqOTVpTHdoQ1pNd0J4NWVj?=
 =?utf-8?B?K0lSVEwxaXIzd2NMd1pBbUYwTXVGVFJQVUk3REErK1ZvYXB6QWkrb2NiSFJL?=
 =?utf-8?B?UVpqWmJ2TTRhc2dWYktGdDFURlRtN2V0dTRDa1NTVy9kMzhjWjVBRjJrN0hI?=
 =?utf-8?B?MXVMNzJ1TEtHWnRKRS9McUQxRDY5cmU0NmkvUTE1eG9IRzZEYVBpSXlyTFYy?=
 =?utf-8?B?TithK0lVVUZTZWV0U3p4UEpRZmlwNExiWWNVdjFmeDU3cGZkT2c4VlE5UGtY?=
 =?utf-8?B?aitlZ0JHaWs3M2xPR1k2cWVhNkQybUlGY2U0UlJKRFBWMzBJVTZFMkVuV05S?=
 =?utf-8?B?WFU0TUxOS2NNMHd2UncwMlRwcGJuYXVSd0taT203WmZuT05VNElqN3ZzUUo1?=
 =?utf-8?B?LzNjVXowUjVTQnZhVHVIQlJ6MVE4WndDRm5VVHErM0ZWVnE3REtCT2pFTVY5?=
 =?utf-8?B?WjUxVmd2Qmw0M0RnV2VZN0hkcGdvQlFUVXdLQTV5b2t6RU5KZEN1YUVnVTJP?=
 =?utf-8?B?TTZ1VmpDQkFkcXFieHFnN25zS2J3bEpCZmNLTWdFcnc3MlNqZkJ5MEFjNmJS?=
 =?utf-8?B?bk1zckx1ZVRGOEllay81TUxBRHJFVDNsQ2JZc01rZ3NNOUpGdFlucmJpTXh5?=
 =?utf-8?B?V25pc0FmcXNrZXJPOTc3Um9YSUE5dTQxNC9xMFNBTnRLcnZVMlU3NkZ6T1Vp?=
 =?utf-8?B?L2c3Ti9BYmF4Sk5mMUdURzhsUGJvbHgvazdsOWo5WDEvQnJIVE5OZHRXTFZ0?=
 =?utf-8?B?T0Y3bnlXK2FYU2VmbElTSmwxUVlZcHo3cmIxbjA0NS9WYS84RllzUkNTU2tr?=
 =?utf-8?B?M3N1R1FvT3M1dWQxdzBrZTdCcWRiRjM0MmhYOHUra0RxdktSZ3RzSHFtZ2R3?=
 =?utf-8?B?SFVPb0wwUFVoY3A0c0ZSWjh2bWdDTEVGeWlkRk1keVF1ZG9oeFJoV0hqY01N?=
 =?utf-8?B?Uy9ic241WG12eE1FaVRrL3hiZ05SZ3B0L1Z1a2t3QjlpQXV6Z0RuVHY0SWZX?=
 =?utf-8?B?V1hRL3QrTjc2OWE4ZVNHRldNWitic09LWGF2K25DeVlLOVYvc2ZFNHBhQTVk?=
 =?utf-8?B?UzFSYU5vSDMycjV0SVY2NXl3byt5K2J1Rkk4Nmx0RTJ0bkU1K0g3eEV6Vll3?=
 =?utf-8?B?bWlQcU5VSGJGVDZ0V1BVVVoyN3RBWlBqckM2Tm5oSUxYZGttVm9kVEdDTkQ3?=
 =?utf-8?B?VzViUTFwRXJsWHpqWEFOaFF6NU0xaWY1L1lNbWJWU04xdFE4RVR2cjFJZGpk?=
 =?utf-8?Q?nKxbH8dWGbcTh1WQzJOp79ioY?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: ntYO3BBOTv6+AXg7teHKFfFkgaKReXlZLMm89d1/Jl29dVD6YR/P06p+xxifyqQ2S0A54rNlNl/kXfWlVzxAzaXijKqZ0Zcseklpd37G89dHCY1psKKRc5UNHgCYrF+PIPB9FB9PNSPTkyrzmn3MCe9aqYno4Lv1De8X+SXJJ8jHHKOrvpzPwE8EW3k/KFGhA8M/cQqsJy430a7bZZKuGCCnFTD2zT313vCY1RByhY8aPC5E8Ia3qqsnCx/yISMc2vqQAhbwOT6xS5e7UTAo2ykLoltKxNH6wVmUhwQEHfmiqrndvk6rc3nHg8rjnhHAneALIeLlqKk5iUlIiueCzPPvy8eqTzBjwg3N1dXaHZGbocG1AlrJKvEZkgi7cTqWO5r2Zs82BQSe8BPBKYLyHuHvSlCyjKqc9aVAn831phqj1fYEbo5tEwleOWuXn05MWYHj3AJtctmZNe8/MTajRrMWsfPV9qWlBJkmUJ3lRABnwCOOBiFA1Dk8e0vG2W83Mmkl2/B/reDD6OTfYLDeGa5K84bqnHU/nqLuTPxZyhVkrK4+AfgG0NCUx6Ykap5XPVRFZhVyMFoDJsHxFpBkYaJF9V0tnaaRyYF+QBuJRNJH9TdJ+Mk65s6JKr+rVuTQ0VQ3CQ/flAnWBJPSBhl8eKQFi6cza8s0/WmUxqxCnj0Sf5ull+XKUQYMf2h95tdxIMfwZM0BqkzDpi1HCSIVLf1VUb5hVMZr5f7cykw3kS0DrZUCWZrqH3oieK5GMe3pG+ZbkMEXrzkT5tF1o9ZoiHqscr6b3spXxXh5hlz4jHwxEqDiM+f2LAQ6J21P9eZVUWYiM2oFv5ebrE0+51V3rFNPrj6j3q0nBUsqQnv4PB3yfYk0ZjC6X1rA0iuF7lvf1K+MM4UnhMGPOFqz2UdF+w==
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ee211348-e545-4042-112f-08dbd4add91a
X-MS-Exchange-CrossTenant-AuthSource: MW4PR10MB6535.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Oct 2023 16:25:34.1056
 (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: 3ZKTxUFtIus4//HVGbbue2ZY2iKxhjvWm+0bmIyMJPoumB/TN5sttmPWDkewqWY/O1CKa8EnmovzXct+fKQLJw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4131
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-10-24_16,2023-10-24_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 malwarescore=0 mlxscore=0
 suspectscore=0 spamscore=0 adultscore=0 mlxlogscore=999 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310170001
 definitions=main-2310240141
X-Proofpoint-GUID: CbyEc0uoFMfuoXClNtvcvj4sYXgJyswZ
X-Proofpoint-ORIG-GUID: CbyEc0uoFMfuoXClNtvcvj4sYXgJyswZ
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Tue, 24 Oct 2023 09:26:06 -0700 (PDT)


On 10/24/2023 9:21 AM, Si-Wei Liu wrote:
>
>
> On 10/23/2023 10:45 PM, Jason Wang wrote:
>> On Sat, Oct 21, 2023 at 5:28 PM Si-Wei Liu <si-wei.liu@oracle.com> 
>> wrote:
>>> Using .compat_reset op from the previous patch, the buggy .reset
>>> behaviour can be kept as-is on older userspace apps, which don't ack 
>>> the
>>> IOTLB_PERSIST backend feature. As this compatibility quirk is 
>>> limited to
>>> those drivers that used to be buggy in the past, it won't affect change
>>> the behaviour or affect ABI on the setups with API compliant driver.
>>>
>>> The separation of .compat_reset from the regular .reset allows
>>> vhost-vdpa able to know which driver had broken behaviour before, so it
>>> can apply the corresponding compatibility quirk to the individual 
>>> driver
>>> whenever needed.  Compared to overloading the existing .reset with
>>> flags, .compat_reset won't cause any extra burden to the implementation
>>> of every compliant driver.
>>>
>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>> ---
>>>   drivers/vhost/vdpa.c         | 17 +++++++++++++----
>>>   drivers/virtio/virtio_vdpa.c |  2 +-
>>>   include/linux/vdpa.h         |  7 +++++--
>>>   3 files changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>>> index acc7c74ba7d6..9ce40003793b 100644
>>> --- a/drivers/vhost/vdpa.c
>>> +++ b/drivers/vhost/vdpa.c
>>> @@ -227,13 +227,22 @@ static void vhost_vdpa_unsetup_vq_irq(struct 
>>> vhost_vdpa *v, u16 qid)
>>> irq_bypass_unregister_producer(&vq->call_ctx.producer);
>>>   }
>>>
>>> -static int vhost_vdpa_reset(struct vhost_vdpa *v)
>>> +static int _compat_vdpa_reset(struct vhost_vdpa *v)
>>>   {
>>>          struct vdpa_device *vdpa = v->vdpa;
>>> +       u32 flags = 0;
>>>
>>> -       v->in_batch = 0;
>>> +       flags |= !vhost_backend_has_feature(v->vdev.vqs[0],
>>> + VHOST_BACKEND_F_IOTLB_PERSIST) ?
>>> +                VDPA_RESET_F_CLEAN_MAP : 0;
>>> +
>>> +       return vdpa_reset(vdpa, flags);
>>> +}
>>>
>>> -       return vdpa_reset(vdpa);
>>> +static int vhost_vdpa_reset(struct vhost_vdpa *v)
>>> +{
>>> +       v->in_batch = 0;
>>> +       return _compat_vdpa_reset(v);
>>>   }
>>>
>>>   static long vhost_vdpa_bind_mm(struct vhost_vdpa *v)
>>> @@ -312,7 +321,7 @@ static long vhost_vdpa_set_status(struct 
>>> vhost_vdpa *v, u8 __user *statusp)
>>>                          vhost_vdpa_unsetup_vq_irq(v, i);
>>>
>>>          if (status == 0) {
>>> -               ret = vdpa_reset(vdpa);
>>> +               ret = _compat_vdpa_reset(v);
>>>                  if (ret)
>>>                          return ret;
>>>          } else
>>> diff --git a/drivers/virtio/virtio_vdpa.c 
>>> b/drivers/virtio/virtio_vdpa.c
>>> index 06ce6d8c2e00..8d63e5923d24 100644
>>> --- a/drivers/virtio/virtio_vdpa.c
>>> +++ b/drivers/virtio/virtio_vdpa.c
>>> @@ -100,7 +100,7 @@ static void virtio_vdpa_reset(struct 
>>> virtio_device *vdev)
>>>   {
>>>          struct vdpa_device *vdpa = vd_get_vdpa(vdev);
>>>
>>> -       vdpa_reset(vdpa);
>>> +       vdpa_reset(vdpa, 0);
>>>   }
>>>
>>>   static bool virtio_vdpa_notify(struct virtqueue *vq)
>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>> index 6b8cbf75712d..db15ac07f8a6 100644
>>> --- a/include/linux/vdpa.h
>>> +++ b/include/linux/vdpa.h
>>> @@ -519,14 +519,17 @@ static inline struct device 
>>> *vdpa_get_dma_dev(struct vdpa_device *vdev)
>>>          return vdev->dma_dev;
>>>   }
>>>
>>> -static inline int vdpa_reset(struct vdpa_device *vdev)
>>> +static inline int vdpa_reset(struct vdpa_device *vdev, u32 flags)
>>>   {
>>>          const struct vdpa_config_ops *ops = vdev->config;
>>>          int ret;
>>>
>>>          down_write(&vdev->cf_lock);
>>>          vdev->features_valid = false;
>>> -       ret = ops->reset(vdev);
>>> +       if (ops->compat_reset && flags)
>>> +               ret = ops->compat_reset(vdev, flags);
>>> +       else
>>> +               ret = ops->reset(vdev);
>> Instead of inventing a new API that carries the flags. Tweak the
>> existing one seems to be simpler and better?
> Well, as indicated in the commit message, this allows vhost-vdpa be 
> able to know which driver had broken behavior before, so it
> can apply the corresponding compatibility quirk to the individual 
> driver when it's really necessary. If sending all flags 
> unconditionally down to every driver, it's hard for driver writers to 
> distinguish which are compatibility quirks that they can safely ignore 
> and which are feature flags that are encouraged to implement. In that 
> sense, gating features from being polluted by compatibility quirks 
> with an implicit op 
s/implicit/explicit/
> would be better.
>
> Regards,
> -Siwei
>>
>> As compat_reset(vdev, 0) == reset(vdev)
>>
>> Then you don't need the switch in the parent as well
>>
>> +static int vdpasim_reset(struct vdpa_device *vdpa)
>> +{
>> +       return vdpasim_compat_reset(vdpa, 0);
>> +}
>>
>> Thanks
>>
>>
>>> up_write(&vdev->cf_lock);
>>>          return ret;
>>>   }
>>> -- 
>>> 2.39.3
>>>
>