Received-SPF: pass (google.com: domain of linux-kernel+bounces-62028-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <e3515937-7d36-4167-84dc-a9c1721249cc@oracle.com>
Date: Mon, 12 Feb 2024 11:37:12 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V1] vdpa: suspend and resume require DRIVER_OK
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
        Jason Wang <jasowang@redhat.com>, Si-Wei Liu <si-wei.liu@oracle.com>,
        Eugenio Perez Martin <eperezma@redhat.com>,
        Stefano Garzarella <sgarzare@redhat.com>
References: <1707517799-137286-1-git-send-email-steven.sistare@oracle.com>
 <20240212031722-mutt-send-email-mst@kernel.org>
 <a4d2626d-7d74-4243-93a9-01d7adc8cda4@oracle.com>
 <20240212105604-mutt-send-email-mst@kernel.org>
Content-Language: en-US
From: Steven Sistare <steven.sistare@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <20240212105604-mutt-send-email-mst@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?aWN4Vmw0VzFyajlKZWtlQ3h1THU4S0RBSHVDbEY2YWh0Y1NiOStmYlo5SWJO?=
 =?utf-8?B?akNJU0JLVDNQMTZXMmNVY295V0NtRWVYZFBPVnUwb05DTjFoK0c4WWNaTXNW?=
 =?utf-8?B?TlZMaE1MSXdLZXBQQjJZOGdtdWtkb1ZncVM1d0RDREEwS1VLYUltVW96TS93?=
 =?utf-8?B?WFJ3TnlxTjNwWXJmWTZVMlFveTFTV081T3VWQnQ0QWZ2dkYxOXVHOFUwYXBv?=
 =?utf-8?B?QnBtTkZELzlQb0tjTkpjbzFyTkExSktjTS9CMkZ3bjk4RFJpSzdZelYyaitt?=
 =?utf-8?B?aDVuc081MC9hZjBadUNNY1BneS8yVzVUTnhjN29LU3FYbVdtcHVYVWhzeDZJ?=
 =?utf-8?B?V3RwMyt1NXd4Ylp6OWIyc1M1SjFzK3FhVkJnOGtuVFl6ZGx2Y2drOGdJcEhD?=
 =?utf-8?B?aytBRDlnakJtRUFKLzZPTnlSb0FObWtvWkNUVEtKcXM5d1ZBMllGL0tOUmZH?=
 =?utf-8?B?ZVRDZkE1aDRJYmJqR1RTMWk5VjZmdHdPOGxZQXN6WGNiemdEaVlBcWNmamxN?=
 =?utf-8?B?anVWVlJkNUZmUGRUMmg4dkJEdnpySHpHRUdDVWFVNDN0cDd3VFh1SHpMaHdM?=
 =?utf-8?B?M1VDeXk1Q3JRVVo2Zk5XZHFSQW80KzRUa0VXZEF5aTlJUTNIeFJGeDZjMmw0?=
 =?utf-8?B?WUszdlpnaHNrRHQzaDZaUUdwV0FydmgwSzZBTDZaeXM1cGVVUWFndVhaRlJL?=
 =?utf-8?B?MEs3UXYzZVcvMm9PUzAyVGpoV0xRQmVsQkhsR2NmWVVVeVczTjRUT3lSUzRI?=
 =?utf-8?B?M1U2K0NHa0QwaWxxS05ISHU2OGdzNVdPcEVsYVpzM2NGZUxrZkVkUVFJeEU5?=
 =?utf-8?B?WXM0Q0JkRGZURXBrQVgydXhTVDhxQnZRbkVYd3U2dGNuVmNKOWFnaGZQN3dP?=
 =?utf-8?B?WVpBckpSakxqaXdRTk1UZWFjMWZRc2x0WmtDbHlIWTlNMndDVEZkMzFYQmxz?=
 =?utf-8?B?ekpPTWYyMFBQNlB2dm9zbDZUYWJHbGxwZXJWdThuME5yTG8rZjBoeS84d1ZH?=
 =?utf-8?B?Z01QQ1FyU3ZIZ2drV2E1ekRQODV0akF4RXFHRWV5RVZSd3lKYlZuWVl2czhs?=
 =?utf-8?B?RXdBVExhQUsxTzB0d0x5THFsYnRUaVdkWHhHdWsxSzBoL0g4Y2o3azE3S205?=
 =?utf-8?B?ZHg1aFViRDJ4UWNIU0JLV0p1bDFUZyszODNrbUVnQVNTWXFNcHpOeTV3anh5?=
 =?utf-8?B?eDFQYURiamNSZlZCYjZhbkcxOXkzZ2ZHcWJuRHQ1eDF6THZadHZ5RXoxc0Uy?=
 =?utf-8?B?bW95R2FHTDk3UTlqMTQ3OFBXL1VnbWd4ZWtaVTMydDl0Y1FmQTJNQmJEb2Rq?=
 =?utf-8?B?SndrSHZmVVIwYlNmYmdEZytDYUdBQklXV1Q5Z01uVEswTnEvVC9QazVkYmhQ?=
 =?utf-8?B?TkpvZGtuQmV5UWV5V0tmQUU4RCsvVG8vY1Y0TzdSbHNmQTRuZklybzNKLzZ3?=
 =?utf-8?B?b3Zsd3ozblRMMGxBbGtLR29wKzVSTzhMWFNWOUNtTHEzUmRiNkZUL01Xb282?=
 =?utf-8?B?L2JsN3hzQ0plSXdoZVZVL2JKSFpvZlNiSmNESnFhNGdxZklsQVlwVGJjN0d1?=
 =?utf-8?B?WXdOMGRpTTBHNXFpVmdrVW10UEM2QW1DRHg4UVplNlB4Q1AwOS9DajNGOGNT?=
 =?utf-8?B?dkNMTGtsSytQTVIvUHpwWmpndTY3WVNsSFRRdHM5ME9aZHRxQkxONW4xaE5n?=
 =?utf-8?B?ZzRyYTcwM2pON25USWhUekduNWMyQnA3VUNKdHVUV1NJUm0vUVNha2NrdDQ3?=
 =?utf-8?B?RHJDelN6MVdSNWNKbHlmOU8xWkdhcm5RV255ZGNxdklMNVR3eEZ3TC9Gb2lD?=
 =?utf-8?B?eUZzSG9pMlY2MC9EYW5SZjVITkpQcUVhb01KMXBiZUV1WGZyN1I3cTA2ODZ3?=
 =?utf-8?B?ZWVnRlhkbGdPWE1sODVmVnc4cVF2RFJsSmt6cUVhd2JXSmE0MDYybjZId1R4?=
 =?utf-8?B?bFRWSzFoRlZjcFZBVVVPR0VWV292TGJGdmdFdjZhbnoxTTdteEUycnFObE9z?=
 =?utf-8?B?S1dUZFVzRFBhaEdPMXp3NnAxa3pWeVZTTkd4SWd4ZmI4MTRCWjliRnpMVVc2?=
 =?utf-8?B?dGsrRllsTUJsRXBZVW5SK1ZCdlJnT2ZJc2FOZmJqRFFTSFBLa3pucS91V2Zq?=
 =?utf-8?B?K1FLd1NZZ0lGSm9UaG1yUy91SWcwVUZZQ1k5cUI5UTNublRXVUdSMnRrTzBp?=
 =?utf-8?B?Z2c9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	+f5UqvWdsV1txnN/TDQL7rxn1aNVW51hfVL0WlnLrxDWEJqSf5DqwkCT2vJBMcz5D9231M0dCOa1v2TCsOeETPuH8HywP9Wm/bF2mAZhhG5J+ukDSdpnbJRMIe+llbTFSwoqWnX1KJqV27eEnwOYIjjEH+Pnjiyruivt9cFyZ0bE3JTeSP4B2sGfgVib1qVj+2hf94l5DKpev67B8QfkeSjfos5jcvZbRDr3dQ+nKqVQXjevGOK9RRf5+enOy6NaE6dpZdVTabeoI0lEOWCbixNakBhn80RbnCKkv+t2n3J6wZqXT7XSUevyLWuHSNRF1JfXOf+tpZ4Dzope67dTsXlZHsZSGEuRJlaH+fmK6wUkbqlkpkOO0fHd0C3absQzdwsp4draCLiKjWy2yoPZtqjd/g6JxfpTWqzQ7bBIzypNkIXv2xzwWLnDveGtagaF3IO3Ik0vEmGbNYnCcUxKZhARL1QZorE4gY6etc2JBwOvUN3Py+SR4BYKwGNg0tBTq5V9wwltQHo1HJtcPb7XE3HzKYAnA2C6nAnHpkZL763BmUtrlUJnvnpRPnVqcSixmiIYwh24SydTxOAtkckCLvJCWUXVUPLjmSHoRNDmA2g=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: abb479f9-8a28-427d-d6d8-08dc2be8df45
X-MS-Exchange-CrossTenant-AuthSource: SA2PR10MB4684.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Feb 2024 16:37:15.9478
 (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: PI8dTsSgGCKaE+zSoNOIJtb7DfANTO0+CfJVcAn81AbgmRxtb+B4QUQCEwmlTtn0nvNj7pUuu/prK3vTY3tjDbnShYlQMXRI9ZgEmmyW+TM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB5222
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-12_14,2024-02-12_03,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 phishscore=0 mlxscore=0
 bulkscore=0 spamscore=0 malwarescore=0 adultscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2402120125
X-Proofpoint-ORIG-GUID: PKCLQD6eemIfghxpV_2xkOGNz_OxChXG
X-Proofpoint-GUID: PKCLQD6eemIfghxpV_2xkOGNz_OxChXG

On 2/12/2024 10:56 AM, Michael S. Tsirkin wrote:
> On Mon, Feb 12, 2024 at 09:56:31AM -0500, Steven Sistare wrote:
>> On 2/12/2024 3:19 AM, Michael S. Tsirkin wrote:
>>> On Fri, Feb 09, 2024 at 02:29:59PM -0800, Steve Sistare wrote:
>>>> Calling suspend or resume requires VIRTIO_CONFIG_S_DRIVER_OK, for all
>>>> vdpa devices.
>>>>
>>>> Suggested-by: Eugenio Perez Martin <eperezma@redhat.com>"
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>
>>> I don't think failing suspend or resume makes sense though -
>>> e.g. practically failing suspend will just prevent sleeping I think -
>>> why should guest not having driver loaded prevent system suspend?
>>
>> Got it, my fix is too heavy handed.
>>
>>> there's also state such as features set which does need to be
>>> preserved.
>>>
>>> I think the thing to do is to skip invoking suspend/resume callback
>>
>> OK.
>>
>>>  and in
>>> fact checking suspend/resume altogether.
>>
>> Currently ops->suspend, vhost_vdpa_can_suspend(), and VHOST_BACKEND_F_SUSPEND
>> are equivalent.  Hence if !ops->suspend, then then the driver does not support
>> it, and indeed may break if suspend is used, so system suspend must be blocked,
>> AFAICT.  Yielding:
> 
> If DRIVER_OK is not set then there's nothing to be done for migration.
> So callback not needed.

OK, I missed your point.  Next attempt:

   vhost_vdpa_suspend()
       if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK))
           return 0;

       if (!ops->suspend)
           return -EOPNOTSUPP;

- Steve
>>     vhost_vdpa_suspend()
>>         if (!ops->suspend)
>>             return -EOPNOTSUPP;
>>
>>         if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK))
>>             return 0;
>>
>> - Steve
>>
>>>> ---
>>>>  drivers/vhost/vdpa.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>>>> index bc4a51e4638b..ce1882acfc3b 100644
>>>> --- a/drivers/vhost/vdpa.c
>>>> +++ b/drivers/vhost/vdpa.c
>>>> @@ -598,6 +598,9 @@ static long vhost_vdpa_suspend(struct vhost_vdpa *v)
>>>>  	if (!ops->suspend)
>>>>  		return -EOPNOTSUPP;
>>>>  
>>>> +	if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK))
>>>> +		return -EINVAL;
>>>> +
>>>>  	ret = ops->suspend(vdpa);
>>>>  	if (!ret)
>>>>  		v->suspended = true;
>>>> @@ -618,6 +621,9 @@ static long vhost_vdpa_resume(struct vhost_vdpa *v)
>>>>  	if (!ops->resume)
>>>>  		return -EOPNOTSUPP;
>>>>  
>>>> +	if (!(ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK))
>>>> +		return -EINVAL;
>>>> +
>>>>  	ret = ops->resume(vdpa);
>>>>  	if (!ret)
>>>>  		v->suspended = false;
>>>> -- 
>>>> 2.39.3
>>>
>