Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9f68802c-2692-7321-f916-670ee0abfc40@oracle.com>
Date:   Mon, 23 May 2022 17:01:27 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH 1/4] vdpa: Add stop operation
Content-Language: en-US
From:   Si-Wei Liu <si-wei.liu@oracle.com>
To:     Eugenio Perez Martin <eperezma@redhat.com>
Cc:     virtualization <virtualization@lists.linux-foundation.org>,
        Jason Wang <jasowang@redhat.com>,
        kvm list <kvm@vger.kernel.org>,
        "Michael S. Tsirkin" <mst@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Stefano Garzarella <sgarzare@redhat.com>,
        Longpeng <longpeng2@huawei.com>,
        Zhu Lingshan <lingshan.zhu@intel.com>,
        Martin Petrus Hubertus Habets <martinh@xilinx.com>,
        Harpreet Singh Anand <hanand@xilinx.com>, dinang@xilinx.com,
        Eli Cohen <elic@nvidia.com>,
        Laurent Vivier <lvivier@redhat.com>, pabloc@xilinx.com,
        "Dawar, Gautam" <gautam.dawar@amd.com>,
        Xie Yongji <xieyongji@bytedance.com>, habetsm.xilinx@gmail.com,
        Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
        tanuj.kamde@amd.com, Wu Zongyong <wuzongyong@linux.alibaba.com>,
        martinpo@xilinx.com, Cindy Lu <lulu@redhat.com>,
        ecree.xilinx@gmail.com, Parav Pandit <parav@nvidia.com>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Zhang Min <zhang.min9@zte.com.cn>
References: <20220520172325.980884-1-eperezma@redhat.com>
 <20220520172325.980884-2-eperezma@redhat.com>
 <79089dc4-07c4-369b-826c-1c6e12edcaff@oracle.com>
 <CAJaqyWd3BqZfmJv+eBYOGRwNz3OhNKjvHPiFOafSjzAnRMA_tQ@mail.gmail.com>
 <4de97962-cf7e-c334-5874-ba739270c705@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <4de97962-cf7e-c334-5874-ba739270c705@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?RlRJQk5hUFdDRkdrSDVTQ3BZS2R2YWd2bHJqWVdwR3pjMkF1aDBDMUc4WDY2?=
 =?utf-8?B?RUNMcFVvZGtNWjArMzZSd2tsOERmczFOUlQzTmFzWmVKUFdBRkJBZUhnWFZi?=
 =?utf-8?B?c1pHbGNQV1BqMnJLUnB2eEF6N3lUckJML1dYaWtacTFuRDJGbW03WnNmaVQ4?=
 =?utf-8?B?S01KZmFGSDRhN0p2RjI1WE53anhqWWF3OWpBYWEzdHpjTjhmVUc3NWVSWXdn?=
 =?utf-8?B?SDBDZks0SHRjT3JwVjdnMTVlZGpPYUxkN3pVcDRtTmI1eURzME5UUkdHc0FZ?=
 =?utf-8?B?TFYwU2h6SlVNa2M3TnFOTUdnT2JzUlAyWUZuOXZMM3lCZVY3WnBSL2FqbmZ1?=
 =?utf-8?B?WkJyN2owa2JpTWRtRmo2Z2JsTEV3RGdZZm1kQno3b2djdjg4YjEvNEUxRWdp?=
 =?utf-8?B?V1hGRHg3TERVdG9QeGtmNzdNMDV4WmZibmxVV05XbW5xSlZWOGJzOU14bnpT?=
 =?utf-8?B?ZGh0UFMvUVRHMFN1bnllTlE5dUo5QzVDWjBqTkV3a2JZSWFjWlZRemIxWjBB?=
 =?utf-8?B?RnNvL2FDL1g5Q2R4TktNaHhxeC9SL3d6bExLTTVFY1Uxa1lNSGZXY0kyUGlG?=
 =?utf-8?B?YzFlcEMrUFBFejRkZEhyWkhwZHFFSk1mckIra25GWFA4dFhGYS9MSWJZYThj?=
 =?utf-8?B?VmcrU0VtZVcyendIU3BBYk1LL0ZoVkJoQkZKZE5xQWhneDNISitaMW5RZVRR?=
 =?utf-8?B?THNRSUtVWGM3cTFNTGFWM2V5UEFUVk4xekVJd3M3dzRSaGJGUzBYN3dXcDZI?=
 =?utf-8?B?dzJSMU11YjQ2R2VDSDkyWjEyMHAvSXd4UVBWMnBEYjFVeHI2azlWRyszZ2Fm?=
 =?utf-8?B?SHBJN2h3T0xBZ24vZFM0NUZ6UzlXYlh2YnJYWmZUZ29jUnZQNkhQUm55ZUNH?=
 =?utf-8?B?azZycmt0S2orNGVJZlRRVGxnWVFqRnk3Uy85Zjd1MWhqWXgveWhZb2dqbGE2?=
 =?utf-8?B?bGdQUmRCN1V0QjhuQVNNOFV5S2xZNkxsRTlmS0h6MG9UbmxlY1ZIRkJEak16?=
 =?utf-8?B?eWs0ckZmNWdkcnJGQko4V1R1L3lacWJ1QlM5b2hYVmtMNWJLT0RtSEVPQUF1?=
 =?utf-8?B?bEtnQXJNMVNkQTQ3SjZ3dEZRWkZWRWxNeGhweXNYZ3hobkxZdnY3c3FiU2Vx?=
 =?utf-8?B?MlArNndQZUZMME9XNW5mWmlXMlBuMUFwd0orK01Rc216TnNqV1NtVUVyaFVw?=
 =?utf-8?B?dDRNTTZiT1lsakpaZlpUbGEwZWRqM1VwTEVZWTdrZkxFdmpzVzFzTGVoMUFk?=
 =?utf-8?B?Nk9lWWUvU29RSUZIK25XUzZwNENnNW1GcEtJSHBlUHBKR1JCanRiNXFoZk9H?=
 =?utf-8?B?MlNwOFNKVmUyYTVXUEpjbG96QVdLazVPT01uRzBzb1VldGZTNkZZdWhjbDNQ?=
 =?utf-8?B?bzcvS24zTlRyVkY5SlBQVlRuRHJPT1owQzFMdDFGWHFKTnAwNm5KbWkxOGk1?=
 =?utf-8?B?dXR2ck5pem9wV3dnRVc2Qkw1dnkxMFJ0S0ZuUFJJNGhLOHZYOUFzMDJnaDll?=
 =?utf-8?B?ZWMyZCs2aDFjSHRrbzN6R2J1UVdLKzYybXBxTnFqUTE4dnFtc0xuMyt0TFdM?=
 =?utf-8?B?eDVid3Y5emd6ZFczVnhSL1lsdk9wT3l1dEMxT0hMZ29BYUtIOEl3T1hoeCtn?=
 =?utf-8?B?RXlNU3hCeld0dm1YUzgvalFRMXhZK1V0OWVteVZRbG5aMWJZcXVNZForUmtC?=
 =?utf-8?B?Yy9BWi91aENZVTBsTm1iR01MQjl2V2I3c0l5ZGZHcVlYN2FtMjIxMU4yeGZ4?=
 =?utf-8?B?QllmdnR2TDkwVXJhMEtVdGJPM2FVVU8xTWQ1ekZIV3BISSszZ25teVo1aGV2?=
 =?utf-8?B?SGxJMDh1VFhMOStZRTNiMUYvRndPdFVQZ1p3dDBVUW1nQU5VSmFVUXBKaUdL?=
 =?utf-8?B?c1U4bTRrSlRoOUFmcXhwMmo4Ly9aREcwV2h3MzRuNno3QmVNUjJuMUswd2ZU?=
 =?utf-8?B?UGtLQ2thSG5aMFRBWm5zU3lFbGY3UGJsaTdPazVLSklwZ2o3SzZZellYVHRj?=
 =?utf-8?B?T1hmL3JTempDN0Q1b2FiSTlTRGxJZUJidzN3S08xSEdFWTBwK2RUaVBPcEVF?=
 =?utf-8?B?OFdiWUt0eW43aHlPWS9mRzVqVHhkRXVMUUNJNk1EcU9vMHRUSU05YzdYeVBK?=
 =?utf-8?B?WjZrREtRb2NybUNzR0E0a0FvVjVERk42S2ZNK2pUdkkzdmo0bGduY3dtR01E?=
 =?utf-8?B?bUdYYktJMFhsOVZMdlNPRU9lSSt2UFk4bk56eiswWWtTQS83ZWRQby9CWndz?=
 =?utf-8?B?S1Q1WjFvK1ErKzRzRnZrZFhkS013ZC9NNEIzRGlhNDNlT2xJaGh4WFVQYTFk?=
 =?utf-8?B?NVBHZGQ1RDh2OWdMUUVJd2lHK2t5ajVhTTdkODhvOHZ4WVpRbnFTOW1NVUx0?=
 =?utf-8?Q?ZXw1AMwnr18m2Cy0=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 086429e7-4e43-4e75-a04e-08da3d188e5f
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB3287.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 May 2022 00:01:30.5362
 (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: 4X97nxx/xaZyz+y2vEfwjvO4GgtogHTMvjn9yUPyv8VbhBJ4yOSrr5WkHewdJtTWBONo+xuNO5ZtoBTX3SETQw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3173
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.874
 definitions=2022-05-23_10:2022-05-23,2022-05-23 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 adultscore=0 bulkscore=0
 mlxlogscore=999 mlxscore=0 malwarescore=0 phishscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2205230123
X-Proofpoint-GUID: ToxyOj29RwLtRaUMfUF91ZIQBQzXURB5
X-Proofpoint-ORIG-GUID: ToxyOj29RwLtRaUMfUF91ZIQBQzXURB5
X-Spam-Status: No, score=-6.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 5/23/2022 4:54 PM, Si-Wei Liu wrote:
>
>
> On 5/23/2022 12:20 PM, Eugenio Perez Martin wrote:
>> On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> 
>> wrote:
>>>
>>>
>>> On 5/20/2022 10:23 AM, Eugenio Pérez wrote:
>>>> This operation is optional: It it's not implemented, backend 
>>>> feature bit
>>>> will not be exposed.
>>>>
>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>> ---
>>>>    include/linux/vdpa.h | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>>> index 15af802d41c4..ddfebc4e1e01 100644
>>>> --- a/include/linux/vdpa.h
>>>> +++ b/include/linux/vdpa.h
>>>> @@ -215,6 +215,11 @@ struct vdpa_map_file {
>>>>     * @reset:                  Reset device
>>>>     *                          @vdev: vdpa device
>>>>     *                          Returns integer: success (0) or 
>>>> error (< 0)
>>>> + * @stop:                    Stop or resume the device (optional, 
>>>> but it must
>>>> + *                           be implemented if require device stop)
>>>> + *                           @vdev: vdpa device
>>>> + *                           @stop: stop (true), not stop (false)
>>>> + *                           Returns integer: success (0) or error 
>>>> (< 0)
>>> Is this uAPI meant to address all use cases described in the full blown
>>> _F_STOP virtio spec proposal, such as:
>>>
>>> --------------%<--------------
>>>
>>> ...... the device MUST finish any in flight
>>> operations after the driver writes STOP.  Depending on the device, it
>>> can do it
>>> in many ways as long as the driver can recover its normal operation 
>>> if it
>>> resumes the device without the need of resetting it:
>>>
>>> - Drain and wait for the completion of all pending requests until a
>>>     convenient avail descriptor. Ignore any other posterior descriptor.
>>> - Return a device-specific failure for these descriptors, so the driver
>>>     can choose to retry or to cancel them.
>>> - Mark them as done even if they are not, if the kind of device can
>>>     assume to lose them.
>>> --------------%<--------------
>>>
>> Right, this is totally underspecified in this series.
>>
>> I'll expand on it in the next version, but that text proposed to
>> virtio-comment was complicated and misleading. I find better to get
>> the previous version description. Would the next description work?
>>
>> ```
>> After the return of ioctl, the device MUST finish any pending 
>> operations like
>> in flight requests. It must also preserve all the necessary state (the
>> virtqueue vring base plus the possible device specific states)
> Hmmm, "possible device specific states" is a bit vague. Does it 
> require the device to save any device internal state that is not 
> defined in the virtio spec - such as any failed in-flight requests to 
> resubmit upon resume? Or you would lean on SVQ to intercept it in 
> depth and save it with some other means? I think network device also 
> has internal state such as flow steering state that needs bookkeeping 
> as well.
Noted that I understand you may introduce additional feature call 
similar to VHOST_USER_GET_INFLIGHT_FD for (failed) in-flight request, 
but since that's is a get interface, I assume the actual state 
preserving should still take place in this STOP call.

-Siwei

>
> A follow-up question is what is the use of the `stop` argument of 
> false, does it require the device to support resume? I seem to recall 
> this is something to abandon in favor of device reset plus setting 
> queue base/addr after. Or it's just a optional feature that may be 
> device specific (if one can do so in simple way).
>
> -Siwei
>
>>   that is required
>> for restoring in the future.
>>
>> In the future, we will provide features similar to 
>> VHOST_USER_GET_INFLIGHT_FD
>> so the device can save pending operations.
>> ```
>>
>> Thanks for pointing it out!
>>
>>
>>
>>
>>
>>> E.g. do I assume correctly all in flight requests are flushed after
>>> return from this uAPI call? Or some of pending requests may be subject
>>> to loss or failure? How does the caller/user specify these various
>>> options (if there are) for device stop?
>>>
>>> BTW, it would be nice to add the corresponding support to vdpa_sim_blk
>>> as well to demo the stop handling. To just show it on vdpa-sim-net IMHO
>>> is perhaps not so convincing.
>>>
>>> -Siwei
>>>
>>>>     * @get_config_size: Get the size of the configuration space 
>>>> includes
>>>>     *                          fields that are conditional on 
>>>> feature bits.
>>>>     *                          @vdev: vdpa device
>>>> @@ -316,6 +321,7 @@ struct vdpa_config_ops {
>>>>        u8 (*get_status)(struct vdpa_device *vdev);
>>>>        void (*set_status)(struct vdpa_device *vdev, u8 status);
>>>>        int (*reset)(struct vdpa_device *vdev);
>>>> +     int (*stop)(struct vdpa_device *vdev, bool stop);
>>>>        size_t (*get_config_size)(struct vdpa_device *vdev);
>>>>        void (*get_config)(struct vdpa_device *vdev, unsigned int 
>>>> offset,
>>>>                           void *buf, unsigned int len);
>