Received-SPF: pass (google.com: domain of linux-kernel+bounces-181671-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB03.amd.com; pr=C
Message-ID: <1539e7ab-f27f-4cea-bb79-4c341bb3c69c@amd.com>
Date: Fri, 17 May 2024 09:17:43 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] drivers/xen: Improve the late XenStore init protocol
To: Stefano Stabellini <sstabellini@kernel.org>
CC: <xen-devel@lists.xenproject.org>, <linux-kernel@vger.kernel.org>, "Juergen
 Gross" <jgross@suse.com>, Oleksandr Tyshchenko
	<oleksandr_tyshchenko@epam.com>, Michal Orzel <michal.orzel@amd.com>
References: <20240515014330.1044617-1-xin.wang2@amd.com>
 <alpine.DEB.2.22.394.2405151524270.2544314@ubuntu-linux-20-04-desktop>
 <028f29be-0393-4a57-83e2-ea27fe0320d5@amd.com>
 <alpine.DEB.2.22.394.2405161743170.2544314@ubuntu-linux-20-04-desktop>
Content-Language: en-US
From: Henry Wang <xin.wang2@amd.com>
In-Reply-To: <alpine.DEB.2.22.394.2405161743170.2544314@ubuntu-linux-20-04-desktop>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Received-SPF: None (SATLEXMB03.amd.com: xin.wang2@amd.com does not designate
 permitted sender hosts)
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 May 2024 01:17:46.7839
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 2b122ba2-0dff-4fa6-5dea-08dc760f293e
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB03.amd.com]
X-MS-Exchange-CrossTenant-AuthSource:
	BN3PEPF0000B074.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR12MB5599

Hi Stefano,

On 5/17/2024 8:52 AM, Stefano Stabellini wrote:
> On Thu, 16 May 2024, Henry Wang wrote:
>>>>    enum xenstore_init xen_store_domain_type;
>>>>    EXPORT_SYMBOL_GPL(xen_store_domain_type);
>>>>    @@ -751,9 +755,10 @@ static void xenbus_probe(void)
>>>>    {
>>>>    	xenstored_ready = 1;
>>>>    -	if (!xen_store_interface) {
>>>> -		xen_store_interface = memremap(xen_store_gfn <<
>>>> XEN_PAGE_SHIFT,
>>>> -					       XEN_PAGE_SIZE, MEMREMAP_WB);
>>>> +	if (!xen_store_interface || XS_INTERFACE_READY) {
>>>> +		if (!xen_store_interface)
>>> These two nested if's don't make sense to me. If XS_INTERFACE_READY
>>> succeeds, it means that  ((xen_store_interface != NULL) &&
>>> (xen_store_interface->connection == XENSTORE_CONNECTED)).
>>>
>>> So it is not possible that xen_store_interface == NULL immediately
>>> after. Right?
>> I think this is because we want to free the irq for the late init case,
>> otherwise the init-dom0less will fail. For the xenstore PFN allocated case,
>> the connection is already set to CONNECTED when we execute init-dom0less. But
>> I agree with you, would below diff makes more sense to you?
>>
>> diff --git a/drivers/xen/xenbus/xenbus_probe.c
>> b/drivers/xen/xenbus/xenbus_probe.c
>> index 8aec0ed1d047..b8005b651a29 100644
>> --- a/drivers/xen/xenbus/xenbus_probe.c
>> +++ b/drivers/xen/xenbus/xenbus_probe.c
>> @@ -76,6 +76,8 @@ EXPORT_SYMBOL_GPL(xen_store_interface);
>>          ((xen_store_interface != NULL) && \
>>           (xen_store_interface->connection == XENSTORE_CONNECTED))
>>
>> +static bool xs_late_init = false;
>> +
>>   enum xenstore_init xen_store_domain_type;
>>   EXPORT_SYMBOL_GPL(xen_store_domain_type);
>>
>> @@ -755,7 +757,7 @@ static void xenbus_probe(void)
>>   {
>>          xenstored_ready = 1;
>>
>> -       if (!xen_store_interface || XS_INTERFACE_READY) {
>> +       if (xs_late_init) {
>>                  if (!xen_store_interface)
>>                          xen_store_interface = memremap(xen_store_gfn <<
>
> I would just remove the outer 'if' and do this:
>
>
> 	if (!xen_store_interface)
> 		xen_store_interface = memremap(xen_store_gfn << XEN_PAGE_SHIFT,
> 				XEN_PAGE_SIZE, MEMREMAP_WB);
> 	/*
> 	 * Now it is safe to free the IRQ used for xenstore late
> 	 * initialization. No need to unbind: it is about to be
> 	 * bound again from xb_init_comms. Note that calling
> 	 * unbind_from_irqhandler now would result in xen_evtchn_close()
> 	 * being called and the event channel not being enabled again
> 	 * afterwards, resulting in missed event notifications.
> 	 */
> 	if (xs_init_irq > 0)
> 		free_irq(xs_init_irq, &xb_waitq);
>
>
> I think this should work fine in all cases.

Thanks. I followed your suggestion in v2.

>   I am unsure if
> xs_init_irq==0 is possible valid value for xs_init_irq. If it is not,
> then we are fine. If 0 is a possible valid irq number, then we should
> initialize xs_init_irq to -1, and here check for xs_init_irq >= 0.

Yeah the xs_init_irq==0 is a valid value. I followed your latter comment 
to init it to -1 and check it >=0.

Kind regards,
Henry