[PATCH LINUX 0/2] dom0less + PV drivers

[PATCH LINUX 0/2] dom0less + PV drivers

[Stefano Stabellini]

Hi all,

This small Linux patch series implements support for initializing
xenstore later, if not immediately available at boot time. It enables
dom0less + PV drivers support.


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

Stefano Stabellini (1):
      xen: sync xs_wire.h header with upstream xen

 drivers/xen/xenbus/xenbus_probe.c  | 91 +++++++++++++++++++++++++++++---------
 include/xen/interface/io/xs_wire.h | 34 ++++++++++++--
 2 files changed, 102 insertions(+), 23 deletions(-)

[PATCH v4 1/2] xen: sync xs_wire.h header with upstream xen

[Stefano Stabellini]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>

Sync the xs_wire.h header file in Linux with the one in Xen.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 include/xen/interface/io/xs_wire.h | 34 +++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/include/xen/interface/io/xs_wire.h b/include/xen/interface/io/xs_wire.h
index d40a44f09b16..04dca77abc45 100644
--- a/include/xen/interface/io/xs_wire.h
+++ b/include/xen/interface/io/xs_wire.h
@@ -10,7 +10,8 @@
 
 enum xsd_sockmsg_type
 {
-    XS_DEBUG,
+    XS_CONTROL,
+#define XS_DEBUG XS_CONTROL
     XS_DIRECTORY,
     XS_READ,
     XS_GET_PERMS,
@@ -30,8 +31,13 @@ enum xsd_sockmsg_type
     XS_IS_DOMAIN_INTRODUCED,
     XS_RESUME,
     XS_SET_TARGET,
-    XS_RESTRICT,
-    XS_RESET_WATCHES,
+    /* XS_RESTRICT has been removed */
+    XS_RESET_WATCHES = XS_SET_TARGET + 2,
+    XS_DIRECTORY_PART,
+
+    XS_TYPE_COUNT,      /* Number of valid types. */
+
+    XS_INVALID = 0xffff /* Guaranteed to remain an invalid type */
 };
 
 #define XS_WRITE_NONE "NONE"
@@ -87,9 +93,31 @@ struct xenstore_domain_interface {
     char rsp[XENSTORE_RING_SIZE]; /* Replies and async watch events. */
     XENSTORE_RING_IDX req_cons, req_prod;
     XENSTORE_RING_IDX rsp_cons, rsp_prod;
+    uint32_t server_features; /* Bitmap of features supported by the server */
+    uint32_t connection;
+    uint32_t error;
 };
 
 /* Violating this is very bad.  See docs/misc/xenstore.txt. */
 #define XENSTORE_PAYLOAD_MAX 4096
 
+/* Violating these just gets you an error back */
+#define XENSTORE_ABS_PATH_MAX 3072
+#define XENSTORE_REL_PATH_MAX 2048
+
+/* The ability to reconnect a ring */
+#define XENSTORE_SERVER_FEATURE_RECONNECTION 1
+/* The presence of the "error" field in the ring page */
+#define XENSTORE_SERVER_FEATURE_ERROR        2
+
+/* Valid values for the connection field */
+#define XENSTORE_CONNECTED 0 /* the steady-state */
+#define XENSTORE_RECONNECT 1 /* guest has initiated a reconnect */
+
+/* Valid values for the error field */
+#define XENSTORE_ERROR_NONE    0 /* No error */
+#define XENSTORE_ERROR_COMM    1 /* Communication problem */
+#define XENSTORE_ERROR_RINGIDX 2 /* Invalid ring index */
+#define XENSTORE_ERROR_PROTO   3 /* Protocol violation (payload too long) */
+
 #endif /* _XS_WIRE_H */
-- 
2.25.1

Re: [PATCH v4 1/2] xen: sync xs_wire.h header with upstream xen

[Luca Fancellu]

Hi Stefano,

> On 5 May 2022, at 01:23, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> Sync the xs_wire.h header file in Linux with the one in Xen.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
> include/xen/interface/io/xs_wire.h | 34 +++++++++++++++++++++++++++---
> 1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/include/xen/interface/io/xs_wire.h b/include/xen/interface/io/xs_wire.h
> index d40a44f09b16..04dca77abc45 100644
> --- a/include/xen/interface/io/xs_wire.h
> +++ b/include/xen/interface/io/xs_wire.h
> @@ -10,7 +10,8 @@
> 
> enum xsd_sockmsg_type
> {
> -    XS_DEBUG,
> +    XS_CONTROL,
> +#define XS_DEBUG XS_CONTROL
>     XS_DIRECTORY,
>     XS_READ,
>     XS_GET_PERMS,
> @@ -30,8 +31,13 @@ enum xsd_sockmsg_type
>     XS_IS_DOMAIN_INTRODUCED,
>     XS_RESUME,
>     XS_SET_TARGET,
> -    XS_RESTRICT,
> -    XS_RESET_WATCHES,
> +    /* XS_RESTRICT has been removed */
> +    XS_RESET_WATCHES = XS_SET_TARGET + 2,
> +    XS_DIRECTORY_PART,
> +
> +    XS_TYPE_COUNT,      /* Number of valid types. */
> +
> +    XS_INVALID = 0xffff /* Guaranteed to remain an invalid type */
> };
> 

I’ve checked and seems that here there is this missing?

@@ -59,8 +71,10 @@ static struct xsd_errors xsd_errors[] __attribute__((unused)) = {
     XSD_ERROR(EROFS),
     XSD_ERROR(EBUSY),
     XSD_ERROR(EAGAIN),
-    XSD_ERROR(EISCONN)
+    XSD_ERROR(EISCONN),
+    XSD_ERROR(E2BIG)
 };

> #define XS_WRITE_NONE "NONE"
> @@ -87,9 +93,31 @@ struct xenstore_domain_interface {
>     char rsp[XENSTORE_RING_SIZE]; /* Replies and async watch events. */
>     XENSTORE_RING_IDX req_cons, req_prod;
>     XENSTORE_RING_IDX rsp_cons, rsp_prod;
> +    uint32_t server_features; /* Bitmap of features supported by the server */
> +    uint32_t connection;
> +    uint32_t error;
> };
> 
> /* Violating this is very bad.  See docs/misc/xenstore.txt. */
> #define XENSTORE_PAYLOAD_MAX 4096
> 
> +/* Violating these just gets you an error back */
> +#define XENSTORE_ABS_PATH_MAX 3072
> +#define XENSTORE_REL_PATH_MAX 2048
> +
> +/* The ability to reconnect a ring */
> +#define XENSTORE_SERVER_FEATURE_RECONNECTION 1
> +/* The presence of the "error" field in the ring page */
> +#define XENSTORE_SERVER_FEATURE_ERROR        2
> +
> +/* Valid values for the connection field */
> +#define XENSTORE_CONNECTED 0 /* the steady-state */
> +#define XENSTORE_RECONNECT 1 /* guest has initiated a reconnect */
> +
> +/* Valid values for the error field */
> +#define XENSTORE_ERROR_NONE    0 /* No error */
> +#define XENSTORE_ERROR_COMM    1 /* Communication problem */
> +#define XENSTORE_ERROR_RINGIDX 2 /* Invalid ring index */
> +#define XENSTORE_ERROR_PROTO   3 /* Protocol violation (payload too long) */
> +
> #endif /* _XS_WIRE_H */
> -- 
> 2.25.1
> 
> 

[PATCH v4 2/2] 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.

The xenstore page has few extra field. Add them to the shared struct.
One of the field is "connection", when the connection is ready, it is
zero. If the connection is not-zero, wait for a notification.

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
Changes in v4:
- improve in-code comments
- move header sync to separate patch
- use XENSTORE_CONNECTED

Changes in v3:
- check for the connection field, if it is not zero, wait for event

Changes in v2:
- remove XENFEAT_xenstore_late_init
---
 drivers/xen/xenbus/xenbus_probe.c | 91 ++++++++++++++++++++++++-------
 1 file changed, 71 insertions(+), 20 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index fe360c33ce71..0a785d5e3e40 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,20 @@ 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 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.
+		 */
+		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 +813,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,10 +924,25 @@ 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;
 	uint64_t v = 0;
+	bool wait = false;
 	xen_store_domain_type = XS_UNKNOWN;
 
 	if (!xen_domain())
@@ -957,25 +989,44 @@ static int __init xenbus_init(void)
 		 * been properly initialized. Instead of attempting to map a
 		 * wrong guest physical address return error.
 		 *
-		 * Also recognize all bits set as an invalid value.
+		 * Also recognize all bits set as an invalid/uninitialized value.
 		 */
-		if (!v || !~v) {
+		if (!v) {
 			err = -ENOENT;
 			goto out_error;
 		}
-		/* Avoid truncation on 32-bit. */
+		if (v == ~0ULL) {
+			wait = true;
+		} 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);
+			if (xen_store_interface->connection != XENSTORE_CONNECTED)
+				wait = true;
+		}
+		if (wait) {
+			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;
+		}
 		break;
 	default:
 		pr_warn("Xenstore state unknown\n");
-- 
2.25.1

Re: [PATCH v4 2/2] xen: add support for initializing xenstore later as HVM domain

[Michal Orzel]

Hi Luca,

On 05.05.2022 02:23, 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.
> 
> The xenstore page has few extra field. Add them to the shared struct.
> One of the field is "connection", when the connection is ready, it is
> zero. If the connection is not-zero, wait for a notification.
> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
> Changes in v4:
> - improve in-code comments
> - move header sync to separate patch
> - use XENSTORE_CONNECTED
> 
> Changes in v3:
> - check for the connection field, if it is not zero, wait for event
> 
> Changes in v2:
> - remove XENFEAT_xenstore_late_init
> ---
>  drivers/xen/xenbus/xenbus_probe.c | 91 ++++++++++++++++++++++++-------
>  1 file changed, 71 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
> index fe360c33ce71..0a785d5e3e40 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,20 @@ 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 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.
> +		 */
> +		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 +813,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,10 +924,25 @@ 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;
No need to set up initial value as it is being reassigned without using the initial value.

> +	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;
>  	uint64_t v = 0;
> +	bool wait = false;
>  	xen_store_domain_type = XS_UNKNOWN;
>  
>  	if (!xen_domain())
> @@ -957,25 +989,44 @@ static int __init xenbus_init(void)
>  		 * been properly initialized. Instead of attempting to map a
>  		 * wrong guest physical address return error.
>  		 *
> -		 * Also recognize all bits set as an invalid value.
> +		 * Also recognize all bits set as an invalid/uninitialized value.
>  		 */
> -		if (!v || !~v) {
> +		if (!v) {
>  			err = -ENOENT;
>  			goto out_error;
>  		}
> -		/* Avoid truncation on 32-bit. */
> +		if (v == ~0ULL) {
No need for brackets for a single instruction.

> +			wait = true;
> +		} 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);
__func shall be aligned with "%s... as it was before.

> +				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);
> +			if (xen_store_interface->connection != XENSTORE_CONNECTED)
> +				wait = true;
> +		}
> +		if (wait) {
> +			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;
> +		}
>  		break;
>  	default:
>  		pr_warn("Xenstore state unknown\n");

Cheers,
Michal

Re: [PATCH v4 2/2] xen: add support for initializing xenstore later as HVM domain

[Juergen Gross]

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

On 05.05.22 14:26, Michal Orzel wrote:
> Hi Luca,
> 
> On 05.05.2022 02:23, 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.
>>
>> The xenstore page has few extra field. Add them to the shared struct.
>> One of the field is "connection", when the connection is ready, it is
>> zero. If the connection is not-zero, wait for a notification.
>>
>> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>> ---
>> Changes in v4:
>> - improve in-code comments
>> - move header sync to separate patch
>> - use XENSTORE_CONNECTED
>>
>> Changes in v3:
>> - check for the connection field, if it is not zero, wait for event
>>
>> Changes in v2:
>> - remove XENFEAT_xenstore_late_init
>> ---
>>   drivers/xen/xenbus/xenbus_probe.c | 91 ++++++++++++++++++++++++-------
>>   1 file changed, 71 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
>> index fe360c33ce71..0a785d5e3e40 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,20 @@ 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 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.
>> +		 */
>> +		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 +813,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,10 +924,25 @@ 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;
> No need to set up initial value as it is being reassigned without using the initial value.
> 
>> +	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;
>>   	uint64_t v = 0;
>> +	bool wait = false;
>>   	xen_store_domain_type = XS_UNKNOWN;
>>   
>>   	if (!xen_domain())
>> @@ -957,25 +989,44 @@ static int __init xenbus_init(void)
>>   		 * been properly initialized. Instead of attempting to map a
>>   		 * wrong guest physical address return error.
>>   		 *
>> -		 * Also recognize all bits set as an invalid value.
>> +		 * Also recognize all bits set as an invalid/uninitialized value.
>>   		 */
>> -		if (!v || !~v) {
>> +		if (!v) {
>>   			err = -ENOENT;
>>   			goto out_error;
>>   		}
>> -		/* Avoid truncation on 32-bit. */
>> +		if (v == ~0ULL) {
> No need for brackets for a single instruction.

The coding style says otherwise:

   This does not apply if only one branch of a conditional statement is a single
   statement; in the latter case use braces in both branches:

   .. code-block:: c

         if (condition) {
                 do_this();
                 do_that();
         } else {
                 otherwise();
         }


Juergen

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

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

Re: [PATCH v4 2/2] xen: add support for initializing xenstore later as HVM domain

[Michal Orzel]

On 05.05.2022 14:29, Juergen Gross wrote:
>>> -        /* Avoid truncation on 32-bit. */
>>> +        if (v == ~0ULL) {
>> No need for brackets for a single instruction.
> 
> The coding style says otherwise:
> 
>   This does not apply if only one branch of a conditional statement is a single
>   statement; in the latter case use braces in both branches:
> 
>   .. code-block:: c
> 
>         if (condition) {
>                 do_this();
>                 do_that();
>         } else {
>                 otherwise();
>         }
> 
> 
Good to know. So Luca, you can omit this comment.

> Juergen

Cheers,
Michal

Re: [PATCH v4 2/2] xen: add support for initializing xenstore later as HVM domain

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

On Thu, 5 May 2022, Michal Orzel wrote:
> On 05.05.2022 14:29, Juergen Gross wrote:
> >>> -        /* Avoid truncation on 32-bit. */
> >>> +        if (v == ~0ULL) {
> >> No need for brackets for a single instruction.
> > 
> > The coding style says otherwise:
> > 
> >   This does not apply if only one branch of a conditional statement is a single
> >   statement; in the latter case use braces in both branches:
> > 
> >   .. code-block:: c
> > 
> >         if (condition) {
> >                 do_this();
> >                 do_that();
> >         } else {
> >                 otherwise();
> >         }
> > 
> > 
> Good to know. So Luca, you can omit this comment.

Thanks Michal, I addressed the other two comments.

Re: [PATCH v4 2/2] xen: add support for initializing xenstore later as HVM domain

[Boris Ostrovsky]

On 5/4/22 8:23 PM, Stefano Stabellini wrote:
> @@ -957,25 +989,44 @@ static int __init xenbus_init(void)
>   		 * been properly initialized. Instead of attempting to map a
>   		 * wrong guest physical address return error.
>   		 *
> -		 * Also recognize all bits set as an invalid value.
> +		 * Also recognize all bits set as an invalid/uninitialized value.


What I really meant (but not what I actually wrote I guess) was that now we are treating -1 differently than 0 and so that comment should go ...


>   		 */
> -		if (!v || !~v) {
> +		if (!v) {
>   			err = -ENOENT;
>   			goto out_error;
>   		}
> -		/* Avoid truncation on 32-bit. */


... here.


But this is ntpicking so for the series


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>


> +		if (v == ~0ULL) {
> +			wait = true;
> +		} else {
> +			/* Avoid truncation on 32-bit. */