diff mbox series

[1/5] xen: add "not_essential" flag to struct xenbus_driver

Message ID 20211022064800.14978-2-jgross@suse.com (mailing list archive)
State New
Headers show
Series xen: cleanup detection of non-essential pv devices | expand

Commit Message

Juergen Gross Oct. 22, 2021, 6:47 a.m. UTC
When booting the xenbus driver will wait for PV devices to have
connected to their backends before continuing. The timeout is different
between essential and non-essential devices.

Non-essential devices are identified by their nodenames directly in the
xenbus driver, which requires to update this list in case a new device
type being non-essential is added (this was missed for several types
in the past).

In order to avoid this problem, add a "not_essential" flag to struct
xenbus_driver which can be set to "true" by the respective frontend.

Set this flag for the frontends currently regarded to be not essential
(vkbs and vfb) and use it for testing in the xenbus driver.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/input/misc/xen-kbdfront.c          |  1 +
 drivers/video/fbdev/xen-fbfront.c          |  1 +
 drivers/xen/xenbus/xenbus_probe_frontend.c | 14 +++-----------
 include/xen/xenbus.h                       |  1 +
 4 files changed, 6 insertions(+), 11 deletions(-)

Comments

Andrew Cooper Oct. 22, 2021, 9:28 a.m. UTC | #1
On 22/10/2021 07:47, Juergen Gross wrote:
> When booting the xenbus driver will wait for PV devices to have
> connected to their backends before continuing. The timeout is different
> between essential and non-essential devices.
>
> Non-essential devices are identified by their nodenames directly in the
> xenbus driver, which requires to update this list in case a new device
> type being non-essential is added (this was missed for several types
> in the past).
>
> In order to avoid this problem, add a "not_essential" flag to struct
> xenbus_driver which can be set to "true" by the respective frontend.
>
> Set this flag for the frontends currently regarded to be not essential
> (vkbs and vfb) and use it for testing in the xenbus driver.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Wouldn't it be better to annotate essential?  That way, when new misc
drivers come along, they don't by default block boot.

~Andrew
Juergen Gross Oct. 25, 2021, 9:30 a.m. UTC | #2
On 22.10.21 11:28, Andrew Cooper wrote:
> On 22/10/2021 07:47, Juergen Gross wrote:
>> When booting the xenbus driver will wait for PV devices to have
>> connected to their backends before continuing. The timeout is different
>> between essential and non-essential devices.
>>
>> Non-essential devices are identified by their nodenames directly in the
>> xenbus driver, which requires to update this list in case a new device
>> type being non-essential is added (this was missed for several types
>> in the past).
>>
>> In order to avoid this problem, add a "not_essential" flag to struct
>> xenbus_driver which can be set to "true" by the respective frontend.
>>
>> Set this flag for the frontends currently regarded to be not essential
>> (vkbs and vfb) and use it for testing in the xenbus driver.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Wouldn't it be better to annotate essential?  That way, when new misc
> drivers come along, they don't by default block boot.

It isn't as if new drivers would "block boot". Normally the short
timeout for all drivers of 30 seconds is more than enough for all of
them.

I'm a little bit hesitant to have a kind of "white listing" essential
drivers, as there might be different views which drivers should have
that flag. Doing this the other way round is easier: in case of
disagreement such a patch just wouldn't go in, not breaking anything
in that case.

Additionally there might be out-of-tree PV drivers, which could be
hit by not being flagged to be essential. With the not_essential flag
the situation wouldn't change for such a driver.


Juergen
diff mbox series

Patch

diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
index 4ff5cd2a6d8d..3d17a0b3fe51 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -542,6 +542,7 @@  static struct xenbus_driver xenkbd_driver = {
 	.remove = xenkbd_remove,
 	.resume = xenkbd_resume,
 	.otherend_changed = xenkbd_backend_changed,
+	.not_essential = true,
 };
 
 static int __init xenkbd_init(void)
diff --git a/drivers/video/fbdev/xen-fbfront.c b/drivers/video/fbdev/xen-fbfront.c
index 5ec51445bee8..6826f986da43 100644
--- a/drivers/video/fbdev/xen-fbfront.c
+++ b/drivers/video/fbdev/xen-fbfront.c
@@ -695,6 +695,7 @@  static struct xenbus_driver xenfb_driver = {
 	.remove = xenfb_remove,
 	.resume = xenfb_resume,
 	.otherend_changed = xenfb_backend_changed,
+	.not_essential = true,
 };
 
 static int __init xenfb_init(void)
diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c
index 480944606a3c..07b010a68fcf 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -211,19 +211,11 @@  static int is_device_connecting(struct device *dev, void *data, bool ignore_none
 	if (drv && (dev->driver != drv))
 		return 0;
 
-	if (ignore_nonessential) {
-		/* With older QEMU, for PVonHVM guests the guest config files
-		 * could contain: vfb = [ 'vnc=1, vnclisten=0.0.0.0']
-		 * which is nonsensical as there is no PV FB (there can be
-		 * a PVKB) running as HVM guest. */
+	xendrv = to_xenbus_driver(dev->driver);
 
-		if ((strncmp(xendev->nodename, "device/vkbd", 11) == 0))
-			return 0;
+	if (ignore_nonessential && xendrv->not_essential)
+		return 0;
 
-		if ((strncmp(xendev->nodename, "device/vfb", 10) == 0))
-			return 0;
-	}
-	xendrv = to_xenbus_driver(dev->driver);
 	return (xendev->state < XenbusStateConnected ||
 		(xendev->state == XenbusStateConnected &&
 		 xendrv->is_ready && !xendrv->is_ready(xendev)));
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index b94074c82772..b13eb86395e0 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -112,6 +112,7 @@  struct xenbus_driver {
 	const char *name;       /* defaults to ids[0].devicetype */
 	const struct xenbus_device_id *ids;
 	bool allow_rebind; /* avoid setting xenstore closed during remove */
+	bool not_essential;     /* is not mandatory for boot progress */
 	int (*probe)(struct xenbus_device *dev,
 		     const struct xenbus_device_id *id);
 	void (*otherend_changed)(struct xenbus_device *dev,