[LINUX PATCH v2 0/1] late xenstore initialization

[LINUX PATCH v2 0/1] late xenstore initialization

[Stefano Stabellini]

Hi all,

This small patch series for Linux implements support for initializing
xenstore later when booting as Dom0less kernel (HVM guest on ARM). With
this small patch series applied, it is possible to use PV drivers in
Linux when Linux is booted as dom0less kernel.

Cheers,

Stefano

Luca Miccio (1):
      xen: add support for initializing xenstore later as HVM domain

 drivers/xen/xenbus/xenbus_probe.c | 80 +++++++++++++++++++++++++++++----------
 1 file changed, 61 insertions(+), 19 deletions(-)

[LINUX PATCH v2 1/1] xen: add support for initializing xenstore later as HVM domain

[Stefano Stabellini]

From: Luca Miccio <lucmiccio@gmail.com>

When running as dom0less guest (HVM domain on ARM) the xenstore event
channel is available at domain creation but the shared xenstore
interface page only becomes available later on.

In that case, wait for a notification on the xenstore event channel,
then complete the xenstore initialization later, when the shared page
is actually available.

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
Changes in v2:
- remove XENFEAT_xenstore_late_init
---
 drivers/xen/xenbus/xenbus_probe.c | 80 +++++++++++++++++++++++--------
 1 file changed, 61 insertions(+), 19 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index fe360c33ce71..51e52e175892 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -65,6 +65,7 @@
 #include "xenbus.h"
 
 
+static int xs_init_irq;
 int xen_store_evtchn;
 EXPORT_SYMBOL_GPL(xen_store_evtchn);
 
@@ -750,6 +751,17 @@ static void xenbus_probe(void)
 {
 	xenstored_ready = 1;
 
+	if (!xen_store_interface) {
+		xen_store_interface = xen_remap(xen_store_gfn << XEN_PAGE_SHIFT,
+						XEN_PAGE_SIZE);
+		/*
+		 * Now it is safe to free the IRQ used for xenstore late
+		 * initialization. No need to unbind: it is about to be
+		 * bound again.
+		 */
+		free_irq(xs_init_irq, &xb_waitq);
+	}
+
 	/*
 	 * In the HVM case, xenbus_init() deferred its call to
 	 * xs_init() in case callbacks were not operational yet.
@@ -798,20 +810,22 @@ static int __init xenbus_probe_initcall(void)
 {
 	/*
 	 * Probe XenBus here in the XS_PV case, and also XS_HVM unless we
-	 * need to wait for the platform PCI device to come up.
+	 * need to wait for the platform PCI device to come up or
+	 * xen_store_interface is not ready.
 	 */
 	if (xen_store_domain_type == XS_PV ||
 	    (xen_store_domain_type == XS_HVM &&
-	     !xs_hvm_defer_init_for_callback()))
+	     !xs_hvm_defer_init_for_callback() &&
+	     xen_store_interface != NULL))
 		xenbus_probe();
 
 	/*
-	 * For XS_LOCAL, spawn a thread which will wait for xenstored
-	 * or a xenstore-stubdom to be started, then probe. It will be
-	 * triggered when communication starts happening, by waiting
-	 * on xb_waitq.
+	 * For XS_LOCAL or when xen_store_interface is not ready, spawn a
+	 * thread which will wait for xenstored or a xenstore-stubdom to be
+	 * started, then probe.  It will be triggered when communication
+	 * starts happening, by waiting on xb_waitq.
 	 */
-	if (xen_store_domain_type == XS_LOCAL) {
+	if (xen_store_domain_type == XS_LOCAL || xen_store_interface == NULL) {
 		struct task_struct *probe_task;
 
 		probe_task = kthread_run(xenbus_probe_thread, NULL,
@@ -907,6 +921,20 @@ static struct notifier_block xenbus_resume_nb = {
 	.notifier_call = xenbus_resume_cb,
 };
 
+static irqreturn_t xenbus_late_init(int irq, void *unused)
+{
+	int err = 0;
+	uint64_t v = 0;
+
+	err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
+	if (err || !v || !~v)
+		return IRQ_HANDLED;
+	xen_store_gfn = (unsigned long)v;
+
+	wake_up(&xb_waitq);
+	return IRQ_HANDLED;
+}
+
 static int __init xenbus_init(void)
 {
 	int err;
@@ -959,23 +987,37 @@ static int __init xenbus_init(void)
 		 *
 		 * Also recognize all bits set as an invalid value.
 		 */
-		if (!v || !~v) {
+		if (!v) {
 			err = -ENOENT;
 			goto out_error;
 		}
-		/* Avoid truncation on 32-bit. */
+		if (v == ~0ULL) {
+			err = bind_evtchn_to_irqhandler(xen_store_evtchn,
+							xenbus_late_init,
+							0, "xenstore_late_init",
+							&xb_waitq);
+			if (err < 0) {
+				pr_err("xenstore_late_init couldn't bind irq err=%d\n",
+				       err);
+				return err;
+			}
+
+			xs_init_irq = err;
+		} else {
+			/* Avoid truncation on 32-bit. */
 #if BITS_PER_LONG == 32
-		if (v > ULONG_MAX) {
-			pr_err("%s: cannot handle HVM_PARAM_STORE_PFN=%llx > ULONG_MAX\n",
-			       __func__, v);
-			err = -EINVAL;
-			goto out_error;
-		}
+			if (v > ULONG_MAX) {
+				pr_err("%s: cannot handle HVM_PARAM_STORE_PFN=%llx > ULONG_MAX\n",
+						__func__, v);
+				err = -EINVAL;
+				goto out_error;
+			}
 #endif
-		xen_store_gfn = (unsigned long)v;
-		xen_store_interface =
-			xen_remap(xen_store_gfn << XEN_PAGE_SHIFT,
-				  XEN_PAGE_SIZE);
+			xen_store_gfn = (unsigned long)v;
+			xen_store_interface =
+				xen_remap(xen_store_gfn << XEN_PAGE_SHIFT,
+					  XEN_PAGE_SIZE);
+		}
 		break;
 	default:
 		pr_warn("Xenstore state unknown\n");
-- 
2.25.1

Re: [LINUX PATCH v2 1/1] xen: add support for initializing xenstore later as HVM domain

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 3650 bytes --]

On 13.01.22 02:00, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> When running as dom0less guest (HVM domain on ARM) the xenstore event
> channel is available at domain creation but the shared xenstore
> interface page only becomes available later on.
> 
> In that case, wait for a notification on the xenstore event channel,
> then complete the xenstore initialization later, when the shared page
> is actually available.
> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
> Changes in v2:
> - remove XENFEAT_xenstore_late_init
> ---
>   drivers/xen/xenbus/xenbus_probe.c | 80 +++++++++++++++++++++++--------
>   1 file changed, 61 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
> index fe360c33ce71..51e52e175892 100644
> --- a/drivers/xen/xenbus/xenbus_probe.c
> +++ b/drivers/xen/xenbus/xenbus_probe.c
> @@ -65,6 +65,7 @@
>   #include "xenbus.h"
>   
>   
> +static int xs_init_irq;
>   int xen_store_evtchn;
>   EXPORT_SYMBOL_GPL(xen_store_evtchn);
>   
> @@ -750,6 +751,17 @@ static void xenbus_probe(void)
>   {
>   	xenstored_ready = 1;
>   
> +	if (!xen_store_interface) {
> +		xen_store_interface = xen_remap(xen_store_gfn << XEN_PAGE_SHIFT,
> +						XEN_PAGE_SIZE);
> +		/*
> +		 * Now it is safe to free the IRQ used for xenstore late
> +		 * initialization. No need to unbind: it is about to be
> +		 * bound again.
> +		 */
> +		free_irq(xs_init_irq, &xb_waitq);
> +	}
> +
>   	/*
>   	 * In the HVM case, xenbus_init() deferred its call to
>   	 * xs_init() in case callbacks were not operational yet.
> @@ -798,20 +810,22 @@ static int __init xenbus_probe_initcall(void)
>   {
>   	/*
>   	 * Probe XenBus here in the XS_PV case, and also XS_HVM unless we
> -	 * need to wait for the platform PCI device to come up.
> +	 * need to wait for the platform PCI device to come up or
> +	 * xen_store_interface is not ready.
>   	 */
>   	if (xen_store_domain_type == XS_PV ||
>   	    (xen_store_domain_type == XS_HVM &&
> -	     !xs_hvm_defer_init_for_callback()))
> +	     !xs_hvm_defer_init_for_callback() &&
> +	     xen_store_interface != NULL))
>   		xenbus_probe();
>   
>   	/*
> -	 * For XS_LOCAL, spawn a thread which will wait for xenstored
> -	 * or a xenstore-stubdom to be started, then probe. It will be
> -	 * triggered when communication starts happening, by waiting
> -	 * on xb_waitq.
> +	 * For XS_LOCAL or when xen_store_interface is not ready, spawn a
> +	 * thread which will wait for xenstored or a xenstore-stubdom to be
> +	 * started, then probe.  It will be triggered when communication
> +	 * starts happening, by waiting on xb_waitq.
>   	 */
> -	if (xen_store_domain_type == XS_LOCAL) {
> +	if (xen_store_domain_type == XS_LOCAL || xen_store_interface == NULL) {
>   		struct task_struct *probe_task;
>   
>   		probe_task = kthread_run(xenbus_probe_thread, NULL,
> @@ -907,6 +921,20 @@ static struct notifier_block xenbus_resume_nb = {
>   	.notifier_call = xenbus_resume_cb,
>   };
>   
> +static irqreturn_t xenbus_late_init(int irq, void *unused)
> +{
> +	int err = 0;
> +	uint64_t v = 0;
> +
> +	err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
> +	if (err || !v || !~v)
> +		return IRQ_HANDLED;
> +	xen_store_gfn = (unsigned long)v;
> +
> +	wake_up(&xb_waitq);
> +	return IRQ_HANDLED;
> +}
> +

Hmm, wouldn't it be easier to use a static key in the already existing
irq handler instead of switching the handler?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [LINUX PATCH v2 1/1] xen: add support for initializing xenstore later as HVM domain

[Stefano Stabellini]

On Thu, 13 Jan 2022, Juergen Gross wrote:
> > @@ -907,6 +921,20 @@ static struct notifier_block xenbus_resume_nb = {
> >   	.notifier_call = xenbus_resume_cb,
> >   };
> >   +static irqreturn_t xenbus_late_init(int irq, void *unused)
> > +{
> > +	int err = 0;
> > +	uint64_t v = 0;
> > +
> > +	err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
> > +	if (err || !v || !~v)
> > +		return IRQ_HANDLED;
> > +	xen_store_gfn = (unsigned long)v;
> > +
> > +	wake_up(&xb_waitq);
> > +	return IRQ_HANDLED;
> > +}
> > +
> 
> Hmm, wouldn't it be easier to use a static key in the already existing
> irq handler instead of switching the handler?

I did some prototyping and it is certainly not going to be "easier" :-)

- xenbus_irq is setup by xb_init_comms, but xb_init_comms cannot be
  re-used as-is because it assumes xen_store_interface to be != NULL
- also, it is too early to start xenbus_thread at that point

So it looks like we shouldn't call xb_init_comms from xenbus_init
because it would increase the number of "if" statements in xb_init_comms
quiet a bit. 

An alternative would be to leave xb_init_comms unmodified, but reuse the
same interrupt handler (wake_waiting) and xenbus_irq. However, that
doesn't work either because rebind_evtchn_irq fails when it is called
later on from xb_init_comms. (I haven't investigated why.) Also,
xenbus_irq could be allocated as zero, so the existing check in
xb_init_comms also fails. 

To give you a concrete idea, after several tries I managed to get a
dirty patch that works but I don't think the changes to xb_init_comms
are actually correct.

In conclusion I think it is best to keep the current approach.



diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c
index e5fda0256feb..bad87c3a9f72 100644
--- a/drivers/xen/xenbus/xenbus_comms.c
+++ b/drivers/xen/xenbus/xenbus_comms.c
@@ -54,15 +54,9 @@ DEFINE_MUTEX(xb_write_mutex);
 /* Protect xenbus reader thread against save/restore. */
 DEFINE_MUTEX(xs_response_mutex);
 
-static int xenbus_irq;
+int xenbus_irq = -1;
 static struct task_struct *xenbus_task;
 
-static irqreturn_t wake_waiting(int irq, void *unused)
-{
-	wake_up(&xb_waitq);
-	return IRQ_HANDLED;
-}
-
 static int check_indexes(XENSTORE_RING_IDX cons, XENSTORE_RING_IDX prod)
 {
 	return ((prod - cons) <= XENSTORE_RING_SIZE);
@@ -435,6 +429,7 @@ static int xenbus_thread(void *unused)
 /**
  * xb_init_comms - Set up interrupt handler off store event channel.
  */
+extern irqreturn_t wake_waiting(int irq, void *unused);
 int xb_init_comms(void)
 {
 	struct xenstore_domain_interface *intf = xen_store_interface;
@@ -451,10 +446,10 @@ int xb_init_comms(void)
 			intf->rsp_cons = intf->rsp_prod;
 	}
 
-	if (xenbus_irq) {
+	if (xenbus_irq > 0) {
 		/* Already have an irq; assume we're resuming */
 		rebind_evtchn_irq(xen_store_evtchn, xenbus_irq);
-	} else {
+	} else if (xenbus_irq < 0) {
 		int err;
 
 		err = bind_evtchn_to_irqhandler(xen_store_evtchn, wake_waiting,
@@ -465,13 +460,13 @@ int xb_init_comms(void)
 		}
 
 		xenbus_irq = err;
+	}
 
-		if (!xenbus_task) {
-			xenbus_task = kthread_run(xenbus_thread, NULL,
-						  "xenbus");
-			if (IS_ERR(xenbus_task))
-				return PTR_ERR(xenbus_task);
-		}
+	if (!xenbus_task) {
+		xenbus_task = kthread_run(xenbus_thread, NULL,
+				"xenbus");
+		if (IS_ERR(xenbus_task))
+			return PTR_ERR(xenbus_task);
 	}
 
 	return 0;
diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index fe360c33ce71..ad8d640b75d2 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -65,6 +65,7 @@
 #include "xenbus.h"
 
 
+extern int xenbus_irq;
 int xen_store_evtchn;
 EXPORT_SYMBOL_GPL(xen_store_evtchn);
 
@@ -750,6 +751,11 @@ static void xenbus_probe(void)
 {
 	xenstored_ready = 1;
 
+	if (!xen_store_interface) {
+		xen_store_interface = xen_remap(xen_store_gfn << XEN_PAGE_SHIFT,
+						XEN_PAGE_SIZE);
+	}
+
 	/*
 	 * In the HVM case, xenbus_init() deferred its call to
 	 * xs_init() in case callbacks were not operational yet.
@@ -798,20 +804,22 @@ static int __init xenbus_probe_initcall(void)
 {
 	/*
 	 * Probe XenBus here in the XS_PV case, and also XS_HVM unless we
-	 * need to wait for the platform PCI device to come up.
+	 * need to wait for the platform PCI device to come up or
+	 * xen_store_interface is not ready.
 	 */
 	if (xen_store_domain_type == XS_PV ||
 	    (xen_store_domain_type == XS_HVM &&
-	     !xs_hvm_defer_init_for_callback()))
+	     !xs_hvm_defer_init_for_callback() &&
+	     xen_store_interface != NULL))
 		xenbus_probe();
 
 	/*
-	 * For XS_LOCAL, spawn a thread which will wait for xenstored
-	 * or a xenstore-stubdom to be started, then probe. It will be
-	 * triggered when communication starts happening, by waiting
-	 * on xb_waitq.
+	 * For XS_LOCAL or when xen_store_interface is not ready, spawn a
+	 * thread which will wait for xenstored or a xenstore-stubdom to be
+	 * started, then probe.  It will be triggered when communication
+	 * starts happening, by waiting on xb_waitq.
 	 */
-	if (xen_store_domain_type == XS_LOCAL) {
+	if (xen_store_domain_type == XS_LOCAL || xen_store_interface == NULL) {
 		struct task_struct *probe_task;
 
 		probe_task = kthread_run(xenbus_probe_thread, NULL,
@@ -907,6 +915,22 @@ static struct notifier_block xenbus_resume_nb = {
 	.notifier_call = xenbus_resume_cb,
 };
 
+irqreturn_t wake_waiting(int irq, void *unused)
+{
+	int err = 0;
+	uint64_t v = 0;
+
+	if (!xen_store_gfn) {
+		err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
+		if (err || !v || !~v)
+			return IRQ_HANDLED;
+		xen_store_gfn = (unsigned long)v;
+	}
+
+	wake_up(&xb_waitq);
+	return IRQ_HANDLED;
+}
+
 static int __init xenbus_init(void)
 {
 	int err;
@@ -959,23 +983,37 @@ static int __init xenbus_init(void)
 		 *
 		 * Also recognize all bits set as an invalid value.
 		 */
-		if (!v || !~v) {
+		if (!v) {
 			err = -ENOENT;
 			goto out_error;
 		}
-		/* Avoid truncation on 32-bit. */
+		if (v == ~0ULL) {
+			err = bind_evtchn_to_irqhandler(xen_store_evtchn,
+							wake_waiting,
+							0, "xenbus",
+							&xb_waitq);
+			if (err < 0) {
+				pr_err("xenstore_late_init couldn't bind irq err=%d\n",
+				       err);
+				return err;
+			}
+
+			xenbus_irq = err;
+		} else {
+			/* Avoid truncation on 32-bit. */
 #if BITS_PER_LONG == 32
-		if (v > ULONG_MAX) {
-			pr_err("%s: cannot handle HVM_PARAM_STORE_PFN=%llx > ULONG_MAX\n",
-			       __func__, v);
-			err = -EINVAL;
-			goto out_error;
-		}
+			if (v > ULONG_MAX) {
+				pr_err("%s: cannot handle HVM_PARAM_STORE_PFN=%llx > ULONG_MAX\n",
+						__func__, v);
+				err = -EINVAL;
+				goto out_error;
+			}
 #endif
-		xen_store_gfn = (unsigned long)v;
-		xen_store_interface =
-			xen_remap(xen_store_gfn << XEN_PAGE_SHIFT,
-				  XEN_PAGE_SIZE);
+			xen_store_gfn = (unsigned long)v;
+			xen_store_interface =
+				xen_remap(xen_store_gfn << XEN_PAGE_SHIFT,
+					  XEN_PAGE_SIZE);
+		}
 		break;
 	default:
 		pr_warn("Xenstore state unknown\n");