lkml.org 
[lkml]   [2024]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] drivers/xen: Improve the late XenStore init protocol
From
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

\
 
 \ /
  Last update: 2024-05-27 18:30    [W:0.033 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site