Received-SPF: pass (google.com: domain of linux-kernel+bounces-55648-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <6cef4f69-e19d-4741-9cff-a9485dd58d89@intel.com>
Date: Tue, 6 Feb 2024 13:45:22 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 07/17] vfio/pci: Preserve per-interrupt contexts
Content-Language: en-US
To: Alex Williamson <alex.williamson@redhat.com>
CC: <jgg@nvidia.com>, <yishaih@nvidia.com>,
	<shameerali.kolothum.thodi@huawei.com>, <kevin.tian@intel.com>,
	<kvm@vger.kernel.org>, <dave.jiang@intel.com>, <ashok.raj@intel.com>,
	<linux-kernel@vger.kernel.org>, <patches@lists.linux.dev>
References: <cover.1706849424.git.reinette.chatre@intel.com>
 <d6e32e0e7adaf61da39fb6cd2863298b15a2663e.1706849424.git.reinette.chatre@intel.com>
 <20240205153509.333c2c95.alex.williamson@redhat.com>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240205153509.333c2c95.alex.williamson@redhat.com>
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?QlFMbUVBVndvR1B0S3p2YVRTZUJmRm1HZDZCM0I1YlRpZU5GbFdPQVZabGpn?=
 =?utf-8?B?UnlMc3hmek1Cd3Y5VS9sb0JoNHFuV0ZUdDJOOWNMOHJHVkovSjlheVZtRktU?=
 =?utf-8?B?TTFGa3lwY0FrKzFpVlZEUjcyNk5SNGE5S2J1WXkvVmtIUjVxdlBTeVFUT09y?=
 =?utf-8?B?d0JUSXYwRmlZY0Y0K1MzMVFMWUdaZ3NSaVREVWR1VDhvRVRmQ0VtOUhUdnBv?=
 =?utf-8?B?eHZNQjhiemo2dGtJRWcyOUhESE14QVNNcFNKUjZHdTNLdDVBNE5YMEJCWFJZ?=
 =?utf-8?B?UktyV1h3Ynk3Y2w3aWp5L3lTU0VMUWRmZ0k5Z3NDUGdDZDBwa0tnaTZPbE56?=
 =?utf-8?B?NmdqWlgwQWczaUR3UzVLVXJweDhxMERNL08vek53SUpuRkx0MlVHeXZhYVlN?=
 =?utf-8?B?MmxyMnU3aGdaZWJ6ZWJlSDZnRlo2TmhtbEcvTSs3emFXdGNhUFVVb25MRkFq?=
 =?utf-8?B?S05zUFhIVmdURUExdG5QRmljV0tKTVkyMU5KK2NRR2FEdU9PSys5VDliYUNR?=
 =?utf-8?B?d3VXckRHQmhsVXNsREo2OUVzL08xZHgyTlY2Z1FFQnVEY1dMbWFtdmZsTlRs?=
 =?utf-8?B?dHM0clFSbXZKY1Q0dkUxMEVSQ0kzN2VSQTk1SEJlTGFmNXQxdGNOaFdLYzE3?=
 =?utf-8?B?cTlUaSt2Zm9RVkszSFJnOU5BdThRek5lVmZDTE9sZWIxSXJSUXdkTUlFZmJL?=
 =?utf-8?B?WXVDYWhheE40aUVLL3ZkSzBibFVBWEt2Y29HRVR6cGM3aVRrWjRNSm9TVUFB?=
 =?utf-8?B?UHpBZ3dtNG9xdE1oVVByT1EzV0VrZ3VldGRNQkxjSzBKU0tpK1ZQcCtOaGpD?=
 =?utf-8?B?ZjBReW9qWm45OFREV0dTaVduT2hVU3g5a0hJR1NxL2pxZ0pnSlc2eUU3SHp1?=
 =?utf-8?B?MUQ4aGc2bmFxREdEcWZLcUcyejVPQXhxem5icmRBbHhUcERHSVBiVmIrd2ww?=
 =?utf-8?B?clh2S2pPQ09vTlprOThoS0dFMEM2ZUtkdjhLSnVBNk1EZlJkTFFaQVAzU2lY?=
 =?utf-8?B?ZUc5OTgwd1hzL0RXUE5mN1o5d1laSlZvcFozRHhIcEtUUS8wNEhKWG1QblZX?=
 =?utf-8?B?Ym0vYmNkWWZBL3RrTmwzVllTUUV3TmpJazROVkpUVTJXK3dZUDl0WDhHVTdi?=
 =?utf-8?B?KzNKL0YvVDNmU1hKT3JjdENUS2tRZEV2bTdJNVc1MndhVkJ1dWU2eFp0WlRu?=
 =?utf-8?B?Sng2THdOOHIxVDVOTWZhUEw5S053VDJnWkI5UEhyKzhvNXV4QWUydTlReFlq?=
 =?utf-8?B?ekl0VWMxeGR4ZG1vVlFRc01OV09ya3hiQk5ib3RXbFhqeTgrKzZqd20wbXJR?=
 =?utf-8?B?RTh2Snc0Y0tWMERDbnZOYlo3TUNqN1hzOElUR3JlZWEzVUFqS00rUExNWDlE?=
 =?utf-8?B?dTVZN1VIU3lCcDV5MHk0SXZheUlUQkVTZ29BVUdJVmJXZ0gyZWgyT2ZwdlF2?=
 =?utf-8?B?dTBoZU1WQ2xoa0tKL0h5TWtzT05kYTRtSTJIcUU1SG5VQ0J4RjVsU003eS9n?=
 =?utf-8?B?Nm9oVS9hc0EvaGFwR0dqZ0libmFnaUhnN2pOaStCN2ZGbEtnb1htSHRSbk9J?=
 =?utf-8?B?ellRNkJtS0d3OHpQT25Dajc4OW5adTdpbTZUb1hyVUFXMG0zMk85WGRTdElF?=
 =?utf-8?B?b2lhb2pvaUJqTi9aaVpBUUVlcHJkbHdac3BISzV6L2NTcjhLSzNCc2srSE1a?=
 =?utf-8?B?cWtFOUNwQXV0N2ZZQUhGUE0zWG0zb3J2cVgyQm1SVjBKeE1QVENEbDdUVTRT?=
 =?utf-8?B?L2dadm5xRWVXMjJMUm9icWlxRzVvWVQ5M0lSVmFwQjNKN2VFbXB4TlRnOWdh?=
 =?utf-8?B?bE9UUEI4MmhsNnhpOWQxTzhpWVJjaDFnQzE3eEpEY2VuNml5bkZZQTduei8w?=
 =?utf-8?B?YzVCQ09UUkY0bGh6d0lYQVJmei9Ba0U0MHN2dXlJMnMvTXY2Wmt6c2k3cmdm?=
 =?utf-8?B?L3RMcXhIR0pUTkZ3enExV2FjUW1HWVRkQ2JOTTVUdmxHNjYybkx3bDdJMEF3?=
 =?utf-8?B?eGx6SjVKVjd1dmtpdFBvUUhsbWEvV2xMREtkN0ljMjhIT2VXdlhwV0VJK1NX?=
 =?utf-8?B?ZEcxWVJ3clJ3RmZGZllic2M0WDlOTWFFeUJPalQraHZ5ZlVndStEdDZESnF0?=
 =?utf-8?B?N1pRbEx0ZzZNUWg2Qzc3VEJpZ2NqOVMwQmNHMlRMSHpxT0czbENlSWZTVU90?=
 =?utf-8?B?SVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 38c80395-8abf-48f2-b22c-08dc275cec0d
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2024 21:45:23.1753
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1loLP4r65neqPcaNV99jUDjmV8RWFRvlH/PJVtVNaIHMCeoglXo/rbnkaEncCbuMRC9MyAsUx34hhDIit633TKn1Rcasea+V1gLfoZXmkTs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7367
X-OriginatorOrg: intel.com

Hi Alex,

On 2/5/2024 2:35 PM, Alex Williamson wrote:
> On Thu,  1 Feb 2024 20:57:01 -0800
> Reinette Chatre <reinette.chatre@intel.com> wrote:

..

>> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
>> index 31f73c70fcd2..7ca2b983b66e 100644
>> --- a/drivers/vfio/pci/vfio_pci_intrs.c
>> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
>> @@ -427,7 +427,7 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
>>  
>>  	ctx = vfio_irq_ctx_get(vdev, vector);
>>  
>> -	if (ctx) {
>> +	if (ctx && ctx->trigger) {
>>  		irq_bypass_unregister_producer(&ctx->producer);
>>  		irq = pci_irq_vector(pdev, vector);
>>  		cmd = vfio_pci_memory_lock_and_enable(vdev);
>> @@ -435,8 +435,9 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
>>  		vfio_pci_memory_unlock_and_restore(vdev, cmd);
>>  		/* Interrupt stays allocated, will be freed at MSI-X disable. */
>>  		kfree(ctx->name);
>> +		ctx->name = NULL;
> 
> Setting ctx->name = NULL is not strictly necessary and does not match
> the INTx code that we're claiming to try to emulate.  ctx->name is only
> tested immediately after allocation below, otherwise it can be inferred
> from ctx->trigger.  Thanks,

This all matches my understanding. I added ctx->name = NULL after every kfree(ctx->name)
(see below for confirmation of other instance). You are correct that the flow
infers validity of ctx->name from ctx->trigger. My motivation for
adding ctx->name = NULL is that, since the interrupt context persists, this
change ensures that there will be no pointer that points to freed memory. I
am not comfortable leaving pointers to freed memory around.

>>  		eventfd_ctx_put(ctx->trigger);
>> -		vfio_irq_ctx_free(vdev, ctx, vector);
>> +		ctx->trigger = NULL;
>>  	}
>>  
>>  	if (fd < 0)
>> @@ -449,16 +450,17 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
>>  			return irq;
>>  	}
>>  
>> -	ctx = vfio_irq_ctx_alloc(vdev, vector);
>> -	if (!ctx)
>> -		return -ENOMEM;
>> +	/* Per-interrupt context remain allocated. */
>> +	if (!ctx) {
>> +		ctx = vfio_irq_ctx_alloc(vdev, vector);
>> +		if (!ctx)
>> +			return -ENOMEM;
>> +	}
>>  
>>  	ctx->name = kasprintf(GFP_KERNEL_ACCOUNT, "vfio-msi%s[%d](%s)",
>>  			      msix ? "x" : "", vector, pci_name(pdev));
>> -	if (!ctx->name) {
>> -		ret = -ENOMEM;
>> -		goto out_free_ctx;
>> -	}
>> +	if (!ctx->name)
>> +		return -ENOMEM;
>>  
>>  	trigger = eventfd_ctx_fdget(fd);
>>  	if (IS_ERR(trigger)) {
>> @@ -502,8 +504,7 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
>>  	eventfd_ctx_put(trigger);
>>  out_free_name:
>>  	kfree(ctx->name);
>> -out_free_ctx:
>> -	vfio_irq_ctx_free(vdev, ctx, vector);
>> +	ctx->name = NULL;

Here is the other one.

Reinette