Messages in this thread Patch in this message | | | Date | Thu, 16 May 2024 09:25:11 +0800 | Subject | Re: [PATCH] drivers/xen: Improve the late XenStore init protocol | From | Henry Wang <> |
| |
Hi Stefano,
On 5/16/2024 6:30 AM, Stefano Stabellini wrote: > On Wed, 15 May 2024, Henry Wang wrote: >> Currently, the late XenStore init protocol is only triggered properly >> for the case that HVM_PARAM_STORE_PFN is ~0ULL (invalid). For the >> case that XenStore interface is allocated but not ready (the connection >> status is not XENSTORE_CONNECTED), Linux should also wait until the >> XenStore is set up properly. >> >> Introduce a macro to describe the XenStore interface is ready, use >> it in xenbus_probe_initcall() and xenbus_probe() to select the code >> path of doing the late XenStore init protocol or not. >> >> Take the opportunity to enhance the check of the allocated XenStore >> interface can be properly mapped, and return error early if the >> memremap() fails. >> >> Signed-off-by: Henry Wang <xin.wang2@amd.com> >> Signed-off-by: Michal Orzel <michal.orzel@amd.com> > Please add a Fixes: tag
Sure. Will do.
>> --- >> drivers/xen/xenbus/xenbus_probe.c | 21 ++++++++++++++++----- >> 1 file changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c >> index 3205e5d724c8..8aec0ed1d047 100644 >> --- a/drivers/xen/xenbus/xenbus_probe.c >> +++ b/drivers/xen/xenbus/xenbus_probe.c >> @@ -72,6 +72,10 @@ EXPORT_SYMBOL_GPL(xen_store_evtchn); >> struct xenstore_domain_interface *xen_store_interface; >> EXPORT_SYMBOL_GPL(xen_store_interface); >> >> +#define XS_INTERFACE_READY \ >> + ((xen_store_interface != NULL) && \ >> + (xen_store_interface->connection == XENSTORE_CONNECTED)) >> + >> 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 << XEN_PAGE_SHIFT, XEN_PAGE_SIZE, MEMREMAP_WB); @@ -937,6 +939,8 @@ static irqreturn_t xenbus_late_init(int irq, void *unused) int err; uint64_t v = 0;
+ xs_late_init = true; + err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); if (err || !v || !~v) return IRQ_HANDLED; >> + 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 >> @@ -822,7 +827,7 @@ static int __init xenbus_probe_initcall(void) >> if (xen_store_domain_type == XS_PV || >> (xen_store_domain_type == XS_HVM && >> !xs_hvm_defer_init_for_callback() && >> - xen_store_interface != NULL)) >> + XS_INTERFACE_READY)) >> xenbus_probe(); >> >> /* >> @@ -831,7 +836,7 @@ static int __init xenbus_probe_initcall(void) >> * started, then probe. It will be triggered when communication >> * starts happening, by waiting on xb_waitq. >> */ >> - if (xen_store_domain_type == XS_LOCAL || xen_store_interface == NULL) { >> + if (xen_store_domain_type == XS_LOCAL || !XS_INTERFACE_READY) { >> struct task_struct *probe_task; >> >> probe_task = kthread_run(xenbus_probe_thread, NULL, >> @@ -1014,6 +1019,12 @@ static int __init xenbus_init(void) >> xen_store_interface = >> memremap(xen_store_gfn << XEN_PAGE_SHIFT, >> XEN_PAGE_SIZE, MEMREMAP_WB); >> + if (!xen_store_interface) { >> + pr_err("%s: cannot map HVM_PARAM_STORE_PFN=%llx\n", >> + __func__, v); >> + err = -ENOMEM; > I think this should -EINVAL
Will change.
Kind regards, Henry
>> + goto out_error; >> + } >> if (xen_store_interface->connection != XENSTORE_CONNECTED) >> wait = true; >> } >> -- >> 2.34.1 >>
| |