Received-SPF: pass (google.com: domain of linux-kernel+bounces-60117-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <e538b2cf-d1fd-4dc2-a09b-e5b4b90704c2@oracle.com>
Date: Fri, 9 Feb 2024 17:34:06 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V1] vdpa_sim: reset must not run
Content-Language: en-US
To: Stefano Garzarella <sgarzare@redhat.com>,
        Eugenio Perez Martin <eperezma@redhat.com>
Cc: virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>, Si-Wei Liu <si-wei.liu@oracle.com>
References: <1705519403-255169-1-git-send-email-steven.sistare@oracle.com>
 <mhnsrwlvqjeftu5aa77iaowdk2wrq366yofjchbn5fwchgcbna@kcjtoj46fawi>
 <CAJaqyWcLj7Y0rm3_jdfP8KgS4gkE9ns286XJywBGE0iijMireQ@mail.gmail.com>
 <ouq2ntygn4i7gtld5utumipzggcakbqhu2hc5lbznoddhvezkk@jstz4h3vovej>
From: Steven Sistare <steven.sistare@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ouq2ntygn4i7gtld5utumipzggcakbqhu2hc5lbznoddhvezkk@jstz4h3vovej>
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?TWk2NGhQVm1vMXd6cFRKb2lrRUhZdmdzVW5oODdkQ2prMzdvZGtReDZaQTlR?=
 =?utf-8?B?WStSWXozUnVsYTR2am9TejIwdnNEL2w5RE84TWlCbVVObk05Q0ZXY2MxK0Np?=
 =?utf-8?B?bzVtRWNHZlVEWHN3MlpEWUFIYTFqR3RMYklPd25JSWZvRjNyays4V1VOTzBo?=
 =?utf-8?B?dENscVFTZERQQUNiblZ5akNYaHpDUVdUOFZwY1IvUVk0NTVKWjZacUJZVnN2?=
 =?utf-8?B?UUlrelM1WWQ5NmFoMzBnUm9oVjV6VU0xc1VZT3RpTUw3Mkp6OVF4aUdONmxU?=
 =?utf-8?B?YThEWG5Vakc2dlNyOEVtY25ZMFJjRkpRMWlpWVdwdzBjNm5Tc1hzb1p4N0RP?=
 =?utf-8?B?NlhwTy9iTEVCZW1ZeE1aOENIZElHZXRoZ2RZTVhNb0xHMUU0TytGQytvM3lJ?=
 =?utf-8?B?VUUxa3BhU0dsMGkzL1NXR1ZCMFNFdit2T2FmT2UrSnNETFozaTQ1WGwrdkRj?=
 =?utf-8?B?T1V4clc3VGt5b3UrSzNLUDBTVTliUzZ5NnhhUjIyT0hvanllOWd0UTB6Mmt3?=
 =?utf-8?B?OU1HSWliN0RYaEMrR0dFQitqZk1reklldjRxYUdWQlBCVElvRUlXTWVjVkoz?=
 =?utf-8?B?bFJXVGJhcW1zL2JYUHRJNGpRNDhtV2pQWUVFUEJ5UW1aVk84UTJzWExkaHBD?=
 =?utf-8?B?RzlId1piTHdqOEZZUlpyRjBzV2Q1VjhCY09IYmF0bEQvRVp3SkQ3VkI0NHJ4?=
 =?utf-8?B?TGt0OEsrS0hENFJ5bnczQk9uazZQRDM4QW5pNHVjMXJZUFF5cklSNEkySVhR?=
 =?utf-8?B?WmdXMlIzYlZnQmZWOWVVWGZ0aDVHZ1o0VUkzODAvQU5NWWNMeEhGb0U4UnE5?=
 =?utf-8?B?dzJHdmx3alpNeXRqVFVGaEFKRHBSc3YrU1ppVWtCaGJ0WXdMOTdURzIzblNB?=
 =?utf-8?B?UHNZcnFXWlpNY0F0S0Q5b3dvbWhIVnhCSThvNXV6a0tPbDRiOTVjNW16dFI4?=
 =?utf-8?B?SkVPK2NpUnovd0FzL3dURWVLZzZYcTZuelFDSm1YVlU0Qk9ZWmdRamNkbXh6?=
 =?utf-8?B?OXI0RWluZHNhYUl1RWZTeEp0ZUg2bktGeUpTYkNkSEpEOGlnbWNkeVpJMnVY?=
 =?utf-8?B?Smo1ejBDVkVpckxDK1dYbTZqZStsSWVEMi9HK3k5NGN1RE83MDdrT29XZ1dU?=
 =?utf-8?B?MWpET3NrS2ZtK1NXUk1kbzdrbG5HT3JVeURod3FEck5HaEk4VlF3TkhqbDR6?=
 =?utf-8?B?TUE0YlJEYkcvSG8zR0NHZE9SWXMzSXhBRGF4Zy9UU1QvYTJvZFNOTDZTcUpC?=
 =?utf-8?B?SzFXOGdJc255cnBPbys3UFdiN2pSNnlNUW1PeE1QN1lXclpxRWVSd3hmdWY1?=
 =?utf-8?B?dnNGc0hnWnRybGtCVi8va1RKaUpveVlLMHpRYk9CQXEzc1AyUWp4NFROUTVI?=
 =?utf-8?B?R3ExeEtEdkxKb0F2SU53dW01RTRQN3dlcm9mMUxYS2R3QjNlcnJmQWdsdUZ4?=
 =?utf-8?B?TW9NOW9NeGN2K0dIUVdSUmZZTnBqclBlTXk4NkFmUFhuU1pPVDUrR0cvb2lu?=
 =?utf-8?B?VkZHT1RkSDhBWmIraE9nUm00TzFMZldqbUxWNzlrcHBHWThZeW90SUVlVDBY?=
 =?utf-8?B?WDNVbmQwQ0VjQTJuMnpVNDFuelRjcy9tN3BUSzYrWmR5MkcyMlJIUDNhTFE1?=
 =?utf-8?B?eHJQYno0TFBnWUNZZUVrbFkzemkzdm0vUk5TS0FmRGFkRGlNMWNKT2E5ZlRN?=
 =?utf-8?B?SlcrMlY1d2dleUdwMVJRNjFIMHZDWHpUdk5lQ1lRZmlEL0FFekpUUlRuYlpK?=
 =?utf-8?B?TWo0cmZmZ2dDWnVJYjhxK2RkR2pZWUlBVGVnMDBBbDYwM1BoYUJLdU1valdu?=
 =?utf-8?B?Y1JURUVveVpsYkJTT0ZIenIvREVsRHJtQVV2UFRnRDh2T0l0U2JXb2NCc2xl?=
 =?utf-8?B?SUtlck5hL2g2Nnh0UWZRRlo4ZUdjMmVrM1QwOFkzRXNPREg0OE00aGVKVmxD?=
 =?utf-8?B?NWpOU2REcXB2ek1NTWxHekk1TW9EZ0FWRWxiZWszOTYzRTAyLzJsNDZmVU9L?=
 =?utf-8?B?dVlCT0RrRExDNFJ4enJHZVk1ZDN1T1FKSUJucUJ6b0k4OHFBZUs2WDVlbDB0?=
 =?utf-8?B?UURHcGxDTmFRTm5DLzVpVVVTd1ZZT3orSEhyVU9DWktTeldVY3hYV3dIOExi?=
 =?utf-8?B?MVMzWUpUbi9pQ3ZjanJWQXB1UEZmUWYrdlBVVmp1Zmg1S3NhUm81YVZTVXJI?=
 =?utf-8?B?S2c9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	HhUPlM5pfDEti31qMhWWjs+2MG3ZJxx5ei2nc5FE5hSP+XYe6tI5fkIVCECu2I0TnKprUcNmhWH6oat4sop314+ApluocGSeicmv8lyNukjmDSOYkiPEHzqFi7E4HLdfnSyXbJXjE7bdGiWe3eADRFUHHwXrmmb5nvmL2tVpnB1zJ/gtqCPNYscG8Mdrg6Kt+p0d+FiG1Zw3HjJVrn7ZyKIaqtj7lXCy76hCh1HaQdziB5eb5EwyKYw766+Rwgh9cjQ4NXuGSJCk5jq/vLt0PImcqqEMyXLSqAce4LHJlXWMa/1tIFvrzlt7IPDibJaWw4bHo3C1t9hdBKybNh3tkgUFUQUFpq2n8GlLg0nAZhr843hYDNwTSs0dBj/fxhKVURDKR/q3TwpN2n6ce6KWTZI66etKvVlogxA7TeJpH0BGfXwu/o5KW2rn3MQnHjoqsk6Ao3JHqdAdPBemNoT5WZBF9CRNSGFHcCjIGxKEBZB1B+OI8dM7V46kvqHZo7yZTT+wXwetEOXygPDcK91EitAhnDRk85GZU6rfUhkghPaw4j4Ob5g3wY4PFuIO8uLUEorxbOhKtXIBhJqTmySLosa9b9XC0WZQvxbcJNjS3B8=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 57fb2194-1c1a-400a-2839-08dc29bf3ba7
X-MS-Exchange-CrossTenant-AuthSource: SA2PR10MB4684.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Feb 2024 22:34:09.7508
 (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: CyPaW2EgWs0EYXw2fBJgSNgN2bSUEMpf2lGiaW7T5eJNSNg3ovjjWwHGsnaSmumeIslLAZWggJA+Kbu0pbWdLFX1dN4nCWfHz6sEu2w8NXg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR10MB6584
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-09_18,2024-02-08_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 spamscore=0
 mlxlogscore=999 bulkscore=0 mlxscore=0 phishscore=0 adultscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402090165
X-Proofpoint-ORIG-GUID: K36OVJO6aaPNwH-HJEvesQhkah7APHhL
X-Proofpoint-GUID: K36OVJO6aaPNwH-HJEvesQhkah7APHhL

On 1/22/2024 5:59 AM, Stefano Garzarella wrote:
> On Mon, Jan 22, 2024 at 11:47:22AM +0100, Eugenio Perez Martin wrote:
>> On Mon, Jan 22, 2024 at 11:22 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>>
>>> On Wed, Jan 17, 2024 at 11:23:23AM -0800, Steve Sistare wrote:
>>> >vdpasim_do_reset sets running to true, which is wrong, as it allows
>>> >vdpasim_kick_vq to post work requests before the device has been
>>> >configured.  To fix, do not set running until VIRTIO_CONFIG_S_FEATURES_OK
>>> >is set.
>>> >
>>> >Fixes: 0c89e2a3a9d0 ("vdpa_sim: Implement suspend vdpa op")
>>> >Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>> >Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
>>> >---
>>> > drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 ++-
>>> > 1 file changed, 2 insertions(+), 1 deletion(-)
>>> >
>>> >diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>> >index be2925d0d283..6304cb0b4770 100644
>>> >--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>> >+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>> >@@ -160,7 +160,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim, u32 flags)
>>> >               }
>>> >       }
>>> >
>>> >-      vdpasim->running = true;
>>> >+      vdpasim->running = false;
>>> >       spin_unlock(&vdpasim->iommu_lock);
>>> >
>>> >       vdpasim->features = 0;
>>> >@@ -483,6 +483,7 @@ static void vdpasim_set_status(struct vdpa_device *vdpa, u8 status)
>>> >
>>> >       mutex_lock(&vdpasim->mutex);
>>> >       vdpasim->status = status;
>>> >+      vdpasim->running = (status & VIRTIO_CONFIG_S_FEATURES_OK) != 0;
>>> >       mutex_unlock(&vdpasim->mutex);
>>>
>>> Should we do something similar also in vdpasim_resume() ?
>>>
>>> I mean something like this:
>>>
>>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>> index be2925d0d283..55e4633d5442 100644
>>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>> @@ -520,7 +520,7 @@ static int vdpasim_resume(struct vdpa_device *vdpa)
>>>          int i;
>>>
>>>          mutex_lock(&vdpasim->mutex);
>>> -       vdpasim->running = true;
>>> +       vdpasim->running = (vdpasim->status & VIRTIO_CONFIG_S_FEATURES_OK) != 0;
>>>
>>>          if (vdpasim->pending_kick) {
>>>                  /* Process pending descriptors */
>>>
>>> Thanks,
>>> Stefano
>>>
>>
>> The suspend and resume operation should not be called before
>> DRIVER_OK, so maybe we should add that protection at
>> drivers/vhost/vdpa.c actually?
> 
> Yeah, I think so!
> 
> Anyway, IMHO we should at least return an error in vdpa_sim if vdpasim_suspend/resume are called before DRIVER_OK (in another patch of course).

I submitted "vdpa: suspend and resume require DRIVER_OK" to check this in vdpa.c so there
is no need to check it in the leaf drivers.

I also submitted V2 of this patch, "vdpa_sim: reset must not run".
It checks for DRIVER_OK, instead of FEATURES_OK.

- Steve